From ec614c62074ddfdd504ea1bdb57c4aef5dd837da Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Mon, 23 Aug 2021 23:52:59 -0700 Subject: [PATCH 01/12] Implement SubjectMetadataController --- packages/controllers/src/index.ts | 1 + .../SubjectMetadataController.test.ts | 292 ++++++++++++++++++ .../SubjectMetadataController.ts | 263 ++++++++++++++++ .../controllers/src/subject-metadata/index.ts | 1 + 4 files changed, 557 insertions(+) create mode 100644 packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts create mode 100644 packages/controllers/src/subject-metadata/SubjectMetadataController.ts create mode 100644 packages/controllers/src/subject-metadata/index.ts diff --git a/packages/controllers/src/index.ts b/packages/controllers/src/index.ts index fc6cb0e22f..62b4f5165b 100644 --- a/packages/controllers/src/index.ts +++ b/packages/controllers/src/index.ts @@ -2,3 +2,4 @@ export * from './permissions'; export * from './resource'; export * from './services'; export * from './snaps'; +export * from './subject-metadata'; diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts new file mode 100644 index 0000000000..b7a17fc475 --- /dev/null +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts @@ -0,0 +1,292 @@ +import { ControllerMessenger, Json } from '@metamask/controllers'; +import { HasPermissions } from '../permissions'; +import { + SubjectMetadataController, + SubjectMetadataControllerActions, + SubjectMetadataControllerEvents, +} from './SubjectMetadataController'; + +const controllerName = 'SubjectMetadataController'; + +function getSubjectMetadataControllerMessenger() { + const controllerMessenger = new ControllerMessenger< + SubjectMetadataControllerActions | HasPermissions, + SubjectMetadataControllerEvents + >(); + + const hasPermissionsSpy = jest.fn(); + controllerMessenger.registerActionHandler( + 'PermissionController:hasPermissions', + hasPermissionsSpy, + ); + + return [ + controllerMessenger.getRestricted< + typeof controllerName, + SubjectMetadataControllerActions['type'] | HasPermissions['type'], + SubjectMetadataControllerEvents['type'] + >({ + name: controllerName, + allowedActions: [ + 'PermissionController:hasPermissions', + 'SubjectMetadataController:clearState', + 'SubjectMetadataController:getState', + 'SubjectMetadataController:trimMetadataState', + ], + }), + hasPermissionsSpy, + ] as const; +} + +function getSubjectMetadata( + origin: string, + name: string, + opts?: Record, +) { + return { + origin, + name, + iconUrl: null, + host: null, + extensionId: null, + ...opts, + }; +} + +describe('SubjectMetadataController', () => { + describe('constructor', () => { + it('initializes a subject metadata controller', () => { + const controller = new SubjectMetadataController({ + messenger: getSubjectMetadataControllerMessenger()[0], + subjectCacheLimit: 10, + }); + expect(controller.state).toStrictEqual({ subjectMetadata: {} }); + }); + + it('trims subject metadata state on startup', () => { + const [messenger, hasPermissionsSpy] = + getSubjectMetadataControllerMessenger(); + hasPermissionsSpy.mockImplementationOnce(() => false); + hasPermissionsSpy.mockImplementationOnce(() => true); + + const controller = new SubjectMetadataController({ + messenger, + subjectCacheLimit: 10, + state: { + subjectMetadata: { + 'foo.com': getSubjectMetadata('foo.com', 'foo'), + 'bar.io': getSubjectMetadata('bar.io', 'bar'), + }, + }, + }); + + expect(controller.state).toStrictEqual({ + subjectMetadata: { 'bar.io': getSubjectMetadata('bar.io', 'bar') }, + }); + }); + + it('throws if the subject cache limit is invalid', () => { + [0, -1, 1.1].forEach((subjectCacheLimit) => { + expect( + () => + new SubjectMetadataController({ + messenger: getSubjectMetadataControllerMessenger()[0], + subjectCacheLimit, + }), + ).toThrow( + `subjectCacheLimit must be a positive integer. Received: "${subjectCacheLimit}"`, + ); + }); + }); + }); + + describe('clearState', () => { + it('clears the controller state', () => { + const [messenger, hasPermissionsSpy] = + getSubjectMetadataControllerMessenger(); + hasPermissionsSpy.mockImplementation(() => true); + + const controller = new SubjectMetadataController({ + messenger, + subjectCacheLimit: 10, + state: { + subjectMetadata: { + 'foo.com': getSubjectMetadata('foo.com', 'foo'), + }, + }, + }); + + expect(controller.state).toStrictEqual({ + subjectMetadata: { 'foo.com': getSubjectMetadata('foo.com', 'foo') }, + }); + controller.clearState(); + expect(controller.state).toStrictEqual({ subjectMetadata: {} }); + }); + }); + + describe('addSubjectMetadata', () => { + it('adds subject metadata', () => { + const controller = new SubjectMetadataController({ + messenger: getSubjectMetadataControllerMessenger()[0], + subjectCacheLimit: 10, + }); + + controller.addSubjectMetadata(getSubjectMetadata('foo.com', 'foo')); + expect(controller.state).toStrictEqual({ + subjectMetadata: { 'foo.com': getSubjectMetadata('foo.com', 'foo') }, + }); + }); + + it('fills in missing fields of added subject metadata', () => { + const controller = new SubjectMetadataController({ + messenger: getSubjectMetadataControllerMessenger()[0], + subjectCacheLimit: 10, + }); + + controller.addSubjectMetadata({ origin: 'foo.com', name: 'foo' }); + expect(controller.state).toStrictEqual({ + subjectMetadata: { 'foo.com': getSubjectMetadata('foo.com', 'foo') }, + }); + }); + + it('does not delete metadata for subjects with permissions if cache size is exceeded', () => { + const [messenger, hasPermissionsSpy] = + getSubjectMetadataControllerMessenger(); + const controller = new SubjectMetadataController({ + messenger, + subjectCacheLimit: 1, + }); + hasPermissionsSpy.mockImplementationOnce(() => true); + + controller.addSubjectMetadata({ origin: 'foo.com', name: 'foo' }); + controller.addSubjectMetadata({ origin: 'bar.io', name: 'bar' }); + expect(controller.state).toStrictEqual({ + subjectMetadata: { + 'foo.com': getSubjectMetadata('foo.com', 'foo'), + 'bar.io': getSubjectMetadata('bar.io', 'bar'), + }, + }); + }); + + it('deletes metadata for subjects without permissions if cache size is exceeded', () => { + const [messenger, hasPermissionsSpy] = + getSubjectMetadataControllerMessenger(); + const controller = new SubjectMetadataController({ + messenger, + subjectCacheLimit: 1, + }); + hasPermissionsSpy.mockImplementationOnce(() => false); + + controller.addSubjectMetadata({ origin: 'foo.com', name: 'foo' }); + controller.addSubjectMetadata({ origin: 'bar.io', name: 'bar' }); + expect(controller.state).toStrictEqual({ + subjectMetadata: { + 'bar.io': getSubjectMetadata('bar.io', 'bar'), + }, + }); + }); + + it('adds "host" property for valid URL origin', () => { + const controller = new SubjectMetadataController({ + messenger: getSubjectMetadataControllerMessenger()[0], + subjectCacheLimit: 10, + }); + + controller.addSubjectMetadata({ origin: 'https://foo.com', name: 'foo' }); + expect(controller.state).toStrictEqual({ + subjectMetadata: { + 'https://foo.com': getSubjectMetadata('https://foo.com', 'foo', { + host: 'foo.com', + }), + }, + }); + }); + + it('logs error if failing to compute host from URL-like origin', () => { + const controller = new SubjectMetadataController({ + messenger: getSubjectMetadataControllerMessenger()[0], + subjectCacheLimit: 10, + }); + jest.spyOn(console, 'error'); + + controller.addSubjectMetadata({ origin: 'https://foo@', name: 'foo' }); + expect(controller.state).toStrictEqual({ + subjectMetadata: { + 'https://foo@': getSubjectMetadata('https://foo@', 'foo'), + }, + }); + expect(console.error).toHaveBeenCalledTimes(1); + }); + }); + + describe('trimMetadataState', () => { + it('deletes all subjects without permissions from state', () => { + const [messenger, hasPermissionsSpy] = + getSubjectMetadataControllerMessenger(); + const controller = new SubjectMetadataController({ + messenger, + subjectCacheLimit: 4, + }); + + controller.addSubjectMetadata({ origin: 'A', name: 'a' }); + controller.addSubjectMetadata({ origin: 'B', name: 'b' }); + controller.addSubjectMetadata({ origin: 'C', name: 'c' }); + controller.addSubjectMetadata({ origin: 'D', name: 'd' }); + expect(controller.state).toStrictEqual({ + subjectMetadata: { + A: getSubjectMetadata('A', 'a'), + B: getSubjectMetadata('B', 'b'), + C: getSubjectMetadata('C', 'c'), + D: getSubjectMetadata('D', 'd'), + }, + }); + + hasPermissionsSpy.mockImplementationOnce(() => true); + hasPermissionsSpy.mockImplementationOnce(() => false); + hasPermissionsSpy.mockImplementationOnce(() => false); + hasPermissionsSpy.mockImplementationOnce(() => true); + + controller.trimMetadataState(); + expect(controller.state).toStrictEqual({ + subjectMetadata: { + A: getSubjectMetadata('A', 'a'), + D: getSubjectMetadata('D', 'd'), + }, + }); + }); + }); + + describe('actions', () => { + describe('SubjectMetadataController:clearState', () => { + it('calls SubjectMetadataController.clearState', () => { + const [messenger] = getSubjectMetadataControllerMessenger(); + + const controller = new SubjectMetadataController({ + messenger, + subjectCacheLimit: 10, + }); + jest.spyOn(controller, 'clearState'); + + messenger.call('SubjectMetadataController:clearState'); + expect(controller.clearState).toHaveBeenCalledTimes(1); + expect(controller.clearState).toHaveBeenCalledWith(); + }); + }); + + describe('SubjectMetadataController:trimMetadataState', () => { + it('calls SubjectMetadataController.trimMetadataState', () => { + const [messenger] = getSubjectMetadataControllerMessenger(); + + const controller = new SubjectMetadataController({ + messenger, + subjectCacheLimit: 10, + }); + jest.spyOn(controller, 'trimMetadataState'); + + messenger.call('SubjectMetadataController:trimMetadataState'); + expect(controller.trimMetadataState).toHaveBeenCalledTimes(1); + expect(controller.trimMetadataState).toHaveBeenCalledWith(); + }); + }); + }); +}); diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts new file mode 100644 index 0000000000..4e27dc0f28 --- /dev/null +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts @@ -0,0 +1,263 @@ +import { + BaseControllerV2 as BaseController, + RestrictedControllerMessenger, +} from '@metamask/controllers'; +import type { Patch } from 'immer'; +import { Json } from 'json-rpc-engine'; + +import type { + GenericPermissionController, + PermissionSubjectMetadata, + HasPermissions, +} from '../permissions'; + +const controllerName = 'SubjectMetadataController'; + +type SubjectOrigin = string; + +export type SubjectMetadata = PermissionSubjectMetadata & { + [key: string]: Json; + name: string; + // TODO:TS4.4 make optional + extensionId: string | null; + host: string | null; + iconUrl: string | null; +}; + +type SubjectMetadataToAdd = PermissionSubjectMetadata & { + name: string; + extensionId?: string | null; + host?: string | null; + iconUrl?: string | null; +} & Record; + +export type SubjectMetadataControllerState = { + subjectMetadata: Record; +}; + +const stateMetadata = { + subjectMetadata: { persist: true, anonymous: true }, +}; + +const defaultState: SubjectMetadataControllerState = { + subjectMetadata: {}, +}; + +export type GetSubjectMetadataState = { + type: `${typeof controllerName}:getState`; + handler: () => SubjectMetadataControllerState; +}; + +export type ClearSubjectMetadataState = { + type: `${typeof controllerName}:clearState`; + handler: () => void; +}; + +export type TrimSubjectMetadataState = { + type: `${typeof controllerName}:trimMetadataState`; + handler: () => void; +}; + +export type SubjectMetadataControllerActions = + | GetSubjectMetadataState + | ClearSubjectMetadataState + | TrimSubjectMetadataState; + +export type SubjectMetadataStateChange = { + type: `${typeof controllerName}:stateChange`; + payload: [SubjectMetadataControllerState, Patch[]]; +}; + +export type SubjectMetadataControllerEvents = SubjectMetadataStateChange; + +type AllowedActions = HasPermissions; + +export type SubjectMetadataControllerMessenger = RestrictedControllerMessenger< + typeof controllerName, + SubjectMetadataControllerActions | AllowedActions, + SubjectMetadataControllerEvents, + AllowedActions['type'], + never +>; + +type SubjectMetadataControllerOptions = { + messenger: SubjectMetadataControllerMessenger; + subjectCacheLimit: number; + state?: Partial; +}; + +/** + * A controller for storing metadata associated with permission subjects. More + * or less, a cache. + */ +export class SubjectMetadataController extends BaseController< + typeof controllerName, + SubjectMetadataControllerState, + SubjectMetadataControllerMessenger +> { + private _subjectCacheLimit: number; + + get subjectCacheLimit(): number { + return this._subjectCacheLimit; + } + + private subjectsEncounteredSinceStartup: Set; + + private subjectHasPermissions: GenericPermissionController['hasPermissions']; + + constructor({ + messenger, + subjectCacheLimit, + state = {}, + }: SubjectMetadataControllerOptions) { + if (!Number.isInteger(subjectCacheLimit) || subjectCacheLimit < 1) { + throw new Error( + `subjectCacheLimit must be a positive integer. Received: "${subjectCacheLimit}"`, + ); + } + + const hasPermissions = (origin: string) => { + return messenger.call('PermissionController:hasPermissions', origin); + }; + + if (state.subjectMetadata) { + SubjectMetadataController.trimMetadataState( + state as SubjectMetadataControllerState, + hasPermissions, + ); + } + + super({ + name: controllerName, + metadata: stateMetadata, + messenger, + state: { ...defaultState, ...state }, + }); + + this.subjectHasPermissions = hasPermissions; + this._subjectCacheLimit = subjectCacheLimit; + this.subjectsEncounteredSinceStartup = new Set(); + this.registerMessageHandlers(); + } + + /** + * Constructor helper for registering message handlers for the actions of this + * controller. + */ + private registerMessageHandlers(): void { + this.messagingSystem.registerActionHandler( + `${controllerName}:clearState`, + () => this.clearState(), + ); + + this.messagingSystem.registerActionHandler( + `${controllerName}:trimMetadataState`, + () => this.trimMetadataState(), + ); + } + + /** + * Clears the state of this controller. + */ + clearState(): void { + this.update((_draftState) => { + return { ...defaultState }; + }); + } + + /** + * Stores domain metadata for the given origin (subject). Deletes metadata for + * subjects without permissions in a FIFO manner once more than + * {@link SubjectMetadataController.subjectCacheLimit} distinct origins have + * been added since boot. + * + * In order to prevent a degraded user experience, + * metadata is never deleted for subjects with permissions, since metadata + * cannot yet be requested on demand. + * + * @param metadata - The subject metadata to store. + */ + addSubjectMetadata(metadata: SubjectMetadataToAdd): void { + const { origin } = metadata; + const newMetadata: SubjectMetadata = { + ...metadata, + extensionId: metadata.extensionId || null, + host: metadata.host || null, + iconUrl: metadata.iconUrl || null, + }; + + let originToForget: string | null = null; + // We only delete the oldest encountered subject from the cache, again to + // ensure that the user's experience isn't degraded by missing icons etc. + if (this.subjectsEncounteredSinceStartup.size >= this.subjectCacheLimit) { + const cachedOrigin = this.subjectsEncounteredSinceStartup + .values() + .next().value; + + this.subjectsEncounteredSinceStartup.delete(cachedOrigin); + if (!this.subjectHasPermissions(cachedOrigin)) { + originToForget = cachedOrigin; + } + } + + this.subjectsEncounteredSinceStartup.add(origin); + + if ( + !newMetadata.extensionId && + !newMetadata.host && + /[a-z]+:\/\/\w.+/iu.test(origin) + ) { + try { + newMetadata.host = new URL(origin).host; + } catch (_) { + console.error( + `${controllerName}: Encountered invalid URL: "${origin}"`, + ); + } + } + + this.update((draftState) => { + // Typecast: ts(2589) + draftState.subjectMetadata[origin] = newMetadata as any; + if (typeof originToForget === 'string') { + delete draftState.subjectMetadata[originToForget]; + } + }); + } + + /** + * Deletes all subjects without permissions from the controller's state. + */ + trimMetadataState(): void { + this.update((draftState) => { + SubjectMetadataController.trimMetadataState( + // Typecast: ts(2589) + draftState as any, + this.subjectHasPermissions, + ); + }); + } + + /** + * Deletes all subjects without permissions from the given state object. This + * method is static because we want to call it in the constructor, before the + * controller's state is initialized. + * + * @param draftState - The `immer` draft state. + * @param hasPermissions - A function that returns a boolean indicating + * whether a particular subject (identified by its origin) has any + * permissions. + */ + private static trimMetadataState( + draftState: SubjectMetadataControllerState, + hasPermissions: SubjectMetadataController['subjectHasPermissions'], + ): void { + const { subjectMetadata } = draftState; + + Object.keys(subjectMetadata).forEach((origin) => { + if (!hasPermissions(origin)) { + delete subjectMetadata[origin]; + } + }); + } +} diff --git a/packages/controllers/src/subject-metadata/index.ts b/packages/controllers/src/subject-metadata/index.ts new file mode 100644 index 0000000000..3b0bcaa14b --- /dev/null +++ b/packages/controllers/src/subject-metadata/index.ts @@ -0,0 +1 @@ +export * from './SubjectMetadataController'; From f5e389e24d47bf6d63d0ab9c1c09ffbbd44f42e5 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Tue, 23 Nov 2021 16:09:07 -0800 Subject: [PATCH 02/12] Remove console.error for invalid URL origins --- .../src/subject-metadata/SubjectMetadataController.test.ts | 2 -- .../src/subject-metadata/SubjectMetadataController.ts | 4 +--- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts index b7a17fc475..25816b1edb 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts @@ -207,7 +207,6 @@ describe('SubjectMetadataController', () => { messenger: getSubjectMetadataControllerMessenger()[0], subjectCacheLimit: 10, }); - jest.spyOn(console, 'error'); controller.addSubjectMetadata({ origin: 'https://foo@', name: 'foo' }); expect(controller.state).toStrictEqual({ @@ -215,7 +214,6 @@ describe('SubjectMetadataController', () => { 'https://foo@': getSubjectMetadata('https://foo@', 'foo'), }, }); - expect(console.error).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts index 4e27dc0f28..08a25d9636 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts @@ -210,9 +210,7 @@ export class SubjectMetadataController extends BaseController< try { newMetadata.host = new URL(origin).host; } catch (_) { - console.error( - `${controllerName}: Encountered invalid URL: "${origin}"`, - ); + // Do nothing. Some "origins" are not valid URLs. } } From e794d40a37222842f81d83176e69b9d2c1b969f8 Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Wed, 24 Nov 2021 13:02:56 -0800 Subject: [PATCH 03/12] Update packages/controllers/src/subject-metadata/SubjectMetadataController.ts Co-authored-by: Mark Stacey --- .../src/subject-metadata/SubjectMetadataController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts index 08a25d9636..e76315268f 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts @@ -36,7 +36,7 @@ export type SubjectMetadataControllerState = { }; const stateMetadata = { - subjectMetadata: { persist: true, anonymous: true }, + subjectMetadata: { persist: true, anonymous: false }, }; const defaultState: SubjectMetadataControllerState = { From f7bd1ccfee88f8c3f92b1666efe580c30c236694 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 24 Nov 2021 13:11:07 -0800 Subject: [PATCH 04/12] Delete subject cache limit getter --- .../src/subject-metadata/SubjectMetadataController.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts index e76315268f..53318c7cce 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts @@ -97,10 +97,6 @@ export class SubjectMetadataController extends BaseController< > { private _subjectCacheLimit: number; - get subjectCacheLimit(): number { - return this._subjectCacheLimit; - } - private subjectsEncounteredSinceStartup: Set; private subjectHasPermissions: GenericPermissionController['hasPermissions']; @@ -189,7 +185,7 @@ export class SubjectMetadataController extends BaseController< let originToForget: string | null = null; // We only delete the oldest encountered subject from the cache, again to // ensure that the user's experience isn't degraded by missing icons etc. - if (this.subjectsEncounteredSinceStartup.size >= this.subjectCacheLimit) { + if (this.subjectsEncounteredSinceStartup.size >= this._subjectCacheLimit) { const cachedOrigin = this.subjectsEncounteredSinceStartup .values() .next().value; From 7a5aa3d9fe73a2680f77c6c888ca0bac75161f79 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 24 Nov 2021 13:34:45 -0800 Subject: [PATCH 05/12] Remove trim metadata and clear state actions --- .../SubjectMetadataController.test.ts | 36 ------------------- .../SubjectMetadataController.ts | 32 +---------------- 2 files changed, 1 insertion(+), 67 deletions(-) diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts index 25816b1edb..caf8945992 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts @@ -29,9 +29,7 @@ function getSubjectMetadataControllerMessenger() { name: controllerName, allowedActions: [ 'PermissionController:hasPermissions', - 'SubjectMetadataController:clearState', 'SubjectMetadataController:getState', - 'SubjectMetadataController:trimMetadataState', ], }), hasPermissionsSpy, @@ -253,38 +251,4 @@ describe('SubjectMetadataController', () => { }); }); }); - - describe('actions', () => { - describe('SubjectMetadataController:clearState', () => { - it('calls SubjectMetadataController.clearState', () => { - const [messenger] = getSubjectMetadataControllerMessenger(); - - const controller = new SubjectMetadataController({ - messenger, - subjectCacheLimit: 10, - }); - jest.spyOn(controller, 'clearState'); - - messenger.call('SubjectMetadataController:clearState'); - expect(controller.clearState).toHaveBeenCalledTimes(1); - expect(controller.clearState).toHaveBeenCalledWith(); - }); - }); - - describe('SubjectMetadataController:trimMetadataState', () => { - it('calls SubjectMetadataController.trimMetadataState', () => { - const [messenger] = getSubjectMetadataControllerMessenger(); - - const controller = new SubjectMetadataController({ - messenger, - subjectCacheLimit: 10, - }); - jest.spyOn(controller, 'trimMetadataState'); - - messenger.call('SubjectMetadataController:trimMetadataState'); - expect(controller.trimMetadataState).toHaveBeenCalledTimes(1); - expect(controller.trimMetadataState).toHaveBeenCalledWith(); - }); - }); - }); }); diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts index 53318c7cce..b947932ae9 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts @@ -48,20 +48,7 @@ export type GetSubjectMetadataState = { handler: () => SubjectMetadataControllerState; }; -export type ClearSubjectMetadataState = { - type: `${typeof controllerName}:clearState`; - handler: () => void; -}; - -export type TrimSubjectMetadataState = { - type: `${typeof controllerName}:trimMetadataState`; - handler: () => void; -}; - -export type SubjectMetadataControllerActions = - | GetSubjectMetadataState - | ClearSubjectMetadataState - | TrimSubjectMetadataState; +export type SubjectMetadataControllerActions = GetSubjectMetadataState; export type SubjectMetadataStateChange = { type: `${typeof controllerName}:stateChange`; @@ -133,23 +120,6 @@ export class SubjectMetadataController extends BaseController< this.subjectHasPermissions = hasPermissions; this._subjectCacheLimit = subjectCacheLimit; this.subjectsEncounteredSinceStartup = new Set(); - this.registerMessageHandlers(); - } - - /** - * Constructor helper for registering message handlers for the actions of this - * controller. - */ - private registerMessageHandlers(): void { - this.messagingSystem.registerActionHandler( - `${controllerName}:clearState`, - () => this.clearState(), - ); - - this.messagingSystem.registerActionHandler( - `${controllerName}:trimMetadataState`, - () => this.trimMetadataState(), - ); } /** From 03e4acca3d16cc3fb45010046340d9cb53874fe6 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 24 Nov 2021 20:44:38 -0800 Subject: [PATCH 06/12] Remove metadata host property --- .../SubjectMetadataController.test.ts | 31 ------------------- .../SubjectMetadataController.ts | 15 --------- 2 files changed, 46 deletions(-) diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts index caf8945992..29fb968330 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts @@ -45,7 +45,6 @@ function getSubjectMetadata( origin, name, iconUrl: null, - host: null, extensionId: null, ...opts, }; @@ -183,36 +182,6 @@ describe('SubjectMetadataController', () => { }, }); }); - - it('adds "host" property for valid URL origin', () => { - const controller = new SubjectMetadataController({ - messenger: getSubjectMetadataControllerMessenger()[0], - subjectCacheLimit: 10, - }); - - controller.addSubjectMetadata({ origin: 'https://foo.com', name: 'foo' }); - expect(controller.state).toStrictEqual({ - subjectMetadata: { - 'https://foo.com': getSubjectMetadata('https://foo.com', 'foo', { - host: 'foo.com', - }), - }, - }); - }); - - it('logs error if failing to compute host from URL-like origin', () => { - const controller = new SubjectMetadataController({ - messenger: getSubjectMetadataControllerMessenger()[0], - subjectCacheLimit: 10, - }); - - controller.addSubjectMetadata({ origin: 'https://foo@', name: 'foo' }); - expect(controller.state).toStrictEqual({ - subjectMetadata: { - 'https://foo@': getSubjectMetadata('https://foo@', 'foo'), - }, - }); - }); }); describe('trimMetadataState', () => { diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts index b947932ae9..4a86bf5a8b 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts @@ -20,14 +20,12 @@ export type SubjectMetadata = PermissionSubjectMetadata & { name: string; // TODO:TS4.4 make optional extensionId: string | null; - host: string | null; iconUrl: string | null; }; type SubjectMetadataToAdd = PermissionSubjectMetadata & { name: string; extensionId?: string | null; - host?: string | null; iconUrl?: string | null; } & Record; @@ -148,7 +146,6 @@ export class SubjectMetadataController extends BaseController< const newMetadata: SubjectMetadata = { ...metadata, extensionId: metadata.extensionId || null, - host: metadata.host || null, iconUrl: metadata.iconUrl || null, }; @@ -168,18 +165,6 @@ export class SubjectMetadataController extends BaseController< this.subjectsEncounteredSinceStartup.add(origin); - if ( - !newMetadata.extensionId && - !newMetadata.host && - /[a-z]+:\/\/\w.+/iu.test(origin) - ) { - try { - newMetadata.host = new URL(origin).host; - } catch (_) { - // Do nothing. Some "origins" are not valid URLs. - } - } - this.update((draftState) => { // Typecast: ts(2589) draftState.subjectMetadata[origin] = newMetadata as any; From 62a6af43161e18a55b55b13e86464c809dba974f Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 24 Nov 2021 20:54:58 -0800 Subject: [PATCH 07/12] Simplify trimMetadataState --- .../SubjectMetadataController.ts | 50 ++++++++++--------- 1 file changed, 27 insertions(+), 23 deletions(-) diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts index 4a86bf5a8b..f76450177d 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts @@ -101,18 +101,13 @@ export class SubjectMetadataController extends BaseController< return messenger.call('PermissionController:hasPermissions', origin); }; - if (state.subjectMetadata) { - SubjectMetadataController.trimMetadataState( - state as SubjectMetadataControllerState, - hasPermissions, - ); - } - super({ name: controllerName, metadata: stateMetadata, messenger, - state: { ...defaultState, ...state }, + state: { + ...SubjectMetadataController.getTrimmedState(state, hasPermissions), + }, }); this.subjectHasPermissions = hasPermissions; @@ -179,7 +174,7 @@ export class SubjectMetadataController extends BaseController< */ trimMetadataState(): void { this.update((draftState) => { - SubjectMetadataController.trimMetadataState( + return SubjectMetadataController.getTrimmedState( // Typecast: ts(2589) draftState as any, this.subjectHasPermissions, @@ -188,25 +183,34 @@ export class SubjectMetadataController extends BaseController< } /** - * Deletes all subjects without permissions from the given state object. This - * method is static because we want to call it in the constructor, before the - * controller's state is initialized. + * Returns a new state object that only includes subjects with permissions. + * This method is static because we want to call it in the constructor, before + * the controller's state is initialized. * - * @param draftState - The `immer` draft state. + * @param state - The state object to trim. * @param hasPermissions - A function that returns a boolean indicating * whether a particular subject (identified by its origin) has any * permissions. + * @returns The new state object. If the specified `state` object has no + * subject metadata, the returned object will be equivalent to the default + * state of this controller. */ - private static trimMetadataState( - draftState: SubjectMetadataControllerState, + private static getTrimmedState( + state: Partial, hasPermissions: SubjectMetadataController['subjectHasPermissions'], - ): void { - const { subjectMetadata } = draftState; - - Object.keys(subjectMetadata).forEach((origin) => { - if (!hasPermissions(origin)) { - delete subjectMetadata[origin]; - } - }); + ): SubjectMetadataControllerState { + const { subjectMetadata = {} } = state; + + return { + subjectMetadata: Object.keys(subjectMetadata).reduce( + (newSubjectMetadata, origin) => { + if (hasPermissions(origin)) { + newSubjectMetadata[origin] = subjectMetadata[origin]; + } + return newSubjectMetadata; + }, + {} as Record, + ), + }; } } From fae450c68c048c1bdce96d3ec6d29c89a17ac028 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 24 Nov 2021 21:06:33 -0800 Subject: [PATCH 08/12] fixup! Simplify trimMetadataState --- .../SubjectMetadataController.ts | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts index f76450177d..483cad05ca 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts @@ -202,15 +202,14 @@ export class SubjectMetadataController extends BaseController< const { subjectMetadata = {} } = state; return { - subjectMetadata: Object.keys(subjectMetadata).reduce( - (newSubjectMetadata, origin) => { - if (hasPermissions(origin)) { - newSubjectMetadata[origin] = subjectMetadata[origin]; - } - return newSubjectMetadata; - }, - {} as Record, - ), + subjectMetadata: Object.keys(subjectMetadata).reduce< + Record + >((newSubjectMetadata, origin) => { + if (hasPermissions(origin)) { + newSubjectMetadata[origin] = subjectMetadata[origin]; + } + return newSubjectMetadata; + }, {}), }; } } From 6a729cc05e3fd7970338790df298ba96652e1658 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 24 Nov 2021 21:08:51 -0800 Subject: [PATCH 09/12] Rename subjectCacheLimit property name --- .../src/subject-metadata/SubjectMetadataController.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts index 483cad05ca..1872c4f26e 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts @@ -80,7 +80,7 @@ export class SubjectMetadataController extends BaseController< SubjectMetadataControllerState, SubjectMetadataControllerMessenger > { - private _subjectCacheLimit: number; + private subjectCacheLimit: number; private subjectsEncounteredSinceStartup: Set; @@ -111,7 +111,7 @@ export class SubjectMetadataController extends BaseController< }); this.subjectHasPermissions = hasPermissions; - this._subjectCacheLimit = subjectCacheLimit; + this.subjectCacheLimit = subjectCacheLimit; this.subjectsEncounteredSinceStartup = new Set(); } @@ -147,7 +147,7 @@ export class SubjectMetadataController extends BaseController< let originToForget: string | null = null; // We only delete the oldest encountered subject from the cache, again to // ensure that the user's experience isn't degraded by missing icons etc. - if (this.subjectsEncounteredSinceStartup.size >= this._subjectCacheLimit) { + if (this.subjectsEncounteredSinceStartup.size >= this.subjectCacheLimit) { const cachedOrigin = this.subjectsEncounteredSinceStartup .values() .next().value; From fb306e863066adc10233bc843ad1cf11ecf2326a Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 24 Nov 2021 21:26:43 -0800 Subject: [PATCH 10/12] Clear subjectsEncounteredSinceStartup in clearState --- .../SubjectMetadataController.test.ts | 15 ++++++++++----- .../subject-metadata/SubjectMetadataController.ts | 4 +++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts index 29fb968330..166ec04f52 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts @@ -106,18 +106,23 @@ describe('SubjectMetadataController', () => { const controller = new SubjectMetadataController({ messenger, subjectCacheLimit: 10, - state: { - subjectMetadata: { - 'foo.com': getSubjectMetadata('foo.com', 'foo'), - }, - }, }); + controller.addSubjectMetadata(getSubjectMetadata('foo.com', 'foo')); + expect(controller.state).toStrictEqual({ subjectMetadata: { 'foo.com': getSubjectMetadata('foo.com', 'foo') }, }); + + expect( + (controller as any).subjectsEncounteredSinceStartup.size, + ).toStrictEqual(1); + controller.clearState(); expect(controller.state).toStrictEqual({ subjectMetadata: {} }); + expect( + (controller as any).subjectsEncounteredSinceStartup.size, + ).toStrictEqual(0); }); }); diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts index 1872c4f26e..4d6ecb95a8 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts @@ -116,9 +116,11 @@ export class SubjectMetadataController extends BaseController< } /** - * Clears the state of this controller. + * Clears the state of this controller. Also resets the cache of subjects + * encountered since startup, so as to not prematurely reach the cache limit. */ clearState(): void { + this.subjectsEncounteredSinceStartup.clear(); this.update((_draftState) => { return { ...defaultState }; }); From 66d6eb5280692f3ed6da9082d3bd5e267d8d6625 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Thu, 25 Nov 2021 08:36:49 -0800 Subject: [PATCH 11/12] Fix clearState test case --- .../SubjectMetadataController.test.ts | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts index 166ec04f52..07475aa38f 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.test.ts @@ -98,31 +98,35 @@ describe('SubjectMetadataController', () => { }); describe('clearState', () => { - it('clears the controller state', () => { + it('clears the controller state, and continues to function normally afterwards', () => { const [messenger, hasPermissionsSpy] = getSubjectMetadataControllerMessenger(); - hasPermissionsSpy.mockImplementation(() => true); - const controller = new SubjectMetadataController({ messenger, - subjectCacheLimit: 10, + subjectCacheLimit: 3, }); - controller.addSubjectMetadata(getSubjectMetadata('foo.com', 'foo')); + // No subject will have permissions. + hasPermissionsSpy.mockImplementation(() => false); - expect(controller.state).toStrictEqual({ - subjectMetadata: { 'foo.com': getSubjectMetadata('foo.com', 'foo') }, - }); + // Add subjects up to the cache limit + controller.addSubjectMetadata(getSubjectMetadata('foo.com', 'foo')); + controller.addSubjectMetadata(getSubjectMetadata('bar.com', 'bar')); + controller.addSubjectMetadata(getSubjectMetadata('baz.com', 'baz')); - expect( - (controller as any).subjectsEncounteredSinceStartup.size, - ).toStrictEqual(1); + expect(Object.keys(controller.state.subjectMetadata)).toHaveLength(3); + // Clear the state controller.clearState(); - expect(controller.state).toStrictEqual({ subjectMetadata: {} }); - expect( - (controller as any).subjectsEncounteredSinceStartup.size, - ).toStrictEqual(0); + expect(Object.keys(controller.state.subjectMetadata)).toHaveLength(0); + + // Add another subject, which also does not have any permissions + controller.addSubjectMetadata(getSubjectMetadata('fizz.com', 'fizz')); + + // Observe that the subject was added normally + expect(controller.state).toStrictEqual({ + subjectMetadata: { 'fizz.com': getSubjectMetadata('fizz.com', 'fizz') }, + }); }); }); From d4cdb6812b39329b8881750edde3afc473589a96 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Thu, 25 Nov 2021 09:26:16 -0800 Subject: [PATCH 12/12] Rename internal variable --- .../SubjectMetadataController.ts | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts index 4d6ecb95a8..bf9033bf52 100644 --- a/packages/controllers/src/subject-metadata/SubjectMetadataController.ts +++ b/packages/controllers/src/subject-metadata/SubjectMetadataController.ts @@ -82,7 +82,7 @@ export class SubjectMetadataController extends BaseController< > { private subjectCacheLimit: number; - private subjectsEncounteredSinceStartup: Set; + private subjectsWithoutPermissionsEcounteredSinceStartup: Set; private subjectHasPermissions: GenericPermissionController['hasPermissions']; @@ -112,7 +112,7 @@ export class SubjectMetadataController extends BaseController< this.subjectHasPermissions = hasPermissions; this.subjectCacheLimit = subjectCacheLimit; - this.subjectsEncounteredSinceStartup = new Set(); + this.subjectsWithoutPermissionsEcounteredSinceStartup = new Set(); } /** @@ -120,7 +120,7 @@ export class SubjectMetadataController extends BaseController< * encountered since startup, so as to not prematurely reach the cache limit. */ clearState(): void { - this.subjectsEncounteredSinceStartup.clear(); + this.subjectsWithoutPermissionsEcounteredSinceStartup.clear(); this.update((_draftState) => { return { ...defaultState }; }); @@ -149,18 +149,24 @@ export class SubjectMetadataController extends BaseController< let originToForget: string | null = null; // We only delete the oldest encountered subject from the cache, again to // ensure that the user's experience isn't degraded by missing icons etc. - if (this.subjectsEncounteredSinceStartup.size >= this.subjectCacheLimit) { - const cachedOrigin = this.subjectsEncounteredSinceStartup + if ( + this.subjectsWithoutPermissionsEcounteredSinceStartup.size >= + this.subjectCacheLimit + ) { + const cachedOrigin = this.subjectsWithoutPermissionsEcounteredSinceStartup .values() .next().value; - this.subjectsEncounteredSinceStartup.delete(cachedOrigin); + this.subjectsWithoutPermissionsEcounteredSinceStartup.delete( + cachedOrigin, + ); + if (!this.subjectHasPermissions(cachedOrigin)) { originToForget = cachedOrigin; } } - this.subjectsEncounteredSinceStartup.add(origin); + this.subjectsWithoutPermissionsEcounteredSinceStartup.add(origin); this.update((draftState) => { // Typecast: ts(2589)