Skip to content

Conversation

@srinandan
Copy link
Contributor

The oauth plugin will support three new parameters:

  • cacheKeySize (to set the number of api keys that will be stored in cache)
  • cacheKeyTTL (to set the time-to-live for apikeys in cache; note: the entry will also be determined by the JWT expiry)
  • tokenKeyTTL (to set the time-to-live for oauth tokens in cache; note: the entry will also be determined by the JWT expiry)

The oauthv2 plugin will support a new parameter:

  • tokenKeyTTL (to set the time-to-live for oauth tokens in cache; note: the entry will also be determined by the JWT expiry)

The apikey plugin will support two new parameters:

  • cacheKeySize (to set the number of api keys that will be stored in cache)
  • cacheKeyTTL (to set the time-to-live for apikeys in cache; note: the entry will also be determined by the JWT expiry)

@WWitman
Copy link

WWitman commented Jan 6, 2019

This PR requires documentation updates.

@swilliams11
Copy link
Contributor

swilliams11 commented Jan 7, 2019

I reviewed the plugins and naming conventions look good. I agree with @WWitman that we need the docs and the plugin README files to updated.

@WWitman
Copy link

WWitman commented Jan 8, 2019

We can do a beta doc update for this feature and others that go into the next beta. But I'll need to have an idea of when the beta is scheduled, if possible, so I can plan for it.

//cache api keys
cacheKey = config.hasOwnProperty("cacheKey") ? config.cacheKey : false;
//cache ttl
cacheKeyTTL = config.hasOwnProperty("cacheKeyTTL") ? config.cacheKeyTTL : 60000;
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't respect the default cacheKeyTTL value prev set on line 25 - avoid magic numbers. same for cacheSize

oauth/index.js Outdated
var keepAuthHeader = config.hasOwnProperty('keep-authorization-header') ? config['keep-authorization-header'] : false;
cacheKey = config.hasOwnProperty('cacheKey') ? config.cacheKey : false;
//cache ttl
cacheKeyTTL = config.hasOwnProperty("cacheKeyTTL") ? config.cacheKeyTTL : 60000;
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't respect the default cacheKeyTTL value prev set on line 35 - avoid magic numbers. same for other cache vars

oauthv2/index.js Outdated
//token cache settings
tokenCache = config.hasOwnProperty('tokenCache') ? config.tokenCache : false;
//token cache ttl
tokenCacheTTL = config.hasOwnProperty("tokenCacheTTL") ? config.cacheKeyTTL : 60000;
Copy link
Contributor

@relloller relloller Feb 4, 2019

Choose a reason for hiding this comment

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

doesn't respect the default tokenCacheTTL value prev set above avoid magic numbers. same for other cache vars

@srinandan
Copy link
Contributor Author

PTAL

@theganyo
Copy link
Member

It looks like this PR was abandoned and it has conflicts. (Also, it shouldn't be changing the version.) Was this in response to a specific feature request? Is it still needed? Or can this just be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants