Fix some unlockable bugs.

app/controllers/devise/unlocks_controller.rb

@@ -1,4 +1,5 @@
 class Devise::UnlocksController < ApplicationController
+  prepend_before_filter :ensure_email_as_unlock_strategy
   prepend_before_filter :require_no_authentication
   include Devise::Controllers::InternalHelpers
 
@@ -31,4 +32,10 @@ class Devise::UnlocksController < ApplicationController
       render_with_scope :new
     end
   end
+
+  protected
+
+  def ensure_email_as_unlock_strategy
+    raise ActionController::UnknownAction unless resource_class.unlock_strategy_enabled?(:email)
+  end
 end

lib/devise/models/confirmable.rb

@@ -57,15 +57,9 @@ module Devise
         ::Devise::Mailer.confirmation_instructions(self).deliver
       end
 
-      # Remove confirmation date and send confirmation instructions, to ensure
+      # Resend confirmation token. This method does not need to generate a new token.
-      # after sending these instructions the user won't be able to sign in without
-      # confirming it's account
       def resend_confirmation_token
-        unless_confirmed do
+        unless_confirmed { send_confirmation_instructions }
-          generate_confirmation_token
-          save(:validate => false)
-          send_confirmation_instructions
-        end
       end
 
       # Overwrites active? from Devise::Models::Activatable for confirmation

lib/devise/models/lockable.rb

@@ -23,9 +23,10 @@ module Devise
 
       # Lock an user setting it's locked_at to actual time.
       def lock_access!
+        return true if access_locked?
         self.locked_at = Time.now
 
-        if unlock_strategy_enabled?(:email)
+        if self.class.unlock_strategy_enabled?(:email)
           generate_unlock_token
           send_unlock_instructions
         end
@@ -55,11 +56,7 @@ module Devise
 
       # Resend the unlock instructions if the user is locked.
       def resend_unlock_token
-        if_access_locked do
+        if_access_locked { send_unlock_instructions }
-          generate_unlock_token unless unlock_token.present?
-          save(:validate => false)
-          send_unlock_instructions
-        end
       end
 
       # Overwrites active? from Devise::Models::Activatable for locking purposes
@@ -82,10 +79,7 @@ module Devise
           self.failed_attempts = 0
         else
           self.failed_attempts += 1
-          if failed_attempts > self.class.maximum_attempts
+          lock_access! if failed_attempts > self.class.maximum_attempts
-            lock_access! 
-            return false
-          end
         end
         save(:validate => false) if changed?
         result
@@ -100,7 +94,7 @@ module Devise
 
         # Tells if the lock is expired if :time unlock strategy is active
         def lock_expired?
-          if unlock_strategy_enabled?(:time)
+          if self.class.unlock_strategy_enabled?(:time)
             locked_at && locked_at < self.class.unlock_in.ago
           else
             false
@@ -118,11 +112,6 @@ module Devise
           end
         end
 
-        # Is the unlock enabled for the given unlock strategy?
-        def unlock_strategy_enabled?(strategy)
-          [:both, strategy].include?(self.class.unlock_strategy)
-        end
-
       module ClassMethods
         # Attempt to find a user by it's email. If a record is found, send new
         # unlock instructions to it. If not user is found, returns a new user
@@ -144,6 +133,11 @@ module Devise
           lockable
         end
 
+        # Is the unlock enabled for the given unlock strategy?
+        def unlock_strategy_enabled?(strategy)
+          [:both, strategy].include?(self.unlock_strategy)
+        end
+
         Devise::Models.config(self, :maximum_attempts, :unlock_strategy, :unlock_in)
       end
     end

test/integration/lockable_test.rb

@@ -36,6 +36,15 @@ class LockTest < ActionController::IntegrationTest
     assert_equal 0, ActionMailer::Base.deliveries.size
   end
 
