diff --git a/.changeset/quiet-keys-protect.md b/.changeset/quiet-keys-protect.md new file mode 100644 index 00000000000..0dd77df6eed --- /dev/null +++ b/.changeset/quiet-keys-protect.md @@ -0,0 +1,5 @@ +--- +'@clerk/nextjs': patch +--- + +Fix `auth.protect()` skipping authorization checks for machine tokens. The `role`, `permission`, `feature`, `plan`, and `reverification` params and the predicate callback are now evaluated for API key, OAuth token, and M2M token requests. Param-based checks respond with a 401 for those requests. diff --git a/integration/tests/next-machine.test.ts b/integration/tests/next-machine.test.ts index 0a753cce476..80b9cbc878d 100644 --- a/integration/tests/next-machine.test.ts +++ b/integration/tests/next-machine.test.ts @@ -1,8 +1,13 @@ -import { test } from '@playwright/test'; +import type { User } from '@clerk/backend'; +import { TokenType } from '@clerk/backend/internal'; +import { expect, test } from '@playwright/test'; +import type { Application } from '../models/application'; import { appConfigs } from '../presets'; +import { createTestUtils } from '../testUtils'; import type { MachineAuthTestAdapter } from '../testUtils/machineAuthHelpers'; import { registerApiKeyAuthTests, registerM2MAuthTests, registerOAuthAuthTests } from '../testUtils/machineAuthHelpers'; +import type { FakeAPIKey, FakeUser } from '../testUtils/usersService'; const adapter: MachineAuthTestAdapter = { baseConfig: appConfigs.next.appRouter, @@ -94,4 +99,94 @@ test.describe('Next.js machine authentication @machine', () => { registerApiKeyAuthTests(adapter); registerM2MAuthTests(adapter); registerOAuthAuthTests(adapter); + + test.describe('auth.protect() authorization with API keys', () => { + test.describe.configure({ mode: 'parallel' }); + let app: Application; + let fakeUser: FakeUser; + let fakeBapiUser: User; + let fakeAPIKey: FakeAPIKey; + + test.beforeAll(async () => { + test.setTimeout(120_000); + + app = await appConfigs.next.appRouter + .clone() + .addFile( + 'src/app/api/protect/route.ts', + () => ` + import { auth } from '@clerk/nextjs/server'; + + export async function GET() { + const { userId, tokenType } = await auth.protect({ token: 'api_key' }); + + return Response.json({ userId, tokenType }); + } + + export async function POST() { + const { userId } = await auth.protect({ role: 'org:admin' }, { token: ['session_token', 'api_key'] }); + + return Response.json({ userId }); + } + + export async function PUT() { + await auth.protect(() => false, { token: 'api_key' }); + + return Response.json({ protected: true }); + } + + export async function PATCH() { + const { userId, tokenType } = await auth.protect(() => true, { token: 'api_key' }); + + return Response.json({ userId, tokenType }); + } + `, + ) + .commit(); + await app.setup(); + await app.withEnv(appConfigs.envs.withAPIKeys.clone()); + await app.dev(); + + const u = createTestUtils({ app }); + fakeUser = u.services.users.createFakeUser(test); + fakeBapiUser = await u.services.users.createBapiUser(fakeUser); + fakeAPIKey = await u.services.users.createFakeAPIKey(fakeBapiUser.id); + }); + + test.afterAll(async () => { + await fakeAPIKey?.revoke(); + await fakeUser?.deleteIfExists(); + await app?.teardown(); + }); + + test('should return 200 if API key is valid and no authorization params are passed', async ({ request }) => { + const res = await request.get(new URL('/api/protect', app.serverUrl).toString(), { + headers: { Authorization: `Bearer ${fakeAPIKey.secret}` }, + }); + expect(res.status()).toBe(200); + expect(await res.json()).toEqual({ userId: fakeBapiUser.id, tokenType: TokenType.ApiKey }); + }); + + test('should return 401 if API key is valid but a role is required', async ({ request }) => { + const res = await request.post(new URL('/api/protect', app.serverUrl).toString(), { + headers: { Authorization: `Bearer ${fakeAPIKey.secret}` }, + }); + expect(res.status()).toBe(401); + }); + + test('should return 401 if API key is valid but the predicate returns false', async ({ request }) => { + const res = await request.put(new URL('/api/protect', app.serverUrl).toString(), { + headers: { Authorization: `Bearer ${fakeAPIKey.secret}` }, + }); + expect(res.status()).toBe(401); + }); + + test('should return 200 if API key is valid and the predicate returns true', async ({ request }) => { + const res = await request.patch(new URL('/api/protect', app.serverUrl).toString(), { + headers: { Authorization: `Bearer ${fakeAPIKey.secret}` }, + }); + expect(res.status()).toBe(200); + expect(await res.json()).toEqual({ userId: fakeBapiUser.id, tokenType: TokenType.ApiKey }); + }); + }); }); diff --git a/packages/nextjs/src/server/__tests__/clerkMiddleware.test.ts b/packages/nextjs/src/server/__tests__/clerkMiddleware.test.ts index 31757419d0f..4b3e6754dfd 100644 --- a/packages/nextjs/src/server/__tests__/clerkMiddleware.test.ts +++ b/packages/nextjs/src/server/__tests__/clerkMiddleware.test.ts @@ -1,6 +1,6 @@ // There is no need to execute the complete authenticateRequest to test clerkMiddleware // This mock SHOULD exist before the import of authenticateRequest -import { AuthStatus, constants, TokenType } from '@clerk/backend/internal'; +import { authenticatedMachineObject, AuthStatus, constants, TokenType } from '@clerk/backend/internal'; import { MalformedURLError } from '@clerk/shared/pathMatcher'; // used to assert the mock import assert from 'assert'; @@ -9,6 +9,7 @@ import { NextRequest, NextResponse } from 'next/server'; import { afterAll, beforeAll, beforeEach, describe, expect, it, type Mock, vi } from 'vitest'; import { clerkClient } from '../clerkClient'; +import type { ClerkMiddlewareAuth } from '../clerkMiddleware'; import { clerkMiddleware } from '../clerkMiddleware'; import { createRouteMatcher } from '../routeMatcher'; import { decryptClerkRequestData } from '../utils'; @@ -881,6 +882,82 @@ describe('clerkMiddleware(params)', () => { expect(resp?.status).toEqual(401); expect((await clerkClient()).authenticateRequest).toBeCalled(); }); + + describe.each([ + [TokenType.ApiKey, 'ak_123', 'user_123'], + [TokenType.M2MToken, 'mt_123', 'mch_123'], + [TokenType.OAuthToken, 'oat_123', 'user_123'], + ] as const)('when the request is authenticated with a valid %s', (tokenType, token, subject) => { + const protectWith = async (protect: (auth: ClerkMiddlewareAuth) => Promise) => { + const req = mockRequest({ + url: '/api/protected', + headers: new Headers({ [constants.Headers.Authorization]: `Bearer ${token}` }), + }); + + authenticateRequestMock.mockResolvedValueOnce({ + publishableKey, + status: AuthStatus.SignedIn, + headers: new Headers(), + toAuth: () => authenticatedMachineObject(tokenType, token, { id: token, subject, scopes: [] } as any), + }); + + return clerkMiddleware(async auth => { + await protect(auth); + })(req, {} as NextFetchEvent); + }; + + it.each([ + { role: 'org:admin' }, + { permission: 'org:settings:manage' }, + { feature: 'premium' }, + { plan: 'pro' }, + { reverification: 'strict' }, + ] as const)('returns 401 when protect is called with %o', async params => { + const resp = await protectWith(auth => auth.protect(params, { token: 'any' })); + + expect(resp?.status).toEqual(401); + }); + + it('returns 401 when protect is called with a permission and an array of token types', async () => { + const resp = await protectWith(auth => + auth.protect({ permission: 'org:settings:manage' }, { token: [TokenType.SessionToken, tokenType] }), + ); + + expect(resp?.status).toEqual(401); + }); + + it('returns 401 when protect is called with authorization params and token in a single argument', async () => { + const resp = await protectWith(auth => auth.protect({ feature: 'premium', token: tokenType } as any)); + + expect(resp?.status).toEqual(401); + }); + + it('returns 401 when protect is called with a predicate that returns false', async () => { + const predicate = vi.fn().mockReturnValue(false); + const resp = await protectWith(auth => auth.protect(predicate, { token: tokenType })); + + expect(predicate).toHaveBeenCalledOnce(); + expect(resp?.status).toEqual(401); + }); + + it('returns 401 when protect is called with a predicate that relies on has()', async () => { + const resp = await protectWith(auth => auth.protect(has => has({ role: 'org:admin' }), { token: tokenType })); + + expect(resp?.status).toEqual(401); + }); + + it('does not throw when protect is called with a predicate that returns true', async () => { + const resp = await protectWith(auth => auth.protect(() => true, { token: tokenType })); + + expect(resp?.status).toEqual(200); + }); + + it('does not throw when protect is called without authorization params', async () => { + const resp = await protectWith(auth => auth.protect({ token: tokenType })); + + expect(resp?.status).toEqual(200); + }); + }); }); describe('auth().redirectToSignIn()', () => { diff --git a/packages/nextjs/src/server/protect.ts b/packages/nextjs/src/server/protect.ts index 1e72f7128fd..73b4cf74f9d 100644 --- a/packages/nextjs/src/server/protect.ts +++ b/packages/nextjs/src/server/protect.ts @@ -154,30 +154,28 @@ export function createProtect(opts: { } if (authObject.tokenType !== TokenType.SessionToken) { - // For machine tokens, we only check if they're authenticated - // They don't have session status or organization permissions + // Machine tokens don't have session status if (!authObject.isAuthenticated) { return handleUnauthorized(); } - return authObject; - } - - /** - * Redirects the user back to the tasks URL if their session status is pending - */ - if (authObject.sessionStatus === 'pending') { - return handleUnauthenticated(); - } + } else { + /** + * Redirects the user back to the tasks URL if their session status is pending + */ + if (authObject.sessionStatus === 'pending') { + return handleUnauthenticated(); + } - /** - * User is not authenticated - */ - if (!authObject.userId) { - return handleUnauthenticated(); + /** + * User is not authenticated + */ + if (!authObject.userId) { + return handleUnauthenticated(); + } } /** - * User is authenticated + * User or machine is authenticated */ if (!paramsOrFunction) { return authObject;