diff --git a/app/controllers/devise/unlocks_controller.rb b/app/controllers/devise/unlocks_controller.rb index d82c96b9..a813ddfc 100644 --- a/app/controllers/devise/unlocks_controller.rb +++ b/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 diff --git a/lib/devise/models/confirmable.rb b/lib/devise/models/confirmable.rb index 88bef8d3..ffa2e0c5 100644 --- a/lib/devise/models/confirmable.rb +++ b/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 - # after sending these instructions the user won't be able to sign in without - # confirming it's account + # Resend confirmation token. This method does not need to generate a new token. def resend_confirmation_token - unless_confirmed do - generate_confirmation_token - save(:validate => false) - send_confirmation_instructions - end + unless_confirmed { send_confirmation_instructions } end # Overwrites active? from Devise::Models::Activatable for confirmation diff --git a/lib/devise/models/lockable.rb b/lib/devise/models/lockable.rb index 3212601b..6d000716 100644 --- a/lib/devise/models/lockable.rb +++ b/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 - generate_unlock_token unless unlock_token.present? - save(:validate => false) - send_unlock_instructions - end + if_access_locked { send_unlock_instructions } 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! - return false - end + lock_access! if failed_attempts > self.class.maximum_attempts 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 diff --git a/test/integration/lockable_test.rb b/test/integration/lockable_test.rb index 8b59161a..69b18291 100644 --- a/test/integration/lockable_test.rb +++ b/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!" } } diff --git a/test/models/confirmable_test.rb b/test/models/confirmable_test.rb index 5632d1fa..5466e64f 100644 --- a/test/models/confirmable_test.rb +++ b/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 diff --git a/test/models/lockable_test.rb b/test/models/lockable_test.rb index a95f708d..a86d5fab 100644 --- a/test/models/lockable_test.rb +++ b/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 diff --git a/test/support/integration.rb b/test/support/integration.rb index 598dbd4b..b907b137 100644 --- a/test/support/integration.rb +++ b/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'