Uh oh!
There was an error while loading. Please reload this page.
fix(clerk-sdk-node): Inherit verifyToken options from clerkClient - #3296
Conversation
🦋 Changeset detectedLatest commit: 5782b00 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 |
panteliselef
commented
May 1, 2024
!snapshot |
clerk-cookie
commented
May 1, 2024
Hey @panteliselef - 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/backend@1.1.2-snapshot.v9a7208b --save-exact
npm i @clerk/chrome-extension@1.0.4-snapshot.v9a7208b --save-exact
npm i @clerk/clerk-js@5.2.1-snapshot.v9a7208b --save-exact
npm i @clerk/clerk-expo@1.0.4-snapshot.v9a7208b --save-exact
npm i @clerk/express@0.0.5-snapshot.v9a7208b --save-exact
npm i @clerk/fastify@1.0.4-snapshot.v9a7208b --save-exact
npm i gatsby-plugin-clerk@5.0.0-beta.45 --save-exact
npm i @clerk/nextjs@5.0.5-snapshot.v9a7208b --save-exact
npm i @clerk/remix@4.0.4-snapshot.v9a7208b --save-exact
npm i @clerk/clerk-sdk-node@5.0.4-snapshot.v9a7208b --save-exact |
| if (!options.secretKey || !options.jwtKey) { | ||
| return { | ||
| errors: [ | ||
| new TokenVerificationError({ | ||
| action: TokenVerificationErrorAction.SetClerkSecretKey, | ||
| message: 'Both JWT Key and Secret Key are missing. Operation could not be completed.', | ||
| reason: TokenVerificationErrorReason.InvalidSecretKey, | ||
| }), | ||
| ], | ||
| }; | ||
| } | ||
There was a problem hiding this comment.
Do we think that this is a breaking change ? Tbh i think we can skip its introduction
There was a problem hiding this comment.
We could also just throw a warning instead so we don't introduce a breaking change
| ```ts | ||
| import { clerkClient } from "@clerk/clerk-sdk-node"; | ||
| clerkClient.verifyToken(token, {}) |
There was a problem hiding this comment.
Only reason, i've not update the signature is that this package will be deprecated.
There was a problem hiding this comment.
☁️ I understand that the customer use case is valid and that we probably need to make a change to clerkClient.verifyToken(token, options) to support execution without passing options but i am not 100% sure that we should make it this way.
I would expect to either keep it as is and inform the customers using it explicitly that it's just a re-export from the @clerk/backend and all the arguments required should be passed. or support the following:
import{clerkClient}from"@clerk/clerk-sdk-node";// case #1: use options passed in clerkClientclerkClient.verifyToken(token)// case #2: options should be non-conflicting with the ones passed in clerkClient// as it does not make sense to have a different jwtKey set in the clerkClient and a different one used in verifyTokenclerkClient.verifyToken(token,options)If they need to pass explicitly the options i would advise to either create a different clerkClient or use the verifytoken from the @clerk/backend.
I think we should go for ONLY the 1st case. We should check if the @clerk/express supports the same API and make the appropriate changes there.
cc: @nikosdouvlis
panteliselef
commented
May 9, 2024
@dimkl If we are going the extra mile here, let's still support the 2nd param, but update the signature so that 2nd param is optional. It is the least disruptive solution. Your suggestions make sense, but I would skip them for this package and only apply them to the new express package |
Uh oh!
There was an error while loading. Please reload this page.
shadoworion
commented
May 10, 2024
Hi, I use this function to create auth middleware for the "graphql-yoga" server. All options are default and secret from env (which doesn't work now), so "case #1" fits better. If I use different options that conflict with the current "clerkClient," I'll use "createClerkClient" to create a new one (that also doesn't work now) |
| return (...args: Parameters<VerifyTokenWithOptionalSecondArgument>) => | ||
| _verifyToken(args[0], { | ||
| ...params, | ||
| ...args[1], |
There was a problem hiding this comment.
🔧 Could you add 2 tests (with options, without options) to verify that everything works as expected?
| const mockNext = jest.fn(); | ||
| afterEach(() => { | ||
| mockNext.mockReset(); | ||
| }); | ||
fd50eea to
1afcdcdCompare…-during-verification
Description
This PR allows
clerkClient.verifyTokenfrom "@clerk/clerk-sdk-node" to inherit the VerifyTokenOptions set whenclerkClientis instantiated.Added an additional error when both
jwtKeyandsecretKeyare missing.fixes#3283
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change