mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-25 05:00:07 +00:00
fix(delegated): bound the aggregate fallback + correct override-page banner subtitle
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 <x>", matching the aggregated pages. Backend 2486 green, web 481 green + tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
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<TableClient>();
|
||||
|
||||
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<TableClient>();
|
||||
|
||||
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<TableEntity>(
|
||||
It.IsAny<string>(), It.IsAny<int?>(), It.IsAny<IEnumerable<string>>(), It.IsAny<CancellationToken>()),
|
||||
Times.AtLeastOnce());
|
||||
}
|
||||
|
||||
// ── helpers ──────────────────────────────────────────────────────────────
|
||||
|
||||
private static TableStorageService BuildStorage(
|
||||
Mock<TableClient> config, Mock<TableClient> sessions, Mock<TableClient> index)
|
||||
{
|
||||
var serviceClient = new Mock<TableServiceClient>();
|
||||
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<TableStorageService>.Instance);
|
||||
}
|
||||
|
||||
/// <summary>Mocks both QueryAsync overloads (string + expression filter) to return the given rows.</summary>
|
||||
private static Mock<TableClient> MockTableClientReturning(params TableEntity[] rows)
|
||||
{
|
||||
var m = new Mock<TableClient>();
|
||||
m.Setup(c => c.QueryAsync<TableEntity>(
|
||||
It.IsAny<string>(), It.IsAny<int?>(), It.IsAny<IEnumerable<string>>(), It.IsAny<CancellationToken>()))
|
||||
.Returns(() => AsAsyncPageable(rows));
|
||||
m.Setup(c => c.QueryAsync<TableEntity>(
|
||||
It.IsAny<Expression<Func<TableEntity, bool>>>(), It.IsAny<int?>(), It.IsAny<IEnumerable<string>>(), It.IsAny<CancellationToken>()))
|
||||
.Returns(() => AsAsyncPageable(rows));
|
||||
return m;
|
||||
}
|
||||
|
||||
private static void AssertNeverQueried(Mock<TableClient> client)
|
||||
{
|
||||
client.Verify(c => c.QueryAsync<TableEntity>(
|
||||
It.IsAny<string>(), It.IsAny<int?>(), It.IsAny<IEnumerable<string>>(), It.IsAny<CancellationToken>()),
|
||||
Times.Never());
|
||||
client.Verify(c => c.QueryAsync<TableEntity>(
|
||||
It.IsAny<Expression<Func<TableEntity, bool>>>(), It.IsAny<int?>(), It.IsAny<IEnumerable<string>>(), It.IsAny<CancellationToken>()),
|
||||
Times.Never());
|
||||
}
|
||||
|
||||
private static AsyncPageable<TableEntity> AsAsyncPageable(TableEntity[] entities)
|
||||
{
|
||||
var page = Page<TableEntity>.FromValues(entities, continuationToken: null, new Mock<Response>().Object);
|
||||
return AsyncPageable<TableEntity>.FromPages(new[] { page });
|
||||
}
|
||||
}
|
||||
@@ -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<SessionSummary>(), false, null);
|
||||
|
||||
var fallback = await FetchAllSessionsFromPrimaryTableInternalAsync(maxResults, days);
|
||||
return (fallback.Sessions, fallback.HasMore, NextCursor: null);
|
||||
}
|
||||
|
||||
@@ -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 (
|
||||
<ProtectedRoute>
|
||||
<div className="min-h-screen bg-gray-50">
|
||||
<GlobalAdminBanner show={scope.isGlobalAdmin} delegated={scope.isDelegatedScope} />
|
||||
<GlobalAdminBanner show={scope.isGlobalAdmin} delegated={scope.isDelegatedScope} subtitle={globalAdminSubtitle(scope)} />
|
||||
{/* Header */}
|
||||
<header className="bg-white shadow">
|
||||
<div className="max-w-7xl mx-auto py-6 px-4 sm:px-6 lg:px-8">
|
||||
|
||||
@@ -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 (
|
||||
<ProtectedRoute>
|
||||
<div className="min-h-screen bg-gray-50">
|
||||
<GlobalAdminBanner show={scope.isGlobalAdmin} delegated={scope.isDelegatedScope} />
|
||||
<GlobalAdminBanner show={scope.isGlobalAdmin} delegated={scope.isDelegatedScope} subtitle={globalAdminSubtitle(scope)} />
|
||||
{/* Header */}
|
||||
<header className="bg-white shadow">
|
||||
<div className="max-w-7xl mx-auto py-6 px-4 sm:px-6 lg:px-8">
|
||||
|
||||
@@ -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 (
|
||||
<ProtectedRoute>
|
||||
<div className="min-h-screen bg-gray-50 dark:bg-gray-900">
|
||||
<GlobalAdminBanner show={scope.isGlobalAdmin} delegated={scope.isDelegatedScope} />
|
||||
<GlobalAdminBanner show={scope.isGlobalAdmin} delegated={scope.isDelegatedScope} subtitle={globalAdminSubtitle(scope)} />
|
||||
{/* Header */}
|
||||
<header className="bg-white dark:bg-gray-800 shadow">
|
||||
<div className="max-w-7xl mx-auto py-6 px-4 sm:px-6 lg:px-8">
|
||||
|
||||
@@ -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 (
|
||||
<ProtectedRoute>
|
||||
<div className="min-h-screen bg-gray-50">
|
||||
<GlobalAdminBanner show={scope.isGlobalAdmin} delegated={scope.isDelegatedScope} />
|
||||
<GlobalAdminBanner show={scope.isGlobalAdmin} delegated={scope.isDelegatedScope} subtitle={globalAdminSubtitle(scope)} />
|
||||
<header className="bg-white shadow">
|
||||
<div className="max-w-7xl mx-auto py-6 px-4 sm:px-6 lg:px-8">
|
||||
<div className="flex items-center justify-between">
|
||||
|
||||
Reference in New Issue
Block a user