feat: useJWT() caches JWKS

[peterklingelhofer]

This should help us avoid running into the rate limit error, as there's no reason to spam the JWKS endpoint if we already have it available:

ERR JwksRateLimitError: Too many requests to the JWKS endpoint
    at /usr/src/app/node_modules/jwks-rsa/src/wrappers/rateLimit.js:21:16
    at processTicksAndRejections (node:internal/process/task_queues:77:11)

We should check the cache first, and in lieu of a valid cached value, then check the endpoint. This should probably also increase performance a good bit...

@EmrysMyrddin you've helped me out with the useJWT() plugin before, maybe you'd have time to take a look at this at some point? :)

[changeset-bot]

⚠️ No Changeset found

Latest commit: 4200529

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[EmrysMyrddin]

LGTM, just added a minor comment :-)

Thank you for your contribution!

[peterklingelhofer]

Reverting these rejection error text changes, as they shouldn't have been changed.

[bgentry]

Doesn't the underlying JwksClient already have a cache built in? (source) Why would we need to add an additional caching layer on top of this?

Also see my PR from today #3182 which exposes the additional config options in the JwksClient, allowing one to configure more of the cache settings as desired.

[peterklingelhofer]

Doesn't the underlying JwksClient already have a cache built in? (source) Why would we need to add an additional caching layer on top of this?

Also see my PR from today #3182 which exposes the additional config options in the JwksClient, allowing one to configure more of the cache settings as desired.

Interesting, the cache wasn't working as intended for our use case (100+ queries a second), the plugin keeps pinging the issuer for every request and I quickly get rate limited (error in original PR description).

That being said, I'm now just using my own JWT middleware in front of GraphQL Yoga since the rate limiting errors we were getting made the plugin unusable - happy to close this PR unmerged as I don't need the changes anymore myself. 😀

[bgentry]

@peterklingelhofer anything worth sharing or resources you found helpful? I’m dealing with some other shortcomings of the plugin as well (wanting to have the validated jwt available during my context builders so I can use it to load the current user) and may try another approach.

[peterklingelhofer]

@peterklingelhofer anything worth sharing or resources you found helpful? I’m dealing with some other shortcomings of the plugin as well (wanting to have the validated jwt available during my context builders so I can use it to load the current user) and may try another approach.

The useJWT() plugin has worked great for me outside of getting rate limited when attempting to fetch JWKS (which the changes in this PR resolve, at least for my situation).

If you haven't seen this comment about useExtendContext and useJWT(), this might help you: #3094 (comment)

[EmrysMyrddin]

Oh so sorry to have dropped the ball here!
Do you still think this is a good addition ? From what I see in the test, it's clear that the cache was actually not working ?

@bgentry If you have the possibility to test that the cache is working with your PR by taking inspiration of the tests of this PR ?

[peterklingelhofer]

Oh so sorry to have dropped the ball here!

Do you still think this is a good addition ? From what I see in the test, it's clear that the cache was actually not working ?

@bgentry If you have the possibility to test that the cache is working with your PR by taking inspiration of the tests of this PR ?

No problem at all @EmrysMyrddin!

I pushed a few commits since the last time the test workflow was run, you could try re-running to see if that issue was addressed (initially, I had changed an error message text or two that the tests were explicitly checking for, which I shouldn't have).

That being said, I suspect @bgentry is right and this seems to be a problem with the jwks-rsa and not the useJWT() plugin specifically, so I'm hesitant to think that merging this PR would be a good idea as it's not addressing the root cause of the problem.

[bgentry]

I can definitely do some testing and debugging of the JwksClient cache, but it would be a lot easier to do that if #3182 is merged and released first. Mind taking a look at that one @EmrysMyrddin ?

[EmrysMyrddin]

Ok, let me review @bgentry PR tomorrow and see if we can use the test defined here to check that the cache is working as expected 🙂

Then we will take the decision about this PR 🙂

Thank you both to take the time to push this subject forward

[EmrysMyrddin]

Sorry for the very long delay here...
I've finally merged @bgentry PR for JWKS client customization :-) Should be released soon, I'm just waiting for some other changes to be merged.

[peterklingelhofer]

Great, I'll do some more testing when the new release is out. I'm betting it'll probably solve / allow me to solve the issue I was seeing.