Reset lock attempts count when lock is expired. Closes #825

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)

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

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