diff --git a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs index 691bc8a..c4380b2 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs @@ -461,6 +461,14 @@ public sealed class MxAccessGatewayService( string? correlationId, CancellationToken cancellationToken) { + // An identity with no read constraints allows every tag, so the per-item enforcer call below + // can only answer "allowed" — the whole loop (and the plan it would build) is dead work. + // Returning null is exactly what the denied.Count == 0 exit below returns. + if (!constraintEnforcer.HasReadConstraints(identity)) + { + return null; + } + Dictionary denied = []; List allowed = []; for (int index = 0; index < tagAddresses.Count; index++) @@ -491,16 +499,23 @@ public sealed class MxAccessGatewayService( return null; } - MxCommand filtered = command.Clone(); - if (filtered.Kind == MxCommandKind.AddItemBulk) + // Build the filtered command directly instead of cloning the original and clearing it: + // the clone deep-copied every denied address only to drop it. The payload's other fields + // (server_handle) are copied across explicitly. Nothing aliases the request here — these + // bulk payloads carry only strings — and the worker-bound graph is still the unaliased copy + // MapCommand makes. + MxCommand filtered = new() { Kind = command.Kind }; + if (command.Kind == MxCommandKind.AddItemBulk) { - filtered.AddItemBulk.TagAddresses.Clear(); - filtered.AddItemBulk.TagAddresses.Add(allowed); + AddItemBulkCommand payload = new() { ServerHandle = command.AddItemBulk.ServerHandle }; + payload.TagAddresses.Add(allowed); + filtered.AddItemBulk = payload; } else { - filtered.SubscribeBulk.TagAddresses.Clear(); - filtered.SubscribeBulk.TagAddresses.Add(allowed); + SubscribeBulkCommand payload = new() { ServerHandle = command.SubscribeBulk.ServerHandle }; + payload.TagAddresses.Add(allowed); + filtered.SubscribeBulk = payload; } return new SubscribeBulkConstraintPlan(filtered, tagAddresses.Count, denied, allowed.Count > 0); @@ -517,6 +532,11 @@ public sealed class MxAccessGatewayService( // Mirrors FilterTagBulkAsync but produces BulkReadResult denial entries // so the reply payload merges into BulkReadReply.Results, not // BulkSubscribeReply.Results. + if (!constraintEnforcer.HasReadConstraints(identity)) + { + return null; + } + Dictionary denied = []; List allowed = []; for (int index = 0; index < tagAddresses.Count; index++) @@ -548,9 +568,14 @@ public sealed class MxAccessGatewayService( return null; } - MxCommand filtered = command.Clone(); - filtered.ReadBulk.TagAddresses.Clear(); - filtered.ReadBulk.TagAddresses.Add(allowed); + MxCommand filtered = new() { Kind = command.Kind }; + ReadBulkCommand payload = new() + { + ServerHandle = command.ReadBulk.ServerHandle, + TimeoutMs = command.ReadBulk.TimeoutMs, + }; + payload.TagAddresses.Add(allowed); + filtered.ReadBulk = payload; return new ReadBulkConstraintPlan(filtered, tagAddresses.Count, denied, allowed.Count > 0); } @@ -572,6 +597,11 @@ public sealed class MxAccessGatewayService( // Parameterising on TEntry + getItemHandle keeps a single filter // routine for all four and avoids duplicating CheckWriteHandleAsync // calls. + if (!constraintEnforcer.HasWriteConstraints(identity)) + { + return null; + } + Dictionary denied = []; List allowed = []; for (int index = 0; index < entries.Count; index++) @@ -609,33 +639,70 @@ public sealed class MxAccessGatewayService( return null; } - MxCommand filtered = command.Clone(); - ReplaceWriteBulkEntries(filtered, allowed); - return new WriteBulkConstraintPlan(filtered, entries.Count, denied, allowed.Count > 0); + return new WriteBulkConstraintPlan( + BuildFilteredWriteBulkCommand(command, allowed), + entries.Count, + denied, + allowed.Count > 0); } - private static void ReplaceWriteBulkEntries(MxCommand command, IReadOnlyList allowed) + /// + /// Builds the allowed-only bulk-write command. The allowed entries are carried over by + /// reference rather than deep-cloned: the caller only reads this command (TrackCommandReply), + /// and the copy the worker mutates and owns is the one MapCommand clones — the same + /// no-aliasing boundary as before. Cloning the whole command here and clearing it copied + /// every denied entry's payload (including WriteSecured values) for nothing. + /// + /// The per-family bulk-write entry message type. + /// The original command, read for its kind and payload scalars. + /// The entries that survived constraint filtering, in original order. + /// A command of the same kind carrying only the allowed entries. + private static MxCommand BuildFilteredWriteBulkCommand(MxCommand command, IReadOnlyList allowed) where TEntry : class { + MxCommand filtered = new() { Kind = command.Kind }; switch (command.Kind) { case MxCommandKind.WriteBulk: - command.WriteBulk.Entries.Clear(); - command.WriteBulk.Entries.Add((IEnumerable)allowed); + { + WriteBulkCommand payload = new() { ServerHandle = command.WriteBulk.ServerHandle }; + payload.Entries.Add((IEnumerable)allowed); + filtered.WriteBulk = payload; break; + } + case MxCommandKind.Write2Bulk: - command.Write2Bulk.Entries.Clear(); - command.Write2Bulk.Entries.Add((IEnumerable)allowed); + { + Write2BulkCommand payload = new() { ServerHandle = command.Write2Bulk.ServerHandle }; + payload.Entries.Add((IEnumerable)allowed); + filtered.Write2Bulk = payload; break; + } + case MxCommandKind.WriteSecuredBulk: - command.WriteSecuredBulk.Entries.Clear(); - command.WriteSecuredBulk.Entries.Add((IEnumerable)allowed); + { + WriteSecuredBulkCommand payload = new() { ServerHandle = command.WriteSecuredBulk.ServerHandle }; + payload.Entries.Add((IEnumerable)allowed); + filtered.WriteSecuredBulk = payload; break; + } + case MxCommandKind.WriteSecured2Bulk: - command.WriteSecured2Bulk.Entries.Clear(); - command.WriteSecured2Bulk.Entries.Add((IEnumerable)allowed); + { + WriteSecured2BulkCommand payload = new() { ServerHandle = command.WriteSecured2Bulk.ServerHandle }; + payload.Entries.Add((IEnumerable)allowed); + filtered.WriteSecured2Bulk = payload; break; + } + + default: + // Only the four bulk-write kinds above reach FilterWriteBulkAsync, so this is + // unreachable; keep the previous behaviour (the unmodified command) rather than + // emitting a payload-less one if that ever stops holding. + return command.Clone(); } + + return filtered; } private async Task FilterHandleBulkAsync( @@ -647,6 +714,11 @@ public sealed class MxAccessGatewayService( string? correlationId, CancellationToken cancellationToken) { + if (!constraintEnforcer.HasReadConstraints(identity)) + { + return null; + } + Dictionary denied = []; List allowed = []; for (int index = 0; index < itemHandles.Count; index++) @@ -677,9 +749,10 @@ public sealed class MxAccessGatewayService( return null; } - MxCommand filtered = command.Clone(); - filtered.AdviseItemBulk.ItemHandles.Clear(); - filtered.AdviseItemBulk.ItemHandles.Add(allowed); + MxCommand filtered = new() { Kind = command.Kind }; + AdviseItemBulkCommand payload = new() { ServerHandle = command.AdviseItemBulk.ServerHandle }; + payload.ItemHandles.Add(allowed); + filtered.AdviseItemBulk = payload; return new SubscribeBulkConstraintPlan(filtered, itemHandles.Count, denied, allowed.Count > 0); } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/GatewayApiKeyIdentityMapper.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/GatewayApiKeyIdentityMapper.cs index 75d9591..69d73c9 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/GatewayApiKeyIdentityMapper.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/GatewayApiKeyIdentityMapper.cs @@ -19,10 +19,35 @@ namespace ZB.MOM.WW.MxGateway.Server.Security.Authentication; /// public static class GatewayApiKeyIdentityMapper { - private const int MaxCachedConstraintBlobs = 1024; + /// + /// Maximum number of parsed constraint blobs retained in . + /// Blobs are admin-controlled (one per API key), so the cap is only a memory backstop for a + /// store with an unusually large number of distinct constrained keys. + /// + internal const int MaxCachedConstraintBlobs = 1024; + + /// + /// Bounded parsed-constraints cache keyed by the raw constraints JSON. The blob is parsed + /// once per authenticated RPC otherwise, so this keeps the JSON parse off the hot path. + /// Beyond entries the oldest insertion is evicted + /// rather than the cache refusing new entries — a hard stop at the cap would leave every key + /// admitted after it re-parsing its blob on every RPC for the process lifetime. Eviction is + /// approximate (FIFO over insertion order, not true LRU) because only the bound matters. + /// private static readonly ConcurrentDictionary ConstraintCache = new(StringComparer.Ordinal); + /// + /// Insertion-order queue used to evict the oldest cache entry once the cache exceeds + /// . Keeping it separate leaves + /// reads lock-free; the lock guards only the eviction path. + /// + private static readonly ConcurrentQueue InsertionOrder = new(); + private static readonly object EvictionLock = new(); + + /// Current cache size, exposed for tests asserting the cap is honoured. + internal static int CurrentCacheSize => ConstraintCache.Count; + private static ApiKeyConstraints DeserializeConstraints(string? constraintsJson) { if (string.IsNullOrWhiteSpace(constraintsJson)) @@ -36,12 +61,36 @@ public static class GatewayApiKeyIdentityMapper } ApiKeyConstraints parsed = ApiKeyConstraintSerializer.Deserialize(constraintsJson); - if (ConstraintCache.Count < MaxCachedConstraintBlobs) + + // GetOrAdd returns whichever instance is in the cache after the call, so concurrent parsers + // of the same blob converge on one instance; it also avoids the TryAdd-then-read race where + // the key could be evicted between a failed TryAdd and the read back. + ApiKeyConstraints result = ConstraintCache.GetOrAdd(constraintsJson, parsed); + if (ReferenceEquals(result, parsed)) { - ConstraintCache.TryAdd(constraintsJson, parsed); + // We were the inserter — track for FIFO eviction and bound the cache. + InsertionOrder.Enqueue(constraintsJson); + EvictIfOverCapacity(); } - return parsed; + return result; + } + + private static void EvictIfOverCapacity() + { + if (ConstraintCache.Count <= MaxCachedConstraintBlobs) + { + return; + } + + // Serialize eviction so two threads do not race past the cap together. + lock (EvictionLock) + { + while (ConstraintCache.Count > MaxCachedConstraintBlobs && InsertionOrder.TryDequeue(out string? oldest)) + { + ConstraintCache.TryRemove(oldest, out _); + } + } } /// diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintEnforcer.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintEnforcer.cs index e897c9e..a61bcfd 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintEnforcer.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintEnforcer.cs @@ -16,6 +16,14 @@ public sealed class ConstraintEnforcer( IGalaxyHierarchyCache cache, IAuditWriter auditWriter) : IConstraintEnforcer { + /// + public bool HasReadConstraints(ApiKeyIdentity? identity) => + identity?.EffectiveConstraints.HasReadConstraints ?? false; + + /// + public bool HasWriteConstraints(ApiKeyIdentity? identity) => + identity?.EffectiveConstraints.HasWriteConstraints ?? false; + /// public Task CheckReadTagAsync( ApiKeyIdentity? identity, @@ -211,7 +219,25 @@ public sealed class ConstraintEnforcer( return true; } - return subtreeGlobs.Any(glob => GalaxyGlobMatcher.IsMatch(containedPath, glob)) - || tagGlobs.Any(glob => GalaxyGlobMatcher.IsMatch(tagAddress, glob)); + // Plain index loops rather than Any(lambda): this runs once per item of every bulk + // read/write, and the closures the lambdas capture (containedPath / tagAddress) allocate a + // display class plus a delegate per call. Same short-circuit order, same result. + for (int i = 0; i < subtreeGlobs.Count; i++) + { + if (GalaxyGlobMatcher.IsMatch(containedPath, subtreeGlobs[i])) + { + return true; + } + } + + for (int i = 0; i < tagGlobs.Count; i++) + { + if (GalaxyGlobMatcher.IsMatch(tagAddress, tagGlobs[i])) + { + return true; + } + } + + return false; } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/IConstraintEnforcer.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/IConstraintEnforcer.cs index 406f06e..95728ae 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/IConstraintEnforcer.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/IConstraintEnforcer.cs @@ -5,6 +5,30 @@ namespace ZB.MOM.WW.MxGateway.Server.Security.Authorization; public interface IConstraintEnforcer { + /// + /// Gets a value indicating whether any read constraint applies to an identity at all, so a + /// bulk caller can hoist the question out of its per-item loop. + /// + /// The API key identity. + /// when at least one read constraint applies; otherwise . + /// + /// Every per-item / call for + /// an unconstrained identity allows the item, so skipping the loop removes work without + /// changing a decision. The default implementation answers — an + /// implementation that does not model constraints (test doubles, allow-all enforcers) keeps + /// being consulted per item rather than being silently bypassed. + /// + bool HasReadConstraints(ApiKeyIdentity? identity) => true; + + /// + /// Gets a value indicating whether any write constraint applies to an identity at all, the + /// write-side counterpart of . + /// + /// The API key identity. + /// when at least one write constraint applies; otherwise . + /// The same conservative default as applies. + bool HasWriteConstraints(ApiKeyIdentity? identity) => true; + /// Checks whether a read constraint is satisfied for a tag address. /// The API key identity. /// Tag address to check. diff --git a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SparseArrayExpander.cs b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SparseArrayExpander.cs index c6006db..c8f81eb 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Sessions/SparseArrayExpander.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Sessions/SparseArrayExpander.cs @@ -120,6 +120,10 @@ internal static class SparseArrayExpander case MxDataType.Boolean: { BoolArray values = new(); + + // Size the backing store once: the fill below adds exactly `length` elements, + // so without this the RepeatedField doubles its array log2(length) times. + values.Values.Capacity = length; for (int i = 0; i < length; i++) { values.Values.Add(false); @@ -137,6 +141,7 @@ internal static class SparseArrayExpander case MxDataType.Integer when UsesInt64(elements): { Int64Array values = new(); + values.Values.Capacity = length; for (int i = 0; i < length; i++) { values.Values.Add(0L); @@ -154,6 +159,7 @@ internal static class SparseArrayExpander case MxDataType.Integer: { Int32Array values = new(); + values.Values.Capacity = length; for (int i = 0; i < length; i++) { values.Values.Add(0); @@ -171,6 +177,7 @@ internal static class SparseArrayExpander case MxDataType.Float: { FloatArray values = new(); + values.Values.Capacity = length; for (int i = 0; i < length; i++) { values.Values.Add(0f); @@ -188,6 +195,7 @@ internal static class SparseArrayExpander case MxDataType.Double: { DoubleArray values = new(); + values.Values.Capacity = length; for (int i = 0; i < length; i++) { values.Values.Add(0d); @@ -205,6 +213,7 @@ internal static class SparseArrayExpander case MxDataType.String: { StringArray values = new(); + values.Values.Capacity = length; for (int i = 0; i < length; i++) { values.Values.Add(string.Empty); @@ -222,6 +231,7 @@ internal static class SparseArrayExpander case MxDataType.Time: { TimestampArray values = new(); + values.Values.Capacity = length; for (int i = 0; i < length; i++) { values.Values.Add(new Timestamp { Seconds = 0, Nanos = 0 }); diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs index 2577a0f..a75046c 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs @@ -8,10 +8,12 @@ using LibApiKeyIdentity = ZB.MOM.WW.Auth.Abstractions.ApiKeys.ApiKeyIdentity; namespace ZB.MOM.WW.MxGateway.Tests.Security.Authentication; /// -/// Hot-path decorators. Covers both mechanisms: -/// (read/verification coalescing plus revoke/rotate invalidation) and +/// Hot-path decorators. Covers all three mechanisms: +/// (read/verification coalescing plus revoke/rotate invalidation), /// (the last_used write coalescing that keeps the -/// per-RPC database write off the throughput ceiling). +/// per-RPC database write off the throughput ceiling), and the constraint-blob cache inside +/// (which keeps the per-RPC constraints JSON parse off +/// the authenticated path). /// public sealed class CachingApiKeyVerifierTests { @@ -245,6 +247,50 @@ public sealed class CachingApiKeyVerifierTests Assert.Equal(keyId, CachingApiKeyVerifier.TryParseKeyId($"Bearer mxgw_{keyId}_secret")); } + /// + /// The mapper's constraint-blob cache is bounded by eviction, not by a hard stop at the cap: + /// once it is full the oldest entry is dropped so a newly-seen blob is still cached. A cache + /// that merely stopped accepting entries would re-parse every blob beyond the cap on every + /// single RPC, forever. Asserted behaviourally through instance identity — a cached blob maps + /// to the same instance, a re-parsed one does not. + /// + [Fact] + public void ToGatewayIdentity_ConstraintCacheOverCapacity_EvictsOldestAndKeepsCaching() + { + string firstJson = ConstraintsJson("Area_FifoProbe"); + ApiKeyConstraints first = MapConstraints(firstJson); + Assert.Same(first, MapConstraints(firstJson)); + + // Push strictly more than the cap through the cache after the probe blob, so FIFO eviction + // is guaranteed to have reached it however full the (process-wide) cache already was. + for (int i = 0; i < GatewayApiKeyIdentityMapper.MaxCachedConstraintBlobs + 8; i++) + { + MapConstraints(ConstraintsJson($"Area_FifoFlood_{i}")); + } + + Assert.True( + GatewayApiKeyIdentityMapper.CurrentCacheSize <= GatewayApiKeyIdentityMapper.MaxCachedConstraintBlobs, + $"cache grew to {GatewayApiKeyIdentityMapper.CurrentCacheSize} entries, past the {GatewayApiKeyIdentityMapper.MaxCachedConstraintBlobs} cap"); + + // Evicted, so the probe blob is parsed afresh... + ApiKeyConstraints reparsed = MapConstraints(firstJson); + Assert.NotSame(first, reparsed); + Assert.Equal(first.ReadSubtrees, reparsed.ReadSubtrees); + + // ...and re-cached, rather than re-parsed on every later call. + Assert.Same(reparsed, MapConstraints(firstJson)); + } + + private static ApiKeyConstraints MapConstraints(string constraintsJson) => + GatewayApiKeyIdentityMapper.ToGatewayIdentity(new LibApiKeyIdentity( + KeyId: "operator01", + DisplayName: "Operator Key", + Scopes: new HashSet(StringComparer.Ordinal), + Constraints: constraintsJson)).EffectiveConstraints; + + private static string ConstraintsJson(string readSubtree) => + ApiKeyConstraintSerializer.Serialize(ApiKeyConstraints.Empty with { ReadSubtrees = [readSubtree] })!; + private static MemoryCache NewCache() => new(new MemoryCacheOptions()); private static ApiKeyVerification Success(string keyId) => new(