diff --git a/.changeset/resume-oauth-transfer-after-protect-check.md b/.changeset/resume-oauth-transfer-after-protect-check.md new file mode 100644 index 00000000000..5883020c0ac --- /dev/null +++ b/.changeset/resume-oauth-transfer-after-protect-check.md @@ -0,0 +1,14 @@ +--- +'@clerk/clerk-js': patch +'@clerk/react': patch +'@clerk/shared': patch +'@clerk/ui': patch +--- + +Complete an OAuth account transfer that was interrupted by a verification challenge, instead of returning the user to the start of sign-in. + +Signing up with a social provider from the sign-in page works by transfer: the sign-in comes back with a transferable first-factor verification, and the client completes it as a sign-up. That continuation lives in the redirect-callback router, and a challenge on the sign-in short-circuits the router before it is reached. When the challenge cleared, the challenge card routed onward using only the interactive sign-in statuses, so a sign-in awaiting transfer fell through to the start form — which surfaced a stale `external_account_not_found` and reset the attempt, leaving a flow that could not be completed and reproduced on every retry. + +The card now hands back to the redirect-callback router, which resumes from where it stopped rather than starting over. The pending transfer is latched before the challenge runs, so it survives a response that re-serializes the sign-in without it. + +Also fixed alongside it: a stale or direct visit to the sign-in `protect-check` route now returns to the start of the flow instead of rendering an empty card, matching the sign-up side; and a failure in the SSO callback now shows a message and recovers, where previously the error handler could throw out of its own `catch` and leave the page loading indefinitely. diff --git a/packages/clerk-js/src/core/__tests__/clerk.test.ts b/packages/clerk-js/src/core/__tests__/clerk.test.ts index 55b8b91c9e1..3fb100a8d9f 100644 --- a/packages/clerk-js/src/core/__tests__/clerk.test.ts +++ b/packages/clerk-js/src/core/__tests__/clerk.test.ts @@ -1850,6 +1850,181 @@ describe('Clerk singleton', () => { expect(mockNavigate).not.toHaveBeenCalled(); }); + describe('__internal_resumeAfterProtectCheck', () => { + // A verification challenge can interrupt an OAuth callback partway through routing. The + // challenge card clears it and hands control back here, from a page that is no longer + // the callback route, so the remaining routing has to run rather than start over. + + const gatedTransferableSignIn = (extra: Record = {}) => + new SignIn({ + status: 'needs_identifier', + first_factor_verification: { + status: 'transferable', + strategy: 'oauth_google', + external_verification_redirect_url: '', + error: { + code: 'external_account_not_found', + long_message: 'The External Account was not found.', + message: 'Invalid external account', + }, + }, + second_factor_verification: null, + identifier: '', + user_data: null, + created_session_id: null, + created_user_id: null, + ...extra, + } as any as SignInJSON); + + const loadEnvironment = () => + mockEnvironmentFetch.mockReturnValue( + Promise.resolve({ + authConfig: {}, + userSettings: mockUserSettings, + displayConfig: mockDisplayConfig, + isSingleSession: () => false, + isProduction: () => false, + isDevelopmentOrStaging: () => true, + onWindowLocationHost: () => false, + }), + ); + + it('completes the transfer as a SIGN-UP and finalizes on the after-sign-up url', async () => { + loadEnvironment(); + mockClientFetch.mockReturnValue( + Promise.resolve({ + signedInSessions: [], + signIn: gatedTransferableSignIn(), + signUp: new SignUp(null), + }), + ); + + const mockSetActive = vi.fn(); + const mockSignUpCreate = vi + .fn() + .mockReturnValue(Promise.resolve({ status: 'complete', createdSessionId: '123' })); + + const sut = new Clerk(productionPublishableKey); + await sut.load(mockedLoadOptions); + if (!sut.client) { + fail('we should always have a client'); + } + sut.client.signUp.create = mockSignUpCreate; + sut.setActive = mockSetActive; + + await sut.__internal_resumeAfterProtectCheck({ continuation: 'transfer_to_sign_up' }); + + await waitFor(() => { + expect(mockSignUpCreate).toHaveBeenCalledTimes(1); + expect(mockSignUpCreate).toHaveBeenCalledWith({ transfer: true, unsafeMetadata: undefined }); + expect(mockSetActive).toHaveBeenCalledWith(expect.objectContaining({ session: '123' })); + }); + }); + + it('completes the transfer even when the cleared response dropped the transferable marker', async () => { + // `SignIn.fromJSON` replaces `firstFactorVerification` wholesale on every write, so the + // caller latches the continuation before running the challenge and passes it explicitly. + // Re-reading it here would silently fall back to returning the user to sign-in. + loadEnvironment(); + mockClientFetch.mockReturnValue( + Promise.resolve({ + signedInSessions: [], + signIn: new SignIn({ + status: 'needs_identifier', + first_factor_verification: null, + second_factor_verification: null, + identifier: '', + user_data: null, + created_session_id: null, + created_user_id: null, + } as any as SignInJSON), + signUp: new SignUp(null), + }), + ); + + const mockSignUpCreate = vi + .fn() + .mockReturnValue(Promise.resolve({ status: 'complete', createdSessionId: '123' })); + + const sut = new Clerk(productionPublishableKey); + await sut.load(mockedLoadOptions); + if (!sut.client) { + fail('we should always have a client'); + } + sut.client.signUp.create = mockSignUpCreate; + sut.setActive = vi.fn(); + + await sut.__internal_resumeAfterProtectCheck({ continuation: 'transfer_to_sign_up' }); + + await waitFor(() => + expect(mockSignUpCreate).toHaveBeenCalledWith({ transfer: true, unsafeMetadata: undefined }), + ); + }); + + it('does not bounce back into the challenge when a stale gate is still on the resource', async () => { + // This is the test that proves `resuming` is load-bearing rather than decorative. The + // caller IS the challenge card; re-checking the gate here would hand control straight + // back to it, or — through the sign-up arm — to the wrong card entirely. + loadEnvironment(); + mockClientFetch.mockReturnValue( + Promise.resolve({ + signedInSessions: [], + signIn: gatedTransferableSignIn({ + protect_check: { status: 'pending', token: 'stale-token', sdk_url: 'https://example.com/sdk.js' }, + }), + signUp: new SignUp(null), + }), + ); + + const mockSignUpCreate = vi + .fn() + .mockReturnValue(Promise.resolve({ status: 'complete', createdSessionId: '123' })); + + const sut = new Clerk(productionPublishableKey); + await sut.load(mockedLoadOptions); + if (!sut.client) { + fail('we should always have a client'); + } + sut.client.signUp.create = mockSignUpCreate; + sut.setActive = vi.fn(); + + await sut.__internal_resumeAfterProtectCheck({ continuation: 'transfer_to_sign_up' }); + + await waitFor(() => + expect(mockSignUpCreate).toHaveBeenCalledWith({ transfer: true, unsafeMetadata: undefined }), + ); + expect(mockNavigate).not.toHaveBeenCalledWith(expect.stringContaining('protect-check'), expect.anything()); + }); + + it('still honours transferable: false', async () => { + loadEnvironment(); + mockClientFetch.mockReturnValue( + Promise.resolve({ + signedInSessions: [], + signIn: gatedTransferableSignIn(), + signUp: new SignUp(null), + }), + ); + + const mockSignUpCreate = vi.fn(); + + const sut = new Clerk(productionPublishableKey); + await sut.load(mockedLoadOptions); + if (!sut.client) { + fail('we should always have a client'); + } + sut.client.signUp.create = mockSignUpCreate; + sut.setActive = vi.fn(); + + await sut.__internal_resumeAfterProtectCheck({ + continuation: 'transfer_to_sign_up', + transferable: false, + }); + + await waitFor(() => expect(mockSignUpCreate).not.toHaveBeenCalled()); + }); + }); + it('does not initiate the transfer flow when transferable: false is passed', async () => { mockEnvironmentFetch.mockReturnValue( Promise.resolve({ diff --git a/packages/clerk-js/src/core/clerk.ts b/packages/clerk-js/src/core/clerk.ts index 9b41d90341f..9376efa4bc6 100644 --- a/packages/clerk-js/src/core/clerk.ts +++ b/packages/clerk-js/src/core/clerk.ts @@ -112,6 +112,7 @@ import type { PublicKeyCredentialWithAuthenticatorAttestationResponse, RedirectOptions, Resources, + ResumeAfterProtectCheckParams, SDKMetadata, SessionResource, SessionTouchParams, @@ -2450,15 +2451,22 @@ export class Clerk implements ClerkInterface { }; private _handleRedirectCallback = async ( - params: HandleOAuthCallbackParams, + params: ResumeAfterProtectCheckParams, { signIn, signUp, navigate, + resuming = false, }: { signIn: SignInResource; signUp: SignUpResource; navigate: (to: string) => Promise; + /** + * Set when this is re-entered after a verification challenge the caller has already + * cleared, which skips the two gate short-circuits below. Without it the resumed flow + * bounces straight back into the card it was resumed from. + */ + resuming?: boolean; }, ): Promise => { if (!this.loaded || !this.environment || !this.client) { @@ -2602,14 +2610,18 @@ export class Clerk implements ClerkInterface { // sign-in's challenge. We only consult `si` here unless this is explicitly a sign-up callback. // Transfers are unaffected: the `signIn.create({ transfer })` path below checks its own fresh // response for the gate. - if (params.reloadResource !== 'signUp' && (si.protectCheck || si.status === 'needs_protect_check')) { + // + // Both gate checks are skipped when `resuming`: the caller IS the challenge card, so + // re-checking would either bounce control back into it, or — through the sign-up arm + // below, on a stale gate — hand it to the wrong card entirely. + if (!resuming && params.reloadResource !== 'signUp' && (si.protectCheck || si.status === 'needs_protect_check')) { return navigateToSignInProtectCheck(); } // The sign-up resource can be gated the same way (e.g. a callback that resolves straight into a // gated sign-up). Scope to the sign-up intent for the symmetric reason — a stale sign-up's gate // shouldn't hijack a sign-in callback. - if (params.reloadResource !== 'signIn' && su.protectCheck) { + if (!resuming && params.reloadResource !== 'signIn' && su.protectCheck) { return navigateToSignUpProtectCheck(); } @@ -2669,7 +2681,11 @@ export class Clerk implements ClerkInterface { return navigateToResetPassword(); } - const userNeedsToBeCreated = si.firstFactorVerificationStatus === 'transferable'; + // `SignIn.fromJSON` replaces `firstFactorVerification` wholesale on every write, so a + // caller that observed the pending transfer BEFORE clearing a challenge cannot rely on + // the marker still being here afterwards. It tells us instead of us re-reading it. + const userNeedsToBeCreated = + si.firstFactorVerificationStatus === 'transferable' || params.continuation === 'transfer_to_sign_up'; if (userNeedsToBeCreated) { if (params.transferable === false) { @@ -2772,6 +2788,32 @@ export class Clerk implements ClerkInterface { return navigateToSignIn(); }; + public __internal_resumeAfterProtectCheck = async ( + params: ResumeAfterProtectCheckParams = {}, + customNavigate?: (to: string) => Promise, + ): Promise => { + if (!this.loaded || !this.environment || !this.client) { + return; + } + const { signIn, signUp } = this.client; + + // Deliberately mirrors `handleRedirectCallback` rather than + // `__internal_handleResourceCallback`: the latter runs every path through + // `buildUrlWithAuth`, which resolves a relative path against the ORIGIN on development + // instances and so turns `../factor-one` into an absolute URL, losing the component + // router's context. + const resolvedNavigate = customNavigate ?? params.__internal_navigate; + const navigate = (to: string) => + resolvedNavigate && typeof resolvedNavigate === 'function' ? resolvedNavigate(to) : this.navigate(to); + + return this._handleRedirectCallback(params, { + signUp, + signIn, + navigate, + resuming: true, + }); + }; + public handleRedirectCallback = async ( params: HandleOAuthCallbackParams = {}, customNavigate?: (to: string) => Promise, diff --git a/packages/react/src/isomorphicClerk.ts b/packages/react/src/isomorphicClerk.ts index f04a9de9258..6560dcbb9e1 100644 --- a/packages/react/src/isomorphicClerk.ts +++ b/packages/react/src/isomorphicClerk.ts @@ -49,6 +49,7 @@ import type { ProtectAssertion, RedirectOptions, Resources, + ResumeAfterProtectCheckParams, SetActiveParams, SignInProps, SignInRedirectOptions, @@ -1595,6 +1596,17 @@ export class IsomorphicClerk implements IsomorphicLoadedClerk { } }; + __internal_resumeAfterProtectCheck = async (params?: ResumeAfterProtectCheckParams): Promise => { + const callback = () => this.clerkjs?.__internal_resumeAfterProtectCheck(params); + if (this.clerkjs && this.loaded) { + void callback()?.catch(() => { + // Same React 18 strict-mode double-mount caveat as handleRedirectCallback above. + }); + } else { + this.premountMethodCalls.set('__internal_resumeAfterProtectCheck', callback); + } + }; + handleGoogleOneTapCallback = async ( signInOrUp: SignInResource | SignUpResource, params: HandleOAuthCallbackParams, diff --git a/packages/shared/src/types/clerk.ts b/packages/shared/src/types/clerk.ts index 403a85d50c6..4e267e9d7ef 100644 --- a/packages/shared/src/types/clerk.ts +++ b/packages/shared/src/types/clerk.ts @@ -1155,6 +1155,22 @@ export interface Clerk { customNavigate?: (to: string) => Promise, ) => Promise; + /** + * Resumes redirect-callback routing after a verification challenge has been cleared, from + * a page that is no longer the callback route. + * + * A challenge can interrupt a callback partway through routing, on a step whose + * continuation is not one of the interactive sign-in cards — an OAuth sign-in that has to + * become a sign-up, for instance. Once the challenge clears, the flow has to pick up where + * the callback left off rather than start over. + * + * @internal + */ + __internal_resumeAfterProtectCheck: ( + params?: ResumeAfterProtectCheckParams, + customNavigate?: (to: string) => Promise, + ) => Promise; + /** * Completes an email link verification flow started by `Clerk.client.signIn.createEmailLinkFlow` or `Clerk.client.signUp.createEmailLinkFlow`, by processing the verification results from the redirect URL query parameters. This method should be called after the user is redirected back from visiting the verification link in their email. * @@ -1352,6 +1368,26 @@ export type HandleOAuthCallbackParams = TransferableOption & export type HandleSamlCallbackParams = HandleOAuthCallbackParams; +/** + * The continuation a caller observed on the resource *before* it ran a verification + * challenge. Supplied explicitly, because resolving a challenge re-serializes the sign-in + * and sign-up resources and can drop the marker the router would otherwise read back off + * them. + * + * @internal + */ +export type ProtectCheckContinuation = 'transfer_to_sign_up'; + +export type ResumeAfterProtectCheckParams = HandleOAuthCallbackParams & { + /** + * What the flow was doing before the challenge interrupted it. See + * {@link ProtectCheckContinuation}. + * + * @internal + */ + continuation?: ProtectCheckContinuation; +}; + /** * A function used to navigate to a given URL after certain steps in the Clerk processes. * diff --git a/packages/ui/bundlewatch.config.json b/packages/ui/bundlewatch.config.json index f40753d60b1..06083331324 100644 --- a/packages/ui/bundlewatch.config.json +++ b/packages/ui/bundlewatch.config.json @@ -6,7 +6,7 @@ { "path": "./dist/framework*.js", "maxSize": "44KB" }, { "path": "./dist/vendors*.js", "maxSize": "73KB" }, { "path": "./dist/ui-common*.js", "maxSize": "133KB" }, - { "path": "./dist/signin*.js", "maxSize": "17KB" }, + { "path": "./dist/signin*.js", "maxSize": "18KB" }, { "path": "./dist/signup*.js", "maxSize": "13KB" }, { "path": "./dist/userprofile*.js", "maxSize": "16KB" }, { "path": "./dist/organizationprofile*.js", "maxSize": "13KB" }, diff --git a/packages/ui/src/common/SSOCallback.tsx b/packages/ui/src/common/SSOCallback.tsx index 374de774e37..a4ac198af8e 100644 --- a/packages/ui/src/common/SSOCallback.tsx +++ b/packages/ui/src/common/SSOCallback.tsx @@ -29,8 +29,21 @@ export const SSOCallbackCard = (props: HandleOAuthCallbackParams | HandleSamlCal const intent = new URLSearchParams(window.location.search).get('intent'); const reloadResource = intent === 'signIn' || intent === 'signUp' ? intent : undefined; handleRedirectCallback({ ...props, reloadResource }, navigate).catch(e => { - handleError(e, [], card.setError); + // Schedule the bounce FIRST, and never let the error reporting escape this handler. + // + // `handleError` re-throws anything it does not recognise, and the callback's own + // "did not complete" guards throw a plain `Error` — which it does not. A throw from + // inside this `.catch` skipped BOTH statements below, so the user got no message and + // no recovery: the card sat on its spinner indefinitely while the failure surfaced + // only as an unhandled rejection in the console. Every callback dead-end was + // invisible for that reason, which is a bad property for a route whose whole job is + // to be the last step of somebody's sign-in. timeoutId = setTimeout(() => void navigate('../'), 4000); + try { + handleError(e, [], card.setError); + } catch { + card.setError('Unable to complete action at this time. If the problem persists please contact support.'); + } }); } diff --git a/packages/ui/src/components/SignIn/SignInProtectCheck.tsx b/packages/ui/src/components/SignIn/SignInProtectCheck.tsx index 0024975407d..91298080658 100644 --- a/packages/ui/src/components/SignIn/SignInProtectCheck.tsx +++ b/packages/ui/src/components/SignIn/SignInProtectCheck.tsx @@ -1,5 +1,6 @@ import { useClerk } from '@clerk/shared/react'; import type { SignInResource } from '@clerk/shared/types'; +import { useEffect, useRef, useState } from 'react'; import { Card } from '@/ui/elements/Card'; import { useCardState, withCardStateProvider } from '@/ui/elements/contexts'; @@ -19,50 +20,46 @@ import { useLocalizations, } from '../../customizables'; import { useSpinDelay } from '../../hooks'; +import { useNavigateToFlowStart } from '../../hooks/useNavigateToFlowStart'; import { useProtectCheckRunner } from '../../hooks/useProtectCheckRunner'; import { useRouter } from '../../router'; +import { buildSignInOAuthCallbackParams } from './buildOAuthCallbackParams'; +import { isSignInPendingOAuthTransfer, resumeSignInAfterProtectCheck } from './handleProtectCheck'; -/** - * Routes the user to the next step after a protect check has been resolved (or short-circuits - * to the same route to handle a chained challenge). - * - * After the gate clears, the client should retry the operation that was gated. - * For most steps (factor-one/factor-two cards), the underlying card uses `useFetch` to call - * `prepareFirstFactor`/`prepareSecondFactor` on mount, so navigating back is sufficient to - * re-trigger the gated work. - */ -function navigateNext(signIn: SignInResource, navigate: (to: string) => Promise): Promise { - // Chained challenge — stay here and re-run the new challenge on next render. Both - // signals are checked: `protectCheck` is the authoritative field, and - // `'needs_protect_check'` is the SDK-version-gated status. - if (signIn.protectCheck || signIn.status === 'needs_protect_check') { - return navigate('.'); - } - - switch (signIn.status) { - case 'needs_first_factor': - return navigate('../factor-one'); - case 'needs_second_factor': - return navigate('../factor-two'); - case 'needs_client_trust': - return navigate('../client-trust'); - case 'needs_new_password': - return navigate('../reset-password'); - case 'complete': - // Finalization is handled by the caller via setActive; just bounce to index. - return navigate('..'); - default: - return navigate('..'); - } -} - -function SignInProtectCheckInternal(): JSX.Element { +function SignInProtectCheckInternal(): JSX.Element | null { const card = useCardState(); const { t } = useLocalizations(); const signIn = useCoreSignIn(); const { navigate } = useRouter(); - const { setActive } = useClerk(); - const { afterSignInUrl, navigateOnSetActive } = useSignInContext(); + const { navigateToFlowStart } = useNavigateToFlowStart(); + const clerk = useClerk(); + const { setActive, __internal_resumeAfterProtectCheck } = clerk; + const ctx = useSignInContext(); + const { afterSignInUrl, navigateOnSetActive } = ctx; + + // Latched at mount, BEFORE the challenge is submitted. `SignIn.fromJSON` replaces + // `firstFactorVerification` wholesale on every write, so the transferable marker that routed + // us here is not guaranteed to survive `submitProtectCheck` — and it is the only thing that + // distinguishes "an OAuth sign-up is in progress" from "an ordinary gated sign-in". + const startedAsOAuthTransfer = useRef(isSignInPendingOAuthTransfer(signIn)); + + // Latches that a protect check existed at some point, so the resolution race + // (submitProtectCheck clearing protectCheck mid-navigation) isn't mistaken for a stale + // visit. Mirrors SignUpProtectCheck, which has had this since it shipped. State adjusted + // during render (guarded) rather than a ref write, which React disallows in the render body. + const [everSawProtectCheck, setEverSawProtectCheck] = useState(!!signIn.protectCheck); + const didStartNoCheckFallbackRef = useRef(false); + + if (signIn.protectCheck && !everSawProtectCheck) { + setEverSawProtectCheck(true); + } + + useEffect(() => { + if (!signIn.protectCheck && !everSawProtectCheck && !didStartNoCheckFallbackRef.current) { + didStartNoCheckFallbackRef.current = true; + void navigateToFlowStart(); + } + }, [everSawProtectCheck, navigateToFlowStart, signIn.protectCheck]); const { containerRef, isRunning, isWidgetVisible, hasError, retry } = useProtectCheckRunner({ getProtectCheck: () => signIn.protectCheck, @@ -85,7 +82,26 @@ function SignInProtectCheckInternal(): JSX.Element { }); return; } - await navigateNext(updatedSignIn, navigate); + await resumeSignInAfterProtectCheck(updatedSignIn, { + navigate, + startedAsOAuthTransfer: startedAsOAuthTransfer.current, + // No isCancelled() guard around this one: completing the transfer calls setActive, + // which flips the withRedirectToAfterSignIn guard and blanks this card. That unmount + // must not abort the continuation — the router owns its navigation from here. + resumeOAuthContinuation: () => + __internal_resumeAfterProtectCheck( + { + ...buildSignInOAuthCallbackParams(ctx), + continuation: 'transfer_to_sign_up', + // Carried for the same reason the social buttons carry it: without it a + // completed transfer whose session has a pending task is routed with the + // component's base URL rather than its mounted route, which lands on + // `#/tasks/...` instead of `#/create/tasks/...` in the combined flow. + __internal_navigateOnSetActive: ctx.navigateOnSetActive, + }, + navigate, + ), + }); }, }); @@ -96,6 +112,13 @@ function SignInProtectCheckInternal(): JSX.Element { // resolves" guarantee, nor keep a spinner next to the retry button. const showSpinner = useSpinDelay(isRunning, { delay: 300 }); + // Stale/direct visit that never had a check: render nothing while the flow-start redirect + // scheduled above kicks in, instead of flashing the card shell for one paint. Must stay + // below every hook call. + if (!signIn.protectCheck && !everSawProtectCheck) { + return null; + } + return ( diff --git a/packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx b/packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx index 1c60b1187d4..0034213069b 100644 --- a/packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx +++ b/packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx @@ -467,4 +467,101 @@ describe('SignInProtectCheck', () => { expect(fixtures.signIn.submitProtectCheck).toHaveBeenCalledWith({ proofToken: 'proof-retry' }); }); }); + + describe('a sign-in that is pending an OAuth account transfer', () => { + // An OAuth sign-in for an identity with no account yet comes back as `needs_identifier` + // with a transferable first-factor verification: the server has recorded the transfer and + // the client is expected to complete it as a sign-up. None of the interactive sign-in + // steps apply, so before this the status fell to the default arm and returned to the + // start form — which renders the transfer's error and then resets the attempt, discarding + // the transfer for good. + + it('resumes the callback continuation instead of returning to the start form', async () => { + const { wrapper, fixtures } = await createFixtures(f => { + f.startSignInWithProtectCheck({ pendingOAuthTransfer: true, status: 'needs_identifier' }); + }); + mockExecute.mockResolvedValue('proof-abc'); + fixtures.signIn.submitProtectCheck.mockResolvedValue({ + status: 'needs_identifier', + protectCheck: null, + createdSessionId: null, + firstFactorVerification: { status: 'transferable' }, + } as unknown as SignInResource); + + render(, { wrapper }); + + await waitFor(() => { + expect(fixtures.clerk.__internal_resumeAfterProtectCheck).toHaveBeenCalledWith( + expect.objectContaining({ continuation: 'transfer_to_sign_up' }), + expect.any(Function), + ); + }); + expect(fixtures.router.navigate).not.toHaveBeenCalledWith('..'); + }); + + it('resumes even when the resolved sign-in no longer carries the transferable marker', async () => { + // `SignIn.fromJSON` replaces `firstFactorVerification` wholesale on every write, so the + // marker that routed us here is not guaranteed to survive `submitProtectCheck`. The + // component latches it at mount for exactly this case; re-reading it afterwards would + // silently fall back to the broken path. + const { wrapper, fixtures } = await createFixtures(f => { + f.startSignInWithProtectCheck({ pendingOAuthTransfer: true, status: 'needs_identifier' }); + }); + mockExecute.mockResolvedValue('proof-abc'); + fixtures.signIn.submitProtectCheck.mockResolvedValue({ + status: 'needs_identifier', + protectCheck: null, + createdSessionId: null, + firstFactorVerification: { status: null }, + } as unknown as SignInResource); + + render(, { wrapper }); + + await waitFor(() => { + expect(fixtures.clerk.__internal_resumeAfterProtectCheck).toHaveBeenCalledWith( + expect.objectContaining({ continuation: 'transfer_to_sign_up' }), + expect.any(Function), + ); + }); + expect(fixtures.router.navigate).not.toHaveBeenCalledWith('..'); + }); + + it('leaves an ordinary gated sign-in on the existing path', async () => { + // The guard above must not divert every gated sign-in into the OAuth router. + const { wrapper, fixtures } = await createFixtures(f => { + f.startSignInWithProtectCheck(); + }); + mockExecute.mockResolvedValue('proof-abc'); + fixtures.signIn.submitProtectCheck.mockResolvedValue({ + status: 'needs_identifier', + protectCheck: null, + createdSessionId: null, + firstFactorVerification: { status: null }, + } as unknown as SignInResource); + + render(, { wrapper }); + + await waitFor(() => expect(fixtures.router.navigate).toHaveBeenCalledWith('..')); + expect(fixtures.clerk.__internal_resumeAfterProtectCheck).not.toHaveBeenCalled(); + }); + }); + + it('routes stale standalone protect-check visits back to the flow start', async () => { + // The sign-up card has had this guard since it shipped; without it this card renders an + // empty shell forever on a back-button or a bookmarked URL. + const { wrapper, fixtures } = await createFixtures(f => { + f.startSignInWithEmailAddress(); + }); + fixtures.router.currentPath = '/sign-in/protect-check'; + fixtures.router.fullPath = '/sign-in'; + fixtures.router.indexPath = '/sign-in'; + + const { queryByText } = render(, { wrapper }); + + // The card shell must not flash while the redirect below kicks in. + expect(queryByText(/verifying your request/i)).not.toBeInTheDocument(); + + await waitFor(() => expect(fixtures.router.navigate).toHaveBeenCalledWith('/sign-in')); + expect(mockExecute).not.toHaveBeenCalled(); + }); }); diff --git a/packages/ui/src/components/SignIn/handleProtectCheck.ts b/packages/ui/src/components/SignIn/handleProtectCheck.ts index 1c061cc66b8..d0b21687a8c 100644 --- a/packages/ui/src/components/SignIn/handleProtectCheck.ts +++ b/packages/ui/src/components/SignIn/handleProtectCheck.ts @@ -36,3 +36,78 @@ export function navigateOnSignInProtectGate( } return false; } + +/** + * Whether this sign-in is waiting to become a sign-up. + * + * An OAuth sign-in for an identity that has no account yet comes back as a *transferable* + * first-factor verification: the server has recorded the account transfer and the client is + * expected to complete it as a sign-up. It is not a sign-in that can continue on its own, + * and none of the interactive sign-in steps apply to it. + * + * Read this BEFORE clearing a gate, never after — see `resumeSignInAfterProtectCheck`. + */ +export function isSignInPendingOAuthTransfer(signIn: SignInResource): boolean { + return signIn.firstFactorVerification?.status === 'transferable'; +} + +/** + * The exit choke point, and the counterpart to `navigateOnSignInProtectGate` above. + * + * The gate has two halves and both live in this file: one for routing *into* the challenge, + * one for routing *out* of it. A new caller needs both — a card that enters through the + * helper and then hand-rolls its own exit is exactly the shape that produced the outage this + * function was written for. + * + * `resumeOAuthContinuation` is how the card hands back to the redirect-callback router. It is + * injected rather than called directly so this module stays free of the Clerk instance. + */ +export function resumeSignInAfterProtectCheck( + signIn: SignInResource, + { + navigate, + resumeOAuthContinuation, + startedAsOAuthTransfer, + }: { + navigate: (to: string) => Promise; + resumeOAuthContinuation: () => Promise; + startedAsOAuthTransfer: boolean; + }, +): Promise { + // Chained challenge — stay here and re-run the new challenge on next render. Both + // signals are checked: `protectCheck` is the authoritative field, and + // `'needs_protect_check'` is the SDK-version-gated status. + if (isSignInProtectGated(signIn)) { + return navigate('.'); + } + + switch (signIn.status) { + case 'needs_first_factor': + return navigate('../factor-one'); + case 'needs_second_factor': + return navigate('../factor-two'); + case 'needs_client_trust': + return navigate('../client-trust'); + case 'needs_new_password': + return navigate('../reset-password'); + case 'complete': + // Finalization is handled by the caller via setActive; just bounce to index. + return startedAsOAuthTransfer || isSignInPendingOAuthTransfer(signIn) + ? resumeOAuthContinuation() + : navigate('..'); + default: + // Everything above is an interactive sign-in step the user can be shown. Anything + // else means this sign-in cannot continue on its own, and today that is an OAuth + // account transfer: `needs_identifier` carrying a transferable first-factor + // verification, whose continuation lives in the redirect-callback router. + // + // Returning to the start form instead is not merely a wrong destination — the start + // card renders the transfer's `external_account_not_found` error and then calls + // `signIn.create({})` to clear it, which replaces the attempt and discards the only + // reference to the pending transfer. The user is then stranded permanently, and every + // retry reproduces it. + return startedAsOAuthTransfer || isSignInPendingOAuthTransfer(signIn) + ? resumeOAuthContinuation() + : navigate('..'); + } +} diff --git a/packages/ui/src/test/fixture-helpers.ts b/packages/ui/src/test/fixture-helpers.ts index 51320d1077f..e6a60243b49 100644 --- a/packages/ui/src/test/fixture-helpers.ts +++ b/packages/ui/src/test/fixture-helpers.ts @@ -239,15 +239,39 @@ const createSignInFixtureHelpers = (baseClient: ClientJSON) => { expiresAt?: number; uiHints?: Record; sdkUrl?: string; + /** + * Set for an OAuth sign-in that has no account yet: the server has recorded the account + * transfer and marked the first factor `transferable`, so the flow's continuation is a + * sign-up rather than any interactive sign-in step. + */ + pendingOAuthTransfer?: boolean; + /** Overrides the gated status; `needs_identifier` is what a pending transfer carries. */ + status?: string; }) => { - const { expiresAt, uiHints, sdkUrl = 'https://protect.example.com/sdk.js' } = params || {}; + const { + expiresAt, + uiHints, + sdkUrl = 'https://protect.example.com/sdk.js', + pendingOAuthTransfer = false, + status = 'needs_protect_check', + } = params || {}; baseClient.sign_in = { id: 'sia_2HseAXFGN12eqlwARPMxyyUa9o9', - status: 'needs_protect_check', + status, identifier: 'test@clerk.com', supported_first_factors: [], supported_second_factors: [], - first_factor_verification: null, + first_factor_verification: pendingOAuthTransfer + ? { + status: 'transferable', + strategy: 'oauth_google', + error: { + code: 'external_account_not_found', + message: 'Invalid external account', + long_message: 'The External Account was not found.', + }, + } + : null, second_factor_verification: null, created_session_id: null, protect_check: {