feat: Replacing jsonwebtoken with fast-jwt #184

radomird · 2021-10-19T08:01:13Z

Replacing the jsonwebtoken with more performant fast-jwt library.

Since options for fast-jwt are somewhat different, I will update the options exposed through fastify-jwt.

Important note: This is a BREAKING CHANGE, so after this PR is approved and merged, a new major version of the plugin will be released.

Closes #174
Closes #166

Update 10-11-2021: fast-jwt has been updated to support password protected private keys, This PR includes those changes as well.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

Updated verify and decode methods

Disabled tests with encoded private key because of compatibility issues

jwt.d.ts

test/test.test.js

Co-authored-by: KaKa <climba03003@gmail.com>

jwt.js

mcollina

lgtm

mcollina

Important note: fast-jwt DOES NOT support passphrase protected private keys yet! For this reason the test cases covering this scenario were (temporarily) commented out.

This needs to be fixed before landing

radomird · 2021-10-26T09:12:50Z

Opened an issue in fast-jwt about the passphrase protected private keys support.
nearform/fast-jwt#116

jwt.js

jsumners · 2021-10-26T11:54:13Z

@mcollina is there anything specific you would like me to review here? I am not familiar with this module. Cursorily, you have already covered what I would say: it needs 1:1 feature parity before it can be accepted.

simoneb · 2021-10-26T13:19:14Z

@mcollina @jsumners yes this is being worked on, feature parity will first need a change in fast-jwt to support password protected private keys, which are currently not supported. See nearform/fast-jwt#117.

climba03003

LGTM. We wait for nearform/fast-jwt#117 before landing.

darkgl0w

LGTM !

Some suggestions:

We should take the opportunity offered by this PR to rework the file structure of the plugin and move jwt.test-d.ts to test/types/jwt.test-d.ts, rename test/test.test.js to test/jwt.test.js and update the corresponding references in package.json file.
Update to require at least fastify >= 3.0.0 (currently it requires fastify >= 3.0.0-alpha-1) in jwt.js file module export.
We should add an explicit warning in the README.md stating that we migrated from jsonwebtoken to fast-jwt and that it may break "exotic" implementations even though it is a 1:1 feature implementation.

NB: this PR superseeds and closes #166 too.

test/test.test.js

simoneb

Overall LGTM, a couple of inline comments (some may be outdated as it took me a while to review this and you may have addressed some of them already).

One main question: shall we write a short migration guide so users know what to change when upgrading?

README.md

jwt.d.ts

simoneb · 2021-11-17T08:39:21Z

jwt.d.ts

@@ -1,5 +1,5 @@
+import { DecoderOptions, KeyFetcher, SignerCallback, SignerOptions, VerifierCallback, VerifierOptions } from 'fast-jwt'


given the many changes in typings, let's make sure we are running typing checks in CI and that we have tests in place for typing

jwt.test-d.ts

darkgl0w

Added some changes suggestions to types. (mostly string time span related)

jwt.test-d.ts

jwt.d.ts

mcollina

lgtm

README.md

mcollina · 2021-11-18T08:35:36Z

You might want to run some benchmarks using wrk or autocannon.

Added new types for Sign and Verify Options, updated typings test Co-authored-by: darkgl0w <31093081+darkgl0w@users.noreply.github.com>

radomird · 2021-11-18T12:09:43Z

I think we are good to go. After the workflow is approved and the tests are green - the PR can be merged.

Eomm

LGTM, just a suggestion on the readme

README.md

simoneb · 2021-11-19T11:09:56Z

LGTM, just a suggestion on the readme

Yes absolutely, I suggested the same. We should have a section in the readme or other sensible place and link it from the release notes

radomird · 2021-11-19T13:33:51Z

@Eomm @simoneb & @darkgl0w I've updated the README with breaking changes and also all of the examples in the README have been updated to use the new option names.

darkgl0w

Seems good to me ^^
I have left a few nits

README.md

Co-authored-by: darkgl0w <31093081+darkgl0w@users.noreply.github.com>

darkgl0w

That's way better with a separate file 👍