+  test 'unlocked pages should not be available if email strategy is disabled' do
+    visit new_user_unlock_path
+    swap Devise, :unlock_strategy => :time do
+      assert_raise AbstractController::ActionNotFound do
+        visit new_user_unlock_path
+      end
+    end
+  end
+
   test 'user with invalid unlock token should not be able to unlock an account' do
     visit_user_unlock_with_token('invalid_token')
 
@@ -60,7 +69,6 @@ class LockTest < ActionController::IntegrationTest
   test "sign in user automatically after unlocking it's account" do
     user = create_user(:locked => true)
     visit_user_unlock_with_token(user.unlock_token)
-
     assert warden.authenticated?(:user)
   end
 
@@ -71,6 +79,16 @@ class LockTest < ActionController::IntegrationTest
     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.update_attribute(:failed_attempts, User.maximum_attempts + 1)
+    ActionMailer::Base.deliveries.clear
+
+    sign_in_as_user(:password => "invalid")
+    assert_contain 'Invalid email or password.'
+    assert ActionMailer::Base.deliveries.empty?
+  end
+
   test 'error message is configurable by resource name' do
     store_translations :en, :devise => {
       :sessions => { :admin => { :locked => "You are locked!" } }

test/models/confirmable_test.rb

@@ -11,15 +11,6 @@ class ConfirmableTest < ActiveSupport::TestCase
     assert_not_nil create_user.confirmation_token
   end
 
-  test 'should regenerate confirmation token each time' do
-    user = create_user
-    3.times do
-      token = user.confirmation_token
-      user.resend_confirmation_token
-      assert_not_equal token, user.confirmation_token
-    end
-  end
-
   test 'should never generate the same confirmation token for different users' do
     confirmation_tokens = []
     3.times do
@@ -137,13 +128,6 @@ class ConfirmableTest < ActiveSupport::TestCase
     assert_equal "not found", confirmation_user.errors[:email].join
   end
 
-  test 'should generate a confirmation token before send the confirmation instructions email' do
-    user = create_user
-    token = user.confirmation_token
-    confirmation_user = User.send_confirmation_instructions(:email => user.email)
-    assert_not_equal token, user.reload.confirmation_token
-  end
-
   test 'should send email instructions for the user confirm it\'s email' do
     user = create_user
     assert_email_sent do

test/models/lockable_test.rb

@@ -37,7 +37,7 @@ class LockableTest < ActiveSupport::TestCase
     assert_equal 0, user.reload.failed_attempts
   end
 
-  test "should verify wheter a user is locked or not" do
+  test "should verify whether a user is locked or not" do
     user = create_user
     assert_not user.access_locked?
     user.lock_access!
@@ -64,6 +64,14 @@ class LockableTest < ActiveSupport::TestCase
     assert 0, user.reload.failed_attempts
   end
 
+  test "should not lock a locked account" do
+    user = create_user
+    user.lock_access!
+    assert_no_difference "ActionMailer::Base.deliveries.size" do
+      user.lock_access!
+    end
+  end
+
   test 'should not unlock an unlocked user' do
     user = create_user
     assert_not user.unlock_access!
@@ -101,16 +109,6 @@ class LockableTest < ActiveSupport::TestCase
     assert_not_nil user.unlock_token
   end
 
-  test 'should not regenerate unlock token if it already exists' do
-    user = create_user
-    user.lock!
-    3.times do
-      token = user.unlock_token
-      user.resend_unlock_token
-      assert_equal token, user.unlock_token
-    end
-  end
-
   test "should never generate the same unlock token for different users" do
     unlock_tokens = []
     3.times do

test/support/integration.rb

@@ -33,7 +33,7 @@ class ActionDispatch::IntegrationTest
     user = create_user(options)
     get new_user_session_path unless options[:visit] == false
     fill_in 'email', :with => 'user@test.com'
-    fill_in 'password', :with => '123456'
+    fill_in 'password', :with => options[:password] || '123456'
     check 'remember me' if options[:remember_me] == true
     yield if block_given?
     click_button 'Sign In'