feat: improve auth URL generation and add extra params. #49
Conversation
WalkthroughThis pull request introduces modifications to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
DanielRivers
force-pushed
the
feat/auth-url-params-improvement
branch
from
3469068 to
6bd3f87
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/SDK/KindeSDK.ts (1)
110-112: Ensure correct merging of additional parameters inloginmethod.The
loginmethod now acceptsLoginMethodParams | AdditionalParameters, enhancing flexibility. When mergingthis.additionalParametersandadditionalParameters, verify that there are no conflicting keys that might cause unintended overwrites. Consider explicitly handling key conflicts to preserve important parameters.__tests__/index.spec.ts (1)
110-111: Improve test coverage forisAdditionalParametersandadditionalParametersToLoginMethodParams.Currently, the mocked implementations always return
trueand an empty object, respectively. To enhance test robustness, add cases whereisAdditionalParametersreturnsfalseand whereadditionalParametersToLoginMethodParamsreturns populated objects. This will ensure the authentication flow handles different parameter scenarios correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
__tests__/index.spec.ts(1 hunks)src/SDK/KindeSDK.ts(2 hunks)src/SDK/OAuth/AuthorizationCode.ts(2 hunks)src/SDK/Utils.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/SDK/Utils.ts
[error] 228-228: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 229-229: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 230-230: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 231-231: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 232-232: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 233-233: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 234-234: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (1)
src/SDK/OAuth/AuthorizationCode.ts (1)
49-78: Verify that PKCE flow is correctly handled after refactoring.
The refactoring replaces explicit PKCE handling with the generateAuthUrl utility. Ensure that PKCE code challenge and verifier are correctly generated and utilized, maintaining compliance with OAuth 2.0 security standards. Confirm that authUrl.state and authUrl.codeVerifier contain the expected values.
DanielRivers
force-pushed
the
feat/auth-url-params-improvement
branch
from
6bd3f87 to
c8e8954
Compare
DanielRivers
force-pushed
the
feat/auth-url-params-improvement
branch
from
e4932f6 to
6674c8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/SDK/OAuth/AuthorizationCode.ts (2)
52-57: Consider adding error handling for parameter mapping.While the parameter mapping logic is clean, consider adding error handling for edge cases where the mapping might fail.
// Map additional parameters to the correct format of LoginOptions if (isAdditionalParameters(additionalParameters)) { + try { additionalParameters = additionalParametersToLoginMethodParams(additionalParameters); + } catch (error) { + console.error('Failed to map additional parameters:', error); + throw new Error('Invalid additional parameters provided'); + } }
58-78: Consider adding URL validation before opening the browser.The authentication flow is well-structured, but consider adding URL validation before opening the browser to ensure security.
const authUrl = await generateAuthUrl( kindeSDK.issuer, startPage === 'login' ? IssuerRouteTypes.login : IssuerRouteTypes.register, { ...(additionalParameters as LoginMethodParams), prompt: startPage === 'login' ? IssuerRouteTypes.login : IssuerRouteTypes.register, clientId: kindeSDK.clientId, redirectURL: kindeSDK.redirectUri, scope: kindeSDK.scope.split(' ') as Scopes[] } ); +// Validate the generated URL +const urlObj = new URL(authUrl.url.toString()); +if (!urlObj.protocol.startsWith('https')) { + throw new Error('Invalid authentication URL protocol'); +} Storage.setState(authUrl.state); Storage.setCodeVerifier(authUrl.codeVerifier); return OpenWebInApp(authUrl.url.toString(), kindeSDK, options);src/SDK/Utils.ts (1)
253-265: Add null checks in parameter mapping.The parameter mapping function should handle null/undefined values gracefully.
export const additionalParametersToLoginMethodParams = ( additionalParameters: AdditionalParameters ): Partial<LoginMethodParams> => { + if (!additionalParameters) { + return {}; + } return { - audience: additionalParameters.audience, - isCreateOrg: additionalParameters.is_create_org, - orgCode: additionalParameters.org_code, - orgName: additionalParameters.org_name, - connectionId: additionalParameters.connection_id, - lang: additionalParameters.lang, - loginHint: additionalParameters.login_hint + audience: additionalParameters.audience ?? undefined, + isCreateOrg: additionalParameters.is_create_org ?? undefined, + orgCode: additionalParameters.org_code ?? undefined, + orgName: additionalParameters.org_name ?? undefined, + connectionId: additionalParameters.connection_id ?? undefined, + lang: additionalParameters.lang ?? undefined, + loginHint: additionalParameters.login_hint ?? undefined }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!**/*.jsontsconfig.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
__tests__/index.spec.ts(1 hunks)src/SDK/KindeSDK.ts(2 hunks)src/SDK/OAuth/AuthorizationCode.ts(2 hunks)src/SDK/Utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/SDK/KindeSDK.ts
🔇 Additional comments (3)
src/SDK/OAuth/AuthorizationCode.ts (2)
29-35: Clean and well-organized imports from js-utils!
The new imports from @kinde/js-utils are properly structured and include all necessary types and utilities for the authentication flow.
48-51: Improved method signature with better type safety!
The updated signature removes the redundant usePKCE parameter and enhances type safety by accepting both LoginMethodParams and AdditionalParameters.
__tests__/index.spec.ts (1)
110-111: Well-structured mocks for new utility functions!
The mocks for isAdditionalParameters and additionalParametersToLoginMethodParams are properly set up and integrated into the test suite.
DanielRivers
force-pushed
the
feat/auth-url-params-improvement
branch
from
6674c8a to
e235388
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/SDK/OAuth/AuthorizationCode.ts (1)
Line range hint
38-49: Update JSDoc comments to match new method signatureThe JSDoc comments are outdated and don't reflect the current method signature. They still mention the removed
usePKCEparameter and show incorrect type information foradditionalParameters.Apply this diff to update the documentation:
/** * It opens the login page in the browser. * @param {KindeSDK} kindSDK - The KindeSDK instance - * @param {boolean} [usePKCE=false] - boolean = false * @param {'login' | 'registration'} [startPage=login] - 'login' | 'registration' = 'login' - * @param {AdditionalParameters} additionalParameters - AdditionalParameters = {} + * @param {LoginMethodParams | AdditionalParameters} additionalParameters - Additional parameters for login + * @param {AuthBrowserOptions} [options] - Browser options for authentication * @returns A promise that resolves when the URL is opened. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!**/*.jsontsconfig.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
__tests__/index.spec.ts(1 hunks)src/SDK/KindeSDK.ts(2 hunks)src/SDK/OAuth/AuthorizationCode.ts(2 hunks)src/SDK/Utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/SDK/Utils.ts
- src/SDK/KindeSDK.ts
- tests/index.spec.ts
🔇 Additional comments (5)
src/SDK/OAuth/AuthorizationCode.ts (5)
25-35: LGTM: Clean transition to js-utils library
The new imports effectively support the transition to using js-utils for auth URL generation while maintaining good separation of concerns with utility functions.
52-55: LGTM: Clean parameter type handling
The code effectively handles parameter type conversion using type guards and utility functions, ensuring type safety while maintaining flexibility.
75-78: LGTM: Clean auth flow implementation
The authentication flow is well-implemented:
- Proper state management using Storage
- Secure handling of code verifier
- Clean URL handling with OpenWebInApp
Line range hint 1-81: Verify authentication flow changes in integration tests
The transition to js-utils and removal of PKCE complexity represents a significant architectural change. While the implementation looks solid, ensure that:
- Integration tests cover the new authentication flow
- Error cases are properly handled
- The changes maintain backward compatibility if required
Run the following script to check test coverage:
✅ Verification successful
Authentication flow changes are well-tested and can be safely merged
The test coverage for the authentication flow is comprehensive, including:
- Full test suite for authentication lifecycle (login, register, logout)
- Error handling for missing parameters and failed authentication
- Token management and refresh flows
- Mock implementations for browser interactions
- Integration with js-utils through proper parameter mapping
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for authentication flow
# Look for relevant test cases
rg -A 5 "authenticate.*test"
# Check for error handling tests
ast-grep --pattern 'it($_, async () => {
$$$
expect($_).toThrow($_)
$$$
})'
Length of output: 115
Script:
#!/bin/bash
# Let's try a broader search for test files related to authentication and OAuth
fd -e test.ts -e spec.ts | xargs rg -l "AuthorizationCode|authenticate"
# Also search for any test files in the SDK directory
fd -e test.ts -e spec.ts . src/SDK/
# Look for test setup files that might contain authentication tests
fd -e test.ts -e spec.ts | xargs rg -l "describe.*Auth"
# Search for any files importing the AuthorizationCode class
rg "import.*AuthorizationCode" -A 2
Length of output: 392
Script:
#!/bin/bash
# Let's examine the test file content to understand the test coverage
cat __tests__/index.spec.ts
# Also check the KindeSDK implementation to see how AuthorizationCode is used
rg -A 10 "class KindeSDK" src/SDK/KindeSDK.ts
# Look for any mocks or test utilities that might be used in testing
fd -e ts | xargs rg -l "jest.mock|jest.spyOn"
Length of output: 27423
58-72: Verify scope string format before splitting
While the conversion to Scopes[] is type-safe, consider validating the scope string format before splitting to prevent runtime errors with invalid scopes.
Run the following script to check scope usage across the codebase:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/SDK/OAuth/AuthorizationCode.ts (1)
75-78: Consider adding error handling for storage operationsWhile the storage operations are straightforward, consider adding try-catch blocks to handle potential storage failures gracefully.
- Storage.setState(authUrl.state); - Storage.setCodeVerifier(authUrl.codeVerifier); - Storage.setNonce(authUrl.nonce); - Storage.setCodeChallenge(authUrl.codeChallenge); + try { + Storage.setState(authUrl.state); + Storage.setCodeVerifier(authUrl.codeVerifier); + Storage.setNonce(authUrl.nonce); + Storage.setCodeChallenge(authUrl.codeChallenge); + } catch (error) { + console.error('Failed to store auth parameters:', error); + throw new Error('Authentication failed: Unable to store parameters'); + }__tests__/index.spec.ts (1)
283-300: Consider adding negative test casesWhile the happy path is well tested, consider adding test cases for invalid additional parameters.
Example test case to add:
test('[RNStorage] Open registration endpoint with invalid parameters', async () => { InAppBrowser.isAvailable = jest.fn().mockReturnValue(true); const invalidParams = { invalid_param: 'test' }; await expect(globalClient.register(invalidParams)).rejects.toThrow(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
__tests__/index.spec.ts(5 hunks)src/SDK/KindeSDK.ts(3 hunks)src/SDK/OAuth/AuthorizationCode.ts(2 hunks)src/SDK/Storage/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/SDK/KindeSDK.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/SDK/Storage/index.ts
[error] 106-106: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 114-117: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
🔇 Additional comments (5)
src/SDK/OAuth/AuthorizationCode.ts (3)
29-35: LGTM: Good modularization using js-utils library
The transition to using @kinde/js-utils for auth-related functionality improves code maintainability and standardization.
58-73: LGTM: Improved URL generation with proper parameter handling
The new implementation using generateAuthUrl provides better parameter handling and type safety. The mapping of parameters is clear and well-structured.
52-55: Verify parameter mapping implementation
The parameter mapping logic needs verification to ensure all possible parameter types are handled correctly.
✅ Verification successful
Let me gather more information about the parameter mapping implementation.
Let me check one more thing to see the implementation of the parameter mapping function and type guard.
Parameter mapping implementation is correctly handling all defined types
The parameter mapping implementation is properly handling all possible parameter types. The code includes:
- A comprehensive type guard (
isAdditionalParameters) that checks for the presence of expected properties - A complete mapping function that converts all defined
AdditionalParametersfields to their correspondingLoginMethodParamsformat:- audience → audience
- is_create_org → isCreateOrg
- org_code → orgCode
- org_name → orgName
- connection_id → connectionId
- lang → lang
- login_hint → loginHint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for all possible parameter types being passed to authenticate method
# Test: Search for authenticate method calls with different parameter types
rg -A 3 "authenticate\("
Length of output: 744
Script:
#!/bin/bash
# Check the implementation of additionalParametersToLoginMethodParams and isAdditionalParameters
ast-grep --pattern 'function additionalParametersToLoginMethodParams($_) {
$$$
}'
# Also check the type definitions
ast-grep --pattern 'type AdditionalParameters = {
$$$
}'
# And LoginMethodParams type
ast-grep --pattern 'type LoginMethodParams = {
$$$
}'
Length of output: 639
Script:
#!/bin/bash
# Search for the implementation of isAdditionalParameters and additionalParametersToLoginMethodParams
rg -A 10 "isAdditionalParameters|additionalParametersToLoginMethodParams" --type ts
Length of output: 4015
__tests__/index.spec.ts (2)
87-91: LGTM: Proper mocking of utility functions
Good practice using requireActual for the parameter validation functions, ensuring actual implementation is tested.
248-263: LGTM: Comprehensive test coverage for auth parameters
The test properly verifies all required auth parameters are included in the URL.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
__tests__/index.spec.ts (2)
11-28: Improve algorithm name handling in crypto mockThe current implementation of algorithm name transformation might not handle all valid hash algorithm identifiers correctly.
Consider using a more robust approach:
-algorithm.replace('-', '').toLowerCase() +algorithm.toLowerCase().replace(/[^a-z0-9]/g, '')This change ensures proper handling of algorithm names with multiple special characters.
302-319: Consider extracting common URL parameters logicThe URL parameters construction is duplicated across login, register, and createOrg tests.
Consider extracting this into a helper function:
function getCommonAuthParams(state: string, nonce: string, codeChallenge: string) { return new URLSearchParams({ client_id: configuration.clientId || '', response_type: 'code', redirect_uri: configuration.redirectUri || '', audience: '', scope: configuration.scope || '', state, nonce, code_challenge: codeChallenge, code_challenge_method: 'S256' }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
__tests__/index.spec.ts(5 hunks)
🔇 Additional comments (3)
__tests__/index.spec.ts (3)
9-10: LGTM! Storage module integration
The addition of the Storage module import aligns with the PR's objective to improve authentication URL generation by using dynamic storage values.
267-282: LGTM! Enhanced login test implementation
The changes improve the test by:
- Using dynamic storage values instead of hardcoded ones
- Including the nonce parameter for enhanced security
- Following OAuth 2.0 best practices
389-405: LGTM! Proper organization creation test implementation
The changes correctly implement the organization creation test with:
- Dynamic storage values for security parameters
- Proper inclusion of the is_create_org flag
- Consistent URL parameter handling
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/SDK/OAuth/AuthorizationCode.ts (1)
60-62: Consider moving random string generation to js-utils.For consistency and maintainability, consider moving the
generateRandomStringandgenerateChallengefunctions to thejs-utilslibrary since it's now handling the auth URL generation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
src/SDK/OAuth/AuthorizationCode.ts(2 hunks)
🔇 Additional comments (3)
src/SDK/OAuth/AuthorizationCode.ts (3)
25-37: LGTM! Imports align with PR objectives.
The new imports from @kinde/js-utils support the transition to using the library for auth URL generation, while the additional utility imports enable proper type checking and parameter transformation.
54-57: LGTM! Robust parameter handling.
The implementation properly handles backward compatibility through type checking and parameter transformation, ensuring a smooth transition to the new parameter format.
51-51: Verify PKCE handling is maintained.
The removal of the usePKCE parameter suggests PKCE is now handled internally. While this simplifies the API, we should ensure PKCE is always enabled as it's crucial for security in mobile applications.
✅ Verification successful
Let me gather more information about the PKCE implementation by checking the actual code where it's being used.
Let me try a different approach to find the implementation.
PKCE is now mandatory and correctly implemented
The code review confirms that PKCE is now always enabled by default. The implementation shows that:
- Code challenge and verifier are automatically generated via
generateChallenge() - PKCE parameters are always included in the auth URL
- The code verifier is properly stored for the token exchange
- There's no conditional logic that would make PKCE optional
The removal of the usePKCE parameter is actually an improvement as it enforces better security practices by making PKCE mandatory rather than optional.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify PKCE is always enabled and not optional
# Test: Search for any conditional PKCE handling
rg -A 5 "PKCE|pkce|code_challenge"
Length of output: 2204
Script:
#!/bin/bash
# Search for the implementation of authenticate method to see how PKCE is handled
ast-grep --pattern 'async authenticate($$$) {
$$$
}'
Length of output: 56
Script:
#!/bin/bash
# Search for the authenticate method implementation in the AuthorizationCode.ts file
cat src/SDK/OAuth/AuthorizationCode.ts
Length of output: 3032
…gin-params feat: deprecate old additionalParameters
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/types/KindeSDK.ts (2)
22-26: Enhance deprecation notices with additional details.While the deprecation notices correctly indicate the replacement fields, they could be more informative. Consider adding:
- The version when these fields will be removed
- A migration guide or example of using the new fields
Example enhancement:
- /** @deprecated use isCreateOrg field instead */ + /** @deprecated Since version X.Y.Z. Will be removed in version A.B.C. + * Use isCreateOrg field instead. + * @example + * // Instead of: + * { is_create_org: true } + * // Use: + * { isCreateOrg: true } + */
22-31: Overall approach to deprecation looks good.The transition from snake_case to camelCase naming convention is a good improvement that aligns with JavaScript/TypeScript best practices. The deprecation notices maintain backward compatibility while encouraging migration to the new field names. This change supports the PR's objective of improving authentication parameter handling.
Consider adding a migration guide in the README or documentation to help users transition to the new field names smoothly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/types/KindeSDK.ts(1 hunks)
🔇 Additional comments (1)
src/types/KindeSDK.ts (1)
28-31: Consider grouping related fields together.
The lang field is placed between deprecated fields, which affects readability. Consider grouping all deprecated fields together and moving non-deprecated fields to a separate section.
Let's verify if these type changes are properly handled in the auth URL generation:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/SDK/OAuth/AuthorizationCode.ts (2)
41-56: Consider validating required parametersWhile the parameter setup looks good, consider adding validation for required parameters before proceeding with URL generation.
const { codeChallenge, codeVerifier, state } = generateChallenge(); const nonce = generateRandomString(); + // Validate required parameters + if (!kindeSDK.clientId || !kindeSDK.redirectUri || !kindeSDK.scope) { + throw new Error('Missing required parameters: clientId, redirectUri, or scope'); + } const params = {
66-69: Consider atomic storage operationsThe storage operations look good, but consider making them atomic to prevent partial state updates.
+ try { Storage.setState(state); Storage.setCodeVerifier(codeVerifier); Storage.setNonce(nonce); Storage.setCodeChallenge(codeChallenge); + } catch (error) { + // Rollback on failure + await Storage.clearAll(); + throw error; + }src/SDK/Storage/index.ts (2)
79-85: Consider adding type safety for stored valuesThe new storage methods look good, but consider adding type safety for stored values.
+ private readonly STORAGE_KEYS = { + nonce: 'nonce', + codeChallenge: 'codeChallenge', + state: 'state', + codeVerifier: 'codeVerifier' + } as const; getNonce() { - return this.getItem('nonce'); + return this.getItem(this.STORAGE_KEYS.nonce); } setNonce(newNonce: string): void { - this.setItem('nonce', this.convertString(newNonce)); + this.setItem(this.STORAGE_KEYS.nonce, this.convertString(newNonce)); }Also applies to: 87-92
79-92: Consider adding value validationConsider adding validation for the stored values to ensure they meet the required format.
+ private validateNonce(nonce: string): boolean { + return typeof nonce === 'string' && nonce.length >= 8; + } + private validateCodeChallenge(challenge: string): boolean { + return typeof challenge === 'string' && /^[A-Za-z0-9-._~]{43,128}$/.test(challenge); + } setNonce(newNonce: string): void { + if (!this.validateNonce(newNonce)) { + throw new Error('Invalid nonce format'); + } this.setItem('nonce', this.convertString(newNonce)); } setCodeChallenge(newCodeChallenge: string): void { + if (!this.validateCodeChallenge(newCodeChallenge)) { + throw new Error('Invalid code challenge format'); + } this.setItem('codeChallenge', this.convertString(newCodeChallenge)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!**/*.jsontsconfig.jsonis excluded by!**/*.json
📒 Files selected for processing (8)
src/SDK/Enums/TokenType.enum.ts(0 hunks)src/SDK/KindeSDK.ts(10 hunks)src/SDK/OAuth/AuthorizationCode.ts(2 hunks)src/SDK/Storage/Base.ts(0 hunks)src/SDK/Storage/RNStorage.ts(0 hunks)src/SDK/Storage/index.ts(1 hunks)src/SDK/Utils.ts(2 hunks)src/types/KindeSDK.ts(1 hunks)
💤 Files with no reviewable changes (3)
- src/SDK/Enums/TokenType.enum.ts
- src/SDK/Storage/RNStorage.ts
- src/SDK/Storage/Base.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/SDK/Utils.ts
- src/types/KindeSDK.ts
- src/SDK/KindeSDK.ts
🔇 Additional comments (5)
src/SDK/OAuth/AuthorizationCode.ts (4)
6-18: LGTM: Good modularization using js-utils
The transition to using @kinde/js-utils for auth URL generation and types is a good architectural decision that promotes code reuse and maintainability.
50-51: Add scope format validation
The code assumes the scope is space-separated. Consider adding validation or using a more robust scope parsing mechanism.
-scope: kindeSDK.scope.split(' ') as Scopes[],
+scope: kindeSDK.scope.trim().split(/\s+/).filter(Boolean) as Scopes[],58-64: LGTM: Proper URL generation and sanitization
Good practice using the js-utils library for URL generation and sanitizing the URL before opening it in the app.
Also applies to: 72-72
32-32: Verify type compatibility with js-utils
The parameter type change and mapping logic looks good, but we should verify compatibility with js-utils types.
Also applies to: 35-38
✅ Verification successful
Type compatibility with js-utils is properly handled
The code shows proper type compatibility handling between local AdditionalParameters and js-utils' LoginMethodParams:
- The
additionalParametersToLoginMethodParamsutility function correctly maps all fields fromAdditionalParameterstoLoginMethodParams - The
isAdditionalParameterstype guard ensures correct type detection - The authenticate method properly handles both types and performs the necessary conversion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check type compatibility between local types and js-utils
ast-grep --pattern 'type $type = $$$' src/types/KindeSDK.ts
ast-grep --pattern 'interface $interface { $$$ }' src/types/KindeSDK.ts
Length of output: 135
Script:
#!/bin/bash
# Let's try a different approach to check type definitions
# First, let's check the actual file content
cat src/SDK/OAuth/AuthorizationCode.ts
# Then check for any type imports from js-utils
rg -l "from.*js-utils.*" --type ts
# And check for LoginMethodParams and AdditionalParameters definitions
rg "type (LoginMethodParams|AdditionalParameters)" --type ts
rg "interface (LoginMethodParams|AdditionalParameters)" --type ts
Length of output: 3065
Script:
#!/bin/bash
# Let's check the LoginMethodParams type from js-utils and local AdditionalParameters type
cat src/types/KindeSDK.ts
# Check the utility functions that handle the type conversion
cat src/SDK/Utils.ts
Length of output: 10403
src/SDK/Storage/index.ts (1)
68-68: LGTM: Fixed void return types
Good fix removing the return statements from void methods.
Also applies to: 76-76
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/SDK/KindeSDK.ts (1)
204-205: Consider enhancing error handling for URL parsing.The URL parsing implementation is more concise with proper type safety through destructuring. However, consider adding type checking for the parsed URL parameters.
const URLParsed = new Url(url!, true); const { code, error, error_description: errorDescription -} = URLParsed.query; +} = URLParsed.query as { + code?: string; + error?: string; + error_description?: string; +};Also applies to: 233-238
📜 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 (3)
src/ApiClient.ts(0 hunks)src/SDK/KindeSDK.ts(10 hunks)src/SDK/Utils.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/ApiClient.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/SDK/Utils.ts
🔇 Additional comments (6)
src/SDK/KindeSDK.ts (6)
49-49: LGTM! Type safety improvements in constructor.
The constructor parameter type updates properly complement the class property type changes. Using Omit utility type ensures type safety by preventing the 'audience' parameter from being passed through the constructor.
Also applies to: 58-61
129-131: LGTM! Consistent type updates.
The register method's type updates maintain consistency with the login method changes, providing similar flexibility while preserving type safety.
158-160: LGTM! Proper parameter handling in createOrg.
The type updates using Omit utility and the addition of isCreateOrg parameter are well implemented. The parameter spreading ensures all values are properly passed to the register method.
Also applies to: 164-164
25-25: LGTM! Verify js-utils dependency.
The import of LoginMethodParams and type updates align well with the transition to use the js-utils library. The type definition provides better type safety through the Pick utility type.
Also applies to: 34-37
✅ Verification successful
Import from @kinde/js-utils is properly configured
The dependency @kinde/js-utils is correctly declared in package.json with version ^0.5.1-5, which supports the import of LoginMethodParams. The type updates and dependency configuration are aligned.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify js-utils dependency is properly declared
# Check if js-utils is declared in package.json
if [ -f "package.json" ]; then
echo "Checking @kinde/js-utils dependency..."
cat package.json | jq '.dependencies["@kinde/js-utils"]'
fi
Length of output: 182
98-100: LGTM! Verify login method usage patterns.
The type update to use union type with LoginMethodParams provides better flexibility while maintaining type safety.
✅ Verification successful
Let me gather more information about the LoginMethodParams type and its usage.
Type change is safe and properly handled throughout the codebase
The union type Partial<LoginMethodParams> | AdditionalParameters is safely implemented with proper type checking and conversion:
- The
isAdditionalParametersutility function correctly distinguishes between the two types - When
AdditionalParametersis provided, it's converted toLoginMethodParamsviaadditionalParametersToLoginMethodParams - All login method calls in tests pass without parameters, which is valid as the type is optional
- Type validation and conversion is properly handled in
AuthorizationCode.authenticate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check login method usage patterns in the codebase
# Search for login method calls to verify parameter usage
echo "Checking login method usage patterns..."
rg -A 2 "login\(" --type ts
Length of output: 1011
Script:
#!/bin/bash
# Check LoginMethodParams type definition and usage
echo "Checking LoginMethodParams type definition and usage..."
rg "LoginMethodParams" --type ts -A 3
# Check AdditionalParameters type definition
echo -e "\nChecking AdditionalParameters type definition..."
rg "AdditionalParameters" --type ts -A 3
Length of output: 14067
318-318: LGTM! Verify version consistency.
Using the version from package.json for the SDK header is a good practice that ensures version consistency.
✅ Verification successful
Version consistency verified successfully
The version is correctly imported from package.json (version: "1.3.1") and properly used in the SDK header as ReactNative/${version}. The implementation maintains version consistency as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify version in package.json
# Check if version is properly declared in package.json
if [ -f "package.json" ]; then
echo "Checking version declaration..."
cat package.json | jq '.version'
fi
Length of output: 139
Script:
#!/bin/bash
# Let's verify how the version is imported and used in KindeSDK.ts
ast-grep --pattern 'import { version } from $path'
# Also check the direct usage of version in KindeSDK.ts
rg "version.*ReactNative" -A 1 -B 1 src/SDK/KindeSDK.ts
Length of output: 110
Script:
#!/bin/bash
# Let's check the entire KindeSDK.ts file to see how version is imported and used
cat src/SDK/KindeSDK.ts
# Also check for any version-related imports in the file
rg "import.*version" src/SDK/KindeSDK.ts
Length of output: 23887
Explain your changes
Expands the auth login params to full options and switches auth URL generation to
js-utilsChecklist
🛟 If you need help, consider asking for advice over in the Kinde community.