fix(clerk-js,types): Fix the redirection that happens when SSO fails by anagstef · Pull Request #2955 · clerk/javascript

anagstef · 2024-03-07T22:09:37Z

Description

Current Behaviour:

If a SignIn or SignUp SSO fails and needs to redirect back to the component, it always redirects to the Account Portal.

Screen.Recording.2024-03-08.at.14.09.17.mov

Expected Behaviour:

If the component is mounted on the user's app, then it should redirect back to the user's app.

Screen.Recording.2024-03-08.at.14.10.18.mov

Tech Details:

This happens because in the handleRedirectCallback we extract the signInUrl and signUpUrl from the displayConfig, without taking into account the env vars or the props that the user has passed.

This can be easily reproduced by adding a user's email to the blocklist on dashboard.

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

changeset-bot · 2024-03-07T22:09:41Z

🦋 Changeset detected

Latest commit: 017c68b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/types	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch
@clerk/backend	Patch
@clerk/fastify	Patch
gatsby-plugin-clerk	Patch
@clerk/nextjs	Patch
@clerk/clerk-react	Patch
@clerk/remix	Patch
@clerk/clerk-sdk-node	Patch
@clerk/themes	Patch

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

anagstef · 2024-03-07T22:19:47Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


Removed the ctx from passing it inside the pickRedirectionProp here, as it never contains a signUpUrl prop from the SignUp component. The priority now is:
The ctx.path is the path prop that user passes to the SignUp component, when using routing=path.
The options passed in pickRedirectionProp is the ClerkProvider signUpUrl prop or the NEXT_PUBLIC_CLERK_SIGN_UP_URL env var, with that order.
Lastly, the displayConfig that has the Account Portal value.
cc @nikosdouvlis @dimkl

I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

@nikosdouvlis Exactly! The first argument of the pickRedirectionProp utility is the key to search for. If I wanted to use it I should to something like:
pickRedirectionProp('path', { ctx }, false) || pickRedirectionProp('signUpUrl', { options, displayConfig }, false)
which looks even more confusing.

nikosdouvlis · 2024-03-11T11:32:35Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

…nUpUrl and signInUrl

…ap-when-failing-even-if-the

anagstef · 2024-03-07T22:09:37Z

Description

Current Behaviour:

If a SignIn or SignUp SSO fails and needs to redirect back to the component, it always redirects to the Account Portal.

Screen.Recording.2024-03-08.at.14.09.17.mov

Expected Behaviour:

If the component is mounted on the user's app, then it should redirect back to the user's app.

Screen.Recording.2024-03-08.at.14.10.18.mov

Tech Details:

This happens because in the handleRedirectCallback we extract the signInUrl and signUpUrl from the displayConfig, without taking into account the env vars or the props that the user has passed.

This can be easily reproduced by adding a user's email to the blocklist on dashboard.

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

changeset-bot · 2024-03-07T22:09:41Z

🦋 Changeset detected

Latest commit: 017c68b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/types	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch
@clerk/backend	Patch
@clerk/fastify	Patch
gatsby-plugin-clerk	Patch
@clerk/nextjs	Patch
@clerk/clerk-react	Patch
@clerk/remix	Patch
@clerk/clerk-sdk-node	Patch
@clerk/themes	Patch

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

anagstef · 2024-03-07T22:19:47Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


Removed the ctx from passing it inside the pickRedirectionProp here, as it never contains a signUpUrl prop from the SignUp component. The priority now is:
The ctx.path is the path prop that user passes to the SignUp component, when using routing=path.
The options passed in pickRedirectionProp is the ClerkProvider signUpUrl prop or the NEXT_PUBLIC_CLERK_SIGN_UP_URL env var, with that order.
Lastly, the displayConfig that has the Account Portal value.
cc @nikosdouvlis @dimkl

I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

@nikosdouvlis Exactly! The first argument of the pickRedirectionProp utility is the key to search for. If I wanted to use it I should to something like:
pickRedirectionProp('path', { ctx }, false) || pickRedirectionProp('signUpUrl', { options, displayConfig }, false)
which looks even more confusing.

nikosdouvlis · 2024-03-11T11:32:35Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

…nUpUrl and signInUrl

