perf(acl): O(n) browse/translate filtering compaction

2026-08-16 04:03:32 -04:00
parent 4b6a93bb78
commit 541a223b96
2 changed files with 172 additions and 11 deletions
@@ -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);
    }

    /// <inheritdoc />
@@ -268,6 +268,85 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable
        finally { await host.DisposeAsync(); }
    }

+    /// <summary>
+    ///     Browse filtering removes the denied references from the page it just appended <b>in place</b>,
+    ///     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.
+    /// </summary>
+    /// <remarks>
+    ///     <para>
+    ///         The grant pattern is deliberately non-contiguous and denies the <b>first</b> 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.
+    ///     </para>
+    ///     <para>
+    ///         The expectation is taken from a baseline browse under a blanket grant rather than
+    ///         hand-written, so the assertion is about what filtering <em>did</em> to the base browser's
+    ///         page — not about the SDK's own reference ordering, which is not this manager's contract.
+    ///     </para>
+    ///     <para>
+    ///         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 <b>not</b> granted —
+    ///         so an off-by-one that walks from index 0 rather than from the appended range removes it.
+    ///     </para>
+    /// </remarks>
+    [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<NodeId>();
+            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);
    }

+    /// <summary>
+    ///     Drives the manager's <c>Browse</c> override directly, optionally with references already in the
+    ///     list to stand in for an earlier BrowseNext page.
+    /// </summary>
+    private static List<ReferenceDescription> 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<ReferenceDescription>(previousPage);
+        nm.Browse(context, ref continuationPoint, references);
+        continuationPoint?.Dispose();
+        return references;
+    }
+
+    /// <summary>A Browse-only grant per named raw tag path, each in its own <c>Tag</c>-kind scope.</summary>
+    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<NodeAclPath>)
+                    [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,