-
Notifications
You must be signed in to change notification settings - Fork 437
feat(clerk-js): Sign up if missing #8030
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
145d697
6df492a
0cb052d
f84547c
36bf49c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@clerk/clerk-js': minor | ||
| '@clerk/shared': minor | ||
| --- | ||
|
|
||
| Support `sign_up_if_missing` on SignIn.create, including captcha |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,13 +173,6 @@ export class SignUp extends BaseResource implements SignUpResource { | |
| finalParams = { ...finalParams, ...captchaParams }; | ||
| } | ||
|
|
||
| if (finalParams.transfer && this.shouldBypassCaptchaForAttempt(finalParams)) { | ||
| const strategy = SignUp.clerk.client?.signIn.firstFactorVerification.strategy; | ||
| if (strategy) { | ||
| finalParams = { ...finalParams, strategy: strategy as SignUpCreateParams['strategy'] }; | ||
| } | ||
| } | ||
|
|
||
| return this._basePost({ | ||
| path: this.pathRoot, | ||
| body: normalizeUnsafeMetadata(finalParams), | ||
|
|
@@ -561,22 +554,30 @@ export class SignUp extends BaseResource implements SignUpResource { | |
| * We delegate bot detection to the following providers, instead of relying on turnstile exclusively | ||
| */ | ||
| protected shouldBypassCaptchaForAttempt(params: SignUpCreateParams) { | ||
| if (!params.strategy) { | ||
| return false; | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const captchaOauthBypass = SignUp.clerk.__internal_environment!.displayConfig.captchaOauthBypass; | ||
|
|
||
| if (captchaOauthBypass.some(strategy => strategy === params.strategy)) { | ||
| return true; | ||
| // Check for transfer captcha bypass. | ||
| if (params.transfer) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new logic explicitly checks transfer first and checks both paths that can lead to a bypass: OAuth strategy, and sign up if missing. We fall through on EnterpriseSSO transfers, which then ends up returning |
||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const signInVerificationStrategy = SignUp.clerk.client!.signIn.firstFactorVerification.strategy; | ||
|
|
||
| // OAuth transfers: If we delegate captcha detection to OAuth provider, | ||
| // do not show another captcha on sign up. | ||
| if (captchaOauthBypass.some(strategy => strategy === signInVerificationStrategy)) { | ||
| return true; | ||
| } | ||
|
|
||
| // Sign up if missing transfers: We let sign in handle the captcha, | ||
| // do not show another captcha on sign up. | ||
| if (isSignUpIfMissingCaptchaBypassStrategy(signInVerificationStrategy)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if ( | ||
| params.transfer && | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| captchaOauthBypass.some(strategy => strategy === SignUp.clerk.client!.signIn.firstFactorVerification.strategy) | ||
| ) { | ||
| // OAuth sign ups: If we delegate captcha detection to OAuth provider, | ||
| // do not show another captcha on sign up. | ||
| if (params.strategy && captchaOauthBypass.some(strategy => strategy === params.strategy)) { | ||
| return true; | ||
| } | ||
|
|
||
|
|
@@ -595,6 +596,22 @@ export class SignUp extends BaseResource implements SignUpResource { | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true if the given strategy is one where captcha is already handled | ||
| * by the sign-in attempt with sign up if missing, so a subsequent sign-up should | ||
| * not show another captcha. Matches email_link, email_code, phone_code, and any | ||
| * web3 wallet strategy. This should be kept in sync with `validateSignUpIfMissing` | ||
| * in the backend. | ||
| */ | ||
| const SIGN_UP_IF_MISSING_CAPTCHA_BYPASS_STRATEGIES = new Set(['email_link', 'email_code', 'phone_code']); | ||
|
|
||
| export function isSignUpIfMissingCaptchaBypassStrategy(strategy: string | null): boolean { | ||
| if (!strategy) { | ||
| return false; | ||
| } | ||
| return SIGN_UP_IF_MISSING_CAPTCHA_BYPASS_STRATEGIES.has(strategy) || strategy.startsWith('web3_'); | ||
| } | ||
|
|
||
| type SignUpFutureVerificationsMethods = Pick< | ||
| SignUpFutureVerifications, | ||
| | 'sendEmailCode' | ||
|
|
@@ -787,22 +804,30 @@ class SignUpFuture implements SignUpFutureResource { | |
| } | ||
|
|
||
| private shouldBypassCaptchaForAttempt(params: { strategy?: string; transfer?: boolean }) { | ||
| if (!params.strategy) { | ||
| return false; | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const captchaOauthBypass = SignUp.clerk.__internal_environment!.displayConfig.captchaOauthBypass; | ||
|
|
||
| if (captchaOauthBypass.some(strategy => strategy === params.strategy)) { | ||
| return true; | ||
| // Check for transfer captcha bypass. | ||
| if (params.transfer) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's exactly the same change for sign up future. |
||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const signInVerificationStrategy = SignUp.clerk.client!.signIn.firstFactorVerification.strategy; | ||
|
|
||
| // OAuth transfers: If we delegate captcha detection to OAuth provider, | ||
| // do not show another captcha on sign up. | ||
| if (captchaOauthBypass.some(strategy => strategy === signInVerificationStrategy)) { | ||
| return true; | ||
| } | ||
|
|
||
| // Sign up if missing transfers: We let sign in handle the captcha, | ||
| // do not show another captcha on sign up. | ||
| if (isSignUpIfMissingCaptchaBypassStrategy(signInVerificationStrategy)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if ( | ||
| params.transfer && | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| captchaOauthBypass.some(strategy => strategy === SignUp.clerk.client!.signIn.firstFactorVerification.strategy) | ||
| ) { | ||
| // OAuth sign ups: If we delegate captcha detection to OAuth provider, | ||
| // do not show another captcha on sign up. | ||
| if (params.strategy && captchaOauthBypass.some(strategy => strategy === params.strategy)) { | ||
| return true; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the previous logic, which unconditionally showed captchas on EnterpriseSSO and OAuth transfers, since transfers do not have a strategy. The former behavior is correct, the latter was a bug.