fix(management): honor MgmtDeployArtifactsCommand.SiteId + enforce site scope (arch-review C2)
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
This commit is contained in:
@@ -162,6 +162,8 @@ scadabridge deploy artifacts [--site-id <id>]
|
|||||||
scadabridge deploy status [--instance-id <id>] [--status <status>] [--page <n>] [--page-size <n>]
|
scadabridge deploy status [--instance-id <id>] [--status <status>] [--page <n>] [--page-size <n>]
|
||||||
```
|
```
|
||||||
|
|
||||||
|
`deploy artifacts` (and its alias `site deploy-artifacts`): `--site-id` is now honored — supplying it deploys system-wide artifacts to exactly that site; omitting it deploys fleet-wide to every site. A site-scoped (non-Administrator) Deployer must supply an in-scope `--site-id`; fleet-wide deployment (no `--site-id`) requires a system-wide Deployer or Administrator (arch-review C2).
|
||||||
|
|
||||||
### Data Connection Commands
|
### Data Connection Commands
|
||||||
```
|
```
|
||||||
scadabridge data-connection list [--site-id <id>]
|
scadabridge data-connection list [--site-id <id>]
|
||||||
|
|||||||
@@ -139,7 +139,7 @@ Both endpoints honour any site-scope rules attached to the caller's audit role b
|
|||||||
### Deployments
|
### Deployments
|
||||||
|
|
||||||
- **DeployInstance**: Deploy configuration to a specific instance (includes pre-deployment validation).
|
- **DeployInstance**: Deploy configuration to a specific instance (includes pre-deployment validation).
|
||||||
- **DeployArtifacts**: Deploy system-wide artifacts (shared scripts, external system definitions, DB connections, data connections) to all sites or a specific site.
|
- **DeployArtifacts**: Deploy system-wide artifacts (shared scripts, external system definitions, DB connections, data connections) to all sites or a specific site. The command's `SiteId` is honored (arch-review C2): a value routes to the single-site deploy path (`ArtifactDeploymentService.DeployToSiteAsync`), while `null` deploys fleet-wide (`DeployToAllSitesAsync`). Site scope is enforced — a site-scoped (non-Administrator) Deployer may only target a site within its `PermittedSiteIds`, and may **not** deploy fleet-wide (`SiteId is null` is rejected with `SiteScopeViolationException` → `ManagementUnauthorized`, since `EnforceSiteScope(null)` is a deliberate no-op); fleet-wide deployment requires a system-wide Deployer or Administrator.
|
||||||
- **GetDeploymentStatus**: Query deployment status.
|
- **GetDeploymentStatus**: Query deployment status.
|
||||||
|
|
||||||
### Secured Writes (MxGateway, two-person)
|
### Secured Writes (MxGateway, two-person)
|
||||||
|
|||||||
@@ -760,7 +760,7 @@ scadabridge --url <url> site deploy-artifacts [--site-id <int>]
|
|||||||
|
|
||||||
| Option | Required | Description |
|
| Option | Required | Description |
|
||||||
|--------|----------|-------------|
|
|--------|----------|-------------|
|
||||||
| `--site-id` | no | Target site ID; omit to deploy to all sites |
|
| `--site-id` | no | Target site ID; deploys system-wide artifacts to exactly that site. Omit to deploy fleet-wide to all sites. A site-scoped (non-Administrator) Deployer must supply an in-scope `--site-id`; fleet-wide deployment requires a system-wide Deployer or Administrator (arch-review C2). |
|
||||||
|
|
||||||
#### `site area list`
|
#### `site area list`
|
||||||
|
|
||||||
@@ -840,7 +840,7 @@ scadabridge --url <url> deploy artifacts [--site-id <int>]
|
|||||||
|
|
||||||
| Option | Required | Description |
|
| Option | Required | Description |
|
||||||
|--------|----------|-------------|
|
|--------|----------|-------------|
|
||||||
| `--site-id` | no | Target site ID; omit for all sites |
|
| `--site-id` | no | Target site ID; deploys to exactly that site. Omit to deploy fleet-wide. A site-scoped (non-Administrator) Deployer must supply an in-scope `--site-id`; fleet-wide requires a system-wide Deployer or Administrator (arch-review C2). |
|
||||||
|
|
||||||
#### `deploy status`
|
#### `deploy status`
|
||||||
|
|
||||||
|
|||||||
@@ -225,7 +225,7 @@ public class ArtifactDeploymentService
|
|||||||
/// <param name="user">The user initiating the deployment.</param>
|
/// <param name="user">The user initiating the deployment.</param>
|
||||||
/// <param name="cancellationToken">Cancellation token.</param>
|
/// <param name="cancellationToken">Cancellation token.</param>
|
||||||
/// <returns>A summary of the artifact deployment results for all sites.</returns>
|
/// <returns>A summary of the artifact deployment results for all sites.</returns>
|
||||||
public async Task<Result<ArtifactDeploymentSummary>> DeployToAllSitesAsync(
|
public virtual async Task<Result<ArtifactDeploymentSummary>> DeployToAllSitesAsync(
|
||||||
string user,
|
string user,
|
||||||
CancellationToken cancellationToken = default)
|
CancellationToken cancellationToken = default)
|
||||||
{
|
{
|
||||||
@@ -243,7 +243,7 @@ public class ArtifactDeploymentService
|
|||||||
/// <param name="user">The user initiating the deployment.</param>
|
/// <param name="user">The user initiating the deployment.</param>
|
||||||
/// <param name="cancellationToken">Cancellation token.</param>
|
/// <param name="cancellationToken">Cancellation token.</param>
|
||||||
/// <returns>A summary of the artifact deployment result for the single site.</returns>
|
/// <returns>A summary of the artifact deployment result for the single site.</returns>
|
||||||
public async Task<Result<ArtifactDeploymentSummary>> DeployToSiteAsync(
|
public virtual async Task<Result<ArtifactDeploymentSummary>> DeployToSiteAsync(
|
||||||
int siteId,
|
int siteId,
|
||||||
string user,
|
string user,
|
||||||
CancellationToken cancellationToken = default)
|
CancellationToken cancellationToken = default)
|
||||||
|
|||||||
@@ -433,7 +433,7 @@ public class ManagementActor : ReceiveActor
|
|||||||
DeleteScopeRuleCommand cmd => await HandleDeleteScopeRule(sp, cmd, user.Username),
|
DeleteScopeRuleCommand cmd => await HandleDeleteScopeRule(sp, cmd, user.Username),
|
||||||
|
|
||||||
// Deployments
|
// Deployments
|
||||||
MgmtDeployArtifactsCommand cmd => await HandleDeployArtifacts(sp, cmd, user.Username),
|
MgmtDeployArtifactsCommand cmd => await HandleDeployArtifacts(sp, cmd, user),
|
||||||
QueryDeploymentsCommand cmd => await HandleQueryDeployments(sp, cmd, user),
|
QueryDeploymentsCommand cmd => await HandleQueryDeployments(sp, cmd, user),
|
||||||
GetDeploymentDiffCommand cmd => await HandleGetDeploymentDiff(sp, cmd, user),
|
GetDeploymentDiffCommand cmd => await HandleGetDeploymentDiff(sp, cmd, user),
|
||||||
|
|
||||||
@@ -2205,10 +2205,24 @@ public class ManagementActor : ReceiveActor
|
|||||||
// Deployment handlers
|
// Deployment handlers
|
||||||
// ========================================================================
|
// ========================================================================
|
||||||
|
|
||||||
private static async Task<object?> HandleDeployArtifacts(IServiceProvider sp, MgmtDeployArtifactsCommand cmd, string user)
|
private static async Task<object?> HandleDeployArtifacts(IServiceProvider sp, MgmtDeployArtifactsCommand cmd, AuthenticatedUser user)
|
||||||
{
|
{
|
||||||
|
// A site-scoped Deployer may not trigger a FLEET-WIDE artifact deployment:
|
||||||
|
// EnforceSiteScope(null) is a no-op by design, so the fleet-wide case needs
|
||||||
|
// its own guard (arch-review C2 authorization gap).
|
||||||
|
if (cmd.SiteId is null
|
||||||
|
&& user.PermittedSiteIds.Length > 0
|
||||||
|
&& !user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase))
|
||||||
|
{
|
||||||
|
throw new SiteScopeViolationException(
|
||||||
|
"Site-scoped users cannot deploy artifacts fleet-wide; specify a target site id.");
|
||||||
|
}
|
||||||
|
EnforceSiteScope(user, cmd.SiteId);
|
||||||
|
|
||||||
var svc = sp.GetRequiredService<ArtifactDeploymentService>();
|
var svc = sp.GetRequiredService<ArtifactDeploymentService>();
|
||||||
var result = await svc.DeployToAllSitesAsync(user);
|
var result = cmd.SiteId is int siteId
|
||||||
|
? await svc.DeployToSiteAsync(siteId, user.Username)
|
||||||
|
: await svc.DeployToAllSitesAsync(user.Username);
|
||||||
return result.IsSuccess
|
return result.IsSuccess
|
||||||
? result.Value
|
? result.Value
|
||||||
: throw new ManagementCommandException(result.Error);
|
: throw new ManagementCommandException(result.Error);
|
||||||
|
|||||||
@@ -29,6 +29,7 @@ public class ManagementActorTests : TestKit, IDisposable
|
|||||||
private readonly ITemplateEngineRepository _templateRepo;
|
private readonly ITemplateEngineRepository _templateRepo;
|
||||||
private readonly IAuditService _auditService;
|
private readonly IAuditService _auditService;
|
||||||
private readonly ServiceCollection _services;
|
private readonly ServiceCollection _services;
|
||||||
|
private ArtifactDeploymentService _artifactService = null!;
|
||||||
|
|
||||||
public ManagementActorTests()
|
public ManagementActorTests()
|
||||||
{
|
{
|
||||||
@@ -56,6 +57,35 @@ public class ManagementActorTests : TestKit, IDisposable
|
|||||||
new(new AuthenticatedUser("scopeduser", "Scoped User", roles, permittedSiteIds),
|
new(new AuthenticatedUser("scopeduser", "Scoped User", roles, permittedSiteIds),
|
||||||
command, Guid.NewGuid().ToString("N"));
|
command, Guid.NewGuid().ToString("N"));
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Registers a substitutable <see cref="ArtifactDeploymentService"/> into the
|
||||||
|
/// harness DI so DeployArtifacts tests can verify which deploy path was taken
|
||||||
|
/// (single-site vs. fleet-wide) without exercising the real cluster comms.
|
||||||
|
/// The virtual <c>DeployToSiteAsync</c>/<c>DeployToAllSitesAsync</c> members are
|
||||||
|
/// intercepted; all other members keep their real (unused) bodies.
|
||||||
|
/// </summary>
|
||||||
|
private void RegisterArtifactService()
|
||||||
|
{
|
||||||
|
var comms = new CommunicationService(
|
||||||
|
Options.Create(new CommunicationOptions()),
|
||||||
|
NullLogger<CommunicationService>.Instance);
|
||||||
|
_artifactService = Substitute.For<ArtifactDeploymentService>(
|
||||||
|
Substitute.For<ISiteRepository>(),
|
||||||
|
Substitute.For<IDeploymentManagerRepository>(),
|
||||||
|
Substitute.For<ITemplateEngineRepository>(),
|
||||||
|
Substitute.For<IExternalSystemRepository>(),
|
||||||
|
Substitute.For<INotificationRepository>(),
|
||||||
|
comms,
|
||||||
|
Substitute.For<IAuditService>(),
|
||||||
|
Options.Create(new DeploymentManagerOptions()),
|
||||||
|
NullLogger<ArtifactDeploymentService>.Instance);
|
||||||
|
_services.AddScoped(_ => _artifactService);
|
||||||
|
}
|
||||||
|
|
||||||
|
private static Result<ArtifactDeploymentSummary> DeploySummary() =>
|
||||||
|
Result<ArtifactDeploymentSummary>.Success(
|
||||||
|
new ArtifactDeploymentSummary("deploy-1", Array.Empty<SiteArtifactResult>(), 1, 0));
|
||||||
|
|
||||||
void IDisposable.Dispose()
|
void IDisposable.Dispose()
|
||||||
{
|
{
|
||||||
Shutdown();
|
Shutdown();
|
||||||
@@ -1285,6 +1315,69 @@ public class ManagementActorTests : TestKit, IDisposable
|
|||||||
Assert.Contains("deploy-2", response.JsonData);
|
Assert.Contains("deploy-2", response.JsonData);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ========================================================================
|
||||||
|
// DeployArtifacts honors SiteId + site-scope enforcement (arch-review C2)
|
||||||
|
//
|
||||||
|
// MgmtDeployArtifactsCommand.SiteId is now honored: a value routes to the
|
||||||
|
// single-site deploy path; null routes fleet-wide. A site-scoped (non-Admin)
|
||||||
|
// Deployer may not deploy fleet-wide, and may not target a site outside scope.
|
||||||
|
// ========================================================================
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void DeployArtifacts_WithSiteId_CallsSingleSiteDeploy()
|
||||||
|
{
|
||||||
|
RegisterArtifactService();
|
||||||
|
_artifactService.DeployToSiteAsync(7, "testuser", Arg.Any<CancellationToken>())
|
||||||
|
.Returns(DeploySummary());
|
||||||
|
|
||||||
|
var actor = CreateActor();
|
||||||
|
actor.Tell(Envelope(new MgmtDeployArtifactsCommand(SiteId: 7), "Deployer"));
|
||||||
|
|
||||||
|
ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
|
||||||
|
_artifactService.Received(1).DeployToSiteAsync(7, "testuser", Arg.Any<CancellationToken>());
|
||||||
|
_artifactService.DidNotReceiveWithAnyArgs().DeployToAllSitesAsync(default!, default);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void DeployArtifacts_FleetWide_CallsAllSitesDeploy()
|
||||||
|
{
|
||||||
|
RegisterArtifactService();
|
||||||
|
_artifactService.DeployToAllSitesAsync("testuser", Arg.Any<CancellationToken>())
|
||||||
|
.Returns(DeploySummary());
|
||||||
|
|
||||||
|
var actor = CreateActor();
|
||||||
|
actor.Tell(Envelope(new MgmtDeployArtifactsCommand(SiteId: null), "Deployer"));
|
||||||
|
|
||||||
|
ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
|
||||||
|
_artifactService.Received(1).DeployToAllSitesAsync("testuser", Arg.Any<CancellationToken>());
|
||||||
|
_artifactService.DidNotReceiveWithAnyArgs().DeployToSiteAsync(default, default!, default);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void DeployArtifacts_SiteScopedUser_OutOfScopeSite_Unauthorized()
|
||||||
|
{
|
||||||
|
RegisterArtifactService();
|
||||||
|
|
||||||
|
var actor = CreateActor();
|
||||||
|
actor.Tell(ScopedEnvelope(new MgmtDeployArtifactsCommand(SiteId: 7), new[] { "3" }, "Deployer"));
|
||||||
|
|
||||||
|
ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
|
||||||
|
_artifactService.DidNotReceiveWithAnyArgs().DeployToSiteAsync(default, default!, default);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void DeployArtifacts_SiteScopedUser_FleetWide_Unauthorized()
|
||||||
|
{
|
||||||
|
RegisterArtifactService();
|
||||||
|
|
||||||
|
var actor = CreateActor();
|
||||||
|
actor.Tell(ScopedEnvelope(new MgmtDeployArtifactsCommand(SiteId: null), new[] { "3" }, "Deployer"));
|
||||||
|
|
||||||
|
var resp = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
|
||||||
|
Assert.Contains("fleet-wide", resp.Message, StringComparison.OrdinalIgnoreCase);
|
||||||
|
_artifactService.DidNotReceiveWithAnyArgs().DeployToAllSitesAsync(default!, default);
|
||||||
|
}
|
||||||
|
|
||||||
// ========================================================================
|
// ========================================================================
|
||||||
// SetInstanceOverrides atomicity (finding -015)
|
// SetInstanceOverrides atomicity (finding -015)
|
||||||
//
|
//
|
||||||
|
|||||||
Reference in New Issue
Block a user