diff --git a/.claude/commands b/.claude/commands index 59a7eb8a..b4fc66f0 160000 --- a/.claude/commands +++ b/.claude/commands @@ -1 +1 @@ -Subproject commit 59a7eb8a005afe721d67737937e7c70eefa4cb9a +Subproject commit b4fc66f0dffe1e106b402fe99e757b77cb939f59 diff --git a/internal b/internal index 43055e93..650ea7e0 160000 --- a/internal +++ b/internal @@ -1 +1 @@ -Subproject commit 43055e93ea71ed1afda3ccf65439497b8cd8bac6 +Subproject commit 650ea7e00662f16a703b5cdb71263037987992b8 diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs index 8b88d728..d2d227ea 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs @@ -43,7 +43,8 @@ public class AuthFunctionSideEffectTests private readonly FakeOffboardingAuditRepository _offboardingRepo = new(); private readonly StoredTenantConfig _store; private readonly Mock _globalAdminMock; - private readonly Func _buildSut; + /// Builds the function; null arguments fall back to the class mocks. + private readonly Func _buildSut; private readonly AuthFunction _sut; public AuthFunctionSideEffectTests() @@ -106,14 +107,14 @@ public class AuthFunctionSideEffectTests _opsEventsMock = new Mock(null!, null!, null!) { CallBase = false }; _opsEventsMock.Setup(x => x.RecordTenantSignupAsync(It.IsAny(), It.IsAny())).Returns(Task.CompletedTask); - _buildSut = pushSignIn => new AuthFunction( + _buildSut = (pushSignIn, tenantAdmins, preview) => new AuthFunction( Mock.Of>(), globalAdminMock.Object, delegatedAdminMock.Object, _tenantConfigMock.Object, - _tenantAdminsMock.Object, + tenantAdmins ?? _tenantAdminsMock.Object, _metricsRepoMock.Object, - _previewMock.Object, + preview ?? _previewMock.Object, _telegramMock.Object, _globalNotificationMock.Object, _mcpUserMock.Object, @@ -128,7 +129,7 @@ public class AuthFunctionSideEffectTests _offboardingRepo, pushSignIn: pushSignIn, opsEvents: _opsEventsMock.Object); - _sut = _buildSut(null); + _sut = _buildSut(null, null, null); // Writes go through UpdateAsync / CreateOrUpdateAsync over one stored row (none by default: // a brand-new tenant). Default: all fire-and-forget calls succeed. @@ -880,7 +881,8 @@ public class AuthFunctionSideEffectTests { _tenantConfigMock.Setup(x => x.TryGetConfigurationAsync(TenantId)).ReturnsAsync((config, exists)); _previewMock.Setup(x => x.IsApprovedAsync(TenantId)).ReturnsAsync(approved); - _tenantAdminsMock.Setup(x => x.GetTenantAdminsAsync(TenantId)).ReturnsAsync(new List()); + _tenantAdminsMock.Setup(x => x.GetMembershipSnapshotAsync(TenantId, Upn)) + .ReturnsAsync((TableMemberState.NotPresent, null, false)); _mcpUserMock .Setup(x => x.IsAllowedAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny?>(), It.IsAny())) @@ -940,6 +942,53 @@ public class AuthFunctionSideEffectTests Assert.Contains(external ? "\"isTenantAdmin\":false" : "\"isTenantAdmin\":true", await ReadBodyAsync(response)); } + /// + /// The sequence behind new tenants' first admins landing on the progress page, over the real role caches. + /// While the tenant waits for activation, this instance resolves the member's role (the feedback-status + /// read after every activation poll) and finds no row. The auto-approve worker then activates the tenant + /// and promotes the member on another instance, whose cache invalidation never reaches this one. The next + /// poll must answer the member as Admin: not as still pending (a cached "not activated") and not as + /// roleless (a cached "no row" next to a fresh member list). + /// + [Fact] + public async Task GetCurrentUser_FirstPollAfterActivation_AnswersThePromotedMemberAsAdmin() + { + var config = DefaultConfig(); + config.DomainName = "contoso.com"; + config.OnboardedBy = Upn; + ArrangeAuthMe(config, exists: true, approved: false); + + var activated = false; + var rows = new List(); + var configRepo = new Mock(); + configRepo.Setup(r => r.IsInPreviewWhitelistAsync(TenantId)).ReturnsAsync(() => activated); + var adminRepo = new Mock(); + adminRepo.Setup(r => r.GetTenantMembersAsync(TenantId)).ReturnsAsync(() => rows.ToList()); + adminRepo.Setup(r => r.GetTenantMemberAsync(TenantId, Upn)).ReturnsAsync(() => rows.FirstOrDefault()); + var cache = new MemoryCache(new MemoryCacheOptions()); + var tenantAdmins = new TenantAdminsService(adminRepo.Object, cache, Mock.Of>()); + var sut = _buildSut(null, tenantAdmins, new PreviewWhitelistService( + configRepo.Object, cache, Mock.Of>(), _tenantConfigMock.Object)); + + var (pollReq, pollCtx) = BuildRequest(SignedIn(external: false)); + Assert.Equal(HttpStatusCode.Forbidden, (await sut.GetCurrentUser(pollReq, pollCtx)).StatusCode); + Assert.Equal(TableMemberState.NotPresent, (await tenantAdmins.GetTableMembershipAsync(TenantId, Upn)).State); + + activated = true; + rows.Add(new TenantMember { TenantId = TenantId, Upn = Upn, Role = Constants.TenantRoles.Admin, IsEnabled = true }); + + var (req, ctx) = BuildRequest(SignedIn(external: false)); + var response = await sut.GetCurrentUser(req, ctx); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await ReadBodyAsync(response); + Assert.Contains("\"isTenantAdmin\":true", body); + Assert.Contains("\"role\":\"Admin\"", body); + // The promotion already wrote the row: no first-user auto-admin on top of it. + adminRepo.Verify(r => r.AddTenantMemberAsync(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny()), Times.Never); + } + // ------------------------------------------------------------------------- // GetCurrentUser: the push sign-in (stamp with the app-role claim, resume) // ------------------------------------------------------------------------- @@ -951,10 +1000,9 @@ public class AuthFunctionSideEffectTests config.DomainName = "contoso.com"; config.EntraAppRolesEnabled = true; ArrangeAuthMe(config, exists: true, approved: true); - _tenantAdminsMock.Setup(x => x.GetTenantAdminsAsync(TenantId)) - .ReturnsAsync(new List { new() { TenantId = TenantId, Upn = "first@contoso.com" } }); - _tenantAdminsMock.Setup(x => x.GetTableMembershipAsync(TenantId, Upn)).ReturnsAsync((memberRow, - memberRow == TableMemberState.Enabled ? new MemberRoleInfo { Role = Constants.TenantRoles.Operator } : null)); + _tenantAdminsMock.Setup(x => x.GetMembershipSnapshotAsync(TenantId, Upn)).ReturnsAsync((memberRow, + memberRow == TableMemberState.Enabled ? new MemberRoleInfo { Role = Constants.TenantRoles.Operator } : null, + true)); } private (AuthFunction Sut, Push.PushDeliveryServiceTests.Harness Push) BuildWithPushSignIn() @@ -962,7 +1010,7 @@ public class AuthFunctionSideEffectTests var push = new Push.PushDeliveryServiceTests.Harness(); var signIn = new PushOwnerSignInService(push.Repo, new Mock().Object, push.Settings, push.Sut, Mock.Of>()); - return (_buildSut(signIn), push); + return (_buildSut(signIn, null, null), push); } [Theory] diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs index 2cc8d635..6d040b5e 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs @@ -269,6 +269,28 @@ public class PolicyEnforcementMiddlewareTests private static ClaimsPrincipal AuthedPrincipal(string tenantId, string upn, string objectId = TestOid) => new(new ClaimsIdentity(new[] { new Claim("tid", tenantId), new Claim("upn", upn), new Claim("oid", objectId) }, "TestAuth")); + // ── A grant written on another instance counts on the next request ────────────── + + /// + /// A new tenant's first admin opens the dashboard on an instance that resolved "no row" for them seconds + /// earlier, while the auto-approve worker wrote their promotion out of reach of this process's cache + /// invalidation. The session list must already admit them. + /// + [Fact] + public async Task MemberRead_RowWrittenAfterADenial_IsAdmittedOnTheNextRequest() + { + const string upn = "first.admin@contoso.com"; + var h = BuildHarness(); + + var before = await h.Middleware.DecideAsync("GET", "/api/sessions", null, AuthedPrincipal(TenantA, upn)); + h.AsTenantAdmin(TenantA, upn); + var after = await h.Middleware.DecideAsync("GET", "/api/sessions", null, AuthedPrincipal(TenantA, upn)); + + Assert.False(before.Allowed); + Assert.True(after.Allowed); + Assert.True(after.Context!.IsTenantAdmin); + } + // ── ADDITIVE: GlobalReader + own-tenant Admin keeps IsTenantAdmin ────────────── [Fact] diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/PreviewWhitelistApprovalCacheTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/PreviewWhitelistApprovalCacheTests.cs new file mode 100644 index 00000000..d033e600 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/PreviewWhitelistApprovalCacheTests.cs @@ -0,0 +1,45 @@ +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared.DataAccess; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// The activation check auth/me gates on. The auto-approve worker writes the activation on another instance +/// while the activation page polls, so "not activated" is never cached; an activation is. +/// +public class PreviewWhitelistApprovalCacheTests +{ + private const string TenantId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; + + private readonly Mock _repo = new(); + private readonly PreviewWhitelistService _sut; + + public PreviewWhitelistApprovalCacheTests() + { + var cache = new MemoryCache(new MemoryCacheOptions()); + _sut = new PreviewWhitelistService(_repo.Object, cache, NullLogger.Instance, + new TenantConfigurationService(_repo.Object, NullLogger.Instance, cache)); + } + + [Fact] + public async Task NotActivated_IsNotCached_SoTheWorkersActivationCountsAtOnce() + { + _repo.SetupSequence(r => r.IsInPreviewWhitelistAsync(TenantId)).ReturnsAsync(false).ReturnsAsync(true); + + Assert.False(await _sut.IsApprovedAsync(TenantId)); + Assert.True(await _sut.IsApprovedAsync(TenantId)); + } + + [Fact] + public async Task Activated_IsServedFromTheCache() + { + _repo.SetupSequence(r => r.IsInPreviewWhitelistAsync(TenantId)).ReturnsAsync(true).ReturnsAsync(false); + + Assert.True(await _sut.IsApprovedAsync(TenantId)); + Assert.True(await _sut.IsApprovedAsync(TenantId)); + _repo.Verify(r => r.IsInPreviewWhitelistAsync(TenantId), Times.Once); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/TenantAdminsServiceMembershipTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/TenantAdminsServiceMembershipTests.cs new file mode 100644 index 00000000..24497bb2 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/TenantAdminsServiceMembershipTests.cs @@ -0,0 +1,151 @@ +using AutopilotMonitor.Functions.Security; +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared; +using AutopilotMonitor.Shared.DataAccess; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// The membership reads behind every role decision. A grant can be written where this process's cache +/// invalidation never reaches (activation promotes the onboarding member on the auto-approve worker), so a +/// missing or disabled row is never cached; members stay on the cached path. The snapshot auth/me decides +/// the first-user auto-admin from reads the caller's row and "the tenant has members" in one go. +/// +public class TenantAdminsServiceMembershipTests +{ + private const string TenantId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; + private const string Upn = "first.admin@contoso.com"; + + private readonly Mock _repo = new(); + private readonly TenantAdminsService _sut; + + public TenantAdminsServiceMembershipTests() + { + _sut = new TenantAdminsService( + _repo.Object, new MemoryCache(new MemoryCacheOptions()), NullLogger.Instance); + } + + private static TenantMember Row(string upn, string role = Constants.TenantRoles.Admin, bool enabled = true) => + new() { TenantId = TenantId, Upn = upn, Role = role, IsEnabled = enabled }; + + private void PointReadsReturn(params TenantMember?[] rows) + { + var answers = new Queue(rows); + _repo.Setup(r => r.GetTenantMemberAsync(TenantId, Upn)).ReturnsAsync(() => answers.Dequeue()); + } + + private void MembersAre(params TenantMember[] rows) => + _repo.Setup(r => r.GetTenantMembersAsync(TenantId)).ReturnsAsync(rows.ToList()); + + [Fact] + public async Task MissingRow_IsNotCached_SoAGrantWrittenElsewhereCountsAtOnce() + { + PointReadsReturn(null, Row(Upn)); + + Assert.Equal(TableMemberState.NotPresent, (await _sut.GetTableMembershipAsync(TenantId, Upn)).State); + var (state, role) = await _sut.GetTableMembershipAsync(TenantId, Upn); + + Assert.Equal(TableMemberState.Enabled, state); + Assert.Equal(Constants.TenantRoles.Admin, role!.Role); + } + + [Fact] + public async Task DisabledRow_IsNotCached_SoReEnablingCountsAtOnce() + { + PointReadsReturn(Row(Upn, enabled: false), Row(Upn)); + + Assert.Equal(TableMemberState.Disabled, (await _sut.GetTableMembershipAsync(TenantId, Upn)).State); + Assert.Equal(TableMemberState.Enabled, (await _sut.GetTableMembershipAsync(TenantId, Upn)).State); + } + + [Fact] + public async Task EnabledRow_IsServedFromTheCache() + { + PointReadsReturn(Row(Upn), null); + + Assert.Equal(TableMemberState.Enabled, (await _sut.GetTableMembershipAsync(TenantId, Upn)).State); + Assert.Equal(TableMemberState.Enabled, (await _sut.GetTableMembershipAsync(TenantId, Upn)).State); + _repo.Verify(r => r.GetTenantMemberAsync(TenantId, Upn), Times.Once); + } + + [Theory] + [InlineData(Constants.TenantRoles.Admin, true, true)] + [InlineData(Constants.TenantRoles.Operator, true, false)] + [InlineData(Constants.TenantRoles.Admin, false, false)] + public async Task IsTenantAdmin_MeansAnEnabledAdminRow(string role, bool enabled, bool expected) + { + PointReadsReturn(Row(Upn, role, enabled)); + + Assert.Equal(expected, await _sut.IsTenantAdminAsync(TenantId, Upn)); + } + + [Fact] + public async Task Snapshot_TheCallersOwnRowDecides_WhateverTheTokenCasing() + { + MembersAre(Row("someone.else@contoso.com", Constants.TenantRoles.Viewer), Row(Upn, Constants.TenantRoles.Operator)); + + var (state, role, hasMembers) = await _sut.GetMembershipSnapshotAsync(TenantId, "First.Admin@Contoso.com"); + + Assert.Equal(TableMemberState.Enabled, state); + Assert.Equal(Constants.TenantRoles.Operator, role!.Role); + Assert.True(hasMembers); + } + + [Fact] + public async Task Snapshot_EmptyTenant_HasNoMembers() + { + MembersAre(); + + var (state, role, hasMembers) = await _sut.GetMembershipSnapshotAsync(TenantId, Upn); + + Assert.Equal(TableMemberState.NotPresent, state); + Assert.Null(role); + Assert.False(hasMembers); + } + + [Fact] + public async Task Snapshot_ADisabledRowOfSomeoneElse_StillCountsAsAMember() + { + // A tenant whose only admin is disabled must not make the next person who signs in its admin. + MembersAre(Row("someone.else@contoso.com", enabled: false)); + + var (state, _, hasMembers) = await _sut.GetMembershipSnapshotAsync(TenantId, Upn); + + Assert.Equal(TableMemberState.NotPresent, state); + Assert.True(hasMembers); + } + + [Fact] + public async Task Snapshot_TheCallersDisabledRow_IsAnExplicitDeny() + { + MembersAre(Row(Upn, enabled: false)); + + var (state, role, hasMembers) = await _sut.GetMembershipSnapshotAsync(TenantId, Upn); + + Assert.Equal(TableMemberState.Disabled, state); + Assert.Null(role); + Assert.True(hasMembers); + } + + [Fact] + public async Task Snapshot_IsNeverCached() + { + _repo.SetupSequence(r => r.GetTenantMembersAsync(TenantId)) + .ReturnsAsync(new List()) + .ReturnsAsync(new List { Row(Upn) }); + + Assert.False((await _sut.GetMembershipSnapshotAsync(TenantId, Upn)).TenantHasMembers); + Assert.Equal(TableMemberState.Enabled, (await _sut.GetMembershipSnapshotAsync(TenantId, Upn)).State); + } + + [Fact] + public async Task Snapshot_RefusesATenantIdThatIsNotAGuid() + { + // The repository builds its partition filter from the tenant id. + await Assert.ThrowsAsync(() => _sut.GetMembershipSnapshotAsync("x' or PartitionKey ne '", Upn)); + _repo.Verify(r => r.GetTenantMembersAsync(It.IsAny()), Times.Never); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/TenantApprovalServiceTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/TenantApprovalServiceTests.cs index 9e6cbd0a..a323e9ec 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/TenantApprovalServiceTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/TenantApprovalServiceTests.cs @@ -1,5 +1,7 @@ +using AutopilotMonitor.Functions.Security; using AutopilotMonitor.Functions.Services; using AutopilotMonitor.Functions.Services.Notifications; +using AutopilotMonitor.Shared; using AutopilotMonitor.Shared.DataAccess; using AutopilotMonitor.Shared.Models; using Microsoft.Extensions.Caching.Memory; @@ -54,7 +56,8 @@ public class TenantApprovalServiceTests configRepo, NullLogger.Instance, cache); var alertDispatch = TestNotifications.InertOpsAlertDispatch(adminConfig.Object); - // IsTenantAdminAsync is not virtual: it runs for real against the empty repository (no member yet). + // IsTenantAdminAsync runs for real and reads through the virtual GetTableMembershipAsync, + // which ArrangeNewlyApproved sets up per test. _tenantAdminsMock = new Mock( Mock.Of(), cache, NullLogger.Instance); @@ -112,7 +115,7 @@ public class TenantApprovalServiceTests // --- Auto-promote on activation: only the onboarding member (OnboardedBy) becomes Tenant Admin. - private void ArrangeNewlyApproved(string? onboardedBy, string updatedBy = "System") + private void ArrangeNewlyApproved(string? onboardedBy, string updatedBy = "System", bool alreadyAdmin = false) { var config = ConfigWith(contactEmail: null); config.OnboardedBy = onboardedBy; @@ -120,6 +123,9 @@ public class TenantApprovalServiceTests _previewMock.Setup(x => x.ApproveAsync(TenantId, ApprovedBy)).ReturnsAsync(true); _previewMock.Setup(x => x.GetNotificationEmailAsync(TenantId)).ReturnsAsync((string?)null); _tenantConfigMock.Setup(x => x.GetConfigurationAsync(TenantId)).ReturnsAsync(config); + _tenantAdminsMock.Setup(x => x.GetTableMembershipAsync(TenantId, It.IsAny())).ReturnsAsync(alreadyAdmin + ? (TableMemberState.Enabled, new MemberRoleInfo { Role = Constants.TenantRoles.Admin }) + : (TableMemberState.NotPresent, null)); } private void VerifyNoPromotion() => @@ -138,6 +144,16 @@ public class TenantApprovalServiceTests x => x.AddTenantAdminAsync(TenantId, "first.member@contoso.example", ApprovedBy), Times.Once); } + [Fact] + public async Task NewlyApproved_OnboardingMemberAlreadyAdmin_IsNotWrittenAgain() + { + ArrangeNewlyApproved(onboardedBy: "first.member@contoso.example", alreadyAdmin: true); + + await _sut.ApproveWithSideEffectsAsync(TenantId, ApprovedBy); + + VerifyNoPromotion(); + } + [Fact] public async Task NewlyApproved_NeverPromotesAnExternalAccountsUpn() { diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/TenantMemberRoleResolverTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/TenantMemberRoleResolverTests.cs index 9bf005a4..a7c2fb1f 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/TenantMemberRoleResolverTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/TenantMemberRoleResolverTests.cs @@ -13,7 +13,7 @@ namespace AutopilotMonitor.Functions.Tests; /// /// The shared effective-role resolver (policy middleware + MCP AllMembers gate): TenantAdmins table first, /// the token's Entra app-role claim only when no row exists AND the tenant opted in, and no tenant-config -/// read at all for a roleless token — so the common "no row, no claim" path costs one cached membership read. +/// read at all for a roleless token — so the common "no row, no claim" path costs one membership read. /// public class TenantMemberRoleResolverTests { diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs index 539cae96..f9b96ef2 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs @@ -127,15 +127,16 @@ public class AuthFunction var globalRoleTask = _globalAdminService.GetGlobalRoleAsync(identity); var delegatedScopeTask = _delegatedAdminService.GetScopeAsync(identity); var isApprovedTask = _previewWhitelistService.IsApprovedAsync(tenantId); - var membershipTask = _tenantAdminsService.GetTableMembershipAsync(tenantId, upn); + // The caller's row and whether the tenant has any member come from ONE read: the first-user + // auto-admin decision must never combine two states of the TenantAdmins partition. + var membershipTask = _tenantAdminsService.GetMembershipSnapshotAsync(tenantId, upn); // App roles ride along so the AllMembers path sees a claim-derived member the same way the policy // middleware does (the table-vs-claim reconciliation happens inside the shared role resolver). var mcpCheckTask = _mcpUserService.IsAllowedAsync(upn, tenantId, objectId, principal.GetAppRoles()); - var existingAdminsTask = _tenantAdminsService.GetTenantAdminsAsync(tenantId); var whatsNewSeenTask = _metricsRepo.GetUserWhatsNewSeenAsync(tenantId, upn); await Task.WhenAll(tenantConfigTask, globalRoleTask, delegatedScopeTask, isApprovedTask, - membershipTask, mcpCheckTask, existingAdminsTask, whatsNewSeenTask); + membershipTask, mcpCheckTask, whatsNewSeenTask); var (tenantConfig, _) = tenantConfigTask.Result; var globalRole = globalRoleTask.Result; @@ -145,9 +146,8 @@ public class AuthFunction // Surfaced to the web app so it can show fleet/switcher UI and bound it to this set. var delegatedTenantIds = delegatedScopeTask.Result.TenantIds; var isApproved = isApprovedTask.Result; - var (tableState, tableRole) = membershipTask.Result; + var (tableState, tableRole, tenantHasMembers) = membershipTask.Result; var mcpCheck = mcpCheckTask.Result; - var existingAdmins = existingAdminsTask.Result; var whatsNewSeen = whatsNewSeenTask.Result; // Reconcile the TenantAdmins table state with any Entra app-role claim. An enabled row @@ -195,7 +195,7 @@ public class AuthFunction // --- Pure decision logic (tested by AuthFunctionTests) --- var decision = BuildAuthResult( tenantConfig, isGlobalAdmin, isGlobalReader, isApproved, - memberRole, mcpCheck, existingAdmins.Count > 0, + memberRole, mcpCheck, tenantHasMembers, tenantId, upn, displayName ?? string.Empty, objectId ?? string.Empty, delegatedTenantIds, homedApp: _appRegistry.ResolveForTenant(tenantConfig).IsLegacy ? "legacy" : "primary", diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Rules/PreviewWhitelistFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Rules/PreviewWhitelistFunction.cs index e6f80358..b6814cbc 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Rules/PreviewWhitelistFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Rules/PreviewWhitelistFunction.cs @@ -201,13 +201,13 @@ public class PreviewWhitelistFunction // Send-on-save half of the welcome-mail race: with auto-approve, activation // typically finishes before the user has typed this address on the activation - // page — the approval path then found no address and deferred to us. Fresh - // approval read: a cached "not approved" can be stale for exactly this window. + // page — the approval path then found no address and deferred to us. IsApprovedAsync + // never caches "not approved", so an activation that landed before this write is seen. // The sent-marker inside TrySendWelcomeEmailAsync dedupes against the approval // path and against repeated saves. var welcomeEmailSent = false; if (!string.IsNullOrWhiteSpace(email) && - await _previewWhitelistService.IsApprovedFreshAsync(tenantId)) + await _previewWhitelistService.IsApprovedAsync(tenantId)) { welcomeEmailSent = await _tenantApprovalService.TrySendWelcomeEmailAsync(tenantId); } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/PreviewWhitelistService.cs b/src/Backend/AutopilotMonitor.Functions/Services/PreviewWhitelistService.cs index afa7e871..c9fb0fca 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/PreviewWhitelistService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/PreviewWhitelistService.cs @@ -20,14 +20,6 @@ public class PreviewWhitelistService private readonly TenantConfigurationService _tenantConfigService; private static readonly TimeSpan CacheDuration = TimeSpan.FromMinutes(5); - /// - /// Short TTL for a negative ("not approved") result. The activation page polls auth/me - /// while the auto-approve worker activates the tenant ~1 minute after signup on another - /// instance — a 5-minute negative cache would hide that activation for up to 4 extra - /// minutes. Positive results keep the long TTL (activation is rarely revoked). - /// - private static readonly TimeSpan NotApprovedCacheDuration = TimeSpan.FromSeconds(30); - public PreviewWhitelistService( IConfigRepository configRepo, IMemoryCache cache, @@ -41,7 +33,11 @@ public class PreviewWhitelistService } /// - /// Checks whether a tenant is activated (cached). + /// Checks whether a tenant is activated. Only an activation is cached: "not activated" is read + /// fresh on every call, because the activation is written on the auto-approve worker (another + /// instance, out of reach of this cache) while the activation page polls auth/me. The + /// notification-email save path relies on this, too: it must see an activation that landed + /// just before its own write (welcome-mail arbitration). /// public virtual async Task IsApprovedAsync(string tenantId) { @@ -56,7 +52,8 @@ public class PreviewWhitelistService { var result = await _configRepo.IsInPreviewWhitelistAsync(tenantId); - _cache.Set(cacheKey, result, result ? CacheDuration : NotApprovedCacheDuration); + if (result) + _cache.Set(cacheKey, true, CacheDuration); return result; } catch (Exception ex) @@ -85,29 +82,6 @@ public class PreviewWhitelistService return newlyApproved; } - /// - /// Fresh, uncached approval check. Used by the notification-email save path to decide - /// send-on-save: the 30s negative cache of would hide an - /// activation for exactly the race window this check exists to close (auto-approve on - /// another instance finishing seconds before the user saves their address). - /// Fail-closed: false on storage errors — the GA resend button remains the fallback. - /// - public virtual async Task IsApprovedFreshAsync(string tenantId) - { - if (string.IsNullOrWhiteSpace(tenantId)) - return false; - - try - { - return await _configRepo.IsInPreviewWhitelistAsync(tenantId); - } - catch (Exception ex) - { - _logger.LogWarning(ex, "Error checking preview whitelist (fresh) for tenant {TenantId}", tenantId); - return false; - } - } - /// /// Revokes a tenant's activation. /// diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TenantAdminsService.cs b/src/Backend/AutopilotMonitor.Functions/Services/TenantAdminsService.cs index 452d7bf7..3cdf5d85 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TenantAdminsService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TenantAdminsService.cs @@ -20,11 +20,14 @@ public class TenantAdminsService // Authorization state is cached per-process (IMemoryCache). On a scaled-out Flex Consumption // plan InvalidateMemberCache only clears the instance that handled the grant/revoke mutation; // other warm instances keep serving the stale role until their entry expires. A short TTL caps - // that cross-instance divergence window (newly-granted role appears, revoked role disappears) - // so a role change self-heals in seconds instead of minutes. The underlying lookup is a single - // Table Storage point-read (PK+RK), so the extra reads are negligible. Do NOT raise this back - // to minutes "for performance" — it reintroduces the role flip-flop. Matches the 30s - // cross-instance revalidation convention used by BlockedDeviceService. + // that cross-instance divergence window so a revoked or changed role self-heals in seconds + // instead of minutes. Only an ENABLED membership is cached: a missing or disabled row is re-read + // on every call, because the grant that ends it may run where no invalidation reaches this + // process (activation promotes the onboarding member on the queue worker) and a cached "no" + // answered that new admin as a roleless user. The underlying lookup is a single Table Storage + // point-read (PK+RK), so the extra reads are negligible. Do NOT raise this back to minutes + // "for performance" — it reintroduces the role flip-flop. Matches the 30s cross-instance + // revalidation convention used by BlockedDeviceService. private readonly TimeSpan _cacheDuration = TimeSpan.FromSeconds(30); public TenantAdminsService( @@ -62,44 +65,13 @@ public class TenantAdminsService => _pushRevoker?.RevokeOwnerAsync(Notifications.NotificationScope.Tenant(tenantId), upn, reason) ?? Task.CompletedTask; /// - /// Checks if a UPN is a Tenant Admin for a specific tenant - /// Uses caching for performance + /// True if the UPN holds an enabled Admin row in the tenant (a row without a role is an Admin). + /// Reads through , so it shares that cache and its rules. /// - /// Tenant ID - /// User Principal Name (e.g., oliver@contoso.com) - /// True if the user is a Tenant Admin for this tenant public async Task IsTenantAdminAsync(string tenantId, string? upn) { - if (string.IsNullOrWhiteSpace(tenantId) || string.IsNullOrWhiteSpace(upn)) - { - _logger.LogDebug("IsTenantAdminAsync: TenantId or UPN is null or empty"); - return false; - } - - // Normalize for case-insensitive comparison - tenantId = tenantId.ToLowerInvariant(); - upn = upn.ToLowerInvariant(); - - // Check cache first - var cacheKey = $"tenant-admin:{tenantId}:{upn}"; - if (_cache.TryGetValue(cacheKey, out var isAdmin)) - { - _logger.LogDebug("Tenant Admin check (from cache): {Upn} @ {TenantId} -> {IsAdmin}", upn, tenantId, isAdmin); - return isAdmin; - } - - // Query via repository - _logger.LogDebug("Querying repository for Tenant Admin: {Upn} @ {TenantId}", upn, tenantId); - var member = await _adminRepo.GetTenantMemberAsync(tenantId, upn); - // Only true for Admin role (null Role = Admin for backward compat) - var result = member != null && member.IsEnabled - && (member.Role == null || member.Role == Constants.TenantRoles.Admin); - - _logger.LogDebug("Tenant Admin check result: {Upn} @ {TenantId} -> {Result}", upn, tenantId, result); - - _cache.Set(cacheKey, result, _cacheDuration); - - return result; + var (state, role) = await GetTableMembershipAsync(tenantId, upn); + return state == TableMemberState.Enabled && role?.Role == Constants.TenantRoles.Admin; } /// @@ -202,37 +174,15 @@ public class TenantAdminsService }).ToList(); } - /// - /// Clears the cache for a specific tenant's admins - /// Useful after bulk updates - /// - public void ClearCacheForTenant(string tenantId) - { - // Note: IMemoryCache doesn't have a clear all method by default - // In production, consider using a distributed cache with better cache invalidation - // For now, cache entries will expire after _cacheDuration - } - // ----------------------------------------------------------------------- // Role-aware methods (Admin / Operator / Viewer) // ----------------------------------------------------------------------- - /// - /// Gets the role info for a tenant member. Returns null if the user has no enabled row - /// (either no row at all or a disabled row). For Entra app-role reconciliation use - /// , which distinguishes "no row" from "disabled row". - /// - public virtual async Task GetMemberRoleAsync(string tenantId, string? upn) - { - var (state, role) = await GetTableMembershipAsync(tenantId, upn); - return state == TableMemberState.Enabled ? role : null; - } - /// /// Gets the tri-state table membership for a user: whether a TenantAdmins row is absent, /// disabled, or enabled (with its role). The disabled/absent distinction is required by the /// Entra app-role resolver so a disabled row stays an explicit deny and is never silently - /// re-authorized through an app-role claim. + /// re-authorized through an app-role claim. Only the enabled state is cached (see _cacheDuration). /// public virtual async Task<(TableMemberState State, MemberRoleInfo? Role)> GetTableMembershipAsync(string tenantId, string? upn) { @@ -246,33 +196,41 @@ public class TenantAdminsService if (_cache.TryGetValue<(TableMemberState, MemberRoleInfo?)>(cacheKey, out var cached)) return cached; - var member = await _adminRepo.GetTenantMemberAsync(tenantId, upn); - - (TableMemberState State, MemberRoleInfo? Role) result; - if (member == null) - result = (TableMemberState.NotPresent, null); - else if (!member.IsEnabled) - result = (TableMemberState.Disabled, null); - else - result = (TableMemberState.Enabled, new MemberRoleInfo - { - Role = member.Role ?? Constants.TenantRoles.Admin, - CanManageBootstrapTokens = member.CanManageBootstrapTokens - }); - - _cache.Set(cacheKey, result, _cacheDuration); + var result = ToMembership(await _adminRepo.GetTenantMemberAsync(tenantId, upn)); + if (result.State == TableMemberState.Enabled) + _cache.Set(cacheKey, result, _cacheDuration); return result; } /// - /// Checks if a user can manage bootstrap tokens (Admin, or Operator with CanManageBootstrapTokens). + /// The caller's table membership and whether the tenant has any member row at all (disabled and + /// application rows included), both from ONE uncached partition read. auth/me decides the first-user + /// auto-admin from these two facts; read apart they disagree right after activation promotes the + /// onboarding member, and the new admin is answered as a roleless user. Never cache this: a cached + /// "no members" would make the next person who signs in the tenant's admin. /// - public async Task CanManageBootstrapAsync(string tenantId, string? upn) + public virtual async Task<(TableMemberState State, MemberRoleInfo? Role, bool TenantHasMembers)> GetMembershipSnapshotAsync( + string tenantId, string upn) { - var role = await GetMemberRoleAsync(tenantId, upn); - if (role == null) return false; - if (role.Role == Constants.TenantRoles.Admin) return true; - return role.Role == Constants.TenantRoles.Operator && role.CanManageBootstrapTokens; + SecurityValidator.EnsureValidGuid(tenantId, nameof(tenantId)); + + var members = await _adminRepo.GetTenantMembersAsync(tenantId); + var (state, role) = ToMembership( + members.FirstOrDefault(m => string.Equals(m.Upn, upn, StringComparison.OrdinalIgnoreCase))); + return (state, role, members.Count > 0); + } + + private static (TableMemberState State, MemberRoleInfo? Role) ToMembership(TenantMember? member) + { + if (member == null) + return (TableMemberState.NotPresent, null); + if (!member.IsEnabled) + return (TableMemberState.Disabled, null); + return (TableMemberState.Enabled, new MemberRoleInfo + { + Role = member.Role ?? Constants.TenantRoles.Admin, + CanManageBootstrapTokens = member.CanManageBootstrapTokens + }); } /// @@ -324,12 +282,10 @@ public class TenantAdminsService } /// - /// Invalidates both admin and member cache entries for a user. + /// Drops this process's cached membership of a user (other instances expire on their own). /// private void InvalidateMemberCache(string tenantId, string upn) { - _cache.Remove($"tenant-admin:{tenantId}:{upn}"); - _cache.Remove($"tenant-member:{tenantId}:{upn}"); _cache.Remove($"tenant-member-state:{tenantId}:{upn}"); } }