diff --git a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md index d6f82684..81c1d238 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md @@ -669,3 +669,21 @@ for the orchestrator. - 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. + +### T7 — node-manager expiry tests + xmldoc correction + +- **RED: the three new cases PASSED on first run — as the plan predicted** ("the expiry case may already + PASS … that is the point: it pins the property the timer will rely on before the timer exists"). The + pass already reads the authorizer's `TimeProvider`, so no production change was needed. +- **Falsification run (recorded, since "it passed immediately" proves nothing on its own):** shortening the + clock advance in `An_expired_key_stands_down_a_live_item_with_no_deploy` from 20 min to 5 min (i.e. + short of `ExpiresUtc`) turned it **`[FAIL]`**, then restored. So the assertion tracks the clock rather + than a blanket deny. +- `A_renewed_key_re_attaches_the_item_on_the_next_pass` observes the restore **indirectly and + falsifiably**: the pass skips an already-suppressed item, so standing the item down a second time can + only succeed if it was genuinely re-attached in between. (`NodeHandle.MonitoredNode` is null on a + hand-tracked stub item, so attach/detach is not directly observable at this seam — it is proven over the + wire in T9.) +- Two stale remarks corrected, not one: `OnAuthorityChanged`'s "this is the only trigger" **and** + `IsSubscriptionStillPermitted`'s "a subscription is re-checked at a deployment, never on a timer". +- OpcUaServer.Tests: 493 passed / 1 skipped. diff --git a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json index db16b8fb..b2a67fb8 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json @@ -51,7 +51,7 @@ { "id": "T7", "subject": "Node-manager expiry tests (FakeTimeProvider advanced past ExpiresUtc, no deploy) + correct the ':316-328' only-trigger xmldoc", - "status": "pending", + "status": "done", "blockedBy": [ "T1" ] diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index b12a03bc..8a1c3d1a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -317,12 +317,21 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// Re-authorizes live subscriptions when a deployment installs a new authorization generation. /// /// - /// ⚠️ This is the only trigger, and the pass's memo depends on that. - /// memoizes key revocation and expiry for the span of one - /// pass — safe only while revalidation is deploy-triggered, because a pass is then a momentary - /// event whose verdicts cannot go stale within it. A future periodic trigger must give each tick a - /// fresh memo (it already does — the dictionary is pass-local) and must not be tempted to reuse one - /// across ticks, or an expiry landing between ticks would be served the previous tick's allow. + /// + /// There are TWO triggers (arch-review 03/S19): this one, the deployment axis; and the + /// host's low-frequency AclRevalidationTimer, the clock axis — a key's + /// ExpiresUtc passes with the clock, not with a deployment, so a deploy-only trigger + /// left an expired key streaming to a live subscription indefinitely on a fleet that had been + /// deployed once and left running. + /// + /// + /// ⚠️ The second trigger is safe only because the memo is pass-local. + /// memoizes key revocation and expiry for the span + /// of one pass, which is sound because a pass is a momentary event whose verdicts cannot go + /// stale within it — its dictionary is constructed per call. Hoisting it across ticks, to save + /// the walk, would serve an expiry that landed between two ticks the earlier tick's allow, and + /// re-open exactly the gap the timer closes. + /// /// /// The newly published snapshot; unused — the current state is re-read per item. private void OnAuthorityChanged(DataPlaneAuthoritySnapshot snapshot) => RevalidateMonitoredItems(); @@ -3311,9 +3320,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// /// /// This memo does cover the key expiry check that the authorizer's own cache excludes, but only - /// for the duration of one pass — and a subscription is re-checked at a deployment, never on a - /// timer, so an expiry that lands inside a single pass was already going to be caught by the - /// next deployment rather than by this one. + /// for the duration of one pass. Since arch-review 03/S19 a subscription is re-checked on the + /// host's low-frequency timer as well as at a deployment, so an expiry that lands inside a + /// single pass is caught by the next TICK — one interval later, not one deployment later. What + /// makes that hold is that the memo is constructed per call and never reused across passes. /// /// private bool IsSubscriptionStillPermitted( diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs index 6d74b601..e4d0b926 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs @@ -615,8 +615,156 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable finally { await host.DisposeAsync(); } } + /// + /// A key that expires by the CLOCK stands its live monitored item down, with no deployment + /// anywhere in the test (arch-review 03/S19). + /// + /// + /// + /// The first pass is the falsification control, and it is not optional: without it a build + /// whose pass detached everything it saw — unconditionally, or because the authorizer denied + /// for some unrelated reason — would pass the second half and prove nothing about expiry. + /// + /// + /// DataPlaneAuthorityState.Apply is called exactly once here, in setup. Nothing + /// advances the generation between the two passes, so the only thing that changed is the + /// clock — which is the whole claim. + /// + /// + [Fact] + public async Task An_expired_key_stands_down_a_live_item_with_no_deploy() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var clock = new FakeTimeProvider(new DateTime(2026, 8, 20, 12, 0, 0, DateTimeKind.Utc)); + nm.Authorizer = KeyedSubscribeAuthorizer( + clock, out _, expiresUtc: new DateTime(2026, 8, 20, 12, 10, 0, DateTimeKind.Utc)); + + var item = TrackItem(nm, Variable(nm, "Plant/Modbus/dev1/Speed"), KeyedIdentity()); + + nm.RevalidateMonitoredItems(); + item.Denials.ShouldBe(0, "the key is still valid, so nothing may stand down"); + + clock.Advance(TimeSpan.FromMinutes(20)); + nm.RevalidateMonitoredItems(); + + item.Denials.ShouldBe(1); + } + finally { await host.DisposeAsync(); } + } + + /// + /// A key renewed with a later expiry re-attaches its item on the next pass — the clock axis + /// inherits the deploy axis's reversibility. + /// + /// + /// The restore is asserted by standing the item down a SECOND time: the pass skips an item that is + /// already suppressed, so a second denial can only be queued if the item was genuinely restored + /// in between. That is why this pins the design decision not to close the session — a client that + /// renews its key recovers with no action of its own. + /// + [Fact] + public async Task A_renewed_key_re_attaches_the_item_on_the_next_pass() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var clock = new FakeTimeProvider(new DateTime(2026, 8, 20, 12, 0, 0, DateTimeKind.Utc)); + nm.Authorizer = KeyedSubscribeAuthorizer( + clock, out var state, expiresUtc: new DateTime(2026, 8, 20, 12, 10, 0, DateTimeKind.Utc)); + + var item = TrackItem(nm, Variable(nm, "Plant/Modbus/dev1/Speed"), KeyedIdentity()); + + clock.Advance(TimeSpan.FromMinutes(20)); + nm.RevalidateMonitoredItems(); + item.Denials.ShouldBe(1, "non-vacuity: the item really was stood down before the renewal"); + + // Renew: same key id, a later expiry. + state.Apply( + Cluster, "rev-2", + [Key(new DateTime(2026, 8, 20, 14, 0, 0, DateTimeKind.Utc))], + [GrantOn(NodePermissions.Subscribe)], + Scopes()); + nm.RevalidateMonitoredItems(); + + // Expire it again. A still-suppressed item is skipped, so a second denial proves the restore. + clock.Advance(TimeSpan.FromHours(3)); + nm.RevalidateMonitoredItems(); + + item.Denials.ShouldBe(2); + } + finally { await host.DisposeAsync(); } + } + + /// A clock that moves but does not reach the expiry stands nothing down. + [Fact] + public async Task An_unexpired_key_is_untouched_by_a_clock_only_pass() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var clock = new FakeTimeProvider(new DateTime(2026, 8, 20, 12, 0, 0, DateTimeKind.Utc)); + nm.Authorizer = KeyedSubscribeAuthorizer( + clock, out _, expiresUtc: new DateTime(2026, 8, 20, 13, 0, 0, DateTimeKind.Utc)); + + var item = TrackItem(nm, Variable(nm, "Plant/Modbus/dev1/Speed"), KeyedIdentity()); + + clock.Advance(TimeSpan.FromMinutes(30)); + nm.RevalidateMonitoredItems(); + + item.Denials.ShouldBe(0); + } + finally { await host.DisposeAsync(); } + } + // ───────────────────────────── helpers ───────────────────────────── + /// The public id of the key the clock-driven expiry cases authenticate with. + private const string ExpiringKeyId = "k-expiry"; + + /// + /// A cluster-wide Subscribe grant PLUS a key that expires at , so the + /// verdict moves with the clock rather than with a deployment. + /// + /// The authorizer's clock — the one Evaluate checks expiry against. + /// The authority state, so a test can renew the key without a second helper. + /// When the key stops being usable. + /// The gate, enforcing. + private static DataPlaneAuthorizer KeyedSubscribeAuthorizer( + TimeProvider clock, out DataPlaneAuthorityState state, DateTime expiresUtc) + { + state = new DataPlaneAuthorityState(); + state.Apply(Cluster, "rev-1", [Key(expiresUtc)], [GrantOn(NodePermissions.Subscribe)], Scopes()); + return new DataPlaneAuthorizer( + state, new TriePermissionEvaluator(state.Tries), enabled: true, NullLogger.Instance, clock); + } + + /// The expiring key, at whatever expiry a case needs. + /// When the key stops being usable. + /// The key row. + private static ApiKey Key(DateTime expiresUtc) => new() + { + ApiKeyId = ExpiringKeyId, + Name = ExpiringKeyId, + SecretHash = "hash", + Principal = Principal, + Enabled = true, + CreatedUtc = new DateTime(2026, 8, 20, 0, 0, 0, DateTimeKind.Utc), + ExpiresUtc = expiresUtc, + }; + + /// + /// An identity carrying the key id, which is what makes the per-call expiry re-check apply — an + /// identity with no ApiKeyId skips it entirely. + /// + /// The identity. + private static RoleCarryingUserIdentity KeyedIdentity() => + new(new UserNameIdentityToken { UserName = "apikey" }, [], Principal, ExpiringKeyId); + /// A cluster-wide Subscribe grant — what CreateMonitoredItems resolves to. private static DataPlaneAuthorizer SubscribeAuthorizer(TimeProvider clock) {