From e704aa735935b5f8a9aa55286aedd6d72e4b69e3 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Thu, 22 Oct 2026 20:47:53 +0200 Subject: [PATCH] Activation seats the onboarding admin before it activates the tenant; the first-user seat belongs to that member alone (D-378) Security finding (CWE-367): the activation wrote the approved row first and the OnboardedBy admin row a few storage round-trips later. In between the tenant was activated with no members, and auth/me seated any roleless caller of that moment as Admin; every member of the customer's Entra tenant could poll auth/me across the auto-approve moment. - TenantApprovalService: uncached pre-check, then the OnboardedBy Admin row (conditional insert, best-effort, fresh config read), then the activation row (still the arbiter), then the welcome mail. The two tables cannot share a batch, so "before" is the guarantee. - AuthFunction.BuildAuthResult: with a promotable OnboardedBy the first-user auto-admin goes to that UPN only; other roleless callers stay roleless. Without one (rows from before the field existed) the first sign-in still becomes the admin. The grant logs at Warning. - Both first-admin writers use IAdminRepository.TryAddTenantMemberAsync (AddEntityAsync, 409 = left untouched) instead of an upsert; TenantAdminsService.TryAddTenantAdminAsync replaces AddTenantAdminAsync. PreviewWhitelistService.IsApprovedFreshAsync is the pre-check. - Dead after this: IsTenantAdminAsync on the service, the repository interface and the table repository, plus its test. - Tests pin the write order, the designated-member guard (unit and auth/me end to end) and the conditional write; each part was mutated back to the old code and failed its tests. Co-Authored-By: Claude Fable 5.1 --- .claude/commands | 2 +- internal | 2 +- .../AuthFunctionSideEffectTests.cs | 66 +++++++++++--- .../AuthFunctionTests.cs | 59 ++++++++++++ .../TenantAdminsServiceMembershipTests.cs | 35 ++++--- .../TenantApprovalServiceTests.cs | 77 +++++++++++++--- .../TableStorage/TableAdminRepository.cs | 38 +++++--- .../Functions/Infrastructure/AuthFunction.cs | 21 ++++- .../Services/PreviewWhitelistService.cs | 8 ++ .../Services/TenantAdminsService.cs | 35 ++----- .../Services/TenantApprovalService.cs | 91 +++++++++++-------- .../DataAccess/IAdminRepository.cs | 8 +- 12 files changed, 320 insertions(+), 122 deletions(-) diff --git a/.claude/commands b/.claude/commands index b4fc66f0..5581e3e6 160000 --- a/.claude/commands +++ b/.claude/commands @@ -1 +1 @@ -Subproject commit b4fc66f0dffe1e106b402fe99e757b77cb939f59 +Subproject commit 5581e3e6c4a24c6bc9dd0198cb0d9410a00e6a27 diff --git a/internal b/internal index 2e4d84b0..db41d716 160000 --- a/internal +++ b/internal @@ -1 +1 @@ -Subproject commit 2e4d84b014672173a9b6184ddc21a658f9edf8d1 +Subproject commit db41d716d93a34fbe4b94a1df576ca853f78be1f diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs index d2d227ea..521ea607 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs @@ -780,14 +780,14 @@ public class AuthFunctionSideEffectTests public async Task PostDecision_NeedsAutoAdmin_CallsAddTenantAdmin() { _tenantAdminsMock - .Setup(x => x.AddTenantAdminAsync(TenantId, Upn, "System")) - .ReturnsAsync(new TenantAdminRow()); + .Setup(x => x.TryAddTenantAdminAsync(TenantId, Upn, "System")) + .ReturnsAsync(true); var decision = AuthDecisionResult.Success(new { }, needsAutoAdmin: true); await _sut.HandlePostDecisionSideEffectsAsync(decision, TenantId, Upn, DisplayName, ObjectId); - _tenantAdminsMock.Verify(x => x.AddTenantAdminAsync(TenantId, Upn, "System"), Times.Once); + _tenantAdminsMock.Verify(x => x.TryAddTenantAdminAsync(TenantId, Upn, "System"), Times.Once); } [Fact] @@ -798,7 +798,7 @@ public class AuthFunctionSideEffectTests await _sut.HandlePostDecisionSideEffectsAsync(decision, TenantId, Upn, DisplayName, ObjectId); _tenantAdminsMock.Verify( - x => x.AddTenantAdminAsync(It.IsAny(), It.IsAny(), It.IsAny()), + x => x.TryAddTenantAdminAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); } @@ -818,14 +818,14 @@ public class AuthFunctionSideEffectTests public async Task PostDecision_AutoAdminAndMetrics_BothExecute() { _tenantAdminsMock - .Setup(x => x.AddTenantAdminAsync(TenantId, Upn, "System")) - .ReturnsAsync(new TenantAdminRow()); + .Setup(x => x.TryAddTenantAdminAsync(TenantId, Upn, "System")) + .ReturnsAsync(true); var decision = AuthDecisionResult.Success(new { }, needsAutoAdmin: true); await _sut.HandlePostDecisionSideEffectsAsync(decision, TenantId, Upn, DisplayName, ObjectId); - _tenantAdminsMock.Verify(x => x.AddTenantAdminAsync(TenantId, Upn, "System"), Times.Once); + _tenantAdminsMock.Verify(x => x.TryAddTenantAdminAsync(TenantId, Upn, "System"), Times.Once); _metricsRepoMock.Verify( x => x.RecordUserLoginAsync(TenantId, Upn, DisplayName, ObjectId), Times.Once); @@ -849,7 +849,7 @@ public class AuthFunctionSideEffectTests public async Task PostDecision_AutoAdminFailure_Throws() { _tenantAdminsMock - .Setup(x => x.AddTenantAdminAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .Setup(x => x.TryAddTenantAdminAsync(It.IsAny(), It.IsAny(), It.IsAny())) .ThrowsAsync(new InvalidOperationException("Table Storage down")); var decision = AuthDecisionResult.Success(new { }, needsAutoAdmin: true); @@ -859,6 +859,21 @@ public class AuthFunctionSideEffectTests () => _sut.HandlePostDecisionSideEffectsAsync(decision, TenantId, Upn, DisplayName, ObjectId)); } + [Fact] + public async Task PostDecision_AutoAdminSeatAlreadyTaken_DoesNotThrow() + { + // The activation seated the same person a moment earlier (conditional insert lost): nothing to overwrite. + _tenantAdminsMock + .Setup(x => x.TryAddTenantAdminAsync(TenantId, Upn, "System")) + .ReturnsAsync(false); + + var decision = AuthDecisionResult.Success(new { }, needsAutoAdmin: true); + + await _sut.HandlePostDecisionSideEffectsAsync(decision, TenantId, Upn, DisplayName, ObjectId); + + _tenantAdminsMock.Verify(x => x.TryAddTenantAdminAsync(TenantId, Upn, "System"), Times.Once); + } + // ------------------------------------------------------------------------- // GetCurrentUser end to end: an external identity never claims the tenant // ------------------------------------------------------------------------- @@ -929,19 +944,46 @@ public class AuthFunctionSideEffectTests config.DomainName = "contoso.com"; ArrangeAuthMe(config, exists: true, approved: true); _tenantAdminsMock - .Setup(x => x.AddTenantAdminAsync(TenantId, Upn, "System")) - .ReturnsAsync(new TenantAdminRow()); + .Setup(x => x.TryAddTenantAdminAsync(TenantId, Upn, "System")) + .ReturnsAsync(true); var (req, ctx) = BuildRequest(SignedIn(external)); var response = await _sut.GetCurrentUser(req, ctx); Assert.Equal(HttpStatusCode.OK, response.StatusCode); _tenantAdminsMock.Verify( - x => x.AddTenantAdminAsync(It.IsAny(), It.IsAny(), It.IsAny()), + x => x.TryAddTenantAdminAsync(It.IsAny(), It.IsAny(), It.IsAny()), external ? Times.Never() : Times.Once()); Assert.Contains(external ? "\"isTenantAdmin\":false" : "\"isTenantAdmin\":true", await ReadBodyAsync(response)); } + /// + /// The activation race: the tenant is activated and its partition is empty for a moment while an + /// ordinary member polls auth/me. The first-user seat belongs to the designated onboarding member + /// alone; everybody else is answered roleless and nothing is written. + /// + [Theory] + [InlineData("first.member@contoso.com", false)] + [InlineData("USER@CONTOSO.COM", true)] + public async Task GetCurrentUser_ActivatedTenantWithoutAdmins_OnlyTheDesignatedOnboardingMemberTakesTheSeat( + string onboardedBy, bool callerIsTheOnboardingMember) + { + var config = DefaultConfig(); + config.DomainName = "contoso.com"; + config.OnboardedBy = onboardedBy; + ArrangeAuthMe(config, exists: true, approved: true); + _tenantAdminsMock.Setup(x => x.TryAddTenantAdminAsync(TenantId, Upn, "System")).ReturnsAsync(true); + var (req, ctx) = BuildRequest(SignedIn(external: false)); + + var response = await _sut.GetCurrentUser(req, ctx); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Contains(callerIsTheOnboardingMember ? "\"isTenantAdmin\":true" : "\"isTenantAdmin\":false", await ReadBodyAsync(response)); + _tenantAdminsMock.Verify( + x => x.TryAddTenantAdminAsync(It.IsAny(), It.IsAny(), It.IsAny()), + callerIsTheOnboardingMember ? Times.Once() : Times.Never()); + } + /// /// 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 @@ -987,6 +1029,8 @@ public class AuthFunctionSideEffectTests // 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); + adminRepo.Verify(r => r.TryAddTenantMemberAsync(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny()), Times.Never); } // ------------------------------------------------------------------------- diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionTests.cs index eb823100..e571c6a2 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionTests.cs @@ -545,6 +545,65 @@ public class AuthFunctionTests Assert.True((bool)body.isTenantAdmin); } + private static TenantConfiguration ConfigOnboardedBy(string? onboardedBy) + { + var config = DefaultConfig(); + config.OnboardedBy = onboardedBy; + return config; + } + + [Fact] + public void AutoAdmin_DesignatedOnboardingMember_AnyOtherRolelessCallerStaysRoleless() + { + // The activation race: the tenant is activated, its partition is empty for a moment, and a colleague + // polls auth/me. The first-user seat belongs to the onboarding member alone. + var result = AuthFunction.BuildAuthResult( + ConfigOnboardedBy("first.member@contoso.com"), isGlobalAdmin: false, isGlobalReader: false, isPreviewApproved: true, + memberRole: null, mcpCheck: McpDenied(), + hasTenantAdmins: false, + TenantId, Upn, DisplayName, ObjectId); + + Assert.True(result.IsSuccess); + Assert.False(result.NeedsAutoAdmin); + var body = ToDynamic(result.Body); + Assert.False((bool)body.isTenantAdmin); + Assert.Null((string?)body.role); + Assert.False((bool)body.canManageBootstrapTokens); + } + + [Fact] + public void AutoAdmin_DesignatedOnboardingMember_TakesTheSeatWhateverTheCasing() + { + var result = AuthFunction.BuildAuthResult( + ConfigOnboardedBy(Upn.ToUpperInvariant()), isGlobalAdmin: false, isGlobalReader: false, isPreviewApproved: true, + memberRole: null, mcpCheck: McpDenied(), + hasTenantAdmins: false, + TenantId, Upn, DisplayName, ObjectId); + + Assert.True(result.IsSuccess); + Assert.True(result.NeedsAutoAdmin); + var body = ToDynamic(result.Body); + Assert.True((bool)body.isTenantAdmin); + Assert.Equal(Constants.TenantRoles.Admin, (string)body.role); + } + + [Theory] + [InlineData(null)] + [InlineData("System (Global Rate Limit Sync)")] + [InlineData("alice_fabrikam.example#EXT#@contoso.example")] + public void AutoAdmin_WithoutADesignatedOnboardingMember_TheFirstSignInStillBecomesAdmin(string? onboardedBy) + { + // Rows from before OnboardedBy existed, and stored values the activation would never promote either. + var result = AuthFunction.BuildAuthResult( + ConfigOnboardedBy(onboardedBy), isGlobalAdmin: false, isGlobalReader: false, isPreviewApproved: true, + memberRole: null, mcpCheck: McpDenied(), + hasTenantAdmins: false, + TenantId, Upn, DisplayName, ObjectId); + + Assert.True(result.NeedsAutoAdmin); + Assert.True((bool)ToDynamic(result.Body).isTenantAdmin); + } + // ------------------------------------------------------------------------- // MCP Access // ------------------------------------------------------------------------- diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/TenantAdminsServiceMembershipTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/TenantAdminsServiceMembershipTests.cs index 24497bb2..30bd6cc2 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/TenantAdminsServiceMembershipTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/TenantAdminsServiceMembershipTests.cs @@ -71,17 +71,6 @@ public class TenantAdminsServiceMembershipTests _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() { @@ -148,4 +137,28 @@ public class TenantAdminsServiceMembershipTests await Assert.ThrowsAsync(() => _sut.GetMembershipSnapshotAsync("x' or PartitionKey ne '", Upn)); _repo.Verify(r => r.GetTenantMembersAsync(It.IsAny()), Times.Never); } + + [Fact] + public async Task TryAddTenantAdmin_CreatesTheRowLowercased_AndDropsTheCachedMembership() + { + _repo.Setup(r => r.TryAddTenantMemberAsync(TenantId, Upn, "system (auto-approve)", Constants.TenantRoles.Admin)).ReturnsAsync(true); + // A cached enabled row first, so the re-read after the seat is visible as a role change. + PointReadsReturn(Row(Upn, role: Constants.TenantRoles.Viewer), Row(Upn)); + Assert.Equal(Constants.TenantRoles.Viewer, (await _sut.GetTableMembershipAsync(TenantId, Upn)).Role!.Role); + + Assert.True(await _sut.TryAddTenantAdminAsync(TenantId, Upn.ToUpperInvariant(), "System (auto-approve)")); + + Assert.Equal(Constants.TenantRoles.Admin, (await _sut.GetTableMembershipAsync(TenantId, Upn)).Role!.Role); + } + + [Fact] + public async Task TryAddTenantAdmin_RowExists_AnswersFalseAndNeverUpserts() + { + _repo.Setup(r => r.TryAddTenantMemberAsync(TenantId, Upn, "system", Constants.TenantRoles.Admin)).ReturnsAsync(false); + + Assert.False(await _sut.TryAddTenantAdminAsync(TenantId, Upn, "System")); + + _repo.Verify(r => r.AddTenantMemberAsync(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny()), Times.Never); + } } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/TenantApprovalServiceTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/TenantApprovalServiceTests.cs index a323e9ec..684e487f 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/TenantApprovalServiceTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/TenantApprovalServiceTests.cs @@ -56,8 +56,8 @@ public class TenantApprovalServiceTests configRepo, NullLogger.Instance, cache); var alertDispatch = TestNotifications.InertOpsAlertDispatch(adminConfig.Object); - // IsTenantAdminAsync runs for real and reads through the virtual GetTableMembershipAsync, - // which ArrangeNewlyApproved sets up per test. + // TryAddTenantAdminAsync is virtual: ArrangeNewlyApproved answers it per test (true = seated, + // false = the row exists). The unset IsApprovedFreshAsync answers false: not activated yet. _tenantAdminsMock = new Mock( Mock.Of(), cache, NullLogger.Instance); @@ -84,16 +84,18 @@ public class TenantApprovalServiceTests } [Fact] - public async Task AlreadyApproved_ReturnsFalse_AndRunsNoSideEffects() + public async Task AlreadyApproved_ReturnsFalse_AndWritesNothing() { - _previewMock.Setup(x => x.ApproveAsync(TenantId, ApprovedBy)).ReturnsAsync(false); + // The fresh pre-check decides, not the 5-minute activation cache: a re-activation right after a + // revoke on another instance must not be answered as "already activated" from that cache. + _previewMock.Setup(x => x.IsApprovedFreshAsync(TenantId)).ReturnsAsync(true); var newlyApproved = await _sut.ApproveWithSideEffectsAsync(TenantId, ApprovedBy); Assert.False(newlyApproved); - // The side-effect block (auto-promote + welcome mail) starts with the config read — - // it must never run for a lost activation race. - _tenantConfigMock.Verify(x => x.GetConfigurationAsync(It.IsAny()), Times.Never); + _previewMock.Verify(x => x.ApproveAsync(It.IsAny(), It.IsAny()), Times.Never); + _tenantConfigMock.Verify(x => x.GetConfigurationFreshAsync(It.IsAny()), Times.Never); + _tenantAdminsMock.Verify(x => x.TryAddTenantAdminAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); _previewMock.Verify(x => x.GetNotificationEmailAsync(It.IsAny()), Times.Never); } @@ -123,14 +125,13 @@ 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)); + _tenantConfigMock.Setup(x => x.GetConfigurationFreshAsync(TenantId)).ReturnsAsync(config); + _tenantAdminsMock.Setup(x => x.TryAddTenantAdminAsync(TenantId, It.IsAny(), ApprovedBy)).ReturnsAsync(!alreadyAdmin); } private void VerifyNoPromotion() => _tenantAdminsMock.Verify( - x => x.AddTenantAdminAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + x => x.TryAddTenantAdminAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); [Fact] public async Task NewlyApproved_PromotesTheOnboardingMember() @@ -141,17 +142,19 @@ public class TenantApprovalServiceTests await _sut.ApproveWithSideEffectsAsync(TenantId, ApprovedBy); _tenantAdminsMock.Verify( - x => x.AddTenantAdminAsync(TenantId, "first.member@contoso.example", ApprovedBy), Times.Once); + x => x.TryAddTenantAdminAsync(TenantId, "first.member@contoso.example", ApprovedBy), Times.Once); } [Fact] - public async Task NewlyApproved_OnboardingMemberAlreadyAdmin_IsNotWrittenAgain() + public async Task NewlyApproved_OnboardingMemberAlreadySeated_IsLeftUntouchedAndActivationProceeds() { ArrangeNewlyApproved(onboardedBy: "first.member@contoso.example", alreadyAdmin: true); - await _sut.ApproveWithSideEffectsAsync(TenantId, ApprovedBy); + Assert.True(await _sut.ApproveWithSideEffectsAsync(TenantId, ApprovedBy)); - VerifyNoPromotion(); + // The conditional insert is the only write: a row the tenant holds (disabled, demoted) is never overwritten. + _tenantAdminsMock.Verify(x => x.TryAddTenantAdminAsync(TenantId, "first.member@contoso.example", ApprovedBy), Times.Once); + _previewMock.Verify(x => x.ApproveAsync(TenantId, ApprovedBy), Times.Once); } [Fact] @@ -175,6 +178,50 @@ public class TenantApprovalServiceTests VerifyNoPromotion(); } + [Fact] + public async Task NewlyApproved_SeatsTheOnboardingMemberBeforeTheActivationRow() + { + // The invariant behind the activation race: no observable state has the tenant activated while the + // designated admin's row is still missing. The seat is written first, the activation row second. + var writes = new List(); + ArrangeNewlyApproved(onboardedBy: "first.member@contoso.example"); + _tenantAdminsMock.Setup(x => x.TryAddTenantAdminAsync(TenantId, "first.member@contoso.example", ApprovedBy)) + .Callback(() => writes.Add("seat")).ReturnsAsync(true); + _previewMock.Setup(x => x.ApproveAsync(TenantId, ApprovedBy)) + .Callback(() => writes.Add("activate")).ReturnsAsync(true); + + Assert.True(await _sut.ApproveWithSideEffectsAsync(TenantId, ApprovedBy)); + + Assert.Equal(new[] { "seat", "activate" }, writes); + } + + [Fact] + public async Task LostActivationRace_SeatIsTheWinnersRow_ButNoMailAndNoOpsEvent() + { + ArrangeNewlyApproved(onboardedBy: "first.member@contoso.example"); + _previewMock.Setup(x => x.ApproveAsync(TenantId, ApprovedBy)).ReturnsAsync(false); + + Assert.False(await _sut.ApproveWithSideEffectsAsync(TenantId, ApprovedBy)); + + _tenantAdminsMock.Verify(x => x.TryAddTenantAdminAsync(TenantId, "first.member@contoso.example", ApprovedBy), Times.Once); + _previewMock.Verify(x => x.GetNotificationEmailAsync(It.IsAny()), Times.Never); + Assert.Empty(_opsEvents); + } + + [Fact] + public async Task NewlyApproved_SeatWriteFails_ActivationStillProceeds() + { + // Best-effort on purpose: with the seat missing, auth/me seats the designated member on their next + // sign-in and nobody else. + ArrangeNewlyApproved(onboardedBy: "first.member@contoso.example"); + _tenantAdminsMock.Setup(x => x.TryAddTenantAdminAsync(TenantId, It.IsAny(), ApprovedBy)) + .ThrowsAsync(new InvalidOperationException("Table Storage down")); + + Assert.True(await _sut.ApproveWithSideEffectsAsync(TenantId, ApprovedBy)); + + _previewMock.Verify(x => x.ApproveAsync(TenantId, ApprovedBy), Times.Once); + } + // --- TrySendWelcomeEmailAsync: the once-only send shared by the approval path and the // notification-email save path. Ordering contract under test: the sent-marker must be // consumed strictly AFTER the address check (a marker consumed with no address would diff --git a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableAdminRepository.cs b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableAdminRepository.cs index 221ac7d8..3a0ca28c 100644 --- a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableAdminRepository.cs +++ b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableAdminRepository.cs @@ -825,12 +825,33 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage } public async Task AddTenantMemberAsync(string tenantId, string upn, string addedBy, string role, bool canManageBootstrapTokens = false) + { + await _tenantAdminsTableClient.UpsertEntityAsync(NewMemberEntity(tenantId, upn, addedBy, role, canManageBootstrapTokens)); + return true; + } + + public async Task TryAddTenantMemberAsync(string tenantId, string upn, string addedBy, string role) + { + try + { + // Conditional INSERT, never upsert: a row the tenant already holds for this UPN + // (disabled, demoted) stays as the tenant set it, and two first-admin writers for + // the same UPN cannot both land. + await _tenantAdminsTableClient.AddEntityAsync(NewMemberEntity(tenantId, upn, addedBy, role, canManageBootstrapTokens: false)); + return true; + } + catch (RequestFailedException ex) when (ex.Status == 409) + { + return false; + } + } + + private static TenantAdminEntity NewMemberEntity(string tenantId, string upn, string addedBy, string role, bool canManageBootstrapTokens) { tenantId = tenantId.ToLowerInvariant(); upn = upn.ToLowerInvariant(); - addedBy = addedBy.ToLowerInvariant(); - var entity = new TenantAdminEntity + return new TenantAdminEntity { PartitionKey = tenantId, RowKey = upn, @@ -838,13 +859,10 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage Upn = upn, IsEnabled = true, AddedDate = DateTime.UtcNow, - AddedBy = addedBy, + AddedBy = addedBy.ToLowerInvariant(), Role = role, CanManageBootstrapTokens = canManageBootstrapTokens }; - - await _tenantAdminsTableClient.UpsertEntityAsync(entity); - return true; } public async Task RemoveTenantMemberAsync(string tenantId, string upn) @@ -921,14 +939,6 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage } } - public async Task IsTenantAdminAsync(string tenantId, string upn) - { - var member = await GetTenantMemberAsync(tenantId, upn); - if (member == null || !member.IsEnabled) return false; - // Only true for Admin role (null Role = Admin for backward compat) - return member.Role == null || member.Role == Constants.TenantRoles.Admin; - } - // --- Helpers --- private static DelegatedAdminEntry MapToDelegatedEntry(DelegatedAdminEntity entity) diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs index f9b96ef2..ff151e40 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs @@ -701,8 +701,15 @@ public class AuthFunction { if (decision.NeedsAutoAdmin) { - _logger.LogInformation("First user login for tenant {TenantId}: {Upn} - Auto-assigning as admin", tenantId, upn); - await _tenantAdminsService.AddTenantAdminAsync(tenantId, upn, "System"); + // Warning on purpose: activation seats the onboarding member before it activates the tenant, so + // this path is the exception (a tenant from before OnboardedBy existed, or a seat that went + // missing), and Information never reaches Application Insights. A lost conditional insert means + // a row for this caller appeared meanwhile (activation seating the same person, or a Global + // Admin); it is left as written and the next request reads it. + if (await _tenantAdminsService.TryAddTenantAdminAsync(tenantId, upn, "System")) + _logger.LogWarning("First-user auto-admin: {Upn} seated as Admin of tenant {TenantId}", upn, tenantId); + else + _logger.LogInformation("First-user auto-admin for {Upn} in tenant {TenantId}: a member row appeared meanwhile — left untouched", upn, tenantId); } _ = _metricsRepo.RecordUserLoginAsync(tenantId, upn, displayName, objectId) @@ -788,8 +795,16 @@ public class AuthFunction // it. A delegated user who legitimately administers their own tenant has a TenantAdmins row (memberRole // != null), so this only suppresses the silent grant, never an existing right. The same holds for an // external identity (a guest), which is not a member of this tenant at all. + // Once the tenant has a designated onboarding member (a promotable OnboardedBy), the first-user + // seat is theirs alone: activation seats them before it activates the tenant, and should that + // seat ever be missing (removed, or the write failed) only they may take it on sign-in. Any other + // roleless caller is answered roleless, never the first caller to see an activated tenant with + // no rows. Without a designated member (rows from before OnboardedBy existed) the first sign-in + // still becomes the admin. + var onboardingMember = TenantApprovalService.IsPromotableUpn(tenantConfig.OnboardedBy) ? tenantConfig.OnboardedBy : null; bool isTenantAdmin = memberRole?.Role == Constants.TenantRoles.Admin; - bool needsAutoAdmin = memberRole == null && !hasTenantAdmins && !isGlobalReader && !isDelegated && !isExternalIdentity; + bool needsAutoAdmin = memberRole == null && !hasTenantAdmins && !isGlobalReader && !isDelegated && !isExternalIdentity + && (onboardingMember == null || string.Equals(onboardingMember, upn, StringComparison.OrdinalIgnoreCase)); if (needsAutoAdmin) { isTenantAdmin = true; diff --git a/src/Backend/AutopilotMonitor.Functions/Services/PreviewWhitelistService.cs b/src/Backend/AutopilotMonitor.Functions/Services/PreviewWhitelistService.cs index c9fb0fca..9fc337a2 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/PreviewWhitelistService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/PreviewWhitelistService.cs @@ -64,6 +64,14 @@ public class PreviewWhitelistService } } + /// + /// Uncached read of the activation, for the activation path's own pre-check: a cached activation can + /// outlive a revoke on another instance by up to 5 minutes, and a re-activation must not be answered + /// as "already activated" from that cache. Storage errors throw. + /// + public virtual Task IsApprovedFreshAsync(string tenantId) + => _configRepo.IsInPreviewWhitelistAsync(tenantId); + /// /// Activates a tenant (adds it to the whitelist). Returns true when this call created /// the entry, false when the tenant was already activated — the conditional insert at diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TenantAdminsService.cs b/src/Backend/AutopilotMonitor.Functions/Services/TenantAdminsService.cs index 3cdf5d85..c6f856dc 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TenantAdminsService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TenantAdminsService.cs @@ -65,41 +65,24 @@ public class TenantAdminsService => _pushRevoker?.RevokeOwnerAsync(Notifications.NotificationScope.Tenant(tenantId), upn, reason) ?? Task.CompletedTask; /// - /// 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. + /// Seats a user as Tenant Admin unless the tenant already holds a row for them: true when the row was + /// created, false when one exists (left untouched, whatever it says). The two first-admin writers, + /// activation seating the onboarding member and auth/me's first-user auto-admin, go through here, so + /// neither can overwrite a row the tenant set (disabled, demoted) nor race the other for the same UPN. /// - public async Task IsTenantAdminAsync(string tenantId, string? upn) - { - var (state, role) = await GetTableMembershipAsync(tenantId, upn); - return state == TableMemberState.Enabled && role?.Role == Constants.TenantRoles.Admin; - } - - /// - /// Adds a user as a Tenant Admin - /// - /// Tenant ID - /// User Principal Name - /// UPN of the admin who is adding this user - public virtual async Task AddTenantAdminAsync(string tenantId, string upn, string addedBy) + public virtual async Task TryAddTenantAdminAsync(string tenantId, string upn, string addedBy) { tenantId = tenantId.ToLowerInvariant(); upn = upn.ToLowerInvariant(); addedBy = addedBy.ToLowerInvariant(); - await _adminRepo.AddTenantMemberAsync(tenantId, upn, addedBy, Constants.TenantRoles.Admin); + if (!await _adminRepo.TryAddTenantMemberAsync(tenantId, upn, addedBy, Constants.TenantRoles.Admin)) + return false; InvalidateMemberCache(tenantId, upn); - _logger.LogInformation($"Added Tenant Admin: {upn} to tenant {tenantId} by {addedBy}"); - - return new TenantAdminRow - { - TenantId = tenantId, - Upn = upn, - IsEnabled = true, - AddedDate = DateTime.UtcNow, - AddedBy = addedBy - }; + _logger.LogInformation("Added Tenant Admin: {Upn} to tenant {TenantId} by {AddedBy}", upn, tenantId, addedBy); + return true; } /// diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TenantApprovalService.cs b/src/Backend/AutopilotMonitor.Functions/Services/TenantApprovalService.cs index e42983b3..8a1b97c3 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TenantApprovalService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TenantApprovalService.cs @@ -36,77 +36,90 @@ public class TenantApprovalService } /// - /// Activates a tenant: adds it to the whitelist, then runs the best-effort side - /// effects. The whitelist add is the activation itself and fails loud; everything - /// after it is non-fatal. Returns false when the tenant was ALREADY activated - /// (concurrent duplicate or repeat approve) — the conditional whitelist insert makes - /// this idempotent, so a lost race never re-sends the welcome mail or re-promotes. + /// Activates a tenant. The order of the writes is the invariant: the onboarding member's Admin row + /// is seated BEFORE the activation row, so no observable state has the tenant activated while the + /// designated admin's row is still missing (auth/me would otherwise seat the first roleless caller + /// of that moment). The two rows live in different tables, which rules out one atomic batch, hence + /// "before". The conditional activation insert stays the arbiter and fails loud; false means the + /// tenant was activated concurrently (double signup), and the seat written just before is the same + /// row the winner seats, so the loser only skips the mail. A tenant that is already activated + /// returns false before anything is written. Everything after the activation row is best-effort. /// public virtual async Task ApproveWithSideEffectsAsync(string tenantId, string approvedBy) { + if (await _previewWhitelistService.IsApprovedFreshAsync(tenantId)) + { + _logger.LogInformation( + "Tenant {TenantId} already activated — nothing to do (approve by {ApprovedBy})", + tenantId, approvedBy); + return false; + } + + await TrySeatOnboardingMemberAsync(tenantId, approvedBy); + if (!await _previewWhitelistService.ApproveAsync(tenantId, approvedBy)) { _logger.LogInformation( - "Tenant {TenantId} already activated — skipping activation side effects (approve by {ApprovedBy})", + "Tenant {TenantId} was activated concurrently — skipping activation side effects (approve by {ApprovedBy})", tenantId, approvedBy); return false; } _logger.LogInformation("Tenant activated: {TenantId} by {ApprovedBy}", tenantId, approvedBy); - // Auto-promote the onboarding member (OnboardedBy, written once by the first member login and - // never by an external identity) as TenantAdmin if they are not already one. - // This ensures whoever signed up doesn't need manual admin assignment after activation. - // Never UpdatedBy: that is the last writer, not the requester, and any signed-in user sets it. + await TrySendWelcomeEmailAsync(tenantId); + + return true; + } + + /// + /// Seats the onboarding member (OnboardedBy, written once by the first member login and never by an + /// external identity; never UpdatedBy, the last writer) as Tenant Admin, so whoever signed up needs + /// no manual assignment after activation. A row the tenant already holds for them stays untouched. + /// Best-effort: a storage failure here must not block the activation. With the seat missing, auth/me + /// seats the designated member on their next sign-in and nobody else. + /// + private async Task TrySeatOnboardingMemberAsync(string tenantId, string approvedBy) + { try { - var tenantConfig = await _tenantConfigurationService.GetConfigurationAsync(tenantId); - var requesterUpn = tenantConfig.OnboardedBy; + // Fresh read: a cached view can be the default row an earlier request on this instance + // created before the first sign-in seeded OnboardedBy. + var tenantConfig = await _tenantConfigurationService.GetConfigurationFreshAsync(tenantId); + var requesterUpn = tenantConfig?.OnboardedBy; // Positive UPN-shape validation: real Azure AD UPNs always contain '@', while // every system-written sentinel ("System", "System (auto-re-enable)", // "System (Global Rate Limit Sync)", …) does not. An equality list against // known sentinels previously missed a third sentinel and corrupted 10 tenants' // TenantAdmins rows — keep this check shape-based, not enumeration-based. - if (IsPromotableUpn(requesterUpn)) + if (!IsPromotableUpn(requesterUpn)) { - // IsPromotableUpn returned true → requesterUpn is non-null, non-empty, contains '@' - var validUpn = requesterUpn!; - var isAlreadyAdmin = await _tenantAdminsService.IsTenantAdminAsync(tenantId, validUpn); - if (!isAlreadyAdmin) - { - await _tenantAdminsService.AddTenantAdminAsync(tenantId, validUpn, approvedBy); - _logger.LogInformation( - "Auto-promoted tenant requester {RequesterUpn} as TenantAdmin for tenant {TenantId} on activation by {ApprovedBy}", - validUpn, tenantId, approvedBy); - } - else - { - _logger.LogInformation( - "Tenant requester {RequesterUpn} is already a TenantAdmin for tenant {TenantId} — skipping auto-promote", - validUpn, tenantId); - } + _logger.LogInformation( + "No promotable onboarding UPN in TenantConfiguration for tenant {TenantId} (OnboardedBy: '{OnboardedBy}') — skipping auto-promote", + tenantId, requesterUpn ?? ""); + return; + } + + if (await _tenantAdminsService.TryAddTenantAdminAsync(tenantId, requesterUpn!, approvedBy)) + { + _logger.LogInformation( + "Auto-promoted tenant requester {RequesterUpn} as TenantAdmin for tenant {TenantId} on activation by {ApprovedBy}", + requesterUpn, tenantId, approvedBy); } else { _logger.LogInformation( - "No promotable onboarding UPN in TenantConfiguration for tenant {TenantId} (OnboardedBy: '{OnboardedBy}') — skipping auto-promote", - tenantId, tenantConfig.OnboardedBy ?? ""); + "Tenant requester {RequesterUpn} already has a member row in tenant {TenantId} — left untouched", + requesterUpn, tenantId); } - } catch (Exception ex) { - // Non-fatal: activation already succeeded, admin promotion is best-effort _logger.LogWarning(ex, - "Failed to auto-promote tenant requester as TenantAdmin for tenant {TenantId} — activation still succeeded", + "Failed to auto-promote tenant requester as TenantAdmin for tenant {TenantId} — activation continues", tenantId); } - - // Outside the promote try/catch: a promote failure must not swallow the mail. - await TrySendWelcomeEmailAsync(tenantId); - - return true; } /// diff --git a/src/Shared/AutopilotMonitor.Shared/DataAccess/IAdminRepository.cs b/src/Shared/AutopilotMonitor.Shared/DataAccess/IAdminRepository.cs index 8af1bb00..cf0ea0d7 100644 --- a/src/Shared/AutopilotMonitor.Shared/DataAccess/IAdminRepository.cs +++ b/src/Shared/AutopilotMonitor.Shared/DataAccess/IAdminRepository.cs @@ -98,11 +98,17 @@ namespace AutopilotMonitor.Shared.DataAccess // --- Tenant Members --- Task> GetTenantMembersAsync(string tenantId); Task AddTenantMemberAsync(string tenantId, string upn, string addedBy, string role, bool canManageBootstrapTokens = false); + /// + /// Conditional insert of a member row: true when this call created it, false when a row for the + /// UPN already exists (left untouched, whatever it says). The first-admin writers (activation + /// seating the onboarding member, the first-user auto-admin) use this so a row the tenant already + /// holds is never overwritten. Storage errors throw. + /// + Task TryAddTenantMemberAsync(string tenantId, string upn, string addedBy, string role); Task RemoveTenantMemberAsync(string tenantId, string upn); Task UpdateMemberPermissionsAsync(string tenantId, string upn, string role, bool canManageBootstrapTokens); Task SetTenantMemberEnabledAsync(string tenantId, string upn, bool isEnabled); Task GetTenantMemberAsync(string tenantId, string upn); - Task IsTenantAdminAsync(string tenantId, string upn); } public class GlobalAdminEntry