mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-24 04:32:32 +00:00
fix: code-review follow-ups (watcher dispose race, revoke-enforcement tests, scan guard)
- ConsoleBypassWatcher: re-check _disposed after watcher.Start() — a Dispose() racing the background arm could leave an armed WMI subscription no code path could ever stop (leaked until process exit). - SignalRContext: collapse rapid accessRevoked pushes into one restart via an in-flight guard — a second start() on a non-Disconnected connection throws and burns the retry budget. - config/all: negative-cache managed ids a rescue scan proved missing (5 min TTL) — one stale grant (offboarded tenant) no longer makes every delegated config/all call pay a full table scan. - DelegatedAdmin/TenantGroup functions: extract testable revoke-enforcement cores (RevokeCoreAsync, SetEnabledCoreAsync, UnassignCoreAsync, DeleteGroupCoreAsync, ValidateGrantRequest) and cover the security flows: 404-on-missing-row writes no audit row and fires no disconnect; real revoke/disable/unassign/group-delete always cuts live streams; grant validation is fail-closed (GUID tenantId, recognized roles only). - dashboard/page.tsx: drop unused next/link import left by the banner removal. Tests: backend 3099 passed, web 557 passed, agent ConsoleBypassWatcher suite passed, tsc clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
+19
@@ -136,6 +136,25 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Telemetry.Security
|
||||
}
|
||||
|
||||
watcher.Start();
|
||||
|
||||
// Re-check after arming: Dispose() may have run between the lock release above and
|
||||
// Start() — it then stopped a not-yet-armed watcher and this thread armed it right
|
||||
// back, leaving a WMI subscription no code path could ever reach again. If disposal
|
||||
// won that race, tear the watcher down here.
|
||||
bool disposedDuringArm;
|
||||
lock (_lock)
|
||||
{
|
||||
disposedDuringArm = _disposed;
|
||||
if (disposedDuringArm) _startWatcher = null;
|
||||
}
|
||||
if (disposedDuringArm)
|
||||
{
|
||||
try { watcher.EventArrived -= OnProcessStartTrace; } catch { }
|
||||
try { watcher.Stop(); } catch { }
|
||||
try { watcher.Dispose(); } catch { }
|
||||
return;
|
||||
}
|
||||
|
||||
_logger.Info($"[ConsoleBypassWatcher] watching Win32_ProcessStartTrace for " +
|
||||
$"{string.Join(", ", WatchedProcessNames)} (interactive-session + bare command line)");
|
||||
}
|
||||
|
||||
+26
@@ -129,4 +129,30 @@ public class GetAllTenantConfigurationsFunctionTests
|
||||
[Fact]
|
||||
public void DelegatedBareArrayView_Empty_Empty()
|
||||
=> Assert.Empty(GetAllTenantConfigurationsFunction.DelegatedBareArrayView(new TenantConfiguration[0]));
|
||||
|
||||
[Fact]
|
||||
public void MissingManagedIds_ReturnsOnlyIdsWithoutConfig_CaseInsensitive()
|
||||
{
|
||||
var configs = new List<TenantConfiguration>
|
||||
{
|
||||
new() { TenantId = TenantB.ToUpperInvariant() }, // case variant still counts as present
|
||||
};
|
||||
|
||||
var missing = GetAllTenantConfigurationsFunction.MissingManagedIds(new[] { TenantB, TenantC }, configs);
|
||||
|
||||
var only = Assert.Single(missing);
|
||||
Assert.Equal(TenantC, only);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void MissingManagedIds_AllPresent_Empty()
|
||||
=> Assert.Empty(GetAllTenantConfigurationsFunction.MissingManagedIds(
|
||||
new[] { TenantB },
|
||||
new List<TenantConfiguration> { new() { TenantId = TenantB } }));
|
||||
|
||||
[Fact]
|
||||
public void MissingConfigCacheKey_LowercasesId_SoCaseVariantsShareOneVerdict()
|
||||
=> Assert.Equal(
|
||||
GetAllTenantConfigurationsFunction.MissingConfigCacheKey(TenantB),
|
||||
GetAllTenantConfigurationsFunction.MissingConfigCacheKey(TenantB.ToUpperInvariant()));
|
||||
}
|
||||
|
||||
@@ -0,0 +1,269 @@
|
||||
using AutopilotMonitor.Functions.Functions.Admin;
|
||||
using AutopilotMonitor.Functions.Services;
|
||||
using AutopilotMonitor.Shared;
|
||||
using AutopilotMonitor.Shared.DataAccess;
|
||||
using Microsoft.Extensions.Caching.Memory;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using Moq;
|
||||
using Xunit;
|
||||
|
||||
namespace AutopilotMonitor.Functions.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Covers the SignalR revoke-enforcement cores of <see cref="DelegatedAdminManagementFunction"/> and
|
||||
/// <see cref="TenantGroupManagementFunction"/> (extracted seams — the HTTP entry points are intentionally
|
||||
/// not exercised, same rationale as <see cref="GetAllBlockedDevicesFunctionTests"/>). Locks in three
|
||||
/// security invariants: (1) a miss (no assignment row) neither audits nor disconnects — a typo must not
|
||||
/// write false customer-visible "access removed" rows while the real grant stays live; (2) a real
|
||||
/// revoke/disable/unassign/group-delete always cuts the affected UPN's live streams (group authorization
|
||||
/// is join-time only); (3) grant validation is fail-closed (GUID tenantId, recognized roles only).
|
||||
/// </summary>
|
||||
public class RevokeEnforcementTests
|
||||
{
|
||||
private const string Upn = "MSP-Admin@Partner.Example";
|
||||
private const string UpnLower = "msp-admin@partner.example";
|
||||
private const string TenantA = "11111111-1111-1111-1111-111111111111";
|
||||
private const string TenantB = "22222222-2222-2222-2222-222222222222";
|
||||
private const string GroupId = "tpl-aaaaaaaa";
|
||||
|
||||
private static DelegatedAdminService BuildService(Mock<IAdminRepository> repo) =>
|
||||
new(
|
||||
repo.Object,
|
||||
new StubTenantEntitlementService(AutopilotMonitor.Functions.Security.TenantEdition.Enterprise),
|
||||
new MemoryCache(new MemoryCacheOptions()),
|
||||
NullLogger<DelegatedAdminService>.Instance);
|
||||
|
||||
private static Mock<IMaintenanceRepository> BuildAuditRepo()
|
||||
{
|
||||
var audit = new Mock<IMaintenanceRepository>();
|
||||
audit.Setup(a => a.LogAuditEntryAsync(
|
||||
It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>(),
|
||||
It.IsAny<string>(), It.IsAny<string>(), It.IsAny<Dictionary<string, string>?>()))
|
||||
.ReturnsAsync(true);
|
||||
return audit;
|
||||
}
|
||||
|
||||
private static void VerifyNoAudit(Mock<IMaintenanceRepository> audit) =>
|
||||
audit.Verify(a => a.LogAuditEntryAsync(
|
||||
It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>(),
|
||||
It.IsAny<string>(), It.IsAny<string>(), It.IsAny<Dictionary<string, string>?>()), Times.Never);
|
||||
|
||||
private static (DelegatedAdminManagementFunction Fn, Mock<IAdminRepository> Repo,
|
||||
Mock<IMaintenanceRepository> Audit, FakeSignalRNotificationService SignalR) BuildAdminFn()
|
||||
{
|
||||
var repo = new Mock<IAdminRepository>();
|
||||
var audit = BuildAuditRepo();
|
||||
var signalR = new FakeSignalRNotificationService();
|
||||
var fn = new DelegatedAdminManagementFunction(
|
||||
NullLogger<DelegatedAdminManagementFunction>.Instance, BuildService(repo), audit.Object, signalR);
|
||||
return (fn, repo, audit, signalR);
|
||||
}
|
||||
|
||||
private static (TenantGroupManagementFunction Fn, Mock<IAdminRepository> Repo,
|
||||
Mock<IMaintenanceRepository> Audit, FakeSignalRNotificationService SignalR) BuildGroupFn()
|
||||
{
|
||||
var repo = new Mock<IAdminRepository>();
|
||||
// Loose-mock defaults return a null Task — every repo read the flows touch needs a concrete setup.
|
||||
repo.Setup(r => r.GetTenantGroupAsync(It.IsAny<string>())).ReturnsAsync((TenantGroup?)null);
|
||||
repo.Setup(r => r.GetGroupAssignmentsForUpnAsync(It.IsAny<string>()))
|
||||
.ReturnsAsync(new List<TenantGroupAssignment>());
|
||||
repo.Setup(r => r.GetGroupAssigneesAsync(It.IsAny<string>()))
|
||||
.ReturnsAsync(new List<TenantGroupAssignment>());
|
||||
var audit = BuildAuditRepo();
|
||||
var signalR = new FakeSignalRNotificationService();
|
||||
var fn = new TenantGroupManagementFunction(
|
||||
NullLogger<TenantGroupManagementFunction>.Instance, BuildService(repo), audit.Object, signalR);
|
||||
return (fn, repo, audit, signalR);
|
||||
}
|
||||
|
||||
// --- Delegated grant validation (fail-closed) ---
|
||||
|
||||
[Fact]
|
||||
public void ValidateGrant_NullBody_Rejected()
|
||||
=> Assert.Equal("upn and tenantId are required",
|
||||
DelegatedAdminManagementFunction.ValidateGrantRequest(null, out _));
|
||||
|
||||
[Theory]
|
||||
[InlineData("", TenantA)]
|
||||
[InlineData(" ", TenantA)]
|
||||
[InlineData(Upn, "")]
|
||||
public void ValidateGrant_MissingFields_Rejected(string upn, string tenantId)
|
||||
=> Assert.Equal("upn and tenantId are required",
|
||||
DelegatedAdminManagementFunction.ValidateGrantRequest(
|
||||
new GrantDelegatedAdminRequest { Upn = upn, TenantId = tenantId }, out _));
|
||||
|
||||
[Theory]
|
||||
[InlineData("contoso.com")]
|
||||
[InlineData("not-a-guid")]
|
||||
[InlineData("11111111-1111-1111-1111")]
|
||||
public void ValidateGrant_NonGuidTenantId_Rejected(string tenantId)
|
||||
=> Assert.Equal("a valid tenantId (GUID) is required",
|
||||
DelegatedAdminManagementFunction.ValidateGrantRequest(
|
||||
new GrantDelegatedAdminRequest { Upn = Upn, TenantId = tenantId }, out _));
|
||||
|
||||
[Fact]
|
||||
public void ValidateGrant_UnknownRole_Rejected()
|
||||
{
|
||||
var error = DelegatedAdminManagementFunction.ValidateGrantRequest(
|
||||
new GrantDelegatedAdminRequest { Upn = Upn, TenantId = TenantA, Role = "GlobalAdmin" }, out _);
|
||||
Assert.NotNull(error);
|
||||
Assert.Contains(Constants.DelegatedRoles.DelegatedReader, error);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ValidateGrant_EmptyRole_DefaultsToLeastPrivilegedReader()
|
||||
{
|
||||
Assert.Null(DelegatedAdminManagementFunction.ValidateGrantRequest(
|
||||
new GrantDelegatedAdminRequest { Upn = Upn, TenantId = TenantA }, out var role));
|
||||
Assert.Equal(Constants.DelegatedRoles.DelegatedReader, role);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ValidateGrant_ExplicitDelegatedAdmin_Accepted()
|
||||
{
|
||||
Assert.Null(DelegatedAdminManagementFunction.ValidateGrantRequest(
|
||||
new GrantDelegatedAdminRequest { Upn = Upn, TenantId = TenantA, Role = Constants.DelegatedRoles.DelegatedAdmin },
|
||||
out var role));
|
||||
Assert.Equal(Constants.DelegatedRoles.DelegatedAdmin, role);
|
||||
}
|
||||
|
||||
// --- Revoke: 404-on-missing-row vs. audit + disconnect ---
|
||||
|
||||
[Fact]
|
||||
public async Task RevokeCore_MissingRow_NoAuditNoDisconnect()
|
||||
{
|
||||
var (fn, repo, audit, signalR) = BuildAdminFn();
|
||||
repo.Setup(r => r.RemoveDelegatedAdminAsync(UpnLower, TenantA)).ReturnsAsync(false);
|
||||
|
||||
var removed = await fn.RevokeCoreAsync(Upn, TenantA, "ga@vendor.example");
|
||||
|
||||
Assert.False(removed);
|
||||
VerifyNoAudit(audit);
|
||||
Assert.Empty(signalR.DisconnectedUsers);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task RevokeCore_RemovedRow_AuditsAndDisconnectsLowercasedUpn()
|
||||
{
|
||||
var (fn, repo, audit, signalR) = BuildAdminFn();
|
||||
repo.Setup(r => r.RemoveDelegatedAdminAsync(UpnLower, TenantA)).ReturnsAsync(true);
|
||||
|
||||
var removed = await fn.RevokeCoreAsync(Upn, TenantA, "ga@vendor.example");
|
||||
|
||||
Assert.True(removed);
|
||||
// SignalR user ids are lowercased UPNs — a mixed-case route param must still hit the user's connections.
|
||||
Assert.Equal(new[] { UpnLower }, signalR.DisconnectedUsers);
|
||||
audit.Verify(a => a.LogAuditEntryAsync(
|
||||
TenantA, "DELETE", "DelegatedAdmin", UpnLower, "ga@vendor.example", null), Times.Once);
|
||||
}
|
||||
|
||||
// --- Disable is a revocation too; enable never disconnects ---
|
||||
|
||||
[Fact]
|
||||
public async Task SetEnabledCore_MissingRow_NoAuditNoDisconnect()
|
||||
{
|
||||
var (fn, repo, audit, signalR) = BuildAdminFn();
|
||||
repo.Setup(r => r.SetDelegatedAdminEnabledAsync(UpnLower, TenantA, false)).ReturnsAsync(false);
|
||||
|
||||
var ok = await fn.SetEnabledCoreAsync(Upn, TenantA, isEnabled: false, "ga@vendor.example");
|
||||
|
||||
Assert.False(ok);
|
||||
VerifyNoAudit(audit);
|
||||
Assert.Empty(signalR.DisconnectedUsers);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SetEnabledCore_Disable_Disconnects()
|
||||
{
|
||||
var (fn, repo, _, signalR) = BuildAdminFn();
|
||||
repo.Setup(r => r.SetDelegatedAdminEnabledAsync(UpnLower, TenantA, false)).ReturnsAsync(true);
|
||||
|
||||
var ok = await fn.SetEnabledCoreAsync(Upn, TenantA, isEnabled: false, "ga@vendor.example");
|
||||
|
||||
Assert.True(ok);
|
||||
Assert.Equal(new[] { UpnLower }, signalR.DisconnectedUsers);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SetEnabledCore_Enable_DoesNotDisconnect()
|
||||
{
|
||||
var (fn, repo, _, signalR) = BuildAdminFn();
|
||||
repo.Setup(r => r.SetDelegatedAdminEnabledAsync(UpnLower, TenantA, true)).ReturnsAsync(true);
|
||||
|
||||
var ok = await fn.SetEnabledCoreAsync(Upn, TenantA, isEnabled: true, "ga@vendor.example");
|
||||
|
||||
Assert.True(ok);
|
||||
Assert.Empty(signalR.DisconnectedUsers);
|
||||
}
|
||||
|
||||
// --- Group unassign: 404-on-not-assigned vs. disconnect + per-tenant audit ---
|
||||
|
||||
[Fact]
|
||||
public async Task UnassignCore_NotAssigned_NoAuditNoDisconnect()
|
||||
{
|
||||
var (fn, _, audit, signalR) = BuildGroupFn(); // default: no assignments for the UPN
|
||||
|
||||
var unassigned = await fn.UnassignCoreAsync(GroupId, Upn, "ga@vendor.example");
|
||||
|
||||
Assert.False(unassigned);
|
||||
VerifyNoAudit(audit);
|
||||
Assert.Empty(signalR.DisconnectedUsers);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task UnassignCore_Assigned_DisconnectsAndAuditsPerGroupTenant()
|
||||
{
|
||||
var (fn, repo, audit, signalR) = BuildGroupFn();
|
||||
repo.Setup(r => r.GetTenantGroupAsync(GroupId)).ReturnsAsync(new TenantGroup
|
||||
{
|
||||
GroupId = GroupId,
|
||||
Name = "MSP Customers",
|
||||
TenantIds = new List<string> { TenantA, TenantB },
|
||||
AssigneeCount = 1,
|
||||
});
|
||||
repo.Setup(r => r.GetGroupAssignmentsForUpnAsync(UpnLower))
|
||||
.ReturnsAsync(new List<TenantGroupAssignment> { new() { Upn = UpnLower, GroupId = GroupId } });
|
||||
repo.Setup(r => r.UnassignGroupAsync(UpnLower, GroupId)).ReturnsAsync(true);
|
||||
|
||||
var unassigned = await fn.UnassignCoreAsync(GroupId, Upn, "ga@vendor.example");
|
||||
|
||||
Assert.True(unassigned);
|
||||
Assert.Equal(new[] { UpnLower }, signalR.DisconnectedUsers);
|
||||
// Lost access is audited under EACH of the group's tenants (customer-visible trail).
|
||||
audit.Verify(a => a.LogAuditEntryAsync(
|
||||
TenantA, "DELETE", "DelegatedGroupAccess", UpnLower, "ga@vendor.example",
|
||||
It.IsAny<Dictionary<string, string>?>()), Times.Once);
|
||||
audit.Verify(a => a.LogAuditEntryAsync(
|
||||
TenantB, "DELETE", "DelegatedGroupAccess", UpnLower, "ga@vendor.example",
|
||||
It.IsAny<Dictionary<string, string>?>()), Times.Once);
|
||||
}
|
||||
|
||||
// --- Group delete: every (former) assignee's streams are cut ---
|
||||
|
||||
[Fact]
|
||||
public async Task DeleteGroupCore_DisconnectsEveryAssignee_AndAuditsPerTenant()
|
||||
{
|
||||
var (fn, repo, audit, signalR) = BuildGroupFn();
|
||||
var assignees = new List<TenantGroupAssignment>
|
||||
{
|
||||
new() { Upn = "one@partner.example", GroupId = GroupId },
|
||||
new() { Upn = "two@partner.example", GroupId = GroupId },
|
||||
};
|
||||
repo.Setup(r => r.GetTenantGroupAsync(GroupId)).ReturnsAsync(new TenantGroup
|
||||
{
|
||||
GroupId = GroupId,
|
||||
Name = "MSP Customers",
|
||||
TenantIds = new List<string> { TenantA },
|
||||
AssigneeCount = assignees.Count,
|
||||
});
|
||||
repo.Setup(r => r.GetGroupAssigneesAsync(GroupId)).ReturnsAsync(assignees);
|
||||
repo.Setup(r => r.DeleteTenantGroupAsync(GroupId)).ReturnsAsync(true);
|
||||
|
||||
await fn.DeleteGroupCoreAsync(GroupId, "ga@vendor.example");
|
||||
|
||||
Assert.Equal(new[] { "one@partner.example", "two@partner.example" }, signalR.DisconnectedUsers);
|
||||
audit.Verify(a => a.LogAuditEntryAsync(
|
||||
TenantA, "DELETE", "DelegatedGroupAccess", "*", "ga@vendor.example",
|
||||
It.IsAny<Dictionary<string, string>?>()), Times.Once);
|
||||
}
|
||||
}
|
||||
+69
-32
@@ -64,19 +64,9 @@ public class DelegatedAdminManagementFunction
|
||||
var currentUpn = context.GetRequestContext().UserPrincipalName;
|
||||
|
||||
var body = await req.ReadFromJsonAsync<GrantDelegatedAdminRequest>();
|
||||
if (body == null || string.IsNullOrWhiteSpace(body.Upn) || string.IsNullOrWhiteSpace(body.TenantId))
|
||||
return await Bad(req, "upn and tenantId are required");
|
||||
|
||||
// Mirror the tenant-group path: a mistyped tenantId must not create a garbage scope entry
|
||||
// that pollutes the allow-list and seeds a per-tenant audit trail keyed on the typo.
|
||||
if (!Guid.TryParse(body.TenantId, out _))
|
||||
return await Bad(req, "a valid tenantId (GUID) is required");
|
||||
|
||||
// Default to the least-privileged role; reject anything we don't recognize (fail-closed) so a typo
|
||||
// can never silently widen access.
|
||||
var role = string.IsNullOrWhiteSpace(body.Role) ? Constants.DelegatedRoles.DelegatedReader : body.Role;
|
||||
if (role != Constants.DelegatedRoles.DelegatedReader && role != Constants.DelegatedRoles.DelegatedAdmin)
|
||||
return await Bad(req, $"role must be '{Constants.DelegatedRoles.DelegatedReader}' or '{Constants.DelegatedRoles.DelegatedAdmin}'");
|
||||
var validationError = ValidateGrantRequest(body, out var role);
|
||||
if (validationError != null)
|
||||
return await Bad(req, validationError);
|
||||
|
||||
// Edition note: TARGET tenants may be any edition — an MSP on Enterprise may manage Community
|
||||
// customers. The Enterprise requirement applies to the delegated admin's HOME tenant and is
|
||||
@@ -84,7 +74,7 @@ public class DelegatedAdminManagementFunction
|
||||
// be checked here: at grant time only the UPN is known, and UPN-domain → tenant mapping is not
|
||||
// reliable (multi-domain tenants). Grants to non-Enterprise-homed admins are simply inert.
|
||||
var entry = await _delegatedAdminService.UpsertAsync(
|
||||
body.Upn, body.TenantId, role,
|
||||
body!.Upn, body.TenantId, role,
|
||||
Constants.DelegatedStatus.Active, Constants.DelegatedSource.OperatorGranted, currentUpn ?? "");
|
||||
|
||||
await _maintenanceRepo.LogAuditEntryAsync(
|
||||
@@ -112,10 +102,7 @@ public class DelegatedAdminManagementFunction
|
||||
string upn, string tenantId, FunctionContext context)
|
||||
{
|
||||
var currentUpn = context.GetRequestContext().UserPrincipalName;
|
||||
// The service reports whether a row was actually deleted — a mistyped upn/tenantId must not
|
||||
// 200 and write a false customer-visible "access removed" audit row while the real grant
|
||||
// stays live (mirrors the tenant-group revoke semantics).
|
||||
var removed = await _delegatedAdminService.RemoveAsync(upn, tenantId);
|
||||
var removed = await RevokeCoreAsync(upn, tenantId, currentUpn);
|
||||
if (!removed)
|
||||
{
|
||||
var notFound = req.CreateResponse(HttpStatusCode.NotFound);
|
||||
@@ -123,20 +110,34 @@ public class DelegatedAdminManagementFunction
|
||||
return notFound;
|
||||
}
|
||||
|
||||
await _maintenanceRepo.LogAuditEntryAsync(
|
||||
tenantId.ToLowerInvariant(), "DELETE", "DelegatedAdmin", upn.ToLowerInvariant(), currentUpn ?? "");
|
||||
|
||||
// Enforcement: cut the revoked caller's already-joined live streams — group authorization is
|
||||
// join-time only, so without this they keep receiving tenant telemetry until the connection drops.
|
||||
await _signalRService.DisconnectUserAsync(upn.ToLowerInvariant());
|
||||
|
||||
_logger.LogInformation("Delegated revoke: {Upn} -> {TenantId} by {By}", upn, tenantId, currentUpn);
|
||||
|
||||
var response = req.CreateResponse(HttpStatusCode.OK);
|
||||
await response.WriteAsJsonAsync(new { message = "Delegated assignment revoked" });
|
||||
return response;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Testable core of the revoke flow (no HTTP plumbing — see GetAllBlockedDevicesFunctionTests for the
|
||||
/// convention). Returns false when no assignment row existed (caller 404s): a mistyped upn/tenantId
|
||||
/// must not 200 and write a false customer-visible "access removed" audit row while the real grant
|
||||
/// stays live (mirrors the tenant-group revoke semantics). On a real removal writes the audit entry
|
||||
/// and cuts the revoked UPN's live streams — group authorization is join-time only, so without the
|
||||
/// kick they keep receiving tenant telemetry until the connection drops.
|
||||
/// </summary>
|
||||
internal async Task<bool> RevokeCoreAsync(string upn, string tenantId, string? currentUpn)
|
||||
{
|
||||
var removed = await _delegatedAdminService.RemoveAsync(upn, tenantId);
|
||||
if (!removed)
|
||||
return false;
|
||||
|
||||
await _maintenanceRepo.LogAuditEntryAsync(
|
||||
tenantId.ToLowerInvariant(), "DELETE", "DelegatedAdmin", upn.ToLowerInvariant(), currentUpn ?? "");
|
||||
|
||||
await _signalRService.DisconnectUserAsync(upn.ToLowerInvariant());
|
||||
|
||||
_logger.LogInformation("Delegated revoke: {Upn} -> {TenantId} by {By}", upn, tenantId, currentUpn);
|
||||
return true;
|
||||
}
|
||||
|
||||
/// <summary>PATCH /api/global/delegated-admins/{upn}/{tenantId}/enable — re-enable a disabled assignment. GlobalAdminOnly.</summary>
|
||||
[Function("EnableDelegatedAdmin")]
|
||||
[Authorize]
|
||||
@@ -157,7 +158,7 @@ public class DelegatedAdminManagementFunction
|
||||
HttpRequestData req, string upn, string tenantId, bool isEnabled, FunctionContext context)
|
||||
{
|
||||
var currentUpn = context.GetRequestContext().UserPrincipalName;
|
||||
var ok = await _delegatedAdminService.SetEnabledAsync(upn, tenantId, isEnabled);
|
||||
var ok = await SetEnabledCoreAsync(upn, tenantId, isEnabled, currentUpn);
|
||||
if (!ok)
|
||||
{
|
||||
var notFound = req.CreateResponse(HttpStatusCode.NotFound);
|
||||
@@ -165,20 +166,56 @@ public class DelegatedAdminManagementFunction
|
||||
return notFound;
|
||||
}
|
||||
|
||||
var response = req.CreateResponse(HttpStatusCode.OK);
|
||||
await response.WriteAsJsonAsync(new { message = isEnabled ? "Delegated assignment enabled" : "Delegated assignment disabled" });
|
||||
return response;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Testable core of enable/disable (no HTTP plumbing). Returns false when no assignment row
|
||||
/// existed (caller 404s — no audit, no disconnect). Disable is a revocation too, so it cuts the
|
||||
/// UPN's already-joined live streams (see <see cref="RevokeCoreAsync"/>); enable never disconnects.
|
||||
/// </summary>
|
||||
internal async Task<bool> SetEnabledCoreAsync(string upn, string tenantId, bool isEnabled, string? currentUpn)
|
||||
{
|
||||
var ok = await _delegatedAdminService.SetEnabledAsync(upn, tenantId, isEnabled);
|
||||
if (!ok)
|
||||
return false;
|
||||
|
||||
await _maintenanceRepo.LogAuditEntryAsync(
|
||||
tenantId.ToLowerInvariant(), "UPDATE", "DelegatedAdmin", upn.ToLowerInvariant(), currentUpn ?? "",
|
||||
new Dictionary<string, string> { { "IsEnabled", isEnabled.ToString() } });
|
||||
|
||||
// Disable is a revocation too — cut already-joined live streams (see RevokeDelegatedAdmin).
|
||||
if (!isEnabled)
|
||||
await _signalRService.DisconnectUserAsync(upn.ToLowerInvariant());
|
||||
|
||||
_logger.LogInformation("Delegated {Action}: {Upn} -> {TenantId} by {By}",
|
||||
isEnabled ? "enable" : "disable", upn, tenantId, currentUpn);
|
||||
return true;
|
||||
}
|
||||
|
||||
var response = req.CreateResponse(HttpStatusCode.OK);
|
||||
await response.WriteAsJsonAsync(new { message = isEnabled ? "Delegated assignment enabled" : "Delegated assignment disabled" });
|
||||
return response;
|
||||
/// <summary>
|
||||
/// Pure grant-request validation (testable seam). Returns the 400 error message, or null when valid —
|
||||
/// then <paramref name="role"/> carries the effective role. A mistyped tenantId must not create a
|
||||
/// garbage scope entry that pollutes the allow-list and seeds a per-tenant audit trail keyed on the
|
||||
/// typo; the role defaults to least-privileged and unknown roles are rejected (fail-closed) so a typo
|
||||
/// can never silently widen access.
|
||||
/// </summary>
|
||||
internal static string? ValidateGrantRequest(GrantDelegatedAdminRequest? body, out string role)
|
||||
{
|
||||
role = Constants.DelegatedRoles.DelegatedReader;
|
||||
|
||||
if (body == null || string.IsNullOrWhiteSpace(body.Upn) || string.IsNullOrWhiteSpace(body.TenantId))
|
||||
return "upn and tenantId are required";
|
||||
|
||||
if (!Guid.TryParse(body.TenantId, out _))
|
||||
return "a valid tenantId (GUID) is required";
|
||||
|
||||
role = string.IsNullOrWhiteSpace(body.Role) ? Constants.DelegatedRoles.DelegatedReader : body.Role!;
|
||||
if (role != Constants.DelegatedRoles.DelegatedReader && role != Constants.DelegatedRoles.DelegatedAdmin)
|
||||
return $"role must be '{Constants.DelegatedRoles.DelegatedReader}' or '{Constants.DelegatedRoles.DelegatedAdmin}'";
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
private static async Task<HttpResponseData> Bad(HttpRequestData req, string error)
|
||||
|
||||
+34
-16
@@ -112,15 +112,26 @@ public class TenantGroupManagementFunction
|
||||
string groupId, FunctionContext context)
|
||||
{
|
||||
var currentUpn = context.GetRequestContext().UserPrincipalName;
|
||||
await DeleteGroupCoreAsync(groupId, currentUpn);
|
||||
|
||||
// Capture tenants + assignees BEFORE the cascade delete removes them, so we can audit the
|
||||
// bulk access removal under each affected tenant and cut every assignee's live streams.
|
||||
var response = req.CreateResponse(HttpStatusCode.OK);
|
||||
await response.WriteAsJsonAsync(new { message = "Group deleted" });
|
||||
return response;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Testable core of the group cascade delete (no HTTP plumbing). Captures tenants + assignees
|
||||
/// BEFORE the delete removes them, so the bulk access removal is audited under each affected
|
||||
/// tenant and EVERY (former) assignee's live streams are cut — group authorization is join-time
|
||||
/// only, so without the kick they keep receiving the managed tenants' telemetry until their
|
||||
/// connection drops.
|
||||
/// </summary>
|
||||
internal async Task DeleteGroupCoreAsync(string groupId, string? currentUpn)
|
||||
{
|
||||
var group = await _delegatedAdminService.GetGroupAsync(groupId);
|
||||
var assignees = await _delegatedAdminService.GetGroupAssigneesAsync(groupId);
|
||||
await _delegatedAdminService.DeleteGroupAsync(groupId);
|
||||
|
||||
// Enforcement: group authorization is join-time only — without the kick, every (former)
|
||||
// assignee keeps receiving the managed tenants' live telemetry until their connection drops.
|
||||
foreach (var assignee in assignees)
|
||||
await _signalRService.DisconnectUserAsync(assignee.Upn);
|
||||
|
||||
@@ -137,10 +148,6 @@ public class TenantGroupManagementFunction
|
||||
}
|
||||
|
||||
_logger.LogInformation("Tenant group deleted: {GroupId} by {By}", groupId, currentUpn);
|
||||
|
||||
var response = req.CreateResponse(HttpStatusCode.OK);
|
||||
await response.WriteAsJsonAsync(new { message = "Group deleted" });
|
||||
return response;
|
||||
}
|
||||
|
||||
/// <summary>POST /api/global/tenant-groups/{groupId}/tenants — add a tenant. GlobalAdminOnly. Body: { "tenantId": "<guid>" }.</summary>
|
||||
@@ -271,17 +278,31 @@ public class TenantGroupManagementFunction
|
||||
string groupId, string upn, FunctionContext context)
|
||||
{
|
||||
var currentUpn = context.GetRequestContext().UserPrincipalName;
|
||||
var unassigned = await UnassignCoreAsync(groupId, upn, currentUpn);
|
||||
if (!unassigned)
|
||||
return await NotFound(req);
|
||||
|
||||
var response = req.CreateResponse(HttpStatusCode.OK);
|
||||
await response.WriteAsJsonAsync(new { message = "Unassigned from group" });
|
||||
return response;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Testable core of the unassign flow (no HTTP plumbing). Returns false when the UPN was not
|
||||
/// actually assigned (caller 404s) — so a mistyped UPN can't 200 and write false "access removed"
|
||||
/// audit rows, and no disconnect fires. On a real unassign cuts the UPN's already-joined live
|
||||
/// streams (join-time-only authz) and audits the lost access under each of the group's tenants.
|
||||
/// </summary>
|
||||
internal async Task<bool> UnassignCoreAsync(string groupId, string upn, string? currentUpn)
|
||||
{
|
||||
var normalizedUpn = upn.ToLowerInvariant();
|
||||
|
||||
// Read the group's tenants (unchanged by unassign) so we can audit the UPN's lost access per tenant.
|
||||
var group = await _delegatedAdminService.GetGroupAsync(groupId);
|
||||
// The service returns false when the UPN was not actually assigned — so a mistyped UPN can't 200 and
|
||||
// write false "access removed" audit rows. Only audit after a real unassign.
|
||||
var unassigned = await _delegatedAdminService.UnassignGroupAsync(normalizedUpn, groupId);
|
||||
if (!unassigned)
|
||||
return await NotFound(req);
|
||||
return false;
|
||||
|
||||
// Enforcement: cut the unassigned caller's already-joined live streams (join-time-only authz).
|
||||
await _signalRService.DisconnectUserAsync(normalizedUpn);
|
||||
|
||||
if (group != null)
|
||||
@@ -296,10 +317,7 @@ public class TenantGroupManagementFunction
|
||||
}
|
||||
|
||||
_logger.LogInformation("Group {GroupId} unassigned from {Upn} by {By}", groupId, normalizedUpn, currentUpn);
|
||||
|
||||
var response = req.CreateResponse(HttpStatusCode.OK);
|
||||
await response.WriteAsJsonAsync(new { message = "Unassigned from group" });
|
||||
return response;
|
||||
return true;
|
||||
}
|
||||
|
||||
/// <summary>Logs one audit entry per tenant (the customer-visible "who can read my tenant" trail).</summary>
|
||||
|
||||
+41
-3
@@ -10,6 +10,7 @@ using AutopilotMonitor.Functions.Services;
|
||||
using AutopilotMonitor.Shared.Models;
|
||||
using AutopilotMonitor.Shared.Pagination;
|
||||
using Microsoft.Azure.Functions.Worker;
|
||||
using Microsoft.Extensions.Caching.Memory;
|
||||
using Microsoft.Azure.Functions.Worker.Http;
|
||||
using Microsoft.Extensions.Logging;
|
||||
|
||||
@@ -17,15 +18,23 @@ namespace AutopilotMonitor.Functions.Functions.Config
|
||||
{
|
||||
public class GetAllTenantConfigurationsFunction
|
||||
{
|
||||
// TTL for the "this managed id has NO config row in any casing" verdict a rescue scan proved.
|
||||
// Bounds how long a tenant onboarded moments after that scan stays absent from a delegated
|
||||
// caller's config/all (matches the positive config cache TTL in TenantConfigurationService).
|
||||
private static readonly TimeSpan MissingConfigNegativeCacheDuration = TimeSpan.FromMinutes(5);
|
||||
|
||||
private readonly ILogger<GetAllTenantConfigurationsFunction> _logger;
|
||||
private readonly TenantConfigurationService _configService;
|
||||
private readonly IMemoryCache _cache;
|
||||
|
||||
public GetAllTenantConfigurationsFunction(
|
||||
ILogger<GetAllTenantConfigurationsFunction> logger,
|
||||
TenantConfigurationService configService)
|
||||
TenantConfigurationService configService,
|
||||
IMemoryCache cache)
|
||||
{
|
||||
_logger = logger;
|
||||
_configService = configService;
|
||||
_cache = cache;
|
||||
}
|
||||
|
||||
[Function("GetAllTenantConfigurations")]
|
||||
@@ -73,9 +82,21 @@ namespace AutopilotMonitor.Functions.Functions.Config
|
||||
// (agent paths auto-create rows with the caller-supplied id verbatim) — the same caveat
|
||||
// the delegated sessions fan-out handles case-insensitively. A point-read is exact-case,
|
||||
// so any id it missed gets ONE scan+filter rescue pass (the pre-point-read behavior).
|
||||
// Only stale grants or case-variant rows pay for it; the common path stays read-bounded.
|
||||
// An id a previous scan already proved missing in ANY casing is negative-cached and
|
||||
// skipped for the TTL — otherwise a single stale grant (offboarded / never-onboarded
|
||||
// tenant) would make EVERY config/all call pay a full table scan.
|
||||
if (subset.Count < managedIds.Count)
|
||||
subset = MergeRescuedConfigs(managedIds, subset, await _configService.GetAllConfigurationsAsync());
|
||||
{
|
||||
var unproven = MissingManagedIds(managedIds, subset)
|
||||
.Where(id => !_cache.TryGetValue(MissingConfigCacheKey(id), out _))
|
||||
.ToList();
|
||||
if (unproven.Count > 0)
|
||||
{
|
||||
subset = MergeRescuedConfigs(managedIds, subset, await _configService.GetAllConfigurationsAsync());
|
||||
foreach (var id in MissingManagedIds(unproven, subset))
|
||||
_cache.Set(MissingConfigCacheKey(id), true, MissingConfigNegativeCacheDuration);
|
||||
}
|
||||
}
|
||||
_logger.LogInformation("GetAllTenantConfigurations (delegated subset, {Count} tenants) by {User}",
|
||||
subset.Count, userIdentifier);
|
||||
|
||||
@@ -177,6 +198,23 @@ namespace AutopilotMonitor.Functions.Functions.Config
|
||||
IEnumerable<TenantConfiguration?> reads)
|
||||
=> reads.Where(c => c != null).Select(c => c!).ToList();
|
||||
|
||||
/// <summary>
|
||||
/// The managed ids not present (case-insensitively) in <paramref name="configs"/> — before the
|
||||
/// rescue scan these are the point-read misses, after it the proven-missing ids to negative-cache.
|
||||
/// Pure + testable.
|
||||
/// </summary>
|
||||
internal static List<string> MissingManagedIds(
|
||||
IEnumerable<string> managedIds, IReadOnlyCollection<TenantConfiguration> configs)
|
||||
{
|
||||
var have = new HashSet<string>(configs.Select(c => c.TenantId), StringComparer.OrdinalIgnoreCase);
|
||||
return managedIds.Where(id => !have.Contains(id)).ToList();
|
||||
}
|
||||
|
||||
/// <summary>Negative-cache key for a managed id a rescue scan proved has no config row.
|
||||
/// Lowercased so case variants of the same grant share one verdict.</summary>
|
||||
internal static string MissingConfigCacheKey(string tenantId)
|
||||
=> $"tenant-config-missing:{tenantId.ToLowerInvariant()}";
|
||||
|
||||
/// <summary>
|
||||
/// Case-variant rescue for managed ids the exact-case point-reads missed: from the full config scan,
|
||||
/// add every config whose TenantId matches a managed id case-insensitively and is not already in the
|
||||
|
||||
@@ -1,7 +1,6 @@
|
||||
"use client";
|
||||
|
||||
import { Suspense, useCallback, useEffect, useMemo, useRef, useState } from "react";
|
||||
import Link from "next/link";
|
||||
import { useRouter, useSearchParams } from "next/navigation";
|
||||
import { ProtectedRoute } from "../../components/ProtectedRoute";
|
||||
import { GlobalAdminBanner } from "@/components/GlobalAdminBanner";
|
||||
|
||||
@@ -31,6 +31,9 @@ export function SignalRProvider({ children }: { children: React.ReactNode }) {
|
||||
const [joinedGroups, setJoinedGroups] = useState<string[]>([]);
|
||||
const retryCountRef = useRef(0);
|
||||
const retryTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
|
||||
// Collapses rapid successive accessRevoked pushes (one per revoked grant) into a single
|
||||
// restart — a second start() on a non-Disconnected connection throws and burns the retry budget.
|
||||
const revokeRestartInFlightRef = useRef(false);
|
||||
// Set in onreconnecting, read in onreconnected so we can report downtime as a measurement.
|
||||
const disconnectStartedAtRef = useRef<number | null>(null);
|
||||
const maxRetries = 3;
|
||||
@@ -157,12 +160,20 @@ export function SignalRProvider({ children }: { children: React.ReactNode }) {
|
||||
// the next Connected transition — through the join endpoint, which re-runs authorization
|
||||
// against the fresh scope. Revoked groups 403; still-authorized streams recover automatically.
|
||||
newConnection.on('accessRevoked', async () => {
|
||||
// Dropping an event that arrives mid-restart is safe: the rejoin after start() re-runs
|
||||
// authorization against the CURRENT scope, which already reflects that later revoke.
|
||||
if (revokeRestartInFlightRef.current) return;
|
||||
revokeRestartInFlightRef.current = true;
|
||||
console.warn('[SignalR] accessRevoked received — restarting connection to re-authorize group memberships');
|
||||
trackEvent('signalr_access_revoked');
|
||||
try {
|
||||
await newConnection.stop();
|
||||
} catch { /* proceed to restart regardless */ }
|
||||
startConnection();
|
||||
try {
|
||||
await startConnection();
|
||||
} finally {
|
||||
revokeRestartInFlightRef.current = false;
|
||||
}
|
||||
});
|
||||
|
||||
// Start as soon as the user is authenticated. The previous 2s setTimeout
|
||||
|
||||
Reference in New Issue
Block a user