feat: support session cookies #66
Conversation
WalkthroughThis pull request updates the function signature for Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant CC as ClientCredentials
participant JW as jwtVerify (jose)
participant TU as Token Utils
participant SM as SessionManager
C->>CC: getToken(sessionManager)
CC->>CC: fetchAccessTokenFor()
CC->>JW: jwtVerify(access_token, keyProvider)
JW-->>CC: verificationResult (payload with ksp)
Note over CC: Derive sessionType from payload.ksp
CC->>TU: commitTokenToSession(sessionManager,\naccess_token, 'access_token',\nvalidationDetails, sessionType)
TU->>SM: setSessionItem(token, sessionType)
SM-->>TU: Confirmation
TU-->>CC: Token committed
CC-->>C: Return access token
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
lib/sdk/utilities/token-utils.ts (2)
17-23: Update JSDoc to reflect the newsessionTypeparameterThe function signature has been updated to include a new
sessionTypeparameter, but the JSDoc comments don't reflect this change. Please update the function documentation to include the new parameter and explain its purpose./** * Saves the provided token to the current session. * * @param {SessionManager} sessionManager * @param {string} token * @param {TokenType} type + * @param {TokenValidationDetailsType} validationDetails + * @param {string} sessionType - Determines if the token should be stored as 'session' or 'persistent' */
41-41: Consider error handling for session storage issuesWhile the code properly handles JWT verification errors, it might be good to add error handling around the session storage operation as well.
- await sessionManager.setSessionItem(type, token, sessionType); + try { + await sessionManager.setSessionItem(type, token, sessionType); + } catch (e) { + throw new KindeSDKError( + KindeSDKErrorCode.SESSION_STORAGE_ERROR, + `Failed to store ${type} token to session: ${e.message}` + ); + }lib/__tests__/sdk/utilities/token-utils.spec.ts (1)
29-52: Add test case for non-persistent session typeThe current test cases only verify the 'persistent' session type. Consider adding a test case to verify that tokens with
ksp: 'non_persistent'in the payload correctly create 'session' type cookies.it('uses session type based on ksp claim in the token', async () => { // Create a mock token with ksp: 'non_persistent' const { token: mockAccessTokenNonPersistent } = await mocks.getMockAccessToken(domain, false, { ksp: 'non_persistent' }); const { token: mockIdToken } = await mocks.getMockAccessToken(domain); const tokenCollection: TokenCollection = { refresh_token: 'refresh_token', access_token: mockAccessTokenNonPersistent, id_token: mockIdToken, }; // Mock the setSessionItem method to verify the sessionType parameter const setSessionItemSpy = jest.spyOn(sessionManager, 'setSessionItem'); await commitTokensToSession( sessionManager, tokenCollection, validationDetails ); // Verify that 'session' type was used for tokens expect(setSessionItemSpy).toHaveBeenCalledWith('access_token', mockAccessTokenNonPersistent, 'session'); expect(setSessionItemSpy).toHaveBeenCalledWith('refresh_token', 'refresh_token', 'session'); expect(setSessionItemSpy).toHaveBeenCalledWith('id_token', mockIdToken, 'session'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package.jsonis excluded by!**/*.json
📒 Files selected for processing (5)
lib/__tests__/sdk/oauth2-flows/ClientCredentials.spec.ts(1 hunks)lib/__tests__/sdk/utilities/token-utils.spec.ts(3 hunks)lib/sdk/oauth2-flows/ClientCredentials.ts(2 hunks)lib/sdk/session-managers/types.ts(1 hunks)lib/sdk/utilities/token-utils.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: main (18.x)
🔇 Additional comments (8)
lib/__tests__/sdk/oauth2-flows/ClientCredentials.spec.ts (1)
85-86: Session type addition appears correct.
The addition of the'persistent'argument here aligns with the updatedcommitTokenToSessionfunction signature. Consider adding a test scenario for the'non_persistent'session type to ensure comprehensive coverage of all session state possibilities.lib/sdk/oauth2-flows/ClientCredentials.ts (2)
1-1: Validate the correctness of the newjwtVerifyimport.
ImportingjwtVerifyfrom 'jose' is the correct approach for verifying JWT payloads. Ensure that the project dependencies are updated to includejoseand that any runtime errors from asynchronous verification are properly handled upstream.
58-70: Confirm robust fallback logic forkspand session type.
Whenresult.payload.kspis'non_persistent', this code uses'session'; otherwise, it defaults to'persistent'. If unknown or missingkspvalues should also default to'persistent', then this is appropriate. Otherwise, consider explicitly handling unexpected values to avoid misclassifying session types. Also ensure any exceptions fromjwtVerifyare captured and handled appropriately.lib/sdk/session-managers/types.ts (1)
10-14: Ensure all session managers handle the newsessionTypeparameter.
The updated signature adds flexibility for specifying either'session'or'persistent'. Verify that all implementations ofSessionManagerproperly apply thesessionTypeparameter (e.g., cookies, storage, or other underlying session mechanisms) to avoid inconsistent session handling.lib/sdk/utilities/token-utils.ts (1)
55-59: Good implementation of session persistence detectionThe code correctly verifies the JWT and extracts the
kspproperty to determine the session type. This aligns well with the PR objective of supporting session cookies.lib/__tests__/sdk/utilities/token-utils.spec.ts (3)
65-67: Test function updated correctly with new parameterThe test has been updated to include the new
sessionTypeparameter, which is good.
83-85: Test function updated correctly with new parameterThis test case has been properly updated to include the new
sessionTypeparameter.
101-103: Test function updated correctly with new parameterThis test case has been properly updated to include the new
sessionTypeparameter.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
lib/sdk/utilities/token-utils.ts (1)
139-139: 💡 Verification agent🧩 Analysis chain
Fix type mismatch in UserType
The pipeline is failing with a type error on line 139. The error indicates that you're trying to assign an object with a
phoneproperty toUserType, butphoneis not defined in theUserTypeinterface.
🏁 Script executed:
#!/bin/bash # Check the definition of UserType to verify the correct properties grep -r "UserType" --include="*.ts" . | grep -v "import" | head -n 10Length of output: 938
Action Required: Address Type Mismatch on UserType
- The build failure indicates that at
lib/sdk/utilities/token-utils.ts:139the object being constructed includes aphoneproperty which isn’t defined in the currentUserTypeinterface.- Please verify if the
phoneproperty should be part of the user model. If it is required, update theUserTypeinterface accordingly. Otherwise, remove thephoneproperty from the returned object so that its structure aligns withUserType.- Although the shown snippet only displays the assignment of
id(i.e.,id: idToken.payload.sub!), it appears that additional properties (includingphone) are being added elsewhere in the construction of the user object. A review of the complete returned object is needed to ensure full type compliance.🧰 Tools
🪛 GitHub Check: main (18.x)
[failure] 139-139:
Type '{ family_name: string; given_name: string; picture: string; email: string; phone: string; id: string; }' is not assignable to type 'UserType'.🪛 GitHub Actions: Build and test TypeScript SDK CI
[error] 139-139: Type '{ family_name: string; given_name: string; picture: string; email: string; phone: string; id: string; }' is not assignable to type 'UserType'.
🧹 Nitpick comments (1)
lib/sdk/utilities/token-utils.ts (1)
58-62: Consider adding error handling for JWT verificationThe JWT verification looks good and the logic to determine the session type based on the
kspclaim is clear. However, there's no error handling if the JWT verification fails in this function.export const commitTokensToSession = async ( sessionManager: SessionManager, tokens: TokenCollection, validationDetails: TokenValidationDetailsType ): Promise<void> => { - const key = await validationDetails.keyProvider(); - const result = await jwtVerify(tokens.access_token, key); + let sessionType = 'persistent'; // Default to persistent + try { + const key = await validationDetails.keyProvider(); + const result = await jwtVerify(tokens.access_token, key); + sessionType = result.payload.ksp === 'non_persistent' ? 'session' : 'persistent'; + } catch (e) { + // Log error or handle gracefully, but continue with default sessionType + console.error('Failed to verify token for session type determination:', e); + } - const sessionType = - result.payload.ksp === 'non_persistent' ? 'session' : 'persistent';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/sdk/utilities/token-utils.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build and test TypeScript SDK CI
lib/sdk/utilities/token-utils.ts
[error] 139-139: Type '{ family_name: string; given_name: string; picture: string; email: string; phone: string; id: string; }' is not assignable to type 'UserType'.
🔇 Additional comments (4)
lib/sdk/utilities/token-utils.ts (4)
16-17: Good addition of JSDoc for the new parameterThe documentation for the new
sessionTypeparameter clearly explains its purpose, which enhances code maintainability.
24-25: Function signature update looks correctThe function signature has been properly updated to include the new
sessionTypeparameter, matching the JSDoc changes.
44-44: Session manager integration looks goodThe updated call to
setSessionItemnow includes thesessionTypeparameter, allowing for differentiated session storage behavior.
69-70: Function calls correctly updated with the new parameterAll calls to
commitTokenToSessionhave been consistently updated to include thesessionTypeparameter.Also applies to: 76-77, 83-84
Explain your changes
Add support for
kspto enable session cookiesChecklist
🛟 If you need help, consider asking for advice over in the Kinde community.