Merge pull request #21000 from twalpole/find_or_parameter_issues

Update and fix forbidden attributes test issues caused by AC::Parameters change

activemodel/lib/active_model/attribute_assignment.rb

@@ -27,7 +27,7 @@ def assign_attributes(new_attributes)
       if !new_attributes.respond_to?(:stringify_keys)
         raise ArgumentError, "When assigning attributes, you must pass a hash as an argument."
       end
-      return if new_attributes.blank?
+      return if new_attributes.nil? || new_attributes.empty?
 
       attributes = new_attributes.stringify_keys
       _assign_attributes(sanitize_for_mass_assignment(attributes))

activemodel/test/cases/attribute_assignment_test.rb

@@ -23,13 +23,32 @@ def broken_attribute=(value)
   class ErrorFromAttributeWriter < StandardError
   end
 
-  class ProtectedParams < ActiveSupport::HashWithIndifferentAccess
+  class ProtectedParams
+    attr_accessor :permitted
+    alias :permitted? :permitted
+
+    delegate :keys, :key?, :has_key?, :empty?, to: :@parameters
+
+    def initialize(attributes)
+      @parameters = attributes.with_indifferent_access
+      @permitted = false
+    end
+
     def permit!
       @permitted = true
+      self
+    end
+
+    def [](key)
+      @parameters[key]
+    end
+
+    def to_h
+      @parameters
     end
 
-    def permitted?
-      @permitted ||= false
+    def stringify_keys
+      dup
     end
 
     def dup

activemodel/test/cases/forbidden_attributes_protection_test.rb

@@ -2,19 +2,25 @@
 require 'active_support/core_ext/hash/indifferent_access'
 require 'models/account'
 
-class ProtectedParams < ActiveSupport::HashWithIndifferentAccess
+class ProtectedParams
   attr_accessor :permitted
   alias :permitted? :permitted
 
+  delegate :keys, :key?, :has_key?, :empty?, to: :@parameters
+
   def initialize(attributes)
-    super(attributes)
+    @parameters = attributes
     @permitted = false
   end
 
   def permit!
     @permitted = true
     self
   end
+
+  def to_h
+    @parameters
+  end
 end
 
 class ActiveModelMassUpdateProtectionTest < ActiveSupport::TestCase

activerecord/lib/active_record/inheritance.rb

@@ -198,10 +198,11 @@ def type_condition(table = arel_table)
       # If this is a StrongParameters hash, and access to inheritance_column is not permitted,
       # this will ignore the inheritance column and return nil
       def subclass_from_attributes?(attrs)
-        attribute_names.include?(inheritance_column) && attrs.is_a?(Hash)
+        attribute_names.include?(inheritance_column) && (attrs.is_a?(Hash) || attrs.respond_to?(:permitted?))
       end
 
       def subclass_from_attributes(attrs)
+        attrs = attrs.to_h if attrs.respond_to?(:permitted?)
         subclass_name = attrs.with_indifferent_access[inheritance_column]
 
         if subclass_name.present?

activerecord/lib/active_record/relation/query_methods.rb

@@ -13,6 +13,8 @@ module QueryMethods
     # WhereChain objects act as placeholder for queries in which #where does not have any parameter.
     # In this case, #where must be chained with #not to return a new relation.
     class WhereChain
+      include ActiveModel::ForbiddenAttributesProtection
+
       def initialize(scope)
         @scope = scope
       end
@@ -41,6 +43,8 @@ def initialize(scope)
       #    User.where.not(name: "Jon", role: "admin")
       #    # SELECT * FROM users WHERE name != 'Jon' AND role != 'admin'
       def not(opts, *rest)
+        opts = sanitize_forbidden_attributes(opts)
+
         where_clause = @scope.send(:where_clause_factory).build(opts, rest)
 
         @scope.references!(PredicateBuilder.references(opts)) if Hash === opts

activerecord/test/cases/forbidden_attributes_protection_test.rb

@@ -3,12 +3,14 @@
 require 'models/person'
 require 'models/company'
 
-class ProtectedParams < ActiveSupport::HashWithIndifferentAccess
+class ProtectedParams
   attr_accessor :permitted
   alias :permitted? :permitted
 
