diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/FakeSignalRNotificationService.cs b/src/Backend/AutopilotMonitor.Functions.Tests/FakeSignalRNotificationService.cs index fc833a7f..46833cf1 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/FakeSignalRNotificationService.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/FakeSignalRNotificationService.cs @@ -75,4 +75,19 @@ public class FakeSignalRNotificationService : ISignalRNotificationService GlobalDismissAllCount++; return Task.CompletedTask; } + + public List NegotiatedUserIds { get; } = new(); + public List DisconnectedUsers { get; } = new(); + + public Task<(string Url, string AccessToken)?> NegotiateClientAsync(string userId) + { + NegotiatedUserIds.Add(userId); + return Task.FromResult<(string Url, string AccessToken)?>(("https://fake.signalr.test/client", "fake-token")); + } + + public Task DisconnectUserAsync(string userId) + { + DisconnectedUsers.Add(userId); + return Task.CompletedTask; + } } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionHandlerOrderingTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionHandlerOrderingTests.cs index b5f6921f..aeece7d2 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionHandlerOrderingTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/SessionDeletionHandlerOrderingTests.cs @@ -306,6 +306,9 @@ public class SessionDeletionHandlerOrderingTests public Task SendGlobalNotificationAsync(object dto) => Task.CompletedTask; public Task SendGlobalNotificationDismissedAsync(string notificationId) => Task.CompletedTask; public Task SendGlobalNotificationDismissedAllAsync() => Task.CompletedTask; + public Task<(string Url, string AccessToken)?> NegotiateClientAsync(string userId) + => Task.FromResult<(string Url, string AccessToken)?>(null); + public Task DisconnectUserAsync(string userId) => Task.CompletedTask; } } } diff --git a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableAdminRepository.cs b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableAdminRepository.cs index ab28aa27..b1e50f9e 100644 --- a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableAdminRepository.cs +++ b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableAdminRepository.cs @@ -423,8 +423,11 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage { upn = upn.ToLowerInvariant(); tenantId = tenantId.ToLowerInvariant(); - await _delegatedAdminsTableClient.DeleteEntityAsync(upn, tenantId); - return true; + // DeleteEntityAsync is idempotent: a missing row does NOT throw, it returns the 404 + // response. Report whether a row was actually deleted so the revoke endpoint can 404 a + // typo instead of writing a false "access removed" audit entry. + var response = await _delegatedAdminsTableClient.DeleteEntityAsync(upn, tenantId); + return response.Status != 404; } // --- Tenant Groups (app-internal tenant bundles for delegated admins / "MSP mode") --- diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/DelegatedAdminManagementFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/DelegatedAdminManagementFunction.cs index 66d74088..13a27a61 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/DelegatedAdminManagementFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/DelegatedAdminManagementFunction.cs @@ -25,15 +25,18 @@ public class DelegatedAdminManagementFunction private readonly ILogger _logger; private readonly DelegatedAdminService _delegatedAdminService; private readonly IMaintenanceRepository _maintenanceRepo; + private readonly ISignalRNotificationService _signalRService; public DelegatedAdminManagementFunction( ILogger logger, DelegatedAdminService delegatedAdminService, - IMaintenanceRepository maintenanceRepo) + IMaintenanceRepository maintenanceRepo, + ISignalRNotificationService signalRService) { _logger = logger; _delegatedAdminService = delegatedAdminService; _maintenanceRepo = maintenanceRepo; + _signalRService = signalRService; } /// GET /api/global/delegated-admins — list every delegated assignment. GlobalReadOrAdmin. @@ -64,6 +67,11 @@ public class DelegatedAdminManagementFunction 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; @@ -99,11 +107,24 @@ public class DelegatedAdminManagementFunction string upn, string tenantId, FunctionContext context) { var currentUpn = context.GetRequestContext().UserPrincipalName; - await _delegatedAdminService.RemoveAsync(upn, tenantId); + // 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); + if (!removed) + { + var notFound = req.CreateResponse(HttpStatusCode.NotFound); + await notFound.WriteAsJsonAsync(new { error = "Delegated assignment not found" }); + 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); @@ -143,6 +164,10 @@ public class DelegatedAdminManagementFunction 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); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs index 5c9d3130..31de5fcc 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs @@ -30,17 +30,20 @@ public class TenantGroupManagementFunction private readonly ILogger _logger; private readonly DelegatedAdminService _delegatedAdminService; private readonly IMaintenanceRepository _maintenanceRepo; + private readonly ISignalRNotificationService _signalRService; private const string AuditEntity = "DelegatedGroupAccess"; public TenantGroupManagementFunction( ILogger logger, DelegatedAdminService delegatedAdminService, - IMaintenanceRepository maintenanceRepo) + IMaintenanceRepository maintenanceRepo, + ISignalRNotificationService signalRService) { _logger = logger; _delegatedAdminService = delegatedAdminService; _maintenanceRepo = maintenanceRepo; + _signalRService = signalRService; } /// GET /api/global/tenant-groups — list every group with tenants + assignee count. GlobalReadOrAdmin. @@ -110,11 +113,17 @@ public class TenantGroupManagementFunction { var currentUpn = context.GetRequestContext().UserPrincipalName; - // Capture tenants + assignee count BEFORE the cascade delete removes them, so we can audit the - // bulk access removal under each affected tenant. + // 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 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); + if (group != null && group.AssigneeCount > 0) { await AuditPerTenantAsync(group.TenantIds, "DELETE", "*", currentUpn, @@ -192,6 +201,10 @@ public class TenantGroupManagementFunction await _maintenanceRepo.LogAuditEntryAsync( normalizedTenantId, "DELETE", AuditEntity, assignee.Upn, currentUpn ?? "", new Dictionary { { "GroupId", groupId }, { "Reason", "tenant-removed-from-group" } }); + // Enforcement: cut already-joined live streams for the tenant that just left the group. + // Coarse by design (connection close, see DisconnectUserAsync) — still-authorized + // streams recover automatically via the client's reconnect + re-join. + await _signalRService.DisconnectUserAsync(assignee.Upn); } _logger.LogInformation("Tenant {TenantId} removed from group {GroupId} by {By}", normalizedTenantId, groupId, currentUpn); @@ -268,6 +281,9 @@ public class TenantGroupManagementFunction if (!unassigned) return await NotFound(req); + // Enforcement: cut the unassigned caller's already-joined live streams (join-time-only authz). + await _signalRService.DisconnectUserAsync(normalizedUpn); + if (group != null) { await AuditPerTenantAsync(group.TenantIds, "DELETE", normalizedUpn, currentUpn, diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRAddToGroupFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRAddToGroupFunction.cs index dd8b2ef1..84fcaac8 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRAddToGroupFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRAddToGroupFunction.cs @@ -91,7 +91,9 @@ namespace AutopilotMonitor.Functions.Functions.Infrastructure // gets the same live session/event pushes a GA gets for that tenant). The admin/member // notification-group checks below still gate by the real tenant role, so a delegated // caller (no tenant role) receives session/event broadcasts but NOT notification pushes. - if (requestedTenantId != userTenantId) + // OrdinalIgnoreCase, matching the middleware's cross-tenant check and the delegated + // scope dictionary — a casing difference must never flip which branch authorizes. + if (!string.Equals(requestedTenantId, userTenantId, StringComparison.OrdinalIgnoreCase)) { var allowedCrossTenant = requestCtx.HasGlobalScope; if (!allowedCrossTenant && !string.IsNullOrEmpty(userEmail)) diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRNegotiateFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRNegotiateFunction.cs index 15922efd..ab3a632c 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRNegotiateFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRNegotiateFunction.cs @@ -1,8 +1,8 @@ using System.Net; using AutopilotMonitor.Functions.Helpers; +using AutopilotMonitor.Functions.Services; using Microsoft.Azure.Functions.Worker; using Microsoft.Azure.Functions.Worker.Http; -using Microsoft.Azure.Functions.Worker.Extensions.SignalRService; using Microsoft.Extensions.Logging; namespace AutopilotMonitor.Functions.Functions.Infrastructure @@ -10,16 +10,19 @@ namespace AutopilotMonitor.Functions.Functions.Infrastructure public class SignalRNegotiateFunction { private readonly ILogger _logger; + private readonly ISignalRNotificationService _signalRService; - public SignalRNegotiateFunction(ILogger logger) + public SignalRNegotiateFunction( + ILogger logger, + ISignalRNotificationService signalRService) { _logger = logger; + _signalRService = signalRService; } [Function("negotiate")] public async Task Negotiate( - [HttpTrigger(AuthorizationLevel.Anonymous, "post", Route = "realtime/negotiate")] HttpRequestData req, - [SignalRConnectionInfoInput(HubName = "autopilotmonitor")] SignalRConnectionInfo connectionInfo) + [HttpTrigger(AuthorizationLevel.Anonymous, "post", Route = "realtime/negotiate")] HttpRequestData req) { _logger.LogInformation("SignalR negotiate request"); @@ -47,11 +50,26 @@ namespace AutopilotMonitor.Functions.Functions.Infrastructure return unauthorizedResponse; } + // Negotiate via the Management SDK (not the SignalRConnectionInfoInput binding) so the + // client access token is bound to a SignalR USER ID = the lowercased UPN. That binding is + // what makes revocation enforceable: when a delegated grant / tenant-group assignment is + // revoked, the revoke path calls DisconnectUserAsync(upn) to cut already-joined + // live streams — anonymous connections (no user id) cannot be targeted. The UPN comes from + // the middleware-validated JWT, never from client-controlled binding data. var userEmail = TenantHelper.GetUserIdentifier(req); + var negotiation = await _signalRService.NegotiateClientAsync(userEmail.ToLowerInvariant()); + if (negotiation == null) + { + var unavailable = req.CreateResponse(HttpStatusCode.ServiceUnavailable); + await unavailable.WriteAsJsonAsync(new { success = false, message = "SignalR negotiation unavailable" }); + return unavailable; + } + _logger.LogInformation($"SignalR connection negotiated for user: {userEmail}"); var response = req.CreateResponse(HttpStatusCode.OK); - await response.WriteAsJsonAsync(connectionInfo); + // Exactly the shape the @microsoft/signalr client's negotiate protocol expects. + await response.WriteAsJsonAsync(new { url = negotiation.Value.Url, accessToken = negotiation.Value.AccessToken }); return response; } diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRRemoveFromGroupFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRRemoveFromGroupFunction.cs index 7bf24b24..9b082c05 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRRemoveFromGroupFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRRemoveFromGroupFunction.cs @@ -85,7 +85,9 @@ namespace AutopilotMonitor.Functions.Functions.Infrastructure // Check if user is allowed to leave this tenant's group — platform scope (GA or // read-only Global Reader) may leave any tenant's group, symmetric with AddToGroup; a // delegated ("MSP") admin may leave a managed tenant's group (symmetric with the join). - if (requestedTenantId != userTenantId) + // OrdinalIgnoreCase, matching the middleware's cross-tenant check and the delegated + // scope dictionary — a casing difference must never flip which branch authorizes. + if (!string.Equals(requestedTenantId, userTenantId, StringComparison.OrdinalIgnoreCase)) { var allowedCrossTenant = requestCtx.HasGlobalScope; if (!allowedCrossTenant && !string.IsNullOrEmpty(userEmail)) diff --git a/src/Backend/AutopilotMonitor.Functions/Services/ISignalRNotificationService.cs b/src/Backend/AutopilotMonitor.Functions/Services/ISignalRNotificationService.cs index 28f5755a..8e3225bf 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/ISignalRNotificationService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/ISignalRNotificationService.cs @@ -29,5 +29,27 @@ namespace AutopilotMonitor.Functions.Services Task SendGlobalNotificationAsync(object dto); Task SendGlobalNotificationDismissedAsync(string notificationId); Task SendGlobalNotificationDismissedAllAsync(); + + /// + /// Negotiates a client connection whose access token is bound to a SignalR USER ID (the + /// lowercased UPN). Binding the user id is what makes a later revoke enforceable server-side: + /// without it, connections are anonymous to the service and + /// has nothing to target. Returns null when + /// negotiation fails (caller should respond 503 — the client cannot connect anyway). + /// + Task<(string Url, string AccessToken)?> NegotiateClientAsync(string userId); + + /// + /// Cuts every live SignalR stream of the given user (lowercased UPN) — the enforcement half + /// of a delegated-grant / tenant-group revoke. Group authorization is join-time only, so an + /// already-joined, revoked caller would otherwise keep receiving live tenant telemetry until + /// the connection drops. Strips the user from all groups server-side AND pushes + /// "accessRevoked" to the user's connections; the web client restarts its connection on + /// receipt, and every re-join re-runs authorization — revoked groups 403, still-authorized + /// streams recover automatically. Only reaches connections negotiated WITH a user id (see + /// ); best-effort — failures are logged at Warning, the + /// authoritative revoke (table row) has already happened. + /// + Task DisconnectUserAsync(string userId); } } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/SignalRNotificationService.cs b/src/Backend/AutopilotMonitor.Functions/Services/SignalRNotificationService.cs index eeecd7ff..f884f031 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/SignalRNotificationService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/SignalRNotificationService.cs @@ -17,7 +17,7 @@ namespace AutopilotMonitor.Functions.Services { private readonly ILogger _logger; private readonly ServiceManager _serviceManager; - private IServiceHubContext? _hubContext; + private ServiceHubContext? _hubContext; private static readonly SemaphoreSlim _initLock = new(1, 1); private const string HubName = "autopilotmonitor"; @@ -30,6 +30,13 @@ namespace AutopilotMonitor.Functions.Services /// private static readonly TimeSpan SendTimeout = TimeSpan.FromSeconds(5); + /// + /// Ceiling for client negotiation. More generous than SendTimeout: the first negotiate on a + /// cold instance also pays the one-time hub-context initialization, and a negotiate failure + /// is user-visible (the client cannot connect at all), so err toward completing it. + /// + private static readonly TimeSpan NegotiateTimeout = TimeSpan.FromSeconds(10); + public SignalRNotificationService(IConfiguration configuration, ILogger logger) { _logger = logger; @@ -262,7 +269,64 @@ namespace AutopilotMonitor.Functions.Services } } - private async Task GetHubContextAsync(CancellationToken ct = default) + /// + /// Negotiates a client connection with the access token bound to + /// (lowercased UPN) — see . + /// Token generation is local (no service round-trip beyond hub-context init). + /// + public virtual async Task<(string Url, string AccessToken)?> NegotiateClientAsync(string userId) + { + try + { + using var cts = new CancellationTokenSource(NegotiateTimeout); + var hub = await GetHubContextAsync(cts.Token); + var negotiation = await hub.NegotiateAsync(new NegotiationOptions { UserId = userId }, cts.Token); + if (string.IsNullOrEmpty(negotiation?.Url) || string.IsNullOrEmpty(negotiation.AccessToken)) + { + _logger.LogError("SignalR negotiation returned an empty url/token for user {UserId}", userId); + return null; + } + return (negotiation.Url, negotiation.AccessToken); + } + catch (Exception ex) + { + _logger.LogError(ex, "SignalR negotiation failed for user {UserId}", userId); + return null; + } + } + + /// + /// Cuts the user's live streams after a revoke (see + /// ). Two prongs, because the + /// 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 + /// whose connection-scoped memberships survive (1) keeps its streams until the connection + /// drops. Warning (not Debug) on failure so it reaches App Insights. + /// + public virtual async Task DisconnectUserAsync(string userId) + { + try + { + using var cts = new CancellationTokenSource(SendTimeout); + var hub = await GetHubContextAsync(cts.Token); + await hub.UserGroups.RemoveFromAllGroupsAsync(userId, cts.Token); + await hub.Clients.User(userId).SendCoreAsync("accessRevoked", new object[] + { + new { timestamp = DateTime.UtcNow.ToString("o") } + }, cts.Token); + _logger.LogInformation("Revoked SignalR streams of user {UserId} (group strip + accessRevoked push)", userId); + } + catch (Exception ex) + { + _logger.LogWarning(ex, + "Failed to cut SignalR streams of user {UserId} after revoke — already-joined connections keep streaming until they drop", + userId); + } + } + + private async Task GetHubContextAsync(CancellationToken ct = default) { if (_hubContext != null) return _hubContext; diff --git a/src/Web/autopilot-monitor-web/contexts/SignalRContext.tsx b/src/Web/autopilot-monitor-web/contexts/SignalRContext.tsx index 39dbf4b5..3420e2f4 100644 --- a/src/Web/autopilot-monitor-web/contexts/SignalRContext.tsx +++ b/src/Web/autopilot-monitor-web/contexts/SignalRContext.tsx @@ -152,6 +152,19 @@ export function SignalRProvider({ children }: { children: React.ReactNode }) { } }; + // Server push after an operator revoked this user's (delegated) access: restart the + // connection. onclose clears the joined-group tracking, and every consumer hook re-joins on + // 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 () => { + 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(); + }); + // Start as soon as the user is authenticated. The previous 2s setTimeout // was meant to avoid competing with the initial dashboard fetches, but // the web origin is HTTP/2 and the API is on a different origin, so