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
23 changes: 12 additions & 11 deletions lib/TokenManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
var util = require('./util');
var AuthSdkError = require('./errors/AuthSdkError');
var cookies = require('./cookies');
var tokenStorageBuilder = require('./tokenStorageBuilder');
var storageBuilder = require('./storageBuilder');
var Q = require('q');
var Emitter = require('tiny-emitter');
var config = require('./config');

// Provides webStorage-like interface for cookies
var cookieStorage = {
Expand Down Expand Up @@ -58,7 +59,7 @@ function setRefreshTimeout(sdk, tokenMgmtRef, storage, key, token) {

function setRefreshTimeoutAll(sdk, tokenMgmtRef, storage) {
try {
var tokenStorage = storage.getTokenStorage();
var tokenStorage = storage.getStorage();
} catch(e) {
// Any errors thrown on instantiation will not be caught,
// because there are no listeners yet
Expand All @@ -76,20 +77,20 @@ function setRefreshTimeoutAll(sdk, tokenMgmtRef, storage) {
}

function add(sdk, tokenMgmtRef, storage, key, token) {
var tokenStorage = storage.getTokenStorage();
var tokenStorage = storage.getStorage();
if (!util.isObject(token) ||
!token.scopes ||
(!token.expiresAt && token.expiresAt !== 0) ||
(!token.idToken && !token.accessToken)) {
throw new AuthSdkError('Token must be an Object with scopes, expiresAt, and an idToken or accessToken properties');
}
tokenStorage[key] = token;
storage.setTokenStorage(tokenStorage);
storage.setStorage(tokenStorage);
setRefreshTimeout(sdk, tokenMgmtRef, storage, key, token);
}

function get(storage, key) {
var tokenStorage = storage.getTokenStorage();
var tokenStorage = storage.getStorage();
return tokenStorage[key];
}

Expand All @@ -98,9 +99,9 @@ function remove(tokenMgmtRef, storage, key) {
clearRefreshTimeout(tokenMgmtRef, key);

// Remove it from storage
var tokenStorage = storage.getTokenStorage();
var tokenStorage = storage.getStorage();
delete tokenStorage[key];
storage.setTokenStorage(tokenStorage);
storage.setStorage(tokenStorage);
}

function refresh(sdk, tokenMgmtRef, storage, key) {
Expand Down Expand Up @@ -133,7 +134,7 @@ function refresh(sdk, tokenMgmtRef, storage, key) {

function clear(tokenMgmtRef, storage) {
clearRefreshTimeoutAll(tokenMgmtRef);
storage.clearTokenStorage();
storage.clearStorage();
}

function TokenManager(sdk, options) {
Expand All @@ -146,13 +147,13 @@ function TokenManager(sdk, options) {
var storage;
switch(options.storage) {
case 'localStorage':
storage = tokenStorageBuilder(localStorage);
storage = storageBuilder(localStorage, config.TOKEN_STORAGE_NAME);
break;
case 'sessionStorage':
storage = tokenStorageBuilder(sessionStorage);
storage = storageBuilder(sessionStorage, config.TOKEN_STORAGE_NAME);
break;
case 'cookie':
storage = tokenStorageBuilder(cookieStorage);
storage = storageBuilder(cookieStorage, config.TOKEN_STORAGE_NAME);
break;
default:
throw new AuthSdkError('Unrecognized storage option');
Expand Down
45 changes: 34 additions & 11 deletions lib/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough


function httpRequest(sdk, options) {
options = options || {};
var url = options.url,
method = options.method,
args = options.args,
dontSaveResponse = options.dontSaveResponse,
cacheState = options.cacheState,
accessToken = options.accessToken;

if (options.responseCacheDuration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

return Q.resolve(cachedResponse.response);
}
}

var headers = {
'Accept': 'application/json',
'Content-Type': 'application/json',
Expand All @@ -37,7 +48,7 @@ function httpRequest(sdk, options) {
res = JSON.parse(res);
}

if (!dontSaveResponse) {
if (cacheState) {
if (!res.stateToken) {
cookies.deleteCookie(config.STATE_TOKEN_COOKIE_NAME);
}
Expand All @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense to me

cache[url] = {
expiresAt: Math.floor(Date.now()/1000) + options.responseCacheDuration,
response: res
};
httpCache.setStorage(cache);
}

return res;
})
.fail(function(resp) {
Expand Down Expand Up @@ -79,23 +99,26 @@ function httpRequest(sdk, options) {
});
}

function get(sdk, url, saveResponse) {
function get(sdk, url, options) {
url = util.isAbsoluteUrl(url) ? url : sdk.options.url + url;
return httpRequest(sdk, {
var getOptions = {
url: url,
method: 'GET',
dontSaveResponse: !saveResponse
});
method: 'GET'
};
util.extend(getOptions, options);
return httpRequest(sdk, getOptions);
}

function post(sdk, url, args, dontSaveResponse) {
function post(sdk, url, args, options) {
url = util.isAbsoluteUrl(url) ? url : sdk.options.url + url;
return httpRequest(sdk, {
var postOptions = {
url: url,
method: 'POST',
args: args,
dontSaveResponse: dontSaveResponse
});
cacheState: true
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cacheAuthnState?

};
util.extend(postOptions, options);
return httpRequest(sdk, postOptions);
}

module.exports = {
Expand Down
5 changes: 4 additions & 1 deletion lib/oauthUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
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 @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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'm fine with changing everything to use DEFAULT_CACHE_DURATION for now.

});
}

function validateClaims(sdk, claims, aud, iss) {
Expand Down
3 changes: 1 addition & 2 deletions lib/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ function getSession(sdk) {
function closeSession(sdk) {
return http.httpRequest(sdk, {
url: sdk.options.url + '/api/v1/sessions/me',
method: 'DELETE',
dontSaveResponse: true
method: 'DELETE'
});
}

Expand Down
35 changes: 35 additions & 0 deletions lib/storageBuilder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
var AuthSdkError = require('./errors/AuthSdkError');

// storage must have getItem and setItem
function storageBuilder(webstorage, storageName) {
function getStorage() {
var storageString = webstorage.getItem(storageName);
storageString = storageString || '{}';
try {
return JSON.parse(storageString);
} catch(e) {
throw new AuthSdkError('Unable to parse storage string: ' + storageName);
}
}

function setStorage(storage) {
try {
var storageString = JSON.stringify(storage);
webstorage.setItem(storageName, storageString);
} catch(e) {
throw new AuthSdkError('Unable to set storage: ' + storageName);
}
}

function clearStorage() {
setStorage({});
}

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

module.exports = storageBuilder;
1 change: 0 additions & 1 deletion lib/token.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,7 +590,6 @@ function getUserInfo(sdk, accessTokenObject) {
return http.httpRequest(sdk, {
url: accessTokenObject.userinfoUrl,
method: 'GET',
dontSaveResponse: true,
accessToken: accessTokenObject.accessToken
})
.fail(function(err) {
Expand Down
36 changes: 0 additions & 36 deletions lib/tokenStorageBuilder.js
View file
Open in desktop

This file was deleted.

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

ref.isPolling = true;
Expand Down
7 changes: 5 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@
"STATE_TOKEN_COOKIE_NAME": "oktaStateToken",
"DEFAULT_POLLING_DELAY": 500,
"DEFAULT_MAX_CLOCK_SKEW": 300,
"DEFAULT_CACHE_DURATION": 86400,
"FRAME_ID": "okta-oauth-helper-frame",
"REDIRECT_OAUTH_PARAMS_COOKIE_NAME": "okta-oauth-redirect-params",
"TOKEN_STORAGE_NAME": "okta-token-storage"
"TOKEN_STORAGE_NAME": "okta-token-storage",
"CACHE_STORAGE_NAME": "okta-cache-storage"
}
}
}
48 changes: 48 additions & 0 deletions test/spec/oauthUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,54 @@
define(function(require) {
var OktaAuth = require('OktaAuth');
var oauthUtil = require('../../lib/oauthUtil');
var util = require('../util/util');
var wellKnown = require('../xhr/well-known');

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',
setup: {
calls: [
{
request: {
method: 'get',
uri: '/.well-known/openid-configuration'
},
response: 'well-known'
}
],
time: 1449699929
},
execute: function(test) {
localStorage.clear();
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: 'uses cached response',
setup: {
time: 1449699929
},
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);
}
});
});

describe('getOAuthUrls', function() {
function setupOAuthUrls(options) {
Expand Down
4 changes: 2 additions & 2 deletions test/spec/tokenManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,9 @@ define(function(require) {
.fail(function(err) {
util.expectErrorToEqual(err, {
name: 'AuthSdkError',
message: 'Unable to parse token storage string',
message: 'Unable to parse storage string: okta-token-storage',
errorCode: 'INTERNAL',
errorSummary: 'Unable to parse token storage string',
errorSummary: 'Unable to parse storage string: okta-token-storage',
errorLink: 'INTERNAL',
errorId: 'INTERNAL',
errorCauses: []
Expand Down
Loading