From 261d5df5e18de8effdac3535e5a89cd202339e22 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Thu, 25 Jun 2026 21:49:53 +0200 Subject: [PATCH] fix(backend): don't onboard MSP home tenant on delegated-only login [Phase 3a follow-up] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two linked Codex findings created by the Phase-3a preview-gate bypass. (1) HIGH — auto-admin escalation: the first-login auto-admin rule excluded only GlobalReader, not delegated callers. An external MSP user delegated to manage tenant B, logging into a home tenant A with no existing admins, would be silently written as Tenant Admin of A — converting a read-only delegated assignment into write authority over the MSP home tenant. Fix: needsAutoAdmin also excludes delegated callers (!isDelegated). A delegated user who legitimately administers their own tenant has a TenantAdmins row, so this only suppresses the silent grant. Regression test added (delegatedTenantIds non-empty, memberRole null, hasTenantAdmins false → NeedsAutoAdmin false, isTenantAdmin false, role null). (2) side-effect class: /auth/me read the home-tenant config via the auto-creating GetConfigurationAsync and then ran first-login onboarding side-effects (domain / OnboardedBy write, "new tenant signup" Telegram, auto-re-enable). For a delegated-only external MSP login this phantom-onboarded the home tenant. Fix: read via the non-creating TryGetConfigurationAsync, and run the onboarding side-effects only for a genuine home-tenant participant (not delegated-only, OR global, OR an existing member). Genuine first-users still onboard normally. Full Functions suite green (2473). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../AuthFunctionTests.cs | 21 ++++++++++++ .../Functions/Infrastructure/AuthFunction.cs | 32 ++++++++++++++++--- 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionTests.cs index 083de15e..df518c43 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionTests.cs @@ -305,6 +305,27 @@ public class AuthFunctionTests Assert.Equal("TenantSuspended", (string)body.error); } + [Fact] + public void AutoAdmin_DelegatedFirstUser_DoesNotAutoAdminHomeTenant() + { + // A delegated ("MSP") admin logging into a home tenant with NO existing admins must NOT be silently + // made its Tenant Admin — that would convert a read-only delegated assignment (for OTHER tenants) + // into write authority over the home tenant. Regression for the preview-gate bypass opening the + // auto-admin path to delegated callers. + var result = AuthFunction.BuildAuthResult( + DefaultConfig(), isGlobalAdmin: false, isGlobalReader: false, isPreviewApproved: false, + memberRole: null, mcpCheck: McpDenied(), + hasTenantAdmins: false, + TenantId, Upn, DisplayName, ObjectId, + delegatedTenantIds: new[] { "22222222-2222-2222-2222-222222222222" }); + + Assert.True(result.IsSuccess); + Assert.False(result.NeedsAutoAdmin); + var body = ToDynamic(result.Body); + Assert.False((bool)body.isTenantAdmin); + Assert.Null((string?)body.role); + } + // ------------------------------------------------------------------------- // Auto-Admin Logic // ------------------------------------------------------------------------- diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs index ca8d7c8b..4287e9eb 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs @@ -89,7 +89,10 @@ public class AuthFunction // --- Parallel data fetch: all independent queries run concurrently --- // Fail-fast: if any fetch throws, AggregateException propagates → Azure Functions returns 500. // This is intentional — all 6 queries are required for the auth decision. - var tenantConfigTask = _tenantConfigService.GetConfigurationAsync(tenantId); + // Non-creating read: do NOT auto-persist a default config row just because someone authenticated. + // A delegated-only external MSP login must not phantom-onboard its home tenant (see the side-effect + // gate below). A genuine first-user still gets their config created by HandleNewTenantDomainAsync. + var tenantConfigTask = _tenantConfigService.TryGetConfigurationAsync(tenantId); var globalRoleTask = _globalAdminService.GetGlobalRoleAsync(upn); var delegatedScopeTask = _delegatedAdminService.GetScopeAsync(upn); var isApprovedTask = _previewWhitelistService.IsApprovedAsync(tenantId); @@ -100,7 +103,7 @@ public class AuthFunction await Task.WhenAll(tenantConfigTask, globalRoleTask, delegatedScopeTask, isApprovedTask, membershipTask, mcpCheckTask, existingAdminsTask); - var tenantConfig = tenantConfigTask.Result; + var (tenantConfig, _) = tenantConfigTask.Result; var globalRole = globalRoleTask.Result; var isGlobalAdmin = globalRole == Constants.GlobalRoles.GlobalAdmin; var isGlobalReader = globalRole == Constants.GlobalRoles.GlobalReader; @@ -119,8 +122,20 @@ public class AuthFunction tableState, tableRole, principal.GetAppRoles(), tenantConfig.EntraAppRolesEnabled); // --- Side-effects that don't affect the auth decision --- - await HandleNewTenantDomainAsync(tenantConfig, tenantId, upn); - await HandleAutoReEnableAsync(tenantConfig, tenantId); + // Run first-login onboarding side-effects (domain/OnboardedBy write + "new tenant signup" + // notification, auto-re-enable, and the implicit config-row persistence) ONLY for a genuine + // home-tenant participant. A delegated-only external MSP caller — who bypassed the preview gate to + // manage OTHER tenants and has no stake in this home tenant (no membership, no platform role) — is + // 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. + var isDelegated = delegatedTenantIds.Count > 0; + var isHomeTenantParticipant = !isDelegated || isGlobalAdmin || isGlobalReader || memberRole != null; + if (isHomeTenantParticipant) + { + await HandleNewTenantDomainAsync(tenantConfig, tenantId, upn); + await HandleAutoReEnableAsync(tenantConfig, tenantId); + } // --- Pure decision logic (tested by AuthFunctionTests) --- var decision = BuildAuthResult( @@ -396,8 +411,15 @@ public class AuthFunction // it removes nothing, and a GlobalReader who should also administer their tenant can be added // to TenantAdmins explicitly. (A pure GlobalReader stays read-only everywhere via the write // evaluators denying a roleless caller.) + // Auto-admin is NOT an existing right — it SILENTLY grants write on first login. Decline it for any + // identity that reached this point on a cross-tenant ticket rather than as a genuine home-tenant + // first-user: a read-only GlobalReader, AND a delegated ("MSP") admin. A delegated caller logged in + // 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. bool isTenantAdmin = memberRole?.Role == Constants.TenantRoles.Admin; - bool needsAutoAdmin = memberRole == null && !hasTenantAdmins && !isGlobalReader; + bool needsAutoAdmin = memberRole == null && !hasTenantAdmins && !isGlobalReader && !isDelegated; if (needsAutoAdmin) { isTenantAdmin = true;