Move param filtering to its own object and make all finder methods pa…

…ss through it, closes #1413.

lib/devise.rb

@@ -9,6 +9,7 @@ module Devise
   autoload :Delegator,   'devise/delegator'
   autoload :FailureApp,  'devise/failure_app'
   autoload :OmniAuth,    'devise/omniauth'
+  autoload :ParamFilter, 'devise/param_filter'
   autoload :PathChecker, 'devise/path_checker'
   autoload :Schema,      'devise/schema'
   autoload :TestHelpers, 'devise/test_helpers'

lib/devise/models/authenticatable.rb

@@ -112,10 +112,11 @@ def http_authenticatable?(strategy)
         #   end
         #
         def find_for_authentication(conditions)
-          conditions = filter_auth_params(conditions.dup)
-          (case_insensitive_keys || []).each { |k| conditions[k].try(:downcase!) }
-          (strip_whitespace_keys || []).each { |k| conditions[k].try(:strip!) }
-          to_adapter.find_first(conditions)
+          find_first_by_auth_conditions(conditions)
+        end
+
+        def find_first_by_auth_conditions(conditions)
+          to_adapter.find_first devise_param_filter.filter(conditions)
         end
 
         # Find an initialize a record setting an error if it can't be found.
@@ -125,14 +126,11 @@ def find_or_initialize_with_error_by(attribute, value, error=:invalid) #:nodoc:
 
         # Find an initialize a group of attributes based on a list of required attributes.
         def find_or_initialize_with_errors(required_attributes, attributes, error=:invalid) #:nodoc:
-          (case_insensitive_keys || []).each { |k| attributes[k].try(:downcase!) }
-          (strip_whitespace_keys || []).each { |k| attributes[k].try(:strip!) }
-
           attributes = attributes.slice(*required_attributes)
           attributes.delete_if { |key, value| value.blank? }
 
           if attributes.size == required_attributes.size
-            record = to_adapter.find_first(filter_auth_params(attributes))
+            record = find_first_by_auth_conditions(attributes)
           end
 
           unless record
@@ -150,16 +148,8 @@ def find_or_initialize_with_errors(required_attributes, attributes, error=:inval
 
         protected
 
-        # Force keys to be string to avoid injection on mongoid related database.
-        def filter_auth_params(conditions)
-          conditions.each do |k, v|
-            conditions[k] = v.to_s if auth_param_requires_string_conversion?(v)
-          end if conditions.is_a?(Hash)
-        end
-
-        # Determine which values should be transformed to string or passed as-is to the query builder underneath
-        def auth_param_requires_string_conversion?(value)
-          true unless value.is_a?(TrueClass) || value.is_a?(FalseClass) || value.is_a?(Fixnum)
+        def devise_param_filter
+          @devise_param_filter ||= Devise::ParamFilter.new(case_insensitive_keys, strip_whitespace_keys)
         end
 
         # Generate a token by looping and ensuring does not already exist.

lib/devise/param_filter.rb

@@ -0,0 +1,41 @@
+module Devise
+  class ParamFilter
+    def initialize(case_insensitive_keys, strip_whitespace_keys)
+      @case_insensitive_keys = case_insensitive_keys || []
+      @strip_whitespace_keys = strip_whitespace_keys || []
+    end
+
+    def filter(conditions)
+      conditions = stringify_params(conditions.dup)
+
+      @case_insensitive_keys.each do |k|
+        value = conditions[k]
+        next unless value.respond_to?(:downcase)
+        conditions[k] = value.downcase
+      end
+
+      @strip_whitespace_keys.each do |k|
+        value = conditions[k]
+        next unless value.respond_to?(:strip)
+        conditions[k] = value.strip
+      end
+
+      conditions
+    end
+
+    # Force keys to be string to avoid injection on mongoid related database.
+    def stringify_params(conditions)
+      return conditions unless conditions.is_a?(Hash)
+      conditions.each do |k, v|
+        conditions[k] = v.to_s if param_requires_string_conversion?(v)
+      end
+    end
+
+    private
+
+    # Determine which values should be transformed to string or passed as-is to the query builder underneath
+    def param_requires_string_conversion?(value)
+      true unless value.is_a?(TrueClass) || value.is_a?(FalseClass) || value.is_a?(Fixnum)
+    end
+  end
+end

test/models/database_authenticatable_test.rb

@@ -22,26 +22,9 @@ class DatabaseAuthenticatableTest < ActiveSupport::TestCase
     assert_equal email.strip, user.email
   end
 
-  test 'find_for_authentication and filter_auth_params should not modify the conditions hash' do
-    FilterAuthUser = Class.new(User) do
-      def self.filter_auth_params(conditions)
-        if conditions.is_a?(Hash) && login = conditions.delete('login')
-          key = login.include?('@') ? :email : :username
-          conditions[key] = login
-        end
-        super(conditions)
-      end
-    end
-
-    conditions = { 'login' => 'foo@bar.com' }
-    FilterAuthUser.find_for_authentication(conditions)
-
-    assert_equal({ 'login' => 'foo@bar.com' }, conditions)
-  end
-
-  test "filter_auth_params should not convert booleans and integer to strings" do
+  test "param filter should not convert booleans and integer to strings" do
     conditions = { 'login' => 'foo@bar.com', "bool1" => true, "bool2" => false, "fixnum" => 123, "will_be_converted" => (1..10) }
-    conditions = User.__send__(:filter_auth_params, conditions)
+    conditions = Devise::ParamFilter.new([], []).filter(conditions)
     assert_equal( { 'login' => 'foo@bar.com', "bool1" => true, "bool2" => false, "fixnum" => 123, "will_be_converted" => "1..10" }, conditions)
   end
 

test/support/helpers.rb

@@ -51,22 +51,33 @@ def swap(object, new_values)
       old_values[key] = object.send key
       object.send :"#{key}=", value
     end
+    clear_cached_variables(new_values)
     yield
   ensure
+    clear_cached_variables(new_values)
     old_values.each do |key, value|
       object.send :"#{key}=", value
     end
   end
 
-  def with_rails_version(constants, &block)
+  def clear_cached_variables(options)
+    if options.key?(:case_insensitive_keys) || options.key?(:strip_whitespace_keys)
+      Devise.mappings.each do |_, mapping|
+        mapping.to.instance_variable_set(:@devise_param_filter, nil)
+      end
+    end
+  end
+
+  def with_rails_version(constants)
     saved_constants = {}
+
     constants.each do |constant, val|
       saved_constants[constant] = ::Rails::VERSION.const_get constant
       Kernel::silence_warnings { ::Rails::VERSION.const_set(constant, val) }
     end
 
     begin
-      block.call
+      yield
     ensure
       constants.each do |constant, val|
         Kernel::silence_warnings { ::Rails::VERSION.const_set(constant, saved_constants[constant]) }