From e2030a740d87facc5a29b76145c260a8bcf21116 Mon Sep 17 00:00:00 2001
From: Jigyasa Makkar <jigyasam@thoughtworks.com>
Date: Thu, 29 Dec 2011 23:55:16 +0530
Subject: [PATCH] Fixed a bug in lockable wherein when a user tries to login
 with correct password after being locked, failed attempts count gets reset.
 When the user tries to login with an incorrect password next, the message
 shown is for invalid password instead of locked account since this check
 depended mainly on failed attempts count.

---
 lib/devise/models/lockable.rb     |  2 +-
 test/integration/lockable_test.rb | 34 +++++++++++++++++++++----------
 test/models/lockable_test.rb      | 13 ++++++++++++
 3 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/lib/devise/models/lockable.rb b/lib/devise/models/lockable.rb
index 2fd729a6..b63f05b5 100644
--- a/lib/devise/models/lockable.rb
+++ b/lib/devise/models/lockable.rb
@@ -79,7 +79,7 @@ module Devise
         # if the user can login or not (wrong password, etc)
         unlock_access! if lock_expired?
 
-        if super
+        if super && !access_locked?
           self.failed_attempts = 0
           save(:validate => false)
           true
diff --git a/test/integration/lockable_test.rb b/test/integration/lockable_test.rb
index ed077156..d6a3606a 100644
--- a/test/integration/lockable_test.rb
+++ b/test/integration/lockable_test.rb
@@ -92,13 +92,6 @@ class LockTest < ActionController::IntegrationTest
     assert_not warden.authenticated?(:user)
   end
 
-  test "user should not be able to sign in when locked" do
-    user = sign_in_as_user(:locked => true)
-    assert_template 'sessions/new'
-    assert_contain 'Your account is locked.'
-    assert_not warden.authenticated?(:user)
-  end
-
   test "user should not send a new e-mail if already locked" do
     user = create_user(:locked => true)
     user.failed_attempts = User.maximum_attempts + 1
@@ -113,10 +106,29 @@ class LockTest < ActionController::IntegrationTest
 
   test 'error message is configurable by resource name' do
     store_translations :en, :devise => {
-      :failure => { :user => { :locked => "You are locked!" } }
+        :failure => {:user => {:locked => "You are locked!"}}
     } do
-      user = sign_in_as_user(:locked => true)
-      assert_contain 'You are locked!'
+
+      user = create_user(:locked => true)
+      user.failed_attempts = User.maximum_attempts + 1
+      user.save!
+
+      sign_in_as_user(:password => "invalid")
+      assert_contain "You are locked!"
+    end
+  end
+
+  test "user should not be able to sign in when locked" do
+    store_translations :en, :devise => {
+        :failure => {:user => {:locked => "You are locked!"}}
+    } do
+
+      user = create_user(:locked => true)
+      user.failed_attempts = User.maximum_attempts + 1
+      user.save!
+
+      sign_in_as_user(:password => "123456")
+      assert_contain "You are locked!"
     end
   end
 
@@ -157,7 +169,7 @@ class LockTest < ActionController::IntegrationTest
 
   test "when using json to ask a unlock request, should not return the user" do
     user = create_user(:locked => true)
-    post  user_unlock_path(:format => "json", :user => {:email => user.email})
+    post user_unlock_path(:format => "json", :user => {:email => user.email})
     assert_response :success
     assert_equal response.body, {}.to_json
   end
diff --git a/test/models/lockable_test.rb b/test/models/lockable_test.rb
index 1a825495..77c58153 100644
--- a/test/models/lockable_test.rb
+++ b/test/models/lockable_test.rb
@@ -23,6 +23,19 @@ class LockableTest < ActiveSupport::TestCase
     assert_equal 0, user.reload.failed_attempts
   end
 
+  test "should increment failed_attempts on successfull validation if the user is already locked" do
+    user = create_user
+    user.confirm!
+
+    swap Devise, :maximum_attempts => 2 do
+      3.times { user.valid_for_authentication?{ false } }
+      assert user.reload.access_locked?
+    end
+
+    user.valid_for_authentication?{ true }
+    assert_equal 4, user.reload.failed_attempts
+  end
+
   test "should not touch failed_attempts if lock_strategy is none" do
     user = create_user
     user.confirm!