Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Don't use mass-assignment protection when applying the scoped.scope_f…

…or_create. Fixes #481.
  • Loading branch information...
commit 9a7dbe2c0570e11b9033df735c937d5f5416e0ca 1 parent 8f999a3
@jonleighton jonleighton authored
View
6 activerecord/lib/active_record/associations/collection_association.rb
@@ -99,7 +99,6 @@ def build(attributes = {}, options = {}, &block)
else
add_to_target(build_record(attributes, options)) do |record|
yield(record) if block_given?
- set_owner_attributes(record)
end
end
end
@@ -423,7 +422,10 @@ def insert_record(record, validate = true)
end
def build_record(attributes, options)
- reflection.build_association(scoped.scope_for_create.merge(attributes || {}), options)
+ record = reflection.build_association
@pixeltrix Owner

I've some STI models that override new to return a subclass based upon a key in the attributes hash - any chance we can pass the attributes to build_association or is that going to run into problems with attribute protection?

@jonleighton Collaborator

@pixeltrix I am not sure about this.

We need to assign scoped.scope_for_create without protection, so it cannot be through new.

And we need attributes to be assigned, with protection, after scoped.scope_for_create so that the attributes can override any defaults if necessary.

So the only way I can think to support what you're saying is to assign attributes once via new and then again after the scoped.scope_for_create has been applied. This seems sub-optimal and could lead to bugs where the user is not expecting something to be assigned twice.

So my suggestion would be that you create a different class method which does the subclass disambiguation before passing the attributes on to new/build/etc.

@josevalim, your thoughts?

@josevalim Owner

You can pass :without_protection => true to new. It should work fine.

@jonleighton Collaborator

Ok, but @pixeltrix said that his code works based on the attributes hash, not based on scoped.scoped_for_create... @pixeltrix: c/d?

@josevalim Owner

Oh, touchez. There is nothing we can do then.

@pixeltrix Owner

This is basically what I do at the moment:

class Menu < ActiveRecord::Base
  has_many :links, :dependent => :destroy
  accept_nested_attributes_for :links
end

class Link < ActiveRecord::Base
  include Rails.application.routes.url_helpers

  class << self
    def new(attributes = {}, &block)
      link_type = attributes.delete(:link_type)
      return super if link_type.blank?

      begin
        link_class(link_type).create_with(scoped.scope_for_create).new(attributes, &block)
      rescue NameError
        raise RuntimeError, "Unknown link type #{link_type}"
      end
    end

    def link_class(link)
      "#{link.to_s.camelize}Link".constantize
    end
  end
end

class IndexLink < Link
  def url(options = {})
    root_path(options)
  end
end

page.links.build(:link_type => :index) 

The only workaround I've found so far is to override the build method in the association definition, e.g:

class Menu < ActiveRecord::Base
  has_many :links do
    def build(attributes = {}, options = {}, &block)
      # do stuff
    end
  end
end

This works but is kinda sucky - what about adding support for specifying :type in the attributes hash and the association code using the appropriate subclass?

@jonleighton Collaborator

what about adding support for specifying :type in the attributes hash

That would open up the possibility for user input to specify what class an object is (without the author necessarily have intended that to be the case). :type => 'AdminUser', anyone?

I think the only proper way to support this would be to add to the API, and I am not sure that we should...

@pixeltrix Owner

Well I was thinking of limiting to subclasses of the association class. :-)

It would be disabled by default - you'd have to use attr_accessible to enable it and it would also honour the :as option.

@pixeltrix Owner

@jonleighton what about this way of passing attributes to new?
pixeltrix@c6854ac

@jonleighton Collaborator

@pixeltrix that looks good to me, though I would prefer to call stringify_keys on attributes than convert the scope to a HashWithIndifferentAccess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ record.assign_attributes(scoped.scope_for_create, :without_protection => true)
@jonleighton Collaborator

@josevalim: could you please sanity-check this for me? I'm pretty sure it's okay to do this without protection (as 'scoped' shouldn't contain user input), but I would appreciate another pair of eyes on this so it doesn't come back to bite me later ;)

@jonleighton Collaborator