…ap-when-failing-even-if-the

anagstef · 2024-03-07T22:09:37Z

Description

Current Behaviour:

If a SignIn or SignUp SSO fails and needs to redirect back to the component, it always redirects to the Account Portal.

Screen.Recording.2024-03-08.at.14.09.17.mov

Expected Behaviour:

If the component is mounted on the user's app, then it should redirect back to the user's app.

Screen.Recording.2024-03-08.at.14.10.18.mov

Tech Details:

This happens because in the handleRedirectCallback we extract the signInUrl and signUpUrl from the displayConfig, without taking into account the env vars or the props that the user has passed.

This can be easily reproduced by adding a user's email to the blocklist on dashboard.

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

changeset-bot · 2024-03-07T22:09:41Z

🦋 Changeset detected

Latest commit: 017c68b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/types	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch
@clerk/backend	Patch
@clerk/fastify	Patch
gatsby-plugin-clerk	Patch
@clerk/nextjs	Patch
@clerk/clerk-react	Patch
@clerk/remix	Patch
@clerk/clerk-sdk-node	Patch
@clerk/themes	Patch

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

anagstef · 2024-03-07T22:19:47Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


Removed the ctx from passing it inside the pickRedirectionProp here, as it never contains a signUpUrl prop from the SignUp component. The priority now is:
The ctx.path is the path prop that user passes to the SignUp component, when using routing=path.
The options passed in pickRedirectionProp is the ClerkProvider signUpUrl prop or the NEXT_PUBLIC_CLERK_SIGN_UP_URL env var, with that order.
Lastly, the displayConfig that has the Account Portal value.
cc @nikosdouvlis @dimkl

I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

@nikosdouvlis Exactly! The first argument of the pickRedirectionProp utility is the key to search for. If I wanted to use it I should to something like:
pickRedirectionProp('path', { ctx }, false) || pickRedirectionProp('signUpUrl', { options, displayConfig }, false)
which looks even more confusing.

nikosdouvlis · 2024-03-11T11:32:35Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

…nUpUrl and signInUrl

…ap-when-failing-even-if-the

anagstef · 2024-03-07T22:09:37Z

Description

Current Behaviour:

If a SignIn or SignUp SSO fails and needs to redirect back to the component, it always redirects to the Account Portal.

Screen.Recording.2024-03-08.at.14.09.17.mov

Expected Behaviour:

If the component is mounted on the user's app, then it should redirect back to the user's app.

Screen.Recording.2024-03-08.at.14.10.18.mov

Tech Details:

This happens because in the handleRedirectCallback we extract the signInUrl and signUpUrl from the displayConfig, without taking into account the env vars or the props that the user has passed.

This can be easily reproduced by adding a user's email to the blocklist on dashboard.

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

changeset-bot · 2024-03-07T22:09:41Z

🦋 Changeset detected

Latest commit: 017c68b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/types	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch
@clerk/backend	Patch
@clerk/fastify	Patch
gatsby-plugin-clerk	Patch
@clerk/nextjs	Patch
@clerk/clerk-react	Patch
@clerk/remix	Patch
@clerk/clerk-sdk-node	Patch
@clerk/themes	Patch

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

anagstef · 2024-03-07T22:19:47Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


Removed the ctx from passing it inside the pickRedirectionProp here, as it never contains a signUpUrl prop from the SignUp component. The priority now is:
The ctx.path is the path prop that user passes to the SignUp component, when using routing=path.
The options passed in pickRedirectionProp is the ClerkProvider signUpUrl prop or the NEXT_PUBLIC_CLERK_SIGN_UP_URL env var, with that order.
Lastly, the displayConfig that has the Account Portal value.
cc @nikosdouvlis @dimkl

I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

@nikosdouvlis Exactly! The first argument of the pickRedirectionProp utility is the key to search for. If I wanted to use it I should to something like:
pickRedirectionProp('path', { ctx }, false) || pickRedirectionProp('signUpUrl', { options, displayConfig }, false)
which looks even more confusing.

nikosdouvlis · 2024-03-11T11:32:35Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

…nUpUrl and signInUrl

…ap-when-failing-even-if-the

