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

fix(acl): keep the SDK's audit guard and EventsMonitored bookkeeping on Server-object delivery (#586)

Browse Source 
Review of 75e559f6:
- OnConditionReportEvent now mirrors MonitoredNode2's AuditEventState guard (only while Server.Auditing,
  over SignAndEncrypt / https-uabinary). Conditions report their Part 9 method audit events through
  themselves, so the first cut queued AuditConditionAcknowledge (i=8944) etc. to every i=2253 item with
  auditing off -- the removed folder->Server forward used to drop them.
- ReconcileAllEventsMonitoring keeps one AreEventsMonitored unit on every materialised condition while any
  i=2253 item exists (called from SubscribeToAllEvents, materialise, removal, rebuild), so
  ConditionState.EventsMonitored() is true on a server whose only subscribers are Server-object items.
  A protected SubscribeToEvents override no-ops all-events items, which stops the SDK's AddRootNotifier
  (unsubscribe: true bug) taking that unit back from conditions rebuilt under a live item.

Flag-off over-the-wire coverage: Acknowledge/AddComment with and without a folder co-subscriber (no audit
event, transitions once each), EventsMonitored on/after-rebuild/off, rebuild survival with one copy per
event, native + scripted conditions once each, refresh once per condition with RefreshStart/End; plus an
ACL-on native-vs-scripted notifier-folder decision. Falsified: removing the audit guard reds both audit
cases; removing the reconcile reds the monitored test.

Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
This commit is contained in:
Joseph Doherty
2026-10-08 00:05:12 -04:00
parent 75e559f6d8
commit fe92ff4c34
4 changed files with 460 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs
+133 -4
View File
@@ -350,6 +350,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
_liveItems.Clear();
_eventItems.Clear();
_allEventsItems.Clear();
lock (Lock) _allEventsMonitoredConditions.Clear();
}
base.Dispose(disposing);
@@ -1362,6 +1363,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
AddPredefinedNode(SystemContext, alarm);
_alarmConditions[conditionKey] = alarm;
ReconcileAllEventsMonitoring();
// H6a: record native (driver-fed) conditions so a later task can route their inbound
// Acknowledge to the driver rather than the scripted engine.
if (isNative) _nativeAlarmNodeIds.Add(conditionKey);
@@ -2743,6 +2745,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
// Phase C: drop the event-history source registrations alongside the notifier folders
// they map; re-materialised alarms re-register them (with the HistoryRead bit) on re-promotion.
_eventNotifierSources.Clear();
ReconcileAllEventsMonitoring();
}
}
@@ -2832,6 +2835,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
condition.Parent?.RemoveChild(condition);
PredefinedNodes?.Remove(condition.NodeId);
_nativeAlarmNodeIds.Remove(key);
ReconcileAllEventsMonitoring();
e = BuildNodesRemovedModelChange(alarmNodeId, realm);
}
ReportModelChangeOutsideLock(e, alarmNodeId);
@@ -2881,6 +2885,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
}
_nativeAlarmNodeIds.Remove(id);
}
ReconcileAllEventsMonitoring();
// v3 Batch 4 WP4: a native alarm condition lives at its raw tag (Raw realm) and SURVIVES this
// UNS-scoped subtree removal, but it may hold an in-scope equipment folder as an extra
@@ -3428,11 +3433,94 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
public override ServiceResult SubscribeToAllEvents(
OperationContext context, uint subscriptionId, IEventMonitoredItem monitoredItem, bool unsubscribe)
{
if (unsubscribe) _allEventsItems.TryRemove(monitoredItem, out _);
else _allEventsItems[monitoredItem] = 0;
lock (Lock)
{
if (unsubscribe) _allEventsItems.TryRemove(monitoredItem, out _);
else _allEventsItems[monitoredItem] = 0;
ReconcileAllEventsMonitoring();
}
return ServiceResult.Good;
}
/// <inheritdoc />
/// <remarks>
/// <para>
/// A Server-object (all-events) item is never attached to a notifier of ours (Gitea #586), so a
/// subscribe or unsubscribe of one against a notifier is a no-op here. The only SDK caller that
/// still reaches this with such an item is <c>AddRootNotifier</c>, which — an SDK bug — "subscribes"
/// every existing all-events item to a new root notifier with <c>unsubscribe: true</c>. Passed to the
/// base, that lowers <see cref="NodeState.AreEventsMonitored"/> on the new notifier, its children and
/// its forward notifiers — taking back, from every condition materialised under a live item, the
/// unit <see cref="ReconcileAllEventsMonitoring"/> just gave it (and, before #586, a folder
/// subscriber's unit too). Caught by the rebuild leg of the monitored-bookkeeping test.
/// </para>
/// <para>
/// Direct notifier items, including the #562 revalidation pass's attach and detach, go to the base
/// unchanged.
/// </para>
/// </remarks>
protected override ServiceResult SubscribeToEvents(
ServerSystemContext context, NodeState source, IEventMonitoredItem monitoredItem, bool unsubscribe) =>
monitoredItem.MonitoringAllEvents
? ServiceResult.Good
: base.SubscribeToEvents(context, source, monitoredItem, unsubscribe);
/// <summary>
/// The conditions currently carrying this manager's one "a Server-object item is listening" unit of
/// <see cref="NodeState.AreEventsMonitored"/> (Gitea #586 review).
/// </summary>
/// <remarks>Mutated only by <see cref="ReconcileAllEventsMonitoring"/>, under <c>Lock</c>.</remarks>
private readonly HashSet<AlarmConditionState> _allEventsMonitoredConditions =
new(System.Collections.Generic.ReferenceEqualityComparer.Instance);
/// <summary>
/// Keeps <see cref="NodeState.AreEventsMonitored"/> raised on every materialised condition while at
/// least one Server-object item exists, and lowered once none does. Call under <c>Lock</c>.
/// </summary>
/// <remarks>
/// <para>
/// The SDK base attached an all-events item to every root notifier through its own
/// <c>SubscribeToEvents</c>, which also calls <c>SetAreEventsMonitored(true, includeChildren)</c>
/// on the notifier — a counter that propagates to every condition beneath or wired to it. Not
/// attaching the items lost that bookkeeping, and a condition reads it: <c>ConditionState</c>
/// reports its own method-driven state changes (and their audit events) only while
/// <c>EventsMonitored()</c> is true. A server whose only subscribers are Server-object items — the
/// common machine-consumer shape — would otherwise report none of them.
/// </para>
/// <para>
/// Held per <b>condition</b>, exactly one unit each, rather than per root notifier per item as the
/// base did. The counter is shared with the folder subscribers' own units, so it must balance
/// exactly; a notifier-level unit would also reach a condition materialised under an already-
/// raised folder never, or be taken back from one it never reached. This reconciles the desired
/// set (every condition, while any item exists) against what it has raised, so materialise,
/// remove, rebuild and subscribe all converge through one place.
/// </para>
/// </remarks>
private void ReconcileAllEventsMonitoring()
{
// The protected SubscribeToEvents override is what stops the SDK taking these units back.
lock (Lock)
{
var desired = _allEventsItems.IsEmpty
? new HashSet<AlarmConditionState>(System.Collections.Generic.ReferenceEqualityComparer.Instance)
: new HashSet<AlarmConditionState>(
_alarmConditions.Values, System.Collections.Generic.ReferenceEqualityComparer.Instance);
foreach (var condition in _allEventsMonitoredConditions.Where(c => !desired.Contains(c)).ToList())
{
condition.SetAreEventsMonitored(SystemContext, false, includeChildren: true);
_allEventsMonitoredConditions.Remove(condition);
}
foreach (var condition in desired)
{
if (_allEventsMonitoredConditions.Add(condition))
condition.SetAreEventsMonitored(SystemContext, true, includeChildren: true);
}
}
}
/// <inheritdoc />
/// <remarks>
/// <para>
@@ -3484,6 +3572,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
foreach (var item in _allEventsItems.Keys)
{
if (e is AuditEventState && !IsAuditDeliverableTo(item)) continue;
if (scopes is not null && !IsConditionVisibleTo(item, scopes)) continue;
if (ServiceResult.IsBad(ValidateEventRolePermissions(item, e))) continue;
if (reportingSession is not null
@@ -3497,6 +3586,28 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
}
}
/// <summary>
/// The SDK's audit-event guard from <c>MonitoredNode2.OnReportEvent</c>, reproduced exactly: an audit
/// event reaches an item only while the server is auditing, and only over a SignAndEncrypt or
/// https-binary endpoint.
/// </summary>
/// <param name="item">The Server-object event item.</param>
/// <returns><see langword="true"/> when the audit event may be queued to the item.</returns>
/// <remarks>
/// A condition reports its Part 9 method audit events (<c>AuditConditionAcknowledgeEventType</c>,
/// Comment, Enable, Shelving…) through its own <c>ReportEvent</c>, so they pass through
/// <see cref="OnConditionReportEvent"/>. Before #586 the Server-object copy took the root-notifier
/// forward, where <c>ServerInternalData.ReportEvent</c> dropped every audit event unless auditing was
/// on; without this guard they reached every Server-object item regardless (caught in review, flag off).
/// </remarks>
private bool IsAuditDeliverableTo(IEventMonitoredItem item)
{
if (!Server.Auditing) return false;
var endpoint = item.Session?.EndpointDescription;
return endpoint?.SecurityMode == MessageSecurityMode.SignAndEncrypt
|| endpoint?.TransportProfileUri == Profiles.HttpsBinaryTransport;
}
/// <summary>
/// The nodes whose grant admits a condition to a session: the condition itself, every root-notifier
/// folder its events bubble up through, and every equipment folder wired as its extra notifier.
@@ -3571,8 +3682,9 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
/// client connecting after a transition gets the current alarm picture — and the SDK base replays
/// every condition under every root notifier to it after only its own role-permission check. With
/// enforcement on, those items are refreshed here instead, through the same
/// <see cref="IsConditionVisibleTo"/> decision live delivery uses (Gitea #586), and each condition
/// is replayed once rather than once per notifier it is wired to.
/// <see cref="IsConditionVisibleTo"/> decision live delivery uses (Gitea #586); an event that names no
/// condition is withheld. With it off, the base runs unchanged (its per-notifier repeats of one
/// condition share an EventId, which the item's queue already collapses to one copy).
/// </para>
/// <para>
/// Items on one of this manager's own notifiers keep the base behaviour: a stood-down item is off
@@ -3633,6 +3745,23 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
_ => null,
};
/// <summary>
/// Whether the condition at <paramref name="conditionNodeId"/> believes it is being listened to
/// (<c>ConditionState.EventsMonitored()</c>) — the gate on the SDK reporting its method-driven state
/// changes. Test observability for the Gitea #586 review.
/// </summary>
/// <param name="conditionNodeId">The condition's NodeId identifier.</param>
/// <param name="realm">The condition's realm.</param>
/// <returns><see langword="true"/> when the condition exists and reports itself monitored.</returns>
internal bool ConditionEventsMonitored(string conditionNodeId, AddressSpaceRealm realm)
{
lock (Lock)
{
return _alarmConditions.TryGetValue(MapKey(realm, conditionNodeId), out var condition)
&& condition.EventsMonitored();
}
}
/// <summary>
/// How many event items are attached to the event fan-out of nodes this manager owns. Test
/// observability for Gitea #562: an item resurrected onto a notifier after its client deleted it is