feat(security): AclRevalidationTimer — clock-driven monitored-item revalidation (archreview 03/S19)

archreview/plans/R3-05-acl-hot-path-expiry-plan.md

@@ -657,3 +657,15 @@ for the orchestrator.
   remark is now qualified by node role and points at the bound, and `ConfigDbApiKeyStore` records both
   that it is wrapped in production and that "revocation on the next request" is **activation**-scoped.
 - Host.Tests: 123 green (was 106 at base).
+
+### T6 — `AclRevalidationTimer`
+
+- **RED observed:** `error CS0246: The type or namespace name 'AclRevalidationTimer' could not be found`.
+- **DEVIATION (visibility):** the plan declares the class `internal sealed`. `ZB.MOM.WW.OtOpcUa.Host` has
+  **no `InternalsVisibleTo`**, so the tests could not reach it. Made `public sealed`, matching
+  `BoundedApiKeyStore` and every other type in `Host/OpcUa/`.
+- Added beyond the plan's list: `A_tick_runs_the_pass` — the control for the two "nothing happened" cases
+  (a `Tick()` that silently did nothing would satisfy the throwing-pass and dispose cases on its own).
+- 5 tests, green. Also corrected the T4 `MeterRecorder` remark: the assembly declares
+  `CollectionBehavior(DisableTestParallelization = true)`, which is a stronger and simpler reason the
+  name-only filter is safe than the one first written.

archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json

