From 4f25b376f5a6126cdad9ae1c4cb5a674058a71d5 Mon Sep 17 00:00:00 2001 From: Greg Lauckhart Date: Wed, 13 Nov 2024 15:44:43 -0800 Subject: [PATCH] Rewrite AccessControlServer event triggering Untested as yet --- packages/node/src/behavior/AccessControl.ts | 4 +- .../src/behavior/cluster/ClusterEvents.ts | 2 +- .../access-control/AccessControlServer.ts | 149 +++++++++++------- .../behavior/cluster/ClusterBehaviorTest.ts | 4 +- .../behavior/cluster/ClusterEventsTest.ts | 4 +- .../behavior/state/managed/DatasourceTest.ts | 6 +- 6 files changed, 105 insertions(+), 64 deletions(-) diff --git a/packages/node/src/behavior/AccessControl.ts b/packages/node/src/behavior/AccessControl.ts index d12e9d8f88..53045d3de1 100644 --- a/packages/node/src/behavior/AccessControl.ts +++ b/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. */ diff --git a/packages/node/src/behavior/cluster/ClusterEvents.ts b/packages/node/src/behavior/cluster/ClusterEvents.ts index 27dc1904f4..a60e919f91 100644 --- a/packages/node/src/behavior/cluster/ClusterEvents.ts +++ b/packages/node/src/behavior/cluster/ClusterEvents.ts @@ -29,7 +29,7 @@ export namespace ClusterEvents { * Properties the cluster contributes to Events. */ export type Properties = AttributeObservables, "Changing", ActionContext> & - AttributeObservables, "Changed", AccessControl.Subject> & + AttributeObservables, "Changed", AccessControl.Actor> & EventObservables>; export type AttributeObservables, N extends string, C> = { diff --git a/packages/node/src/behaviors/access-control/AccessControlServer.ts b/packages/node/src/behaviors/access-control/AccessControlServer.ts index 10ecb8e5da..480cbe2114 100644 --- a/packages/node/src/behaviors/access-control/AccessControlServer.ts +++ b/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( + actor: AccessControl.Actor, + oldEntries: T[], + newEntries: T[], +) { + const fabricLists = {} as FabricLists; + 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( + 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 = Record; + +function groupByFabrics( + target: FabricLists, + 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); + } +} diff --git a/packages/node/test/behavior/cluster/ClusterBehaviorTest.ts b/packages/node/test/behavior/cluster/ClusterBehaviorTest.ts index cbd9351edd..09c1d68a3d 100644 --- a/packages/node/test/behavior/cluster/ClusterBehaviorTest.ts +++ b/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]>; }; }; diff --git a/packages/node/test/behavior/cluster/ClusterEventsTest.ts b/packages/node/test/behavior/cluster/ClusterEventsTest.ts index c00f95d987..c2490094ee 100644 --- a/packages/node/test/behavior/cluster/ClusterEventsTest.ts +++ b/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 >; diff --git a/packages/node/test/behavior/state/managed/DatasourceTest.ts b/packages/node/test/behavior/state/managed/DatasourceTest.ts index 1aa02453df..44afd4da2e 100644 --- a/packages/node/test/behavior/state/managed/DatasourceTest.ts +++ b/packages/node/test/behavior/state/managed/DatasourceTest.ts @@ -365,9 +365,9 @@ describe("Datasource", () => { const { promise, resolver } = createPromise(); - 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 }); }); });