From aad3522dab8ede2ca0365bf5cfb6b8ada475aea2 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Fri, 26 Jun 2026 18:59:02 +0200 Subject: [PATCH] fix(delegated): bound the aggregate fallback + correct override-page banner subtitle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings. High — delegated bounded aggregate could fall back to an unbounded all-tenant scan: FetchAllSessionsPageInternalAsync filters the config tenant list by allowedTenantIds, but when that intersection is empty (managed tenant has no config row, config momentarily empty, or a casing mismatch) it still invoked the legacy unbounded primary-table scan — leaking ALL tenants' sessions to a delegated caller on the no-tenantId /api/global/sessions path. Fix: when allowedTenantIds != null, return an empty page and NEVER touch the primary-table fallback (which stays the GA all-tenants safety net). New DelegatedBoundedAggregateTests proves both: bounded+empty → empty with the Sessions table QueryAsync never called; unbounded(GA)+empty config → fallback runs. Low — override-only pages showed "Delegated Admin View — access to all tenants": usage-metrics, sla, gather-rules, analyze-rules rendered the banner without a subtitle, defaulting to the all-tenants wording that contradicts the bounded model. Now pass globalAdminSubtitle(scope) so a delegated (always a concrete managed tenant) reads "viewing tenant ", matching the aggregated pages. Backend 2486 green, web 481 green + tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../DelegatedBoundedAggregateTests.cs | 107 ++++++++++++++++++ .../Services/TableStorageService.Sessions.cs | 7 ++ .../app/analyze-rules/page.tsx | 4 +- .../app/gather-rules/page.tsx | 4 +- .../autopilot-monitor-web/app/sla/page.tsx | 4 +- .../app/usage-metrics/page.tsx | 4 +- 6 files changed, 122 insertions(+), 8 deletions(-) create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/DelegatedBoundedAggregateTests.cs diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/DelegatedBoundedAggregateTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/DelegatedBoundedAggregateTests.cs new file mode 100644 index 00000000..d411afdf --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/DelegatedBoundedAggregateTests.cs @@ -0,0 +1,107 @@ +using System; +using System.Collections.Generic; +using System.Linq.Expressions; +using System.Threading; +using System.Threading.Tasks; +using Azure; +using Azure.Data.Tables; +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Security guard for the delegated ("MSP") bounded aggregate. GetAllSessionsPageAsync with a non-null +/// allowedTenantIds must NEVER fall back to the unbounded primary-table scan when the managed set resolves +/// to no in-config tenants — otherwise /api/global/sessions (no tenantId), now admitted for a delegated +/// caller, could leak ALL tenants' sessions when a managed tenant has no config row, config is momentarily +/// empty, or a casing mismatch yields no overlap. +/// +public class DelegatedBoundedAggregateTests +{ + private const string ConfigTenant = "11111111-1111-1111-1111-111111111111"; + private const string ManagedTenant = "22222222-2222-2222-2222-222222222222"; + + [Fact] + public async Task BoundedAggregate_EmptyManagedSet_ReturnsEmpty_NeverUnboundedScan() + { + // Config knows only ConfigTenant; the delegated caller manages a DIFFERENT tenant → bounded set is empty. + var configClient = MockTableClientReturning(new TableEntity(ConfigTenant, "")); + // Sessions table backs the UNBOUNDED fallback — it must never be queried for a bounded request. + var sessionsClient = MockTableClientReturning(new TableEntity("99999999-9999-9999-9999-999999999999", "leak")); + var indexClient = new Mock(); + + var storage = BuildStorage(configClient, sessionsClient, indexClient); + + var page = await storage.GetAllSessionsPageAsync( + tenantIdFilter: null, days: null, pageSize: 10, continuation: null, + allowedTenantIds: new[] { ManagedTenant }); + + Assert.Empty(page.Items); + AssertNeverQueried(sessionsClient); + } + + [Fact] + public async Task UnboundedAggregate_EmptyConfig_StillUsesPrimaryTableFallback() + { + // Sanity: with allowedTenantIds == null (GA all-tenants) and empty config, the legacy fallback DOES run — + // proving it is the bounded guard, not some unrelated gate, that suppresses the scan above. + var configClient = MockTableClientReturning(); + var sessionsClient = MockTableClientReturning(); + var indexClient = new Mock(); + + var storage = BuildStorage(configClient, sessionsClient, indexClient); + + var page = await storage.GetAllSessionsPageAsync( + tenantIdFilter: null, days: null, pageSize: 10, continuation: null, allowedTenantIds: null); + + Assert.Empty(page.Items); + sessionsClient.Verify(c => c.QueryAsync( + It.IsAny(), It.IsAny(), It.IsAny>(), It.IsAny()), + Times.AtLeastOnce()); + } + + // ── helpers ────────────────────────────────────────────────────────────── + + private static TableStorageService BuildStorage( + Mock config, Mock sessions, Mock index) + { + var serviceClient = new Mock(); + serviceClient.Setup(s => s.GetTableClient(Constants.TableNames.TenantConfiguration)).Returns(config.Object); + serviceClient.Setup(s => s.GetTableClient(Constants.TableNames.Sessions)).Returns(sessions.Object); + serviceClient.Setup(s => s.GetTableClient(Constants.TableNames.SessionsIndex)).Returns(index.Object); + return new TableStorageService(serviceClient.Object, NullLogger.Instance); + } + + /// Mocks both QueryAsync overloads (string + expression filter) to return the given rows. + private static Mock MockTableClientReturning(params TableEntity[] rows) + { + var m = new Mock(); + m.Setup(c => c.QueryAsync( + It.IsAny(), It.IsAny(), It.IsAny>(), It.IsAny())) + .Returns(() => AsAsyncPageable(rows)); + m.Setup(c => c.QueryAsync( + It.IsAny>>(), It.IsAny(), It.IsAny>(), It.IsAny())) + .Returns(() => AsAsyncPageable(rows)); + return m; + } + + private static void AssertNeverQueried(Mock client) + { + client.Verify(c => c.QueryAsync( + It.IsAny(), It.IsAny(), It.IsAny>(), It.IsAny()), + Times.Never()); + client.Verify(c => c.QueryAsync( + It.IsAny>>(), It.IsAny(), It.IsAny>(), It.IsAny()), + Times.Never()); + } + + private static AsyncPageable AsAsyncPageable(TableEntity[] entities) + { + var page = Page.FromValues(entities, continuationToken: null, new Mock().Object); + return AsyncPageable.FromPages(new[] { page }); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs index 4059aa62..9950c069 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs @@ -1101,6 +1101,13 @@ namespace AutopilotMonitor.Functions.Services if (tenantIds.Count == 0 && string.IsNullOrEmpty(cursor)) { + // A BOUNDED (delegated/MSP) request must NEVER fall back to the unbounded primary-table + // scan: an empty allowed set (managed tenant has no config row, config momentarily empty, + // or a casing mismatch) means "none of YOUR tenants" → empty page, not ALL tenants. The + // primary-table fallback is only the GA all-tenants safety net for a fresh/empty config. + if (allowedTenantIds != null) + return (new List(), false, null); + var fallback = await FetchAllSessionsFromPrimaryTableInternalAsync(maxResults, days); return (fallback.Sessions, fallback.HasMore, NextCursor: null); } diff --git a/src/Web/autopilot-monitor-web/app/analyze-rules/page.tsx b/src/Web/autopilot-monitor-web/app/analyze-rules/page.tsx index 684427b4..edae2e39 100644 --- a/src/Web/autopilot-monitor-web/app/analyze-rules/page.tsx +++ b/src/Web/autopilot-monitor-web/app/analyze-rules/page.tsx @@ -14,7 +14,7 @@ import { RuleFilterBar } from "@/components/rules/RuleFilterBar"; import { EmptyState } from "@/components/rules/EmptyState"; import { FormJsonToggle, JsonModeToggleButtons } from "@/components/rules/FormJsonToggle"; import { useAuthenticatedFetch, useNotificationMessages, useGlobalAdminScope } from "@/hooks"; -import { GlobalAdminBanner } from "@/components/GlobalAdminBanner"; +import { GlobalAdminBanner, globalAdminSubtitle } from "@/components/GlobalAdminBanner"; import { TenantScopeSelector } from "@/components/TenantScopeSelector"; import { isTemplateRule } from "@/lib/analyzeRuleTabs"; @@ -407,7 +407,7 @@ export default function AnalyzeRulesPage() { return (
- + {/* Header */}
diff --git a/src/Web/autopilot-monitor-web/app/gather-rules/page.tsx b/src/Web/autopilot-monitor-web/app/gather-rules/page.tsx index a3339789..c02aa0e6 100644 --- a/src/Web/autopilot-monitor-web/app/gather-rules/page.tsx +++ b/src/Web/autopilot-monitor-web/app/gather-rules/page.tsx @@ -14,7 +14,7 @@ import { RuleFilterBar } from "@/components/rules/RuleFilterBar"; import { EmptyState } from "@/components/rules/EmptyState"; import { FormJsonToggle, JsonModeToggleButtons } from "@/components/rules/FormJsonToggle"; import { useAuthenticatedFetch, useNotificationMessages, useGlobalAdminScope } from "@/hooks"; -import { GlobalAdminBanner } from "@/components/GlobalAdminBanner"; +import { GlobalAdminBanner, globalAdminSubtitle } from "@/components/GlobalAdminBanner"; import { TenantScopeSelector } from "@/components/TenantScopeSelector"; import { GatherRule, NewRuleForm, EMPTY_FORM, CATEGORY_COLORS } from "./types"; import { GatherRuleFormFields } from "./components/GatherRuleFormFields"; @@ -362,7 +362,7 @@ export default function GatherRulesPage() { return (
- + {/* Header */}
diff --git a/src/Web/autopilot-monitor-web/app/sla/page.tsx b/src/Web/autopilot-monitor-web/app/sla/page.tsx index 181030a9..7f88a6d6 100644 --- a/src/Web/autopilot-monitor-web/app/sla/page.tsx +++ b/src/Web/autopilot-monitor-web/app/sla/page.tsx @@ -23,7 +23,7 @@ const AppLineChart = dynamic(() => import("@/components/charts/AppLineChart"), { }); import { trackEvent } from "@/lib/appInsights"; import { useGlobalAdminScope } from "@/hooks"; -import { GlobalAdminBanner } from "@/components/GlobalAdminBanner"; +import { GlobalAdminBanner, globalAdminSubtitle } from "@/components/GlobalAdminBanner"; import { TenantScopeSelector } from "@/components/TenantScopeSelector"; import Link from "next/link"; @@ -182,7 +182,7 @@ export default function SlaPage() { return (
- + {/* Header */}
diff --git a/src/Web/autopilot-monitor-web/app/usage-metrics/page.tsx b/src/Web/autopilot-monitor-web/app/usage-metrics/page.tsx index 10139d58..5cef24b9 100644 --- a/src/Web/autopilot-monitor-web/app/usage-metrics/page.tsx +++ b/src/Web/autopilot-monitor-web/app/usage-metrics/page.tsx @@ -8,7 +8,7 @@ import { ProtectedRoute } from '../../components/ProtectedRoute'; import { api } from "@/lib/api"; import { authenticatedFetch, TokenExpiredError } from "@/lib/authenticatedFetch"; import { useGlobalAdminScope } from "@/hooks"; -import { GlobalAdminBanner } from "@/components/GlobalAdminBanner"; +import { GlobalAdminBanner, globalAdminSubtitle } from "@/components/GlobalAdminBanner"; import { TenantScopeSelector } from "@/components/TenantScopeSelector"; interface SessionMetrics { @@ -171,7 +171,7 @@ export default function UsageMetricsPage() { return (
- +