Merge pull request #5289 from rafaelfranca/fix-through-associations

Fix has_many through associations when mass_assignment_sanitizer is strict

6 files changed, 43 insertions, 8 deletions

diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index 9657cb081d..53d49fef2e 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -73,7 +73,9 @@ module ActiveRecord
         # association
         def build_through_record(record)
           @through_records[record.object_id] ||= begin
-            through_record = through_association.build(construct_join_attributes(record))
+            ensure_mutable
+
+            through_record = through_association.build
             through_record.send("#{source_reflection.name}=", record)
             through_record
           end
diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb
index f95e5337c2..fd0e90aaf0 100644
--- a/activerecord/lib/active_record/associations/through_association.rb
+++ b/activerecord/lib/active_record/associations/through_association.rb
@@ -37,9 +37,7 @@ module ActiveRecord
         # situation it is more natural for the user to just create or modify their join records
         # directly as required.
         def construct_join_attributes(*records)
-          if source_reflection.macro != :belongs_to
-            raise HasManyThroughCantAssociateThroughHasOneOrManyReflection.new(owner, reflection)
-          end
+          ensure_mutable
 
           join_attributes = {
             source_reflection.foreign_key =>
@@ -73,6 +71,12 @@ module ActiveRecord
           !owner[through_reflection.foreign_key].nil?
         end
 
+        def ensure_mutable
+          if source_reflection.macro != :belongs_to
+            raise HasManyThroughCantAssociateThroughHasOneOrManyReflection.new(owner, reflection)
+          end
+        end
+
         def ensure_not_nested
           if reflection.nested?
             raise HasManyThroughNestedAssociationsAreReadonly.new(owner, reflection)
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
index 12cae934b6..e9b930204f 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -44,17 +44,33 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
   end
 
   def test_associate_existing
-    posts(:thinking); people(:david) # Warm cache
+    post   = posts(:thinking)
+    person = people(:david)
 
     assert_queries(1) do
-      posts(:thinking).people << people(:david)
+      post.people << person
     end
 
     assert_queries(1) do
-      assert posts(:thinking).people.include?(people(:david))
+      assert post.people.include?(person)
     end
 
-    assert posts(:thinking).reload.people(true).include?(people(:david))
+    assert post.reload.people(true).include?(person)
+  end
+
+  def test_associate_existing_with_strict_mass_assignment_sanitizer
+    SecureReader.mass_assignment_sanitizer = :strict
+
+    SecureReader.new
+
+    post   = posts(:thinking)
+    person = people(:david)
+
+    assert_queries(1) do
+      post.secure_people << person
+    end
+  ensure
+    SecureReader.mass_assignment_sanitizer = :logger
   end
 
   def test_associate_existing_record_twice_should_add_to_target_twice
diff --git a/activerecord/test/models/person.rb b/activerecord/test/models/person.rb
index d2a0c6b40c..84bc901b5e 100644
--- a/activerecord/test/models/person.rb
+++ b/activerecord/test/models/person.rb
@@ -1,8 +1,10 @@
 class Person < ActiveRecord::Base
   has_many :readers
+  has_many :secure_readers
   has_one  :reader
 
   has_many :posts, :through => :readers
+  has_many :secure_posts, :through => :secure_readers
   has_many :posts_with_no_comments, :through => :readers, :source => :post, :include => :comments,
                                     :conditions => 'comments.id is null', :references => :comments
 
diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb
index 1cab78d8c7..0fc22ac6a3 100644
--- a/activerecord/test/models/post.rb
+++ b/activerecord/test/models/post.rb
@@ -115,8 +115,10 @@ class Post < ActiveRecord::Base
   has_many :named_categories, :through => :standard_categorizations
 
   has_many :readers
+  has_many :secure_readers
   has_many :readers_with_person, :include => :person, :class_name => "Reader"
   has_many :people, :through => :readers
+  has_many :secure_people, :through => :secure_readers
   has_many :single_people, :through => :readers
   has_many :people_with_callbacks, :source=>:person, :through => :readers,
               :before_add    => lambda {|owner, reader| log(:added,   :before, reader.first_name) },
diff --git a/activerecord/test/models/reader.rb b/activerecord/test/models/reader.rb
index 0207a2bd92..59005ac604 100644
--- a/activerecord/test/models/reader.rb
+++ b/activerecord/test/models/reader.rb
@@ -3,3 +3,12 @@ class Reader < ActiveRecord::Base
   belongs_to :person, :inverse_of => :readers
   belongs_to :single_person, :class_name => 'Person', :foreign_key => :person_id, :inverse_of => :reader
 end
+
+class SecureReader < ActiveRecord::Base
+  self.table_name = "readers"
+
+  belongs_to :secure_post, :class_name => "Post", :foreign_key => "post_id"
+  belongs_to :secure_person, :inverse_of => :secure_readers, :class_name => "Person", :foreign_key => "person_id"
+
+  attr_accessible nil
+end