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

Folder-event verdict invalidation is untested and its orphan test is non-discriminating since #855 #878

New Issue
Closed
opened 2026-11-12 08:54:56 -05:00 by dohertj2 · 1 comment
dohertj2 commented 2026-11-12 08:54:56 -05:00
Owner
Copy Link
Copy Source

Found while closing #838 item 1.

FolderEventVerdictInvalidationTests.An_orphaned_folder_item_does_not_keep_a_stale_allow_across_a_deploy_and_a_transfer no longer discriminates. Mutation check on master e5a03e3c: with InvalidateFolderEventVerdicts skipping every node (neither the private-SDK route nor the per-session/detach-reattach fallback), the test still passes. Since #855 a condition's own events are decided per item at report time, uncached, so the SDK's memoized verdict only matters for events that travel the SDK per-notifier channel. A condition's method audits are the documented candidate, but a probe (auditing on, encrypted channel, AddComment by a cluster-granted caller, observer on an equipment folder) showed none reach a folder item at all: the SDK's type-level ReceiveEvents check denies audit types for ordinary sessions (see AclServerObjectModelChangeTests). So today InvalidateFolderEventVerdicts, its reflection and its fallback guard a path no producible event exercises.

Consequence: the fallback route (detach/re-attach of session-less items, which can lose an event) is untested, and the orphan test gives false assurance for the invalidation.

