From dd4975b73e0c724104a4984954cd8d084e3a042b Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Tue, 13 Oct 2026 01:19:56 +0200 Subject: [PATCH] Auth: an external identity never claims a tenant (D-353) A caller whose token shows it is not a member of the token's tenant (acct = 1, a #EXT# user name, or an idp naming another tenant, a personal Microsoft account or an external sign-in provider) no longer seeds DomainName/OnboardedBy or queues the activation, is never the first-user Tenant Admin, and cannot set the activation-page address in the roleless signup window. Positive evidence only: a token without these signals stays a member's, and tid/idp are read under their inbound-mapped names as well. Activation promotes OnboardedBy only; the UpdatedBy fallback is gone and a stored #EXT# UPN is refused. Explicit grants (TenantAdmins row, app role) still apply. The once-per-interval log gate of the client-app binding becomes Helpers/LogThrottle and also throttles the new warning, which the polling activation page would otherwise repeat. Co-Authored-By: Claude Opus 5.5 --- .../AuthFunctionSideEffectTests.cs | 103 ++++++++++++++-- .../AuthFunctionTests.cs | 37 ++++++ ...nticationMiddlewareTokenValidationTests.cs | 39 +++++- .../ExternalIdentityTests.cs | 108 +++++++++++++++++ .../LogThrottleTests.cs | 53 ++++++++ .../PreviewNotificationEmailWriteGateTests.cs | 30 ++++- .../PreviewWhitelistRequesterUpnTests.cs | 114 ++++-------------- .../TenantApprovalServiceTests.cs | 61 +++++++++- .../TenantAutoApproveHandlerTests.cs | 2 +- .../Extensions/ClaimsPrincipalExtensions.cs | 69 +++++++++++ .../Functions/Infrastructure/AuthFunction.cs | 28 ++++- .../Rules/PreviewWhitelistFunction.cs | 21 ++-- .../Helpers/LogThrottle.cs | 34 ++++++ .../Middleware/ClientAppBindingMiddleware.cs | 13 +- .../Security/EndpointAccessPolicyCatalog.cs | 3 +- .../Activation/TenantAutoApproveHandler.cs | 2 +- .../Services/TenantApprovalService.cs | 44 +++---- .../src/generated/wire-types.generated.ts | 4 +- .../Models/Config/TenantConfiguration.cs | 19 +-- .../components/AdminManagementSection.tsx | 2 +- .../lib/generated/shared-manifests.json | 4 +- .../lib/generated/wire-types.generated.ts | 4 +- 22 files changed, 609 insertions(+), 185 deletions(-) create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/ExternalIdentityTests.cs create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/LogThrottleTests.cs create mode 100644 src/Backend/AutopilotMonitor.Functions/Helpers/LogThrottle.cs diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs index 20317ebc..e0d43f39 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs @@ -31,6 +31,8 @@ public class AuthFunctionSideEffectTests private readonly Mock _tenantConfigMock; private readonly Mock _tenantAdminsMock; + private readonly Mock _previewMock; + private readonly Mock _mcpUserMock; private readonly Mock _telegramMock; private readonly Mock _opsEventsMock; private readonly Mock _globalNotificationMock; @@ -71,7 +73,7 @@ public class AuthFunctionSideEffectTests adminRepo, cache, Mock.Of>()) { CallBase = false }; - var previewMock = new Mock( + _previewMock = new Mock( configRepo, cache, Mock.Of>(), _tenantConfigMock.Object) { CallBase = false }; @@ -87,7 +89,7 @@ public class AuthFunctionSideEffectTests configRepo, Mock.Of>(), cache) { CallBase = false }; - var mcpUserMock = new Mock( + _mcpUserMock = new Mock( adminRepo, new StubAdminIdentityBindingService(bound: true), cache, Mock.Of>(), globalAdminMock.Object, delegatedAdminMock.Object, adminConfigService.Object, new TenantMemberRoleResolver(_tenantAdminsMock.Object, _tenantConfigMock.Object), _tenantConfigMock.Object) @@ -107,10 +109,10 @@ public class AuthFunctionSideEffectTests _tenantConfigMock.Object, _tenantAdminsMock.Object, _metricsRepoMock.Object, - previewMock.Object, + _previewMock.Object, _telegramMock.Object, _globalNotificationMock.Object, - mcpUserMock.Object, + _mcpUserMock.Object, _autoApproveEnqueuerMock.Object, new AutopilotMonitor.Functions.Security.EntraAppRegistry( new Microsoft.Extensions.Configuration.ConfigurationBuilder().Build(), @@ -169,6 +171,13 @@ public class AuthFunctionSideEffectTests } private static (HttpRequestData Req, FunctionContext Ctx) BuildAuthenticatedRequest(string callerUpn, string? tenantId = null) + { + var claims = new List { new("preferred_username", callerUpn) }; + if (tenantId != null) claims.Add(new Claim("tid", tenantId)); + return BuildRequest(new ClaimsPrincipal(new ClaimsIdentity(claims, "test"))); + } + + private static (HttpRequestData Req, FunctionContext Ctx) BuildRequest(ClaimsPrincipal principal) { var services = new ServiceCollection(); services.AddOptions(); @@ -177,10 +186,6 @@ public class AuthFunctionSideEffectTests new System.Text.Json.JsonSerializerOptions(System.Text.Json.JsonSerializerDefaults.Web))); var provider = services.BuildServiceProvider(); - var claims = new List { new("preferred_username", callerUpn) }; - if (tenantId != null) claims.Add(new Claim("tid", tenantId)); - var principal = new ClaimsPrincipal(new ClaimsIdentity(claims, "test")); - var context = new Mock(); context.SetupGet(c => c.Items).Returns( new Dictionary { ["ClaimsPrincipal"] = principal }); @@ -846,4 +851,86 @@ public class AuthFunctionSideEffectTests await Assert.ThrowsAsync( () => _sut.HandlePostDecisionSideEffectsAsync(decision, TenantId, Upn, DisplayName, ObjectId)); } + + // ------------------------------------------------------------------------- + // GetCurrentUser end to end: an external identity never claims the tenant + // ------------------------------------------------------------------------- + + /// + /// A validated token as the middleware hands it on: tid, oid and idp only under their mapped names. The + /// member's idp names its own tenant, the guest's another one — the same principal shape otherwise. + /// + private static ClaimsPrincipal SignedIn(bool external) => new(new ClaimsIdentity(new[] + { + new Claim("http://schemas.microsoft.com/identity/claims/tenantid", TenantId), + new Claim("http://schemas.microsoft.com/identity/claims/objectidentifier", ObjectId), + new Claim(ClaimTypes.Upn, Upn), + new Claim("http://schemas.microsoft.com/identity/claims/identityprovider", external + ? "https://sts.windows.net/bbbbbbbb-1111-2222-3333-444444444444/" + : $"https://sts.windows.net/{TenantId}/"), + }, "test")); + + private void ArrangeAuthMe(TenantConfiguration config, bool exists, bool approved) + { + _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()); + _mcpUserMock + .Setup(x => x.IsAllowedAsync(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny?>(), It.IsAny())) + .ReturnsAsync(McpAccessCheckResult.Denied("not allowed")); + _metricsRepoMock.Setup(x => x.GetUserWhatsNewSeenAsync(TenantId, Upn)).ReturnsAsync(new UserWhatsNewSeen()); + } + + private static async Task ReadBodyAsync(HttpResponseData response) + { + response.Body.Position = 0; + return await new StreamReader(response.Body).ReadToEndAsync(); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task GetCurrentUser_FirstSignInOfANewTenant_OnlyAMemberOnboardsIt(bool external) + { + // The pointer read stays healthy on purpose: a throwing read would skip the seed for the guest too, and + // the test would pass for the wrong reason. + ArrangeAuthMe(DefaultConfig(), exists: false, approved: false); + var (req, ctx) = BuildRequest(SignedIn(external)); + + var response = await _sut.GetCurrentUser(req, ctx); + + // Both wait at the activation gate; only the member's sign-in seeded the tenant and queued its activation. + Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); + Assert.Contains("PendingActivation", await ReadBodyAsync(response)); + Assert.Equal(external ? 0 : 1, _store.Writes.Count); + _autoApproveEnqueuerMock.Verify(x => x.EnqueueAsync( + It.IsAny(), + It.IsAny(), It.IsAny()), external ? Times.Never() : Times.Once()); + _opsEventsMock.Verify(x => x.RecordTenantSignupAsync(TenantId, Upn), external ? Times.Never() : Times.Once()); + if (!external) + Assert.Equal(Upn, _store.Row!.OnboardedBy); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task GetCurrentUser_ActivatedTenantWithoutAdmins_OnlyAMemberBecomesItsFirstAdmin(bool external) + { + var config = DefaultConfig(); + config.DomainName = "contoso.com"; + ArrangeAuthMe(config, exists: true, approved: true); + _tenantAdminsMock + .Setup(x => x.AddTenantAdminAsync(TenantId, Upn, "System")) + .ReturnsAsync(new TenantAdminRow()); + 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()), + external ? Times.Never() : Times.Once()); + Assert.Contains(external ? "\"isTenantAdmin\":false" : "\"isTenantAdmin\":true", await ReadBodyAsync(response)); + } } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionTests.cs index 64023730..eb823100 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionTests.cs @@ -414,6 +414,43 @@ public class AuthFunctionTests Assert.Null((string?)body.role); } + [Fact] + public void AutoAdmin_ExternalIdentityFirstUser_IsNeverTheFirstAdmin() + { + // A guest signing in to an activated tenant that has no admins yet must not be made its Tenant Admin: + // it is not a member of the tenant at all. + var result = AuthFunction.BuildAuthResult( + DefaultConfig(), isGlobalAdmin: false, isGlobalReader: false, isPreviewApproved: true, + memberRole: null, mcpCheck: McpDenied(), + hasTenantAdmins: false, + TenantId, Upn, DisplayName, ObjectId, + isExternalIdentity: true); + + 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 ExternalIdentity_KeepsARoleTheTenantGrantedExplicitly() + { + var result = AuthFunction.BuildAuthResult( + DefaultConfig(), isGlobalAdmin: false, isGlobalReader: false, isPreviewApproved: true, + memberRole: AdminRole(), mcpCheck: McpDenied(), + hasTenantAdmins: true, + TenantId, Upn, DisplayName, ObjectId, + isExternalIdentity: true); + + Assert.True(result.IsSuccess); + Assert.False(result.NeedsAutoAdmin); + var body = ToDynamic(result.Body); + Assert.True((bool)body.isTenantAdmin); + Assert.Equal(Constants.TenantRoles.Admin, (string)body.role); + } + // ------------------------------------------------------------------------- // Auto-Admin Logic // ------------------------------------------------------------------------- diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AuthenticationMiddlewareTokenValidationTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AuthenticationMiddlewareTokenValidationTests.cs index 6fada4ea..b7a48ab0 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AuthenticationMiddlewareTokenValidationTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AuthenticationMiddlewareTokenValidationTests.cs @@ -1,6 +1,7 @@ using System.IdentityModel.Tokens.Jwt; using System.Security.Claims; using System.Security.Cryptography; +using AutopilotMonitor.Functions.Extensions; using AutopilotMonitor.Functions.Middleware; using Microsoft.IdentityModel.Tokens; using Xunit; @@ -34,12 +35,13 @@ public class AuthenticationMiddlewareTokenValidationTests string audience = ClientId, string tid = Tid, DateTime? notBefore = null, - DateTime? expires = null) + DateTime? expires = null, + IEnumerable? extraClaims = null) { var token = new JwtSecurityToken( issuer: issuer ?? V2Issuer, audience: audience, - claims: new[] { new Claim("tid", tid) }, + claims: new[] { new Claim("tid", tid) }.Concat(extraClaims ?? Array.Empty()), notBefore: notBefore ?? DateTime.UtcNow.AddMinutes(-5), expires: expires ?? DateTime.UtcNow.AddMinutes(30), signingCredentials: creds); // null creds → alg "none", unsigned @@ -136,6 +138,39 @@ public class AuthenticationMiddlewareTokenValidationTests Assert.ThrowsAny(() => Validate(forged)); } + // ── External identity: the guest signals survive inbound claim mapping ── + // A validated token carries tid and idp only under their mapped names, so a classifier reading the raw + // names would call every member with an idp external, or every guest a member. + + [Theory] + [InlineData("https://sts.windows.net/bbbbbbbb-1111-2222-3333-444444444444/", true)] // a guest of another Entra tenant + [InlineData("live.com", true)] // a personal Microsoft account + [InlineData("https://sts.windows.net/" + Tid + "/", false)] // a member: own tenant, v1 form under the v2 issuer + public void IdpClaim_after_validation_decides_external_identity(string idp, bool external) + { + var principal = Validate(Mint(RsaCreds, extraClaims: new[] { new Claim("idp", idp) })); + + Assert.Equal(external, principal.IsExternalIdentity()); + } + + [Theory] + [InlineData("1", true)] + [InlineData("0", false)] + public void AcctClaim_after_validation_is_read_as_the_number_Entra_sends(string acct, bool external) + { + var principal = Validate(Mint(RsaCreds, extraClaims: new[] { new Claim("acct", acct, ClaimValueTypes.Integer32) })); + + Assert.Equal(external, principal.IsExternalIdentity()); + } + + [Fact] + public void MemberToken_without_guest_signals_is_not_external() + { + var principal = Validate(Mint(RsaCreds, extraClaims: new[] { new Claim("upn", "alice@contoso.example") })); + + Assert.False(principal.IsExternalIdentity()); + } + // ── IsValidTenantId: pre-signature tid GUID gate ───────────────────── [Theory] diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/ExternalIdentityTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/ExternalIdentityTests.cs new file mode 100644 index 00000000..27682986 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/ExternalIdentityTests.cs @@ -0,0 +1,108 @@ +using System.Security.Claims; +using AutopilotMonitor.Functions.Extensions; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// : a caller that is not a member of the token's tenant +/// never claims it (no first-login seed, no first-user Tenant Admin, no activation promotion). Positive evidence +/// only — every case expecting a member pins a false positive that would silently strand a genuine first sign-in. +/// A validated token carries tid and idp only under their inbound-mapped names, so both forms are covered. +/// +public class ExternalIdentityTests +{ + private const string Tid = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; + private const string OtherTenant = "bbbbbbbb-1111-2222-3333-444444444444"; + private const string MappedTenantId = "http://schemas.microsoft.com/identity/claims/tenantid"; + private const string MappedIdp = "http://schemas.microsoft.com/identity/claims/identityprovider"; + + private static ClaimsPrincipal Principal(params Claim[] extra) => WithUpn("alice@contoso.example", extra); + + private static ClaimsPrincipal WithUpn(string upn, params Claim[] extra) + { + var claims = new List { new(MappedTenantId, Tid), new(ClaimTypes.Upn, upn) }; + claims.AddRange(extra); + return new ClaimsPrincipal(new ClaimsIdentity(claims, "TestAuth")); + } + + [Fact] + public void Member_WithoutAnySignal_IsNotExternal() + => Assert.False(Principal().IsExternalIdentity()); + + // ── acct: Entra's guest flag ───────────────────────────────────────────────── + + [Theory] + [InlineData("1", true)] + [InlineData("0", false)] + public void AcctClaim_GuestFlagDecides(string acct, bool external) + => Assert.Equal(external, Principal(new Claim("acct", acct)).IsExternalIdentity()); + + // ── #EXT#: the resource-tenant UPN of an external account ─────────────────── + + [Theory] + [InlineData("upn")] + [InlineData(ClaimTypes.Upn)] + [InlineData("preferred_username")] + [InlineData("unique_name")] + [InlineData(ClaimTypes.Name)] + public void ExternalUpnMarker_InAnyUserNameClaim_IsExternal(string claimType) + { + var principal = new ClaimsPrincipal(new ClaimsIdentity(new[] + { + new Claim(MappedTenantId, Tid), + new Claim(claimType, "alice_contoso.example#ext#@fabrikam.example"), + }, "TestAuth")); + + Assert.True(principal.IsExternalIdentity()); + } + + [Fact] + public void ExternalUpnMarker_InTheDisplayName_IsNoEvidence() + => Assert.False(Principal(new Claim("name", "Alice #EXT# Example")).IsExternalIdentity()); + + [Fact] + public void UnderscoreExtSuffix_IsNoEvidence_MemberNamingSchemesUseIt() + => Assert.False(WithUpn("m.muster_ext_@contoso.example").IsExternalIdentity()); + + // ── idp: where the account authenticated ──────────────────────────────────── + + [Theory] + [InlineData("https://sts.windows.net/" + OtherTenant + "/", true)] // a guest of another Entra tenant + [InlineData("https://login.microsoftonline.com/" + OtherTenant + "/v2.0", true)] + [InlineData("https://sts.windows.net/9188040d-6c67-4c5b-b112-36a304b66dad/", true)] // the personal-account tenant + [InlineData("live.com", true)] // a personal Microsoft account + [InlineData("LIVE.COM", true)] + [InlineData("mail", true)] // email one-time passcode + [InlineData("google.com", true)] + [InlineData("facebook.com", true)] + [InlineData("https://sts.windows.net/" + Tid + "/", false)] // a member: its own tenant (v1 form, also under a v2 issuer) + [InlineData("https://login.microsoftonline.com/" + Tid + "/v2.0", false)] + [InlineData("http://adfs.contoso.example/adfs/services/trust", false)] // an unknown host: no evidence either way + [InlineData("https://auth.idp.example/" + OtherTenant + "/as", false)] // a GUID segment on a foreign host: no evidence + [InlineData("", false)] + public void IdpClaim_NamesWhereTheAccountAuthenticated(string idp, bool external) + => Assert.Equal(external, Principal(new Claim("idp", idp)).IsExternalIdentity()); + + [Theory] + [InlineData("idp")] + [InlineData(MappedIdp)] + public void IdpNamingAnotherTenant_IsExternal_InEitherClaimForm(string idpClaimType) + => Assert.True(Principal(new Claim(idpClaimType, $"https://sts.windows.net/{OtherTenant}/")).IsExternalIdentity()); + + [Theory] + [InlineData("tid")] + [InlineData(MappedTenantId)] + public void IdpNamingTheOwnTenant_IsAMember_InEitherTenantClaimForm(string tenantClaimType) + { + // A validated token has no raw tid: comparing against it would call every member with an idp external. + var principal = new ClaimsPrincipal(new ClaimsIdentity(new[] + { + new Claim(tenantClaimType, Tid), + new Claim(ClaimTypes.Upn, "alice@contoso.example"), + new Claim(MappedIdp, $"https://sts.windows.net/{Tid}/"), + }, "TestAuth")); + + Assert.False(principal.IsExternalIdentity()); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/LogThrottleTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/LogThrottleTests.cs new file mode 100644 index 00000000..e2144c37 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/LogThrottleTests.cs @@ -0,0 +1,53 @@ +using AutopilotMonitor.Functions.Helpers; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// : one line per key and interval, so a polled path (auth/me under the activation page, +/// a delegated token on every request) shows up in App Insights without flooding it; bounded memory. +/// +public class LogThrottleTests +{ + private static readonly DateTime T0 = new(2026, 10, 13, 8, 0, 0, DateTimeKind.Utc); + + [Fact] + public void FirstLine_Passes_ARepeatWithinTheInterval_DoesNot() + { + var throttle = new LogThrottle(TimeSpan.FromHours(1)); + + Assert.True(throttle.ShouldLog("k", T0)); + Assert.False(throttle.ShouldLog("k", T0.AddMinutes(59))); + } + + [Fact] + public void ARepeat_PassesAgain_OnceTheIntervalHasElapsed() + { + var throttle = new LogThrottle(TimeSpan.FromHours(1)); + + Assert.True(throttle.ShouldLog("k", T0)); + Assert.True(throttle.ShouldLog("k", T0.AddHours(1))); + Assert.False(throttle.ShouldLog("k", T0.AddHours(1).AddMinutes(1))); + } + + [Fact] + public void Keys_AreThrottledIndependently() + { + var throttle = new LogThrottle(TimeSpan.FromHours(1)); + + Assert.True(throttle.ShouldLog("a", T0)); + Assert.True(throttle.ShouldLog("b", T0)); + Assert.False(throttle.ShouldLog("a", T0)); + } + + [Fact] + public void AtTheKeyCap_TheMemoryIsDropped_SoALineRepeatsEarlyInsteadOfGrowingWithoutBound() + { + var throttle = new LogThrottle(TimeSpan.FromHours(1), maxKeys: 2); + + Assert.True(throttle.ShouldLog("a", T0)); + Assert.True(throttle.ShouldLog("b", T0)); + Assert.True(throttle.ShouldLog("c", T0)); + Assert.True(throttle.ShouldLog("a", T0)); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/PreviewNotificationEmailWriteGateTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/PreviewNotificationEmailWriteGateTests.cs index dba1d82c..ac504150 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/PreviewNotificationEmailWriteGateTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/PreviewNotificationEmailWriteGateTests.cs @@ -19,6 +19,7 @@ namespace AutopilotMonitor.Functions.Tests; /// farewell mails. The route must admit roleless callers (first-touch signup) but only while the /// tenant has no member yet — afterwards a member role is required, so an ordinary employee with a /// valid JWT and no product role can no longer redirect or suppress the tenant's correspondence. +/// The window is never an external identity's: it does not activate the tenant. /// public class PreviewNotificationEmailWriteGateTests { @@ -43,7 +44,7 @@ public class PreviewNotificationEmailWriteGateTests var admins = AdminsWith(enabledMember: true); var ctx = new RequestContext { UserRole = role }; - Assert.True(await PreviewWhitelistFunction.MayWriteNotificationEmailAsync(ctx, TenantId, admins.Object)); + Assert.True(await PreviewWhitelistFunction.MayWriteNotificationEmailAsync(ctx, externalIdentity: false, TenantId, admins.Object)); admins.Verify(a => a.GetTenantAdminsAsync(It.IsAny()), Times.Never); } @@ -53,7 +54,7 @@ public class PreviewNotificationEmailWriteGateTests var admins = AdminsWith(enabledMember: true); var ctx = new RequestContext { IsGlobalAdmin = true }; - Assert.True(await PreviewWhitelistFunction.MayWriteNotificationEmailAsync(ctx, TenantId, admins.Object)); + Assert.True(await PreviewWhitelistFunction.MayWriteNotificationEmailAsync(ctx, externalIdentity: false, TenantId, admins.Object)); } [Fact] @@ -62,7 +63,7 @@ public class PreviewNotificationEmailWriteGateTests var admins = AdminsWith(enabledMember: false); var ctx = new RequestContext { UserRole = "Authenticated" }; - Assert.True(await PreviewWhitelistFunction.MayWriteNotificationEmailAsync(ctx, TenantId, admins.Object)); + Assert.True(await PreviewWhitelistFunction.MayWriteNotificationEmailAsync(ctx, externalIdentity: false, TenantId, admins.Object)); } [Fact] @@ -71,7 +72,7 @@ public class PreviewNotificationEmailWriteGateTests var admins = AdminsWith(enabledMember: true); var ctx = new RequestContext { UserRole = "Authenticated" }; - Assert.False(await PreviewWhitelistFunction.MayWriteNotificationEmailAsync(ctx, TenantId, admins.Object)); + Assert.False(await PreviewWhitelistFunction.MayWriteNotificationEmailAsync(ctx, externalIdentity: false, TenantId, admins.Object)); } [Fact] @@ -86,7 +87,26 @@ public class PreviewNotificationEmailWriteGateTests }); var ctx = new RequestContext { UserRole = "Authenticated" }; - Assert.True(await PreviewWhitelistFunction.MayWriteNotificationEmailAsync(ctx, TenantId, admins.Object)); + Assert.True(await PreviewWhitelistFunction.MayWriteNotificationEmailAsync(ctx, externalIdentity: false, TenantId, admins.Object)); + } + + [Fact] + public async Task External_identity_never_writes_in_the_signup_window() + { + // The window exists for the activating user, and an external identity never activates a tenant. + var admins = AdminsWith(enabledMember: false); + var ctx = new RequestContext { UserRole = "Authenticated" }; + + Assert.False(await PreviewWhitelistFunction.MayWriteNotificationEmailAsync(ctx, externalIdentity: true, TenantId, admins.Object)); + } + + [Fact] + public async Task External_identity_with_a_role_the_tenant_granted_may_write() + { + var admins = AdminsWith(enabledMember: true); + var ctx = new RequestContext { UserRole = Constants.TenantRoles.Admin }; + + Assert.True(await PreviewWhitelistFunction.MayWriteNotificationEmailAsync(ctx, externalIdentity: true, TenantId, admins.Object)); } private static Mock AdminsWith(bool enabledMember) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/PreviewWhitelistRequesterUpnTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/PreviewWhitelistRequesterUpnTests.cs index 980999a5..6ea76836 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/PreviewWhitelistRequesterUpnTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/PreviewWhitelistRequesterUpnTests.cs @@ -1,37 +1,29 @@ using AutopilotMonitor.Functions.Services; -using AutopilotMonitor.Shared.Models; using Xunit; namespace AutopilotMonitor.Functions.Tests; /// -/// Unit tests for the static helpers TenantApprovalService.PickRequesterUpn -/// and TenantApprovalService.IsRealUserUpn. +/// Unit tests for TenantApprovalService.IsPromotableUpn, the shape guard on the stored onboarding UPN that +/// activation promotes to TenantAdmin. /// -/// These guard the auto-promote-on-preview-approval path, which previously corrupted -/// 10 tenants' TenantAdmins rows by writing the sentinel string -/// "System (Global Rate Limit Sync)" as a TenantAdmin Upn. The fixes: -/// -/// shape-based UPN validation (no enumeration of sentinel strings), -/// prefer the immutable over the -/// mutable . -/// +/// The auto-promote-on-approval path once corrupted 10 tenants' TenantAdmins rows by writing the sentinel string +/// "System (Global Rate Limit Sync)" as a TenantAdmin Upn. The guard is therefore shape-based (no +/// enumeration of sentinel strings), and it also refuses an external account's resource-tenant UPN. Which value is +/// promoted at all (OnboardedBy only, never UpdatedBy) is pinned in . /// /// public class PreviewWhitelistRequesterUpnTests { - // ------------------------------------------------------------------------- - // IsRealUserUpn — positive shape check, immune to new sentinel strings - // ------------------------------------------------------------------------- - [Theory] [InlineData("user@contoso.com")] [InlineData("admin@tenant.onmicrosoft.com")] [InlineData("first.last@example.de")] [InlineData("cadm_user@contoso.onmicrosoft.com")] - public void IsRealUserUpn_AcceptsRealUpnShapes(string upn) + [InlineData("m.muster_ext_@contoso.example")] // a member naming scheme, not the external marker + public void IsPromotableUpn_AcceptsRealUpnShapes(string upn) { - Assert.True(TenantApprovalService.IsRealUserUpn(upn)); + Assert.True(TenantApprovalService.IsPromotableUpn(upn)); } [Theory] @@ -41,12 +33,20 @@ public class PreviewWhitelistRequesterUpnTests [InlineData("System (Global Rate Limit Sync)")] [InlineData("system (global rate limit sync)")] [InlineData("System (whatever-future-sentinel)")] - public void IsRealUserUpn_RejectsAnySystemPrefixSentinel(string upn) + public void IsPromotableUpn_RejectsAnySystemPrefixSentinel(string upn) { // The shape-based prefix check is exactly the defense against the corruption // pattern: any new "System (...)" sentinel introduced anywhere in the codebase // is rejected without anyone needing to remember to update this method. - Assert.False(TenantApprovalService.IsRealUserUpn(upn)); + Assert.False(TenantApprovalService.IsPromotableUpn(upn)); + } + + [Theory] + [InlineData("alice_fabrikam.example#EXT#@contoso.example")] + [InlineData("alice_fabrikam.example#ext#@contoso.example")] + public void IsPromotableUpn_RejectsAnExternalAccountsResourceTenantUpn(string upn) + { + Assert.False(TenantApprovalService.IsPromotableUpn(upn)); } [Theory] @@ -55,80 +55,8 @@ public class PreviewWhitelistRequesterUpnTests [InlineData(" ")] [InlineData("nodomain")] [InlineData("just-a-string")] - public void IsRealUserUpn_RejectsNullOrNonEmailShapes(string? upn) + public void IsPromotableUpn_RejectsNullOrNonEmailShapes(string? upn) { - Assert.False(TenantApprovalService.IsRealUserUpn(upn)); - } - - // ------------------------------------------------------------------------- - // PickRequesterUpn — prefer OnboardedBy, fall back to UpdatedBy - // ------------------------------------------------------------------------- - - [Fact] - public void PickRequesterUpn_PrefersOnboardedByEvenWhenUpdatedByIsClean() - { - var cfg = new TenantConfiguration - { - OnboardedBy = "onboarder@contoso.com", - UpdatedBy = "editor@contoso.com", - }; - - Assert.Equal("onboarder@contoso.com", TenantApprovalService.PickRequesterUpn(cfg)); - } - - [Fact] - public void PickRequesterUpn_FallsBackToUpdatedByWhenOnboardedByNull() - { - var cfg = new TenantConfiguration - { - OnboardedBy = null, - UpdatedBy = "editor@contoso.com", - }; - - Assert.Equal("editor@contoso.com", TenantApprovalService.PickRequesterUpn(cfg)); - } - - [Fact] - public void PickRequesterUpn_FallsBackToUpdatedByWhenOnboardedByEmpty() - { - var cfg = new TenantConfiguration - { - OnboardedBy = "", - UpdatedBy = "editor@contoso.com", - }; - - Assert.Equal("editor@contoso.com", TenantApprovalService.PickRequesterUpn(cfg)); - } - - [Fact] - public void PickRequesterUpn_ShieldsAgainstUpdatedByClobberWhenOnboardedBySet() - { - // Regression: this is the exact scenario that corrupted 10 tenants. - // The global rate-limit sync clobbered UpdatedBy with the sentinel string; - // OnboardedBy preserves the real requester so auto-promote still picks them. - var cfg = new TenantConfiguration - { - OnboardedBy = "real.requester@contoso.com", - UpdatedBy = "System (Global Rate Limit Sync)", - }; - - Assert.Equal("real.requester@contoso.com", TenantApprovalService.PickRequesterUpn(cfg)); - Assert.True(TenantApprovalService.IsRealUserUpn(TenantApprovalService.PickRequesterUpn(cfg))); - } - - [Fact] - public void PickRequesterUpn_LegacyTenant_NoOnboardedBy_SentinelInUpdatedBy_IsRejectedByGuard() - { - // Defense-in-depth: even without OnboardedBy (pre-fix tenants), the IsRealUserUpn - // guard alone prevents the sentinel from leaking into TenantAdmins. - var cfg = new TenantConfiguration - { - OnboardedBy = null, - UpdatedBy = "System (Global Rate Limit Sync)", - }; - - var picked = TenantApprovalService.PickRequesterUpn(cfg); - Assert.Equal("System (Global Rate Limit Sync)", picked); - Assert.False(TenantApprovalService.IsRealUserUpn(picked)); + Assert.False(TenantApprovalService.IsPromotableUpn(upn)); } } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/TenantApprovalServiceTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/TenantApprovalServiceTests.cs index 5a56d9e9..9e6cbd0a 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/TenantApprovalServiceTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/TenantApprovalServiceTests.cs @@ -21,6 +21,7 @@ public class TenantApprovalServiceTests private readonly Mock _previewMock; private readonly Mock _tenantConfigMock; + private readonly Mock _tenantAdminsMock; private readonly Mock _emailMock; private readonly List _opsEvents = new(); private readonly TenantApprovalService _sut; @@ -53,12 +54,15 @@ public class TenantApprovalServiceTests configRepo, NullLogger.Instance, cache); var alertDispatch = TestNotifications.InertOpsAlertDispatch(adminConfig.Object); + // IsTenantAdminAsync is not virtual: it runs for real against the empty repository (no member yet). + _tenantAdminsMock = new Mock( + Mock.Of(), cache, NullLogger.Instance); + _sut = new TenantApprovalService( NullLogger.Instance, _previewMock.Object, _tenantConfigMock.Object, - new Mock( - Mock.Of(), cache, NullLogger.Instance).Object, + _tenantAdminsMock.Object, _emailMock.Object, new OpsEventService(opsRepo.Object, NullLogger.Instance, alertDispatch)); } @@ -95,8 +99,8 @@ public class TenantApprovalServiceTests { _previewMock.Setup(x => x.ApproveAsync(TenantId, ApprovedBy)).ReturnsAsync(true); _previewMock.Setup(x => x.GetNotificationEmailAsync(TenantId)).ReturnsAsync((string?)null); - // Default config has no OnboardedBy/UpdatedBy UPN → auto-promote is skipped by the - // shape guard; no notification email → no mail. Only the return value is under test. + // Default config has no OnboardedBy → nothing to promote; no notification email → no + // mail. Only the return value is under test. _tenantConfigMock.Setup(x => x.GetConfigurationAsync(TenantId)) .ReturnsAsync(TenantConfiguration.CreateDefault(TenantId)); @@ -106,6 +110,55 @@ public class TenantApprovalServiceTests _previewMock.Verify(x => x.GetNotificationEmailAsync(TenantId), Times.Once); } + // --- Auto-promote on activation: only the onboarding member (OnboardedBy) becomes Tenant Admin. + + private void ArrangeNewlyApproved(string? onboardedBy, string updatedBy = "System") + { + var config = ConfigWith(contactEmail: null); + config.OnboardedBy = onboardedBy; + config.UpdatedBy = updatedBy; + _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); + } + + private void VerifyNoPromotion() => + _tenantAdminsMock.Verify( + x => x.AddTenantAdminAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + + [Fact] + public async Task NewlyApproved_PromotesTheOnboardingMember() + { + // A sync job clobbering UpdatedBy with its sentinel once leaked into TenantAdmins; OnboardedBy decides alone. + ArrangeNewlyApproved(onboardedBy: "first.member@contoso.example", updatedBy: "System (Global Rate Limit Sync)"); + + await _sut.ApproveWithSideEffectsAsync(TenantId, ApprovedBy); + + _tenantAdminsMock.Verify( + x => x.AddTenantAdminAsync(TenantId, "first.member@contoso.example", ApprovedBy), Times.Once); + } + + [Fact] + public async Task NewlyApproved_NeverPromotesAnExternalAccountsUpn() + { + ArrangeNewlyApproved(onboardedBy: "alice_fabrikam.example#EXT#@contoso.example"); + + await _sut.ApproveWithSideEffectsAsync(TenantId, ApprovedBy); + + VerifyNoPromotion(); + } + + [Fact] + public async Task NewlyApproved_WithoutOnboardedBy_NeverFallsBackToTheLastWriter() + { + // UpdatedBy is whoever wrote the row last (any signed-in user can, e.g. by accepting the DPA), not the requester. + ArrangeNewlyApproved(onboardedBy: null, updatedBy: "first.member@contoso.example"); + + await _sut.ApproveWithSideEffectsAsync(TenantId, ApprovedBy); + + VerifyNoPromotion(); + } + // --- 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.Tests/TenantAutoApproveHandlerTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/TenantAutoApproveHandlerTests.cs index 17ca45a2..cceb1bbd 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/TenantAutoApproveHandlerTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/TenantAutoApproveHandlerTests.cs @@ -181,6 +181,6 @@ public class TenantAutoApproveHandlerTests { // The sentinel must never pass the promotable-user shape check — that is the // defense against writing a system string into the TenantAdmins table. - Assert.False(TenantApprovalService.IsRealUserUpn(TenantAutoApproveHandler.AutoApprovedBy)); + Assert.False(TenantApprovalService.IsPromotableUpn(TenantAutoApproveHandler.AutoApprovedBy)); } } diff --git a/src/Backend/AutopilotMonitor.Functions/Extensions/ClaimsPrincipalExtensions.cs b/src/Backend/AutopilotMonitor.Functions/Extensions/ClaimsPrincipalExtensions.cs index 1e99e35c..de72672d 100644 --- a/src/Backend/AutopilotMonitor.Functions/Extensions/ClaimsPrincipalExtensions.cs +++ b/src/Backend/AutopilotMonitor.Functions/Extensions/ClaimsPrincipalExtensions.cs @@ -86,6 +86,75 @@ public static class ClaimsPrincipalExtensions public static bool IsCappedPrincipal(this ClaimsPrincipal principal) => principal.IsApplicationPrincipal() || principal.IsClientCapped(); + private const string ExternalUpnMarker = "#EXT#"; + private const string IdentityProviderClaimType = "http://schemas.microsoft.com/identity/claims/identityprovider"; + + // Raw and inbound-mapped names of the claims that carry a user name — never the display name ("name"). + private static readonly string[] UserNameClaimTypes = + { "upn", ClaimTypes.Upn, "preferred_username", "unique_name", ClaimTypes.Name }; + + // idp values that only an external account carries in a workforce tenant: a personal Microsoft account, + // an email one-time passcode, the social sign-in providers. + private static readonly HashSet ExternalOnlyIdentityProviders = + new(StringComparer.OrdinalIgnoreCase) { "live.com", "mail", "google.com", "facebook.com" }; + + // The Entra token service: an idp URI on one of these hosts names the authenticating tenant first. + private static readonly HashSet EntraSignInHosts = new(StringComparer.OrdinalIgnoreCase) + { + new Uri(AutopilotMonitor.Shared.Constants.EntraLoginBaseUrl).Host, "sts.windows.net", "login.windows.net", "login.microsoft.com", + }; + + private static readonly Guid PersonalMicrosoftAccountTenantId = new("9188040d-6c67-4c5b-b112-36a304b66dad"); + + /// + /// True when the token shows that the caller is not a member of the token's tenant (tid): a B2B guest + /// or another external identity. Such a caller keeps what the tenant grants it explicitly but never claims the + /// tenant — no first-login seed, no first-user Tenant Admin, no activation promotion, no write in the roleless + /// signup window. Positive evidence only; a token without any of these signals is a member's: + /// + /// acct = 1, Entra's guest flag (an optional claim of the API app registration); + /// #EXT# in a user-name claim, the resource-tenant UPN Entra gives external accounts; + /// an idp naming a personal Microsoft account, an external sign-in provider, or another tenant on + /// an Entra sign-in host. A member's token carries no idp or one naming its own tenant. An idp on + /// any other host is no evidence: a false positive would silently strand a genuine first sign-in. + /// + /// Inbound claim mapping renames tid and idp, so both are read in either form. + /// + public static bool IsExternalIdentity(this ClaimsPrincipal principal) + { + if (string.Equals(principal.FindFirst("acct")?.Value?.Trim(), "1", StringComparison.Ordinal)) + return true; + + if (UserNameClaimTypes.Any(type => IsExternalUpn(principal.FindFirst(type)?.Value))) + return true; + + var identityProvider = principal.FindFirst("idp")?.Value ?? principal.FindFirst(IdentityProviderClaimType)?.Value; + return IsForeignIdentityProvider(identityProvider, principal.GetTenantId()); + } + + /// True when a user name carries the #EXT# marker of an external account's resource-tenant UPN. + internal static bool IsExternalUpn(string? userName) + => userName != null && userName.Contains(ExternalUpnMarker, StringComparison.OrdinalIgnoreCase); + + /// + /// True when an idp claim value shows that the account authenticated outside . + /// + internal static bool IsForeignIdentityProvider(string? identityProvider, string? tenantId) + { + var idp = identityProvider?.Trim(); + if (string.IsNullOrEmpty(idp)) + return false; + if (ExternalOnlyIdentityProviders.Contains(idp)) + return true; + if (!Uri.TryCreate(idp, UriKind.Absolute, out var uri) || !EntraSignInHosts.Contains(uri.Host)) + return false; + if (!Guid.TryParse(uri.AbsolutePath.Trim('/').Split('/')[0], out var authenticatingTenant)) + return false; + return authenticatingTenant == PersonalMicrosoftAccountTenantId + || !Guid.TryParse(tenantId, out var tokenTenant) + || authenticatingTenant != tokenTenant; + } + /// /// The caller's rate-limit budget class (), from signed claims /// only: appidacr (v1.0) / azpacr (v2.0) say how the CLIENT authenticated — 0 is a diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs index 4d3b5b4c..246dea5d 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs @@ -22,6 +22,8 @@ namespace AutopilotMonitor.Functions.Functions.Infrastructure; /// public class AuthFunction { + private static readonly LogThrottle ExternalIdentityLog = new(TimeSpan.FromHours(1)); + private readonly ILogger _logger; private readonly GlobalAdminService _globalAdminService; private readonly DelegatedAdminService _delegatedAdminService; @@ -162,8 +164,13 @@ public class AuthFunction // excluded, so a read-only MSP login does not phantom-onboard its home tenant (false signup, default // config row). A delegated user who legitimately participates in their own tenant (member/global) // still onboards normally; the handlers self-gate when the config already exists. + // An external identity (a guest: this tid is not its home) never participates, whatever it was + // granted: its home domain would become this tenant's DomainName, which resolves cross-tenant grants, + // and its UPN the OnboardedBy that activation promotes to Tenant Admin. var isDelegated = delegatedTenantIds.Count > 0; - var isHomeTenantParticipant = !isDelegated || isGlobalAdmin || isGlobalReader || memberRole != null; + var isExternalIdentity = principal.IsExternalIdentity(); + var isHomeTenantParticipant = !isExternalIdentity + && (!isDelegated || isGlobalAdmin || isGlobalReader || memberRole != null); // Which app registration minted this login's token (dual app-reg window) — drives the // onboarding homing decision and the per-tenant last-seen provenance below. var tokenAudience = principal.GetAudience(); @@ -176,6 +183,14 @@ public class AuthFunction await HandleAutoReEnableAsync(tenantConfig, tenantId); await HandleAuthClientIdTrackingAsync(tenantConfig, tenantId, tokenAudience); } + else if (isExternalIdentity && string.IsNullOrEmpty(tenantConfig.DomainName) + && ExternalIdentityLog.ShouldLog($"{tenantId}|{objectId}")) + { + // The activation page polls auth/me, so this is throttled per caller. + _logger.LogWarning( + "External identity {Upn} signed in to tenant {TenantId}, which no member has onboarded yet; it does not onboard it", + upn, tenantId); + } // --- Pure decision logic (tested by AuthFunctionTests) --- var decision = BuildAuthResult( @@ -186,7 +201,8 @@ public class AuthFunction homedApp: _appRegistry.ResolveForTenant(tenantConfig).IsLegacy ? "legacy" : "primary", whatsNewSeenPlatformUtc: whatsNewSeen.PlatformUtc, whatsNewSeenAgentUtc: whatsNewSeen.AgentUtc, - mcpClientRegistrationEnabled: (await _adminConfigService.GetConfigurationAsync())?.McpClientRegistrationEnabled ?? false); + mcpClientRegistrationEnabled: (await _adminConfigService.GetConfigurationAsync())?.McpClientRegistrationEnabled ?? false, + isExternalIdentity: isExternalIdentity); if (!decision.IsSuccess) { @@ -723,7 +739,8 @@ public class AuthFunction string homedApp = "primary", DateTime? whatsNewSeenPlatformUtc = null, DateTime? whatsNewSeenAgentUtc = null, - bool mcpClientRegistrationEnabled = false) + bool mcpClientRegistrationEnabled = false, + bool isExternalIdentity = false) { // A delegated ("MSP") admin manages a subset of OTHER tenants. They are explicitly authorized, so — // like a Global Admin / Reader — they bypass the private-preview gate even when their own home tenant @@ -773,9 +790,10 @@ public class AuthFunction // to manage OTHER tenants and bypassed the preview gate; auto-admining their own (possibly // non-customer) home tenant would convert a read-only delegated assignment into write authority over // 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. + // != 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. bool isTenantAdmin = memberRole?.Role == Constants.TenantRoles.Admin; - bool needsAutoAdmin = memberRole == null && !hasTenantAdmins && !isGlobalReader && !isDelegated; + bool needsAutoAdmin = memberRole == null && !hasTenantAdmins && !isGlobalReader && !isDelegated && !isExternalIdentity; if (needsAutoAdmin) { isTenantAdmin = true; diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Rules/PreviewWhitelistFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Rules/PreviewWhitelistFunction.cs index e3994e59..e6f80358 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Rules/PreviewWhitelistFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Rules/PreviewWhitelistFunction.cs @@ -169,16 +169,18 @@ public class PreviewWhitelistFunction var principal = context.GetUser(); var tenantId = principal?.GetTenantId(); - if (string.IsNullOrWhiteSpace(tenantId)) + if (principal == null || string.IsNullOrWhiteSpace(tenantId)) { return await req.BadRequestAsync("Could not determine tenant"); } - if (!await MayWriteNotificationEmailAsync(req.GetRequestContext(), tenantId, _tenantAdminsService)) + var externalIdentity = principal.IsExternalIdentity(); + if (!await MayWriteNotificationEmailAsync(req.GetRequestContext(), externalIdentity, tenantId, _tenantAdminsService)) { _logger.LogWarning( - "Preview notification email write denied for roleless caller {Upn} — tenant {TenantId} already has members", - principal?.GetUserPrincipalName() ?? "unknown", tenantId); + "Preview notification email write denied for roleless caller {Upn} in tenant {TenantId}: {Reason}", + principal.GetUserPrincipalName() ?? "unknown", tenantId, + externalIdentity ? "external identity" : "the tenant already has members"); return await req.ForbiddenAsync("A tenant member role is required to change the notification email"); } @@ -218,15 +220,18 @@ public class PreviewWhitelistFunction /// offboarding farewell, operator contact). A member role (or platform scope) always may write /// it. A roleless caller may write it ONLY while the tenant has no enabled member at all — the /// first-touch signup window, where the activating user is still roleless because the requester - /// is auto-promoted to Admin only on approval. Once any member exists, that window is closed for - /// good: an ordinary employee with a valid JWT but no product role must not be able to redirect - /// or suppress the tenant's correspondence. + /// is auto-promoted to Admin only on approval. The window is never an external identity's: it + /// neither onboards a tenant nor is promoted on activation. Once any member exists, that window is + /// closed for good: an ordinary employee with a valid JWT but no product role must not be able to + /// redirect or suppress the tenant's correspondence. /// internal static async Task MayWriteNotificationEmailAsync( - RequestContext ctx, string tenantId, TenantAdminsService tenantAdminsService) + RequestContext ctx, bool externalIdentity, string tenantId, TenantAdminsService tenantAdminsService) { if (ctx.IsTenantMemberOrGlobalAdmin()) return true; + if (externalIdentity) + return false; var members = await tenantAdminsService.GetTenantAdminsAsync(tenantId); return !members.Any(m => m.IsEnabled); diff --git a/src/Backend/AutopilotMonitor.Functions/Helpers/LogThrottle.cs b/src/Backend/AutopilotMonitor.Functions/Helpers/LogThrottle.cs new file mode 100644 index 00000000..a1f39710 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions/Helpers/LogThrottle.cs @@ -0,0 +1,34 @@ +using System.Collections.Concurrent; + +namespace AutopilotMonitor.Functions.Helpers; + +/// +/// Lets one log line per key through per interval, for request paths that would otherwise repeat it on every +/// call (worker traces reach App Insights at Warning+ only, so a polled or hot path must not flood them). +/// Process-local and bounded: past maxKeys the memory is dropped, which at worst repeats a line early. +/// +public sealed class LogThrottle +{ + private readonly ConcurrentDictionary _lastLogged = new(); + private readonly TimeSpan _interval; + private readonly int _maxKeys; + + public LogThrottle(TimeSpan interval, int maxKeys = 1000) + { + _interval = interval; + _maxKeys = maxKeys; + } + + /// True when the line for may be written now; records that it was. + public bool ShouldLog(string key) => ShouldLog(key, DateTime.UtcNow); + + internal bool ShouldLog(string key, DateTime nowUtc) + { + if (_lastLogged.TryGetValue(key, out var last) && nowUtc - last < _interval) + return false; + if (_lastLogged.Count >= _maxKeys) + _lastLogged.Clear(); + _lastLogged[key] = nowUtc; + return true; + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Middleware/ClientAppBindingMiddleware.cs b/src/Backend/AutopilotMonitor.Functions/Middleware/ClientAppBindingMiddleware.cs index 03286c4b..56561463 100644 --- a/src/Backend/AutopilotMonitor.Functions/Middleware/ClientAppBindingMiddleware.cs +++ b/src/Backend/AutopilotMonitor.Functions/Middleware/ClientAppBindingMiddleware.cs @@ -1,4 +1,3 @@ -using System.Collections.Concurrent; using System.Net; using System.Security.Claims; using AutopilotMonitor.Functions.Extensions; @@ -21,12 +20,9 @@ namespace AutopilotMonitor.Functions.Middleware; /// public class ClientAppBindingMiddleware : IFunctionsWorkerMiddleware { - private static readonly TimeSpan LogInterval = TimeSpan.FromHours(1); - private const int MaxLogKeys = 1000; - // Worker traces reach App Insights at Warning+ only; the request dimensions carry the per-request // measurement, this line makes a new (tenant, client, outcome) visible without flooding. - private static readonly ConcurrentDictionary LastLogged = new(); + private static readonly LogThrottle OutcomeLog = new(TimeSpan.FromHours(1)); private readonly ILogger _logger; private readonly IConfiguration _configuration; @@ -118,13 +114,8 @@ public class ClientAppBindingMiddleware : IFunctionsWorkerMiddleware private void LogOncePerInterval(string? tenantId, string? clientAppId, string outcome, bool enforced) { - var key = $"{tenantId}|{clientAppId}|{outcome}|{enforced}"; - var now = DateTime.UtcNow; - if (LastLogged.TryGetValue(key, out var last) && now - last < LogInterval) + if (!OutcomeLog.ShouldLog($"{tenantId}|{clientAppId}|{outcome}|{enforced}")) return; - if (LastLogged.Count >= MaxLogKeys) - LastLogged.Clear(); - LastLogged[key] = now; _logger.LogWarning( "[ClientAppBinding] Delegated token from client app {ClientAppId} (tid={TenantId}): {Outcome}, enforced={Enforced}", diff --git a/src/Backend/AutopilotMonitor.Functions/Security/EndpointAccessPolicyCatalog.cs b/src/Backend/AutopilotMonitor.Functions/Security/EndpointAccessPolicyCatalog.cs index 3c2ba48a..8d7f9127 100644 --- a/src/Backend/AutopilotMonitor.Functions/Security/EndpointAccessPolicyCatalog.cs +++ b/src/Backend/AutopilotMonitor.Functions/Security/EndpointAccessPolicyCatalog.cs @@ -274,7 +274,8 @@ public static class EndpointAccessPolicyCatalog // AuthenticatedUserWithRole (not plain AuthenticatedUser): the activating user is still // roleless during first-touch signup, so the tier must admit non-members — but the address is // tenant-shared state feeding the welcome/farewell mails, so the function re-gates on the - // resolved role: roleless callers may only write while the tenant has no member yet. + // resolved role: roleless callers may only write while the tenant has no member yet, and never + // as an external identity, which does not activate a tenant. new("PUT", "preview/notification-email", EndpointPolicy.AuthenticatedUserWithRole), new("GET", "feedback/status", EndpointPolicy.AuthenticatedUser), new("POST", "feedback", EndpointPolicy.AuthenticatedUser), diff --git a/src/Backend/AutopilotMonitor.Functions/Services/Activation/TenantAutoApproveHandler.cs b/src/Backend/AutopilotMonitor.Functions/Services/Activation/TenantAutoApproveHandler.cs index fc077492..9ff9cf24 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/Activation/TenantAutoApproveHandler.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/Activation/TenantAutoApproveHandler.cs @@ -16,7 +16,7 @@ namespace AutopilotMonitor.Functions.Services.Activation public class TenantAutoApproveHandler { /// ApprovedBy sentinel. Deliberately without '@' so the shape-based - /// guard can never treat it as a + /// guard can never treat it as a /// promotable user. public const string AutoApprovedBy = "System (auto-approve)"; diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TenantApprovalService.cs b/src/Backend/AutopilotMonitor.Functions/Services/TenantApprovalService.cs index a688ef79..e42983b3 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TenantApprovalService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TenantApprovalService.cs @@ -1,4 +1,4 @@ -using AutopilotMonitor.Shared.Models; +using AutopilotMonitor.Functions.Extensions; using Microsoft.Extensions.Logging; namespace AutopilotMonitor.Functions.Services; @@ -54,22 +54,23 @@ public class TenantApprovalService _logger.LogInformation("Tenant activated: {TenantId} by {ApprovedBy}", tenantId, approvedBy); - // Auto-promote the tenant requester (first user who triggered tenant config creation) - // as TenantAdmin if they are not already one. + // 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. try { var tenantConfig = await _tenantConfigurationService.GetConfigurationAsync(tenantId); - var requesterUpn = PickRequesterUpn(tenantConfig); + 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 (IsRealUserUpn(requesterUpn)) + if (IsPromotableUpn(requesterUpn)) { - // IsRealUserUpn returned true → requesterUpn is non-null, non-empty, contains '@' + // IsPromotableUpn returned true → requesterUpn is non-null, non-empty, contains '@' var validUpn = requesterUpn!; var isAlreadyAdmin = await _tenantAdminsService.IsTenantAdminAsync(tenantId, validUpn); if (!isAlreadyAdmin) @@ -89,8 +90,8 @@ public class TenantApprovalService else { _logger.LogInformation( - "No valid tenant requester UPN found in TenantConfiguration for tenant {TenantId} (OnboardedBy: '{OnboardedBy}', UpdatedBy: '{UpdatedBy}') — skipping auto-promote", - tenantId, tenantConfig.OnboardedBy ?? "", tenantConfig.UpdatedBy ?? ""); + "No promotable onboarding UPN in TenantConfiguration for tenant {TenantId} (OnboardedBy: '{OnboardedBy}') — skipping auto-promote", + tenantId, tenantConfig.OnboardedBy ?? ""); } } @@ -213,32 +214,15 @@ public class TenantApprovalService } /// - /// Picks the UPN of the user to auto-promote as TenantAdmin on activation. - /// Prefers (immutable, set once on first - /// user login) over (mutable, can be - /// clobbered by background jobs such as the global rate-limit sync). Fall back to - /// for tenants onboarded before - /// existed — callers still guard the - /// result with so a sentinel value cannot leak into - /// the TenantAdmins table. + /// True when a stored onboarding UPN may be promoted to TenantAdmin: shaped like a real user + /// principal name (contains '@', no "System" sentinel prefix) and not an external account's + /// resource-tenant UPN (#EXT#), which no member login writes. /// - internal static string? PickRequesterUpn(TenantConfiguration config) - { - if (config == null) return null; - return !string.IsNullOrWhiteSpace(config.OnboardedBy) - ? config.OnboardedBy - : config.UpdatedBy; - } - - /// - /// True when the value looks like a real Azure AD user principal name - /// (contains '@' and does not start with the "System" sentinel prefix - /// used by background jobs that touch ). - /// - internal static bool IsRealUserUpn(string? upn) + internal static bool IsPromotableUpn(string? upn) { if (string.IsNullOrWhiteSpace(upn)) return false; if (upn.StartsWith("System", StringComparison.OrdinalIgnoreCase)) return false; + if (ClaimsPrincipalExtensions.IsExternalUpn(upn)) return false; return upn.Contains('@'); } } diff --git a/src/McpServer/autopilot-monitor-mcp/src/generated/wire-types.generated.ts b/src/McpServer/autopilot-monitor-mcp/src/generated/wire-types.generated.ts index 33bbc4ed..08e5a925 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/generated/wire-types.generated.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/generated/wire-types.generated.ts @@ -4961,13 +4961,13 @@ export interface TenantConfigPatchOutcomeResponse { export interface TenantConfiguration { /** Tenant ID (PartitionKey in Table Storage) */ tenantId: string; - /** Domain name extracted from the first user's UPN Used for display purposes (e.g., contoso.com) */ + /** Domain of the onboarding member's UPN (e.g. contoso.com), seeded once by the first member login and never from an external identity. Tenant identity, not only display: transactional mails render it, and cross-tenant grants resolve a never-signed-in UPN's home tenant through it. */ domainName: string; /** When the configuration was last updated */ lastUpdated: string; /** Updated by (user email or system) */ updatedBy: string; - /** UPN of the user whose first login created this tenant configuration. Set once in HandleNewTenantDomainAsync alongside DomainName and never overwritten. Used by the preview-approval auto-promote path so background jobs that mutate UpdatedBy (e.g. global rate-limit sync) cannot leak a sentinel string into the TenantAdmins table. Null on rows that pre-date the OnboardedBy field — auto-promote falls back to UpdatedBy with a UPN-shape guard. */ + /** UPN of the member whose first login onboarded this tenant. Set once in HandleNewTenantDomainAsync alongside DomainName and never overwritten; an external identity (guest) never sets it. Activation promotes this UPN, and only this one, to Tenant Admin, so background jobs that mutate UpdatedBy (e.g. global rate-limit sync) cannot leak a sentinel or a last writer into the TenantAdmins table. Null on rows that pre-date the field: such a tenant gets no promotion on activation. */ onboardedBy?: string | null; /** Version of the data processing agreement this tenant accepted — CurrentDpaVersion at that moment. Written once: with the onboarding when the first sign-in came through the get-started tick, otherwise when the portal's acceptance dialog is confirmed (DpaAcceptancePending). Never client-writable. Null on tenants onboarded before the field existed. */ dpaVersion?: string | null; diff --git a/src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs b/src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs index 3512e59d..3c874103 100644 --- a/src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs +++ b/src/Shared/AutopilotMonitor.Shared/Models/Config/TenantConfiguration.cs @@ -18,8 +18,9 @@ namespace AutopilotMonitor.Shared.Models public string TenantId { get; set; } = default!; /// - /// Domain name extracted from the first user's UPN - /// Used for display purposes (e.g., contoso.com) + /// Domain of the onboarding member's UPN (e.g. contoso.com), seeded once by the first member + /// login and never from an external identity. Tenant identity, not only display: transactional + /// mails render it, and cross-tenant grants resolve a never-signed-in UPN's home tenant through it. /// public string DomainName { get; set; } = default!; @@ -34,13 +35,13 @@ namespace AutopilotMonitor.Shared.Models public string UpdatedBy { get; set; } = default!; /// - /// UPN of the user whose first login created this tenant configuration. Set once - /// in HandleNewTenantDomainAsync alongside and - /// never overwritten. Used by the preview-approval auto-promote path so background - /// jobs that mutate (e.g. global rate-limit sync) cannot - /// leak a sentinel string into the TenantAdmins table. - /// Null on rows that pre-date the OnboardedBy field — auto-promote falls back to - /// with a UPN-shape guard. + /// UPN of the member whose first login onboarded this tenant. Set once in + /// HandleNewTenantDomainAsync alongside and never + /// overwritten; an external identity (guest) never sets it. Activation promotes this UPN, + /// and only this one, to Tenant Admin, so background jobs that mutate + /// (e.g. global rate-limit sync) cannot leak a sentinel or a last + /// writer into the TenantAdmins table. Null on rows that pre-date the field: such a tenant + /// gets no promotion on activation. /// public string? OnboardedBy { get; set; } diff --git a/src/Web/autopilot-monitor-web/app/settings/components/AdminManagementSection.tsx b/src/Web/autopilot-monitor-web/app/settings/components/AdminManagementSection.tsx index 40e9f971..23fe46bc 100644 --- a/src/Web/autopilot-monitor-web/app/settings/components/AdminManagementSection.tsx +++ b/src/Web/autopilot-monitor-web/app/settings/components/AdminManagementSection.tsx @@ -142,7 +142,7 @@ export default function AdminManagementSection({

