From 37f04786929be6d30cd212e6e48bd3a38b69376e Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 16 Aug 2024 18:34:51 -0400 Subject: [PATCH 01/39] WIP - first rough pass at args-based approach --- packages/backend/src/tokens/authStatus.ts | 1 + packages/backend/src/tokens/request.ts | 101 ++++++++++++++++++++-- packages/backend/src/tokens/types.ts | 9 ++ 3 files changed, 106 insertions(+), 5 deletions(-) diff --git a/packages/backend/src/tokens/authStatus.ts b/packages/backend/src/tokens/authStatus.ts index 78a7de9c129..19e373a7299 100644 --- a/packages/backend/src/tokens/authStatus.ts +++ b/packages/backend/src/tokens/authStatus.ts @@ -66,6 +66,7 @@ export const AuthErrorReason = { SessionTokenMissing: 'session-token-missing', SessionTokenOutdated: 'session-token-outdated', SessionTokenWithoutClientUAT: 'session-token-but-no-client-uat', + ActiveOrganizationMismatch: 'active-organization-mismatch', UnexpectedError: 'unexpected-error', } as const; diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index 09078f39797..464ea228abf 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -11,8 +11,11 @@ import { AuthErrorReason, handshake, signedIn, signedOut } from './authStatus'; import { createClerkRequest } from './clerkRequest'; import { getCookieName, getCookieValue } from './cookie'; import { verifyHandshakeToken } from './handshake'; -import type { AuthenticateRequestOptions } from './types'; +import type { AuthenticateRequestOptions, OrganizationSyncOptions } from './types'; import { verifyToken } from './verify'; +import { ClerkUrl } from './clerkUrl'; +import { Clerk } from '@clerk/types'; +import { match } from 'path-to-regexp'; function assertSignInUrlExists(signInUrl: string | undefined, key: string): asserts signInUrl is string { if (!signInUrl && isDevelopmentFromSecretKey(key)) { @@ -84,7 +87,7 @@ export async function authenticateRequest( return updatedURL; } - function buildRedirectToHandshake() { + function buildRedirectToHandshake(requestURL: URL) { const redirectUrl = removeDevBrowserFromURL(authenticateContext.clerkUrl); const frontendApiNoProtocol = authenticateContext.frontendApi.replace(/http(s)?:\/\//, ''); @@ -96,6 +99,16 @@ export async function authenticateRequest( url.searchParams.append(constants.QueryParameters.DevBrowser, authenticateContext.devBrowserToken); } + if (options.organizationSync) { + const toActivate = getActivationEntity(requestURL, options.organizationSync); + if (toActivate) { + const params = getActivationParam(toActivate); + Object.entries(params).forEach(([key, value]) => { + url.searchParams.append(key, value); + }); + } + } + return new Headers({ location: url.href }); } @@ -173,7 +186,7 @@ ${error.getFullMessage()}`, if (isRequestEligibleForHandshake(authenticateContext)) { // Right now the only usage of passing in different headers is for multi-domain sync, which redirects somewhere else. // In the future if we want to decorate the handshake redirect with additional headers per call we need to tweak this logic. - const handshakeHeaders = headers ?? buildRedirectToHandshake(); + const handshakeHeaders = headers ?? buildRedirectToHandshake(authenticateContext.clerkUrl); // Introduce the mechanism to protect for infinite handshake redirect loops // using a cookie and returning true if it's infinite redirect loop or false if we can // proceed with triggering handshake. @@ -381,10 +394,36 @@ ${error.getFullMessage()}`, return signedOut(authenticateContext, AuthErrorReason.UnexpectedError); } + + let requestState = null; + if (authenticateContext.sessionTokenInHeader) { - return authenticateRequestWithTokenInHeader(); + requestState = await authenticateRequestWithTokenInHeader(); } - return authenticateRequestWithTokenInCookie(); + else { + requestState = await authenticateRequestWithTokenInCookie(); + } + + if (options.organizationSync) { + const toActivate = getActivationEntity(authenticateContext.clerkUrl, options.organizationSync); + if (toActivate) { + const auth = requestState.toAuth() + if (auth) { + let mustActivate = false; + if (toActivate.organizationSlug && auth.orgSlug && toActivate.organizationSlug !== auth.orgSlug) { + mustActivate = true; + } + if (toActivate.organizationId && auth.orgId && toActivate.organizationId !== auth.orgId) { + mustActivate = true; + } + if (mustActivate) { + return handleMaybeHandshakeStatus(authenticateContext, AuthErrorReason.ActiveOrganizationMismatch, ''); + } + } + + } + } + return requestState } /** @@ -394,3 +433,55 @@ export const debugRequestState = (params: RequestState) => { const { isSignedIn, proxyUrl, reason, message, publishableKey, isSatellite, domain } = params; return { isSignedIn, proxyUrl, reason, message, publishableKey, isSatellite, domain }; }; + + + +function getActivationEntity(url: URL, options: OrganizationSyncOptions): ActivatibleEntity | null { + // Check for personal workspace activation + if (options.personalWorkspacePattern) { + const personalResult = match(options.personalWorkspacePattern)(url.pathname); + if (personalResult) { + return { type: 'personalWorkspace' }; + } + } + + // Check for organization activation + if (options.organizationPattern) { + const orgResult = match(options.organizationPattern)(url.pathname); + if (orgResult) { + const { slug = '', id = '' } = orgResult.params as { slug?: string; id?: string }; + if (id) { + return { type: 'organization', organizationId: id }; + } + if (slug) { + return { type: 'organization', organizationSlug: slug }; + } + } + } + return null; +} + +type ActivatibleEntity = { + type: 'personalWorkspace' | 'organization' | 'none'; + organizationId?: string; + organizationSlug?: string; +}; + +// TODO(izaak): Find a better spot for this? +function getActivationParam(toActivate: ActivatibleEntity): Map { + const ret = new Map(); + if (toActivate.type === 'personalWorkspace') { + ret.set('type', 'personalWorkspace'); + } + if (toActivate.type === 'organization') { + if (toActivate.organizationId) { + ret.set('type', 'organization'); + ret.set('organizationId', toActivate.organizationId); + } + if (toActivate.organizationSlug) { + ret.set('type', 'organization'); + ret.set('organizationSlug', toActivate.organizationSlug); + } + } + return ret; +} diff --git a/packages/backend/src/tokens/types.ts b/packages/backend/src/tokens/types.ts index 63be1784103..86d9020d357 100644 --- a/packages/backend/src/tokens/types.ts +++ b/packages/backend/src/tokens/types.ts @@ -9,4 +9,13 @@ export type AuthenticateRequestOptions = { signUpUrl?: string; afterSignInUrl?: string; afterSignUpUrl?: string; + organizationSync?: OrganizationSyncOptions; } & VerifyTokenOptions; + +// OrganizationSyncOptions define the options for syncing an organization +// or personal workspace state from the URL to the clerk session +export type OrganizationSyncOptions = { + organizationPattern: string, + personalWorkspacePattern: string, + invalidOrganizationRedirectURL: string +} From 7c3f936035f43c3bc148424787fbe9e37cbecbd7 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Wed, 28 Aug 2024 14:54:13 -0400 Subject: [PATCH 02/39] Tested end-to-end with corresponding fapi changes locally --- packages/backend/src/internal.ts | 2 +- .../backend/src/tokens/authenticateContext.ts | 1 + packages/backend/src/tokens/request.ts | 78 +++++++++---------- packages/backend/src/tokens/types.ts | 11 ++- packages/nextjs/src/server/clerkMiddleware.ts | 32 +++++++- 5 files changed, 77 insertions(+), 47 deletions(-) diff --git a/packages/backend/src/internal.ts b/packages/backend/src/internal.ts index 11978dae045..af12ba0655a 100644 --- a/packages/backend/src/internal.ts +++ b/packages/backend/src/internal.ts @@ -7,7 +7,7 @@ export { createAuthenticateRequest } from './tokens/factory'; export { debugRequestState } from './tokens/request'; -export type { AuthenticateRequestOptions } from './tokens/types'; +export type { AuthenticateRequestOptions, OrganizationSyncOptions } from './tokens/types'; export type { SignedInAuthObjectOptions, SignedInAuthObject, SignedOutAuthObject } from './tokens/authObjects'; export { makeAuthObjectSerializable, signedOutAuthObject, signedInAuthObject } from './tokens/authObjects'; diff --git a/packages/backend/src/tokens/authenticateContext.ts b/packages/backend/src/tokens/authenticateContext.ts index cbf2057ff24..c3345580b1f 100644 --- a/packages/backend/src/tokens/authenticateContext.ts +++ b/packages/backend/src/tokens/authenticateContext.ts @@ -77,6 +77,7 @@ class AuthenticateContext { domain: options.domain, }); this.instanceType = pk.instanceType; + this.frontendApi = pk.frontendApi; } diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index 464ea228abf..380e4a27f22 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -1,3 +1,5 @@ +import { match } from 'path-to-regexp'; + import { constants } from '../constants'; import type { TokenCarrier } from '../errors'; import { TokenVerificationError, TokenVerificationErrorReason } from '../errors'; @@ -13,9 +15,6 @@ import { getCookieName, getCookieValue } from './cookie'; import { verifyHandshakeToken } from './handshake'; import type { AuthenticateRequestOptions, OrganizationSyncOptions } from './types'; import { verifyToken } from './verify'; -import { ClerkUrl } from './clerkUrl'; -import { Clerk } from '@clerk/types'; -import { match } from 'path-to-regexp'; function assertSignInUrlExists(signInUrl: string | undefined, key: string): asserts signInUrl is string { if (!signInUrl && isDevelopmentFromSecretKey(key)) { @@ -103,7 +102,8 @@ export async function authenticateRequest( const toActivate = getActivationEntity(requestURL, options.organizationSync); if (toActivate) { const params = getActivationParam(toActivate); - Object.entries(params).forEach(([key, value]) => { + + params.forEach((value, key) => { url.searchParams.append(key, value); }); } @@ -364,7 +364,33 @@ ${error.getFullMessage()}`, throw errors[0]; } // use `await` to force this try/catch handle the signedIn invocation - return await signedIn(authenticateContext, data, undefined, authenticateContext.sessionTokenInCookie!); + const requestState = await signedIn( + authenticateContext, + data, + undefined, + authenticateContext.sessionTokenInCookie!, + ); + + // Org sync if necessary + // TODO(izaak): Also apply for auth in header scenario? Likely move to a standalone helper. + if (options.organizationSync) { + const toActivate = getActivationEntity(authenticateContext.clerkUrl, options.organizationSync); + if (toActivate) { + const auth = requestState.toAuth(); + if (auth) { + let mustActivate = false; + if (toActivate.organizationSlug && auth.orgSlug && toActivate.organizationSlug !== auth.orgSlug) { + mustActivate = true; + } + if (toActivate.organizationId && auth.orgId && toActivate.organizationId !== auth.orgId) { + mustActivate = true; + } + if (mustActivate) { + return handleMaybeHandshakeStatus(authenticateContext, AuthErrorReason.ActiveOrganizationMismatch, ''); + } + } + } + } } catch (err) { return handleError(err, 'cookie'); } @@ -390,40 +416,13 @@ ${error.getFullMessage()}`, } return signedOut(authenticateContext, err.reason, err.getFullMessage()); } - return signedOut(authenticateContext, AuthErrorReason.UnexpectedError); } - - let requestState = null; - if (authenticateContext.sessionTokenInHeader) { - requestState = await authenticateRequestWithTokenInHeader(); + return authenticateRequestWithTokenInHeader(); } - else { - requestState = await authenticateRequestWithTokenInCookie(); - } - - if (options.organizationSync) { - const toActivate = getActivationEntity(authenticateContext.clerkUrl, options.organizationSync); - if (toActivate) { - const auth = requestState.toAuth() - if (auth) { - let mustActivate = false; - if (toActivate.organizationSlug && auth.orgSlug && toActivate.organizationSlug !== auth.orgSlug) { - mustActivate = true; - } - if (toActivate.organizationId && auth.orgId && toActivate.organizationId !== auth.orgId) { - mustActivate = true; - } - if (mustActivate) { - return handleMaybeHandshakeStatus(authenticateContext, AuthErrorReason.ActiveOrganizationMismatch, ''); - } - } - - } - } - return requestState + return authenticateRequestWithTokenInCookie(); } /** @@ -434,8 +433,6 @@ export const debugRequestState = (params: RequestState) => { return { isSignedIn, proxyUrl, reason, message, publishableKey, isSatellite, domain }; }; - - function getActivationEntity(url: URL, options: OrganizationSyncOptions): ActivatibleEntity | null { // Check for personal workspace activation if (options.personalWorkspacePattern) { @@ -456,6 +453,7 @@ function getActivationEntity(url: URL, options: OrganizationSyncOptions): Activa if (slug) { return { type: 'organization', organizationSlug: slug }; } + // TODO(izaak): Consider console warning if there's a pattern given (e.g. :orgSlug) that isn't :id or :slug } } return null; @@ -471,16 +469,14 @@ type ActivatibleEntity = { function getActivationParam(toActivate: ActivatibleEntity): Map { const ret = new Map(); if (toActivate.type === 'personalWorkspace') { - ret.set('type', 'personalWorkspace'); + ret.set('organization_id', ''); } if (toActivate.type === 'organization') { if (toActivate.organizationId) { - ret.set('type', 'organization'); - ret.set('organizationId', toActivate.organizationId); + ret.set('organization_id', toActivate.organizationId); } if (toActivate.organizationSlug) { - ret.set('type', 'organization'); - ret.set('organizationSlug', toActivate.organizationSlug); + ret.set('organization_id', toActivate.organizationSlug); } } return ret; diff --git a/packages/backend/src/tokens/types.ts b/packages/backend/src/tokens/types.ts index 86d9020d357..f1d030b9bec 100644 --- a/packages/backend/src/tokens/types.ts +++ b/packages/backend/src/tokens/types.ts @@ -15,7 +15,12 @@ export type AuthenticateRequestOptions = { // OrganizationSyncOptions define the options for syncing an organization // or personal workspace state from the URL to the clerk session export type OrganizationSyncOptions = { - organizationPattern: string, - personalWorkspacePattern: string, - invalidOrganizationRedirectURL: string + // Must have a path component named either ":id" or ":slug". + // Example: "/orgs/:slug" + organizationPattern: Pattern, + personalWorkspacePattern: Pattern, } + +// Pattern is a URL Pattern API style matcher +// Examples: "/orgs/:slug", "/orgs/:id" +type Pattern = string; diff --git a/packages/nextjs/src/server/clerkMiddleware.ts b/packages/nextjs/src/server/clerkMiddleware.ts index 10ce52b6a32..2e5d2e62567 100644 --- a/packages/nextjs/src/server/clerkMiddleware.ts +++ b/packages/nextjs/src/server/clerkMiddleware.ts @@ -1,7 +1,7 @@ import { AsyncLocalStorage } from 'node:async_hooks'; import type { AuthObject } from '@clerk/backend'; -import type { AuthenticateRequestOptions, ClerkRequest, RedirectFun, RequestState } from '@clerk/backend/internal'; +import type { AuthenticateRequestOptions, OrganizationSyncOptions, ClerkRequest, RedirectFun, RequestState} from '@clerk/backend/internal'; import { AuthStatus, constants, createClerkRequest, createRedirect } from '@clerk/backend/internal'; import { eventMethodCalled } from '@clerk/shared/telemetry'; import type { NextMiddleware } from 'next/server'; @@ -187,7 +187,35 @@ const parseHandlerAndOptions = (args: unknown[]) => { ] as [ClerkMiddlewareHandler | undefined, ClerkMiddlewareOptions]; }; -export const createAuthenticateRequestOptions = (clerkRequest: ClerkRequest, options: ClerkMiddlewareOptions) => { +export const createAuthenticateRequestOptions: (clerkRequest: ClerkRequest, options: ClerkMiddlewareOptions) => { + /* TODO(izaak): I get this error unless I specify the type explicitly: + ``` + error TS2742: The inferred type of 'createAuthenticateRequestOptions' cannot be named without a reference to + '../../../../node_modules/@clerk/backend/dist/tokens/types'. This is likely not portable. A type annotation is necessary. + ``` + It looks like it's because i've added OrganizationSyncOptions to AuthenticateRequestOptions (types.ts). Not sure + what the best workaround is, but it's likely not this. + */ + publishableKey?: string; + audience?: string | string[]; + debug?: boolean; + secretKey?: string; + signInUrl: string; + signUpUrl?: string; + proxyUrl: any; + afterSignUpUrl?: string; + clockSkewInMs?: number; + isSatellite: boolean; + organizationSync?: OrganizationSyncOptions; + apiVersion?: string; + apiUrl?: string; + domain: string; + afterSignInUrl?: string; + authorizedParties?: string[]; + skipJwksCache?: boolean; + jwksCacheTtlInMs?: number; + jwtKey?: string +} = (clerkRequest: ClerkRequest, options: ClerkMiddlewareOptions) => { return { ...options, ...handleMultiDomainAndProxy(clerkRequest, options), From 54e67521f4cdabc75b6d8607b6be87e3fd98e967 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 6 Sep 2024 18:03:17 -0400 Subject: [PATCH 03/39] Starting on integration tests --- .../src/app/organizations-by-id/[id]/page.tsx | 17 ++++ .../app/organizations-by-slug/[slug]/page.tsx | 17 ++++ integration/testUtils/handshake.ts | 14 ++- integration/tests/handshake.test.ts | 98 +++++++++++++++++++ 4 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 integration/templates/next-app-router/src/app/organizations-by-id/[id]/page.tsx create mode 100644 integration/templates/next-app-router/src/app/organizations-by-slug/[slug]/page.tsx diff --git a/integration/templates/next-app-router/src/app/organizations-by-id/[id]/page.tsx b/integration/templates/next-app-router/src/app/organizations-by-id/[id]/page.tsx new file mode 100644 index 00000000000..231f92b42e0 --- /dev/null +++ b/integration/templates/next-app-router/src/app/organizations-by-id/[id]/page.tsx @@ -0,0 +1,17 @@ +import { auth } from '@clerk/nextjs/server'; + +export default function Home({ params }): {} { + const { orgId } = auth(); + + if (params.id != orgId) { + console.log('Mismatch - returning nothing for now...', params.slug, orgId); + } + + console.log("I'm the server and I got this slug: ", orgId); + + return ( + <> +

From auth(), I know your org slug is: {orgId}

+ + ); +} diff --git a/integration/templates/next-app-router/src/app/organizations-by-slug/[slug]/page.tsx b/integration/templates/next-app-router/src/app/organizations-by-slug/[slug]/page.tsx new file mode 100644 index 00000000000..ae65a381209 --- /dev/null +++ b/integration/templates/next-app-router/src/app/organizations-by-slug/[slug]/page.tsx @@ -0,0 +1,17 @@ +import { auth } from '@clerk/nextjs/server'; + +export default function Home({ params }): {} { + const { orgSlug } = auth(); + + if (params.slug != orgSlug) { + console.log('Mismatch - returning nothing for now...', params.slug, orgSlug); + } + + console.log("I'm the server and I got this slug: ", orgSlug); + + return ( + <> +

From auth(), I know your org slug is: {orgSlug}

+ + ); +} diff --git a/integration/testUtils/handshake.ts b/integration/testUtils/handshake.ts index 63c4fbf2d64..e6dbdbc8b33 100644 --- a/integration/testUtils/handshake.ts +++ b/integration/testUtils/handshake.ts @@ -104,7 +104,13 @@ export function generateConfig({ mode, matchedKeys = true }: { mode: 'test' | 'l exp: number; nbf: number; }; - const generateToken = ({ state }: { state: 'active' | 'expired' | 'early' }) => { + const generateToken = ({ + state, + extraClaims, + }: { + state: 'active' | 'expired' | 'early'; + extraClaims?: Map; + }) => { const claims = { sub: 'user_12345' } as Claims; const now = Math.floor(Date.now() / 1000); @@ -121,6 +127,12 @@ export function generateConfig({ mode, matchedKeys = true }: { mode: 'test' | 'l claims.nbf = now - 10 + 600; claims.exp = now + 60 + 600; } + + // Merge claims with extraClaims + for (const [key, value] of extraClaims) { + claims[key] = value; + } + return { token: jwt.sign(claims, rsa.private, { algorithm: 'RS256', diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index ffc2b638196..6afabbd1a2e 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -885,3 +885,101 @@ test.describe('Client handshake @generic', () => { expect(res.status).toBe(200); }); }); + +test.describe('Client handshake with organization activation (by ID) @nextjs', () => { + test.describe.configure({ mode: 'serial' }); + + let app: Application; + const jwksServer = http.createServer(function (req, res) { + const sk = req.headers.authorization?.replace('Bearer ', ''); + if (!sk) { + console.log('No SK to', req.url, req.headers); + } + + res.setHeader('Content-Type', 'application/json'); + res.write(JSON.stringify(getJwksFromSecretKey(sk))); + res.end(); + }); + // Strip trailing slash + const devBrowserCookie = '__clerk_db_jwt=needstobeset;'; + const devBrowserQuery = '&__clerk_db_jwt=needstobeset'; + + test.beforeAll('setup local Clerk API mock', async () => { + const env = appConfigs.envs.withEmailCodes + .clone() + .setEnvVariable('private', 'CLERK_API_URL', `http://localhost:${PORT}`); + + // Start the jwks server + await new Promise(resolve => jwksServer.listen(4199, resolve)); + + app = await appConfigs.next.appRouter + .clone() + .addFile( + 'src/middleware.ts', + () => `import { authMiddleware } from '@clerk/nextjs/server'; + // Set the paths that don't require the user to be signed in + const publicPaths = ['/', /^(\\/(sign-in|sign-up|app-dir|custom)\\/*).*$/]; + export const middleware = (req, evt) => { + return authMiddleware({ + publicRoutes: publicPaths, + publishableKey: req.headers.get("x-publishable-key"), + secretKey: req.headers.get("x-secret-key"), + proxyUrl: req.headers.get("x-proxy-url"), + domain: req.headers.get("x-domain"), + isSatellite: req.headers.get('x-satellite') === 'true', + signInUrl: req.headers.get("x-sign-in-url"), + organizationSync: { // <--- CRITICAL + organizationPattern: "/organizations-by-id/:id", + } + })(req, evt) + }; + export const config = { + matcher: ['/((?!.+\\.[\\w]+$|_next).*)', '/', '/(api|trpc)(.*)'], + }; + `, + ) + .commit(); + + await app.setup(); + await app.withEnv(env); + await app.dev(); + }); + + test.afterAll(async () => { + await app.teardown(); + await new Promise(resolve => jwksServer.close(() => resolve())); + }); + + test('expired session token - with organization mismatch - dev', async () => { + + const config = generateConfig({ + mode: 'test', + }); + // Create a new map with an org_id key + const { token, claims } = config.generateToken({ + state: 'expired', + extraClaims: new Map([ + // Start the test with org A active + ['org_id', 'org_a'] + ]), + }); + const clientUat = claims.iat; + const res = await fetch(app.serverUrl + + '/organizations-by-id/org_b', // But attempt to visit org B + { + headers: new Headers({ + Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, + 'X-Publishable-Key': config.pk, + 'X-Secret-Key': config.sk, + 'Sec-Fetch-Dest': 'document', + }), + redirect: 'manual', + }); + expect(res.status).toBe(307); + expect(res.headers.get('location')).toBe( + `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( + `${app.serverUrl}/organizations-by-id/org_b`, // Redirects to org_b's path (normal) + )}&suffixed_cookies=false${devBrowserQuery}&organization_id=org_b`, // Should attempt to activate org B in the redirect + ); + }); +}); From d7246956c9bcc50c4f30cb0bb330b5f61a39f8ba Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 6 Sep 2024 19:04:10 -0400 Subject: [PATCH 04/39] Two more tests --- .../src/app/personal-workspace/page.tsx | 15 ++++ integration/tests/handshake.test.ts | 70 ++++++++++++++++++- 2 files changed, 82 insertions(+), 3 deletions(-) create mode 100644 integration/templates/next-app-router/src/app/personal-workspace/page.tsx diff --git a/integration/templates/next-app-router/src/app/personal-workspace/page.tsx b/integration/templates/next-app-router/src/app/personal-workspace/page.tsx new file mode 100644 index 00000000000..a9677855ccd --- /dev/null +++ b/integration/templates/next-app-router/src/app/personal-workspace/page.tsx @@ -0,0 +1,15 @@ +import { auth } from '@clerk/nextjs/server'; + +export default function Home(): {} { + const { orgId } = auth(); + + if (orgId != null) { + console.log('Oh no, this page should only activate on the personal workspace!'); + } + + return ( + <> +

Welcome to your personal workspace

+ + ); +} diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index 6afabbd1a2e..b6ca832f5aa 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -930,6 +930,7 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( signInUrl: req.headers.get("x-sign-in-url"), organizationSync: { // <--- CRITICAL organizationPattern: "/organizations-by-id/:id", + personalWorkspacePattern: "/personal-workspace", } })(req, evt) }; @@ -950,14 +951,13 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( await new Promise(resolve => jwksServer.close(() => resolve())); }); - test('expired session token - with organization mismatch - dev', async () => { - + test('expired session token - with organization id mismatch - dev', async () => { const config = generateConfig({ mode: 'test', }); // Create a new map with an org_id key const { token, claims } = config.generateToken({ - state: 'expired', + state: 'expired', // <-- Critical extraClaims: new Map([ // Start the test with org A active ['org_id', 'org_a'] @@ -982,4 +982,68 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( )}&suffixed_cookies=false${devBrowserQuery}&organization_id=org_b`, // Should attempt to activate org B in the redirect ); }); + + test('valid session token - with organization id mismatch - dev', async () => { + const config = generateConfig({ + mode: 'test', + }); + // Create a new map with an org_id key + const { token, claims } = config.generateToken({ + state: 'active', // <-- Critical + extraClaims: new Map([ + // Start the test with org A active + ['org_id', 'org_a'] + ]), + }); + const clientUat = claims.iat; + const res = await fetch(app.serverUrl + + '/organizations-by-id/org_b', // But attempt to visit org B + { + headers: new Headers({ + Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, + 'X-Publishable-Key': config.pk, + 'X-Secret-Key': config.sk, + 'Sec-Fetch-Dest': 'document', + }), + redirect: 'manual', + }); + expect(res.status).toBe(307); + expect(res.headers.get('location')).toBe( + `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( + `${app.serverUrl}/organizations-by-id/org_b`, // Redirects to org_b's path (normal) + )}&suffixed_cookies=false${devBrowserQuery}&organization_id=org_b`, // Should attempt to activate org B in the redirect + ); + }); + + test('expired session token - with personal workspace mismatch - dev', async () => { + const config = generateConfig({ + mode: 'test', + }); + // Create a new map with an org_id key + const { token, claims } = config.generateToken({ + state: 'expired', // <-- Critical + extraClaims: new Map([ + // Start the test with org A active + ['org_id', 'org_a'] + ]), + }); + const clientUat = claims.iat; + const res = await fetch(app.serverUrl + + '/personal-workspace', // But attempt to visit the personal workspace + { + headers: new Headers({ + Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, + 'X-Publishable-Key': config.pk, + 'X-Secret-Key': config.sk, + 'Sec-Fetch-Dest': 'document', + }), + redirect: 'manual', + }); + expect(res.status).toBe(307); + expect(res.headers.get('location')).toBe( + `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( + `${app.serverUrl}/personal-workspace`, // Redirects to the personal workspace (normal) + )}&suffixed_cookies=false${devBrowserQuery}&organization_id=`, // Should attempt to activate the personal workspace with a blank query param + ); + }); }); From 925be56c3f00b6a56b3de46505520dbbc45d648a Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Mon, 9 Sep 2024 09:47:59 -0400 Subject: [PATCH 05/39] First pass at table-driven tests --- integration/tests/handshake.test.ts | 282 +++++++++++++++++++--------- 1 file changed, 192 insertions(+), 90 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index b6ca832f5aa..d035322030a 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -928,10 +928,13 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( domain: req.headers.get("x-domain"), isSatellite: req.headers.get('x-satellite') === 'true', signInUrl: req.headers.get("x-sign-in-url"), - organizationSync: { // <--- CRITICAL + + // Critical + organizationSync: { organizationPattern: "/organizations-by-id/:id", personalWorkspacePattern: "/personal-workspace", } + })(req, evt) }; export const config = { @@ -951,99 +954,198 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( await new Promise(resolve => jwksServer.close(() => resolve())); }); - test('expired session token - with organization id mismatch - dev', async () => { - const config = generateConfig({ - mode: 'test', - }); - // Create a new map with an org_id key - const { token, claims } = config.generateToken({ - state: 'expired', // <-- Critical - extraClaims: new Map([ - // Start the test with org A active + type testCase = { + name: string; + state: 'active' | 'expired' | 'early'; + appRequestPath: string; + initialClaims: Map; + fapiOrganizationIdParamValue: string; + } + + const testCases: testCase[] = [ + { + name: 'When org A is active but org B is requested, attempts to activate org B (expired)', + state: 'expired', + appRequestPath: '/organizations-by-id/org_b', + initialClaims: new Map([ ['org_id', 'org_a'] ]), - }); - const clientUat = claims.iat; - const res = await fetch(app.serverUrl + - '/organizations-by-id/org_b', // But attempt to visit org B - { - headers: new Headers({ - Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, - 'X-Publishable-Key': config.pk, - 'X-Secret-Key': config.sk, - 'Sec-Fetch-Dest': 'document', - }), - redirect: 'manual', - }); - expect(res.status).toBe(307); - expect(res.headers.get('location')).toBe( - `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( - `${app.serverUrl}/organizations-by-id/org_b`, // Redirects to org_b's path (normal) - )}&suffixed_cookies=false${devBrowserQuery}&organization_id=org_b`, // Should attempt to activate org B in the redirect - ); - }); - - test('valid session token - with organization id mismatch - dev', async () => { - const config = generateConfig({ - mode: 'test', - }); - // Create a new map with an org_id key - const { token, claims } = config.generateToken({ - state: 'active', // <-- Critical - extraClaims: new Map([ - // Start the test with org A active + fapiOrganizationIdParamValue: 'org_b', + }, + { + name: 'When org A is active but org B is requested, attempts to activate org B (active)', + state: 'active', + appRequestPath: '/organizations-by-id/org_b', + initialClaims: new Map([ ['org_id', 'org_a'] ]), - }); - const clientUat = claims.iat; - const res = await fetch(app.serverUrl + - '/organizations-by-id/org_b', // But attempt to visit org B - { - headers: new Headers({ - Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, - 'X-Publishable-Key': config.pk, - 'X-Secret-Key': config.sk, - 'Sec-Fetch-Dest': 'document', - }), - redirect: 'manual', + fapiOrganizationIdParamValue: 'org_b', + }, + ]; + + for (const testCase of testCases) { + test(`organization activation by ID - ${testCase.name} - dev`, async () => { + const config = generateConfig({ + mode: 'test', }); - expect(res.status).toBe(307); - expect(res.headers.get('location')).toBe( - `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( - `${app.serverUrl}/organizations-by-id/org_b`, // Redirects to org_b's path (normal) - )}&suffixed_cookies=false${devBrowserQuery}&organization_id=org_b`, // Should attempt to activate org B in the redirect - ); - }); - - test('expired session token - with personal workspace mismatch - dev', async () => { - const config = generateConfig({ - mode: 'test', - }); - // Create a new map with an org_id key - const { token, claims } = config.generateToken({ - state: 'expired', // <-- Critical - extraClaims: new Map([ - // Start the test with org A active - ['org_id', 'org_a'] - ]), - }); - const clientUat = claims.iat; - const res = await fetch(app.serverUrl + - '/personal-workspace', // But attempt to visit the personal workspace - { - headers: new Headers({ - Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, - 'X-Publishable-Key': config.pk, - 'X-Secret-Key': config.sk, - 'Sec-Fetch-Dest': 'document', - }), - redirect: 'manual', + // Create a new map with an org_id key + const { token, claims } = config.generateToken({ + state: testCase.state, // <-- Critical + extraClaims: new Map([ + // Start the test with org A active + ['org_id', 'org_a'] + ]), }); - expect(res.status).toBe(307); - expect(res.headers.get('location')).toBe( - `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( - `${app.serverUrl}/personal-workspace`, // Redirects to the personal workspace (normal) - )}&suffixed_cookies=false${devBrowserQuery}&organization_id=`, // Should attempt to activate the personal workspace with a blank query param - ); - }); + const clientUat = claims.iat; + const res = await fetch(app.serverUrl + + '/organizations-by-id/org_b', // But attempt to visit org B + { + headers: new Headers({ + Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, + 'X-Publishable-Key': config.pk, + 'X-Secret-Key': config.sk, + 'Sec-Fetch-Dest': 'document', + }), + redirect: 'manual', + }); + expect(res.status).toBe(307); + expect(res.headers.get('location')).toBe( + `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( + `${app.serverUrl}/organizations-by-id/org_b`, // Redirects to org_b's path (normal) + )}&suffixed_cookies=false${devBrowserQuery}&organization_id=org_b`, // Should attempt to activate org B in the redirect + ); + }); + } + + + // TODO(izaak): refactor into table-driven test? + // test('expired session token - with organization id mismatch - dev', async () => { + // const config = generateConfig({ + // mode: 'test', + // }); + // // Create a new map with an org_id key + // const { token, claims } = config.generateToken({ + // state: 'expired', // <-- Critical + // extraClaims: new Map([ + // // Start the test with org A active + // ['org_id', 'org_a'] + // ]), + // }); + // const clientUat = claims.iat; + // const res = await fetch(app.serverUrl + + // '/organizations-by-id/org_b', // But attempt to visit org B + // { + // headers: new Headers({ + // Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, + // 'X-Publishable-Key': config.pk, + // 'X-Secret-Key': config.sk, + // 'Sec-Fetch-Dest': 'document', + // }), + // redirect: 'manual', + // }); + // expect(res.status).toBe(307); + // expect(res.headers.get('location')).toBe( + // `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( + // `${app.serverUrl}/organizations-by-id/org_b`, // Redirects to org_b's path (normal) + // )}&suffixed_cookies=false${devBrowserQuery}&organization_id=org_b`, // Should attempt to activate org B in the redirect + // ); + // }); + // + // test('valid session token - with organization id mismatch - dev', async () => { + // const config = generateConfig({ + // mode: 'test', + // }); + // // Create a new map with an org_id key + // const { token, claims } = config.generateToken({ + // state: 'active', // <-- Critical + // extraClaims: new Map([ + // // Start the test with org A active + // ['org_id', 'org_a'] + // ]), + // }); + // const clientUat = claims.iat; + // const res = await fetch(app.serverUrl + + // '/organizations-by-id/org_b', // But attempt to visit org B + // { + // headers: new Headers({ + // Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, + // 'X-Publishable-Key': config.pk, + // 'X-Secret-Key': config.sk, + // 'Sec-Fetch-Dest': 'document', + // }), + // redirect: 'manual', + // }); + // expect(res.status).toBe(307); + // expect(res.headers.get('location')).toBe( + // `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( + // `${app.serverUrl}/organizations-by-id/org_b`, // Redirects to org_b's path (normal) + // )}&suffixed_cookies=false${devBrowserQuery}&organization_id=org_b`, // Should attempt to activate org B in the redirect + // ); + // }); + // + // test('expired session token - with personal workspace mismatch - dev', async () => { + // const config = generateConfig({ + // mode: 'test', + // }); + // // Create a new map with an org_id key + // const { token, claims } = config.generateToken({ + // state: 'expired', // <-- Critical + // extraClaims: new Map([ + // // Start the test with org A active + // ['org_id', 'org_a'] + // ]), + // }); + // const clientUat = claims.iat; + // const res = await fetch(app.serverUrl + + // '/personal-workspace', // But attempt to visit the personal workspace + // { + // headers: new Headers({ + // Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, + // 'X-Publishable-Key': config.pk, + // 'X-Secret-Key': config.sk, + // 'Sec-Fetch-Dest': 'document', + // }), + // redirect: 'manual', + // }); + // expect(res.status).toBe(307); + // expect(res.headers.get('location')).toBe( + // `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( + // `${app.serverUrl}/personal-workspace`, // Redirects to the personal workspace (normal) + // )}&suffixed_cookies=false${devBrowserQuery}&organization_id=`, // Should attempt to activate the personal workspace with a blank query param + // ); + // }); + // + // test('valid session token - with personal workspace mismatch - dev', async () => { + // const config = generateConfig({ + // mode: 'test', + // }); + // // Create a new map with an org_id key + // const { token, claims } = config.generateToken({ + // state: 'active', // <-- Critical + // extraClaims: new Map([ + // // Start the test with org A active + // ['org_id', 'org_a'] + // ]), + // }); + // const clientUat = claims.iat; + // const res = await fetch(app.serverUrl + + // '/personal-workspace', // But attempt to visit the personal workspace + // { + // headers: new Headers({ + // Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, + // 'X-Publishable-Key': config.pk, + // 'X-Secret-Key': config.sk, + // 'Sec-Fetch-Dest': 'document', + // }), + // redirect: 'manual', + // }); + // expect(res.status).toBe(307); + // expect(res.headers.get('location')).toBe( + // `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( + // `${app.serverUrl}/personal-workspace`, // Redirects to the personal workspace (normal) + // )}&suffixed_cookies=false${devBrowserQuery}&organization_id=`, // Should attempt to activate the personal workspace with a blank query param + // ); + // }); + + // TODO(izaak): test from personal workspace to org transition }); From fee771a978953d6ba0fa65caceab6b10cc60cda8 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Mon, 9 Sep 2024 11:01:17 -0400 Subject: [PATCH 06/39] Trying out server-per-test --- integration/tests/handshake.test.ts | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index d035322030a..7ad0e77ae05 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -904,7 +904,7 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( const devBrowserCookie = '__clerk_db_jwt=needstobeset;'; const devBrowserQuery = '&__clerk_db_jwt=needstobeset'; - test.beforeAll('setup local Clerk API mock', async () => { + const start = async (): Promise => { const env = appConfigs.envs.withEmailCodes .clone() .setEnvVariable('private', 'CLERK_API_URL', `http://localhost:${PORT}`); @@ -947,12 +947,13 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( await app.setup(); await app.withEnv(env); await app.dev(); - }); + return app + } - test.afterAll(async () => { + const end = async (app: Application): Promise => { await app.teardown(); - await new Promise(resolve => jwksServer.close(() => resolve())); - }); + return new Promise(resolve => jwksServer.close(() => resolve())); + } type testCase = { name: string; @@ -985,6 +986,8 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( for (const testCase of testCases) { test(`organization activation by ID - ${testCase.name} - dev`, async () => { + const app = await start(); + const config = generateConfig({ mode: 'test', }); @@ -1014,10 +1017,12 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( `${app.serverUrl}/organizations-by-id/org_b`, // Redirects to org_b's path (normal) )}&suffixed_cookies=false${devBrowserQuery}&organization_id=org_b`, // Should attempt to activate org B in the redirect ); + + await end(app); }); } - + // TODO(izaak): refactor into table-driven test? // test('expired session token - with organization id mismatch - dev', async () => { // const config = generateConfig({ From 5c575eefd3c82ec650cfb619d5c14a7650789ba5 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Mon, 9 Sep 2024 11:06:26 -0400 Subject: [PATCH 07/39] Now in parallel! --- integration/tests/handshake.test.ts | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index 7ad0e77ae05..37888a2feeb 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -887,9 +887,11 @@ test.describe('Client handshake @generic', () => { }); test.describe('Client handshake with organization activation (by ID) @nextjs', () => { - test.describe.configure({ mode: 'serial' }); + test.describe.configure({ mode: 'parallel' }); + + const devBrowserCookie = '__clerk_db_jwt=needstobeset;'; + const devBrowserQuery = '&__clerk_db_jwt=needstobeset'; - let app: Application; const jwksServer = http.createServer(function (req, res) { const sk = req.headers.authorization?.replace('Bearer ', ''); if (!sk) { @@ -900,19 +902,23 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( res.write(JSON.stringify(getJwksFromSecretKey(sk))); res.end(); }); - // Strip trailing slash - const devBrowserCookie = '__clerk_db_jwt=needstobeset;'; - const devBrowserQuery = '&__clerk_db_jwt=needstobeset'; + + test.beforeAll('setup local jwks server', async () => { + // Start the jwks server + await new Promise(resolve => jwksServer.listen(0, resolve)); + }); + + test.afterAll('setup local Clerk API mock', async () => { + return new Promise(resolve => jwksServer.close(() => resolve())); + }); const start = async (): Promise => { const env = appConfigs.envs.withEmailCodes .clone() - .setEnvVariable('private', 'CLERK_API_URL', `http://localhost:${PORT}`); + .setEnvVariable('private', 'CLERK_API_URL', `http://localhost:${jwksServer.address().port}`); - // Start the jwks server - await new Promise(resolve => jwksServer.listen(4199, resolve)); - app = await appConfigs.next.appRouter + const app = await appConfigs.next.appRouter .clone() .addFile( 'src/middleware.ts', @@ -952,7 +958,6 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( const end = async (app: Application): Promise => { await app.teardown(); - return new Promise(resolve => jwksServer.close(() => resolve())); } type testCase = { From aa84f66ac6dcbc54ef7d12aa63afca038844c2a2 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Mon, 9 Sep 2024 11:52:12 -0400 Subject: [PATCH 08/39] The table-driven tests are now looking quite nice! --- integration/tests/handshake.test.ts | 216 +++++++--------------------- 1 file changed, 52 insertions(+), 164 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index 37888a2feeb..3a4fc89e4e6 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -5,6 +5,7 @@ import { expect, test } from '@playwright/test'; import type { Application } from '../models/application'; import { appConfigs } from '../presets'; import { generateConfig, getJwksFromSecretKey } from '../testUtils/handshake'; +import { OrganizationSyncOptions } from '../../packages/backend/src/tokens/types'; const PORT = 4199; @@ -912,17 +913,12 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( return new Promise(resolve => jwksServer.close(() => resolve())); }); - const start = async (): Promise => { + const start = async (orgSyncOptions: OrganizationSyncOptions): Promise => { const env = appConfigs.envs.withEmailCodes .clone() .setEnvVariable('private', 'CLERK_API_URL', `http://localhost:${jwksServer.address().port}`); - - const app = await appConfigs.next.appRouter - .clone() - .addFile( - 'src/middleware.ts', - () => `import { authMiddleware } from '@clerk/nextjs/server'; + const middlewareFile = `import { authMiddleware } from '@clerk/nextjs/server'; // Set the paths that don't require the user to be signed in const publicPaths = ['/', /^(\\/(sign-in|sign-up|app-dir|custom)\\/*).*$/]; export const middleware = (req, evt) => { @@ -936,17 +932,20 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( signInUrl: req.headers.get("x-sign-in-url"), // Critical - organizationSync: { - organizationPattern: "/organizations-by-id/:id", - personalWorkspacePattern: "/personal-workspace", - } + organizationSync: ${JSON.stringify(orgSyncOptions)} })(req, evt) }; export const config = { matcher: ['/((?!.+\\.[\\w]+$|_next).*)', '/', '/(api|trpc)(.*)'], }; - `, + ` + + const app = await appConfigs.next.appRouter + .clone() + .addFile( + 'src/middleware.ts', + () => middlewareFile, ) .commit(); @@ -962,51 +961,66 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( type testCase = { name: string; - state: 'active' | 'expired' | 'early'; + // With this initial state... + initialAuthState: 'active' | 'expired' | 'early'; + initialSessionClaims: Map; + + // When the customer app specifies these orgSyncOptions to middleware... + orgSyncOptions: OrganizationSyncOptions; + + // And a request arrives to the app at this path... appRequestPath: string; - initialClaims: Map; - fapiOrganizationIdParamValue: string; + + // The middleware should redirect to fapi with this query param value: + fapiOrganizationIdParamValue: string | null; } const testCases: testCase[] = [ { - name: 'When org A is active but org B is requested, attempts to activate org B (expired)', - state: 'expired', - appRequestPath: '/organizations-by-id/org_b', - initialClaims: new Map([ + name: 'When org A is active but org B is requested by ID, attempts to activate org B (expired)', + initialAuthState: 'expired', + initialSessionClaims: new Map([ ['org_id', 'org_a'] ]), - fapiOrganizationIdParamValue: 'org_b', + + appRequestPath: '/organizations-by-id/org_b', + fapiOrganizationIdParamValue: 'org_b' || null, + orgSyncOptions: { + organizationPattern: "/organizations-by-id/:id", + personalWorkspacePattern: "/personal-workspace", + } }, { - name: 'When org A is active but org B is requested, attempts to activate org B (active)', - state: 'active', + name: 'When org A is active but org B is requested by ID, attempts to activate org B (active)', + initialAuthState: 'active', appRequestPath: '/organizations-by-id/org_b', - initialClaims: new Map([ + initialSessionClaims: new Map([ ['org_id', 'org_a'] ]), fapiOrganizationIdParamValue: 'org_b', + orgSyncOptions: { + organizationPattern: "/organizations-by-id/:id", + personalWorkspacePattern: "/personal-workspace", + } }, ]; for (const testCase of testCases) { test(`organization activation by ID - ${testCase.name} - dev`, async () => { - const app = await start(); + const app = await start(testCase.orgSyncOptions); const config = generateConfig({ mode: 'test', }); // Create a new map with an org_id key const { token, claims } = config.generateToken({ - state: testCase.state, // <-- Critical - extraClaims: new Map([ - // Start the test with org A active - ['org_id', 'org_a'] - ]), + state: testCase.initialAuthState, // <-- Critical + extraClaims: testCase.initialSessionClaims, }); + const clientUat = claims.iat; const res = await fetch(app.serverUrl + - '/organizations-by-id/org_b', // But attempt to visit org B + testCase.appRequestPath, // But attempt to visit org B { headers: new Headers({ Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, @@ -1017,145 +1031,19 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( redirect: 'manual', }); expect(res.status).toBe(307); + + let expectedUrlSuffix = ""; + if (testCase.fapiOrganizationIdParamValue) { + expectedUrlSuffix += `&organization_id=${testCase.fapiOrganizationIdParamValue}`; + } + expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( - `${app.serverUrl}/organizations-by-id/org_b`, // Redirects to org_b's path (normal) - )}&suffixed_cookies=false${devBrowserQuery}&organization_id=org_b`, // Should attempt to activate org B in the redirect + `${app.serverUrl}${testCase.appRequestPath}`, // Redirects to the app's original request path + )}&suffixed_cookies=false${devBrowserQuery}${expectedUrlSuffix}`, ); await end(app); }); } - - - // TODO(izaak): refactor into table-driven test? - // test('expired session token - with organization id mismatch - dev', async () => { - // const config = generateConfig({ - // mode: 'test', - // }); - // // Create a new map with an org_id key - // const { token, claims } = config.generateToken({ - // state: 'expired', // <-- Critical - // extraClaims: new Map([ - // // Start the test with org A active - // ['org_id', 'org_a'] - // ]), - // }); - // const clientUat = claims.iat; - // const res = await fetch(app.serverUrl + - // '/organizations-by-id/org_b', // But attempt to visit org B - // { - // headers: new Headers({ - // Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, - // 'X-Publishable-Key': config.pk, - // 'X-Secret-Key': config.sk, - // 'Sec-Fetch-Dest': 'document', - // }), - // redirect: 'manual', - // }); - // expect(res.status).toBe(307); - // expect(res.headers.get('location')).toBe( - // `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( - // `${app.serverUrl}/organizations-by-id/org_b`, // Redirects to org_b's path (normal) - // )}&suffixed_cookies=false${devBrowserQuery}&organization_id=org_b`, // Should attempt to activate org B in the redirect - // ); - // }); - // - // test('valid session token - with organization id mismatch - dev', async () => { - // const config = generateConfig({ - // mode: 'test', - // }); - // // Create a new map with an org_id key - // const { token, claims } = config.generateToken({ - // state: 'active', // <-- Critical - // extraClaims: new Map([ - // // Start the test with org A active - // ['org_id', 'org_a'] - // ]), - // }); - // const clientUat = claims.iat; - // const res = await fetch(app.serverUrl + - // '/organizations-by-id/org_b', // But attempt to visit org B - // { - // headers: new Headers({ - // Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, - // 'X-Publishable-Key': config.pk, - // 'X-Secret-Key': config.sk, - // 'Sec-Fetch-Dest': 'document', - // }), - // redirect: 'manual', - // }); - // expect(res.status).toBe(307); - // expect(res.headers.get('location')).toBe( - // `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( - // `${app.serverUrl}/organizations-by-id/org_b`, // Redirects to org_b's path (normal) - // )}&suffixed_cookies=false${devBrowserQuery}&organization_id=org_b`, // Should attempt to activate org B in the redirect - // ); - // }); - // - // test('expired session token - with personal workspace mismatch - dev', async () => { - // const config = generateConfig({ - // mode: 'test', - // }); - // // Create a new map with an org_id key - // const { token, claims } = config.generateToken({ - // state: 'expired', // <-- Critical - // extraClaims: new Map([ - // // Start the test with org A active - // ['org_id', 'org_a'] - // ]), - // }); - // const clientUat = claims.iat; - // const res = await fetch(app.serverUrl + - // '/personal-workspace', // But attempt to visit the personal workspace - // { - // headers: new Headers({ - // Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, - // 'X-Publishable-Key': config.pk, - // 'X-Secret-Key': config.sk, - // 'Sec-Fetch-Dest': 'document', - // }), - // redirect: 'manual', - // }); - // expect(res.status).toBe(307); - // expect(res.headers.get('location')).toBe( - // `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( - // `${app.serverUrl}/personal-workspace`, // Redirects to the personal workspace (normal) - // )}&suffixed_cookies=false${devBrowserQuery}&organization_id=`, // Should attempt to activate the personal workspace with a blank query param - // ); - // }); - // - // test('valid session token - with personal workspace mismatch - dev', async () => { - // const config = generateConfig({ - // mode: 'test', - // }); - // // Create a new map with an org_id key - // const { token, claims } = config.generateToken({ - // state: 'active', // <-- Critical - // extraClaims: new Map([ - // // Start the test with org A active - // ['org_id', 'org_a'] - // ]), - // }); - // const clientUat = claims.iat; - // const res = await fetch(app.serverUrl + - // '/personal-workspace', // But attempt to visit the personal workspace - // { - // headers: new Headers({ - // Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, - // 'X-Publishable-Key': config.pk, - // 'X-Secret-Key': config.sk, - // 'Sec-Fetch-Dest': 'document', - // }), - // redirect: 'manual', - // }); - // expect(res.status).toBe(307); - // expect(res.headers.get('location')).toBe( - // `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( - // `${app.serverUrl}/personal-workspace`, // Redirects to the personal workspace (normal) - // )}&suffixed_cookies=false${devBrowserQuery}&organization_id=`, // Should attempt to activate the personal workspace with a blank query param - // ); - // }); - - // TODO(izaak): test from personal workspace to org transition }); From 45a982be339fed27042abc0c50656db298245639 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Mon, 9 Sep 2024 15:16:55 -0400 Subject: [PATCH 09/39] Doc comments --- packages/backend/src/tokens/types.ts | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/packages/backend/src/tokens/types.ts b/packages/backend/src/tokens/types.ts index f1d030b9bec..945ecc6f611 100644 --- a/packages/backend/src/tokens/types.ts +++ b/packages/backend/src/tokens/types.ts @@ -15,12 +15,29 @@ export type AuthenticateRequestOptions = { // OrganizationSyncOptions define the options for syncing an organization // or personal workspace state from the URL to the clerk session export type OrganizationSyncOptions = { - // Must have a path component named either ":id" or ":slug". - // Example: "/orgs/:slug" - organizationPattern: Pattern, - personalWorkspacePattern: Pattern, + // organizationPattern defines the URL pattern that is organization-specific and contains + // an organization ID or slug as a path token. If a request arrives to the application at + // a URL matching this path, the organization identifier will be extracted and activated + // before rendering. + // If no organization with the given identifier can be activated, + // either because it does not exist or the user does not have access to it, organization-related + // fields will be set to null, and the server component must detect this case and respond with + // an appropriate error (e.g. notFound()). + // If the route also matches with the personalWorkspacePattern, the personalWorkspacePattern + // takes precedence. + // + // Must have a group named either ":id", which matches to a clerk organization id, + // or ":slug", which matches to a clerk organization slug. + // Examples: "/orgs/:slug", "/orgs/:id" + organizationPattern?: Pattern, + + // personalWorkspacePattern defines the URL pattern for resources that exist in the context + // of a clerk personal workspace (user-specific, outside any other organization). + // If the route also matches with the organizationPattern, this takes precedence + personalWorkspacePattern?: Pattern, } // Pattern is a URL Pattern API style matcher -// Examples: "/orgs/:slug", "/orgs/:id" +// Syntax: https://developer.mozilla.org/en-US/docs/Web/API/URL_Pattern_API#pattern_syntax +// Examples: "/orgs/:slug", "/orgs/:id", "/personal-workspace" type Pattern = string; From edffbe0ae09808f997ce0448e4befcb52478cb89 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Mon, 9 Sep 2024 16:37:58 -0400 Subject: [PATCH 10/39] Detecting and handling handshake org sync loops https://clerkinc.slack.com/archives/C078JFZLLQ3/p1725908922766149 --- packages/backend/src/tokens/request.ts | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index 380e4a27f22..0b816f15471 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -364,7 +364,7 @@ ${error.getFullMessage()}`, throw errors[0]; } // use `await` to force this try/catch handle the signedIn invocation - const requestState = await signedIn( + const signedInRequestState = await signedIn( authenticateContext, data, undefined, @@ -372,11 +372,11 @@ ${error.getFullMessage()}`, ); // Org sync if necessary - // TODO(izaak): Also apply for auth in header scenario? Likely move to a standalone helper. + // TODO(izaak): Also apply for auth in header scenario? Likely move to a standalone helper. Maybe "handleMaybeOrganizationSyncHandshake" if (options.organizationSync) { const toActivate = getActivationEntity(authenticateContext.clerkUrl, options.organizationSync); if (toActivate) { - const auth = requestState.toAuth(); + const auth = signedInRequestState.toAuth(); if (auth) { let mustActivate = false; if (toActivate.organizationSlug && auth.orgSlug && toActivate.organizationSlug !== auth.orgSlug) { @@ -386,11 +386,23 @@ ${error.getFullMessage()}`, mustActivate = true; } if (mustActivate) { - return handleMaybeHandshakeStatus(authenticateContext, AuthErrorReason.ActiveOrganizationMismatch, ''); + console.log("GOT HERE - MUST ACTIVATE A THINGER!", authenticateContext.handshakeRedirectLoopCounter) + if (authenticateContext.handshakeRedirectLoopCounter > 0) { + // We have an organization that needs to be activated, but this isn't our first time redirecting. + // This is because we attempted to activate the organization previously, but the organization + // must not have been valid (either not found, or not valid for this user), and gave us back + // a null organization. We won't re-try the handshake, and leave it to the server component to handle + console.log("Nothing to worry about - we won't handshake again. Just going to return signedInRequestState") + return signedInRequestState; + } else { + return handleMaybeHandshakeStatus(authenticateContext, AuthErrorReason.ActiveOrganizationMismatch, ''); + } } } } } + // No organization sync was necessary + return signedInRequestState; } catch (err) { return handleError(err, 'cookie'); } From 256bef601ef52b3a7e66953f0ed3060329887ceb Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Tue, 10 Sep 2024 10:30:14 -0400 Subject: [PATCH 11/39] A handful more tests --- integration/tests/handshake.test.ts | 42 +++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index 3a4fc89e4e6..079217f2092 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -987,7 +987,6 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( fapiOrganizationIdParamValue: 'org_b' || null, orgSyncOptions: { organizationPattern: "/organizations-by-id/:id", - personalWorkspacePattern: "/personal-workspace", } }, { @@ -1000,9 +999,48 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( fapiOrganizationIdParamValue: 'org_b', orgSyncOptions: { organizationPattern: "/organizations-by-id/:id", - personalWorkspacePattern: "/personal-workspace", + personalWorkspacePattern: "/personal-workspace", // <-- Unnecessary } }, + { + name: 'When the personal workspace is active but org A is requested by ID, attempts to activate org A', + initialAuthState: 'active', + appRequestPath: '/organizations-by-id/org_b', + initialSessionClaims: new Map([ + // Intentionally no org claims - means personal workspace + ]), + fapiOrganizationIdParamValue: 'org_a', + orgSyncOptions: { + organizationPattern: "/organizations-by-id/:id", + personalWorkspacePattern: "/personal-workspace", + }, + }, + { + name: 'When org A is active but the personal workspace is requested, attempt to activate the personal workspace', + initialAuthState: 'active', + appRequestPath: '/personal-workspace', + initialSessionClaims: new Map([ + ['org_id', 'org_a'] + ]), + fapiOrganizationIdParamValue: '', + orgSyncOptions: { + organizationPattern: "/organizations-by-id/:id", + personalWorkspacePattern: "/personal-workspace", + }, + }, + { + name: 'Activates nothing with a broken path pattern', + initialAuthState: 'expired', // Tricky to test the non-expired case, because it won't handshake at all + appRequestPath: '/personal-workspace', + initialSessionClaims: new Map([ + ['org_id', 'org_a'] + ]), + fapiOrganizationIdParamValue: null, + orgSyncOptions: { + organizationPattern: "i am not a valid path pattern", + personalWorkspacePattern: "And neither am I!", + }, + }, ]; for (const testCase of testCases) { From 830dfeaa4009c9e39121265f0b8103f8b2d037eb Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Tue, 10 Sep 2024 17:59:41 -0400 Subject: [PATCH 12/39] Handling multiple path patterns, better test structure --- integration/tests/handshake.test.ts | 169 ++++++++++++++++--------- packages/backend/src/tokens/request.ts | 56 ++++++-- packages/backend/src/tokens/types.ts | 12 +- 3 files changed, 158 insertions(+), 79 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index 079217f2092..bb6855290bc 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -961,6 +961,10 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( type testCase = { name: string; + when: when; + then: then; + } + type when = { // With this initial state... initialAuthState: 'active' | 'expired' | 'early'; initialSessionClaims: Map; @@ -970,6 +974,11 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( // And a request arrives to the app at this path... appRequestPath: string; + } + + type then = { + // A handshake should (or should not) occur: + expectStatus: number; // The middleware should redirect to fapi with this query param value: fapiOrganizationIdParamValue: string | null; @@ -977,88 +986,129 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( const testCases: testCase[] = [ { - name: 'When org A is active but org B is requested by ID, attempts to activate org B (expired)', - initialAuthState: 'expired', - initialSessionClaims: new Map([ - ['org_id', 'org_a'] - ]), - - appRequestPath: '/organizations-by-id/org_b', - fapiOrganizationIdParamValue: 'org_b' || null, - orgSyncOptions: { - organizationPattern: "/organizations-by-id/:id", - } + name: 'When no org is active in a signed-out session but org A is requested by ID, attempts to activate org A', + when: { + initialAuthState: 'expired', + initialSessionClaims: new Map([ + // Intentionally empty + ]), + orgSyncOptions: { + organizationPatterns: ["/organizations-by-id/:id"], + }, + appRequestPath: '/organizations-by-id/org_a', + }, + then: { + expectStatus: 307, + fapiOrganizationIdParamValue: 'org_a', + }, + }, + { + name: 'When org A is active in a signed-out session but org B is requested by ID, attempts to activate org B', + when: { + initialAuthState: 'expired', + initialSessionClaims: new Map([ + ['org_id', 'org_a'] + ]), + orgSyncOptions: { + organizationPatterns: ["/organizations-by-id/:id"], + }, + appRequestPath: '/organizations-by-id/org_b', + }, + then: { + expectStatus: 307, + fapiOrganizationIdParamValue: 'org_b', + }, }, { name: 'When org A is active but org B is requested by ID, attempts to activate org B (active)', - initialAuthState: 'active', - appRequestPath: '/organizations-by-id/org_b', - initialSessionClaims: new Map([ - ['org_id', 'org_a'] - ]), - fapiOrganizationIdParamValue: 'org_b', - orgSyncOptions: { - organizationPattern: "/organizations-by-id/:id", - personalWorkspacePattern: "/personal-workspace", // <-- Unnecessary + when: { + initialAuthState: 'active', + initialSessionClaims: new Map([ + ['org_id', 'org_a'] + ]), + orgSyncOptions: { + organizationPatterns: ["/organizations-by-id/:id"], + personalWorkspacePatterns: ["/personal-workspace"], // <-- Unnecessary + }, + appRequestPath: '/organizations-by-id/org_b', + }, + then: { + expectStatus: 307, + fapiOrganizationIdParamValue: 'org_b', } }, { name: 'When the personal workspace is active but org A is requested by ID, attempts to activate org A', - initialAuthState: 'active', - appRequestPath: '/organizations-by-id/org_b', - initialSessionClaims: new Map([ - // Intentionally no org claims - means personal workspace - ]), - fapiOrganizationIdParamValue: 'org_a', - orgSyncOptions: { - organizationPattern: "/organizations-by-id/:id", - personalWorkspacePattern: "/personal-workspace", + when: { + initialAuthState: 'active', + initialSessionClaims: new Map([ + // Intentionally no org claims - means personal workspace + ]), + orgSyncOptions: { + organizationPatterns: ["/organizations-by-id/:id"], + personalWorkspacePatterns: ["/personal-workspace"], + }, + appRequestPath: '/organizations-by-id/org_a', }, + then: { + expectStatus: 307, + fapiOrganizationIdParamValue: 'org_a', + } }, { name: 'When org A is active but the personal workspace is requested, attempt to activate the personal workspace', - initialAuthState: 'active', - appRequestPath: '/personal-workspace', - initialSessionClaims: new Map([ - ['org_id', 'org_a'] - ]), - fapiOrganizationIdParamValue: '', - orgSyncOptions: { - organizationPattern: "/organizations-by-id/:id", - personalWorkspacePattern: "/personal-workspace", + when: { + initialAuthState: 'active', + initialSessionClaims: new Map([ + ['org_id', 'org_a'] + ]), + orgSyncOptions: { + organizationPatterns: ["/organizations-by-id/:id"], + personalWorkspacePatterns: ["/personal-workspace"], + }, + appRequestPath: '/personal-workspace', }, + then: { + expectStatus: 307, + fapiOrganizationIdParamValue: '', + } }, { name: 'Activates nothing with a broken path pattern', - initialAuthState: 'expired', // Tricky to test the non-expired case, because it won't handshake at all - appRequestPath: '/personal-workspace', - initialSessionClaims: new Map([ - ['org_id', 'org_a'] - ]), - fapiOrganizationIdParamValue: null, - orgSyncOptions: { - organizationPattern: "i am not a valid path pattern", - personalWorkspacePattern: "And neither am I!", + when: { + initialAuthState: 'active', + initialSessionClaims: new Map([ + ['org_id', 'org_a'] + ]), + orgSyncOptions: { + organizationPatterns: ["i am not a valid path pattern"], + personalWorkspacePatterns: ["And neither am I!"], + }, + appRequestPath: '/personal-workspace', }, + then: { + expectStatus: 200, + fapiOrganizationIdParamValue: null, + } }, ]; for (const testCase of testCases) { test(`organization activation by ID - ${testCase.name} - dev`, async () => { - const app = await start(testCase.orgSyncOptions); + const app = await start(testCase.when.orgSyncOptions); const config = generateConfig({ mode: 'test', }); // Create a new map with an org_id key const { token, claims } = config.generateToken({ - state: testCase.initialAuthState, // <-- Critical - extraClaims: testCase.initialSessionClaims, + state: testCase.when.initialAuthState, // <-- Critical + extraClaims: testCase.when.initialSessionClaims, }); const clientUat = claims.iat; const res = await fetch(app.serverUrl + - testCase.appRequestPath, // But attempt to visit org B + testCase.when.appRequestPath, // But attempt to visit org B { headers: new Headers({ Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, @@ -1067,20 +1117,13 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( 'Sec-Fetch-Dest': 'document', }), redirect: 'manual', - }); - expect(res.status).toBe(307); - - let expectedUrlSuffix = ""; - if (testCase.fapiOrganizationIdParamValue) { - expectedUrlSuffix += `&organization_id=${testCase.fapiOrganizationIdParamValue}`; - } - - expect(res.headers.get('location')).toBe( - `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( - `${app.serverUrl}${testCase.appRequestPath}`, // Redirects to the app's original request path - )}&suffixed_cookies=false${devBrowserQuery}${expectedUrlSuffix}`, + } ); + expect(res.status).toBe(testCase.then.expectStatus); + const redirectSearchParams = new URLSearchParams(res.headers.get('location')) + expect(redirectSearchParams.get('organization_id')).toBe(testCase.then.fapiOrganizationIdParamValue); + await end(app); }); } diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index 0b816f15471..fc316811fdd 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -1,4 +1,4 @@ -import { match } from 'path-to-regexp'; +import { match, MatchFunction, MatchResult } from 'path-to-regexp'; import { constants } from '../constants'; import type { TokenCarrier } from '../errors'; @@ -370,7 +370,6 @@ ${error.getFullMessage()}`, undefined, authenticateContext.sessionTokenInCookie!, ); - // Org sync if necessary // TODO(izaak): Also apply for auth in header scenario? Likely move to a standalone helper. Maybe "handleMaybeOrganizationSyncHandshake" if (options.organizationSync) { @@ -379,20 +378,24 @@ ${error.getFullMessage()}`, const auth = signedInRequestState.toAuth(); if (auth) { let mustActivate = false; - if (toActivate.organizationSlug && auth.orgSlug && toActivate.organizationSlug !== auth.orgSlug) { + // Activate an org by slug? + if (toActivate.organizationSlug && toActivate.organizationSlug !== auth.orgSlug) { + mustActivate = true; + } + // Activate an org by ID? + if (toActivate.organizationId && toActivate.organizationId !== auth.orgId) { mustActivate = true; } - if (toActivate.organizationId && auth.orgId && toActivate.organizationId !== auth.orgId) { + // Activate the personal workspace? + if (toActivate.type === 'personalWorkspace' && auth.orgId) { mustActivate = true; } if (mustActivate) { - console.log("GOT HERE - MUST ACTIVATE A THINGER!", authenticateContext.handshakeRedirectLoopCounter) if (authenticateContext.handshakeRedirectLoopCounter > 0) { // We have an organization that needs to be activated, but this isn't our first time redirecting. // This is because we attempted to activate the organization previously, but the organization // must not have been valid (either not found, or not valid for this user), and gave us back // a null organization. We won't re-try the handshake, and leave it to the server component to handle - console.log("Nothing to worry about - we won't handshake again. Just going to return signedInRequestState") return signedInRequestState; } else { return handleMaybeHandshakeStatus(authenticateContext, AuthErrorReason.ActiveOrganizationMismatch, ''); @@ -404,6 +407,7 @@ ${error.getFullMessage()}`, // No organization sync was necessary return signedInRequestState; } catch (err) { + console.log('Huh, got an error:', err.stack); return handleError(err, 'cookie'); } @@ -447,16 +451,48 @@ export const debugRequestState = (params: RequestState) => { function getActivationEntity(url: URL, options: OrganizationSyncOptions): ActivatibleEntity | null { // Check for personal workspace activation - if (options.personalWorkspacePattern) { - const personalResult = match(options.personalWorkspacePattern)(url.pathname); + if (options.personalWorkspacePatterns) { + let matcher: MatchFunction + try { + matcher = match(options.personalWorkspacePatterns) + } catch (e) { + console.error(`Invalid personal workspace pattern "${options.personalWorkspacePatterns}": "${e}". See https://www.npmjs.com/package/path-to-regexp`); + return null; + } + + let personalResult: boolean | MatchResult; + try { + personalResult = matcher(url.pathname); + } catch (e) { + // Intentionally not logging the path to avoid potentially leaking anything sensitive + console.error(`Failed to apply personal workspace pattern "${options.personalWorkspacePatterns}" to a path`, e); + return null; + } + if (personalResult) { return { type: 'personalWorkspace' }; } } // Check for organization activation - if (options.organizationPattern) { - const orgResult = match(options.organizationPattern)(url.pathname); + if (options.organizationPatterns) { + let matcher: MatchFunction + try { + matcher = match(options.organizationPatterns) + } catch (e) { + console.error(`Invalid organization pattern "${options.organizationPatterns}": "${e}". See https://www.npmjs.com/package/path-to-regexp`); + return null; + } + + let orgResult: boolean | MatchResult; + try { + orgResult = matcher(url.pathname); + } catch (e) { + // Intentionally not logging the path to avoid potentially leaking anything sensitive + console.error(`Failed to apply organization pattern "${options.organizationPatterns}" to a path`, e); + return null; + } + if (orgResult) { const { slug = '', id = '' } = orgResult.params as { slug?: string; id?: string }; if (id) { diff --git a/packages/backend/src/tokens/types.ts b/packages/backend/src/tokens/types.ts index 945ecc6f611..b18cb48d563 100644 --- a/packages/backend/src/tokens/types.ts +++ b/packages/backend/src/tokens/types.ts @@ -15,7 +15,7 @@ export type AuthenticateRequestOptions = { // OrganizationSyncOptions define the options for syncing an organization // or personal workspace state from the URL to the clerk session export type OrganizationSyncOptions = { - // organizationPattern defines the URL pattern that is organization-specific and contains + // organizationPattern defines the URL patterns that are organization-specific and contains // an organization ID or slug as a path token. If a request arrives to the application at // a URL matching this path, the organization identifier will be extracted and activated // before rendering. @@ -28,16 +28,16 @@ export type OrganizationSyncOptions = { // // Must have a group named either ":id", which matches to a clerk organization id, // or ":slug", which matches to a clerk organization slug. - // Examples: "/orgs/:slug", "/orgs/:id" - organizationPattern?: Pattern, + // Examples: "/orgs/:slug+*", "/orgs/:id+*" + organizationPatterns?: Array, // personalWorkspacePattern defines the URL pattern for resources that exist in the context // of a clerk personal workspace (user-specific, outside any other organization). // If the route also matches with the organizationPattern, this takes precedence - personalWorkspacePattern?: Pattern, + personalWorkspacePatterns?: Array, } -// Pattern is a URL Pattern API style matcher -// Syntax: https://developer.mozilla.org/en-US/docs/Web/API/URL_Pattern_API#pattern_syntax +// Pattern is a path-to-regexp style matcher +// Syntax: https://www.npmjs.com/package/path-to-regexp // Examples: "/orgs/:slug", "/orgs/:id", "/personal-workspace" type Pattern = string; From 63519d96da9988110be161974b23ae024d9fb9a9 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Tue, 10 Sep 2024 18:12:41 -0400 Subject: [PATCH 13/39] testing paths with sub-resources --- .../src/app/organizations-by-id/[id]/page.tsx | 7 +- .../[id]/settings/page.tsx | 18 +++++ .../app/organizations-by-slug/[slug]/page.tsx | 1 + .../[slug]/settings/page.tsx | 18 +++++ integration/tests/handshake.test.ts | 69 ++++++++++++++++++- packages/backend/src/tokens/request.ts | 1 - 6 files changed, 109 insertions(+), 5 deletions(-) create mode 100644 integration/templates/next-app-router/src/app/organizations-by-id/[id]/settings/page.tsx create mode 100644 integration/templates/next-app-router/src/app/organizations-by-slug/[slug]/settings/page.tsx diff --git a/integration/templates/next-app-router/src/app/organizations-by-id/[id]/page.tsx b/integration/templates/next-app-router/src/app/organizations-by-id/[id]/page.tsx index 231f92b42e0..fd0882d84cb 100644 --- a/integration/templates/next-app-router/src/app/organizations-by-id/[id]/page.tsx +++ b/integration/templates/next-app-router/src/app/organizations-by-id/[id]/page.tsx @@ -4,14 +4,15 @@ export default function Home({ params }): {} { const { orgId } = auth(); if (params.id != orgId) { - console.log('Mismatch - returning nothing for now...', params.slug, orgId); + console.log('Mismatch - returning nothing for now...', params.id, orgId); } - console.log("I'm the server and I got this slug: ", orgId); + console.log("I'm the server and I got this id: ", orgId); return ( <> -

From auth(), I know your org slug is: {orgId}

+

Org-specific home

+

From auth(), I know your org id is: {orgId}

); } diff --git a/integration/templates/next-app-router/src/app/organizations-by-id/[id]/settings/page.tsx b/integration/templates/next-app-router/src/app/organizations-by-id/[id]/settings/page.tsx new file mode 100644 index 00000000000..51a297ac08a --- /dev/null +++ b/integration/templates/next-app-router/src/app/organizations-by-id/[id]/settings/page.tsx @@ -0,0 +1,18 @@ +import { auth } from '@clerk/nextjs/server'; + +export default function Home({ params }): {} { + const { orgId } = auth(); + + if (params.id != orgId) { + console.log('Mismatch - returning nothing for now...', params.id, orgId); + } + + console.log("I'm the server and I got this id: ", orgId); + + return ( + <> +

Org-specific settings

+

From auth(), I know your org id is: {orgId}

+ + ); +} diff --git a/integration/templates/next-app-router/src/app/organizations-by-slug/[slug]/page.tsx b/integration/templates/next-app-router/src/app/organizations-by-slug/[slug]/page.tsx index ae65a381209..9f32b349f1b 100644 --- a/integration/templates/next-app-router/src/app/organizations-by-slug/[slug]/page.tsx +++ b/integration/templates/next-app-router/src/app/organizations-by-slug/[slug]/page.tsx @@ -11,6 +11,7 @@ export default function Home({ params }): {} { return ( <> +

Org-specific home

From auth(), I know your org slug is: {orgSlug}

); diff --git a/integration/templates/next-app-router/src/app/organizations-by-slug/[slug]/settings/page.tsx b/integration/templates/next-app-router/src/app/organizations-by-slug/[slug]/settings/page.tsx new file mode 100644 index 00000000000..e3f64f58f2f --- /dev/null +++ b/integration/templates/next-app-router/src/app/organizations-by-slug/[slug]/settings/page.tsx @@ -0,0 +1,18 @@ +import { auth } from '@clerk/nextjs/server'; + +export default function Home({ params }): {} { + const { orgSlug } = auth(); + + if (params.slug != orgSlug) { + console.log('Mismatch - returning nothing for now...', params.slug, orgSlug); + } + + console.log("I'm the server and I got this slug: ", orgSlug); + + return ( + <> +

Org-specific settings

+

From auth(), I know your org slug is: {orgSlug}

+ + ); +} diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index bb6855290bc..c14514ae4bb 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -1010,7 +1010,53 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( ['org_id', 'org_a'] ]), orgSyncOptions: { - organizationPatterns: ["/organizations-by-id/:id"], + organizationPatterns: [ + "/organizations-by-id/:id", + "/organizations-by-id/:id/", + "/organizations-by-id/:id/*splat", + ], + }, + appRequestPath: '/organizations-by-id/org_b', + }, + then: { + expectStatus: 307, + fapiOrganizationIdParamValue: 'org_b', + }, + }, + { + name: 'When org A is active in a signed-out session but an org B is requested by ID (with a trailing slash), attempts to activate org B', + when: { + initialAuthState: 'expired', + initialSessionClaims: new Map([ + ['org_id', 'org_a'] + ]), + orgSyncOptions: { + organizationPatterns: [ + "/organizations-by-id/:id", + "/organizations-by-id/:id/", + "/organizations-by-id/:id/*splat", + ], + }, + appRequestPath: '/organizations-by-id/org_b/', + }, + then: { + expectStatus: 307, + fapiOrganizationIdParamValue: 'org_b', + }, + }, + { + name: 'When org A is active in a signed-out session but an org B sub-resource is requested by ID, attempts to activate org B', + when: { + initialAuthState: 'expired', + initialSessionClaims: new Map([ + ['org_id', 'org_a'] + ]), + orgSyncOptions: { + organizationPatterns: [ + "/organizations-by-id/:id", + "/organizations-by-id/:id/", + "/organizations-by-id/:id/*splat", + ], }, appRequestPath: '/organizations-by-id/org_b', }, @@ -1019,6 +1065,27 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( fapiOrganizationIdParamValue: 'org_b', }, }, + { + name: 'When org A is active in an expired session and an org-agnostic resource is selected, no handshake param is added', + when: { + initialAuthState: 'expired', + initialSessionClaims: new Map([ + ['org_id', 'org_a'] + ]), + orgSyncOptions: { + organizationPatterns: [ + "/organizations-by-id/:id", + "/organizations-by-id/:id/", + "/organizations-by-id/:id/*splat", + ], + }, + appRequestPath: '/', + }, + then: { + expectStatus: 307, + fapiOrganizationIdParamValue: null, + }, + }, { name: 'When org A is active but org B is requested by ID, attempts to activate org B (active)', when: { diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index fc316811fdd..d52f3ece02f 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -407,7 +407,6 @@ ${error.getFullMessage()}`, // No organization sync was necessary return signedInRequestState; } catch (err) { - console.log('Huh, got an error:', err.stack); return handleError(err, 'cookie'); } From 02823ef8bda4fc8a1dce432a20ce60c35de0e35f Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 13 Sep 2024 13:02:26 -0400 Subject: [PATCH 14/39] Upgrading path-to-regexp I'll admit, i'm a bit fuzzy here on why it wasn't a dependency before, and also as to why it's only a dev dependency. --- package-lock.json | 22 ++++++++++++++++++++-- package.json | 1 + 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 32735f96a0a..daf992ca00f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -48,6 +48,7 @@ "jest-chrome": "^0.8.0", "jest-environment-jsdom": "^29.3.1", "lint-staged": "^14.0.1", + "path-to-regexp": "^8.1.0", "prettier": "^3.3.2", "prettier-plugin-tailwindcss": "^0.6.3", "publint": "^0.2.4", @@ -14791,6 +14792,12 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/astro/node_modules/path-to-regexp": { + "version": "6.3.0", + "resolved": "https://registry.npmjs.org/path-to-regexp/-/path-to-regexp-6.3.0.tgz", + "integrity": "sha512-Yhpw4T9C6hPpgPeA28us07OJeqZ5EzQTkbfwuhsUg0c237RomFoETJgmp2sa3F/41gfLE6G5cqcYwznmeEeOlQ==", + "peer": true + }, "node_modules/astro/node_modules/preferred-pm": { "version": "4.0.0", "license": "MIT", @@ -31087,8 +31094,13 @@ } }, "node_modules/path-to-regexp": { - "version": "6.2.2", - "license": "MIT" + "version": "8.1.0", + "resolved": "https://registry.npmjs.org/path-to-regexp/-/path-to-regexp-8.1.0.tgz", + "integrity": "sha512-Bqn3vc8CMHty6zuD+tG23s6v2kwxslHEhTj4eYaVKGIEB+YX/2wd0/rgXLFD9G9id9KCtbVy/3ZgmvZjpa0UdQ==", + "dev": true, + "engines": { + "node": ">=16" + } }, "node_modules/path-type": { "version": "4.0.0", @@ -44273,6 +44285,12 @@ "node": ">=6" } }, + "packages/tanstack-start/node_modules/path-to-regexp": { + "version": "6.3.0", + "resolved": "https://registry.npmjs.org/path-to-regexp/-/path-to-regexp-6.3.0.tgz", + "integrity": "sha512-Yhpw4T9C6hPpgPeA28us07OJeqZ5EzQTkbfwuhsUg0c237RomFoETJgmp2sa3F/41gfLE6G5cqcYwznmeEeOlQ==", + "dev": true + }, "packages/tanstack-start/node_modules/recast": { "version": "0.23.9", "dev": true, diff --git a/package.json b/package.json index 5d86e1126b7..54a8eac4689 100644 --- a/package.json +++ b/package.json @@ -92,6 +92,7 @@ "jest-chrome": "^0.8.0", "jest-environment-jsdom": "^29.3.1", "lint-staged": "^14.0.1", + "path-to-regexp": "^8.1.0", "prettier": "^3.3.2", "prettier-plugin-tailwindcss": "^0.6.3", "publint": "^0.2.4", From a6d413633cc521d7ca8225f44cf65f9308b7a748 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 13 Sep 2024 13:22:38 -0400 Subject: [PATCH 15/39] Test case with erroneous trailing slash now passing Turns out someone in the call chain was redirecting before we hit handshake. That's great! --- integration/tests/handshake.test.ts | 176 ++++++++++++---------------- 1 file changed, 72 insertions(+), 104 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index c14514ae4bb..97b4a6cb530 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -2,10 +2,10 @@ import * as http from 'node:http'; import { expect, test } from '@playwright/test'; +import type { OrganizationSyncOptions } from '../../packages/backend/src/tokens/types'; import type { Application } from '../models/application'; import { appConfigs } from '../presets'; import { generateConfig, getJwksFromSecretKey } from '../testUtils/handshake'; -import { OrganizationSyncOptions } from '../../packages/backend/src/tokens/types'; const PORT = 4199; @@ -73,7 +73,7 @@ test.describe('Client handshake @generic', () => { await new Promise(resolve => jwksServer.close(() => resolve())); }); - test('Test standard signed-in - dev', async () => { + test('standard signed-in - dev', async () => { const config = generateConfig({ mode: 'test' }); const { token, claims } = config.generateToken({ state: 'active' }); const clientUat = claims.iat; @@ -89,7 +89,7 @@ test.describe('Client handshake @generic', () => { expect(res.status).toBe(200); }); - test('Test standard signed-in - authorization header - dev', async () => { + test('standard signed-in - authorization header - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -108,7 +108,7 @@ test.describe('Client handshake @generic', () => { expect(res.status).toBe(200); }); - test('Test standard signed-in - prod', async () => { + test('standard signed-in - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -126,7 +126,7 @@ test.describe('Client handshake @generic', () => { expect(res.status).toBe(200); }); - test('Test standard signed-in - authorization header - prod', async () => { + test('standard signed-in - authorization header - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -144,7 +144,7 @@ test.describe('Client handshake @generic', () => { expect(res.status).toBe(200); }); - test('Test expired session token - dev', async () => { + test('expired session token - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -167,7 +167,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test expired session token - prod', async () => { + test('expired session token - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -190,7 +190,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test expired session token - authorization header - prod', async () => { + test('expired session token - authorization header - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -214,7 +214,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test early session token - dev', async () => { + test('early session token - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -237,7 +237,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test early session token - authorization header - dev', async () => { + test('early session token - authorization header - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -261,7 +261,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test proxyUrl - dev', async () => { + test('proxyUrl - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -285,7 +285,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test proxyUrl - prod', async () => { + test('proxyUrl - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -309,7 +309,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test domain - dev', async () => { + test('domain - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -333,7 +333,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test domain - prod', async () => { + test('domain - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -357,7 +357,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test missing session token, positive uat - dev', async () => { + test('missing session token, positive uat - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -378,7 +378,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test missing session token, positive uat - prod', async () => { + test('missing session token, positive uat - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -399,7 +399,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test missing session token, 0 uat (indicating signed out) - dev', async () => { + test('missing session token, 0 uat (indicating signed out) - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -415,7 +415,7 @@ test.describe('Client handshake @generic', () => { expect(res.status).toBe(200); }); - test('Test missing session token, 0 uat (indicating signed out) - prod', async () => { + test('missing session token, 0 uat (indicating signed out) - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -431,7 +431,7 @@ test.describe('Client handshake @generic', () => { expect(res.status).toBe(200); }); - test('Test missing session token, missing uat (indicating signed out) - dev', async () => { + test('missing session token, missing uat (indicating signed out) - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -447,7 +447,7 @@ test.describe('Client handshake @generic', () => { expect(res.status).toBe(200); }); - test('Test missing session token, missing uat (indicating signed out) - prod', async () => { + test('missing session token, missing uat (indicating signed out) - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -462,7 +462,7 @@ test.describe('Client handshake @generic', () => { expect(res.status).toBe(200); }); - test('Test signed out satellite no sec-fetch-dest=document - prod', async () => { + test('signed out satellite no sec-fetch-dest=document - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -478,7 +478,7 @@ test.describe('Client handshake @generic', () => { expect(res.status).toBe(200); }); - test('Test signed out satellite with sec-fetch-dest=document - prod', async () => { + test('signed out satellite with sec-fetch-dest=document - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -500,7 +500,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test signed out satellite - dev', async () => { + test('signed out satellite - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -517,7 +517,7 @@ test.describe('Client handshake @generic', () => { expect(res.status).toBe(200); }); - test('Test missing session token, missing uat (indicating signed out), missing devbrowser - dev', async () => { + test('missing session token, missing uat (indicating signed out), missing devbrowser - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -537,7 +537,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test redirect url - path and qs - dev', async () => { + test('redirect url - path and qs - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -560,7 +560,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test redirect url - path and qs - prod', async () => { + test('redirect url - path and qs - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -583,7 +583,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test redirect url - proxy - dev', async () => { + test('redirect url - proxy - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -606,7 +606,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test redirect url - proxy - prod', async () => { + test('redirect url - proxy - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -629,7 +629,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test redirect url - proxy with port - dev', async () => { + test('redirect url - proxy with port - dev', async () => { const config = generateConfig({ mode: 'test', }); @@ -652,7 +652,7 @@ test.describe('Client handshake @generic', () => { ); }); - test('Test redirect url - proxy with port - prod', async () => { + test('redirect url - proxy with port - prod', async () => { const config = generateConfig({ mode: 'live', }); @@ -891,7 +891,6 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( test.describe.configure({ mode: 'parallel' }); const devBrowserCookie = '__clerk_db_jwt=needstobeset;'; - const devBrowserQuery = '&__clerk_db_jwt=needstobeset'; const jwksServer = http.createServer(function (req, res) { const sk = req.headers.authorization?.replace('Bearer ', ''); @@ -939,31 +938,28 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( export const config = { matcher: ['/((?!.+\\.[\\w]+$|_next).*)', '/', '/(api|trpc)(.*)'], }; - ` + `; const app = await appConfigs.next.appRouter .clone() - .addFile( - 'src/middleware.ts', - () => middlewareFile, - ) + .addFile('src/middleware.ts', () => middlewareFile) .commit(); await app.setup(); await app.withEnv(env); await app.dev(); - return app - } + return app; + }; const end = async (app: Application): Promise => { await app.teardown(); - } + }; type testCase = { name: string; when: when; then: then; - } + }; type when = { // With this initial state... initialAuthState: 'active' | 'expired' | 'early'; @@ -974,7 +970,7 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( // And a request arrives to the app at this path... appRequestPath: string; - } + }; type then = { // A handshake should (or should not) occur: @@ -982,7 +978,7 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( // The middleware should redirect to fapi with this query param value: fapiOrganizationIdParamValue: string | null; - } + }; const testCases: testCase[] = [ { @@ -993,7 +989,7 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( // Intentionally empty ]), orgSyncOptions: { - organizationPatterns: ["/organizations-by-id/:id"], + organizationPatterns: ['/organizations-by-id/:id'], }, appRequestPath: '/organizations-by-id/org_a', }, @@ -1006,15 +1002,9 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( name: 'When org A is active in a signed-out session but org B is requested by ID, attempts to activate org B', when: { initialAuthState: 'expired', - initialSessionClaims: new Map([ - ['org_id', 'org_a'] - ]), + initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: [ - "/organizations-by-id/:id", - "/organizations-by-id/:id/", - "/organizations-by-id/:id/*splat", - ], + organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], }, appRequestPath: '/organizations-by-id/org_b', }, @@ -1024,39 +1014,29 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( }, }, { - name: 'When org A is active in a signed-out session but an org B is requested by ID (with a trailing slash), attempts to activate org B', + // This case ensures that, for the prototypical nextjs app, we permanent redirect before attempting the handshake logic. + // If this wasn't the case, we'd need to recommend adding an additional pattern with a trailing slash to our docs. + name: 'When org A is active in a signed-out session but an org B is requested by ID with a trailing slash, permanent redirects to the non-slash route without error.', when: { initialAuthState: 'expired', - initialSessionClaims: new Map([ - ['org_id', 'org_a'] - ]), + initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: [ - "/organizations-by-id/:id", - "/organizations-by-id/:id/", - "/organizations-by-id/:id/*splat", - ], + organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], }, appRequestPath: '/organizations-by-id/org_b/', }, then: { - expectStatus: 307, - fapiOrganizationIdParamValue: 'org_b', + expectStatus: 308, // Handshake never 308's - this points to `/organizations-by-id/org_b` (no trailing slash) + fapiOrganizationIdParamValue: null, }, }, { name: 'When org A is active in a signed-out session but an org B sub-resource is requested by ID, attempts to activate org B', when: { initialAuthState: 'expired', - initialSessionClaims: new Map([ - ['org_id', 'org_a'] - ]), + initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: [ - "/organizations-by-id/:id", - "/organizations-by-id/:id/", - "/organizations-by-id/:id/*splat", - ], + organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], }, appRequestPath: '/organizations-by-id/org_b', }, @@ -1069,15 +1049,9 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( name: 'When org A is active in an expired session and an org-agnostic resource is selected, no handshake param is added', when: { initialAuthState: 'expired', - initialSessionClaims: new Map([ - ['org_id', 'org_a'] - ]), + initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: [ - "/organizations-by-id/:id", - "/organizations-by-id/:id/", - "/organizations-by-id/:id/*splat", - ], + organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], }, appRequestPath: '/', }, @@ -1090,19 +1064,17 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( name: 'When org A is active but org B is requested by ID, attempts to activate org B (active)', when: { initialAuthState: 'active', - initialSessionClaims: new Map([ - ['org_id', 'org_a'] - ]), + initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: ["/organizations-by-id/:id"], - personalWorkspacePatterns: ["/personal-workspace"], // <-- Unnecessary + organizationPatterns: ['/organizations-by-id/:id'], + personalWorkspacePatterns: ['/personal-workspace'], // <-- Unnecessary }, appRequestPath: '/organizations-by-id/org_b', }, then: { expectStatus: 307, fapiOrganizationIdParamValue: 'org_b', - } + }, }, { name: 'When the personal workspace is active but org A is requested by ID, attempts to activate org A', @@ -1112,56 +1084,52 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( // Intentionally no org claims - means personal workspace ]), orgSyncOptions: { - organizationPatterns: ["/organizations-by-id/:id"], - personalWorkspacePatterns: ["/personal-workspace"], + organizationPatterns: ['/organizations-by-id/:id'], + personalWorkspacePatterns: ['/personal-workspace'], }, appRequestPath: '/organizations-by-id/org_a', }, then: { expectStatus: 307, fapiOrganizationIdParamValue: 'org_a', - } + }, }, { name: 'When org A is active but the personal workspace is requested, attempt to activate the personal workspace', when: { initialAuthState: 'active', - initialSessionClaims: new Map([ - ['org_id', 'org_a'] - ]), + initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: ["/organizations-by-id/:id"], - personalWorkspacePatterns: ["/personal-workspace"], + organizationPatterns: ['/organizations-by-id/:id'], + personalWorkspacePatterns: ['/personal-workspace'], }, appRequestPath: '/personal-workspace', }, then: { expectStatus: 307, fapiOrganizationIdParamValue: '', - } + }, }, { name: 'Activates nothing with a broken path pattern', when: { initialAuthState: 'active', - initialSessionClaims: new Map([ - ['org_id', 'org_a'] - ]), + initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: ["i am not a valid path pattern"], - personalWorkspacePatterns: ["And neither am I!"], + organizationPatterns: ['i am not a valid path pattern'], + personalWorkspacePatterns: ['And neither am I!'], }, appRequestPath: '/personal-workspace', }, then: { expectStatus: 200, fapiOrganizationIdParamValue: null, - } + }, }, ]; for (const testCase of testCases) { - test(`organization activation by ID - ${testCase.name} - dev`, async () => { + test(`${testCase.name}`, async () => { const app = await start(testCase.when.orgSyncOptions); const config = generateConfig({ @@ -1174,8 +1142,8 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( }); const clientUat = claims.iat; - const res = await fetch(app.serverUrl + - testCase.when.appRequestPath, // But attempt to visit org B + const res = await fetch( + app.serverUrl + testCase.when.appRequestPath, // But attempt to visit org B { headers: new Headers({ Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, @@ -1184,11 +1152,11 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( 'Sec-Fetch-Dest': 'document', }), redirect: 'manual', - } + }, ); expect(res.status).toBe(testCase.then.expectStatus); - const redirectSearchParams = new URLSearchParams(res.headers.get('location')) + const redirectSearchParams = new URLSearchParams(res.headers.get('location')); expect(redirectSearchParams.get('organization_id')).toBe(testCase.then.fapiOrganizationIdParamValue); await end(app); From 3a8f583543575b1fd4c38dbf439e83d4ab0281b5 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 13 Sep 2024 17:51:14 -0400 Subject: [PATCH 16/39] Handling header auth in addition to cookie auth --- integration/tests/handshake.test.ts | 74 ++++++++------- packages/backend/src/tokens/request.ts | 121 ++++++++++++++++--------- 2 files changed, 121 insertions(+), 74 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index 97b4a6cb530..5845de286bb 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -980,7 +980,7 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( fapiOrganizationIdParamValue: string | null; }; - const testCases: testCase[] = [ + const cookieAuthCases: testCase[] = [ { name: 'When no org is active in a signed-out session but org A is requested by ID, attempts to activate org A', when: { @@ -1128,38 +1128,48 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( }, ]; - for (const testCase of testCases) { - test(`${testCase.name}`, async () => { - const app = await start(testCase.when.orgSyncOptions); - - const config = generateConfig({ - mode: 'test', - }); - // Create a new map with an org_id key - const { token, claims } = config.generateToken({ - state: testCase.when.initialAuthState, // <-- Critical - extraClaims: testCase.when.initialSessionClaims, + for (const testCase of cookieAuthCases) { + ['Cookie', 'Authorization'].forEach(authHeader => { + test(`${authHeader} auth: ${testCase.name}`, async () => { + const app = await start(testCase.when.orgSyncOptions); + + const config = generateConfig({ + mode: 'test', + }); + // Create a new map with an org_id key + const { token, claims } = config.generateToken({ + state: testCase.when.initialAuthState, // <-- Critical + extraClaims: testCase.when.initialSessionClaims, + }); + + const headers = new Headers({ + 'X-Publishable-Key': config.pk, + 'X-Secret-Key': config.sk, + 'Sec-Fetch-Dest': 'document', + }); + switch (authHeader) { + case 'Cookie': + headers.set('Cookie', `${devBrowserCookie} __client_uat=${claims.iat}; __session=${token}`); + break; + case 'Authorization': + headers.set('Authorization', `Bearer ${token}`); + break; + } + + const res = await fetch( + app.serverUrl + testCase.when.appRequestPath, // But attempt to visit org B + { + headers: headers, + redirect: 'manual', + }, + ); + + expect(res.status).toBe(testCase.then.expectStatus); + const redirectSearchParams = new URLSearchParams(res.headers.get('location')); + expect(redirectSearchParams.get('organization_id')).toBe(testCase.then.fapiOrganizationIdParamValue); + + await end(app); }); - - const clientUat = claims.iat; - const res = await fetch( - app.serverUrl + testCase.when.appRequestPath, // But attempt to visit org B - { - headers: new Headers({ - Cookie: `${devBrowserCookie} __client_uat=${clientUat}; __session=${token}`, - 'X-Publishable-Key': config.pk, - 'X-Secret-Key': config.sk, - 'Sec-Fetch-Dest': 'document', - }), - redirect: 'manual', - }, - ); - - expect(res.status).toBe(testCase.then.expectStatus); - const redirectSearchParams = new URLSearchParams(res.headers.get('location')); - expect(redirectSearchParams.get('organization_id')).toBe(testCase.then.fapiOrganizationIdParamValue); - - await end(app); }); } }); diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index 7d41ed12c9c..0d015e90f38 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -1,4 +1,5 @@ -import { match, MatchFunction, MatchResult } from 'path-to-regexp'; +import type { MatchFunction, MatchResult } from 'path-to-regexp'; +import { match } from 'path-to-regexp'; import { constants } from '../constants'; import type { TokenCarrier } from '../errors'; @@ -8,7 +9,8 @@ import { assertValidSecretKey } from '../util/optionsAssertions'; import { isDevelopmentFromSecretKey } from '../util/shared'; import type { AuthenticateContext } from './authenticateContext'; import { createAuthenticateContext } from './authenticateContext'; -import type { RequestState } from './authStatus'; +import type { SignedInAuthObject } from './authObjects'; +import type { HandshakeState, RequestState, SignedOutState } from './authStatus'; import { AuthErrorReason, handshake, signedIn, signedOut } from './authStatus'; import { createClerkRequest } from './clerkRequest'; import { getCookieName, getCookieValue } from './cookie'; @@ -201,6 +203,53 @@ ${error.getFullMessage()}`, return signedOut(authenticateContext, reason, message); } + // NOTE(izaak): I don't love that null return implies signed-in... + function handleMaybeOrganizationSyncHandshake( + authenticateContext: AuthenticateContext, + auth: SignedInAuthObject, + ): HandshakeState | SignedOutState | null { + if (!options.organizationSync) { + return null; + } + const toActivate = getActivationEntity(authenticateContext.clerkUrl, options.organizationSync); + if (!toActivate) { + return null; + } + let mustActivate = false; + // Activate an org by slug? + if (toActivate.organizationSlug && toActivate.organizationSlug !== auth.orgSlug) { + mustActivate = true; + } + // Activate an org by ID? + if (toActivate.organizationId && toActivate.organizationId !== auth.orgId) { + mustActivate = true; + } + // Activate the personal workspace? + if (toActivate.type === 'personalWorkspace' && auth.orgId) { + mustActivate = true; + } + if (!mustActivate) { + return null; + } + if (authenticateContext.handshakeRedirectLoopCounter > 0) { + // We have an organization that needs to be activated, but this isn't our first time redirecting. + // This is because we attempted to activate the organization previously, but the organization + // must not have been valid (either not found, or not valid for this user), and gave us back + // a null organization. We won't re-try the handshake, and leave it to the server component to handle. + return null; + } + const handshakeState = handleMaybeHandshakeStatus( + authenticateContext, + AuthErrorReason.ActiveOrganizationMismatch, + '', + ); + if (handshakeState.status !== 'handshake') { + // Currently, this is only possible if we're in a redirect loop, but the above check should guard against that. + return null; + } + return handshakeState; + } + async function authenticateRequestWithTokenInHeader() { const { sessionTokenInHeader } = authenticateContext; @@ -210,7 +259,16 @@ ${error.getFullMessage()}`, throw errors[0]; } // use `await` to force this try/catch handle the signedIn invocation - return await signedIn(authenticateContext, data, undefined, sessionTokenInHeader!); + const signedInRequestState = await signedIn(authenticateContext, data, undefined, sessionTokenInHeader!); + // Org sync if necessary + const handshakeRequestState = handleMaybeOrganizationSyncHandshake( + authenticateContext, + signedInRequestState.toAuth(), + ); + if (handshakeRequestState) { + return handshakeRequestState; + } + return signedInRequestState; } catch (err) { return handleError(err, 'header'); } @@ -372,41 +430,16 @@ ${error.getFullMessage()}`, undefined, authenticateContext.sessionTokenInCookie!, ); + // Org sync if necessary - // TODO(izaak): Also apply for auth in header scenario? Likely move to a standalone helper. Maybe "handleMaybeOrganizationSyncHandshake" - if (options.organizationSync) { - const toActivate = getActivationEntity(authenticateContext.clerkUrl, options.organizationSync); - if (toActivate) { - const auth = signedInRequestState.toAuth(); - if (auth) { - let mustActivate = false; - // Activate an org by slug? - if (toActivate.organizationSlug && toActivate.organizationSlug !== auth.orgSlug) { - mustActivate = true; - } - // Activate an org by ID? - if (toActivate.organizationId && toActivate.organizationId !== auth.orgId) { - mustActivate = true; - } - // Activate the personal workspace? - if (toActivate.type === 'personalWorkspace' && auth.orgId) { - mustActivate = true; - } - if (mustActivate) { - if (authenticateContext.handshakeRedirectLoopCounter > 0) { - // We have an organization that needs to be activated, but this isn't our first time redirecting. - // This is because we attempted to activate the organization previously, but the organization - // must not have been valid (either not found, or not valid for this user), and gave us back - // a null organization. We won't re-try the handshake, and leave it to the server component to handle - return signedInRequestState; - } else { - return handleMaybeHandshakeStatus(authenticateContext, AuthErrorReason.ActiveOrganizationMismatch, ''); - } - } - } - } + const handshakeRequestState = handleMaybeOrganizationSyncHandshake( + authenticateContext, + signedInRequestState.toAuth(), + ); + if (handshakeRequestState) { + return handshakeRequestState; } - // No organization sync was necessary + return signedInRequestState; } catch (err) { return handleError(err, 'cookie'); @@ -453,11 +486,13 @@ export const debugRequestState = (params: RequestState) => { function getActivationEntity(url: URL, options: OrganizationSyncOptions): ActivatibleEntity | null { // Check for personal workspace activation if (options.personalWorkspacePatterns) { - let matcher: MatchFunction + let matcher: MatchFunction; try { - matcher = match(options.personalWorkspacePatterns) + matcher = match(options.personalWorkspacePatterns); } catch (e) { - console.error(`Invalid personal workspace pattern "${options.personalWorkspacePatterns}": "${e}". See https://www.npmjs.com/package/path-to-regexp`); + console.error( + `Invalid personal workspace pattern "${options.personalWorkspacePatterns}": "${e}". See https://www.npmjs.com/package/path-to-regexp`, + ); return null; } @@ -477,11 +512,13 @@ function getActivationEntity(url: URL, options: OrganizationSyncOptions): Activa // Check for organization activation if (options.organizationPatterns) { - let matcher: MatchFunction + let matcher: MatchFunction; try { - matcher = match(options.organizationPatterns) + matcher = match(options.organizationPatterns); } catch (e) { - console.error(`Invalid organization pattern "${options.organizationPatterns}": "${e}". See https://www.npmjs.com/package/path-to-regexp`); + console.error( + `Invalid organization pattern "${options.organizationPatterns}": "${e}". See https://www.npmjs.com/package/path-to-regexp`, + ); return null; } From b149e6b5cee5e351a8dfeba61e925e236162b75e Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 13 Sep 2024 17:57:16 -0400 Subject: [PATCH 17/39] Cleaning up todos, documenting functions --- .../backend/src/tokens/authenticateContext.ts | 1 - packages/backend/src/tokens/request.ts | 16 ++++++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/packages/backend/src/tokens/authenticateContext.ts b/packages/backend/src/tokens/authenticateContext.ts index c3345580b1f..cbf2057ff24 100644 --- a/packages/backend/src/tokens/authenticateContext.ts +++ b/packages/backend/src/tokens/authenticateContext.ts @@ -77,7 +77,6 @@ class AuthenticateContext { domain: options.domain, }); this.instanceType = pk.instanceType; - this.frontendApi = pk.frontendApi; } diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index 0d015e90f38..c6d0a88f97c 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -103,7 +103,7 @@ export async function authenticateRequest( if (options.organizationSync) { const toActivate = getActivationEntity(requestURL, options.organizationSync); if (toActivate) { - const params = getActivationParam(toActivate); + const params = getHandshakeActivationParam(toActivate); params.forEach((value, key) => { url.searchParams.append(key, value); @@ -203,6 +203,9 @@ ${error.getFullMessage()}`, return signedOut(authenticateContext, reason, message); } + // handleMaybeOrganizationSyncHandshake determines if a handshake must occur to resolve a mismatch between + // the organization as specified by the URL (according to the options) and the actual active organization + // on the session. // NOTE(izaak): I don't love that null return implies signed-in... function handleMaybeOrganizationSyncHandshake( authenticateContext: AuthenticateContext, @@ -483,6 +486,7 @@ export const debugRequestState = (params: RequestState) => { return { isSignedIn, proxyUrl, reason, message, publishableKey, isSatellite, domain }; }; +// getActivationEntity determines if the given URL and settings indicate a desire to activate a specific organization or personal workspace. function getActivationEntity(url: URL, options: OrganizationSyncOptions): ActivatibleEntity | null { // Check for personal workspace activation if (options.personalWorkspacePatterns) { @@ -539,20 +543,24 @@ function getActivationEntity(url: URL, options: OrganizationSyncOptions): Activa if (slug) { return { type: 'organization', organizationSlug: slug }; } - // TODO(izaak): Consider console warning if there's a pattern given (e.g. :orgSlug) that isn't :id or :slug + console.warn( + 'Detected an organization pattern match, but no organization ID or slug was found in the URL. Does the pattern include `:id` or `:slug`?', + ); } } return null; } +// ActivatibleEntity is an entity that can be activated by the handshake API. type ActivatibleEntity = { type: 'personalWorkspace' | 'organization' | 'none'; organizationId?: string; organizationSlug?: string; }; -// TODO(izaak): Find a better spot for this? -function getActivationParam(toActivate: ActivatibleEntity): Map { +// getHandshakeActivationParam takes an activatibatle entity (organization or personal workspace) +// and generates the query parameters that FAPI expects on the handshake API to activate that entity. +function getHandshakeActivationParam(toActivate: ActivatibleEntity): Map { const ret = new Map(); if (toActivate.type === 'personalWorkspace') { ret.set('organization_id', ''); From 32e4e0d6d1a8b016b3ae08aa85369b48b7fdf04e Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Mon, 16 Sep 2024 11:51:18 -0400 Subject: [PATCH 18/39] More thoughtful test cases To keep them all straight, I made a matrix: https://docs.google.com/spreadsheets/d/15OVcrHz_tj1K9XSfgPDzEQbjQLqn7XotJaZdZbLrLI4/edit?gid=0#gid=0 --- integration/tests/handshake.test.ts | 141 +++++++++++++++++++--------- 1 file changed, 99 insertions(+), 42 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index 5845de286bb..21162efad74 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -981,8 +981,11 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( }; const cookieAuthCases: testCase[] = [ + // ---------------- Session active vs expired tests ---------------- + // Note: it would be possible to run _every_ test with both active and expired initial states + // and expect the same results, but we're avoiding that to save some test execution time. { - name: 'When no org is active in a signed-out session but org A is requested by ID, attempts to activate org A', + name: 'Expired session, no org in session, but org a requested by ID => attempts to activate org A', when: { initialAuthState: 'expired', initialSessionClaims: new Map([ @@ -999,9 +1002,28 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( }, }, { - name: 'When org A is active in a signed-out session but org B is requested by ID, attempts to activate org B', + name: 'Active session, no org in session, but org a requested by ID => attempts to activate org A', when: { - initialAuthState: 'expired', + initialAuthState: 'active', + initialSessionClaims: new Map([ + // Intentionally empty + ]), + orgSyncOptions: { + organizationPatterns: ['/organizations-by-id/:id'], + }, + appRequestPath: '/organizations-by-id/org_a', + }, + then: { + expectStatus: 307, + fapiOrganizationIdParamValue: 'org_a', + }, + }, + + // ---------------- Existing session active org tests ---------------- + { + name: 'Active session, org A active in session, but org B is requested by ID => attempts to activate org B', + when: { + initialAuthState: 'active', initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], @@ -1014,112 +1036,147 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( }, }, { - // This case ensures that, for the prototypical nextjs app, we permanent redirect before attempting the handshake logic. - // If this wasn't the case, we'd need to recommend adding an additional pattern with a trailing slash to our docs. - name: 'When org A is active in a signed-out session but an org B is requested by ID with a trailing slash, permanent redirects to the non-slash route without error.', + name: 'Active session, no active org in session, but org B is requested by slug => attempts to activate org B', when: { - initialAuthState: 'expired', - initialSessionClaims: new Map([['org_id', 'org_a']]), + initialAuthState: 'active', + initialSessionClaims: new Map([ + // Intentionally empty + ]), orgSyncOptions: { - organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], + organizationPatterns: [ + '/organizations-by-id/:id', + '/organizations-by-id/:id/*splat', + '/organizations-by-slug/:slug', + '/organizations-by-slug/:id/*splat', + ], }, - appRequestPath: '/organizations-by-id/org_b/', + appRequestPath: '/organizations-by-slug/bcorp', }, then: { - expectStatus: 308, // Handshake never 308's - this points to `/organizations-by-id/org_b` (no trailing slash) - fapiOrganizationIdParamValue: null, + expectStatus: 307, + fapiOrganizationIdParamValue: 'bcorp', }, }, { - name: 'When org A is active in a signed-out session but an org B sub-resource is requested by ID, attempts to activate org B', + name: 'Active session, org a in session, but *an org B subresource* is requested by slug => attempts to activate org B', when: { - initialAuthState: 'expired', + initialAuthState: 'active', initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], + organizationPatterns: [ + '/organizations-by-slug/:slug', + '/organizations-by-slug/:id/*splat', + '/organizations-by-id/:id', + '/organizations-by-id/:id/*splat', + ], }, - appRequestPath: '/organizations-by-id/org_b', + appRequestPath: '/organizations-by-slug/bcorp/settings', }, then: { expectStatus: 307, - fapiOrganizationIdParamValue: 'org_b', + fapiOrganizationIdParamValue: 'bcorp', }, }, { - name: 'When org A is active in an expired session and an org-agnostic resource is selected, no handshake param is added', + // This case ensures that, for the prototypical nextjs app, we permanent redirect before attempting the handshake logic. + // If this wasn't the case, we'd need to recommend adding an additional pattern with a trailing slash to our docs. + name: 'When org A is active in a signed-out session but an org B is requested by ID with a trailing slash, permanent redirects to the non-slash route without error.', when: { initialAuthState: 'expired', initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], }, - appRequestPath: '/', + appRequestPath: '/organizations-by-id/org_b/', }, then: { - expectStatus: 307, + expectStatus: 308, // Handshake never 308's - this points to `/organizations-by-id/org_b` (no trailing slash) fapiOrganizationIdParamValue: null, }, }, + + // ---------------- Personal workspace tests ---------------- { - name: 'When org A is active but org B is requested by ID, attempts to activate org B (active)', + name: 'Active session, org a in session, but *the personal workspace* is requested => attempts to activate PWS', when: { initialAuthState: 'active', initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: ['/organizations-by-id/:id'], - personalWorkspacePatterns: ['/personal-workspace'], // <-- Unnecessary + organizationPatterns: [ + '/organizations-by-id/:id', + '/organizations-by-id/:id/*splat', + '/organizations-by-slug/:slug', + '/organizations-by-slug/:id/*splat', + ], + personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/*splat'], }, - appRequestPath: '/organizations-by-id/org_b', + appRequestPath: '/personal-workspace', }, then: { expectStatus: 307, - fapiOrganizationIdParamValue: 'org_b', + fapiOrganizationIdParamValue: '', // <-- Empty string indicates personal workspace }, }, + + // ---------------- No activation required tests ---------------- { - name: 'When the personal workspace is active but org A is requested by ID, attempts to activate org A', + name: 'Active session, nothing session, and the personal workspace is requested => nothing to activate!', when: { initialAuthState: 'active', initialSessionClaims: new Map([ - // Intentionally no org claims - means personal workspace + // Intentionally empty ]), orgSyncOptions: { - organizationPatterns: ['/organizations-by-id/:id'], - personalWorkspacePatterns: ['/personal-workspace'], + organizationPatterns: ['/organizations-by-slug/:slug', '/organizations-by-slug/:id/*splat'], + personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/*splat'], }, - appRequestPath: '/organizations-by-id/org_a', + appRequestPath: '/personal-workspace', }, then: { - expectStatus: 307, - fapiOrganizationIdParamValue: 'org_a', + expectStatus: 200, + fapiOrganizationIdParamValue: null, }, }, { - name: 'When org A is active but the personal workspace is requested, attempt to activate the personal workspace', + name: 'Active session, org a active in session, and org a is requested => nothing to activate!', when: { initialAuthState: 'active', initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: ['/organizations-by-id/:id'], - personalWorkspacePatterns: ['/personal-workspace'], + organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], + personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/*splat'], }, - appRequestPath: '/personal-workspace', + appRequestPath: '/organizations-by-id/org_a', }, then: { - expectStatus: 307, - fapiOrganizationIdParamValue: '', + expectStatus: 200, + fapiOrganizationIdParamValue: null, }, }, { - name: 'Activates nothing with a broken path pattern', + // NOTE(izaak): Would we prefer 500ing in this case? + name: 'Invalid config => ignore it and return 200', when: { initialAuthState: 'active', initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: ['i am not a valid path pattern'], - personalWorkspacePatterns: ['And neither am I!'], + organizationPatterns: ['i am not valid config'], }, - appRequestPath: '/personal-workspace', + appRequestPath: '/organizations-by-id/org_a', + }, + then: { + expectStatus: 200, + fapiOrganizationIdParamValue: null, + }, + }, + { + // NOTE(izaak): Would we prefer 500ing in this case? + name: 'No config => nothing to activate, return 200', + when: { + initialAuthState: 'active', + initialSessionClaims: new Map([['org_id', 'org_a']]), + orgSyncOptions: null, + appRequestPath: '/organizations-by-id/org_a', }, then: { expectStatus: 200, From 27257f9adc01658964b6817998baf44d662678ff Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Mon, 16 Sep 2024 13:04:26 -0400 Subject: [PATCH 19/39] Testing redirect loop protection --- integration/tests/handshake.test.ts | 158 +++++++++++++++++++++++-- packages/backend/src/tokens/request.ts | 9 +- 2 files changed, 156 insertions(+), 11 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index 21162efad74..372eca86d3f 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -887,7 +887,8 @@ test.describe('Client handshake @generic', () => { }); }); -test.describe('Client handshake with organization activation (by ID) @nextjs', () => { +// TODO(izaak): revert: test.describe('Client handshake with organization activation @nextjs', () => { +test.describe('Client handshake with organization activation', () => { test.describe.configure({ mode: 'parallel' }); const devBrowserCookie = '__clerk_db_jwt=needstobeset;'; @@ -1213,13 +1214,10 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( break; } - const res = await fetch( - app.serverUrl + testCase.when.appRequestPath, // But attempt to visit org B - { - headers: headers, - redirect: 'manual', - }, - ); + const res = await fetch(app.serverUrl + testCase.when.appRequestPath, { + headers: headers, + redirect: 'manual', + }); expect(res.status).toBe(testCase.then.expectStatus); const redirectSearchParams = new URLSearchParams(res.headers.get('location')); @@ -1230,3 +1228,147 @@ test.describe('Client handshake with organization activation (by ID) @nextjs', ( }); } }); + +test.describe('Client handshake with an organization activation avoids infinite loops @nextjs', () => { + test.describe.configure({ mode: 'parallel' }); + + const devBrowserCookie = '__clerk_db_jwt=needstobeset;'; + + const jwksServer = http.createServer(function (req, res) { + const sk = req.headers.authorization?.replace('Bearer ', ''); + if (!sk) { + console.log('No SK to', req.url, req.headers); + } + + res.setHeader('Content-Type', 'application/json'); + res.write(JSON.stringify(getJwksFromSecretKey(sk))); + res.end(); + }); + + // define app as an application + let thisApp: Application; + + test.beforeAll('setup local jwks server', async () => { + // Start the jwks server + await new Promise(resolve => jwksServer.listen(0, resolve)); + + thisApp = await start({ + organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], + personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/*splat'], + }); + }); + + test.afterAll('setup local Clerk API mock', async () => { + await end(thisApp); + return new Promise(resolve => jwksServer.close(() => resolve())); + }); + + const start = async (orgSyncOptions: OrganizationSyncOptions): Promise => { + const env = appConfigs.envs.withEmailCodes + .clone() + .setEnvVariable('private', 'CLERK_API_URL', `http://localhost:${jwksServer.address().port}`); + + const middlewareFile = `import { authMiddleware } from '@clerk/nextjs/server'; + // Set the paths that don't require the user to be signed in + const publicPaths = ['/', /^(\\/(sign-in|sign-up|app-dir|custom)\\/*).*$/]; + export const middleware = (req, evt) => { + return authMiddleware({ + publicRoutes: publicPaths, + publishableKey: req.headers.get("x-publishable-key"), + secretKey: req.headers.get("x-secret-key"), + proxyUrl: req.headers.get("x-proxy-url"), + domain: req.headers.get("x-domain"), + isSatellite: req.headers.get('x-satellite') === 'true', + signInUrl: req.headers.get("x-sign-in-url"), + + // Critical + organizationSync: ${JSON.stringify(orgSyncOptions)} + + })(req, evt) + }; + export const config = { + matcher: ['/((?!.+\\.[\\w]+$|_next).*)', '/', '/(api|trpc)(.*)'], + }; + `; + + const app = await appConfigs.next.appRouter + .clone() + .addFile('src/middleware.ts', () => middlewareFile) + .commit(); + + await app.setup(); + await app.withEnv(env); + await app.dev(); + return app; + }; + + const end = async (app: Application): Promise => { + await app.teardown(); + }; + + // -------------- Test begin ------------ + + const config = generateConfig({ + mode: 'test', + }); + + test('Sets the redirect loop tracking cookie', async () => { + // Create a new map with an org_id key + const { token, claims } = config.generateToken({ + state: 'active', + extraClaims: new Map([]), + }); + + const headers = new Headers({ + 'X-Publishable-Key': config.pk, + 'X-Secret-Key': config.sk, + 'Sec-Fetch-Dest': 'document', + }); + headers.set('Cookie', `${devBrowserCookie} __client_uat=${claims.iat}; __session=${token}`); + + const res = await fetch(thisApp.serverUrl + '/organizations-by-id/org_a', { + headers: headers, + redirect: 'manual', + }); + + expect(res.status).toBe(307); + const redirectSearchParams = new URLSearchParams(res.headers.get('location')); + expect(redirectSearchParams.get('organization_id')).toBe('org_a'); + + // read the set-cookie directives + const setCookie = res.headers.get('set-cookie'); + + expect(setCookie).toContain(`__clerk_redirect_count=1`); // <-- Critical + }); + + test('Ignores organization config when being redirected to', async () => { + // Create a new map with an org_id key + const { token, claims } = config.generateToken({ + state: 'active', // Must be active - handshake logic only runs once session is determined to be active + extraClaims: new Map([]), + }); + + const headers = new Headers({ + 'X-Publishable-Key': config.pk, + 'X-Secret-Key': config.sk, + 'Sec-Fetch-Dest': 'document', + }); + + // Critical cookie: __clerk_redirect_count + headers.set( + 'Cookie', + `${devBrowserCookie} __client_uat=${claims.iat}; __session=${token}; __clerk_redirect_count=1`, + ); + + const res = await fetch(thisApp.serverUrl + '/organizations-by-id/org_a', { + headers: headers, + redirect: 'manual', + }); + + expect(res.status).toBe(200); + const redirectSearchParams = new URLSearchParams(res.headers.get('location')); + expect(redirectSearchParams.get('organization_id')).toBe(null); + + expect(res.headers.get('set-cookie')).toBe(null); + }); +}); diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index c6d0a88f97c..7f18361dbde 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -239,6 +239,9 @@ ${error.getFullMessage()}`, // This is because we attempted to activate the organization previously, but the organization // must not have been valid (either not found, or not valid for this user), and gave us back // a null organization. We won't re-try the handshake, and leave it to the server component to handle. + console.warn( + 'Clerk: Organization activation handshake loop detected. This is likely due to an invalid organization ID or slug. Skipping organization activation.', + ); return null; } const handshakeState = handleMaybeHandshakeStatus( @@ -521,7 +524,7 @@ function getActivationEntity(url: URL, options: OrganizationSyncOptions): Activa matcher = match(options.organizationPatterns); } catch (e) { console.error( - `Invalid organization pattern "${options.organizationPatterns}": "${e}". See https://www.npmjs.com/package/path-to-regexp`, + `Clerk: Invalid organization pattern "${options.organizationPatterns}": "${e}". See https://www.npmjs.com/package/path-to-regexp`, ); return null; } @@ -531,7 +534,7 @@ function getActivationEntity(url: URL, options: OrganizationSyncOptions): Activa orgResult = matcher(url.pathname); } catch (e) { // Intentionally not logging the path to avoid potentially leaking anything sensitive - console.error(`Failed to apply organization pattern "${options.organizationPatterns}" to a path`, e); + console.error(`Clerk: Failed to apply organization pattern "${options.organizationPatterns}" to a path`, e); return null; } @@ -544,7 +547,7 @@ function getActivationEntity(url: URL, options: OrganizationSyncOptions): Activa return { type: 'organization', organizationSlug: slug }; } console.warn( - 'Detected an organization pattern match, but no organization ID or slug was found in the URL. Does the pattern include `:id` or `:slug`?', + 'Clerk: Detected an organization pattern match, but no organization ID or slug was found in the URL. Does the pattern include `:id` or `:slug`?', ); } } From 494e4686825a836d5c717c9430a65f6ca64f1331 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Mon, 16 Sep 2024 13:21:10 -0400 Subject: [PATCH 20/39] Not re-running every test with header/cookie auth In my judgement, the time to value tradeoff isn't there. It adds too much time. --- integration/tests/handshake.test.ts | 113 ++++++++++++++++++---------- 1 file changed, 74 insertions(+), 39 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index 372eca86d3f..7fc09a62e46 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -887,8 +887,7 @@ test.describe('Client handshake @generic', () => { }); }); -// TODO(izaak): revert: test.describe('Client handshake with organization activation @nextjs', () => { -test.describe('Client handshake with organization activation', () => { +test.describe('Client handshake with organization activation @nextjs', () => { test.describe.configure({ mode: 'parallel' }); const devBrowserCookie = '__clerk_db_jwt=needstobeset;'; @@ -971,6 +970,9 @@ test.describe('Client handshake with organization activation', () => { // And a request arrives to the app at this path... appRequestPath: string; + + // With a token specified in... + tokenAppearsIn: 'header' | 'cookie'; }; type then = { @@ -996,6 +998,7 @@ test.describe('Client handshake with organization activation', () => { organizationPatterns: ['/organizations-by-id/:id'], }, appRequestPath: '/organizations-by-id/org_a', + tokenAppearsIn: 'cookie', }, then: { expectStatus: 307, @@ -1013,6 +1016,29 @@ test.describe('Client handshake with organization activation', () => { organizationPatterns: ['/organizations-by-id/:id'], }, appRequestPath: '/organizations-by-id/org_a', + tokenAppearsIn: 'cookie', + }, + then: { + expectStatus: 307, + fapiOrganizationIdParamValue: 'org_a', + }, + }, + + // ---------------- Header-based auth tests ---------------- + // Note: it would be possible to run _every_ test with the token in the header or the cookies + // and expect the same results, but we're avoiding that to save some test execution time. + { + name: 'Header-based auth, active session, no org in session, but org a requested by ID => attempts to activate org A', + when: { + initialAuthState: 'active', + initialSessionClaims: new Map([ + // Intentionally empty + ]), + orgSyncOptions: { + organizationPatterns: ['/organizations-by-id/:id'], + }, + appRequestPath: '/organizations-by-id/org_a', + tokenAppearsIn: 'header', }, then: { expectStatus: 307, @@ -1030,6 +1056,7 @@ test.describe('Client handshake with organization activation', () => { organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], }, appRequestPath: '/organizations-by-id/org_b', + tokenAppearsIn: 'cookie', }, then: { expectStatus: 307, @@ -1052,6 +1079,7 @@ test.describe('Client handshake with organization activation', () => { ], }, appRequestPath: '/organizations-by-slug/bcorp', + tokenAppearsIn: 'cookie', }, then: { expectStatus: 307, @@ -1072,6 +1100,7 @@ test.describe('Client handshake with organization activation', () => { ], }, appRequestPath: '/organizations-by-slug/bcorp/settings', + tokenAppearsIn: 'cookie', }, then: { expectStatus: 307, @@ -1089,6 +1118,7 @@ test.describe('Client handshake with organization activation', () => { organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], }, appRequestPath: '/organizations-by-id/org_b/', + tokenAppearsIn: 'cookie', }, then: { expectStatus: 308, // Handshake never 308's - this points to `/organizations-by-id/org_b` (no trailing slash) @@ -1112,6 +1142,7 @@ test.describe('Client handshake with organization activation', () => { personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/*splat'], }, appRequestPath: '/personal-workspace', + tokenAppearsIn: 'cookie', }, then: { expectStatus: 307, @@ -1132,6 +1163,7 @@ test.describe('Client handshake with organization activation', () => { personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/*splat'], }, appRequestPath: '/personal-workspace', + tokenAppearsIn: 'cookie', }, then: { expectStatus: 200, @@ -1148,12 +1180,15 @@ test.describe('Client handshake with organization activation', () => { personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/*splat'], }, appRequestPath: '/organizations-by-id/org_a', + tokenAppearsIn: 'cookie', }, then: { expectStatus: 200, fapiOrganizationIdParamValue: null, }, }, + + // ---------------- Invalid permutation tests ---------------- { // NOTE(izaak): Would we prefer 500ing in this case? name: 'Invalid config => ignore it and return 200', @@ -1164,6 +1199,7 @@ test.describe('Client handshake with organization activation', () => { organizationPatterns: ['i am not valid config'], }, appRequestPath: '/organizations-by-id/org_a', + tokenAppearsIn: 'cookie', }, then: { expectStatus: 200, @@ -1178,6 +1214,7 @@ test.describe('Client handshake with organization activation', () => { initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: null, appRequestPath: '/organizations-by-id/org_a', + tokenAppearsIn: 'cookie', }, then: { expectStatus: 200, @@ -1187,44 +1224,42 @@ test.describe('Client handshake with organization activation', () => { ]; for (const testCase of cookieAuthCases) { - ['Cookie', 'Authorization'].forEach(authHeader => { - test(`${authHeader} auth: ${testCase.name}`, async () => { - const app = await start(testCase.when.orgSyncOptions); - - const config = generateConfig({ - mode: 'test', - }); - // Create a new map with an org_id key - const { token, claims } = config.generateToken({ - state: testCase.when.initialAuthState, // <-- Critical - extraClaims: testCase.when.initialSessionClaims, - }); - - const headers = new Headers({ - 'X-Publishable-Key': config.pk, - 'X-Secret-Key': config.sk, - 'Sec-Fetch-Dest': 'document', - }); - switch (authHeader) { - case 'Cookie': - headers.set('Cookie', `${devBrowserCookie} __client_uat=${claims.iat}; __session=${token}`); - break; - case 'Authorization': - headers.set('Authorization', `Bearer ${token}`); - break; - } - - const res = await fetch(app.serverUrl + testCase.when.appRequestPath, { - headers: headers, - redirect: 'manual', - }); - - expect(res.status).toBe(testCase.then.expectStatus); - const redirectSearchParams = new URLSearchParams(res.headers.get('location')); - expect(redirectSearchParams.get('organization_id')).toBe(testCase.then.fapiOrganizationIdParamValue); - - await end(app); + test(`${testCase.name}`, async () => { + const app = await start(testCase.when.orgSyncOptions); + + const config = generateConfig({ + mode: 'test', }); + // Create a new map with an org_id key + const { token, claims } = config.generateToken({ + state: testCase.when.initialAuthState, // <-- Critical + extraClaims: testCase.when.initialSessionClaims, + }); + + const headers = new Headers({ + 'X-Publishable-Key': config.pk, + 'X-Secret-Key': config.sk, + 'Sec-Fetch-Dest': 'document', + }); + switch (testCase.when.tokenAppearsIn) { + case 'cookie': + headers.set('Cookie', `${devBrowserCookie} __client_uat=${claims.iat}; __session=${token}`); + break; + case 'header': + headers.set('Authorization', `Bearer ${token}`); + break; + } + + const res = await fetch(app.serverUrl + testCase.when.appRequestPath, { + headers: headers, + redirect: 'manual', + }); + + expect(res.status).toBe(testCase.then.expectStatus); + const redirectSearchParams = new URLSearchParams(res.headers.get('location')); + expect(redirectSearchParams.get('organization_id')).toBe(testCase.then.fapiOrganizationIdParamValue); + + await end(app); }); } }); From 4c1dbb36ee9b6383ec0cc1c85232f3dae58a7820 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Wed, 18 Sep 2024 10:08:54 -0400 Subject: [PATCH 21/39] Using vendored path-to-regexp --- .../src/tokens/__tests__/request.test.ts | 14 +- packages/backend/src/tokens/request.ts | 30 +- .../src/compiled/path-to-regexp/index.js | 431 +++++++++++------- packages/shared/src/pathToRegexp.ts | 102 ++++- 4 files changed, 391 insertions(+), 186 deletions(-) diff --git a/packages/backend/src/tokens/__tests__/request.test.ts b/packages/backend/src/tokens/__tests__/request.test.ts index 73a68c74baf..00b81faa54b 100644 --- a/packages/backend/src/tokens/__tests__/request.test.ts +++ b/packages/backend/src/tokens/__tests__/request.test.ts @@ -13,7 +13,7 @@ import { import runtime from '../../runtime'; import { jsonOk } from '../../util/testUtils'; import { AuthErrorReason, type AuthReason, AuthStatus, type RequestState } from '../authStatus'; -import { authenticateRequest } from '../request'; +import { authenticateRequest, getActivationEntity } from '../request'; import type { AuthenticateRequestOptions } from '../types'; const PK_TEST = 'pk_test_Y2xlcmsuaW5zcGlyZWQucHVtYS03NC5sY2wuZGV2JA'; @@ -165,6 +165,18 @@ export default (QUnit: QUnit) => { return mockRequest({ cookie: cookieStr, ...headers }, requestUrl); }; + module('tokens.getActivationEntity(url,options)', _ => { + test('does anything', async assert => { + const path = new URL('http://localhost:3000/orgs/foo'); + const toActivate = getActivationEntity(path, { + organizationPatterns: ['/orgs/:id'], + }); + console.log('Here izaak', toActivate); + + assert.equal(toActivate?.organizationId, 'foo'); + }); + }); + module('tokens.authenticateRequest(options)', hooks => { let fakeClock; let fakeFetch; diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index d0a7ad2cb94..45857247958 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -1,5 +1,5 @@ -import type { MatchFunction, MatchResult } from 'path-to-regexp'; -import { match } from 'path-to-regexp'; +import type { Match, MatchFunction } from '@clerk/shared/pathToRegexp'; +import { match } from '@clerk/shared/pathToRegexp'; import type { ApiClient } from '../api'; import { constants } from '../constants'; @@ -569,21 +569,24 @@ export const debugRequestState = (params: RequestState) => { return { isSignedIn, proxyUrl, reason, message, publishableKey, isSatellite, domain }; }; -// getActivationEntity determines if the given URL and settings indicate a desire to activate a specific organization or personal workspace. -function getActivationEntity(url: URL, options: OrganizationSyncOptions): ActivatibleEntity | null { +// TODO(izaak): Convert all to this style (jsdoc) comment +/* + * Determines if the given URL and settings indicate a desire to activate a specific organization or personal workspace. + * @example test + */ +export function getActivationEntity(url: URL, options: OrganizationSyncOptions): ActivatibleEntity | null { // Check for personal workspace activation + console.log('THis too izaak'); if (options.personalWorkspacePatterns) { - let matcher: MatchFunction; + let matcher: MatchFunction>>; try { matcher = match(options.personalWorkspacePatterns); } catch (e) { - console.error( - `Invalid personal workspace pattern "${options.personalWorkspacePatterns}": "${e}". See https://www.npmjs.com/package/path-to-regexp`, - ); + console.error(`Invalid personal workspace pattern "${options.personalWorkspacePatterns}": "${e}"`); return null; } - let personalResult: boolean | MatchResult; + let personalResult: Match>>; try { personalResult = matcher(url.pathname); } catch (e) { @@ -599,17 +602,16 @@ function getActivationEntity(url: URL, options: OrganizationSyncOptions): Activa // Check for organization activation if (options.organizationPatterns) { - let matcher: MatchFunction; + let matcher: MatchFunction>>; try { matcher = match(options.organizationPatterns); + console.log('Izaak: This matcher', matcher); } catch (e) { - console.error( - `Clerk: Invalid organization pattern "${options.organizationPatterns}": "${e}". See https://www.npmjs.com/package/path-to-regexp`, - ); + console.error(`Clerk: Invalid organization pattern "${options.organizationPatterns}": "${e}"`); return null; } - let orgResult: boolean | MatchResult; + let orgResult: Match>>; try { orgResult = matcher(url.pathname); } catch (e) { diff --git a/packages/shared/src/compiled/path-to-regexp/index.js b/packages/shared/src/compiled/path-to-regexp/index.js index d7e7902de18..d279e5ed727 100644 --- a/packages/shared/src/compiled/path-to-regexp/index.js +++ b/packages/shared/src/compiled/path-to-regexp/index.js @@ -1,238 +1,331 @@ -/* eslint-disable no-redeclare */ +/* eslint-disable no-redeclare, curly, no-cond-assign */ function _(r) { for (var n = [], e = 0; e < r.length; ) { - var t = r[e]; - if (t === '*' || t === '+' || t === '?') { - n.push({ type: 'MODIFIER', index: e, value: r[e++] }); + var a = r[e]; + if (a === '*' || a === '+' || a === '?') { + n.push({ + type: 'MODIFIER', + index: e, + value: r[e++], + }); continue; } - if (t === '\\') { - n.push({ type: 'ESCAPED_CHAR', index: e++, value: r[e++] }); + if (a === '\\') { + n.push({ + type: 'ESCAPED_CHAR', + index: e++, + value: r[e++], + }); continue; } - if (t === '{') { - n.push({ type: 'OPEN', index: e, value: r[e++] }); + if (a === '{') { + n.push({ + type: 'OPEN', + index: e, + value: r[e++], + }); continue; } - if (t === '}') { - n.push({ type: 'CLOSE', index: e, value: r[e++] }); + if (a === '}') { + n.push({ + type: 'CLOSE', + index: e, + value: r[e++], + }); continue; } - if (t === ':') { - for (var u = '', a = e + 1; a < r.length; ) { - var f = r.charCodeAt(a); - if ((f >= 48 && f <= 57) || (f >= 65 && f <= 90) || (f >= 97 && f <= 122) || f === 95) { - u += r[a++]; + if (a === ':') { + for (var u = '', t = e + 1; t < r.length; ) { + var c = r.charCodeAt(t); + if ((c >= 48 && c <= 57) || (c >= 65 && c <= 90) || (c >= 97 && c <= 122) || c === 95) { + u += r[t++]; continue; } break; } - if (!u) { - throw new TypeError('Missing parameter name at '.concat(e)); - } - n.push({ type: 'NAME', index: e, value: u }), (e = a); + if (!u) throw new TypeError('Missing parameter name at '.concat(e)); + n.push({ + type: 'NAME', + index: e, + value: u, + }), + (e = t); continue; } - if (t === '(') { - var l = 1, - d = '', - a = e + 1; - if (r[a] === '?') { - throw new TypeError('Pattern cannot start with "?" at '.concat(a)); - } - for (; a < r.length; ) { - if (r[a] === '\\') { - d += r[a++] + r[a++]; + if (a === '(') { + var o = 1, + h = '', + t = e + 1; + if (r[t] === '?') throw new TypeError('Pattern cannot start with "?" at '.concat(t)); + for (; t < r.length; ) { + if (r[t] === '\\') { + h += r[t++] + r[t++]; continue; } - if (r[a] === ')') { - if ((l--, l === 0)) { - a++; + if (r[t] === ')') { + if ((o--, o === 0)) { + t++; break; } - } else if (r[a] === '(' && (l++, r[a + 1] !== '?')) { - throw new TypeError('Capturing groups are not allowed at '.concat(a)); - } - d += r[a++]; - } - if (l) { - throw new TypeError('Unbalanced pattern at '.concat(e)); + } else if (r[t] === '(' && (o++, r[t + 1] !== '?')) + throw new TypeError('Capturing groups are not allowed at '.concat(t)); + h += r[t++]; } - if (!d) { - throw new TypeError('Missing pattern at '.concat(e)); - } - n.push({ type: 'PATTERN', index: e, value: d }), (e = a); + if (o) throw new TypeError('Unbalanced pattern at '.concat(e)); + if (!h) throw new TypeError('Missing pattern at '.concat(e)); + n.push({ + type: 'PATTERN', + index: e, + value: h, + }), + (e = t); continue; } - n.push({ type: 'CHAR', index: e, value: r[e++] }); + n.push({ + type: 'CHAR', + index: e, + value: r[e++], + }); } - return n.push({ type: 'END', index: e, value: '' }), n; + return ( + n.push({ + type: 'END', + index: e, + value: '', + }), + n + ); } -function D(r, n) { + +function F(r, n) { n === void 0 && (n = {}); for ( var e = _(r), - t = n.prefixes, - u = t === void 0 ? './' : t, - a = '[^'.concat(y(n.delimiter || '/#?'), ']+?'), - f = [], - l = 0, - d = 0, - p = '', - c = function (v) { - if (d < e.length && e[d].type === v) { - return e[d++].value; - } + a = n.prefixes, + u = a === void 0 ? './' : a, + t = n.delimiter, + c = t === void 0 ? '/#?' : t, + o = [], + h = 0, + m = 0, + d = '', + f = function (l) { + if (m < e.length && e[m].type === l) return e[m++].value; }, - w = function (v) { - var g = c(v); - if (g !== void 0) { - return g; - } - var h = e[d], - b = h.type, - N = h.index; - throw new TypeError('Unexpected '.concat(b, ' at ').concat(N, ', expected ').concat(v)); + w = function (l) { + var v = f(l); + if (v !== void 0) return v; + var E = e[m], + P = E.type, + S = E.index; + throw new TypeError('Unexpected '.concat(P, ' at ').concat(S, ', expected ').concat(l)); + }, + p = function () { + for (var l = '', v; (v = f('CHAR') || f('ESCAPED_CHAR')); ) l += v; + return l; }, - A = function () { - for (var v = '', g; (g = c('CHAR') || c('ESCAPED_CHAR')); ) { - v += g; + O = function (l) { + for (var v = 0, E = c; v < E.length; v++) { + var P = E[v]; + if (l.indexOf(P) > -1) return !0; } - return v; + return !1; + }, + A = function (l) { + var v = o[o.length - 1], + E = l || (v && typeof v == 'string' ? v : ''); + if (v && !E) + throw new TypeError('Must have text between two parameters, missing text after "'.concat(v.name, '"')); + return !E || O(E) ? '[^'.concat(s(c), ']+?') : '(?:(?!'.concat(s(E), ')[^').concat(s(c), '])+?'); }; - d < e.length; + m < e.length; ) { - var s = c('CHAR'), - C = c('NAME'), - E = c('PATTERN'); - if (C || E) { - var x = s || ''; - u.indexOf(x) === -1 && ((p += x), (x = '')), - p && (f.push(p), (p = '')), - f.push({ name: C || l++, prefix: x, suffix: '', pattern: E || a, modifier: c('MODIFIER') || '' }); + var T = f('CHAR'), + x = f('NAME'), + C = f('PATTERN'); + if (x || C) { + var g = T || ''; + u.indexOf(g) === -1 && ((d += g), (g = '')), + d && (o.push(d), (d = '')), + o.push({ + name: x || h++, + prefix: g, + suffix: '', + pattern: C || A(g), + modifier: f('MODIFIER') || '', + }); continue; } - var o = s || c('ESCAPED_CHAR'); - if (o) { - p += o; + var i = T || f('ESCAPED_CHAR'); + if (i) { + d += i; continue; } - p && (f.push(p), (p = '')); - var R = c('OPEN'); + d && (o.push(d), (d = '')); + var R = f('OPEN'); if (R) { - var x = A(), - T = c('NAME') || '', - i = c('PATTERN') || '', - m = A(); + var g = p(), + y = f('NAME') || '', + N = f('PATTERN') || '', + b = p(); w('CLOSE'), - f.push({ - name: T || (i ? l++ : ''), - pattern: T && !i ? a : i, - prefix: x, - suffix: m, - modifier: c('MODIFIER') || '', + o.push({ + name: y || (N ? h++ : ''), + pattern: y && !N ? A(g) : N, + prefix: g, + suffix: b, + modifier: f('MODIFIER') || '', }); continue; } w('END'); } - return f; + return o; +} + +function H(r, n) { + var e = [], + a = M(r, e, n); + return I(a, e, n); +} + +function I(r, n, e) { + e === void 0 && (e = {}); + var a = e.decode, + u = + a === void 0 + ? function (t) { + return t; + } + : a; + return function (t) { + var c = r.exec(t); + if (!c) return !1; + for ( + var o = c[0], + h = c.index, + m = Object.create(null), + d = function (w) { + if (c[w] === void 0) return 'continue'; + var p = n[w - 1]; + p.modifier === '*' || p.modifier === '+' + ? (m[p.name] = c[w].split(p.prefix + p.suffix).map(function (O) { + return u(O, p); + })) + : (m[p.name] = u(c[w], p)); + }, + f = 1; + f < c.length; + f++ + ) + d(f); + return { + path: o, + index: h, + params: m, + }; + }; } -function y(r) { + +function s(r) { return r.replace(/([.+*?=^!:${}()[\]|/\\])/g, '\\$1'); } -function O(r) { + +function D(r) { return r && r.sensitive ? '' : 'i'; } -function M(r, n) { - if (!n) { - return r; - } - for (var e = /\((?:\?<(.*?)>)?(?!\?)/g, t = 0, u = e.exec(r.source); u; ) { - n.push({ name: u[1] || t++, prefix: '', suffix: '', modifier: '', pattern: '' }), (u = e.exec(r.source)); - } + +function $(r, n) { + if (!n) return r; + for (var e = /\((?:\?<(.*?)>)?(?!\?)/g, a = 0, u = e.exec(r.source); u; ) + n.push({ + name: u[1] || a++, + prefix: '', + suffix: '', + modifier: '', + pattern: '', + }), + (u = e.exec(r.source)); return r; } -function S(r, n, e) { - var t = r.map(function (u) { - return P(u, n, e).source; + +function W(r, n, e) { + var a = r.map(function (u) { + return M(u, n, e).source; }); - return new RegExp('(?:'.concat(t.join('|'), ')'), O(e)); + return new RegExp('(?:'.concat(a.join('|'), ')'), D(e)); } -function F(r, n, e) { - return H(D(r, e), n, e); + +function L(r, n, e) { + return U(F(r, e), n, e); } -function H(r, n, e) { + +function U(r, n, e) { e === void 0 && (e = {}); for ( - var t = e.strict, - u = t === void 0 ? !1 : t, - a = e.start, - f = a === void 0 ? !0 : a, - l = e.end, - d = l === void 0 ? !0 : l, - p = e.encode, - c = - p === void 0 - ? function (N) { - return N; + var a = e.strict, + u = a === void 0 ? !1 : a, + t = e.start, + c = t === void 0 ? !0 : t, + o = e.end, + h = o === void 0 ? !0 : o, + m = e.encode, + d = + m === void 0 + ? function (v) { + return v; } - : p, - w = e.delimiter, - A = w === void 0 ? '/#?' : w, - s = e.endsWith, - C = s === void 0 ? '' : s, - E = '['.concat(y(C), ']|$'), - x = '['.concat(y(A), ']'), - o = f ? '^' : '', - R = 0, - T = r; - R < T.length; - R++ + : m, + f = e.delimiter, + w = f === void 0 ? '/#?' : f, + p = e.endsWith, + O = p === void 0 ? '' : p, + A = '['.concat(s(O), ']|$'), + T = '['.concat(s(w), ']'), + x = c ? '^' : '', + C = 0, + g = r; + C < g.length; + C++ ) { - var i = T[R]; - if (typeof i == 'string') { - o += y(c(i)); - } else { - var m = y(c(i.prefix)), - v = y(c(i.suffix)); - if (i.pattern) { - if ((n && n.push(i), m || v)) { + var i = g[C]; + if (typeof i == 'string') x += s(d(i)); + else { + var R = s(d(i.prefix)), + y = s(d(i.suffix)); + if (i.pattern) + if ((n && n.push(i), R || y)) if (i.modifier === '+' || i.modifier === '*') { - var g = i.modifier === '*' ? '?' : ''; - o += '(?:' - .concat(m, '((?:') + var N = i.modifier === '*' ? '?' : ''; + x += '(?:' + .concat(R, '((?:') .concat(i.pattern, ')(?:') - .concat(v) - .concat(m, '(?:') + .concat(y) + .concat(R, '(?:') .concat(i.pattern, '))*)') - .concat(v, ')') - .concat(g); - } else { - o += '(?:'.concat(m, '(').concat(i.pattern, ')').concat(v, ')').concat(i.modifier); - } - } else { - i.modifier === '+' || i.modifier === '*' - ? (o += '((?:'.concat(i.pattern, ')').concat(i.modifier, ')')) - : (o += '('.concat(i.pattern, ')').concat(i.modifier)); + .concat(y, ')') + .concat(N); + } else x += '(?:'.concat(R, '(').concat(i.pattern, ')').concat(y, ')').concat(i.modifier); + else { + if (i.modifier === '+' || i.modifier === '*') + throw new TypeError('Can not repeat "'.concat(i.name, '" without a prefix and suffix')); + x += '('.concat(i.pattern, ')').concat(i.modifier); } - } else { - o += '(?:'.concat(m).concat(v, ')').concat(i.modifier); - } + else x += '(?:'.concat(R).concat(y, ')').concat(i.modifier); } } - if (d) { - u || (o += ''.concat(x, '?')), (o += e.endsWith ? '(?='.concat(E, ')') : '$'); - } else { - var h = r[r.length - 1], - b = typeof h == 'string' ? x.indexOf(h[h.length - 1]) > -1 : h === void 0; - u || (o += '(?:'.concat(x, '(?=').concat(E, '))?')), b || (o += '(?='.concat(x, '|').concat(E, ')')); + if (h) u || (x += ''.concat(T, '?')), (x += e.endsWith ? '(?='.concat(A, ')') : '$'); + else { + var b = r[r.length - 1], + l = typeof b == 'string' ? T.indexOf(b[b.length - 1]) > -1 : b === void 0; + u || (x += '(?:'.concat(T, '(?=').concat(A, '))?')), l || (x += '(?='.concat(T, '|').concat(A, ')')); } - return new RegExp(o, O(e)); + return new RegExp(x, D(e)); } -function P(r, n, e) { - return r instanceof RegExp ? M(r, n) : Array.isArray(r) ? S(r, n, e) : F(r, n, e); + +function M(r, n, e) { + return r instanceof RegExp ? $(r, n) : Array.isArray(r) ? W(r, n, e) : L(r, n, e); } -export { P as pathToRegexp }; +export { H as match, M as pathToRegexp }; diff --git a/packages/shared/src/pathToRegexp.ts b/packages/shared/src/pathToRegexp.ts index 64004295659..729799df567 100644 --- a/packages/shared/src/pathToRegexp.ts +++ b/packages/shared/src/pathToRegexp.ts @@ -1,4 +1,4 @@ -import { pathToRegexp as pathToRegexpBase } from './compiled/path-to-regexp'; +import { match as matchBase, pathToRegexp as pathToRegexpBase } from './compiled/path-to-regexp'; export const pathToRegexp = (path: string) => { try { @@ -6,7 +6,105 @@ export const pathToRegexp = (path: string) => { return pathToRegexpBase(path) as RegExp; } catch (e: any) { throw new Error( - `Invalid path: ${path}.\nConsult the documentation of path-to-regexp here: https://github.com/pillarjs/path-to-regexp\n${e.message}`, + `Invalid path: ${path}.\nConsult the documentation of path-to-regexp here: https://github.com/pillarjs/path-to-regexp/tree/6.x\n${e.message}`, ); } }; + +export function match

( + str: Path, + options?: ParseOptions & TokensToRegexpOptions & RegexpToFunctionOptions, +): MatchFunction

{ + try { + // @ts-ignore no types exists for the pre-compiled package + return matchBase(str, options) as MatchFunction

; + } catch (e: any) { + throw new Error( + `Invalid path and options: Consult the documentation of path-to-regexp here: https://github.com/pillarjs/path-to-regexp/tree/6.x\n${e.message}`, + ); + } +} + +/** + * A match is either `false` (no match) or a match result. + */ +export type Match

= false | MatchResult

; + +/** + * The match function takes a string and returns whether it matched the path. + */ +export type MatchFunction

= (path: string) => Match

; + +/** + * Supported `path-to-regexp` input types. + */ +export type Path = string | RegExp | Array; + +/** + * A match result contains data about the path match. + */ +export interface MatchResult

{ + path: string; + index: number; + params: P; +} + +export interface ParseOptions { + /** + * Set the default delimiter for repeat parameters. (default: `'/'`) + */ + delimiter?: string; + /** + * List of characters to automatically consider prefixes when parsing. + */ + prefixes?: string; +} + +export interface TokensToRegexpOptions { + /** + * When `true` the regexp will be case sensitive. (default: `false`) + */ + sensitive?: boolean; + /** + * When `true` the regexp won't allow an optional trailing delimiter to match. (default: `false`) + */ + strict?: boolean; + /** + * When `true` the regexp will match to the end of the string. (default: `true`) + */ + end?: boolean; + /** + * When `true` the regexp will match from the beginning of the string. (default: `true`) + */ + start?: boolean; + /** + * Sets the final character for non-ending optimistic matches. (default: `/`) + */ + delimiter?: string; + /** + * List of characters that can also be "end" characters. + */ + endsWith?: string; + /** + * Encode path tokens for use in the `RegExp`. + */ + encode?: (value: string) => string; +} + +export interface RegexpToFunctionOptions { + /** + * Function for decoding strings for params. + */ + decode?: (value: string, token: Key) => string; +} + +/** + * Metadata about a key. + */ +export interface Key { + name: string | number; + prefix: string; + suffix: string; + pattern: string; + modifier: string; +} From 5381882311acf4a253b7d0f90d4924dbef533c50 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Wed, 18 Sep 2024 14:55:28 -0400 Subject: [PATCH 22/39] Now using tsup to export types --- .../src/compiled/path-to-regexp/index.d.ts | 102 ++++++++++++++++ .../src/compiled/path-to-regexp/index.js | 112 +++++++++--------- packages/shared/src/pathToRegexp.ts | 96 ++------------- 3 files changed, 169 insertions(+), 141 deletions(-) create mode 100644 packages/shared/src/compiled/path-to-regexp/index.d.ts diff --git a/packages/shared/src/compiled/path-to-regexp/index.d.ts b/packages/shared/src/compiled/path-to-regexp/index.d.ts new file mode 100644 index 00000000000..d05fd814dae --- /dev/null +++ b/packages/shared/src/compiled/path-to-regexp/index.d.ts @@ -0,0 +1,102 @@ +interface ParseOptions { + /** + * Set the default delimiter for repeat parameters. (default: `'/'`) + */ + delimiter?: string; + /** + * List of characters to automatically consider prefixes when parsing. + */ + prefixes?: string; +} +interface RegexpToFunctionOptions { + /** + * Function for decoding strings for params. + */ + decode?: (value: string, token: Key) => string; +} +/** + * A match result contains data about the path match. + */ +interface MatchResult

{ + path: string; + index: number; + params: P; +} +/** + * A match is either `false` (no match) or a match result. + */ +type Match

= false | MatchResult

; +/** + * The match function takes a string and returns whether it matched the path. + */ +type MatchFunction

= (path: string) => Match

; +/** + * Create path match function from `path-to-regexp` spec. + */ +declare function match

( + str: Path, + options?: ParseOptions & TokensToRegexpOptions & RegexpToFunctionOptions, +): MatchFunction

; +/** + * Metadata about a key. + */ +interface Key { + name: string | number; + prefix: string; + suffix: string; + pattern: string; + modifier: string; +} +interface TokensToRegexpOptions { + /** + * When `true` the regexp will be case sensitive. (default: `false`) + */ + sensitive?: boolean; + /** + * When `true` the regexp won't allow an optional trailing delimiter to match. (default: `false`) + */ + strict?: boolean; + /** + * When `true` the regexp will match to the end of the string. (default: `true`) + */ + end?: boolean; + /** + * When `true` the regexp will match from the beginning of the string. (default: `true`) + */ + start?: boolean; + /** + * Sets the final character for non-ending optimistic matches. (default: `/`) + */ + delimiter?: string; + /** + * List of characters that can also be "end" characters. + */ + endsWith?: string; + /** + * Encode path tokens for use in the `RegExp`. + */ + encode?: (value: string) => string; +} +/** + * Supported `path-to-regexp` input types. + */ +type Path = string | RegExp | Array; +/** + * Normalize the given path string, returning a regular expression. + * + * An empty array can be passed in for the keys, which will hold the + * placeholder key descriptions. For example, using `/user/:id`, `keys` will + * contain `[{ name: 'id', delimiter: '/', optional: false, repeat: false }]`. + */ +declare function pathToRegexp(path: Path, keys?: Key[], options?: TokensToRegexpOptions & ParseOptions): RegExp; + +export { + type Match, + type MatchFunction, + type ParseOptions, + type Path, + type RegexpToFunctionOptions, + type TokensToRegexpOptions, + match, + pathToRegexp, +}; diff --git a/packages/shared/src/compiled/path-to-regexp/index.js b/packages/shared/src/compiled/path-to-regexp/index.js index d279e5ed727..f8ec5369fd5 100644 --- a/packages/shared/src/compiled/path-to-regexp/index.js +++ b/packages/shared/src/compiled/path-to-regexp/index.js @@ -55,12 +55,12 @@ function _(r) { } if (a === '(') { var o = 1, - h = '', + m = '', t = e + 1; if (r[t] === '?') throw new TypeError('Pattern cannot start with "?" at '.concat(t)); for (; t < r.length; ) { if (r[t] === '\\') { - h += r[t++] + r[t++]; + m += r[t++] + r[t++]; continue; } if (r[t] === ')') { @@ -70,14 +70,14 @@ function _(r) { } } else if (r[t] === '(' && (o++, r[t + 1] !== '?')) throw new TypeError('Capturing groups are not allowed at '.concat(t)); - h += r[t++]; + m += r[t++]; } if (o) throw new TypeError('Unbalanced pattern at '.concat(e)); - if (!h) throw new TypeError('Missing pattern at '.concat(e)); + if (!m) throw new TypeError('Missing pattern at '.concat(e)); n.push({ type: 'PATTERN', index: e, - value: h, + value: m, }), (e = t); continue; @@ -107,28 +107,28 @@ function F(r, n) { t = n.delimiter, c = t === void 0 ? '/#?' : t, o = [], - h = 0, m = 0, - d = '', + h = 0, + p = '', f = function (l) { - if (m < e.length && e[m].type === l) return e[m++].value; + if (h < e.length && e[h].type === l) return e[h++].value; }, w = function (l) { var v = f(l); if (v !== void 0) return v; - var E = e[m], - P = E.type, + var E = e[h], + N = E.type, S = E.index; - throw new TypeError('Unexpected '.concat(P, ' at ').concat(S, ', expected ').concat(l)); + throw new TypeError('Unexpected '.concat(N, ' at ').concat(S, ', expected ').concat(l)); }, - p = function () { + d = function () { for (var l = '', v; (v = f('CHAR') || f('ESCAPED_CHAR')); ) l += v; return l; }, - O = function (l) { + M = function (l) { for (var v = 0, E = c; v < E.length; v++) { - var P = E[v]; - if (l.indexOf(P) > -1) return !0; + var N = E[v]; + if (l.indexOf(N) > -1) return !0; } return !1; }, @@ -137,9 +137,9 @@ function F(r, n) { E = l || (v && typeof v == 'string' ? v : ''); if (v && !E) throw new TypeError('Must have text between two parameters, missing text after "'.concat(v.name, '"')); - return !E || O(E) ? '[^'.concat(s(c), ']+?') : '(?:(?!'.concat(s(E), ')[^').concat(s(c), '])+?'); + return !E || M(E) ? '[^'.concat(s(c), ']+?') : '(?:(?!'.concat(s(E), ')[^').concat(s(c), '])+?'); }; - m < e.length; + h < e.length; ) { var T = f('CHAR'), @@ -147,10 +147,10 @@ function F(r, n) { C = f('PATTERN'); if (x || C) { var g = T || ''; - u.indexOf(g) === -1 && ((d += g), (g = '')), - d && (o.push(d), (d = '')), + u.indexOf(g) === -1 && ((p += g), (g = '')), + p && (o.push(p), (p = '')), o.push({ - name: x || h++, + name: x || m++, prefix: g, suffix: '', pattern: C || A(g), @@ -160,20 +160,20 @@ function F(r, n) { } var i = T || f('ESCAPED_CHAR'); if (i) { - d += i; + p += i; continue; } - d && (o.push(d), (d = '')); + p && (o.push(p), (p = '')); var R = f('OPEN'); if (R) { - var g = p(), + var g = d(), y = f('NAME') || '', - N = f('PATTERN') || '', - b = p(); + O = f('PATTERN') || '', + b = d(); w('CLOSE'), o.push({ - name: y || (N ? h++ : ''), - pattern: y && !N ? A(g) : N, + name: y || (O ? m++ : ''), + pattern: y && !O ? A(g) : O, prefix: g, suffix: b, modifier: f('MODIFIER') || '', @@ -187,7 +187,7 @@ function F(r, n) { function H(r, n) { var e = [], - a = M(r, e, n); + a = P(r, e, n); return I(a, e, n); } @@ -205,26 +205,26 @@ function I(r, n, e) { if (!c) return !1; for ( var o = c[0], - h = c.index, - m = Object.create(null), - d = function (w) { + m = c.index, + h = Object.create(null), + p = function (w) { if (c[w] === void 0) return 'continue'; - var p = n[w - 1]; - p.modifier === '*' || p.modifier === '+' - ? (m[p.name] = c[w].split(p.prefix + p.suffix).map(function (O) { - return u(O, p); + var d = n[w - 1]; + d.modifier === '*' || d.modifier === '+' + ? (h[d.name] = c[w].split(d.prefix + d.suffix).map(function (M) { + return u(M, d); })) - : (m[p.name] = u(c[w], p)); + : (h[d.name] = u(c[w], d)); }, f = 1; f < c.length; f++ ) - d(f); + p(f); return { path: o, - index: h, - params: m, + index: m, + params: h, }; }; } @@ -253,7 +253,7 @@ function $(r, n) { function W(r, n, e) { var a = r.map(function (u) { - return M(u, n, e).source; + return P(u, n, e).source; }); return new RegExp('(?:'.concat(a.join('|'), ')'), D(e)); } @@ -270,19 +270,19 @@ function U(r, n, e) { t = e.start, c = t === void 0 ? !0 : t, o = e.end, - h = o === void 0 ? !0 : o, - m = e.encode, - d = - m === void 0 + m = o === void 0 ? !0 : o, + h = e.encode, + p = + h === void 0 ? function (v) { return v; } - : m, + : h, f = e.delimiter, w = f === void 0 ? '/#?' : f, - p = e.endsWith, - O = p === void 0 ? '' : p, - A = '['.concat(s(O), ']|$'), + d = e.endsWith, + M = d === void 0 ? '' : d, + A = '['.concat(s(M), ']|$'), T = '['.concat(s(w), ']'), x = c ? '^' : '', C = 0, @@ -291,14 +291,14 @@ function U(r, n, e) { C++ ) { var i = g[C]; - if (typeof i == 'string') x += s(d(i)); + if (typeof i == 'string') x += s(p(i)); else { - var R = s(d(i.prefix)), - y = s(d(i.suffix)); + var R = s(p(i.prefix)), + y = s(p(i.suffix)); if (i.pattern) if ((n && n.push(i), R || y)) if (i.modifier === '+' || i.modifier === '*') { - var N = i.modifier === '*' ? '?' : ''; + var O = i.modifier === '*' ? '?' : ''; x += '(?:' .concat(R, '((?:') .concat(i.pattern, ')(?:') @@ -306,7 +306,7 @@ function U(r, n, e) { .concat(R, '(?:') .concat(i.pattern, '))*)') .concat(y, ')') - .concat(N); + .concat(O); } else x += '(?:'.concat(R, '(').concat(i.pattern, ')').concat(y, ')').concat(i.modifier); else { if (i.modifier === '+' || i.modifier === '*') @@ -316,7 +316,7 @@ function U(r, n, e) { else x += '(?:'.concat(R).concat(y, ')').concat(i.modifier); } } - if (h) u || (x += ''.concat(T, '?')), (x += e.endsWith ? '(?='.concat(A, ')') : '$'); + if (m) u || (x += ''.concat(T, '?')), (x += e.endsWith ? '(?='.concat(A, ')') : '$'); else { var b = r[r.length - 1], l = typeof b == 'string' ? T.indexOf(b[b.length - 1]) > -1 : b === void 0; @@ -325,7 +325,7 @@ function U(r, n, e) { return new RegExp(x, D(e)); } -function M(r, n, e) { +function P(r, n, e) { return r instanceof RegExp ? $(r, n) : Array.isArray(r) ? W(r, n, e) : L(r, n, e); } -export { H as match, M as pathToRegexp }; +export { H as match, P as pathToRegexp }; diff --git a/packages/shared/src/pathToRegexp.ts b/packages/shared/src/pathToRegexp.ts index 729799df567..fcb11a1e4fe 100644 --- a/packages/shared/src/pathToRegexp.ts +++ b/packages/shared/src/pathToRegexp.ts @@ -1,9 +1,17 @@ +import type { + Match, + MatchFunction, + ParseOptions, + Path, + RegexpToFunctionOptions, + TokensToRegexpOptions, +} from './compiled/path-to-regexp'; import { match as matchBase, pathToRegexp as pathToRegexpBase } from './compiled/path-to-regexp'; export const pathToRegexp = (path: string) => { try { // @ts-ignore no types exists for the pre-compiled package - return pathToRegexpBase(path) as RegExp; + return pathToRegexpBase(path); } catch (e: any) { throw new Error( `Invalid path: ${path}.\nConsult the documentation of path-to-regexp here: https://github.com/pillarjs/path-to-regexp/tree/6.x\n${e.message}`, @@ -17,7 +25,7 @@ export function match

( ): MatchFunction

{ try { // @ts-ignore no types exists for the pre-compiled package - return matchBase(str, options) as MatchFunction

; + return matchBase(str, options); } catch (e: any) { throw new Error( `Invalid path and options: Consult the documentation of path-to-regexp here: https://github.com/pillarjs/path-to-regexp/tree/6.x\n${e.message}`, @@ -25,86 +33,4 @@ export function match

( } } -/** - * A match is either `false` (no match) or a match result. - */ -export type Match

= false | MatchResult

; - -/** - * The match function takes a string and returns whether it matched the path. - */ -export type MatchFunction

= (path: string) => Match

; - -/** - * Supported `path-to-regexp` input types. - */ -export type Path = string | RegExp | Array; - -/** - * A match result contains data about the path match. - */ -export interface MatchResult

{ - path: string; - index: number; - params: P; -} - -export interface ParseOptions { - /** - * Set the default delimiter for repeat parameters. (default: `'/'`) - */ - delimiter?: string; - /** - * List of characters to automatically consider prefixes when parsing. - */ - prefixes?: string; -} - -export interface TokensToRegexpOptions { - /** - * When `true` the regexp will be case sensitive. (default: `false`) - */ - sensitive?: boolean; - /** - * When `true` the regexp won't allow an optional trailing delimiter to match. (default: `false`) - */ - strict?: boolean; - /** - * When `true` the regexp will match to the end of the string. (default: `true`) - */ - end?: boolean; - /** - * When `true` the regexp will match from the beginning of the string. (default: `true`) - */ - start?: boolean; - /** - * Sets the final character for non-ending optimistic matches. (default: `/`) - */ - delimiter?: string; - /** - * List of characters that can also be "end" characters. - */ - endsWith?: string; - /** - * Encode path tokens for use in the `RegExp`. - */ - encode?: (value: string) => string; -} - -export interface RegexpToFunctionOptions { - /** - * Function for decoding strings for params. - */ - decode?: (value: string, token: Key) => string; -} - -/** - * Metadata about a key. - */ -export interface Key { - name: string | number; - prefix: string; - suffix: string; - pattern: string; - modifier: string; -} +export { type Match, type MatchFunction }; From d0d545f8fd19f506a67c366c618ffe472064f3c1 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Wed, 18 Sep 2024 15:37:48 -0400 Subject: [PATCH 23/39] Responding to https://github.com/clerk/javascript/pull/3977/files#r1761476490 Thanks @LauraBeatris! --- packages/backend/src/tokens/request.ts | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index 45857247958..777fa513ebf 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -119,15 +119,13 @@ export async function authenticateRequest( url.searchParams.append(constants.QueryParameters.DevBrowser, authenticateContext.devBrowserToken); } - if (options.organizationSync) { - const toActivate = getActivationEntity(requestURL, options.organizationSync); - if (toActivate) { - const params = getHandshakeActivationParam(toActivate); - - params.forEach((value, key) => { - url.searchParams.append(key, value); - }); - } + const toActivate = getActivationEntity(requestURL, options.organizationSync); + if (toActivate) { + const params = getHandshakeActivationParam(toActivate); + + params.forEach((value, key) => { + url.searchParams.append(key, value); + }); } return new Headers({ [constants.Headers.Location]: url.href }); @@ -274,9 +272,6 @@ ${error.getFullMessage()}`, authenticateContext: AuthenticateContext, auth: SignedInAuthObject, ): HandshakeState | SignedOutState | null { - if (!options.organizationSync) { - return null; - } const toActivate = getActivationEntity(authenticateContext.clerkUrl, options.organizationSync); if (!toActivate) { return null; @@ -574,9 +569,12 @@ export const debugRequestState = (params: RequestState) => { * Determines if the given URL and settings indicate a desire to activate a specific organization or personal workspace. * @example test */ -export function getActivationEntity(url: URL, options: OrganizationSyncOptions): ActivatibleEntity | null { +export function getActivationEntity(url: URL, options: OrganizationSyncOptions | undefined): ActivatibleEntity | null { + if (!options) { + return null; + } + // Check for personal workspace activation - console.log('THis too izaak'); if (options.personalWorkspacePatterns) { let matcher: MatchFunction>>; try { From 26d07b4b273ec7364f250f6996ac9b45b26e12d9 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Wed, 18 Sep 2024 17:11:00 -0400 Subject: [PATCH 24/39] Responding to: https://github.com/clerk/javascript/pull/3977#discussion_r1761573288 https://github.com/clerk/javascript/pull/3977#discussion_r1761554204 (doing it this way because I have a big enough local change that it's not worth trying to preserve the github-ui-created commits) --- packages/backend/src/tokens/request.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index 777fa513ebf..ae5e757e6e1 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -121,7 +121,7 @@ export async function authenticateRequest( const toActivate = getActivationEntity(requestURL, options.organizationSync); if (toActivate) { - const params = getHandshakeActivationParam(toActivate); + const params = getOrganizationSyncQueryParams(toActivate); params.forEach((value, key) => { url.searchParams.append(key, value); @@ -329,10 +329,7 @@ ${error.getFullMessage()}`, authenticateContext, signedInRequestState.toAuth(), ); - if (handshakeRequestState) { - return handshakeRequestState; - } - return signedInRequestState; + return handshakeRequestState || signedInRequestState; } catch (err) { return handleError(err, 'header'); } @@ -641,9 +638,9 @@ type ActivatibleEntity = { organizationSlug?: string; }; -// getHandshakeActivationParam takes an activatibatle entity (organization or personal workspace) +// getOrganizationSyncQueryParams takes an activatibatle entity (organization or personal workspace) // and generates the query parameters that FAPI expects on the handshake API to activate that entity. -function getHandshakeActivationParam(toActivate: ActivatibleEntity): Map { +function getOrganizationSyncQueryParams(toActivate: ActivatibleEntity): Map { const ret = new Map(); if (toActivate.type === 'personalWorkspace') { ret.set('organization_id', ''); From 76be5f676664c9d019107a69f635e9a414566e98 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Wed, 18 Sep 2024 17:18:34 -0400 Subject: [PATCH 25/39] Update packages/backend/src/tokens/types.ts Co-authored-by: Laura Beatris <48022589+LauraBeatris@users.noreply.github.com> --- packages/backend/src/tokens/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/tokens/types.ts b/packages/backend/src/tokens/types.ts index c140fb307dd..81866a073c3 100644 --- a/packages/backend/src/tokens/types.ts +++ b/packages/backend/src/tokens/types.ts @@ -10,7 +10,7 @@ export type AuthenticateRequestOptions = { signUpUrl?: string; afterSignInUrl?: string; afterSignUpUrl?: string; - organizationSync?: OrganizationSyncOptions; + organizationSyncOptions?: OrganizationSyncOptions; apiClient?: ApiClient; } & VerifyTokenOptions; From cbde8fd42d62025522dbeb46746605bb988fd317 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Wed, 18 Sep 2024 17:40:17 -0400 Subject: [PATCH 26/39] Better naming and types --- integration/tests/handshake.test.ts | 2 +- packages/backend/src/tokens/request.ts | 39 +++++++++++++------------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index 7fc09a62e46..f8753820e5a 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -1317,7 +1317,7 @@ test.describe('Client handshake with an organization activation avoids infinite signInUrl: req.headers.get("x-sign-in-url"), // Critical - organizationSync: ${JSON.stringify(orgSyncOptions)} + organizationSyncOptions: ${JSON.stringify(orgSyncOptions)} })(req, evt) }; diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index ae5e757e6e1..76978355385 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -119,7 +119,7 @@ export async function authenticateRequest( url.searchParams.append(constants.QueryParameters.DevBrowser, authenticateContext.devBrowserToken); } - const toActivate = getActivationEntity(requestURL, options.organizationSync); + const toActivate = getActivationEntity(requestURL, options.organizationSyncOptions); if (toActivate) { const params = getOrganizationSyncQueryParams(toActivate); @@ -272,18 +272,20 @@ ${error.getFullMessage()}`, authenticateContext: AuthenticateContext, auth: SignedInAuthObject, ): HandshakeState | SignedOutState | null { - const toActivate = getActivationEntity(authenticateContext.clerkUrl, options.organizationSync); + const toActivate = getActivationEntity(authenticateContext.clerkUrl, options.organizationSyncOptions); if (!toActivate) { return null; } let mustActivate = false; - // Activate an org by slug? - if (toActivate.organizationSlug && toActivate.organizationSlug !== auth.orgSlug) { - mustActivate = true; - } - // Activate an org by ID? - if (toActivate.organizationId && toActivate.organizationId !== auth.orgId) { - mustActivate = true; + if (toActivate.type === 'organization') { + // Activate an org by slug? + if (toActivate.organizationSlug && toActivate.organizationSlug !== auth.orgSlug) { + mustActivate = true; + } + // Activate an org by ID? + if (toActivate.organizationId && toActivate.organizationId !== auth.orgId) { + mustActivate = true; + } } // Activate the personal workspace? if (toActivate.type === 'personalWorkspace' && auth.orgId) { @@ -561,12 +563,14 @@ export const debugRequestState = (params: RequestState) => { return { isSignedIn, proxyUrl, reason, message, publishableKey, isSatellite, domain }; }; -// TODO(izaak): Convert all to this style (jsdoc) comment /* * Determines if the given URL and settings indicate a desire to activate a specific organization or personal workspace. * @example test */ -export function getActivationEntity(url: URL, options: OrganizationSyncOptions | undefined): ActivatibleEntity | null { +export function getActivationEntity( + url: URL, + options: OrganizationSyncOptions | undefined, +): OrganizationSyncTarget | null { if (!options) { return null; } @@ -600,7 +604,6 @@ export function getActivationEntity(url: URL, options: OrganizationSyncOptions | let matcher: MatchFunction>>; try { matcher = match(options.organizationPatterns); - console.log('Izaak: This matcher', matcher); } catch (e) { console.error(`Clerk: Invalid organization pattern "${options.organizationPatterns}": "${e}"`); return null; @@ -631,16 +634,14 @@ export function getActivationEntity(url: URL, options: OrganizationSyncOptions | return null; } -// ActivatibleEntity is an entity that can be activated by the handshake API. -type ActivatibleEntity = { - type: 'personalWorkspace' | 'organization' | 'none'; - organizationId?: string; - organizationSlug?: string; -}; +// OrganizationSyncTarget is an entity that can be activated by the handshake API. +export type OrganizationSyncTarget = + | { type: 'personalWorkspace' } + | { type: 'organization'; organizationId?: string; organizationSlug?: string }; // getOrganizationSyncQueryParams takes an activatibatle entity (organization or personal workspace) // and generates the query parameters that FAPI expects on the handshake API to activate that entity. -function getOrganizationSyncQueryParams(toActivate: ActivatibleEntity): Map { +function getOrganizationSyncQueryParams(toActivate: OrganizationSyncTarget): Map { const ret = new Map(); if (toActivate.type === 'personalWorkspace') { ret.set('organization_id', ''); From c0b291dec43337ca364b942149690cb2f1060704 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Thu, 19 Sep 2024 18:46:09 -0400 Subject: [PATCH 27/39] Switching conclusively to the internal path-to-regexp --- integration/tests/handshake.test.ts | 35 ++-- .../src/tokens/__tests__/request.test.ts | 149 ++++++++++++++++-- packages/backend/src/tokens/request.ts | 8 +- 3 files changed, 162 insertions(+), 30 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index f8753820e5a..7f32ce41182 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -931,7 +931,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { signInUrl: req.headers.get("x-sign-in-url"), // Critical - organizationSync: ${JSON.stringify(orgSyncOptions)} + organizationSyncOptions: ${JSON.stringify(orgSyncOptions)} })(req, evt) }; @@ -1053,7 +1053,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { initialAuthState: 'active', initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], + organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/(.*)'], }, appRequestPath: '/organizations-by-id/org_b', tokenAppearsIn: 'cookie', @@ -1073,9 +1073,9 @@ test.describe('Client handshake with organization activation @nextjs', () => { orgSyncOptions: { organizationPatterns: [ '/organizations-by-id/:id', - '/organizations-by-id/:id/*splat', + '/organizations-by-id/:id/(.*)', '/organizations-by-slug/:slug', - '/organizations-by-slug/:id/*splat', + '/organizations-by-slug/:id/(.*)', ], }, appRequestPath: '/organizations-by-slug/bcorp', @@ -1094,9 +1094,9 @@ test.describe('Client handshake with organization activation @nextjs', () => { orgSyncOptions: { organizationPatterns: [ '/organizations-by-slug/:slug', - '/organizations-by-slug/:id/*splat', + '/organizations-by-slug/:id/(.*)', '/organizations-by-id/:id', - '/organizations-by-id/:id/*splat', + '/organizations-by-id/:id/(.*)', ], }, appRequestPath: '/organizations-by-slug/bcorp/settings', @@ -1115,7 +1115,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { initialAuthState: 'expired', initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], + organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/(.*)'], }, appRequestPath: '/organizations-by-id/org_b/', tokenAppearsIn: 'cookie', @@ -1135,11 +1135,11 @@ test.describe('Client handshake with organization activation @nextjs', () => { orgSyncOptions: { organizationPatterns: [ '/organizations-by-id/:id', - '/organizations-by-id/:id/*splat', + '/organizations-by-id/:id/(.*)', '/organizations-by-slug/:slug', - '/organizations-by-slug/:id/*splat', + '/organizations-by-slug/:id/(.*)', ], - personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/*splat'], + personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/(.*)'], }, appRequestPath: '/personal-workspace', tokenAppearsIn: 'cookie', @@ -1159,8 +1159,8 @@ test.describe('Client handshake with organization activation @nextjs', () => { // Intentionally empty ]), orgSyncOptions: { - organizationPatterns: ['/organizations-by-slug/:slug', '/organizations-by-slug/:id/*splat'], - personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/*splat'], + organizationPatterns: ['/organizations-by-slug/:slug', '/organizations-by-slug/:id/(.*)'], + personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/(.*)'], }, appRequestPath: '/personal-workspace', tokenAppearsIn: 'cookie', @@ -1176,8 +1176,8 @@ test.describe('Client handshake with organization activation @nextjs', () => { initialAuthState: 'active', initialSessionClaims: new Map([['org_id', 'org_a']]), orgSyncOptions: { - organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], - personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/*splat'], + organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/(.*)'], + personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/(.*)'], }, appRequestPath: '/organizations-by-id/org_a', tokenAppearsIn: 'cookie', @@ -1264,7 +1264,8 @@ test.describe('Client handshake with organization activation @nextjs', () => { } }); -test.describe('Client handshake with an organization activation avoids infinite loops @nextjs', () => { +// TODO(izaak): revert test.describe('Client handshake with an organization activation avoids infinite loops @nextjs', () => { +test.describe('Client handshake with an organization activation avoids infinite loops', () => { test.describe.configure({ mode: 'parallel' }); const devBrowserCookie = '__clerk_db_jwt=needstobeset;'; @@ -1288,8 +1289,8 @@ test.describe('Client handshake with an organization activation avoids infinite await new Promise(resolve => jwksServer.listen(0, resolve)); thisApp = await start({ - organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/*splat'], - personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/*splat'], + organizationPatterns: ['/organizations-by-id/:id', '/organizations-by-id/:id/(.*)'], + personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/(.*)'], }); }); diff --git a/packages/backend/src/tokens/__tests__/request.test.ts b/packages/backend/src/tokens/__tests__/request.test.ts index 00b81faa54b..1f76777a45e 100644 --- a/packages/backend/src/tokens/__tests__/request.test.ts +++ b/packages/backend/src/tokens/__tests__/request.test.ts @@ -13,8 +13,8 @@ import { import runtime from '../../runtime'; import { jsonOk } from '../../util/testUtils'; import { AuthErrorReason, type AuthReason, AuthStatus, type RequestState } from '../authStatus'; -import { authenticateRequest, getActivationEntity } from '../request'; -import type { AuthenticateRequestOptions } from '../types'; +import { authenticateRequest, getOrganizationSyncTarget, type OrganizationSyncTarget } from '../request'; +import type { AuthenticateRequestOptions, OrganizationSyncOptions } from '../types'; const PK_TEST = 'pk_test_Y2xlcmsuaW5zcGlyZWQucHVtYS03NC5sY2wuZGV2JA'; const PK_LIVE = 'pk_live_Y2xlcmsuaW5zcGlyZWQucHVtYS03NC5sY2wuZGV2JA'; @@ -165,15 +165,146 @@ export default (QUnit: QUnit) => { return mockRequest({ cookie: cookieStr, ...headers }, requestUrl); }; + // Tests both getActivationEntity and the organizationSyncOptions usage patterns + // that are recommended for typical use. module('tokens.getActivationEntity(url,options)', _ => { - test('does anything', async assert => { - const path = new URL('http://localhost:3000/orgs/foo'); - const toActivate = getActivationEntity(path, { - organizationPatterns: ['/orgs/:id'], + type testCase = { + name: string; + // When the customer app specifies these orgSyncOptions to middleware... + whenOrgSyncOptions: OrganizationSyncOptions | undefined; + // And the path arrives at this URL path... + whenAppRequestPath: string; + // A handshake should (or should not) occur: + thenExpectActivationEntity: OrganizationSyncTarget | null; + }; + + const testCases: testCase[] = [ + { + name: 'none activates nothing', + whenOrgSyncOptions: undefined, + whenAppRequestPath: '/orgs/org_foo', + thenExpectActivationEntity: null, + }, + { + name: 'Can activate an org by ID (basic)', + whenOrgSyncOptions: { + organizationPatterns: ['/orgs/:id'], + }, + whenAppRequestPath: '/orgs/org_foo', + thenExpectActivationEntity: { + type: 'organization', + organizationId: 'org_foo', + }, + }, + { + name: 'mimatch activates nothing', + whenOrgSyncOptions: { + organizationPatterns: ['/orgs/:id'], + }, + whenAppRequestPath: '/personal-workspace/my-resource', + thenExpectActivationEntity: null, + }, + { + name: 'Can activate an org by ID (recommended matchers)', + whenOrgSyncOptions: { + organizationPatterns: ['/orgs/:id', '/orgs/:id/', '/orgs/:id/(.*)'], + }, + whenAppRequestPath: '/orgs/org_foo', + thenExpectActivationEntity: { + type: 'organization', + organizationId: 'org_foo', + }, + }, + { + name: 'Can activate an org by ID with a trailing slash', + whenOrgSyncOptions: { + organizationPatterns: ['/orgs/:id', '/orgs/:id/', '/orgs/:id/(.*)'], + }, + whenAppRequestPath: '/orgs/org_foo/', + thenExpectActivationEntity: { + type: 'organization', + organizationId: 'org_foo', + }, + }, + { + name: 'Can activate an org by ID with trailing path components', + whenOrgSyncOptions: { + organizationPatterns: ['/orgs/:id', '/orgs/:id/', '/orgs/:id/(.*)'], + }, + whenAppRequestPath: '/orgs/org_foo/nested-resource', + thenExpectActivationEntity: { + type: 'organization', + organizationId: 'org_foo', + }, + }, + { + name: 'Can activate an org by slug', + whenOrgSyncOptions: { + organizationPatterns: ['/orgs/:slug'], + }, + whenAppRequestPath: '/orgs/my-org', + thenExpectActivationEntity: { + type: 'organization', + organizationSlug: 'my-org', + }, + }, + { + name: 'Can activate the personal workspace', + whenOrgSyncOptions: { + personalWorkspacePatterns: ['/personal-workspace'], + }, + whenAppRequestPath: '/personal-workspace', + thenExpectActivationEntity: { + type: 'personalWorkspace', + }, + }, + { + name: 'ID match precedes slug match', + whenOrgSyncOptions: { + organizationPatterns: ['/orgs/:id', '/orgs/:slug'], // bad practice + }, + whenAppRequestPath: '/orgs/my-org', + thenExpectActivationEntity: { + type: 'organization', + organizationId: 'my-org', + }, + }, + { + name: 'personal workspace match precedes org match', + whenOrgSyncOptions: { + organizationPatterns: ['/personal-workspace'], // bad practice + personalWorkspacePatterns: ['/personal-workspace'], + }, + whenAppRequestPath: '/personal-workspace', + thenExpectActivationEntity: { + type: 'personalWorkspace', + }, + }, + { + name: 'All of the config at once', + whenOrgSyncOptions: { + organizationPatterns: [ + '/orgs-by-id/:id', + '/orgs-by-id/:id/(.*)', + '/orgs-by-slug/:slug', + '/orgs-by-slug/:slug/(.*)', + ], + personalWorkspacePatterns: ['/personal-workspace', '/personal-workspace/(.*)'], + }, + whenAppRequestPath: '/orgs-by-slug/org_bar/sub-resource', + thenExpectActivationEntity: { + type: 'organization', + organizationSlug: 'org_bar', + }, + }, + ]; + + testCases.forEach(testCase => { + test(testCase.name, assert => { + const path = new URL(`http://localhost:3000${testCase.whenAppRequestPath}`); + const toActivate = getOrganizationSyncTarget(path, testCase.whenOrgSyncOptions); + assert.deepEqual(toActivate, testCase.thenExpectActivationEntity); }); - console.log('Here izaak', toActivate); - - assert.equal(toActivate?.organizationId, 'foo'); }); }); diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index 76978355385..f55114cd10d 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -119,7 +119,7 @@ export async function authenticateRequest( url.searchParams.append(constants.QueryParameters.DevBrowser, authenticateContext.devBrowserToken); } - const toActivate = getActivationEntity(requestURL, options.organizationSyncOptions); + const toActivate = getOrganizationSyncTarget(requestURL, options.organizationSyncOptions); if (toActivate) { const params = getOrganizationSyncQueryParams(toActivate); @@ -272,7 +272,7 @@ ${error.getFullMessage()}`, authenticateContext: AuthenticateContext, auth: SignedInAuthObject, ): HandshakeState | SignedOutState | null { - const toActivate = getActivationEntity(authenticateContext.clerkUrl, options.organizationSyncOptions); + const toActivate = getOrganizationSyncTarget(authenticateContext.clerkUrl, options.organizationSyncOptions); if (!toActivate) { return null; } @@ -565,9 +565,9 @@ export const debugRequestState = (params: RequestState) => { /* * Determines if the given URL and settings indicate a desire to activate a specific organization or personal workspace. - * @example test + * @example todo(izaak) */ -export function getActivationEntity( +export function getOrganizationSyncTarget( url: URL, options: OrganizationSyncOptions | undefined, ): OrganizationSyncTarget | null { From 618ac481a2534d8435f09cd1da565a9b269776e1 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 20 Sep 2024 08:28:53 -0400 Subject: [PATCH 28/39] jsdoc comment everything --- .../src/tokens/__tests__/request.test.ts | 34 ++++++++- packages/backend/src/tokens/request.ts | 33 ++++++--- packages/backend/src/tokens/types.ts | 74 ++++++++++++------- 3 files changed, 105 insertions(+), 36 deletions(-) diff --git a/packages/backend/src/tokens/__tests__/request.test.ts b/packages/backend/src/tokens/__tests__/request.test.ts index 1f76777a45e..cf9a556eb1a 100644 --- a/packages/backend/src/tokens/__tests__/request.test.ts +++ b/packages/backend/src/tokens/__tests__/request.test.ts @@ -227,7 +227,7 @@ export default (QUnit: QUnit) => { }, }, { - name: 'Can activate an org by ID with trailing path components', + name: 'Can activate an org by ID with a trailing path component', whenOrgSyncOptions: { organizationPatterns: ['/orgs/:id', '/orgs/:id/', '/orgs/:id/(.*)'], }, @@ -237,6 +237,28 @@ export default (QUnit: QUnit) => { organizationId: 'org_foo', }, }, + { + name: 'Can activate an org by ID with many trailing path component', + whenOrgSyncOptions: { + organizationPatterns: ['/orgs/:id/(.*)'], + }, + whenAppRequestPath: '/orgs/org_foo/nested-resource/and/more/deeply/nested/resources', + thenExpectActivationEntity: { + type: 'organization', + organizationId: 'org_foo', + }, + }, + { + name: 'Can activate an org by ID with an unrelated path token in the prefix', + whenOrgSyncOptions: { + organizationPatterns: ['/unknown-thing/:any/orgs/:id'], + }, + whenAppRequestPath: '/unknown-thing/thing/orgs/org_foo', + thenExpectActivationEntity: { + type: 'organization', + organizationId: 'org_foo', + }, + }, { name: 'Can activate an org by slug', whenOrgSyncOptions: { @@ -280,6 +302,16 @@ export default (QUnit: QUnit) => { type: 'personalWorkspace', }, }, + { + name: 'personal workspace may contain path tokens', + whenOrgSyncOptions: { + personalWorkspacePatterns: ['/user/:any', '/user/:any/(.*)'], + }, + whenAppRequestPath: '/user/123/home', + thenExpectActivationEntity: { + type: 'personalWorkspace', + }, + }, { name: 'All of the config at once', whenOrgSyncOptions: { diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index f55114cd10d..54a8f49fdc4 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -264,10 +264,15 @@ ${error.getFullMessage()}`, return signedOut(authenticateContext, reason, message); } - // handleMaybeOrganizationSyncHandshake determines if a handshake must occur to resolve a mismatch between - // the organization as specified by the URL (according to the options) and the actual active organization - // on the session. - // NOTE(izaak): I don't love that null return implies signed-in... + /** + * Determines if a handshake must occur to resolve a mismatch between the organization as specified + * by the URL (according to the options) and the actual active organization on the session. + * + * @returns {HandshakeState | SignedOutState | null} - The function can return the following: + * - {HandshakeState}: If a handshake is needed to resolve the mismatched organization. + * - {SignedOutState}: If a handshake is required but cannot be performed. + * - {null}: If no action is required. + */ function handleMaybeOrganizationSyncHandshake( authenticateContext: AuthenticateContext, auth: SignedInAuthObject, @@ -563,9 +568,12 @@ export const debugRequestState = (params: RequestState) => { return { isSignedIn, proxyUrl, reason, message, publishableKey, isSatellite, domain }; }; -/* - * Determines if the given URL and settings indicate a desire to activate a specific organization or personal workspace. - * @example todo(izaak) +/** + * Determines if the given URL and settings indicate a desire to activate a specific + * organization or personal workspace. + * + * @returns {OrganizationSyncTarget | null} - The function can return the following: + * - {OrganizationSyncTarget}: If the URL and settings indicate a desire to activate an organization or personal workspace. */ export function getOrganizationSyncTarget( url: URL, @@ -634,13 +642,18 @@ export function getOrganizationSyncTarget( return null; } -// OrganizationSyncTarget is an entity that can be activated by the handshake API. +/** + * Represents an organization or a personal workspace - e.g. an + * entity that can be activated by the handshake API. + */ export type OrganizationSyncTarget = | { type: 'personalWorkspace' } | { type: 'organization'; organizationId?: string; organizationSlug?: string }; -// getOrganizationSyncQueryParams takes an activatibatle entity (organization or personal workspace) -// and generates the query parameters that FAPI expects on the handshake API to activate that entity. +/** + * Generates the query parameters to activate an organization or personal workspace + * via the FAPI handshake api. + */ function getOrganizationSyncQueryParams(toActivate: OrganizationSyncTarget): Map { const ret = new Map(); if (toActivate.type === 'personalWorkspace') { diff --git a/packages/backend/src/tokens/types.ts b/packages/backend/src/tokens/types.ts index 81866a073c3..c115204b3a6 100644 --- a/packages/backend/src/tokens/types.ts +++ b/packages/backend/src/tokens/types.ts @@ -14,32 +14,56 @@ export type AuthenticateRequestOptions = { apiClient?: ApiClient; } & VerifyTokenOptions; -// OrganizationSyncOptions define the options for syncing an organization -// or personal workspace state from the URL to the clerk session +/** + * Defines the options for syncing an organization or personal workspace state from the URL to the clerk session. + * Useful if the application requires the inclusion of a URL that indicates that a given clerk organization + * (or personal workspace) must be active on the clerk session. + * + * If a mismatch between the active organization on the session and the organization as indicated by the URL is + * detected, an attempt to activate the given organization will be made. + * + * WARNING: If the activation cannot be performed, either because an organization does not exist or the user lacks + * access, then the active organization on the session will not be changed (and a warning will be logged). It is + * ultimately the responsibility of the page to verify that the resources are appropriate to render given the URL, + * and handle mismatches appropriately (e.g. by returning a 404). + */ export type OrganizationSyncOptions = { - // organizationPattern defines the URL patterns that are organization-specific and contains - // an organization ID or slug as a path token. If a request arrives to the application at - // a URL matching this path, the organization identifier will be extracted and activated - // before rendering. - // If no organization with the given identifier can be activated, - // either because it does not exist or the user does not have access to it, organization-related - // fields will be set to null, and the server component must detect this case and respond with - // an appropriate error (e.g. notFound()). - // If the route also matches with the personalWorkspacePattern, the personalWorkspacePattern - // takes precedence. - // - // Must have a group named either ":id", which matches to a clerk organization id, - // or ":slug", which matches to a clerk organization slug. - // Examples: "/orgs/:slug+*", "/orgs/:id+*" - organizationPatterns?: Array, + /** + * URL patterns that are organization-specific and contain an organization ID or slug as a path token. + * If a request matches this path, the organization identifier will be extracted and activated before rendering. + * + * WARNING: If the organization cannot be activated either because it does not exist or the user lacks access, + * organization-related fields will be set to null. The server component must detect this and respond + * with an appropriate error (e.g., notFound()). + * + * If the route also matches the personalWorkspacePatterns, the personalWorkspacePattern takes precedence. + * + * Must have a path token named either ":id" (matches a clerk organization ID) or ":slug" (matches a clerk + * organization slug). + * + * Common examples: + * - ["/orgs/:slug", "/orgs/:slug/(.*)"] + * - ["/orgs/:id", "/orgs/:id/(.*)"] + * - ["/app/:any/orgs/:slug", "/app/:any/orgs/:slug/(.*)"] + */ + organizationPatterns?: Array; - // personalWorkspacePattern defines the URL pattern for resources that exist in the context - // of a clerk personal workspace (user-specific, outside any other organization). - // If the route also matches with the organizationPattern, this takes precedence - personalWorkspacePatterns?: Array, -} + /** + * URL patterns for resources in the context of a clerk personal workspace (user-specific, outside any organization). + * If the route also matches the organizationPattern, this takes precedence. + * + * Common examples: + * - ["/user", "/user/(.*)"] + * - ["/user/:any", "/user/:any/(.*)"] + */ + personalWorkspacePatterns?: Array; +}; -// Pattern is a path-to-regexp style matcher -// Syntax: https://www.npmjs.com/package/path-to-regexp -// Examples: "/orgs/:slug", "/orgs/:id", "/personal-workspace" +/** + * A pattern representing the structure of a URL path. + * In addition to a valid URL, may include: + * - Named path tokens prefixed with a colon (e.g., ":id", ":slug", ":any") + * - Wildcard token (e.g., ".(*)"), which will match the remainder of the path + * Examples: "/orgs/:slug", "/app/:any/orgs/:id", "/personal-workspace/(.*)" + */ type Pattern = string; From c2028328caaf9cc198ea36a14aa42112ebf55df5 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 20 Sep 2024 09:13:27 -0400 Subject: [PATCH 29/39] Throwing errors that are likely to be encountered during development Rather than logging them. --- packages/backend/src/tokens/request.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index 40121561fbe..f3f459fcd40 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -653,8 +653,8 @@ export function getOrganizationSyncTarget( try { matcher = match(options.personalWorkspacePatterns); } catch (e) { - console.error(`Invalid personal workspace pattern "${options.personalWorkspacePatterns}": "${e}"`); - return null; + // Likely to be encountered during development, so throwing the error is more prudent than logging + throw new Error(`Invalid personal workspace pattern "${options.personalWorkspacePatterns}": "${e}"`); } let personalResult: Match>>; @@ -677,8 +677,8 @@ export function getOrganizationSyncTarget( try { matcher = match(options.organizationPatterns); } catch (e) { - console.error(`Clerk: Invalid organization pattern "${options.organizationPatterns}": "${e}"`); - return null; + // Likely to be encountered during development, so throwing the error is more prudent than logging + throw new Error(`Clerk: Invalid organization pattern "${options.organizationPatterns}": "${e}"`); } let orgResult: Match>>; From 6c8f99f02a9f6285e89e7f133bf39499441509ec Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 20 Sep 2024 09:18:15 -0400 Subject: [PATCH 30/39] Making fewer type assumptions Thanks @LauraBeatris! https://github.com/pillarjs/path-to-regexp/blob/df39d6c30b2a8bdc9b8bda186f658cc89d47e44c/src/index.ts#discussion_r1761524255 Co-authored-by: Laura Beatris <48022589+LauraBeatris@users.noreply.github.com> --- packages/backend/src/tokens/request.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index f3f459fcd40..6f773ac42e3 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -690,13 +690,14 @@ export function getOrganizationSyncTarget( return null; } - if (orgResult) { - const { slug = '', id = '' } = orgResult.params as { slug?: string; id?: string }; - if (id) { - return { type: 'organization', organizationId: id }; + if (orgResult && 'params' in orgResult) { + const params = orgResult.params; + + if ('id' in params && typeof params.id === 'string') { + return { type: 'organization', organizationId: params.id }; } - if (slug) { - return { type: 'organization', organizationSlug: slug }; + if ('slug' in params && typeof params.slug === 'string') { + return { type: 'organization', organizationSlug: params.slug }; } console.warn( 'Clerk: Detected an organization pattern match, but no organization ID or slug was found in the URL. Does the pattern include `:id` or `:slug`?', From 3d2dd3bcfeece42c1ed512e3ce7659d9cad505f5 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 20 Sep 2024 09:36:12 -0400 Subject: [PATCH 31/39] Small test rearrangement --- integration/testUtils/handshake.ts | 6 ++++-- integration/tests/handshake.test.ts | 17 ++++++++--------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/integration/testUtils/handshake.ts b/integration/testUtils/handshake.ts index e6dbdbc8b33..74c4cea2026 100644 --- a/integration/testUtils/handshake.ts +++ b/integration/testUtils/handshake.ts @@ -129,8 +129,10 @@ export function generateConfig({ mode, matchedKeys = true }: { mode: 'test' | 'l } // Merge claims with extraClaims - for (const [key, value] of extraClaims) { - claims[key] = value; + if (extraClaims) { + for (const [key, value] of extraClaims) { + claims[key] = value; + } } return { diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index f23f70df3a2..e6e5e2b09a5 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -1187,17 +1187,13 @@ test.describe('Client handshake with organization activation @nextjs', () => { fapiOrganizationIdParamValue: null, }, }, - - // ---------------- Invalid permutation tests ---------------- { // NOTE(izaak): Would we prefer 500ing in this case? - name: 'Invalid config => ignore it and return 200', + name: 'No config => nothing to activate, return 200', when: { initialAuthState: 'active', initialSessionClaims: new Map([['org_id', 'org_a']]), - orgSyncOptions: { - organizationPatterns: ['i am not valid config'], - }, + orgSyncOptions: null, appRequestPath: '/organizations-by-id/org_a', tokenAppearsIn: 'cookie', }, @@ -1206,13 +1202,16 @@ test.describe('Client handshake with organization activation @nextjs', () => { fapiOrganizationIdParamValue: null, }, }, + + // ---------------- Invalid permutation tests ---------------- { - // NOTE(izaak): Would we prefer 500ing in this case? - name: 'No config => nothing to activate, return 200', + name: 'Invalid config => ignore it and return 200', when: { initialAuthState: 'active', initialSessionClaims: new Map([['org_id', 'org_a']]), - orgSyncOptions: null, + orgSyncOptions: { + organizationPatterns: ['i am not valid config'], + }, appRequestPath: '/organizations-by-id/org_a', tokenAppearsIn: 'cookie', }, From 1539d2c73f35631f60e5967b346271b761bc7afe Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 20 Sep 2024 09:43:10 -0400 Subject: [PATCH 32/39] Removing the path-to-regexp dependency (now that we're using the internal version) --- package-lock.json | 10 ---------- package.json | 1 - 2 files changed, 11 deletions(-) diff --git a/package-lock.json b/package-lock.json index 54bc636699c..31238480ce3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -48,7 +48,6 @@ "jest-chrome": "^0.8.0", "jest-environment-jsdom": "^29.3.1", "lint-staged": "^14.0.1", - "path-to-regexp": "^8.1.0", "prettier": "^3.3.2", "prettier-plugin-tailwindcss": "^0.6.3", "publint": "^0.2.4", @@ -31095,15 +31094,6 @@ "node": "14 || >=16.14" } }, - "node_modules/path-to-regexp": { - "version": "8.1.0", - "resolved": "https://registry.npmjs.org/path-to-regexp/-/path-to-regexp-8.1.0.tgz", - "integrity": "sha512-Bqn3vc8CMHty6zuD+tG23s6v2kwxslHEhTj4eYaVKGIEB+YX/2wd0/rgXLFD9G9id9KCtbVy/3ZgmvZjpa0UdQ==", - "dev": true, - "engines": { - "node": ">=16" - } - }, "node_modules/path-type": { "version": "4.0.0", "license": "MIT", diff --git a/package.json b/package.json index 54a8eac4689..5d86e1126b7 100644 --- a/package.json +++ b/package.json @@ -92,7 +92,6 @@ "jest-chrome": "^0.8.0", "jest-environment-jsdom": "^29.3.1", "lint-staged": "^14.0.1", - "path-to-regexp": "^8.1.0", "prettier": "^3.3.2", "prettier-plugin-tailwindcss": "^0.6.3", "publint": "^0.2.4", From 3fef10d6ce7ca2c1452c81e8faeefc040155ece3 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 20 Sep 2024 09:45:05 -0400 Subject: [PATCH 33/39] Updating names in comments --- packages/backend/src/tokens/__tests__/request.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/backend/src/tokens/__tests__/request.test.ts b/packages/backend/src/tokens/__tests__/request.test.ts index cf9a556eb1a..f4c15ef76e8 100644 --- a/packages/backend/src/tokens/__tests__/request.test.ts +++ b/packages/backend/src/tokens/__tests__/request.test.ts @@ -165,9 +165,9 @@ export default (QUnit: QUnit) => { return mockRequest({ cookie: cookieStr, ...headers }, requestUrl); }; - // Tests both getActivationEntity and the organizationSyncOptions usage patterns + // Tests both getOrganizationSyncTarget and the organizationSyncOptions usage patterns // that are recommended for typical use. - module('tokens.getActivationEntity(url,options)', _ => { + module('tokens.getOrganizationSyncTarget(url,options)', _ => { type testCase = { name: string; // When the customer app specifies these orgSyncOptions to middleware... From 59f1c15847925288b1caceb78029653f99b79c49 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 20 Sep 2024 09:47:50 -0400 Subject: [PATCH 34/39] Removing unnecesary awaits --- packages/backend/src/tokens/request.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index 6f773ac42e3..6b0d67d9877 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -376,8 +376,7 @@ ${error.getFullMessage()}`, if (errors) { throw errors[0]; } - // use `await` to force this try/catch handle the signedIn invocation - const signedInRequestState = await signedIn(authenticateContext, data, undefined, sessionTokenInHeader!); + const signedInRequestState = signedIn(authenticateContext, data, undefined, sessionTokenInHeader!); // Org sync if necessary const handshakeRequestState = handleMaybeOrganizationSyncHandshake( authenticateContext, @@ -547,8 +546,7 @@ ${error.getFullMessage()}`, if (errors) { throw errors[0]; } - // use `await` to force this try/catch handle the signedIn invocation - const signedInRequestState = await signedIn( + const signedInRequestState = signedIn( authenticateContext, data, undefined, From c8aad63bcf1284dc6aaa49abebf045eb6d1074da Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 20 Sep 2024 09:49:17 -0400 Subject: [PATCH 35/39] Cleanining up vestages of a merge conflict --- packages/nextjs/src/server/clerkMiddleware.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/nextjs/src/server/clerkMiddleware.ts b/packages/nextjs/src/server/clerkMiddleware.ts index 01e1489739f..592064c8a34 100644 --- a/packages/nextjs/src/server/clerkMiddleware.ts +++ b/packages/nextjs/src/server/clerkMiddleware.ts @@ -210,7 +210,6 @@ export const createAuthenticateRequestOptions = ( clerkRequest: ClerkRequest, options: ClerkMiddlewareOptions, ): Parameters[1] => { -// >>>>>>> main return { ...options, ...handleMultiDomainAndProxy(clerkRequest, options), From c996bdaeed1b24d2925f422672fefe4205aabb3e Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 20 Sep 2024 09:55:12 -0400 Subject: [PATCH 36/39] Changeset --- .changeset/olive-trainers-heal.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/olive-trainers-heal.md diff --git a/.changeset/olive-trainers-heal.md b/.changeset/olive-trainers-heal.md new file mode 100644 index 00000000000..ff1aa1bb149 --- /dev/null +++ b/.changeset/olive-trainers-heal.md @@ -0,0 +1,5 @@ +--- +"@clerk/nextjs": patch +--- + +New nextjs middleware option `OrganizationSyncOptions`, which syncs an active organization or personal workspace from a URL to the clerk session. From 0b93ac1b1393f436b923138531a5f7c3fc96af4d Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Fri, 20 Sep 2024 11:05:43 -0400 Subject: [PATCH 37/39] Update .changeset/olive-trainers-heal.md Co-authored-by: Laura Beatris <48022589+LauraBeatris@users.noreply.github.com> --- .changeset/olive-trainers-heal.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/olive-trainers-heal.md b/.changeset/olive-trainers-heal.md index ff1aa1bb149..9c78342af7e 100644 --- a/.changeset/olive-trainers-heal.md +++ b/.changeset/olive-trainers-heal.md @@ -2,4 +2,4 @@ "@clerk/nextjs": patch --- -New nextjs middleware option `OrganizationSyncOptions`, which syncs an active organization or personal workspace from a URL to the clerk session. +Introduces `organizationSyncOptions` option to `clerkMiddleware`, which syncs an active organization or personal workspace from a URL to the Clerk session. From 2aeda05d78d0a00d284b090d846410f7ad44dd82 Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Mon, 23 Sep 2024 17:47:47 -0400 Subject: [PATCH 38/39] Not handshaking on header-based auth Header-based auth indicates a non-browser actor, which means they don't have the __client cookie and a handshake won't be useful. --- integration/tests/handshake.test.ts | 51 +++++++++++++++++++++++--- packages/backend/src/tokens/request.ts | 9 +---- 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index e6e5e2b09a5..dd8954de155 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -973,6 +973,9 @@ test.describe('Client handshake with organization activation @nextjs', () => { // With a token specified in... tokenAppearsIn: 'header' | 'cookie'; + + // And the Sec-fetch-dest header is... + secFetchDestHeader: string | null; }; type then = { @@ -999,6 +1002,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { }, appRequestPath: '/organizations-by-id/org_a', tokenAppearsIn: 'cookie', + secFetchDestHeader: 'document', }, then: { expectStatus: 307, @@ -1017,6 +1021,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { }, appRequestPath: '/organizations-by-id/org_a', tokenAppearsIn: 'cookie', + secFetchDestHeader: 'document', }, then: { expectStatus: 307, @@ -1025,10 +1030,11 @@ test.describe('Client handshake with organization activation @nextjs', () => { }, // ---------------- Header-based auth tests ---------------- - // Note: it would be possible to run _every_ test with the token in the header or the cookies - // and expect the same results, but we're avoiding that to save some test execution time. + // Header-based auth requests come from non-browser actors, which don't have the __client cookie. + // Handshaking depends on a redirect that includes that __client cookie, so we should not handshake + // for this auth method, even if there's an org mismatch { - name: 'Header-based auth, active session, no org in session, but org a requested by ID => attempts to activate org A', + name: 'Header-based auth should not handshake with active auth', when: { initialAuthState: 'active', initialSessionClaims: new Map([ @@ -1039,10 +1045,30 @@ test.describe('Client handshake with organization activation @nextjs', () => { }, appRequestPath: '/organizations-by-id/org_a', tokenAppearsIn: 'header', + secFetchDestHeader: null, }, then: { - expectStatus: 307, - fapiOrganizationIdParamValue: 'org_a', + expectStatus: 200, + fapiOrganizationIdParamValue: null, + }, + }, + { + name: 'Header-based auth should not handshake with expired auth', + when: { + initialAuthState: 'expired', + initialSessionClaims: new Map([ + // Intentionally empty + ]), + orgSyncOptions: { + organizationPatterns: ['/organizations-by-id/:id'], + }, + appRequestPath: '/organizations-by-id/org_a', + tokenAppearsIn: 'header', + secFetchDestHeader: null, + }, + then: { + expectStatus: 307, // Should redirect to sign-in + fapiOrganizationIdParamValue: null, }, }, @@ -1057,6 +1083,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { }, appRequestPath: '/organizations-by-id/org_b', tokenAppearsIn: 'cookie', + secFetchDestHeader: 'document', }, then: { expectStatus: 307, @@ -1080,6 +1107,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { }, appRequestPath: '/organizations-by-slug/bcorp', tokenAppearsIn: 'cookie', + secFetchDestHeader: 'document', }, then: { expectStatus: 307, @@ -1101,6 +1129,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { }, appRequestPath: '/organizations-by-slug/bcorp/settings', tokenAppearsIn: 'cookie', + secFetchDestHeader: 'document', }, then: { expectStatus: 307, @@ -1119,6 +1148,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { }, appRequestPath: '/organizations-by-id/org_b/', tokenAppearsIn: 'cookie', + secFetchDestHeader: 'document', }, then: { expectStatus: 308, // Handshake never 308's - this points to `/organizations-by-id/org_b` (no trailing slash) @@ -1143,6 +1173,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { }, appRequestPath: '/personal-workspace', tokenAppearsIn: 'cookie', + secFetchDestHeader: 'document', }, then: { expectStatus: 307, @@ -1164,6 +1195,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { }, appRequestPath: '/personal-workspace', tokenAppearsIn: 'cookie', + secFetchDestHeader: 'document', }, then: { expectStatus: 200, @@ -1181,6 +1213,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { }, appRequestPath: '/organizations-by-id/org_a', tokenAppearsIn: 'cookie', + secFetchDestHeader: 'document', }, then: { expectStatus: 200, @@ -1196,6 +1229,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { orgSyncOptions: null, appRequestPath: '/organizations-by-id/org_a', tokenAppearsIn: 'cookie', + secFetchDestHeader: 'document', }, then: { expectStatus: 200, @@ -1214,6 +1248,7 @@ test.describe('Client handshake with organization activation @nextjs', () => { }, appRequestPath: '/organizations-by-id/org_a', tokenAppearsIn: 'cookie', + secFetchDestHeader: 'document', }, then: { expectStatus: 200, @@ -1238,8 +1273,12 @@ test.describe('Client handshake with organization activation @nextjs', () => { const headers = new Headers({ 'X-Publishable-Key': config.pk, 'X-Secret-Key': config.sk, - 'Sec-Fetch-Dest': 'document', }); + + if (testCase.when.secFetchDestHeader) { + headers.set('Sec-Fetch-Dest', testCase.when.secFetchDestHeader); + } + switch (testCase.when.tokenAppearsIn) { case 'cookie': headers.set('Cookie', `${devBrowserCookie} __client_uat=${claims.iat}; __session=${token}`); diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index 6b0d67d9877..c92a887bc2b 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -376,13 +376,8 @@ ${error.getFullMessage()}`, if (errors) { throw errors[0]; } - const signedInRequestState = signedIn(authenticateContext, data, undefined, sessionTokenInHeader!); - // Org sync if necessary - const handshakeRequestState = handleMaybeOrganizationSyncHandshake( - authenticateContext, - signedInRequestState.toAuth(), - ); - return handshakeRequestState || signedInRequestState; + // use `await` to force this try/catch handle the signedIn invocation + return signedIn(authenticateContext, data, undefined, sessionTokenInHeader!); } catch (err) { return handleError(err, 'header'); } From e62e239a5b93966d0055300618805431ee6dc34e Mon Sep 17 00:00:00 2001 From: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com> Date: Tue, 24 Sep 2024 14:53:42 -0400 Subject: [PATCH 39/39] Using a consistent clerkUrl --- packages/backend/src/tokens/request.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index c92a887bc2b..32aa9fcc029 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -109,11 +109,9 @@ export async function authenticateRequest( } function buildRedirectToHandshake({ - requestURL, handshakeReason, refreshError, }: { - requestURL: URL; handshakeReason: AuthErrorReason; refreshError: string; }) { @@ -130,7 +128,7 @@ export async function authenticateRequest( url.searchParams.append(constants.QueryParameters.DevBrowser, authenticateContext.devBrowserToken); } - const toActivate = getOrganizationSyncTarget(requestURL, options.organizationSyncOptions); + const toActivate = getOrganizationSyncTarget(authenticateContext.clerkUrl, options.organizationSyncOptions); if (toActivate) { const params = getOrganizationSyncQueryParams(toActivate); @@ -284,7 +282,6 @@ ${error.getFullMessage()}`, const handshakeHeaders = headers ?? buildRedirectToHandshake({ - requestURL: authenticateContext.clerkUrl, handshakeReason: reason, refreshError, });