Finish check_unpermitted, but leave one open question failing test

actionpack/lib/action_controller/metal/strong_parameters.rb

@@ -698,7 +698,7 @@ def require(key)
     #     params.permit(person: { '0': [:email], '1': [:phone]}).to_h
     #     # => {"person"=>{"0"=>{"email"=>"[email protected]"}, "1"=>{"phone"=>"555-6789"}}}
     def permit(*filters)
-      permit_filters(filters)
+      permit_and_check_filters(filters, check_unpermitted: self.class.action_on_unpermitted_parameters, explicit_arrays: false)
     end
 
     # `expect` is the preferred way to require and permit parameters.
@@ -802,7 +802,7 @@ def permit(*filters)
     #     permitted.size            # => 2
     #
     def expect(*filters)
-      params = permit_filters(filters, check_unpermitted: false, explicit_arrays: true)
+      params = permit_keys(filters)
       keys = filters.flatten.flat_map { |f| f.is_a?(Hash) ? f.keys : f }
       values = params.require(keys)
       values.size == 1 ? values.first : values
@@ -816,7 +816,10 @@ def expect(*filters)
     # in a client library that should be fixed.
     #
     def expect!(*filters)
-      expect(*filters)
+      params = permit_keys(filters, check_unpermitted: :raise)
+      keys = filters.flatten.flat_map { |f| f.is_a?(Hash) ? f.keys : f }
+      values = params.require(keys)
+      values.size == 1 ? values.first : values
     rescue ParameterMissing => e
       raise ExpectedParameterMissing.new(e.param, e.keys)
     end
@@ -1158,24 +1161,11 @@ def each_nested_attribute
         hash
       end
 
-      # Filters params given the set of filters.
-      def permit_filters(filters, check_unpermitted: true, explicit_arrays: false)
-        params = self.class.new
-
-        filters.flatten.each do |filter|
-          case filter
-          when Symbol, String
-            # Declaration [:name, "age"]
-            permitted_scalar_filter(params, filter)
-          when Hash
-            # Declaration [{ person: ... }]
-            hash_filter(params, filter, explicit_arrays: explicit_arrays)
-          end
-        end
-
-        unpermitted_parameters!(params) if check_unpermitted && self.class.action_on_unpermitted_parameters
-
-        params.permit!
+      # Filters self and optionally checks for unpermitted keys
+      def permit_and_check_filters(filters, check_unpermitted: false, explicit_arrays: true)
+        params = permit_keys(filters, check_unpermitted: check_unpermitted, explicit_arrays: explicit_arrays)
+        unpermitted_parameters!(params, check_unpermitted: check_unpermitted)
+        params
       end
 
     private
@@ -1301,10 +1291,11 @@ def each_array_element(object, filter, &block)
         end
       end
 
-      def unpermitted_parameters!(params)
+      def unpermitted_parameters!(params, check_unpermitted: self.class.action_on_unpermitted_parameters)
+        return unless check_unpermitted
         unpermitted_keys = unpermitted_keys(params)
         if unpermitted_keys.any?
-          case self.class.action_on_unpermitted_parameters
+          case check_unpermitted
           when :log
             name = "unpermitted_parameters.action_controller"
             ActiveSupport::Notifications.instrument(name, keys: unpermitted_keys, context: @logging_context)
@@ -1349,6 +1340,24 @@ def permitted_scalar?(value)
         PERMITTED_SCALAR_TYPES.any? { |type| value.is_a?(type) }
       end
 
+      # Filters self according to the provided filters, returning a new permitted instance.
+      def permit_keys(filters, check_unpermitted: false, explicit_arrays: true)
+        params = self.class.new
+
+        filters.flatten.each do |filter|
+          case filter
+          when Symbol, String
+            # Declaration [:name, "age"]
+            permitted_scalar_filter(params, filter)
+          when Hash
+            # Declaration [{ person: ... }]
+            hash_filter(params, filter, check_unpermitted: check_unpermitted, explicit_arrays: explicit_arrays)
+          end
+        end
+
+        params.permit!
+      end
+
       # Adds existing keys to the params if their values are scalar.
       #
       # For example:
