Revoke all authorized applications on password reset (#21325)

* 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 <effy@effy.space>
This commit is contained in:
Francis Murillo 2022-12-15 14:47:06 +00:00 committed by GitHub
parent fe9eab51d1
commit 5fb1c3e934
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 73 additions and 6 deletions

View file

@ -10,6 +10,8 @@ class Auth::PasswordsController < Devise::PasswordsController
super do |resource| super do |resource|
if resource.errors.empty? if resource.errors.empty?
resource.session_activations.destroy_all resource.session_activations.destroy_all
resource.revoke_access!
end end
end end
end end

View file

@ -377,6 +377,15 @@ class User < ApplicationRecord
super super
end end
def revoke_access!
Doorkeeper::AccessGrant.by_resource_owner(self).update_all(revoked_at: Time.now.utc)
Doorkeeper::AccessToken.by_resource_owner(self).in_batches do |batch|
batch.update_all(revoked_at: Time.now.utc)
Web::PushSubscription.where(access_token_id: batch).delete_all
end
end
def reset_password! def reset_password!
# First, change password to something random and deactivate all sessions # First, change password to something random and deactivate all sessions
transaction do transaction do
@ -385,12 +394,7 @@ class User < ApplicationRecord
end end
# Then, remove all authorized applications and connected push subscriptions # Then, remove all authorized applications and connected push subscriptions
Doorkeeper::AccessGrant.by_resource_owner(self).in_batches.update_all(revoked_at: Time.now.utc) revoke_access!
Doorkeeper::AccessToken.by_resource_owner(self).in_batches do |batch|
batch.update_all(revoked_at: Time.now.utc)
Web::PushSubscription.where(access_token_id: batch).delete_all
end
# Finally, send a reset password prompt to the user # Finally, send a reset password prompt to the user
send_reset_password_instructions send_reset_password_instructions

View file

@ -35,4 +35,65 @@ describe Auth::PasswordsController, type: :controller do
end end
end end
end end
describe 'POST #update' do
let(:user) { Fabricate(:user) }
before do
@password = 'reset0password'
request.env['devise.mapping'] = Devise.mappings[:user]
end
context 'with valid reset_password_token' do
let!(:session_activation) { Fabricate(:session_activation, user: user) }
let!(:access_token) { Fabricate(:access_token, resource_owner_id: user.id) }
let!(:web_push_subscription) { Fabricate(:web_push_subscription, access_token: access_token) }
before do
@token = user.send_reset_password_instructions
post :update, params: { user: { password: @password, password_confirmation: @password, reset_password_token: @token } }
end
it 'redirect to sign in' do
expect(response).to redirect_to '/auth/sign_in'
end
it 'changes password' do
this_user = User.find(user.id)
expect(this_user).to_not be_nil
expect(this_user.valid_password?(@password)).to be true
end
it 'deactivates all sessions' do
expect(user.session_activations.count).to eq 0
end
it 'revokes all access tokens' do
expect(Doorkeeper::AccessToken.active_for(user).count).to eq 0
end
it 'removes push subscriptions' do
expect(Web::PushSubscription.where(user: user).or(Web::PushSubscription.where(access_token: access_token)).count).to eq 0
end
end
context 'with invalid reset_password_token' do
before do
post :update, params: { user: { password: @password, password_confirmation: @password, reset_password_token: 'some_invalid_value' } }
end
it 'renders reset password' do
expect(response).to render_template(:new)
end
it 'retains password' do
this_user = User.find(user.id)
expect(this_user).to_not be_nil
expect(this_user.external_or_valid_password?(user.password)).to be true
end
end
end
end end