dohertj2/lmxopcua
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(code-review): resolve Batch 1 open findings (AdminUI auth, AlarmHistorian dispose guards, docs)

Browse Source 
- AdminUI-001: gate Script editor pages at Administrator,Designer + loosen ScriptAnalysis backend to match
- AdminUI-004: explicit [Authorize] on FleetStatus/Alert/ScriptLog hubs
- Core.AlarmHistorian-014: ObjectDisposedException guards on GetStatus/RetryDeadLettered (+ regression test)
- Core.Scripting.Abstractions-004/-007: Deadband tolerance doc + stale ScriptedAlarms.md path
- Host-003: correct config-overlay precedence in ServiceHosting.md
- Configuration-014: LdapGroupRoleMapping collation-dependency doc
- Driver.TwinCAT.Contracts-002: Structure enum doc (discovery-only sentinel)
This commit is contained in:
Joseph Doherty
2026-06-20 22:30:33 -04:00
parent c13fcc1d51
commit 98b27fc1b6
19 changed files with 96 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/AdminUI/findings.md
+16 -5
View File
@@ -7,7 +7,7 @@
| Review date | 2026-06-19 |
| Commit reviewed | `7286d320` |
| Status | Reviewed |
| Open findings | 2 |
| Open findings | 0 |
## Checklist coverage
@@ -33,7 +33,7 @@
| Severity | Medium |
| Category | Security |
| Location | `Components/Pages/ScriptEdit.razor:5`, `Components/Pages/Scripts.razor` |
| Status | Open |
| Status | Resolved |
**Description:** The Script CRUD surface (`/scripts/new`, `/scripts/{id}`, and the `/scripts` list) is
gated by only `[Authorize]` (any authenticated user), whereas the *same editor's* Roslyn-analysis
@@ -50,7 +50,14 @@ and `Scripts` with `Authorize(Roles = "Administrator,Designer")` (matching `/dep
confirm the intended tier with the security owner, since it changes who can reach the editor. Left Open
pending that decision (an authorization-policy decision, not a self-contained bug fix).
**Resolution:** _(open)_
**Resolution:** Resolved 2026-06-20 — `ScriptEdit.razor:5` and `Scripts.razor:2` changed from
`[Authorize]` to `[Authorize(Roles = "Administrator,Designer")]` (namespace-qualified form, matching
`Deployments.razor`). `ScriptAnalysisEndpoints.cs` MapGroup changed from `.RequireAuthorization("FleetAdmin")`
to `.RequireAuthorization(new AuthorizeAttribute { Roles = "Administrator,Designer" })` so the
IntelliSense backend matches the page gate (Designer was previously 403'd by the FleetAdmin policy).
`using Microsoft.AspNetCore.Authorization;` added to `ScriptAnalysisEndpoints.cs`. Build-verified:
`dotnet build ZB.MOM.WW.OtOpcUa.AdminUI.csproj` — 0 errors. Live page-gating verification deferred
to docker-dev `/run`.
### AdminUI-002
@@ -107,7 +114,7 @@ CTS (and its timer) is disposed when the handler returns. Razor-behavioural chan
| Severity | Low |
| Category | Security |
| Location | `Hubs/FleetStatusHub.cs`, `Hubs/AlertHub.cs`, `Hubs/ScriptLogHub.cs` (vs `Hubs/DriverStatusHub.cs:12`) |
| Status | Open |
| Status | Resolved |
**Description:** Of the four SignalR hubs mapped by `MapOtOpcUaHubs`, only `DriverStatusHub` carries an
explicit `[Authorize]` attribute. `FleetStatusHub`, `AlertHub`, and `ScriptLogHub` (which broadcast
@@ -124,7 +131,11 @@ host's fallback policy. Low-risk (the fallback already requires auth, so observa
unchanged) but left Open because hub authorization is a cross-cutting concern worth confirming with the
Host/Security owners rather than silently changing in a single-module pass.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-06-20 — `[Authorize]` added to `FleetStatusHub`, `AlertHub`, and
`ScriptLogHub`, each with `using Microsoft.AspNetCore.Authorization;`, mirroring the exact form used by
`DriverStatusHub`. Observable behaviour is unchanged (the global FallbackPolicy already required auth);
authorization is now explicit and hub-local. Build-verified: `dotnet build ZB.MOM.WW.OtOpcUa.AdminUI.csproj`
— 0 errors.
### AdminUI-005
code-reviews/Configuration/findings.md
+3 -3
View File
@@ -7,7 +7,7 @@
| Review date | 2026-06-19 (re-review; first reviewed 2026-05-22) |
| Commit reviewed | `7286d320` (re-review; was `76d35d1`) |
| Status | Reviewed |
| Open findings | 2 |
| Open findings | 1 |
## Checklist coverage
@@ -247,10 +247,10 @@ Prior findings Configuration-001…011 remain Resolved. Notable since the first
| Severity | Low |
| Category | Documentation & comments |
| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Services/ILdapGroupRoleMappingService.cs:23`, `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Services/LdapGroupRoleMappingService.cs:24` |
| Status | Open |
| Status | Resolved |
**Description:** `ILdapGroupRoleMappingService.GetByGroupsAsync`'s XML doc asserts "Case-insensitive per LDAP conventions", but the implementation is `db.LdapGroupRoleMappings.Where(m => groupSet.Contains(m.LdapGroup))`, which translates to a SQL `IN (…)` whose case-sensitivity is entirely determined by the column's collation. On the default case-insensitive server collation (the dev/Docker SQL) the doc holds; on a case-sensitive-collation deployment the lookup would silently miss rows that differ only in case — and the in-memory EF unit tests (which match exact case) would not catch it. This is a hot path: it fires on every Admin-UI sign-in, so a silent miss denies the user their role grant. The doc overstates a guarantee the code does not enforce.
**Recommendation:** Either (a) soften the doc to state the match is collation-dependent and document the required CI collation as a deployment constraint, or (b) pin the `LdapGroup` column to an explicit `_CI_` collation in the model so the guarantee is enforced regardless of server default. Option (b) is a schema/migration change and must be deferred (no migration edits in this review); option (a) is a one-line doc change but only papers over the gap. Left **Open** pending a decision on which guarantee to commit to — no code change applied because the documented behaviour currently matches the deployment reality (CI server collation).
**Resolution:** _(open — preferred fix (b) pins the column collation, which needs an EF migration; deferred per the no-migration rule)_
**Resolution:** Fixed 2026-06-20 (option a). Replaced the "Case-insensitive per LDAP conventions" claim in `ILdapGroupRoleMappingService.GetByGroupsAsync`'s XML `<remarks>` with a note that the match is a SQL `IN (…)` whose case-sensitivity is determined by the `LdapGroup` column collation, and that case-insensitive behaviour requires a CI server or column collation as a deployment requirement. The impl (`LdapGroupRoleMappingService`) carries no duplicate claim. Build confirmed 0 errors. Schema/migration change (option b) remains deferred.
code-reviews/Core.AlarmHistorian/findings.md
+3 -3
View File
@@ -7,7 +7,7 @@
| Review date | 2026-06-19 |
| Commit reviewed | `621d00e4` |
| Status | Reviewed |
| Open findings | 1 |
| Open findings | 0 |
## Checklist coverage
@@ -249,10 +249,10 @@ All 11 prior findings confirmed resolved at HEAD. The re-review covered all 10 c
| Severity | Low |
| Category | Error handling & resilience |
| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/SqliteStoreAndForwardSink.cs:515-555,558-568` |
| Status | Open |
| Status | Resolved |
**Description:** `GetStatus()` and `RetryDeadLettered()` have no `_disposed` guard. After `Dispose()` is called both methods still open new `SqliteConnection` objects against the database file (which persists on disk after disposal). This works because SQLite is file-backed, but the behavior is surprising and inconsistent with the convention that post-disposal calls on an `IDisposable` object throw `ObjectDisposedException`. `RetryDeadLettered()` in particular performs a write (`UPDATE Queue …`) against a sink that the owner has declared finished. If a lingering Admin UI polling loop calls `GetStatus()` after the host shuts down and disposes the sink, no error is surfaced — the method silently succeeds.
**Recommendation:** Add `if (_disposed) throw new ObjectDisposedException(nameof(SqliteStoreAndForwardSink));` guards at the top of `GetStatus()` and `RetryDeadLettered()`, consistent with the guards already present on `EnqueueAsync` and `StartDrainLoop`.
**Deferred:** The current behavior (silently succeeds post-disposal) is benign in production because the Admin UI is also shutting down when the host disposes the sink. Adding the guard is a one-line change per method but requires coordinating with the Admin UI callers to confirm they tolerate `ObjectDisposedException` after shutdown — that is a cross-module concern. Deferring until the Admin UI shutdown sequence is reviewed.
**Resolution:** Resolved 2026-06-20 via TDD. Added `if (_disposed) throw new ObjectDisposedException(nameof(SqliteStoreAndForwardSink));` as the first statement in both `GetStatus()` and `RetryDeadLettered()`, matching the identical guard form used by `EnqueueAsync` and `StartDrainLoop`. Regression test `Disposed_sink_throws_ObjectDisposedException_on_GetStatus_and_RetryDeadLettered` added; confirmed fail-before / pass-after. All 29 tests green.
code-reviews/Core.Scripting.Abstractions/findings.md
+10 -8
View File
@@ -11,7 +11,7 @@
| Review date | 2026-06-19 |
| Commit reviewed | `7286d320` |
| Status | Reviewed |
| Open findings | 2 |
| Open findings | 0 |
## Checklist coverage
@@ -112,7 +112,7 @@ current naming convention. Verified by build (no test project for this module).
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/ScriptContext.cs:84` |
| Status | Open |
| Status | Resolved |
**Description:** `ScriptContext.Deadband` has no guard or documentation for invalid `tolerance`
values. A negative `tolerance` makes `Math.Abs(current - previous) > tolerance` trivially `true`
@@ -132,9 +132,9 @@ changes the public API contract for authored scripts and needs a plan-level deci
/// Negative values cause the function to always return true; NaN always returns false.</param>
```
**Resolution:** _(Open — the doc-only fix is zero-risk but the tolerance param is part of the
locked script API surface; the decision on whether to also add a guard belongs at the plan level.
Deferred until that call is made. No test project exists for this module.)_
**Resolution:** Applied doc-only fix — updated the `<param name="tolerance">` XML doc in
`ScriptContext.cs:90` to document that negative values always return true and NaN always returns
false. No guard added (API contract change deferred per owner decision). 2026-06-20.
---
@@ -189,7 +189,7 @@ remarks in `ScriptContext.cs`. Verified by build (no test project for this modul
| Severity | Low |
| Category | Design-document adherence |
| Location | `docs/ScriptedAlarms.md:298` |
| Status | Open |
| Status | Resolved |
**Description:** `docs/ScriptedAlarms.md` line 298 lists the file path as
`src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/AlarmPredicateContext.cs`. The file was moved to
@@ -204,5 +204,7 @@ only thing that changed.
- `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/AlarmPredicateContext.cs` — script-side `ScriptContext` (read-only, write rejected)
```
**Resolution:** _(Open — out of scope for this module's src-only fix pass. Should be fixed in a
docs-cleanup commit targeting `docs/ScriptedAlarms.md`.)_
**Resolution:** Updated `docs/ScriptedAlarms.md` line 298 — replaced the stale
`Core.ScriptedAlarms/AlarmPredicateContext.cs` path with the correct
`Core.Scripting.Abstractions/AlarmPredicateContext.cs` path. Verified the file exists at the new
location before editing. 2026-06-20.
code-reviews/Driver.TwinCAT.Contracts/findings.md
+8 -3
View File
@@ -7,7 +7,7 @@
| Review date | 2026-06-19 |
| Commit reviewed | `a19b0f86` |
| Status | Reviewed |
| Open findings | 1 |
| Open findings | 0 |
## Checklist coverage
@@ -83,7 +83,7 @@ the actual `TryGetInt32` call; no behaviour change. Verified by build: 0 errors,
| Severity | Low |
| Category | Documentation & comments |
| Location | `TwinCATDataType.cs:33` (`Structure` member) |
| Status | Open |
| Status | Resolved |
**Description:** The `Structure` enum member's XML summary says "UDT / FB instance. Resolved
per member at discovery time." This is accurate for the discovery path, but the comment omits
@@ -108,4 +108,9 @@ not appear in operator-authored config.
Structure,
```
**Resolution:** _(empty until closed)_
**Resolution:** Resolved 2026-06-20 — expanded the `Structure` XML doc on `TwinCATDataType.cs:33`
to document the full contract: discovery-only sentinel, rejected at pre-declaration by
`TwinCATDriverFactoryExtensions.BuildTag` (`InvalidOperationException`), and not offered by the
AdminUI typed editor (parser falls back to `DInt`). Facts verified against
`TwinCATDriverFactoryExtensions.cs` (line 96–99) and `TwinCATEquipmentTagParser.cs` (line 31).
Build: 0 errors, 0 warnings.
code-reviews/Host/findings.md
+3 -3
View File
@@ -11,7 +11,7 @@
| Review date | 2026-06-19 |
| Commit reviewed | `7286d320` |
| Status | Reviewed |
| Open findings | 1 |
| Open findings | 0 |
## Checklist coverage
@@ -78,13 +78,13 @@ a category produced nothing rather than leaving it blank.
| Severity | Low |
| Category | Design-document adherence |
| Location | `docs/ServiceHosting.md` (section "Per-role configuration overlays") |
| Status | Open |
| Status | Resolved |
**Description:** `docs/ServiceHosting.md` states the configuration loading order as "base `appsettings.json` → role overlay (`appsettings.{role}.json`) → environment overlay (`appsettings.{Environment}.json`) — later layers win." This is incorrect. The actual order established by `Program.cs:59–71` is: `appsettings.json` → `appsettings.{Environment}.json` (WebApplicationBuilder default) → `appsettings.{role}.json` (appended by Program.cs) → environment variables (re-appended) → command-line args (re-appended). The role overlay therefore **wins over** `appsettings.{Environment}.json`, not the other way around. The code behaviour is correct and intentional (explained by the comment at Program.cs:66–70); only the doc is wrong.
**Recommendation:** Update `docs/ServiceHosting.md` to reflect the actual precedence: `appsettings.json` < `appsettings.{Environment}.json` < `appsettings.{role}.json` < environment variables < command-line args. Note that the role overlay intentionally outranks the environment-specific JSON so role-level security defaults cannot be overridden by a developer's local `appsettings.Development.json`, while environment variables and command-line args still outrank everything. Docs-only change; no src change needed.
**Resolution:** _(open — docs/ edit outside this pass's src-only scope; no code change required)_
**Resolution:** Fixed 2026-06-20. Rewrote the "Per-role configuration overlays" loading-order sentence in `docs/ServiceHosting.md` to show the correct ascending-precedence chain (`appsettings.json` < `appsettings.{Environment}.json` < `appsettings.{role}.json` < env vars < CLI args) and added a sentence explaining that the role overlay intentionally outranks the environment-specific JSON so role-level security defaults (e.g. `DevStubMode = false`) cannot be overridden by a developer's local `appsettings.Development.json`. No src change required.
---