Revoke all authorized applications on password reset #21325
Conversation
app/models/user.rb
Outdated
| @@ -373,6 +373,15 @@ def reset_password(new_password, new_password_confirmation) | |||
| super | |||
| end | |||
|
|
|||
| def clear_sessions! | |||
There was a problem hiding this comment.
This name feels a little misleading, given that what it's really doing is removing/revoking authorised applications and push subscriptions.
There was a problem hiding this comment.
Any suggestions for a clearer name?
There was a problem hiding this comment.
revoke_tokens would be even more explicit, I think
There was a problem hiding this comment.
I considered that, but it's also revoking access grants and deleting push subscriptions.
Maybe that's implementation detail.
There was a problem hiding this comment.
If I refactor this method to also delete session_activations if it was used for change password, would the original name be more helpful?
|
Thank you for your contribution! I don't think it's addressing #20431 as this issue was explicitly about I am not completely sure we want that behavior, especially since current apps handle revoked tokens quite poorly (see #20431 (comment)) but I understand the reasoning, and I think revoking tokens in the password reset flow as addressed in this PR would cause less issue than doing it in the
The password reset is taken care of by
That would be great! This would be a new |
|
I started with the password reset to understand if this behavior is acceptable which I was hoping then to reuse for change password without removing the current session that is. If this behavior is currently not desired, I will remove marking the issue resolved. For now, will just add the controller test. |
| it 'returns http success' do | ||
| expect(response).to have_http_status(200) | ||
| end |
There was a problem hiding this comment.
Is that actually the expected behavior?
There was a problem hiding this comment.
I see your point. So I changed the test expectation to redirection on success and render a error template on failure which might be better.
FrancisMurillo
force-pushed
the
fix-clear-sessions-on-password-change
branch
from
2a7b4fa
to
ace0341
Compare
|
@FrancisMurillo your commits are showing up as Unverified, as you've requested GitHub to require GPG signing for your account. Can you look into this? |
FrancisMurillo
force-pushed
the
fix-clear-sessions-on-password-change
branch
from
ace0341
to
2a57e6e
Compare
|
@ineffyble Was able to finally recover my keys and gpg sign the commits so they are now marked as verified. |
Gargron
changed the title
* Clear sessions on password change * Rename User::clear_sessions to revoke_access for a clearer meaning * Add reset paassword controller test * Use User.find instead of User.find_for_authentication for reset password test * Use redirect and render for better test meaning in reset password Co-authored-by: Effy Elden <[email protected]>
* Clear sessions on password change * Rename User::clear_sessions to revoke_access for a clearer meaning * Add reset paassword controller test * Use User.find instead of User.find_for_authentication for reset password test * Use redirect and render for better test meaning in reset password Co-authored-by: Effy Elden <[email protected]>
Clear all user sessions after a successful password reset if this is the desired behavior. I noticed that
User::reset_password!is not really being invoked by the password reset flow so I am not sure if this is intentional as the tests are working as well. I would also like to add a controller test for this behavior but would need some guidance.For change password, the behavior is the same and may not clear the sessions except the current. So this might also be considered.
Steps I took when I tested with my instance: