-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Revoke Refresh Token on access token use #578
Conversation
time in case of network or other failures
def revoke_previous_refresh_token! | ||
return if previous_refresh_token.nil? | ||
old_refresh_token = AccessToken.by_refresh_token(previous_refresh_token) | ||
old_refresh_token = nil if old_refresh_token.nil? or old_refresh_token.revoked? |
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.
Line is too long. [87/80]
Use ||
instead of or
.
Thanks @rsharrott! We'll need some massaging of code and git history. Regarding the commit message (and history rewriting, in general), you can always force push the branch after doing this. This will include the discussion from the PR in sync with what you are updating, which is nice, no need to open new PRs. Your PR title and description are good, the commit message should be similar! We could amend the commit to something along the following lines:
Will do a code review later. In general, taking into account @houndci comments is a 👍. Thanks for your good work! |
end | ||
end | ||
update_attribute :previous_refresh_token, nil | ||
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.
Suggested rewrite of this method:
def revoke_previous_refresh_token!
old_refresh_token = AccessToken.by_refresh_token(previous_refresh_token)
if old_refresh_token && !old_refresh_token.revoked?
if configuration.refresh_token_revoked_in
old_refresh_token.revoke
else
old_refresh_token.revoke_in(configuration.refresh_token_revoked_in)
end
end
update_attributes(previous_refresh_token: nil)
end
def configuration
Doorkeeper.configuration
end
ping @rsharrott |
ping @rsharrott? |
Thanks for your early work @rsharrott, let's not let it get stale! I'll close it for now for lack of activity, but I'll be happy to reopen for working on it again. |
This PR allows refresh tokens to not either revoke at some point in the future (if you want to allow some wiggle room for network failures, etc) set refresh_token_revoked_in to some number of seconds in the future.
Also allows for refresh tokens to not be revoked until an access token created with that refresh token is successfully used once. Set refresh_token_revoked_on_use to true.
Both are off by default.
I made this work with ActiveRecord and it is passing test but no work was done for Mongoid.