+  delegate :keys, :key?, :has_key?, :empty?, to: :@parameters
+
   def initialize(attributes)
-    super(attributes)
+    @parameters = attributes.with_indifferent_access
     @permitted = false
   end
 
@@ -17,6 +19,18 @@ def permit!
     self
   end
 
+  def [](key)
+    @parameters[key]
+  end
+
+  def to_h
+    @parameters
+  end
+
+  def stringify_keys
+    dup
+  end
+
   def dup
     super.tap do |duplicate|
       duplicate.instance_variable_set :@permitted, @permitted
@@ -75,6 +89,13 @@ def test_create_with_checks_permitted
     end
   end
 
+  def test_create_with_works_with_permitted_params
+    params = ProtectedParams.new(first_name: 'Guille').permit!
+
+    person = Person.create_with(params).create!
+    assert_equal 'Guille', person.first_name
+  end
+
   def test_create_with_works_with_params_values
     params = ProtectedParams.new(first_name: 'Guille')
 
@@ -90,10 +111,51 @@ def test_where_checks_permitted
     end
   end
 
+  def test_where_works_with_permitted_params
+    params = ProtectedParams.new(first_name: 'Guille').permit!
+
+    person = Person.where(params).create!
+    assert_equal 'Guille', person.first_name
+  end
+
   def test_where_works_with_params_values
     params = ProtectedParams.new(first_name: 'Guille')
 
     person = Person.where(first_name: params[:first_name]).create!
     assert_equal 'Guille', person.first_name
   end
+
+  def test_where_not_checks_permitted
+    params = ProtectedParams.new(first_name: 'Guille', gender: 'm')
+
+    assert_raises(ActiveModel::ForbiddenAttributesError) do
+      Person.where().not(params)
+    end
+  end
+
+  def test_where_not_works_with_permitted_params
+    params = ProtectedParams.new(first_name: 'Guille').permit!
+    Person.create!(params)
+    assert_empty Person.where.not(params).select {|p| p.first_name == 'Guille' }
+  end
+
+  def test_strong_params_style_objects_work_with_singular_associations
+    params = ProtectedParams.new( name: "Stern", ship_attributes: ProtectedParams.new(name: "The Black Rock").permit!).permit!
+    part = ShipPart.new(params)
+
+    assert_equal "Stern", part.name
+    assert_equal "The Black Rock", part.ship.name
+  end
+
+  def test_strong_params_style_objects_work_with_collection_associations
+    params = ProtectedParams.new(
+      trinkets_attributes: ProtectedParams.new(
+        "0" => ProtectedParams.new(name: "Necklace").permit!,
+        "1" => ProtectedParams.new(name: "Spoon").permit! ) ).permit!
+    part = ShipPart.new(params)
+
+    assert_equal "Necklace", part.trinkets[0].name
+    assert_equal "Spoon", part.trinkets[1].name
+  end
+
 end

activerecord/test/cases/nested_attributes_test.rb

@@ -1068,39 +1068,4 @@ def setup
     assert_not part.valid?
     assert_equal ["Ship name can't be blank"], part.errors.full_messages
   end
-
-  class ProtectedParameters
-    def initialize(hash)
-      @hash = hash
-    end
-
-    def permitted?
-      true
-    end
-
-    def [](key)
-      @hash[key]
-    end
-
-    def to_h
-      @hash
-    end
-  end
-
-  test "strong params style objects can be assigned for singular associations" do
-    params = { name: "Stern", ship_attributes:
-               ProtectedParameters.new(name: "The Black Rock") }
-    part = ShipPart.new(params)
-
-    assert_equal "Stern", part.name
-    assert_equal "The Black Rock", part.ship.name
-  end
-
-  test "strong params style objects can be assigned for collection associations" do
-    params = { trinkets_attributes: ProtectedParameters.new("0" => ProtectedParameters.new(name: "Necklace"), "1" => ProtectedParameters.new(name: "Spoon")) }
-    part = ShipPart.new(params)
-
-    assert_equal "Necklace", part.trinkets[0].name
-    assert_equal "Spoon", part.trinkets[1].name
-  end
 end