fix(clerk-js,types): Fix stale SignIn on authenticateWithRedirect for enterprise_sso
#6160
Conversation
🦋 Changeset detectedLatest commit: b41e0fa The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
LauraBeatris
changed the title
SignIn object on authenticateWithRedirect for enterprise_sso custom flowsSignIn object on authenticateWithRedirect for enterprise_sso custom flows
LauraBeatris
changed the title
SignIn object on authenticateWithRedirect for enterprise_sso custom flowsSignIn on authenticateWithRedirect for enterprise_sso custom flows
LauraBeatris
changed the title
SignIn on authenticateWithRedirect for enterprise_sso custom flowsSignIn on authenticateWithRedirect for enterprise_sso
| const { status, externalVerificationRedirectURL } = firstFactorVerification; | ||
| const { strategy, redirectUrl, redirectUrlComplete, identifier, oidcPrompt, continueSignIn } = params || {}; | ||
|
|
||
| if (!this.id || !continueSignIn) { |
There was a problem hiding this comment.
I found a PR comment a while ago (going to track it again to paste here) that the reasoning for not creating within here was to avoid issues if we created sign-in from AIOs.
My changes tackle exactly for this case, where SignIn provides continueSignIn as true
There was a problem hiding this comment.
Is the issue only saml and enterprise_sso ?
I think adding a new property just makes things more complicated for the caller. My understanding is that now developers would need to pass both identifier and continueSignIn.
Can we get around this by checking if identifiers match ?
There was a problem hiding this comment.
enterprise_sso and saml since those rely on the identifier to query for the SSO connection.
They wouldn't have to pass continueSignIn to authenticateWithRedirect since it defaults to false, but it's being passed from SignInStart since it always creates a fresh sign-in:
javascript/packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx
Lines 299 to 312 in e182055
There was a problem hiding this comment.
Can we get around this by checking if identifiers match ?
I guess this will work 🤔 but we still have the problem where SignInStart calls authenticateWithRedirect, so we cannot place this logic there otherwise it'd create two sign-ins.
There was a problem hiding this comment.
I see, is that because on needs_identifier we want to call prepare instead of create ?
There was a problem hiding this comment.
I see, is that because on needs_identifier we want to call prepare instead of create ?
Yeah, exactly, so SignInStart needs to handle the progressive flow. Custom flows also need to do so, so it's fair to also always create a fresh sign-in.
📝 Walkthrough## Walkthrough
This change addresses an issue in the authentication flow for SAML and Enterprise SSO strategies within the `@clerk/clerk-js` package. The main update ensures that the connection identifier is refreshed or updated with each call to `authenticateWithRedirect`, preventing redirection to the wrong identity provider. The implementation involves refactoring the `authenticateWithRedirectOrPopup` method to adjust the sequence of creating or preparing the sign-in factors, adding a new optional `continueSignIn` property to the `AuthenticateWithRedirectParams` type, and updating both the `SignInStart` component and its tests to include this new property in relevant authentication flows. No exported or public API signatures were otherwise altered.
## Suggested reviewers
- aeliox🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/grumpy-lamps-study.md (1)
8-8: Fix minor grammar issue in the changeset description.The sentence could benefit from improved punctuation for clarity.
Apply this diff to improve readability:
-Previously, the same connection identifier would be used on every `authenticateWithRedirect` call leading to redirecting to the wrong identity provider +Previously, the same connection identifier would be used on every `authenticateWithRedirect` call, leading to redirecting to the wrong identity provider
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/grumpy-lamps-study.md(1 hunks)packages/clerk-js/src/core/resources/SignIn.ts(1 hunks)packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx(1 hunks)packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx(2 hunks)packages/types/src/redirects.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/grumpy-lamps-study.md
[uncategorized] ~8-~8: Possible missing comma found.
Context: ...sed on every authenticateWithRedirect call leading to redirecting to the wrong ide...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (6)
packages/types/src/redirects.ts (1)
64-67: LGTM! Well-documented type addition.The new
continueSignInproperty follows the same pattern as the existingcontinueSignUpproperty and is properly documented. The optional nature ensures backward compatibility.packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx (1)
408-408: Correct implementation of the continueSignIn flag.Setting
continueSignIn: truein the SignInStart component ensures that the existing SignIn object is reused, preventing the stale connection identifier issue described in the PR objectives.packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx (2)
294-294: Test correctly updated for SAML flow.The test now properly expects
continueSignIn: truein the authenticateWithRedirect call, validating the updated behavior in the SignInStart component.
318-318: Test correctly updated for Enterprise SSO flow.The test now properly expects
continueSignIn: truein the authenticateWithRedirect call, ensuring the component behavior is properly validated.packages/clerk-js/src/core/resources/SignIn.ts (2)
235-254: Excellent refactoring that addresses the core issue.The conditional logic properly handles the
continueSignInflag:
- When
continueSignInis true and a SignIn already exists (this.id), it skips creating a new SignIn object- For SAML/Enterprise SSO strategies, it always calls
prepareFirstFactorto ensure proper setup- This prevents the stale connection identifier issue by reusing existing SignIn objects when appropriate
The separation of concerns between
createandprepareFirstFactormakes the flow clearer and more maintainable.
255-255: Correct access pattern for verification state.Accessing
this.firstFactorVerificationdirectly is appropriate here since we're using the internal state that was updated by the previousprepareFirstFactorcall, rather than relying on destructured response values.
LauraBeatris
changed the title
SignIn on authenticateWithRedirect for enterprise_sso SignIn on authenticateWithRedirect for enterprise_sso
LauraBeatris
force-pushed
the
laura/orgs-540-authenticatewithredirect-does-not-create-new-signin-on-a
branch
from
0607b66 to
e18281a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/grumpy-lamps-study.md (1)
6-8: Polish wording and include the newcontinueSignInflagUsing the imperative voice is preferred in changesets (
Fixvs.Fixes).
A comma is needed after call, and it’s helpful to explicitly mention the newly-addedcontinueSignInoption so downstream consumers immediately see the API addition.-Fixes stale `SignIn` object on `authenticateWithRedirect` for `saml` and `enterprise_sso` custom flows - -Previously, the same connection identifier would be used on every `authenticateWithRedirect` call leading to redirecting to the wrong identity provider +Fix stale `SignIn` object when calling `authenticateWithRedirect` in `saml` and `enterprise_sso` custom flows. + +Previously, the same connection identifier was reused on every `authenticateWithRedirect` call, leading to redirection to the wrong identity provider. + +This patch also introduces an optional `continueSignIn` flag (default `false`) that lets you explicitly reuse the current `SignIn` instance when desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/grumpy-lamps-study.md(1 hunks)packages/clerk-js/src/core/resources/SignIn.ts(1 hunks)packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx(1 hunks)packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx(2 hunks)packages/types/src/redirects.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/types/src/redirects.ts
- packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx
- packages/clerk-js/src/ui/components/SignIn/tests/SignInStart.test.tsx
- packages/clerk-js/src/core/resources/SignIn.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/grumpy-lamps-study.md
[uncategorized] ~8-~8: Possible missing comma found.
Context: ...sed on every authenticateWithRedirect call leading to redirecting to the wrong ide...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
LauraBeatris
force-pushed
the
laura/orgs-540-authenticatewithredirect-does-not-create-new-signin-on-a
branch
from
e18281a to
b41e0fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.changeset/grumpy-lamps-study.md (2)
6-9: Add comma for readability and fix run-on sentenceThe sentence is currently hard to parse. Insert a comma after “call” (or break it into two sentences) to eliminate the run-on and match the LanguageTool hint.
-Previously, the same connection identifier would be used on every `authenticateWithRedirect` call leading to redirecting to the wrong identity provider +Previously, the same connection identifier would be used on every `authenticateWithRedirect` call, leading to redirection to the wrong identity provider.
6-6: Mention the newcontinueSignInflag in the summaryThe changeset headline explains what was fixed but omits the newly-added
continueSignInoption that developers must be aware of. Explicitly calling this out will make the release notes clearer.-Fixes stale `SignIn` object on `authenticateWithRedirect` for `saml` and `enterprise_sso` custom flows +Adds `continueSignIn` flag and fixes stale `SignIn` object on `authenticateWithRedirect` for `saml` and `enterprise_sso` custom flows
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/grumpy-lamps-study.md(1 hunks)packages/clerk-js/src/core/resources/SignIn.ts(1 hunks)packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx(1 hunks)packages/clerk-js/src/ui/components/SignIn/__tests__/SignInStart.test.tsx(2 hunks)packages/types/src/redirects.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/types/src/redirects.ts
- packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx
- packages/clerk-js/src/ui/components/SignIn/tests/SignInStart.test.tsx
- packages/clerk-js/src/core/resources/SignIn.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/grumpy-lamps-study.md
[uncategorized] ~8-~8: Possible missing comma found.
Context: ...sed on every authenticateWithRedirect call leading to redirecting to the wrong ide...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
LauraBeatris
deleted the
laura/orgs-540-authenticatewithredirect-does-not-create-new-signin-on-a
branch
Description
This PR introduces a fix specifically for custom flows with
enterprise_ssoCurrently,
authenticateWithRedirectalways uses the existing clientSignInforsamlorenterprise_ssostrategy, even if the identifier gets changed by the user in a custom flow, which leads to an issue where FAPI queries the wrong enterprise connection:CleanShot.2025-02-10.at.16.20.39.mp4
Our
SignInStartcomponent always creates a newSignIn; therefore, the flow above works smoothly:javascript/packages/clerk-js/src/ui/components/SignIn/SignInStart.tsx
Lines 299 to 312 in e182055
With fix
This PR introduces a new
continueSignInoption forsignIn.authenticateWithRedirect, which defaults tofalse, therefore it'll create a newSignInobject on every call.Within
SignInStart, we passcontinueSignInasfalsein order to reuse the currentSignIncreated, and call/prepare-first-factorafter.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit