diff --git a/.circleci/config.yml b/.circleci/config.yml new file mode 100644 index 00000000..699863d5 --- /dev/null +++ b/.circleci/config.yml @@ -0,0 +1,17 @@ +version: 2.1 + +# Placeholder pipeline: this branch predates the real CircleCI config being added on another, +# not-yet-merged branch. This just gives CircleCI a valid config to parse so the pipeline +# succeeds instead of failing on a missing/empty config.yml. Replace once merged with the +# branch that introduces the real pipeline definition. +jobs: + noop: + docker: + - image: cimg/base:current + steps: + - run: echo "No-op CI config - real pipeline will be introduced when merged from its source branch." + +workflows: + noop-workflow: + jobs: + - noop \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 27a9cbb7..218ff79e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,23 @@ All notable changes to this project will be documented in this file. +## [0.8.0] - 2026-09-02 + +### `@okta/auth-foundation` + +#### Added +- Added `dispose()` to `Credential`, `OAuth2Client`, and `APIClient` to release listeners and cached resources when a credential is removed ([#39](https://github.com/okta/okta-client-javascript/pull/39)) +- Added an optional `{ signal: AbortSignal }` option to `EventEmitter.on()`, and a `clear()` method, for automatic listener cleanup ([#39](https://github.com/okta/okta-client-javascript/pull/39)) + +#### Fixed +- Fixed a memory leak where every constructed `Credential` added a listener to the shared `CredentialCoordinator` emitter that was never removed, retaining every `Credential` (and its `OAuth2Client`) for the lifetime of the page ([#39](https://github.com/okta/okta-client-javascript/pull/39)) +- `DefaultCredentialDataSource.remove()`/`.clear()` now dispose removed credentials instead of only removing them from the internal cache ([#39](https://github.com/okta/okta-client-javascript/pull/39)) + +### `@okta/spa-platform` + +#### Fixed +- Cross-tab credential sync no longer broadcasts full token payloads over `BroadcastChannel`; tabs now read the current value from storage, and only when they already reference the credential in question, reducing memory pressure across many open tabs ([#39](https://github.com/okta/okta-client-javascript/pull/39)) + ## [0.7.2] - 2026-04-09 ### `@okta/spa-platform` diff --git a/e2e/apps/redirect-model/src/component/Landing.tsx b/e2e/apps/redirect-model/src/component/Landing.tsx index 02614584..80b175a6 100644 --- a/e2e/apps/redirect-model/src/component/Landing.tsx +++ b/e2e/apps/redirect-model/src/component/Landing.tsx @@ -30,12 +30,19 @@ export function Landing () { setCredentialIds(allIDs); }; + const removeHandler = async ({ id }) => { + if (credential?.id === id) { + setCredential(null); + } + await updateHandler(); + } + const defaultHandler = ({ id }) => { setDefault(id); }; Credential.on('credential_added', updateHandler); - Credential.on('credential_removed', updateHandler); + Credential.on('credential_removed', removeHandler); Credential.on('cleared', updateHandler); Credential.on('default_changed', defaultHandler); @@ -45,7 +52,7 @@ export function Landing () { Credential.off('cleared', updateHandler); Credential.off('default_changed', defaultHandler); }; - }, [setCredentialIds, setCredential, setDefault]); + }, [credential, setCredentialIds, setCredential, setDefault]); const clear = async () => { await Credential.clear(); diff --git a/e2e/apps/redirect-model/src/component/Token.tsx b/e2e/apps/redirect-model/src/component/Token.tsx index ca512bba..8092c6ef 100644 --- a/e2e/apps/redirect-model/src/component/Token.tsx +++ b/e2e/apps/redirect-model/src/component/Token.tsx @@ -18,16 +18,14 @@ export function Token ({ credential }: { credential: Credential }) { } Credential.on('tags_updated', tagsHandler); + setToken(credential.token); + setTags(credential.tags); + return () => { Credential.off('credential_refreshed', handler); Credential.off('tags_updated', tagsHandler); }; - }, [setToken]); - - useEffect(() => { - setToken(credential.token); - setTags(credential.tags); - }, [credential]); + }, [credential, setToken, setTags]); const remove = async () => { await credential.remove(); diff --git a/package.json b/package.json index 375a76e4..f3996508 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@okta/okta-client-js", - "version": "0.7.2", + "version": "0.8.0", "private": true, "packageManager": "yarn@1.22.19", "engines": { diff --git a/packages/auth-foundation/package.json b/packages/auth-foundation/package.json index 0735a817..66b1daee 100644 --- a/packages/auth-foundation/package.json +++ b/packages/auth-foundation/package.json @@ -1,6 +1,6 @@ { "name": "@okta/auth-foundation", - "version": "0.7.2", + "version": "0.8.0", "type": "module", "main": "dist/esm/index.js", "module": "dist/esm/index.js", diff --git a/packages/auth-foundation/src/Credential/Credential.ts b/packages/auth-foundation/src/Credential/Credential.ts index be3a09d7..88d98809 100644 --- a/packages/auth-foundation/src/Credential/Credential.ts +++ b/packages/auth-foundation/src/Credential/Credential.ts @@ -20,10 +20,8 @@ import { CredentialError, OAuth2Error } from '../errors/index.ts'; type CredentialEvents = { - 'credential_added': { credential: Credential }; - 'credential_removed': { id: string }; 'tags_updated': { id: string, tags: string[] }; -} & Omit; +} & CredentialCoordinatorEvents; /** * Wrapper around a {@link Token.Token | Token}, providing methods to interact with Tokens without the hassle of managing them @@ -49,19 +47,22 @@ export class Credential implements RequestAuthorizer, JSONSerializable { // unbinds listeners of previous coordinator ( [ - 'credential_added', 'credential_removed', 'credential_refreshed', 'default_changed', 'cleared' + 'credential_added', + 'credential_removed', + 'credential_refreshed', + 'default_changed', + 'cleared', + 'metadata_updated' ] satisfies (keyof CredentialCoordinatorEvents)[] ).forEach((evt) => previousCoordinator.emitter.off(evt)); // binds listeners (and event relays) from coordinator to Credential.emitter - this.emitter.relay(this.coordinator.emitter, ['cleared', 'default_changed', 'credential_refreshed']); + this.emitter.relay(this.coordinator.emitter, [ + 'credential_added', 'credential_removed', 'cleared', 'default_changed', 'credential_refreshed' + ]); - this.coordinator.emitter.on('credential_added', ({ credential }) => { - this.emitter.emit('credential_added', { credential }); - }); - - this.coordinator.emitter.on('credential_removed', ({ id }) => { - this.emitter.emit('credential_removed', { id }); + this.coordinator.emitter.on('metadata_updated', async ({ id, metadata }) => { + this.emitter.emit('tags_updated', { id, tags: metadata?.tags ?? [] }); }); } @@ -87,6 +88,9 @@ export class Credential implements RequestAuthorizer, JSONSerializable { /** @internal */ protected _userInfo: UserInfo | undefined; + /** @internal */ + #controller = new AbortController(); + /** * @remarks * Do not use directly, use {@link store | Credential.store} instead @@ -321,6 +325,20 @@ export class Credential implements RequestAuthorizer, JSONSerializable { /////// public instances methods /////// + /** + * Cleans up resources associated with the Credential instance, so that it may be garbage collected. + * + * @remarks + * This method is meant to be used in conjunction with {@link CredentialDataSource.remove}. Calling this + * method on an active {@link Credential} may have significant consequences + * + * @internal + */ + public dispose () { + this.oauth2.dispose(); + this.#controller.abort('dispose'); + } + /** * Updates tags associated with {@link Credential} * @@ -383,14 +401,7 @@ export class Credential implements RequestAuthorizer, JSONSerializable { this.oauth2.emitter.on('token_did_refresh', ({ token }) => { if (Token.isEqual(token, this.token)) { return; } this.token = token; - }); - - // bind listener to Derived class instance - this.coordinator.emitter.on('metadata_updated', async ({ id, metadata }) => { - if (this.id === id) { - Credential.emitter.emit('tags_updated', { id, tags: metadata?.tags ?? [] }); - } - }); + }, { signal: this.#controller.signal }); } // oauth2 methods diff --git a/packages/auth-foundation/src/Credential/CredentialCoordinator.ts b/packages/auth-foundation/src/Credential/CredentialCoordinator.ts index d5c82be2..cd0cb787 100644 --- a/packages/auth-foundation/src/Credential/CredentialCoordinator.ts +++ b/packages/auth-foundation/src/Credential/CredentialCoordinator.ts @@ -24,12 +24,13 @@ function log (...args: any[]) {} export type CredentialCoordinatorEvents = { + 'credential_added': { credential?: Credential, id: string }; + 'credential_removed': { id: string } 'credential_expired': { credential: Credential }; 'credential_refreshed': { credential: Credential }; 'cleared': void; } -& Pick -& CredentialDataSourceEvents; +& Pick; /** * @public @interface @@ -138,13 +139,14 @@ export class CredentialCoordinatorImpl implements CredentialCoordinator { console.error('Failed to replace token after refresh'); } }); + + this.emitter.emit('credential_added', { credential, id: credential.id }); }); this.credentialDataSource.emitter.on('credential_removed', ({ id }) => { this.clearExpireEventTimeout(id); + this.emitter.emit('credential_removed', { id }); }); - - this.emitter.relay(this.credentialDataSource.emitter, ['credential_added', 'credential_removed']); } public get tokenStorage (): TokenStorage { diff --git a/packages/auth-foundation/src/Credential/CredentialDataSource.ts b/packages/auth-foundation/src/Credential/CredentialDataSource.ts index 2574b03f..d3e94b4b 100644 --- a/packages/auth-foundation/src/Credential/CredentialDataSource.ts +++ b/packages/auth-foundation/src/Credential/CredentialDataSource.ts @@ -34,6 +34,7 @@ export interface CredentialDataSource { * represents the provided {@link Token.Token | Token}. */ hasCredential (token: Token): boolean; + hasCredential (id: string): boolean; /** * Checks {@link CredentialDataSource} for an existing {@link Credential} instance which * represents the provided {@link Token.Token | Token}. If one does not exist, a new {@link Credential} @@ -80,8 +81,8 @@ export class DefaultCredentialDataSource implements CredentialDataSource { return new this.CredentialConstructor(token, client, metadata); } - public hasCredential (token: Token): boolean { - return this.credentials.has(token.id); + public hasCredential (key: string | Token): boolean { + return this.credentials.has(typeof key === 'string' ? key : key.id); } public credentialFor (token: Token, metadata?: Token.Metadata): Credential { @@ -101,12 +102,14 @@ export class DefaultCredentialDataSource implements CredentialDataSource { const id = typeof cred === 'string' ? cred : cred.id; if (this.credentials.has(id)) { const cred = this.credentials.get(id)!; + cred.dispose(); this.credentials.delete(id); this.emitter.emit('credential_removed', { dataSource: this, id: cred.id }); } } public clear () { + this.credentials.forEach(cred => cred.dispose()); this.credentials.clear(); } diff --git a/packages/auth-foundation/src/http/APIClient.ts b/packages/auth-foundation/src/http/APIClient.ts index ca72bf24..bf8a75fc 100644 --- a/packages/auth-foundation/src/http/APIClient.ts +++ b/packages/auth-foundation/src/http/APIClient.ts @@ -56,6 +56,19 @@ export abstract class APIClient { await this.dpopNonceCache.cacheNonce(this.getDPoPNonceCacheKey(request), nonce); } + /** + * Cleans up resources associated with the client instance, so that it may be garbage collected. + * + * > [!Warning] + * > **DO NOT** use this method on active clients. + * + * @internal + */ + public dispose () { + this.emitter.clear(); + this.interceptors.splice(0, this.interceptors.length); // clears array in place + } + /** * Registers an {@link APIClient.RequestInterceptor} on the {@link APIClient} * diff --git a/packages/auth-foundation/src/oauth2/client.ts b/packages/auth-foundation/src/oauth2/client.ts index 751ac27b..99bd1343 100644 --- a/packages/auth-foundation/src/oauth2/client.ts +++ b/packages/auth-foundation/src/oauth2/client.ts @@ -145,6 +145,14 @@ export class OAuth2Client e return json; } + /** + * Cleans up resourece associated with the client instance to prevent leaks. + */ + public dispose () { + super.dispose(); + this.#httpCache.clear(); + } + /** * Retrieves the Authorization Server's OpenID configuration */ diff --git a/packages/auth-foundation/src/utils/EventEmitter.ts b/packages/auth-foundation/src/utils/EventEmitter.ts index 6487d650..d5130766 100644 --- a/packages/auth-foundation/src/utils/EventEmitter.ts +++ b/packages/auth-foundation/src/utils/EventEmitter.ts @@ -8,6 +8,8 @@ type EventMap = { }; type EventListener = T extends void ? () => void : (event: T) => void; +type EventListenerOptions = { signal: AbortSignal } + /** * @group EventEmitter */ @@ -21,13 +23,39 @@ export interface Emitter { */ export class EventEmitter { listeners: { [K in keyof Events]?: Array> } = {}; + // scoped per-event, since the same `handler` function reference may be registered against + // multiple events (or reused across `on()` calls) with different signals attached + signals: Map void, { signal: AbortSignal, abortHandler: () => void }>> = new Map(); + + on( + eventName: K, + handler: EventListener, + options: Partial = {} + ): this { + const { signal } = options; + + if (signal?.aborted) { + // if the provided `AbortSignal` has already been aborted, do not bind listener + return this; + } - on(eventName: K, handler: EventListener): this { if (!this.listeners[eventName]) { this.listeners[eventName] = []; } this.listeners[eventName]!.push(handler); + if (signal) { + const abortHandler = () => { + this.off(eventName, handler); + }; + signal.addEventListener('abort', abortHandler, { once: true }); + + if (!this.signals.has(eventName)) { + this.signals.set(eventName, new WeakMap()); + } + this.signals.get(eventName)!.set(handler, { signal, abortHandler }); + } + return this; } @@ -37,14 +65,25 @@ export class EventEmitter { } if (!handler) { + this.listeners[eventName]?.forEach(h => this.detachSignal(eventName, h)); delete this.listeners[eventName]; return this; } + this.detachSignal(eventName, handler); this.listeners[eventName] = this.listeners[eventName]?.filter(l => l !== handler); return this; } + /** @internal removes the `AbortSignal` registration (if any) associated with `handler` for `eventName` */ + protected detachSignal (eventName: K, handler: EventListener): void { + const entry = this.signals.get(eventName)?.get(handler); + if (entry) { + entry.signal.removeEventListener('abort', entry.abortHandler); + this.signals.get(eventName)!.delete(handler); + } + } + emit(eventName: K, data: Events[K]): void; emit(eventName: K): void; emit(eventName: K, data?: Events[K]): void { @@ -82,4 +121,9 @@ export class EventEmitter { emitter.on(event, handler); } } -} \ No newline at end of file + + clear (): this { + (Object.keys(this.listeners) as (keyof Events)[]).forEach(eventName => this.off(eventName)); + return this; + } +} diff --git a/packages/auth-foundation/test/spec/Credential/Credential.spec.ts b/packages/auth-foundation/test/spec/Credential/Credential.spec.ts index af126c96..ea7e03b9 100644 --- a/packages/auth-foundation/test/spec/Credential/Credential.spec.ts +++ b/packages/auth-foundation/test/spec/Credential/Credential.spec.ts @@ -38,8 +38,8 @@ describe('Credential', () => { const token = makeTestToken(); let cred = await Credential.store(token); - expect(onAdded1).toHaveBeenNthCalledWith(1, { credential: cred }); - expect(onAdded2).toHaveBeenNthCalledWith(1, { credential: cred }); + expect(onAdded1).toHaveBeenNthCalledWith(1, { credential: cred, id: cred.id }); + expect(onAdded2).toHaveBeenNthCalledWith(1, { credential: cred, id: cred.id }); expect(onRemoved1).toHaveBeenCalledTimes(0); expect(onRemoved2).toHaveBeenCalledTimes(0); await cred.remove(); @@ -57,6 +57,42 @@ describe('Credential', () => { expect(onRemoved1).toHaveBeenCalledTimes(1); // was removed, should not be called again expect(onRemoved2).toHaveBeenNthCalledWith(2, { id: cred.id }); }); + + // OKTA-1262864 + describe('Memory leak regression', () => { + // leak: every constructed Credential added one more listener to the page-lifetime coordinator emitter, and nothing ever removed it. + // fix/test: Now, only a single listener should be bound to `metadata_updated`, independent of the number of constructed Credentials + + // confirms `metadata_updated` listeners does not grow as Credentials are constructed + it('should not accumulate `metadata_updated` listeners on the coordinator emitter across store/remove cycles', async () => { + const emitter = (Credential as any).coordinator.emitter; + const baseline = emitter.listeners['metadata_updated']?.length ?? 0; + + for (let i = 0; i < 25; i++) { + const cred = await Credential.store(makeTestToken()); + await cred.remove(); + } + + expect(emitter.listeners['metadata_updated']?.length ?? 0).toBe(baseline); + }); + + // confirms the single `metadata_updated` listener is maintained when updating `coordinator` instance (coordinator setter) + it('binds exactly one metadata_updated listener via the coordinator setter during reassignment', () => { + const previousCoordinator = (Credential as any).coordinator; + const newCoordinator = new previousCoordinator.constructor(Credential); + + (Credential as any).coordinator = newCoordinator; + + try { + expect(previousCoordinator.emitter.listeners['metadata_updated']).toBeFalsy(); + expect(newCoordinator.emitter.listeners['metadata_updated']?.length).toBe(1); + } + finally { + // restore original coordinator so later tests aren't affected + (Credential as any).coordinator = previousCoordinator; + } + }); + }); }); describe('getters/setters', () => { @@ -154,12 +190,14 @@ describe('Credential', () => { it('clear', async () => { expect(Credential.size).toEqual(0); - await Credential.store(makeTestToken()); - await Credential.store(makeTestToken()); - await Credential.store(makeTestToken()); + const c1 = await Credential.store(makeTestToken()); + const c2 = await Credential.store(makeTestToken()); + const c3 = await Credential.store(makeTestToken()); + const disposeSpies = [c1, c2, c3].map(c => jest.spyOn(c, 'dispose')); expect(Credential.size).toEqual(3); await Credential.clear(); expect(Credential.size).toEqual(0); + disposeSpies.forEach(spy => expect(spy).toHaveBeenCalledTimes(1)); }); it('isEqual', async () => { @@ -239,15 +277,21 @@ describe('Credential', () => { it('remove', async () => { const c1 = await Credential.store(makeTestToken()); const c2 = await Credential.store(makeTestToken()); + const c1DisposeSpy = jest.spyOn(c1, 'dispose'); + const c2DisposeSpy = jest.spyOn(c2, 'dispose'); expect(Credential.size).toEqual(2); await c1.remove(); await expect(Credential.with(c1.id)).resolves.toBe(null); expect(Credential.size).toEqual(1); + expect(c1DisposeSpy).toHaveBeenCalledTimes(1); await c1.remove(); // remove c1 again, no ops expect(Credential.size).toEqual(1); + expect(c1DisposeSpy).toHaveBeenCalledTimes(1); // not called again on the no-op + expect(c2DisposeSpy).not.toHaveBeenCalled(); await c2.remove(); await expect(Credential.with(c2.id)).resolves.toBe(null); expect(Credential.size).toEqual(0); + expect(c2DisposeSpy).toHaveBeenCalledTimes(1); }); it('getAuthHeader', async () => { diff --git a/packages/auth-foundation/test/spec/Credential/CredentialCoordinatorImpl.spec.ts b/packages/auth-foundation/test/spec/Credential/CredentialCoordinatorImpl.spec.ts index 676f1946..88b58497 100644 --- a/packages/auth-foundation/test/spec/Credential/CredentialCoordinatorImpl.spec.ts +++ b/packages/auth-foundation/test/spec/Credential/CredentialCoordinatorImpl.spec.ts @@ -119,7 +119,7 @@ describe('CredentialCoordinatorImpl', () => { expect(cc.tokenStorage.defaultTokenId).toEqual(cred.id); expect(cred.tags).toEqual(['test']); expect(onAdded).toHaveBeenCalledTimes(1); - expect(onAdded).toHaveBeenCalledWith({ credential: cred, dataSource: cc.credentialDataSource }); + expect(onAdded).toHaveBeenCalledWith({ credential: cred, id: cred.id }); }); it('with', async () => { @@ -173,7 +173,7 @@ describe('CredentialCoordinatorImpl', () => { expect(cc.credentialDataSource.hasCredential(c1)).toEqual(false); expect(cc.size).toEqual(3); expect(clearExpireTimeoutSpy).toHaveBeenNthCalledWith(1, c1.id); - expect(onRemove).toHaveBeenNthCalledWith(1, { id: c1.id, dataSource: cc.credentialDataSource }); + expect(onRemove).toHaveBeenNthCalledWith(1, { id: c1.id }); expect(onDefaultChanged).toHaveBeenCalledTimes(0); await cc.remove(c2); // remove default credenital @@ -181,7 +181,7 @@ describe('CredentialCoordinatorImpl', () => { expect(cc.credentialDataSource.hasCredential(c2)).toEqual(false); expect(cc.size).toEqual(2); expect(clearExpireTimeoutSpy).toHaveBeenNthCalledWith(2, c2.id); - expect(onRemove).toHaveBeenNthCalledWith(2, { id: c2.id, dataSource: cc.credentialDataSource }); + expect(onRemove).toHaveBeenNthCalledWith(2, { id: c2.id }); expect(onDefaultChanged).toHaveBeenNthCalledWith(1, { id: null, storage: cc.tokenStorage }); }); diff --git a/packages/auth-foundation/test/spec/Credential/DefaultCredentialDataSource.spec.ts b/packages/auth-foundation/test/spec/Credential/DefaultCredentialDataSource.spec.ts index fe53a559..8b8c6671 100644 --- a/packages/auth-foundation/test/spec/Credential/DefaultCredentialDataSource.spec.ts +++ b/packages/auth-foundation/test/spec/Credential/DefaultCredentialDataSource.spec.ts @@ -69,20 +69,25 @@ describe('DefaultCredentialDataSource', () => { it('remove', () => { const { c1, dataSrc } = context; + const disposeSpy = jest.spyOn(c1, 'dispose'); expect(dataSrc.size).toEqual(3); expect(dataSrc.hasCredential(c1)).toEqual(true); dataSrc.remove(c1); expect(dataSrc.size).toEqual(2); expect(dataSrc.hasCredential(c1)).toEqual(false); + expect(disposeSpy).toHaveBeenCalledTimes(1); dataSrc.remove(c1); // removing non-existing Credential no-ops expect(dataSrc.size).toEqual(2); + expect(disposeSpy).toHaveBeenCalledTimes(1); // not called again on the no-op }); it('clear', () => { - const { dataSrc } = context; + const { c1, c2, c3, dataSrc } = context; + const disposeSpies = [c1, c2, c3].map(c => jest.spyOn(c, 'dispose')); expect(dataSrc.size).toEqual(3); dataSrc.clear(); expect(dataSrc.size).toEqual(0); + disposeSpies.forEach(spy => expect(spy).toHaveBeenCalledTimes(1)); }); it('size', () => { diff --git a/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts b/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts index 89af3ed9..0ef4d251 100644 --- a/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts +++ b/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts @@ -7,7 +7,6 @@ describe('EventEmitter', () => { const listener2 = jest.fn(); // emit event with no registered handlers at all - // @ts-expect-error `.emit` is protected method emitter.emit('bar', { foo: 'foo '}); // register handlers @@ -15,13 +14,11 @@ describe('EventEmitter', () => { emitter.on('foo', listener2); // emit event with no handlers registered for specific event - // @ts-expect-error `.emit` is protected method emitter.emit('bar', { foo: 'foo '}); expect(listener1).not.toHaveBeenCalled(); expect(listener2).not.toHaveBeenCalled(); // emit event with registered handlers - // @ts-expect-error `.emit` is protected method emitter.emit('foo', { bar: 'bar' }); expect(listener1).toHaveBeenCalledWith({ bar: 'bar' }); expect(listener2).toHaveBeenCalledWith({ bar: 'bar' }); @@ -32,7 +29,6 @@ describe('EventEmitter', () => { listener2.mockClear(); // emit event again (with a single registered handler) - // @ts-expect-error `.emit` is protected method emitter.emit('foo', { baz: 'baz' }); expect(listener1).not.toHaveBeenCalled(); expect(listener2).toHaveBeenCalledWith({ baz: 'baz' }); @@ -41,7 +37,6 @@ describe('EventEmitter', () => { emitter.on('foo', listener1); listener1.mockClear(); listener2.mockClear(); - // @ts-expect-error `.emit` is protected method emitter.emit('foo', { bar: 'bar' }); expect(listener1).toHaveBeenCalledWith({ bar: 'bar' }); expect(listener2).toHaveBeenCalledWith({ bar: 'bar' }); @@ -50,7 +45,6 @@ describe('EventEmitter', () => { listener1.mockClear(); listener2.mockClear(); emitter.off('foo'); - // @ts-expect-error `.emit` is protected method emitter.emit('foo', { bar: 'bar' }); expect(listener1).not.toHaveBeenCalled(); expect(listener2).not.toHaveBeenCalled(); @@ -64,11 +58,9 @@ describe('EventEmitter', () => { outer.on('test_event', listener); outer.relay(inner, ['test_event']); - // @ts-expect-error `.emit` is protected method inner.emit('foo', { bar: 'baz' }); expect(listener).not.toHaveBeenCalled(); - // @ts-expect-error `.emit` is protected method inner.emit('test_event', { bar: 'baz' }); expect(listener).toHaveBeenCalledWith({ bar: 'baz' }); }); @@ -90,9 +82,153 @@ describe('EventEmitter', () => { emitter.on('foo', handler1); emitter.on('foo', handler2); - // @ts-expect-error `.emit` is protected method emitter.emit('foo', { bar: 'baz' }); expect(handler2).toHaveBeenCalled(); }); + + describe('AbortSignal support (`{ signal }` option on `.on()`)', () => { + it('removes the listener once the signal is aborted', () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + const listener = jest.fn(); + + emitter.on('foo', listener, { signal: controller.signal }); + emitter.emit('foo', { bar: 'baz' }); + expect(listener).toHaveBeenCalledTimes(1); + + controller.abort(); + emitter.emit('foo', { bar: 'baz' }); + expect(listener).toHaveBeenCalledTimes(1); // not called again after abort + }); + + it('removes every listener sharing the same signal when it is aborted', () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + const listener1 = jest.fn(); + const listener2 = jest.fn(); + + emitter.on('foo', listener1, { signal: controller.signal }); + emitter.on('bar', listener2, { signal: controller.signal }); + + controller.abort(); + + emitter.emit('foo', {}); + emitter.emit('bar', {}); + expect(listener1).not.toHaveBeenCalled(); + expect(listener2).not.toHaveBeenCalled(); + }); + + it('does not register a listener if the signal is already aborted', () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + controller.abort(); + const listener = jest.fn(); + + emitter.on('foo', listener, { signal: controller.signal }); + emitter.emit('foo', { bar: 'baz' }); + expect(listener).not.toHaveBeenCalled(); + }); + + it('tears down the abort listener when `.off()` is called directly, not just on abort', () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + const listener = jest.fn(); + const removeEventListenerSpy = jest.spyOn(controller.signal, 'removeEventListener'); + + emitter.on('foo', listener, { signal: controller.signal }); + emitter.off('foo', listener); + expect(removeEventListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function)); + + // aborting afterward should not throw, nor re-invoke the already-removed listener + expect(() => controller.abort()).not.toThrow(); + emitter.emit('foo', {}); + expect(listener).not.toHaveBeenCalled(); + }); + + it('tears down signal registrations for every handler when `.off(event)` is called with no handler', () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + const listener1 = jest.fn(); + const listener2 = jest.fn(); + const removeEventListenerSpy = jest.spyOn(controller.signal, 'removeEventListener'); + + emitter.on('foo', listener1, { signal: controller.signal }); + emitter.on('foo', listener2, { signal: controller.signal }); + + emitter.off('foo'); + expect(removeEventListenerSpy).toHaveBeenCalledTimes(2); + + expect(() => controller.abort()).not.toThrow(); + emitter.emit('foo', {}); + expect(listener1).not.toHaveBeenCalled(); + expect(listener2).not.toHaveBeenCalled(); + }); + + it('does not require a `signal` option', () => { + const emitter = new EventEmitter(); + const listener = jest.fn(); + expect(() => emitter.on('foo', listener)).not.toThrow(); + emitter.emit('foo', {}); + expect(listener).toHaveBeenCalledTimes(1); + }); + + it('does not cross-contaminate signal cleanup when the same handler is shared across different events', () => { + const emitter = new EventEmitter(); + const controllerA = new AbortController(); + const controllerB = new AbortController(); + const sharedHandler = jest.fn(); + const removeEventListenerSpyA = jest.spyOn(controllerA.signal, 'removeEventListener'); + const removeEventListenerSpyB = jest.spyOn(controllerB.signal, 'removeEventListener'); + + emitter.on('foo', sharedHandler, { signal: controllerA.signal }); + emitter.on('bar', sharedHandler, { signal: controllerB.signal }); + + // explicitly detach only the 'foo' registration + emitter.off('foo', sharedHandler); + expect(removeEventListenerSpyA).toHaveBeenCalledWith('abort', expect.any(Function)); + expect(removeEventListenerSpyB).not.toHaveBeenCalled(); // 'bar's own signal registration must be untouched + + // 'bar' should still be live... + emitter.emit('bar', {}); + expect(sharedHandler).toHaveBeenCalledTimes(1); + + // ...and still correctly cleaned up when ITS OWN signal aborts + controllerB.abort(); + emitter.emit('bar', {}); + expect(sharedHandler).toHaveBeenCalledTimes(1); // not called again + + // aborting the already-detached controllerA afterward should not throw or double-invoke anything + expect(() => controllerA.abort()).not.toThrow(); + }); + }); + + describe('clear', () => { + it('removes every listener for every event', () => { + const emitter = new EventEmitter(); + const foo = jest.fn(); + const bar = jest.fn(); + emitter.on('foo', foo); + emitter.on('bar', bar); + + emitter.clear(); + + emitter.emit('foo', {}); + emitter.emit('bar', {}); + expect(foo).not.toHaveBeenCalled(); + expect(bar).not.toHaveBeenCalled(); + }); + + it('tears down any signal registrations for the listeners it clears', () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + const listener = jest.fn(); + const removeEventListenerSpy = jest.spyOn(controller.signal, 'removeEventListener'); + + emitter.on('foo', listener, { signal: controller.signal }); + emitter.clear(); + + expect(removeEventListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function)); + }); + }); }); diff --git a/packages/oauth2-flows/package.json b/packages/oauth2-flows/package.json index 72b47f3a..5f113670 100644 --- a/packages/oauth2-flows/package.json +++ b/packages/oauth2-flows/package.json @@ -1,6 +1,6 @@ { "name": "@okta/oauth2-flows", - "version": "0.7.2", + "version": "0.8.0", "type": "module", "main": "dist/esm/index.js", "module": "dist/esm/index.js", diff --git a/packages/spa-platform/package.json b/packages/spa-platform/package.json index 1af964c1..d0d9c980 100644 --- a/packages/spa-platform/package.json +++ b/packages/spa-platform/package.json @@ -1,6 +1,6 @@ { "name": "@okta/spa-platform", - "version": "0.7.2", + "version": "0.8.0", "type": "module", "main": "dist/esm/index.js", "module": "dist/esm/index.js", diff --git a/packages/spa-platform/src/Credential/CredentialCoordinator.ts b/packages/spa-platform/src/Credential/CredentialCoordinator.ts index b3fc4d59..cb3d8d9a 100644 --- a/packages/spa-platform/src/Credential/CredentialCoordinator.ts +++ b/packages/spa-platform/src/Credential/CredentialCoordinator.ts @@ -7,8 +7,7 @@ import type { TokenStorage, JsonPrimitive, TokenStorageEvents, - JsonRecord, - TokenInit, + JsonRecord } from '@okta/auth-foundation/core'; import { Token, @@ -30,7 +29,7 @@ import { isFirefox } from '../utils/UserAgent.ts'; function log (...args: any[]) {} -type BroadcastMessage = { eventName: string, id: string, source: string, value: JsonRecord }; +type BroadcastMessage = { eventName: string, id: string, source: string }; /** * Browser-specific implementation of {@link CredentialCoordinator} @@ -50,8 +49,7 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme this.registerTabListeners(); this.emitter.on('credential_refreshed', ({ credential }) => { - const { token } = credential; - this.broadcast('credential_refreshed', { id: token.id, value: token.toJSON() }); + this.broadcast('credential_refreshed', { id: credential.id }); }); } @@ -73,7 +71,7 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme super.tokenStorage = tokenStorage; this.tokenStorage.emitter.on('token_added', ({ token }) => { - this.broadcast('credential_added', { id: token.id, value: token.toJSON() }); + this.broadcast('credential_added', { id: token.id }); }); this.tokenStorage.emitter.on('token_removed', ({ id }) => { @@ -112,7 +110,7 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme await pause(50); } - const { eventName, id, value, source } = event.data as BroadcastMessage; + const { eventName, id, source } = event.data as BroadcastMessage; log('tab sync event: ', { eventName, source }); if (source == this.id) { return; // do not listen to messages broadcasted by this instance @@ -136,45 +134,49 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme this.emitter.emit('metadata_updated', { storage: this.tokenStorage, id, metadata }); } } - else { - // TODO: confirm client info - const token = new Token({ ...value, id } as TokenInit); - - if (eventName === 'credential_removed') { - log('removal'); - if (this.credentialDataSource.hasCredential(token)) { - this.credentialDataSource.remove(id); - } - else { - // TODO: is this needed? - // ensures removal event is broadcast, regardless of the DataSource knowledge of the Credential - this.emitter.emit('credential_removed', { dataSource: this.credentialDataSource, id }); - } + else if (eventName === 'credential_added') { + log('added'); + this.emitter.emit('credential_added', { id }); + // NOTE: cross-tab 'credential_added' no longer defaults to adding a `Credential` instance to `dataSource` + } + else if (eventName === 'credential_removed') { + log('removal'); + if (this.credentialDataSource.hasCredential(id)) { + // if a `Credential` exists for the given token, a event will be relayed via `dataSource.emitter` + this.credentialDataSource.remove(id); } else { - const credential = this.credentialDataSource.credentialFor(token); - - if (eventName === 'credential_added') { - log('added'); - // credentialDataSource.credentialFor() call above handles updating credDataSrc - } - else if (eventName === 'credential_refreshed') { - log('refresh'); - - // when a Credential is updated in a separate tab, the Token passed via the broadcast - // may differ from cred.token via DataSource, so the update should continue. - // If the tokens are equal, this means this DataSource has already updated the token to the new value - // eslint-disable-next-line max-depth - if (Token.isEqual(token, credential.token)) { - log('token has already been updated'); - return; - } - - // @ts-expect-error - Credential `set token()` is a private setter to avoid exposing this to the public API - credential.token = token; - this.emitter.emit('credential_refreshed', { credential }); - } + // No event will be relayed if a `Credential` exists does not exist, emit one directly + this.emitter.emit('credential_removed', { id }); + } + } + else if (eventName === 'credential_refreshed') { + log('refresh'); + + // if the tab receiving this event does not "know" (have a corresponding `Credential` instance) + // for the token which refresh, skip processing this event + if (!this.credentialDataSource.hasCredential(id)) { + log('token not known to tab'); + return; } + + const token = await this.tokenStorage.get(id); + if (!token) { + return; + } + const credential = this.credentialDataSource.credentialFor(token); + + // when a Credential is updated in a separate tab, the Token read from storage + // may differ from cred.token via DataSource, so the update should continue. + // If the tokens are equal, this means this DataSource has already updated the token to the new value + if (Token.isEqual(token, credential.token)) { + log('token has already been updated'); + return; + } + + // @ts-expect-error - Credential `set token()` is a private setter to avoid exposing this to the public API + credential.token = token; + this.emitter.emit('credential_refreshed', { credential }); } log('allIDs: ', this.allIDs(), 'size: ', this.credentialDataSource.size); diff --git a/packages/spa-platform/src/Credential/TokenStorage.ts b/packages/spa-platform/src/Credential/TokenStorage.ts index 04206eb3..8192acf1 100644 --- a/packages/spa-platform/src/Credential/TokenStorage.ts +++ b/packages/spa-platform/src/Credential/TokenStorage.ts @@ -337,6 +337,7 @@ export class BrowserTokenStorage implements TokenStorage { protected async handleReadError (error: unknown, id: string) { // remove token if json structure is malformed localStorage.removeItem(this.idToStoreKey(id)); + this.emitter.emit('token_removed', { storage: this, id }); return null; } @@ -347,6 +348,7 @@ export class BrowserTokenStorage implements TokenStorage { // if token cannot be decrypted, remove it from storage localStorage.removeItem(this.idToStoreKey(id)); await this.removeEncryptionKeyIfEmpty(); + this.emitter.emit('token_removed', { storage: this, id }); return null; } diff --git a/packages/spa-platform/test/spec/BrowserTokenStorage.spec.ts b/packages/spa-platform/test/spec/BrowserTokenStorage.spec.ts index d97f667e..17de4ca0 100644 --- a/packages/spa-platform/test/spec/BrowserTokenStorage.spec.ts +++ b/packages/spa-platform/test/spec/BrowserTokenStorage.spec.ts @@ -1,4 +1,4 @@ -import { Token, CredentialError } from '@okta/auth-foundation'; +import { Token, CredentialError, randomBytes } from '@okta/auth-foundation'; import { BrowserTokenStorage } from 'src/Credential/TokenStorage'; import { makeTestToken, MockIndexedDBStore } from '../helpers/makeTestResource'; @@ -210,6 +210,9 @@ describe('BrowserTokenStorage', () => { }); // NOTE: potentially flaky test + // generating test tokens via `makeTestToken(randomBytes())` seem to help with the flakiness. + // `token.id` is used as the "iv" in `.encrypt({ name: 'AES-GCM', iv: buf(iv) }, ...)` calls + // it seems 'AES-GCM' "typically expects a IV of exactly 12 bytes", the default `shortId()` was not it('encrypts and decrypts tokens in/out of storage', async () => { const expectedKey = { type: 'secret', @@ -220,13 +223,13 @@ describe('BrowserTokenStorage', () => { await expect(storage.encryptionKeyStore.get(storage.encryptionKeyName)).resolves.toBe(null); - const t1 = makeTestToken(); + const t1 = makeTestToken(randomBytes()); await storage.add(t1); // cannot assert .instanceOf(CryptoKey) - Jest throws 'CryptoKey' not defined await expect(storage.encryptionKeyStore.get(storage.encryptionKeyName)).resolves.toMatchObject(expectedKey); - const t2 = makeTestToken(); + const t2 = makeTestToken(randomBytes()); await storage.add(t2); const t1Stored = JSON.parse(localStorage.getItem((storage as any).idToStoreKey(t1.id))!).token; @@ -244,14 +247,14 @@ describe('BrowserTokenStorage', () => { }); it('can gracefully handle `encryptedAtRest` flag being toggled', async () => { - const encryptedToken = makeTestToken(); + const encryptedToken = makeTestToken(randomBytes()); await storage.add(encryptedToken); await expect(storage.get(encryptedToken.id)).resolves.toEqual(encryptedToken); storage.encryptAtRest = false; await expect(storage.get(encryptedToken.id)).resolves.toEqual(encryptedToken); - const unencryptedToken = makeTestToken(); + const unencryptedToken = makeTestToken(randomBytes()); await storage.add(unencryptedToken); await expect(storage.get(unencryptedToken.id)).resolves.toEqual(unencryptedToken); @@ -267,8 +270,8 @@ describe('BrowserTokenStorage', () => { await expect(storage.get(unencryptedToken.id)).resolves.toEqual(unencryptedToken); }); - it('removes token from storage when decryption fails, is found', async () => { - const token = makeTestToken(); + it('removes token from storage when decryption fails', async () => { + const token = makeTestToken(randomBytes()); await storage.add(token); await expect(storage.allIDs()).resolves.toEqual([token.id]); diff --git a/packages/spa-platform/test/spec/Credential/CredentialCoordinatorImpl.spec.ts b/packages/spa-platform/test/spec/Credential/CredentialCoordinatorImpl.spec.ts new file mode 100644 index 00000000..5dc22346 --- /dev/null +++ b/packages/spa-platform/test/spec/Credential/CredentialCoordinatorImpl.spec.ts @@ -0,0 +1,229 @@ +import { CredentialCoordinatorImpl } from 'src/Credential/CredentialCoordinator'; +import { Credential } from 'src/Credential'; +import { BrowserTokenStorage } from 'src/Credential/TokenStorage'; +import { makeTestToken } from '../../helpers/makeTestResource'; + + +describe('CredentialCoordinatorImpl', () => { + let cc: CredentialCoordinatorImpl; + let channel: any; + + // simulates an incoming cross-tab BroadcastChannel message, since BroadcastChannel is mocked + function receive (eventName: string, data: Record = {}, source = 'other-tab') { + return channel.onmessage({ data: { eventName, source, ...data } }); + } + + beforeEach(() => { + // required to prevent open handles: `store()` creates an expiration timer (inherited from the base class) + jest.useFakeTimers(); + cc = new CredentialCoordinatorImpl(Credential); + Credential.coordinator = cc; + channel = (cc as any).channel; + (cc.tokenStorage as BrowserTokenStorage).encryptAtRest = false; // disables crypto/indexedDB requirements + }); + + afterEach(() => { + jest.clearAllTimers(); + }); + + describe('Instantiate', () => { + it('should construct', () => { + expect(cc).toBeInstanceOf(CredentialCoordinatorImpl); + }); + }); + + describe('Broadcast local Credential* events cross-tab', () => { + describe('Events', () => { + test('credential_added', async () => { + const cred = await cc.store(makeTestToken()); + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'credential_added', + source: expect.any(String), + id: cred.id + }); + }); + + test('credential_refreshed', async () => { + const cred = await cc.store(makeTestToken()); + channel.postMessage.mockClear(); + + const newToken = makeTestToken(cred.id); + jest.spyOn(cred.oauth2, 'refresh').mockResolvedValue(newToken); + // mocking `oauth2.refresh` means the `token_did_refresh` is not fired, emitting manually for test + cred.oauth2.emitter.emit('token_did_refresh', { token: newToken }); + await cred.refresh(); + + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'credential_refreshed', + source: expect.any(String), + id: cred.id + }); + }); + + test('credential_removed', async () => { + const cred = await cc.store(makeTestToken()); + channel.postMessage.mockClear(); + + await cc.remove(cred); + + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'credential_removed', + source: expect.any(String), + id: cred.id + }); + }); + + test('default_changed', async () => { + const cred = await cc.store(makeTestToken()); + channel.postMessage.mockClear(); + + await cc.setDefault(cred); + + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'default_changed', + source: expect.any(String), + id: cred.id + }); + }); + + test('metadata_updated', async () => { + const cred = await cc.store(makeTestToken()); + + await cred.setTags(['foo']); + + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'metadata_updated', + source: expect.any(String), + id: cred.id + }); + }); + + test('cleared', async () => { + // does not broadcast when `localOnly` = true + await cc.clear(true); + expect(channel.postMessage).not.toHaveBeenCalledWith(expect.objectContaining({ eventName: 'cleared' })); + + // broadcasts by default (when `localOnly` = false) + await cc.clear(); + expect(channel.postMessage).toHaveBeenCalledWith({ eventName: 'cleared', source: expect.any(String) }); + }); + }); + + it('detaches broadcast listeners from a replaced tokenStorage', () => { + const oldStorage = cc.tokenStorage; + cc.tokenStorage = new BrowserTokenStorage(); + channel.postMessage.mockClear(); + + oldStorage.emitter.emit('token_added', { storage: oldStorage, id: 'foo', token: makeTestToken() }); + + expect(channel.postMessage).not.toHaveBeenCalled(); + }); + }); + + describe('Receiving cross-tab messages', () => { + describe('Events', () => { + test('credential_added', async () => { + const addedSpy = jest.fn(); + cc.emitter.on('credential_added', addedSpy); + + await receive('credential_added', { id: 'foo' }); + + expect(addedSpy).toHaveBeenCalledWith({ id: 'foo' }); + expect(cc.credentialDataSource.hasCredential('foo')).toBe(false); + }); + + describe('credential_refreshed', () => { + it('applies the freshly stored token and emits credential_refreshed when tokens differ', async () => { + const cred = await cc.store(makeTestToken()); + const refreshed = makeTestToken(cred.id); + await cc.tokenStorage.replace(cred.id, refreshed); // simulates another tab's refresh landing in shared storage + const refreshedSpy = jest.fn(); + cc.emitter.on('credential_refreshed', refreshedSpy); + + await receive('credential_refreshed', { id: cred.id }); + + expect(cred.token).toEqual(refreshed); + expect(refreshedSpy).toHaveBeenCalledWith({ credential: cred }); + }); + + it('does not re-emit credential_refreshed if the stored token already matches', async () => { + const cred = await cc.store(makeTestToken()); + const refreshedSpy = jest.fn(); + cc.emitter.on('credential_refreshed', refreshedSpy); + + await receive('credential_refreshed', { id: cred.id }); // storage already matches cred.token + + expect(refreshedSpy).not.toHaveBeenCalled(); + }); + }); + + test('credential_removed', async () => { + const removedSpy = jest.fn(); + cc.emitter.on('credential_removed', removedSpy); + + const cred = await cc.store(makeTestToken()); + expect(cc.credentialDataSource.hasCredential(cred)).toBe(true); + + // simulates removing Credential **not** present in `DataSource` + receive('credential_removed', { id: 'never-seen' }); + expect(cc.credentialDataSource.hasCredential(cred)).toBe(true); + expect(removedSpy).toHaveBeenLastCalledWith({ id: 'never-seen' }); // `credential_removed` is still emitted + + // simulates removing Credential which **is** present in `DataSource` + await receive('credential_removed', { id: cred.id }); + expect(cc.credentialDataSource.hasCredential(cred)).toBe(false); + expect(removedSpy).toHaveBeenLastCalledWith({ id: cred.id }); + }); + + test('default_changed', async () => { + const cred = await cc.store(makeTestToken()); + const defaultChangedSpy = jest.fn(); + cc.emitter.on('default_changed', defaultChangedSpy); + + await receive('default_changed', { id: cred.id }); + + expect(defaultChangedSpy).toHaveBeenCalledWith({ storage: cc.tokenStorage, id: cred.id }); + }); + + test('metadata_updated', async () => { + const cred = await cc.store(makeTestToken(), ['foo']); + const metadataSpy = jest.fn(); + cc.emitter.on('metadata_updated', metadataSpy); + + await receive('metadata_updated', { id: cred.id }); + + expect(metadataSpy).toHaveBeenCalledWith(expect.objectContaining({ id: cred.id })); + }); + + test('cleared', async () => { + await cc.store(makeTestToken()); + channel.postMessage.mockClear(); + const clearedSpy = jest.fn(); + cc.emitter.on('cleared', clearedSpy); + + await receive('cleared'); + + expect(cc.size).toEqual(0); + expect(clearedSpy).toHaveBeenCalled(); + expect(channel.postMessage).not.toHaveBeenCalledWith({ eventName: 'cleared', source: expect.any(String) }); + }); + }); + + it('will not process messages it broadcasts', async () => { + const ownId = (cc as any).id; + const addedSpy = jest.fn(); + cc.emitter.on('credential_added', addedSpy); + + await receive('credential_added', { id: 'foo' }, ownId); + + expect(addedSpy).not.toHaveBeenCalled(); + }); + }); + + describe('close', () => { + it('closes the underlying BroadcastChannel', () => { + cc.close(); + expect(channel.close).toHaveBeenCalledTimes(1); + }); + }); +}); \ No newline at end of file