diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/GetAllTenantConfigurationsFunctionTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/GetAllTenantConfigurationsFunctionTests.cs index 82d8f426..8cfec54a 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/GetAllTenantConfigurationsFunctionTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/GetAllTenantConfigurationsFunctionTests.cs @@ -24,10 +24,10 @@ public class GetAllTenantConfigurationsFunctionTests [Fact] public void ExistingManagedConfigs_DropsTenantsWithoutConfigRow() { - var reads = new (TenantConfiguration config, bool exists)[] + var reads = new TenantConfiguration?[] { - (new TenantConfiguration { TenantId = TenantB }, true), - (new TenantConfiguration { TenantId = TenantC }, false), // no config row → must be dropped + new TenantConfiguration { TenantId = TenantB }, + null, // no config row (404) → must be dropped }; var result = GetAllTenantConfigurationsFunction.ExistingManagedConfigs(reads); @@ -39,10 +39,10 @@ public class GetAllTenantConfigurationsFunctionTests [Fact] public void ExistingManagedConfigs_AllExist_KeepsAll() { - var reads = new (TenantConfiguration config, bool exists)[] + var reads = new TenantConfiguration?[] { - (new TenantConfiguration { TenantId = TenantB }, true), - (new TenantConfiguration { TenantId = TenantC }, true), + new TenantConfiguration { TenantId = TenantB }, + new TenantConfiguration { TenantId = TenantC }, }; var result = GetAllTenantConfigurationsFunction.ExistingManagedConfigs(reads); @@ -52,13 +52,54 @@ public class GetAllTenantConfigurationsFunctionTests [Fact] public void ExistingManagedConfigs_NoneExist_Empty() + => Assert.Empty(GetAllTenantConfigurationsFunction.ExistingManagedConfigs(new TenantConfiguration?[] { null })); + + [Fact] + public void MergeRescuedConfigs_AddsCaseVariantRow_MissedByExactCasePointRead() { - var reads = new (TenantConfiguration config, bool exists)[] + // AllowedTenantIds is lowercased; the stored config row has an upper-case PartitionKey, + // so the exact-case point-read missed it. The scan rescue must restore it. + var managedIds = new[] { TenantB, TenantC }; + var hits = new List { new() { TenantId = TenantB } }; + var scanned = new[] { - (new TenantConfiguration { TenantId = TenantB }, false), + new TenantConfiguration { TenantId = TenantB }, + new TenantConfiguration { TenantId = TenantC.ToUpperInvariant() }, // case-variant row }; - Assert.Empty(GetAllTenantConfigurationsFunction.ExistingManagedConfigs(reads)); + var merged = GetAllTenantConfigurationsFunction.MergeRescuedConfigs(managedIds, hits, scanned); + + Assert.Equal(2, merged.Count); + Assert.Contains(merged, c => c.TenantId == TenantC.ToUpperInvariant()); + } + + [Fact] + public void MergeRescuedConfigs_NeverAddsUnmanagedTenant() + { + var managedIds = new[] { TenantB, TenantC }; + var hits = new List { new() { TenantId = TenantB } }; + var scanned = new[] + { + new TenantConfiguration { TenantId = TenantB }, + new TenantConfiguration { TenantId = "44444444-4444-4444-4444-444444444444" }, // NOT managed + }; + + var merged = GetAllTenantConfigurationsFunction.MergeRescuedConfigs(managedIds, hits, scanned); + + var only = Assert.Single(merged); + Assert.Equal(TenantB, only.TenantId); + } + + [Fact] + public void MergeRescuedConfigs_DoesNotDuplicatePointReadHits() + { + var managedIds = new[] { TenantB }; + var hits = new List { new() { TenantId = TenantB } }; + var scanned = new[] { new TenantConfiguration { TenantId = TenantB.ToUpperInvariant() } }; + + var merged = GetAllTenantConfigurationsFunction.MergeRescuedConfigs(managedIds, hits, scanned); + + Assert.Single(merged); // scan row is the same tenant (case-insensitive) → not added twice } [Fact] diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/TenantConfigurationServiceReadSemanticsTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/TenantConfigurationServiceReadSemanticsTests.cs new file mode 100644 index 00000000..b275b156 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/TenantConfigurationServiceReadSemanticsTests.cs @@ -0,0 +1,103 @@ +using System; +using System.Threading.Tasks; +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared.DataAccess; +using AutopilotMonitor.Shared.Models; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Logging; +using Moq; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Pins the two point-read semantics of : +/// PROPAGATES storage +/// failures (a caller like the delegated config/all subset must fail the request rather than +/// silently drop a tenant), while +/// maps them to exists=false (agent security gates fail closed). Both are non-creating. +/// +public sealed class TenantConfigurationServiceReadSemanticsTests +{ + private const string TenantId = "11111111-1111-1111-1111-111111111111"; + + private static (TenantConfigurationService service, Mock repo) Build() + { + var repo = new Mock(MockBehavior.Strict); + var service = new TenantConfigurationService( + repo.Object, + Mock.Of>(), + new MemoryCache(new MemoryCacheOptions())); + return (service, repo); + } + + [Fact] + public async Task GetConfigurationIfExistsAsync_StorageError_Propagates() + { + var (service, repo) = Build(); + repo.Setup(r => r.GetTenantConfigurationAsync(TenantId)) + .ThrowsAsync(new InvalidOperationException("throttled")); + + await Assert.ThrowsAsync( + () => service.GetConfigurationIfExistsAsync(TenantId)); + } + + [Fact] + public async Task GetConfigurationIfExistsAsync_NoRow_ReturnsNull_NeverCreates() + { + var (service, repo) = Build(); + repo.Setup(r => r.GetTenantConfigurationAsync(TenantId)) + .ReturnsAsync((TenantConfiguration?)null); + + Assert.Null(await service.GetConfigurationIfExistsAsync(TenantId)); + // MockBehavior.Strict: any SaveTenantConfigurationAsync call would have thrown. + } + + [Fact] + public async Task GetConfigurationIfExistsAsync_RowFound_ReturnsAndCaches() + { + var (service, repo) = Build(); + repo.Setup(r => r.GetTenantConfigurationAsync(TenantId)) + .ReturnsAsync(new TenantConfiguration { TenantId = TenantId }); + + var first = await service.GetConfigurationIfExistsAsync(TenantId); + var second = await service.GetConfigurationIfExistsAsync(TenantId); + + Assert.Equal(TenantId, first!.TenantId); + Assert.Same(first, second); + repo.Verify(r => r.GetTenantConfigurationAsync(TenantId), Times.Once); + } + + [Fact] + public async Task GetConfigurationIfExistsAsync_EmptyTenantId_ReturnsNull() + { + var (service, _) = Build(); + Assert.Null(await service.GetConfigurationIfExistsAsync("")); + } + + [Fact] + public async Task TryGetConfigurationAsync_StorageError_FailsSafe_ExistsFalse() + { + var (service, repo) = Build(); + repo.Setup(r => r.GetTenantConfigurationAsync(TenantId)) + .ThrowsAsync(new InvalidOperationException("throttled")); + + var (config, exists) = await service.TryGetConfigurationAsync(TenantId); + + Assert.False(exists); + Assert.Equal(TenantId, config.TenantId); // fail-safe default, not a stored row + } + + [Fact] + public async Task TryGetConfigurationAsync_RowFound_ExistsTrue() + { + var (service, repo) = Build(); + repo.Setup(r => r.GetTenantConfigurationAsync(TenantId)) + .ReturnsAsync(new TenantConfiguration { TenantId = TenantId }); + + var (config, exists) = await service.TryGetConfigurationAsync(TenantId); + + Assert.True(exists); + Assert.Equal(TenantId, config.TenantId); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Config/GetAllTenantConfigurationsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Config/GetAllTenantConfigurationsFunction.cs index 0ae1cf66..10236445 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Config/GetAllTenantConfigurationsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Config/GetAllTenantConfigurationsFunction.cs @@ -56,11 +56,26 @@ namespace AutopilotMonitor.Functions.Functions.Config { // READ-bounded, not just response-bounded: point-read ONLY the caller's managed tenants // instead of scanning every tenant config and filtering in memory. A delegated MSP scoped - // to k tenants triggers k point reads (cached, non-creating — TryGetConfigurationAsync - // never writes a default row). exists==false drops any id with no config row. - var reads = await Task.WhenAll(requestCtx.AllowedTenantIds.Select(async tid => - await _configService.TryGetConfigurationAsync(tid))); + // to k tenants triggers k point reads (cached, non-creating — GetConfigurationIfExistsAsync + // never writes a default row). Only a true "no config row" (404) drops an id; a storage + // READ FAILURE propagates to the outer catch → 500, so a transient error can never + // silently drop a managed tenant from a success response. Ids are deduped case- + // insensitively (the old scan+filter semantics) so a duplicated grant list cannot + // produce duplicate config entries. + var managedIds = requestCtx.AllowedTenantIds + .Distinct(StringComparer.OrdinalIgnoreCase) + .ToList(); + var reads = await Task.WhenAll(managedIds.Select(tid => + _configService.GetConfigurationIfExistsAsync(tid))); var subset = ExistingManagedConfigs(reads); + + // AllowedTenantIds is lowercased while the config PartitionKey casing is not guaranteed + // (agent paths auto-create rows with the caller-supplied id verbatim) — the same caveat + // the delegated sessions fan-out handles case-insensitively. A point-read is exact-case, + // so any id it missed gets ONE scan+filter rescue pass (the pre-point-read behavior). + // Only stale grants or case-variant rows pay for it; the common path stays read-bounded. + if (subset.Count < managedIds.Count) + subset = MergeRescuedConfigs(managedIds, subset, await _configService.GetAllConfigurationsAsync()); _logger.LogInformation("GetAllTenantConfigurations (delegated subset, {Count} tenants) by {User}", subset.Count, userIdentifier); @@ -153,13 +168,32 @@ namespace AutopilotMonitor.Functions.Functions.Config /// /// From the per-tenant point-read results, keep only the managed tenants that actually HAVE a config - /// row (exists). A delegated caller's AllowedTenantIds is the authoritative bound; an id with no row + /// row (non-null). A delegated caller's AllowedTenantIds is the authoritative bound; an id with no row /// (offboarded / never onboarded) is silently dropped rather than surfaced as an empty default. + /// Storage errors never reach this seam — GetConfigurationIfExistsAsync throws, failing the request. /// Pure + testable seam (handler HTTP entry is not unit-tested — see GetAllBlockedDevicesFunctionTests). /// internal static List ExistingManagedConfigs( - IEnumerable<(TenantConfiguration config, bool exists)> reads) - => reads.Where(r => r.exists).Select(r => r.config).ToList(); + IEnumerable reads) + => reads.Where(c => c != null).Select(c => c!).ToList(); + + /// + /// Case-variant rescue for managed ids the exact-case point-reads missed: from the full config scan, + /// add every config whose TenantId matches a managed id case-insensitively and is not already in the + /// point-read hits. The managed-id bound stays authoritative — an unmanaged config is never added, so + /// this can only ever RESTORE tenants the pre-point-read scan+filter would have returned. Pure + testable. + /// + internal static List MergeRescuedConfigs( + IReadOnlyCollection managedIds, + IReadOnlyCollection pointReadHits, + IEnumerable allConfigs) + { + var managed = new HashSet(managedIds, StringComparer.OrdinalIgnoreCase); + var found = new HashSet(pointReadHits.Select(c => c.TenantId), StringComparer.OrdinalIgnoreCase); + var merged = new List(pointReadHits); + merged.AddRange(allConfigs.Where(c => managed.Contains(c.TenantId) && !found.Contains(c.TenantId))); + return merged; + } /// /// The delegated bare-array view of config/all: every managed-tenant config with its secrets diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TenantConfigurationService.cs b/src/Backend/AutopilotMonitor.Functions/Services/TenantConfigurationService.cs index 720b19d2..72a1d5f7 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TenantConfigurationService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TenantConfigurationService.cs @@ -121,28 +121,45 @@ namespace AutopilotMonitor.Functions.Services _cache.Remove($"tenant-config:{tenantId}"); } + /// + /// Strict point-read: returns the config, or null when no row exists (404) — does NOT + /// auto-create. Any other storage failure PROPAGATES to the caller, so "read failed" can + /// never be conflated with "tenant not configured". Use where a dropped tenant is worse + /// than a failed request (e.g. the delegated config/all subset). + /// + public virtual async Task GetConfigurationIfExistsAsync(string tenantId) + { + if (string.IsNullOrEmpty(tenantId)) + return null; + + var cacheKey = $"tenant-config:{tenantId}"; + + if (_cache.TryGetValue(cacheKey, out TenantConfiguration? cachedConfig) && cachedConfig != null) + return cachedConfig; + + var config = await _configRepo.GetTenantConfigurationAsync(tenantId); + if (config != null) + _cache.Set(cacheKey, config, CacheDuration); + + return config; + } + /// /// Returns (config, exists). exists=false when no row was found — does NOT auto-create. - /// Use for agent security gates where unknown tenants must be rejected. + /// Use for agent security gates where unknown tenants must be rejected: storage errors are + /// mapped to exists=false (fail closed). Callers that must instead surface read failures + /// use . /// public virtual async Task<(TenantConfiguration config, bool exists)> TryGetConfigurationAsync(string tenantId) { if (string.IsNullOrEmpty(tenantId)) return (TenantConfiguration.CreateDefault("unknown"), false); - var cacheKey = $"tenant-config:{tenantId}"; - - if (_cache.TryGetValue(cacheKey, out TenantConfiguration? cachedConfig) && cachedConfig != null) - return (cachedConfig, true); - try { - var config = await _configRepo.GetTenantConfigurationAsync(tenantId); + var config = await GetConfigurationIfExistsAsync(tenantId); if (config != null) - { - _cache.Set(cacheKey, config, CacheDuration); return (config, true); - } return (TenantConfiguration.CreateDefault(tenantId), false); }