fix(security): backend-review M1-M3 — kill-switch + MCP cache TTL + SignalR broadcast gate

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 <noreply@anthropic.com>
This commit is contained in:
Oliver Kieselbach
2026-07-03 15:44:36 +02:00
parent 34582c0f0e
commit e9c5bfbafc
12 changed files with 216 additions and 59 deletions
@@ -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<string>()))
.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<string>()))
.ReturnsAsync(Constants.GlobalRoles.GlobalAdmin);
_adminRepo.Setup(x => x.GetMcpUserAsync(It.IsAny<string>()))
.ReturnsAsync(new McpUserEntry { Upn = DelegatedUpn, IsEnabled = false });
var result = await _sut.IsAllowedAsync(DelegatedUpn);
Assert.False(result.IsAllowed);
}
}
@@ -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));
}
}
@@ -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,
@@ -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
@@ -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
@@ -48,6 +48,38 @@ public static class SignalRGroupHelper
return null;
}
/// <summary>
/// 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 <see cref="IsTenantBroadcastJoinDenied"/>.
/// </summary>
public static bool IsTenantBroadcastGroup(string groupName)
=> groupName.StartsWith("tenant-")
&& !groupName.EndsWith(TenantNotifyAdminSuffix)
&& !groupName.EndsWith(TenantNotifyMemberSuffix);
/// <summary>
/// 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.
/// </summary>
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();
}
/// <summary>
/// True for the Admin-tier tenant notification group. Joining requires Tenant-Admin or Global-Admin.
/// </summary>
@@ -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
@@ -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).
/// </summary>
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<McpAccessPolicy>(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
/// <summary>
/// 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 <see cref="IsAllowedAsync"/>.
/// kill-switch that denies access under EVERY policy and grant path — see <see cref="IsAllowedAsync"/>.
/// 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).
/// </summary>
@@ -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);
});
@@ -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
@@ -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<Session | null>;
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;
@@ -65,7 +65,6 @@ export default function ProgressPortalPage() {
useProgressSignalR({
session,
sessionRef,
tenantId,
signalR,
scheduleFetchEvents,
});