diff --git a/.changeset/olive-oranges-march.md b/.changeset/olive-oranges-march.md new file mode 100644 index 00000000000..75333487305 --- /dev/null +++ b/.changeset/olive-oranges-march.md @@ -0,0 +1,6 @@ +--- +'@clerk/clerk-js': patch +'@clerk/testing': patch +--- + +Fix `toBeSignedOut` test-helper so it only resolves when `user === null`. It previously resolved for any falsy value, which could give false positives when Clerk had not loaded yet, or during auth-state changes. diff --git a/integration/templates/next-app-router/src/app/transitive-state/organization-switcher/[orgId]/page.tsx b/integration/templates/next-app-router/src/app/transitive-state/organization-switcher/[orgId]/page.tsx new file mode 100644 index 00000000000..0fd000d2df9 --- /dev/null +++ b/integration/templates/next-app-router/src/app/transitive-state/organization-switcher/[orgId]/page.tsx @@ -0,0 +1,9 @@ +export default async function Page({ params }: { params: Promise<{ orgId: string }> }) { + const { orgId } = await params; + + return ( +
+

{orgId}

+
+ ); +} diff --git a/integration/templates/next-app-router/src/app/transitive-state/organization-switcher/layout.tsx b/integration/templates/next-app-router/src/app/transitive-state/organization-switcher/layout.tsx new file mode 100644 index 00000000000..bdd2abb4091 --- /dev/null +++ b/integration/templates/next-app-router/src/app/transitive-state/organization-switcher/layout.tsx @@ -0,0 +1,47 @@ +'use client'; + +import { OrganizationSwitcher, useAuth } from '@clerk/nextjs'; +import { useState } from 'react'; +import { usePathname } from 'next/navigation'; + +function EmissionLog() { + const { orgId } = useAuth(); + const pathname = usePathname(); + const [log, setLog] = useState([]); + + const entry = `${pathname} - ${orgId}`; + if (entry !== log[log.length - 1]) { + setLog(prev => [...prev, entry]); + } + + return ( + + ); +} + +export default function Layout({ children }: { children: React.ReactNode }) { + return ( +
+
+ Loading organization switcher
} + afterSelectOrganizationUrl='/transitive-state/organization-switcher/:id' + /> +
+
+

Emission log

+ +
+ {children} + + ); +} diff --git a/integration/templates/next-app-router/src/app/transitive-state/sign-out/layout.tsx b/integration/templates/next-app-router/src/app/transitive-state/sign-out/layout.tsx new file mode 100644 index 00000000000..3e7a7449c17 --- /dev/null +++ b/integration/templates/next-app-router/src/app/transitive-state/sign-out/layout.tsx @@ -0,0 +1,41 @@ +'use client'; + +import { useAuth } from '@clerk/nextjs'; +import { useState } from 'react'; +import { usePathname } from 'next/navigation'; + +function EmissionLog() { + const { userId } = useAuth(); + const pathname = usePathname(); + const [log, setLog] = useState([]); + + const entry = `${pathname} - ${String(userId)}`; + if (entry !== log[log.length - 1]) { + setLog(prev => [...prev, entry]); + } + + return ( + + ); +} + +export default function Layout({ children }: { children: React.ReactNode }) { + return ( +
+
+

Emission log

+ +
+ {children} +
+ ); +} diff --git a/integration/templates/next-app-router/src/app/transitive-state/sign-out/page.tsx b/integration/templates/next-app-router/src/app/transitive-state/sign-out/page.tsx new file mode 100644 index 00000000000..e058bf8951a --- /dev/null +++ b/integration/templates/next-app-router/src/app/transitive-state/sign-out/page.tsx @@ -0,0 +1,10 @@ +import { SignOutButton } from '@clerk/nextjs'; + +export default function Page() { + return ( +
+

sign-out

+ +
+ ); +} diff --git a/integration/templates/next-app-router/src/app/transitive-state/sign-out/sign-in/page.tsx b/integration/templates/next-app-router/src/app/transitive-state/sign-out/sign-in/page.tsx new file mode 100644 index 00000000000..e4adf0c066b --- /dev/null +++ b/integration/templates/next-app-router/src/app/transitive-state/sign-out/sign-in/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return

sign-in

; +} diff --git a/integration/templates/next-app-router/src/app/transitive-state/user-button/layout.tsx b/integration/templates/next-app-router/src/app/transitive-state/user-button/layout.tsx new file mode 100644 index 00000000000..838e7ec86c2 --- /dev/null +++ b/integration/templates/next-app-router/src/app/transitive-state/user-button/layout.tsx @@ -0,0 +1,47 @@ +'use client'; + +import { UserButton, useAuth } from '@clerk/nextjs'; +import { useState } from 'react'; +import { usePathname } from 'next/navigation'; + +function EmissionLog() { + const { userId } = useAuth(); + const pathname = usePathname(); + const [log, setLog] = useState([]); + + const entry = `${pathname} - ${userId}`; + if (entry !== log[log.length - 1]) { + setLog(prev => [...prev, entry]); + } + + return ( +
    + {log.map((entry, i) => ( +
  • + {entry} +
  • + ))} +
+ ); +} + +export default function Layout({ children }: { children: React.ReactNode }) { + return ( +
+
+ Loading user button
} + afterSwitchSessionUrl='/transitive-state/user-button/switched' + /> +
+
+

Emission log

+ +
+ {children} + + ); +} diff --git a/integration/templates/next-app-router/src/app/transitive-state/user-button/page.tsx b/integration/templates/next-app-router/src/app/transitive-state/user-button/page.tsx new file mode 100644 index 00000000000..97b93f31041 --- /dev/null +++ b/integration/templates/next-app-router/src/app/transitive-state/user-button/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return

initial

; +} diff --git a/integration/templates/next-app-router/src/app/transitive-state/user-button/switched/page.tsx b/integration/templates/next-app-router/src/app/transitive-state/user-button/switched/page.tsx new file mode 100644 index 00000000000..9eb3ec71004 --- /dev/null +++ b/integration/templates/next-app-router/src/app/transitive-state/user-button/switched/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return

switched

; +} diff --git a/integration/tests/localhost/localhost-different-port-different-instance.test.ts b/integration/tests/localhost/localhost-different-port-different-instance.test.ts index ea81fd22caa..19ad35a3f5d 100644 --- a/integration/tests/localhost/localhost-different-port-different-instance.test.ts +++ b/integration/tests/localhost/localhost-different-port-different-instance.test.ts @@ -61,8 +61,8 @@ test.describe('multiple apps running on localhost using different Clerk instance expect(tab0Cookies.filter(c => c.name.startsWith('__clerk_db_jwt'))).toHaveLength(2); expect(tab0Cookies.filter(c => c.name.startsWith('__client_uat'))).toHaveLength(2); - await u[1].po.expect.toBeSignedOut(); await u[1].po.signIn.goTo(); + await u[1].po.expect.toBeSignedOut(); await u[1].po.signIn.signInWithEmailAndInstantPassword(fakeUsers[1]); await u[1].po.expect.toBeSignedIn(); diff --git a/integration/tests/localhost/localhost-different-port-same-instance.test.ts b/integration/tests/localhost/localhost-different-port-same-instance.test.ts index ed40508ced2..b0aa48974a5 100644 --- a/integration/tests/localhost/localhost-different-port-same-instance.test.ts +++ b/integration/tests/localhost/localhost-different-port-same-instance.test.ts @@ -93,6 +93,8 @@ test.describe('multiple apps running on localhost using same Clerk instance @loc // sign out from tab1 await u[1].page.goToAppHome(); + // This also ensures Clerk has loaded before evaluating the signOut + await u[1].po.expect.toBeSignedIn(); await u[1].page.evaluate(() => window.Clerk.signOut()); await u[1].po.expect.toBeSignedOut(); diff --git a/integration/tests/sign-out-smoke.test.ts b/integration/tests/sign-out-smoke.test.ts index 950269bd35f..6b040080bd5 100644 --- a/integration/tests/sign-out-smoke.test.ts +++ b/integration/tests/sign-out-smoke.test.ts @@ -87,6 +87,7 @@ testAgainstRunningApps({ withEnv: [appConfigs.envs.withEmailCodes] })('sign out await u.page.getByRole('button', { name: 'Open user menu' }).click(); await u.page.getByRole('menuitem', { name: 'Sign out' }).click(); + await u.po.expect.toBeSignedOut(); await u.page.getByRole('link', { name: 'Protected', exact: true }).click(); await u.page.waitForURL(url => url.href.includes('/sign-in?redirect_url')); }); diff --git a/integration/tests/transitive-state.test.ts b/integration/tests/transitive-state.test.ts new file mode 100644 index 00000000000..374743cbc4a --- /dev/null +++ b/integration/tests/transitive-state.test.ts @@ -0,0 +1,262 @@ +import { parsePublishableKey } from '@clerk/shared/keys'; +import { clerkSetup } from '@clerk/testing/playwright'; +import { expect, test } from '@playwright/test'; + +import { appConfigs } from '../presets'; +import type { FakeOrganization, FakeUser } from '../testUtils'; +import { createTestUtils, testAgainstRunningApps } from '../testUtils'; + +/* + These tests verify that useAuth emits the correct transitive state sequence when switching + auth context (org or user) with navigation. The expected pattern is: + Path A - Value A, Path A - undefined, Path B - undefined, Path B - Value B +*/ + +testAgainstRunningApps({ withEnv: [appConfigs.envs.withEmailCodes] })('transitive state @nextjs', ({ app }) => { + //test.describe.configure({ mode: 'serial' }); + + let fakeUser: FakeUser; + let orgA: FakeOrganization; + let orgB: FakeOrganization; + let userA: FakeUser; + let userB: FakeUser; + let userAId: string; + let userBId: string; + + test.beforeAll(async () => { + const u = createTestUtils({ app }); + + const publishableKey = appConfigs.envs.withEmailCodes.publicVariables.get('CLERK_PUBLISHABLE_KEY'); + const secretKey = appConfigs.envs.withEmailCodes.privateVariables.get('CLERK_SECRET_KEY'); + const apiUrl = appConfigs.envs.withEmailCodes.privateVariables.get('CLERK_API_URL'); + const { frontendApi: frontendApiUrl } = parsePublishableKey(publishableKey); + + await clerkSetup({ + publishableKey, + frontendApiUrl, + secretKey, + // @ts-expect-error Not typed + apiUrl, + dotenv: false, + }); + + // Org switching test: 1 user with 2 orgs + fakeUser = u.services.users.createFakeUser(); + const user = await u.services.users.createBapiUser(fakeUser); + orgB = await u.services.users.createFakeOrganization(user.id); + orgA = await u.services.users.createFakeOrganization(user.id); + + // User switching test: 2 users for multi-session + userA = u.services.users.createFakeUser(); + userB = u.services.users.createFakeUser(); + const createdUserA = await u.services.users.createBapiUser(userA); + const createdUserB = await u.services.users.createBapiUser(userB); + userAId = createdUserA.id; + userBId = createdUserB.id; + }); + + test.afterAll(async () => { + await orgA.delete(); + await orgB.delete(); + await fakeUser.deleteIfExists(); + await userA.deleteIfExists(); + await userB.deleteIfExists(); + await app.teardown(); + }); + + test('should emit correct transitive auth state when switching orgs with navigation', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + await u.po.signIn.goTo(); + await u.po.signIn.signInWithEmailAndInstantPassword({ email: fakeUser.email, password: fakeUser.password }); + await u.po.expect.toBeSignedIn(); + + const pathA = `/transitive-state/organization-switcher/${orgA.organization.id}`; + const pathB = `/transitive-state/organization-switcher/${orgB.organization.id}`; + + await u.po.page.goToRelative(pathA); + + // Wait for initial state to settle - emission log should contain pathA with orgA + await test + .expect(u.po.page.getByTestId('emission-log').locator(`li:has-text("${pathA} - ${orgA.organization.id}")`)) + .toBeVisible(); + + // Switch to orgB via OrganizationSwitcher + await u.po.organizationSwitcher.waitForMounted(); + await u.po.organizationSwitcher.waitForAnOrganizationToSelected(); + await u.po.organizationSwitcher.toggleTrigger(); + await test.expect(u.page.locator('.cl-organizationSwitcherPopoverCard')).toBeVisible(); + await u.page.getByText(orgB.name, { exact: true }).click(); + + // Wait for transition to complete - current-org-id shows orgB + await test.expect(u.po.page.getByTestId('current-org-id')).toHaveText(orgB.organization.id); + + // Assert the emission sequence: last 4 entries are Path A - Org A, Path A - undefined, Path B - undefined, Path B - Org B + const emissionItems = u.po.page.getByTestId('emission-log').locator('li'); + const count = await emissionItems.count(); + const texts: string[] = []; + for (let i = 0; i < count; i++) { + texts.push((await emissionItems.nth(i).textContent()) ?? ''); + } + + expect(texts.slice(-4)).toEqual([ + `${pathA} - ${orgA.organization.id}`, + `${pathA} - undefined`, + `${pathB} - undefined`, + `${pathB} - ${orgB.organization.id}`, + ]); + }); + + test('should emit correct transitive auth state when switching users with navigation', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + + const pathInitial = '/transitive-state/user-button'; + const pathSwitched = '/transitive-state/user-button/switched'; + + // Clear session from previous test + await context.clearCookies(); + + // Sign in as userA + await u.po.signIn.goTo(); + await u.po.signIn.signInWithEmailAndInstantPassword({ email: userA.email, password: userA.password }); + await u.po.expect.toBeSignedIn(); + + // Sign in as userB to create second session (multi-session) + await u.po.signIn.goTo(); + await u.po.signIn.setIdentifier(userB.email); + await u.po.signIn.continue(); + await u.po.signIn.setPassword(userB.password); + await u.po.signIn.continue(); + + // Avoid backend rate-limiting on session touch + await new Promise(resolve => setTimeout(resolve, 3000)); + + // Navigate to user-button page (userB is active) + await u.po.page.goToRelative(pathInitial); + + // Wait for initial state to settle - emission log should contain pathInitial with userB + await test + .expect(u.po.page.getByTestId('emission-log').locator(`li:has-text("${pathInitial} - ${userBId}")`)) + .toBeVisible(); + + // Switch to userA via UserButton + await u.po.userButton.waitForMounted(); + await u.po.userButton.toggleTrigger(); + await u.po.userButton.waitForPopover(); + await u.po.userButton.switchAccount(userA.email); + await u.po.userButton.waitForPopoverClosed(); + + // Wait for navigation to switched page + await test.expect(u.po.page.getByTestId('page-name')).toHaveText('switched'); + + // Assert the emission sequence + const emissionItems = u.po.page.getByTestId('emission-log').locator('li'); + const count = await emissionItems.count(); + const texts: string[] = []; + for (let i = 0; i < count; i++) { + texts.push((await emissionItems.nth(i).textContent()) ?? ''); + } + + expect(texts.slice(-4)).toEqual([ + `${pathInitial} - ${userBId}`, + `${pathInitial} - undefined`, + `${pathSwitched} - undefined`, + `${pathSwitched} - ${userAId}`, + ]); + }); + + test('should emit correct transitive auth state when signing out with navigation', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + + await context.clearCookies(); + + // Sign in as userA + await u.po.signIn.goTo(); + await u.po.signIn.signInWithEmailAndInstantPassword({ email: userA.email, password: userA.password }); + await u.po.expect.toBeSignedIn(); + + const pathA = '/transitive-state/sign-out'; + const pathB = '/transitive-state/sign-out/sign-in'; + + // Navigate to sign-out page + await u.po.page.goToRelative(pathA); + + // Wait for initial state to settle + await test + .expect(u.po.page.getByTestId('emission-log').locator(`li:has-text("${pathA} - ${userAId}")`)) + .toBeVisible(); + + // Click SignOutButton + await u.page.getByRole('button', { name: 'Sign out' }).click(); + + // Wait for navigation to sign-in page + await test.expect(u.po.page.getByTestId('page-name')).toHaveText('sign-in'); + + // Assert emission sequence + const emissionItems = u.po.page.getByTestId('emission-log').locator('li'); + const count = await emissionItems.count(); + const texts: string[] = []; + for (let i = 0; i < count; i++) { + texts.push((await emissionItems.nth(i).textContent()) ?? ''); + } + + expect(texts.slice(-4)).toEqual([ + `${pathA} - ${userAId}`, + `${pathA} - undefined`, + `${pathB} - undefined`, + `${pathB} - null`, + ]); + }); + + test('should emit correct transitive auth state when signing out with navigation (multi-session)', async ({ + page, + context, + }) => { + const u = createTestUtils({ app, page, context }); + + await context.clearCookies(); + + // Sign in as userA + await u.po.signIn.goTo(); + await u.po.signIn.signInWithEmailAndInstantPassword({ email: userA.email, password: userA.password }); + await u.po.expect.toBeSignedIn(); + + // Sign in as userB to create second session (multi-session) + await u.po.signIn.goTo(); + await u.po.signIn.setIdentifier(userB.email); + await u.po.signIn.continue(); + await u.po.signIn.setPassword(userB.password); + await u.po.signIn.continue(); + + const pathA = '/transitive-state/sign-out'; + const pathB = '/transitive-state/sign-out/sign-in'; + + // Navigate to sign-out page + await u.po.page.goToRelative(pathA); + + // Wait for initial state to settle + await test + .expect(u.po.page.getByTestId('emission-log').locator(`li:has-text("${pathA} - ${userBId}")`)) + .toBeVisible(); + + // Click SignOutButton + await u.page.getByRole('button', { name: 'Sign out' }).click(); + + // Wait for navigation to sign-in page + await test.expect(u.po.page.getByTestId('page-name')).toHaveText('sign-in'); + + // Assert emission sequence + const emissionItems = u.po.page.getByTestId('emission-log').locator('li'); + const count = await emissionItems.count(); + const texts: string[] = []; + for (let i = 0; i < count; i++) { + texts.push((await emissionItems.nth(i).textContent()) ?? ''); + } + + expect(texts.slice(-4)).toEqual([ + `${pathA} - ${userBId}`, + `${pathA} - undefined`, + `${pathB} - undefined`, + `${pathB} - null`, + ]); + }); +}); diff --git a/packages/clerk-js/bundlewatch.config.json b/packages/clerk-js/bundlewatch.config.json index c694488eb56..3114f47435d 100644 --- a/packages/clerk-js/bundlewatch.config.json +++ b/packages/clerk-js/bundlewatch.config.json @@ -3,7 +3,7 @@ { "path": "./dist/clerk.js", "maxSize": "539KB" }, { "path": "./dist/clerk.browser.js", "maxSize": "66KB" }, { "path": "./dist/clerk.chips.browser.js", "maxSize": "66KB" }, - { "path": "./dist/clerk.legacy.browser.js", "maxSize": "106KB" }, + { "path": "./dist/clerk.legacy.browser.js", "maxSize": "108KB" }, { "path": "./dist/clerk.no-rhc.js", "maxSize": "307KB" }, { "path": "./dist/clerk.native.js", "maxSize": "65KB" }, { "path": "./dist/vendors*.js", "maxSize": "7KB" }, diff --git a/packages/clerk-js/src/core/__tests__/clerk.test.ts b/packages/clerk-js/src/core/__tests__/clerk.test.ts index b467a476cad..7b9342ec05a 100644 --- a/packages/clerk-js/src/core/__tests__/clerk.test.ts +++ b/packages/clerk-js/src/core/__tests__/clerk.test.ts @@ -165,6 +165,7 @@ describe('Clerk singleton', () => { status: 'active', user: {}, touch: vi.fn(() => Promise.resolve()), + __internal_touch: vi.fn(() => Promise.resolve()), getToken: vi.fn(), lastActiveToken: { getRawString: () => 'mocked-token' }, }; @@ -177,6 +178,7 @@ describe('Clerk singleton', () => { afterEach(() => { mockSession.remove.mockReset(); mockSession.touch.mockReset(); + mockSession.__internal_touch.mockReset(); eventBusSpy?.mockRestore(); // cleanup global window pollution @@ -285,7 +287,7 @@ describe('Clerk singleton', () => { }); it('redirects the user to the /v1/client/touch endpoint if the cookie_expires_at is less than 8 days away', async () => { - mockSession.touch.mockReturnValue(Promise.resolve()); + mockSession.__internal_touch.mockReturnValue(Promise.resolve()); mockClientFetch.mockReturnValue( Promise.resolve({ signedInSessions: [mockSession], @@ -309,7 +311,7 @@ describe('Clerk singleton', () => { }); it('does not redirect the user to the /v1/client/touch endpoint if the cookie_expires_at is more than 8 days away', async () => { - mockSession.touch.mockReturnValue(Promise.resolve()); + mockSession.__internal_touch.mockReturnValue(Promise.resolve()); mockClientFetch.mockReturnValue( Promise.resolve({ signedInSessions: [mockSession], @@ -331,7 +333,7 @@ describe('Clerk singleton', () => { }); it('does not redirect the user to the /v1/client/touch endpoint if the cookie_expires_at is not set', async () => { - mockSession.touch.mockReturnValue(Promise.resolve()); + mockSession.__internal_touch.mockReturnValue(Promise.resolve()); mockClientFetch.mockReturnValue( Promise.resolve({ signedInSessions: [mockSession], @@ -353,19 +355,19 @@ describe('Clerk singleton', () => { }); it('calls `navigate`', async () => { - mockSession.touch.mockReturnValue(Promise.resolve()); + mockSession.__internal_touch.mockReturnValue(Promise.resolve()); mockClientFetch.mockReturnValue(Promise.resolve({ signedInSessions: [mockSession] })); const navigate = vi.fn(); const sut = new Clerk(productionPublishableKey); await sut.load(); await sut.setActive({ session: mockSession as any as PendingSessionResource, navigate }); - expect(mockSession.touch).toHaveBeenCalled(); + expect(mockSession.__internal_touch).toHaveBeenCalled(); expect(navigate).toHaveBeenCalled(); }); it('passes decorateUrl to the navigate callback', async () => { - mockSession.touch.mockReturnValue(Promise.resolve()); + mockSession.__internal_touch.mockReturnValue(Promise.resolve()); mockClientFetch.mockReturnValue(Promise.resolve({ signedInSessions: [mockSession] })); const navigate = vi.fn(); @@ -382,7 +384,7 @@ describe('Clerk singleton', () => { }); it('decorateUrl returns touch URL when isEligibleForTouch is true', async () => { - mockSession.touch.mockReturnValue(Promise.resolve()); + mockSession.__internal_touch.mockReturnValue(Promise.resolve()); mockClientFetch.mockReturnValue( Promise.resolve({ signedInSessions: [mockSession], @@ -412,7 +414,7 @@ describe('Clerk singleton', () => { }); it('decorateUrl returns original URL when isEligibleForTouch is false', async () => { - mockSession.touch.mockReturnValue(Promise.resolve()); + mockSession.__internal_touch.mockReturnValue(Promise.resolve()); mockClientFetch.mockReturnValue( Promise.resolve({ signedInSessions: [mockSession], @@ -468,6 +470,7 @@ describe('Clerk singleton', () => { status: 'pending', user: {}, touch: vi.fn(() => Promise.resolve()), + __internal_touch: vi.fn(() => Promise.resolve()), getToken: vi.fn(), lastActiveToken: { getRawString: () => 'mocked-token' }, tasks: [{ key: 'choose-organization' }], @@ -493,6 +496,7 @@ describe('Clerk singleton', () => { afterEach(() => { mockSession.remove.mockReset(); mockSession.touch.mockReset(); + mockSession.__internal_touch?.mockReset(); eventBusSpy?.mockRestore(); // cleanup global window pollution @@ -537,7 +541,7 @@ describe('Clerk singleton', () => { }); it('navigate to `taskUrl` option', async () => { - mockSession.touch.mockReturnValue(Promise.resolve()); + mockSession.__internal_touch.mockReturnValue(Promise.resolve()); mockClientFetch.mockReturnValue(Promise.resolve({ signedInSessions: [mockSession] })); const sut = new Clerk(productionPublishableKey); @@ -548,19 +552,19 @@ describe('Clerk singleton', () => { }, }); await sut.setActive({ session: mockSession as any as PendingSessionResource }); - expect(mockSession.touch).toHaveBeenCalled(); + expect(mockSession.__internal_touch).toHaveBeenCalled(); expect(sut.navigate).toHaveBeenCalledWith('/choose-organization'); }); it('calls `navigate`', async () => { - mockSession.touch.mockReturnValue(Promise.resolve()); + mockSession.__internal_touch.mockReturnValue(Promise.resolve()); mockClientFetch.mockReturnValue(Promise.resolve({ signedInSessions: [mockSession] })); const navigate = vi.fn(); const sut = new Clerk(productionPublishableKey); await sut.load(); await sut.setActive({ session: mockSession as any as PendingSessionResource, navigate }); - expect(mockSession.touch).toHaveBeenCalled(); + expect(mockSession.__internal_touch).toHaveBeenCalled(); expect(navigate).toHaveBeenCalled(); }); }); @@ -1045,11 +1049,12 @@ describe('Clerk singleton', () => { ...mockSession, remove: vi.fn(), touch: vi.fn(() => Promise.resolve()), + __internal_touch: vi.fn(() => Promise.resolve()), getToken: vi.fn(), reload: vi.fn(() => Promise.resolve(mockSession)), }; - mockResource.touch.mockReturnValueOnce(Promise.resolve()); + mockResource.__internal_touch.mockReturnValueOnce(Promise.resolve()); mockClientFetch.mockReturnValue( Promise.resolve({ signedInSessions: [mockResource], @@ -1587,6 +1592,7 @@ describe('Clerk singleton', () => { status: 'active', user: {}, touch: vi.fn(() => Promise.resolve()), + __internal_touch: vi.fn(() => Promise.resolve()), getToken: vi.fn(), lastActiveToken: { getRawString: () => 'mocked-token' }, }; @@ -1648,6 +1654,7 @@ describe('Clerk singleton', () => { status: 'active', user: {}, touch: vi.fn(() => Promise.resolve()), + __internal_touch: vi.fn(() => Promise.resolve()), getToken: vi.fn(), lastActiveToken: { getRawString: () => 'mocked-token' }, }; @@ -2607,6 +2614,35 @@ describe('Clerk singleton', () => { expect(mockOnAfterSetActive).toHaveBeenCalledTimes(1); }); }); + + it('does not emit to listeners when __internal_dangerouslySkipEmit is true', () => { + const mockSession = { + id: 'session_1', + status: 'active', + user: { id: 'user_1' }, + lastActiveToken: { getRawString: () => 'token_1' }, + }; + + const mockClient = { + sessions: [mockSession], + signedInSessions: [mockSession], + lastActiveSessionId: 'session_1', + }; + + const sut = new Clerk(productionPublishableKey); + sut.updateClient(mockClient as any); + + const listener = vi.fn(); + const unsubscribe = sut.addListener(listener, { skipInitialEmit: true }); + + listener.mockClear(); + + sut.updateClient(mockClient as any, { __internal_dangerouslySkipEmit: true }); + + unsubscribe(); + + expect(listener).not.toHaveBeenCalled(); + }); }); describe('__internal_attemptToEnableEnvironmentSetting', () => { diff --git a/packages/clerk-js/src/core/clerk.ts b/packages/clerk-js/src/core/clerk.ts index 0ddb68921bf..1fab086f54f 100644 --- a/packages/clerk-js/src/core/clerk.ts +++ b/packages/clerk-js/src/core/clerk.ts @@ -176,6 +176,7 @@ import { APIKeys } from './modules/apiKeys'; import { Billing } from './modules/billing'; import { createCheckoutInstance } from './modules/checkout/instance'; import { Protect } from './protect'; +import type { Session } from './resources/internal'; import { BaseResource, Client, Environment, Organization, Waitlist } from './resources/internal'; import { State } from './state'; @@ -615,8 +616,6 @@ export class Clerk implements ClerkInterface { // Notify other tabs that user is signing out and clean up cookies. eventBus.emit(events.UserSignOut, null); - this.#setTransitiveState(); - await tracker.track(async () => { if (signOutCallback) { await signOutCallback(); @@ -640,7 +639,10 @@ export class Clerk implements ClerkInterface { * Since we are calling `onBeforeSetActive` before signing out, we should NOT pass `"sign-out"`. */ await onBeforeSetActive(); + if (!opts.sessionId || this.client.signedInSessions.length === 1) { + this.#setTransitiveState(); + if (this.#options.experimental?.persistClient ?? true) { await this.client.removeSessions(); } else { @@ -657,11 +659,13 @@ export class Clerk implements ClerkInterface { const session = this.client.signedInSessions.find(s => s.id === opts.sessionId); const shouldSignOutCurrent = session?.id && this.session?.id === session.id; - await session?.remove(); - if (shouldSignOutCurrent) { + this.#setTransitiveState(); + await session?.remove(); await executeSignOut(); debugLogger.info('signOut() complete', { redirectUrl: stripOrigin(redirectUrl) }, 'clerk'); + } else { + await session?.remove(); } }; @@ -1550,13 +1554,53 @@ export class Clerk implements ClerkInterface { await onBeforeSetActive(newSession === null ? 'sign-out' : undefined); } + const taskUrl = + newSession?.status === 'pending' && + newSession?.currentTask && + this.#options.taskUrls?.[newSession?.currentTask.key]; + const shouldNavigate = !!(redirectUrl || taskUrl || setActiveNavigate); + //1. setLastActiveSession to passed user session (add a param). // Note that this will also update the session's active organization // id. + /* + The introduction of useSyncExternalStore introduced a bug here, where the + updateClient that happens as a result of this touch emitted and immediately + caused components to re-render. Previously, that emit was batched with the + setTransitiveState call. + + This whole block should likely happen later, after the transitive state is set. + + Because we want to minimize behavioral changes until we can tackle this properly, + this was refactored so that updateClient does not emit under these circumstances. + */ if (inActiveBrowserTab() || !this.#options.standardBrowser) { - await this.#touchCurrentSession(newSession); - // reload session from updated client - newSession = this.#getSessionFromClient(newSession?.id); + let updatedClient: ClientResource | undefined; + if (shouldNavigate && newSession) { + try { + // __internal_touch does not call updateClient automatically + updatedClient = await (newSession as Session).__internal_touch(); + if (updatedClient) { + // We call updateClient manually, but without letting it emit + // It's important that the setTransitiveState call happens somewhat + // quickly after this, as during this period, the internal Clerk + // state does not match the emitted state. + this.updateClient(updatedClient, { __internal_dangerouslySkipEmit: true }); + } + } catch (e) { + if (is4xxError(e)) { + void this.handleUnauthenticated(); + } else { + throw e; + } + } + } else { + await this.#touchCurrentSession(newSession); + } + // If we do have the updatedClient, read from that, otherwise getSessionFromClient + // will fallback to this.client. This makes no difference now, but will if we + // decide to move the `updateClient` call out of this block later. + newSession = this.#getSessionFromClient(newSession?.id, updatedClient); } // getToken syncs __session and __client_uat to cookies using events.TokenUpdate dispatched event. @@ -1583,12 +1627,7 @@ export class Clerk implements ClerkInterface { // automatic reloading when reloading shouldn't be happening. const tracker = createBeforeUnloadTracker(this.#options.standardBrowser); - const taskUrl = - newSession?.status === 'pending' && - newSession?.currentTask && - this.#options.taskUrls?.[newSession?.currentTask.key]; - - if (redirectUrl || taskUrl || setActiveNavigate) { + if (shouldNavigate) { await tracker.track(async () => { if (!this.client) { // Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous. @@ -2657,7 +2696,10 @@ export class Clerk implements ClerkInterface { this.internal_last_error = value; } - updateClient = (newClient: ClientResource): void => { + // Skipping emit is dangerous because if there is a gap between setting these + // and emitting, library consumers that both read state directly and set up listeners + // could end up in a inconsistent state. + updateClient = (newClient: ClientResource, options?: { __internal_dangerouslySkipEmit?: boolean }): void => { if (!this.client) { // This is the first time client is being // set, so we also need to set session @@ -2702,7 +2744,9 @@ export class Clerk implements ClerkInterface { eventBus.emit(events.TokenUpdate, { token: this.session?.lastActiveToken }); } - this.#emit(); + if (!options?.__internal_dangerouslySkipEmit) { + this.#emit(); + } }; get __internal_environment(): EnvironmentResource | null | undefined { diff --git a/packages/clerk-js/src/core/resources/Base.ts b/packages/clerk-js/src/core/resources/Base.ts index cc1b5db9819..2c865c9858e 100644 --- a/packages/clerk-js/src/core/resources/Base.ts +++ b/packages/clerk-js/src/core/resources/Base.ts @@ -13,11 +13,11 @@ import { debugLogger } from '@/utils/debug'; import { clerkMissingFapiClientInResources } from '../errors'; import type { FapiClient, FapiRequestInit, FapiResponse, FapiResponseJSON, HTTPMethod } from '../fapiClient'; import { FraudProtection } from '../fraudProtection'; -import type { Clerk } from './internal'; -import { Client } from './internal'; +import { type Clerk, getClientResourceFromPayload } from './internal'; export type BaseFetchOptions = ClerkResourceReloadParams & { forceUpdateClient?: boolean; + skipUpdateClient?: boolean; fetchMaxTries?: number; }; @@ -123,7 +123,7 @@ export abstract class BaseResource { } // TODO: Link to Client payload piggybacking design document - if (requestInit.method !== 'GET' || opts.forceUpdateClient) { + if ((requestInit.method !== 'GET' || opts.forceUpdateClient) && !opts.skipUpdateClient) { this._updateClient(payload); } @@ -163,15 +163,9 @@ export abstract class BaseResource { } protected static _updateClient(responseJSON: FapiResponseJSON | null): void { - if (!responseJSON) { - return; - } - - // TODO: Revise Client piggybacking - const client = responseJSON.client || responseJSON.meta?.client; - + const client = getClientResourceFromPayload(responseJSON); if (client && BaseResource.clerk) { - BaseResource.clerk.updateClient(Client.getOrCreateInstance().fromJSON(client)); + BaseResource.clerk.updateClient(client); } } diff --git a/packages/clerk-js/src/core/resources/Client.ts b/packages/clerk-js/src/core/resources/Client.ts index e437e24d9e5..986252b20dd 100644 --- a/packages/clerk-js/src/core/resources/Client.ts +++ b/packages/clerk-js/src/core/resources/Client.ts @@ -10,9 +10,18 @@ import type { import { unixEpochToDate } from '../../utils/date'; import { eventBus } from '../events'; +import type { FapiResponseJSON } from '../fapiClient'; import { SessionTokenCache } from '../tokenCache'; import { BaseResource, Session, SignIn, SignUp } from './internal'; +export function getClientResourceFromPayload(responseJSON: FapiResponseJSON | null): ClientResource | undefined { + if (!responseJSON) { + return undefined; + } + const clientJSON = responseJSON.client || responseJSON.meta?.client; + return clientJSON ? Client.getOrCreateInstance().fromJSON(clientJSON) : undefined; +} + export class Client extends BaseResource implements ClientResource { private static instance: Client | null | undefined; diff --git a/packages/clerk-js/src/core/resources/Session.ts b/packages/clerk-js/src/core/resources/Session.ts index 5facf261e91..894e0639734 100644 --- a/packages/clerk-js/src/core/resources/Session.ts +++ b/packages/clerk-js/src/core/resources/Session.ts @@ -10,6 +10,7 @@ import { retry } from '@clerk/shared/retry'; import type { ActClaim, CheckAuthorization, + ClientResource, EmailCodeConfig, EnterpriseSSOConfig, GetToken, @@ -38,8 +39,9 @@ import { TokenId } from '@/utils/tokenId'; import { clerkInvalidStrategy, clerkMissingWebAuthnPublicKeyOptions } from '../errors'; import { eventBus, events } from '../events'; +import type { FapiResponseJSON } from '../fapiClient'; import { SessionTokenCache } from '../tokenCache'; -import { BaseResource, PublicUserData, Token, User } from './internal'; +import { BaseResource, getClientResourceFromPayload, PublicUserData, Token, User } from './internal'; import { SessionVerification } from './SessionVerification'; export class Session extends BaseResource implements SessionResource { @@ -91,17 +93,50 @@ export class Session extends BaseResource implements SessionResource { }); }; - touch = (): Promise => { - return this._basePost({ - action: 'touch', - body: { active_organization_id: this.lastActiveOrganizationId }, - }).then(res => { - // touch() will potentially change the session state, and so we need to ensure we emit the updated token that comes back in the response. This avoids potential issues where the session cookie is out of sync with the current session state. - if (res.lastActiveToken) { - eventBus.emit(events.TokenUpdate, { token: res.lastActiveToken }); - } - return res; - }); + private _touchPost = async ( + { skipUpdateClient }: { skipUpdateClient: boolean } = { skipUpdateClient: false }, + ): Promise | null> => { + const json = await BaseResource._fetch( + { + method: 'POST', + path: this.path('touch'), + // any is how we type the body in the BaseMutateParams as well + body: { active_organization_id: this.lastActiveOrganizationId } as any, + }, + { skipUpdateClient }, + ); + + // Update session in-place from response (same as BaseResource._baseMutate) + this.fromJSON((json?.response || json) as SessionJSON); + + return json; + }; + + touch = async (): Promise => { + await this._touchPost(); + + // _touchPost() will have updated `this` in-place + // The post has potentially changed the session state, and so we need to ensure we emit the updated token that comes back in the response. This avoids potential issues where the session cookie is out of sync with the current session state. + if (this.lastActiveToken) { + eventBus.emit(events.TokenUpdate, { token: this.lastActiveToken }); + } + + return this; + }; + + /** + * Internal method to touch the session without updating the client or explicitly emitting the TokenUpdate event. + * + * Returns the piggybacked client resource if it exists, otherwise undefined. + * + * The caller is responsible for calling updateClient(result), which internally also emits TokenUpdate. + * If updateClient() is not called, the server state and client state will be out of sync. + * + * @internal + */ + __internal_touch = async (): Promise => { + const json = await this._touchPost({ skipUpdateClient: true }); + return getClientResourceFromPayload(json); }; clearCache = (): void => { diff --git a/packages/clerk-js/src/core/resources/__tests__/Session.test.ts b/packages/clerk-js/src/core/resources/__tests__/Session.test.ts index eecf9a4fb40..0968ea56f8c 100644 --- a/packages/clerk-js/src/core/resources/__tests__/Session.test.ts +++ b/packages/clerk-js/src/core/resources/__tests__/Session.test.ts @@ -658,6 +658,110 @@ describe('Session', () => { }); }); + describe('__internal_touch()', () => { + const mockSessionData = { + status: 'active', + id: 'session_1', + object: 'session', + user: createUser({}), + last_active_organization_id: 'org_123', + actor: null, + created_at: new Date().getTime(), + updated_at: new Date().getTime(), + last_active_token: { object: 'token', jwt: mockJwt }, + } as SessionJSON; + + const mockClientData = { + object: 'client', + id: 'client_1', + sessions: [mockSessionData], + sign_up: null, + sign_in: null, + last_active_session_id: 'session_1', + created_at: new Date().getTime(), + updated_at: new Date().getTime(), + }; + + beforeEach(() => { + BaseResource.clerk = clerkMock(); + }); + + afterEach(() => { + BaseResource.clerk = null as any; + }); + + it('does not dispatch token:update event', async () => { + const dispatchSpy = vi.spyOn(eventBus, 'emit'); + const session = new Session(mockSessionData); + + (BaseResource.clerk.getFapiClient().request as Mock).mockResolvedValue({ + payload: { response: mockSessionData, client: mockClientData }, + }); + + await session.__internal_touch(); + + expect(dispatchSpy).not.toHaveBeenCalledWith('token:update', expect.anything()); + dispatchSpy.mockRestore(); + }); + + it('does not call clerk.updateClient', async () => { + const session = new Session(mockSessionData); + const updateClientSpy = vi.fn(); + BaseResource.clerk = clerkMock({ updateClient: updateClientSpy } as any); + + (BaseResource.clerk.getFapiClient().request as Mock).mockResolvedValue({ + payload: { response: mockSessionData, client: mockClientData }, + status: 200, + }); + + await session.__internal_touch(); + + expect(updateClientSpy).not.toHaveBeenCalled(); + }); + + it('returns piggybacked client when present in response', async () => { + const session = new Session(mockSessionData); + + (BaseResource.clerk.getFapiClient().request as Mock).mockResolvedValue({ + payload: { response: mockSessionData, client: mockClientData }, + status: 200, + }); + + const result = await session.__internal_touch(); + + expect(result).toBeDefined(); + expect(result?.id).toBe('client_1'); + expect(result?.sessions).toHaveLength(1); + }); + + it('returns undefined when response has no piggybacked client', async () => { + const session = new Session(mockSessionData); + + (BaseResource.clerk.getFapiClient().request as Mock).mockResolvedValue({ + payload: { response: mockSessionData }, + status: 200, + }); + + const result = await session.__internal_touch(); + + expect(result).toBeUndefined(); + }); + + it('updates session in-place from response', async () => { + const session = new Session(mockSessionData); + const updatedSessionData = { ...mockSessionData, last_active_organization_id: 'org_456' }; + + (BaseResource.clerk.getFapiClient().request as Mock).mockResolvedValue({ + payload: { response: updatedSessionData }, + status: 200, + }); + + await session.__internal_touch(); + + expect(session.lastActiveOrganizationId).toBe('org_456'); + }); + }); + describe('isAuthorized()', () => { it('user with permission to delete the organization should be able to delete the organization', async () => { const session = new Session({ diff --git a/packages/testing/src/playwright/unstable/page-objects/expect.ts b/packages/testing/src/playwright/unstable/page-objects/expect.ts index 82ab7834e98..e019a01d481 100644 --- a/packages/testing/src/playwright/unstable/page-objects/expect.ts +++ b/packages/testing/src/playwright/unstable/page-objects/expect.ts @@ -15,7 +15,7 @@ export const createExpectPageObject = ({ page }: { page: EnhancedPage }) => { toBeSignedOut: (args?: { timeOut: number }) => { return page.waitForFunction( () => { - return !window.Clerk?.user; + return window.Clerk?.user === null; }, null, { timeout: args?.timeOut },