chore(backend,nextjs): [WIP] auth helper improvements and integration tests #6163
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
wobsoriano
changed the title
wobsoriano
changed the title
wobsoriano
changed the title
wobsoriano
changed the title
wobsoriano
changed the title
📝 WalkthroughWalkthroughThis change introduces new methods ( Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
integration/templates/next-app-router/src/app/api/machine/route.ts (1)
8-8: Inconsistent error response format with middleware.The error response format
{ error: 'Unauthorized' }differs from the middleware's{ message: 'Unauthorized' }format. Consider using consistent error response structure across the application.Apply this diff for consistency:
- return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }); + return NextResponse.json({ message: 'Unauthorized' }, { status: 401 });And similarly for the POST handler:
- return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }); + return NextResponse.json({ message: 'Unauthorized' }, { status: 401 });Also applies to: 18-18
integration/testUtils/usersService.ts (1)
185-202: LGTM! Solid implementation with good practices.The implementation correctly:
- Uses a reasonable 1-hour expiration for testing
- Retrieves the secret after creation
- Provides cleanup via the revoke method
- Uses faker for realistic test data
Consider adding error handling for the secret retrieval:
- const { secret } = await clerkClient.apiKeys.getSecret(apiKey.id); + try { + const { secret } = await clerkClient.apiKeys.getSecret(apiKey.id); + // ... rest of the implementation + } catch (error) { + // Clean up the created API key if secret retrieval fails + await clerkClient.apiKeys.revoke({ apiKeyId: apiKey.id, revocationReason: 'Failed to retrieve secret' }); + throw error; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
integration/.keys.json.sample(1 hunks)integration/presets/envs.ts(2 hunks)integration/presets/longRunningApps.ts(1 hunks)integration/templates/next-app-router/src/app/api/machine/route.ts(1 hunks)integration/testUtils/index.ts(1 hunks)integration/testUtils/usersService.ts(4 hunks)integration/tests/api-keys/auth.test.ts(1 hunks)integration/tests/api-keys/protect.test.ts(1 hunks)packages/backend/src/api/endpoints/APIKeysApi.ts(1 hunks)packages/backend/src/index.ts(1 hunks)packages/nextjs/src/app-router/server/auth.ts(1 hunks)packages/nextjs/src/server/clerkMiddleware.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/nextjs/src/app-router/server/auth.ts (1)
Learnt from: wobsoriano
PR: clerk/javascript#6123
File: packages/nextjs/src/server/__tests__/getAuthDataFromRequest.test.ts:63-75
Timestamp: 2025-06-16T01:27:54.563Z
Learning: In packages/nextjs/src/server/data/getAuthDataFromRequest.ts, the tokenType behavior on mismatch is intentionally different between array and single acceptsToken values: when acceptsToken is an array and the token type doesn't match any in the array, tokenType returns null; when acceptsToken is a single value and the token type doesn't match, tokenType returns the requested single value. This design aligns with developer intent and provides a more ergonomic API for common use cases.
🔇 Additional comments (19)
integration/.keys.json.sample (1)
57-61: LGTM! Clean configuration addition.The new API key entry follows the established format and naming conventions perfectly.
packages/backend/src/index.ts (1)
109-109: LGTM! Proper type export addition.The APIKey type export is correctly positioned and follows the established pattern.
integration/presets/longRunningApps.ts (1)
45-45: LGTM! Well-structured configuration addition.The new long-running app configuration correctly reuses existing components and follows the established pattern.
integration/testUtils/index.ts (1)
9-9: LGTM! Consistent type handling.The FakeAPIKey type import and export follow the established pattern for other fake types.
Also applies to: 12-12
integration/presets/envs.ts (1)
166-170: LGTM! Exemplary pattern adherence.The withAPIKeys environment configuration follows the established structure perfectly and uses the correct instance keys.
Also applies to: 196-196
packages/nextjs/src/server/clerkMiddleware.ts (1)
468-468: Correct fix for terminating unauthorized requests.This change properly terminates requests with invalid tokens by returning a JSON response instead of continuing the middleware chain. This aligns with the requirement mentioned in the past review comment.
integration/templates/next-app-router/src/app/api/machine/route.ts (2)
4-12: LGTM! Good demonstration of API key-only authentication.The GET handler correctly demonstrates restricting access to API keys only, which is useful for machine-to-machine authentication scenarios.
14-22: LGTM! Good demonstration of flexible token acceptance.The POST handler correctly demonstrates accepting multiple token types, which provides flexibility for different client authentication methods.
packages/nextjs/src/app-router/server/auth.ts (1)
190-191: LGTM! Correctly implements configurable token type support.The implementation properly extracts the token type from parameters and passes it to the auth() call, enabling auth.protect() to specify which token types it accepts. The fallback to SessionToken maintains backward compatibility.
integration/testUtils/usersService.ts (1)
60-64: LGTM! Well-designed API key testing utility type.The FakeAPIKey type provides a clean interface with the API key object, its secret, and a convenient revoke method for test cleanup.
integration/tests/api-keys/auth.test.ts (4)
8-26: LGTM! Excellent test setup and teardown.The test properly sets up fake users and API keys before all tests and cleans them up afterward. The serial test execution mode is appropriate for stateful integration tests.
28-51: LGTM! Comprehensive API key validation testing.The test correctly covers all authentication scenarios:
- Missing API key (401)
- Invalid API key (401)
- Valid API key (200 with correct userId)
53-91: LGTM! Thorough testing of multiple token type scenarios.The test effectively validates that:
- GET endpoint restricts to API keys only (rejects session tokens)
- POST endpoint accepts both token types
- Both authentication methods return correct user IDs
70-79: ```shell
#!/bin/bashCorrectly display the headers-utils.ts to inspect header and cookie fallback logic
file=$(fd headers-utils.ts packages/nextjs/src/server)
echo "=== File: $file ==="
sed -n '1,200p' "$file"</details> <details> <summary>integration/tests/api-keys/protect.test.ts (3)</summary> `16-46`: **Excellent test setup with comprehensive API route configuration.** The beforeAll setup correctly configures the test environment with: - Proper app cloning and file addition - Clear separation of GET (API key only) and POST (multiple token types) handlers - Appropriate environment configuration and resource creation --- `54-85`: **Thorough API key validation test coverage.** This test comprehensively validates API key protection by testing all the critical scenarios: - Missing API key (401) - Invalid API key (401) - Malformed authorization header (401) - Valid API key (200 with correct userId) The test assertions are appropriate and the flow is logical. --- `87-124`: **Well-designed test for multiple token type handling.** This test effectively validates the mixed token authentication scenario by: - Establishing a session through sign-in - Testing GET endpoint rejection without API key - Verifying POST endpoint accepts both session tokens and API keys - Asserting correct userId response in both cases The test logic correctly demonstrates the different token acceptance policies between endpoints. </details> <details> <summary>packages/backend/src/api/endpoints/APIKeysApi.ts (2)</summary> `7-49`: **Well-defined parameter types for API key operations.** The type definitions are comprehensive and well-documented: - `CreateAPIKeyParams` includes all necessary fields with clear JSDoc comments - `UpdateAPIKeyParams` and `RevokeAPIKeyParams` are appropriately structured - Optional fields are correctly typed with null unions --- `52-58`: **LGTM for create, revoke, and getSecret methods.** These methods are correctly implemented: - `create`: POST to base path with all parameters - `revoke`: POST to specific revoke endpoint with proper path construction - `getSecret`: GET to secret endpoint with proper ID validation The HTTP methods and paths are appropriate for their respective operations. Also applies to: 72-91 </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
wobsoriano
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
integration/tests/api-keys/auth.test.ts (2)
10-126: Duplicate comment - code duplication already noted.This test suite has significant structural duplication with the auth.protect() suite below, as previously identified. The logic and test scenarios are sound, but consider the refactoring suggestion from the previous review to extract common test patterns.
128-234: Duplicate comment - structural duplication persists.As noted in the previous review, this test suite has nearly identical structure to the auth() suite above. While the test coverage is comprehensive and appropriate, the duplication should be addressed using the helper function approach suggested in the earlier comment.
🧹 Nitpick comments (1)
integration/tests/api-keys/auth.test.ts (1)
176-176: Remove commented code.These commented lines should be removed to keep the codebase clean.
- // // No API key provided- // // Invalid API keyAlso applies to: 180-180
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration/tests/api-keys/auth.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
integration/tests/api-keys/auth.test.ts (1)
1-9: Imports look good.The import statements are well-organized and include all necessary types and utilities for the integration tests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
integration/tests/api-keys/auth.test.ts (1)
10-126: Address the significant code duplication between test suites.The two test suites share nearly identical structure, setup, and test logic, differing mainly in using
auth()vsauth.protect(). This creates maintenance overhead and violates the DRY principle.Consider extracting common test scenarios into shared helper functions that accept parameters to differentiate between the authentication methods, as suggested in the previous review.
Also applies to: 128-251
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration/tests/api-keys/auth.test.ts(1 hunks)packages/nextjs/src/server/clerkMiddleware.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nextjs/src/server/clerkMiddleware.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: semgrep/ci
- GitHub Check: check-major-bump
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
integration/tests/api-keys/auth.test.ts (3)
190-190: Clarify the reason for skipping the API key validation test.The test is currently skipped without explanation, which could indicate an unresolved issue or temporary debugging state.
Please verify if this test skip is intentional and add a comment explaining why, or remove the
.skipif the test should be running:- test.skip('should validate API key', async ({ page, context }) => { + test('should validate API key', async ({ page, context }) => {
17-57: Excellent test setup and resource management.Both test suites properly handle resource creation and cleanup with appropriate beforeAll/afterAll hooks. The fake user, backend API user, and API key creation/deletion is well managed.
Also applies to: 135-182
65-91: Comprehensive test coverage for API key authentication scenarios.The tests effectively cover the key authentication scenarios:
- No API key provided (401 response)
- Invalid API key (401 response)
- Valid API key (200 response with correct user ID and token type)
- Multiple token type handling with proper differentiation between endpoints
The assertions are accurate and the test logic is sound.
Also applies to: 93-125, 218-250
| () => ` | ||
| import { clerkMiddleware } from '@clerk/nextjs/server'; | ||
|
|
||
| // @ts-ignore |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the TypeScript ignore directive.
The @ts-ignore comment bypasses TypeScript's type checking, which can hide potential issues and reduce code safety.
Consider properly typing the middleware configuration or using a more specific type assertion if needed:
- // @ts-ignore
- export default clerkMiddleware({ acceptsToken: 'api_key' });
+ export default clerkMiddleware({ acceptsToken: 'api_key' });If there's a legitimate typing issue, consider creating a proper type definition or using a targeted type assertion instead of ignoring all type checking.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // @ts-ignore | |
| export default clerkMiddleware({ acceptsToken: 'api_key' }); |
🤖 Prompt for AI Agents
In integration/tests/api-keys/auth.test.ts at line 143, remove the '@ts-ignore'
directive to avoid bypassing TypeScript's type checking. Instead, address the
underlying typing issue by properly typing the middleware configuration or
applying a more specific type assertion. If necessary, define an appropriate
type for the middleware or use targeted type assertions to maintain type safety
without suppressing errors globally.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
integration/tests/api-keys/auth.test.ts (1)
10-126: Code duplication with the second test suite.This test suite has nearly identical structure and logic to the second suite (lines 128-244), differing mainly in using
auth()vsauth.protect(). The duplication makes maintenance harder and increases the chance of inconsistencies.Consider extracting common test patterns into shared helper functions:
+const createApiKeyTestSuite = ( + suiteName: string, + authMethod: 'auth' | 'auth.protect' +) => { + const createRouteHandler = (acceptsToken: string | string[]) => { + if (authMethod === 'auth') { + return ` + export async function GET() { + const { userId, tokenType } = await auth({ acceptsToken: '${acceptsToken}' }); + if (!userId) { + return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }); + } + return NextResponse.json({ userId, tokenType }); + } + `; + } else { + return ` + export async function GET() { + const { userId, tokenType } = await auth.protect({ token: '${acceptsToken}' }); + return NextResponse.json({ userId, tokenType }); + } + `; + } + }; + + // Common test implementation... +};
🧹 Nitpick comments (2)
integration/tests/api-keys/auth.test.ts (2)
149-149: Remove debugging console.log statements.These console.log statements appear to be debugging code and should be removed from production test code.
- console.log('Caught an error in auth.protect()', error);- console.log('Caught an error in auth.protect()', error);Also applies to: 159-159
183-209: Address the skipped test.This test is marked as
test.skipbut contains the same validation logic as the working test in the first suite. Either fix the underlying issue causing it to be skipped or remove it if it's redundant.What is the reason for skipping this test? If it's a known issue, consider:
- Adding a comment explaining why it's skipped
- Creating a tracking issue for the fix
- Removing it if it's truly redundant with the working test above
Do you want me to help investigate why this test might be failing when using
auth.protect()?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration/tests/api-keys/auth.test.ts(1 hunks)packages/nextjs/src/server/clerkMiddleware.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nextjs/src/server/clerkMiddleware.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
integration/tests/api-keys/auth.test.ts (2)
1-9: LGTM! Clean imports and type usage.The imports are well-organized and use appropriate types from the Clerk backend and test utilities.
144-152: Inconsistent error handling patterns.The
auth.protect()suite uses try/catch blocks while theauth()suite relies on checkinguserIdorisAuthenticated. This inconsistency could indicate different error behaviors between the two methods that should be documented or standardized.#!/bin/bash # Description: Check if auth() and auth.protect() have different error handling patterns in the codebase # Expected: Find documentation or implementation differences explaining when each method throws vs returns null/false echo "=== Searching for auth() error handling patterns ===" rg -A 5 -B 2 "await auth\(" --type ts | head -20 echo "=== Searching for auth.protect() error handling patterns ===" rg -A 5 -B 2 "await auth\.protect\(" --type ts | head -20 echo "=== Looking for documentation on auth vs auth.protect error handling ===" fd -e md -e ts | xargs rg -l "auth\.protect.*throw|auth.*throw" | head -10Consider adding comments explaining why different error handling approaches are used, or standardizing the approach if both methods have similar error behaviors.
Also applies to: 154-162
Description
auth.protect()calls are not correctly terminating requests for invalid tokensauth.protect()in route handlers failed to respect thetokenoptionauth()andauth.protect()calls insideclerkMiddlewareauth()andauth.protect()The underlying tests also validate the general token acceptance mechanism, which is applicable to other token types like
oauth_tokenandmachine_tokensimply by changing theacceptsTokenoption.Resolves USER-2233
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Tests
Chores