@@ -1380,55 +1389,55 @@ def non_scalar?(value)
 
       EMPTY_ARRAY = [] # :nodoc:
       EMPTY_HASH  = {} # :nodoc:
-      def hash_filter(params, filter, explicit_arrays: false)
+      def hash_filter(params, filter, check_unpermitted: false, explicit_arrays: false)
         filter = filter.with_indifferent_access
 
         # Slicing filters out non-declared keys.
         slice(*filter.keys).each do |key, value|
           next unless value
           next unless has_key? key
-          result = permit_value(value, filter[key], explicit_arrays: explicit_arrays)
+          result = permit_value(value, filter[key], check_unpermitted: check_unpermitted, explicit_arrays: explicit_arrays)
           params[key] = result unless result.nil?
         end
       end
 
-      def permit_value(value, filter, explicit_arrays:)
+      def permit_value(value, filter, check_unpermitted:,  explicit_arrays:)
         if filter == EMPTY_ARRAY # Declaration { comment_ids: [] }.
           permit_array_of_scalars(value)
         elsif filter == EMPTY_HASH # Declaration { preferences: {} }.
-          permit_hash(value, filter, explicit_arrays: explicit_arrays)
+          permit_hash(value, filter, check_unpermitted: check_unpermitted, explicit_arrays: explicit_arrays)
         elsif array_filter?(filter) # Declaration { comments: [[:text]] }
-          permit_array_of_hashes(value, filter.first, explicit_arrays: explicit_arrays)
+          permit_array_of_hashes(value, filter.first, check_unpermitted: check_unpermitted, explicit_arrays: explicit_arrays)
         elsif explicit_arrays # Declaration { user: { address: ... } } or { user: [:name, ...] } (only allows hash value)
-          permit_hash(value, filter, explicit_arrays: explicit_arrays)
-        elsif non_scalar?(value) # Declaration { user: { address: ... } } or { user: [:name, ...] } (allows array or hash)
-          permit_hash_or_array(value, filter, explicit_arrays: explicit_arrays)
+          permit_hash(value, filter, check_unpermitted: check_unpermitted, explicit_arrays: explicit_arrays)
+        elsif non_scalar?(value) # Declaration { user: { address: ... } } or { user: [:name, ...] }
+          permit_hash_or_array(value, filter, check_unpermitted: check_unpermitted, explicit_arrays: explicit_arrays)
         end
       end
 
       def permit_array_of_scalars(value)
         value if value.is_a?(Array) && value.all? { |element| permitted_scalar?(element) }
       end
 
-      def permit_array_of_hashes(value, filter, explicit_arrays:)
+      def permit_array_of_hashes(value, filter, check_unpermitted:, explicit_arrays:)
         each_array_element(value, filter) do |element|
-          element.permit_filters(Array.wrap(filter), explicit_arrays: explicit_arrays)
+          element.permit_and_check_filters(Array.wrap(filter), check_unpermitted: check_unpermitted, explicit_arrays: explicit_arrays)
         end
       end
 
-      def permit_hash(value, filter, explicit_arrays:)
+      def permit_hash(value, filter, check_unpermitted:, explicit_arrays:)
         return unless value.is_a?(Parameters)
 
         if filter == EMPTY_HASH
           permit_any_in_parameters(value)
         else
-          value.permit_filters(Array.wrap(filter), explicit_arrays:)
+          value.permit_and_check_filters(Array.wrap(filter), check_unpermitted: check_unpermitted, explicit_arrays:)
         end
       end
 
-      def permit_hash_or_array(value, filter, explicit_arrays:)
-        permit_array_of_hashes(value, filter, explicit_arrays: explicit_arrays) ||
-         permit_hash(value, filter, explicit_arrays: explicit_arrays)
+      def permit_hash_or_array(value, filter, check_unpermitted:, explicit_arrays:)
+        permit_array_of_hashes(value, filter, check_unpermitted: check_unpermitted, explicit_arrays: explicit_arrays) ||
+          permit_hash(value, filter, check_unpermitted: check_unpermitted, explicit_arrays: explicit_arrays)
       end
 
       def permit_any_in_parameters(params)

