From e9c5bfbafc064e9730db9548b32fa50802eee057 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Fri, 3 Jul 2026 15:44:36 +0200 Subject: [PATCH] =?UTF-8?q?fix(security):=20backend-review=20M1-M3=20?= =?UTF-8?q?=E2=80=94=20kill-switch=20+=20MCP=20cache=20TTL=20+=20SignalR?= =?UTF-8?q?=20broadcast=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit M1: hoist the McpUsers Disabled-row kill-switch above the policy switch so an explicitly disabled account is denied MCP access under EVERY policy and grant path (AllMembers, platform roles, delegated auto-grant) — previously inert outside the WhitelistOnly branch. M2: drop the MCP access-guard verdict cache from 5 min to 60s. The verdict carries the kill-switch and the delegated tenant set; 5 min stretched the backend's 30s revocation target to minutes for MCP callers. M3: gate same-tenant joins of the plain tenant-{tid} SignalR broadcast group on a member role (new pure SignalRGroupHelper.IsTenantBroadcastJoinDenied) — roleless authenticated users (Progress Portal) could passively stream MemberRead-tier live telemetry they cannot fetch over REST. Companions: MarkSessionSucceeded/Failed additionally push "newevents" to the per-session group so a roleless progress watcher still sees admin-marked completion, and the Progress Portal no longer joins the tenant group (session group covers all its signals). Co-Authored-By: Claude Fable 5 --- .../McpUserServiceDelegatedTests.cs | 42 ++++++++++++++++ .../SignalRGroupHelperTests.cs | 49 +++++++++++++++++++ .../SignalRAddToGroupFunction.cs | 14 ++++++ .../Sessions/MarkSessionFailedFunction.cs | 25 +++++++--- .../Sessions/MarkSessionSucceededFunction.cs | 25 +++++++--- .../Helpers/SignalRGroupHelper.cs | 32 ++++++++++++ .../Security/EndpointAccessPolicyCatalog.cs | 4 +- .../Services/McpUserService.cs | 30 +++++++----- .../src/__tests__/access-guard.test.ts | 2 +- .../autopilot-monitor-mcp/src/access-guard.ts | 26 ++++++---- .../app/progress/hooks/useProgressSignalR.ts | 25 +++------- .../app/progress/page.tsx | 1 - 12 files changed, 216 insertions(+), 59 deletions(-) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceDelegatedTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceDelegatedTests.cs index d6365ead..39869716 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceDelegatedTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/McpUserServiceDelegatedTests.cs @@ -162,4 +162,46 @@ public class McpUserServiceDelegatedTests Assert.False(result.IsAllowed); } + + [Fact] + public async Task AllMembers_WithDisabledMcpUserRow_IsDenied_KillSwitch() + { + // The kill-switch must not be inert under a permissive policy: an operator who disables a + // user's McpUsers row believes access is revoked — under AllMembers that must hold too. + SetPolicy(McpAccessPolicy.AllMembers); + _adminRepo.Setup(x => x.GetMcpUserAsync(It.IsAny())) + .ReturnsAsync(new McpUserEntry { Upn = DelegatedUpn, IsEnabled = false }); + + var result = await _sut.IsAllowedAsync(DelegatedUpn); + + Assert.False(result.IsAllowed); + } + + [Fact] + public async Task AllMembers_WithoutMcpUserRow_IsAllowed() + { + // Control for the hoisted kill-switch: a MISSING row is the normal AllMembers case and must + // still be granted — only an explicit Disabled row denies. + SetPolicy(McpAccessPolicy.AllMembers); + + var result = await _sut.IsAllowedAsync(DelegatedUpn); + + Assert.True(result.IsAllowed); + Assert.Equal("AllMembers", result.AccessGrant); + } + + [Fact] + public async Task PlatformRole_WithDisabledMcpUserRow_IsDenied_KillSwitch() + { + // The Disabled row wins over EVERY grant path, including a platform role — the kill-switch + // is absolute, so an operator never has to reason about which grant path a user holds. + _globalAdmin.Setup(x => x.GetGlobalRoleAsync(It.IsAny())) + .ReturnsAsync(Constants.GlobalRoles.GlobalAdmin); + _adminRepo.Setup(x => x.GetMcpUserAsync(It.IsAny())) + .ReturnsAsync(new McpUserEntry { Upn = DelegatedUpn, IsEnabled = false }); + + var result = await _sut.IsAllowedAsync(DelegatedUpn); + + Assert.False(result.IsAllowed); + } } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/SignalRGroupHelperTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/SignalRGroupHelperTests.cs index f7c010a1..2b766093 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/SignalRGroupHelperTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/SignalRGroupHelperTests.cs @@ -152,4 +152,53 @@ public class SignalRGroupHelperTests Assert.Equal(SignalRGroupHelper.NotifyGroupDenial.None, SignalRGroupHelper.CheckNotifyGroupAccess($"tenant-{ManagedTenant}", ManagedTenant, ctx)); } + + // ── IsTenantBroadcastJoinDenied ────────────────────────────────────────── + // The plain "tenant-{tid}" group streams MemberRead-tier telemetry, but the join route admits ANY + // authenticated user of the tenant (the Progress Portal's roleless end users). A same-tenant join must + // therefore require a member role; session groups stay open so the Progress Portal keeps working. + + [Fact] + public void BroadcastJoin_RolelessSameTenantUser_Denied() + { + // THE FIX: a roleless authenticated employee (Progress Portal user) must NOT join their tenant's + // broadcast group and passively stream org-wide enrollment activity they cannot fetch over REST. + var ctx = new RequestContext { TenantId = HomeTenant }; + Assert.True(SignalRGroupHelper.IsTenantBroadcastJoinDenied($"tenant-{HomeTenant}", HomeTenant, ctx)); + } + + [Fact] + public void BroadcastJoin_SameTenantMember_Allowed() + { + var ctx = new RequestContext { TenantId = HomeTenant, UserRole = Constants.TenantRoles.Viewer }; + Assert.False(SignalRGroupHelper.IsTenantBroadcastJoinDenied($"tenant-{HomeTenant}", HomeTenant, ctx)); + } + + [Fact] + public void BroadcastJoin_GlobalScope_Allowed() + { + var ctx = new RequestContext { TenantId = HomeTenant, IsGlobalReader = true }; + Assert.False(SignalRGroupHelper.IsTenantBroadcastJoinDenied($"tenant-{HomeTenant}", HomeTenant, ctx)); + } + + [Fact] + public void BroadcastJoin_CrossTenant_NotDecidedHere() + { + // A cross-tenant join only reaches this check after the upstream cross-tenant admission + // (platform scope or delegated scope over the group's tenant) — the helper must not re-deny it. + var ctx = new RequestContext { TenantId = HomeTenant }; + Assert.False(SignalRGroupHelper.IsTenantBroadcastJoinDenied($"tenant-{ManagedTenant}", ManagedTenant, ctx)); + } + + [Fact] + public void BroadcastJoin_SessionAndNotifyGroups_NotCovered() + { + // Session groups stay joinable for roleless users (Progress Portal); notify groups have their own + // gate (CheckNotifyGroupAccess). + var ctx = new RequestContext { TenantId = HomeTenant }; + Assert.False(SignalRGroupHelper.IsTenantBroadcastJoinDenied( + $"session-{HomeTenant}-{SessionId}", HomeTenant, ctx)); + Assert.False(SignalRGroupHelper.IsTenantBroadcastJoinDenied(MemberGroup(HomeTenant), HomeTenant, ctx)); + Assert.False(SignalRGroupHelper.IsTenantBroadcastJoinDenied(AdminGroup(HomeTenant), HomeTenant, ctx)); + } } diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRAddToGroupFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRAddToGroupFunction.cs index bdcbec16..dd8b2ef1 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRAddToGroupFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Infrastructure/SignalRAddToGroupFunction.cs @@ -118,6 +118,20 @@ namespace AutopilotMonitor.Functions.Functions.Infrastructure } } + // The plain "tenant-{tid}" broadcast group streams MemberRead-tier telemetry (new-session + // pushes, per-session "newevents" deltas incl. rule results), but this route's policy + // admits ANY authenticated user of the tenant — the Progress Portal's roleless end users, + // who only need their own "session-{tid}-{sid}" group. A same-tenant join skipped the + // cross-tenant gate above, so bind the broadcast group to a member role here; cross-tenant + // callers reaching this point were already admitted as platform-scope or delegated. + if (SignalRGroupHelper.IsTenantBroadcastJoinDenied(request.GroupName, requestedTenantId, requestCtx)) + { + _logger.LogWarning($"User {userEmail} (role={requestCtx.UserRole}) attempted to join tenant broadcast group without a member role: {request.GroupName}"); + var forbiddenResponse = req.CreateResponse(HttpStatusCode.Forbidden); + await forbiddenResponse.WriteAsJsonAsync(new { success = false, message = "Access denied: Only tenant members can join the tenant broadcast group" }); + return new AddToGroupOutput { HttpResponse = forbiddenResponse }; + } + // Notification groups carry the full per-tenant payload (the REST list is Member/Admin-tier // gated), so they are role-gated AGAINST THE GROUP'S TENANT — not the caller's home tenant. // This is leak-critical for a cross-tenant caller admitted above (a delegated "MSP" reader, diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/MarkSessionFailedFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/MarkSessionFailedFunction.cs index 86f91500..83b5d303 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/MarkSessionFailedFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/MarkSessionFailedFunction.cs @@ -95,21 +95,32 @@ namespace AutopilotMonitor.Functions.Functions.Sessions updatedSession.DiagnosticsBlobName } : null; + object messagePayload = new { + sessionId = sessionId, + tenantId = tenantId, + eventCount = 0, + sessionUpdate = sessionDelta + }; + var tenantMessage = new SignalRMessageAction("newevents") { GroupName = $"tenant-{tenantId}", - Arguments = new object[] { new { - sessionId = sessionId, - tenantId = tenantId, - eventCount = 0, - sessionUpdate = sessionDelta - } } + Arguments = new[] { messagePayload } + }; + + // Also push to the per-session group: the tenant broadcast group is member-role + // gated, so a roleless Progress-Portal watcher of this session would otherwise + // never see the admin-marked failure. + var sessionMessage = new SignalRMessageAction("newevents") + { + GroupName = $"session-{tenantId}-{sessionId}", + Arguments = new[] { messagePayload } }; return new MarkSessionFailedOutput { HttpResponse = response, - SignalRMessages = new[] { tenantMessage } + SignalRMessages = new[] { tenantMessage, sessionMessage } }; } else diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/MarkSessionSucceededFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/MarkSessionSucceededFunction.cs index 7fc12cd6..1ce6ea7f 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/MarkSessionSucceededFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/MarkSessionSucceededFunction.cs @@ -92,21 +92,32 @@ namespace AutopilotMonitor.Functions.Functions.Sessions updatedSession.DiagnosticsBlobName } : null; + object messagePayload = new { + sessionId = sessionId, + tenantId = tenantId, + eventCount = 0, + sessionUpdate = sessionDelta + }; + var tenantMessage = new SignalRMessageAction("newevents") { GroupName = $"tenant-{tenantId}", - Arguments = new object[] { new { - sessionId = sessionId, - tenantId = tenantId, - eventCount = 0, - sessionUpdate = sessionDelta - } } + Arguments = new[] { messagePayload } + }; + + // Also push to the per-session group: the tenant broadcast group is member-role + // gated, so a roleless Progress-Portal watcher of this session would otherwise + // never see the admin-marked completion. + var sessionMessage = new SignalRMessageAction("newevents") + { + GroupName = $"session-{tenantId}-{sessionId}", + Arguments = new[] { messagePayload } }; return new MarkSessionSucceededOutput { HttpResponse = response, - SignalRMessages = new[] { tenantMessage } + SignalRMessages = new[] { tenantMessage, sessionMessage } }; } else diff --git a/src/Backend/AutopilotMonitor.Functions/Helpers/SignalRGroupHelper.cs b/src/Backend/AutopilotMonitor.Functions/Helpers/SignalRGroupHelper.cs index 05b6d3cd..22327cc9 100644 --- a/src/Backend/AutopilotMonitor.Functions/Helpers/SignalRGroupHelper.cs +++ b/src/Backend/AutopilotMonitor.Functions/Helpers/SignalRGroupHelper.cs @@ -48,6 +48,38 @@ public static class SignalRGroupHelper return null; } + /// + /// True for the plain tenant-wide broadcast group ("tenant-{tid}" with no notify suffix). It carries + /// MemberRead-tier live telemetry (new-session pushes, per-session "newevents" deltas incl. rule + /// results), so joining it requires member-tier standing — see . + /// + public static bool IsTenantBroadcastGroup(string groupName) + => groupName.StartsWith("tenant-") + && !groupName.EndsWith(TenantNotifyAdminSuffix) + && !groupName.EndsWith(TenantNotifyMemberSuffix); + + /// + /// Gates a SAME-TENANT join of the plain tenant broadcast group. The route policy admits ANY + /// authenticated user of the tenant (the Progress Portal's roleless end users, who only need their + /// own "session-{tid}-{sid}" group), so without this gate such a user could passively stream + /// org-wide enrollment activity that is MemberRead-gated at the REST layer (GET sessions et al.). + /// Cross-tenant joins are NOT decided here: they must already have passed the cross-tenant + /// admission (platform scope, or delegated scope over the group's tenant), both of which may + /// receive the broadcast. Pure (no I/O) so it stays unit-testable. + /// + public static bool IsTenantBroadcastJoinDenied(string groupName, string? requestedTenantId, RequestContext ctx) + { + if (!IsTenantBroadcastGroup(groupName)) + return false; + + var sameTenant = !string.IsNullOrEmpty(requestedTenantId) + && string.Equals(requestedTenantId, ctx.TenantId, StringComparison.OrdinalIgnoreCase); + if (!sameTenant) + return false; // cross-tenant admission (platform/delegated scope) is enforced upstream + + return !ctx.HasGlobalScope && !ctx.IsTenantMemberRole(); + } + /// /// True for the Admin-tier tenant notification group. Joining requires Tenant-Admin or Global-Admin. /// diff --git a/src/Backend/AutopilotMonitor.Functions/Security/EndpointAccessPolicyCatalog.cs b/src/Backend/AutopilotMonitor.Functions/Security/EndpointAccessPolicyCatalog.cs index 9043b056..4865d0cc 100644 --- a/src/Backend/AutopilotMonitor.Functions/Security/EndpointAccessPolicyCatalog.cs +++ b/src/Backend/AutopilotMonitor.Functions/Security/EndpointAccessPolicyCatalog.cs @@ -179,7 +179,9 @@ public static class EndpointAccessPolicyCatalog // requires the requested group's tenant to match the caller's JWT tenant (platform-scope // callers — GA or read-only Global Reader — excepted), gates the global-admins read-broadcast // group on HasGlobalScope (GA ∪ GlobalReader), the admin-tier notification group on - // IsTenantAdmin (or GA), and the member-tier notification group on tenant membership (or scope). + // IsTenantAdmin (or GA), the member-tier notification group on tenant membership (or scope), + // and the plain tenant-{tid} broadcast group on tenant membership (or scope) — roleless + // Progress-Portal users may only join their own session-{tid}-{sid} group. // The Progress Portal is an END-USER feature: employees tracking their own device are // authenticated (JWT tid = their tenant) but are typically NOT tenant members (no // TenantAdmins row, no Entra app-role). Gating join at MemberRead let them load the diff --git a/src/Backend/AutopilotMonitor.Functions/Services/McpUserService.cs b/src/Backend/AutopilotMonitor.Functions/Services/McpUserService.cs index 5ff8e7a8..df09f3c9 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/McpUserService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/McpUserService.cs @@ -13,6 +13,8 @@ namespace AutopilotMonitor.Functions.Services; /// - Disabled: no one can access MCP /// - WhitelistOnly: Global Admins + explicit McpUsers table entries /// - AllMembers: any authenticated user +/// An EXPLICITLY DISABLED McpUsers row is an operator kill-switch that denies access under every +/// policy and every grant path (platform role, delegated auto-grant, AllMembers). /// public class McpUserService { @@ -72,6 +74,18 @@ public class McpUserService var delegatedTenantIds = delegatedScope.IsEmpty ? null : delegatedScope.TenantIds; var delegatedRole = delegatedScope.IsEmpty ? null : StrongestDelegatedRole(delegatedScope); + // Resolve the caller's McpUsers whitelist state ONCE (cached): NotPresent / Enabled / Disabled. + var whitelistState = await ResolveWhitelistStateAsync(upn); + + // SECURITY (operator kill-switch): an EXPLICITLY DISABLED McpUsers row denies MCP access under + // EVERY policy and EVERY grant path — platform roles, the delegated (MSP) auto-grant, and + // AllMembers alike. Checked BEFORE the policy switch so the one lever an operator reaches for + // ("disable this account's MCP access") can never be silently inert under a permissive policy. + // A MISSING row is NOT a kill-switch: most callers (delegated admins, AllMembers users) have no + // McpUsers row at all and must still be granted by their policy branch below. + if (whitelistState == McpWhitelistState.Disabled) + return McpAccessCheckResult.Denied("User not enabled for MCP usage (account is disabled on the MCP whitelist)"); + var config = await _adminConfigService.GetConfigurationAsync(); if (!Enum.TryParse(config.McpAccessPolicy, out var policy)) policy = McpAccessPolicy.WhitelistOnly; @@ -88,22 +102,12 @@ public class McpUserService case McpAccessPolicy.WhitelistOnly: default: - // Any platform role (GlobalAdmin or read-only GlobalReader) always has MCP access. + // Any platform role (GlobalAdmin or read-only GlobalReader) always has MCP access + // (unless explicitly disabled via the kill-switch above). if (globalRole != null) return McpAccessCheckResult.Allowed( upn, globalRole, isGlobalAdmin, globalRole, delegatedTenantIds, delegatedRole); - // Resolve the caller's McpUsers whitelist state ONCE (cached): NotPresent / Enabled / Disabled. - var whitelistState = await ResolveWhitelistStateAsync(upn); - - // SECURITY (operator kill-switch): an EXPLICITLY DISABLED McpUsers row overrides the - // delegated (MSP) auto-grant below. An operator can revoke a delegated caller's MCP access - // by disabling their McpUsers row WITHOUT unwinding every delegated assignment — one lever. - // A MISSING row is NOT a kill-switch: a delegated admin normally has no McpUsers row at all - // and must still be auto-granted. (Order matters — this MUST precede the delegated grant.) - if (whitelistState == McpWhitelistState.Disabled) - return McpAccessCheckResult.Denied("User not enabled for MCP usage (account is disabled on the MCP whitelist)"); - // A delegated (MSP) admin with an active scope is granted MCP access automatically under // WhitelistOnly — "delegated = scoped global", and they are already curated via the // Delegated Admins UI, so a separate enabled McpUsers row would be redundant friction. Their @@ -206,7 +210,7 @@ public class McpUserService /// /// Resolves (and briefly caches) the caller's McpUsers whitelist state: NotPresent (no row), /// Enabled (row + IsEnabled), or Disabled (row + !IsEnabled). A Disabled row is an operator - /// kill-switch that also revokes a delegated/MSP caller's auto-grant — see . + /// kill-switch that denies access under EVERY policy and grant path — see . /// Cached under the same key the mutation methods (Add/Remove/SetEnabled) invalidate, so an /// enable/disable self-heals within the TTL (and immediately on the mutating instance). /// diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard.test.ts index 370c5be7..c9afb4f9 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard.test.ts @@ -348,7 +348,7 @@ describe('accessGuard — 429 (rate limit) and caching', () => { await runGuard(mockReq(`Bearer ${token}`)); await runGuard(mockReq(`Bearer ${token}`)); - // Second request hits the 5-min UPN+tokenHash cache — no second backend call. + // Second request hits the 60s UPN+tokenHash cache — no second backend call. expect(fetchFn).toHaveBeenCalledTimes(1); }); diff --git a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts index fb199860..b37cf3f4 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts @@ -3,7 +3,7 @@ * * Flow per request: * 1. Extract Bearer token → decode JWT claims (upn, exp) - * 2. Check access: call backend /api/global/mcp-users/check (cached 5min per UPN) + * 2. Check access: call backend /api/global/mcp-users/check (cached 60s per UPN+token) * 3. Rate limit: sliding window per UPN (default 60 req/min) * 4. Set token for pass-through to backend API */ @@ -25,10 +25,10 @@ const RATE_LIMIT = parsePositiveInt(process.env.MCP_RATE_LIMIT_PER_MINUTE, 60); // only on the cache-MISS path that would call the backend. Bounds forged- / // distinct-token floods at the source without ever touching a victim's per-UPN // budget (different key space). It counts ONLY cache misses — a legitimate user -// misses at most ~once per token per 5-min TTL (~0.2/min), and a NAT'd org of N -// active users ~N/5 per min — so the default 120 (= 2× the per-UPN rate) clears -// even a ~600-user shared-IP org while capping flood amplification to ~2 backend -// calls/sec/IP. NOTE: this is per-replica; with maxReplicas=1 it is effectively +// misses at most ~once per token per 60s TTL (~1/min), and a NAT'd org of N +// active users ~N per min — so the default 120 (= 2× the per-UPN rate) clears +// a ~120-concurrently-active-user shared-IP org while capping flood +// amplification to ~2 backend calls/sec/IP. NOTE: this is per-replica; with maxReplicas=1 it is effectively // global, but if the app is ever scaled out the effective per-IP ceiling becomes // 120×replicas (a shared store would be needed for a true global cap). Tune via // MCP_PRE_AUTH_RATE_LIMIT_PER_MINUTE; raise it (or allowlist the egress IP) only @@ -68,7 +68,13 @@ const MAX_RATE_BUCKET_ENTRIES = parsePositiveInt(process.env.MCP_RATE_BUCKET_MAX // cannot pile up held requests. const ACCESS_CHECK_TIMEOUT_MS = parsePositiveInt(process.env.MCP_ACCESS_CHECK_TIMEOUT_MS, 15_000); -const ACCESS_CACHE_TTL_MS = 5 * 60 * 1000; // 5 minutes +// The access verdict carries the operator kill-switch (a Disabled McpUsers row) and the delegated +// (MSP) tenant set, so this TTL IS the revocation lag of this layer. The backend caches its own +// verdict for 30s (McpUserService) with the explicit goal that a revoke self-heals in seconds — +// a long TTL here would quietly stretch that to minutes for MCP callers. Keep it ≤60s: worst-case +// revocation = this TTL + the backend's 30s. Do NOT raise it back "for performance" — a cache miss +// is one backend point-read per user per minute. +const ACCESS_CACHE_TTL_MS = 60 * 1000; // 60 seconds // --- Access check cache --- @@ -77,7 +83,7 @@ interface AccessCacheEntry { reason: string; isGlobalAdmin: boolean; isGlobalReader: boolean; - // Delegated (scoped-global / MSP) scope. Cached alongside the platform flags so the 5-min-cached + // Delegated (scoped-global / MSP) scope. Cached alongside the platform flags so a cached // second request keeps the managed tenant set — without this the follow-up would silently drop the // scope and downgrade the caller to home-tenant-only routing. delegatedTenantIds?: string[]; @@ -139,7 +145,7 @@ async function checkAccess(upn: string, token: string, clientIp: string): Promis if (cached && Date.now() < cached.expiresAt) { // A cached DENY is cheap to serve but still costs request processing — and // deny verdicts ARE cached, so without this an attacker could repeat one - // forged/denied token and farm unlimited cached 403s for the whole 5-min TTL + // forged/denied token and farm unlimited cached 403s for the whole TTL // while never tripping the pre-auth limiter (it only ran on the miss path). // Count cached denials against the per-source-IP budget too, returning 429 // once it is exhausted. Cached ALLOWs stay free, so a legitimate user never @@ -213,7 +219,7 @@ async function checkAccess(upn: string, token: string, clientIp: string): Promis // Cache only the persisted fields (infraError is request-derived, always // false for a cached verdict — see the cache-hit path above). The delegated - // scope MUST be cached too, or the 5-min-cached follow-up loses it. + // scope MUST be cached too, or the cached follow-up loses it. // boundedSet caps the map so a distinct-token flood cannot exhaust memory. boundedSet(accessCache, cacheKey, { allowed: result.allowed, @@ -432,7 +438,7 @@ export function accessGuard(req: Request, res: Response, next: NextFunction): vo // victim's UPN. If the per-UPN limiter incremented before validation, those // forgeries would burn the victim's per-minute budget and 429 their // legitimate calls. checkAccess delegates signature verification to the - // backend; both allow- and deny-decisions are cached for 5 min keyed on + // backend; both allow- and deny-decisions are cached for 60s keyed on // UPN+tokenHash, so a forged token cannot piggyback on a legitimate user's // cached `allowed: true`. On a cache MISS, checkAccess additionally applies a // per-source-IP throttle (keyed on clientIp, not UPN) so a distinct-token diff --git a/src/Web/autopilot-monitor-web/app/progress/hooks/useProgressSignalR.ts b/src/Web/autopilot-monitor-web/app/progress/hooks/useProgressSignalR.ts index d4d94b94..27968e39 100644 --- a/src/Web/autopilot-monitor-web/app/progress/hooks/useProgressSignalR.ts +++ b/src/Web/autopilot-monitor-web/app/progress/hooks/useProgressSignalR.ts @@ -1,6 +1,6 @@ "use client"; -import { useEffect, useRef } from "react"; +import { useEffect } from "react"; import { Session } from "@/types"; interface SignalRApi { @@ -16,40 +16,27 @@ interface SignalRApi { interface UseProgressSignalRParams { session: Session | null; sessionRef: React.MutableRefObject; - tenantId: string; signalR: SignalRApi; scheduleFetchEvents: (delayMs?: number) => void; } /** * Owns the progress page's SignalR integration: - * - joins tenant group once per connection - * - joins session-specific group when a session is selected + * - joins the session-specific group when a session is selected + * (the tenant-wide broadcast group is member-role gated server-side; roleless + * progress viewers would get a 403, and all signals for the selected session — + * eventStream on ingest, newevents on admin mark-succeeded/failed — arrive on + * the session group anyway) * - listens for newevents / newSession / eventStream → debounced refetch * - cleans up groups + handlers on unmount / session change */ export function useProgressSignalR({ session, sessionRef, - tenantId, signalR, scheduleFetchEvents, }: UseProgressSignalRParams): void { const { on, off, isConnected, joinGroup, leaveGroup } = signalR; - const hasJoinedTenantGroup = useRef(false); - - useEffect(() => { - if (isConnected && !hasJoinedTenantGroup.current) { - joinGroup(`tenant-${tenantId}`); - hasJoinedTenantGroup.current = true; - } - return () => { - if (hasJoinedTenantGroup.current) { - leaveGroup(`tenant-${tenantId}`); - hasJoinedTenantGroup.current = false; - } - }; - }, [isConnected, tenantId, joinGroup, leaveGroup]); useEffect(() => { if (!isConnected || !session) return; diff --git a/src/Web/autopilot-monitor-web/app/progress/page.tsx b/src/Web/autopilot-monitor-web/app/progress/page.tsx index c7d03ef0..daec1e40 100644 --- a/src/Web/autopilot-monitor-web/app/progress/page.tsx +++ b/src/Web/autopilot-monitor-web/app/progress/page.tsx @@ -65,7 +65,6 @@ export default function ProgressPortalPage() { useProgressSignalR({ session, sessionRef, - tenantId, signalR, scheduleFetchEvents, });