Rewrite AccessControlServer event triggering

Untested as yet

packages/node/src/behavior/AccessControl.ts

@@ -124,7 +124,7 @@ export namespace AccessControl {
     /**
      * Information about the subject that triggered a change.
      */
-    export interface Subject {
+    export interface Actor {
         /**
          * The fabric of the authorized client.
          */
@@ -148,7 +148,7 @@ export namespace AccessControl {
     /**
      * Authorization metadata that varies with session.
      */
-    export interface Session extends Subject {
+    export interface Session extends Actor {
         /**
          * Checks if the authorized client has a certain Access Privilege granted.
          */

packages/node/src/behavior/cluster/ClusterEvents.ts

@@ -29,7 +29,7 @@ export namespace ClusterEvents {
      * Properties the cluster contributes to Events.
      */
     export type Properties<C> = AttributeObservables<ClusterType.AttributesOf<C>, "Changing", ActionContext> &
-        AttributeObservables<ClusterType.AttributesOf<C>, "Changed", AccessControl.Subject> &
+        AttributeObservables<ClusterType.AttributesOf<C>, "Changed", AccessControl.Actor> &
         EventObservables<ClusterType.EventsOf<C>>;
 
     export type AttributeObservables<A extends Record<string, ClusterType.Attribute>, N extends string, C> = {

packages/node/src/behaviors/access-control/AccessControlServer.ts

@@ -16,6 +16,7 @@ import {
     ClusterId,
     DeviceTypeId,
     EndpointNumber,
+    FabricIndex,
     GroupId,
     NodeId,
     StatusCode,
@@ -190,67 +191,21 @@ export class AccessControlServer extends AccessControlBehavior {
         }
     }
 
+    /**
+     * Emit ACL change events.  Matter spec really only contemplates changes to ACLs in the context of a specific fabric
+     * but this logic supports multi-fabric changes just for completeness.
+     */
     #handleAccessControlListChange(
         value: AccessControlTypes.AccessControlEntry[],
         oldValue: AccessControlTypes.AccessControlEntry[],
+        actor: AccessControl.Actor,
     ) {
         if (this.internal.aclManager === undefined) {
             return; // Too early to send events
         }
-        const { session } = this.context;
-
-        // TODO: This might be not really correct for local ACL changes because there the session fabric could be
-        //  different which would lead to missing events
-        const relevantFabricIndex = session?.associatedFabric.fabricIndex;
 
-        if (relevantFabricIndex === undefined || this.events.accessControlEntryChanged === undefined) {
-            return;
-        }
-        const adminPasscodeId = session === undefined || session?.isPase ? 0 : null;
-        const adminNodeId = adminPasscodeId === null ? session?.associatedFabric.rootNodeId : null;
-        if (adminNodeId === undefined) {
-            // Should never happen
-            return;
-        }
-        const fabricAcls = value.filter(entry => entry.fabricIndex === relevantFabricIndex);
-        const oldFabricAcls = oldValue.filter(entry => entry.fabricIndex === relevantFabricIndex);
-
-        let i = 0;
-        for (; i < fabricAcls.length; i++) {
-            if (!isDeepEqual(fabricAcls[i], oldFabricAcls[i])) {
-                const changeType =
-                    oldFabricAcls[i] === undefined
-                        ? AccessControlTypes.ChangeType.Added
-                        : fabricAcls[i] === undefined
-                          ? AccessControlTypes.ChangeType.Removed
-                          : AccessControlTypes.ChangeType.Changed;
-                this.events.accessControlEntryChanged.emit(
-                    {
-                        changeType,
-                        adminNodeId,
-                        adminPasscodeId,
-                        latestValue:
-                            (changeType === AccessControlTypes.ChangeType.Removed ? oldFabricAcls[i] : fabricAcls[i]) ??
-                            null,
-                        fabricIndex: relevantFabricIndex,
-                    },
-                    this.context,
-                );
-            }
-        }
-        if (oldFabricAcls.length > i) {
-            for (let j = oldFabricAcls.length - 1; j >= i; j--) {
-                this.events.accessControlEntryChanged.emit(
-                    {
-                        changeType: AccessControlTypes.ChangeType.Removed,
-                        adminNodeId,
-                        adminPasscodeId,
-                        latestValue: oldValue[j],
-                        fabricIndex: relevantFabricIndex,
-                    },
-                    this.context,
-                );
-            }
+        for (const change of computeAclChanges(actor, oldValue, value)) {
+            this.events.accessControlEntryChanged.emit(change, this.context);
         }
     }
 
@@ -279,10 +234,15 @@ export class AccessControlServer extends AccessControlBehavior {
     #handleAccessControlExtensionChange(
         value: AccessControlTypes.AccessControlExtension[],
         oldValue: AccessControlTypes.AccessControlExtension[],
+        actor: AccessControl.Actor,
     ) {
         if (this.internal.aclManager === undefined) {
             return; // Too early to send events
         }
+
+        for (const change of computeAclChanges(actor, oldValue, value)) {
+            this.events.accessControlExtensionChanged.emit(change, this.context);
+        }
         const { session } = this.context;
 
         // TODO: This might be not really correct for local ACL changes because there the session fabric could be
@@ -458,3 +418,86 @@ export namespace AccessControlServer {
         delayedAclData?: AccessControlTypes.AccessControlEntry[];
     }
 }
+
+/**
+ * Spec indicates there should be notifications for "add", "remove" and "change".  It does not specify how to match
+ * entries across lists.  Based on how CHIP typically works we just match entries based on index in the fabric-filtered
+ * list.
+ */
+function* computeAclChanges<T extends { fabricIndex: FabricIndex }>(
+    actor: AccessControl.Actor,
+    oldEntries: T[],
+    newEntries: T[],
+) {
+    const fabricLists = {} as FabricLists<T>;
+    groupByFabrics(fabricLists, "old", oldEntries);
+    groupByFabrics(fabricLists, "new", newEntries);
+
+    for (const fabricIndex in fabricLists) {
+        const entry = fabricLists[fabricIndex as unknown as FabricIndex];
+        yield* computeFabricAclChanges(actor, fabricIndex as unknown as FabricIndex, entry.old, entry.new);
+    }
+}
+
+function* computeFabricAclChanges<T>(
+    actor: AccessControl.Actor,
+    fabricIndex: FabricIndex,
+    oldEntries: T[],
+    newEntries: T[],
+) {
+    const maxIndex = Math.max(oldEntries.length, newEntries.length);
+    for (let i = 0; i < maxIndex; i++) {
+        let changeType: AccessControlTypes.ChangeType;
+        if (oldEntries[i] === undefined) {
+            changeType = AccessControlTypes.ChangeType.Added;
+        } else if (newEntries[i] === undefined) {
+            changeType = AccessControlTypes.ChangeType.Removed;
+        } else if (isDeepEqual(oldEntries[i], newEntries[i])) {
+            continue;
+        } else {
+            changeType = AccessControlTypes.ChangeType.Changed;
+        }
+
+        let adminNodeId: NodeId | null, adminPasscodeId: number | null;
+        if (actor.fabric === fabricIndex && actor.subject !== undefined) {
+            adminNodeId = actor.subject;
+            adminPasscodeId = null;
+        } else {
+            // In theory the local node could mutate its lists.  Matter spec only contemplates changes via remote
+            // sessions so we just treat as a PASE change
+            if (actor.fabric !== undefined) {
+                // This is likely a bug but is theoretically possible with a custom cluster.  We also treat this as a
+                // PASE change
+                logger.warn(`Reporting change of ${fabricIndex} ACLs made by fabric ${actor.fabric}`);
+            }
+            adminNodeId = null;
+            adminPasscodeId = 0;
+        }
+
+        yield {
+            adminNodeId,
+            adminPasscodeId,
+            changeType,
+            latestValue: newEntries[i] ?? null,
+            fabricIndex,
+        };
+    }
+}
+
+type FabricLists<T extends { fabricIndex: FabricIndex }> = Record<FabricIndex, { new: T[]; old: T[] }>;
+
+function groupByFabrics<T extends { fabricIndex: FabricIndex }>(
+    target: FabricLists<T>,
+    type: "new" | "old",
+    entries: T[],
+) {
+    for (const entry of entries) {
+        let fabricList;
+        if (entry.fabricIndex in target) {
+            fabricList = target[entry.fabricIndex];
+        } else {
+            fabricList = target[entry.fabricIndex] = { new: [], old: [] };
+        }
+        fabricList[type].push(entry);
+    }
+}

packages/node/test/behavior/cluster/ClusterBehaviorTest.ts

@@ -107,9 +107,7 @@ describe("ClusterBehavior", () => {
 
             ({}) as MyBehavior satisfies {
                 events: EventEmitter & {
-                    reqAttr$Changed: AsyncObservable<
-                        [value: string, oldValue: string, context?: AccessControl.Subject]
-                    >;
+                    reqAttr$Changed: AsyncObservable<[value: string, oldValue: string, context?: AccessControl.Actor]>;
                 };
             };
 

packages/node/test/behavior/cluster/ClusterEventsTest.ts

@@ -47,7 +47,7 @@ describe("ClusterEvents", () => {
         it("includes required", () => {
             ({}) as Ep satisfies EventEmitter & {
                 reqAttr$Changed: Observable<
-                    [value: string, oldValue: string, context?: AccessControl.Subject],
+                    [value: string, oldValue: string, context?: AccessControl.Actor],
                     MaybePromise
                 >;
 
@@ -108,7 +108,7 @@ describe("ClusterEvents", () => {
         it("requires mandatory", () => {
             ({}) as Ei satisfies {
                 reqAttr$Changed: Observable<
-                    [value: string, oldValue: string, context: AccessControl.Subject],
+                    [value: string, oldValue: string, context: AccessControl.Actor],
                     MaybePromise
                 >;
 

packages/node/test/behavior/state/managed/DatasourceTest.ts

@@ -365,9 +365,9 @@ describe("Datasource", () => {
 
             const { promise, resolver } = createPromise<void>();
 
-            let subject: AccessControl.Subject | undefined;
+            let actor: AccessControl.Actor | undefined;
             events.foo$Changed.on(async (_v1, _v2, subj) => {
-                subject = subj;
+                actor = subj;
 
                 await withReference(ds2, ref => {
                     ref.state.bar = true;
@@ -383,7 +383,7 @@ describe("Datasource", () => {
             await promise;
 
             expect(ds2.view.bar).true;
-            expect(subject).deep.equals({ fabric: undefined, subject: undefined, offline: true });
+            expect(actor).deep.equals({ fabric: undefined, subject: undefined, offline: true });
         });
     });