From 0c1b83268caf268c33ceafee6e9aa480346e8bbe Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Mon, 19 Oct 2026 16:52:39 +0200 Subject: [PATCH] A self-service assignee reads the managed tenants only while an enabled member of the managing tenant (D-364) Removing or disabling a member under Access Management left their assignment to the tenant's self-service group in place, and the delegated scope never re-checked the membership, so the person kept reading every managed customer. - Scope: an assignment to msp-{owner} confers the group's tenants only while the assignee holds an enabled TenantAdmins row in the owner (inside the cached row resolution, fail-closed, Warning when ignored). Operator groups and direct grants are unchanged. - Access Management: remove and disable end the self-service assignment (DelegatedGroupAccess DELETE under every managed tenant, reason member-removed / member-disabled) before the existing stream cut. Re-enabling restores nothing. - Assigning requires an enabled member and re-reads the membership after the write; the Global Admin group route follows the same rule for a self-service group (enabled member of the owner, bound to it, read-only). - The SignalR group join resolves the delegated scope uncached, so the re-join right after a revoke cannot be admitted by another instance's warm cache. - Web: Delegated Access offers only enabled members and says that removing or disabling a member ends their access; the Global Admin page hides the role select for self-service groups. Co-Authored-By: Claude Opus 5.5 --- .../DelegatedAdminServiceTests.cs | 156 ++++++++++++++++++ .../DelegationSelfServiceTests.cs | 113 +++++++++++++ .../FakeSelfServiceAssignees.cs | 44 +++++ .../MiddlewarePipelineOrderTests.cs | 15 +- .../PolicyEnforcementMiddlewareTests.cs | 51 ++++++ .../RevokeEnforcementTests.cs | 80 ++++++++- .../TenantAdminManagementFunctionTests.cs | 72 +++++++- .../Admin/TenantAdminManagementFunction.cs | 26 ++- .../Admin/TenantGroupManagementFunction.cs | 49 +++++- .../SignalRAddToGroupFunction.cs | 6 +- .../AutopilotMonitor.Functions/Program.cs | 2 + .../Services/DelegatedAdminService.cs | 43 ++++- .../Services/DelegationSelfService.cs | 79 +++++++-- .../Services/Offboarding/SafeWipeService.cs | 2 +- .../Services/SignalRNotificationService.cs | 2 +- .../AutopilotMonitor.Shared/Constants.cs | 13 +- .../settings/sections/SectionTenantGroups.tsx | 21 ++- .../sections/SectionDelegatedAccess.tsx | 6 +- 18 files changed, 735 insertions(+), 45 deletions(-) create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/FakeSelfServiceAssignees.cs diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/DelegatedAdminServiceTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/DelegatedAdminServiceTests.cs index 5a4d4dcb..a0554a4c 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/DelegatedAdminServiceTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/DelegatedAdminServiceTests.cs @@ -598,4 +598,160 @@ public class DelegatedAdminServiceTests repo.Verify(r => r.UpdateTenantGroupAsync(GroupId1, "Renamed", "Service Desk"), Times.Once); repo.Verify(r => r.GetGroupAssignmentsForUpnAsync(It.IsAny()), Times.Once); } + + // ── Self-service groups: an assignment counts only while its assignee is an enabled member of the owner ── + + private static readonly string SelfServiceGroup = Constants.TenantGroupIds.ForHomeTenant(HomeTenant); + + /// The assignee's TenantAdmins row in the owning tenant: enabled, disabled, or none (null). + private static void OwnerMember(Mock repo, string upn, bool? enabled) => + repo.Setup(r => r.GetTenantMemberAsync(HomeTenant, upn)).ReturnsAsync(enabled is bool isEnabled + ? new TenantMember { TenantId = HomeTenant, Upn = upn, Role = Constants.TenantRoles.Viewer, IsEnabled = isEnabled } + : null); + + [Fact] + public async Task GetScope_SelfServiceGroup_EnabledOwnerMember_ConfersTheGroupsTenants() + { + var (svc, repo) = Build(); + ReturnsGroups(repo, Assignment(SelfServiceGroup)); + GroupTenants(repo, SelfServiceGroup, TenantA, TenantB); + OwnerMember(repo, Upn, enabled: true); + + var scope = await svc.GetScopeAsync(Id(Upn)); + + Assert.Equal(Constants.DelegatedRoles.DelegatedReader, scope.RoleFor(TenantA)); + Assert.Equal(Constants.DelegatedRoles.DelegatedReader, scope.RoleFor(TenantB)); + } + + [Theory] + [InlineData(false)] // disabled under Access Management + [InlineData(null)] // removed — a failed read reaches the gate the same way (the repository returns null) + public async Task GetScope_SelfServiceGroup_WithoutAnEnabledOwnerMembership_ConfersNothing(bool? enabled) + { + var (svc, repo) = Build(); + ReturnsGroups(repo, Assignment(SelfServiceGroup)); + GroupTenants(repo, SelfServiceGroup, TenantA); + OwnerMember(repo, Upn, enabled); + + var scope = await svc.GetScopeAsync(Id(Upn)); + + Assert.True(scope.IsEmpty); + repo.Verify(r => r.GetGroupMembershipAsync(It.IsAny()), Times.Never); + } + + [Fact] + public async Task GetScope_SelfServiceGroup_MembershipInAnotherTenant_DoesNotCount() + { + var (svc, repo) = Build(); + ReturnsGroups(repo, Assignment(SelfServiceGroup)); + GroupTenants(repo, SelfServiceGroup, TenantA); + repo.Setup(r => r.GetTenantMemberAsync(TenantC, Upn)).ReturnsAsync(new TenantMember { TenantId = TenantC, Upn = Upn }); + + var scope = await svc.GetScopeAsync(Id(Upn)); + + Assert.True(scope.IsEmpty); + repo.Verify(r => r.GetTenantMemberAsync(HomeTenant, Upn), Times.Once); + } + + [Fact] + public async Task GetScope_SelfServiceGroup_ApplicationPrincipal_IsGatedOnItsAppKeyRow() + { + const string appKey = "app:0a0a0a0a-0000-0000-0000-000000000001"; + var (svc, repo) = Build(); + repo.Setup(r => r.GetGroupAssignmentsForUpnAsync(appKey)).ReturnsAsync(new List + { + new() { Upn = appKey, GroupId = SelfServiceGroup, Role = Constants.DelegatedRoles.DelegatedReader, IsEnabled = true }, + }); + GroupTenants(repo, SelfServiceGroup, TenantA); + var app = new AdminIdentity(appKey, HomeTenant, Oid); + + Assert.True((await svc.GetScopeAsync(app)).IsEmpty); + + OwnerMember(repo, appKey, enabled: true); + Assert.Equal(Constants.DelegatedRoles.DelegatedReader, (await svc.GetFreshScopeAsync(app)).RoleFor(TenantA)); + } + + [Fact] + public async Task GetScope_OperatorGroup_NeverReadsTenantMembership() + { + // Operator groups (GUID ids) are granted by a Global Admin to people who are not members of any owner. + var (svc, repo) = Build(); + ReturnsGroups(repo, Assignment(GroupId1)); + GroupTenants(repo, GroupId1, TenantA); + + var scope = await svc.GetScopeAsync(Id(Upn)); + + Assert.Equal(Constants.DelegatedRoles.DelegatedReader, scope.RoleFor(TenantA)); + repo.Verify(r => r.GetTenantMemberAsync(It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task GetScope_DirectGrantAndOperatorGroup_SurviveASuppressedSelfServiceGroup() + { + var (svc, repo) = Build(); + Returns(repo, Row(TenantC)); + ReturnsGroups(repo, Assignment(SelfServiceGroup), Assignment(GroupId1)); + GroupTenants(repo, SelfServiceGroup, TenantA); + GroupTenants(repo, GroupId1, TenantB); + OwnerMember(repo, Upn, enabled: false); + + var scope = await svc.GetScopeAsync(Id(Upn)); + + Assert.Null(scope.RoleFor(TenantA)); + Assert.Equal(Constants.DelegatedRoles.DelegatedReader, scope.RoleFor(TenantB)); + Assert.Equal(Constants.DelegatedRoles.DelegatedReader, scope.RoleFor(TenantC)); + } + + [Fact] + public async Task GetScope_OwnerMembershipRead_IsPartOfTheCachedRowScope() + { + var (svc, repo) = Build(); + ReturnsGroups(repo, Assignment(SelfServiceGroup)); + GroupTenants(repo, SelfServiceGroup, TenantA); + OwnerMember(repo, Upn, enabled: true); + + await svc.GetScopeAsync(Id(Upn)); + await svc.GetScopeAsync(Id(Upn)); + + repo.Verify(r => r.GetTenantMemberAsync(HomeTenant, Upn), Times.Once); + } + + [Fact] + public async Task GetFreshScope_BypassesAWarmCache_AndRefreshesIt() + { + // A SignalR re-join right after a revoke must not be admitted by a scope cached before it. + var (svc, repo) = Build(); + ReturnsGroups(repo, Assignment(SelfServiceGroup)); + GroupTenants(repo, SelfServiceGroup, TenantA); + OwnerMember(repo, Upn, enabled: true); + Assert.NotNull((await svc.GetScopeAsync(Id(Upn))).RoleFor(TenantA)); // warms this instance's cache + + OwnerMember(repo, Upn, enabled: null); // removed elsewhere — this cache was not invalidated + + Assert.NotNull((await svc.GetScopeAsync(Id(Upn))).RoleFor(TenantA)); // the cached answer (≤ 30 s) + Assert.True((await svc.GetFreshScopeAsync(Id(Upn))).IsEmpty); + Assert.True((await svc.GetScopeAsync(Id(Upn))).IsEmpty); // written back: the cache now agrees + repo.Verify(r => r.GetTenantMemberAsync(HomeTenant, Upn), Times.Exactly(2)); + } + + [Fact] + public async Task GetFreshScope_NullIdentity_IsEmpty_WithoutTouchingStorage() + { + var (svc, repo) = Build(); + + Assert.True((await svc.GetFreshScopeAsync(null)).IsEmpty); + repo.Verify(r => r.GetDelegatedTenantsAsync(It.IsAny()), Times.Never); + } + + [Theory] + [InlineData("msp-99999999-9999-9999-9999-999999999999", "99999999-9999-9999-9999-999999999999")] + [InlineData("tpl-aaaaaaaa", null)] + [InlineData("0f8fad5bd9cb469fa16570867728950e", null)] + [InlineData("msp-", null)] + [InlineData("MSP-99999999-9999-9999-9999-999999999999", null)] + [InlineData(null, null)] + public void TenantGroupIds_OwnerOf_IsTheInverseOfForHomeTenant_ForSelfServiceIdsOnly(string? groupId, string? expectedOwner) + { + Assert.Equal(expectedOwner, Constants.TenantGroupIds.OwnerOf(groupId)); + } } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/DelegationSelfServiceTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/DelegationSelfServiceTests.cs index d4049a7a..9d614397 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/DelegationSelfServiceTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/DelegationSelfServiceTests.cs @@ -500,6 +500,119 @@ public class DelegationSelfServiceTests Assert.Equal(404, r.Failure!.Status); } + // ── Assignees are ENABLED members of the managing tenant — at grant time, and when the membership ends ── + + private static void AssignedToTheGroup(Harness h) => + h.Repo.Setup(x => x.GetGroupAssignmentsForUpnAsync(MspUser)) + .ReturnsAsync(new List { new() { Upn = MspUser, GroupId = GroupId } }); + + private static void VerifyNoAuditOf(Harness h, string action) => + h.Audit.Verify(a => a.LogAuditEntryAsync(It.IsAny(), action, It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny?>()), Times.Never); + + [Fact] + public async Task Assign_DisabledMember_400_NothingWritten() + { + var h = Build(); + h.GroupTenants.Add(Customer); + h.Repo.Setup(x => x.GetTenantMemberAsync(Home, MspUser)) + .ReturnsAsync(new TenantMember { Upn = MspUser, TenantId = Home, Role = "Operator", IsEnabled = false }); + + var r = await h.Svc.AssignAsync(Home, MspUser, MspAdmin); + + Assert.Equal(400, r.Failure!.Status); + Assert.Equal(Constants.DelegationCodes.NotATenantMember, r.Failure.Code); + h.Repo.Verify(x => x.AssignGroupAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + h.Repo.Verify(x => x.EnsureOwnedTenantGroupAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + VerifyNoAuditOf(h, "CREATE"); + } + + [Fact] + public async Task Assign_MemberRemovedWhileAssigning_IsRolledBack_400_NoCreateAudit() + { + // The removal wrote the member row before this assignment existed, so its cleanup found nothing to end; + // the re-read after the write catches it instead. + var h = Build(); + h.GroupTenants.Add(Customer); + h.Repo.SetupSequence(x => x.GetTenantMemberAsync(Home, MspUser)) + .ReturnsAsync(new TenantMember { Upn = MspUser, TenantId = Home, Role = "Operator" }) + .ReturnsAsync((TenantMember?)null); + AssignedToTheGroup(h); + + var r = await h.Svc.AssignAsync(Home, MspUser, MspAdmin); + + Assert.Equal(400, r.Failure!.Status); + Assert.Equal(Constants.DelegationCodes.NotATenantMember, r.Failure.Code); + h.Repo.Verify(x => x.AssignGroupAsync(MspUser, GroupId, Constants.DelegatedRoles.DelegatedReader, true, MspAdmin), Times.Once); + h.Repo.Verify(x => x.UnassignGroupAsync(MspUser, GroupId), Times.Once); + VerifyNoAuditOf(h, "CREATE"); + } + + [Fact] + public async Task AssignMember_ReasonAndActorReachEveryCustomersAudit() + { + var h = Build(); + h.GroupTenants.Add(Customer); + h.GroupTenants.Add(Other); + + var r = await h.Svc.AssignMemberAsync(Home, MspUser, "GA@Vendor.Example", DelegationSelfService.AssigneeReasons.GroupAssigned); + + Assert.True(r.Ok); + foreach (var tenant in new[] { Customer, Other }) + { + h.Audit.Verify(a => a.LogAuditEntryAsync(tenant, "CREATE", "DelegatedGroupAccess", MspUser, "ga@vendor.example", + It.Is?>(d => d != null && d["Reason"] == "group-assigned" && d["HomeTenantId"] == Home)), Times.Once); + } + } + + [Fact] + public async Task EndAssignment_Assigned_UnassignsAndAuditsEveryCustomerWithTheReason_WithoutDisconnecting() + { + var h = Build(); + h.GroupTenants.Add(Customer); + h.GroupTenants.Add(Other); + AssignedToTheGroup(h); + + var ended = await h.Svc.EndAssignmentAsync(Home, "Analyst@Partner.Example", "Admin@Partner.Example", DelegationSelfService.AssigneeReasons.MemberRemoved); + + Assert.True(ended); + h.Repo.Verify(x => x.UnassignGroupAsync(MspUser, GroupId), Times.Once); + foreach (var tenant in new[] { Customer, Other }) + { + h.Audit.Verify(a => a.LogAuditEntryAsync(tenant, "DELETE", "DelegatedGroupAccess", MspUser, MspAdmin, + It.Is?>(d => d != null && d["Reason"] == "member-removed" && d["GroupId"] == GroupId)), Times.Once); + } + Assert.Empty(h.SignalR.DisconnectedUsers); // the caller cuts the streams after its own writes + } + + [Fact] + public async Task EndAssignment_NotAssigned_IsANoOp_WithoutAudit() + { + var h = Build(); + h.GroupTenants.Add(Customer); + + var ended = await h.Svc.EndAssignmentAsync(Home, MspUser, MspAdmin, DelegationSelfService.AssigneeReasons.MemberDisabled); + + Assert.False(ended); + h.Repo.Verify(x => x.UnassignGroupAsync(It.IsAny(), It.IsAny()), Times.Never); + VerifyNoAuditOf(h, "DELETE"); + } + + [Fact] + public async Task Unassign_Assigned_AuditsTheSelfServiceReason_AndDisconnectsOnce() + { + var h = Build(); + h.GroupTenants.Add(Customer); + AssignedToTheGroup(h); + + var r = await h.Svc.UnassignAsync(Home, MspUser, MspAdmin); + + Assert.True(r.Ok); + h.Audit.Verify(a => a.LogAuditEntryAsync(Customer, "DELETE", "DelegatedGroupAccess", MspUser, MspAdmin, + It.Is?>(d => d != null && d["Reason"] == "self-service-unassigned")), Times.Once); + Assert.Equal(new[] { MspUser }, h.SignalR.DisconnectedUsers); + } + // ── Managers (customer side) ──────────────────────────────────────────────── [Fact] diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/FakeSelfServiceAssignees.cs b/src/Backend/AutopilotMonitor.Functions.Tests/FakeSelfServiceAssignees.cs new file mode 100644 index 00000000..1fc49840 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/FakeSelfServiceAssignees.cs @@ -0,0 +1,44 @@ +using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared.DataAccess; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// In-memory for the function tests: records every call, and — paired with a +/// — how many disconnects had happened when it was called, which is +/// the proof that a handler ends the assignment BEFORE it cuts the live streams. +/// +public class FakeSelfServiceAssignees : ISelfServiceAssignees +{ + private readonly FakeSignalRNotificationService? _signalR; + + public FakeSelfServiceAssignees(FakeSignalRNotificationService? signalR = null) => _signalR = signalR; + + public List<(string HomeTenantId, string Upn, string Actor, string Reason, int DisconnectsBefore)> Ended { get; } = new(); + public List<(string HomeTenantId, string Upn, string Actor, string Reason)> Assigned { get; } = new(); + + /// When set, throws it (fail-soft path of the caller). + public Exception? EndThrows { get; set; } + + /// When set, returns it instead of a success. + public DelegationFailure? AssignFails { get; set; } + + public Task EndAssignmentAsync(string homeTenantId, string upn, string actor, string reason) + { + Ended.Add((homeTenantId, upn, actor, reason, _signalR?.DisconnectedUsers.Count ?? 0)); + if (EndThrows != null) + throw EndThrows; + return Task.FromResult(true); + } + + public Task> AssignMemberAsync(string homeTenantId, string upn, string actor, string reason) + { + Assigned.Add((homeTenantId, upn, actor, reason)); + return Task.FromResult(AssignFails is { } failure + ? DelegationResult.Fail(failure.Status, failure.Code, failure.Message) + : DelegationResult.Success(new TenantGroupAssignment { Upn = upn, GroupId = homeTenantId })); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/MiddlewarePipelineOrderTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/MiddlewarePipelineOrderTests.cs index 9b9dfbd0..e940e625 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/MiddlewarePipelineOrderTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/MiddlewarePipelineOrderTests.cs @@ -1,5 +1,6 @@ using System.IO; using AutopilotMonitor.Functions.Middleware; +using AutopilotMonitor.Functions.Services; using Xunit; namespace AutopilotMonitor.Functions.Tests; @@ -9,7 +10,8 @@ namespace AutopilotMonitor.Functions.Tests; /// HTTP route, and the raw table/log proxies rely on the RequestContext it resolves. Nothing /// at runtime would notice if the registration in Program.cs were dropped or moved in front of /// (the policy step reads the principal the auth step stores) — -/// the routes would simply open up. This source scan pins the registration and its order. +/// the routes would simply open up. This source scan pins the registration and its order, plus the one +/// service registration that ending delegated access depends on. /// public class MiddlewarePipelineOrderTests { @@ -87,6 +89,17 @@ public class MiddlewarePipelineOrderTests } } + [Fact] + public void Self_service_assignee_rule_is_registered_as_the_delegation_service() + { + // Access Management (member remove/disable) and the GA group route take it by constructor; without the + // registration every route of both classes fails to activate, and nothing checks DI before a request. + var src = ProgramSource(); + + Assert.Equal(1, Count(src, + $"AddSingleton<{nameof(ISelfServiceAssignees)}>(sp => sp.GetRequiredService<{nameof(DelegationSelfService)}>())")); + } + private static int Count(string haystack, string needle) { var count = 0; diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs index f33caed1..2cc8d635 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs @@ -646,6 +646,57 @@ public class PolicyEnforcementMiddlewareTests Assert.True(result.Context!.IsDelegatedReader); } + /// The caller is assigned to 's self-service group, which manages . + private static void AsSelfServiceAssignee(Harness h, string home, string managed) + { + var groupId = Constants.TenantGroupIds.ForHomeTenant(home); + h.Repo.Setup(r => r.GetGroupAssignmentsForUpnAsync(It.IsAny())) + .ReturnsAsync(new List + { + new() { GroupId = groupId, Role = Constants.DelegatedRoles.DelegatedReader, IsEnabled = true }, + }); + h.Repo.Setup(r => r.GetGroupMembershipAsync(groupId)) + .ReturnsAsync(new TenantGroupMembership { GroupId = groupId, TenantIds = new List { managed } }); + } + + [Fact] + public async Task Delegated_SelfServiceAssignee_RemovedFromTheManagingTenant_IsForbiddenOnTheManagedTenant() + { + // A member removed or disabled under Access Management may still hold the msp-{home} assignment row; + // it no longer rescues them into the customers the managing tenant manages. + const string upn = "leaver@partner.example"; + var h = BuildHarness(); + AsSelfServiceAssignee(h, home: TenantA, managed: TenantB); // harness default: no TenantAdmins row anywhere + + var result = await h.Middleware.DecideAsync( + "GET", "/api/sessions/abc-123/events", TenantB, AuthedPrincipal(TenantA, upn)); + + Assert.False(result.Allowed); + Assert.Equal(403, result.StatusCode); + } + + [Fact] + public async Task Delegated_SelfServiceAssignee_EnabledInTheManagingTenant_ReadsTheManagedTenant() + { + const string upn = "analyst@partner.example"; + var h = BuildHarness(); + AsSelfServiceAssignee(h, home: TenantA, managed: TenantB); + h.Repo.Setup(r => r.GetTenantMemberAsync(TenantA, upn)).ReturnsAsync(new TenantMember + { + Upn = upn, + TenantId = TenantA, + Role = Constants.TenantRoles.Viewer, + IsEnabled = true, + }); + + var result = await h.Middleware.DecideAsync( + "GET", "/api/sessions/abc-123/events", TenantB, AuthedPrincipal(TenantA, upn)); + + Assert.True(result.Allowed); + Assert.Equal(TenantB, result.Context!.TargetTenantId); + Assert.True(result.Context!.IsDelegatedReader); + } + [Fact] public async Task Delegated_SessionMutation_IsForbidden() { diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/RevokeEnforcementTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/RevokeEnforcementTests.cs index 8b43e04d..a1a5f944 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/RevokeEnforcementTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/RevokeEnforcementTests.cs @@ -77,7 +77,8 @@ public class RevokeEnforcementTests } private static (TenantGroupManagementFunction Fn, Mock Repo, - Mock Audit, FakeSignalRNotificationService SignalR) BuildGroupFn(ProConferralService? proConferral = null) + Mock Audit, FakeSignalRNotificationService SignalR) BuildGroupFn( + ProConferralService? proConferral = null, ISelfServiceAssignees? assignees = null) { var repo = new Mock(); // Loose-mock defaults return a null Task — every repo read the flows touch needs a concrete setup. @@ -90,7 +91,7 @@ public class RevokeEnforcementTests var signalR = new FakeSignalRNotificationService(); var fn = new TenantGroupManagementFunction( NullLogger.Instance, BuildService(repo), Resolver(), audit.Object, signalR, BuildSlots(repo), - proConferral ?? TestProConferral.Inert()); + proConferral ?? TestProConferral.Inert(), assignees ?? new FakeSelfServiceAssignees(signalR)); return (fn, repo, audit, signalR); } @@ -475,4 +476,79 @@ public class RevokeEnforcementTests Assert.Equal(TenantGroupManagementFunction.GroupUpdateOutcome.NotFound, await fn.UpdateGroupCoreAsync("tpl-missing", new UpdateTenantGroupRequest { Name = "Renamed" }, "ga@vendor.example")); } + + // --- A Global Admin assigning into a tenant's self-service group gets the self-service rule --- + + [Fact] + public async Task AssignToOwnedGroupCore_AppliesTheSelfServiceRule_AsTheGlobalAdmin() + { + var assignees = new FakeSelfServiceAssignees(); + var (fn, _, audit, _) = BuildGroupFn(assignees: assignees); + + var failure = await fn.AssignToOwnedGroupCoreAsync( + OwnerTenant, UpnLower, Constants.DelegatedRoles.DelegatedReader, null, null, "ga@vendor.example"); + + Assert.Null(failure); + var assigned = Assert.Single(assignees.Assigned); + Assert.Equal((OwnerTenant, UpnLower, "ga@vendor.example", DelegationSelfService.AssigneeReasons.GroupAssigned), assigned); + VerifyNoAudit(audit); // the rule writes the customers' CREATE rows itself + } + + [Fact] + public async Task AssignToOwnedGroupCore_DelegatedAdminRole_IsRefused_RuleNotConsulted() + { + var assignees = new FakeSelfServiceAssignees(); + var (fn, _, _, _) = BuildGroupFn(assignees: assignees); + + var failure = await fn.AssignToOwnedGroupCoreAsync( + OwnerTenant, UpnLower, Constants.DelegatedRoles.DelegatedAdmin, null, null, "ga@vendor.example"); + + Assert.Equal(400, failure?.Status); + Assert.Empty(assignees.Assigned); + } + + [Theory] + [InlineData("44444444-4444-4444-4444-444444444444", null)] + [InlineData(null, "55555555-5555-5555-5555-555555555555")] + [InlineData(OwnerTenant, "55555555-5555-5555-5555-555555555555")] + public async Task AssignToOwnedGroupCore_ForeignHomeOrObjectId_IsRefused_RuleNotConsulted(string? homeTenantId, string? objectId) + { + var assignees = new FakeSelfServiceAssignees(); + var (fn, _, _, _) = BuildGroupFn(assignees: assignees); + + var failure = await fn.AssignToOwnedGroupCoreAsync( + OwnerTenant, UpnLower, Constants.DelegatedRoles.DelegatedReader, homeTenantId, objectId, "ga@vendor.example"); + + Assert.Equal(400, failure?.Status); + Assert.Empty(assignees.Assigned); + } + + [Fact] + public async Task AssignToOwnedGroupCore_HomeNamingTheOwner_Passes() + { + var assignees = new FakeSelfServiceAssignees(); + var (fn, _, _, _) = BuildGroupFn(assignees: assignees); + + var failure = await fn.AssignToOwnedGroupCoreAsync( + OwnerTenant, UpnLower, Constants.DelegatedRoles.DelegatedReader, OwnerTenant.ToUpperInvariant(), null, "ga@vendor.example"); + + Assert.Null(failure); + Assert.Single(assignees.Assigned); + } + + [Theory] + [InlineData(400, Constants.DelegationCodes.NotATenantMember)] + [InlineData(403, Constants.DelegationCodes.DelegatedAdminNotAllowed)] + [InlineData(409, Constants.DelegationCodes.IdentityBindingConflict)] + public async Task AssignToOwnedGroupCore_RuleFailure_IsPassedThrough(int status, string code) + { + var assignees = new FakeSelfServiceAssignees { AssignFails = new DelegationFailure(status, code, "refused") }; + var (fn, _, _, _) = BuildGroupFn(assignees: assignees); + + var failure = await fn.AssignToOwnedGroupCoreAsync( + OwnerTenant, UpnLower, Constants.DelegatedRoles.DelegatedReader, null, null, "ga@vendor.example"); + + Assert.Equal(status, failure?.Status); + Assert.Equal(code, failure?.Code); + } } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/TenantAdminManagementFunctionTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/TenantAdminManagementFunctionTests.cs index d6c9077d..427069fe 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/TenantAdminManagementFunctionTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/TenantAdminManagementFunctionTests.cs @@ -184,6 +184,73 @@ public class TenantAdminManagementFunctionTests Assert.Equal(new[] { "user@contoso.com" }, h.SignalR.DisconnectedUsers); } + // ── removal and disable end the member's self-service delegation, before the stream cut ── + + [Fact] + public async Task Remove_ends_the_self_service_assignment_before_cutting_streams() + { + var h = BuildHarness(); + var (req, ctx) = BuildRequest(new { }); + + var response = await h.Function.RemoveTenantAdmin(req, TenantId, "Leaver@Contoso.com", ctx); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var ended = Assert.Single(h.Assignees.Ended); + Assert.Equal(TenantId, ended.HomeTenantId); + Assert.Equal("leaver@contoso.com", ended.Upn.ToLowerInvariant()); + Assert.Equal(CallerUpn, ended.Actor); + Assert.Equal(DelegationSelfService.AssigneeReasons.MemberRemoved, ended.Reason); + Assert.Equal(0, ended.DisconnectsBefore); + Assert.Equal(new[] { "leaver@contoso.com" }, h.SignalR.DisconnectedUsers); + } + + [Fact] + public async Task Disable_ends_the_self_service_assignment_before_cutting_streams() + { + var h = BuildHarness(); + var (req, ctx) = BuildRequest(new { }); + + var response = await h.Function.DisableTenantAdmin(req, TenantId, "Leaver@Contoso.com", ctx); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var ended = Assert.Single(h.Assignees.Ended); + Assert.Equal(TenantId, ended.HomeTenantId); + Assert.Equal("leaver@contoso.com", ended.Upn.ToLowerInvariant()); + Assert.Equal(DelegationSelfService.AssigneeReasons.MemberDisabled, ended.Reason); + Assert.Equal(0, ended.DisconnectsBefore); + Assert.Equal(new[] { "leaver@contoso.com" }, h.SignalR.DisconnectedUsers); + } + + [Fact] + public async Task Enable_add_and_role_change_never_end_an_assignment() + { + var h = BuildHarness(); + + var (enable, ctx1) = BuildRequest(new { }); + await h.Function.EnableTenantAdmin(enable, TenantId, "user@contoso.com", ctx1); + var (add, ctx2) = BuildRequest(new { upn = "new@contoso.com", role = "viewer" }); + await h.Function.AddTenantAdmin(add, TenantId, ctx2); + var (demote, ctx3) = BuildRequest(new { role = "viewer", canManageBootstrapTokens = false }); + await h.Function.UpdateMemberPermissions(demote, TenantId, "user@contoso.com", ctx3); + + Assert.Empty(h.Assignees.Ended); + } + + [Fact] + public async Task Remove_still_succeeds_and_cuts_streams_when_ending_the_assignment_fails() + { + // Fail-soft: the scope resolution ignores an assignment without an enabled membership anyway. + var h = BuildHarness(); + h.Assignees.EndThrows = new InvalidOperationException("storage unavailable"); + var (req, ctx) = BuildRequest(new { }); + + var response = await h.Function.RemoveTenantAdmin(req, TenantId, "leaver@contoso.com", ctx); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Single(h.Assignees.Ended); + Assert.Equal(new[] { "leaver@contoso.com" }, h.SignalR.DisconnectedUsers); + } + // ── harness ───────────────────────────────────────────────────────────── private sealed class Harness @@ -191,6 +258,7 @@ public class TenantAdminManagementFunctionTests public TenantAdminManagementFunction Function = default!; public Mock AdminRepo = default!; public FakeSignalRNotificationService SignalR = default!; + public FakeSelfServiceAssignees Assignees = default!; } private static Harness BuildHarness() @@ -210,13 +278,15 @@ public class TenantAdminManagementFunctionTests var maintenanceRepo = new Mock(MockBehavior.Loose); var signalR = new FakeSignalRNotificationService(); + var assignees = new FakeSelfServiceAssignees(signalR); return new Harness { Function = new TenantAdminManagementFunction( - NullLogger.Instance, service, maintenanceRepo.Object, signalR), + NullLogger.Instance, service, maintenanceRepo.Object, signalR, assignees), AdminRepo = adminRepo, SignalR = signalR, + Assignees = assignees, }; } diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantAdminManagementFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantAdminManagementFunction.cs index 7cb6a4da..b3c03cb2 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantAdminManagementFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantAdminManagementFunction.cs @@ -20,17 +20,39 @@ public class TenantAdminManagementFunction private readonly TenantAdminsService _tenantAdminsService; private readonly IMaintenanceRepository _maintenanceRepo; private readonly ISignalRNotificationService _signalRService; + private readonly ISelfServiceAssignees _selfServiceAssignees; public TenantAdminManagementFunction( ILogger logger, TenantAdminsService tenantAdminsService, IMaintenanceRepository maintenanceRepo, - ISignalRNotificationService signalRService) + ISignalRNotificationService signalRService, + ISelfServiceAssignees selfServiceAssignees) { _logger = logger; _tenantAdminsService = tenantAdminsService; _maintenanceRepo = maintenanceRepo; _signalRService = signalRService; + _selfServiceAssignees = selfServiceAssignees; + } + + /// + /// A member who is removed or disabled loses their self-service delegation (read access to every tenant this + /// tenant manages) with it: the assignment ends and each managed customer's trail records the loss. Runs after + /// the member write and before , so the re-join sees both. Fail-soft — the scope + /// resolution ignores an assignment without an enabled membership anyway, so a failure here costs only the + /// cleanup and the customer audit row. + /// + private async Task EndSelfServiceAssignmentAsync(string tenantId, string adminUpn, string actor, string reason) + { + try + { + await _selfServiceAssignees.EndAssignmentAsync(tenantId, adminUpn, actor, reason); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Ending the self-service delegation of {AdminUpn} in tenant {TenantId} failed ({Reason})", adminUpn, tenantId, reason); + } } /// @@ -187,6 +209,7 @@ public class TenantAdminManagementFunction upn! ); + await EndSelfServiceAssignmentAsync(requestCtx.TargetTenantId, adminUpn, upn!, DelegationSelfService.AssigneeReasons.MemberRemoved); await CutLiveStreamsAsync(adminUpn); _logger.LogInformation($"Tenant Admin removed: {adminUpn} from tenant {requestCtx.TargetTenantId} by {upn}"); @@ -223,6 +246,7 @@ public class TenantAdminManagementFunction new Dictionary { { "Action", "Disable" } } ); + await EndSelfServiceAssignmentAsync(requestCtx.TargetTenantId, adminUpn, upn!, DelegationSelfService.AssigneeReasons.MemberDisabled); await CutLiveStreamsAsync(adminUpn); _logger.LogInformation($"Tenant Admin disabled: {adminUpn} for tenant {requestCtx.TargetTenantId} by {upn}"); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs index 8fc09021..c9e1b6df 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs @@ -26,6 +26,9 @@ namespace AutopilotMonitor.Functions.Functions.Admin; /// customer sees "who can read my tenant" / "access removed"); the group appears there under its customer-facing /// name (), never its internal name. Group create/rename/label /// carry no access change — they are logged operationally only (no AuditLogs partition). +/// +/// A tenant's self-service group () keeps the self-service assignee rule +/// even here: a Global Admin assigns only enabled members of the owning tenant, read-only, bound to it as home. /// public class TenantGroupManagementFunction { @@ -36,6 +39,7 @@ public class TenantGroupManagementFunction private readonly ISignalRNotificationService _signalRService; private readonly DelegatedSlotService _slots; private readonly ProConferralService _proConferral; + private readonly ISelfServiceAssignees _selfServiceAssignees; private const string AuditEntity = "DelegatedGroupAccess"; @@ -46,7 +50,8 @@ public class TenantGroupManagementFunction IMaintenanceRepository maintenanceRepo, ISignalRNotificationService signalRService, DelegatedSlotService slots, - ProConferralService proConferral) + ProConferralService proConferral, + ISelfServiceAssignees selfServiceAssignees) { _logger = logger; _delegatedAdminService = delegatedAdminService; @@ -55,6 +60,7 @@ public class TenantGroupManagementFunction _signalRService = signalRService; _slots = slots; _proConferral = proConferral; + _selfServiceAssignees = selfServiceAssignees; } /// The managing tenant that owns (self-service group), or null for operator groups / unknown groups. @@ -319,7 +325,8 @@ public class TenantGroupManagementFunction /// "homeTenantId": "<optional override>", "objectId": "<optional>" }. /// The UPN's identity binding is resolved automatically (sign-in history, then UPN domain → onboarded /// tenant); 422 HomeTenantUnresolved when neither works, 409 if the UPN is already bound elsewhere - /// (rebind explicitly via global/identity-bindings). + /// (rebind explicitly via global/identity-bindings). A tenant's self-service group takes only enabled members of + /// its owner, read-only and bound to the owner (). /// [Function("AssignTenantGroup")] [Authorize] @@ -350,6 +357,16 @@ public class TenantGroupManagementFunction return await NotFound(req); var upn = body.Upn.ToLowerInvariant(); + + var owner = Constants.TenantGroupIds.OwnerOf(group.GroupId); + if (owner != null) + { + var failure = await AssignToOwnedGroupCoreAsync(owner, upn, role, body.HomeTenantId, body.ObjectId, currentUpn); + return failure == null + ? await req.CreatedAsync(new MessageResponse { Message = "Assigned to group" }) + : await DelegationSelfServiceFunction.FailAsync(req, failure); + } + var identity = await IdentityBindingRules.ResolveForGrantAsync(_identityResolver, upn, body.HomeTenantId, body.ObjectId); if (identity == null) { @@ -383,7 +400,7 @@ public class TenantGroupManagementFunction { "Group", DelegationSelfService.CustomerNameOf(group) }, { "GroupId", groupId }, { "Role", role }, - { "Reason", "group-assigned" }, + { "Reason", DelegationSelfService.AssigneeReasons.GroupAssigned }, }); _logger.LogInformation("Group {GroupId} assigned to {Upn} ({Role}) by {By}", groupId, upn, role, currentUpn); @@ -391,6 +408,30 @@ public class TenantGroupManagementFunction return await req.CreatedAsync(new MessageResponse { Message = "Assigned to group" }); } + /// + /// Testable core of a Global Admin assignment into a tenant's self-service group. The self-service assignee rule + /// applies (): an enabled member of the owning tenant, bound to + /// it as home, read-only. The group's tenants already occupy the owner's slots, so no slot check. Null on success. + /// + internal async Task AssignToOwnedGroupCoreAsync( + string ownerTenantId, string upn, string role, string? homeTenantId, string? objectId, string? currentUpn) + { + if (role != Constants.DelegatedRoles.DelegatedReader) + return new DelegationFailure(400, Constants.ApiErrorCodes.BadRequest, + $"A self-service group grants read-only access; role must be '{Constants.DelegatedRoles.DelegatedReader}'."); + if (!string.IsNullOrWhiteSpace(objectId) + || (!string.IsNullOrWhiteSpace(homeTenantId) && !string.Equals(homeTenantId, ownerTenantId, StringComparison.OrdinalIgnoreCase))) + return new DelegationFailure(400, Constants.ApiErrorCodes.BadRequest, + "Assignees of a self-service group are members of the owning tenant and are bound to it; omit homeTenantId and objectId."); + + var result = await _selfServiceAssignees.AssignMemberAsync(ownerTenantId, upn, currentUpn ?? "", DelegationSelfService.AssigneeReasons.GroupAssigned); + if (!result.Ok) + return result.Failure; + + _logger.LogInformation("Self-service group of {OwnerTenantId} assigned to {Upn} by {By}", ownerTenantId, upn, currentUpn); + return null; + } + /// DELETE /api/global/tenant-groups/{groupId}/assignees/{upn} — unassign a UPN. GlobalAdminOnly. [Function("UnassignTenantGroup")] [Authorize] @@ -431,7 +472,7 @@ public class TenantGroupManagementFunction { { "Group", DelegationSelfService.CustomerNameOf(group) }, { "GroupId", groupId }, - { "Reason", "group-unassigned" }, + { "Reason", DelegationSelfService.AssigneeReasons.GroupUnassigned }, }); } diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRAddToGroupFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRAddToGroupFunction.cs index 80729459..cfae8592 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRAddToGroupFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRAddToGroupFunction.cs @@ -92,8 +92,10 @@ namespace AutopilotMonitor.Functions.Functions.Infrastructure { // realtime/groups/join is not a tenant-scoped route, so the middleware did not // resolve the delegated scope onto RequestContext — resolve it here, bounded to - // the requested tenant only. Home tid gates the MSP seat (Pro-only). - var scope = await _delegatedAdminService.GetScopeAsync(AdminIdentity.FromRequestContext(requestCtx)); + // the requested tenant only. Home tid gates the MSP seat (Pro-only). FRESH, not + // cached: a join streams until the connection drops, and after a revoke the client + // re-joins at once — possibly on an instance whose cached scope predates the revoke. + var scope = await _delegatedAdminService.GetFreshScopeAsync(AdminIdentity.FromRequestContext(requestCtx)); allowedCrossTenant = scope.RoleFor(requestedTenantId) != null; if (allowedCrossTenant) _logger.LogInformation($"Delegated user {userEmail} joining managed cross-tenant group: {request.GroupName}"); diff --git a/src/Backend/AutopilotMonitor.Functions/Program.cs b/src/Backend/AutopilotMonitor.Functions/Program.cs index 05a18aa3..9238c137 100644 --- a/src/Backend/AutopilotMonitor.Functions/Program.cs +++ b/src/Backend/AutopilotMonitor.Functions/Program.cs @@ -271,6 +271,8 @@ builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); +// The self-service assignee rule for Access Management (member remove/disable) and the GA group route. +builder.Services.AddSingleton(sp => sp.GetRequiredService()); builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); diff --git a/src/Backend/AutopilotMonitor.Functions/Services/DelegatedAdminService.cs b/src/Backend/AutopilotMonitor.Functions/Services/DelegatedAdminService.cs index 787c006e..5e43b08b 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/DelegatedAdminService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/DelegatedAdminService.cs @@ -20,6 +20,10 @@ namespace AutopilotMonitor.Functions.Services; /// GATED on the caller's home tenant (JWT tid) being Pro — the MSP seat is a Pro capability of the tenant /// the admin is homed in, while the managed targets may be any edition. Row resolution is cached briefly /// (UPN-keyed; binding + gate applied per call); every mutation invalidates the cache. +/// +/// An assignment to a tenant's self-service group () confers scope only +/// while its assignee is an ENABLED member of the owning tenant — re-checked at every row resolution, so a +/// person removed or disabled under Access Management stops reading the managed customers on every path. /// public class DelegatedAdminService { @@ -66,12 +70,24 @@ public class DelegatedAdminService /// and a rebind / home-tenant edition change converges within the respective cache TTLs. /// /// - public virtual async Task GetScopeAsync(AdminIdentity? identity) + public virtual Task GetScopeAsync(AdminIdentity? identity) + => ResolveScopeAsync(identity, bypassRowCache: false); + + /// + /// with the assignment rows and the self-service membership read fresh, never + /// from this instance's cache (the fresh result is written back). For decisions that outlive the request — + /// a SignalR group join keeps streaming until the connection drops, and the client re-joins right after a + /// revoke, possibly on an instance whose cached scope predates it. Binding and entitlement keep their caches. + /// + public virtual Task GetFreshScopeAsync(AdminIdentity? identity) + => ResolveScopeAsync(identity, bypassRowCache: true); + + private async Task ResolveScopeAsync(AdminIdentity? identity, bool bypassRowCache) { if (identity == null) return DelegatedScope.Empty; - var scope = await GetRowScopeAsync(identity.Upn); + var scope = await GetRowScopeAsync(identity.Upn, bypassRowCache); if (scope.IsEmpty) return scope; @@ -89,11 +105,11 @@ public class DelegatedAdminService } /// The scope the assignment ROWS (direct + group-derived) confer on a UPN, ignoring identity binding - /// and entitlement. Cached briefly (UPN-keyed). - private async Task GetRowScopeAsync(string upn) + /// and entitlement. Cached briefly (UPN-keyed); skips the read, never the write. + private async Task GetRowScopeAsync(string upn, bool bypassCache) { var cacheKey = $"delegated-scope:{upn}"; - if (_cache.TryGetValue(cacheKey, out var cached) && cached != null) + if (!bypassCache && _cache.TryGetValue(cacheKey, out var cached) && cached != null) return cached; var rows = await _adminRepo.GetDelegatedTenantsAsync(upn); @@ -129,6 +145,18 @@ public class DelegatedAdminService if (role == null) continue; + // A self-service group's assignees are its owner's own members, so the precondition of that grant is + // re-checked here, not trusted from grant time: no enabled TenantAdmins row in the owning tenant (removed, + // disabled, or the read failed) ⇒ the assignment confers nothing. + var owner = Constants.TenantGroupIds.OwnerOf(assignment.GroupId); + if (owner != null && (await _adminRepo.GetTenantMemberAsync(owner, upn))?.IsEnabled != true) + { + _logger.LogWarning( + "[DelegatedAdmin] Ignoring the assignment of {Upn} to self-service group {GroupId}: not an enabled member of the owning tenant {OwnerTenantId}", + upn, assignment.GroupId, owner); + continue; + } + // Members from ONE partition read (meta-backed: a missing meta row ⇒ no group, no scope). var membership = await _adminRepo.GetGroupMembershipAsync(assignment.GroupId); if (membership == null) @@ -529,8 +557,9 @@ public class TenantGroupEntity : ITableEntity /// /// A row in the table: delegated-admin UPN X /// is assigned to group T at role . PK = UPN (lowercase), RK = groupId. Resolving -/// a UPN's scope point-scans this by PK, then expands each group into its tenant members. Operator-managed -/// only (no PendingApproval flow); fail-closed — only enabled + recognized-role assignments confer scope. +/// a UPN's scope point-scans this by PK, then expands each group into its tenant members. Written by operators +/// and by self-service delegation (no PendingApproval flow); fail-closed — only enabled + recognized-role +/// assignments confer scope, and a self-service assignment only while its assignee is an enabled member of the owner. /// public class TenantGroupAssignmentEntity : ITableEntity { diff --git a/src/Backend/AutopilotMonitor.Functions/Services/DelegationSelfService.cs b/src/Backend/AutopilotMonitor.Functions/Services/DelegationSelfService.cs index 7abd94ba..83e3c947 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/DelegationSelfService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/DelegationSelfService.cs @@ -26,13 +26,32 @@ public sealed record ManagedTenantsView(DelegatedSlotUsage Slots, IReadOnlyList< public sealed record TenantManagerView(string? GroupId, string? OwnerTenantId, string? OwnerDomain, string Name, string Source, IReadOnlyList Assignees, DateTime? SinceUtc, bool Revocable); +/// +/// The self-service assignee rule for the routes outside the self-service surface: Access Management ends the +/// assignment of a member it removes or disables, and a Global Admin assigns into a tenant's self-service group +/// under the same rule. Implementations may throw; each caller decides how a failure surfaces. +/// +public interface ISelfServiceAssignees +{ + /// + /// Ends 's assignment to 's self-service group and audits the + /// loss under every managed tenant with . False, with nothing written, when the person + /// is not assigned. Never cuts live streams — the caller does that after its own writes. + /// + Task EndAssignmentAsync(string homeTenantId, string upn, string actor, string reason); + + /// Assigns an enabled member of to its self-service group (read-only), audited with . + Task> AssignMemberAsync(string homeTenantId, string upn, string actor, string reason); +} + /// /// The customer-facing half of delegated ("MSP") administration — no Global Admin involved: /// /// A Pro (managing) tenant owns exactly ONE implicit Tenant Group, msp-{tenantId} /// (). Its members are the customer tenants that ACCEPTED an invitation; -/// its assignees are the managing tenant's own users. DelegatedAdminService.GetScopeAsync, the policy -/// middleware and the MCP server need no change — the group is an ordinary group to them. +/// its assignees are the managing tenant's own ENABLED members — DelegatedAdminService re-checks that +/// membership at every scope resolution, and removing or disabling the member under Access Management ends the +/// assignment (). /// Invitations are signed single-use links (); the row's Pending /// status + ETag flip is the one-shot authority. The ACCEPTING tenant is always the caller's validated JWT /// tenant — a link never names its target. @@ -43,10 +62,22 @@ public sealed record TenantManagerView(string? GroupId, string? OwnerTenantId, s /// /// Self-service grants are always (read-only). /// -public class DelegationSelfService +public class DelegationSelfService : ISelfServiceAssignees { public const string SourceSelfService = "self-service"; public const string SourceOperator = "operator"; + + /// The Reason an assignee's DelegatedGroupAccess audit row carries under every managed tenant. + public static class AssigneeReasons + { + public const string SelfServiceAssigned = "self-service-assigned"; + public const string SelfServiceUnassigned = "self-service-unassigned"; + public const string GroupAssigned = "group-assigned"; + public const string GroupUnassigned = "group-unassigned"; + public const string MemberRemoved = "member-removed"; + public const string MemberDisabled = "member-disabled"; + } + /// /// The label a managed tenant sees for operator-provisioned access: direct grants, and operator-created groups /// without a . Operator groups are internal organisation — their names @@ -315,14 +346,17 @@ public class DelegationSelfService public Task> ListAssigneesAsync(string homeTenantId) => _delegatedAdmins.GetGroupAssigneesAsync(Constants.TenantGroupIds.ForHomeTenant(homeTenantId.ToLowerInvariant())); - public async Task> AssignAsync(string homeTenantId, string upn, string actor) + public Task> AssignAsync(string homeTenantId, string upn, string actor) + => AssignMemberAsync(homeTenantId, upn, actor, AssigneeReasons.SelfServiceAssigned); + + public async Task> AssignMemberAsync(string homeTenantId, string upn, string actor, string reason) { var home = homeTenantId.ToLowerInvariant(); var normalizedUpn = upn.ToLowerInvariant(); if (!(await _entitlements.GetEntitlementsAsync(home)).DelegatedAdminAllowed) return DelegationResult.Fail(403, Constants.DelegationCodes.DelegatedAdminNotAllowed, "Delegated administration is a Pro capability."); - if (await _adminRepo.GetTenantMemberAsync(home, normalizedUpn) == null) - return DelegationResult.Fail(400, Constants.DelegationCodes.NotATenantMember, "Only members of your own tenant (Access Management) can be assigned."); + if (!await IsEnabledMemberAsync(home, normalizedUpn)) + return NotAnEnabledMember(); var groupId = await EnsureOwnedGroupAsync(home); try @@ -336,11 +370,20 @@ public class DelegationSelfService return DelegationResult.Fail(409, Constants.DelegationCodes.IdentityBindingConflict, ex.Message); } + // Removing or disabling a member writes the member row first and then ends the assignment; this side writes + // the assignment first and then reads the member again. Whichever runs second sees the other's write, so an + // assignment racing the member's removal never outlives it. + if (!await IsEnabledMemberAsync(home, normalizedUpn)) + { + await _delegatedAdmins.UnassignGroupAsync(normalizedUpn, groupId); + return NotAnEnabledMember(); + } + var tenants = await _adminRepo.GetGroupTenantsAsync(groupId); foreach (var tenant in tenants) { await _audit.LogAuditEntryAsync(tenant, "CREATE", AuditGroupAccess, normalizedUpn, actor.ToLowerInvariant(), - new Dictionary { { "GroupId", groupId }, { "Role", Constants.DelegatedRoles.DelegatedReader }, { "Reason", "self-service-assigned" }, { "HomeTenantId", home } }); + new Dictionary { { "GroupId", groupId }, { "Role", Constants.DelegatedRoles.DelegatedReader }, { "Reason", reason }, { "HomeTenantId", home } }); } var assignment = (await _delegatedAdmins.GetGroupAssigneesAsync(groupId)).FirstOrDefault(a => a.Upn == normalizedUpn) ?? new TenantGroupAssignment { Upn = normalizedUpn, GroupId = groupId, Role = Constants.DelegatedRoles.DelegatedReader, IsEnabled = true, AssignedBy = actor.ToLowerInvariant() }; @@ -348,22 +391,36 @@ public class DelegationSelfService } public async Task> UnassignAsync(string homeTenantId, string upn, string actor) + { + if (!await EndAssignmentAsync(homeTenantId, upn, actor, AssigneeReasons.SelfServiceUnassigned)) + return DelegationResult.Fail(404, Constants.DelegationCodes.AssigneeNotFound, "That person is not assigned."); + + await _signalR.DisconnectUserAsync(upn.ToLowerInvariant()); + return DelegationResult.Success(true); + } + + public async Task EndAssignmentAsync(string homeTenantId, string upn, string actor, string reason) { var home = homeTenantId.ToLowerInvariant(); var normalizedUpn = upn.ToLowerInvariant(); var groupId = Constants.TenantGroupIds.ForHomeTenant(home); if (!await _delegatedAdmins.UnassignGroupAsync(normalizedUpn, groupId)) - return DelegationResult.Fail(404, Constants.DelegationCodes.AssigneeNotFound, "That person is not assigned."); + return false; - await _signalR.DisconnectUserAsync(normalizedUpn); foreach (var tenant in await _adminRepo.GetGroupTenantsAsync(groupId)) { await _audit.LogAuditEntryAsync(tenant, "DELETE", AuditGroupAccess, normalizedUpn, actor.ToLowerInvariant(), - new Dictionary { { "GroupId", groupId }, { "Reason", "self-service-unassigned" }, { "HomeTenantId", home } }); + new Dictionary { { "GroupId", groupId }, { "Reason", reason }, { "HomeTenantId", home } }); } - return DelegationResult.Success(true); + return true; } + private async Task IsEnabledMemberAsync(string tenantId, string upn) + => (await _adminRepo.GetTenantMemberAsync(tenantId, upn))?.IsEnabled == true; + + private static DelegationResult NotAnEnabledMember() + => DelegationResult.Fail(400, Constants.DelegationCodes.NotATenantMember, "Only enabled members of the managing tenant (Access Management) can be assigned."); + // ── Managers (managed tenant's side) ───────────────────────────────────────── public async Task> ListManagersAsync(string managedTenantId) diff --git a/src/Backend/AutopilotMonitor.Functions/Services/Offboarding/SafeWipeService.cs b/src/Backend/AutopilotMonitor.Functions/Services/Offboarding/SafeWipeService.cs index 87528d7c..95211c56 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/Offboarding/SafeWipeService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/Offboarding/SafeWipeService.cs @@ -176,7 +176,7 @@ namespace AutopilotMonitor.Functions.Services.Offboarding /// /// Variant D for the tenant's own self-service Tenant Group: rows whose - /// RowKey == "msp-{tenantId}" (TenantGroupAssignments: PK=managed tenant). Takes + /// RowKey == "msp-{tenantId}" (TenantGroupAssignments: PK=assignee UPN). Takes /// the tenant GUID and derives the group id itself. /// public Task WipeByOwnedGroupRowKeyAsync( diff --git a/src/Backend/AutopilotMonitor.Functions/Services/SignalRNotificationService.cs b/src/Backend/AutopilotMonitor.Functions/Services/SignalRNotificationService.cs index 817a1fde..67e4714b 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/SignalRNotificationService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/SignalRNotificationService.cs @@ -302,7 +302,7 @@ namespace AutopilotMonitor.Functions.Services /// Management SDK (1.33) exposes no user-level connection close: (1) strip the user from all /// groups server-side, and (2) push "accessRevoked" to the user's connections — the web /// client restarts its connection on receipt, and every re-join then re-runs authorization - /// against the fresh (≤30s-cached) scope. Residual: a tampered client that ignores (2) and + /// (a delegated cross-tenant join against an uncached scope). Residual: a tampered client that ignores (2) and /// whose connection-scoped memberships survive (1) keeps its streams until the connection /// drops. Warning (not Debug) on failure so it reaches App Insights. /// diff --git a/src/Shared/AutopilotMonitor.Shared/Constants.cs b/src/Shared/AutopilotMonitor.Shared/Constants.cs index 461ec253..464077bb 100644 --- a/src/Shared/AutopilotMonitor.Shared/Constants.cs +++ b/src/Shared/AutopilotMonitor.Shared/Constants.cs @@ -1378,13 +1378,21 @@ namespace AutopilotMonitor.Shared /// /// Deterministic ids of the implicit self-service Tenant Group every managing (MSP) tenant owns: /// msp-{homeTenantId}. Its members are the customers that accepted an invitation; its assignees are - /// the managing tenant's own users. Ordinary group to every consumer (scope resolution, middleware, MCP). + /// the managing tenant's own users, and an assignment confers scope only while its assignee is an enabled + /// member of that tenant (checked at every scope resolution). Operator-created groups carry GUID ids and + /// never match the prefix. /// public static class TenantGroupIds { public const string SelfServicePrefix = "msp-"; public static string ForHomeTenant(string homeTenantId) => SelfServicePrefix + homeTenantId.ToLowerInvariant(); public static bool IsSelfService(string? groupId) => groupId != null && groupId.StartsWith(SelfServicePrefix, System.StringComparison.Ordinal); + + /// The managing tenant that owns a self-service group (the inverse of ), or null for any other group id. + public static string? OwnerOf(string? groupId) + => IsSelfService(groupId) && groupId!.Length > SelfServicePrefix.Length + ? groupId.Substring(SelfServicePrefix.Length) + : null; } /// Machine-readable code values of the self-service delegation 4xx bodies. @@ -1519,7 +1527,8 @@ namespace AutopilotMonitor.Shared // Tenant Group assignments — which delegated-admin UPN is assigned to which group. // PK=UPN (lowercase), RK=groupId. Hot path: GetScopeAsync point-scans by PK to expand the - // UPN's groups into the effective tenant set. NOT tenant-id-keyed (offboarding never touches it). + // UPN's groups into the effective tenant set. NOT tenant-id-keyed: offboarding removes only the + // assignments to the tenant's own self-service group (RowKey == "msp-{tenantId}"). public const string TenantGroupAssignments = "TenantGroupAssignments"; // Self-service delegation invitations + slot release holds of a managing (MSP) tenant. diff --git a/src/Web/autopilot-monitor-web/app/admin/settings/sections/SectionTenantGroups.tsx b/src/Web/autopilot-monitor-web/app/admin/settings/sections/SectionTenantGroups.tsx index d88d91d3..0d6ba4c4 100644 --- a/src/Web/autopilot-monitor-web/app/admin/settings/sections/SectionTenantGroups.tsx +++ b/src/Web/autopilot-monitor-web/app/admin/settings/sections/SectionTenantGroups.tsx @@ -231,7 +231,8 @@ export function SectionTenantGroups() { async (t: TenantGroup) => { const upn = (assignUpn[t.groupId] || "").trim(); if (!canAssign(t.groupId)) return; - const role = assignRole[t.groupId] || "DelegatedReader"; + // A self-service group is read-only for every assignee; the backend refuses any other role there. + const role = t.ownerTenantId ? "DelegatedReader" : assignRole[t.groupId] || "DelegatedReader"; const ok = await mutate( `assign:${t.groupId}`, api.tenantGroups.assign(t.groupId), @@ -507,14 +508,16 @@ export function SectionTenantGroups() { autoComplete="off" className="w-full sm:w-auto sm:flex-1 min-w-0 px-3 py-1.5 border border-gray-300 rounded-lg text-sm text-gray-900 placeholder-gray-500 focus:outline-none focus:ring-2 focus:ring-sky-500 transition-colors" /> - + {!t.ownerTenantId && ( + + )} {user?.upn && assignableUpns.length === 0 && (assignees?.assignees.length ?? 0) === 0 && ( -

Add members under Access Management first.

+

Add or enable members under Access Management first.

)}