diff --git a/.changeset/combined-flow-sso-callback-sign-in-steps.md b/.changeset/combined-flow-sso-callback-sign-in-steps.md new file mode 100644 index 00000000000..62177f545ba --- /dev/null +++ b/.changeset/combined-flow-sso-callback-sign-in-steps.md @@ -0,0 +1,5 @@ +--- +'@clerk/ui': patch +--- + +Fixed `` in the combined sign-in-or-up flow sending users back to the start after an OAuth or SAML redirect when the sign-in still needed a step, such as a Clerk Protect check, a second factor, or a password reset. Sign-ups created from an OAuth sign-in, including after a Protect check, now continue to their remaining steps inside the component. diff --git a/packages/ui/src/components/SignIn/__tests__/SignInCombinedFlowSSOCallback.test.tsx b/packages/ui/src/components/SignIn/__tests__/SignInCombinedFlowSSOCallback.test.tsx new file mode 100644 index 00000000000..b8432b38d2b --- /dev/null +++ b/packages/ui/src/components/SignIn/__tests__/SignInCombinedFlowSSOCallback.test.tsx @@ -0,0 +1,143 @@ +import type { HandleOAuthCallbackParams, SignInResource } from '@clerk/shared/types'; +import { waitFor } from '@testing-library/react'; +import React from 'react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { bindCreateFixtures } from '@/test/create-fixtures'; +import { render } from '@/test/utils'; + +import { PathRouter } from '../../../router'; +import { SignIn } from '../index'; + +vi.mock('@clerk/shared/internal/clerk-js/protectCheck', () => ({ + executeProtectCheck: vi.fn(), +})); + +import { executeProtectCheck } from '@clerk/shared/internal/clerk-js/protectCheck'; + +const { createFixtures } = bindCreateFixtures('SignIn'); + +const mockExecute = executeProtectCheck as unknown as ReturnType; + +const signInProtectCheckFallbackUrl = `${window.location.origin}/sign-in#/protect-check`; + +type Fixtures = Awaited>['fixtures']; + +const setup = async (opts: { pendingOAuthTransfer?: boolean } = {}) => { + const { wrapper, fixtures, props } = await createFixtures(f => { + f.withEmailAddress(); + f.withSocialProvider({ provider: 'google' }); + f.withPasskey(); + f.withPasskeySettings({ allow_autofill: true, show_sign_in_button: false }); + f.startSignInWithProtectCheck( + opts.pendingOAuthTransfer ? { pendingOAuthTransfer: true, status: 'needs_identifier' } : undefined, + ); + }); + props.setProps({ routing: 'path', path: '/sign-in', withSignUp: true } as any); + + // @ts-expect-error - This is not a public API + fixtures.clerk.__internal_isWebAuthnAutofillSupported = () => Promise.resolve(true); + fixtures.signIn.authenticateWithPasskey.mockReturnValue(new Promise(() => {})); + vi.mocked(fixtures.clerk.navigate).mockImplementation((to: string) => { + const url = new URL(to, window.location.href); + if (url.origin === window.location.origin) { + window.history.pushState({}, '', url.href); + } + return Promise.resolve(); + }); + vi.mocked(fixtures.clerk.handleRedirectCallback).mockImplementation( + async (params: HandleOAuthCallbackParams, navigate?: (to: string) => Promise) => + navigate!(params.signInProtectCheckUrl || signInProtectCheckFallbackUrl), + ); + + return { wrapper, fixtures }; +}; + +const renderAtCallback = (wrapper: React.FC<{ children: React.ReactNode }>) => { + window.history.replaceState({}, '', '/sign-in/create/sso-callback'); + return render( + + + + + , + { wrapper }, + ); +}; + +const expectNoReplacementSignIn = (fixtures: Fixtures) => { + expect(fixtures.signIn.authenticateWithPasskey).not.toHaveBeenCalled(); + expect(fixtures.signIn.create).not.toHaveBeenCalled(); +}; + +describe('SignIn combined-flow SSO callback gated by a Protect check', () => { + beforeEach(() => { + mockExecute.mockReset(); + mockExecute.mockResolvedValue('proof-abc'); + }); + + it('runs the challenge for the signed-in OAuth attempt and activates the session', async () => { + const { wrapper, fixtures } = await setup(); + fixtures.signIn.submitProtectCheck.mockResolvedValue({ + status: 'complete', + protectCheck: null, + createdSessionId: 'sess_1', + } as unknown as SignInResource); + + renderAtCallback(wrapper); + + await waitFor(() => { + expect(fixtures.signIn.submitProtectCheck).toHaveBeenCalledWith({ proofToken: 'proof-abc' }); + }); + await waitFor(() => { + expect(fixtures.clerk.setActive).toHaveBeenCalledWith(expect.objectContaining({ session: 'sess_1' })); + }); + expect(window.location.pathname).toBe('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/sign-in/protect-check'); + expectNoReplacementSignIn(fixtures); + }); + + it('continues to the second factor after the challenge', async () => { + const { wrapper, fixtures } = await setup(); + fixtures.signIn.submitProtectCheck.mockResolvedValue({ + status: 'needs_second_factor', + protectCheck: null, + createdSessionId: null, + } as unknown as SignInResource); + + renderAtCallback(wrapper); + + await waitFor(() => { + expect(fixtures.clerk.navigate).toHaveBeenCalledWith( + expect.stringMatching(/^\/sign-in\/factor-two/), + expect.anything(), + ); + }); + expect(fixtures.signIn.submitProtectCheck).toHaveBeenCalledWith({ proofToken: 'proof-abc' }); + expectNoReplacementSignIn(fixtures); + }); + + it('continues an incomplete OAuth transfer inside the embedded sign-up routes', async () => { + const { wrapper, fixtures } = await setup({ pendingOAuthTransfer: true }); + fixtures.signIn.submitProtectCheck.mockResolvedValue({ + status: 'needs_identifier', + protectCheck: null, + createdSessionId: null, + firstFactorVerification: { status: 'transferable' }, + } as unknown as SignInResource); + vi.mocked(fixtures.clerk.__internal_resumeAfterProtectCheck).mockImplementation( + async (params: HandleOAuthCallbackParams = {}, navigate?: (to: string) => Promise) => + navigate!(params.continueSignUpUrl!), + ); + + renderAtCallback(wrapper); + + await waitFor(() => { + expect(fixtures.clerk.navigate).toHaveBeenCalledWith( + expect.stringMatching(/^\/sign-in\/create\/continue/), + expect.anything(), + ); + }); + expect(fixtures.signIn.submitProtectCheck).toHaveBeenCalledWith({ proofToken: 'proof-abc' }); + expectNoReplacementSignIn(fixtures); + }); +}); diff --git a/packages/ui/src/components/SignIn/__tests__/combinedFlowSSOCallbackRouting.test.tsx b/packages/ui/src/components/SignIn/__tests__/combinedFlowSSOCallbackRouting.test.tsx new file mode 100644 index 00000000000..d526cf7565d --- /dev/null +++ b/packages/ui/src/components/SignIn/__tests__/combinedFlowSSOCallbackRouting.test.tsx @@ -0,0 +1,200 @@ +import type { Clerk, HandleOAuthCallbackParams } from '@clerk/shared/types'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import React from 'react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { HashRouter, PathRouter, Route, useRouter, VirtualRouter } from '../../../router'; +import { buildCombinedFlowOAuthCallbackParams, buildSignInOAuthCallbackParams } from '../buildOAuthCallbackParams'; + +vi.mock('@clerk/shared/react', () => { + return { + useClerk: () => { + return { + navigate: () => Promise.resolve(), + } as unknown as Clerk; + }, + }; +}); + +const rootParams = buildSignInOAuthCallbackParams({ + signUpUrl: '/sign-in#/create', + signInUrl: '/sign-in', + signUpContinueUrl: '/sign-in#/create/continue', + signUpProtectCheckUrl: '/sign-in#/create/protect-check', + isCombinedFlow: true, +} as any); + +const createParams = buildCombinedFlowOAuthCallbackParams({ + signUpUrl: '/sign-in#/create', + signInUrl: '/sign-in', + secondFactorUrl: '/sign-in#/factor-two', +} as any); + +const destinations = { + signInProtectCheckUrl: 'protect-check', + firstFactorUrl: 'factor-one', + secondFactorUrl: 'factor-two', + resetPasswordUrl: 'reset-password', + continueSignUpUrl: 'create/continue', + verifyEmailAddressUrl: 'create/verify-email-address', + verifyPhoneNumberUrl: 'create/verify-phone-number', + signUpProtectCheckUrl: 'create/protect-check', +} as const; + +type Destination = keyof typeof destinations; + +const starts: Array<[string, HandleOAuthCallbackParams]> = [ + ['create/sso-callback', createParams], + ['sso-callback', rootParams], + ['protect-check', rootParams], +]; + +const cases = starts.flatMap(([start, params]) => + (Object.keys(destinations) as Destination[]) + .filter(key => !(start === destinations[key])) + .map(key => [start, key, destinations[key], params[key] as string] as const), +); + +const Marker = ({ at }: { at: string }) =>