anagstef · 2024-03-07T22:09:37Z

Description

Current Behaviour:

If a SignIn or SignUp SSO fails and needs to redirect back to the component, it always redirects to the Account Portal.

Screen.Recording.2024-03-08.at.14.09.17.mov

Expected Behaviour:

If the component is mounted on the user's app, then it should redirect back to the user's app.

Screen.Recording.2024-03-08.at.14.10.18.mov

Tech Details:

This happens because in the handleRedirectCallback we extract the signInUrl and signUpUrl from the displayConfig, without taking into account the env vars or the props that the user has passed.

This can be easily reproduced by adding a user's email to the blocklist on dashboard.

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

changeset-bot · 2024-03-07T22:09:41Z

🦋 Changeset detected

Latest commit: 017c68b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/types	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch
@clerk/backend	Patch
@clerk/fastify	Patch
gatsby-plugin-clerk	Patch
@clerk/nextjs	Patch
@clerk/clerk-react	Patch
@clerk/remix	Patch
@clerk/clerk-sdk-node	Patch
@clerk/themes	Patch

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

anagstef · 2024-03-07T22:19:47Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


Removed the ctx from passing it inside the pickRedirectionProp here, as it never contains a signUpUrl prop from the SignUp component. The priority now is:
The ctx.path is the path prop that user passes to the SignUp component, when using routing=path.
The options passed in pickRedirectionProp is the ClerkProvider signUpUrl prop or the NEXT_PUBLIC_CLERK_SIGN_UP_URL env var, with that order.
Lastly, the displayConfig that has the Account Portal value.
cc @nikosdouvlis @dimkl

I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

@nikosdouvlis Exactly! The first argument of the pickRedirectionProp utility is the key to search for. If I wanted to use it I should to something like:
pickRedirectionProp('path', { ctx }, false) || pickRedirectionProp('signUpUrl', { options, displayConfig }, false)
which looks even more confusing.

nikosdouvlis · 2024-03-11T11:32:35Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

…nUpUrl and signInUrl

…ap-when-failing-even-if-the

anagstef · 2024-03-07T22:09:37Z

Description

Current Behaviour:

If a SignIn or SignUp SSO fails and needs to redirect back to the component, it always redirects to the Account Portal.

Screen.Recording.2024-03-08.at.14.09.17.mov

Expected Behaviour:

If the component is mounted on the user's app, then it should redirect back to the user's app.

Screen.Recording.2024-03-08.at.14.10.18.mov

Tech Details:

This happens because in the handleRedirectCallback we extract the signInUrl and signUpUrl from the displayConfig, without taking into account the env vars or the props that the user has passed.

This can be easily reproduced by adding a user's email to the blocklist on dashboard.

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

changeset-bot · 2024-03-07T22:09:41Z

🦋 Changeset detected

Latest commit: 017c68b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/types	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch
@clerk/backend	Patch
@clerk/fastify	Patch
gatsby-plugin-clerk	Patch
@clerk/nextjs	Patch
@clerk/clerk-react	Patch
@clerk/remix	Patch
@clerk/clerk-sdk-node	Patch
@clerk/themes	Patch

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

anagstef · 2024-03-07T22:19:47Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


Removed the ctx from passing it inside the pickRedirectionProp here, as it never contains a signUpUrl prop from the SignUp component. The priority now is:
The ctx.path is the path prop that user passes to the SignUp component, when using routing=path.
The options passed in pickRedirectionProp is the ClerkProvider signUpUrl prop or the NEXT_PUBLIC_CLERK_SIGN_UP_URL env var, with that order.
Lastly, the displayConfig that has the Account Portal value.
cc @nikosdouvlis @dimkl

I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

@nikosdouvlis Exactly! The first argument of the pickRedirectionProp utility is the key to search for. If I wanted to use it I should to something like:
pickRedirectionProp('path', { ctx }, false) || pickRedirectionProp('signUpUrl', { options, displayConfig }, false)
which looks even more confusing.

nikosdouvlis · 2024-03-11T11:32:35Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

…nUpUrl and signInUrl

…ap-when-failing-even-if-the

anagstef · 2024-03-07T22:09:37Z

Description

Current Behaviour:

