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

Auto refresh bearer token #45

Merged
merged 3 commits into from
May 9, 2016
Merged

Auto refresh bearer token #45

merged 3 commits into from
May 9, 2016

Conversation

brian-c
Copy link
Contributor

@brian-c brian-c commented May 9, 2016

Depends on zooniverse/json-api-client#30.

Instead of using setTimeout to fetch a new bearer token before the current one expires, this gives the auth module a checkBearerToken method (which will refresh if needed) and calls if beforeEveryRequest on the API client.

We also talked about refreshing the token and remaking requests when we get a 401, which I thought would require a major rewrite, but now that I think about it again it might not be a big deal. Any strong preference for me to look into it more?

(The handleError method is just moved, add &w=1 to the PR URL to ignore it.)

var refresh = this._refreshBearerToken.bind(this, response.refresh_token);
var timeToRefresh = (response.expires_in * 1000) - TOKEN_EXPIRATION_ALLOWANCE;
this._bearerRefreshTimeout = setTimeout(refresh, timeToRefresh);
this._bearerTokenExpiration = Date.now() + (response.expires_in * 1000);
Copy link
Contributor

@parrish parrish May 9, 2016

Choose a reason for hiding this comment

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

It's pretty minor, but it might be safer to use

this._bearerTokenExpiration = new Date(response.created_at * 1000) + (response.expires_in * 1000);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the way I've got it means the expiration time is relative to the user's clock, and this would make it relative to the server's clock. Isn't that a little safer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you're saying. Using Date.now() should be within the allowance anyway.

@parrish
Copy link
Contributor

parrish commented May 9, 2016

Any strong preference for me to look into it more?

Nope, this is working for me with some simulated expirations. Happy to go this route.

@parrish parrish merged commit 5980856 into master May 9, 2016
@parrish
Copy link
Contributor

parrish commented May 9, 2016

👍 :shipit:

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.

2 participants