Pace token refresh requests to avoid rate-limiting #79
Conversation
There was a problem hiding this comment.
The code looks good but I want to make sure I understand the implementation and I have one question.
This is how I understand it: If the code determines that it's time for a refresh, it kicks off a token request but in the meantime it just returns the token that's already been stored. If any more requests come in during the refresh, it still uses the stored token thanks to the 60 second addition to the time.
If that's correct, I have one question: what happens in the case that an app makes a request for which a token is retrieved, idles for long enough that the token expires completely, and then tries to make another request? It seems like the refresh would get kicked off but the stored token would still be used initially, resulting in a failed request. I may be misunderstanding but if that's the case, it might be a good idea to add a check that says "if the token is completely expired, request a new one in a blocking manner" like we do for the initial token grab. What are your thoughts?
| @@ -90,13 +90,19 @@ export class JwtTokenManager { | |||
| * has expired. | |||
| */ | |||
| public getToken(): Promise<any> { | |||
| if (!this.tokenInfo[this.tokenName] || this.isTokenExpired()) { | |||
| if (!this.tokenInfo[this.tokenName]) { | |||
There was a problem hiding this comment.
I agree with @dpopp07 - I think the expiry check needs to remain here so that a synchronous request for a new token can be made in the case that a token's validity has lapsed after a period with no requests having been made to trigger a preemptive refresh.
I guess you could add a test for this with a very short expiry time and a short block in a test case.
| * @private | ||
| * @returns {void} | ||
| */ | ||
| private calculateTimeForNewToken(accessToken): number { | ||
| // the time of expiration is found by decoding the JWT access token |
There was a problem hiding this comment.
I had to integrate calculateTimeForNewToken into saveTokenInfo because we now need to set both the expireTime and refreshTime from the values in the decoded token.
| return timeForNewToken; | ||
| const { exp, iat } = decodedResponse; | ||
| // There are no required claims in JWT | ||
| if (!exp || !iat) { |
There was a problem hiding this comment.
This is just extra bullet-proofing for the case where the JWT does not have an exp or iat claim.
| token_type: 'Bearer', | ||
| expires_in: 3600, | ||
| expiration: Math.floor(Date.now() / 1000), | ||
| access_token: CURRENT_ACCESS_TOKEN, |
There was a problem hiding this comment.
Since the iam-token-manager was changed to simply treat the access token as a JWT, none of the other fields in the IAM response are used, so we can eliminate them.
|
|
||
| it('should refresh an access token without expires_in field', async done => { |
There was a problem hiding this comment.
This test was actually no longer valid, since the expires_in field is not used. It only happened to pass because the test did not set the expireTime in the instance.
| @@ -33,7 +34,7 @@ describe('JWT Token Manager', () => { | |||
|
|
|||
| const decodeSpy = jest | |||
| .spyOn(jwt, 'decode') | |||
| .mockImplementation(token => ({ iat: 0, exp: 100 })); | |||
| .mockImplementation(token => ({ iat: 10, exp: 100 })); | |||
There was a problem hiding this comment.
Can't use iat = 0 because that triggers the new check in the jwt-token-manager.
|
I've updated the code to handle an expired token the same a missing token -- with a synchronous request for a new token. This turned out to be more invasive that I expected, but I think the new code now works as intended. |
mkistler
force-pushed
the
mdk/serialize-token-refresh
branch
from
cc10ba6 to
487a864
Compare
# [2.1.0](v2.0.4...v2.1.0) (2020-02-14) ### Features * Pace token refresh requests to avoid rate-limiting ([#79](#79)) ([d908c0d](d908c0d))
|
🎉 This PR is included in version 2.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
) Bumps [eslint-plugin-prettier](https://github.com/prettier/eslint-plugin-prettier) from 3.2.0 to 3.3.0. - [Release notes](https://github.com/prettier/eslint-plugin-prettier/releases) - [Changelog](https://github.com/prettier/eslint-plugin-prettier/blob/master/CHANGELOG.md) - [Commits](prettier/eslint-plugin-prettier@v3.2.0...v3.3.0) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This PR modifies the token refresh logic to pace refresh requests to avoid exceeding rate-limiting by the token server. This design also improves performance by continuing to use an existing token while the refresh is performed concurrently.
I also cleaned up some misinformation in the
CONTRIBUTING.mdthat I uncovered in the process of developing this change.