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

[SDK-1221] Modify Cache Defaults #123

Merged
merged 1 commit into from
Feb 5, 2020
Merged

[SDK-1221] Modify Cache Defaults #123

merged 1 commit into from
Feb 5, 2020

Conversation

davidpatrick
Copy link
Contributor

Description

In order to allow for better expiration of keys after a signing key rotation the following changes have been made to the cache:

  • enables cache by default
  • changes cache time from 10h to 10m
  • updated tests to reflect the intent of the cache

Testing

Updated the tests to more properly reflect the caching mechanism.

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@davidpatrick davidpatrick requested a review from a team January 31, 2020 20:06
@davidpatrick davidpatrick force-pushed the cacheChanges branch 2 times, most recently from cf941c9 to 88f7295 Compare February 4, 2020 17:26
@@ -14,6 +14,7 @@ describe('JwksClient (cache)', () => {
describe('#getSigningKeys', () => {
it('should prevent too many requests', (done) => {
const client = new JwksClient({
cache: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously the cache was off by default for this test, turning off the cache to not change functionality in this test

@jimmyjames jimmyjames requested review from jimmyjames and removed request for a team February 4, 2020 17:41
Copy link
Contributor

@jimmyjames jimmyjames left a comment

Choose a reason for hiding this comment

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

Changes look good, the README just needs a little updating for clarity.

README.md Outdated
@@ -39,15 +39,15 @@ Integrations are also provided with:

### Caching

In order to prevent a call to be made each time a signing key needs to be retrieved you can also configure a cache as follows. If a signing key matching the `kid` is found, this will be cached and the next time this `kid` is requested the signing key will be served from the cache instead of calling back to the JWKS endpoint.
In order to prevent a call to be made each time a signing key needs to be retrieved a cache is implemented. If a signing key matching the `kid` is found, this will be cached and the next time this `kid` is requested the signing key will be served from the cache instead of calling back to the JWKS endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

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

The first sentence reads a bit awkwardly. Perhaps something like By default, signing keys are cached to prevent....

- enables cache by default (previously not)
- changes cache time from 10h to 10m
- updated tests to reflect the intent of the cache
@davidpatrick davidpatrick merged commit 73a087d into master Feb 5, 2020
@stevehobbsdev stevehobbsdev deleted the cacheChanges branch February 7, 2020 11:13
@stevehobbsdev stevehobbsdev changed the title Modify Cache Defaults [SDK-1221] Modify Cache Defaults Feb 7, 2020
@davidpatrick davidpatrick added this to the 1.7.0 milestone Feb 18, 2020
@davidpatrick davidpatrick mentioned this pull request Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants