Skip to content

Commit

Permalink
chore(clerk-js): Simplify dev browser handling (#2345)
Browse files Browse the repository at this point in the history
1. Work only with URL based session syncing dev instances and drop support for cookie-based syncing dev instances.
2. Align nomenclature across cookie, hearer and search param names
3. Drop cookie handler in favor of dedicated cookie handlers, one for each cookie (__clerk_db_jwt, __session, __client_uat)
  • Loading branch information
SokratisVidros committed Dec 15, 2023
1 parent f58a994 commit 7e76b41
Show file tree
Hide file tree
Showing 22 changed files with 235 additions and 552 deletions.
2 changes: 2 additions & 0 deletions .changeset/eighty-walls-appear.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
---
---
4 changes: 2 additions & 2 deletions packages/chrome-extension/src/singleton.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Clerk } from '@clerk/clerk-js';
import type { ClerkProp } from '@clerk/clerk-react';
import { DEV_BROWSER_JWT_MARKER } from '@clerk/shared';
import { DEV_BROWSER_JWT_KEY } from '@clerk/shared';
import { parsePublishableKey } from '@clerk/shared/keys';
import browser from 'webextension-polyfill';

Expand Down Expand Up @@ -50,7 +50,7 @@ export async function buildClerk({
}
: {
urls: validHosts,
name: DEV_BROWSER_JWT_MARKER,
name: DEV_BROWSER_JWT_KEY,
};

// Get client cookie from browser
Expand Down
77 changes: 25 additions & 52 deletions packages/clerk-js/src/core/clerk.redirects.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,13 @@ jest.mock('./resources/Client');
jest.mock('./resources/Environment');

// Because Jest, don't ask me why...
jest.mock('./devBrowserHandler', () => ({
createDevBrowserHandler: () => ({
jest.mock('./devBrowser', () => ({
createDevBrowser: () => ({
clear: jest.fn(),
setup: jest.fn(),
getDevBrowserJWT: jest.fn(() => 'deadbeef'),
setDevBrowserJWT: jest.fn(),
removeDevBrowserJWT: jest.fn(),
usesUrlBasedSessionSync: mockUsesUrlBasedSessionSync,
}),
}));

Expand Down Expand Up @@ -278,61 +277,35 @@ describe('Clerk singleton - Redirects', () => {
let clerkForProductionInstance: Clerk;
let clerkForDevelopmentInstance: Clerk;

describe('for cookie-based dev instances', () => {
beforeEach(async () => {
mockUsesUrlBasedSessionSync.mockReturnValue(false);

clerkForProductionInstance = new Clerk(productionPublishableKey);
await clerkForProductionInstance.load({
routerPush: mockNavigate,
});

clerkForDevelopmentInstance = new Clerk(developmentPublishableKey);
await clerkForDevelopmentInstance.load({
routerPush: mockNavigate,
});
beforeEach(async () => {
mockUsesUrlBasedSessionSync.mockReturnValue(true);
mockEnvironmentFetch.mockReturnValue(
Promise.resolve({
authConfig: { urlBasedSessionSyncing: true } as AuthConfig,
userSettings: mockUserSettings,
displayConfig: mockDisplayConfigWithDifferentOrigin,
isProduction: () => false,
isDevelopmentOrStaging: () => true,
}),
);

clerkForProductionInstance = new Clerk(productionPublishableKey);
await clerkForProductionInstance.load({
routerPush: mockNavigate,
});

it('redirects to the provided url without __clerk_db_jwt in the url', async () => {
await clerkForProductionInstance.redirectWithAuth('https://app.example.com');
expect(mockHref).toHaveBeenNthCalledWith(1, 'https://app.example.com/');

await clerkForDevelopmentInstance.redirectWithAuth('https://app.example.com');
expect(mockHref).toHaveBeenNthCalledWith(2, 'https://app.example.com/');
clerkForDevelopmentInstance = new Clerk(developmentPublishableKey);
await clerkForDevelopmentInstance.load({
routerPush: mockNavigate,
});
});

describe('for dev instances that use url based session syncing', () => {
beforeEach(async () => {
mockUsesUrlBasedSessionSync.mockReturnValue(true);
mockEnvironmentFetch.mockReturnValue(
Promise.resolve({
authConfig: { urlBasedSessionSyncing: true } as AuthConfig,
userSettings: mockUserSettings,
displayConfig: mockDisplayConfigWithDifferentOrigin,
isProduction: () => false,
isDevelopmentOrStaging: () => true,
}),
);

clerkForProductionInstance = new Clerk(productionPublishableKey);
await clerkForProductionInstance.load({
routerPush: mockNavigate,
});

clerkForDevelopmentInstance = new Clerk(developmentPublishableKey);
await clerkForDevelopmentInstance.load({
routerPush: mockNavigate,
});
});
it('redirects to the provided url with __clerk_db_jwt in the url', async () => {
await clerkForProductionInstance.redirectWithAuth('https://app.example.com');
expect(mockHref).toHaveBeenNthCalledWith(1, 'https://app.example.com/');

it('redirects to the provided url with __clerk_db_jwt in the url', async () => {
await clerkForProductionInstance.redirectWithAuth('https://app.example.com');
expect(mockHref).toHaveBeenNthCalledWith(1, 'https://app.example.com/');

await clerkForDevelopmentInstance.redirectWithAuth('https://app.example.com');
expect(mockHref).toHaveBeenNthCalledWith(2, 'https://app.example.com/#__clerk_db_jwt[deadbeef]');
});
await clerkForDevelopmentInstance.redirectWithAuth('https://app.example.com');
expect(mockHref).toHaveBeenNthCalledWith(2, 'https://app.example.com/#__clerk_db_jwt[deadbeef]');
});
});
});
25 changes: 2 additions & 23 deletions packages/clerk-js/src/core/clerk.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,13 @@ jest.mock('./resources/Client');
jest.mock('./resources/Environment');

// Because Jest, don't ask me why...
jest.mock('./devBrowserHandler', () => ({
createDevBrowserHandler: () => ({
jest.mock('./devBrowser', () => ({
createDevBrowser: () => ({
clear: jest.fn(),
setup: jest.fn(),
getDevBrowserJWT: jest.fn(() => 'deadbeef'),
setDevBrowserJWT: jest.fn(),
removeDevBrowserJWT: jest.fn(),
usesUrlBasedSessionSync: mockUsesUrlBasedSessionSync,
}),
}));

Expand Down Expand Up @@ -1859,26 +1858,6 @@ describe('Clerk singleton', () => {
expect(url).toBe('foo');
});

it('returns what was passed when not using url based session syncing', async () => {
mockUsesUrlBasedSessionSync.mockReturnValue(false);
mockEnvironmentFetch.mockReturnValue(
Promise.resolve({
authConfig: {},
userSettings: mockUserSettings,
displayConfig: mockDisplayConfig,
isSingleSession: () => false,
isProduction: () => false,
isDevelopmentOrStaging: () => true,
}),
);

const sut = new Clerk(developmentPublishableKey);
await sut.load();

const url = sut.buildUrlWithAuth('foo');
expect(url).toBe('foo');
});

it('uses the hash to propagate the dev_browser JWT by default on dev', async () => {
mockUsesUrlBasedSessionSync.mockReturnValue(true);
const sut = new Clerk(developmentPublishableKey);
Expand Down
26 changes: 12 additions & 14 deletions packages/clerk-js/src/core/clerk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ import {
completeSignUpFlow,
createAllowedRedirectOrigins,
createBeforeUnloadTracker,
createCookieHandler,
createPageLifecycle,
errorThrower,
getClerkQueryParam,
Expand All @@ -86,10 +85,11 @@ import {
toURL,
windowNavigate,
} from '../utils';
import { getClientUatCookie } from '../utils/cookies/clientUat';
import { memoizeListenerCallback } from '../utils/memoizeStateListenerCallback';
import { CLERK_SATELLITE_URL, CLERK_SYNCED, ERROR_CODES } from './constants';
import type { DevBrowserHandler } from './devBrowserHandler';
import { createDevBrowserHandler } from './devBrowserHandler';
import type { DevBrowser } from './devBrowser';
import { createDevBrowser } from './devBrowser';
import {
clerkErrorInitFailed,
clerkInvalidSignInUrlFormat,
Expand Down Expand Up @@ -160,7 +160,7 @@ export class Clerk implements ClerkInterface {
#authService: SessionCookieService | null = null;
#broadcastChannel: LocalStorageBroadcastChannel<ClerkCoreBroadcastChannelEvent> | null = null;
#componentControls?: ReturnType<MountComponentRenderer> | null;
#devBrowserHandler: DevBrowserHandler | null = null;
#devBrowser: DevBrowser | null = null;
#environment?: EnvironmentResource | null;
//@ts-expect-error with being undefined even though it's not possible - related to issue with ts and error thrower
#fapiClient: FapiClient;
Expand Down Expand Up @@ -703,7 +703,7 @@ export class Clerk implements ClerkInterface {
};

public buildUrlWithAuth(to: string, options?: BuildUrlWithAuthParams): string {
if (this.#instanceType === 'production' || !this.#devBrowserHandler?.usesUrlBasedSessionSync()) {
if (this.#instanceType === 'production') {
return to;
}

Expand All @@ -713,7 +713,7 @@ export class Clerk implements ClerkInterface {
return toURL.href;
}

const devBrowserJwt = this.#devBrowserHandler?.getDevBrowserJWT();
const devBrowserJwt = this.#devBrowser?.getDevBrowserJWT();
if (!devBrowserJwt) {
return clerkMissingDevBrowserJwt();
}
Expand Down Expand Up @@ -1244,8 +1244,7 @@ export class Clerk implements ClerkInterface {
return false;
}

const cookieHandler = createCookieHandler();
return cookieHandler.getClientUatCookie() <= 0;
return getClientUatCookie() <= 0;
};

#shouldRedirectToSatellite = (): boolean => {
Expand Down Expand Up @@ -1306,7 +1305,7 @@ export class Clerk implements ClerkInterface {
* At this point the devBrowser is not yet setup, but its API is ready for use
* Multi-domain SSO needs this handler to be initiated
*/
this.#devBrowserHandler = createDevBrowserHandler({
this.#devBrowser = createDevBrowser({
frontendApi: this.frontendApi,
fapiClient: this.#fapiClient,
});
Expand All @@ -1330,9 +1329,9 @@ export class Clerk implements ClerkInterface {
* For multi-domain we want to avoid retrieving a fresh JWT from FAPI, and we need to get the token as a result of multi-domain session syncing.
*/
if (this.#instanceType === 'production') {
await this.#devBrowserHandler.clear();
await this.#devBrowser.clear();
} else {
await this.#devBrowserHandler.setup();
await this.#devBrowser.setup();
}

/**
Expand Down Expand Up @@ -1387,9 +1386,8 @@ export class Clerk implements ClerkInterface {
break;
} catch (err) {
if (isError(err, 'dev_browser_unauthenticated')) {
// Purge and try setup again
await this.#devBrowserHandler.clear();
await this.#devBrowserHandler.setup();
await this.#devBrowser.clear();
await this.#devBrowser.setup();
} else if (!isValidBrowserOnline()) {
console.warn(err);
return false;
Expand Down
105 changes: 105 additions & 0 deletions packages/clerk-js/src/core/devBrowser.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import { DEV_BROWSER_JWT_HEADER, getDevBrowserJWTFromURL, setDevBrowserJWTInURL } from '@clerk/shared/devBrowser';

import { isDevOrStagingUrl } from '../utils';
import { getDevBrowserCookie, removeDevBrowserCookie, setDevBrowserCookie } from '../utils/cookies/devBrowser';
import { clerkErrorDevInitFailed } from './errors';
import type { FapiClient } from './fapiClient';

export interface DevBrowser {
clear(): void;

setup(): Promise<void>;

getDevBrowserJWT(): string | undefined;

setDevBrowserJWT(jwt: string): void;

removeDevBrowserJWT(): void;
}

export type CreateDevBrowserOptions = {
frontendApi: string;
fapiClient: FapiClient;
};

export function createDevBrowser({ frontendApi, fapiClient }: CreateDevBrowserOptions): DevBrowser {
function getDevBrowserJWT() {
return getDevBrowserCookie();
}

function setDevBrowserJWT(jwt: string) {
setDevBrowserCookie(jwt);
}

function removeDevBrowserJWT() {
removeDevBrowserCookie();
}

function clear() {
removeDevBrowserJWT();
}

async function setup(): Promise<void> {
if (!isDevOrStagingUrl(frontendApi)) {
return;
}

// 1. Set network interceptors to Pass dev
fapiClient.onBeforeRequest(request => {
const devBrowserJWT = getDevBrowserJWT();
if (devBrowserJWT && request?.url) {
request.url = setDevBrowserJWTInURL(request.url, devBrowserJWT, true);
}
});

fapiClient.onAfterResponse((_, response) => {
const newDevBrowserJWT = response?.headers?.get(DEV_BROWSER_JWT_HEADER);
if (newDevBrowserJWT) {
setDevBrowserJWT(newDevBrowserJWT);
}
});

// 1. If a cookie already exists, it might have SameSite=Strict. Re-set it to make sure it has SameSite=Lax
const existingDevBrowserCookie = getDevBrowserCookie();
if (existingDevBrowserCookie) {
removeDevBrowserCookie();
setDevBrowserCookie(existingDevBrowserCookie);
}

// 2. Get the JWT from hash or search parameters when the redirection comes from AP
const devBrowserToken = getDevBrowserJWTFromURL(new URL(window.location.href));
if (devBrowserToken) {
setDevBrowserJWT(devBrowserToken);
return;
}

// 3. If no JWT is found in the first step, check if a JWT is already available in the JS cookie
if (getDevBrowserCookie()) {
return;
}

// 4. Otherwise, fetch a new DevBrowser JWT from FAPI and cache it
const createDevBrowserUrl = fapiClient.buildUrl({
path: '/dev_browser',
});

const resp = await fetch(createDevBrowserUrl.toString(), {
method: 'POST',
});

if (!resp.ok) {
clerkErrorDevInitFailed();
}

const data = await resp.json();
setDevBrowserJWT(data?.token);
}

return {
clear,
setup,
getDevBrowserJWT,
setDevBrowserJWT,
removeDevBrowserJWT,
};
}
Loading

0 comments on commit 7e76b41

Please sign in to comment.