mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-23 20:26:10 +00:00
New tenants' first admin keeps the Admin role right after activation (D-376)
After auto-activation, a third of new tenants' first admins were answered by auth/me without a role and sent from the activation page to /progress. The auto-approve worker promotes the onboarding member on another instance, so the portal's caches never hear of it: auth/me combined a cached "no row" (refreshed 1.5 s after every activation poll by feedback/status) with a fresh member list, and neither the role nor the first-user auto-admin applied. - TenantAdminsService caches only an enabled membership; a missing or disabled row is read on every call. IsTenantAdminAsync reads through the same cache. - auth/me takes the caller's row and "the tenant has any row" from one uncached partition read (GetMembershipSnapshotAsync); disabled and application rows still count as members. - PreviewWhitelistService caches only an activation; "not activated" is read on every call, so the activation page leaves at the first poll after approval. IsApprovedFreshAsync is folded into IsApprovedAsync. - Removed the dead ClearCacheForTenant, GetMemberRoleAsync and CanManageBootstrapAsync. Tests reproduce the sequence over the real caches (auth/me and the first dashboard request); each part fails against the old caching. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
+1
-1
Submodule .claude/commands updated: 59a7eb8a00...b4fc66f0df
+1
-1
Submodule internal updated: 43055e93ea...650ea7e006
@@ -43,7 +43,8 @@ public class AuthFunctionSideEffectTests
|
||||
private readonly FakeOffboardingAuditRepository _offboardingRepo = new();
|
||||
private readonly StoredTenantConfig _store;
|
||||
private readonly Mock<GlobalAdminService> _globalAdminMock;
|
||||
private readonly Func<PushOwnerSignInService?, AuthFunction> _buildSut;
|
||||
/// <summary>Builds the function; null arguments fall back to the class mocks.</summary>
|
||||
private readonly Func<PushOwnerSignInService?, TenantAdminsService?, PreviewWhitelistService?, AuthFunction> _buildSut;
|
||||
private readonly AuthFunction _sut;
|
||||
|
||||
public AuthFunctionSideEffectTests()
|
||||
@@ -106,14 +107,14 @@ public class AuthFunctionSideEffectTests
|
||||
_opsEventsMock = new Mock<OpsEventService>(null!, null!, null!) { CallBase = false };
|
||||
_opsEventsMock.Setup(x => x.RecordTenantSignupAsync(It.IsAny<string>(), It.IsAny<string>())).Returns(Task.CompletedTask);
|
||||
|
||||
_buildSut = pushSignIn => new AuthFunction(
|
||||
_buildSut = (pushSignIn, tenantAdmins, preview) => new AuthFunction(
|
||||
Mock.Of<ILogger<AuthFunction>>(),
|
||||
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<TenantAdminRow>());
|
||||
_tenantAdminsMock.Setup(x => x.GetMembershipSnapshotAsync(TenantId, Upn))
|
||||
.ReturnsAsync((TableMemberState.NotPresent, null, false));
|
||||
_mcpUserMock
|
||||
.Setup(x => x.IsAllowedAsync(It.IsAny<string?>(), It.IsAny<string?>(), It.IsAny<string?>(),
|
||||
It.IsAny<IReadOnlyList<string>?>(), It.IsAny<bool>()))
|
||||
@@ -940,6 +942,53 @@ public class AuthFunctionSideEffectTests
|
||||
Assert.Contains(external ? "\"isTenantAdmin\":false" : "\"isTenantAdmin\":true", await ReadBodyAsync(response));
|
||||
}
|
||||
|
||||
/// <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
|
||||
/// 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).
|
||||
/// </summary>
|
||||
[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<TenantMember>();
|
||||
var configRepo = new Mock<IConfigRepository>();
|
||||
configRepo.Setup(r => r.IsInPreviewWhitelistAsync(TenantId)).ReturnsAsync(() => activated);
|
||||
var adminRepo = new Mock<IAdminRepository>();
|
||||
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<ILogger<TenantAdminsService>>());
|
||||
var sut = _buildSut(null, tenantAdmins, new PreviewWhitelistService(
|
||||
configRepo.Object, cache, Mock.Of<ILogger<PreviewWhitelistService>>(), _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<string>(), It.IsAny<string>(), It.IsAny<string>(),
|
||||
It.IsAny<string>(), It.IsAny<bool>()), 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<TenantAdminRow> { 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<IAdminRepository>().Object, push.Settings, push.Sut,
|
||||
Mock.Of<ILogger<PushOwnerSignInService>>());
|
||||
return (_buildSut(signIn), push);
|
||||
return (_buildSut(signIn, null, null), push);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
|
||||
@@ -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 ──────────────
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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]
|
||||
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public class PreviewWhitelistApprovalCacheTests
|
||||
{
|
||||
private const string TenantId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee";
|
||||
|
||||
private readonly Mock<IConfigRepository> _repo = new();
|
||||
private readonly PreviewWhitelistService _sut;
|
||||
|
||||
public PreviewWhitelistApprovalCacheTests()
|
||||
{
|
||||
var cache = new MemoryCache(new MemoryCacheOptions());
|
||||
_sut = new PreviewWhitelistService(_repo.Object, cache, NullLogger<PreviewWhitelistService>.Instance,
|
||||
new TenantConfigurationService(_repo.Object, NullLogger<TenantConfigurationService>.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);
|
||||
}
|
||||
}
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public class TenantAdminsServiceMembershipTests
|
||||
{
|
||||
private const string TenantId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee";
|
||||
private const string Upn = "first.admin@contoso.com";
|
||||
|
||||
private readonly Mock<IAdminRepository> _repo = new();
|
||||
private readonly TenantAdminsService _sut;
|
||||
|
||||
public TenantAdminsServiceMembershipTests()
|
||||
{
|
||||
_sut = new TenantAdminsService(
|
||||
_repo.Object, new MemoryCache(new MemoryCacheOptions()), NullLogger<TenantAdminsService>.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<TenantMember?>(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<TenantMember>())
|
||||
.ReturnsAsync(new List<TenantMember> { 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<ArgumentException>(() => _sut.GetMembershipSnapshotAsync("x' or PartitionKey ne '", Upn));
|
||||
_repo.Verify(r => r.GetTenantMembersAsync(It.IsAny<string>()), Times.Never);
|
||||
}
|
||||
}
|
||||
@@ -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<AdminConfigurationService>.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<TenantAdminsService>(
|
||||
Mock.Of<IAdminRepository>(), cache, NullLogger<TenantAdminsService>.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<string?>())).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()
|
||||
{
|
||||
|
||||
@@ -13,7 +13,7 @@ namespace AutopilotMonitor.Functions.Tests;
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public class TenantMemberRoleResolverTests
|
||||
{
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -20,14 +20,6 @@ public class PreviewWhitelistService
|
||||
private readonly TenantConfigurationService _tenantConfigService;
|
||||
private static readonly TimeSpan CacheDuration = TimeSpan.FromMinutes(5);
|
||||
|
||||
/// <summary>
|
||||
/// 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).
|
||||
/// </summary>
|
||||
private static readonly TimeSpan NotApprovedCacheDuration = TimeSpan.FromSeconds(30);
|
||||
|
||||
public PreviewWhitelistService(
|
||||
IConfigRepository configRepo,
|
||||
IMemoryCache cache,
|
||||
@@ -41,7 +33,11 @@ public class PreviewWhitelistService
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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).
|
||||
/// </summary>
|
||||
public virtual async Task<bool> 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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Fresh, uncached approval check. Used by the notification-email save path to decide
|
||||
/// send-on-save: the 30s negative cache of <see cref="IsApprovedAsync"/> 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.
|
||||
/// </summary>
|
||||
public virtual async Task<bool> 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;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Revokes a tenant's activation.
|
||||
/// </summary>
|
||||
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// 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 <see cref="GetTableMembershipAsync"/>, so it shares that cache and its rules.
|
||||
/// </summary>
|
||||
/// <param name="tenantId">Tenant ID</param>
|
||||
/// <param name="upn">User Principal Name (e.g., oliver@contoso.com)</param>
|
||||
/// <returns>True if the user is a Tenant Admin for this tenant</returns>
|
||||
public async Task<bool> 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<bool>(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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
@@ -202,37 +174,15 @@ public class TenantAdminsService
|
||||
}).ToList();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Clears the cache for a specific tenant's admins
|
||||
/// Useful after bulk updates
|
||||
/// </summary>
|
||||
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)
|
||||
// -----------------------------------------------------------------------
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
/// <see cref="GetTableMembershipAsync"/>, which distinguishes "no row" from "disabled row".
|
||||
/// </summary>
|
||||
public virtual async Task<MemberRoleInfo?> GetMemberRoleAsync(string tenantId, string? upn)
|
||||
{
|
||||
var (state, role) = await GetTableMembershipAsync(tenantId, upn);
|
||||
return state == TableMemberState.Enabled ? role : null;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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).
|
||||
/// </summary>
|
||||
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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public async Task<bool> 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
|
||||
});
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
@@ -324,12 +282,10 @@ public class TenantAdminsService
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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).
|
||||
/// </summary>
|
||||
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}");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user