Move FilterParameterLogging to a stand alone module and make it work on new base

actionpack/Rakefile

@@ -66,7 +66,7 @@ Rake::TestTask.new(:test_new_base_on_old_tests) do |t|
     render render_json render_xml
     send_file request_forgery_protection rescue url_rewriter verification webservice
     http_basic_authentication http_digest_authentication
-    action_pack_assertions assert_select
+    action_pack_assertions assert_select filter_params
   ).map { |name| "test/controller/#{name}_test.rb" }
 end
 

actionpack/lib/action_controller.rb

@@ -66,6 +66,7 @@ module ActionController
   autoload :UrlRewriter, 'action_controller/routing/generation/url_rewriter'
   autoload :UrlWriter, 'action_controller/routing/generation/url_rewriter'
   autoload :Verification, 'action_controller/base/verification'
+  autoload :FilterParameterLogging, 'action_controller/base/filter_parameter_logging'
 
   module Assertions
     autoload :DomAssertions, 'action_controller/testing/assertions/dom'

actionpack/lib/action_controller/base/base.rb

@@ -449,55 +449,6 @@ module ActionController #:nodoc:
         @view_paths.push(*path)
       end
       
-      # Replace sensitive parameter data from the request log.
-      # Filters parameters that have any of the arguments as a substring.
-      # Looks in all subhashes of the param hash for keys to filter.
-      # If a block is given, each key and value of the parameter hash and all
-      # subhashes is passed to it, the value or key
-      # can be replaced using String#replace or similar method.
-      #
-      # Examples:
-      #   filter_parameter_logging
-      #   => Does nothing, just slows the logging process down
-      #
-      #   filter_parameter_logging :password
-      #   => replaces the value to all keys matching /password/i with "[FILTERED]"
-      #
-      #   filter_parameter_logging :foo, "bar"
-      #   => replaces the value to all keys matching /foo|bar/i with "[FILTERED]"
-      #
-      #   filter_parameter_logging { |k,v| v.reverse! if k =~ /secret/i }
-      #   => reverses the value to all keys matching /secret/i
-      #
-      #   filter_parameter_logging(:foo, "bar") { |k,v| v.reverse! if k =~ /secret/i }
-      #   => reverses the value to all keys matching /secret/i, and
-      #      replaces the value to all keys matching /foo|bar/i with "[FILTERED]"
-      def filter_parameter_logging(*filter_words, &block)
-        parameter_filter = Regexp.new(filter_words.collect{ |s| s.to_s }.join('|'), true) if filter_words.length > 0
-
-        define_method(:filter_parameters) do |unfiltered_parameters|
-          filtered_parameters = {}
-
-          unfiltered_parameters.each do |key, value|
-            if key =~ parameter_filter
-              filtered_parameters[key] = '[FILTERED]'
-            elsif value.is_a?(Hash)
-              filtered_parameters[key] = filter_parameters(value)
-            elsif block_given?
-              key = key.dup
-              value = value.dup if value
-              yield key, value
-              filtered_parameters[key] = value
-            else
-              filtered_parameters[key] = value
-            end
-          end
-
-          filtered_parameters
-        end
-        protected :filter_parameters
-      end
-      
       @@exempt_from_layout = [ActionView::TemplateHandlers::RJS]
       
       def exempt_from_layout(*types)
@@ -853,13 +804,6 @@ module ActionController #:nodoc:
         logger.info(request_id)
       end
 
-      def log_processing_for_parameters
-        parameters = respond_to?(:filter_parameters) ? filter_parameters(params) : params.dup
-        parameters = parameters.except!(:controller, :action, :format, :_method)
-
-        logger.info "  Parameters: #{parameters.inspect}" unless parameters.empty?
-      end
-
       def default_render #:nodoc:
         render
       end
@@ -933,7 +877,7 @@ module ActionController #:nodoc:
     [ Filters, Layout, Renderer, Redirector, Responder, Benchmarking, Rescue, Flash, MimeResponds, Helpers,
       Cookies, Caching, Verification, Streaming, SessionManagement,
       HttpAuthentication::Basic::ControllerMethods, HttpAuthentication::Digest::ControllerMethods, RecordIdentifier,
-      RequestForgeryProtection, Translation
+      RequestForgeryProtection, Translation, FilterParameterLogging
     ].each do |mod|
       include mod
     end

actionpack/lib/action_controller/base/filter_parameter_logging.rb

