Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add options for jwks-rsa to enable cache and rate limiting #6

Merged
merged 11 commits into from
Feb 25, 2019

Conversation

cdwills
Copy link
Contributor

@cdwills cdwills commented Feb 6, 2019

Add options to take advantage of the built in caching from jwks-rsa. Description is here: https://github.com/auth0/node-jwks-rsa#caching

To see the caching in action add DEBUG=jwks to your local env for the previous version vs this one. You'll see the caching configured and in action.

image

Bonus:

  • bumped eslint, nyc, nock
  • bumped boom
  • added option to run Travis tests in node 10.
  • I believe we can remove support for node 6, 7 in the future.

@cdwills cdwills self-assigned this Feb 6, 2019
@cdwills cdwills added the enhancement New feature or request label Feb 6, 2019
Copy link

@dbertram dbertram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possible bug around merging of jwks option defaults, but otherwise lgtm

.travis.yml Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@cdwills
Copy link
Contributor Author

cdwills commented Feb 14, 2019

@dbertram great catch on the merge, i changed it up. Tested locally and feeling good about this. Would like some final 👀

Copy link

@dbertram dbertram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like one piece of 🧹ing left and this lgtm

index.js Outdated Show resolved Hide resolved
@dbertram
Copy link

image

@pklingem
Copy link
Member

👀

Copy link
Member

@pklingem pklingem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just the one comment, other than that, I can confirm that I'm seeing caching on my side. First request shows the keys, subsequent requests do not.

README.md Outdated

## Options

`authentic` accepts a JSON object of options that will be passed to the underlying libraries responsibile for validation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the options section is a bit confusing. Am I correct that any options other that jwks or issWhitelist will be passed to jwt.verify()? If so, should it be:

{
  verify: { ... },
  jwks: { ... },
  issWhitelist: ...,
}

for some futureproofing? It seems odd to separate jwks' options, but not verify's and I could imagine that causing conflicts eventually.

@dbertram
Copy link

@pklingem @cdwills Looks like the CI builds are failing due to reduced code coverage:

ERROR: Coverage for branches (80%) does not meet global threshold (100%)
----------|----------|----------|----------|----------|-------------------|
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files | 100 | 80 | 100 | 100 | |
index.js | 100 | 80 | 100 | 100 | 45,46 |
----------|----------|----------|----------|----------|-------------------|
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
The command "yarn run test:ci" exited with 1.

:sadpanda:

@cdwills
Copy link
Contributor Author

cdwills commented Feb 22, 2019

💯

@dbertram
Copy link

image

@cdwills cdwills merged commit 0d45e63 into master Feb 25, 2019
@lucasadrianof lucasadrianof deleted the feature/add-jwks-caching branch January 3, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants