From 541a223b9675695c2bc5d2d985f429094f9f4329 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 16 Aug 2026 04:03:32 -0400 Subject: [PATCH] perf(acl): O(n) browse/translate filtering compaction --- .../OtOpcUaNodeManager.cs | 46 ++++-- .../NodeManagerStage6EnforcementTests.cs | 137 ++++++++++++++++++ 2 files changed, 172 insertions(+), 11 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 8c2883d0..3121f84f 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -2943,22 +2943,36 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 base.Browse(context, ref continuationPoint, references); var returned = references.Count - before; + // Forward two-pointer compaction over the appended range, NOT a per-denial RemoveAt. Removing + // from the middle shifts every later element down one slot, so denying a large prefix of a wide + // browse costs O(kept × removed) element moves; copying each survivor down to a write cursor and + // trimming the tail afterwards is one pass. The tail trim removes from the END, where RemoveAt + // moves nothing. var filtered = 0; - for (var ii = references.Count - 1; ii >= before; ii--) + var write = before; + for (var ii = before; ii < references.Count; ii++) { // ExpandedNodeId → NodeId only makes sense for a reference into our own server; a reference // to another server's node is left alone, as this manager has no grants that could speak to it. var target = references[ii].NodeId; - if (target is null || target.IsAbsolute) continue; + var keep = target is null + || target.IsAbsolute + || ExpandedNodeId.ToNodeId(target, Server.NamespaceUris) is not { } targetId + || !OwnsNamespace(targetId) + || Permits(identity, OpcUaOperation.Browse, targetId); - var targetId = ExpandedNodeId.ToNodeId(target, Server.NamespaceUris); - if (targetId is null || !OwnsNamespace(targetId)) continue; - if (Permits(identity, OpcUaOperation.Browse, targetId)) continue; + if (!keep) + { + filtered++; + continue; + } - references.RemoveAt(ii); - filtered++; + if (write != ii) references[write] = references[ii]; + write++; } + for (var ii = references.Count - 1; ii >= write; ii--) references.RemoveAt(ii); + LogAclFiltering("Browse", identity, filtered, returned); } @@ -2995,14 +3009,24 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 var before = targetIds.Count; base.TranslateBrowsePath(context, sourceHandle, relativePath, targetIds, unresolvedTargetIds); - for (var ii = targetIds.Count - 1; ii >= before; ii--) + // Forward two-pointer compaction over the appended range — same reasoning as Browse above: a + // mid-list RemoveAt per denial is O(kept × removed) element moves, one compacting pass plus a + // tail trim is O(n). + var write = before; + for (var ii = before; ii < targetIds.Count; ii++) { var targetId = ExpandedNodeId.ToNodeId(targetIds[ii], Server.NamespaceUris); - if (targetId is null || !OwnsNamespace(targetId)) continue; - if (Permits(identity, OpcUaOperation.Browse, targetId)) continue; + var keep = targetId is null + || !OwnsNamespace(targetId) + || Permits(identity, OpcUaOperation.Browse, targetId); - targetIds.RemoveAt(ii); + if (!keep) continue; + + if (write != ii) targetIds[write] = targetIds[ii]; + write++; } + + for (var ii = targetIds.Count - 1; ii >= write; ii--) targetIds.RemoveAt(ii); } /// 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 595c677f..f60d7dd2 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs @@ -268,6 +268,85 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable finally { await host.DisposeAsync(); } } + /// + /// Browse filtering removes the denied references from the page it just appended in place, + /// so the survivors must come back exactly once each, in the order the base browser produced them, + /// and an earlier BrowseNext page already sitting in the list must be left untouched. + /// + /// + /// + /// The grant pattern is deliberately non-contiguous and denies the first child: the + /// filtering walk compacts survivors down over the gaps, so a denied prefix is what actually + /// exercises the copy-down path. A pattern granting a contiguous head would pass against a + /// compaction that never moves anything. + /// + /// + /// The expectation is taken from a baseline browse under a blanket grant rather than + /// hand-written, so the assertion is about what filtering did to the base browser's + /// page — not about the SDK's own reference ordering, which is not this manager's contract. + /// + /// + /// The pre-seeded sentinel reference is the range-boundary check: it belongs to a previous + /// page, sits below the filtering range, and names a node the session is not granted — + /// so an off-by-one that walks from index 0 rather than from the appended range removes it. + /// + /// + [Fact] + public async Task Browse_filtering_preserves_survivor_order() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + const string device = "Plant/Modbus/dev1"; + nm.EnsureFolder(device, parentNodeId: null, displayName: "dev1", AddressSpaceRealm.Raw); + + var children = new List(); + for (var ii = 1; ii <= 6; ii++) + { + var rawPath = $"{device}/T{ii}"; + nm.EnsureVariable(rawPath, parentFolderNodeId: device, displayName: rawPath, dataType: "Float", + writable: false, historianTagname: null, realm: AddressSpaceRealm.Raw); + children.Add(nm.TryGetVariable(rawPath, AddressSpaceRealm.Raw)!.NodeId); + } + + var deviceId = new NodeId(device, children[0].NamespaceIndex); + + // Baseline: everything the base browser returns for this folder, unfiltered. + nm.Authorizer = Authorizer(enabled: true, GrantOn(NodePermissions.Browse)); + var baseline = Browse(nm, Identity(), deviceId); + baseline.Count.ShouldBeGreaterThanOrEqualTo(6); + + // Now grant Browse on the 1st, 3rd and 5th child only. + var granted = new[] { children[0], children[2], children[4] }; + nm.Authorizer = TagBrowseAuthorizer($"{device}/T1", $"{device}/T3", $"{device}/T5"); + + var sentinel = new ReferenceDescription + { + NodeId = children[1], // an explicitly DENIED node, from an earlier page + DisplayName = "previous-page", + }; + var references = Browse(nm, Identity(), deviceId, sentinel); + + var denied = new[] { children[1], children[3], children[5] }; + var expected = new[] { (ExpandedNodeId)children[1] } + .Concat(baseline + .Select(r => r.NodeId) + .Where(id => !denied.Any(d => (ExpandedNodeId)d == id))) + .ToList(); + + // Non-vacuity: three references really were dropped (and the sentinel really was added). + expected.Count.ShouldBe(baseline.Count - 2); + + references.Select(r => r.NodeId).ShouldBe(expected); + + // Belt and braces: each survivor appears exactly once, and every granted child survived. + foreach (var id in granted) + references.Count(r => r.NodeId == (ExpandedNodeId)id).ShouldBe(1, $"{id} survives once"); + } + finally { await host.DisposeAsync(); } + } + // ───────────────────────────── helpers ───────────────────────────── private static NodeId Variable(OtOpcUaNodeManager nm, string rawPath) @@ -297,6 +376,64 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable return (values, errors); } + /// + /// Drives the manager's Browse override directly, optionally with references already in the + /// list to stand in for an earlier BrowseNext page. + /// + private static List Browse( + OtOpcUaNodeManager nm, + IUserIdentity? identity, + NodeId source, + params ReferenceDescription[] previousPage) + { + var context = new OperationContext( + new RequestHeader(), secureChannelContext: null, RequestType.Browse, identity); + + var continuationPoint = new ContinuationPoint + { + NodeToBrowse = nm.GetManagerHandle(source), + BrowseDirection = BrowseDirection.Forward, + ReferenceTypeId = null, + IncludeSubtypes = true, + NodeClassMask = 0, + ResultMask = BrowseResultMask.All, + MaxResultsToReturn = 0, + }; + + var references = new List(previousPage); + nm.Browse(context, ref continuationPoint, references); + continuationPoint?.Dispose(); + return references; + } + + /// A Browse-only grant per named raw tag path, each in its own Tag-kind scope. + private static DataPlaneAuthorizer TagBrowseAuthorizer(params string[] rawPaths) + { + var grants = rawPaths + .Select((_, ii) => new NodeAcl + { + NodeAclId = $"acl-tag-{ii}", + ClusterId = Cluster, + Principal = Principal, + ScopeKind = NodeAclScopeKind.Tag, + ScopeId = $"tag-{ii}", + PermissionFlags = NodePermissions.Browse, + }) + .ToList(); + + var scopePaths = rawPaths + .Select((path, ii) => (Ref: new AclScopeRef(NodeAclScopeKind.Tag, $"tag-{ii}"), Path: path)) + .ToDictionary( + e => e.Ref, + e => (IReadOnlyList) + [new NodeAclPath([NodeAclRealms.Raw, .. e.Path.Split('/')])]); + + var state = new DataPlaneAuthorityState(); + state.Apply(Cluster, "rev-1", [], grants, scopePaths); + return new DataPlaneAuthorizer( + state, new TriePermissionEvaluator(state.Tries), enabled: true, NullLogger.Instance); + } + private static DataPlaneAuthorizer Authorizer( bool enabled, NodeAcl grant,