Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 470
fix(clerk-js): Use focus event to trigger session touch#3786
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
05efaacff2b9d6793ea1c557c75ca9f47d287d7ddbd04455566b4acbbeb531fc6fa4d000358313c96329cd38a58766db84d6ba3d103ea9bdFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@clerk/clerk-js": minor | ||
| --- | ||
| Fixes a bug where multiple tabs with different active organizations would not always respect the selected organization. Going forward, when a tab is focused the active organization will immediately be updated to the tab's last active organization. | ||
| Additionally, `Clerk.session.getToken()` now accepts an `organizationId` option. The provided organization ID will be used to set organization-related claims in the generated session token. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "files": [ | ||
| { "path": "./dist/clerk.browser.js", "maxSize": "62kB" }, | ||
| { "path": "./dist/clerk.browser.js", "maxSize": "63kB" }, | ||
nikosdouvlis marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| { "path": "./dist/clerk.headless.js", "maxSize": "43kB" }, | ||
| { "path": "./dist/ui-common*.js", "maxSize": "85KB" }, | ||
| { "path": "./dist/vendors*.js", "maxSize": "70KB" }, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -56,7 +56,7 @@ export class AuthCookieService { | ||
| this.setClientUatCookieForDevelopmentInstances(); | ||
| }); | ||
| this.refreshTokenOnVisibilityChange(); | ||
| this.refreshTokenOnFocus(); | ||
| this.startPollingForToken(); | ||
| this.clientUat = createClientUatCookie(cookieSuffix); | ||
| @@ -110,27 +110,47 @@ export class AuthCookieService { | ||
| this.poller.startPollingForSessionToken(() => this.refreshSessionToken()); | ||
| } | ||
| private refreshTokenOnVisibilityChange() { | ||
| document.addEventListener('visibilitychange', () => { | ||
| private refreshTokenOnFocus() { | ||
| window.addEventListener('focus', () => { | ||
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. We should either replace this event listener call here with a MemberAuthor 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. Agreed, good idea 👍 | ||
| if (document.visibilityState === 'visible') { | ||
| void this.refreshSessionToken(); | ||
| // Certain data-fetching libraries that refetch on focus (such as swr) use setTimeout(cb, 0) to schedule a task on the event loop. | ||
| // This gives us an opportunity to ensure the session cookie is updated with a fresh token before the fetch occurs, but it needs to | ||
| // be done with a microtask. Promises schedule microtasks, and so by using `updateCookieImmediately: true`, we ensure that the cookie | ||
| // is updated as part of the scheduled microtask. Our existing event-based mechanism to update the cookie schedules a task, and so the cookie | ||
| // is updated too late and not guaranteed to be fresh before the refetch occurs. | ||
| void this.refreshSessionToken({ updateCookieImmediately: true }); | ||
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. Very helpful context here 👏🏻
Taken from https://developer.mozilla.org/en-US/docs/Web/API/HTML_DOM_API/Microtask_guide/In_depth#run_javascript_run:~:text=Tasks%20vs.%20microtasks (which I recommend everyone to read) | ||
| } | ||
| }); | ||
| } | ||
| private async refreshSessionToken(): Promise<void> { | ||
| private async refreshSessionToken({ | ||
| updateCookieImmediately = false, | ||
| }: { | ||
| updateCookieImmediately?: boolean; | ||
| } = {}): Promise<void> { | ||
| if (!this.clerk.session) { | ||
| return; | ||
| } | ||
| try { | ||
| await this.clerk.session.getToken(); | ||
| const token = await this.clerk.session.getToken(); | ||
| if (updateCookieImmediately) { | ||
| this.updateSessionCookie(token); | ||
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. Again, for any future readers: The reason the token update gets scheduled as a microtask here because it's part of the promise that was initialized when the async @brkalow please correct me if I'm wrong here | ||
| } | ||
| } catch (e) { | ||
| return this.handleGetTokenError(e); | ||
| } | ||
| } | ||
| private updateSessionCookie(token: string | null) { | ||
| // only update session cookie from the active tab, | ||
| // or if the tab's selected organization matches the session's active organization | ||
| if (!document.hasFocus() && !this.isCurrentOrganizationActive()) { | ||
| return; | ||
| } | ||
| this.setActiveOrganizationInStorage(); | ||
| return token ? this.sessionCookie.set(token) : this.sessionCookie.remove(); | ||
| } | ||
| @@ -163,4 +183,30 @@ export class AuthCookieService { | ||
| clerkCoreErrorTokenRefreshFailed(e.toString()); | ||
| } | ||
| /** | ||
| * The below methods are used to determine whether or not an unfocused tab can be responsible | ||
| * for setting the session cookie. A session cookie should only be set by a tab who's selected | ||
| * organization matches the session's active organization. By storing the active organization | ||
| * ID in local storage, we can check the value across tabs. If a tab's organization ID does not | ||
| * match the value in local storage, it is not responsible for updating the session cookie. | ||
| */ | ||
| public setActiveOrganizationInStorage() { | ||
| if (this.clerk.organization?.id) { | ||
| localStorage.setItem('clerk_active_org', this.clerk.organization.id); | ||
| } else { | ||
| localStorage.removeItem('clerk_active_org'); | ||
| } | ||
| } | ||
| private isCurrentOrganizationActive() { | ||
| const activeOrganizationId = localStorage.getItem('clerk_active_org'); | ||
| if (!activeOrganizationId && !this.clerk.organization?.id) { | ||
| return true; | ||
| } | ||
| return this.clerk.organization?.id === activeOrganizationId; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -117,8 +117,8 @@ export class Session extends BaseResource implements SessionResource { | ||
| // and retrieve it using the session id concatenated with the jwt template name. | ||
| // e.g. session id is 'sess_abc12345' and jwt template name is 'haris' | ||
| // The session token ID will be 'sess_abc12345' and the jwt template token ID will be 'sess_abc12345-haris' | ||
| #getCacheId(template?: string) { | ||
| return `${template ? `${this.id}-${template}` : this.id}-${this.updatedAt.getTime()}`; | ||
| #getCacheId(template?: string, organizationId?: string) { | ||
| return [this.id, template, organizationId, this.updatedAt.getTime()].filter(Boolean).join('-'); | ||
| } | ||
| protected fromJSON(data: SessionJSON | null): this { | ||
| @@ -151,28 +151,38 @@ export class Session extends BaseResource implements SessionResource { | ||
| return null; | ||
| } | ||
| const { leewayInSeconds, template, skipCache = false } = options || {}; | ||
| const { | ||
| leewayInSeconds, | ||
| template, | ||
| skipCache = false, | ||
| organizationId = Session.clerk.organization?.id, | ||
| } = options || {}; | ||
| if (!template && Number(leewayInSeconds) >= 60) { | ||
| throw new Error('Leeway can not exceed the token lifespan (60 seconds)'); | ||
| } | ||
| const tokenId = this.#getCacheId(template); | ||
| const tokenId = this.#getCacheId(template, organizationId); | ||
| const cachedEntry = skipCache ? undefined : SessionTokenCache.get({ tokenId }, leewayInSeconds); | ||
| // Dispatch tokenUpdate only for __session tokens with the session's active organization ID, and not JWT templates | ||
| const shouldDispatchTokenUpdate = !template && options?.organizationId === Session.clerk.organization?.id; | ||
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. 🤔 I am kind of sceptic about using the MemberAuthor 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. @dimkl We don't want the session cookie to contain an org ID that isn't the active one. So if someone provides a different org ID that does not match the active org for the session, we don't want the cookie to be updated. MemberAuthor 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. Do you have a recommendation for a better way to access the currently selected org? 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. Could we take it from the session claims or from the MemberAuthor 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. switching the organization via | ||
| if (cachedEntry) { | ||
| const cachedToken = await cachedEntry.tokenResolver; | ||
| if (!template) { | ||
| if (shouldDispatchTokenUpdate) { | ||
| eventBus.dispatch(events.TokenUpdate, { token: cachedToken }); | ||
| } | ||
| // Return null when raw string is empty to indicate that there it's signed-out | ||
| return cachedToken.getRawString() || null; | ||
| } | ||
| const path = template ? `${this.path()}/tokens/${template}` : `${this.path()}/tokens`; | ||
| const tokenResolver = Token.create(path); | ||
| // TODO: update template endpoint to accept organizationId | ||
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. Is there a ticket for this? Just making sure we don't forget about it as it sounds pretty important :)
| ||
| const params = template ? {} : { ...(organizationId && { organizationId }) }; | ||
| const tokenResolver = Token.create(path, params); | ||
| SessionTokenCache.set({ tokenId, tokenResolver }); | ||
| return tokenResolver.then(token => { | ||
| // Dispatch tokenUpdate only for __session tokens and not JWT templates | ||
| if (!template) { | ||
| if (shouldDispatchTokenUpdate) { | ||
| eventBus.dispatch(events.TokenUpdate, { token }); | ||
| } | ||
| // Return null when raw string is empty to indicate that there it's signed-out | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.