From 434fc2ec5a94640d92b1a1aa17521ec1b1f03aec Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Fri, 22 Dec 2023 12:52:09 -0600 Subject: [PATCH 1/2] feat(clerk-sdk-node): Implement support for the handshake flow --- .changeset/funny-pots-brush.md | 5 ++ .../sdk-node/src/__tests__/middleware.test.ts | 48 +++++++++++++++++++ .../sdk-node/src/clerkExpressRequireAuth.ts | 11 +++-- packages/sdk-node/src/clerkExpressWithAuth.ts | 10 +++- 4 files changed, 69 insertions(+), 5 deletions(-) create mode 100644 .changeset/funny-pots-brush.md diff --git a/.changeset/funny-pots-brush.md b/.changeset/funny-pots-brush.md new file mode 100644 index 00000000000..152336e7a46 --- /dev/null +++ b/.changeset/funny-pots-brush.md @@ -0,0 +1,5 @@ +--- +'@clerk/clerk-sdk-node': patch +--- + +Integrate handshake handling into `ClerkExpressWithAuth()` and `ClerkExpressRequireWith()`. If the `authenticateRequest()` returns a redirect or is in a handshake state, the middlewares will properly handle this and respond accordingly. diff --git a/packages/sdk-node/src/__tests__/middleware.test.ts b/packages/sdk-node/src/__tests__/middleware.test.ts index 74d7f7ca6ca..d4fe5973154 100644 --- a/packages/sdk-node/src/__tests__/middleware.test.ts +++ b/packages/sdk-node/src/__tests__/middleware.test.ts @@ -25,6 +25,7 @@ describe('ClerkExpressWithAuth', () => { const clerkClient = mockClerkClient() as any; clerkClient.authenticateRequest.mockReturnValue({ toAuth: () => ({ sessionId: null }), + headers: new Headers(), } as RequestState); await createClerkExpressWithAuth({ clerkClient })()(req, res, mockNext as NextFunction); @@ -40,12 +41,35 @@ describe('ClerkExpressWithAuth', () => { const clerkClient = mockClerkClient() as any; clerkClient.authenticateRequest.mockReturnValue({ toAuth: () => ({ sessionId: '1' }), + headers: new Headers(), } as RequestState); await createClerkExpressWithAuth({ clerkClient })()(req, res, mockNext as NextFunction); expect((req as WithAuthProp).auth.sessionId).toEqual('1'); expect(mockNext).toHaveBeenCalledWith(); }); + + it('should redirect if a Location header is returned', async () => { + const req = createRequest(); + const res = { + status: jest.fn(() => res), + set: jest.fn(() => res), + end: jest.fn(), + getHeader: jest.fn(), + } as unknown as Response; + + const headers = new Headers({ Location: 'https://clerk.example.com/v1/handshake' }); + + const clerkClient = mockClerkClient() as any; + clerkClient.authenticateRequest.mockReturnValue({ + toAuth: () => ({ sessionId: '1' }), + headers: new Headers({ Location: 'https://clerk.example.com/v1/handshake' }), + } as RequestState); + + await createClerkExpressWithAuth({ clerkClient })()(req, res, mockNext as NextFunction); + expect(res.status).toHaveBeenCalledWith(307); + expect(res.set).toHaveBeenCalledWith(headers); + }); }); describe('ClerkExpressRequireAuth', () => { @@ -56,6 +80,7 @@ describe('ClerkExpressRequireAuth', () => { const clerkClient = mockClerkClient() as any; clerkClient.authenticateRequest.mockReturnValue({ toAuth: () => ({ sessionId: null }), + headers: new Headers(), } as RequestState); await createClerkExpressRequireAuth({ clerkClient })()(req, res, mockNext as NextFunction); @@ -72,10 +97,33 @@ describe('ClerkExpressRequireAuth', () => { clerkClient.authenticateRequest.mockReturnValue({ isSignedIn: true, toAuth: () => ({ sessionId: '1' }), + headers: new Headers(), } as RequestState); await createClerkExpressRequireAuth({ clerkClient })()(req, res, mockNext as NextFunction); expect((req as WithAuthProp).auth.sessionId).toEqual('1'); expect(mockNext).toHaveBeenCalledWith(); }); + + it('should redirect if a Location header is returned', async () => { + const req = createRequest(); + const res = { + status: jest.fn(() => res), + set: jest.fn(() => res), + end: jest.fn(), + getHeader: jest.fn(), + } as unknown as Response; + + const headers = new Headers({ Location: 'https://clerk.example.com/v1/handshake' }); + + const clerkClient = mockClerkClient() as any; + clerkClient.authenticateRequest.mockReturnValue({ + toAuth: () => ({ sessionId: '1' }), + headers: new Headers({ Location: 'https://clerk.example.com/v1/handshake' }), + } as RequestState); + + await createClerkExpressRequireAuth({ clerkClient })()(req, res, mockNext as NextFunction); + expect(res.status).toHaveBeenCalledWith(307); + expect(res.set).toHaveBeenCalledWith(headers); + }); }); diff --git a/packages/sdk-node/src/clerkExpressRequireAuth.ts b/packages/sdk-node/src/clerkExpressRequireAuth.ts index fa9880ab199..fa03bc1e01c 100644 --- a/packages/sdk-node/src/clerkExpressRequireAuth.ts +++ b/packages/sdk-node/src/clerkExpressRequireAuth.ts @@ -25,12 +25,17 @@ export const createClerkExpressRequireAuth = (createOpts: CreateClerkExpressMidd }); decorateResponseWithObservabilityHeaders(res, requestState); - if (requestState.status === AuthStatus.Handshake) { - // TODO: Handle handshake - // This needs to be refactored and reused by clerkExpressWithAuth as well + const hasLocationHeader = requestState.headers.get('location'); + if (hasLocationHeader) { + // triggering a handshake redirect + res.status(307).set(requestState.headers).end(); return; } + if (requestState.status === AuthStatus.Handshake) { + next(new Error('Clerk: unexpected handshake without redirect')); + } + if (requestState.isSignedIn) { (req as RequireAuthProp).auth = { ...requestState.toAuth(), claims: requestState.toAuth().sessionClaims }; next(); diff --git a/packages/sdk-node/src/clerkExpressWithAuth.ts b/packages/sdk-node/src/clerkExpressWithAuth.ts index bf3ce796e99..bce16affcc2 100644 --- a/packages/sdk-node/src/clerkExpressWithAuth.ts +++ b/packages/sdk-node/src/clerkExpressWithAuth.ts @@ -17,9 +17,15 @@ export const createClerkExpressWithAuth = (createOpts: CreateClerkExpressMiddlew }); decorateResponseWithObservabilityHeaders(res, requestState); + const hasLocationHeader = requestState.headers.get('location'); + if (hasLocationHeader) { + // triggering a handshake redirect + res.status(307).set(requestState.headers).end(); + return; + } + if (requestState.status === AuthStatus.Handshake) { - // TODO: Handle handshake - // This needs to be refactored and reused by clerkExpressRequireAuth as well + next(new Error('Clerk: unexpected handshake without redirect')); return; } From 393f2aed99072b871a1b2b82cc9ae2cae05d7bd7 Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Fri, 22 Dec 2023 14:29:44 -0600 Subject: [PATCH 2/2] feat(clerk-sdk-node): Refactor implementation to fix setting headers --- .../sdk-node/src/__tests__/middleware.test.ts | 14 +++++++--- packages/sdk-node/src/authenticateRequest.ts | 28 ++++++++++++++++++- .../sdk-node/src/clerkExpressRequireAuth.ts | 20 ++++++------- packages/sdk-node/src/clerkExpressWithAuth.ts | 24 ++++++++-------- 4 files changed, 58 insertions(+), 28 deletions(-) diff --git a/packages/sdk-node/src/__tests__/middleware.test.ts b/packages/sdk-node/src/__tests__/middleware.test.ts index d4fe5973154..38593bcbc8f 100644 --- a/packages/sdk-node/src/__tests__/middleware.test.ts +++ b/packages/sdk-node/src/__tests__/middleware.test.ts @@ -53,7 +53,7 @@ describe('ClerkExpressWithAuth', () => { const req = createRequest(); const res = { status: jest.fn(() => res), - set: jest.fn(() => res), + appendHeader: jest.fn(), end: jest.fn(), getHeader: jest.fn(), } as unknown as Response; @@ -68,7 +68,10 @@ describe('ClerkExpressWithAuth', () => { await createClerkExpressWithAuth({ clerkClient })()(req, res, mockNext as NextFunction); expect(res.status).toHaveBeenCalledWith(307); - expect(res.set).toHaveBeenCalledWith(headers); + + for (const [key, value] of headers.entries()) { + expect(res.appendHeader).toHaveBeenCalledWith(key, value); + } }); }); @@ -109,7 +112,7 @@ describe('ClerkExpressRequireAuth', () => { const req = createRequest(); const res = { status: jest.fn(() => res), - set: jest.fn(() => res), + appendHeader: jest.fn(), end: jest.fn(), getHeader: jest.fn(), } as unknown as Response; @@ -124,6 +127,9 @@ describe('ClerkExpressRequireAuth', () => { await createClerkExpressRequireAuth({ clerkClient })()(req, res, mockNext as NextFunction); expect(res.status).toHaveBeenCalledWith(307); - expect(res.set).toHaveBeenCalledWith(headers); + + for (const [key, value] of headers.entries()) { + expect(res.appendHeader).toHaveBeenCalledWith(key, value); + } }); }); diff --git a/packages/sdk-node/src/authenticateRequest.ts b/packages/sdk-node/src/authenticateRequest.ts index 04d346fb97f..66ca17cabe3 100644 --- a/packages/sdk-node/src/authenticateRequest.ts +++ b/packages/sdk-node/src/authenticateRequest.ts @@ -1,8 +1,9 @@ import type { RequestState } from '@clerk/backend/internal'; -import { constants, createClerkRequest } from '@clerk/backend/internal'; +import { AuthStatus, constants, createClerkRequest } from '@clerk/backend/internal'; import { handleValueOrFn } from '@clerk/shared/handleValueOrFn'; import { isDevelopmentFromSecretKey } from '@clerk/shared/keys'; import { isHttpOrHttps, isProxyUrlRelative, isValidProxyUrl } from '@clerk/shared/proxy'; +import type { Response } from 'express'; import type { IncomingMessage, ServerResponse } from 'http'; import type { AuthenticateRequestParams } from './types'; @@ -56,6 +57,31 @@ const incomingMessageToRequest = (req: IncomingMessage): Request => { }); }; +/** + * Depending on the auth state of the request, handles applying redirects and validating that a handshake state was properly handled. + * + * Returns an error if state is handshake without a redirect, otherwise returns undefined. res.writableEnded should be checked after this method is called. + */ +export const setResponseForHandshake = (requestState: RequestState, res: Response) => { + const hasLocationHeader = requestState.headers.get('location'); + if (hasLocationHeader) { + requestState.headers.forEach((value, key) => { + res.appendHeader(key, value); + }); + + // triggering a handshake redirect + res.status(307).end(); + + return; + } + + if (requestState.status === AuthStatus.Handshake) { + return new Error('Clerk: unexpected handshake without redirect'); + } + + return; +}; + // TODO: Move to backend export const decorateResponseWithObservabilityHeaders = (res: ServerResponse, requestState: RequestState) => { requestState.message && res.setHeader(constants.Headers.AuthMessage, encodeURIComponent(requestState.message)); diff --git a/packages/sdk-node/src/clerkExpressRequireAuth.ts b/packages/sdk-node/src/clerkExpressRequireAuth.ts index fa03bc1e01c..7d65ab2707e 100644 --- a/packages/sdk-node/src/clerkExpressRequireAuth.ts +++ b/packages/sdk-node/src/clerkExpressRequireAuth.ts @@ -1,7 +1,10 @@ import type { createClerkClient } from '@clerk/backend'; -import { AuthStatus } from '@clerk/backend/internal'; -import { authenticateRequest, decorateResponseWithObservabilityHeaders } from './authenticateRequest'; +import { + authenticateRequest, + decorateResponseWithObservabilityHeaders, + setResponseForHandshake, +} from './authenticateRequest'; import type { ClerkMiddlewareOptions, MiddlewareRequireAuthProp, RequireAuthProp } from './types'; export type CreateClerkExpressMiddlewareOptions = { @@ -25,17 +28,14 @@ export const createClerkExpressRequireAuth = (createOpts: CreateClerkExpressMidd }); decorateResponseWithObservabilityHeaders(res, requestState); - const hasLocationHeader = requestState.headers.get('location'); - if (hasLocationHeader) { - // triggering a handshake redirect - res.status(307).set(requestState.headers).end(); + const err = setResponseForHandshake(requestState, res); + if (err || res.writableEnded) { + if (err) { + next(err); + } return; } - if (requestState.status === AuthStatus.Handshake) { - next(new Error('Clerk: unexpected handshake without redirect')); - } - if (requestState.isSignedIn) { (req as RequireAuthProp).auth = { ...requestState.toAuth(), claims: requestState.toAuth().sessionClaims }; next(); diff --git a/packages/sdk-node/src/clerkExpressWithAuth.ts b/packages/sdk-node/src/clerkExpressWithAuth.ts index bce16affcc2..510b292758e 100644 --- a/packages/sdk-node/src/clerkExpressWithAuth.ts +++ b/packages/sdk-node/src/clerkExpressWithAuth.ts @@ -1,6 +1,8 @@ -import { AuthStatus } from '@clerk/backend/internal'; - -import { authenticateRequest, decorateResponseWithObservabilityHeaders } from './authenticateRequest'; +import { + authenticateRequest, + decorateResponseWithObservabilityHeaders, + setResponseForHandshake, +} from './authenticateRequest'; import type { CreateClerkExpressMiddlewareOptions } from './clerkExpressRequireAuth'; import type { ClerkMiddlewareOptions, MiddlewareWithAuthProp, WithAuthProp } from './types'; @@ -17,21 +19,17 @@ export const createClerkExpressWithAuth = (createOpts: CreateClerkExpressMiddlew }); decorateResponseWithObservabilityHeaders(res, requestState); - const hasLocationHeader = requestState.headers.get('location'); - if (hasLocationHeader) { - // triggering a handshake redirect - res.status(307).set(requestState.headers).end(); - return; - } - - if (requestState.status === AuthStatus.Handshake) { - next(new Error('Clerk: unexpected handshake without redirect')); + const err = setResponseForHandshake(requestState, res); + if (err || res.writableEnded) { + if (err) { + next(err); + } return; } (req as WithAuthProp).auth = { ...requestState.toAuth(), - claims: requestState.toAuth().sessionClaims, + claims: requestState.toAuth()?.sessionClaims, }; next(); };