mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-22 19:55:35 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
+1
-1
Submodule .claude/commands updated: b4fc66f0df...5581e3e6c4
+1
-1
Submodule internal updated: 2e4d84b014...db41d716d9
@@ -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<string>(), It.IsAny<string>(), It.IsAny<string>()),
|
||||
x => x.TryAddTenantAdminAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>()),
|
||||
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<string>(), It.IsAny<string>(), It.IsAny<string>()))
|
||||
.Setup(x => x.TryAddTenantAdminAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>()))
|
||||
.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<string>(), It.IsAny<string>(), It.IsAny<string>()),
|
||||
x => x.TryAddTenantAdminAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>()),
|
||||
external ? Times.Never() : Times.Once());
|
||||
Assert.Contains(external ? "\"isTenantAdmin\":false" : "\"isTenantAdmin\":true", await ReadBodyAsync(response));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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<string>(), It.IsAny<string>(), It.IsAny<string>()),
|
||||
callerIsTheOnboardingMember ? Times.Once() : Times.Never());
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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<string>(), It.IsAny<string>(), It.IsAny<string>(),
|
||||
It.IsAny<string>(), It.IsAny<bool>()), Times.Never);
|
||||
adminRepo.Verify(r => r.TryAddTenantMemberAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>(),
|
||||
It.IsAny<string>()), Times.Never);
|
||||
}
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
|
||||
@@ -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
|
||||
// -------------------------------------------------------------------------
|
||||
|
||||
+24
-11
@@ -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<ArgumentException>(() => _sut.GetMembershipSnapshotAsync("x' or PartitionKey ne '", Upn));
|
||||
_repo.Verify(r => r.GetTenantMembersAsync(It.IsAny<string>()), 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<string>(), It.IsAny<string>(), It.IsAny<string>(),
|
||||
It.IsAny<string>(), It.IsAny<bool>()), Times.Never);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -56,8 +56,8 @@ public class TenantApprovalServiceTests
|
||||
configRepo, NullLogger<AdminConfigurationService>.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<TenantAdminsService>(
|
||||
Mock.Of<IAdminRepository>(), cache, NullLogger<TenantAdminsService>.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<string>()), Times.Never);
|
||||
_previewMock.Verify(x => x.ApproveAsync(It.IsAny<string>(), It.IsAny<string>()), Times.Never);
|
||||
_tenantConfigMock.Verify(x => x.GetConfigurationFreshAsync(It.IsAny<string>()), Times.Never);
|
||||
_tenantAdminsMock.Verify(x => x.TryAddTenantAdminAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>()), Times.Never);
|
||||
_previewMock.Verify(x => x.GetNotificationEmailAsync(It.IsAny<string>()), 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<string?>())).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<string>(), ApprovedBy)).ReturnsAsync(!alreadyAdmin);
|
||||
}
|
||||
|
||||
private void VerifyNoPromotion() =>
|
||||
_tenantAdminsMock.Verify(
|
||||
x => x.AddTenantAdminAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>()), Times.Never);
|
||||
x => x.TryAddTenantAdminAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>()), 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<string>();
|
||||
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<string>()), 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<string>(), 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
|
||||
|
||||
+24
-14
@@ -825,12 +825,33 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage
|
||||
}
|
||||
|
||||
public async Task<bool> 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<bool> 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<bool> RemoveTenantMemberAsync(string tenantId, string upn)
|
||||
@@ -921,14 +939,6 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage
|
||||
}
|
||||
}
|
||||
|
||||
public async Task<bool> 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)
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -64,6 +64,14 @@ public class PreviewWhitelistService
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public virtual Task<bool> IsApprovedFreshAsync(string tenantId)
|
||||
=> _configRepo.IsInPreviewWhitelistAsync(tenantId);
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
|
||||
@@ -65,41 +65,24 @@ public class TenantAdminsService
|
||||
=> _pushRevoker?.RevokeOwnerAsync(Notifications.NotificationScope.Tenant(tenantId), upn, reason) ?? Task.CompletedTask;
|
||||
|
||||
/// <summary>
|
||||
/// True if the UPN holds an enabled Admin row in the tenant (a row without a role is an Admin).
|
||||
/// Reads through <see cref="GetTableMembershipAsync"/>, 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.
|
||||
/// </summary>
|
||||
public async Task<bool> IsTenantAdminAsync(string tenantId, string? upn)
|
||||
{
|
||||
var (state, role) = await GetTableMembershipAsync(tenantId, upn);
|
||||
return state == TableMemberState.Enabled && role?.Role == Constants.TenantRoles.Admin;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Adds a user as a Tenant Admin
|
||||
/// </summary>
|
||||
/// <param name="tenantId">Tenant ID</param>
|
||||
/// <param name="upn">User Principal Name</param>
|
||||
/// <param name="addedBy">UPN of the admin who is adding this user</param>
|
||||
public virtual async Task<TenantAdminRow> AddTenantAdminAsync(string tenantId, string upn, string addedBy)
|
||||
public virtual async Task<bool> 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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
|
||||
@@ -36,77 +36,90 @@ public class TenantApprovalService
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public virtual async Task<bool> 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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
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 ?? "<null>");
|
||||
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 ?? "<null>");
|
||||
"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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
|
||||
@@ -98,11 +98,17 @@ namespace AutopilotMonitor.Shared.DataAccess
|
||||
// --- Tenant Members ---
|
||||
Task<List<TenantMember>> GetTenantMembersAsync(string tenantId);
|
||||
Task<bool> AddTenantMemberAsync(string tenantId, string upn, string addedBy, string role, bool canManageBootstrapTokens = false);
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
Task<bool> TryAddTenantMemberAsync(string tenantId, string upn, string addedBy, string role);
|
||||
Task<bool> RemoveTenantMemberAsync(string tenantId, string upn);
|
||||
Task<bool> UpdateMemberPermissionsAsync(string tenantId, string upn, string role, bool canManageBootstrapTokens);
|
||||
Task<bool> SetTenantMemberEnabledAsync(string tenantId, string upn, bool isEnabled);
|
||||
Task<TenantMember?> GetTenantMemberAsync(string tenantId, string upn);
|
||||
Task<bool> IsTenantAdminAsync(string tenantId, string upn);
|
||||
}
|
||||
|
||||
public class GlobalAdminEntry
|
||||
|
||||
Reference in New Issue
Block a user