Add oauth refresh support #214
Conversation
Don't reuse the OAuth grant token if the access token has expired. The grant token is only intended to be used once, and if the access token has expired then the grant token will likely have expired as well anyway. A later commit will add support for refreshing an expired access token using its refresh token, instead.
Simplify the oauth-auth service by not caching the grant token in memory - it is (now) only used once anyway.
Save the refresh token, as well as the access token, from the server in memory.
Codecov Report
@@ Coverage Diff @@
## master #214 +/- ##
=========================================
Coverage ? 75.18%
=========================================
Files ? 114
Lines ? 5702
Branches ? 929
=========================================
Hits ? 4287
Misses ? 1415
Partials ? 0
Continue to review full report at Codecov.
|
|
I still need to test that this actually works in a browser, but I think it can be reviewed already. @robertknight You might be a good person to review this? There's currently no error handling if the refresh token response fails. Since this response is done on a There needs to be better end-to-end error handling if a grant token or refresh token response fails and the client ends up with either no access token, or with an expired access token. I haven't tried to see what happens currently. Calling |
|
Given this a quick look over, I don't see anything that jumps out at me as a major issue. I'd need to do some testing on it as well. |
|
Testing this using Rob's example site and having changed the access token expiry time from one hour to ten seconds server-side, the client seems to be sending two refresh token requests every ten seconds, with two different refresh tokens. Weird. I'll try to figure out what's going on after lunch. |
|
Ok, I think see what the problem is. This is my theory after a little debugging. This problem exists on master as well:
On this branch the problem is compounded because:
Again, it doesn't really do any harm, it's just always sending two requests instead of one. If |
This commit fixes a problem that the client would send multiple concurrent access token and multiple concurrent refresh token requests each time: 1. tokenGetter() is called, sends an HTTP request for the access token and returns a Promise for the access token from the request's response 2. Before the HTTP response is received tokenGetter() is called again, sends another HTTP request, returns another Promise. As soon as one access token response is received then tokenGetter() caches the access token and just returns it, doesn't send any more HTTP requests, but each time tokenGetter() is called *before the first access token response has been received* it sends another, concurrent access token request. It does not "know" that it already has an access token request in-flight. 3. Each time it receives an access token response tokenGetter() also sets a timer to do a refresh token request for that access token. So if it sends multiple access token requests, then it receives multiple access token responses, sets multiple timers, and ends up sending multiple refresh token requests. 4. Each time it receives a refresh token response it sets a timer to do another refresh token request, so multiple refresh token requests will be sent next time round as well. The fix is to cache the first access token request Promise when the first access token request is sent and then just keep returning that Promise, instead of sending more requests: * Instead of caching the access token itself (`cachedToken`) we now cache a Promise for the access token (`accessTokenPromise`) the first time we send the access token request. Each further time that `tokenGetter()` is called, if it already has a `accessTokenPromise`, then it just returns the Promise and doesn't send another HTTP request. It now "knows" that it already has an access token request in-flight. * `refreshAccessToken()` replaces `accessTokenPromise` with a _resolved_ Promise for the new access token _when it receives the refresh token response_. `refreshAccessToken()` does not replace `accessTokenPromise` when it _sends_ the refresh token request, it replaces it when it receives the response. Refresh token requests are sent a few minutes _before_ the access token expires, so if `tokenGetter()` is called again while a refresh token request is in-flight we want to just return the access token immediately because it can still be used, we _don't_ want to return a pending Promise for the next access token that hasn't arrived yet.
Slightly refactor and improve the naming of the cachedTokenFromData() helper function.
5e1ef26 to
37ff84e
Compare
|
Closing this as it ended up getting replaced with a series of pull requests, the last of which was #221 |
No description provided.