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

Add warning when refreshLeeway is set to high #122

Closed
wants to merge 2 commits into from
Closed

Add warning when refreshLeeway is set to high #122

wants to merge 2 commits into from

Conversation

albertovasquez
Copy link

When the expiresAt is in the future we still might skip it if the refreshLeeway is set to high.
In lieu of running the refreshAccessToken method and possibly cause non stop calls to refresh endpoint
lets add a warning to let admin know why their tokens are not refreshing.

When the expiresAt is in the future we still might skip it if the refreshLeeway is set to high.
In lieu of running the refreshAccessToken method and possibly cause non stop calls to refresh endpoint
lets add a warning to let admin know why their tokens are not refreshing.
…to-high

Add warning when refreshLeeway is set to high
@eluciano11
Copy link
Contributor

Hi @albertovasquez , thank you for your PR. I got around to looking at this today. I don't really understand how (expiresAt - now > 0) indicates that the refreshLeeway is too large. I was thinking about this problem and shouldn't it be ((this.refreshLeeway * 1000) > (expiresAt - now)) ? The default refreshLeeway is 0 but it can be configured by the user. So if the refreshLeeway is greater than the time we have can subtract from the token expiration time than it's too big.

Please let me know what you think and the thought process behind (expiresAt - now > 0) so we can add this warning as soon as posible.

Thank you for your work.

@homu
Copy link
Contributor

homu commented Mar 22, 2017

☔ The latest upstream changes (presumably #190) made this pull request unmergeable. Please resolve the merge conflicts.

@fenichelar
Copy link
Owner

Great idea! Due to the depreciation of Ember.Logger, now if refreshLeeway is too large, an error is thrown: https://github.com/jpadilla/ember-simple-auth-token/blob/master/addon/authenticators/jwt.js#L171-L172

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants