feat(repo): Add E2E test suite for handshake #2331
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
* 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
…st() regardless of auth status
🦋 Changeset detectedLatest commit: 8f88da2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
brkalow
commented
Dec 13, 2023
Comment on lines
+128
to
+133
| if ( | ||
| err instanceof TokenVerificationError && | ||
| instanceType === 'development' && | ||
| (err.reason === TokenVerificationErrorReason.TokenExpired || | ||
| err.reason === TokenVerificationErrorReason.TokenNotActiveYet) | ||
| ) { |
There was a problem hiding this comment.
Previously, we weren't re-throwing the error for production, which is incorrect. In the scenario where verification fails on the token from the handshake, production should treat this as a fatal error.
brkalow
commented
Dec 13, 2023
Comment on lines
+197
to
+198
| const locationHeader = requestState.headers.get('location'); | ||
| if (locationHeader) { |
There was a problem hiding this comment.
There are a few scenarios where we pass a location header when the status is not a handshake. Notably, in development we strip the handshake query parameters.
nikosdouvlis
approved these changes
Dec 13, 2023
SokratisVidros
force-pushed
the
client_handshake_Refactor
branch
from
577b981 to
91404bc
Compare
nikosdouvlis
force-pushed
the
brk.feat/handshake-e2e-tests
branch
from
05f9bdf to
ffd1cac
Compare
octoper
pushed a commit
that referenced
this pull request
Dec 13, 2023
…#2331) * 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 * feat(repo): Migrate handshake tests to e2e test suite * fix(nextjs): Ensure we respect location header from authenticateRequest() regardless of auth status * Create bright-ways-provide.md * chore(clerk-sdk-node): Properly export createClerkClient * chore(clerk-sdk-node): Properly export createClerkClient * fix(nextjs): Fix undefined headers in tests --------- 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]>
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
Introduces a test suite for our handshake functionality in our E2E test suite. Previously, we were using a one-off test suite that was run manually. Migrating the tests into our E2E test suite will ensure they run in CI and will allow us to run them against other frameworks as we roll out the handshake functionality.
I also fixed some logic in
@clerk/nextjsto ensure we always followlocationheaders returned fromauthenticateRequest().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