From 87cb1f1ceab95a85c543d4a67ad45c6c7ccb09f5 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Sun, 30 Aug 2026 17:38:16 +0200 Subject: [PATCH] =?UTF-8?q?fix(backend):=20mcp-usage/user/{oid}=20filters?= =?UTF-8?q?=20usage=20rows=20to=20the=20caller's=20tenant=20and=20never=20?= =?UTF-8?q?signals=20foreign=20existence=20=E2=80=94=20UsageCrossTenantGua?= =?UTF-8?q?rd=20skipped=20rows=20with=20an=20empty=20TenantId=20(legacy=20?= =?UTF-8?q?rows=20without=20the=20column,=20tokens=20without=20a=20resolva?= =?UTF-8?q?ble=20tid),=20so=20a=20self-service=20tenant=20admin=20of=20any?= =?UTF-8?q?=20tenant=20could=20read=20another=20tenant's=20user=20usage=20?= =?UTF-8?q?(UPN,=20endpoints/tool=20names,=20per-day=20counts)=20by=20oid;?= =?UTF-8?q?=20and=20the=20distinct=20403=20CrossTenantAccessDenied=20vs=20?= =?UTF-8?q?200-empty=20made=20the=20route=20a=20cross-tenant=20user-existe?= =?UTF-8?q?nce=20oracle;=20FilterForCaller=20now=20returns=20only=20rows?= =?UTF-8?q?=20whose=20TenantId=20equals=20the=20caller's=20tid=20for=20non?= =?UTF-8?q?-global=20callers=20(empty=20TenantId=20or=20empty=20caller=20t?= =?UTF-8?q?id=20=E2=87=92=20nothing),=20the=20handler=20always=20answers?= =?UTF-8?q?=20200=20with=20the=20filtered=20set=20(foreign=20oid=20?= =?UTF-8?q?=E2=89=A1=20unknown=20oid),=20dropped=20rows=20are=20logged=20a?= =?UTF-8?q?s=20an=20ops=20warning=20only;=20tests=20rewritten=20for=20the?= =?UTF-8?q?=20filter=20semantics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- .../UsageCrossTenantGuardTests.cs | 100 +++++++++--------- .../Metrics/McpUsageMetricsFunction.cs | 35 +++--- .../Metrics/UsageCrossTenantGuard.cs | 44 ++++---- 3 files changed, 90 insertions(+), 89 deletions(-) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/UsageCrossTenantGuardTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/UsageCrossTenantGuardTests.cs index 7f4d4896..52082ff0 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/UsageCrossTenantGuardTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/UsageCrossTenantGuardTests.cs @@ -6,10 +6,13 @@ namespace AutopilotMonitor.Functions.Tests; /// /// Unit tests for the cross-tenant guard on GET /api/metrics/mcp-usage/user/{userId}. /// -/// Background: the route is catalog-policy TenantAdminOrGA with no TenantScoping -/// (the path parameter is an Azure AD object id, not a tenant id). Without this guard, a tenant -/// admin from tenant A could read MCP usage records of any oid — including oids in tenant B — -/// because the function blindly hands the oid to IUserUsageRepository.GetUsageByUserAsync. +/// Background: the route is catalog-policy TenantAdminOrGlobalReader with no +/// TenantScoping (the path parameter is an Azure AD object id, not a tenant id). The +/// function hands the oid to IUserUsageRepository.GetUsageByUserAsync and then projects +/// the result down to the caller's tenant. Two invariants are pinned here: +/// (1) a non-global caller never receives a row that is not attributed to their own tenant — +/// foreign AND empty TenantId alike; (2) the projection is silent, so a foreign oid +/// and an unknown oid both yield an empty set (no cross-tenant existence oracle). /// public class UsageCrossTenantGuardTests { @@ -19,7 +22,7 @@ public class UsageCrossTenantGuardTests private static UserUsageRecord Rec(string tid, string endpoint = "ep") => new() { UserId = "oid-1", - UserPrincipalName = "alice@contoso.com", + UserPrincipalName = "alice@example.com", TenantId = tid, Endpoint = endpoint, Date = "20260505", @@ -27,100 +30,93 @@ public class UsageCrossTenantGuardTests }; [Fact] - public void OwnTenantOnly_NonGA_Allowed() + public void OwnTenantOnly_NonGlobal_AllReturned() { var records = new[] { Rec(TenantA), Rec(TenantA, "ep2") }; - var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + var visible = UsageCrossTenantGuard.FilterForCaller(records, TenantA, hasGlobalScope: false); - Assert.False(blocked); + Assert.Equal(2, visible.Count); + Assert.All(visible, r => Assert.Equal(TenantA, r.TenantId)); } [Fact] - public void ForeignTenantOnly_NonGA_Blocked() + public void ForeignTenantOnly_NonGlobal_EmptyLikeUnknownOid() { - var records = new[] { Rec(TenantB), Rec(TenantB, "ep2") }; + // Oracle closure: a foreign oid must look exactly like an oid with no usage at all. + var foreign = UsageCrossTenantGuard.FilterForCaller(new[] { Rec(TenantB), Rec(TenantB, "ep2") }, TenantA, hasGlobalScope: false); + var unknown = UsageCrossTenantGuard.FilterForCaller(Array.Empty(), TenantA, hasGlobalScope: false); - var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); - - Assert.True(blocked); + Assert.Empty(foreign); + Assert.Empty(unknown); } [Fact] - public void MixedTenants_NonGA_Blocked() + public void MixedTenants_NonGlobal_OnlyOwnRowsReturned() { - // Even one foreign record must trip the guard — a tenant admin must not see another tenant's row. - var records = new[] { Rec(TenantA), Rec(TenantB) }; + var records = new[] { Rec(TenantA), Rec(TenantB), Rec(TenantA, "ep2") }; - var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + var visible = UsageCrossTenantGuard.FilterForCaller(records, TenantA, hasGlobalScope: false); - Assert.True(blocked); + Assert.Equal(2, visible.Count); + Assert.DoesNotContain(visible, r => string.Equals(r.TenantId, TenantB, StringComparison.OrdinalIgnoreCase)); } [Fact] - public void ForeignTenant_GlobalAdmin_Allowed() + public void ForeignTenant_GlobalScope_AllReturned() { - // GA can see usage across all tenants. - var records = new[] { Rec(TenantB) }; + // GA / Global Reader can see usage across all tenants — including unattributed rows. + var records = new[] { Rec(TenantB), Rec("") }; - var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: true); + var visible = UsageCrossTenantGuard.FilterForCaller(records, TenantA, hasGlobalScope: true); - Assert.False(blocked); + Assert.Equal(2, visible.Count); } [Fact] - public void EmptyRecords_NonGA_Allowed() + public void NullRecords_ReturnsEmpty() { - // No records → caller learns nothing; safe to return 200 with empty list. - var blocked = UsageCrossTenantGuard.IsForeignTenantAccess( - Array.Empty(), TenantA, isGlobalAdmin: false); - - Assert.False(blocked); + var visible = UsageCrossTenantGuard.FilterForCaller(null, TenantA, hasGlobalScope: false); + Assert.Empty(visible); } [Fact] - public void NullRecords_DoesNotThrow() + public void RecordWithEmptyTenantId_NonGlobal_Excluded() { - var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(null!, TenantA, isGlobalAdmin: false); - Assert.False(blocked); - } - - [Fact] - public void RecordWithEmptyTenantId_Ignored() - { - // Defensive: legacy records without TenantId must not be treated as a leak signal - // (they could belong to anyone). Other records still drive the decision. + // Tenant-less rows exist by construction (legacy rows without the TenantId column, + // tokens without a resolvable tid). They cannot be attributed to the caller's tenant, + // so a tenant admin of ANY tenant must not receive them. var records = new[] { - Rec(""), // unknown tenant — ignored - Rec(TenantA), // own tenant + Rec(""), // unattributed — dropped + Rec(TenantA), // own tenant — kept }; - var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + var visible = UsageCrossTenantGuard.FilterForCaller(records, TenantA, hasGlobalScope: false); - Assert.False(blocked); + Assert.Single(visible); + Assert.Equal(TenantA, visible[0].TenantId); } [Fact] - public void CallerTenantIdEmpty_NotBlocked() + public void CallerTenantIdEmpty_NonGlobal_ReturnsNothing() { - // Anonymous / device path scenarios shouldn't reach this code — but if they do, - // we don't synthesize a 403 from missing claims; let the catalog policy handle auth. - var records = new[] { Rec(TenantA) }; + // A caller without a tid cannot own any row — fail closed rather than open. + var records = new[] { Rec(TenantA), Rec("") }; - var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, "", isGlobalAdmin: false); + var visible = UsageCrossTenantGuard.FilterForCaller(records, "", hasGlobalScope: false); - Assert.False(blocked); + Assert.Empty(visible); } [Fact] public void TenantIdComparison_IsCaseInsensitive() { - // Azure AD tids are GUIDs; some sources upper-case them. Comparison must not flag those as foreign. + // Azure AD tids are GUIDs; some sources upper-case them. Comparison must not drop those as foreign. var records = new[] { Rec(TenantA.ToUpperInvariant()) }; - var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + var visible = UsageCrossTenantGuard.FilterForCaller(records, TenantA, hasGlobalScope: false); - Assert.False(blocked); + Assert.Single(visible); } } diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs index 31bbabea..6b1dbb99 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs @@ -99,10 +99,12 @@ namespace AutopilotMonitor.Functions.Functions.Metrics /// /// GET /api/metrics/mcp-usage/user/{userId}?dateFrom=&dateTo= — Usage for a specific user. /// - /// Catalog policy is TenantAdminOrGA, but the route has no TenantScoping — middleware can't - /// enforce cross-tenant access since {userId} is an Azure AD object id, not a tenant id. - /// We therefore enforce here: if the records' TenantId differs from the caller's tenant - /// (and the caller is not a Global Admin), block with 403. + /// Catalog policy is TenantAdminOrGlobalReader, but the route has no TenantScoping — middleware + /// can't enforce cross-tenant access since {userId} is an Azure AD object id, not a tenant id. + /// We therefore enforce here: non-global callers receive only the records attributed to their + /// own tenant (see ). The response is always 200 — a foreign + /// oid and an unknown oid are indistinguishable, so the route is not a cross-tenant + /// user-existence oracle. /// [Function("GetMcpUserUsage")] public async Task GetUserUsage( @@ -116,27 +118,24 @@ namespace AutopilotMonitor.Functions.Functions.Metrics var dateFrom = req.Query["dateFrom"]; var dateTo = req.Query["dateTo"]; - var records = await _userUsageRepo.GetUsageByUserAsync(userId, dateFrom, dateTo); + var allRecords = await _userUsageRepo.GetUsageByUserAsync(userId, dateFrom, dateTo); var ctx = req.GetRequestContext(); - if (UsageCrossTenantGuard.IsForeignTenantAccess(records, ctx.TenantId, ctx.HasGlobalScope)) + var records = UsageCrossTenantGuard.FilterForCaller(allRecords, ctx.TenantId, ctx.HasGlobalScope); + + var dropped = allRecords.Count - records.Count; + if (dropped > 0) { - var foundTenants = records + // Ops signal only — the response shape is identical to an unknown oid. + var foreignTenants = allRecords .Select(r => r.TenantId) - .Where(t => !string.IsNullOrEmpty(t)) + .Where(t => !string.IsNullOrEmpty(t) + && !string.Equals(t, ctx.TenantId, StringComparison.OrdinalIgnoreCase)) .Distinct(StringComparer.OrdinalIgnoreCase) .ToArray(); _logger.LogWarning( - "[McpUsage] Blocked cross-tenant access: caller={Caller} callerTid={CallerTid} targetUser={UserId} foundTenants={Tenants}", - ctx.UserPrincipalName, ctx.TenantId, userId, string.Join(",", foundTenants)); - - var forbidden = req.CreateResponse(HttpStatusCode.Forbidden); - await forbidden.WriteAsJsonAsync(new - { - error = "CrossTenantAccessDenied", - message = "Access denied. You can only access usage for users in your own tenant." - }); - return forbidden; + "[McpUsage] Filtered cross-tenant usage rows: caller={Caller} callerTid={CallerTid} targetUser={UserId} dropped={Dropped} foreignTenants={Tenants}", + ctx.UserPrincipalName, ctx.TenantId, userId, dropped, string.Join(",", foreignTenants)); } var response = req.CreateResponse(HttpStatusCode.OK); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/UsageCrossTenantGuard.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/UsageCrossTenantGuard.cs index 9682da89..42aad485 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/UsageCrossTenantGuard.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/UsageCrossTenantGuard.cs @@ -7,31 +7,37 @@ namespace AutopilotMonitor.Functions.Functions.Metrics; /// /// The route accepts an Azure AD object id (oid), which has no inherent tenant scoping — /// middleware can't validate it the way it does for {tenantId} routes. The function -/// therefore inspects the returned s and blocks the response -/// if any record belongs to a tenant other than the caller's, unless the caller is a Global Admin. +/// therefore projects the repository result down to the caller's own tenant before it is +/// returned. Two properties matter: +/// +/// +/// Records that cannot be attributed to the caller's tenant — foreign TenantId +/// OR an empty one (legacy rows / tokens without a resolvable tid) — are dropped for +/// non-global callers. Unattributed data is not the caller's data. +/// The filter is silent: a foreign oid and an unknown oid both yield an empty set, so +/// the response cannot be used as an existence oracle for users of other tenants. +/// /// public static class UsageCrossTenantGuard { /// - /// Returns true if the records contain at least one entry whose tenant differs from - /// AND the caller is not a Global Admin. - /// Empty record sets and GA callers always pass (return false). + /// Returns the subset of the caller may see. Global-scope + /// callers (GA / Global Reader) see everything; everyone else sees only records whose + /// equals + /// (case-insensitive). A caller without a tenant id sees nothing. /// - public static bool IsForeignTenantAccess( - IEnumerable records, - string callerTenantId, - bool isGlobalAdmin) + public static IReadOnlyList FilterForCaller( + IEnumerable? records, + string? callerTenantId, + bool hasGlobalScope) { - if (isGlobalAdmin) return false; - if (records == null) return false; - if (string.IsNullOrEmpty(callerTenantId)) return false; + if (records == null) return Array.Empty(); + if (hasGlobalScope) return records.ToList(); + if (string.IsNullOrEmpty(callerTenantId)) return Array.Empty(); - foreach (var r in records) - { - if (string.IsNullOrEmpty(r.TenantId)) continue; - if (!string.Equals(r.TenantId, callerTenantId, StringComparison.OrdinalIgnoreCase)) - return true; - } - return false; + return records + .Where(r => !string.IsNullOrEmpty(r.TenantId) + && string.Equals(r.TenantId, callerTenantId, StringComparison.OrdinalIgnoreCase)) + .ToList(); } }