diff --git a/code-reviews/CLI/findings.md b/code-reviews/CLI/findings.md index c2b1272e..4fd24af0 100644 --- a/code-reviews/CLI/findings.md +++ b/code-reviews/CLI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 7 | +| Open findings | 6 | ## Summary @@ -836,7 +836,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Commands/AuditQueryHelpers.cs:186-193`, `src/ScadaLink.CLI/Commands/AuditExportHelpers.cs:147-153` | **Description** @@ -867,7 +867,22 @@ audit `SendGetAsync` already populates. **Resolution** -_Unresolved._ +Resolved 2026-05-28 (commit pending). Promoted `CommandHelpers.IsAuthorizationFailure` +from `private` to `internal` so both helpers can reuse the same auth-failure rule +(HTTP 403 OR `FORBIDDEN`/`UNAUTHORIZED` error code, case-insensitive). +`AuditQueryHelpers.RunQueryAsync` now returns +`CommandHelpers.IsAuthorizationFailure(response) ? 2 : 1` on the error path instead +of an unconditional 1. `AuditExportHelpers.RunExportAsync` doesn't ride +`ManagementResponse` (it streams directly via `SendGetStreamAsync`), so a new +`AuditExportHelpers.TryExtractErrorCode` helper parses the server's JSON error +envelope to extract `code`, and the `!IsSuccessStatusCode` branch returns exit 2 on +either HTTP 403 or a `FORBIDDEN`/`UNAUTHORIZED` envelope code. Regression tests: +`AuditQueryCommandTests.RunQuery_Http403_ReturnsExitCode2`, +`..._UnauthorizedCodeOnNon403_ReturnsExitCode2`, +`..._GenericServerError_ReturnsExitCode1` (negative guard); +`AuditExportCommandTests.RunExport_Http403_ReturnsExitCode2`, +`..._UnauthorizedCodeOnNon403_ReturnsExitCode2`. All five fail on the pre-fix code +and pass after. ### CLI-019 — `bundle export` decodes the entire base64 bundle into memory before writing diff --git a/code-reviews/CentralUI/findings.md b/code-reviews/CentralUI/findings.md index 25599720..a64fec51 100644 --- a/code-reviews/CentralUI/findings.md +++ b/code-reviews/CentralUI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 8 | +| Open findings | 7 | ## Summary @@ -1341,7 +1341,7 @@ at least one representative page so the helper's continued use is enforced. |--|--| | Severity | High | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Pages/Notifications/NotificationReport.razor:2,434,472,502`; `src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor:2,52-59`; `src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor.cs:97-110,201,250-251,278-279` | **Description** @@ -1378,6 +1378,29 @@ Add `Site_ScopedDeploymentUser_OnlySeesPermittedRows` and `Site_ScopedDeploymentUser_CannotRetryRowOnNonPermittedSite` regression tests modelled on `TopologyPageTests.SiteScoping_*`. +**Resolution** + +Resolved 2026-05-28 (commit pending). Both pages now inject `SiteScopeService` and apply +three layers of restriction. (1) `OnInitializedAsync` keeps an unfiltered `_allSites` +list as the source of truth for site-identifier → Site.Id lookups, runs the dropdown +through `SiteScope.FilterSitesAsync`, and caches `IsSystemWideAsync` + permitted-site +ids so the row-level filter is synchronous. (2) The query response is run through a new +`FilterPermittedAsync` helper that drops any row whose `SourceSiteId` / `SourceSite` +resolves (via the unfiltered list) to a Site.Id outside the permitted set — a stale +source-site identifier not present in the loaded list defaults to allowed, mirroring +the existing tolerance for deleted-site rows. (3) `RetryNotification` / +`DiscardNotification` / `RetrySiteCall` / `DiscardSiteCall` each re-check +`IsRowSiteAllowedAsync` against the row's site BEFORE relaying, surfacing +"You are not permitted to act on …" via toast on failure. Cross-module partner +Security-017 was resolved in the same batch (the dead `SiteScopeAuthorizationHandler` +was deleted; `SiteScopeService` is now documented as the sole site-scoping mechanism). +Regression test `SiteCallsReportPageTests.SiteScoping_ScopedDeploymentUser_HidesOutOfScopeRows` +seeds a Deployment user with a single `SiteId=1` claim, asserts only the Plant-A row +renders, and verifies the Plant-B row is dropped (the page's row count drops from 2 to +1). All three existing report-page test fixtures register `SiteScopeService` so the +default system-wide path is unaffected — the full `ScadaLink.CentralUI.Tests` suite +still passes (568 / 568). + ### CentralUI-029 — `ConfigurationAuditLog` uses `JS.InvokeAsync("eval", ...)` instead of a dedicated JS module | | | diff --git a/code-reviews/ManagementService/findings.md b/code-reviews/ManagementService/findings.md index 7f04a0b8..1217d1c0 100644 --- a/code-reviews/ManagementService/findings.md +++ b/code-reviews/ManagementService/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 6 (1 Deferred — see ManagementService-012) | +| Open findings | 4 (1 Deferred — see ManagementService-012) | ## Summary @@ -796,7 +796,7 @@ Deployment user and an Admin user, in- and out-of-scope |--|--| | Severity | High | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ManagementService/ManagementActor.cs:153`–`:207`, `:336`, `:1302` | **Description** @@ -838,13 +838,26 @@ Recommended: option 1 plus a deprecation comment on `QueryAuditLogCommand` point so the ManagementActor route is redundant. Add a regression test asserting that a no-role / `Deployment`-only caller gets `ManagementUnauthorized` for `QueryAuditLogCommand`. +**Resolution** + +Resolved 2026-05-28 (commit pending) per recommendation option 1. `QueryAuditLogCommand` +was added to the Admin-required group in `GetRequiredRole`, with an inline comment +documenting the deliberate strictness vs. the keyset-paged `/api/audit/query` +(`OperationalAuditRoles`) and pointing new audit consumers at the REST endpoint. +The CentralUI `ConfigurationAuditLog` page reads via `ICentralUiRepository` directly +(not through this command), so the gate tightening does not break any UI flow. Two +regression tests pin the new behaviour: +`QueryAuditLogCommand_WithNoRoles_ReturnsUnauthorized` and +`QueryAuditLogCommand_WithDeploymentRole_ReturnsUnauthorized` — both fail on the +pre-fix code (the command fell through to "any authenticated user") and pass after. + ### ManagementService-019 — AuditEndpoints builds PermittedSiteIds but never enforces them | | | |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ManagementService/AuditEndpoints.cs:358`–`:368`, `:397`–`:437` | **Description** @@ -887,6 +900,27 @@ Recommended: option 1, mirroring the `ManagementActor` pattern — same security across both surfaces. Add a regression test that a site-scoped `AuditReadOnly` user filtering on an out-of-scope site gets a 403 (or an empty page). +**Resolution** + +Resolved 2026-05-28 (commit pending) per recommendation option 1. Added a public +helper `AuditEndpoints.ApplySiteScope(AuditLogQueryFilter, AuthenticatedUser)` that +returns the restricted filter (or `null` when the caller explicitly asks for an +out-of-scope site). Three cases: +- Empty `PermittedSiteIds` (Admin or any unscoped role) → filter returned unchanged. +- Scoped user with empty caller filter → `SourceSiteIds` set to the permitted set. +- Scoped user with explicit `SourceSiteIds` → intersected with the permitted set; + empty intersection returns `null` so `HandleQuery` / `HandleExport` emit a 403 + rather than silently producing an empty page. + +Both `HandleQuery` and `HandleExport` now call the helper after the role check and +short-circuit to `Forbidden("OperationalAudit"|"AuditExport")` on a `null` result. +Audit roles remain non-site-scoped by design (the design doc unchanged), but the +helper honours scope rules if an operator attaches them via the LDAP-mapping UI, +matching the existing `ManagementActor` pattern. Regression tests added in +`AuditEndpointsTests.ApplySiteScope_*` (5 tests): system-wide unchanged, +empty-caller-filter restricted, in-scope kept verbatim, out-of-scope returns null, +mixed-set intersected. + ### ManagementService-020 — UpdateSmtpConfig returns and audits the SMTP Credentials field verbatim | | | diff --git a/code-reviews/README.md b/code-reviews/README.md index 5c1d1ff0..60db5aa5 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,18 +40,18 @@ module file and counted in **Total**. | Severity | Open findings | |----------|---------------| | Critical | 0 | -| High | 18 | -| Medium | 62 | -| Low | 92 | -| **Total** | **172** | +| High | 16 | +| Medium | 58 | +| Low | 90 | +| **Total** | **164** | ## Module Status | Module | Last reviewed | Commit | Open (C/H/M/L) | Open | Total | |--------|---------------|--------|----------------|------|-------| | [AuditLog](AuditLog/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/8 | 11 | 11 | -| [CLI](CLI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/4 | 7 | 23 | -| [CentralUI](CentralUI/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/2/5 | 8 | 33 | +| [CLI](CLI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/4 | 6 | 23 | +| [CentralUI](CentralUI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/5 | 7 | 33 | | [ClusterInfrastructure](ClusterInfrastructure/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/4 | 4 | 14 | | [Commons](Commons/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/6 | 9 | 23 | | [Communication](Communication/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/1/5 | 7 | 22 | @@ -62,10 +62,10 @@ module file and counted in **Total**. | [HealthMonitoring](HealthMonitoring/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/5 | 7 | 23 | | [Host](Host/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/5 | 7 | 22 | | [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/3/4 | 8 | 25 | -| [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/3/2 | 6 | 23 | +| [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/2 | 4 | 23 | | [NotificationOutbox](NotificationOutbox/findings.md) | 2026-05-28 | `1eb6e97` | 0/2/5/3 | 10 | 10 | | [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/2/2/3 | 7 | 25 | -| [Security](Security/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/4 | 6 | 21 | +| [Security](Security/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 21 | | [SiteCallAudit](SiteCallAudit/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/4 | 6 | 6 | | [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-28 | `1eb6e97` | 0/1/2/6 | 9 | 23 | | [SiteRuntime](SiteRuntime/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/4/3 | 7 | 26 | @@ -84,18 +84,16 @@ description, location, recommendation — lives in the module's `findings.md`. _None open._ -### High (18) +### High (16) | ID | Module | Title | |----|--------|-------| -| CentralUI-028 | [CentralUI](CentralUI/findings.md) | `NotificationReport` and `SiteCallsReport` bypass `SiteScopeService` — Deployment role site-scoping defeated on the two new central-mirror pages | | Communication-016 | [Communication](Communication/findings.md) | `HandleConnectionStateChanged` is dead code — the documented disconnect-cleanup workflow never fires | | ConfigurationDatabase-015 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | `NotificationOutboxRepository.InsertIfNotExistsAsync` is a check-then-act race with no duplicate-key catch | | DataConnectionLayer-018 | [DataConnectionLayer](DataConnectionLayer/findings.md) | Concurrent subscribes for the same tag from different instances orphan an adapter subscription handle | | DeploymentManager-018 | [DeploymentManager](DeploymentManager/findings.md) | Reconciliation force-sets `Enabled`, overwriting an intentional `Disabled` after central failover | | ExternalSystemGateway-018 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `DeliverBufferedAsync` lets `JsonException` propagate, turning a corrupt buffered row into a permanent retry-forever poison message | | InboundAPI-022 | [InboundAPI](InboundAPI/findings.md) | `IActiveNodeGate` has no production registration in Host — standby-node gating is silently disabled in production | -| ManagementService-018 | [ManagementService](ManagementService/findings.md) | QueryAuditLogCommand has no role gate | | NotificationOutbox-001 | [NotificationOutbox](NotificationOutbox/findings.md) | `EmailNotificationDeliveryAdapter` inherits the OAuth2 empty-user SASL bug (NS-021) on the M365 send path | | NotificationOutbox-002 | [NotificationOutbox](NotificationOutbox/findings.md) | Dispatcher parks on first transient failure when `SmtpConfiguration.MaxRetries == 0` | | NotificationService-019 | [NotificationService](NotificationService/findings.md) | `NotificationDeliveryService` and `INotificationDeliveryService` are orphaned by the central-only redesign | @@ -107,7 +105,7 @@ _None open._ | Transport-002 | [Transport](Transport/findings.md) | ExternalSystem Overwrite never syncs methods | | Transport-003 | [Transport](Transport/findings.md) | Unlock lockout is enforced only client-side; server session is never marked Locked | -### Medium (62) +### Medium (58) | ID | Module | Title | |----|--------|-------| @@ -115,7 +113,6 @@ _None open._ | AuditLog-004 | [AuditLog](AuditLog/findings.md) | `SiteAuditReconciliationActor` advances cursor even on per-row insert failure, silently abandoning permanently-failing rows | | AuditLog-005 | [AuditLog](AuditLog/findings.md) | `GetBacklogStatsAsync` holds the SQLite hot-path write lock for the full COUNT+MIN scan | | CLI-017 | [CLI](CLI/findings.md) | `BundleCommands.RunBundleCommandAsync` duplicates `ExecuteCommandAsync` and breaks the auth exit-code contract | -| CLI-018 | [CLI](CLI/findings.md) | `audit query` and `audit export` never return exit 2 for an authorization failure | | CLI-019 | [CLI](CLI/findings.md) | `bundle export` decodes the entire base64 bundle into memory before writing | | CentralUI-026 | [CentralUI](CentralUI/findings.md) | `AuditFilterBar` From/To filters treat browser-local datetimes as UTC | | CentralUI-027 | [CentralUI](CentralUI/findings.md) | Same UTC misinterpretation in `SiteCallsReport`, `NotificationReport`, and `EventLogs` | @@ -141,7 +138,6 @@ _None open._ | InboundAPI-018 | [InboundAPI](InboundAPI/findings.md) | `AuditWriteMiddleware` fires `WriteAsync` as `_ = task` — faulted async writes are unobserved | | InboundAPI-021 | [InboundAPI](InboundAPI/findings.md) | `ParentExecutionId` correlation flows only through `Call`; attribute reads/writes lose the inbound→site execution-tree link | | InboundAPI-025 | [InboundAPI](InboundAPI/findings.md) | `AuditWriteMiddleware` runs against the entire `/api/*` branch — emits spurious `ApiInbound` audit rows for `/api/audit/query` and `/api/audit/export` | -| ManagementService-019 | [ManagementService](ManagementService/findings.md) | AuditEndpoints builds PermittedSiteIds but never enforces them | | ManagementService-020 | [ManagementService](ManagementService/findings.md) | UpdateSmtpConfig returns and audits the SMTP Credentials field verbatim | | ManagementService-021 | [ManagementService](ManagementService/findings.md) | Transport bundle handlers have zero test coverage | | NotificationOutbox-003 | [NotificationOutbox](NotificationOutbox/findings.md) | Dispatcher does not propagate a `CancellationToken` into delivery; in-flight SMTP sends cannot be cancelled on shutdown | @@ -151,8 +147,6 @@ _None open._ | NotificationOutbox-010 | [NotificationOutbox](NotificationOutbox/findings.md) | Comment claims `PipeTo` is not used "because the writer never throws"; the surrounding try/catch is dead-letter for the documented failure mode | | NotificationService-020 | [NotificationService](NotificationService/findings.md) | NS-001 fix superseded; `AkkaHostedService` would register two competing `Notification` S&F handlers if both code paths ran | | NotificationService-024 | [NotificationService](NotificationService/findings.md) | No test affirms the central-only invariant; the orphaned-path tests give a false coverage signal | -| Security-016 | [Security](Security/findings.md) | `RoleMapper` silently drops the system-wide Deployment grant when a user is also in any site-scoped Deployment group | -| Security-017 | [Security](Security/findings.md) | `SiteScopeRequirement` / `SiteScopeAuthorizationHandler` are dead code from production callers — `[Authorize(Policy = RequireDeployment)]` does NOT enforce site scoping | | SiteCallAudit-001 | [SiteCallAudit](SiteCallAudit/findings.md) | SupervisorStrategy override is dead code; XML claims Resume that is not enforced | | SiteCallAudit-003 | [SiteCallAudit](SiteCallAudit/findings.md) | `OnUpsertAsync` does not refresh `IngestedAtUtc`; direct-write callers must remember to stamp it | | SiteEventLogging-015 | [SiteEventLogging](SiteEventLogging/findings.md) | Background write queue is unbounded; can grow without limit under sustained writer slowness | @@ -174,7 +168,7 @@ _None open._ | Transport-007 | [Transport](Transport/findings.md) | Failed import sessions retain decrypted plaintext for the full 30-minute TTL | | Transport-010 | [Transport](Transport/findings.md) | Critical Overwrite + cross-cutting paths uncovered by tests | -### Low (92) +### Low (90) | ID | Module | Title | |----|--------|-------| @@ -245,8 +239,6 @@ _None open._ | NotificationService-022 | [NotificationService](NotificationService/findings.md) | `MailKitSmtpClientWrapper` holds a long-lived `SmtpClient`; combined with per-send factory, the design comment about pooling is contradicted | | NotificationService-023 | [NotificationService](NotificationService/findings.md) | XML docs on the orphaned classes still describe the removed site-delivery flow; misleading to maintainers | | NotificationService-025 | [NotificationService](NotificationService/findings.md) | `CredentialRedactor` over-masks: any 4-character credential component is masked anywhere it appears, including unrelated log text | -| Security-018 | [Security](Security/findings.md) | Role names are hard-coded magic strings duplicated across `RoleMapper`, `SiteScopeAuthorizationHandler`, and `AuthorizationPolicies` | -| Security-019 | [Security](Security/findings.md) | Service-account rebind failure is reported as "Invalid username or password" — masks misconfiguration as a user-credential error | | Security-020 | [Security](Security/findings.md) | `SecurityOptions` has no startup validation for required fields (`LdapServer`, `LdapSearchBase`) | | Security-021 | [Security](Security/findings.md) | `RequireHttpsCookie=false` dev opt-out has no warning path — an HTTP production deployment silently transmits the JWT bearer credential in cleartext | | SiteCallAudit-002 | [SiteCallAudit](SiteCallAudit/findings.md) | Singleton failover does not wait for in-flight async upserts | diff --git a/code-reviews/Security/findings.md b/code-reviews/Security/findings.md index a6be4314..62f4374c 100644 --- a/code-reviews/Security/findings.md +++ b/code-reviews/Security/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 6 (1 deferred — Security-008) | +| Open findings | 2 (Security-020, Security-021); 1 deferred (Security-008) | ## Summary @@ -706,7 +706,7 @@ use the single canonical identity. Regression tests |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/RoleMapper.cs:30-31`, `:41-55`, `:59` | **Description** @@ -742,13 +742,29 @@ scopedSiteIds` (left empty for system-wide users). Add a regression test `MapGroupsToRoles_UserInBothSystemWideAndScopedDeploymentGroup_IsSystemWide` covering the design's example pair `SCADA-Deploy-All` + `SCADA-Deploy-SiteA`. +**Resolution** + +Resolved 2026-05-28 (commit pending). `RoleMapper.MapGroupsToRolesAsync` now tracks two +independent flags per matched Deployment mapping: `hasUnscopedDeploymentMapping` (any +matched mapping with no scope rules) and `hasScopedDeploymentMapping` (any matched +mapping with scope rules). `isSystemWide` is `hasUnscopedDeploymentMapping || +(hasDeploymentRole && !hasScopedDeploymentMapping)` — so a user in both +`SCADA-Deploy-All` and `SCADA-Deploy-SiteA` is now correctly system-wide, with the +accumulated scope ids cleared. Magic string `"Deployment"` was replaced with the new +`Roles.Deployment` constant (Security-018). Regression test +`MapGroupsToRoles_UserInBothSystemWideAndScopedDeploymentGroup_IsSystemWide` +(seeds Site A, attaches a scope rule to the seeded `SCADA-Deploy-SiteA` mapping, and +asserts a user mapped via both `SCADA-Deploy-All` and `SCADA-Deploy-SiteA` resolves +to system-wide with empty `PermittedSiteIds`) fails on the pre-fix code and passes +after. + ### Security-017 — `SiteScopeRequirement` / `SiteScopeAuthorizationHandler` are dead code from production callers — `[Authorize(Policy = RequireDeployment)]` does NOT enforce site scoping | | | |--|--| | Severity | Medium | | Category | Design-document adherence | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/SiteScopeAuthorizationHandler.cs:8-58`; `src/ScadaLink.Security/AuthorizationPolicies.cs:113-143` | **Description** @@ -787,13 +803,25 @@ the request — that is a meaningful design extension and would need to be plann alongside the Central UI's existing `SiteScopeService` usage rather than replacing it piecemeal. +**Resolution** + +Resolved 2026-05-28 (commit pending) per recommendation option (a): deleted +`SiteScopeRequirement` and `SiteScopeAuthorizationHandler` outright, along with the +unwired `services.AddSingleton()` +registration in `AuthorizationPolicies.AddScadaLinkAuthorization` and the four +isolation tests in `SecurityTests.cs`. `SiteScopeService` (CentralUI-002) is now +documented as the sole site-scoping mechanism — the stale "mirrors +SiteScopeAuthorizationHandler" comment in `SiteScopeService.ResolveAsync` was rewritten +to say so. The cross-module partner CentralUI-028 is fixed in the same batch (the two +new report pages now consume `SiteScopeService`). + ### Security-018 — Role names are hard-coded magic strings duplicated across `RoleMapper`, `SiteScopeAuthorizationHandler`, and `AuthorizationPolicies` | | | |--|--| | Severity | Low | | Category | Code organization & conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/RoleMapper.cs:41`; `src/ScadaLink.Security/SiteScopeAuthorizationHandler.cs:36`; `src/ScadaLink.Security/AuthorizationPolicies.cs:118,121,124,95,107` | **Description** @@ -822,13 +850,26 @@ project and replace every magic-string occurrence — including the elements of `OperationalAuditRoles` and `AuditExportRoles` — with the constants. A single rename will then either succeed everywhere or fail to compile. +**Resolution** + +Resolved 2026-05-28 (commit pending). Added `src/ScadaLink.Security/Roles.cs` holding +`Admin`/`Design`/`Deployment`/`Audit`/`AuditReadOnly` as `public const string` +fields. Replaced every magic-string occurrence in this module: +`RoleMapper.MapGroupsToRolesAsync` now compares against `Roles.Deployment`; +`AuthorizationPolicies.AddScadaLinkAuthorization` binds `RequireClaim(...)` to +`Roles.{Admin,Design,Deployment}`; `OperationalAuditRoles` / +`AuditExportRoles` are now built from `Roles.Admin`, `Roles.Audit`, `Roles.AuditReadOnly`. +`SiteScopeAuthorizationHandler.cs` was deleted under Security-017, so its +`"Deployment"` literal is gone with it. A future rename now propagates by a +single edit or fails to compile. + ### Security-019 — Service-account rebind failure is reported as "Invalid username or password" — masks misconfiguration as a user-credential error | | | |--|--| | Severity | Low | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/LdapAuthService.cs:85-89`, `:147-151` | **Description** @@ -859,6 +900,25 @@ Add a regression test that exercises the service-account-bind failure path (a mo or seamed `LdapConnection.Bind` that throws on the second call) and asserts the distinct error message. +**Resolution** + +Resolved 2026-05-28 (commit pending). Added a new `ServiceAccountBindException` +(`src/ScadaLink.Security/ServiceAccountBindException.cs`) — deliberately NOT an +`LdapException` subtype so it short-circuits the generic LDAP catch — and a private +`BindServiceAccountAsync` helper on `LdapAuthService` that wraps both service-account +rebind sites (the post-user-bind group-lookup rebind AND the `ResolveUserDnAsync` +DN-search rebind). On `LdapException`, the helper logs Error +("Service-account rebind failed; check LdapServiceAccountDn / +LdapServiceAccountPassword configuration") and rethrows as `ServiceAccountBindException`, +which the outer `AuthenticateAsync` catch chain maps to the distinct user-facing message +"Authentication service is misconfigured. Contact an administrator." Service-account +faults no longer surface as "Invalid username or password". Regression test +`ServiceAccountBindException_DoesNotInheritLdapException_SoCatchOrderIsCorrect` pins the +load-bearing inheritance contract; a full end-to-end auth test that exercises the +service-account-bind failure path is not feasible without an LDAP seam in +`LdapAuthService` (the `LdapConnection` is constructed in-method), so the structural test +is the closest meaningful unit-level coverage. + ### Security-020 — `SecurityOptions` has no startup validation for required fields (`LdapServer`, `LdapSearchBase`) | | | diff --git a/src/ScadaLink.CLI/Commands/AuditExportHelpers.cs b/src/ScadaLink.CLI/Commands/AuditExportHelpers.cs index b693d4aa..86ef2011 100644 --- a/src/ScadaLink.CLI/Commands/AuditExportHelpers.cs +++ b/src/ScadaLink.CLI/Commands/AuditExportHelpers.cs @@ -1,5 +1,6 @@ using System.Globalization; using System.Net; +using System.Text.Json; namespace ScadaLink.CLI.Commands; @@ -147,10 +148,18 @@ public static class AuditExportHelpers if (!response.IsSuccessStatusCode) { var message = await response.Content.ReadAsStringAsync(); + // CLI-018: honour the documented "authorization failure → exit 2" + // contract on the REST audit surface as well. HTTP 403 is the + // primary signal; the server may also surface UNAUTHORIZED / + // FORBIDDEN via the JSON error envelope on a non-403 status. + var errorCode = TryExtractErrorCode(message); + var isAuthFailure = (int)response.StatusCode == 403 + || string.Equals(errorCode, "FORBIDDEN", StringComparison.OrdinalIgnoreCase) + || string.Equals(errorCode, "UNAUTHORIZED", StringComparison.OrdinalIgnoreCase); OutputFormatter.WriteError( string.IsNullOrWhiteSpace(message) ? $"Export failed (HTTP {(int)response.StatusCode})." : message, - "ERROR"); - return 1; + errorCode ?? "ERROR"); + return isAuthFailure ? 2 : 1; } await using var source = await response.Content.ReadAsStreamAsync(); @@ -163,4 +172,32 @@ public static class AuditExportHelpers output.WriteLine($"Exported audit log to {args.Output}"); return 0; } + + /// + /// Best-effort parse of the server's JSON error envelope ({ "error": ..., "code": ... }) + /// to extract the code field. Returns null if the body is empty, not valid JSON, or + /// has no code property — callers fall back to "ERROR" in that case. + /// + internal static string? TryExtractErrorCode(string body) + { + if (string.IsNullOrWhiteSpace(body)) + return null; + + try + { + using var doc = JsonDocument.Parse(body); + if (doc.RootElement.ValueKind == JsonValueKind.Object + && doc.RootElement.TryGetProperty("code", out var codeProp) + && codeProp.ValueKind == JsonValueKind.String) + { + return codeProp.GetString(); + } + } + catch (JsonException) + { + // Body is not a JSON envelope (e.g. an HTML proxy error page); no code to extract. + } + + return null; + } } diff --git a/src/ScadaLink.CLI/Commands/AuditQueryHelpers.cs b/src/ScadaLink.CLI/Commands/AuditQueryHelpers.cs index 173cce1e..cdb9c5d3 100644 --- a/src/ScadaLink.CLI/Commands/AuditQueryHelpers.cs +++ b/src/ScadaLink.CLI/Commands/AuditQueryHelpers.cs @@ -189,7 +189,9 @@ public static class AuditQueryHelpers { OutputFormatter.WriteError( response.Error ?? "Audit query failed.", response.ErrorCode ?? "ERROR"); - return 1; + // CLI-018: surface the documented "authorization failure → exit 2" + // contract for the audit REST surface too, not just /management. + return CommandHelpers.IsAuthorizationFailure(response) ? 2 : 1; } using var doc = JsonDocument.Parse(response.JsonData); diff --git a/src/ScadaLink.CLI/Commands/CommandHelpers.cs b/src/ScadaLink.CLI/Commands/CommandHelpers.cs index f1689fa8..6c056623 100644 --- a/src/ScadaLink.CLI/Commands/CommandHelpers.cs +++ b/src/ScadaLink.CLI/Commands/CommandHelpers.cs @@ -164,7 +164,7 @@ internal static class CommandHelpers /// both channels are honoured. (Authentication failure — HTTP 401 / bad credentials /// — is deliberately not treated as authorization failure; it is exit 1.) /// - private static bool IsAuthorizationFailure(ManagementResponse response) + internal static bool IsAuthorizationFailure(ManagementResponse response) { if (response.StatusCode == 403) return true; diff --git a/src/ScadaLink.CentralUI/Auth/SiteScopeService.cs b/src/ScadaLink.CentralUI/Auth/SiteScopeService.cs index d3ab48c5..85b08bea 100644 --- a/src/ScadaLink.CentralUI/Auth/SiteScopeService.cs +++ b/src/ScadaLink.CentralUI/Auth/SiteScopeService.cs @@ -88,8 +88,9 @@ public sealed class SiteScopeService ids.Add(id); } - // No SiteId claims => system-wide. This mirrors SiteScopeAuthorizationHandler: - // absence of scope rules means an unrestricted deployer. + // No SiteId claims => system-wide. Absence of scope rules means an + // unrestricted deployer (Security-017 made this service the sole + // site-scoping mechanism — there is no separate handler to mirror). var result = (IsSystemWide: ids.Count == 0, Sites: (IReadOnlySet)ids); _cached = result; return result; diff --git a/src/ScadaLink.CentralUI/Components/Pages/Notifications/NotificationReport.razor b/src/ScadaLink.CentralUI/Components/Pages/Notifications/NotificationReport.razor index a1e2c1ea..2d776ac7 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Notifications/NotificationReport.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Notifications/NotificationReport.razor @@ -1,11 +1,13 @@ @page "/notifications/report" @attribute [Authorize(Policy = ScadaLink.Security.AuthorizationPolicies.RequireDeployment)] +@using ScadaLink.CentralUI.Auth @using ScadaLink.Commons.Entities.Sites @using ScadaLink.Commons.Interfaces.Repositories @using ScadaLink.Commons.Messages.Notification @using ScadaLink.Communication @inject CommunicationService CommunicationService @inject ISiteRepository SiteRepository +@inject SiteScopeService SiteScope @inject IDialogService Dialog @inject ILogger Logger @@ -366,6 +368,12 @@ private ToastNotification _toast = default!; private List _sites = new(); + // CentralUI-028: full site list (kept unfiltered) so a permitted-site check + // resolves correctly for a SourceSiteId whose Site was filtered out of the + // dropdown. Set once in OnInitializedAsync alongside _sites. + private List _allSites = new(); + private bool _siteScopeSystemWide; + private HashSet _permittedSiteIds = new(); // List private List? _notifications; @@ -396,7 +404,13 @@ { try { - _sites = (await SiteRepository.GetAllSitesAsync()).ToList(); + _allSites = (await SiteRepository.GetAllSitesAsync()).ToList(); + // CentralUI-028: restrict the site dropdown to the user's permitted set + // so a site-scoped Deployment user cannot select a site they have no + // grant for. System-wide users see the full list back unchanged. + _sites = await SiteScope.FilterSitesAsync(_allSites); + _siteScopeSystemWide = await SiteScope.IsSystemWideAsync(); + _permittedSiteIds = new HashSet(await SiteScope.PermittedSiteIdsAsync()); } catch (Exception ex) { @@ -444,7 +458,12 @@ var response = await CommunicationService.QueryNotificationOutboxAsync(request); if (response.Success) { - _notifications = response.Notifications.ToList(); + // CentralUI-028: drop any row whose source site is outside the + // user's permitted set. The query API accepts only a single + // SourceSiteFilter, so a scoped user with an empty filter could + // otherwise see every site's rows; this is the row-level safety + // net behind the dropdown restriction. + _notifications = await FilterPermittedAsync(response.Notifications); _totalCount = response.TotalCount; } else @@ -461,6 +480,15 @@ private async Task RetryNotification(NotificationSummary n) { + // CentralUI-028: server-side re-check before relaying — even if the row + // somehow made it into the grid for an out-of-scope user (race with a + // permission change, stale circuit cache), the relay must not fire. + if (!await IsRowSiteAllowedAsync(n.SourceSiteId)) + { + _toast.ShowError("You are not permitted to act on notifications for this site."); + return; + } + var confirmed = await Dialog.ConfirmAsync( "Retry notification", $"Re-queue notification {ShortId(n.NotificationId)} (\"{n.Subject}\") for delivery?"); @@ -490,6 +518,12 @@ private async Task DiscardNotification(NotificationSummary n) { + if (!await IsRowSiteAllowedAsync(n.SourceSiteId)) + { + _toast.ShowError("You are not permitted to act on notifications for this site."); + return; + } + var confirmed = await Dialog.ConfirmAsync( "Discard notification", $"Permanently discard notification {ShortId(n.NotificationId)} (\"{n.Subject}\")? This cannot be undone.", @@ -650,4 +684,50 @@ "Discarded" => "bg-secondary", _ => "bg-light text-dark" }; + + /// + /// Drops any notification whose SourceSiteId resolves to a Site.Id outside + /// the caller's permitted set. A system-wide user gets the list back unchanged. + /// Lookup uses _allSites (NOT _sites) so a row whose Site was + /// filtered OUT of the dropdown is correctly classified as out-of-scope. + /// A truly unknown SourceSiteId (stale row from a deleted site) is kept — + /// there is no Site.Id to gate it on. + /// + private Task> FilterPermittedAsync( + IEnumerable notifications) + { + if (_siteScopeSystemWide) + return Task.FromResult(notifications.ToList()); + + var filtered = notifications + .Where(n => + { + var resolved = _allSites.FirstOrDefault(s => s.SiteIdentifier == n.SourceSiteId); + return resolved is null || _permittedSiteIds.Contains(resolved.Id); + }) + .ToList(); + return Task.FromResult(filtered); + } + + /// + /// Server-side re-check for the Retry/Discard relay actions. Returns true for a + /// system-wide user, or when the row's source site resolves to a Site.Id in the + /// caller's permitted set. An unresolvable site identifier defaults to allowed + /// (legacy behaviour); the relay's own site-scope re-check is then the + /// final gate. + /// + private bool IsRowSiteAllowedSync(string sourceSiteId) + { + if (_siteScopeSystemWide) + return true; + + var resolved = _allSites.FirstOrDefault(s => s.SiteIdentifier == sourceSiteId); + if (resolved is null) + return true; + + return _permittedSiteIds.Contains(resolved.Id); + } + + private Task IsRowSiteAllowedAsync(string sourceSiteId) + => Task.FromResult(IsRowSiteAllowedSync(sourceSiteId)); } diff --git a/src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor b/src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor index d24af8c4..f199e1ea 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor @@ -1,5 +1,6 @@ @page "/site-calls/report" @attribute [Authorize(Policy = ScadaLink.Security.AuthorizationPolicies.RequireDeployment)] +@using ScadaLink.CentralUI.Auth @using ScadaLink.Commons.Entities.Sites @using ScadaLink.Commons.Interfaces.Repositories @using ScadaLink.Commons.Messages.Audit diff --git a/src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor.cs b/src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor.cs index 9d0cf91d..100a8149 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor.cs +++ b/src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor.cs @@ -1,6 +1,7 @@ using Microsoft.AspNetCore.Components; using Microsoft.AspNetCore.WebUtilities; using Microsoft.Extensions.Logging; +using ScadaLink.CentralUI.Auth; using ScadaLink.CentralUI.Components.Shared; using ScadaLink.Commons.Entities.Sites; using ScadaLink.Commons.Messages.Audit; @@ -44,6 +45,7 @@ public partial class SiteCallsReport private const int PageSize = 50; [Inject] private NavigationManager Navigation { get; set; } = null!; + [Inject] private SiteScopeService SiteScope { get; set; } = null!; // The Status filter