Added caching to well-known configuration #48

lboyette-okta · 2016-10-15T01:11:12Z

To get the authorization_endpoint, userinfo_endpoint, and jwks_uri, we have to call .well-known/openid-configuration. This information rarely changes (if ever), and we use the information often, so to prevent unnecessary api calls, we cache the result.

Resolves: OKTA-100724

magizh-okta · 2016-10-18T01:00:40Z

Description in the commit ? Would be helpful for other devs to understand why this PR is needed.

rchild-okta · 2016-10-18T03:15:42Z

test/spec/oauthUtil.js

+  var wellKnown = require('../xhr/well-known');
+
+  describe('getWellKnown', function() {
+    util.itMakesCorrectRequestResponse({


Here it might be better to just test that "we get well known, and use the cached version when it's stored", i.e. the end-to-end where we can verify that we got the same response both times, but only made a request once

ok, it's easy enough to add an extra getWellKnown request

rchild-okta · 2016-10-18T03:17:03Z

test/spec/oauthUtil.js

+  var wellKnown = require('../xhr/well-known');
+
+  describe('getWellKnown', function() {
+    util.itMakesCorrectRequestResponse({


It also seems like we probably need a test for not using the cached item if it exceeds the time when it expired (1 day)

yup, will add

rchild-okta · 2016-10-18T03:20:51Z

lib/http.js

@@ -4,15 +4,26 @@ var cookies = require('./cookies');
 var Q = require('q');
 var AuthApiError = require('./errors/AuthApiError');
 var config = require('./config');
+var storageBuilder = require('./storageBuilder');
+
+var httpCache = storageBuilder(localStorage, config.CACHE_STORAGE_NAME);


Something that we might want to consider for the future, but it seems like overkill to have a separate instance of storage for every storage key (as opposed to having 1 localStorage, and 1 cookieStorage for example that each know how to lookup the val given a key).

fair enough

rchild-okta · 2016-10-18T03:22:43Z

lib/http.js

+  if (options.responseCacheDuration) {
+    var cacheContents = httpCache.getStorage();
+    var cachedResponse = cacheContents[url];
+    if (cachedResponse && Date.now()/1000 < cachedResponse.expiresAt) {


Can we simplify and remove the math (i.e. just use Date.now() and work in ms) since we control both ends and don't plan on exposing this outwards?

I had it that way originally, but thought this way was easier to read (and easier to understand when debugging the cache)

rchild-okta · 2016-10-18T03:26:58Z

lib/http.js

+  if (options.responseCacheDuration) {
+    var cacheContents = httpCache.getStorage();
+    var cachedResponse = cacheContents[url];
+    if (cachedResponse && Date.now()/1000 < cachedResponse.expiresAt) {


I'm also curious how we're planning on handling the keys call, specifically:

First time user, gets keys, it's cached

Second time coming in, cached and kid is still there (no problem)

Keys rotate

Make request, is cached, kid is not there; make request again <-- what happens if for whatever reason the kid is not there?

I.e. it doesn't look like we expose whether we retrieved the response from the cache or not - is that going to be a problem when we need to have logic like "see what we have, looks like it's not there, make a request to get the new info".

Code from the next PR

function getKey(sdk, issuer, kid) { return getWellKnown(sdk, issuer) .then(function(wellKnown) { // Generate a jwks uri var jwksUri = wellKnown['jwks_uri']; // Check our kid against the cached version (if it exists) var cacheContents = httpCache.getStorage(); var cachedResponse = cacheContents[jwksUri]; var cachedKey = util.find(cachedResponse.keys, { kid: kid }); if (cachedKey) { return cachedKey; } // Pull the latest keys if the key wasn't in the cache return http.get(jwksUri, { responseCacheDuration: config.DEFAULT_CACHE_DURATION }) .then(function(keys) { var key = util.find(keys, { kid: kid }); if (key) { return key; } throw new AuthSdkError('The key id, ' + kid + ', was not found in the server\'s keys'); }); }); }

rchild-okta · 2016-10-18T03:29:44Z

lib/http.js

      accessToken = options.accessToken;

+  if (options.responseCacheDuration) {


Do we actually need to pass in responseCacheDuration, or can we just get it from the config? I.e. are we planning on exposing this to the user later on down the line?

It could change depending on the response you're trying to cache (in theory we could store the keys indefinitely and only request new ones when key validation fails). I guess I'm defaulting to 2 weeks for everything now, so I'll just change that here.

rchild-okta · 2016-10-18T03:32:26Z

lib/http.js

@@ -47,6 +58,15 @@ function httpRequest(sdk, options) {
        cookies.setCookie(config.STATE_TOKEN_COOKIE_NAME, res.stateToken, res.expiresAt);
      }

+      if (res && options.responseCacheDuration) {
+        var cache = httpCache.getStorage();


It might be nice to provide a method for "updateStorage" - namely, underneath the hood it's going to get, write, and set, but we can just set a new url with the value here.

makes sense to me

rchild-okta · 2016-10-18T03:33:38Z

lib/http.js

    url: url,
    method: 'POST',
    args: args,
-    dontSaveResponse: dontSaveResponse
-  });
+    cacheState: true


Hmm, why is cacheState true for post? When would we actually cache a post response?

rchild-okta · 2016-10-18T03:34:28Z

lib/oauthUtil.js

@@ -45,7 +46,9 @@ function loadPopup(src, options) {

 function getWellKnown(sdk) {
  // TODO: Use the issuer when known (usually from the id_token)
-  return http.get(sdk, sdk.options.url + '/.well-known/openid-configuration');
+  return http.get(sdk, sdk.options.url + '/.well-known/openid-configuration', {
+    responseCacheDuration: config.DEFAULT_CACHE_DURATION


Related to other comment, but seems like it'd be nicer to make the a boolean "cacheResponse" and just load the time from the config. Or do you foresee having different cache timeouts for different requests?

I'm fine with changing everything to use DEFAULT_CACHE_DURATION for now.

rchild-okta · 2016-10-18T03:38:39Z

lib/http.js

    url: url,
    method: 'POST',
    args: args,
-    dontSaveResponse: dontSaveResponse
-  });
+    cacheState: true


Ah, I think I see what's going on here - the "dontSaveResponse" from before was the idea of remembering the previous response in the authn flow.

I think "cacheState" is a confusing variable name now that we have a cache that persists past the current auth flow.

cacheAuthnState?

rchild-okta · 2016-10-20T20:53:21Z

lib/http.js

@@ -13,10 +13,10 @@ function httpRequest(sdk, options) {
  var url = options.url,
      method = options.method,
      args = options.args,
-      cacheState = options.cacheState,
+      cacheAuthnState = options.cacheAuthnState,


A bit pedantic, but how about "saveAuthnState"? The difference for me is that we're using "cacheResponse" to mean "let's save this response for up to the DEFAULT_CACHE_DURATION, and I do not expect the response to change very frequently", where authState gets updated on every request and is expected to hold different values as the user goes through the auth flow

I buy that, will change

rchild-okta

Other changes look good though, so update variable name and then 🚀

lboyette-okta force-pushed the lb-100724-caching branch from de9baaa to f6cb272 Compare October 17, 2016 22:42

Added caching to well-known configuration

fe5c376

Resolves: OKTA-100724

lboyette-okta force-pushed the lb-100724-caching branch from f6cb272 to fe5c376 Compare October 17, 2016 23:34

rchild-okta suggested changes Oct 18, 2016

View reviewed changes

Addressed comments

762c7f8

rchild-okta reviewed Oct 20, 2016

View reviewed changes

rchild-okta approved these changes Oct 20, 2016

View reviewed changes

cacheAuthnState => saveAuthnState

45da199

lboyette-okta merged commit af21c3b into master Oct 20, 2016

jmelberg-okta deleted the lb-100724-caching branch April 23, 2018 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added caching to well-known configuration #48

Added caching to well-known configuration #48

lboyette-okta commented Oct 15, 2016 •

edited

Loading

magizh-okta commented Oct 18, 2016

rchild-okta Oct 18, 2016

lboyette-okta Oct 19, 2016

rchild-okta Oct 18, 2016

lboyette-okta Oct 19, 2016

rchild-okta Oct 18, 2016

lboyette-okta Oct 19, 2016

rchild-okta Oct 18, 2016

lboyette-okta Oct 19, 2016

rchild-okta Oct 18, 2016

lboyette-okta Oct 19, 2016

rchild-okta Oct 18, 2016

lboyette-okta Oct 19, 2016

rchild-okta Oct 18, 2016

lboyette-okta Oct 19, 2016

rchild-okta Oct 18, 2016

rchild-okta Oct 18, 2016

lboyette-okta Oct 19, 2016

rchild-okta Oct 18, 2016

lboyette-okta Oct 19, 2016

rchild-okta Oct 20, 2016

lboyette-okta Oct 20, 2016

rchild-okta left a comment

		accessToken = options.accessToken;

		if (options.responseCacheDuration) {

Added caching to well-known configuration #48

Added caching to well-known configuration #48

Conversation

lboyette-okta commented Oct 15, 2016 • edited Loading

magizh-okta commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchild-okta left a comment

Choose a reason for hiding this comment

lboyette-okta commented Oct 15, 2016 •

edited

Loading