-
Notifications
You must be signed in to change notification settings - Fork 32
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
Refresh tokens 60 seconds before expire #165
Conversation
Codecov ReportBase: 73.58% // Head: 73.55% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #165 +/- ##
==========================================
- Coverage 73.58% 73.55% -0.04%
==========================================
Files 411 411
Lines 25526 25535 +9
Branches 561 561
==========================================
- Hits 18783 18781 -2
- Misses 6657 6668 +11
Partials 86 86
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
status = 401 | ||
content_type = "application/json-rpc" | ||
body = response.to_json(:allow_nan => true) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you're rescuing AuthErrors and explicitly setting the response. This will aid debugging but doesn't fix any issues with authentication.
@expires_at = current_time + oath["expires_in"] | ||
@refresh_expires_at = current_time + oath["refresh_expires_in"] | ||
@expires_at = current_time + oath["expires_in"] - REFRESH_OFFSET_SECONDS | ||
@refresh_expires_at = current_time + oath["refresh_expires_in"] - REFRESH_OFFSET_SECONDS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh we were previously letting the token go right down to the wire and then recreating? Do you think this resulted in a single bad un-authenticated request before the token was regenerated? Does this still have an issue where by if you don't make a request within 60s of the token expiring it will result in a single failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would have lead to edge case requests, that failed, and the next request should have refreshed the token. If you completely miss the token expiring then it will refresh (potentially a complete reissue).
closes #142