feat(backend): introduce handshake #2300
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: fb6e5c1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
| let verifyResult: JwtPayload; | ||
|
|
||
| try { | ||
| verifyResult = await verifyToken(sessionToken, authenticateContext); |
There was a problem hiding this comment.
TODO: for a follow up PR: verifyToken should return result, error to avoid controlling the flow with via an exception. The code will read much better.
cc: @nikosdouvlis this affects the V5 API exports
|
nikosdouvlis
force-pushed
the
client_handshake_Refactor
branch
from
8210b4a to
1b6aed6
Compare
SokratisVidros
approved these changes
Dec 12, 2023
| return handleInterstitialCase(res, requestState, interstitial.data); | ||
|
|
||
| if (requestState.status === AuthStatus.Handshake) { | ||
| // TODO: Handle handshake |
There was a problem hiding this comment.
@nikosdouvlis Reminder for this ☝️
There was a problem hiding this comment.
@SokratisVidros
Posting here for posterity - this will be handled with a 2nd PR once we've completed testing the nextjs integration.
brkalow
force-pushed
the
client_handshake_Refactor
branch
from
eb57432 to
073be40
Compare
brkalow
force-pushed
the
client_handshake_Refactor
branch
from
073be40 to
da79ae6
Compare
* Test suite start * feat(backend,nextjs,utils): Fix jest * first test * Fix bug in jwks cache for multiple runtime keys * Add all the tests, including many failing * Add all the tests, including many failing * fix(shared): Correctly construct proxy URL --------- Co-authored-by: Nikos Douvlis <[email protected]> Co-authored-by: Bryce Kalow <[email protected]>
…sts to support additional URLs
…ng to authenticateRequest
* feat(backend): Remove interstitial endpoints * feat(backend,types): Remove local interstitial script * feat(types): Clean retheme types * feat(backend): Remove interstitial and interstitial rules * feat(clerk-js): Remove interstitial from clerk-js * feat(nextjs): Remove interstitial from authMiddleware * feat(fastify): Remove interstitial * feat(gatsby-plugin-clerk): Remove interstitial * feat(remix): Remove interstitial * feat(clerk-sdk-node): Remove interstitial * fix(nextjs): Always respect redirect header if found As it's possible that we trigger a redirect from authenticateRequest that isn't a handshake status (dev multi-domain sync, for example) * chore(repo): Fix sdk tests * fix(clerk-js): Fix tests related to db-jwt * fix(clerk-js): Keep hasJustSynced check
… from authenticateContext
SokratisVidros
force-pushed
the
client_handshake_Refactor
branch
from
577b981 to
91404bc
Compare
SokratisVidros
force-pushed
the
client_handshake_Refactor
branch
from
c3ec485 to
59943a5
Compare
octoper
pushed a commit
that referenced
this pull request
Dec 13, 2023
* feat(backend): Try the new Client Handshake mechanism * feat(backend): Update authenticateRequest handler to support multi-domain handshake * feat(repo): Introduce tests for client handshake (#2265) * Test suite start * feat(backend,nextjs,utils): Fix jest * first test * Fix bug in jwks cache for multiple runtime keys * Add all the tests, including many failing * Add all the tests, including many failing * fix(shared): Correctly construct proxy URL --------- Co-authored-by: Nikos Douvlis <[email protected]> Co-authored-by: Bryce Kalow <[email protected]> * chore(backend): Refactor authenticateRequest to clarify logic * fix(backend): Fix options passing to authenticateRequest * feat(backend): Add sec-fetch-dest check for satellite sync, adjust tests to support additional URLs * feat(backend): Account for clock skew in dev, but still log error * feat(backend): Refactor backend tests to account for recent refactoring to authenticateRequest * feat(backend): Treat handshake payload as a signed jwt * fix(backend): Add tests and adjust logic to ensure existing tests pass * chore(backend): Refactor tests to conform to new method signature * chore(repo): Add changeset * feat(*): Drop interstitial (#2304) * feat(backend): Remove interstitial endpoints * feat(backend,types): Remove local interstitial script * feat(types): Clean retheme types * feat(backend): Remove interstitial and interstitial rules * feat(clerk-js): Remove interstitial from clerk-js * feat(nextjs): Remove interstitial from authMiddleware * feat(fastify): Remove interstitial * feat(gatsby-plugin-clerk): Remove interstitial * feat(remix): Remove interstitial * feat(clerk-sdk-node): Remove interstitial * fix(nextjs): Always respect redirect header if found As it's possible that we trigger a redirect from authenticateRequest that isn't a handshake status (dev multi-domain sync, for example) * chore(repo): Fix sdk tests * fix(clerk-js): Fix tests related to db-jwt * fix(clerk-js): Keep hasJustSynced check * chore(*): Fix linter * chore(backend): Remove unused AuthErrorReason properties, destructure from authenticateContext * chore(clerk-js): Remove unused @ts-expect-error directive * fix: Address build issues in sdk-node * fix(remix): Correct Remix build issues * chore(repo): Apply linting fixes --------- Co-authored-by: Sokratis Vidros <[email protected]> Co-authored-by: Colin Sidoti <[email protected]> Co-authored-by: Nikos Douvlis <[email protected]> Co-authored-by: Colin Sidoti <[email protected]>
dimkl
reviewed
Dec 15, 2023
| const verifyToken = (token: string, verifyOpts?: VerifyTokenOptions) => { | ||
| const issuer = (iss: string) => iss.startsWith('https://clerk.') || iss.includes('.clerk.accounts'); | ||
| return _verifyToken(token, { issuer, ...options, ...verifyOpts }); | ||
| }; |
There was a problem hiding this comment.
Before this change, we supported using the following:
import { clerkClient } from '@clerk/clerk-sdk-node';
clerkClient.verifyToken('token');
With the above change the customers using the @clerk/clerk-sdk-node and clerkClient.verifyToken
would have to pass an issuer and some options for the function to work.
Customers were using the previous one since we had a reported issue to fix this not a long time ago: #2280
Was this change intentional or accidental?
cc: @nikosdouvlis , @SokratisVidros
There was a problem hiding this comment.
@dimkl the issuer check has been removed entirely
There was a problem hiding this comment.
ooh. I see. thanks for the clarification 😄
What was the reasoning behind removing the issuer check? Do we think that the rest of the claims are sufficient?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change
Packages affected
@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/clerk-expo@clerk/fastifygatsby-plugin-clerk@clerk/localizations@clerk/nextjs@clerk/clerk-react@clerk/remix@clerk/clerk-sdk-node@clerk/shared@clerk/themes@clerk/typesbuild/tooling/chore