From 6d1ff8d7f0687efe2efd4baaa38b824196dcbd8e Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Mon, 20 Jul 2026 13:13:26 +0200 Subject: [PATCH] fix: code-review follow-ups (watcher dispose race, revoke-enforcement tests, scan guard) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ConsoleBypassWatcher: re-check _disposed after watcher.Start() — a Dispose() racing the background arm could leave an armed WMI subscription no code path could ever stop (leaked until process exit). - SignalRContext: collapse rapid accessRevoked pushes into one restart via an in-flight guard — a second start() on a non-Disconnected connection throws and burns the retry budget. - config/all: negative-cache managed ids a rescue scan proved missing (5 min TTL) — one stale grant (offboarded tenant) no longer makes every delegated config/all call pay a full table scan. - DelegatedAdmin/TenantGroup functions: extract testable revoke-enforcement cores (RevokeCoreAsync, SetEnabledCoreAsync, UnassignCoreAsync, DeleteGroupCoreAsync, ValidateGrantRequest) and cover the security flows: 404-on-missing-row writes no audit row and fires no disconnect; real revoke/disable/unassign/group-delete always cuts live streams; grant validation is fail-closed (GUID tenantId, recognized roles only). - dashboard/page.tsx: drop unused next/link import left by the banner removal. Tests: backend 3099 passed, web 557 passed, agent ConsoleBypassWatcher suite passed, tsc clean. Co-Authored-By: Claude Fable 5 --- .../Security/ConsoleBypassWatcher.cs | 19 ++ ...GetAllTenantConfigurationsFunctionTests.cs | 26 ++ .../RevokeEnforcementTests.cs | 269 ++++++++++++++++++ .../Admin/DelegatedAdminManagementFunction.cs | 101 ++++--- .../Admin/TenantGroupManagementFunction.cs | 50 ++-- .../GetAllTenantConfigurationsFunction.cs | 44 ++- .../app/dashboard/page.tsx | 1 - .../contexts/SignalRContext.tsx | 13 +- 8 files changed, 470 insertions(+), 53 deletions(-) create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/RevokeEnforcementTests.cs diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Telemetry/Security/ConsoleBypassWatcher.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Telemetry/Security/ConsoleBypassWatcher.cs index f427cf94..4bc2e36a 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Telemetry/Security/ConsoleBypassWatcher.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Telemetry/Security/ConsoleBypassWatcher.cs @@ -136,6 +136,25 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Telemetry.Security } watcher.Start(); + + // Re-check after arming: Dispose() may have run between the lock release above and + // Start() — it then stopped a not-yet-armed watcher and this thread armed it right + // back, leaving a WMI subscription no code path could ever reach again. If disposal + // won that race, tear the watcher down here. + bool disposedDuringArm; + lock (_lock) + { + disposedDuringArm = _disposed; + if (disposedDuringArm) _startWatcher = null; + } + if (disposedDuringArm) + { + try { watcher.EventArrived -= OnProcessStartTrace; } catch { } + try { watcher.Stop(); } catch { } + try { watcher.Dispose(); } catch { } + return; + } + _logger.Info($"[ConsoleBypassWatcher] watching Win32_ProcessStartTrace for " + $"{string.Join(", ", WatchedProcessNames)} (interactive-session + bare command line)"); } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/GetAllTenantConfigurationsFunctionTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/GetAllTenantConfigurationsFunctionTests.cs index 8cfec54a..4905abe0 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/GetAllTenantConfigurationsFunctionTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/GetAllTenantConfigurationsFunctionTests.cs @@ -129,4 +129,30 @@ public class GetAllTenantConfigurationsFunctionTests [Fact] public void DelegatedBareArrayView_Empty_Empty() => Assert.Empty(GetAllTenantConfigurationsFunction.DelegatedBareArrayView(new TenantConfiguration[0])); + + [Fact] + public void MissingManagedIds_ReturnsOnlyIdsWithoutConfig_CaseInsensitive() + { + var configs = new List + { + new() { TenantId = TenantB.ToUpperInvariant() }, // case variant still counts as present + }; + + var missing = GetAllTenantConfigurationsFunction.MissingManagedIds(new[] { TenantB, TenantC }, configs); + + var only = Assert.Single(missing); + Assert.Equal(TenantC, only); + } + + [Fact] + public void MissingManagedIds_AllPresent_Empty() + => Assert.Empty(GetAllTenantConfigurationsFunction.MissingManagedIds( + new[] { TenantB }, + new List { new() { TenantId = TenantB } })); + + [Fact] + public void MissingConfigCacheKey_LowercasesId_SoCaseVariantsShareOneVerdict() + => Assert.Equal( + GetAllTenantConfigurationsFunction.MissingConfigCacheKey(TenantB), + GetAllTenantConfigurationsFunction.MissingConfigCacheKey(TenantB.ToUpperInvariant())); } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/RevokeEnforcementTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/RevokeEnforcementTests.cs new file mode 100644 index 00000000..fdab1fef --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/RevokeEnforcementTests.cs @@ -0,0 +1,269 @@ +using AutopilotMonitor.Functions.Functions.Admin; +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared; +using AutopilotMonitor.Shared.DataAccess; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Covers the SignalR revoke-enforcement cores of and +/// (extracted seams — the HTTP entry points are intentionally +/// not exercised, same rationale as ). Locks in three +/// security invariants: (1) a miss (no assignment row) neither audits nor disconnects — a typo must not +/// write false customer-visible "access removed" rows while the real grant stays live; (2) a real +/// revoke/disable/unassign/group-delete always cuts the affected UPN's live streams (group authorization +/// is join-time only); (3) grant validation is fail-closed (GUID tenantId, recognized roles only). +/// +public class RevokeEnforcementTests +{ + private const string Upn = "MSP-Admin@Partner.Example"; + private const string UpnLower = "msp-admin@partner.example"; + private const string TenantA = "11111111-1111-1111-1111-111111111111"; + private const string TenantB = "22222222-2222-2222-2222-222222222222"; + private const string GroupId = "tpl-aaaaaaaa"; + + private static DelegatedAdminService BuildService(Mock repo) => + new( + repo.Object, + new StubTenantEntitlementService(AutopilotMonitor.Functions.Security.TenantEdition.Enterprise), + new MemoryCache(new MemoryCacheOptions()), + NullLogger.Instance); + + private static Mock BuildAuditRepo() + { + var audit = new Mock(); + audit.Setup(a => a.LogAuditEntryAsync( + It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny?>())) + .ReturnsAsync(true); + return audit; + } + + private static void VerifyNoAudit(Mock audit) => + audit.Verify(a => a.LogAuditEntryAsync( + It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny?>()), Times.Never); + + private static (DelegatedAdminManagementFunction Fn, Mock Repo, + Mock Audit, FakeSignalRNotificationService SignalR) BuildAdminFn() + { + var repo = new Mock(); + var audit = BuildAuditRepo(); + var signalR = new FakeSignalRNotificationService(); + var fn = new DelegatedAdminManagementFunction( + NullLogger.Instance, BuildService(repo), audit.Object, signalR); + return (fn, repo, audit, signalR); + } + + private static (TenantGroupManagementFunction Fn, Mock Repo, + Mock Audit, FakeSignalRNotificationService SignalR) BuildGroupFn() + { + var repo = new Mock(); + // Loose-mock defaults return a null Task — every repo read the flows touch needs a concrete setup. + repo.Setup(r => r.GetTenantGroupAsync(It.IsAny())).ReturnsAsync((TenantGroup?)null); + repo.Setup(r => r.GetGroupAssignmentsForUpnAsync(It.IsAny())) + .ReturnsAsync(new List()); + repo.Setup(r => r.GetGroupAssigneesAsync(It.IsAny())) + .ReturnsAsync(new List()); + var audit = BuildAuditRepo(); + var signalR = new FakeSignalRNotificationService(); + var fn = new TenantGroupManagementFunction( + NullLogger.Instance, BuildService(repo), audit.Object, signalR); + return (fn, repo, audit, signalR); + } + + // --- Delegated grant validation (fail-closed) --- + + [Fact] + public void ValidateGrant_NullBody_Rejected() + => Assert.Equal("upn and tenantId are required", + DelegatedAdminManagementFunction.ValidateGrantRequest(null, out _)); + + [Theory] + [InlineData("", TenantA)] + [InlineData(" ", TenantA)] + [InlineData(Upn, "")] + public void ValidateGrant_MissingFields_Rejected(string upn, string tenantId) + => Assert.Equal("upn and tenantId are required", + DelegatedAdminManagementFunction.ValidateGrantRequest( + new GrantDelegatedAdminRequest { Upn = upn, TenantId = tenantId }, out _)); + + [Theory] + [InlineData("contoso.com")] + [InlineData("not-a-guid")] + [InlineData("11111111-1111-1111-1111")] + public void ValidateGrant_NonGuidTenantId_Rejected(string tenantId) + => Assert.Equal("a valid tenantId (GUID) is required", + DelegatedAdminManagementFunction.ValidateGrantRequest( + new GrantDelegatedAdminRequest { Upn = Upn, TenantId = tenantId }, out _)); + + [Fact] + public void ValidateGrant_UnknownRole_Rejected() + { + var error = DelegatedAdminManagementFunction.ValidateGrantRequest( + new GrantDelegatedAdminRequest { Upn = Upn, TenantId = TenantA, Role = "GlobalAdmin" }, out _); + Assert.NotNull(error); + Assert.Contains(Constants.DelegatedRoles.DelegatedReader, error); + } + + [Fact] + public void ValidateGrant_EmptyRole_DefaultsToLeastPrivilegedReader() + { + Assert.Null(DelegatedAdminManagementFunction.ValidateGrantRequest( + new GrantDelegatedAdminRequest { Upn = Upn, TenantId = TenantA }, out var role)); + Assert.Equal(Constants.DelegatedRoles.DelegatedReader, role); + } + + [Fact] + public void ValidateGrant_ExplicitDelegatedAdmin_Accepted() + { + Assert.Null(DelegatedAdminManagementFunction.ValidateGrantRequest( + new GrantDelegatedAdminRequest { Upn = Upn, TenantId = TenantA, Role = Constants.DelegatedRoles.DelegatedAdmin }, + out var role)); + Assert.Equal(Constants.DelegatedRoles.DelegatedAdmin, role); + } + + // --- Revoke: 404-on-missing-row vs. audit + disconnect --- + + [Fact] + public async Task RevokeCore_MissingRow_NoAuditNoDisconnect() + { + var (fn, repo, audit, signalR) = BuildAdminFn(); + repo.Setup(r => r.RemoveDelegatedAdminAsync(UpnLower, TenantA)).ReturnsAsync(false); + + var removed = await fn.RevokeCoreAsync(Upn, TenantA, "ga@vendor.example"); + + Assert.False(removed); + VerifyNoAudit(audit); + Assert.Empty(signalR.DisconnectedUsers); + } + + [Fact] + public async Task RevokeCore_RemovedRow_AuditsAndDisconnectsLowercasedUpn() + { + var (fn, repo, audit, signalR) = BuildAdminFn(); + repo.Setup(r => r.RemoveDelegatedAdminAsync(UpnLower, TenantA)).ReturnsAsync(true); + + var removed = await fn.RevokeCoreAsync(Upn, TenantA, "ga@vendor.example"); + + Assert.True(removed); + // SignalR user ids are lowercased UPNs — a mixed-case route param must still hit the user's connections. + Assert.Equal(new[] { UpnLower }, signalR.DisconnectedUsers); + audit.Verify(a => a.LogAuditEntryAsync( + TenantA, "DELETE", "DelegatedAdmin", UpnLower, "ga@vendor.example", null), Times.Once); + } + + // --- Disable is a revocation too; enable never disconnects --- + + [Fact] + public async Task SetEnabledCore_MissingRow_NoAuditNoDisconnect() + { + var (fn, repo, audit, signalR) = BuildAdminFn(); + repo.Setup(r => r.SetDelegatedAdminEnabledAsync(UpnLower, TenantA, false)).ReturnsAsync(false); + + var ok = await fn.SetEnabledCoreAsync(Upn, TenantA, isEnabled: false, "ga@vendor.example"); + + Assert.False(ok); + VerifyNoAudit(audit); + Assert.Empty(signalR.DisconnectedUsers); + } + + [Fact] + public async Task SetEnabledCore_Disable_Disconnects() + { + var (fn, repo, _, signalR) = BuildAdminFn(); + repo.Setup(r => r.SetDelegatedAdminEnabledAsync(UpnLower, TenantA, false)).ReturnsAsync(true); + + var ok = await fn.SetEnabledCoreAsync(Upn, TenantA, isEnabled: false, "ga@vendor.example"); + + Assert.True(ok); + Assert.Equal(new[] { UpnLower }, signalR.DisconnectedUsers); + } + + [Fact] + public async Task SetEnabledCore_Enable_DoesNotDisconnect() + { + var (fn, repo, _, signalR) = BuildAdminFn(); + repo.Setup(r => r.SetDelegatedAdminEnabledAsync(UpnLower, TenantA, true)).ReturnsAsync(true); + + var ok = await fn.SetEnabledCoreAsync(Upn, TenantA, isEnabled: true, "ga@vendor.example"); + + Assert.True(ok); + Assert.Empty(signalR.DisconnectedUsers); + } + + // --- Group unassign: 404-on-not-assigned vs. disconnect + per-tenant audit --- + + [Fact] + public async Task UnassignCore_NotAssigned_NoAuditNoDisconnect() + { + var (fn, _, audit, signalR) = BuildGroupFn(); // default: no assignments for the UPN + + var unassigned = await fn.UnassignCoreAsync(GroupId, Upn, "ga@vendor.example"); + + Assert.False(unassigned); + VerifyNoAudit(audit); + Assert.Empty(signalR.DisconnectedUsers); + } + + [Fact] + public async Task UnassignCore_Assigned_DisconnectsAndAuditsPerGroupTenant() + { + var (fn, repo, audit, signalR) = BuildGroupFn(); + repo.Setup(r => r.GetTenantGroupAsync(GroupId)).ReturnsAsync(new TenantGroup + { + GroupId = GroupId, + Name = "MSP Customers", + TenantIds = new List { TenantA, TenantB }, + AssigneeCount = 1, + }); + repo.Setup(r => r.GetGroupAssignmentsForUpnAsync(UpnLower)) + .ReturnsAsync(new List { new() { Upn = UpnLower, GroupId = GroupId } }); + repo.Setup(r => r.UnassignGroupAsync(UpnLower, GroupId)).ReturnsAsync(true); + + var unassigned = await fn.UnassignCoreAsync(GroupId, Upn, "ga@vendor.example"); + + Assert.True(unassigned); + Assert.Equal(new[] { UpnLower }, signalR.DisconnectedUsers); + // Lost access is audited under EACH of the group's tenants (customer-visible trail). + audit.Verify(a => a.LogAuditEntryAsync( + TenantA, "DELETE", "DelegatedGroupAccess", UpnLower, "ga@vendor.example", + It.IsAny?>()), Times.Once); + audit.Verify(a => a.LogAuditEntryAsync( + TenantB, "DELETE", "DelegatedGroupAccess", UpnLower, "ga@vendor.example", + It.IsAny?>()), Times.Once); + } + + // --- Group delete: every (former) assignee's streams are cut --- + + [Fact] + public async Task DeleteGroupCore_DisconnectsEveryAssignee_AndAuditsPerTenant() + { + var (fn, repo, audit, signalR) = BuildGroupFn(); + var assignees = new List + { + new() { Upn = "one@partner.example", GroupId = GroupId }, + new() { Upn = "two@partner.example", GroupId = GroupId }, + }; + repo.Setup(r => r.GetTenantGroupAsync(GroupId)).ReturnsAsync(new TenantGroup + { + GroupId = GroupId, + Name = "MSP Customers", + TenantIds = new List { TenantA }, + AssigneeCount = assignees.Count, + }); + repo.Setup(r => r.GetGroupAssigneesAsync(GroupId)).ReturnsAsync(assignees); + repo.Setup(r => r.DeleteTenantGroupAsync(GroupId)).ReturnsAsync(true); + + await fn.DeleteGroupCoreAsync(GroupId, "ga@vendor.example"); + + Assert.Equal(new[] { "one@partner.example", "two@partner.example" }, signalR.DisconnectedUsers); + audit.Verify(a => a.LogAuditEntryAsync( + TenantA, "DELETE", "DelegatedGroupAccess", "*", "ga@vendor.example", + It.IsAny?>()), Times.Once); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/DelegatedAdminManagementFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/DelegatedAdminManagementFunction.cs index 3244f90e..5240b5d9 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/DelegatedAdminManagementFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/DelegatedAdminManagementFunction.cs @@ -64,19 +64,9 @@ public class DelegatedAdminManagementFunction var currentUpn = context.GetRequestContext().UserPrincipalName; var body = await req.ReadFromJsonAsync(); - if (body == null || string.IsNullOrWhiteSpace(body.Upn) || string.IsNullOrWhiteSpace(body.TenantId)) - return await Bad(req, "upn and tenantId are required"); - - // Mirror the tenant-group path: a mistyped tenantId must not create a garbage scope entry - // that pollutes the allow-list and seeds a per-tenant audit trail keyed on the typo. - if (!Guid.TryParse(body.TenantId, out _)) - return await Bad(req, "a valid tenantId (GUID) is required"); - - // Default to the least-privileged role; reject anything we don't recognize (fail-closed) so a typo - // can never silently widen access. - var role = string.IsNullOrWhiteSpace(body.Role) ? Constants.DelegatedRoles.DelegatedReader : body.Role; - if (role != Constants.DelegatedRoles.DelegatedReader && role != Constants.DelegatedRoles.DelegatedAdmin) - return await Bad(req, $"role must be '{Constants.DelegatedRoles.DelegatedReader}' or '{Constants.DelegatedRoles.DelegatedAdmin}'"); + var validationError = ValidateGrantRequest(body, out var role); + if (validationError != null) + return await Bad(req, validationError); // Edition note: TARGET tenants may be any edition — an MSP on Enterprise may manage Community // customers. The Enterprise requirement applies to the delegated admin's HOME tenant and is @@ -84,7 +74,7 @@ public class DelegatedAdminManagementFunction // be checked here: at grant time only the UPN is known, and UPN-domain → tenant mapping is not // reliable (multi-domain tenants). Grants to non-Enterprise-homed admins are simply inert. var entry = await _delegatedAdminService.UpsertAsync( - body.Upn, body.TenantId, role, + body!.Upn, body.TenantId, role, Constants.DelegatedStatus.Active, Constants.DelegatedSource.OperatorGranted, currentUpn ?? ""); await _maintenanceRepo.LogAuditEntryAsync( @@ -112,10 +102,7 @@ public class DelegatedAdminManagementFunction string upn, string tenantId, FunctionContext context) { var currentUpn = context.GetRequestContext().UserPrincipalName; - // The service reports whether a row was actually deleted — a mistyped upn/tenantId must not - // 200 and write a false customer-visible "access removed" audit row while the real grant - // stays live (mirrors the tenant-group revoke semantics). - var removed = await _delegatedAdminService.RemoveAsync(upn, tenantId); + var removed = await RevokeCoreAsync(upn, tenantId, currentUpn); if (!removed) { var notFound = req.CreateResponse(HttpStatusCode.NotFound); @@ -123,20 +110,34 @@ public class DelegatedAdminManagementFunction return notFound; } - await _maintenanceRepo.LogAuditEntryAsync( - tenantId.ToLowerInvariant(), "DELETE", "DelegatedAdmin", upn.ToLowerInvariant(), currentUpn ?? ""); - - // Enforcement: cut the revoked caller's already-joined live streams — group authorization is - // join-time only, so without this they keep receiving tenant telemetry until the connection drops. - await _signalRService.DisconnectUserAsync(upn.ToLowerInvariant()); - - _logger.LogInformation("Delegated revoke: {Upn} -> {TenantId} by {By}", upn, tenantId, currentUpn); - var response = req.CreateResponse(HttpStatusCode.OK); await response.WriteAsJsonAsync(new { message = "Delegated assignment revoked" }); return response; } + /// + /// Testable core of the revoke flow (no HTTP plumbing — see GetAllBlockedDevicesFunctionTests for the + /// convention). Returns false when no assignment row existed (caller 404s): a mistyped upn/tenantId + /// must not 200 and write a false customer-visible "access removed" audit row while the real grant + /// stays live (mirrors the tenant-group revoke semantics). On a real removal writes the audit entry + /// and cuts the revoked UPN's live streams — group authorization is join-time only, so without the + /// kick they keep receiving tenant telemetry until the connection drops. + /// + internal async Task RevokeCoreAsync(string upn, string tenantId, string? currentUpn) + { + var removed = await _delegatedAdminService.RemoveAsync(upn, tenantId); + if (!removed) + return false; + + await _maintenanceRepo.LogAuditEntryAsync( + tenantId.ToLowerInvariant(), "DELETE", "DelegatedAdmin", upn.ToLowerInvariant(), currentUpn ?? ""); + + await _signalRService.DisconnectUserAsync(upn.ToLowerInvariant()); + + _logger.LogInformation("Delegated revoke: {Upn} -> {TenantId} by {By}", upn, tenantId, currentUpn); + return true; + } + /// PATCH /api/global/delegated-admins/{upn}/{tenantId}/enable — re-enable a disabled assignment. GlobalAdminOnly. [Function("EnableDelegatedAdmin")] [Authorize] @@ -157,7 +158,7 @@ public class DelegatedAdminManagementFunction HttpRequestData req, string upn, string tenantId, bool isEnabled, FunctionContext context) { var currentUpn = context.GetRequestContext().UserPrincipalName; - var ok = await _delegatedAdminService.SetEnabledAsync(upn, tenantId, isEnabled); + var ok = await SetEnabledCoreAsync(upn, tenantId, isEnabled, currentUpn); if (!ok) { var notFound = req.CreateResponse(HttpStatusCode.NotFound); @@ -165,20 +166,56 @@ public class DelegatedAdminManagementFunction return notFound; } + var response = req.CreateResponse(HttpStatusCode.OK); + await response.WriteAsJsonAsync(new { message = isEnabled ? "Delegated assignment enabled" : "Delegated assignment disabled" }); + return response; + } + + /// + /// Testable core of enable/disable (no HTTP plumbing). Returns false when no assignment row + /// existed (caller 404s — no audit, no disconnect). Disable is a revocation too, so it cuts the + /// UPN's already-joined live streams (see ); enable never disconnects. + /// + internal async Task SetEnabledCoreAsync(string upn, string tenantId, bool isEnabled, string? currentUpn) + { + var ok = await _delegatedAdminService.SetEnabledAsync(upn, tenantId, isEnabled); + if (!ok) + return false; + await _maintenanceRepo.LogAuditEntryAsync( tenantId.ToLowerInvariant(), "UPDATE", "DelegatedAdmin", upn.ToLowerInvariant(), currentUpn ?? "", new Dictionary { { "IsEnabled", isEnabled.ToString() } }); - // Disable is a revocation too — cut already-joined live streams (see RevokeDelegatedAdmin). if (!isEnabled) await _signalRService.DisconnectUserAsync(upn.ToLowerInvariant()); _logger.LogInformation("Delegated {Action}: {Upn} -> {TenantId} by {By}", isEnabled ? "enable" : "disable", upn, tenantId, currentUpn); + return true; + } - var response = req.CreateResponse(HttpStatusCode.OK); - await response.WriteAsJsonAsync(new { message = isEnabled ? "Delegated assignment enabled" : "Delegated assignment disabled" }); - return response; + /// + /// Pure grant-request validation (testable seam). Returns the 400 error message, or null when valid — + /// then carries the effective role. A mistyped tenantId must not create a + /// garbage scope entry that pollutes the allow-list and seeds a per-tenant audit trail keyed on the + /// typo; the role defaults to least-privileged and unknown roles are rejected (fail-closed) so a typo + /// can never silently widen access. + /// + internal static string? ValidateGrantRequest(GrantDelegatedAdminRequest? body, out string role) + { + role = Constants.DelegatedRoles.DelegatedReader; + + if (body == null || string.IsNullOrWhiteSpace(body.Upn) || string.IsNullOrWhiteSpace(body.TenantId)) + return "upn and tenantId are required"; + + if (!Guid.TryParse(body.TenantId, out _)) + return "a valid tenantId (GUID) is required"; + + role = string.IsNullOrWhiteSpace(body.Role) ? Constants.DelegatedRoles.DelegatedReader : body.Role!; + if (role != Constants.DelegatedRoles.DelegatedReader && role != Constants.DelegatedRoles.DelegatedAdmin) + return $"role must be '{Constants.DelegatedRoles.DelegatedReader}' or '{Constants.DelegatedRoles.DelegatedAdmin}'"; + + return null; } private static async Task Bad(HttpRequestData req, string error) diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs index 31de5fcc..ec39364c 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs @@ -112,15 +112,26 @@ public class TenantGroupManagementFunction string groupId, FunctionContext context) { var currentUpn = context.GetRequestContext().UserPrincipalName; + await DeleteGroupCoreAsync(groupId, currentUpn); - // Capture tenants + assignees BEFORE the cascade delete removes them, so we can audit the - // bulk access removal under each affected tenant and cut every assignee's live streams. + var response = req.CreateResponse(HttpStatusCode.OK); + await response.WriteAsJsonAsync(new { message = "Group deleted" }); + return response; + } + + /// + /// Testable core of the group cascade delete (no HTTP plumbing). Captures tenants + assignees + /// BEFORE the delete removes them, so the bulk access removal is audited under each affected + /// tenant and EVERY (former) assignee's live streams are cut — group authorization is join-time + /// only, so without the kick they keep receiving the managed tenants' telemetry until their + /// connection drops. + /// + internal async Task DeleteGroupCoreAsync(string groupId, string? currentUpn) + { var group = await _delegatedAdminService.GetGroupAsync(groupId); var assignees = await _delegatedAdminService.GetGroupAssigneesAsync(groupId); await _delegatedAdminService.DeleteGroupAsync(groupId); - // Enforcement: group authorization is join-time only — without the kick, every (former) - // assignee keeps receiving the managed tenants' live telemetry until their connection drops. foreach (var assignee in assignees) await _signalRService.DisconnectUserAsync(assignee.Upn); @@ -137,10 +148,6 @@ public class TenantGroupManagementFunction } _logger.LogInformation("Tenant group deleted: {GroupId} by {By}", groupId, currentUpn); - - var response = req.CreateResponse(HttpStatusCode.OK); - await response.WriteAsJsonAsync(new { message = "Group deleted" }); - return response; } /// POST /api/global/tenant-groups/{groupId}/tenants — add a tenant. GlobalAdminOnly. Body: { "tenantId": "<guid>" }. @@ -271,17 +278,31 @@ public class TenantGroupManagementFunction string groupId, string upn, FunctionContext context) { var currentUpn = context.GetRequestContext().UserPrincipalName; + var unassigned = await UnassignCoreAsync(groupId, upn, currentUpn); + if (!unassigned) + return await NotFound(req); + + var response = req.CreateResponse(HttpStatusCode.OK); + await response.WriteAsJsonAsync(new { message = "Unassigned from group" }); + return response; + } + + /// + /// Testable core of the unassign flow (no HTTP plumbing). Returns false when the UPN was not + /// actually assigned (caller 404s) — so a mistyped UPN can't 200 and write false "access removed" + /// audit rows, and no disconnect fires. On a real unassign cuts the UPN's already-joined live + /// streams (join-time-only authz) and audits the lost access under each of the group's tenants. + /// + internal async Task UnassignCoreAsync(string groupId, string upn, string? currentUpn) + { var normalizedUpn = upn.ToLowerInvariant(); // Read the group's tenants (unchanged by unassign) so we can audit the UPN's lost access per tenant. var group = await _delegatedAdminService.GetGroupAsync(groupId); - // The service returns false when the UPN was not actually assigned — so a mistyped UPN can't 200 and - // write false "access removed" audit rows. Only audit after a real unassign. var unassigned = await _delegatedAdminService.UnassignGroupAsync(normalizedUpn, groupId); if (!unassigned) - return await NotFound(req); + return false; - // Enforcement: cut the unassigned caller's already-joined live streams (join-time-only authz). await _signalRService.DisconnectUserAsync(normalizedUpn); if (group != null) @@ -296,10 +317,7 @@ public class TenantGroupManagementFunction } _logger.LogInformation("Group {GroupId} unassigned from {Upn} by {By}", groupId, normalizedUpn, currentUpn); - - var response = req.CreateResponse(HttpStatusCode.OK); - await response.WriteAsJsonAsync(new { message = "Unassigned from group" }); - return response; + return true; } /// Logs one audit entry per tenant (the customer-visible "who can read my tenant" trail). diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Config/GetAllTenantConfigurationsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Config/GetAllTenantConfigurationsFunction.cs index 10236445..b73ed34e 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Config/GetAllTenantConfigurationsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Config/GetAllTenantConfigurationsFunction.cs @@ -10,6 +10,7 @@ using AutopilotMonitor.Functions.Services; using AutopilotMonitor.Shared.Models; using AutopilotMonitor.Shared.Pagination; using Microsoft.Azure.Functions.Worker; +using Microsoft.Extensions.Caching.Memory; using Microsoft.Azure.Functions.Worker.Http; using Microsoft.Extensions.Logging; @@ -17,15 +18,23 @@ namespace AutopilotMonitor.Functions.Functions.Config { public class GetAllTenantConfigurationsFunction { + // TTL for the "this managed id has NO config row in any casing" verdict a rescue scan proved. + // Bounds how long a tenant onboarded moments after that scan stays absent from a delegated + // caller's config/all (matches the positive config cache TTL in TenantConfigurationService). + private static readonly TimeSpan MissingConfigNegativeCacheDuration = TimeSpan.FromMinutes(5); + private readonly ILogger _logger; private readonly TenantConfigurationService _configService; + private readonly IMemoryCache _cache; public GetAllTenantConfigurationsFunction( ILogger logger, - TenantConfigurationService configService) + TenantConfigurationService configService, + IMemoryCache cache) { _logger = logger; _configService = configService; + _cache = cache; } [Function("GetAllTenantConfigurations")] @@ -73,9 +82,21 @@ namespace AutopilotMonitor.Functions.Functions.Config // (agent paths auto-create rows with the caller-supplied id verbatim) — the same caveat // the delegated sessions fan-out handles case-insensitively. A point-read is exact-case, // so any id it missed gets ONE scan+filter rescue pass (the pre-point-read behavior). - // Only stale grants or case-variant rows pay for it; the common path stays read-bounded. + // An id a previous scan already proved missing in ANY casing is negative-cached and + // skipped for the TTL — otherwise a single stale grant (offboarded / never-onboarded + // tenant) would make EVERY config/all call pay a full table scan. if (subset.Count < managedIds.Count) - subset = MergeRescuedConfigs(managedIds, subset, await _configService.GetAllConfigurationsAsync()); + { + var unproven = MissingManagedIds(managedIds, subset) + .Where(id => !_cache.TryGetValue(MissingConfigCacheKey(id), out _)) + .ToList(); + if (unproven.Count > 0) + { + subset = MergeRescuedConfigs(managedIds, subset, await _configService.GetAllConfigurationsAsync()); + foreach (var id in MissingManagedIds(unproven, subset)) + _cache.Set(MissingConfigCacheKey(id), true, MissingConfigNegativeCacheDuration); + } + } _logger.LogInformation("GetAllTenantConfigurations (delegated subset, {Count} tenants) by {User}", subset.Count, userIdentifier); @@ -177,6 +198,23 @@ namespace AutopilotMonitor.Functions.Functions.Config IEnumerable reads) => reads.Where(c => c != null).Select(c => c!).ToList(); + /// + /// The managed ids not present (case-insensitively) in — before the + /// rescue scan these are the point-read misses, after it the proven-missing ids to negative-cache. + /// Pure + testable. + /// + internal static List MissingManagedIds( + IEnumerable managedIds, IReadOnlyCollection configs) + { + var have = new HashSet(configs.Select(c => c.TenantId), StringComparer.OrdinalIgnoreCase); + return managedIds.Where(id => !have.Contains(id)).ToList(); + } + + /// Negative-cache key for a managed id a rescue scan proved has no config row. + /// Lowercased so case variants of the same grant share one verdict. + internal static string MissingConfigCacheKey(string tenantId) + => $"tenant-config-missing:{tenantId.ToLowerInvariant()}"; + /// /// Case-variant rescue for managed ids the exact-case point-reads missed: from the full config scan, /// add every config whose TenantId matches a managed id case-insensitively and is not already in the diff --git a/src/Web/autopilot-monitor-web/app/dashboard/page.tsx b/src/Web/autopilot-monitor-web/app/dashboard/page.tsx index 9a176e90..679906a4 100644 --- a/src/Web/autopilot-monitor-web/app/dashboard/page.tsx +++ b/src/Web/autopilot-monitor-web/app/dashboard/page.tsx @@ -1,7 +1,6 @@ "use client"; import { Suspense, useCallback, useEffect, useMemo, useRef, useState } from "react"; -import Link from "next/link"; import { useRouter, useSearchParams } from "next/navigation"; import { ProtectedRoute } from "../../components/ProtectedRoute"; import { GlobalAdminBanner } from "@/components/GlobalAdminBanner"; diff --git a/src/Web/autopilot-monitor-web/contexts/SignalRContext.tsx b/src/Web/autopilot-monitor-web/contexts/SignalRContext.tsx index 3420e2f4..f2b55114 100644 --- a/src/Web/autopilot-monitor-web/contexts/SignalRContext.tsx +++ b/src/Web/autopilot-monitor-web/contexts/SignalRContext.tsx @@ -31,6 +31,9 @@ export function SignalRProvider({ children }: { children: React.ReactNode }) { const [joinedGroups, setJoinedGroups] = useState([]); const retryCountRef = useRef(0); const retryTimeoutRef = useRef | null>(null); + // Collapses rapid successive accessRevoked pushes (one per revoked grant) into a single + // restart — a second start() on a non-Disconnected connection throws and burns the retry budget. + const revokeRestartInFlightRef = useRef(false); // Set in onreconnecting, read in onreconnected so we can report downtime as a measurement. const disconnectStartedAtRef = useRef(null); const maxRetries = 3; @@ -157,12 +160,20 @@ export function SignalRProvider({ children }: { children: React.ReactNode }) { // the next Connected transition — through the join endpoint, which re-runs authorization // against the fresh scope. Revoked groups 403; still-authorized streams recover automatically. newConnection.on('accessRevoked', async () => { + // Dropping an event that arrives mid-restart is safe: the rejoin after start() re-runs + // authorization against the CURRENT scope, which already reflects that later revoke. + if (revokeRestartInFlightRef.current) return; + revokeRestartInFlightRef.current = true; console.warn('[SignalR] accessRevoked received — restarting connection to re-authorize group memberships'); trackEvent('signalr_access_revoked'); try { await newConnection.stop(); } catch { /* proceed to restart regardless */ } - startConnection(); + try { + await startConnection(); + } finally { + revokeRestartInFlightRef.current = false; + } }); // Start as soon as the user is authenticated. The previous 2s setTimeout