Action (pick one):

  1. Find an event type that really rides the SDK channel to a folder item with a cached verdict, and rebuild the orphan test on it, with a fallback leg (a test seam that nulls InvalidateNodePermissionCaches per instance is a ~5-line change); or
  2. If nothing can reach it, delete the invalidation + reflection + fallback and the guard test, keeping the per-event decision (removes the private-SDK dependency that #838 item 1 worries about).

Option 2 is the better fit if option 1's search comes up empty, because it removes the SDK-bump hazard rather than guarding it. Refs #838, #826, #855.

Found while closing #838 item 1. `FolderEventVerdictInvalidationTests.An_orphaned_folder_item_does_not_keep_a_stale_allow_across_a_deploy_and_a_transfer` no longer discriminates. Mutation check on master `e5a03e3c`: with `InvalidateFolderEventVerdicts` skipping every node (neither the private-SDK route nor the per-session/detach-reattach fallback), the test still passes. Since #855 a condition's own events are decided per item at report time, uncached, so the SDK's memoized verdict only matters for events that travel the SDK per-notifier channel. A condition's method audits are the documented candidate, but a probe (auditing on, encrypted channel, AddComment by a cluster-granted caller, observer on an equipment folder) showed none reach a folder item at all: the SDK's type-level `ReceiveEvents` check denies audit types for ordinary sessions (see `AclServerObjectModelChangeTests`). So today `InvalidateFolderEventVerdicts`, its reflection and its fallback guard a path no producible event exercises. Consequence: the fallback route (detach/re-attach of session-less items, which can lose an event) is untested, and the orphan test gives false assurance for the invalidation. Action (pick one): 1. Find an event type that really rides the SDK channel to a folder item with a cached verdict, and rebuild the orphan test on it, with a fallback leg (a test seam that nulls `InvalidateNodePermissionCaches` per instance is a ~5-line change); or 2. If nothing can reach it, delete the invalidation + reflection + fallback and the guard test, keeping the per-event decision (removes the private-SDK dependency that #838 item 1 worries about). Option 2 is the better fit if option 1's search comes up empty, because it removes the SDK-bump hazard rather than guarding it. Refs #838, #826, #855.
dohertj2 added the priority:p3 label 2026-11-12 08:54:56 -05:00
dohertj2 commented 2026-11-12 11:52:22 -05:00
Author
Owner
Copy Link
Copy Source

Decision: option 2. No event the product raises reaches a folder item through the SDK's cached per-item verdict with an allow that depends on a grant. The invalidation, the reflection into MonitoredNode2.InvalidatePermissionCaches, the detach/re-attach fallback, the FolderEventVerdictInvalidations counter and the guard test are deleted. Under enforcement, the SDK channel's verdict for items on our nodes is now a flat deny, so nothing it caches depends on a grant.

Search evidence (SDK 2.0.1, decompiled with ilspycmd: Opc.Ua.Server, Opc.Ua.Core.Types)

Path Uses the cached verdict? Evidence
Where the cache is read n/a MonitoredNode2.GetOrAddEventPermissionAsync is the only reader of m_eventPermissionCache. It is called only from ProcessEventForItemAsync, which is fed by the OnReportEventAsync channel. In 2.0.1 the only caller of the re-implemented INodeManager3.ValidateEventRolePermissions is that method, through AsyncNodeManagerAdapter. The base CustomNodeManager2.ConditionRefresh (line 5378) calls the non-virtual base method, and AsyncCustomNodeManager.ConditionRefresh does not cache.
Condition events (native and scripted; every driver alarm arrives through the address-space sink) No Every one goes through ReportConditionEvent → ReportEventWithoutBlocking → DeliverConditionEventAsync. That code serves our notifiers itself (DeliverToNotifierItems, uncached, #855) and never writes to the channel. grep ReportEvent over src/ turns up no other caller.
SDK's own condition state-change reports from methods (ReportStateChange) No ConditionState.ReportStateChange is gated on AutoReportStateChanges, which we set to false (OtOpcUaNodeManager.cs ~2802).
Condition method audits (Acknowledge/Confirm/AddComment/Enable/Shelving/Silence/Suppression/OutOfService/Reset, raised via NodeState.ReportEvent on the condition) Yes, but unreachable They do travel the channel to folder items. ProcessEventForItemAsync drops them unless Server.Auditing is on (ServerConfiguration.AuditingEnabled is never set; the only route would be an OpcUa:ApplicationConfigPath XML file) and the session uses SignAndEncrypt or https. After that, the SDK's type-level ReceiveEvents check decides. Probe of ValidateRolePermissions(ctx, type, ReceiveEvents) for all nine well-known roles: AuditEventType, AuditConditionEventType, …Comment, …Acknowledge, …Confirm, …Enable, …Shelving, AuditWriteUpdateEventType are DENY for every role except SecurityAdmin. BaseEventType, AlarmConditionType, GeneralModelChangeEventType and SystemStatusChangeEventType are OK for all. Nothing grants SecurityAdmin: the default RoleManager maps identities only to Anonymous, AuthenticatedUser and TrustedApplication. OtOpcUa does not use the DI ConfigureRoles path or a custom role manager, RoleCarryingUserIdentity adds no SDK roles, and the RoleSet AddIdentity methods themselves require SecurityAdmin (RoleAuthorizationGate.CheckAdmin).
Non-condition events on folders No producer Nothing in src/ calls ReportEvent on a folder. Model-change and audit-write events go through ReportToServerObject, which uses the core Server object and not our MonitoredNodes.
Events through the Server object No The root-notifier forward is dropped (OnReportEvent override is empty). Server-object items are decided per event in OnConditionReportEvent / ReportToServerObject. The Server node's MonitoredNode2 belongs to the core node manager, and our invalidation never touched it.
ConditionRefresh replays No Our ConditionRefresh override → RefreshDirectItems decides per item, uncached (IsNotifierEventVisibleTo).
Data-change items on folders (m_permissionCache) Not ACL That verdict is ValidateRolePermissionsAsync, the SDK role check, which no deploy changes. Invalidation was skipped with enforcement off anyway, and never ran on variables.

Mutation on the pre-change tip: with InvalidateFolderEventVerdicts returning immediately, 2 of 347 OpcUaServer integration tests failed. One was its own counter test. The other was the plain-event test, which reports a hand-made BaseEvent on a folder and then flips enforcement off→on. That is a test-only producer crossing a mode flip.

What changed

  • INodeManager3.ValidateEventRolePermissions: under enforcement, items on our nodes get BadUserAccessDenied for everything on the channel. In effect, condition method audits are now withheld from folder items under enforcement; this is stricter than Part 3, which allows it. They still reach Server-object items, decided per event. With the flag off, behaviour is unchanged.
  • Residual, documented on the method and in docs/security.md: a verdict cached while enforcement was off survives the switch to on. The switch happens once, when the host wires the authorizer just after the server starts listening. The only allow it could carry is a method audit to a SecurityAdmin session.
  • Tests (FolderEventVerdictInvalidationTests → FolderEventVerdictFreshnessTests):
    • A_revoked_grant_stops_condition_events_to_that_session_while_a_granted_session_keeps_them (new). The positive control is a still-granted session on the same folder and the Server object. The folder leg fails only when both the #562 stand-down and the per-event grant check are removed. The Server-object leg fails when OnConditionReportEvent skips its grant check.
    • A_condition_method_audit_reaches_a_folder_item_only_with_enforcement_off(true/false) (new). SecurityAdmin is mapped in the test only. Positive controls: the Server-object item gets the audit, and with the flag off the folder item gets it, so the channel really carries it. It fails if the old IsNotifierEventVisibleTo decision or no decision is put back on the channel.
    • The orphan test is kept. It fails when the live-notifier check in DeliverToNotifierItems is skipped, so it pins the per-item decision.
    • The plain-event test now runs each mode on a fresh server (no flip). It fails if the channel stops denying.
    • Removed: Folder_verdicts_are_not_invalidated_while_enforcement_is_off and The_sdks_node_wide_permission_cache_invalidation_is_still_resolvable.
  • Docs: docs/security.md (folder event ACL + "Which permissions an event on a folder follows" + #855 note), .claude/rules/data-plane-auth.md.

Commit: fa811feb on master (rebased over #886's 3a312ffd, which merged without conflicts). Suites on that tip: OpcUaServer.Tests 1275 passed / 1 skipped / 0 failed; OpcUaServer.IntegrationTests 320 passed / 35 skipped / 0 failed.

Closing. This also retires #838 item 1 (the private-SDK reflection watch item): there is no private-SDK dependency left on this path.

**Decision: option 2.** No event the product raises reaches a folder item through the SDK's cached per-item verdict with an allow that depends on a grant. The invalidation, the reflection into `MonitoredNode2.InvalidatePermissionCaches`, the detach/re-attach fallback, the `FolderEventVerdictInvalidations` counter and the guard test are deleted. Under enforcement, the SDK channel's verdict for items on our nodes is now a flat deny, so nothing it caches depends on a grant. ### Search evidence (SDK 2.0.1, decompiled with ilspycmd: Opc.Ua.Server, Opc.Ua.Core.Types) | Path | Uses the cached verdict? | Evidence | |---|---|---| | Where the cache is read | n/a | `MonitoredNode2.GetOrAddEventPermissionAsync` is the only reader of `m_eventPermissionCache`. It is called only from `ProcessEventForItemAsync`, which is fed by the `OnReportEventAsync` channel. In 2.0.1 the only caller of the re-implemented `INodeManager3.ValidateEventRolePermissions` is that method, through `AsyncNodeManagerAdapter`. The base `CustomNodeManager2.ConditionRefresh` (line 5378) calls the non-virtual base method, and `AsyncCustomNodeManager.ConditionRefresh` does not cache. | | Condition events (native and scripted; every driver alarm arrives through the address-space sink) | No | Every one goes through `ReportConditionEvent` → `ReportEventWithoutBlocking` → `DeliverConditionEventAsync`. That code serves our notifiers itself (`DeliverToNotifierItems`, uncached, #855) and never writes to the channel. `grep ReportEvent` over `src/` turns up no other caller. | | SDK's own condition state-change reports from methods (`ReportStateChange`) | No | `ConditionState.ReportStateChange` is gated on `AutoReportStateChanges`, which we set to `false` (`OtOpcUaNodeManager.cs` ~2802). | | Condition method **audits** (Acknowledge/Confirm/AddComment/Enable/Shelving/Silence/Suppression/OutOfService/Reset, raised via `NodeState.ReportEvent` on the condition) | **Yes, but unreachable** | They do travel the channel to folder items. `ProcessEventForItemAsync` drops them unless `Server.Auditing` is on (`ServerConfiguration.AuditingEnabled` is never set; the only route would be an `OpcUa:ApplicationConfigPath` XML file) and the session uses SignAndEncrypt or https. After that, the SDK's type-level `ReceiveEvents` check decides. **Probe** of `ValidateRolePermissions(ctx, type, ReceiveEvents)` for all nine well-known roles: `AuditEventType`, `AuditConditionEventType`, `…Comment`, `…Acknowledge`, `…Confirm`, `…Enable`, `…Shelving`, `AuditWriteUpdateEventType` are **DENY for every role except SecurityAdmin**. BaseEventType, AlarmConditionType, GeneralModelChangeEventType and SystemStatusChangeEventType are OK for all. Nothing grants `SecurityAdmin`: the default `RoleManager` maps identities only to Anonymous, AuthenticatedUser and TrustedApplication. OtOpcUa does not use the DI `ConfigureRoles` path or a custom role manager, `RoleCarryingUserIdentity` adds no SDK roles, and the RoleSet `AddIdentity` methods themselves require `SecurityAdmin` (`RoleAuthorizationGate.CheckAdmin`). | | Non-condition events on folders | No producer | Nothing in `src/` calls `ReportEvent` on a folder. Model-change and audit-write events go through `ReportToServerObject`, which uses the core Server object and not our `MonitoredNodes`. | | Events through the Server object | No | The root-notifier forward is dropped (`OnReportEvent` override is empty). Server-object items are decided per event in `OnConditionReportEvent` / `ReportToServerObject`. The Server node's `MonitoredNode2` belongs to the core node manager, and our invalidation never touched it. | | ConditionRefresh replays | No | Our `ConditionRefresh` override → `RefreshDirectItems` decides per item, uncached (`IsNotifierEventVisibleTo`). | | Data-change items on folders (`m_permissionCache`) | Not ACL | That verdict is `ValidateRolePermissionsAsync`, the SDK role check, which no deploy changes. Invalidation was skipped with enforcement off anyway, and never ran on variables. | **Mutation on the pre-change tip:** with `InvalidateFolderEventVerdicts` returning immediately, 2 of 347 OpcUaServer integration tests failed. One was its own counter test. The other was the plain-event test, which reports a hand-made `BaseEvent` on a folder and then flips enforcement off→on. That is a test-only producer crossing a mode flip. ### What changed - `INodeManager3.ValidateEventRolePermissions`: under enforcement, items on our nodes get `BadUserAccessDenied` for everything on the channel. In effect, condition method audits are now withheld from **folder** items under enforcement; this is stricter than Part 3, which allows it. They still reach Server-object items, decided per event. With the flag off, behaviour is unchanged. - Residual, documented on the method and in `docs/security.md`: a verdict cached while enforcement was **off** survives the switch to on. The switch happens once, when the host wires the authorizer just after the server starts listening. The only allow it could carry is a method audit to a `SecurityAdmin` session. - Tests (`FolderEventVerdictInvalidationTests` → `FolderEventVerdictFreshnessTests`): - `A_revoked_grant_stops_condition_events_to_that_session_while_a_granted_session_keeps_them` (new). The positive control is a still-granted session on the same folder and the Server object. The folder leg fails only when both the #562 stand-down and the per-event grant check are removed. The Server-object leg fails when `OnConditionReportEvent` skips its grant check. - `A_condition_method_audit_reaches_a_folder_item_only_with_enforcement_off(true/false)` (new). `SecurityAdmin` is mapped in the test only. Positive controls: the Server-object item gets the audit, and with the flag off the folder item gets it, so the channel really carries it. It fails if the old `IsNotifierEventVisibleTo` decision or no decision is put back on the channel. - The orphan test is kept. It fails when the live-notifier check in `DeliverToNotifierItems` is skipped, so it pins the per-item decision. - The plain-event test now runs each mode on a fresh server (no flip). It fails if the channel stops denying. - Removed: `Folder_verdicts_are_not_invalidated_while_enforcement_is_off` and `The_sdks_node_wide_permission_cache_invalidation_is_still_resolvable`. - Docs: `docs/security.md` (folder event ACL + "Which permissions an event on a folder follows" + #855 note), `.claude/rules/data-plane-auth.md`. **Commit:** `fa811feb` on master (rebased over #886's `3a312ffd`, which merged without conflicts). **Suites on that tip:** OpcUaServer.Tests 1275 passed / 1 skipped / 0 failed; OpcUaServer.IntegrationTests 320 passed / 35 skipped / 0 failed. Closing. This also retires #838 item 1 (the private-SDK reflection watch item): there is no private-SDK dependency left on this path.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
auto-managed
queue management
 blocked
Blocked by dependency or external hold
 cross-driver
queue management
 cross-repo
Needs coordinated change in another family repo (wire contract etc.)
 driver/abcip
queue management
 driver/ablegacy
queue management
 driver/focas
queue management
 driver/mqtt
MQTT / Sparkplug B broker-subscribe driver
 driver/opcuaclient
queue management
 driver/s7
queue management
 driver/twincat
queue management
 migrated
Migrated from a markdown backlog (2026-11 standardization)
 phase/1
queue management
 phase/2
queue management
 phase/3
queue management
 phase/4
queue management
 phase/5
queue management
 phase/6
queue management
 priority:p0
Must-have / blocking
 priority:p1
High priority
 priority:p2
Normal priority
 priority:p3
Low priority / nice to have
 queue/blocked
queue management
 queue/done
queue management
 queue/failed
queue management
 queue/in-progress
queue management
 queue/queued
queue management
 type:bug
Defect in shipped behaviour
 type:debt
Technical debt / hygiene
 type:deploy-gate
Rollout/deploy/migration step not yet done on an environment
 type:docs
Documentation work
 type:feature
Feature implementation
 type:followup
Follow-up deferred from a plan, PR or gate
 type:infra
Infrastructure and scaffolding
 type:review-finding
Unfixed finding from an architecture/code review
 type:test
Test work
No Label priority:p3
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
dohertj2 (dohertj2)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: dohertj2/lmxopcua#878
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?