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

Add OAuth refresh support #221

Merged
merged 3 commits into from
Feb 9, 2017
Merged

Add OAuth refresh support #221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f10b01e
Factor out tokenInfoFrom() function
seanh Feb 9, 2017
4331bfd
Factor out postToTokenUrl() function
seanh Feb 9, 2017
37eacfc
Refresh access tokens before they expire
seanh Feb 9, 2017
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
79 changes: 67 additions & 12 deletions src/sidebar/oauth-auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,77 @@ function auth($http, settings) {
var accessTokenPromise;
var tokenUrl = resolve('token', settings.apiUrl);

/**
* An object holding the details of an access token from the tokenUrl endpoint.
* @typedef {Object} TokenInfo
* @property {string} accessToken - The access token itself.
* @property {number} expiresIn - The lifetime of the access token,
* in seconds.
* @property {string} refreshToken - The refresh token that can be used to
* get a new access token.
*/

/**
* Return a new TokenInfo object from the given tokenUrl endpoint response.
* @param {Object} response - The HTTP response from a POST to the tokenUrl
* endpoint (an Angular $http response object).
* @returns {TokenInfo}
*/
function tokenInfoFrom(response) {
var data = response.data;
return {
accessToken: data.access_token,
expiresIn: data.expires_in,
Copy link
Member

Choose a reason for hiding this comment

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

In any variables that stores durations, it is helpful to make sure the units are clear (seconds, mills, nano?). This could be either via naming (eg. expiresInSecs) or via documentation of the type somewhere (eg. a JSDoc @typedef)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. expires_in is part of the OAuth spec. I did change it to javaScript case styling but I think maybe we should stick to the spec's names for things as much as possible? So I think adding a typedef sounds good.

refreshToken: data.refresh_token,
};
}

// Post the given data to the tokenUrl endpoint as a form submission.
// Return a Promise for the access token response.
function postToTokenUrl(data) {
data = queryString.stringify(data);
var requestConfig = {
headers: {'Content-Type': 'application/x-www-form-urlencoded'},
};
return $http.post(tokenUrl, data, requestConfig);
}

// Exchange the JWT grant token for an access token.
// See https://tools.ietf.org/html/rfc7523#section-4
function exchangeToken(grantToken) {
var data = queryString.stringify({
var data = {
grant_type: 'urn:ietf:params:oauth:grant-type:jwt-bearer',
assertion: grantToken,
});
var requestConfig = {
headers: {'Content-Type': 'application/x-www-form-urlencoded'},
};
return $http.post(tokenUrl, data, requestConfig)
.then(function (response) {
if (response.status !== 200) {
throw new Error('Failed to retrieve access token');
}
return response.data;
});
return postToTokenUrl(data).then(function (response) {
if (response.status !== 200) {
throw new Error('Failed to retrieve access token');
}
return tokenInfoFrom(response);
});
}

// Exchange the refresh token for a new access token and refresh token pair.
// See https://tools.ietf.org/html/rfc6749#section-6
function refreshAccessToken(refreshToken) {
var data = {grant_type: 'refresh_token', refresh_token: refreshToken};
postToTokenUrl(data).then(function (response) {
Copy link
Member

@robertknight robertknight Feb 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If this fails, the error will be silently dropped [1]. In general, a promise chain should either be returned from the function or rejection should be explicitly handled.

[1] Not always true - modern browsers have heuristics that they will use to warn about unhandled Promise rejections, but I think I still favour handling them explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I know it will be, but oauth error handling in the client is generally pretty nonexistent, so I want to do a separate PR (probably multiple PRs) for it.

var tokenInfo = tokenInfoFrom(response);
refreshAccessTokenBeforeItExpires(tokenInfo);
accessTokenPromise = Promise.resolve(tokenInfo.accessToken);
});
}

// Set a timeout to refresh the access token a few minutes before it expires.
function refreshAccessTokenBeforeItExpires(tokenInfo) {
var delay = tokenInfo.expiresIn * 1000;

// We actually have to refresh the access token _before_ it expires.
// If the access token expires in one hour, this should refresh it in
// about 55 mins.
delay = Math.floor(delay * 0.91);

window.setTimeout(refreshAccessToken, delay, tokenInfo.refreshToken);
Copy link
Member

Choose a reason for hiding this comment

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

Ah. I did not know that setTimeout accepted function params for the callback as argument.

}

function tokenGetter() {
Expand All @@ -45,7 +99,8 @@ function auth($http, settings) {

if (grantToken) {
accessTokenPromise = exchangeToken(grantToken).then(function (tokenInfo) {
return tokenInfo.access_token;
refreshAccessTokenBeforeItExpires(tokenInfo);
return tokenInfo.accessToken;
});
} else {
accessTokenPromise = Promise.resolve(null);
Expand Down
110 changes: 103 additions & 7 deletions src/sidebar/test/oauth-auth-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ describe('oauth auth', function () {
var nowStub;
var fakeHttp;
var fakeSettings;
var clock;

beforeEach(function () {
nowStub = sinon.stub(window.performance, 'now');
Expand All @@ -19,8 +20,9 @@ describe('oauth auth', function () {
post: sinon.stub().returns(Promise.resolve({
status: 200,
data: {
access_token: 'an-access-token',
access_token: 'firstAccessToken',
expires_in: DEFAULT_TOKEN_EXPIRES_IN_SECS,
refresh_token: 'firstRefreshToken',
},
})),
};
Expand All @@ -34,10 +36,13 @@ describe('oauth auth', function () {
};

auth = authService(fakeHttp, fakeSettings);

clock = sinon.useFakeTimers();
});

afterEach(function () {
performance.now.restore();
clock.restore();
});

describe('#tokenGetter', function () {
Expand All @@ -49,7 +54,7 @@ describe('oauth auth', function () {
assert.calledWith(fakeHttp.post, 'https://hypothes.is/api/token', expectedBody, {
headers: {'Content-Type': 'application/x-www-form-urlencoded'},
});
assert.equal(token, 'an-access-token');
assert.equal(token, 'firstAccessToken');
});
});

Expand All @@ -67,10 +72,10 @@ describe('oauth auth', function () {

it('should cache tokens for future use', function () {
return auth.tokenGetter().then(function () {
fakeHttp.post.reset();
resetHttpSpy();
return auth.tokenGetter();
}).then(function (token) {
assert.equal(token, 'an-access-token');
assert.equal(token, 'firstAccessToken');
assert.notCalled(fakeHttp.post);
});
});
Expand All @@ -80,9 +85,7 @@ describe('oauth auth', function () {
// the pending Promise for the first request again (and not send a second
// concurrent HTTP request).
it('should not make two concurrent access token requests', function () {
// Make $http.post() return a pending Promise (simulates an in-flight
// HTTP request).
fakeHttp.post.returns(new Promise(function() {}));
makeServerUnresponsive();

// The first time tokenGetter() is called it sends the access token HTTP
// request and returns a Promise for the access token.
Expand All @@ -109,5 +112,98 @@ describe('oauth auth', function () {
assert.equal(token, null);
});
});

it('should refresh the access token before it expires', function () {
function callTokenGetter () {
var tokenPromise = auth.tokenGetter();

fakeHttp.post.returns(Promise.resolve({
status: 200,
data: {
access_token: 'secondAccessToken',
expires_in: DEFAULT_TOKEN_EXPIRES_IN_SECS,
refresh_token: 'secondRefreshToken',
},
}));

return tokenPromise;
}

function assertRefreshTokenWasUsed (refreshToken) {
return function() {
var expectedBody =
'grant_type=refresh_token&refresh_token=' + refreshToken;

assert.calledWith(
fakeHttp.post,
'https://hypothes.is/api/token',
expectedBody,
{headers: {'Content-Type': 'application/x-www-form-urlencoded'},
});
};
}

function assertThatTokenGetterNowReturnsNewAccessToken () {
return auth.tokenGetter().then(function (token) {
Copy link
Member

Choose a reason for hiding this comment

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

auth.tokenGetter() will be invoked when the promise chain is constructed not when the previous promise in the chain is resolved, which is I think what you intended to do.

Unless you are going to re-use a then handler, I would suggest just inlining this as an anonymous function in the then block below, perhaps with a one line comment at the top.

assert.equal(token, 'secondAccessToken');
});
}

return callTokenGetter()
.then(resetHttpSpy)
.then(expireAccessToken)
.then(assertRefreshTokenWasUsed('firstRefreshToken'))
.then(resetHttpSpy)
.then(assertThatTokenGetterNowReturnsNewAccessToken)
.then(expireAccessToken)
.then(assertRefreshTokenWasUsed('secondRefreshToken'));
});

// While a refresh token HTTP request is in-flight, calls to tokenGetter()
// should just return the old access token immediately.
it('returns the access token while a refresh is in-flight', function() {
return auth.tokenGetter().then(function(firstAccessToken) {
makeServerUnresponsive();

expireAccessToken();

// The refresh token request will still be in-flight, but tokenGetter()
// should still return a Promise for the old access token.
return auth.tokenGetter().then(function(secondAccessToken) {
assert.equal(firstAccessToken, secondAccessToken);
});
});
});

// It only sends one refresh request, even if tokenGetter() is called
// multiple times and the refresh response hasn't come back yet.
it('does not send more than one refresh request', function () {
return auth.tokenGetter()
.then(resetHttpSpy) // Reset fakeHttp.post.callCount to 0 so that the
// initial access token request isn't counted.
.then(auth.tokenGetter)
.then(makeServerUnresponsive)
.then(auth.tokenGetter)
.then(expireAccessToken)
.then(function () {
assert.equal(fakeHttp.post.callCount, 1);
});
});
});

// Advance time forward so that any current access tokens will have expired.
function expireAccessToken () {
clock.tick(DEFAULT_TOKEN_EXPIRES_IN_SECS * 1000);
}

// Make $http.post() return a pending Promise (simulates a still in-flight
// HTTP request).
function makeServerUnresponsive () {
fakeHttp.post.returns(new Promise(function () {}));
}

// Reset fakeHttp.post.callCount and other fields.
function resetHttpSpy () {
fakeHttp.post.reset();
}
});