chore(backend): Improve subject handling for machine auth objects #6099
Conversation
🦋 Changeset detectedLatest commit: b21e16f The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
📝 Walkthrough""" WalkthroughThis change updates the handling of machine authentication objects to provide explicit and type-safe support for API key, machine-to-machine (M2M), and OAuth token types. It revises type definitions, runtime object construction, and related tests to ensure the correct identifiers are present for each token type, and updates machine token prefixes throughout the codebase. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
🪧 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 (
|
wobsoriano
changed the title
| @@ -5,11 +5,11 @@ import { AbstractAPI } from './AbstractApi'; | |||
| const basePath = '/oauth_applications/access_tokens'; | |||
|
|
|||
| export class IdPOAuthAccessTokenApi extends AbstractAPI { | |||
| async verifySecret(secret: string) { | |||
| async verifyAccessToken(accessToken: string) { | |||
There was a problem hiding this comment.
These machine token BAPI methods are not exposed in the SDK yet so it's safe to update this
| return this.request<IdPOAuthAccessToken>({ | ||
| method: 'POST', | ||
| path: joinPaths(basePath, 'verify'), | ||
| bodyParams: { secret }, | ||
| bodyParams: { access_token: accessToken }, |
There was a problem hiding this comment.
originally secret, deprecated in favor of access_token
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
packages/backend/src/tokens/machine.ts (1)
15-18: 🧹 Nitpick (assertive)Comment prefix no longer matches code
The JSDoc examples still reference
"oauth_access_", but the actual prefix constant is"oat_".- * (e.g. "org_a_m2m_", "org_a_oauth_access_", "org_a_api_key_") + * (e.g. "org_a_m2m_", "org_a_oat_", "org_a_api_key_")Aligning docs with code prevents confusion for integrators reading generated typings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
.changeset/mean-items-retire.md(1 hunks)packages/backend/src/api/endpoints/IdPOAuthAccessTokenApi.ts(1 hunks)packages/backend/src/fixtures/machine.ts(2 hunks)packages/backend/src/tokens/__tests__/authObjects.test.ts(3 hunks)packages/backend/src/tokens/__tests__/getAuth.test-d.ts(2 hunks)packages/backend/src/tokens/__tests__/verify.test.ts(4 hunks)packages/backend/src/tokens/authObjects.ts(7 hunks)packages/backend/src/tokens/machine.ts(1 hunks)packages/backend/src/tokens/verify.ts(1 hunks)packages/express/src/__tests__/getAuth.test.ts(1 hunks)packages/fastify/src/__tests__/getAuth.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/fastify/src/__tests__/getAuth.test.ts (1)
packages/fastify/src/getAuth.ts (1)
getAuth(14-22)
packages/express/src/__tests__/getAuth.test.ts (1)
packages/express/src/__tests__/helpers.ts (1)
mockRequestWithAuth(29-39)
packages/backend/src/api/endpoints/IdPOAuthAccessTokenApi.ts (1)
packages/backend/src/api/resources/IdPOAuthAccessToken.ts (1)
IdPOAuthAccessToken(3-33)
🪛 Gitleaks (8.26.0)
packages/backend/src/fixtures/machine.ts
4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
packages/backend/src/tokens/__tests__/verify.test.ts
101-101: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
223-223: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.17.2)
.changeset/mean-items-retire.md
5-5: First line in a file should be a top-level heading
null
(MD041, first-line-heading, first-line-h1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
packages/express/src/__tests__/getAuth.test.ts (1)
31-36:❓ Verification inconclusive
Add coverage for the newly-introduced
machineIdpropertyThe PR description states that
machine_tokenauth objects now expose amachineId.
This test still asserts thatsubjectisundefinedbut never checks formachineId, so we’re missing regression coverage for the new field.- const req = mockRequestWithAuth({ tokenType: 'machine_token', id: 'm2m_1234' }); + const req = mockRequestWithAuth({ + tokenType: 'machine_token', + id: 'm2m_1234', + machineId: 'mch_5678', + }); ... - expect(result.subject).toBeUndefined(); + expect(result.machineId).toBe('mch_5678'); + // `subject` was intentionally removed for machine tokens – document that: + expect(result.subject).toBeUndefined();This small extension both exercises the new field and clarifies intentional absence of
subject.
🏁 Script executed:
#!/bin/bash set -e echo "Searching for mockRequestWithAuth definition..." rg "mockRequestWithAuth" -n . echo echo "Searching for getAuth implementation..." rg "export function getAuth" -n packages/express/src echo echo "Searching for 'machineId' references..." rg "machineId" -n .Length of output: 1740
🏁 Script executed:
#!/bin/bash set -e echo "Searching for AuthObject definition..." rg -n "interface AuthObject" -n packages/express/src rg -n "type AuthObject" -n packages/express/src echo echo "Displaying mockRequestWithAuth implementation..." sed -n '1,200p' packages/express/src/__tests__/helpers.ts echo echo "Searching for getAuth implementation and usage of machineId..." rg -n "getAuth" -n packages/express/src rg -n "machineId" -n packages/express/srcLength of output: 150
🏁 Script executed:
#!/bin/bash set -e echo "Searching for AuthObject references..." rg -n 'AuthObject' packages/express/src echo echo "Searching for getAuth implementation..." rg -n 'export function getAuth' packages/express/src echo echo "Displaying the mockRequestWithAuth helper (first 200 lines)..." sed -n '1,200p' packages/express/src/__tests__/helpers.ts echo echo "Searching for any usage of machineId in the codebase..." rg -n 'machineId' .Length of output: 1524
🏁 Script executed:
#!/bin/bash set -e echo "=== getAuth.ts ===" sed -n '1,200p' packages/express/src/getAuth.ts || echo "getAuth.ts not found" echo echo "=== utils.ts ===" sed -n '1,200p' packages/express/src/utils.ts || echo "utils.ts not found"Length of output: 3291
Add coverage for new
machineIdin getAuth testThe PR notes that
machine_tokenauth objects now include amachineId. The existing test only checks thatsubjectisundefinedand doesn’t exercise the new field. Please update the test atpackages/express/src/__tests__/getAuth.test.ts(lines 31–36) as follows:- const req = mockRequestWithAuth({ tokenType: 'machine_token', id: 'm2m_1234' }); + const req = mockRequestWithAuth({ + tokenType: 'machine_token', + id: 'm2m_1234', + machineId: 'mch_5678', + }); ... - expect(result.subject).toBeUndefined(); + expect(result.machineId).toBe('mch_5678'); + // `subject` remains undefined for machine tokens + expect(result.subject).toBeUndefined();This addition ensures we have regression coverage for the new
machineIdproperty.packages/backend/src/tokens/verify.ts (1)
216-223: 🧹 Nitpick (assertive)Rename local variable to avoid future confusion
Now that the first parameter is
accessToken, the local variable in the catch helper should follow the same naming style for consistency withverifyAPIKeyandverifyMachineToken. (Currently it is correct, just suggesting cohesion.)-async function verifyOAuthToken( - accessToken: string, +async function verifyOAuthToken( + accessToken: string, options: VerifyTokenOptions, ... - const verifiedToken = await client.idPOAuthAccessToken.verifyAccessToken(accessToken); + const verifiedToken = await client.idPOAuthAccessToken.verifyAccessToken(accessToken);No functional change, only readability.
Likely an incorrect or invalid review comment.
packages/backend/src/tokens/__tests__/verify.test.ts (2)
202-224: Consistency in error-case fixturesError-case tokens were updated to
m2m_…, but the mocked endpoint remains/m2m_tokens/verify(👍).
No issues spotted—just confirming the contract stays aligned with backend routes.🧰 Tools
🪛 Gitleaks (8.26.0)
223-223: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
100-109:✅ Verification successful
Ensure
mt_prefix is fully eradicated across the repoGreat catch updating the happy-path test to the new
m2m_prefix. Please double-check that no straymt_tokens remain in the codebase (fixtures, docs, comments) otherwise tests may silently diverge from production behaviour.
🏁 Script executed:
#!/bin/bash # Search for any remaining occurrences of the old machine-token prefix. rg -n $'\bmt_[A-Za-z0-9_]+' --glob '!node_modules'Length of output: 50
Let’s re-run the search using a plain regex (avoiding the bash‐specific
\bescape) to catch any remainingmt_prefixes:
🏁 Script executed:
#!/bin/bash # Search for any occurrences of the old machine-token prefix across the repo rg -n 'mt_[A-Za-z0-9_]+' --glob '!node_modules'Length of output: 49
No remaining
mt_prefixes detectedA repository-wide search found zero occurrences of the old
mt_token prefix—everything now consistently usesm2m_. All fixtures, docs, comments, and code are updated.🧰 Tools
🪛 Gitleaks (8.26.0)
101-101: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
packages/backend/src/fixtures/machine.ts (1)
40-43: LGTM – fixture matches new conventions
idandsubjectcorrectly use them2m_/mch_prefixes and align with the tests. No further action needed.packages/backend/src/tokens/__tests__/authObjects.test.ts (2)
330-337: OAuth expectations updated correctly
clientIdassertion added—matches new typing. No issues.
359-364: Machine token assertions align with newmachineIdfieldHappy path verified; good to go.
packages/backend/src/tokens/__tests__/getAuth.test-d.ts (1)
40-48: Type-level discriminated-union test streamlinedThe reduced assertions still preserve compile-time guarantees and keep the file maintainable. 👍
packages/backend/src/tokens/authObjects.ts (3)
90-104: Type definitions look good!The enhanced type definitions properly model the identity properties for each token type with appropriate conditional typing for authenticated vs unauthenticated states.
277-284: OAuth token handling looks good!The implementation correctly extracts
userIdfrom the subject and includesclientIdandscopesproperties as expected by the type definition.
424-424: Documentation updated correctlyGood fix - the example now uses the correct
m2m_prefix for machine tokens.
| it('returns the actual auth object if its tokenType is included in the acceptsToken array', () => { | ||
| const req = { auth: { tokenType: 'machine_token', id: 'mt_1234' } } as unknown as FastifyRequest; | ||
| const req = { auth: { tokenType: 'machine_token', id: 'm2m_1234' } } as unknown as FastifyRequest; | ||
| const result = getAuth(req, { acceptsToken: ['machine_token', 'api_key'] }); | ||
| expect(result.tokenType).toBe('machine_token'); | ||
| expect(result.id).toBe('mt_1234'); | ||
| expect(result.id).toBe('m2m_1234'); | ||
| expect(result.subject).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror Express-side assertions to keep test suites in sync
For completeness and consistency with the Express tests, please include the new machineId assertion here as well.
- const req = { auth: { tokenType: 'machine_token', id: 'm2m_1234' } } as unknown as FastifyRequest;
+ const req = {
+ auth: { tokenType: 'machine_token', id: 'm2m_1234', machineId: 'mch_5678' },
+ } as unknown as FastifyRequest;
...
- expect(result.subject).toBeUndefined();
+ expect(result.machineId).toBe('mch_5678');
+ expect(result.subject).toBeUndefined();Keeping both frameworks’ tests parallel reduces the probability of silent divergences.
📝 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.
| it('returns the actual auth object if its tokenType is included in the acceptsToken array', () => { | |
| const req = { auth: { tokenType: 'machine_token', id: 'mt_1234' } } as unknown as FastifyRequest; | |
| const req = { auth: { tokenType: 'machine_token', id: 'm2m_1234' } } as unknown as FastifyRequest; | |
| const result = getAuth(req, { acceptsToken: ['machine_token', 'api_key'] }); | |
| expect(result.tokenType).toBe('machine_token'); | |
| expect(result.id).toBe('mt_1234'); | |
| expect(result.id).toBe('m2m_1234'); | |
| expect(result.subject).toBeUndefined(); | |
| }); | |
| it('returns the actual auth object if its tokenType is included in the acceptsToken array', () => { | |
| const req = { | |
| auth: { tokenType: 'machine_token', id: 'm2m_1234', machineId: 'mch_5678' }, | |
| } as unknown as FastifyRequest; | |
| const result = getAuth(req, { acceptsToken: ['machine_token', 'api_key'] }); | |
| expect(result.tokenType).toBe('machine_token'); | |
| expect(result.id).toBe('m2m_1234'); | |
| expect(result.machineId).toBe('mch_5678'); | |
| expect(result.subject).toBeUndefined(); | |
| }); |
🤖 Prompt for AI Agents
In packages/fastify/src/__tests__/getAuth.test.ts around lines 26 to 32, the
test is missing an assertion for the new machineId property to mirror the
Express-side tests. Add an assertion to check that result.machineId is undefined
or matches the expected value to keep the Fastify test suite consistent with the
Express tests and prevent silent divergences.
@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: |
Description
This PR improves the
subjectproperty handling for machine auth objects. Specifically:api_keys, added auserIdandorgIdmachine_token, added amachineIdoauth_token, added auserIdand theclientIdfor convenience.Resolves USER-2118
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation