Add OAuth refresh support #221
Conversation
Codecov Report@@ Coverage Diff @@
## master #221 +/- ##
==========================================
+ Coverage 75.12% 75.19% +0.06%
==========================================
Files 114 114
Lines 5689 5705 +16
Branches 930 930
==========================================
+ Hits 4274 4290 +16
Misses 1415 1415
Continue to review full report at Codecov.
|
| var data = response.data; | ||
| return { | ||
| accessToken: data.access_token, | ||
| expiresIn: data.expires_in, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
| // 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| function assertThatTokenGetterNowReturnsNewAccessToken () { | ||
| return auth.tokenGetter().then(function (token) { |
There was a problem hiding this comment.
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.
src/sidebar/oauth-auth.js
Outdated
| } | ||
|
|
||
| // Post the given data to the tokenUrl endpoint as a form submission. | ||
| // Return a Promise for the POST request. |
There was a problem hiding this comment.
This returns a Promise for the response, not the request ;) In other words, the convention is to say that something is or returns a Promise for the thing it will resolve to.
| // about 55 mins. | ||
| delay = Math.floor(delay * 0.91); | ||
|
|
||
| window.setTimeout(refreshAccessToken, delay, tokenInfo.refreshToken); |
There was a problem hiding this comment.
Ah. I did not know that setTimeout accepted function params for the callback as argument.
src/sidebar/test/oauth-auth-test.js
Outdated
| // 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(first_access_token) { |
There was a problem hiding this comment.
Var names should be camelCased (first_access_token)
src/sidebar/test/oauth-auth-test.js
Outdated
| .then(makeServerUnresponsive) | ||
| .then(auth.tokenGetter) | ||
| .then(expireAccessToken) | ||
| .then(assertThatOnlyOneRefreshRequestWasSent); |
There was a problem hiding this comment.
If you are not re-using the logic for a then callback or it is very simple, I'm not convinced that it is clearer to put it in a separate function - as I have to scroll up to find out what the actual assertion is.
Just separating concerns, and also future code is going to reuse this function as well, reducing duplication.
Just separating concerns, and also future code is going to reuse this function as well, reducing duplication.
bc3958d to
37eacfc
Compare
No description provided.