diff --git a/docs b/docs index a3af579f..bf2371a8 160000 --- a/docs +++ b/docs @@ -1 +1 @@ -Subproject commit a3af579f8dea1425b9cd799ee641142575f0459d +Subproject commit bf2371a89cc80b3605193a644947aadb1b179591 diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AdminIdentityBindingAuthorizationTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AdminIdentityBindingAuthorizationTests.cs index e8cd80c4..0d4a480b 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AdminIdentityBindingAuthorizationTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AdminIdentityBindingAuthorizationTests.cs @@ -79,7 +79,8 @@ public class AdminIdentityBindingAuthorizationTests return new Harness { Middleware = new PolicyEnforcementMiddleware( - NullLogger.Instance, globalAdmin, delegatedAdmin, tenantAdmins, tenantConfig), + NullLogger.Instance, globalAdmin, delegatedAdmin, + new TenantMemberRoleResolver(tenantAdmins, tenantConfig), tenantConfig), Repo = repo, }; } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs index 98d82956..08d77f60 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AuthFunctionSideEffectTests.cs @@ -82,7 +82,8 @@ public class AuthFunctionSideEffectTests var mcpUserMock = new Mock( adminRepo, new StubAdminIdentityBindingService(bound: true), cache, Mock.Of>(), - globalAdminMock.Object, delegatedAdminMock.Object, adminConfigService.Object) + globalAdminMock.Object, delegatedAdminMock.Object, adminConfigService.Object, + new TenantMemberRoleResolver(_tenantAdminsMock.Object, _tenantConfigMock.Object)) { CallBase = false }; _metricsRepoMock = new Mock(); diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/McpQuotaServiceTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/McpQuotaServiceTests.cs index cd797e05..2ea73ae6 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/McpQuotaServiceTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/McpQuotaServiceTests.cs @@ -82,7 +82,7 @@ public class McpQuotaServiceTests cache ??= new MemoryCache(new MemoryCacheOptions()); var mcpUserService = new McpUserService( adminRepo.Object, new StubAdminIdentityBindingService(bound), cache, NullLogger.Instance, - globalAdminService: null!, delegatedAdminService: null!, adminConfigService: null!); + globalAdminService: null!, delegatedAdminService: null!, adminConfigService: null!, memberRoleResolver: null!); var configRepo = new Mock(); configRepo.Setup(r => r.GetAdminConfigurationAsync()) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceAllMembersTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceAllMembersTests.cs new file mode 100644 index 00000000..474bfe48 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceAllMembersTests.cs @@ -0,0 +1,193 @@ +using AutopilotMonitor.Functions.Security; +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared; +using AutopilotMonitor.Shared.DataAccess; +using AutopilotMonitor.Shared.Models; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// McpAccessPolicy.AllMembers means "every effective MEMBER of the token's tenant", never "every +/// authenticated token": an employee who signs in without a role (Progress Portal end-user) or a token from +/// an organization that never onboarded is denied. Membership is decided by the shared +/// (table first, Entra app-role claim when the tenant opted in), the +/// explicit grants (platform role, delegated scope, enabled McpUsers row) keep working, and the McpUsers +/// table doubles as the per-user override list (usage plan, block). +/// +public class McpUserServiceAllMembersTests +{ + private const string Upn = "operator@customer.example"; + private const string HomeTenant = "11111111-1111-1111-1111-111111111111"; + private const string ManagedTenant = "22222222-2222-2222-2222-222222222222"; + private const string Oid = "aaaaaaaa-1111-1111-1111-aaaaaaaaaaaa"; + + private readonly Mock _adminRepo = new(); + private readonly Mock _globalAdmin; + private readonly Mock _delegatedAdmin; + private readonly StubTenantMemberRoleResolver _memberRoles = new(); + private readonly McpUserService _sut; + + public McpUserServiceAllMembersTests() + { + var cache = new MemoryCache(new MemoryCacheOptions()); + var bindings = new StubAdminIdentityBindingService(bound: true); + _globalAdmin = new Mock( + _adminRepo.Object, bindings, cache, NullLogger.Instance) { CallBase = false }; + _globalAdmin.Setup(x => x.GetGlobalRoleAsync(It.IsAny())).ReturnsAsync((string?)null); + _delegatedAdmin = new Mock( + _adminRepo.Object, bindings, new StubTenantEntitlementService(TenantEdition.Pro), + cache, NullLogger.Instance) { CallBase = false }; + _delegatedAdmin.Setup(x => x.GetScopeAsync(It.IsAny())).ReturnsAsync(DelegatedScope.Empty); + var adminConfig = new Mock( + Mock.Of(), NullLogger.Instance, cache) { CallBase = false }; + adminConfig.Setup(x => x.GetConfigurationAsync()) + .ReturnsAsync(new AdminConfiguration { McpAccessPolicy = McpAccessPolicy.AllMembers.ToString() }); + _adminRepo.Setup(x => x.GetMcpUserAsync(It.IsAny())).ReturnsAsync((McpUserEntry?)null); + + _sut = new McpUserService( + _adminRepo.Object, bindings, cache, NullLogger.Instance, + _globalAdmin.Object, _delegatedAdmin.Object, adminConfig.Object, _memberRoles); + } + + private void MemberOf(string tenantId, string role) => + _memberRoles.Verdict = (tid, upn, _) => + string.Equals(tid, tenantId, StringComparison.OrdinalIgnoreCase) && upn == Upn + ? new MemberRoleInfo { Role = role } + : null; + + // ── who is a member ────────────────────────────────────────────────────────── + + [Theory] + [InlineData(Constants.TenantRoles.Admin)] + [InlineData(Constants.TenantRoles.Operator)] + [InlineData(Constants.TenantRoles.Viewer)] + public async Task EffectiveTenantRole_IsAllowed_AsAllMembers(string role) + { + MemberOf(HomeTenant, role); + + var result = await _sut.IsAllowedAsync(Upn, HomeTenant, Oid); + + Assert.True(result.IsAllowed); + Assert.Equal("AllMembers", result.AccessGrant); + Assert.False(result.IsGlobalAdmin); + Assert.Null(result.GlobalRole); + } + + [Fact] + public async Task AuthenticatedWithoutRole_IsDenied() + { + // The Progress Portal end-user: a valid token from an onboarded tenant, no TenantAdmins row, no claim. + var result = await _sut.IsAllowedAsync(Upn, HomeTenant, Oid); + + Assert.False(result.IsAllowed); + Assert.Contains("no role in its organization's tenant", result.Reason); + Assert.Equal(new[] { (HomeTenant, Upn) }, _memberRoles.Lookups); + } + + [Fact] + public async Task MembershipIsResolvedInTheTokenTenant_NotElsewhere() + { + // A role in some OTHER tenant is not a role in the tenant the token was issued for. + MemberOf(ManagedTenant, Constants.TenantRoles.Admin); + + var result = await _sut.IsAllowedAsync(Upn, HomeTenant, Oid); + + Assert.False(result.IsAllowed); + } + + [Fact] + public async Task MissingTenantId_IsDenied_WithoutResolvingMembership() + { + _memberRoles.Verdict = (_, _, _) => new MemberRoleInfo { Role = Constants.TenantRoles.Admin }; + + var result = await _sut.IsAllowedAsync(Upn, homeTenantId: null, objectId: Oid); + + Assert.False(result.IsAllowed); + Assert.Empty(_memberRoles.Lookups); + } + + [Fact] + public async Task AppRoles_ReachTheResolver() + { + // Claim-derived membership is the resolver's job (gated by the tenant's opt-in); the service must + // hand the token's roles through unchanged. + IReadOnlyList? seen = null; + _memberRoles.Verdict = (_, _, roles) => + { + seen = roles; + return roles != null && roles.Contains(Constants.TenantRoles.Operator) + ? new MemberRoleInfo { Role = Constants.TenantRoles.Operator } + : null; + }; + + var result = await _sut.IsAllowedAsync(Upn, HomeTenant, Oid, new[] { Constants.TenantRoles.Operator }); + + Assert.True(result.IsAllowed); + Assert.Equal(new[] { Constants.TenantRoles.Operator }, seen); + } + + // ── explicit grants still hold, in the same order as WhitelistOnly ────────── + + [Fact] + public async Task PlatformRole_WinsOverMembership_AndReportsItsGrant() + { + _globalAdmin.Setup(x => x.GetGlobalRoleAsync(It.IsAny())) + .ReturnsAsync(Constants.GlobalRoles.GlobalAdmin); + + var result = await _sut.IsAllowedAsync(Upn, HomeTenant, Oid); + + Assert.True(result.IsAllowed); + Assert.Equal(Constants.GlobalRoles.GlobalAdmin, result.AccessGrant); + Assert.True(result.IsGlobalAdmin); + Assert.Empty(_memberRoles.Lookups); + } + + [Fact] + public async Task DelegatedAdmin_WithoutHomeMembership_IsAllowed() + { + // An MSP whose own home tenant is not a customer: no home role, but a curated delegated scope. + _delegatedAdmin.Setup(x => x.GetScopeAsync(It.IsAny())) + .ReturnsAsync(new DelegatedScope(new Dictionary + { + [ManagedTenant] = Constants.DelegatedRoles.DelegatedReader, + })); + + var result = await _sut.IsAllowedAsync(Upn, HomeTenant, Oid); + + Assert.True(result.IsAllowed); + Assert.Equal("DelegatedAdmin", result.AccessGrant); + Assert.Equal(new[] { ManagedTenant }, result.DelegatedTenantIds); + Assert.Empty(_memberRoles.Lookups); + } + + [Fact] + public async Task EnabledMcpUsersRow_GrantsANonMember() + { + _adminRepo.Setup(x => x.GetMcpUserAsync(Upn)) + .ReturnsAsync(new McpUserEntry { Upn = Upn, IsEnabled = true, UsagePlan = "pro" }); + + var result = await _sut.IsAllowedAsync(Upn, HomeTenant, Oid); + + Assert.True(result.IsAllowed); + Assert.Equal("McpUser", result.AccessGrant); + Assert.Empty(_memberRoles.Lookups); + } + + [Fact] + public async Task DisabledMcpUsersRow_BlocksAMember() + { + // The per-user override list's "block" lever: a tenant member the operator disabled stays out. + MemberOf(HomeTenant, Constants.TenantRoles.Admin); + _adminRepo.Setup(x => x.GetMcpUserAsync(Upn)) + .ReturnsAsync(new McpUserEntry { Upn = Upn, IsEnabled = false }); + + var result = await _sut.IsAllowedAsync(Upn, HomeTenant, Oid); + + Assert.False(result.IsAllowed); + Assert.Contains("disabled on the MCP whitelist", result.Reason); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceDelegatedTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceDelegatedTests.cs index 272f104f..0c8dedcf 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceDelegatedTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceDelegatedTests.cs @@ -27,6 +27,7 @@ public class McpUserServiceDelegatedTests private readonly Mock _delegatedAdmin; private readonly Mock _adminConfig; private readonly Mock _adminRepo; + private readonly StubTenantMemberRoleResolver _memberRoles = new(); private readonly McpUserService _sut; public McpUserServiceDelegatedTests() @@ -53,7 +54,7 @@ public class McpUserServiceDelegatedTests _sut = new McpUserService( _adminRepo.Object, bindings, cache, NullLogger.Instance, - _globalAdmin.Object, _delegatedAdmin.Object, _adminConfig.Object); + _globalAdmin.Object, _delegatedAdmin.Object, _adminConfig.Object, _memberRoles); } private void SetPolicy(McpAccessPolicy policy) => @@ -187,8 +188,9 @@ public class McpUserServiceDelegatedTests public async Task AllMembers_WithoutMcpUserRow_IsAllowed() { // Control for the hoisted kill-switch: a MISSING row is the normal AllMembers case and must - // still be granted — only an explicit Disabled row denies. + // still be granted to a tenant member — only an explicit Disabled row denies. SetPolicy(McpAccessPolicy.AllMembers); + _memberRoles.Verdict = (_, _, _) => new MemberRoleInfo { Role = Constants.TenantRoles.Operator }; var result = await _sut.IsAllowedAsync(DelegatedUpn, TenantA, Oid); diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceIdentityBindingTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceIdentityBindingTests.cs index 8860e15a..7e553c1b 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceIdentityBindingTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceIdentityBindingTests.cs @@ -28,6 +28,9 @@ public class McpUserServiceIdentityBindingTests private readonly Mock _adminRepo = new(); private readonly StubAdminIdentityBindingService _bindings; + // Home-tenant membership for the AllMembers path: this suite is about the identity binding, so every + // caller counts as a tenant member and only the binding decides (see McpUserServiceAllMembersTests). + private readonly StubTenantMemberRoleResolver _memberRoles = StubTenantMemberRoleResolver.Everyone(); private readonly McpUserService _sut; public McpUserServiceIdentityBindingTests() @@ -56,7 +59,7 @@ public class McpUserServiceIdentityBindingTests _sut = new McpUserService( _adminRepo.Object, _bindings, cache, NullLogger.Instance, - globalAdmin.Object, delegatedAdmin.Object, adminConfig.Object); + globalAdmin.Object, delegatedAdmin.Object, adminConfig.Object, _memberRoles); } private void SetRow(bool enabled, string? plan = "pro") => @@ -128,7 +131,7 @@ public class McpUserServiceIdentityBindingTests adminConfig.Setup(x => x.GetConfigurationAsync()) .ReturnsAsync(new AdminConfiguration { McpAccessPolicy = McpAccessPolicy.WhitelistOnly.ToString() }); var sut = new McpUserService(_adminRepo.Object, bindings, cache, NullLogger.Instance, - globalAdmin.Object, delegatedAdmin.Object, adminConfig.Object); + globalAdmin.Object, delegatedAdmin.Object, adminConfig.Object, _memberRoles); _adminRepo.Setup(x => x.GetMcpUserAsync(It.IsAny())).ReturnsAsync((McpUserEntry?)null); var result = await sut.IsAllowedAsync(Upn, HomeTenant, Oid); @@ -168,7 +171,7 @@ public class McpUserServiceIdentityBindingTests adminConfig.Setup(x => x.GetConfigurationAsync()) .ReturnsAsync(new AdminConfiguration { McpAccessPolicy = McpAccessPolicy.AllMembers.ToString() }); var sut = new McpUserService(_adminRepo.Object, _bindings, cache, NullLogger.Instance, - globalAdmin.Object, delegatedAdmin.Object, adminConfig.Object); + globalAdmin.Object, delegatedAdmin.Object, adminConfig.Object, _memberRoles); SetRow(enabled: false); var bound = await sut.IsAllowedAsync(Upn, HomeTenant, Oid); diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs index 3bec7a5a..83dce944 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs @@ -118,7 +118,8 @@ public class PolicyEnforcementMiddlewareTests configRepo.Object, NullLogger.Instance, cache); var mw = new PolicyEnforcementMiddleware( - NullLogger.Instance, globalAdmin, delegatedAdmin, tenantAdmins, tenantConfig); + NullLogger.Instance, globalAdmin, delegatedAdmin, + new TenantMemberRoleResolver(tenantAdmins, tenantConfig), tenantConfig); return new Harness { Middleware = mw, Repo = repo, ConfigRepo = configRepo }; } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/StubTenantMemberRoleResolver.cs b/src/Backend/AutopilotMonitor.Functions.Tests/StubTenantMemberRoleResolver.cs new file mode 100644 index 00000000..832aacc4 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/StubTenantMemberRoleResolver.cs @@ -0,0 +1,35 @@ +using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using AutopilotMonitor.Functions.Security; +using AutopilotMonitor.Functions.Services; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Scripted : the verdict function receives (tenantId, upn, appRoles) +/// and answers with the effective member role (null = non-member). Default verdict = nobody is a member. +/// +internal sealed class StubTenantMemberRoleResolver : TenantMemberRoleResolver +{ + /// The scripted verdict; replace it mid-test to change who is a member. + public Func?, MemberRoleInfo?> Verdict { get; set; } + + /// Every (tenantId, upn) the resolver was asked about, in call order. + public List<(string TenantId, string Upn)> Lookups { get; } = new(); + + public StubTenantMemberRoleResolver(Func?, MemberRoleInfo?>? verdict = null) + : base(null!, null!) + { + Verdict = verdict ?? ((_, _, _) => null); + } + + public static StubTenantMemberRoleResolver Everyone(string role = AutopilotMonitor.Shared.Constants.TenantRoles.Admin) + => new((_, _, _) => new MemberRoleInfo { Role = role }); + + public override Task ResolveAsync(string tenantId, string upn, IReadOnlyList? appRoles) + { + Lookups.Add((tenantId, upn)); + return Task.FromResult(Verdict(tenantId, upn, appRoles)); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/TenantMemberRoleResolverTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/TenantMemberRoleResolverTests.cs new file mode 100644 index 00000000..9d75bac4 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/TenantMemberRoleResolverTests.cs @@ -0,0 +1,90 @@ +using AutopilotMonitor.Functions.Security; +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared; +using AutopilotMonitor.Shared.DataAccess; +using AutopilotMonitor.Shared.Models; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// 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. +/// +public class TenantMemberRoleResolverTests +{ + private const string Tenant = "11111111-1111-1111-1111-111111111111"; + private const string Upn = "user@customer.example"; + + private readonly Mock _adminRepo = new(); + private readonly Mock _configRepo = new(); + private readonly TenantMemberRoleResolver _sut; + + public TenantMemberRoleResolverTests() + { + var cache = new MemoryCache(new MemoryCacheOptions()); + _adminRepo.Setup(r => r.GetTenantMemberAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync((TenantMember?)null); + _configRepo.Setup(r => r.GetTenantConfigurationAsync(It.IsAny())) + .ReturnsAsync((TenantConfiguration?)null); + _sut = new TenantMemberRoleResolver( + new TenantAdminsService(_adminRepo.Object, cache, NullLogger.Instance), + new TenantConfigurationService(_configRepo.Object, NullLogger.Instance, cache)); + } + + private void Row(string role, bool enabled = true) => + _adminRepo.Setup(r => r.GetTenantMemberAsync(Tenant, Upn)) + .ReturnsAsync(new TenantMember { TenantId = Tenant, Upn = Upn, Role = role, IsEnabled = enabled }); + + private void AppRolesEnabled() => + _configRepo.Setup(r => r.GetTenantConfigurationAsync(Tenant)) + .ReturnsAsync(new TenantConfiguration { TenantId = Tenant, EntraAppRolesEnabled = true }); + + [Fact] + public async Task EnabledRow_WinsOverClaim_WithoutReadingConfig() + { + Row(Constants.TenantRoles.Viewer); + AppRolesEnabled(); + + var role = await _sut.ResolveAsync(Tenant, Upn, new[] { Constants.TenantRoles.Admin }); + + Assert.Equal(Constants.TenantRoles.Viewer, role?.Role); + _configRepo.Verify(r => r.GetTenantConfigurationAsync(It.IsAny()), Times.Never); + } + + [Fact] + public async Task DisabledRow_SuppressesTheClaim() + { + Row(Constants.TenantRoles.Admin, enabled: false); + AppRolesEnabled(); + + var role = await _sut.ResolveAsync(Tenant, Upn, new[] { Constants.TenantRoles.Admin }); + + Assert.Null(role); + } + + [Fact] + public async Task NoRow_NoClaim_IsNonMember_WithoutReadingConfig() + { + Assert.Null(await _sut.ResolveAsync(Tenant, Upn, null)); + Assert.Null(await _sut.ResolveAsync(Tenant, Upn, Array.Empty())); + _configRepo.Verify(r => r.GetTenantConfigurationAsync(It.IsAny()), Times.Never); + } + + [Fact] + public async Task NoRow_Claim_CountsOnlyWhenTheTenantOptedIn() + { + var claim = new[] { Constants.TenantRoles.Operator }; + + Assert.Null(await _sut.ResolveAsync(Tenant, Upn, claim)); + + AppRolesEnabled(); + var role = await _sut.ResolveAsync(Tenant, Upn, claim); + + Assert.Equal(Constants.TenantRoles.Operator, role?.Role); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs index 2ed5c6f3..898dd563 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/AuthFunction.cs @@ -114,7 +114,9 @@ public class AuthFunction var delegatedScopeTask = _delegatedAdminService.GetScopeAsync(identity); var isApprovedTask = _previewWhitelistService.IsApprovedAsync(tenantId); var membershipTask = _tenantAdminsService.GetTableMembershipAsync(tenantId, upn); - var mcpCheckTask = _mcpUserService.IsAllowedAsync(upn, tenantId, objectId); + // 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); await Task.WhenAll(tenantConfigTask, globalRoleTask, delegatedScopeTask, isApprovedTask, diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/McpUserFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/McpUserFunction.cs index ff839546..e8a72d10 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/McpUserFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/McpUserFunction.cs @@ -200,7 +200,9 @@ public class McpUserFunction // JWT tid + oid complete the caller's identity: platform-role and delegated (MSP) grants resolve on // the identity binding (never the UPN alone); the delegated seat additionally requires a Pro home tenant. - var result = await _mcpUserService.IsAllowedAsync(upn, principal?.GetTenantId(), principal?.GetObjectId()); + // The token's app roles feed the AllMembers member check (claim-derived Admin/Operator when the tenant opted in). + var result = await _mcpUserService.IsAllowedAsync( + upn, principal?.GetTenantId(), principal?.GetObjectId(), principal?.GetAppRoles()); var response = req.CreateResponse(result.IsAllowed ? HttpStatusCode.OK : HttpStatusCode.Forbidden); // Only surface platform-role flags when the caller actually has one (null ⇒ key omitted). diff --git a/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs b/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs index 2578d0eb..12c3d602 100644 --- a/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs +++ b/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs @@ -36,20 +36,20 @@ public class PolicyEnforcementMiddleware : IFunctionsWorkerMiddleware private readonly ILogger _logger; private readonly GlobalAdminService _globalAdminService; private readonly DelegatedAdminService _delegatedAdminService; - private readonly TenantAdminsService _tenantAdminsService; + private readonly TenantMemberRoleResolver _memberRoleResolver; private readonly TenantConfigurationService _tenantConfigService; public PolicyEnforcementMiddleware( ILogger logger, GlobalAdminService globalAdminService, DelegatedAdminService delegatedAdminService, - TenantAdminsService tenantAdminsService, + TenantMemberRoleResolver memberRoleResolver, TenantConfigurationService tenantConfigService) { _logger = logger; _globalAdminService = globalAdminService; _delegatedAdminService = delegatedAdminService; - _tenantAdminsService = tenantAdminsService; + _memberRoleResolver = memberRoleResolver; _tenantConfigService = tenantConfigService; } @@ -561,33 +561,13 @@ public class PolicyEnforcementMiddleware : IFunctionsWorkerMiddleware } /// - /// Resolves the effective tenant member role: the TenantAdmins table entry if present, - /// otherwise — when the tenant has Entra app-roles enabled — the role derived from the token's - /// "roles" claim. Table always wins (manual override). The tenant-config lookup is only - /// performed when there is no table entry, so the common (table member) path stays cheap. + /// Effective tenant member role (table first, Entra app-role claim as the gated fallback) — see + /// , which the MCP access check shares so "member" means the same + /// thing on every path. /// - private async Task ResolveEffectiveRoleAsync( + private Task ResolveEffectiveRoleAsync( string tenantId, string upn, ClaimsPrincipal? principal) - { - var (state, tableRole) = await _tenantAdminsService.GetTableMembershipAsync(tenantId, upn); - - // Table-first: an enabled row wins, a disabled row is an explicit deny. Both skip the - // claim path entirely — and avoid the tenant-config lookup. Only when no row exists do - // we consult the Entra app-role claim (gated by the per-tenant opt-in flag). - if (state != TableMemberState.NotPresent) - return EntraAppRoleResolver.Resolve(state, tableRole, appRoles: null, appRolesEnabled: false); - - if (principal == null) - return null; - - // Side-effect-free read: TryGetConfigurationAsync does NOT persist a default row for a missing - // tenant (GetConfigurationAsync would). Authorization role resolution must never create config as - // a side effect — otherwise an external delegated/MSP user whose own home tenant is not onboarded - // would get a phantom TenantConfiguration row written on their first cross-tenant read. A missing - // config simply means EntraAppRolesEnabled = false (the default), so the role result is unchanged. - var (config, _) = await _tenantConfigService.TryGetConfigurationAsync(tenantId); - return EntraAppRoleResolver.Resolve(state, tableRole, principal.GetAppRoles(), config.EntraAppRolesEnabled); - } + => _memberRoleResolver.ResolveAsync(tenantId, upn, principal?.GetAppRoles()); /// /// Platform-wide cross-tenant READ: admits GlobalAdmin and the read-only GlobalReader. The diff --git a/src/Backend/AutopilotMonitor.Functions/Program.cs b/src/Backend/AutopilotMonitor.Functions/Program.cs index 8f024fab..218bffcb 100644 --- a/src/Backend/AutopilotMonitor.Functions/Program.cs +++ b/src/Backend/AutopilotMonitor.Functions/Program.cs @@ -259,6 +259,7 @@ builder.Services.AddSingleton(); // Global Admin approve endpoint and the tenant auto-approve queue worker. builder.Services.AddSingleton(); builder.Services.AddSingleton(); +builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); diff --git a/src/Backend/AutopilotMonitor.Functions/Security/TenantMemberRoleResolver.cs b/src/Backend/AutopilotMonitor.Functions/Security/TenantMemberRoleResolver.cs new file mode 100644 index 00000000..94d2043a --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions/Security/TenantMemberRoleResolver.cs @@ -0,0 +1,52 @@ +using System.Collections.Generic; +using System.Threading.Tasks; +using AutopilotMonitor.Functions.Services; + +namespace AutopilotMonitor.Functions.Security; + +/// +/// The ONE resolver for a caller's effective tenant member role: the TenantAdmins table entry if present, +/// otherwise — when the tenant has Entra app-roles enabled — the role derived from the token's "roles" +/// claim. Table always wins (manual override): an enabled row supplies the role, a disabled row is an +/// explicit deny that suppresses the claim. Shared by the policy-enforcement middleware (per-request +/// authorization) and the MCP access check (AllMembers = "every tenant member", not "every token"), so the +/// two can never disagree about who is a member. +/// +public class TenantMemberRoleResolver +{ + private readonly TenantAdminsService _tenantAdminsService; + private readonly TenantConfigurationService _tenantConfigService; + + public TenantMemberRoleResolver(TenantAdminsService tenantAdminsService, TenantConfigurationService tenantConfigService) + { + _tenantAdminsService = tenantAdminsService; + _tenantConfigService = tenantConfigService; + } + + /// + /// Effective member role of in , or null for a + /// non-member (no row and no usable claim) and for a disabled row. are the + /// token's raw "roles" claim values (null/empty when the token carries none). + /// + public virtual async Task ResolveAsync(string tenantId, string upn, IReadOnlyList? appRoles) + { + var (state, tableRole) = await _tenantAdminsService.GetTableMembershipAsync(tenantId, upn); + + // Table-first: an enabled row wins, a disabled row is an explicit deny. Both skip the claim path + // entirely — and avoid the tenant-config lookup. Only when no row exists do we consult the Entra + // app-role claim (gated by the per-tenant opt-in flag); a token without roles needs no lookup at all. + if (state != TableMemberState.NotPresent) + return EntraAppRoleResolver.Resolve(state, tableRole, appRoles: null, appRolesEnabled: false); + + if (appRoles == null || appRoles.Count == 0) + return null; + + // Side-effect-free read: TryGetConfigurationAsync does NOT persist a default row for a missing + // tenant (GetConfigurationAsync would). Authorization role resolution must never create config as + // a side effect — otherwise an external delegated/MSP user whose own home tenant is not onboarded + // would get a phantom TenantConfiguration row written on their first cross-tenant read. A missing + // config simply means EntraAppRolesEnabled = false (the default), so the role result is unchanged. + var (config, _) = await _tenantConfigService.TryGetConfigurationAsync(tenantId); + return EntraAppRoleResolver.Resolve(state, tableRole, appRoles, config.EntraAppRolesEnabled); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Services/McpUserService.cs b/src/Backend/AutopilotMonitor.Functions/Services/McpUserService.cs index 1b43bb30..889b4647 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/McpUserService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/McpUserService.cs @@ -12,10 +12,14 @@ namespace AutopilotMonitor.Functions.Services; /// Service for managing MCP (Model Context Protocol) user access. /// Access is governed by AdminConfiguration.McpAccessPolicy: /// - Disabled: no one can access MCP -/// - WhitelistOnly: Global Admins + explicit McpUsers table entries -/// - AllMembers: any authenticated user +/// - WhitelistOnly: platform roles, delegated (MSP) admins and enabled McpUsers rows +/// - AllMembers: the WhitelistOnly grants PLUS every effective member (Admin / Operator / Viewer — table +/// row or Entra app-role claim) of the tenant the token was issued for. NOT "any authenticated token": +/// an employee who merely signs in (e.g. for the Progress Portal) holds no role and is denied, exactly +/// as they are on every tenant endpoint the MCP tools would call. /// An EXPLICITLY DISABLED McpUsers row is an operator kill-switch that denies access under every -/// policy and every grant path (platform role, delegated auto-grant, AllMembers). +/// policy and every grant path (platform role, delegated auto-grant, AllMembers). Under AllMembers the +/// McpUsers table is therefore a per-user override list (individual usage plan, block), not a whitelist. /// /// The McpUsers row is looked up by UPN but is honoured (grant, usage-plan override AND kill-switch) only /// for the identity the UPN is bound to in — the caller's validated @@ -33,6 +37,7 @@ public class McpUserService private readonly GlobalAdminService _globalAdminService; private readonly DelegatedAdminService _delegatedAdminService; private readonly AdminConfigurationService _adminConfigService; + private readonly TenantMemberRoleResolver _memberRoleResolver; // Per-process cache for the McpUsers whitelist lookup. On scaled-out Flex Consumption an add/remove // of an McpUsers row only invalidates the mutating instance, so other instances serve a stale // allow/deny until expiry. A short TTL caps that cross-instance window so an MCP-access grant/revoke @@ -47,7 +52,8 @@ public class McpUserService ILogger logger, GlobalAdminService globalAdminService, DelegatedAdminService delegatedAdminService, - AdminConfigurationService adminConfigService) + AdminConfigurationService adminConfigService, + TenantMemberRoleResolver memberRoleResolver) { _adminRepo = adminRepo; _bindings = bindings; @@ -56,6 +62,7 @@ public class McpUserService _globalAdminService = globalAdminService; _delegatedAdminService = delegatedAdminService; _adminConfigService = adminConfigService; + _memberRoleResolver = memberRoleResolver; } /// @@ -64,10 +71,13 @@ public class McpUserService /// together with the UPN they form the the platform-role and delegated /// (MSP) grant paths AND the McpUsers whitelist resolve on — all three are keyed on the identity binding, /// never on the UPN alone, and the delegated path additionally requires a Pro home tenant. Null tid/oid - /// fails closed to no platform role, an empty delegated scope and no whitelist row; only the AllMembers - /// path (tenant-bound by the auth middleware itself) is unaffected. + /// fails closed to no platform role, an empty delegated scope and no whitelist row. The AllMembers path + /// resolves the caller's effective role in (the tenant the token was + /// issued for) from the TenantAdmins table and, when that tenant opted in, the token's + /// claim — a null tenant id is no member of anything. /// - public virtual async Task IsAllowedAsync(string? upn, string? homeTenantId, string? objectId) + public virtual async Task IsAllowedAsync( + string? upn, string? homeTenantId, string? objectId, IReadOnlyList? appRoles = null) { if (string.IsNullOrWhiteSpace(upn)) return McpAccessCheckResult.Denied("Missing UPN"); @@ -109,41 +119,49 @@ public class McpUserService if (!Enum.TryParse(config.McpAccessPolicy, out var policy)) policy = McpAccessPolicy.WhitelistOnly; - switch (policy) - { - case McpAccessPolicy.Disabled: - // Disabled blocks everyone — platform roles and delegated admins alike. - return McpAccessCheckResult.Denied("MCP access is disabled"); + // Disabled blocks everyone — platform roles and delegated admins alike. + if (policy == McpAccessPolicy.Disabled) + return McpAccessCheckResult.Denied("MCP access is disabled"); - case McpAccessPolicy.AllMembers: - return McpAccessCheckResult.Allowed( - upn, "AllMembers", isGlobalAdmin, globalRole, delegatedTenantIds, delegatedRole); + // The explicit grants below hold under BOTH remaining policies; AllMembers only ADDS the tenant-role + // path at the end. Same order everywhere so the reported AccessGrant is policy-independent. - case McpAccessPolicy.WhitelistOnly: - default: - // Any platform role (GlobalAdmin or read-only GlobalReader) always has MCP access - // (unless explicitly disabled via the kill-switch above). - if (globalRole != null) - return McpAccessCheckResult.Allowed( - upn, globalRole, isGlobalAdmin, globalRole, delegatedTenantIds, delegatedRole); + // Any platform role (GlobalAdmin or read-only GlobalReader) always has MCP access + // (unless explicitly disabled via the kill-switch above). + if (globalRole != null) + return McpAccessCheckResult.Allowed( + upn, globalRole, isGlobalAdmin, globalRole, delegatedTenantIds, delegatedRole); - // A delegated (MSP) admin with an active scope is granted MCP access automatically under - // WhitelistOnly — "delegated = scoped global", and they are already curated via the - // Delegated Admins UI, so a separate enabled McpUsers row would be redundant friction. Their - // reach is bounded client- and server-side to the managed tenant set; no platform/aggregate - // path. (An explicit Disabled row above still revokes this.) - if (!delegatedScope.IsEmpty) - return McpAccessCheckResult.Allowed( - upn, "DelegatedAdmin", isGlobalAdmin: false, globalRole: null, - delegatedTenantIds: delegatedTenantIds, delegatedRole: delegatedRole); + // A delegated (MSP) admin with an active scope is granted MCP access automatically — "delegated = + // scoped global", and they are already curated via the Delegated Admins UI, so a separate enabled + // McpUsers row would be redundant friction. Their reach is bounded client- and server-side to the + // managed tenant set; no platform/aggregate path. (An explicit Disabled row above still revokes this.) + // Under AllMembers this also covers an MSP whose own home tenant is not an onboarded customer. + if (!delegatedScope.IsEmpty) + return McpAccessCheckResult.Allowed( + upn, "DelegatedAdmin", isGlobalAdmin: false, globalRole: null, + delegatedTenantIds: delegatedTenantIds, delegatedRole: delegatedRole); - return whitelistState == McpWhitelistState.Enabled - ? McpAccessCheckResult.Allowed(upn, "McpUser", false, null, delegatedTenantIds, delegatedRole) - // Surfaced to the end user by the MCP server's access guard. Keep it - // self-explanatory so a denied colleague understands they simply need - // to be whitelisted, rather than reading it as an auth failure. - : McpAccessCheckResult.Denied("User not enabled for MCP usage (account is not on the MCP whitelist)"); - } + // An enabled McpUsers row is an explicit per-account grant under both policies. + if (whitelistState == McpWhitelistState.Enabled) + return McpAccessCheckResult.Allowed(upn, "McpUser", false, null, delegatedTenantIds, delegatedRole); + + if (policy != McpAccessPolicy.AllMembers) + // Surfaced to the end user by the MCP server's access guard. Keep it + // self-explanatory so a denied colleague understands they simply need + // to be whitelisted, rather than reading it as an auth failure. + return McpAccessCheckResult.Denied("User not enabled for MCP usage (account is not on the MCP whitelist)"); + + // AllMembers: "member" = an effective role (Admin / Operator / Viewer) in the tenant the token was + // issued for — the same resolver the policy middleware uses on every tenant endpoint. A token from + // an organization that never onboarded, or from an employee without a role (Progress Portal + // end-users), resolves to no role and is denied here instead of failing on every tool call. + var memberRole = string.IsNullOrWhiteSpace(homeTenantId) + ? null + : await _memberRoleResolver.ResolveAsync(homeTenantId, upn, appRoles); + return memberRole != null + ? McpAccessCheckResult.Allowed(upn, "AllMembers", false, null, delegatedTenantIds, delegatedRole) + : McpAccessCheckResult.Denied("User not enabled for MCP usage (account has no role in its organization's tenant — ask a Tenant Admin to add you)"); } /// diff --git a/src/Shared/AutopilotMonitor.Shared/Models/Config/McpAccessPolicy.cs b/src/Shared/AutopilotMonitor.Shared/Models/Config/McpAccessPolicy.cs index 71ee02c9..10712d65 100644 --- a/src/Shared/AutopilotMonitor.Shared/Models/Config/McpAccessPolicy.cs +++ b/src/Shared/AutopilotMonitor.Shared/Models/Config/McpAccessPolicy.cs @@ -9,10 +9,15 @@ namespace AutopilotMonitor.Shared.Models /// MCP access completely disabled. Disabled = 0, - /// Only Global Admins and explicitly whitelisted MCP users. + /// Platform roles, delegated (MSP) admins and explicitly whitelisted MCP users. WhitelistOnly = 1, - /// Any authenticated user with a valid tenant membership. + /// + /// The WhitelistOnly grants plus every effective tenant member (Admin / Operator / Viewer) of the + /// tenant the token was issued for. Not "any authenticated user": a sign-in without a role (e.g. a + /// Progress Portal end-user) is denied. The McpUsers table then acts as a per-user override list + /// (individual usage plan, block). + /// AllMembers = 2 } } diff --git a/src/Web/autopilot-monitor-web/app/settings/components/McpUsersSection.tsx b/src/Web/autopilot-monitor-web/app/settings/components/McpUsersSection.tsx index 62745684..edccdabd 100644 --- a/src/Web/autopilot-monitor-web/app/settings/components/McpUsersSection.tsx +++ b/src/Web/autopilot-monitor-web/app/settings/components/McpUsersSection.tsx @@ -33,8 +33,16 @@ const POLICY_LABELS: Record = { const POLICY_DESCRIPTIONS: Record = { Disabled: "MCP access is completely disabled. No one can connect.", - WhitelistOnly: "Only Global Admins and explicitly added MCP users can connect.", - AllMembers: "Any authenticated user can connect to the MCP server.", + WhitelistOnly: "Only Global Admins, delegated admins and explicitly added MCP users can connect.", + AllMembers: + "Every member of an onboarded tenant (Admin, Operator or Viewer) can connect. Signed-in users without a role cannot.", +}; + +// What the list below means under each policy that shows it. +const LIST_PURPOSE: Record, string> = { + WhitelistOnly: "Global Admins always have access, no separate entry needed.", + AllMembers: + "The list below holds per-user overrides: assign an individual usage plan, or disable an entry to block that account.", }; export default function McpUsersSection() { @@ -287,7 +295,7 @@ export default function McpUsersSection() {

MCP User Management

- Control who can access the AI agent interface (Model Context Protocol). + Control who can access the AI agent interface (Model Context Protocol) and assign individual usage plans.

@@ -316,9 +324,7 @@ export default function McpUsersSection() {

Access Policy

{POLICY_DESCRIPTIONS[policy]}

- {policy === "WhitelistOnly" && ( -

Global Admins always have access, no separate entry needed.

- )} + {policy !== "Disabled" &&

{LIST_PURPOSE[policy]}

}