Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 470
feat(clerk-js): Allow token refresh when Client failed to resolve#5345
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
506a6f67c6a61f3bf2569a932787964cdc45a6ff318d87d26843ac1cc6ffed9c3451bd0fcfedcffb01467839ed24c5ccae2b4067811bc05b8ef5751d5c76c6d5ef0fc0eba411cdbb27cb9bd3fcffdee075bca248782039078cd87c14af574File 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,6 @@ | ||
| --- | ||
| '@clerk/clerk-js': patch | ||
| '@clerk/types': patch | ||
| --- | ||
| Allow token refresh when Client failed to resolve. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| import { test } from '@playwright/test'; | ||
| import { appConfigs } from '../presets'; | ||
| import type { FakeUser } from '../testUtils'; | ||
| import { createTestUtils, testAgainstRunningApps } from '../testUtils'; | ||
| testAgainstRunningApps({ withEnv: [appConfigs.envs.withEmailCodes] })('resiliency @generic', ({ app }) => { | ||
| test.describe.configure({ mode: 'serial' }); | ||
| let fakeUser: FakeUser; | ||
| test.beforeAll(async () => { | ||
| const u = createTestUtils({ app }); | ||
| fakeUser = u.services.users.createFakeUser(); | ||
| await u.services.users.createBapiUser(fakeUser); | ||
| }); | ||
| test.afterAll(async () => { | ||
| await fakeUser.deleteIfExists(); | ||
| await app.teardown(); | ||
| }); | ||
| test('signed in users can get a fresh session token when Client fails to load', async ({ page, context }) => { | ||
| const u = createTestUtils({ app, page, context }); | ||
| await u.po.signIn.goTo(); | ||
| let waitForClientImmediately = page.waitForResponse(response => response.url().includes('/sign_ins'), { | ||
| timeout: 3_000, | ||
| }); | ||
| await u.po.signIn.signInWithEmailAndInstantPassword({ email: fakeUser.email, password: fakeUser.password }); | ||
| const clientReponse = await waitForClientImmediately; | ||
| const d = await clientReponse.json(); | ||
| await u.po.expect.toBeSignedIn(); | ||
| // Simulate developer coming back and client fails to load. | ||
| await page.route('**/v1/client?**', route => { | ||
| return route.fulfill({ | ||
| status: 500, | ||
| body: JSON.stringify({ | ||
| errors: [ | ||
| { | ||
| message: 'Oops, an unexpected error occurred', | ||
| long_message: | ||
| "There was an internal error on our servers. We've been notified and are working on fixing it.", | ||
| code: 'internal_clerk_error', | ||
| }, | ||
| ], | ||
| clerk_trace_id: 'some-trace-id', | ||
| }), | ||
| }); | ||
| }); | ||
| await page.reload(); | ||
| waitForClientImmediately = page.waitForResponse( | ||
| response => response.url().includes('/client?') && response.status() === 500, | ||
| { timeout: 3_000 }, | ||
| ); | ||
| const waitForTokenImmediately = page.waitForResponse( | ||
| response => | ||
| response.url().includes('/tokens?') && response.status() === 200 && response.request().method() === 'POST', | ||
| { timeout: 3_000 }, | ||
| ); | ||
| await page.waitForLoadState('domcontentloaded'); | ||
| await waitForClientImmediately; | ||
| await waitForTokenImmediately; | ||
| // Wait for the client to be loaded. and the internal `getToken({skipCache: true})` to have been completed. | ||
| await u.po.clerk.toBeLoaded(); | ||
| await u.po.expect.toBeSignedIn(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -121,6 +121,7 @@ import { | ||
| import { eventBus, events } from './events'; | ||
| import type { FapiClient, FapiRequestCallback } from './fapiClient'; | ||
| import { createFapiClient } from './fapiClient'; | ||
| import { createClientFromJwt } from './jwt-client'; | ||
| import { __experimental_Commerce } from './modules/commerce'; | ||
| import { | ||
| BaseResource, | ||
| @@ -2052,10 +2053,26 @@ export class Clerk implements ClerkInterface { | ||
| this.updateEnvironment(res); | ||
| }); | ||
| const initClient = () => { | ||
| const initClient = async () => { | ||
| return Client.getOrCreateInstance() | ||
| .fetch() | ||
| .then(res => this.updateClient(res)); | ||
| .then(res => this.updateClient(res)) | ||
| .catch(async e => { | ||
| if (isClerkAPIResponseError(e) && e.errors[0].code === 'requires_captcha') { | ||
| throw e; | ||
| } | ||
| const jwtInCookie = this.#authService?.getSessionCookie(); | ||
| const localClient = createClientFromJwt(jwtInCookie); | ||
| this.updateClient(localClient); | ||
| // Always grab a fresh token | ||
| await this.session?.getToken({ skipCache: true }); | ||
| // Allows for Clerk to be marked as loaded with the client and session created from the JWT. | ||
| return null; | ||
| }); | ||
Comment on lines
+2059
to
+2075
ContributorAuthor 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 wouldn't focus too much on where this code is located, we might refactor once both this and #5287 are merged. | ||
| }; | ||
| const initComponents = () => { | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| import type { | ||
| ClientJSON, | ||
| OrganizationMembershipJSON, | ||
| PartialWithClerkResource, | ||
| PublicUserDataJSON, | ||
| SessionJSON, | ||
| TokenJSON, | ||
| UserJSON, | ||
| } from '@clerk/types'; | ||
| import { Token } from './resources'; | ||
| import { Client } from './resources/Client'; | ||
| /** | ||
| * Create a new client instance from a jwt. | ||
| * The caller is responsible for reading the jwt from the `__session` cookie. | ||
| */ | ||
| export function createClientFromJwt(jwt: string | undefined | null): Client { | ||
| // Use `Token` class to parse the JWT token | ||
| let token: Token | null; | ||
| try { | ||
| token = new Token({ | ||
| jwt: jwt || '', | ||
| object: 'token', | ||
| // @ts-expect-error - ts is not happy about it, but this is allowed | ||
| id: undefined, | ||
| }); | ||
| } catch { | ||
| // If the token is invalid, return null | ||
| token = null; | ||
| } | ||
| // Clean up singleton instance | ||
| Client.clearInstance(); | ||
| if (!token?.jwt) { | ||
| return Client.getOrCreateInstance({ | ||
| object: 'client', | ||
| last_active_session_id: null, | ||
| id: 'client_init', | ||
| sessions: [], | ||
| } as unknown as ClientJSON); | ||
| } | ||
| const { sid, sub, org_id, org_role, org_permissions, org_slug, fva } = token.jwt.claims; | ||
| const defaultClient = { | ||
| object: 'client', | ||
| last_active_session_id: sid, | ||
| id: 'client_init', | ||
| sessions: [ | ||
| { | ||
| object: 'session', | ||
| id: sid, | ||
| status: 'active', | ||
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. This could be added in a later PR, but we now have a new claim in the JWT: We should extract it here and default to ContributorAuthor 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. Doing this in another PR sounds proper. TBH i don't think it is that important, because it might force redirect you to the appropriate page, but clerk will be down so you will not be able to proceed. | ||
| last_active_organization_id: org_id || null, | ||
| // @ts-expect-error - ts is not happy about `id:undefined`, but this is allowed and expected | ||
| last_active_token: { | ||
| id: undefined, | ||
| object: 'token', | ||
| jwt, | ||
| } as TokenJSON, | ||
| factor_verification_age: fva || null, | ||
| public_user_data: { | ||
| user_id: sub, | ||
| } as PublicUserDataJSON, | ||
| user: { | ||
| object: 'user', | ||
| id: sub, | ||
| organization_memberships: | ||
| org_id && org_slug && org_role | ||
| ? [ | ||
| { | ||
| object: 'organization_membership', | ||
| id: org_id, | ||
| role: org_role, | ||
| permissions: org_permissions || [], | ||
| organization: { | ||
| object: 'organization', | ||
| id: org_id, | ||
| // Use slug as name for the organization, since name is not available in the token. | ||
| name: org_slug, | ||
| slug: org_slug, | ||
| members_count: 1, | ||
| max_allowed_memberships: 1, | ||
| }, | ||
| } as PartialWithClerkResource<OrganizationMembershipJSON>, | ||
| ] | ||
| : [], | ||
| } as PartialWithClerkResource<UserJSON>, | ||
| } as PartialWithClerkResource<SessionJSON>, | ||
| ], | ||
| } as ClientJSON; | ||
| return Client.getOrCreateInstance(defaultClient); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
We need to stop throwing errors on status 5xx here. These errors are not useful to us, and developers cannot catch them and handle them.
In case of an FAPI outage, the poller would continue to throw errors, that would pollute a customer's Sentry logs