UPGRADING.md

mcollina

Could you add a bit of a migration guide at the top of the README?

There are a few API changes here

simoneb · 2021-11-21T07:39:39Z

Could you add a bit of a migration guide at the top of the README?

There are a few API changes here

There's an UPGRADING.md doc added in this PR

UPGRADING.md

Co-authored-by: darkgl0w <31093081+darkgl0w@users.noreply.github.com>

radomird · 2021-11-22T09:30:13Z

Folks, I think the PR is, finally, ready to be merged. If some small issues arise after the release, we will address them separately.

mcollina

lgtm

radomird · 2021-11-24T16:26:02Z

Can anyone with write access merge the PR? I don't have sufficient access level to do this 😄
@mcollina

climba03003

No blocking for me. LGTM.

Uzlopak · 2021-11-24T19:23:35Z

Neat!

I assume that @lukeed will be happy to read that his package will be used in a this fastify-plugin. (Pinging him on purpose ;) )

mcollina

lgtm

lukeed · 2021-11-25T18:22:43Z

Nice! :) thanks for the ping

radomird added 8 commits October 19, 2021 09:56

feat: Replaced jsonwebtoken with fast-jwt

897cf93

Removed jsowebtoken library from the source and package.json;

b06b07a

Updated verify and decode methods

Updated tests to use new option names;

09b8e68

Disabled tests with encoded private key because of compatibility issues

Enabled all passing tests

9d284bf

Fixed all unit tests

ac38fc6

Added missing code coverage

ba0e155

Updated types

dfc1562

Updated docs

34148a4

radomird marked this pull request as ready for review October 25, 2021 14:43

climba03003 requested changes Oct 25, 2021

View reviewed changes

jwt.d.ts Outdated Show resolved Hide resolved

test/test.test.js Outdated Show resolved Hide resolved

radomird and others added 2 commits October 25, 2021 16:55

Removing unnecessary comment in jwt.d.ts

6a6d3b4

Co-authored-by: KaKa <climba03003@gmail.com>

Added TODO task for disabled tests

df87c80

mcollina requested review from delvedor, jsumners and kibertoad October 25, 2021 22:43

radomird requested a review from climba03003 October 26, 2021 08:00

climba03003 reviewed Oct 26, 2021

View reviewed changes

jwt.js Outdated Show resolved Hide resolved

mcollina approved these changes Oct 26, 2021

View reviewed changes

mcollina requested changes Oct 26, 2021

View reviewed changes

Extracting repeated code to a new function

bc36ce1

radomird requested a review from climba03003 October 26, 2021 10:53

climba03003 reviewed Oct 26, 2021

View reviewed changes

jwt.js Show resolved Hide resolved

Refactored repeating code

2aa247b

climba03003 reviewed Oct 27, 2021

View reviewed changes

Removed console.log statements

cc97627

darkgl0w approved these changes Oct 30, 2021

View reviewed changes

test/test.test.js Outdated Show resolved Hide resolved

test/test.test.js Outdated Show resolved Hide resolved

Updated fast-jwt to v1.3.2 and enabled tests

b482963

simoneb reviewed Nov 17, 2021

View reviewed changes

darkgl0w reviewed Nov 18, 2021

View reviewed changes

mcollina approved these changes Nov 18, 2021

View reviewed changes

README.md Show resolved Hide resolved

radomird and others added 2 commits November 18, 2021 10:11

Apply suggestions from code review

e18137f

Added new types for Sign and Verify Options, updated typings test Co-authored-by: darkgl0w <31093081+darkgl0w@users.noreply.github.com>

Added type definitions for node - fix failing CI build

3832bff

radomird mentioned this pull request Nov 18, 2021

Add JwtHeader definition to the type definitions nearform/fast-jwt#147

Closed

rmuchall mentioned this pull request Nov 19, 2021

Consider switching to fast-jwt for performance krgamestudios/auth-server#6

Closed

Eomm approved these changes Nov 19, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

chore: Updated docs with migration breaking changes and updated examples

c38ef95

darkgl0w reviewed Nov 19, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

