fix(security): backend-review Lows — SignalR revoke enforcement, revoke audit integrity, casing

Revoke enforcement (L4): a revoked delegated (MSP) user kept receiving pushes on
already-joined SignalR groups until the connection dropped (join-time-only
authorization). Negotiate now goes through the Management SDK and binds the
client token to a SignalR user id (lowercased UPN); new DisconnectUserAsync
strips the user from all groups server-side and pushes "accessRevoked", on
which the web client restarts its connection so every re-join re-runs
authorization against the fresh scope. Wired into delegated revoke/disable and
tenant-group unassign / remove-tenant / delete-group. Residual (documented): a
tampered client ignoring the push, and connections negotiated before this
deploy (no user id), keep streams until the connection drops.

Revoke audit integrity (L5): POST /global/delegated-admins now requires a GUID
tenantId (mirrors the tenant-group path), and DELETE reports 404 for an
assignment that never existed instead of returning 200 and writing a false
customer-visible "access removed" audit row (repo delete now checks the 404
response of the idempotent DeleteEntityAsync).

Casing (L6): the SignalR group join/leave cross-tenant check now compares
tenant IDs OrdinalIgnoreCase, aligned with the middleware and the delegated
scope dictionary, so a casing difference can never flip which branch
authorizes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

src/Backend/AutopilotMonitor.Functions.Tests/FakeSignalRNotificationService.cs

@@ -75,4 +75,19 @@ public class FakeSignalRNotificationService : ISignalRNotificationService
         GlobalDismissAllCount++;
         return Task.CompletedTask;
     }
+
+    public List<string> NegotiatedUserIds { get; } = new();
+    public List<string> 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;
+    }
 }

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;
         }
     }
 }

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") ---

src/Backend/AutopilotMonitor.Functions/Functions/Admin/DelegatedAdminManagementFunction.cs

@@ -25,15 +25,18 @@ public class DelegatedAdminManagementFunction
     private readonly ILogger<DelegatedAdminManagementFunction> _logger;
     private readonly DelegatedAdminService _delegatedAdminService;
     private readonly IMaintenanceRepository _maintenanceRepo;
+    private readonly ISignalRNotificationService _signalRService;
 
     public DelegatedAdminManagementFunction(
         ILogger<DelegatedAdminManagementFunction> logger,
         DelegatedAdminService delegatedAdminService,
-        IMaintenanceRepository maintenanceRepo)
+        IMaintenanceRepository maintenanceRepo,
+        ISignalRNotificationService signalRService)
     {
         _logger = logger;
         _delegatedAdminService = delegatedAdminService;
         _maintenanceRepo = maintenanceRepo;
+        _signalRService = signalRService;
     }
 
     /// <summary>GET /api/global/delegated-admins — list every delegated assignment. GlobalReadOrAdmin.</summary>
@@ -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<string, string> { { "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);
 

src/Backend/AutopilotMonitor.Functions/Functions/Admin/TenantGroupManagementFunction.cs

@@ -30,17 +30,20 @@ public class TenantGroupManagementFunction
     private readonly ILogger<TenantGroupManagementFunction> _logger;
     private readonly DelegatedAdminService _delegatedAdminService;
     private readonly IMaintenanceRepository _maintenanceRepo;
+    private readonly ISignalRNotificationService _signalRService;
 
     private const string AuditEntity = "DelegatedGroupAccess";
 
     public TenantGroupManagementFunction(
         ILogger<TenantGroupManagementFunction> logger,
         DelegatedAdminService delegatedAdminService,
-        IMaintenanceRepository maintenanceRepo)
+        IMaintenanceRepository maintenanceRepo,
+        ISignalRNotificationService signalRService)
     {
         _logger = logger;
         _delegatedAdminService = delegatedAdminService;
         _maintenanceRepo = maintenanceRepo;
+        _signalRService = signalRService;
     }
 
     /// <summary>GET /api/global/tenant-groups — list every group with tenants + assignee count. GlobalReadOrAdmin.</summary>
@@ -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<string, string> { { "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,

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))

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<SignalRNegotiateFunction> _logger;
+        private readonly ISignalRNotificationService _signalRService;
 
-        public SignalRNegotiateFunction(ILogger<SignalRNegotiateFunction> logger)
+        public SignalRNegotiateFunction(
+            ILogger<SignalRNegotiateFunction> logger,
+            ISignalRNotificationService signalRService)
         {
             _logger = logger;
+            _signalRService = signalRService;
         }
 
         [Function("negotiate")]
         public async Task<HttpResponseData> 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;
         }

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))

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();
+
+        /// <summary>
+        /// 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
+        /// <see cref="RemoveUserFromAllGroupsAsync"/> has nothing to target. Returns null when
+        /// negotiation fails (caller should respond 503 — the client cannot connect anyway).
+        /// </summary>
+        Task<(string Url, string AccessToken)?> NegotiateClientAsync(string userId);
+
+        /// <summary>
+        /// 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
+        /// <see cref="NegotiateClientAsync"/>); best-effort — failures are logged at Warning, the
+        /// authoritative revoke (table row) has already happened.
+        /// </summary>
+        Task DisconnectUserAsync(string userId);
     }
 }

src/Backend/AutopilotMonitor.Functions/Services/SignalRNotificationService.cs

@@ -17,7 +17,7 @@ namespace AutopilotMonitor.Functions.Services
     {
         private readonly ILogger<SignalRNotificationService> _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
         /// </summary>
         private static readonly TimeSpan SendTimeout = TimeSpan.FromSeconds(5);
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        private static readonly TimeSpan NegotiateTimeout = TimeSpan.FromSeconds(10);
+
         public SignalRNotificationService(IConfiguration configuration, ILogger<SignalRNotificationService> logger)
         {
             _logger = logger;
@@ -262,7 +269,64 @@ namespace AutopilotMonitor.Functions.Services
             }
         }
 
-        private async Task<IServiceHubContext> GetHubContextAsync(CancellationToken ct = default)
+        /// <summary>
+        /// Negotiates a client connection with the access token bound to <paramref name="userId"/>
+        /// (lowercased UPN) — see <see cref="ISignalRNotificationService.NegotiateClientAsync"/>.
+        /// Token generation is local (no service round-trip beyond hub-context init).
+        /// </summary>
+        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;
+            }
+        }
+
+        /// <summary>
+        /// Cuts the user's live streams after a revoke (see
+        /// <see cref="ISignalRNotificationService.DisconnectUserAsync"/>). 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.
+        /// </summary>
+        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<ServiceHubContext> GetHubContextAsync(CancellationToken ct = default)
         {
             if (_hubContext != null) return _hubContext;
 

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