@@ -0,0 +1,97 @@
+module ActionController
+  module FilterParameterLogging
+    extend ActiveSupport::DependencyModule
+
+    # TODO : Remove the defined? check when new base is the main base
+    if defined?(ActionController::Http)
+      depends_on AbstractController::Logger
+    end
+
+    included do
+      if defined?(ActionController::Http)
+        include InstanceMethodsForNewBase
+      end
+    end
+
+    module ClassMethods
+      # Replace sensitive parameter data from the request log.
+      # Filters parameters that have any of the arguments as a substring.
+      # Looks in all subhashes of the param hash for keys to filter.
+      # If a block is given, each key and value of the parameter hash and all
+      # subhashes is passed to it, the value or key
+      # can be replaced using String#replace or similar method.
+      #
+      # Examples:
+      #   filter_parameter_logging
+      #   => Does nothing, just slows the logging process down
+      #
+      #   filter_parameter_logging :password
+      #   => replaces the value to all keys matching /password/i with "[FILTERED]"
+      #
+      #   filter_parameter_logging :foo, "bar"
+      #   => replaces the value to all keys matching /foo|bar/i with "[FILTERED]"
+      #
+      #   filter_parameter_logging { |k,v| v.reverse! if k =~ /secret/i }
+      #   => reverses the value to all keys matching /secret/i
+      #
+      #   filter_parameter_logging(:foo, "bar") { |k,v| v.reverse! if k =~ /secret/i }
+      #   => reverses the value to all keys matching /secret/i, and
+      #      replaces the value to all keys matching /foo|bar/i with "[FILTERED]"
+      def filter_parameter_logging(*filter_words, &block)
+        parameter_filter = Regexp.new(filter_words.collect{ |s| s.to_s }.join('|'), true) if filter_words.length > 0
+
+        define_method(:filter_parameters) do |unfiltered_parameters|
+          filtered_parameters = {}
+
+          unfiltered_parameters.each do |key, value|
+            if key =~ parameter_filter
+              filtered_parameters[key] = '[FILTERED]'
+            elsif value.is_a?(Hash)
+              filtered_parameters[key] = filter_parameters(value)
+            elsif block_given?
+              key = key.dup
+              value = value.dup if value
+              yield key, value
+              filtered_parameters[key] = value
+            else
+              filtered_parameters[key] = value
+            end
+          end
+
+          filtered_parameters
+        end
+        protected :filter_parameters
+      end
+    end
+
+    module InstanceMethodsForNewBase
+      # TODO : Fix the order of information inside such that it's exactly same as the old base
+      def process(*)
+        ret = super
+
+        if logger
+          parameters = respond_to?(:filter_parameters) ? filter_parameters(params) : params.dup
+          parameters = parameters.except!(:controller, :action, :format, :_method)
+
+          unless parameters.empty?
+            # TODO : Move DelayedLog to AS
+            log = AbstractController::Logger::DelayedLog.new { "  Parameters: #{parameters.inspect}" }
+            logger.info(log)
+          end
+        end
+
+        ret
+      end
+    end
+
+    private
+
+    # TODO : This method is not needed for the new base
+    def log_processing_for_parameters
+      parameters = respond_to?(:filter_parameters) ? filter_parameters(params) : params.dup
+      parameters = parameters.except!(:controller, :action, :format, :_method)
+
+      logger.info "  Parameters: #{parameters.inspect}" unless parameters.empty?
+    end
+  end
+end

actionpack/lib/action_controller/new_base.rb

@@ -32,6 +32,7 @@ module ActionController
   autoload :RequestForgeryProtection, 'action_controller/base/request_forgery_protection'
   autoload :Streaming,                'action_controller/base/streaming'
   autoload :HttpAuthentication,       'action_controller/base/http_authentication'
+  autoload :FilterParameterLogging,   'action_controller/base/filter_parameter_logging'
 
   require 'action_controller/routing'
 end

actionpack/lib/action_controller/new_base/base.rb

@@ -33,6 +33,7 @@ module ActionController
     include ActionController::Streaming
     include ActionController::HttpAuthentication::Basic::ControllerMethods
     include ActionController::HttpAuthentication::Digest::ControllerMethods
+    include ActionController::FilterParameterLogging
 
     # TODO: Extract into its own module
     # This should be moved together with other normalizing behavior

actionpack/test/controller/filter_params_test.rb

@@ -1,13 +1,30 @@
 require 'abstract_unit'
 
 class FilterParamController < ActionController::Base
+  def payment
+    head :ok
+  end
 end
 
-class FilterParamTest < Test::Unit::TestCase
-  def setup
-    @controller = FilterParamController.new
+class FilterParamTest < ActionController::TestCase
+  tests FilterParamController
+
+  class MockLogger
+    attr_reader :logged
+    attr_accessor :level
+    
+    def initialize
+      @level = Logger::DEBUG
+    end
+    
+    def method_missing(method, *args)
+      @logged ||= []
+      @logged << args.first
+    end
   end
 
+  setup :set_logger
+
   def test_filter_parameters
     assert FilterParamController.respond_to?(:filter_parameter_logging)
     assert !@controller.respond_to?(:filter_parameters)
@@ -46,4 +63,26 @@ class FilterParamTest < Test::Unit::TestCase
     assert !FilterParamController.action_methods.include?('filter_parameters')
     assert_raise(NoMethodError) { @controller.filter_parameters([{'password' => '[FILTERED]'}]) }
   end
+
+  def test_filter_parameters_inside_logs
+    FilterParamController.filter_parameter_logging(:lifo, :amount)
+
+    get :payment, :lifo => 'Pratik', :amount => '420', :step => '1'
+
+    filtered_params_logs = logs.detect {|l| l =~ /\AParameters/ }
+
+    assert filtered_params_logs.index('"amount"=>"[FILTERED]"')
+    assert filtered_params_logs.index('"lifo"=>"[FILTERED]"')
+    assert filtered_params_logs.index('"step"=>"1"')
+  end
+
+  private
+
+  def set_logger
+    @controller.logger = MockLogger.new
+  end
+  
+  def logs
+    @logs ||= @controller.logger.logged.compact.map {|l| l.to_s.strip}
+  end
 end