From 7346ce709a52f73c347da9573c1bff406d03d7b7 Mon Sep 17 00:00:00 2001 From: Justin Bull Date: Fri, 29 Apr 2016 17:31:33 -0400 Subject: [PATCH 1/2] :beetle: Fix strategy checking in #unlock_strategy_enabled? for :none and undefined strategies A bug that if the unlock strategy was set to `:both`, it would return true for all & any inputs See #4072 --- lib/devise/models/lockable.rb | 4 +++- test/models/lockable_test.rb | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/lib/devise/models/lockable.rb b/lib/devise/models/lockable.rb index 9522dc07..971ab9ba 100644 --- a/lib/devise/models/lockable.rb +++ b/lib/devise/models/lockable.rb @@ -181,7 +181,9 @@ module Devise # Is the unlock enabled for the given unlock strategy? def unlock_strategy_enabled?(strategy) - [:both, strategy].include?(self.unlock_strategy) + self.unlock_strategy == strategy || + # only :time and :email are subsets of the :both strategy + (self.unlock_strategy == :both && [:time, :email].include?(strategy)) end # Is the lock enabled for the given lock strategy? diff --git a/test/models/lockable_test.rb b/test/models/lockable_test.rb index d4ded249..ac86c779 100644 --- a/test/models/lockable_test.rb +++ b/test/models/lockable_test.rb @@ -325,4 +325,26 @@ class LockableTest < ActiveSupport::TestCase user.lock_access! assert_equal :locked, user.unauthenticated_message end + + test 'unlock_strategy_enabled? should return true for both, email, and time strategies if :both is used' do + swap Devise, unlock_strategy: :both do + user = create_user + assert_equal true, user.unlock_strategy_enabled?(:both) + assert_equal true, user.unlock_strategy_enabled?(:time) + assert_equal true, user.unlock_strategy_enabled?(:email) + assert_equal false, user.unlock_strategy_enabled?(:none) + assert_equal false, user.unlock_strategy_enabled?(:an_undefined_strategy) + end + end + + test 'unlock_strategy_enabled? should return true only for the configured strategy' do + swap Devise, unlock_strategy: :email do + user = create_user + assert_equal false, user.unlock_strategy_enabled?(:both) + assert_equal false, user.unlock_strategy_enabled?(:time) + assert_equal true, user.unlock_strategy_enabled?(:email) + assert_equal false, user.unlock_strategy_enabled?(:none) + assert_equal false, user.unlock_strategy_enabled?(:an_undefined_strategy) + end + end end From 3226ab16c1381c5b33cf21a114c4871d8be8d06f Mon Sep 17 00:00:00 2001 From: Justin Bull Date: Mon, 2 May 2016 14:22:09 -0400 Subject: [PATCH 2/2] Extract list of both strategies into class constant --- lib/devise/models/lockable.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/devise/models/lockable.rb b/lib/devise/models/lockable.rb index 971ab9ba..fe05b255 100644 --- a/lib/devise/models/lockable.rb +++ b/lib/devise/models/lockable.rb @@ -155,6 +155,9 @@ module Devise end module ClassMethods + # List of strategies that are enabled/supported if :both is used. + BOTH_STRATEGIES = [:time, :email] + # Attempt to find a user by its unlock keys. If a record is found, send new # unlock instructions to it. If not user is found, returns a new user # with an email not found error. @@ -182,8 +185,7 @@ module Devise # Is the unlock enabled for the given unlock strategy? def unlock_strategy_enabled?(strategy) self.unlock_strategy == strategy || - # only :time and :email are subsets of the :both strategy - (self.unlock_strategy == :both && [:time, :email].include?(strategy)) + (self.unlock_strategy == :both && BOTH_STRATEGIES.include?(strategy)) end # Is the lock enabled for the given lock strategy?