From a3ae35e9c951d1722af9a76fba7c1fa62c643019 Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Mon, 22 Mar 2021 18:26:17 -0300 Subject: [PATCH] Create a model hook around the lockable warden hook to reset attempts Resetting failed attempts after sign in happened inside a warden hook specific for the lockable module, but that was hidden inside the hook implementation and didn't allow any user customization. One such customization needed for example is to direct these updates to a write DB when using a multi-DB setup. With the logic hidden in the warden hook this wasn't possible, now that it's exposed in a model method much like trackable, we can override the model method to wrap it in a connection switch block for example, point to a write DB, and simply call `super`. Closes #5310 Related to #5264 and #5133 --- CHANGELOG.md | 2 ++ lib/devise/hooks/lockable.rb | 7 ++----- lib/devise/models/lockable.rb | 10 +++++++++- test/models/lockable_test.rb | 26 ++++++++++++++++++++++++++ 4 files changed, 39 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cdbdd6c..5370462f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ * Devise now enables the upgrade of OmniAuth 2+. Previously Devise would raise an error if you'd try to upgrade. Please note that OmniAuth 2 is considered a security upgrade and recommended to everyone. You can read more about the details (and possible necessary changes to your app as part of the upgrade) in [their release notes](https://github.com/omniauth/omniauth/releases/tag/v2.0.0). [Devise's OmniAuth Overview wiki](https://github.com/heartcombo/devise/wiki/OmniAuth:-Overview) was also updated to cover OmniAuth 2.0 requirements. - Note that the upgrade required Devise shared links that initiate the OmniAuth flow to be changed to `method: :post`, which is now a requirement for OmniAuth, part of the security improvement. If you have copied and customized the Devise shared links partial to your app, or if you have other links in your app that initiate the OmniAuth flow, they will have to be updated to use `method: :post`, or changed to use buttons (e.g. `button_to`) to work with OmniAuth 2. (if you're using links with `method: :post`, make sure your app has `rails-ujs` or `jquery-ujs` included in order for these links to work properly.) - As part of the OmniAuth 2.0 upgrade you might also need to add the [`omniauth-rails_csrf_protection`](https://github.com/cookpad/omniauth-rails_csrf_protection) gem to your app if you don't have it already. (and you don't want to roll your own code to verify requests.) Check the OmniAuth v2 release notes for more info. + * Introduce `Lockable#reset_failed_attempts!` model method to reset failed attempts counter to 0 after the user signs in. + - This logic existed inside the lockable warden hook and is triggered automatically after the user signs in. The new model method is an extraction to allow you to override it in the application to implement things like switching to a write database if you're using the new multi-DB infrastructure from Rails for example, similar to how it's already possible with `Trackable#update_tracked_fields!`. * Add support for Ruby 3. * Add support for Rails 6.1. * Move CI to GitHub Actions. diff --git a/lib/devise/hooks/lockable.rb b/lib/devise/hooks/lockable.rb index a73a1752..b11db1e8 100644 --- a/lib/devise/hooks/lockable.rb +++ b/lib/devise/hooks/lockable.rb @@ -3,10 +3,7 @@ # After each sign in, if resource responds to failed_attempts, sets it to 0 # This is only triggered when the user is explicitly set (with set_user) Warden::Manager.after_set_user except: :fetch do |record, warden, options| - if record.respond_to?(:failed_attempts) && warden.authenticated?(options[:scope]) - unless record.failed_attempts.to_i.zero? - record.failed_attempts = 0 - record.save(validate: false) - end + if record.respond_to?(:reset_failed_attempts!) && warden.authenticated?(options[:scope]) + record.reset_failed_attempts! end end diff --git a/lib/devise/models/lockable.rb b/lib/devise/models/lockable.rb index 578f5294..ce9e3e57 100644 --- a/lib/devise/models/lockable.rb +++ b/lib/devise/models/lockable.rb @@ -57,6 +57,14 @@ module Devise save(validate: false) end + # Resets failed attempts counter to 0. + def reset_failed_attempts! + if respond_to?(:failed_attempts) && !failed_attempts.to_i.zero? + self.failed_attempts = 0 + save(validate: false) + end + end + # Verifies whether a user is locked or not. def access_locked? !!locked_at && !lock_expired? @@ -110,7 +118,7 @@ module Devise false end end - + def increment_failed_attempts self.class.increment_counter(:failed_attempts, id) reload diff --git a/test/models/lockable_test.rb b/test/models/lockable_test.rb index 8b12d550..4190de92 100644 --- a/test/models/lockable_test.rb +++ b/test/models/lockable_test.rb @@ -50,6 +50,32 @@ class LockableTest < ActiveSupport::TestCase assert_equal initial_failed_attempts + 2, user.reload.failed_attempts end + test "reset_failed_attempts! updates the failed attempts counter back to 0" do + user = create_user(failed_attempts: 3) + assert_equal 3, user.failed_attempts + + user.reset_failed_attempts! + assert_equal 0, user.failed_attempts + + user.reset_failed_attempts! + assert_equal 0, user.failed_attempts + end + + test "reset_failed_attempts! does not run model validations" do + user = create_user(failed_attempts: 1) + user.expects(:after_validation_callback).never + + assert user.reset_failed_attempts! + assert_equal 0, user.failed_attempts + end + + test "reset_failed_attempts! does not try to reset if not using failed attempts strategy" do + admin = create_admin + + refute_respond_to admin, :failed_attempts + refute admin.reset_failed_attempts! + end + test 'should be valid for authentication with a unlocked user' do user = create_user user.lock_access!