@@ -43,7 +43,7 @@
     {
       "id": "T6",
       "subject": "AclRevalidationTimer (pure, TimeProvider-driven, non-throwing tick) + unit tests (03/S19)",
-      "status": "pending",
+      "status": "done",
       "blockedBy": [
         "T1"
       ]

src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/AclRevalidationTimer.cs

@@ -0,0 +1,99 @@
+using Microsoft.Extensions.Logging;
+using ZB.MOM.WW.OtOpcUa.Core.Authorization;
+
+namespace ZB.MOM.WW.OtOpcUa.Host.OpcUa;
+
+/// <summary>
+///     Runs the node manager's monitored-item revalidation pass on a low-frequency tick, so a key that
+///     expires by the CLOCK reaches a live subscription without waiting for a deployment (arch-review
+///     03/S19).
+/// </summary>
+/// <remarks>
+///     <para>
+///         Every request-shaped operation re-checks the calling key's usability per call and so honours
+///         <c>ExpiresUtc</c> the moment it passes. A subscription is authorized once and then streams, and
+///         streaming is how machine consumers actually read this server — so before this the only thing
+///         that could stand a subscription down was a deployment.
+///     </para>
+///     <para>
+///         ⚠️ <b>The tick must never throw.</b> An unhandled exception on a timer callback thread takes
+///         the process down. Every tick is wrapped; a faulting pass is logged and the timer keeps running,
+///         because the next tick is the recovery.
+///     </para>
+///     <para>
+///         ⚠️ <b>Each tick gets a fresh memo, and that is the pass's own constraint, not this class's.</b>
+///         <c>RevalidateMonitoredItems</c> allocates its verdict dictionary per call — see the remarks on
+///         <c>OtOpcUaNodeManager.OnAuthorityChanged</c>. Nothing here may hoist it across ticks, or an
+///         expiry landing between two ticks would be served the earlier tick's allow.
+///     </para>
+///     <para>
+///         It adds no decision logic: the verdict comes from the authorizer's own
+///         <see cref="TimeProvider"/>, and the stand-down (queue <c>BadUserAccessDenied</c>, then detach)
+///         and its restore are the pass's, unchanged. The pass early-returns unless ACL enforcement is on,
+///         so this is inert on the default configuration.
+///     </para>
+/// </remarks>
+public sealed class AclRevalidationTimer : IDisposable
+{
+    private readonly Action _revalidate;
+    private readonly ILogger _log;
+    private readonly ITimer? _timer;
+    private volatile bool _disposed;
+
+    /// <summary>Initializes a new instance of the <see cref="AclRevalidationTimer"/> class.</summary>
+    /// <param name="revalidate">The revalidation pass to run on each tick.</param>
+    /// <param name="interval">
+    ///     How often to run it. Non-positive arms no timer at all — the deploy-triggered-only behaviour
+    ///     that predates 03/S19.
+    /// </param>
+    /// <param name="time">The shared clock; its <c>CreateTimer</c> is what arms the tick.</param>
+    /// <param name="log">The logger.</param>
+    public AclRevalidationTimer(Action revalidate, TimeSpan interval, TimeProvider time, ILogger log)
+    {
+        ArgumentNullException.ThrowIfNull(revalidate);
+        ArgumentNullException.ThrowIfNull(time);
+        ArgumentNullException.ThrowIfNull(log);
+
+        _revalidate = revalidate;
+        _log = log;
+
+        if (interval <= TimeSpan.Zero)
+        {
+            // Say so once. A disabled guard that logs nothing is indistinguishable from a guard that is
+            // running and finding nothing, which is precisely the confusion 03/S19 is about.
+            _log.LogInformation(
+                "AclRevalidationTimer: disabled — {Section}:RevalidationIntervalSeconds is not positive, "
+                + "so live monitored items are re-authorized only when a deployment lands",
+                AclOptions.SectionName);
+            return;
+        }
+
+        _timer = time.CreateTimer(_ => Tick(), state: null, interval, interval);
+        _log.LogInformation(
+            "AclRevalidationTimer: live monitored items will be re-authorized every {Interval}", interval);
+    }
+
+    /// <summary>Runs one revalidation pass, absorbing anything it throws.</summary>
+    /// <remarks>Exposed so a test can drive the callback body deterministically rather than by waiting.</remarks>
+    public void Tick()
+    {
+        if (_disposed) return;
+
+        try
+        {
+            _revalidate();
+        }
+        catch (Exception ex)
+        {
+            _log.LogError(ex, "AclRevalidationTimer: the monitored-item revalidation pass threw; "
+                              + "the timer keeps running and the next tick is the recovery");
+        }
+    }
+
+    /// <inheritdoc />
+    public void Dispose()
+    {
+        _disposed = true;
+        _timer?.Dispose();
+    }
+}

tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/AclRevalidationTimerTests.cs

@@ -0,0 +1,151 @@
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Host.OpcUa;
+
+namespace ZB.MOM.WW.OtOpcUa.Host.Tests.OpcUa;
+
+/// <summary>
+///     Covers <see cref="AclRevalidationTimer"/> — the low-frequency trigger that makes key expiry reach
+///     a live subscription without waiting for a deployment (arch-review 03/S19).
+/// </summary>
+public sealed class AclRevalidationTimerTests
+{
+    /// <summary>A positive interval arms one timer whose due time and period are both that interval.</summary>
+    [Fact]
+    public void A_positive_interval_creates_a_timer_with_that_period()
+    {
+        var clock = new RecordingTimeProvider();
+
+        using var _ = new AclRevalidationTimer(
+            () => { }, TimeSpan.FromSeconds(30), clock, NullLogger.Instance);
+
+        clock.Created.Count.ShouldBe(1);
+        clock.Created[0].DueTime.ShouldBe(TimeSpan.FromSeconds(30));
+        clock.Created[0].Period.ShouldBe(TimeSpan.FromSeconds(30));
+    }
+
+    /// <summary>
+    ///     A non-positive interval arms nothing at all, and says so once — a disabled guard should be
+    ///     visible in the log rather than inferred from an absence of activity.
+    /// </summary>
+    [Fact]
+    public void A_zero_interval_creates_no_timer()
+    {
+        var clock = new RecordingTimeProvider();
+        var log = new CapturingLogger();
+
+        using var _ = new AclRevalidationTimer(() => { }, TimeSpan.Zero, clock, log);
+
+        clock.Created.ShouldBeEmpty();
+        log.Lines.ShouldContain(l => l.Contains("RevalidationIntervalSeconds", StringComparison.Ordinal));
+    }
+
+    /// <summary>
+    ///     A pass that throws must not escape the tick.
+    /// </summary>
+    /// <remarks>
+    ///     An unhandled exception on a timer callback thread takes the process down. The next tick is the
+    ///     recovery, so the fault is logged and the timer keeps running.
+    /// </remarks>
+    [Fact]
+    public void A_throwing_pass_does_not_escape_the_tick()
+    {
+        var log = new CapturingLogger();
+        using var timer = new AclRevalidationTimer(
+            () => throw new InvalidOperationException("pass blew up"),
+            TimeSpan.FromSeconds(30),
+            new RecordingTimeProvider(),
+            log);
+
+        Should.NotThrow(timer.Tick);
+        log.Lines.ShouldContain(l => l.Contains("revalidation", StringComparison.OrdinalIgnoreCase));
+    }
+
+    /// <summary>A tick runs the pass — the control for the two "nothing happened" cases above.</summary>
+    [Fact]
+    public void A_tick_runs_the_pass()
+    {
+        var runs = 0;
+        using var timer = new AclRevalidationTimer(
+            () => runs++, TimeSpan.FromSeconds(30), new RecordingTimeProvider(), NullLogger.Instance);
+
+        timer.Tick();
+        timer.Tick();
+
+        runs.ShouldBe(2);
+    }
+
+    /// <summary>Disposing disposes the underlying timer and stops further ticks.</summary>
+    [Fact]
+    public void Dispose_stops_further_ticks()
+    {
+        var clock = new RecordingTimeProvider();
+        var runs = 0;
+        var timer = new AclRevalidationTimer(
+            () => runs++, TimeSpan.FromSeconds(30), clock, NullLogger.Instance);
+
+        timer.Dispose();
+
+        clock.Created[0].Disposed.ShouldBeTrue();
+        timer.Tick();
+        runs.ShouldBe(0, "a disposed timer must not run the pass again");
+    }
+
+    // ---------- helpers ----------
+
+    /// <summary>
+    ///     A <see cref="TimeProvider"/> that records the timers asked of it instead of arming real ones.
+    /// </summary>
+    private sealed class RecordingTimeProvider : TimeProvider
+    {
+        /// <summary>Every timer this provider was asked to create, in order.</summary>
+        public List<FakeTimer> Created { get; } = [];
+
+        /// <inheritdoc />
+        public override ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period)
+        {
+            var timer = new FakeTimer(dueTime, period);
+            Created.Add(timer);
+            return timer;
+        }
+
+        /// <summary>A timer that does nothing but remember how it was asked for, and whether it was disposed.</summary>
+        internal sealed class FakeTimer(TimeSpan dueTime, TimeSpan period) : ITimer
+        {
+            public TimeSpan DueTime { get; } = dueTime;
+
+            public TimeSpan Period { get; } = period;
+
+            public bool Disposed { get; private set; }
+
+            public bool Change(TimeSpan due, TimeSpan p) => true;
+
+            public void Dispose() => Disposed = true;
+
+            public ValueTask DisposeAsync()
+            {
+                Disposed = true;
+                return ValueTask.CompletedTask;
+            }
+        }
+    }
+
+    /// <summary>Collects rendered log messages so a "said so once" claim can be asserted.</summary>
+    private sealed class CapturingLogger : ILogger
+    {
+        public List<string> Lines { get; } = [];
+
+        public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
+
+        public bool IsEnabled(LogLevel logLevel) => true;
+
+        public void Log<TState>(
+            LogLevel logLevel,
+            EventId eventId,
+            TState state,
+            Exception? exception,
+            Func<TState, Exception?, string> formatter) => Lines.Add(formatter(state, exception));
+    }
+}

tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs

@@ -421,11 +421,11 @@ public sealed class BoundedApiKeyStoreTests
     /// <summary>Collects <c>otopcua.opcua.apikey_store.bounded</c> measurements by their outcome tag.</summary>
     /// <remarks>
     ///     ⚠️ Filtered by instrument name only, which is normally the metrics-test leak (a listener sees
-    ///     a PARALLEL test's measurements too). It is safe HERE for a checkable reason:
+    ///     a PARALLEL test's measurements too). It is safe HERE for two checkable reasons:
     ///     <see cref="BoundedApiKeyStore"/> is the only thing in the product that writes this instrument,
-    ///     and this class is the only one in the assembly that constructs one — and xUnit never runs two
-    ///     tests of the same class in parallel. Adding a class that exercises the decorator means adding
-    ///     a discriminating tag here.
+    ///     and this assembly declares <c>CollectionBehavior(DisableTestParallelization = true)</c> in
+    ///     <c>AssemblyInfo.cs</c>, so there is no parallel test to leak from. Removing that attribute
+    ///     means adding a discriminating tag here.
     /// </remarks>
     private sealed class MeterRecorder : IDisposable
     {