diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/UsageCrossTenantGuardTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/UsageCrossTenantGuardTests.cs new file mode 100644 index 00000000..7f4d4896 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/UsageCrossTenantGuardTests.cs @@ -0,0 +1,126 @@ +using AutopilotMonitor.Functions.Functions.Metrics; +using AutopilotMonitor.Shared.DataAccess; + +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. +/// +public class UsageCrossTenantGuardTests +{ + private const string TenantA = "00000000-0000-0000-0000-aaaaaaaaaaaa"; + private const string TenantB = "00000000-0000-0000-0000-bbbbbbbbbbbb"; + + private static UserUsageRecord Rec(string tid, string endpoint = "ep") => new() + { + UserId = "oid-1", + UserPrincipalName = "alice@contoso.com", + TenantId = tid, + Endpoint = endpoint, + Date = "20260505", + RequestCount = 1, + }; + + [Fact] + public void OwnTenantOnly_NonGA_Allowed() + { + var records = new[] { Rec(TenantA), Rec(TenantA, "ep2") }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + + Assert.False(blocked); + } + + [Fact] + public void ForeignTenantOnly_NonGA_Blocked() + { + var records = new[] { Rec(TenantB), Rec(TenantB, "ep2") }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + + Assert.True(blocked); + } + + [Fact] + public void MixedTenants_NonGA_Blocked() + { + // 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 blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + + Assert.True(blocked); + } + + [Fact] + public void ForeignTenant_GlobalAdmin_Allowed() + { + // GA can see usage across all tenants. + var records = new[] { Rec(TenantB) }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: true); + + Assert.False(blocked); + } + + [Fact] + public void EmptyRecords_NonGA_Allowed() + { + // No records → caller learns nothing; safe to return 200 with empty list. + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess( + Array.Empty(), TenantA, isGlobalAdmin: false); + + Assert.False(blocked); + } + + [Fact] + public void NullRecords_DoesNotThrow() + { + 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. + var records = new[] + { + Rec(""), // unknown tenant — ignored + Rec(TenantA), // own tenant + }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + + Assert.False(blocked); + } + + [Fact] + public void CallerTenantIdEmpty_NotBlocked() + { + // 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) }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, "", isGlobalAdmin: false); + + Assert.False(blocked); + } + + [Fact] + public void TenantIdComparison_IsCaseInsensitive() + { + // Azure AD tids are GUIDs; some sources upper-case them. Comparison must not flag those as foreign. + var records = new[] { Rec(TenantA.ToUpperInvariant()) }; + + var blocked = UsageCrossTenantGuard.IsForeignTenantAccess(records, TenantA, isGlobalAdmin: false); + + Assert.False(blocked); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetAppMetricsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetAppMetricsFunction.cs index bd15970c..6dbb18da 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetAppMetricsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetAppMetricsFunction.cs @@ -28,12 +28,15 @@ namespace AutopilotMonitor.Functions.Functions.Metrics var tenantId = TenantHelper.GetTenantId(req); _logger.LogInformation($"Fetching app metrics for tenant {tenantId}"); - // Optional time filter: ?days=7 (default: 30) + // Optional time filter: ?days=7 (default: 30, clamped to [1, 365] for consistency + // with metrics/summary, metrics/usage, and metrics/platform). var query = System.Web.HttpUtility.ParseQueryString(req.Url.Query); var daysParam = query["days"]; int days = 30; if (!string.IsNullOrEmpty(daysParam) && int.TryParse(daysParam, out var parsedDays) && parsedDays > 0) days = parsedDays; + if (days < 1) days = 1; + if (days > 365) days = 365; var cutoff = DateTime.UtcNow.AddDays(-days); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicLocationSessionsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicLocationSessionsFunction.cs index 51054d2f..7e071625 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicLocationSessionsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicLocationSessionsFunction.cs @@ -46,6 +46,8 @@ namespace AutopilotMonitor.Functions.Functions.Metrics int days = 30; if (!string.IsNullOrEmpty(daysParam) && int.TryParse(daysParam, out var parsedDays) && parsedDays > 0) days = parsedDays; + if (days < 1) days = 1; + if (days > 365) days = 365; var groupBy = query["groupBy"] ?? "city"; diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicMetricsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicMetricsFunction.cs index 432c80c8..db696ed6 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicMetricsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetGeographicMetricsFunction.cs @@ -39,6 +39,8 @@ namespace AutopilotMonitor.Functions.Functions.Metrics int days = 30; if (!string.IsNullOrEmpty(daysParam) && int.TryParse(daysParam, out var parsedDays) && parsedDays > 0) days = parsedDays; + if (days < 1) days = 1; + if (days > 365) days = 365; var groupBy = query["groupBy"] ?? "city"; diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs index d62c7de6..ee6ef382 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/McpUsageMetricsFunction.cs @@ -7,6 +7,8 @@ using Microsoft.Azure.Functions.Worker; using Microsoft.Azure.Functions.Worker.Http; using Microsoft.Extensions.Logging; +// Cross-tenant guard helpers — exposed for unit testing. + namespace AutopilotMonitor.Functions.Functions.Metrics { /// @@ -75,7 +77,12 @@ namespace AutopilotMonitor.Functions.Functions.Metrics } /// - /// GET /api/metrics/mcp-usage/user/{userId}?dateFrom=&dateTo= — Usage for a specific user + /// 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. /// [Function("GetMcpUserUsage")] public async Task GetUserUsage( @@ -91,6 +98,27 @@ namespace AutopilotMonitor.Functions.Functions.Metrics var records = await _userUsageRepo.GetUsageByUserAsync(userId, dateFrom, dateTo); + var ctx = req.GetRequestContext(); + if (UsageCrossTenantGuard.IsForeignTenantAccess(records, ctx.TenantId, ctx.IsGlobalAdmin)) + { + var foundTenants = records + .Select(r => r.TenantId) + .Where(t => !string.IsNullOrEmpty(t)) + .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; + } + var response = req.CreateResponse(HttpStatusCode.OK); await response.WriteAsJsonAsync(new { userId, records }); return response; diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/UsageCrossTenantGuard.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/UsageCrossTenantGuard.cs new file mode 100644 index 00000000..9682da89 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/UsageCrossTenantGuard.cs @@ -0,0 +1,37 @@ +using AutopilotMonitor.Shared.DataAccess; + +namespace AutopilotMonitor.Functions.Functions.Metrics; + +/// +/// Cross-tenant access guard for GET /api/metrics/mcp-usage/user/{userId}. +/// +/// 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. +/// +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). + /// + public static bool IsForeignTenantAccess( + IEnumerable records, + string callerTenantId, + bool isGlobalAdmin) + { + if (isGlobalAdmin) return false; + if (records == null) return false; + if (string.IsNullOrEmpty(callerTenantId)) return false; + + foreach (var r in records) + { + if (string.IsNullOrEmpty(r.TenantId)) continue; + if (!string.Equals(r.TenantId, callerTenantId, StringComparison.OrdinalIgnoreCase)) + return true; + } + return false; + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByCveFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByCveFunction.cs index 18d719bf..da251cb7 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByCveFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByCveFunction.cs @@ -54,7 +54,7 @@ public class SearchSessionsByCveFunction double? minCvssScore = double.TryParse(query["minCvssScore"], out var mcs) ? mcs : null; var overallRisk = query["overallRisk"]; - var limit = int.TryParse(query["limit"], out var lim) ? Math.Min(lim, 100) : 50; + var limit = int.TryParse(query["limit"], out var lim) ? Math.Clamp(lim, 1, 100) : 50; var sessions = await _sessionRepo.SearchSessionsByCveAsync(tenantId, cveId, minCvssScore, overallRisk, limit); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByEventFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByEventFunction.cs index cabd1495..dd34bddd 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByEventFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsByEventFunction.cs @@ -52,7 +52,7 @@ public class SearchSessionsByEventFunction return badReq; } - var limit = int.TryParse(query["limit"], out var lim) ? Math.Min(lim, 100) : 50; + var limit = int.TryParse(query["limit"], out var lim) ? Math.Clamp(lim, 1, 100) : 50; var sessions = await _sessionRepo.SearchSessionsByEventAsync(tenantId, eventType, null, null, null, limit); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsFunction.cs index 8885f3ca..ad173ce2 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/SearchSessionsFunction.cs @@ -58,7 +58,7 @@ public class SearchSessionsFunction GeoCountry = query["geoCountry"], AgentVersion = query["agentVersion"], ImeAgentVersion = query["imeAgentVersion"], - Limit = int.TryParse(query["limit"], out var lim) ? Math.Min(lim, 100) : 50, + Limit = int.TryParse(query["limit"], out var lim) ? Math.Clamp(lim, 1, 100) : 50, }; if (bool.TryParse(query["isPreProvisioned"], out var ipp)) filter.IsPreProvisioned = ipp; diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts index 980a4411..4e963b6d 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts @@ -25,7 +25,7 @@ export function registerAdminTools(server: McpServer): void { let data: unknown; const params: Record = { tenantId: args.tenantId, dateFrom: args.dateFrom, dateTo: args.dateTo }; if (args.userId) { - data = await apiFetch(`/api/metrics/mcp-usage/${encodeURIComponent(args.userId)}${buildQuery(params)}`); + data = await apiFetch(`/api/metrics/mcp-usage/user/${encodeURIComponent(args.userId)}${buildQuery(params)}`); } else if (args.daily) { data = await apiFetch(`/api/global/metrics/mcp-usage/daily${buildQuery(params)}`); } else { @@ -43,10 +43,12 @@ export function registerAdminTools(server: McpServer): void { 'get_geographic_metrics', 'Get geographic distribution of enrollments — where devices are enrolling from, with performance comparisons. ' + 'Shows per-location: session counts, success rates, avg/median/p95 duration, throughput, and outlier detection. ' + - 'Omit tenantId for cross-tenant view (Global Admin). Use get_geographic_sessions to drill into a specific location.', + 'Omit tenantId for cross-tenant view (Global Admin). Use get_geographic_sessions to drill into a specific location. ' + + 'days accepts any value 1-365 (e.g. 5, 7, 12, 30, 90).', { tenantId: z.string().optional().describe('Tenant ID. Omit for cross-tenant view (Global Admin only).'), - days: z.coerce.number().optional().default(30).describe('Time range in days (default: 30)'), + days: z.coerce.number().int().min(1).max(365).optional().default(30) + .describe('Time range in days (1-365). Defaults to 30.'), groupBy: z.enum(['country', 'region', 'city']).optional().default('city') .describe('Geographic grouping level (default: "city")'), }, @@ -77,7 +79,8 @@ export function registerAdminTools(server: McpServer): void { region: z.string().optional().describe('Region/state filter (e.g. "Saxony", "North Carolina"). Used with country.'), city: z.string().optional().describe('City filter (e.g. "Falkenstein"). Used with country.'), tenantId: z.string().optional().describe('Tenant ID. Omit for cross-tenant view (Global Admin only).'), - days: z.coerce.number().optional().default(30).describe('Time range in days (default: 30)'), + days: z.coerce.number().int().min(1).max(365).optional().default(30) + .describe('Time range in days (1-365). Defaults to 30.'), }, READ_ONLY, async (args) => withToolTelemetry('get_geographic_sessions', async () => { diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts index b2575951..e518a759 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts @@ -378,9 +378,10 @@ export function registerSessionTools(server: McpServer): void { server.tool( 'list_blocked_devices', 'List devices currently blocked from enrolling. Blocked devices have their enrollment sessions rejected by the backend. ' + - 'Omit tenantId to list blocked devices across ALL tenants (requires Global Admin).', + 'Global Admin only — both the tenant-scoped (?tenantId=) and cross-tenant variants of this endpoint require Global Admin. ' + + 'Tenant Admins and Operators receive 403 (the backend manages the device block list as a platform-wide concern).', { - tenantId: z.string().optional().describe('Tenant ID to scope results. Omit for cross-tenant listing (Global Admin only).'), + tenantId: z.string().optional().describe('Tenant ID to scope results. Optional — both forms require Global Admin.'), }, READ_ONLY, async (args) => withToolTelemetry('list_blocked_devices', async () => {