diff --git a/.changeset/enterprise-sso-hand-off-challenge.md b/.changeset/enterprise-sso-hand-off-challenge.md index dcef54d036e..7efdbabed59 100644 --- a/.changeset/enterprise-sso-hand-off-challenge.md +++ b/.changeset/enterprise-sso-hand-off-challenge.md @@ -3,4 +3,4 @@ '@clerk/ui': patch --- -Fix enterprise SSO sign-ins erroring instead of showing a verification challenge raised while handing off to the identity provider. +Fix enterprise SSO sign-ins erroring, or appearing to do nothing, instead of showing a verification challenge raised while handing off to the identity provider. This covers the card for choosing between multiple enterprise connections, where clicking a connection left the user on an unchanged card. diff --git a/.changeset/resume-enterprise-sso-after-challenge.md b/.changeset/resume-enterprise-sso-after-challenge.md new file mode 100644 index 00000000000..60f3a9be89e --- /dev/null +++ b/.changeset/resume-enterprise-sso-after-challenge.md @@ -0,0 +1,5 @@ +--- +'@clerk/ui': patch +--- + +Fix sign-ins that use an enterprise connection stranding on "Use another method" after a verification challenge, instead of continuing to the identity provider. diff --git a/packages/ui/src/components/SignIn/SignInFactorOneEnterpriseConnections.tsx b/packages/ui/src/components/SignIn/SignInFactorOneEnterpriseConnections.tsx index 8bf166d4a62..bfa31dacdf9 100644 --- a/packages/ui/src/components/SignIn/SignInFactorOneEnterpriseConnections.tsx +++ b/packages/ui/src/components/SignIn/SignInFactorOneEnterpriseConnections.tsx @@ -8,6 +8,8 @@ import { Flow, localizationKeys } from '@/ui/customizables'; import { withCardStateProvider } from '@/ui/elements/contexts'; import type { AvailableComponentProps } from '@/ui/types'; +import { useRouter } from '../../router'; +import { navigateOnSignInProtectGate } from './handleProtectCheck'; import { hasMultipleEnterpriseConnections } from './shared'; /** @@ -16,6 +18,7 @@ import { hasMultipleEnterpriseConnections } from './shared'; const SignInFactorOneEnterpriseConnectionsInternal = () => { const ctx = useSignInContext(); const clerk = useClerk(); + const { navigate } = useRouter(); const signIn = clerk.client.signIn; if (!hasMultipleEnterpriseConnections(signIn.supportedFirstFactors)) { @@ -28,11 +31,11 @@ const SignInFactorOneEnterpriseConnectionsInternal = () => { name: ff.enterpriseConnectionName, })); - const handleEnterpriseSSO = (enterpriseConnectionId: string) => { + const handleEnterpriseSSO = async (enterpriseConnectionId: string) => { const redirectUrl = ctx.ssoCallbackUrl; const redirectUrlComplete = ctx.afterSignInUrl || '/'; - return signIn.authenticateWithRedirect({ + await signIn.authenticateWithRedirect({ strategy: 'enterprise_sso', redirectUrl, redirectUrlComplete, @@ -40,6 +43,11 @@ const SignInFactorOneEnterpriseConnectionsInternal = () => { continueSignIn: true, enterpriseConnectionId, }); + + // Preparing the hand-off can itself raise a challenge, in which case no redirect was issued + // and the sign-in is sitting on the gate instead. Without this the picker looks inert: the + // user clicks their connection and nothing happens. + navigateOnSignInProtectGate(signIn, navigate, '../protect-check'); }; return ( diff --git a/packages/ui/src/components/SignIn/SignInProtectCheck.tsx b/packages/ui/src/components/SignIn/SignInProtectCheck.tsx index b71845be905..7ea2fb90cdf 100644 --- a/packages/ui/src/components/SignIn/SignInProtectCheck.tsx +++ b/packages/ui/src/components/SignIn/SignInProtectCheck.tsx @@ -24,7 +24,11 @@ import { useNavigateToFlowStart } from '../../hooks/useNavigateToFlowStart'; import { useProtectCheckRunner } from '../../hooks/useProtectCheckRunner'; import { useRouter } from '../../router'; import { buildSignInOAuthCallbackParams } from './buildOAuthCallbackParams'; -import { isSignInPendingOAuthTransfer, resumeSignInAfterProtectCheck } from './handleProtectCheck'; +import { + isSignInPendingOAuthTransfer, + isSignInProtectGated, + resumeSignInAfterProtectCheck, +} from './handleProtectCheck'; function SignInProtectCheckInternal(): JSX.Element | null { const card = useCardState(); @@ -78,6 +82,25 @@ function SignInProtectCheckInternal(): JSX.Element | null { } await resumeSignInAfterProtectCheck(updatedSignIn, { navigate, + // No `enterpriseConnectionId` is passed: this runs only under + // `shouldHandOffToEnterpriseConnection`, which requires a single connection, so the server + // has exactly one to prepare. If that guard is ever loosened to resume a connection the + // user chose, the id has to be carried across the challenge and passed here. + resumeEnterpriseSSO: async () => { + await signIn.authenticateWithRedirect({ + strategy: 'enterprise_sso', + redirectUrl: ctx.ssoCallbackUrl, + redirectUrlComplete: afterSignInUrl || '/', + oidcPrompt: ctx.oidcPrompt, + continueSignIn: true, + }); + + // Preparing the hand-off can raise a further challenge, in which case no redirect was + // issued: stay here and run it on the next render. + if (isSignInProtectGated(signIn)) { + await navigate('.'); + } + }, startedAsOAuthTransfer: startedAsOAuthTransfer.current, resumeOAuthContinuation: () => typeof __internal_resumeAfterProtectCheck === 'function' diff --git a/packages/ui/src/components/SignIn/SignInStart.tsx b/packages/ui/src/components/SignIn/SignInStart.tsx index 81b34dcf9a0..059818c8b93 100644 --- a/packages/ui/src/components/SignIn/SignInStart.tsx +++ b/packages/ui/src/components/SignIn/SignInStart.tsx @@ -38,13 +38,10 @@ import { useLoadingStatus } from '../../hooks'; import { useSupportEmail } from '../../hooks/useSupportEmail'; import { useTotalEnabledAuthMethods } from '../../hooks/useTotalEnabledAuthMethods'; import { useRouter } from '../../router'; +import { hasOnlyEnterpriseSSOFirstFactors, shouldHandOffToEnterpriseConnection } from './enterpriseSSOFactors'; import { handleCombinedFlowTransfer } from './handleCombinedFlowTransfer'; import { navigateOnSignInProtectGate } from './handleProtectCheck'; -import { - hasMultipleEnterpriseConnections, - SIGN_IN_RESET_PASSWORD_INTENT_PARAM, - useHandleAuthenticateWithPasskey, -} from './shared'; +import { SIGN_IN_RESET_PASSWORD_INTENT_PARAM, useHandleAuthenticateWithPasskey } from './shared'; import { SignInAlternativePhoneCodePhoneNumberCard } from './SignInAlternativePhoneCodePhoneNumberCard'; import { SignInSocialButtons } from './SignInSocialButtons'; import { @@ -241,7 +238,7 @@ function SignInStartInternal(): JSX.Element { } switch (res.status) { case 'needs_first_factor': { - if (!hasOnlyEnterpriseSSOFirstFactors(res) || hasMultipleEnterpriseConnections(res.supportedFirstFactors)) { + if (!shouldHandOffToEnterpriseConnection(res)) { return navigate('factor-one'); } @@ -418,7 +415,7 @@ function SignInStartInternal(): JSX.Element { } break; case 'needs_first_factor': { - if (!hasOnlyEnterpriseSSOFirstFactors(res) || hasMultipleEnterpriseConnections(res.supportedFirstFactors)) { + if (!shouldHandOffToEnterpriseConnection(res)) { if (options?.resetPasswordIntent) { return navigate('factor-one', { searchParams: new URLSearchParams({ [SIGN_IN_RESET_PASSWORD_INTENT_PARAM]: 'true' }), @@ -722,14 +719,6 @@ function SignInStartInternal(): JSX.Element { ); } -const hasOnlyEnterpriseSSOFirstFactors = (signIn: SignInResource): boolean => { - if (!signIn.supportedFirstFactors?.length) { - return false; - } - - return signIn.supportedFirstFactors.every(ff => ff.strategy === 'enterprise_sso'); -}; - const InstantPasswordRow = ({ field, onForgotPasswordClick, diff --git a/packages/ui/src/components/SignIn/__tests__/SignInFactorOneEnterpriseConnections.test.tsx b/packages/ui/src/components/SignIn/__tests__/SignInFactorOneEnterpriseConnections.test.tsx new file mode 100644 index 00000000000..f470103a47e --- /dev/null +++ b/packages/ui/src/components/SignIn/__tests__/SignInFactorOneEnterpriseConnections.test.tsx @@ -0,0 +1,62 @@ +import type { SignInResource } from '@clerk/shared/types'; +import { waitFor } from '@testing-library/react'; +import { describe, expect, it } from 'vitest'; + +import { bindCreateFixtures } from '@/test/create-fixtures'; +import { render, screen } from '@/test/utils'; + +import { SignInFactorOneEnterpriseConnections } from '../SignInFactorOneEnterpriseConnections'; + +const { createFixtures } = bindCreateFixtures('SignIn'); + +/** Two connections is what puts the user on this card rather than a direct hand-off. */ +const TWO_CONNECTIONS = [ + { strategy: 'enterprise_sso', enterpriseConnectionId: 'ent_acme', enterpriseConnectionName: 'Acme SSO' }, + { strategy: 'enterprise_sso', enterpriseConnectionId: 'ent_globex', enterpriseConnectionName: 'Globex SSO' }, +]; + +describe('SignInFactorOneEnterpriseConnections', () => { + it('routes to the challenge when preparing the hand-off raises one', async () => { + // GIVEN a user choosing between two enterprise connections + const { wrapper, fixtures } = await createFixtures(f => { + f.withEmailAddress(); + f.startSignInWithEmailAddress(); + }); + (fixtures.signIn as unknown as SignInResource).supportedFirstFactors = TWO_CONNECTIONS as never; + // WHEN preparing the hand-off comes back gated: no redirect is issued, the call just resolves. + fixtures.signIn.authenticateWithRedirect.mockImplementationOnce(() => { + (fixtures.signIn as any).protectCheck = { status: 'pending', token: 'challenge-token-abc' }; + return Promise.resolve(); + }); + + const { userEvent } = render(, { wrapper }); + await userEvent.click(await screen.findByText('Acme SSO')); + + // THEN the challenge is shown, instead of the card sitting there looking inert. + await waitFor(() => { + expect(fixtures.router.navigate).toHaveBeenCalledWith('../protect-check'); + }); + expect(fixtures.signIn.authenticateWithRedirect).toHaveBeenCalledWith( + expect.objectContaining({ strategy: 'enterprise_sso', enterpriseConnectionId: 'ent_acme' }), + ); + }); + + it('does not route to the challenge when the hand-off is issued normally', async () => { + // GIVEN the same card, but nothing gates the hand-off + const { wrapper, fixtures } = await createFixtures(f => { + f.withEmailAddress(); + f.startSignInWithEmailAddress(); + }); + (fixtures.signIn as unknown as SignInResource).supportedFirstFactors = TWO_CONNECTIONS as never; + fixtures.signIn.authenticateWithRedirect.mockResolvedValueOnce(undefined as never); + + const { userEvent } = render(, { wrapper }); + await userEvent.click(await screen.findByText('Globex SSO')); + + // THEN the redirect owns the navigation and we must not steal it. + await waitFor(() => { + expect(fixtures.signIn.authenticateWithRedirect).toHaveBeenCalled(); + }); + expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../protect-check'); + }); +}); diff --git a/packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx b/packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx index d067fbb7396..6367f77b1f2 100644 --- a/packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx +++ b/packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx @@ -23,6 +23,75 @@ beforeEach(() => { }); describe('SignInProtectCheck', () => { + describe('enterprise SSO', () => { + const enterpriseSSOSignIn = (supportedFirstFactors: unknown[]) => + ({ + status: 'needs_first_factor', + protectCheck: null, + createdSessionId: null, + supportedFirstFactors, + }) as unknown as SignInResource; + + it('hands off to the connection once the challenge resolves', async () => { + const { wrapper, fixtures } = await createFixtures(f => { + f.startSignInWithProtectCheck(); + }); + mockExecute.mockResolvedValue('proof-abc'); + fixtures.signIn.submitProtectCheck.mockResolvedValue(enterpriseSSOSignIn([{ strategy: 'enterprise_sso' }])); + + render(, { wrapper }); + + await waitFor(() => { + expect(fixtures.signIn.authenticateWithRedirect).toHaveBeenCalledWith({ + strategy: 'enterprise_sso', + redirectUrl: 'http://localhost:3000/#/sso-callback', + redirectUrlComplete: '/', + oidcPrompt: undefined, + continueSignIn: true, + }); + }); + expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../factor-one'); + }); + + it('stays on the challenge when preparing the hand-off raises another one', async () => { + const { wrapper, fixtures } = await createFixtures(f => { + f.startSignInWithProtectCheck(); + }); + mockExecute.mockResolvedValue('proof-abc'); + fixtures.signIn.submitProtectCheck.mockResolvedValue(enterpriseSSOSignIn([{ strategy: 'enterprise_sso' }])); + fixtures.signIn.authenticateWithRedirect.mockImplementationOnce(() => { + (fixtures.signIn as any).protectCheck = { status: 'pending', token: 'challenge-token-2' }; + return Promise.resolve(); + }); + + render(, { wrapper }); + + await waitFor(() => { + expect(fixtures.router.navigate).toHaveBeenCalledWith('.'); + }); + }); + + it('routes to factor one when there is more than one connection to choose from', async () => { + const { wrapper, fixtures } = await createFixtures(f => { + f.startSignInWithProtectCheck(); + }); + mockExecute.mockResolvedValue('proof-abc'); + fixtures.signIn.submitProtectCheck.mockResolvedValue( + enterpriseSSOSignIn([ + { strategy: 'enterprise_sso', enterpriseConnectionId: 'ent_1', enterpriseConnectionName: 'Okta' }, + { strategy: 'enterprise_sso', enterpriseConnectionId: 'ent_2', enterpriseConnectionName: 'Entra' }, + ]), + ); + + render(, { wrapper }); + + await waitFor(() => { + expect(fixtures.router.navigate).toHaveBeenCalledWith('../factor-one'); + }); + expect(fixtures.signIn.authenticateWithRedirect).not.toHaveBeenCalled(); + }); + }); + it('renders verification UI', async () => { const { wrapper } = await createFixtures(f => { f.startSignInWithProtectCheck(); diff --git a/packages/ui/src/components/SignIn/enterpriseSSOFactors.ts b/packages/ui/src/components/SignIn/enterpriseSSOFactors.ts new file mode 100644 index 00000000000..b4df84e8d5f --- /dev/null +++ b/packages/ui/src/components/SignIn/enterpriseSSOFactors.ts @@ -0,0 +1,54 @@ +import type { EnterpriseSSOFactor, SignInFirstFactor, SignInResource } from '@clerk/shared/types'; + +/** + * Whether every supported first factor hands off to an enterprise connection, i.e. there is no + * factor the sign-in card could render instead. + */ +function hasOnlyEnterpriseSSOFirstFactors(signIn: SignInResource): boolean { + if (!signIn.supportedFirstFactors?.length) { + return false; + } + + return signIn.supportedFirstFactors.every(ff => ff.strategy === 'enterprise_sso'); +} + +/** + * Type guard that checks if all factors in the array are enterprise SSO factors + * with both `enterpriseConnectionId` and `enterpriseConnectionName` properties. + * This is used to determine if the user should be presented with a choice + * between multiple enterprise connections. + * @experimental + */ +function hasMultipleEnterpriseConnections( + factors: SignInFirstFactor[] | null, +): factors is Array { + if (!factors?.length) { + return false; + } + + return ( + factors.filter( + factor => + factor.strategy === 'enterprise_sso' && + 'enterpriseConnectionId' in factor && + 'enterpriseConnectionName' in factor, + ).length > 1 + ); +} + +/** + * Whether the sign-in should be handed straight to an enterprise connection rather than rendered + * as a first factor: SSO is the only way in, and there is a single connection to hand off to. + * + * Every place that continues a sign-in has to ask this — an SSO-only sign-in has no first factor + * to render, so routing it to the factor-one card leaves the user on alternative methods with no + * way to reach their identity provider. More than one connection is the exception: that is a + * choice, and the factor-one card presents it. + */ +function shouldHandOffToEnterpriseConnection(signIn: SignInResource): boolean { + return ( + hasOnlyEnterpriseSSOFirstFactors(signIn) && !hasMultipleEnterpriseConnections(signIn.supportedFirstFactors ?? null) + ); +} + +export { hasMultipleEnterpriseConnections, hasOnlyEnterpriseSSOFirstFactors, shouldHandOffToEnterpriseConnection }; diff --git a/packages/ui/src/components/SignIn/handleProtectCheck.ts b/packages/ui/src/components/SignIn/handleProtectCheck.ts index 71004dd29b5..00d10d1241e 100644 --- a/packages/ui/src/components/SignIn/handleProtectCheck.ts +++ b/packages/ui/src/components/SignIn/handleProtectCheck.ts @@ -1,5 +1,7 @@ import type { SignInResource } from '@clerk/shared/types'; +import { shouldHandOffToEnterpriseConnection } from './enterpriseSSOFactors'; + /** * Detects whether a sign-in response is gated by Clerk Protect. * @@ -48,10 +50,12 @@ export function resumeSignInAfterProtectCheck( signIn: SignInResource, { navigate, + resumeEnterpriseSSO, resumeOAuthContinuation, startedAsOAuthTransfer, }: { navigate: (to: string) => Promise; + resumeEnterpriseSSO: () => Promise; resumeOAuthContinuation: () => Promise; startedAsOAuthTransfer: boolean; }, @@ -63,6 +67,11 @@ export function resumeSignInAfterProtectCheck( switch (signIn.status) { case 'needs_first_factor': + // An SSO-only sign-in has no first factor to render — the hand-off to the identity + // provider is the next step, and it was interrupted before it could be issued. + if (shouldHandOffToEnterpriseConnection(signIn)) { + return resumeEnterpriseSSO(); + } return navigate('../factor-one'); case 'needs_second_factor': return navigate('../factor-two'); diff --git a/packages/ui/src/components/SignIn/shared.ts b/packages/ui/src/components/SignIn/shared.ts index 33cb2026be8..c418d82de6f 100644 --- a/packages/ui/src/components/SignIn/shared.ts +++ b/packages/ui/src/components/SignIn/shared.ts @@ -2,7 +2,7 @@ import { isClerkRuntimeError, isUserLockedError } from '@clerk/shared/error'; import { clerkInvalidFAPIResponse } from '@clerk/shared/internal/clerk-js/errors'; import { __internal_WebAuthnAbortService } from '@clerk/shared/internal/clerk-js/passkeys'; import { useClerk } from '@clerk/shared/react'; -import type { EnterpriseSSOFactor, SignInFirstFactor, SignInResource } from '@clerk/shared/types'; +import type { SignInResource } from '@clerk/shared/types'; import { useCallback, useEffect } from 'react'; import { useCardState } from '@/ui/elements/contexts'; @@ -84,28 +84,5 @@ function useHandleAuthenticateWithPasskey( }, []); } -/** - * Type guard that checks if all factors in the array are enterprise SSO factors - * with both `enterpriseConnectionId` and `enterpriseConnectionName` properties. - * This is used to determine if the user should be presented with a choice - * between multiple enterprise connections. - * @experimental - */ -function hasMultipleEnterpriseConnections( - factors: SignInFirstFactor[] | null, -): factors is Array { - if (!factors?.length) { - return false; - } - - return ( - factors.filter( - factor => - factor.strategy === 'enterprise_sso' && - 'enterpriseConnectionId' in factor && - 'enterpriseConnectionName' in factor, - ).length > 1 - ); -} - -export { hasMultipleEnterpriseConnections, useHandleAuthenticateWithPasskey }; +export { hasMultipleEnterpriseConnections } from './enterpriseSSOFactors'; +export { useHandleAuthenticateWithPasskey };