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.

Sign up for GitHub

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

Added caching to well-known configuration #48

Merged
merged 3 commits into from
Oct 20, 2016
Merged

Added caching to well-known configuration #48

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe5c376
Added caching to well-known configuration
lboyette-okta Sep 27, 2016
762c7f8
Addressed comments
lboyette-okta Oct 20, 2016
45da199
cacheAuthnState => saveAuthnState
lboyette-okta Oct 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions lib/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ function httpRequest(sdk, options) {
var url = options.url,
method = options.method,
args = options.args,
cacheState = options.cacheState,
cacheAuthnState = options.cacheAuthnState,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I buy that, will change

accessToken = options.accessToken;

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. First time user, gets keys, it's cached
  2. Second time coming in, cached and kid is still there (no problem)
  3. Keys rotate
  4. 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".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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');
    });
  });
}

Expand Down Expand Up @@ -48,7 +48,7 @@ function httpRequest(sdk, options) {
res = JSON.parse(res);
}

if (cacheState) {
if (cacheAuthnState) {
if (!res.stateToken) {
cookies.deleteCookie(config.STATE_TOKEN_COOKIE_NAME);
}
Expand All @@ -58,13 +58,11 @@ function httpRequest(sdk, options) {
cookies.setCookie(config.STATE_TOKEN_COOKIE_NAME, res.stateToken, res.expiresAt);
}

if (res && options.responseCacheDuration) {
var cache = httpCache.getStorage();
cache[url] = {
expiresAt: Math.floor(Date.now()/1000) + options.responseCacheDuration,
if (res && options.cacheResponse) {
httpCache.updateStorage(url, {
expiresAt: Math.floor(Date.now()/1000) + config.DEFAULT_CACHE_DURATION,
response: res
};
httpCache.setStorage(cache);
});
}

return res;
Expand Down Expand Up @@ -115,7 +113,7 @@ function post(sdk, url, args, options) {
url: url,
method: 'POST',
args: args,
cacheState: true
cacheAuthnState: true
};
util.extend(postOptions, options);
return httpRequest(sdk, postOptions);
Expand Down
3 changes: 1 addition & 2 deletions lib/oauthUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
var http = require('./http');
var util = require('./util');
var AuthSdkError = require('./errors/AuthSdkError');
var config = require('./config');

function isToken(obj) {
if (obj &&
Expand Down Expand Up @@ -47,7 +46,7 @@ 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', {
responseCacheDuration: config.DEFAULT_CACHE_DURATION
cacheResponse: true
});
}

Expand Down
9 changes: 8 additions & 1 deletion lib/storageBuilder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,17 @@ function storageBuilder(webstorage, storageName) {
setStorage({});
}

function updateStorage(key, value) {
var storage = getStorage();
storage[key] = value;
setStorage(storage);
}

return {
getStorage: getStorage,
setStorage: setStorage,
clearStorage: clearStorage
clearStorage: clearStorage,
updateStorage: updateStorage
};
}

Expand Down
2 changes: 1 addition & 1 deletion lib/tx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ function getPollFn(sdk, res, ref) {
href += '?rememberDevice=true';
}
return http.post(sdk, href, getStateToken(res), {
cacheState: false
cacheAuthnState: false
});
}

Expand Down
49 changes: 47 additions & 2 deletions test/spec/oauthUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ define(function(require) {

describe('getWellKnown', function() {
util.itMakesCorrectRequestResponse({
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, will add

title: 'caches response',
title: 'caches response and uses cache on subsequent requests',
setup: {
calls: [
{
Expand All @@ -21,7 +21,10 @@ define(function(require) {
},
execute: function(test) {
localStorage.clear();
return oauthUtil.getWellKnown(test.oa);
return oauthUtil.getWellKnown(test.oa)
.then(function() {
return oauthUtil.getWellKnown(test.oa);
});
},
expectations: function() {
var cache = localStorage.getItem('okta-cache-storage');
Expand All @@ -46,6 +49,48 @@ define(function(require) {
}
}));
return oauthUtil.getWellKnown(test.oa);
},
expectations: function() {
var cache = localStorage.getItem('okta-cache-storage');
expect(cache).toEqual(JSON.stringify({
'https://auth-js-test.okta.com/.well-known/openid-configuration': {
expiresAt: 1449786329,
response: wellKnown.response
}
}));
}
});
util.itMakesCorrectRequestResponse({
title: 'doesn\'t use cached response if past cache expiration',
setup: {
calls: [
{
request: {
method: 'get',
uri: '/.well-known/openid-configuration'
},
response: 'well-known'
}
],
time: 1450000000
},
execute: function(test) {
localStorage.setItem('okta-cache-storage', JSON.stringify({
'https://auth-js-test.okta.com/.well-known/openid-configuration': {
expiresAt: 1449786329,
response: wellKnown.response
}
}));
return oauthUtil.getWellKnown(test.oa);
},
expectations: function() {
var cache = localStorage.getItem('okta-cache-storage');
expect(cache).toEqual(JSON.stringify({
'https://auth-js-test.okta.com/.well-known/openid-configuration': {
expiresAt: 1450086400,
response: wellKnown.response
}
}));
}
});
});
Expand Down