{`at:${at}`}

; + +const NavigateButton = ({ to }: { to: string }) => { + const router = useRouter(); + return ( + + ); +}; + +const SignInRoutes = ({ start, to }: { start: string; to: string }) => { + const button = (at: string) => (start === at ? : null); + return ( + <> + + + + + + + + + + + + {button('protect-check')} + + {button('sso-callback')} + + + + + + + + + + + + + + {button('create/sso-callback')} + + + ); +}; + +const routers: Array<[string, (start: string, to: string) => React.ReactElement]> = [ + [ + 'path routing mounted at the root', + (start, to) => { + window.history.replaceState({}, '', `/sign-in/${start}`); + return ( + + + + ); + }, + ], + [ + 'path routing mounted at a nested path', + (start, to) => { + window.history.replaceState({}, '', `/auth/sign-in/${start}`); + return ( + + + + ); + }, + ], + [ + 'path routing reached with a trailing slash', + (start, to) => { + window.history.replaceState({}, '', `/sign-in/${start}/`); + return ( + + + + ); + }, + ], + [ + 'hash routing', + (start, to) => { + window.history.replaceState({}, '', `/#/${start}`); + return ( + + + + ); + }, + ], + [ + 'virtual routing', + (start, to) => { + window.history.replaceState({}, '', '/'); + return ( + + + + + + ); + }, + ], +]; + +describe('combined-flow SSO callback and protect-check navigation', () => { + beforeEach(() => { + window.history.replaceState({}, '', '/'); + }); + + describe.each(routers)('with %s', (_, renderRouter) => { + it.each(cases)('from %s, %s reaches the %s step', async (start, _key, destination, to) => { + render(renderRouter(start, to)); + expect(screen.queryByText(`at:${destination}`)).not.toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: 'go' })); + + expect(await screen.findByText(`at:${destination}`)).toBeInTheDocument(); + }); + }); +}); diff --git a/packages/ui/src/components/SignIn/buildOAuthCallbackParams.ts b/packages/ui/src/components/SignIn/buildOAuthCallbackParams.ts index e78b09401fe..30810905250 100644 --- a/packages/ui/src/components/SignIn/buildOAuthCallbackParams.ts +++ b/packages/ui/src/components/SignIn/buildOAuthCallbackParams.ts @@ -4,21 +4,27 @@ import type { HandleOAuthCallbackParams } from '@clerk/shared/types'; import type { SignInContextType } from '../../contexts/components/SignIn'; import type { SignUpContextType } from '../../contexts/components/SignUp'; +export const signUpStepUrls = (prefix: string) => ({ + continueSignUpUrl: `${prefix}continue`, + verifyEmailAddressUrl: `${prefix}verify-email-address`, + verifyPhoneNumberUrl: `${prefix}verify-phone-number`, + signUpProtectCheckUrl: `${prefix}protect-check`, +}); + export function buildSignInOAuthCallbackParams(ctx: SignInContextType): HandleOAuthCallbackParams { return { signUpUrl: ctx.signUpUrl, signInUrl: ctx.signInUrl, signInForceRedirectUrl: ctx.afterSignInUrl, signUpForceRedirectUrl: ctx.afterSignUpUrl, - continueSignUpUrl: ctx.signUpContinueUrl, transferable: ctx.transferable, firstFactorUrl: '../factor-one', secondFactorUrl: '../factor-two', resetPasswordUrl: '../reset-password', signInProtectCheckUrl: '../protect-check', - // Absolute + combined-flow-aware (see SignIn context), so it stays correct regardless of the - // callback route's depth. - signUpProtectCheckUrl: ctx.signUpProtectCheckUrl, + ...(ctx.isCombinedFlow + ? signUpStepUrls('../create/') + : { continueSignUpUrl: ctx.signUpContinueUrl, signUpProtectCheckUrl: ctx.signUpProtectCheckUrl }), unsafeMetadata: ctx.unsafeMetadata, }; } @@ -26,9 +32,6 @@ export function buildSignInOAuthCallbackParams(ctx: SignInContextType): HandleOA export function buildSignInOAuthTransportCallbackParams(ctx: SignInContextType): HandleOAuthCallbackParams { // Path form, not `#/step`: the in-place component router matches on pathname only and would drop the hash. const signUpStepUrl = (step: string): string => { - if (ctx.isCombinedFlow) { - return `create/${step}`; - } const url = buildURL({ base: ctx.signUpUrl }, { stringify: false }); url.pathname = `${trimTrailingSlash(url.pathname)}/${step}`; url.hash = ''; @@ -41,10 +44,14 @@ export function buildSignInOAuthTransportCallbackParams(ctx: SignInContextType): secondFactorUrl: 'factor-two', resetPasswordUrl: 'reset-password', signInProtectCheckUrl: 'protect-check', - continueSignUpUrl: signUpStepUrl('continue'), - verifyEmailAddressUrl: signUpStepUrl('verify-email-address'), - verifyPhoneNumberUrl: signUpStepUrl('verify-phone-number'), - signUpProtectCheckUrl: signUpStepUrl('protect-check'), + ...(ctx.isCombinedFlow + ? signUpStepUrls('create/') + : { + continueSignUpUrl: signUpStepUrl('continue'), + verifyEmailAddressUrl: signUpStepUrl('verify-email-address'), + verifyPhoneNumberUrl: signUpStepUrl('verify-phone-number'), + signUpProtectCheckUrl: signUpStepUrl('protect-check'), + }), }; } @@ -55,20 +62,24 @@ export function buildSignUpOAuthCallbackParams(ctx: SignUpContextType): HandleOA signUpForceRedirectUrl: ctx.afterSignUpUrl, signInForceRedirectUrl: ctx.afterSignInUrl, secondFactorUrl: ctx.secondFactorUrl, - continueSignUpUrl: '../continue', - verifyEmailAddressUrl: '../verify-email-address', - verifyPhoneNumberUrl: '../verify-phone-number', - signUpProtectCheckUrl: '../protect-check', + ...signUpStepUrls('../'), unsafeMetadata: ctx.unsafeMetadata, }; } +export function buildCombinedFlowOAuthCallbackParams(ctx: SignUpContextType): HandleOAuthCallbackParams { + return { + ...buildSignUpOAuthCallbackParams(ctx), + firstFactorUrl: '../../factor-one', + secondFactorUrl: '../../factor-two', + resetPasswordUrl: '../../reset-password', + signInProtectCheckUrl: '../../protect-check', + }; +} + export function buildSignUpOAuthTransportCallbackParams(ctx: SignUpContextType): HandleOAuthCallbackParams { return { ...buildSignUpOAuthCallbackParams(ctx), - continueSignUpUrl: 'continue', - verifyEmailAddressUrl: 'verify-email-address', - verifyPhoneNumberUrl: 'verify-phone-number', - signUpProtectCheckUrl: 'protect-check', + ...signUpStepUrls(''), }; } diff --git a/packages/ui/src/components/SignIn/handleSignUpIfMissingTransfer.ts b/packages/ui/src/components/SignIn/handleSignUpIfMissingTransfer.ts index 0643123c522..8a051bdff6d 100644 --- a/packages/ui/src/components/SignIn/handleSignUpIfMissingTransfer.ts +++ b/packages/ui/src/components/SignIn/handleSignUpIfMissingTransfer.ts @@ -5,6 +5,7 @@ import type { LoadedClerk } from '@clerk/shared/types'; import type { SignInContextType } from '../../contexts'; import type { RouteContextValue } from '../../router/RouteContext'; import { clerkWindowNavigate } from '../../utils/windowNavigate'; +import { signUpStepUrls } from './buildOAuthCallbackParams'; type HandleSignUpIfMissingTransferProps = { clerk: LoadedClerk; @@ -68,10 +69,7 @@ export async function handleSignUpIfMissingTransfer({ // email/phone identifications to their verify pages. return navigateToNextStepSignUp({ signUp: res, - continueSignUpUrl: '../create/continue', - verifyEmailAddressUrl: '../create/verify-email-address', - verifyPhoneNumberUrl: '../create/verify-phone-number', - signUpProtectCheckUrl: '../create/protect-check', + ...signUpStepUrls('../create/'), navigate, }); default: diff --git a/packages/ui/src/components/SignIn/index.tsx b/packages/ui/src/components/SignIn/index.tsx index 01c6a532497..0e900345cf4 100644 --- a/packages/ui/src/components/SignIn/index.tsx +++ b/packages/ui/src/components/SignIn/index.tsx @@ -20,7 +20,7 @@ import type { SignUpCtx } from '@/types'; import { SignInFactorOneSolanaWalletsCard } from '@/ui/components/SignIn/SignInFactorOneSolanaWalletsCard'; import { normalizeRoutingOptions } from '@/utils/normalizeRoutingOptions'; -import { buildSignInOAuthCallbackParams, buildSignUpOAuthCallbackParams } from './buildOAuthCallbackParams'; +import { buildCombinedFlowOAuthCallbackParams, buildSignInOAuthCallbackParams } from './buildOAuthCallbackParams'; import { LazySignUpContinue, LazySignUpProtectCheck, @@ -112,7 +112,7 @@ function SignInRoutes(): JSX.Element { - + { } const signUpContinueUrl = buildURL({ base: signUpUrl, hashPath: '/continue' }, { stringify: true }); - // Built off `signUpUrl`, which is rewritten to `#/create` in the combined flow, so this - // resolves to the embedded `…/create/protect-check` route there and the standalone sign-up route - // otherwise — keeping a Protect-gated sign-up inside whichever component is mounted. + // Built off `signUpUrl`, which is `#/create` in the combined flow. That hash form only reaches + // the embedded route on a fresh page load; navigation inside the mounted component uses relative `create/*` paths. const signUpProtectCheckUrl = buildURL({ base: signUpUrl, hashPath: '/protect-check' }, { stringify: true }); const navigateOnSetActive = async ({