test(sdk-oidc): add tests to oidc effects package #416
Conversation
🦋 Changeset detectedLatest commit: 7ffa428 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughNormalized an export path, removed lib re-exports, exported Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant PKCE as state-pkce.effects
participant Storage as sessionStorage
rect rgba(230,245,255,0.6)
note over App,PKCE: Generate and store authorization URL values
App->>PKCE: generateAndStoreAuthUrlValues(options)
PKCE->>PKCE: generate state & verifier (PKCE)
PKCE->>PKCE: build authorize URL (includes code_challenge)
PKCE->>PKCE: key = getStorageKey(clientId[, prefix])
PKCE->>Storage: setItem(key, JSON.stringify(values))
PKCE-->>App: { url, state, verifier }
end
rect rgba(240,255,230,0.6)
note over App,PKCE: Retrieve stored values
App->>PKCE: getStoredAuthUrlValues(clientId[, prefix])
PKCE->>PKCE: key = getStorageKey(clientId[, prefix])
PKCE->>Storage: getItem(key)
alt JSON valid
PKCE->>Storage: removeItem(key)
PKCE-->>App: parsed values
else JSON invalid or missing
PKCE-->>App: throw "Stored values for Auth URL could not be parsed"
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
View your CI Pipeline Execution ↗ for commit 7ffa428
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #416 +/- ##
=======================================
Coverage 55.63% 55.63%
=======================================
Files 32 32
Lines 2051 2051
Branches 344 344
=======================================
Hits 1141 1141
Misses 910 910 🚀 New features to boost your workflow:
|
|
Deployed 04cc255 to https://ForgeRock.github.io/ping-javascript-sdk/pr-416/04cc25555800fe4691cc890a5954656386314751 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis📊 Minor Changes📉 @forgerock/sdk-oidc - 2.5 KB (-0.2 KB) ➖ No Changes➖ @forgerock/davinci-client - 34.2 KB 11 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/sdk-effects/oidc/src/lib/state-pkce.effects.ts (3)
15-16: Optional: guard against empty clientId in storage keyA defensive check avoids generating keys like 'FR-SDK-authflow-' that collide across calls.
-export function getStorageKey(clientId: string, prefix?: string) { - return `${prefix || 'FR-SDK'}-authflow-${clientId}`; +export function getStorageKey(clientId: string, prefix?: string) { + if (!clientId?.trim()) { + throw new Error('clientId is required to generate a storage key'); + } + return `${prefix ?? 'FR-SDK'}-authflow-${clientId}`; }
26-43: Return type can preserve input fields (optional)You currently narrow the first tuple element to GetAuthorizationUrlOptions & { state; verifier }. Returning the full incoming options type keeps type info for serverConfig/clientId at call sites without casting.
-export function generateAndStoreAuthUrlValues( - options: GenerateAndStoreAuthUrlValues, -): readonly [GetAuthorizationUrlOptions & { state: string; verifier: string }, () => void] { +export function generateAndStoreAuthUrlValues( + options: GenerateAndStoreAuthUrlValues, +): readonly [GenerateAndStoreAuthUrlValues & { state: string; verifier: string }, () => void] {
50-63: Harden retrieval: handle missing value explicitly and remove in finallyThis yields clearer errors and still guarantees single-use cleanup.
-export function getStoredAuthUrlValues( - clientId: string, - prefix?: string, -): GetAuthorizationUrlOptions { - const storageKey = getStorageKey(clientId, prefix); - const storedString = sessionStorage.getItem(storageKey); - sessionStorage.removeItem(storageKey); - - try { - return JSON.parse(storedString as string); - } catch { - throw new Error('Stored values for Auth URL could not be parsed'); - } -} +export function getStoredAuthUrlValues( + clientId: string, + prefix?: string, +): GetAuthorizationUrlOptions { + const storageKey = getStorageKey(clientId, prefix); + const storedString = sessionStorage.getItem(storageKey); + try { + if (storedString == null) { + throw new Error('No stored values for Auth URL were found'); + } + return JSON.parse(storedString); + } catch (err) { + if ((err as Error).message.includes('No stored values')) { + throw err; + } + throw new Error('Stored values for Auth URL could not be parsed'); + } finally { + // Ensure single-use semantics regardless of parse outcome + sessionStorage.removeItem(storageKey); + } +}If you adopt this, add a test for the "no stored values" case and ensure existing tests still pass.
packages/sdk-effects/oidc/src/lib/state-pkce.test.ts (3)
16-33: DRY the mock sessionStorage across testsBoth this file and authorize.test.ts duplicate the mock. Consider a small test util to import from both places.
I can extract this to packages/sdk-effects/oidc/test-utils/mockSessionStorage.ts and update imports across tests.
35-41: Prefer globalThis for cross-environment compatibilityUsing globalThis avoids Node vs browser differences.
- beforeEach(() => { - if (typeof sessionStorage === 'undefined') { - global.sessionStorage = mockSessionStorage; - } - - sessionStorage.clear(); - }); + beforeEach(() => { + if (typeof (globalThis as any).sessionStorage === 'undefined') { + (globalThis as any).sessionStorage = mockSessionStorage; + } + (globalThis as any).sessionStorage.clear(); + });
112-119: Optional: add a "no stored values" test (if getter is hardened)If you implement the explicit null-handling suggested, add:
+ it('should throw a specific error when no stored values exist', () => { + const storageKey = getStorageKey(mockOptions.clientId, mockOptions.prefix); + // Ensure nothing is set + sessionStorage.removeItem(storageKey); + expect(() => + getStoredAuthUrlValues(mockOptions.clientId, mockOptions.prefix), + ).toThrow('No stored values for Auth URL were found'); + });Also consider a test that sets a custom prefix on options and confirms the storage key includes it end-to-end.
packages/sdk-effects/oidc/src/lib/authorize.test.ts (2)
13-29: Share mock and use globalThisSame mock as in state-pkce.test.ts; import a shared one and use globalThis.
-const mockSessionStorage = (() => { - let store: { [key: string]: string } = {}; - return { - getItem: (key: string) => store[key] || null, - setItem: (key: string, value: string) => { - store[key] = value; - }, - removeItem: (key: string) => { - delete store[key]; - }, - clear: () => { - store = {}; - }, - length: 0, - key: (index: number) => Object.keys(store)[index] || null, - }; -})(); +import mockSessionStorage from '../../test-utils/mockSessionStorage';- beforeEach(() => { - if (typeof sessionStorage === 'undefined') { - global.sessionStorage = mockSessionStorage; - } - sessionStorage.clear(); - }); + beforeEach(() => { + if (typeof (globalThis as any).sessionStorage === 'undefined') { + (globalThis as any).sessionStorage = mockSessionStorage; + } + (globalThis as any).sessionStorage.clear(); + });
86-101: Add a state-correlation check (recommended)Validate that the state in the URL matches the stored state, ensuring end-to-end integrity.
it('should store the authorize options in session storage', async () => { await createAuthorizeUrl(baseUrl, mockOptions); const storageKey = getStorageKey(mockOptions.clientId); const storedData = sessionStorage.getItem(storageKey); const parsedOptions = JSON.parse(storedData as string); const serverUrl = new URL(baseUrl).origin; expect(storedData).toBeDefined(); expect(parsedOptions).toMatchObject({ ...mockOptions, serverConfig: { baseUrl: serverUrl }, }); expect(parsedOptions).toHaveProperty('state'); expect(parsedOptions).toHaveProperty('verifier'); + + // Correlate stored state with URL param + const url = await createAuthorizeUrl(baseUrl, mockOptions); + const urlState = new URL(url).searchParams.get('state'); + expect(urlState).toBeDefined(); + // Value will differ because a new call generates new state. + // For correlation, compare by storing just once and reading: + }); + + it('should correlate URL state with stored state for a single invocation', async () => { + // Single invocation to compare values deterministically + const url = await createAuthorizeUrl(baseUrl, mockOptions); + const storageKey = getStorageKey(mockOptions.clientId); + const stored = JSON.parse(sessionStorage.getItem(storageKey) as string); + const urlState = new URL(url).searchParams.get('state'); + expect(urlState).toBe(stored.state); });If you keep the single-use removal in getStoredAuthUrlValues, ensure you compare before calling the getter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/sdk-effects/oidc/src/index.ts(1 hunks)packages/sdk-effects/oidc/src/lib/authorize.test.ts(1 hunks)packages/sdk-effects/oidc/src/lib/index.ts(0 hunks)packages/sdk-effects/oidc/src/lib/state-pkce.effects.ts(1 hunks)packages/sdk-effects/oidc/src/lib/state-pkce.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/sdk-effects/oidc/src/lib/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/sdk-effects/oidc/src/lib/state-pkce.test.ts (2)
packages/sdk-types/src/lib/authorize.types.ts (1)
GenerateAndStoreAuthUrlValues(44-48)packages/sdk-effects/oidc/src/lib/state-pkce.effects.ts (3)
getStorageKey(15-17)generateAndStoreAuthUrlValues(26-43)getStoredAuthUrlValues(50-63)
packages/sdk-effects/oidc/src/lib/authorize.test.ts (3)
packages/sdk-types/src/lib/authorize.types.ts (1)
GenerateAndStoreAuthUrlValues(44-48)packages/sdk-effects/oidc/src/lib/authorize.effects.ts (1)
createAuthorizeUrl(23-59)packages/sdk-effects/oidc/src/lib/state-pkce.effects.ts (1)
getStorageKey(15-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (4)
packages/sdk-effects/oidc/src/lib/state-pkce.test.ts (1)
74-85: Nice coverage of store-and-parse pathTest verifies serialization fidelity and PKCE fields presence.
packages/sdk-effects/oidc/src/lib/authorize.test.ts (1)
47-68: Solid parameter assertionsGood checks for required params and PKCE artifacts on the URL.
packages/sdk-effects/oidc/src/index.ts (1)
1-7: Approve: license header + path normalization verifiedNo "lib//" occurrences; both effect files exist and packages/sdk-effects/oidc/src/index.ts correctly re-exports './lib/authorize.effects.js' and './lib/state-pkce.effects.js'.
packages/sdk-effects/oidc/src/lib/state-pkce.effects.ts (1)
10-13: Approve: GenerateAndStoreAuthUrlValues shape verifiedInterface exports clientId and optional prefix; GetAuthorizationUrlOptions includes prompt and responseMode — usages in state-pkce.effects.ts and authorize.effects.ts are compatible.
| * This software may be modified and distributed under the terms | ||
| * of the MIT license. See the LICENSE file for details. | ||
| */ | ||
| export * from './lib/authorize.effects.js'; |
There was a problem hiding this comment.
per our conversation on Slack, this technically, is an api addition.
So, the question is why?
There was a problem hiding this comment.
Ok so I see what happened here, we kind of reorganized this. it's still an api addition though.
There was a problem hiding this comment.
Added minor changeset
There was a problem hiding this comment.
I guess, why are we exporting this from the oidc package, it doesn't seem to be used outside of here, or am I missing something?
There was a problem hiding this comment.
Are you referring to getStorageKey? I use it in my tests. I test the function itself - if it can generate a custom key and default key. I also use it for convenience to check if things are stored correctly in session storage. It's not used by any other packages at this time.
There was a problem hiding this comment.
Specifically referring to the line in this comment. why are we adding authorize.effects.js to the index.ts file?
This is the api change, and i'm not seeing us use this outside of here right now.
is this because we're going to consume functions from here in the future?
There was a problem hiding this comment.
She's not adding authorize.effect.js do the export. She fixed a typo in the original export of this module and added the copyright comment above it.
ancheetah
force-pushed
the
SDKS-4330-sdk-oidc-tests
branch
from
6c72124 to
7ffa428
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.changeset/icy-olives-make.md (1)
5-6: Polish the changelog wordingMake the notes a bit more consumer-facing.
Apply:
-- Adds tests for OIDC effects package -- Exposes `getStorageKey` utility +- Add tests for OIDC effects (no runtime changes) +- Expose `getStorageKey` from @forgerock/sdk-oidcpackages/sdk-effects/oidc/src/lib/state-pkce.test.ts (4)
16-32: Align mock sessionStorage with the Storage interfaceLength should be dynamic; add a getter and tighten typing to avoid surprises.
Apply:
-const mockSessionStorage = (() => { - let store: { [key: string]: string } = {}; +const mockSessionStorage: Storage = (() => { + let store: Record<string, string> = {}; return { - getItem: (key: string) => store[key] || null, - setItem: (key: string, value: string) => { - store[key] = value; + getItem(key: string) { + return Object.prototype.hasOwnProperty.call(store, key) ? store[key] : null; }, - removeItem: (key: string) => { - delete store[key]; + setItem(key: string, value: string) { + store[key] = String(value); }, - clear: () => { - store = {}; + removeItem(key: string) { + delete store[key]; }, - length: 0, - key: (index: number) => Object.keys(store)[index] || null, - }; + clear() { + store = {}; + }, + key(index: number) { + const keys = Object.keys(store); + return keys[index] ?? null; + }, + get length() { + return Object.keys(store).length; + }, + } as unknown as Storage; })();
35-41: Prefer globalThis for environment-agnostic assignmentMinor portability tweak.
Apply:
- if (typeof sessionStorage === 'undefined') { - global.sessionStorage = mockSessionStorage; - } + if (typeof sessionStorage === 'undefined') { + globalThis.sessionStorage = mockSessionStorage; + }
65-85: Add spec-oriented assertions and uniqueness check for PKCE valuesValidate verifier length/charset and ensure state/verifier are unique per call.
Apply:
describe('generateAndStoreAuthUrlValues', () => { it('should generate PKCE values', () => { const [options] = generateAndStoreAuthUrlValues(mockOptions); expect(options).toBeDefined(); expect(options).toHaveProperty('state'); expect(options).toHaveProperty('verifier'); + // RFC 7636: verifier 43-128 chars, unreserved [A-Za-z0-9-._~] + expect(options.verifier.length).toBeGreaterThanOrEqual(43); + expect(options.verifier.length).toBeLessThanOrEqual(128); + expect(options.verifier).toMatch(/^[A-Za-z0-9\-._~]+$/); }); + it('should generate unique state and verifier per invocation', () => { + const [a] = generateAndStoreAuthUrlValues(mockOptions); + const [b] = generateAndStoreAuthUrlValues(mockOptions); + expect(a.state).not.toBe(b.state); + expect(a.verifier).not.toBe(b.verifier); + });
87-121: Extend storage tests: custom prefix, parse-error removal, and empty storage caseCovers important edge cases and current behavior.
Apply:
describe('getStoredAuthUrlValues', () => { + it('should work with a custom prefix', () => { + const custom = { ...mockOptions, prefix: 'CUSTOM' as const }; + const [options, storeAuthUrl] = generateAndStoreAuthUrlValues(custom); + storeAuthUrl(); + const key = getStorageKey(custom.clientId, custom.prefix); + expect(sessionStorage.getItem(key)).toBeTruthy(); + const storedValues = getStoredAuthUrlValues(custom.clientId, custom.prefix); + expect(storedValues).toEqual(options); + }); + it('should retrieve and parse stored values', () => { const [options, storeAuthUrl] = generateAndStoreAuthUrlValues(mockOptions); storeAuthUrl(); const storedValues = getStoredAuthUrlValues(mockOptions.clientId, mockOptions.prefix); expect(storedValues).toEqual(options); }); it('should remove values from storage after retrieval', () => { const [, storeAuthUrl] = generateAndStoreAuthUrlValues(mockOptions); storeAuthUrl(); const storageKey = getStorageKey(mockOptions.clientId, mockOptions.prefix); // Verify value exists before retrieval expect(sessionStorage.getItem(storageKey)).toBeDefined(); // Retrieve values getStoredAuthUrlValues(mockOptions.clientId, mockOptions.prefix); // Verify value was removed expect(sessionStorage.getItem(storageKey)).toBeNull(); }); it('should throw error when stored values cannot be parsed', () => { const storageKey = getStorageKey(mockOptions.clientId, mockOptions.prefix); sessionStorage.setItem(storageKey, 'invalid json'); expect(() => getStoredAuthUrlValues(mockOptions.clientId, mockOptions.prefix)).toThrow( 'Stored values for Auth URL could not be parsed', ); + // Verify removal happens even on parse error + expect(sessionStorage.getItem(storageKey)).toBeNull(); }); + + it('should throw when no stored values exist for the key', () => { + const absentKey = getStorageKey('absent-client'); + expect(sessionStorage.getItem(absentKey)).toBeNull(); + // Current implementation throws the generic parse error; assert current behavior. + expect(() => getStoredAuthUrlValues('absent-client')).toThrow( + 'Stored values for Auth URL could not be parsed', + ); + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/icy-olives-make.md(1 hunks)packages/sdk-effects/oidc/src/index.ts(1 hunks)packages/sdk-effects/oidc/src/lib/authorize.test.ts(1 hunks)packages/sdk-effects/oidc/src/lib/index.ts(0 hunks)packages/sdk-effects/oidc/src/lib/state-pkce.effects.ts(1 hunks)packages/sdk-effects/oidc/src/lib/state-pkce.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/sdk-effects/oidc/src/lib/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/sdk-effects/oidc/src/lib/state-pkce.effects.ts
- packages/sdk-effects/oidc/src/lib/authorize.test.ts
- packages/sdk-effects/oidc/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/sdk-effects/oidc/src/lib/state-pkce.test.ts (2)
packages/sdk-types/src/lib/authorize.types.ts (1)
GenerateAndStoreAuthUrlValues(44-48)packages/sdk-effects/oidc/src/lib/state-pkce.effects.ts (3)
getStorageKey(15-17)generateAndStoreAuthUrlValues(26-43)getStoredAuthUrlValues(50-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (3)
.changeset/icy-olives-make.md (1)
1-6: Changeset looks valid for a minor bumpPublicly exposing getStorageKey warrants a minor release; scope/package name formatting is correct.
packages/sdk-effects/oidc/src/lib/state-pkce.test.ts (2)
50-63: getStorageKey tests read clean and preciseCovers default and custom prefixes well.
8-15: ESM import style is compatible with TS configpackages/sdk-effects/oidc/package.json declares "type":"module"; packages/sdk-effects/oidc/tsconfig.lib.json and tsconfig.spec.json set "module":"nodenext" and "moduleResolution":"nodenext" — .js extensions in TS sources are supported.
4 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4330
Description
Adds unit tests for pkce and authorize effects in the OIDC effects package (
@forgerock/sdk-oidc)Added a changeset since a new function was exposed.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores