From 93595d265234cd149ff76dbac20e3e1031c3ef5f Mon Sep 17 00:00:00 2001 From: AJ Ancheta <7781450+ancheetah@users.noreply.github.com> Date: Mon, 22 Sep 2025 18:13:16 -0400 Subject: [PATCH] chore(storage): improve storage client type --- .changeset/slow-teeth-melt.md | 7 + .../oidc-client/src/lib/client.store.test.ts | 2 + packages/oidc-client/src/lib/client.store.ts | 8 +- packages/oidc-client/src/lib/client.types.ts | 15 +- .../src/lib/logout.request.test.ts | 3 + .../oidc-client/src/lib/logout.request.ts | 16 +- .../storage/src/lib/storage.effects.test.ts | 140 ++++++++++++------ .../storage/src/lib/storage.effects.ts | 138 ++++++++--------- packages/sdk-types/src/lib/tokens.types.ts | 8 +- 9 files changed, 198 insertions(+), 139 deletions(-) create mode 100644 .changeset/slow-teeth-melt.md diff --git a/.changeset/slow-teeth-melt.md b/.changeset/slow-teeth-melt.md new file mode 100644 index 0000000000..86c9ea15de --- /dev/null +++ b/.changeset/slow-teeth-melt.md @@ -0,0 +1,7 @@ +--- +'@forgerock/storage': minor +'@forgerock/oidc-client': minor +--- + +- Standardizes return types on storage client and updates tests +- Improves OIDC client where storage client methods are used diff --git a/packages/oidc-client/src/lib/client.store.test.ts b/packages/oidc-client/src/lib/client.store.test.ts index 7c8f7785f8..87703d308a 100644 --- a/packages/oidc-client/src/lib/client.store.test.ts +++ b/packages/oidc-client/src/lib/client.store.test.ts @@ -13,6 +13,8 @@ import { oidc } from './client.store.js'; import type { OidcConfig } from './config.types.js'; +Object.defineProperty(global, 'localStorage', { value: null }); + vi.stubGlobal( 'sessionStorage', (() => { diff --git a/packages/oidc-client/src/lib/client.store.ts b/packages/oidc-client/src/lib/client.store.ts index edb9d804d2..e5d4b48890 100644 --- a/packages/oidc-client/src/lib/client.store.ts +++ b/packages/oidc-client/src/lib/client.store.ts @@ -372,14 +372,10 @@ export async function oidc({ // Delete local token and return combined results Micro.flatMap((revokeResponse) => Micro.promise(() => storageClient.remove()).pipe( - Micro.flatMap((deleteRes) => { - const deleteResponse = typeof deleteRes === 'undefined' ? null : deleteRes; - + Micro.flatMap((deleteResponse) => { const isInnerRequestError = (revokeResponse && 'error' in revokeResponse) || - (deleteResponse && - typeof deleteResponse === 'object' && - 'error' in deleteResponse); + (deleteResponse && 'error' in deleteResponse); if (isInnerRequestError) { const result: RevokeErrorResult = { diff --git a/packages/oidc-client/src/lib/client.types.ts b/packages/oidc-client/src/lib/client.types.ts index 9d5320d115..0ffa2de8ca 100644 --- a/packages/oidc-client/src/lib/client.types.ts +++ b/packages/oidc-client/src/lib/client.types.ts @@ -9,20 +9,25 @@ export interface GetTokensOptions { } export type RevokeSuccessResult = { - revokeResponse: GenericError | null; - deleteResponse: GenericError | null; + revokeResponse: null; + deleteResponse: null; }; -export type RevokeErrorResult = RevokeSuccessResult & { +export type RevokeErrorResult = { error: string; + revokeResponse: GenericError | null; + deleteResponse: GenericError | null; }; export type LogoutSuccessResult = RevokeSuccessResult & { - sessionResponse: GenericError | null; + sessionResponse: null; }; -export type LogoutErrorResult = LogoutSuccessResult & { +export type LogoutErrorResult = { error: string; + sessionResponse: GenericError | null; + revokeResponse: GenericError | null; + deleteResponse: GenericError | null; }; export type UserInfoResponse = { diff --git a/packages/oidc-client/src/lib/logout.request.test.ts b/packages/oidc-client/src/lib/logout.request.test.ts index 39beb17653..229c4697ac 100644 --- a/packages/oidc-client/src/lib/logout.request.test.ts +++ b/packages/oidc-client/src/lib/logout.request.test.ts @@ -66,6 +66,9 @@ const config: OidcConfig = { responseType: 'code', }; +Object.defineProperty(global, 'sessionStorage', { value: null }); +Object.defineProperty(global, 'localStorage', { value: null }); + const customStorage: Record = {}; const storageClient = createStorage({ type: 'custom', diff --git a/packages/oidc-client/src/lib/logout.request.ts b/packages/oidc-client/src/lib/logout.request.ts index 04480fb9a9..4bbfc6b9bc 100644 --- a/packages/oidc-client/src/lib/logout.request.ts +++ b/packages/oidc-client/src/lib/logout.request.ts @@ -7,10 +7,10 @@ import { Micro } from 'effect'; import { oidcApi } from './oidc.api.js'; import { createClientStore, createLogoutError } from './client.store.utils.js'; -import { OauthTokens, OidcConfig } from './config.types.js'; -import { WellKnownResponse } from '@forgerock/sdk-types'; -import { createStorage } from '@forgerock/storage'; -import { LogoutErrorResult, LogoutSuccessResult } from './client.types.js'; +import type { OauthTokens, OidcConfig } from './config.types.js'; +import type { WellKnownResponse } from '@forgerock/sdk-types'; +import type { StorageClient } from '@forgerock/storage'; +import type { LogoutErrorResult, LogoutSuccessResult } from './client.types.js'; export function logoutµ({ tokens, @@ -23,7 +23,7 @@ export function logoutµ({ config: OidcConfig; wellknown: WellKnownResponse; store: ReturnType; - storageClient: ReturnType>; + storageClient: StorageClient; }) { return Micro.zip( // End session with the ID token @@ -50,13 +50,11 @@ export function logoutµ({ // Delete local token and return combined results Micro.flatMap(([sessionResponse, revokeResponse]) => Micro.promise(() => storageClient.remove()).pipe( - Micro.flatMap((deleteRes) => { - const deleteResponse = typeof deleteRes === 'undefined' ? null : deleteRes; - + Micro.flatMap((deleteResponse) => { const isInnerRequestError = (sessionResponse && 'error' in sessionResponse) || (revokeResponse && 'error' in revokeResponse) || - (deleteResponse && typeof deleteResponse === 'object' && 'error' in deleteResponse); + (deleteResponse && 'error' in deleteResponse); if (isInnerRequestError) { const result: LogoutErrorResult = { diff --git a/packages/sdk-effects/storage/src/lib/storage.effects.test.ts b/packages/sdk-effects/storage/src/lib/storage.effects.test.ts index b910495abf..25cc7b7645 100644 --- a/packages/sdk-effects/storage/src/lib/storage.effects.test.ts +++ b/packages/sdk-effects/storage/src/lib/storage.effects.test.ts @@ -1,13 +1,12 @@ -/* eslint-disable @typescript-eslint/no-unused-vars */ /* * Copyright (c) 2025 Ping Identity Corporation. All rights reserved. * * This software may be modified and distributed under the terms * of the MIT license. See the LICENSE file for details. */ -import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { createStorage, type StorageConfig } from './storage.effects.js'; -import type { CustomStorageObject } from '@forgerock/sdk-types'; +import type { CustomStorageObject, GenericError } from '@forgerock/sdk-types'; const localStorageMock = (() => { let store: Record = {}; @@ -19,7 +18,7 @@ const localStorageMock = (() => { } return Promise.resolve(value); }), - setItem: vi.fn((key: string, value: any) => { + setItem: vi.fn((key: string, value: unknown) => { const valueIsString = typeof value === 'string'; store[key] = valueIsString ? value : JSON.stringify(value); return Promise.resolve(); @@ -76,10 +75,40 @@ const sessionStorageMock = (() => { Object.defineProperty(global, 'sessionStorage', { value: sessionStorageMock }); +let customStore: Record = {}; const mockCustomStore: CustomStorageObject = { - get: vi.fn((key: string) => Promise.resolve(null)), - set: vi.fn((key: string, value: unknown) => Promise.resolve()), - remove: vi.fn((key: string) => Promise.resolve()), + get: vi.fn(async (key: string): Promise => { + const keys = Object.keys(customStore); + if (!keys.includes(key)) { + return { + error: 'Retrieving_error', + message: 'Key not found', + type: 'unknown_error', + }; + } + return customStore[key]; + }), + set: vi.fn(async (key: string, valueToSet: unknown): Promise => { + if (valueToSet === `"bad-value"` || typeof valueToSet !== 'string') { + return { + error: 'Storing_error', + message: 'Value is bad', + type: 'unknown_error', + }; + } + customStore[key] = valueToSet; + }), + remove: vi.fn(async (key: string): Promise => { + const keys = Object.keys(customStore); + if (!keys.includes(key)) { + return { + error: 'Removing_error', + message: 'Key not found', + type: 'unknown_error', + }; + } + delete customStore[key]; + }), }; describe('storage Effect', () => { @@ -97,9 +126,7 @@ describe('storage Effect', () => { sessionStorageMock.clear(); vi.clearAllMocks(); - (mockCustomStore.get as Mock).mockResolvedValue(null); - (mockCustomStore.set as Mock).mockResolvedValue(undefined); - (mockCustomStore.remove as Mock).mockResolvedValue(undefined); + customStore = {}; }); describe('with localStorage', () => { @@ -112,7 +139,7 @@ describe('storage Effect', () => { const storageInstance = createStorage(config); it('should call localStorage.getItem with the correct key and return value', async () => { - localStorageMock.setItem(expectedKey, JSON.stringify(testValue)); + await localStorageMock.setItem(expectedKey, JSON.stringify(testValue)); const result = await storageInstance.get(); expect(localStorageMock.getItem).toHaveBeenCalledTimes(1); expect(localStorageMock.getItem).toHaveBeenCalledWith(expectedKey); @@ -129,7 +156,8 @@ describe('storage Effect', () => { }); it('should call localStorage.setItem with the correct key and value', async () => { - await storageInstance.set(testValue); + const result = await storageInstance.set(testValue); + expect(result).toBeNull(); expect(localStorageMock.setItem).toHaveBeenCalledTimes(1); expect(localStorageMock.setItem).toHaveBeenCalledWith(expectedKey, JSON.stringify(testValue)); expect(await localStorageMock.getItem(expectedKey)).toBe(JSON.stringify(testValue)); @@ -148,8 +176,9 @@ describe('storage Effect', () => { }); it('should call localStorage.removeItem with the correct key', async () => { - localStorageMock.setItem(expectedKey, testValue); - await storageInstance.remove(); + await localStorageMock.setItem(expectedKey, testValue); + const result = await storageInstance.remove(); + expect(result).toBeNull(); expect(localStorageMock.removeItem).toHaveBeenCalledTimes(1); expect(localStorageMock.removeItem).toHaveBeenCalledWith(expectedKey); expect(await localStorageMock.getItem(expectedKey)).toBeNull(); @@ -158,15 +187,14 @@ describe('storage Effect', () => { it('should parse objects/arrays when calling localStorage.getItem', async () => { const testObject = { a: 1, b: 'test' }; - storageInstance.set(testObject); + await storageInstance.set(testObject); const result = await storageInstance.get(); - console.log(result); expect(localStorageMock.getItem).toHaveBeenCalledTimes(1); expect(localStorageMock.getItem).toHaveBeenCalledWith(expectedKey); - // expect(result).toEqual(testObject); - // expect(mockCustomStore.remove).not.toHaveBeenCalled(); + expect(result).toEqual(testObject); + expect(mockCustomStore.remove).not.toHaveBeenCalled(); }); }); @@ -180,7 +208,7 @@ describe('storage Effect', () => { const storageInstance = createStorage(config); it('should call sessionStorage.getItem with the correct key and return value', async () => { - sessionStorageMock.setItem(expectedKey, JSON.stringify(testValue)); + await sessionStorageMock.setItem(expectedKey, JSON.stringify(testValue)); const result = await storageInstance.get(); expect(sessionStorageMock.getItem).toHaveBeenCalledTimes(1); expect(sessionStorageMock.getItem).toHaveBeenCalledWith(expectedKey); @@ -199,16 +227,14 @@ describe('storage Effect', () => { const obj = { tokens: 123 }; await storageInstance.set(obj); const result = await storageInstance.get(); - if (!result) { - // we should not hit this expect - expect(false).toBe(true); - } + expect(result).toStrictEqual(obj); expect(sessionStorageMock.getItem).toHaveBeenCalledTimes(1); expect(sessionStorageMock.getItem).toHaveBeenCalledWith(expectedKey); }); it('should call sessionStorage.setItem with the correct key and value', async () => { - await storageInstance.set(testValue); + const result = await storageInstance.set(testValue); + expect(result).toBeNull(); expect(sessionStorageMock.setItem).toHaveBeenCalledTimes(1); expect(sessionStorageMock.setItem).toHaveBeenCalledWith( expectedKey, @@ -218,9 +244,10 @@ describe('storage Effect', () => { expect(localStorageMock.setItem).not.toHaveBeenCalled(); expect(mockCustomStore.set).not.toHaveBeenCalled(); }); - it('should call sessionStorage.setItem with the correct key and value', async () => { + it('should call sessionStorage.setItem with the correct key and value and stringify objects', async () => { const obj = { tokens: 123 }; - await storageInstance.set(obj); + const result = await storageInstance.set(obj); + expect(result).toBeNull(); expect(sessionStorageMock.setItem).toHaveBeenCalledTimes(1); expect(sessionStorageMock.setItem).toHaveBeenCalledWith(expectedKey, JSON.stringify(obj)); expect(await sessionStorageMock.getItem(expectedKey)).toBe(JSON.stringify(obj)); @@ -228,7 +255,8 @@ describe('storage Effect', () => { expect(mockCustomStore.set).not.toHaveBeenCalled(); }); it('should call sessionStorage.removeItem with the correct key', async () => { - await storageInstance.remove(); + const result = await storageInstance.remove(); + expect(result).toBeNull(); expect(sessionStorageMock.removeItem).toHaveBeenCalledTimes(1); expect(sessionStorageMock.removeItem).toHaveBeenCalledWith(expectedKey); expect(await sessionStorageMock.getItem(expectedKey)).toBeNull(); @@ -237,8 +265,7 @@ describe('storage Effect', () => { }); }); - describe('with custom TokenStoreObject', () => { - const myStorage = 'MyStorage'; + describe('with custom storage', () => { const config: StorageConfig = { ...baseConfig, type: 'custom', @@ -247,7 +274,7 @@ describe('storage Effect', () => { const storageInstance = createStorage(config); it('should call customStore.get with the correct key and return its value', async () => { - (mockCustomStore.get as Mock).mockResolvedValueOnce(JSON.stringify(testValue)); + await storageInstance.set(testValue); const result = await storageInstance.get(); expect(mockCustomStore.get).toHaveBeenCalledTimes(1); expect(mockCustomStore.get).toHaveBeenCalledWith(expectedKey); @@ -256,32 +283,37 @@ describe('storage Effect', () => { expect(sessionStorageMock.getItem).not.toHaveBeenCalled(); }); - it('should return null if customStore.get returns null', async () => { - (mockCustomStore.get as Mock).mockResolvedValueOnce(null); + it('should parse objects/arrays returned from customStore.get', async () => { + const testObject = { token: 'abc', user: 'xyz' }; + await storageInstance.set(testObject); + const result = await storageInstance.get(); + expect(mockCustomStore.get).toHaveBeenCalledTimes(1); expect(mockCustomStore.get).toHaveBeenCalledWith(expectedKey); + expect(result).toEqual(testObject); // Verify it was parsed }); - it('should parse JSON strings returned from customStore.get', async () => { - const testObject = { token: 'abc', user: 'xyz' }; - const jsonString = JSON.stringify(testObject); - (mockCustomStore.get as Mock).mockResolvedValueOnce(jsonString); // Mock returns JSON string - + it('should return an error if customStore.get errors', async () => { const result = await storageInstance.get(); - + expect(result).toStrictEqual({ + error: 'Retrieving_error', + message: 'Key not found', + type: 'unknown_error', + }); expect(mockCustomStore.get).toHaveBeenCalledTimes(1); expect(mockCustomStore.get).toHaveBeenCalledWith(expectedKey); - expect(result).toEqual(testObject); // Verify it was parsed }); it('should call customStore.set with the correct key and value', async () => { - await storageInstance.set(testValue); + const result = await storageInstance.set(testValue); + expect(result).toBeNull(); expect(mockCustomStore.set).toHaveBeenCalledTimes(1); expect(mockCustomStore.set).toHaveBeenCalledWith(expectedKey, JSON.stringify(testValue)); expect(localStorageMock.setItem).not.toHaveBeenCalled(); expect(sessionStorageMock.setItem).not.toHaveBeenCalled(); }); + it('should call customStore.set with the correct key and value and stringify objects', async () => { await storageInstance.set({ test: { tokens: '123' } }); expect(mockCustomStore.set).toHaveBeenCalledTimes(1); @@ -292,17 +324,41 @@ describe('storage Effect', () => { expect(localStorageMock.setItem).not.toHaveBeenCalled(); expect(sessionStorageMock.setItem).not.toHaveBeenCalled(); }); + + it('should return an error if customStore.set errors', async () => { + const result = await storageInstance.set('bad-value'); + expect(result).toStrictEqual({ + error: 'Storing_error', + message: 'Value is bad', + type: 'unknown_error', + }); + expect(mockCustomStore.set).toHaveBeenCalledTimes(1); + expect(mockCustomStore.set).toHaveBeenCalledWith(expectedKey, JSON.stringify('bad-value')); + }); + it('should call customStore.remove with the correct key', async () => { - await storageInstance.remove(); + await storageInstance.set(testValue); + const result = await storageInstance.remove(); + expect(result).toBeNull(); expect(mockCustomStore.remove).toHaveBeenCalledTimes(1); expect(mockCustomStore.remove).toHaveBeenCalledWith(expectedKey); expect(localStorageMock.removeItem).not.toHaveBeenCalled(); expect(sessionStorageMock.removeItem).not.toHaveBeenCalled(); }); + + it('should return an error if customStore.remove errors', async () => { + const result = await storageInstance.remove(); + expect(result).toStrictEqual({ + error: 'Removing_error', + message: 'Key not found', + type: 'unknown_error', + }); + expect(mockCustomStore.remove).toHaveBeenCalledTimes(1); + expect(mockCustomStore.remove).toHaveBeenCalledWith(expectedKey); + }); }); it('should return a function that returns the storage interface', () => { - const myStorage = 'MyStorage'; const config: StorageConfig = { ...baseConfig, type: 'localStorage' }; const storageInterface = createStorage(config); expect(storageInterface).toHaveProperty('get'); diff --git a/packages/sdk-effects/storage/src/lib/storage.effects.ts b/packages/sdk-effects/storage/src/lib/storage.effects.ts index 0799fd097f..dd76facdf1 100644 --- a/packages/sdk-effects/storage/src/lib/storage.effects.ts +++ b/packages/sdk-effects/storage/src/lib/storage.effects.ts @@ -8,12 +8,8 @@ import { CustomStorageObject, GenericError } from '@forgerock/sdk-types'; export interface StorageClient { get: () => Promise; - set: (value: Value) => Promise; - remove: () => Promise; + set: (value: Value) => Promise; + remove: () => Promise; } export type StorageConfig = BrowserStorageConfig | CustomStorageConfig; @@ -31,109 +27,103 @@ export interface CustomStorageConfig { custom: CustomStorageObject; } -export function createStorage(config: StorageConfig) { +function createStorageError( + storeType: 'localStorage' | 'sessionStorage' | 'custom', + action: 'Storing' | 'Retrieving' | 'Removing' | 'Parsing', +): GenericError { + let storageName; + switch (storeType) { + case 'localStorage': + storageName = 'local'; + break; + case 'sessionStorage': + storageName = 'session'; + break; + case 'custom': + storageName = 'custom'; + break; + default: + break; + } + + return { + error: `${action}_error`, + message: `Error ${action.toLowerCase()} value from ${storageName} storage`, + type: action === 'Parsing' ? 'parse_error' : 'unknown_error', + }; +} + +export function createStorage(config: StorageConfig): StorageClient { const { type: storeType, prefix = 'pic', name } = config; + const key = `${prefix}-${name}`; + const storageTypes = { + sessionStorage, + localStorage, + }; if (storeType === 'custom' && !('custom' in config)) { throw new Error('Custom storage configuration must include a custom storage object'); } - const key = `${prefix}-${name}`; return { get: async function storageGet(): Promise { - if ('custom' in config) { + if (storeType === 'custom') { const value = await config.custom.get(key); - if (value === null) { + if (value === null || (typeof value === 'object' && 'error' in value)) { return value; } + try { const parsed = JSON.parse(value); return parsed as Value; } catch { - return { - error: 'Parse_Error', - message: 'Error parsing value from provided storage', - type: 'parse_error', - }; + return createStorageError(storeType, 'Parsing'); } } - if (storeType === 'sessionStorage') { - const value = await sessionStorage.getItem(key); + + let value: string | null; + try { + value = await storageTypes[storeType].getItem(key); if (value === null) { return value; } - try { - const parsed = JSON.parse(value); - return parsed as Value; - } catch { - return { - error: 'Parse_Error', - message: 'Error parsing value from session storage', - type: 'parse_error', - }; - } + } catch { + return createStorageError(storeType, 'Retrieving'); } - const value = await localStorage.getItem(key); - if (value === null) { - return value; - } try { const parsed = JSON.parse(value); return parsed as Value; } catch { - return { - error: 'Parse_Error', - message: 'Error parsing value from local storage', - type: 'parse_error', - }; + return createStorageError(storeType, 'Parsing'); } }, - set: async function storageSet(value: Value) { + set: async function storageSet(value: Value): Promise { const valueToStore = JSON.stringify(value); - if ('custom' in config) { - try { - await config.custom.set(key, valueToStore); - return Promise.resolve(); - } catch { - return { - code: 'Storing_Error', - message: 'Error storing value in custom storage', - type: 'unknown_error', - }; - } - } - if (storeType === 'sessionStorage') { - try { - await sessionStorage.setItem(key, valueToStore); - return Promise.resolve(); - } catch { - return { - code: 'Storing_Error', - message: 'Error storing value in session storage', - type: 'unknown_error', - }; - } + if (storeType === 'custom') { + const value = await config.custom.set(key, valueToStore); + return Promise.resolve(value ?? null); } + try { - await localStorage.setItem(key, valueToStore); - return Promise.resolve(); + await storageTypes[storeType].setItem(key, valueToStore); + return Promise.resolve(null); } catch { - return { - code: 'Storing_Error', - message: 'Error storing value in local storage', - type: 'unknown_error', - }; + return createStorageError(storeType, 'Storing'); } }, - remove: async function storageSet() { - if ('custom' in config) { - return await config.custom.remove(key); + remove: async function storageRemove(): Promise { + if (storeType === 'custom') { + const value = await config.custom.remove(key); + return Promise.resolve(value ?? null); } - if (storeType === 'sessionStorage') { - return await sessionStorage.removeItem(key); + + try { + await storageTypes[storeType].removeItem(key); + return Promise.resolve(null); + } catch { + return createStorageError(storeType, 'Removing'); } - return await localStorage.removeItem(key); }, - }; + } as StorageClient; } diff --git a/packages/sdk-types/src/lib/tokens.types.ts b/packages/sdk-types/src/lib/tokens.types.ts index 2232430ada..2a55e2eb39 100644 --- a/packages/sdk-types/src/lib/tokens.types.ts +++ b/packages/sdk-types/src/lib/tokens.types.ts @@ -5,6 +5,8 @@ * of the MIT license. See the LICENSE file for details. */ +import type { GenericError } from './error.types.js'; + export interface Tokens { accessToken: string; idToken?: string; @@ -16,7 +18,7 @@ export interface Tokens { * API for implementing a custom token store */ export interface CustomStorageObject { - get: (key: string) => Promise; - set: (key: string, valueToSet: string) => Promise; - remove: (key: string) => Promise; + get: (key: string) => Promise; + set: (key: string, valueToSet: string) => Promise; + remove: (key: string) => Promise; }