Important: Make sure to keep at least one Admin in the list to maintain full access! - The first user to log in was automatically made an admin. + The first member of your organization to log in was automatically made an admin.

diff --git a/src/Web/autopilot-monitor-web/lib/generated/shared-manifests.json b/src/Web/autopilot-monitor-web/lib/generated/shared-manifests.json index 62b6d052..b45b4f21 100644 --- a/src/Web/autopilot-monitor-web/lib/generated/shared-manifests.json +++ b/src/Web/autopilot-monitor-web/lib/generated/shared-manifests.json @@ -22742,7 +22742,7 @@ "kind": "primitive", "name": "string" }, - "doc": "Domain name extracted from the first user's UPN Used for display purposes (e.g., contoso.com)" + "doc": "Domain of the onboarding member's UPN (e.g. contoso.com), seeded once by the first member login and never from an external identity. Tenant identity, not only display: transactional mails render it, and cross-tenant grants resolve a never-signed-in UPN's home tenant through it." }, { "name": "lastUpdated", @@ -22768,7 +22768,7 @@ "nullable": true }, "optional": true, - "doc": "UPN of the user whose first login created this tenant configuration. Set once in HandleNewTenantDomainAsync alongside DomainName and never overwritten. Used by the preview-approval auto-promote path so background jobs that mutate UpdatedBy (e.g. global rate-limit sync) cannot leak a sentinel string into the TenantAdmins table. Null on rows that pre-date the OnboardedBy field — auto-promote falls back to UpdatedBy with a UPN-shape guard." + "doc": "UPN of the member whose first login onboarded this tenant. Set once in HandleNewTenantDomainAsync alongside DomainName and never overwritten; an external identity (guest) never sets it. Activation promotes this UPN, and only this one, to Tenant Admin, so background jobs that mutate UpdatedBy (e.g. global rate-limit sync) cannot leak a sentinel or a last writer into the TenantAdmins table. Null on rows that pre-date the field: such a tenant gets no promotion on activation." }, { "name": "dpaVersion", diff --git a/src/Web/autopilot-monitor-web/lib/generated/wire-types.generated.ts b/src/Web/autopilot-monitor-web/lib/generated/wire-types.generated.ts index 116b931a..f353d32b 100644 --- a/src/Web/autopilot-monitor-web/lib/generated/wire-types.generated.ts +++ b/src/Web/autopilot-monitor-web/lib/generated/wire-types.generated.ts @@ -4961,13 +4961,13 @@ export interface TenantConfigPatchOutcomeResponse { export interface TenantConfiguration { /** Tenant ID (PartitionKey in Table Storage) */ tenantId: string; - /** Domain name extracted from the first user's UPN Used for display purposes (e.g., contoso.com) */ + /** Domain of the onboarding member's UPN (e.g. contoso.com), seeded once by the first member login and never from an external identity. Tenant identity, not only display: transactional mails render it, and cross-tenant grants resolve a never-signed-in UPN's home tenant through it. */ domainName: string; /** When the configuration was last updated */ lastUpdated: string; /** Updated by (user email or system) */ updatedBy: string; - /** UPN of the user whose first login created this tenant configuration. Set once in HandleNewTenantDomainAsync alongside DomainName and never overwritten. Used by the preview-approval auto-promote path so background jobs that mutate UpdatedBy (e.g. global rate-limit sync) cannot leak a sentinel string into the TenantAdmins table. Null on rows that pre-date the OnboardedBy field — auto-promote falls back to UpdatedBy with a UPN-shape guard. */ + /** UPN of the member whose first login onboarded this tenant. Set once in HandleNewTenantDomainAsync alongside DomainName and never overwritten; an external identity (guest) never sets it. Activation promotes this UPN, and only this one, to Tenant Admin, so background jobs that mutate UpdatedBy (e.g. global rate-limit sync) cannot leak a sentinel or a last writer into the TenantAdmins table. Null on rows that pre-date the field: such a tenant gets no promotion on activation. */ onboardedBy?: string | null; /** Version of the data processing agreement this tenant accepted — CurrentDpaVersion at that moment. Written once: with the onboarding when the first sign-in came through the get-started tick, otherwise when the portal's acceptance dialog is confirmed (DpaAcceptancePending). Never client-writable. Null on tenants onboarded before the field existed. */ dpaVersion?: string | null;