mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-24 20:53:06 +00:00
fix(delegated): enforce allow-list on the single-tenant drill (home-tenant escalation)
Review finding (High). GetAllSessionsPageAsync / GetAllSessionsAsync route any non-empty tenantIdFilter straight to the per-tenant scan, ignoring allowedTenantIds. The middleware only resolves the delegated allow-list for a cross-tenant target (namedTarget != JWT tenant); when ?tenantId= equals the caller's own JWT/home tenant, crossTenant is false so the scoped-route check is skipped — but /api/global/sessions is GlobalReadOrDelegatedSubset, so a delegated-only caller is still admitted (with AllowedTenantIds for their managed set). The repo then drilled the home tenant anyway → a delegated MSP user could read their home tenant's sessions/stats without managing it or being a member. Fix: new DrillOutsideBound guard — when allowedTenantIds != null and tenantIdFilter is not in that set, return empty before the per-tenant scan. Applied to both the page (sessions list) and the drain (stats funnel through GetAllSessionsAsync). The middleware contract is unchanged by design (admit + publish AllowedTenantIds, handler/repo bounds); the bound now holds for the home-tenant case too. Tests: DelegatedBoundedAggregateTests.BoundedDrill/Stats_UnmanagedTenant_* (drill an unmanaged tenant → empty, the per-tenant SessionsIndex/Sessions scan never runs); PolicyEnforcementMiddlewareTests.Delegated_GlobalSessions_HomeTenantDrill_AdmittedButNotInAllowedSet pins that the middleware admits the home-tenant drill but never puts it in AllowedTenantIds. Backend 2489 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -24,6 +24,9 @@ public class DelegatedBoundedAggregateTests
|
||||
{
|
||||
private const string ConfigTenant = "11111111-1111-1111-1111-111111111111";
|
||||
private const string ManagedTenant = "22222222-2222-2222-2222-222222222222";
|
||||
// A tenant the delegated caller is NOT managing — e.g. their own JWT/home tenant, the one route the
|
||||
// middleware's crossTenant short-circuit does not allow-list-check.
|
||||
private const string UnmanagedTenant = "33333333-3333-3333-3333-333333333333";
|
||||
|
||||
[Fact]
|
||||
public async Task BoundedAggregate_EmptyManagedSet_ReturnsEmpty_NeverUnboundedScan()
|
||||
@@ -64,6 +67,44 @@ public class DelegatedBoundedAggregateTests
|
||||
Times.AtLeastOnce());
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task BoundedDrill_UnmanagedTenant_ReturnsEmpty_NeverScansThatTenant()
|
||||
{
|
||||
// Delegated caller drills ?tenantId=<a tenant NOT in the managed set> — e.g. their own JWT/home
|
||||
// tenant, where the middleware crossTenant short-circuit skips the allow-list check. Must read nothing.
|
||||
var leak = new[] { new TableEntity(UnmanagedTenant, "row1"), new TableEntity(UnmanagedTenant, "row2") };
|
||||
var indexClient = MockTableClientReturning(leak);
|
||||
var sessionsClient = MockTableClientReturning(leak);
|
||||
|
||||
var storage = BuildStorage(new Mock<TableClient>(), sessionsClient, indexClient);
|
||||
|
||||
var page = await storage.GetAllSessionsPageAsync(
|
||||
tenantIdFilter: UnmanagedTenant, days: null, pageSize: 10, continuation: null,
|
||||
allowedTenantIds: new[] { ManagedTenant });
|
||||
|
||||
Assert.Empty(page.Items);
|
||||
AssertNeverQueried(indexClient);
|
||||
AssertNeverQueried(sessionsClient);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task BoundedStats_UnmanagedTenant_ReturnsZero_NeverScansThatTenant()
|
||||
{
|
||||
// Same escalation via the stats path (global/stats/sessions?tenantId=<home>).
|
||||
var leak = new[] { new TableEntity(UnmanagedTenant, "row1") };
|
||||
var indexClient = MockTableClientReturning(leak);
|
||||
var sessionsClient = MockTableClientReturning(leak);
|
||||
|
||||
var storage = BuildStorage(new Mock<TableClient>(), sessionsClient, indexClient);
|
||||
|
||||
var stats = await storage.GetAllSessionStatsAsync(
|
||||
UnmanagedTenant, days: 7, allowedTenantIds: new[] { ManagedTenant });
|
||||
|
||||
Assert.Equal(0, stats.TotalLastNDays);
|
||||
AssertNeverQueried(indexClient);
|
||||
AssertNeverQueried(sessionsClient);
|
||||
}
|
||||
|
||||
// ── helpers ──────────────────────────────────────────────────────────────
|
||||
|
||||
private static TableStorageService BuildStorage(
|
||||
|
||||
@@ -544,6 +544,29 @@ public class PolicyEnforcementMiddlewareTests
|
||||
Assert.Equal(403, result.StatusCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Delegated_GlobalSessions_HomeTenantDrill_AdmittedButNotInAllowedSet()
|
||||
{
|
||||
// global/sessions?tenantId=<the caller's OWN JWT tenant> — crossTenant is false, so the delegated
|
||||
// scoped-route check is skipped and the subset tier admits the caller. This is BY DESIGN (admit +
|
||||
// publish AllowedTenantIds, handler bounds): the home tenant is NOT in AllowedTenantIds, so the repo
|
||||
// (TableStorageService.DrillOutsideBound) returns empty. This test pins that contract so the bound is
|
||||
// never silently widened to include the unmanaged home tenant. Covered end-to-end by
|
||||
// DelegatedBoundedAggregateTests.BoundedDrill/Stats_UnmanagedTenant_*.
|
||||
const string upn = "msp@partner.example";
|
||||
var h = BuildHarness();
|
||||
h.AsDelegated(TenantB);
|
||||
|
||||
var result = await h.Middleware.DecideAsync("GET", "/api/global/sessions", TenantA, AuthedPrincipal(TenantA, upn));
|
||||
|
||||
Assert.True(result.Allowed);
|
||||
var rc = result.Context!;
|
||||
Assert.True(rc.IsDelegatedReader);
|
||||
Assert.NotNull(rc.AllowedTenantIds);
|
||||
Assert.DoesNotContain(TenantA.ToLowerInvariant(), rc.AllowedTenantIds!);
|
||||
Assert.Contains(TenantB.ToLowerInvariant(), rc.AllowedTenantIds!);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Delegated_AggregateOnlyRoute_WithTenantId_StaysBlocked()
|
||||
{
|
||||
|
||||
@@ -896,12 +896,27 @@ namespace AutopilotMonitor.Functions.Services
|
||||
/// Cross-tenant paged variant of <see cref="GetAllSessionsAsync"/>.
|
||||
/// <paramref name="tenantIdFilter"/> optionally restricts to one tenant.
|
||||
/// </summary>
|
||||
/// <summary>
|
||||
/// True when a BOUNDED (delegated/MSP) caller asks for a single-tenant drill that is OUTSIDE its
|
||||
/// managed set. The middleware enforces the allow-list for a cross-tenant target, but NOT when
|
||||
/// ?tenantId= equals the caller's own JWT tenant (crossTenant=false short-circuits the delegated
|
||||
/// scoped-route check) — so the single-tenant drill in the repo must re-check the bound itself.
|
||||
/// </summary>
|
||||
private static bool DrillOutsideBound(IReadOnlyCollection<string>? allowedTenantIds, string? tenantIdFilter)
|
||||
=> allowedTenantIds != null
|
||||
&& !string.IsNullOrEmpty(tenantIdFilter)
|
||||
&& !new HashSet<string>(allowedTenantIds, StringComparer.OrdinalIgnoreCase).Contains(tenantIdFilter!);
|
||||
|
||||
public async Task<RawPage<SessionSummary>> GetAllSessionsPageAsync(
|
||||
string? tenantIdFilter, int? days, int pageSize, string? continuation,
|
||||
IReadOnlyCollection<string>? allowedTenantIds = null)
|
||||
{
|
||||
if (pageSize < 1) throw new ArgumentOutOfRangeException(nameof(pageSize));
|
||||
|
||||
// Bounded (delegated) single-tenant drill outside the managed set → empty, never the per-tenant scan.
|
||||
if (DrillOutsideBound(allowedTenantIds, tenantIdFilter))
|
||||
return new RawPage<SessionSummary>(new List<SessionSummary>(), null);
|
||||
|
||||
// When tenantIdFilter is set and is a valid tenantId, route through
|
||||
// the per-tenant SessionsIndex scan — natively-ordered, cheaper.
|
||||
if (!string.IsNullOrEmpty(tenantIdFilter))
|
||||
@@ -920,6 +935,11 @@ namespace AutopilotMonitor.Functions.Services
|
||||
/// </summary>
|
||||
public async Task<List<SessionSummary>> GetAllSessionsAsync(string? tenantIdFilter = null, int? days = null, IReadOnlyCollection<string>? allowedTenantIds = null)
|
||||
{
|
||||
// Bounded (delegated) single-tenant drill outside the managed set → empty (covers stats too,
|
||||
// which funnel through here). See DrillOutsideBound.
|
||||
if (DrillOutsideBound(allowedTenantIds, tenantIdFilter))
|
||||
return new List<SessionSummary>();
|
||||
|
||||
if (!string.IsNullOrEmpty(tenantIdFilter))
|
||||
{
|
||||
return await GetSessionsAsync(tenantIdFilter!, days);
|
||||
|
||||
Reference in New Issue
Block a user