review(Driver.Galaxy.Browser): fix mis-shifted MapSecurityClass codes (High)

Review at HEAD 7286d320. Driver.Galaxy.Browser-001 (High): MapSecurityClass codes 2-6 were all shifted vs the runtime SecurityClassification enum (wrong security labels in the picker) -> corrected all 7 arms + tests. -002: DisposeAsync swallows concurrent ObjectDisposedException. -003 (ResolveApiKey dup) deferred to Contracts.
2026-06-19 10:52:23 -04:00
parent 3c908f1df0
commit 960d76ffcb
3 changed files with 236 additions and 7 deletions
@@ -0,0 +1,162 @@
+# Code Review — Driver.Galaxy.Browser
+
+<!-- Template for a per-module findings file. Copy to code-reviews/<Module>/findings.md.
+     See ../../REVIEW-PROCESS.md for the full process. The base README.md is generated
+     from these files by regen-readme.py — do not edit README.md by hand. -->
+
+| Field | Value |
+|---|---|
+| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser` |
+| Reviewer | Claude Code |
+| Review date | 2026-06-19 |
+| Commit reviewed | `7286d320` |
+| Status | Reviewed |
+| Open findings | 1 |
+
+## Checklist coverage
+
+A comprehensive review completes every category, recording "No issues found" where
+a category produced nothing rather than leaving it blank.
+
+| # | Category | Result |
+|---|---|---|
+| 1 | Correctness & logic bugs | Driver.Galaxy.Browser-001 (High): MapSecurityClass codes are shifted — mismatches the runtime SecurityMap |
+| 2 | OtOpcUa conventions | No issues found |
+| 3 | Concurrency & thread safety | Driver.Galaxy.Browser-002 (Medium): DisposeAsync has a TOCTOU race on _rootGate.Dispose() |
+| 4 | Error handling & resilience | No issues found |
+| 5 | Security | No issues found |
+| 6 | Performance & resource management | No issues found |
+| 7 | Design-document adherence | No issues found |
+| 8 | Code organization & conventions | Driver.Galaxy.Browser-003 (Low): ResolveApiKey duplicated from GalaxyDriver with no sync mechanism |
+| 9 | Testing coverage | Driver.Galaxy.Browser-004 (Low): MapSecurityClass not unit-tested; pure static method with no gateway dependency |
+| 10 | Documentation & comments | No issues found |
+
+## Findings
+
+<!-- One ### entry per finding. IDs are <Module>-NNN, sequential within the module,
+     never reused. Findings are never deleted — close them by changing Status and
+     completing Resolution. -->
+
+### Driver.Galaxy.Browser-001
+
+| Field | Value |
+|---|---|
+| Severity | High |
+| Category | Correctness & logic bugs |
+| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyBrowseSession.cs:152` |
+| Status | Resolved |
+
+**Description:** `GalaxyBrowseSession.MapSecurityClass` maps Galaxy `security_classification`
+integer codes incorrectly. The Browser's switch arms are:
+
+| Code | Browser output | Runtime `SecurityMap` / `SecurityClassification` enum |
+|---|---|---|
+| 0 | "FreeAccess" | FreeAccess ✓ |
+| 1 | "Operate" | Operate ✓ |
+| 2 | "Tune" | **SecuredWrite** ✗ |
+| 3 | "Configure" | **VerifiedWrite** ✗ |
+| 4 | "ViewOnly" | **Tune** ✗ |
+| 5 | "Unknown(5)" | **Configure** ✗ |
+| 6 | "Unknown(6)" | **ViewOnly** ✗ |
+
+Codes 2–6 are all wrong. The AdminUI attribute side-panel labels "SecuredWrite" attributes
+as "Tune" and "VerifiedWrite" attributes as "Configure", causing operators to misread
+write-protection levels when selecting Galaxy tags. A tag the operator thinks is
+"Tune"-restricted is actually read-only (SecuredWrite / VerifiedWrite → ViewOnly from
+the OPC UA server's perspective). The correct mapping is already implemented in the
+sibling `Driver.Galaxy/Browse/SecurityMap.cs` and matches the `SecurityClassification`
+enum's integer assignments exactly.
+
+**Recommendation:** Fix `MapSecurityClass` to match `SecurityClassification` enum ordinals:
+0→FreeAccess, 1→Operate, 2→SecuredWrite, 3→VerifiedWrite, 4→Tune, 5→Configure,
+6→ViewOnly; unknown → `"Unknown({raw})"`. Add a unit test asserting each code.
+
+**Resolution:** Fixed 2026-06-19. Corrected all seven code-to-label arms in
+`MapSecurityClass` to match `SecurityClassification` enum ordinals. Regression tests
+`MapSecurityClass_maps_all_known_codes` and
+`MapSecurityClass_unknown_code_returns_Unknown_label` added.
+
+---
+
+### Driver.Galaxy.Browser-002
+
+| Field | Value |
+|---|---|
+| Severity | Medium |
+| Category | Concurrency & thread safety |
+| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyBrowseSession.cs:167` |
+| Status | Resolved |
+
+**Description:** `DisposeAsync` uses a non-atomic read-then-write pattern on the
+`_disposed` volatile field:
+
+```csharp
+if (_disposed) return;
+_disposed = true;
+_rootGate.Dispose();
+```
+
+Two concurrent callers can both observe `_disposed == false`, both set it to `true`, and
+both call `_rootGate.Dispose()`. The second call throws `ObjectDisposedException` from
+`SemaphoreSlim.Dispose()`, which propagates uncaught because the `try/catch` that
+follows only wraps `_client.DisposeAsync()`. The `BrowseSessionReaper` (background
+service) and a browser-side "Close" button can race in production.
+
+**Recommendation:** Wrap `_rootGate.Dispose()` in a `try/catch (ObjectDisposedException)`
+mirroring the existing client disposal pattern, or use an atomic int guard via
+`Interlocked.Exchange`.
+
+**Resolution:** Fixed 2026-06-19. Added `try { _rootGate.Dispose(); } catch (ObjectDisposedException) { }`
+mirroring the existing pattern for `_client.DisposeAsync()`. Regression test
+`DisposeAsync_concurrent_calls_do_not_throw` added.
+
+---
+
+### Driver.Galaxy.Browser-003
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Code organization & conventions |
+| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs:149` |
+| Status | Open |
+
+**Description:** `GalaxyDriverBrowser.ResolveApiKey` is a verbatim copy of
+`GalaxyDriver.ResolveApiKey`. The comment acknowledges this and explains why the Browser
+project intentionally does not reference Driver.Galaxy. However, Finding
+Driver.Galaxy.Browser-001 (the `MapSecurityClass` drift) demonstrates that duplicated
+logic diverges. If a new secret-ref prefix (e.g. `vault:`) is added to the runtime
+resolver, the Browser version will silently fall through to the cleartext-literal arm
+and emit a spurious warning.
+
+**Recommendation:** Extract `ResolveApiKey` into the `Driver.Galaxy.Contracts` project,
+which both `Driver.Galaxy` and `Driver.Galaxy.Browser` already reference. This is a
+one-line addition to Contracts — no migration, no public-contract break.
+
+**Resolution:** _(deferred — requires a change in the Galaxy.Contracts project, outside
+this module's boundary; tracked for a future consolidation pass)_
+
+---
+
+### Driver.Galaxy.Browser-004
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Testing coverage |
+| Location | `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser.Tests/GalaxyBrowseSessionTests.cs` |
+| Status | Resolved |
+
+**Description:** `GalaxyBrowseSession.MapSecurityClass` (all seven codes plus the
+`Unknown(N)` fallback) had zero unit-test coverage. The existing test comment correctly
+notes that RootAsync/ExpandAsync/AttributesAsync traversal is blocked by the internal
+transport seam, but `MapSecurityClass` is a pure static method with no gateway dependency
+— it is entirely testable in the unit suite. Finding Driver.Galaxy.Browser-001 (wrong
+mapping for codes 2–6) would have been caught immediately had these tests existed.
+
+**Recommendation:** Add `[Fact]` tests for each of the seven known codes (0–6) and the
+unknown-code fallback.
+
+**Resolution:** Fixed 2026-06-19. Tests `MapSecurityClass_maps_all_known_codes` and
+`MapSecurityClass_unknown_code_returns_Unknown_label` added, covering all codes 0–6
+plus an out-of-range value.
@@ -146,28 +146,42 @@ internal sealed class GalaxyBrowseSession : IBrowseSession

    /// <summary>
    ///     Maps the Galaxy raw security-classification integer to a display string.
-    ///     Buckets: 0=FreeAccess, 1=Operate, 2=Tune, 3=Configure, 4=ViewOnly;
-    ///     anything else surfaces as <c>Unknown(N)</c>.
+    ///     Matches the <c>SecurityClassification</c> enum ordinals and the runtime
+    ///     <c>SecurityMap</c> in Driver.Galaxy:
+    ///     0=FreeAccess, 1=Operate, 2=SecuredWrite, 3=VerifiedWrite,
+    ///     4=Tune, 5=Configure, 6=ViewOnly; anything else surfaces as <c>Unknown(N)</c>.
    /// </summary>
-    private static string MapSecurityClass(int raw) => raw switch
+    // internal for unit-test access (InternalsVisibleTo on the src project).
+    internal static string MapSecurityClass(int raw) => raw switch
    {
        0 => "FreeAccess",
        1 => "Operate",
-        2 => "Tune",
-        3 => "Configure",
-        4 => "ViewOnly",
+        2 => "SecuredWrite",
+        3 => "VerifiedWrite",
+        4 => "Tune",
+        5 => "Configure",
+        6 => "ViewOnly",
        _ => $"Unknown({raw})",
    };

    /// <summary>
    ///     Idempotently tears down the underlying repository client. Swallows exceptions
    ///     on shutdown — the registry's reaper may be racing a client-initiated close.
+    ///     Both <see cref="SemaphoreSlim.Dispose"/> and the client dispose are wrapped
+    ///     in try/catch so a concurrent second dispose call never propagates.
    /// </summary>
    public async ValueTask DisposeAsync()
    {
        if (_disposed) return;
        _disposed = true;
-        _rootGate.Dispose();
+        try
+        {
+            _rootGate.Dispose();
+        }
+        catch (ObjectDisposedException)
+        {
+            // Concurrent DisposeAsync caller already disposed the gate — safe to ignore.
+        }
        try
        {
            await _client.DisposeAsync().ConfigureAwait(false);
@@ -112,4 +112,57 @@ public sealed class GalaxyBrowseSessionTests
        await Should.ThrowAsync<ArgumentException>(
            () => session.ExpandAsync("Galaxy.Unknown", TestContext.Current.CancellationToken));
    }
+
+    /// <summary>
+    ///     Verifies that each of the seven defined Galaxy security_classification codes
+    ///     maps to the correct label string — matching the <c>SecurityClassification</c>
+    ///     enum ordinals and the runtime <c>SecurityMap</c> in Driver.Galaxy.
+    ///     (Regression for Driver.Galaxy.Browser-001: codes 2–6 were all wrong before
+    ///     this fix.)
+    /// </summary>
+    [Theory]
+    [InlineData(0, "FreeAccess")]
+    [InlineData(1, "Operate")]
+    [InlineData(2, "SecuredWrite")]
+    [InlineData(3, "VerifiedWrite")]
+    [InlineData(4, "Tune")]
+    [InlineData(5, "Configure")]
+    [InlineData(6, "ViewOnly")]
+    public void MapSecurityClass_maps_all_known_codes(int code, string expectedLabel)
+    {
+        GalaxyBrowseSession.MapSecurityClass(code).ShouldBe(expectedLabel);
+    }
+
+    /// <summary>
+    ///     An unrecognised Galaxy security_classification code must produce an
+    ///     <c>Unknown(N)</c> label — not throw and not silently return a valid class.
+    /// </summary>
+    [Theory]
+    [InlineData(7)]
+    [InlineData(99)]
+    [InlineData(-1)]
+    public void MapSecurityClass_unknown_code_returns_Unknown_label(int code)
+    {
+        var label = GalaxyBrowseSession.MapSecurityClass(code);
+        label.ShouldStartWith("Unknown(");
+        label.ShouldContain(code.ToString());
+    }
+
+    /// <summary>
+    ///     Two concurrent <see cref="GalaxyBrowseSession.DisposeAsync"/> calls on the
+    ///     same session must not throw. The registry reaper and a browser-side Close
+    ///     can race in production.
+    ///     (Regression for Driver.Galaxy.Browser-002: the second caller could throw
+    ///     <see cref="ObjectDisposedException"/> from <c>SemaphoreSlim.Dispose()</c>.)
+    /// </summary>
+    [Fact]
+    public async Task DisposeAsync_concurrent_calls_do_not_throw()
+    {
+        var session = new GalaxyBrowseSession(NewClient());
+        // Fire two concurrent dispose calls and await both — neither must throw.
+        await Should.NotThrowAsync(async () =>
+        {
+            await Task.WhenAll(session.DisposeAsync().AsTask(), session.DisposeAsync().AsTask());
+        });
+    }
 }