-
Notifications
You must be signed in to change notification settings - Fork 19
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
Remove LRU cashe for invalid tokens #235
Conversation
…U cache for tokens and always verify them using jwt library
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.
LGTM!
d8e46b9
to
51f77d7
Compare
bool access_token_expired() const { | ||
return (std::chrono::system_clock::now() > | ||
m_expiry + std::chrono::seconds(SECURITY_DYNAMIC_CONFIG(auth_manager->leeway))); | ||
m_expiry - std::chrono::seconds(SECURITY_DYNAMIC_CONFIG(trf_client->trf_expiry_leeway_secs))); |
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.
If the library verify()
already covers the timestamp check, do we need to check it one more time here? I mean, should we still need this "access_token_expired()" method?
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.
LGTM
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.
LG!
Fix token expired issue due to time drift in issued_at claim,, more info https://jirap.corp.ebay.com/browse/SDSTOR-13615?filter=-1
Do not cache invalid tokens. Add leeways for issued_at and not_before claims as an optimization.
Also, add leeway to trf client to download token ahead of its expiry.