From 5fb1c3e934a1a782972ac2732ce7f0208c341ac2 Mon Sep 17 00:00:00 2001
From: Francis Murillo <evacuee.overlap.vs3op@aleeas.com>
Date: Thu, 15 Dec 2022 14:47:06 +0000
Subject: [PATCH] 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>
---
 app/controllers/auth/passwords_controller.rb  |  2 +
 app/models/user.rb                            | 16 +++--
 .../auth/passwords_controller_spec.rb         | 61 +++++++++++++++++++
 3 files changed, 73 insertions(+), 6 deletions(-)

diff --git a/app/controllers/auth/passwords_controller.rb b/app/controllers/auth/passwords_controller.rb
index 2996c0431..a8ad66929 100644
--- a/app/controllers/auth/passwords_controller.rb
+++ b/app/controllers/auth/passwords_controller.rb
@@ -10,6 +10,8 @@ class Auth::PasswordsController < Devise::PasswordsController
     super do |resource|
       if resource.errors.empty?
         resource.session_activations.destroy_all
+
+        resource.revoke_access!
       end
     end
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index 5530a9070..ca98a0afa 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -377,6 +377,15 @@ class User < ApplicationRecord
     super
   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!
     # First, change password to something random and deactivate all sessions
     transaction do
@@ -385,12 +394,7 @@ class User < ApplicationRecord
     end
 
     # Then, remove all authorized applications and connected push subscriptions
-    Doorkeeper::AccessGrant.by_resource_owner(self).in_batches.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
+    revoke_access!
 
     # Finally, send a reset password prompt to the user
     send_reset_password_instructions
diff --git a/spec/controllers/auth/passwords_controller_spec.rb b/spec/controllers/auth/passwords_controller_spec.rb
index dcfdebb17..1c6874f08 100644
--- a/spec/controllers/auth/passwords_controller_spec.rb
+++ b/spec/controllers/auth/passwords_controller_spec.rb
@@ -35,4 +35,65 @@ describe Auth::PasswordsController, type: :controller do
       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