radomird and others added 3 commits November 19, 2021 15:00

fix: typo in README

c9ad90a

Co-authored-by: darkgl0w <31093081+darkgl0w@users.noreply.github.com>

Added missing option in migration section in README

5c9ae34

Co-authored-by: darkgl0w <31093081+darkgl0w@users.noreply.github.com>

chore: Moved migration notes to separate file

bd7e2b5

darkgl0w reviewed Nov 19, 2021

View reviewed changes

UPGRADING.md Outdated Show resolved Hide resolved

mcollina reviewed Nov 20, 2021

View reviewed changes

mcollina reviewed Nov 21, 2021

View reviewed changes

UPGRADING.md Show resolved Hide resolved

Added few more migration points to UPGRADING.md

bb6dd06

Co-authored-by: darkgl0w <31093081+darkgl0w@users.noreply.github.com>

mcollina approved these changes Nov 23, 2021

View reviewed changes

climba03003 approved these changes Nov 24, 2021

View reviewed changes

mcollina approved these changes Nov 25, 2021

View reviewed changes

mcollina merged commit c3a30aa into fastify:master Nov 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Replacing jsonwebtoken with fast-jwt #184

feat: Replacing jsonwebtoken with fast-jwt #184

radomird commented Oct 19, 2021 •

edited

Loading

mcollina left a comment

mcollina left a comment

radomird commented Oct 26, 2021 •

edited

Loading

jsumners commented Oct 26, 2021

simoneb commented Oct 26, 2021 •

edited

Loading

climba03003 left a comment

darkgl0w left a comment •

edited

Loading

simoneb left a comment

simoneb Nov 17, 2021

darkgl0w left a comment

mcollina left a comment

mcollina commented Nov 18, 2021

radomird commented Nov 18, 2021 •

edited

Loading

Eomm left a comment

simoneb commented Nov 19, 2021

radomird commented Nov 19, 2021

darkgl0w left a comment

darkgl0w left a comment

mcollina left a comment

simoneb commented Nov 21, 2021

radomird commented Nov 22, 2021

mcollina left a comment

radomird commented Nov 24, 2021

climba03003 left a comment

Uzlopak commented Nov 24, 2021

mcollina left a comment

lukeed commented Nov 25, 2021

		@@ -1,5 +1,5 @@
		import { DecoderOptions, KeyFetcher, SignerCallback, SignerOptions, VerifierCallback, VerifierOptions } from 'fast-jwt'

feat: Replacing jsonwebtoken with fast-jwt #184

feat: Replacing jsonwebtoken with fast-jwt #184

Conversation

radomird commented Oct 19, 2021 • edited Loading

Checklist

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

radomird commented Oct 26, 2021 • edited Loading

jsumners commented Oct 26, 2021

simoneb commented Oct 26, 2021 • edited Loading

climba03003 left a comment

Choose a reason for hiding this comment

darkgl0w left a comment • edited Loading

Choose a reason for hiding this comment

simoneb left a comment

Choose a reason for hiding this comment

simoneb Nov 17, 2021

Choose a reason for hiding this comment

darkgl0w left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Nov 18, 2021

radomird commented Nov 18, 2021 • edited Loading

Eomm left a comment

Choose a reason for hiding this comment

simoneb commented Nov 19, 2021

radomird commented Nov 19, 2021

darkgl0w left a comment

Choose a reason for hiding this comment

darkgl0w left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

simoneb commented Nov 21, 2021

radomird commented Nov 22, 2021

mcollina left a comment

Choose a reason for hiding this comment

radomird commented Nov 24, 2021

climba03003 left a comment

Choose a reason for hiding this comment

Uzlopak commented Nov 24, 2021

mcollina left a comment

Choose a reason for hiding this comment

lukeed commented Nov 25, 2021

radomird commented Oct 19, 2021 •

edited

Loading

radomird commented Oct 26, 2021 •

edited

Loading

simoneb commented Oct 26, 2021 •

edited

Loading

darkgl0w left a comment •

edited

Loading

radomird commented Nov 18, 2021 •

edited

Loading