If a SignIn or SignUp SSO fails and needs to redirect back to the component, it always redirects to the Account Portal.

Screen.Recording.2024-03-08.at.14.09.17.mov

Expected Behaviour:

If the component is mounted on the user's app, then it should redirect back to the user's app.

Screen.Recording.2024-03-08.at.14.10.18.mov

Tech Details:

This happens because in the handleRedirectCallback we extract the signInUrl and signUpUrl from the displayConfig, without taking into account the env vars or the props that the user has passed.

This can be easily reproduced by adding a user's email to the blocklist on dashboard.

Checklist

npm test runs as expected.
npm run build runs as expected.
(If applicable) JSDoc comments have been added or updated for any package exports
(If applicable) Documentation has been updated

Type of change

changeset-bot · 2024-03-07T22:09:41Z

🦋 Changeset detected

Latest commit: 017c68b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/types	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch
@clerk/backend	Patch
@clerk/fastify	Patch
gatsby-plugin-clerk	Patch
@clerk/nextjs	Patch
@clerk/clerk-react	Patch
@clerk/remix	Patch
@clerk/clerk-sdk-node	Patch
@clerk/themes	Patch

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

anagstef · 2024-03-07T22:19:47Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


Removed the ctx from passing it inside the pickRedirectionProp here, as it never contains a signUpUrl prop from the SignUp component. The priority now is:
The ctx.path is the path prop that user passes to the SignUp component, when using routing=path.
The options passed in pickRedirectionProp is the ClerkProvider signUpUrl prop or the NEXT_PUBLIC_CLERK_SIGN_UP_URL env var, with that order.
Lastly, the displayConfig that has the Account Portal value.
cc @nikosdouvlis @dimkl

I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

@nikosdouvlis Exactly! The first argument of the pickRedirectionProp utility is the key to search for. If I wanted to use it I should to something like:
pickRedirectionProp('path', { ctx }, false) || pickRedirectionProp('signUpUrl', { options, displayConfig }, false)
which looks even more confusing.

nikosdouvlis · 2024-03-11T11:32:35Z

+ let signUpUrl =
+ (ctx.routing === 'path' ? ctx.path : undefined) ||
+ pickRedirectionProp('signUpUrl', { options, displayConfig }, false);


I'm a bit skeptical because the logic here has become somewhat complicated.
I think the reason you're not moving the path check inside pickRedirectionProp is that pickRedirectionProp is too generic for that?

…nUpUrl and signInUrl

…ap-when-failing-even-if-the

fix(clerk-js,types): Fix the redirection that happens when SSO fails
fe7b3cb

anagstef self-assigned this Mar 7, 2024

clerk-cookie added types clerk-js elements labels Mar 7, 2024

anagstef commented Mar 7, 2024
View reviewed changes

Comment threadpackages/clerk-js/src/ui/contexts/ClerkUIComponentsContext.tsx

anagstef commented Mar 7, 2024
View reviewed changes

Comment threadpackages/elements/src/internals/machines/third-party/actors.ts

anagstef requested review from dimkl, nikosdouvlis, octoper and tmilewski March 8, 2024 12:14

anagstef marked this pull request as ready for review March 8, 2024 12:15

nikosdouvlis approved these changes Mar 11, 2024
View reviewed changes

Stefanos Anagnostouand others added 2 commits March 29, 2024 16:46

chore(clerk-js): Add comments as to why we access path prop for sig…
d20717e
…nUpUrl and signInUrl

Merge branch 'main' into stefanos/sdk-1475-sso-callback-redirects-to-…
211bf8a
…ap-when-failing-even-if-the

dimkl approved these changes Mar 29, 2024
View reviewed changes

Merge branch 'main' into stefanos/sdk-1475-sso-callback-redirects-to-…
017c68b
…ap-when-failing-even-if-the

anagstef enabled auto-merge (squash) March 29, 2024 15:35

anagstef merged commit f540e98 into mainMar 29, 2024

anagstef deleted the stefanos/sdk-1475-sso-callback-redirects-to-ap-when-failing-even-if-the branch March 29, 2024 15:41

clerk-cookie mentioned this pull request Mar 29, 2024
Version Packages (beta) #3064
Merged