-
Notifications
You must be signed in to change notification settings - Fork 337
fix(clerk-js): Only refresh when auth with popup is called #5518
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/clerk-js': patch | ||
--- | ||
|
||
Only refresh signIn and signUp resources during an SSO callback if the authentication was performed via a popup. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,7 +112,13 @@ import { memoizeListenerCallback } from '../utils/memoizeStateListenerCallback'; | |
import { RedirectUrls } from '../utils/redirectUrls'; | ||
import { AuthCookieService } from './auth/AuthCookieService'; | ||
import { CaptchaHeartbeat } from './auth/CaptchaHeartbeat'; | ||
import { CLERK_SATELLITE_URL, CLERK_SUFFIXED_COOKIES, CLERK_SYNCED, ERROR_CODES } from './constants'; | ||
import { | ||
CLERK_SATELLITE_URL, | ||
CLERK_SUFFIXED_COOKIES, | ||
CLERK_SYNCED, | ||
ERROR_CODES, | ||
SESSION_STORAGE_AUTH_WITH_POPUP_KEY, | ||
} from './constants'; | ||
import { | ||
clerkErrorInitFailed, | ||
clerkInvalidSignInUrlFormat, | ||
|
@@ -1507,11 +1513,23 @@ export class Clerk implements ClerkInterface { | |
return; | ||
} | ||
|
||
let shouldRefreshResources = false; | ||
try { | ||
const hasCalledAuthWithPopup = sessionStorage.getItem(SESSION_STORAGE_AUTH_WITH_POPUP_KEY); | ||
if (hasCalledAuthWithPopup) { | ||
shouldRefreshResources = true; | ||
} | ||
} catch { | ||
// In the event that sessionStorage is disabled, assume the resource needs to be refreshed. Refreshing when not | ||
// needed doesn't break anything, but it does cause 405 network errors. | ||
shouldRefreshResources = true; | ||
} | ||
|
||
// If `handleRedirectCallback` is called on a window without an opener property (such as when the OAuth flow popup | ||
// directs the opening page to navigate to the /sso-callback route), we need to reload the signIn and signUp resources | ||
// to ensure that we have the latest state. This operation can fail when we try reloading a resource that doesn't | ||
// exist (such as when reloading a signIn resource during a signUp attempt), but this can be safely ignored. | ||
if (!window.opener) { | ||
if (!window.opener && shouldRefreshResources) { | ||
try { | ||
await signIn.reload(); | ||
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. Can you remind me why doing 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. It doesn't always return a 405. It only returns a 405 when the underlying resource doesn't exist. So for example if we created a SignIn resource during the popup, 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. oh i see, thanks for clarifying. |
||
} catch { | ||
|
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.
Could this entire thing be done via a
postMessage()
from the popup? The issue is we don't know which resource we need to fetch so we're just doing both, right?