Skip to content

Commit

Permalink
fix: token renew promise sometimes does not resolve (#246)
Browse files Browse the repository at this point in the history 
* Origin mismatch will cause promise rejection

* TokenManager: return existing promise for concurrent requests
  • Loading branch information
@aarongranick-okta
aarongranick-okta authored Sep 9, 2019
1 parent e03585d commit 7db8766
Show file tree
Hide file tree
Showing 9 changed files with 335 additions and 21 deletions.
17 changes: 16 additions & 1 deletion .vscode/launch.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,26 @@
{
"type": "node",
"request": "launch",
"name": "Jest Current File",
"name": "Jest Current File (browser)",
"program": "${workspaceFolder}/node_modules/jest/bin/jest",
"args": [
"--runInBand",
"--no-cache",
"--config=jest.browser.js",
"${relativeFile}"
],
"console": "integratedTerminal",
"internalConsoleOptions": "neverOpen"
},
{
"type": "node",
"request": "launch",
"name": "Jest Current File (server)",
"program": "${workspaceFolder}/node_modules/jest/bin/jest",
"args": [
"--runInBand",
"--no-cache",
"--config=jest.server.js",
"${relativeFile}"
],
"console": "integratedTerminal",
Expand Down
21 changes: 13 additions & 8 deletions lib/TokenManager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,12 @@ function remove(tokenMgmtRef, storage, key) {
}

function renew(sdk, tokenMgmtRef, storage, key) {
// Multiple callers may receive the same promise. They will all resolve or reject from the same request.
var existingPromise = tokenMgmtRef.renewPromise[key];
if (existingPromise) {
return existingPromise;
}

try {
var token = get(storage, key);
if (!token) {
Expand All @@ -154,8 +160,7 @@ function renew(sdk, tokenMgmtRef, storage, key) {
clearExpireEventTimeout(tokenMgmtRef, key);

// Store the renew promise state, to avoid renewing again
if (!tokenMgmtRef.renewPromise[key]) {
tokenMgmtRef.renewPromise[key] = sdk.token.renew(token)
tokenMgmtRef.renewPromise[key] = sdk.token.renew(token)
.then(function(freshTokens) {
var freshToken = freshTokens;
// With PKCE flow we will receive multiple tokens. Find the one we are looking for
Expand All @@ -174,20 +179,20 @@ function renew(sdk, tokenMgmtRef, storage, key) {
}
add(sdk, tokenMgmtRef, storage, key, freshToken);
tokenMgmtRef.emitter.emit('renewed', key, freshToken, oldToken);

// Remove existing promise key
delete tokenMgmtRef.renewPromise[key];
return freshToken;
})
.fail(function(err) {
.catch(function(err) {
if (err.name === 'OAuthError') {
remove(tokenMgmtRef, storage, key);
emitError(tokenMgmtRef, err);
}

throw err;
})
.finally(function() {
// Remove existing promise key
delete tokenMgmtRef.renewPromise[key];
});
}

return tokenMgmtRef.renewPromise[key];
}

Expand Down
14 changes: 11 additions & 3 deletions lib/token.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,19 @@ function addPostMessageListener(sdk, timeout, state) {
var deferred = Q.defer();

function responseHandler(e) {
if (!e.data ||
e.origin !== sdk.options.url ||
(e.data && util.isString(state) && e.data.state !== state)) {
if (!e.data || e.data.state !== state) {
// A message not meant for us
return;
}

// Configuration mismatch between saved token and current app instance
// This may happen if apps with different issuers are running on the same host url
// If they share the same storage key, they may read and write tokens in the same location.
// Common when developing against http://localhost
if (e.origin !== sdk.options.url) {
return deferred.reject(new AuthSdkError('The request does not match client configuration'));
}

deferred.resolve(e.data);
}

Expand Down
4 changes: 3 additions & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@
"karma-sourcemap-loader": "^0.3.7",
"karma-webpack": "^3.0.5",
"lodash": "4.17.11",
"promise.allsettled": "^1.0.1",
"promise.prototype.finally": "^3.1.1",
"webpack": "^3.0.0"
},
"okta-auth-js": {
Expand All @@ -79,4 +81,4 @@
"jest-junit": {
"output": "./build2/reports/unit/junit-result.xml"
}
}
}
3 changes: 3 additions & 0 deletions test/.eslintrc.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,8 @@
"env": {
"jest": true,
"jasmine": true
},
"globals": {
"Promise": "readonly"
}
}
141 changes: 141 additions & 0 deletions test/spec/token.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
jest.mock('cross-fetch');
var allSettled = require('promise.allsettled');
allSettled.shim(); // will be a no-op if not needed

var OktaAuth = require('OktaAuth');
var tokens = require('../util/tokens');
Expand All @@ -9,6 +11,7 @@ var _ = require('lodash');
var Q = require('q');
var sdkUtil = require('../../lib/oauthUtil');
var pkce = require('../../lib/pkce');
var waitFor = require('../util/waitFor');

function setupSync() {
return new OktaAuth({ issuer: 'http://example.okta.com' });
Expand Down Expand Up @@ -37,6 +40,144 @@ describe('token.decode', function() {
});

describe('token.getWithoutPrompt', function() {

describe('concurrent requests', function() {
var authClient;
var states;
var messageCallbacks;

function fireCallback(index, origin) {
var fn = messageCallbacks[index];
fn({
data: {
'id_token': tokens.standardIdToken,
state: states[index],
},
origin: origin || 'https://auth-js-test.okta.com'
});
}

beforeEach(function() {
var oktaAuthArgs = {
issuer: 'https://auth-js-test.okta.com',
clientId: 'NPSfOkH5eZrTy8PMDlvx',
redirectUri: 'https://example.com/redirect'
};
authClient = new OktaAuth(oktaAuthArgs);
states = [];
messageCallbacks = [];

// Mock the well-known and keys request
oauthUtil.loadWellKnownAndKeysCache();
oauthUtil.mockStateAndNonce();
util.warpToUnixTime(oauthUtil.getTime());

// Unique state per request
var stateCounter = 0;
jest.spyOn(sdkUtil, 'generateState').mockImplementation(function() {
stateCounter++;
states.push(stateCounter);
return states[states.length - 1];
});

// Simulate the postMessage between the window and the popup or iframe
jest.spyOn(window, 'addEventListener').mockImplementation(function(eventName, fn) {
if (eventName === 'message') {
messageCallbacks.push(fn);
}
});

// Capture the iframe
var body = document.getElementsByTagName('body')[0];
var origAppend = body.appendChild;
jest.spyOn(body, 'appendChild').mockImplementation(function (el) {
if (el.tagName === 'IFRAME') {
// Remove the src so it doesn't actually load
el.src = '';
return origAppend.call(this, el);
}
return origAppend.apply(this, arguments);
});

});

it('multiple valid will resolve', function() {
var p1 = authClient.token.getWithoutPrompt();
var p2 = authClient.token.getWithoutPrompt();
var p3 = authClient.token.getWithoutPrompt();
return waitFor(function() {
return messageCallbacks.length === 3;
}).then(function() {
// manually fire callbacks in mixed order. All promises should resolve
fireCallback(1);
fireCallback(2);
fireCallback(0);
return Promise.all([p1, p2, p3]);
});
});

it('multiple invalid will fail (authorizeUrl mismatch)', function() {
var p1 = authClient.token.getWithoutPrompt();
var p2 = authClient.token.getWithoutPrompt();
var p3 = authClient.token.getWithoutPrompt();
return waitFor(function() {
return messageCallbacks.length === 3;
}).then(function() {
// manually fire callbacks in mixed order. All promises should reject
fireCallback(1, 'bogus');
fireCallback(2, 'bogus');
fireCallback(0, 'bogus');
return Promise.allSettled([p1, p2, p3]);
}).then(function(results) {
expect(results).toHaveLength(3);
results.forEach(function(result) {
expect(result.status).toBe('rejected');
util.expectErrorToEqual(result.reason, {
name: 'AuthSdkError',
message: 'The request does not match client configuration',
errorCode: 'INTERNAL',
errorSummary: 'The request does not match client configuration',
errorLink: 'INTERNAL',
errorId: 'INTERNAL',
errorCauses: []
});
});
});
});
});

it('If authorizeUrl does not match configured issuer, promise will reject', function() {
return oauthUtil.setupFrame({
willFail: true,
oktaAuthArgs: {
issuer: 'https://auth-js-test.okta.com',
clientId: 'NPSfOkH5eZrTy8PMDlvx',
redirectUri: 'https://example.com/redirect'
},
getWithoutPromptArgs: [{
sessionToken: 'testSessionToken'
}, {
authorizeUrl: 'https://bogus',
}],
postMessageSrc: {
baseUri: 'https://bogus'
}
})
.then(function() {
expect(true).toEqual(false);
})
.fail(function(err) {
util.expectErrorToEqual(err, {
name: 'AuthSdkError',
message: 'The request does not match client configuration',
errorCode: 'INTERNAL',
errorSummary: 'The request does not match client configuration',
errorLink: 'INTERNAL',
errorId: 'INTERNAL',
errorCauses: []
});
});
});

it('returns id_token using sessionToken', function(done) {
return oauthUtil.setupFrame({
Expand Down
79 changes: 79 additions & 0 deletions test/spec/tokenManager.js
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
var allSettled = require('promise.allsettled');
allSettled.shim(); // will be a no-op if not needed

var promiseFinally = require('promise.prototype.finally');
promiseFinally.shim(); // will be a no-op if not needed

var OktaAuth = require('OktaAuth');
var tokens = require('../util/tokens');
var util = require('../util/util');
Expand Down Expand Up @@ -129,6 +135,79 @@ describe('TokenManager', function() {
});

describe('renew', function() {

it('multiple overlapping calls will produce a single request and promise', function() {
var client = setupSync();
client.tokenManager.add('test-idToken', tokens.standardIdTokenParsed);
jest.spyOn(client.token, 'renew').mockImplementation(function() {
return Promise.resolve(tokens.standardIdTokenParsed);
});
var p1 = client.tokenManager.renew('test-idToken');
var p2 = client.tokenManager.renew('test-idToken');
expect(p1).toBe(p2);
return Promise.all([p1, p2]);
});

it('multiple overlapping calls will produce a single request and promise (failure case)', function() {
var client = setupSync();
client.tokenManager.add('test-idToken', tokens.standardIdTokenParsed);
jest.spyOn(client.token, 'renew').mockImplementation(function() {
return Promise.reject(new Error('expected'));
});
var p1 = client.tokenManager.renew('test-idToken');
var p2 = client.tokenManager.renew('test-idToken');
expect(p1).toBe(p2);
return Promise.allSettled([p1, p2]).then(function(results) {
expect(results).toHaveLength(2);
results.forEach(function(result) {
expect(result.status).toBe('rejected');
util.expectErrorToEqual(result.reason, {
name: 'Error',
message: 'expected',
});
});
});
});

it('sequential calls will produce a unique request and promise', function() {
var client = setupSync();
client.tokenManager.add('test-idToken', tokens.standardIdTokenParsed);
jest.spyOn(client.token, 'renew').mockImplementation(function() {
return Promise.resolve(tokens.standardIdTokenParsed);
});
var p1 = client.tokenManager.renew('test-idToken').then(function() {
var p2 = client.tokenManager.renew('test-idToken');
expect(p1).not.toBe(p2);
return p2;
});
});

it('sequential calls will produce a unique request and promise (failure case)', function() {
var client = setupSync();
client.tokenManager.add('test-idToken', tokens.standardIdTokenParsed);
jest.spyOn(client.token, 'renew').mockImplementation(function() {
return Promise.reject(new Error('expected'));
});
var p1 = client.tokenManager.renew('test-idToken').then(function() {
expect(false).toBe(true);
}).catch(function(err) {
util.expectErrorToEqual(err, {
name: 'Error',
message: 'expected',
});
var p2 = client.tokenManager.renew('test-idToken');
expect(p1).not.toBe(p2);
return p2;
}).then(function() {
expect(false).toBe(true);
}).catch(function(err) {
util.expectErrorToEqual(err, {
name: 'Error',
message: 'expected',
});
});
});

it('allows renewing an idToken', function() {
return oauthUtil.setupFrame({
authClient: setupSync(),
Expand Down
Loading

0 comments on commit 7db8766

Please sign in to comment.