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.

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

WIP Add token refresh #624

Merged
merged 17 commits into from
Aug 30, 2018

Conversation

aoberoi
Copy link
Contributor

@aoberoi aoberoi commented Aug 26, 2018

Summary

This PR implements the changes described in #617.

This is still a work in progress with at least the following TODO items:

  • try running with some actual short lived tokens
  • lots of opportunities for refactors and cleanups
  • get linter to pass
  • get test coverage up to acceptable levels

I wanted to share this even at this slightly less mature stage for visibility into the progress.

Requirements (place an x in each [ ])

@codecov
Copy link

codecov bot commented Aug 26, 2018

Codecov Report

Merging #624 into feat-short-lived-tokens will decrease coverage by 0.54%.
The diff coverage is 92.22%.

Impacted file tree graph

@@                     Coverage Diff                     @@
##           feat-short-lived-tokens     #624      +/-   ##
===========================================================
- Coverage                    92.01%   91.47%   -0.55%     
===========================================================
  Files                            7        7              
  Lines                          426      481      +55     
  Branches                        78       89      +11     
===========================================================
+ Hits                           392      440      +48     
- Misses                          19       22       +3     
- Partials                        15       19       +4
Impacted Files Coverage Δ
src/methods.ts 100% <ø> (ø) ⬆️
src/errors.ts 100% <100%> (ø) ⬆️
src/WebClient.ts 93.42% <92.13%> (-0.98%) ⬇️
src/util.ts 71.11% <0%> (-2.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed5ac1f...f5109e1. Read the comment docs.

@aoberoi
Copy link
Contributor Author

aoberoi commented Aug 27, 2018

Some notes from this implementation:

Idea: isTokenRefreshing as a Promise.

What if the token refresh is in progress when the optimistic token refresh conditions are checked? With this implementation when the API call fails, the code will check the request time and determine that the token should be refreshed (again) and the API call will be tried again.

But another option would be to prevent the first attempt to the API call in the first place by waiting on the token refresh in progress to finish (if we represent isTokenRefreshing not as a boolean, but as a Promise that this API call can await (a mutex of sorts, or maybe a barrier, i can't remember the right terminology). The failure logic doesn't change, but it would be an optimization.

Idea: Only use optimistic checks to perform token refresh

What's the point in doing a token refresh right before a retry if the retry will do an optimistic check anyway? The refresh could just occur in one place.

/**
* The time (in milliseconds) when the current access token will expire
*/
private accessTokenExpiresAt?: number;
Copy link
Contributor

@Roach Roach Aug 27, 2018

Choose a reason for hiding this comment

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

Should be accessTokenExpiresIn, it's a countdown TTL, not an absolute timestamp of the time of expiration.

https://tools.ietf.org/html/rfc6749#section-4.2.2

expires_in
RECOMMENDED. The lifetime in seconds of the access token. For
example, the value "3600" denotes that the access token will
expire in one hour from the time the response was generated.
If omitted, the authorization server SHOULD provide the
expiration time via other means or document the default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the response contains expires_in, you're right. but the client only stores that value after it adds it to the current time, so this is still correct. it wouldn't be useful to store just the expires_in because we would have to compute the future time that the expiration will occur on every request. this way, we only do that computation once and store it. my biggest concern was also clearing this value at the appropriate time, which is handled by the setter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. I missed that. Carry on!

@aoberoi aoberoi merged commit d90a568 into slackapi:feat-short-lived-tokens Aug 30, 2018
@aoberoi aoberoi deleted the add-token-refresh branch August 30, 2018 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants