Operator reads on global routes stay with platform roles; the middleware decides on the tenantId the handler reads (D-363)

Five platform reads take ?tenantId= only as a filter, but their QueryParam
scoping also admitted a delegated (MSP) reader of the named tenant:
global/session-reports, global/rule-submissions, global/metrics/mcp-usage,
global/metrics/mcp-usage/daily and global/metrics/verdict-calibration. They
carry operator data (admin notes, contact data and reviews, an organization's
accounts, classifier diagnostics), so they are catalogued None now; the
handlers still read ?tenantId= themselves, and Global Admins and Global
Readers keep the filter.

- A catalog ratchet in PolicyEnforcementMiddlewareTests lists every platform
  route a delegated caller reaches; a new one fails the build until it is
  added on purpose. The TenantScoping.QueryParam doc states the rule.
- The middleware took the cross-tenant decision on the first ?tenantId=
  value while the handlers read all values comma-joined. It now decides on
  the joined string, with ASP.NET Core's parser (the handlers parse Url.Query,
  where System.Uri has already escaped a stray '%').
- The rule pages show a delegated reader the admin-only hint instead of the
  submission list, as they already did for an Operator or Viewer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Oliver Kieselbach
2026-10-15 14:03:47 +02:00
parent 9252b8081f
commit 38b499b80b
8 changed files with 244 additions and 18 deletions
@@ -1,12 +1,15 @@
using System.Security.Claims;
using AutopilotMonitor.Functions.Functions.Admin;
using AutopilotMonitor.Functions.Functions.Config;
using AutopilotMonitor.Functions.Helpers;
using AutopilotMonitor.Functions.Middleware;
using AutopilotMonitor.Functions.Security;
using AutopilotMonitor.Functions.Services;
using AutopilotMonitor.Shared;
using AutopilotMonitor.Shared.DataAccess;
using AutopilotMonitor.Shared.Models;
using Microsoft.AspNetCore.Http;
using Microsoft.Azure.Functions.Worker;
using Microsoft.Extensions.Caching.Memory;
using Microsoft.Extensions.Logging.Abstractions;
using Moq;
@@ -1123,6 +1126,126 @@ public class PolicyEnforcementMiddlewareTests
Assert.True(result.Allowed);
}
/// <summary>
/// Operator reads whose ?tenantId= is a handler-side filter for a Global Admin / Global Reader: session reports
/// (GA-only AdminNote, submitter contact data), rule submissions (contact e-mail, reviewer, review comment), a
/// tenant's MCP usage (its accounts and their tools) and verdict calibration (classifier diagnostics, drift
/// alerts). None of it is the managed tenant's telemetry, so a delegated (MSP) reader never reaches them.
/// </summary>
public static TheoryData<string> OperatorFilterReads => new()
{
"/api/global/session-reports",
"/api/global/rule-submissions",
"/api/global/metrics/mcp-usage",
"/api/global/metrics/mcp-usage/daily",
"/api/global/metrics/verdict-calibration",
};
[Theory]
[MemberData(nameof(OperatorFilterReads))]
public async Task Delegated_OperatorFilterReads_WithManagedTenantId_AreForbidden(string path)
{
const string upn = "msp@partner.example";
var h = BuildHarness();
h.AsDelegated(TenantB);
var result = await h.Middleware.DecideAsync("GET", path, TenantB, AuthedPrincipal(TenantA, upn));
Assert.False(result.Allowed);
Assert.Equal(403, result.StatusCode);
}
[Theory]
[MemberData(nameof(OperatorFilterReads))]
public async Task GlobalReader_OperatorFilterReads_WithTenantFilter_StayAllowed(string path)
{
const string upn = "reader@vendor.example";
var h = BuildHarness();
h.AsGlobalRole(Constants.GlobalRoles.GlobalReader);
var result = await h.Middleware.DecideAsync("GET", path, TenantB, AuthedPrincipal(TenantA, upn));
Assert.True(result.Allowed);
Assert.True(result.Context!.IsGlobalReader);
}
// ── Invoke adapter: the tenantId the middleware authorizes is the one the handler reads ──
// Handlers read req.Query (HttpUtility.ParseQueryString), which comma-joins a repeated key. Deciding on the
// first value alone let ?tenantId=B&tenantId=C pass the cross-tenant check on B while the handler read "B,C".
private static async Task<(bool NextCalled, int StatusCode, RequestContext? Context)> InvokeAsync(
Harness h, string path, string queryString, ClaimsPrincipal principal)
{
var http = new DefaultHttpContext();
http.Request.Method = "GET";
http.Request.Path = path;
http.Request.QueryString = new QueryString(queryString);
http.Response.Body = new MemoryStream();
var items = new Dictionary<object, object> { ["HttpRequestContext"] = http, ["ClaimsPrincipal"] = principal };
var context = new Mock<FunctionContext>();
context.SetupGet(c => c.Items).Returns(items);
var nextCalled = false;
await h.Middleware.Invoke(context.Object, _ => { nextCalled = true; return Task.CompletedTask; });
items.TryGetValue(RequestContext.ItemsKey, out var resolved);
return (nextCalled, http.Response.StatusCode, resolved as RequestContext);
}
[Fact]
public async Task Invoke_RepeatedTenantId_DelegatedReader_IsForbidden()
{
// B is managed, C is not — the handler would read "B,C", so the decision is taken on "B,C".
const string tenantC = "33333333-3333-3333-3333-333333333333";
var h = BuildHarness();
h.AsDelegated(TenantB);
var run = await InvokeAsync(h, "/api/global/metrics/fleet-health", $"?tenantId={TenantB}&tenantId={tenantC}",
AuthedPrincipal(TenantA, "msp@partner.example"));
Assert.False(run.NextCalled);
Assert.Equal(403, run.StatusCode);
}
[Fact]
public async Task Invoke_ManagedTenantId_DelegatedReader_IsAdmitted()
{
var h = BuildHarness();
h.AsDelegated(TenantB);
var run = await InvokeAsync(h, "/api/global/metrics/fleet-health", $"?tenantId={TenantB}",
AuthedPrincipal(TenantA, "msp@partner.example"));
Assert.True(run.NextCalled);
Assert.True(run.Context!.IsDelegatedReader);
Assert.Equal(TenantB, run.Context.TargetTenantId);
}
[Theory]
[InlineData("?tenantId=22222222-2222-2222-2222-222222222222")]
[InlineData("?tenantId=22222222-2222-2222-2222-222222222222&tenantId=33333333-3333-3333-3333-333333333333")]
[InlineData("?tenantid=22222222-2222-2222-2222-222222222222&TENANTID=33333333-3333-3333-3333-333333333333")]
[InlineData("?tenantId=&tenantId=33333333-3333-3333-3333-333333333333")]
// Encoded keys: %49 is 'I' on both paths. Only HttpUtility decodes the non-standard %u0049, but the handler
// parses Url.Query, where System.Uri has already escaped the stray '%' — neither path sees a tenantId.
[InlineData("?tenant%49d=22222222-2222-2222-2222-222222222222")]
[InlineData("?tenant%u0049d=22222222-2222-2222-2222-222222222222")]
public async Task Invoke_TargetTenantId_IsTheValueTheHandlerReads(string queryString)
{
const string path = "/api/global/metrics/fleet-health";
var h = BuildHarness();
h.AsGlobalRole(Constants.GlobalRoles.GlobalAdmin);
var run = await InvokeAsync(h, path, queryString, AuthedPrincipal(TenantA, "ga@vendor.example"));
// The handler's view: req.Query over the request Url (QueryParamGuardTests pins that implementation).
var handlerRequest = new Mock<Microsoft.Azure.Functions.Worker.Http.HttpRequestData>(Mock.Of<FunctionContext>()) { CallBase = true };
handlerRequest.SetupGet(r => r.Url).Returns(new Uri("https://localhost" + path + queryString));
var handlerTenantId = handlerRequest.Object.Query["tenantId"];
Assert.True(run.NextCalled);
Assert.Equal(string.IsNullOrWhiteSpace(handlerTenantId) ? TenantA : handlerTenantId, run.Context!.TargetTenantId);
}
[Fact]
public async Task Delegated_CustomsArchive_OnManagedTenant_IsForbidden_ExcludeDelegated()
{
@@ -1274,6 +1397,7 @@ public class PolicyEnforcementMiddlewareTests
[InlineData("GET", "/api/global/distress-reports", TenantScoping.None)]
// ops-events is platform-OPERATIONAL data — GA/Reader-only by intent (not a can't-bound limit), so None.
[InlineData("GET", "/api/global/ops-events", TenantScoping.None)]
// Operator reads with a ?tenantId= filter: OperatorFilterReads above.
public void GlobalRoute_DelegatedAccessibility_MatchesContract(string method, string path, TenantScoping expected)
{
var entry = EndpointAccessPolicyCatalog.FindPolicy(method, path);
@@ -1282,6 +1406,63 @@ public class PolicyEnforcementMiddlewareTests
Assert.Equal(expected, entry.TenantScoping);
}
// ── Catalog ratchet: every platform route a delegated (MSP) caller reaches is a deliberate entry ──
// QueryParam/RouteParam on a GlobalReadOrAdmin route admits a delegated reader of the named tenant, whether the
// author meant an authorization scope or only a handler-side filter — that is how session reports and a tenant's
// MCP accounts once reached MSP callers. A route belongs on this list only if its handler binds to the named
// tenant AND returns that tenant's own data: no operator notes or review comments, no contact data, no account
// lists, no platform-internal diagnostics. Anything else stays None (the handler can still read ?tenantId=
// itself) or carries excludeDelegated. The subset tier admits a delegated caller on the bounded aggregate.
private static readonly string[] DelegatedReachablePlatformRoutes =
{
"GET config/all",
"GET global/apps/analytics",
"GET global/apps/list",
"GET global/apps/sessions",
"GET global/audit/logs",
"GET global/metrics/agent-efficiency",
"GET global/metrics/app",
"GET global/metrics/device-journeys",
"GET global/metrics/fleet-health",
"GET global/metrics/geographic",
"GET global/metrics/geographic/sessions",
"GET global/metrics/rule-stats",
"GET global/metrics/sla",
"GET global/metrics/summary",
"GET global/metrics/time-attribution",
"GET global/metrics/usage",
"GET global/metrics/vulnerability",
"GET global/raw/events",
"GET global/raw/sessions",
"GET global/rules/analyze",
"GET global/rules/gather",
"GET global/search/sessions",
"GET global/search/sessions-by-cve",
"GET global/search/sessions-by-event",
"GET global/session-annotations",
"GET global/sessions",
"GET global/stats/sessions",
"GET vulnerability/software-inventory",
};
[Fact]
public void DelegatedReachablePlatformRoutes_AreTheSanctionedSet()
{
var reachable = EndpointAccessPolicyCatalog.Entries
.Where(e => e.Policy == EndpointPolicy.GlobalReadOrDelegatedSubset
|| (e.Policy == EndpointPolicy.GlobalReadOrAdmin
&& (e.TenantScoping is TenantScoping.QueryParam or TenantScoping.RouteParam)
&& !e.ExcludeDelegated))
.Select(e => $"{e.HttpMethod} {e.RouteTemplate}")
.ToArray();
var added = reachable.Except(DelegatedReachablePlatformRoutes).ToArray();
var removed = DelegatedReachablePlatformRoutes.Except(reachable).ToArray();
Assert.True(added.Length == 0 && removed.Length == 0,
$"Delegated-reachable platform routes changed. Added: [{string.Join(", ", added)}] — decide per the rule " +
$"above (None / excludeDelegated, or add it to the list). Removed: [{string.Join(", ", removed)}].");
}
// ── Catalog contract: platform-operational GA/Reader-only reads carry ExcludeDelegated ──
// The None-equivalent for {tenantId}-RouteParam routes (which the {tenantId}-convention forbids from
// being None): GlobalReadOrAdmin keeps GA + read-only Reader, ExcludeDelegated removes the delegated rescue.
@@ -31,7 +31,8 @@ namespace AutopilotMonitor.Functions.Functions.Reports
try
{
// Authentication + GlobalAdminOnly authorization enforced by PolicyEnforcementMiddleware
// Authentication + GlobalReadOrAdmin authorization enforced by PolicyEnforcementMiddleware. The route is
// unscoped on purpose: ?tenantId= is a filter, and no delegated (MSP) reader may reach the AdminNote.
var callerTenantId = TenantHelper.GetTenantId(req);
var query = req.Query;
@@ -72,7 +72,11 @@ public class PolicyEnforcementMiddleware : IFunctionsWorkerMiddleware
var requestPath = httpContext.Request.Path.Value ?? string.Empty;
var httpMethod = httpContext.Request.Method;
var queryTenantId = httpContext.Request.Query["tenantId"].FirstOrDefault();
// Joined like the handlers' req.Query, which comma-joins a repeated key (empty values included): the
// cross-tenant decision is taken on the very string the handler reads, never on the first of several values.
var queryTenantId = httpContext.Request.Query.TryGetValue("tenantId", out var tenantIdValues)
? string.Join(",", tenantIdValues.ToArray())
: null;
// Sanitized copies for logging only — never used for routing/authorization decisions.
var logPath = LogSanitizer.Clean(requestPath);
@@ -17,7 +17,11 @@ public enum TenantScoping
/// <summary>TenantId comes from {tenantId} route parameter. Middleware enforces cross-tenant check.</summary>
RouteParam,
/// <summary>TenantId comes from ?tenantId= query parameter (optional, falls back to JWT tenant). Middleware enforces cross-tenant check.</summary>
/// <summary>
/// TenantId comes from ?tenantId= query parameter (optional, falls back to JWT tenant). Middleware enforces cross-tenant check.
/// On a delegated read tier (GlobalReadOrAdmin included) this also admits a delegated (MSP) reader of the named tenant —
/// a platform route whose ?tenantId= is only a handler-side filter stays <see cref="None"/>.
/// </summary>
QueryParam,
}
@@ -557,7 +561,9 @@ public static class EndpointAccessPolicyCatalog
new("GET", "global/metrics/fleet-health", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
new("GET", "global/metrics/time-attribution", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
new("GET", "global/metrics/device-journeys", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
new("GET", "global/metrics/verdict-calibration", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
// Operator-only classifier diagnostics and drift alerts; the handler reads ?tenantId= itself. None, so no
// delegated (MSP) reader reaches them.
new("GET", "global/metrics/verdict-calibration", EndpointPolicy.GlobalReadOrAdmin),
new("GET", "global/apps/list", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
new("GET", "global/apps/analytics", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
new("GET", "global/apps/sessions", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
@@ -567,19 +573,24 @@ public static class EndpointAccessPolicyCatalog
new("GET", "global/metrics/sla", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
new("GET", "global/metrics/rule-stats", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
new("GET", "global/metrics/vulnerability", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
new("GET", "global/metrics/mcp-usage", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
new("GET", "global/metrics/mcp-usage/daily", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
// One tenant's organization usage by account (?tenantId= read by the handler): deliberately None, not
// QueryParam — the route enumerates accounts (UPNs), which the delegated path must never reach.
// A named tenant's MCP use — its accounts (UPNs) and their tools, the daily counts, the organization view.
// The handlers read ?tenantId= themselves: deliberately None, not QueryParam, so the delegated path never
// reaches them.
new("GET", "global/metrics/mcp-usage", EndpointPolicy.GlobalReadOrAdmin),
new("GET", "global/metrics/mcp-usage/daily", EndpointPolicy.GlobalReadOrAdmin),
new("GET", "global/metrics/mcp-usage/organization", EndpointPolicy.GlobalReadOrAdmin),
new("GET", "global/distress-reports", EndpointPolicy.GlobalReadOrAdmin),
new("GET", "global/session-reports", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
// The operator's report queue: GA-only AdminNote, the submitter's comment and contact data. ?tenantId= is a
// filter the handler reads itself; None, so no delegated reader reaches it.
new("GET", "global/session-reports", EndpointPolicy.GlobalReadOrAdmin),
// Mints the HMAC ticket for the anonymous proxied stream above (the MCP's report download).
new("POST", "global/session-reports/download-ticket", EndpointPolicy.GlobalReadOrAdmin),
new("GET", "global/session-reports/download-url", EndpointPolicy.GlobalReadOrAdmin),
new("PATCH", "global/session-reports/{reportId}/note", EndpointPolicy.GlobalAdminOnly),
// Community rule submissions, operator side: cross-tenant read (Global Reader included), decision GA-only.
new("GET", "global/rule-submissions", EndpointPolicy.GlobalReadOrAdmin, TenantScoping.QueryParam),
// The list carries the contact e-mail and the review; ?tenantId= is a filter the handler reads itself, None
// so no delegated reader reaches it (the tenant's own list, rules/submissions, is admin-only).
new("GET", "global/rule-submissions", EndpointPolicy.GlobalReadOrAdmin),
new("GET", "global/rule-submissions/{submissionId}", EndpointPolicy.GlobalReadOrAdmin),
new("PATCH", "global/rule-submissions/{submissionId}", EndpointPolicy.GlobalAdminOnly),
new("DELETE", "global/rule-submissions/{submissionId}", EndpointPolicy.GlobalAdminOnly),
@@ -43,7 +43,7 @@ import { fetchJson, jsonBody } from "@/lib/apiClient";
import { useWindowDays } from "@/hooks/useWindowDays";
import { utcDateDaysAgo } from "@/lib/timeWindow";
import { CommunityContributionBox } from "@/components/rules/CommunityContributionBox";
import { MySubmissionsList } from "@/components/rules/MySubmissionsList";
import { MySubmissionsList, isSubmissionsAdminOnly } from "@/components/rules/MySubmissionsList";
import { SubmitRulesModal } from "@/components/rules/SubmitRulesModal";
/** Rule telemetry window without `?days=` — the backend's own rule-stats default (today−30 … today). */
@@ -133,9 +133,7 @@ function AnalyzeRulesContent() {
// A read-only Global Reader — and an own-tenant admin viewing a FOREIGN tenant (cross-tenant override) —
// is read-only. Backend also enforces (rules write is TenantAdminOrGA, cross-tenant blocked for non-GA).
const isReadOnly = !(user?.isGlobalAdmin || (user?.isTenantAdmin && !isGlobalOverride));
// Community submissions are TenantAdminOrGA on the own tenant; only a global-scope override reads them through
// the operator list. Everyone else (Operator, Viewer) gets a hint instead of a 403 on the list call.
const submissionsAdminOnly = isReadOnly && !isGlobalOverride;
const submissionsAdminOnly = isSubmissionsAdminOnly(isReadOnly, scope);
// Cross-tenant write routing: in a Global Admin override the JWT-scoped rules/analyze/{id} route
// resolves the tenant from the caller's token and would silently upsert into the GA's OWN tenant
@@ -25,7 +25,7 @@ import { DocsLink } from "@/components/DocsLink";
import type { TenantFeatureFlagsResponse } from "@/lib/generated/wire-types.generated";
import { fetchJson } from "@/lib/apiClient";
import { CommunityContributionBox } from "@/components/rules/CommunityContributionBox";
import { MySubmissionsList } from "@/components/rules/MySubmissionsList";
import { MySubmissionsList, isSubmissionsAdminOnly } from "@/components/rules/MySubmissionsList";
import { SubmitRulesModal } from "@/components/rules/SubmitRulesModal";
export default function GatherRulesPage() {
@@ -83,9 +83,7 @@ export default function GatherRulesPage() {
// A read-only Global Reader — and an own-tenant admin viewing a FOREIGN tenant (cross-tenant override) —
// is read-only. Backend also enforces (rules write is TenantAdminOrGA, cross-tenant blocked for non-GA).
const isReadOnly = !(user?.isGlobalAdmin || (user?.isTenantAdmin && !isGlobalOverride));
// Community submissions are TenantAdminOrGA on the own tenant; only a global-scope override reads them through
// the operator list. Everyone else (Operator, Viewer) gets a hint instead of a 403 on the list call.
const submissionsAdminOnly = isReadOnly && !isGlobalOverride;
const submissionsAdminOnly = isSubmissionsAdminOnly(isReadOnly, scope);
// Cross-tenant write routing: in a Global Admin override the JWT-scoped rules/gather/{id} route
// resolves the tenant from the caller's token and would silently upsert into the GA's OWN tenant
@@ -30,6 +30,18 @@ export function submissionStatusBadge(status: string): { label: string; cls: str
}
}
/**
* Whether a rule page shows the admin-only hint instead of the list. Submissions are TenantAdminOrGA on the own
* tenant, and only a platform-scope (Global Admin / Global Reader) override reads a foreign tenant's through the
* operator list — a delegated (MSP) reader gets the hint like an Operator or Viewer instead of a 403.
*/
export function isSubmissionsAdminOnly(
isReadOnly: boolean,
scope: { isGlobalOverride: boolean; isDelegatedScope: boolean },
): boolean {
return isReadOnly && !(scope.isGlobalOverride && !scope.isDelegatedScope);
}
/**
* The tenant's own community submissions of one rule kind. Pending rows are the ones an admin
* wants to see, so they show by default; decided and withdrawn rows sit behind "Show all". Renders
@@ -0,0 +1,21 @@
import { describe, it, expect } from "vitest";
import { isSubmissionsAdminOnly } from "../MySubmissionsList";
describe("isSubmissionsAdminOnly", () => {
it("lists a foreign tenant's submissions for a platform-scope override", () => {
expect(isSubmissionsAdminOnly(true, { isGlobalOverride: true, isDelegatedScope: false })).toBe(false);
});
it("shows a delegated (MSP) reader the hint on a managed tenant — the operator list is closed to them", () => {
expect(isSubmissionsAdminOnly(true, { isGlobalOverride: true, isDelegatedScope: true })).toBe(true);
});
it("shows an own-tenant Operator or Viewer the hint", () => {
expect(isSubmissionsAdminOnly(true, { isGlobalOverride: false, isDelegatedScope: false })).toBe(true);
});
it("lists the own tenant's submissions for its admin, also one who is a delegated reader elsewhere", () => {
expect(isSubmissionsAdminOnly(false, { isGlobalOverride: false, isDelegatedScope: false })).toBe(false);
expect(isSubmissionsAdminOnly(false, { isGlobalOverride: false, isDelegatedScope: true })).toBe(false);
});
});