-
Notifications
You must be signed in to change notification settings - Fork 474
fix(ui): continue to the identity provider after a challenge #9620
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
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,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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(<SignInFactorOneEnterpriseConnections />, { 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(<SignInFactorOneEnterpriseConnections />, { 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'); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
| }); | ||
|
Comment on lines
+36
to
+38
Contributor
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -t f -e ts -e tsx . packages/ui/src/components/SignIn | \
xargs -r rg -n -C 3 'startSignInWithProtectCheck|invitation.*ticket|ticket.*invitation|organizationInvitation'Repository: clerk/javascript Length of output: 22176 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- protect-check tests (resumption section) ---'
sed -n '520,680p' packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx
printf '%s\n' '--- fixture definition and invitation-ticket fields ---'
rg -n -C 4 'function createFixtures|const createFixtures|createFixtures\s*=|invitationTicket|invitation_ticket|organizationInvitation|organization_invitation|ticket' \
packages/ui/src/components/SignIn packages/ui/src -g '*.{ts,tsx}' | head -n 300Repository: clerk/javascript Length of output: 36358 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- protect-check implementation and ticket propagation ---'
rg -n -C 5 '__internal_resumeAfterProtectCheck|organizationTicket|__clerk_ticket|enterpriseConnectionId|redirect' \
packages/ui/src/components/SignIn packages/ui/src/test/create-fixtures.tsx -g '*.{ts,tsx}' | head -n 350
printf '%s\n' '--- all SignIn protect-check tests with ticket or invitation setup ---'
rg -n -C 3 '__clerk_ticket|organizationTicket|invitation|ticket|resumeAfterProtectCheck' \
packages/ui/src/components/SignIn/__tests__ -g '*.{ts,tsx}' | head -n 350Repository: clerk/javascript Length of output: 46825 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 6 'resumeSignInAfterProtectCheck|buildSignInOAuthCallbackParams|__clerk_ticket|organizationTicket' \
packages/clerk-js packages/ui/src/components/SignIn packages/ui/src/contexts packages/ui/src/test \
-g '*.{ts,tsx}' | head -n 400Repository: clerk/javascript Length of output: 37695 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- protect-check routing contract ---'
sed -n '1,150p' packages/ui/src/components/SignIn/handleProtectCheck.ts
sed -n '80,125p' packages/ui/src/components/SignIn/SignInProtectCheck.tsx
sed -n '1,115p' packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx
printf '%s\n' '--- bound enterprise redirect implementation ---'
rg -n -C 8 'authenticateWithRedirect\s*\(' packages/clerk-js/src/core/resources/SignIn.ts packages/clerk-js/src -g '*.ts' | head -n 250Repository: clerk/javascript Length of output: 18628 Add organization invitation-ticket coverage for protect-check resumption. The 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| mockExecute.mockResolvedValue('proof-abc'); | ||
| fixtures.signIn.submitProtectCheck.mockResolvedValue(enterpriseSSOSignIn([{ strategy: 'enterprise_sso' }])); | ||
|
|
||
| render(<SignInProtectCheck />, { 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(<SignInProtectCheck />, { wrapper }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(fixtures.router.navigate).toHaveBeenCalledWith('.'); | ||
|
Contributor
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Assert that factor-one navigation does not occur. Line 70 only proves that Proposed test assertion await waitFor(() => {
expect(fixtures.router.navigate).toHaveBeenCalledWith('.');
});
+ expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../factor-one');🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| }); | ||
| }); | ||
|
|
||
| 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(<SignInProtectCheck />, { 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(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<EnterpriseSSOFactor & { enterpriseConnectionId: string; enterpriseConnectionName: string }> { | ||
| 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) | ||
| ); | ||
| } | ||
|
Comment on lines
+48
to
+52
Member
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. What's the intended behavior here? If the user has multiple factors, picks one enterprise SSO one, gets challenged and end up here to determine whether to continue, shouldn't we continue that specific factor which they had already chosen?
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. Good catch, and the honest answer is "we don't, and for the multi-connection case we probably should" — with a caveat about which scenario actually reaches this line. Taking the literal case first: with a mix of factors (say password + one enterprise connection), But the underlying point stands. This predicate is lifted verbatim out of The scenario that does reach it is more than one connection: Chasing that turned up something worse in the same path, which I've fixed here in 27b73b1: Resuming that specific connection I've deliberately left out. Nothing carries the choice across the challenge: |
||
|
|
||
| export { hasMultipleEnterpriseConnections, hasOnlyEnterpriseSSOFirstFactors, shouldHandOffToEnterpriseConnection }; | ||
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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: clerk/javascript
Length of output: 1891
🏁 Script executed:
Repository: clerk/javascript
Length of output: 50372
🏁 Script executed:
Repository: clerk/javascript
Length of output: 47225
🏁 Script executed:
Repository: clerk/javascript
Length of output: 11368
Use a typed mutable
SignInResourceview and includesdkUrl.ProtectCheckResource.sdkUrlis required. Both tests currently bypass this contract withanyand assign incomplete challenge objects. Reuse the existing typedSignInResourceview and provide a completeProtectCheckResourcevalue at both sites.📍 Affects 2 files
packages/ui/src/components/SignIn/__tests__/SignInFactorOneEnterpriseConnections.test.tsx#L28-L28(this comment)packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx#L63-L63🤖 Prompt for AI Agents
Source: Coding guidelines