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