fix: use connection name for single connection #1129
Conversation
WalkthroughThe changes in this pull request involve modifications to several files related to the OAuth2 authentication flow. Key updates include the addition of a logging statement in the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
src/routes/oauth2/authorize.ts (1)
Line range hint
168-175: Consider adding type safety for connection propertiesThe conditional block assumes the existence of both
strategyandnameproperties on connections. Consider adding type definitions or runtime validation to ensure these properties are always present and correctly typed.if ( client.connections.length === 1 && - !UI_STRATEGIES.includes(client.connections[0].strategy || "") + !UI_STRATEGIES.includes(client.connections[0].strategy ?? client.connections[0].name ?? "") ) { return socialAuth( ctx, client, client.connections[0].name, authParams, auth0Client, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/authentication-flows/oauth2.tsx(1 hunks)src/routes/oauth2/authorize.ts(1 hunks)src/routes/universal-login/routes.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/authentication-flows/oauth2.tsx
🔇 Additional comments (1)
src/routes/oauth2/authorize.ts (1)
Line range hint 178-179: Ensure consistent connection identification across flows
There's an inconsistency in how connections are identified between the single-connection flow (using connection.name) and the explicit connection flow (using the connection parameter directly). This could lead to confusion or bugs if these values don't match.
| @@ -169,7 +169,7 @@ export const authorizeRoutes = new OpenAPIHono<{ | |||
| return socialAuth( | |||
| ctx, | |||
| client, | |||
| client.connections[0].strategy, | |||
| client.connections[0].name, | |||
There was a problem hiding this comment.
💡 Codebase verification
Change from strategy to name is inconsistent with implementation
The socialAuth implementation in src/authentication-flows/oauth2.tsx still relies on the strategy property to determine the authentication provider and its specific handling logic. The code shows multiple conditional blocks checking connection.strategy for "apple", "google-oauth2", and "facebook". Changing to connection.name would break this logic.
Key issues found:
- socialAuth uses
connection.strategy === "apple"at line 83 - socialAuth uses
connection.strategy === "google-oauth2"at line 116 - socialAuth uses
connection.strategy === "facebook"at line 134
🔗 Analysis chain
Verify consistent usage of connection.name property
The change from strategy to name for identifying connections could impact how social authentication providers are handled. Let's verify this change is consistent with the socialAuth implementation and other connection usages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of connection properties across the codebase
# Check for other usages of connection.strategy
echo "Checking for connection.strategy usage:"
rg "connection.*\.strategy" -A 3
# Check for usages of connection.name
echo "Checking for connection.name usage:"
rg "connection.*\.name" -A 3
# Check the socialAuth function implementation
echo "Checking socialAuth implementation:"
ast-grep --pattern 'function socialAuth($_,$_,$_,$_,$_) { $$$ }'
Length of output: 14114
There was a problem hiding this comment.
Does auth0 user the name, the id of the connection to match it? What is being passed as connection in the querystring parameter to the authorize endpoint?
There was a problem hiding this comment.
@markusahlstrand, you're correct—Auth0 uses the name of the connection in the connection query parameter to match the connection. In this context, passing client.connections[0].name aligns with how Auth0 expects the parameter.
However, the socialAuth function currently relies on the strategy property to determine the authentication provider. It checks connection.strategy for values like "apple", "google-oauth2", and "facebook" to handle specific authentication flows.
Since we're now passing the name of the connection to socialAuth, we need to ensure that the internal logic of socialAuth is updated accordingly. Specifically, we should adjust the provider checks within socialAuth to use connection.name instead of connection.strategy. Alternatively, if the name and strategy properties have the same values in our configuration, this change might not cause issues.
To maintain consistency and prevent potential bugs, it's important to verify that all parts of the authentication flow are using the correct property. I recommend reviewing the socialAuth implementation to ensure it properly handles the change from strategy to name.
Let me know if you'd like assistance in updating socialAuth to reflect this change.
| } catch { | ||
| const user = await getPrimaryUserByEmailAndProvider({ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in code validation.
The current implementation discards valuable error information that could help in debugging and providing better user feedback.
Consider adding error logging and specific error handling:
- } catch {
+ } catch (error) {
+ console.error('Code validation failed:', error);
+ const errorMessage = error instanceof CustomException
+ ? error.message
+ : i18next.t("Wrong email or verification code.");
+
const user = await getPrimaryUserByEmailAndProvider({Committable suggestion skipped: line range outside the PR's diff.
| } catch (err: unknown) { | ||
| const vendorSettings = await fetchVendorSettings( | ||
| env, | ||
| client.id, | ||
| session.authParams.vendor_id, | ||
| ); | ||
|
|
||
| const error = err as Error; | ||
| return ctx.html( | ||
| <SignupPage | ||
| state={state} | ||
| vendorSettings={vendorSettings} | ||
| error={err.message} | ||
| error={error.message} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor error handling in signup process.
The current implementation has two issues:
- Refetching vendorSettings in the error handler is unnecessary as it's already available from the earlier call.
- Generic Error type casting might miss specific error information.
Consider this refactor:
- } catch (err: unknown) {
- const vendorSettings = await fetchVendorSettings(
- env,
- client.id,
- session.authParams.vendor_id,
- );
-
- const error = err as Error;
+ } catch (error: unknown) {
+ console.error('Signup failed:', error);
+ const errorMessage = error instanceof CustomException
+ ? error.message
+ : error instanceof Error
+ ? error.message
+ : i18next.t("signup_failed");
+
return ctx.html(
<SignupPage
state={state}
vendorSettings={vendorSettings}
- error={error.message}
+ error={errorMessage}
email={email}
/>,
400,
);Committable suggestion skipped: line range outside the PR's diff.
| } catch { | ||
| await env.data.passwords.update(client.tenant.id, passwordOptions); | ||
| } |
There was a problem hiding this comment.
Add error handling for password update failure.
The current implementation silently fails if both password creation and update operations fail. This could leave the system in an inconsistent state.
Consider adding proper error handling:
try {
await env.data.passwords.create(client.tenant.id, passwordOptions);
} catch {
+ try {
await env.data.passwords.update(client.tenant.id, passwordOptions);
+ } catch (error) {
+ console.error('Failed to update password:', error);
+ throw new HTTPException(500, { message: "Failed to reset password" });
+ }
}📝 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.
| } catch { | |
| await env.data.passwords.update(client.tenant.id, passwordOptions); | |
| } | |
| try { | |
| await env.data.passwords.create(client.tenant.id, passwordOptions); | |
| } catch { | |
| try { | |
| await env.data.passwords.update(client.tenant.id, passwordOptions); | |
| } catch (error) { | |
| console.error('Failed to update password:', error); | |
| throw new HTTPException(500, { message: "Failed to reset password" }); | |
| } | |
| } |
| } catch { | ||
| const log = createLogMessage(ctx, { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in pre-signup hook.
The current implementation catches all errors without logging them, making it difficult to debug issues in production.
Consider adding error logging and specific error handling:
try {
await preUserSignupHook(ctx, client, ctx.env.data, params.username);
- } catch {
+ } catch (error) {
+ console.error('Pre-signup hook failed:', error);
+ const errorMessage = error instanceof CustomException
+ ? error.message
+ : i18next.t("user_account_does_not_exist");
+
const log = createLogMessage(ctx, {
type: LogTypes.FAILED_SIGNUP,
- description: "Public signup is disabled",
+ description: `Public signup is disabled: ${error.message}`,
});Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Bug Fixes
Refactor
These changes enhance the overall user experience by providing clearer feedback during authentication processes.