gsadmin/AutopilotMonitor
1
0
Fork 0
mirror of https://github.com/okieselbach/AutopilotMonitor.git synced 2026-10-24 04:32:32 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(backend): mcp-usage/user/{oid} filters usage rows to the caller's tenant and never signals foreign existence — UsageCrossTenantGuard skipped rows with an empty TenantId (legacy rows without the column, tokens without a resolvable tid), so a self-service tenant admin of any tenant could read another tenant's user usage (UPN, endpoints/tool names, per-day counts) by oid; and the distinct 403 CrossTenantAccessDenied vs 200-empty made the route a cross-tenant user-existence oracle; FilterForCaller now returns only rows whose TenantId equals the caller's tid for non-global callers (empty TenantId or empty caller tid ⇒ nothing), the handler always answers 200 with the filtered set (foreign oid ≡ unknown oid), dropped rows are logged as an ops warning only; tests rewritten for the filter semantics

Browse Source 
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Oliver Kieselbach
2026-08-30 17:38:16 +02:00
parent 44dd8161df
commit 87cb1f1cea
3 changed files with 90 additions and 89 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Backend/AutopilotMonitor.Functions.Tests/UsageCrossTenantGuardTests.cs
+48 -52
View File
@@ -6,10 +6,13 @@ namespace AutopilotMonitor.Functions.Tests;
/// <summary>
/// Unit tests for the cross-tenant guard on <c>GET /api/metrics/mcp-usage/user/{userId}</c>.
///
/// Background: the route is catalog-policy <c>TenantAdminOrGA</c> with no <c>TenantScoping</c>
/// (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 <c>IUserUsageRepository.GetUsageByUserAsync</c>.
/// Background: the route is catalog-policy <c>TenantAdminOrGlobalReader</c> with no
/// <c>TenantScoping</c> (the path parameter is an Azure AD object id, not a tenant id). The
/// function hands the oid to <c>IUserUsageRepository.GetUsageByUserAsync</c> 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 <c>TenantId</c> alike; (2) the projection is silent, so a foreign oid
/// and an unknown oid both yield an empty set (no cross-tenant existence oracle).
/// </summary>
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<UserUsageRecord>(), 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<UserUsageRecord>(), 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);
}
}
src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs
+17 -18
View File
@@ -99,10 +99,12 @@ namespace AutopilotMonitor.Functions.Functions.Metrics
/// <summary>
/// GET /api/metrics/mcp-usage/user/{userId}?dateFrom=&amp;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 <see cref="UsageCrossTenantGuard"/>). 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.
/// </summary>
[Function("GetMcpUserUsage")]
public async Task<HttpResponseData> 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);
src/Backend/AutopilotMonitor.Functions/Functions/Metrics/UsageCrossTenantGuard.cs
+25 -19
View File
@@ -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 <c>{tenantId}</c> routes. The function
/// therefore inspects the returned <see cref="UserUsageRecord"/>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:
///
/// <list type="bullet">
/// <item>Records that cannot be attributed to the caller's tenant — foreign <c>TenantId</c>
/// 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.</item>
/// <item>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.</item>
/// </list>
/// </summary>
public static class UsageCrossTenantGuard
{
/// <summary>
/// Returns true if the records contain at least one entry whose tenant differs from
/// <paramref name="callerTenantId"/> AND the caller is not a Global Admin.
/// Empty record sets and GA callers always pass (return false).
/// Returns the subset of <paramref name="records"/> the caller may see. Global-scope
/// callers (GA / Global Reader) see everything; everyone else sees only records whose
/// <see cref="UserUsageRecord.TenantId"/> equals <paramref name="callerTenantId"/>
/// (case-insensitive). A caller without a tenant id sees nothing.
/// </summary>
public static bool IsForeignTenantAccess(
IEnumerable<UserUsageRecord> records,
string callerTenantId,
bool isGlobalAdmin)
public static IReadOnlyList<UserUsageRecord> FilterForCaller(
IEnumerable<UserUsageRecord>? 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<UserUsageRecord>();
if (hasGlobalScope) return records.ToList();
if (string.IsNullOrEmpty(callerTenantId)) return Array.Empty<UserUsageRecord>();
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();
}
}