-
Notifications
You must be signed in to change notification settings - Fork 339
feat(backend): Introduce machine authentication #5689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great so far!
}); | ||
|
||
it('returns false for tokens without a recognized prefix', () => { | ||
expect(isMachineToken('unknown_prefix_token')).toBe(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanna note that we do plan to allow custom prefixes in the future - likely these end up being prepended to the token type prefix so i think it should be a fairly straightforward change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note to cover this in the future.
}); | ||
|
||
// Test each token type with parameterized tests | ||
const tokenTypes = ['api_key', 'oauth_token', 'machine_token'] as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mildly confused by the typecasting here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the as const
is needed here so TS knows these are literal types that match the keys in our mock objects, otherwise it would just see it as string[]
const { sessionTokenInHeader } = authenticateContext; | ||
if (!sessionTokenInHeader) { | ||
return handleError(new Error('No token in header'), 'header'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something seems weird about this logic...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe in practice this shouldn't be hit, as we check the existence of the header token before calling this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah either that or we remove and do non-null assertions
26ea907
to
58c0c97
Compare
!snapshot |
Hey @wobsoriano - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Nice work
// Using "/" instead of an actual version since they're bapi-proxy endpoints. | ||
// bapi-proxy connects directly to C1 without URL versioning, | ||
// while API versioning is handled through the Clerk-API-Version header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, this is surprising to me, I would expect the path structure at edge to mirror origin 🤔 I'll start a thread in Slack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw the thread. Yeah, I also thought of creating a separate buildRequest
function for bapi-proxy but that'd be too much considering the logic we already have when building a request. For now I'm passing "/" to bypass the api version during URL normalization
export * from './BetaFeaturesApi'; | ||
export * from './BlocklistIdentifierApi'; | ||
export * from './ClientApi'; | ||
export * from './DomainApi'; | ||
export * from './EmailAddressApi'; | ||
export * from './IdPOAuthAccessTokenApi'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this one start with IdP
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So.. this was hard to name. We already have:
- OauthAccessToken which is used for retrieving the OAuth access token of a user
- OAuthApplication for OAuth applications
I named it like that based on the object name used (clerk_idp_oauth_access_token) for response deserialization but suggestions welcome!
type MachineObjectExtendedProperties<TAuthenticated extends boolean> = { | ||
api_key: { | ||
name: TAuthenticated extends true ? string : null; | ||
claims: TAuthenticated extends true ? Claims | null : null; | ||
}; | ||
machine_token: { | ||
name: TAuthenticated extends true ? string : null; | ||
claims: TAuthenticated extends true ? Claims | null : null; | ||
}; | ||
oauth_token: object; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clever 👀
/** | ||
* @deprecated Use `isAuthenticated` instead. | ||
*/ | ||
isSignedIn: true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSignedIn
feels more natural when dealing with session tokens 🤔 I'm wondering if we're weighing machine token usage too heavily here when considering renaming this API...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point! I think we relied too much on having unified property, but most of our users are still thinking in terms of user sessions so it's best to still have it for sessions 🤔
I think we also named it like that for reasons that a tokenType
property can have an array of tokens, and that would mean both properties (isSignedIn
and isAuthenticated
) will show up in the result
@@ -69,13 +72,64 @@ function isRequestEligibleForRefresh( | |||
); | |||
} | |||
|
|||
export async function authenticateRequest( | |||
function maybeHandleTokenTypeMismatch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this maybe
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally named it to match the handleMaybeHandshakeStatus
pattern, but looking back, that function actually returns either a signed out or handshake state, while maybeHandleTokenTypeMismatch
only returns a signed out state or null. It's vague 😬 - renamed it to checkTokenTypeMismatch
. Suggestions welcome!
if (!sessionOrMachineTokenInHeader) { | ||
return handleError(new Error('Missing token in header'), 'header'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be possible to hit right? I see it's called inside a conditional header token check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct! This or we do non-null assertions which I'm trying to avoid mostly
const { sessionOrMachineTokenInHeader } = authenticateContext; | ||
// Use session token error handling if no token in header (default behavior) | ||
if (!sessionOrMachineTokenInHeader) { | ||
return handleError(new Error('Missing token in header'), 'header'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider renaming handleError
to handleSessionTokenError()
to distinguish between the machine handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
const { sessionTokenInHeader } = authenticateContext; | ||
if (!sessionTokenInHeader) { | ||
return handleError(new Error('No token in header'), 'header'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe in practice this shouldn't be hit, as we check the existence of the header token before calling this method.
const verifiedToken = await client.machineTokens.verifySecret(secret); | ||
return { data: verifiedToken, tokenType: TokenType.MachineToken, errors: undefined }; | ||
} catch (err: any) { | ||
return handleClerkAPIError('machine_token', err, 'Machine token not found'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the enum here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
@@ -131,7 +132,7 @@ export const auth: AuthFn = async () => { | |||
}; | |||
|
|||
return Object.assign(authObject, { redirectToSignIn, redirectToSignUp }); | |||
}; | |||
}) as AuthFn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to cast here now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as my comment here, but for this PR, it's just to make TS happy as getAuthDataFromRequest
returns a AuthObject
type and we want to force it to just the session types.
The full context is in the Nextjs companion PR #5710
Description
This PR adds machine authentication support (atm only in the backend SDK) by introducing support for 4 token types:
api_key
,oauth_token
,machine_token
, andsession_token
. To maintain backwards compatibility,session_token
remains the default authentication method when no specific token type is specified. This ensures existing apps continue to work without modification while allowing new applications to opt-in to machine authentication methods through theacceptsToken
option.Note: The Nextjs part will be handled in #5710. Changesets to follow.
Key changes:
SignedInState
andSignedOutState
in favor ofAuthenticatedState
andUnauthenticatedState
to better represent both session and machine authentication states. They still return the same properties, with an addedtokenType
andisAuthenticated
properties (deprecatingisSignedIn
).toAuth()
method now returns a different value if thetokenType
is not asession_token
. For now, we landed on theid
,name
,subject
,claims
andscopes
property for machine auth tokens.authenticateRequest
:authenticateAnyRequestWithTokenInHeader
andauthenticateMachineRequestWithTokenInHeader
to handle machine authentication.signedIn
andsignedOut
functions have been updated to accommodate machine auth.MachineTokenVerificationErrorCode
)APIKeysApi
,IdPOAuthAccessTokenApi
, andMachineTokensApi
) used inside a newverifyMachineAuthToken
function to validate tokens against their respective endpointsHere's an example usage pattern with API key:
Say C1 wants to protect their endpoints in a Hono app:
Then C2 can access it by passing the api_key:
P.S. I attempted to break this down into smaller PRs but the changes are tightly coupled 😞. So sorry and thank you in advance reviewer! I believe 30-40% of the total changes are from the test files.
Resolves ROBO-36
Checklist
pnpm test
runs as expected.pnpm build
runs as expected.Type of change