feat: config in handle auth fn #183
Conversation
WalkthroughThe recent updates improve configuration management and error handling across the authentication and routing modules. Key enhancements include standardizing functions to use an options object for configuration details, updating redirection logic, and introducing new configuration types. These changes ensure more structured, reliable, and maintainable code by centralizing configuration details and improving error validation. Changes
Sequence DiagramsUpdated Callback Redirection LogicBeforesequenceDiagram
participant User
participant AppRouterClient
participant CallbackHandler
User ->> AppRouterClient: Request login
AppRouterClient ->> CallbackHandler: Redirect with postLoginRedirectURL
CallbackHandler ->> User: Redirected based on config.redirectURL
AftersequenceDiagram
participant User
participant AppRouterClient
participant CallbackHandler
User ->> AppRouterClient: Request login
AppRouterClient ->> CallbackHandler: Redirect with postLoginRedirectURL
CallbackHandler ->> User: Redirected based on routerClient.clientConfig.siteUrl
Configuration Initialization in AppRouterClientsequenceDiagram
participant Request
participant AppRouterClient
participant KindeServerClient
Request ->> AppRouterClient: Instantiate with options
AppRouterClient ->> AppRouterClient: Initialize clientConfig
AppRouterClient ->> KindeServerClient: create with clientConfig
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 8
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/handlers/auth.js (1 hunks)
- src/handlers/callback.js (1 hunks)
- src/routerClients/AppRouterClient.js (1 hunks)
- src/routerClients/PagesRouterClient.js (1 hunks)
- src/routerClients/RouterClient.js (1 hunks)
- types.d.ts (1 hunks)
Additional comments not posted (2)
src/handlers/callback.js (1)
28-30: Verify redirection logic consistency.The changes to the redirection logic after login should be verified to ensure they align with the new configuration settings and intended user flow.
Verification successful
Verify redirection logic consistency.
The redirection URLs for
postLoginRedirectURLandsiteUrlhave been correctly referenced and utilized across the codebase, aligning with the new configuration settings and intended user flow.
postLoginRedirectURLis defined and used appropriately insrc/handlers/callback.js,src/handlers/login.js,src/handlers/health.js,src/components/RegisterLink.js,src/config/index.js,src/authMiddleware/authMiddleware.js,src/components/LoginLink.js, andsrc/handlers/register.js.siteUrlis defined and used appropriately insrc/utils/generateCallbackUrl.js,src/routerClients/PagesRouterClient.js,src/routerClients/AppRouterClient.js,src/handlers/callback.js, andsrc/handlers/auth.js.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the redirection URLs are being set correctly according to the new configuration. # Test: Search for the usage of `postLoginRedirectURL` and `siteUrl`. rg --type python 'postLoginRedirectURL' 'siteUrl'Length of output: 78
Script:
#!/bin/bash # Description: Verify that the redirection URLs are being set correctly according to the new configuration. # Test: Search for the usage of `postLoginRedirectURL` and `siteUrl`. rg --type js 'postLoginRedirectURL' rg --type js 'siteUrl'Length of output: 3506
src/routerClients/RouterClient.js (1)
4-5: Initialization of clientConfig.Ensure that
clientConfigis properly initialized and used throughout the class to maintain consistency and avoid potential null reference errors.
| @@ -36,21 +36,29 @@ const getRoute = (endpoint) => { | |||
| /** | |||
| * @param {object} [request] | |||
| * @param {string} [endpoint] | |||
| * @param {{onError: (error: Error) => void}} [options] | |||
| * @param {{onError?: () => void; config: {audience?: string | string[], clientId?: string, clientSecret?: string, issuerURL?: string, siteUrl?: string, postLoginRedirectUrl?: string, postLogoutRedirectUrl?: string}}} options | |||
There was a problem hiding this comment.
Update function documentation to reflect the new options parameter.
The function now accepts a more complex options parameter. Updating the documentation to explain the structure and usage of this parameter would enhance code maintainability and understandability.
| if (!config.clientOptions.redirectURL && !options?.config?.siteUrl) | ||
| throw new Error( | ||
| "env variable 'KINDE_SITE_URL' is not set and not passed in options" | ||
| ); |
There was a problem hiding this comment.
Clarify the error message for missing site URL.
The current error message might confuse users as it mentions a different environment variable than what might be expected.
- "env variable 'KINDE_SITE_URL' is not set and not passed in options"
+ "The environment variable 'KINDE_SITE_URL' is required but not set. Please set it in your environment or pass it in the options."Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!config.clientOptions.redirectURL && !options?.config?.siteUrl) | |
| throw new Error( | |
| "env variable 'KINDE_SITE_URL' is not set and not passed in options" | |
| ); | |
| if (!config.clientOptions.redirectURL && !options?.config?.siteUrl) | |
| throw new Error( | |
| "The environment variable 'KINDE_SITE_URL' is required but not set. Please set it in your environment or pass it in the options." | |
| ); |
| if (!config.clientOptions.authDomain && !options?.config?.issuerURL) | ||
| throw new Error( | ||
| "env variable 'KINDE_ISSUER_URL' is not set and not passed in options" | ||
| ); |
There was a problem hiding this comment.
Consider refining the error message for clarity.
The error message could be more descriptive. Instead of just stating that the variable is not set, it could guide the user on how to set it or where to find more information.
- "env variable 'KINDE_ISSUER_URL' is not set and not passed in options"
+ "The environment variable 'KINDE_ISSUER_URL' is required but not set. Please set it in your environment or pass it in the options."Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!config.clientOptions.authDomain && !options?.config?.issuerURL) | |
| throw new Error( | |
| "env variable 'KINDE_ISSUER_URL' is not set and not passed in options" | |
| ); | |
| if (!config.clientOptions.authDomain && !options?.config?.issuerURL) | |
| throw new Error( | |
| "The environment variable 'KINDE_ISSUER_URL' is required but not set. Please set it in your environment or pass it in the options." | |
| ); |
| if (!config.clientOptions.clientId && !options?.config?.clientId) | ||
| throw new Error( | ||
| "env variable 'KINDE_CLIENT_ID' is not set and not passed in options" | ||
| ); |
There was a problem hiding this comment.
Consistent error handling for missing configuration.
The error handling for missing configuration values is good, but ensure that the messages are consistent across different checks to help in troubleshooting.
- "env variable 'KINDE_CLIENT_ID' is not set and not passed in options"
+ "The environment variable 'KINDE_CLIENT_ID' is required but not set. Please set it in your environment or pass it in the options."Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!config.clientOptions.clientId && !options?.config?.clientId) | |
| throw new Error( | |
| "env variable 'KINDE_CLIENT_ID' is not set and not passed in options" | |
| ); | |
| if (!config.clientOptions.clientId && !options?.config?.clientId) | |
| throw new Error( | |
| "The environment variable 'KINDE_CLIENT_ID' is required but not set. Please set it in your environment or pass it in the options." | |
| ); |
| if (!config.clientOptions.clientSecret && !options?.config?.clientSecret) | ||
| throw new Error( | ||
| "env variable 'KINDE_CLIENT_SECRET' is not set and not passed in options" | ||
| ); |
There was a problem hiding this comment.
Improve error message for missing client secret.
Similar to previous comments, the error message can be made more informative.
- "env variable 'KINDE_CLIENT_SECRET' is not set and not passed in options"
+ "The environment variable 'KINDE_CLIENT_SECRET' is required but not set. Please set it in your environment or pass it in the options."Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!config.clientOptions.clientSecret && !options?.config?.clientSecret) | |
| throw new Error( | |
| "env variable 'KINDE_CLIENT_SECRET' is not set and not passed in options" | |
| ); | |
| if (!config.clientOptions.clientSecret && !options?.config?.clientSecret) | |
| throw new Error( | |
| "The environment variable 'KINDE_CLIENT_SECRET' is required but not set. Please set it in your environment or pass it in the options." | |
| ); |
| * @param {{onError?: () => void; config: {audience?: string | string[], clientId?: string, clientSecret?: string, issuerURL?: string, siteUrl?: string, postLoginRedirectUrl?: string, postLogoutRedirectUrl?: string}}} options | ||
| */ | ||
| constructor(req, res, clientOptions) { | ||
| constructor(req, res, options) { | ||
| super(); | ||
| const url = req.url.split('/'); | ||
| url.pop(); | ||
|
|
||
| this.clientConfig = { | ||
| ...config.clientOptions, | ||
| audience: options?.config?.audience || config.clientOptions.audience, | ||
| authDomain: options?.config?.issuerURL || config.clientOptions.authDomain, | ||
| clientId: options?.config?.clientId || config.clientOptions.clientId, | ||
| clientSecret: | ||
| options?.config?.clientSecret || config.clientOptions.clientSecret, | ||
| logoutRedirectURL: | ||
| options?.config?.postLogoutRedirectUrl || | ||
| config.clientOptions.logoutRedirectURL, | ||
| redirectURL: | ||
| `${options?.config?.siteUrl}/api/auth/kinde_callback` || | ||
| config.clientOptions.redirectURL, | ||
| siteUrl: config.redirectURL || options.config.siteUrl | ||
| }; |
There was a problem hiding this comment.
Ensure robust configuration handling.
The constructor has been modified significantly to handle a new configuration structure. Ensure that all configuration properties are correctly validated and have sensible defaults to prevent runtime issues.
- this.clientConfig = {
+ this.clientConfig = options?.config || config.clientOptions;Committable suggestion was skipped due to low confidence.
| export type KindeClientConfig = { | ||
| redirectURL: string; | ||
| audience: string | string[]; | ||
| clientId: string; | ||
| clientSecret: string; | ||
| issuerURL?: string; | ||
| siteUrl?: string; | ||
| postLoginRedirectUrl?: string; | ||
| postLogoutRedirectUrl?: string; | ||
| authDomain: any; | ||
| logoutRedirectURL: any; | ||
| frameworkVersion: string; | ||
| framework: string; | ||
| }; |
There was a problem hiding this comment.
Review newly added KindeClientConfig type.
The new type KindeClientConfig is comprehensive and covers many necessary fields. However, ensure consistency in optional properties across the type definition to prevent potential runtime errors or misunderstandings.
- issuerURL?: string;
+ issuerURL: string;
- siteUrl?: string;
+ siteUrl: string;
- postLoginRedirectUrl?: string;
+ postLoginRedirectUrl: string;
- postLogoutRedirectUrl?: string;
+ postLogoutRedirectUrl: string;
- authDomain: any;
+ authDomain: string;
- logoutRedirectURL: any;
+ logoutRedirectURL: string;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type KindeClientConfig = { | |
| redirectURL: string; | |
| audience: string | string[]; | |
| clientId: string; | |
| clientSecret: string; | |
| issuerURL?: string; | |
| siteUrl?: string; | |
| postLoginRedirectUrl?: string; | |
| postLogoutRedirectUrl?: string; | |
| authDomain: any; | |
| logoutRedirectURL: any; | |
| frameworkVersion: string; | |
| framework: string; | |
| }; | |
| export type KindeClientConfig = { | |
| redirectURL: string; | |
| audience: string | string[]; | |
| clientId: string; | |
| clientSecret: string; | |
| issuerURL: string; | |
| siteUrl: string; | |
| postLoginRedirectUrl: string; | |
| postLogoutRedirectUrl: string; | |
| authDomain: string; | |
| logoutRedirectURL: string; | |
| frameworkVersion: string; | |
| framework: string; | |
| }; |
| * @param {{onError?: () => void; config: {audience?: string | string[], clientId?: string, clientSecret?: string, issuerURL?: string, siteUrl?: string, postLoginRedirectUrl?: string, postLogoutRedirectUrl?: string}}} options | ||
| */ | ||
| constructor(req, res, options) { | ||
| super(); | ||
| this.clientConfig = { | ||
| ...config.clientOptions, | ||
| audience: options?.config?.audience || config.clientOptions.audience, | ||
| authDomain: options?.config?.issuerURL || config.clientOptions.authDomain, | ||
| clientId: options?.config?.clientId || config.clientOptions.clientId, | ||
| clientSecret: | ||
| options?.config?.clientSecret || config.clientOptions.clientSecret, | ||
| logoutRedirectURL: | ||
| options?.config?.postLogoutRedirectUrl || | ||
| config.clientOptions.logoutRedirectURL, | ||
| redirectURL: | ||
| `${options?.config?.siteUrl}/api/auth/kinde_callback` || | ||
| config.clientOptions.redirectURL, | ||
| siteUrl: config.redirectURL || options.config.siteUrl | ||
| }; |
There was a problem hiding this comment.
Review configuration initialization in constructor.
The constructor now includes a more complex configuration object. Review and ensure that all properties are correctly initialized and that defaults are handled properly to avoid any configuration errors.
- this.clientConfig = {
+ this.clientConfig = options?.config || config.clientOptions;Committable suggestion was skipped due to low confidence.
feat: config in handle auth fn
Explain your changes
Suppose there is a related issue with enough detail for a reviewer to understand your changes fully. In that case, you can omit an explanation and instead include either “Fixes #XX” or “Updates #XX” where “XX” is the issue number.
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.