Add an option to not automatically sign in a user after changing a password

[knjko]

This is useful for cases where additional strategies might be needed, or generally if it is considered a security risk to automatically log in a user after changing a password.

[tegon]

Some considerations here:

• WDYT about updated_but_not_signed_in as the key name?
• Change the default message to Your account has been updated successfully, but since your password was changed, you need to sign in again

[tegon]

Would resource.password_changed? work here? I ask because I'm worried we might return true here when the password parameter comes as an empty string, for example. The #include clause here will return true, but in fact, no password was provided. Makes sense?

[knjko]

sign_in_after_change_password? is called only when resource_updated, so we do not need to check whether blank or not?

[tegon]

But then how would we know whether the password changed or not? I mean, if it's blank we assume that the user didn't want to change it, so in that case there would be no validation errors.

[tegon]

@knjko Thanks for this! I'm sorry it took so long for us to review it 😞

I made a couple of considerations and this PR also needs a rebase with the master branch. Can you do that, please?

[knjko]

@tegon Thanks for reviewing! I rebased this branch and fix the message 🔧

[tegon]

@knjko I've added a new test to cover the scenario I mentioned before: Devise.sign_in_after_change_password is false but the user doesn't change its password, only other account information - like email, for example. In that case the user was being logged out. I've worked a little bit on this and came out with the patch below. Please let me know your thoughts on this.

Thanks again!

diff --git a/app/controllers/devise/registrations_controller.rb b/app/controllers/devise/registrations_controller.rb
index 79a2bc76..394c921a 100644
--- a/app/controllers/devise/registrations_controller.rb
+++ b/app/controllers/devise/registrations_controller.rb
@@ -51,7 +51,8 @@ class Devise::RegistrationsController < DeviseController
     yield resource if block_given?
     if resource_updated
       set_flash_message_for_update(resource, prev_unconfirmed_email)
-      sign_in_after_change_password
+      bypass_sign_in resource, scope: resource_name if sign_in_after_change_password?
+
       respond_with resource, location: after_update_path_for(resource)
     else
       clean_up_passwords resource
@@ -148,6 +149,7 @@ class Devise::RegistrationsController < DeviseController
 
   def set_flash_message_for_update(resource, prev_unconfirmed_email)
     return unless is_flashing_format?
+
     flash_key = if update_needs_confirmation?(resource, prev_unconfirmed_email)
                   :update_needs_confirmation
                 elsif sign_in_after_change_password?
@@ -158,15 +160,9 @@ class Devise::RegistrationsController < DeviseController
     set_flash_message :notice, flash_key
   end
 
-  def sign_in_after_change_password
-    if sign_in_after_change_password?
-      bypass_sign_in resource, scope: resource_name
-    else
-      sign_out(resource)
-    end
-  end
-
   def sign_in_after_change_password?
-    Devise.sign_in_after_change_password && account_update_params.include?(:password)
+    return true unless resource.saved_change_to_encrypted_password?
+
+    Devise.sign_in_after_change_password
   end
 end
diff --git a/test/integration/registerable_test.rb b/test/integration/registerable_test.rb
index 3070e53b..46f09f37 100644
--- a/test/integration/registerable_test.rb
+++ b/test/integration/registerable_test.rb
@@ -179,7 +179,7 @@ class RegistrationTest < Devise::IntegrationTest
     assert warden.authenticated?(:user)
   end
 
-  test 'a signed in user should not still be able to use the website after changing their password if config.sign_in_after_change_password is false' do
+  test 'a signed in user should not be able to use the website after changing their password if config.sign_in_after_change_password is false' do
     swap Devise, sign_in_after_change_password: false do
       sign_in_as_user
       get edit_user_registration_path
@@ -191,7 +191,24 @@ class RegistrationTest < Devise::IntegrationTest
 
       assert_contain 'Your account has been updated successfully, but since your password was changed, you need to sign in again'
       assert_equal new_user_session_path, @request.path
-      assert !warden.authenticated?(:user)
+      refute warden.authenticated?(:user)
+    end
+  end
+
+  test 'a signed in user should be able to use the website after changing its email with config.sign_in_after_change_password is false' do
+    swap Devise, sign_in_after_change_password: false do
+      sign_in_as_user
+      get edit_user_registration_path
+
+      fill_in 'email', with: '[email protected]'
+      fill_in 'current password', with: '12345678'
+      click_button 'Update'
+
+      assert_current_url '/'
+      assert_contain 'Your account has been updated successfully.'
+
+      assert warden.authenticated?(:user)
+      assert_equal "[email protected]", User.to_adapter.find_first.email
     end
   end

[tegon]

@knjko are you still going to work on this?

I'm asking to know if I should assign it to someone else.

[knjko]

Hi @tegon Sorry for the late reply 💦

Thanks for your patch! 👍 I think you are right, but fixed the following code since saved_change_to_* are not supported in older versions. Can you review it again?

def sign_in_after_change_password?
  return true if account_update_params[:password].blank?
  ...

https://github.com/plataformatec/devise/pull/4569/files#diff-f82eaf0d25964c41f76c557b038284e5R164

Thank you!

[tegon]

@knjko You're right, thanks for noticing this!

Seems good now, I'm going to merge it. Thanks again for your contribution! 😄

[knjko]

@tegon Thanks! Enjoy the holidays! 🎍

[sergey-alekseev]

Is this required to be private? Other methods above are protected. I'm subclassing and overriding some methods of Devise::RegistrationsController and need to copy these new private methods during the gem version upgrade.