diff --git a/CHANGELOG.rdoc b/CHANGELOG.rdoc index 02c8f2ed..75e2e1d8 100644 --- a/CHANGELOG.rdoc +++ b/CHANGELOG.rdoc @@ -18,6 +18,7 @@ * Manual sign_in now triggers remember token * Be sure to halt strategies on failures * Consider SCRIPT_NAME on Omniauth paths + * Reset failed attempts when lock is expired * deprecations * Deprecated anybody_signed_in? in favor of signed_in? (by github.com/gavinhughes) diff --git a/lib/devise/models/lockable.rb b/lib/devise/models/lockable.rb index 20a5ff5e..55aef2ef 100644 --- a/lib/devise/models/lockable.rb +++ b/lib/devise/models/lockable.rb @@ -36,12 +36,10 @@ module Devise # Unlock a user by cleaning locket_at and failed_attempts. def unlock_access! - if_access_locked do - self.locked_at = nil - self.failed_attempts = 0 if respond_to?(:failed_attempts=) - self.unlock_token = nil if respond_to?(:unlock_token=) - save(:validate => false) - end + self.locked_at = nil + self.failed_attempts = 0 if respond_to?(:failed_attempts=) + self.unlock_token = nil if respond_to?(:unlock_token=) + save(:validate => false) end # Verifies whether a user is locked or not. @@ -77,6 +75,10 @@ module Devise def valid_for_authentication? return super unless persisted? && lock_strategy_enabled?(:failed_attempts) + # Unlock the user if the lock is expired, no matter + # if the user can login or not (wrong password, etc) + unlock_access! if lock_expired? + case (result = super) when Symbol return result diff --git a/test/models/lockable_test.rb b/test/models/lockable_test.rb index 81d64c31..ff41b101 100644 --- a/test/models/lockable_test.rb +++ b/test/models/lockable_test.rb @@ -67,12 +67,6 @@ class LockableTest < ActiveSupport::TestCase assert_equal 0, user.reload.failed_attempts end - test 'should not unlock an unlocked user' do - user = create_user - assert_not user.unlock_access! - assert_match "was not locked", user.errors[:email].join - end - test "new user should not be locked and should have zero failed_attempts" do assert_not new_user.access_locked? assert_equal 0, create_user.failed_attempts @@ -201,4 +195,31 @@ class LockableTest < ActiveSupport::TestCase assert_not user.access_locked? assert_equal 'was not locked', user.errors[:email].join end + + test 'should unlock account if lock has expired and increase attempts on failure' do + swap Devise, :unlock_in => 1.minute do + user = create_user + user.confirm! + + user.failed_attempts = 2 + user.locked_at = 2.minutes.ago + + user.valid_for_authentication? { false } + assert_equal 1, user.failed_attempts + end + end + + test 'should unlock account if lock has expired on success' do + swap Devise, :unlock_in => 1.minute do + user = create_user + user.confirm! + + user.failed_attempts = 2 + user.locked_at = 2.minutes.ago + + user.valid_for_authentication? { true } + assert_equal 0, user.failed_attempts + assert_nil user.locked_at + end + end end