actionpack/test/controller/parameters/log_on_unpermitted_params_test.rb

@@ -50,16 +50,26 @@ def teardown
     end
   end
 
-  test "logs on unexpected nested params with expect" do
+  test "does not log on unexpected nested params with expect" do
     request_params = { book: { pages: 65, title: "Green Cats and where to find then.", author: "G. A. Dog" } }
     context = { "action" => "my_action", "controller" => "my_controller" }
     params = ActionController::Parameters.new(request_params, context)
 
-    assert_logged("Unpermitted parameters: :title, :author. Context: { action: my_action, controller: my_controller }") do
+    assert_logged("") do
       params.expect(book: :pages)
     end
   end
 
+  test "raises on unexpected nested params with expect!" do
+    request_params = { book: { pages: 65, title: "Green Cats and where to find then.", author: "G. A. Dog" } }
+    context = { "action" => "my_action", "controller" => "my_controller" }
+    params = ActionController::Parameters.new(request_params, context)
+
+    assert_raises(ActionController::UnpermittedParameters) do
+      params.expect!(book: :pages)
+    end
+  end
+
   test "logs on unexpected param with deep_dup" do
     request_params = { book: { pages: 3, author: "YY" } }
     context = { "action" => "my_action", "controller" => "my_controller" }

actionpack/test/controller/parameters/raise_on_unpermitted_params_test.rb

@@ -31,16 +31,16 @@ def teardown
     end
   end
 
-  test "expect raises on unexpected nested params" do
+  test "expect never raises on unexpected nested params" do
     params = ActionController::Parameters.new(
       book: { pages: 65, title: "Green Cats and where to find them." })
 
-    assert_raises(ActionController::UnpermittedParameters) do
+    assert_nothing_raised do
       params.expect(book: [:pages])
     end
   end
 
-  test "expect doesn't raise on unpermitted root params" do
+  test "expect never raises on unpermitted root params" do
     params = ActionController::Parameters.new(
       id: 1,
       book: { pages: 65, title: "Green Cats and where to find them." })
@@ -50,4 +50,56 @@ def teardown
       params.expect(:id)
     end
   end
+
+  test "expect! raises on unexpected nested params" do
+    params = ActionController::Parameters.new(
+      book: { pages: 65, title: "Green Cats and where to find them." })
+
+    assert_raises(ActionController::UnpermittedParameters) do
+      params.expect!(book: [:pages])
+    end
+  end
+
+  test "expect! raises on unpermitted root params" do
+    params = ActionController::Parameters.new(
+      author_id: 1,
+      id: 1,
+      book: { pages: 65, title: "Green Cats and where to find them." })
+
+    # this one clearly should not raise, everything is covered.
+    assert_nothing_raised do
+      params.expect!(:id, :author_id, book: [:pages, :title])
+    end
+
+    # This one is the tricky one. Should it raise?
+    assert_nothing_raised do
+      params.expect!(book: [:pages, :title])
+    end
+
+    # this one clearly should raise, both a root key and a hash are declared.
+    assert_raises(ActionController::UnpermittedParameters) do
+      params.expect!(:id, book: [:pages, :title])
+    end
+
+    # This one is not quite as obvious. Both scalar root keys are declared
+    assert_raises(ActionController::UnpermittedParameters) do
+      params.expect!(:id, :author_id)
+    end
+
+    assert_raises(ActionController::UnpermittedParameters) do
+      params.expect!(:id)
+    end
+
+    assert_raises(ActionController::UnpermittedParameters) do
+      params[:book].expect!(:pages)
+    end
+
+    assert_nothing_raised do
+      params[:book].expect!(:pages, :title)
+    end
+
+    assert_nothing_raised do
+      params(book: {}).expect!(:pages, :title)
+    end
+  end
 end