The main difference between this and the previous version is that this will incorporate conditions which are defined on the association (and they won't be protected when assigned).

So one question is: should those conditions defined on the association be assigned with protection or not? I would say not (as it isn't user input), but let me hear your thoughts. Once we decide I should add a test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ record.assign_attributes(attributes || {}, options)
@josevalim Owner

It looks fine and I agree scope conditions should be without protection. You just don't need (attributes || {}) here, assign_attributes will automatically abort if attributes is nil. :)

@jonleighton Collaborator

Good point, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ record
end
def delete_or_destroy(records, method)
View
4 activerecord/lib/active_record/associations/through_association.rb
@@ -14,7 +14,9 @@ module ThroughAssociation #:nodoc:
def target_scope
scope = super
@josevalim Owner

Isn't this something we would like to cache or it is cached on super?

@jonleighton Collaborator

We don't want to cache it because the result of super could be different depending on whether or not we are within a scoping { ... } block.

We cache association_scope and then merge it into target_scope when scoped is called.

@josevalim Owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
chain[1..-1].each do |reflection|
- scope = scope.merge(reflection.klass.scoped)
+ # Discard the create with value, as we don't want that the affect the objects we
+ # create on the association
+ scope = scope.merge(reflection.klass.scoped.create_with(nil))
end
scope
end
View
9 activerecord/test/cases/associations/has_many_associations_test.rb
@@ -86,11 +86,20 @@ def test_create_from_association_set_owner_attributes_by_passing_protection
bulb = car.bulbs.new
assert_equal car.id, bulb.car_id
+ bulb = car.bulbs.new :car_id => car.id + 1
+ assert_equal car.id, bulb.car_id
+
bulb = car.bulbs.build
assert_equal car.id, bulb.car_id
+ bulb = car.bulbs.build :car_id => car.id + 1
+ assert_equal car.id, bulb.car_id
+
bulb = car.bulbs.create
assert_equal car.id, bulb.car_id
+
+ bulb = car.bulbs.create :car_id => car.id + 1
+ assert_equal car.id, bulb.car_id
ensure
Bulb.attr_protected :id
end

1 comment on commit 9a7dbe2

@josevalim
Owner

Sweeeeeeet!

@jonleighton

@josevalim: could you please sanity-check this for me? I'm pretty sure it's okay to do this without protection (as 'scoped' shouldn't contain user input), but I would appreciate another pair of eyes on this so it doesn't come back to bite me later ;)

@jonleighton

The main difference between this and the previous version is that this will incorporate conditions which are defined on the association (and they won't be protected when assigned).

So one question is: should those conditions defined on the association be assigned with protection or not? I would say not (as it isn't user input), but let me hear your thoughts. Once we decide I should add a test case.

@josevalim

Isn't this something we would like to cache or it is cached on super?

@josevalim

It looks fine and I agree scope conditions should be without protection. You just don't need (attributes || {}) here, assign_attributes will automatically abort if attributes is nil. :)

@jonleighton

We don't want to cache it because the result of super could be different depending on whether or not we are within a scoping { ... } block.

We cache association_scope and then merge it into target_scope when scoped is called.

@pixeltrix

I've some STI models that override new to return a subclass based upon a key in the attributes hash - any chance we can pass the attributes to build_association or is that going to run into problems with attribute protection?

@jonleighton

@pixeltrix I am not sure about this.

We need to assign scoped.scope_for_create without protection, so it cannot be through new.

And we need attributes to be assigned, with protection, after scoped.scope_for_create so that the attributes can override any defaults if necessary.

So the only way I can think to support what you're saying is to assign attributes once via new and then again after the scoped.scope_for_create has been applied. This seems sub-optimal and could lead to bugs where the user is not expecting something to be assigned twice.

So my suggestion would be that you create a different class method which does the subclass disambiguation before passing the attributes on to new/build/etc.

@josevalim, your thoughts?

@josevalim

You can pass :without_protection => true to new. It should work fine.

@jonleighton

Ok, but @pixeltrix said that his code works based on the attributes hash, not based on scoped.scoped_for_create... @pixeltrix: c/d?

@josevalim

Oh, touchez. There is nothing we can do then.

@pixeltrix

This is basically what I do at the moment:

class Menu < ActiveRecord::Base
  has_many :links, :dependent => :destroy
  accept_nested_attributes_for :links
end

class Link < ActiveRecord::Base
  include Rails.application.routes.url_helpers

  class << self
    def new(attributes = {}, &block)
      link_type = attributes.delete(:link_type)
      return super if link_type.blank?

      begin
        link_class(link_type).create_with(scoped.scope_for_create).new(attributes, &block)
      rescue NameError
        raise RuntimeError, "Unknown link type #{link_type}"
      end
    end

    def link_class(link)
      "#{link.to_s.camelize}Link".constantize
    end
  end
end

class IndexLink < Link
  def url(options = {})
    root_path(options)
  end
end

page.links.build(:link_type => :index) 

The only workaround I've found so far is to override the build method in the association definition, e.g:

class Menu < ActiveRecord::Base
  has_many :links do
    def build(attributes = {}, options = {}, &block)
      # do stuff
    end
  end
end

This works but is kinda sucky - what about adding support for specifying :type in the attributes hash and the association code using the appropriate subclass?

@jonleighton

what about adding support for specifying :type in the attributes hash

That would open up the possibility for user input to specify what class an object is (without the author necessarily have intended that to be the case). :type => 'AdminUser', anyone?

I think the only proper way to support this would be to add to the API, and I am not sure that we should...

@pixeltrix

Well I was thinking of limiting to subclasses of the association class. :-)

It would be disabled by default - you'd have to use attr_accessible to enable it and it would also honour the :as option.

@jonleighton

@pixeltrix that looks good to me, though I would prefer to call stringify_keys on attributes than convert the scope to a HashWithIndifferentAccess.

Please sign in to comment.
Something went wrong with that request. Please try again.