SmartLogic Logo (443) 451-3001

The SmartLogic Blog

SmartLogic is a web and mobile product development studio based in Baltimore. Contact us for help building your product or visit our website to learn more about what we do.

Rails 2.3 Nested Object Forms: I’m not Crazy about Them

February 24th, 2009 by

I’m a couple of weeks late, but I just finished reviewing Rails 2.3 Nested Object Forms. While a very nice and “magical” feature, I’ve got to admit that I’m really not that crazy about how it works.

Let me be the first to admit that there’s no one right way to do things. In fact, I’m writing this post particularly because I have a few objections to how this functionality is ultimately exposed, and I’d like to hear arguments from those who disagree. In other words, let me acknowledge the possibility that I am the misguided one.

Review of Nested Object Forms

As a quick review, we can now prepare our models for nested object mass assignment:

class Person < ActiveRecord::Base

  validates_presence_of :name

  has_many :children
  accepts_nested_attributes_for :children
    # can also be used on has_one etc.. associations

end

class Child < ActiveRecord::Base
  belongs_to :person
end

We specify which nested relationships can be passed in via mass assignment using the accepts_nested_attributes_for() function. Continuing the example, we can now execute the following to actually save a person and two children at the same time:

person = Person.create!(:name => 'George', :children_attributes => {"new_1" => {:name => 'Amy'}, "new_2" => {:name => 'Stephanie'}})

For full disclosure, my console session from start to finish:

john-mbp:playground john$ rails -v
Rails 2.3.0
john-mbp:playground john$ rails anewapp
...
...
john-mbp:playground john$ cd anewapp/
john-mbp:anewapp john$ ruby script/generate model person name:string
...
john-mbp:anewapp john$ ruby script/generate model child name:string person_id:integer
...

# edit app/models/person.rb as above
# edit app/models.child.rb as above

john-mbp:anewapp john$ rake db:migrate
(in /Users/john/playground/anewapp)
==  CreatePeople: migrating ===================================================
-- create_table(:people)
   -> 0.0044s
==  CreatePeople: migrated (0.0046s) ==========================================

==  CreateChildren: migrating =================================================
-- create_table(:children)
   -> 0.0041s
==  CreateChildren: migrated (0.0045s) ========================================

john-mbp:anewapp john$ ruby script/console 
Loading development environment (Rails 2.3.0)
>> person = Person.create!(:name => 'George', :children_attributes => {"new_1" => {:name => 'Amy'}, "new_2" => {:name => 'Stephanie'}})
=> #<Person id: 1, name: "George", created_at: "2009-02-24 00:49:21", updated_at: "2009-02-24 00:49:21">
>> person.children
=> [#<Child id: 1, name: "Amy", person_id: 1, created_at: "2009-02-24 00:49:21", updated_at: "2009-02-24 00:49:21">, #<Child id: 2, name: "Stephanie", person_id: 1, created_at: "2009-02-24 00:49:21", updated_at: "2009-02-24 00:49:21">]
>> person.children.size
=> 2

Pretty cool, huh? I agree. I think this is fantastic, and with the integration with fields_for, it will really ease a lot of the pain that previously existed with multi-model forms.

This said, I’ve got a few major bones to pick with how it’s implemented.

So What’s Wrong?

The primary issue is the fact that this functionality is statically defined (in that you can’t turn it on/off in specific scenarios). Why is this an issue? Well, this is a potentially dangerous functionality to expose everywhere. Let’s say that you wanted to take advantage of this new feature to allow administrators to create new accounts that came pre-loaded with a complementary $X credit to their accounts. So you built a form for admins to fill out that included a fields_for :credits to achieve this single-form implementation of an account with credits. Now, in order to avail of this, I need to turn on the nested mass assignment.

class Person < ActiveRecord::Base
  has_many :credits
  accepts_nested_attributes_for :credits
end

class Credit < ActiveRecord::Base
  belongs_to :person
end

Wait….I’ve just turned this feature on globally…..I can’t just make it available at the controller level? But isn’t the controller where I define authorization, who can edit/update/create what? Precisely.

So….unless I want to go old school and rewrite this all by hand, anytime I want to render a person form (e.g. on an “Edit My Profile” link for authenticated users), I need to recall that I’ve exposed myself to a possible POST data hack (anyone who understands this feature in rails will know how to handcraft the POST data). This is a fairly ridiculous thing to expect on a project of any nontrivial size.

However, there’s another way around this issue. Rather than rewriting all of the multi-model form stuff by hand, I could simply ward off the extra parameters at the controller level, e.g.

class ProfileController < ApplicationController
  before_filter :remove_nested_params

  def update
    @user = User.find(current_user.id)
    @user.update_attributes!(params[:user])
    redirect_to profile_path
  rescue
    render :action => 'edit'
  end

  protected
    def remove_nested_params
      params[:user].delete(:credits)
    end
end

This works fine in isolation or for a one-off, but again, this is a piece of information that is going to be lost as the project trudges along. Your team will forget this, and someone is going to forget to “close the hole” when adding that new “quick edit profile” form that only shows their email address and display name.

Counter Suggestions

I would advocate any solution where this behavior was pushed out to controllers. That said, I don’t have an airtight procedure planned out. I will, however, leave you with a few suggestions.

We could implement it around_filter style where the feature is enabled/disabled in an around_filter. The primary benefit of this approach would be that the majority of the code could probably stay put in ActiveRecord. We’d just need to expose hooks to turn it on/off. Then we could define a controller-level meta-function (e.g. filter_parameter_logging) that could set which actions to apply the behavior on, or which models can be nested. The syntax could look like:

class ProfileController < ApplicationController
  allow_nested_assignment_for User, [:children], :only => [:update]
end

I’ll be perfectly honest in that I don’t love this suggestion. It incorrectly scopes this authorization tweak to the whole action and any before/after filters that execute in between it (still leaving holes, albeit smaller) as opposed to scoping it directly to the single save! action that we really want to special-case. If we perhaps exposed a separate save_with_nested() function, then we could isolate the scope down to the specific call. Needless to say, I doubt this is a course of action rails would take, and it’s not one I would advocate.

Another alternative would be to keep the functionality as is, but then allow you to override or extend it at the controller level (a la my initial suggestion). This would allow developers to avail of the benefits in the less-risky areas, and then conditionally avail of them in more risky areas by simply turning it on for specific controller/action combos. In this case, we’d define a basic harmless nested assignment baseline in the model, and the more risky nested assignment in those controllers where we wanted to isolate it, e.g.

class Person < ActiveRecord::Base
  has_many :children
  has_many :credits
  accepts_nested_attributes_for :children
end

class Admin::PeopleController < ActiveRecord::Base
  allow_nested_assignment_for User, :credits, :only => [:create, :update]
end

The primary benefit of an approach like this is that it aligns properly to a developer’s train of thought. When you go to add a trivial feature, you don’t think “I need to close that gap my unrelated nontrivial feature added for me.” However, when you go to add a nontrival feature (like allowing an admin to auto-create credits in their new account form), you do think “what do I need to differently?” In the former case, the developer thinks his new feature is trivial, and doesn’t expect to have do anything exceptional for it. In the latter case, the developer thinks his new feature is nontrivial, and will think about it from a “what extra work does that mean for me?” standpoint.

Closing Thoughts

One of the biggest things that worries me is that no one is talking about this. The whole community requested this feature, and at this point it has received quite a bit of fanfare. But there are some clear ways to accidentally screw yourself that just aren’t being mentioned as caveats.

To be entirely blunt, I just feel like this is plugin material at this point. Though I commend rails for opening up its feature set to a community vote, I fear that we may have misstepped with this feature in particular. I’m very curious to hear what others think. (Be gentle with the flame)

  • http://ablegray.com Mike Nicholaides

    While I disagree about mass attribute assignment being out of place in active record, you make a very good point about it being easy to accidentally opening yourself up to unexpected attack.

    As I recall, there are a number of plugins that help with mutlti-model forms. Perhaps the Rails core team should do what they did with pagination and force you to use a plugin (or roll your own).

  • ste

    “ActiveRecord was primed specially to accept and parse data coming from HTTP POST data”

    AR’s mass assignments works by accepting a plain and simple Ruby Hash (or a slightly-less-plain-and-simple HashWithIndifferentAccess). This hash might come from an HTTP POST (after being parsed by AC), or some other source (e.g. a CVS file, another record’s attributes etc. etc.), or it could be hardcoded like in your example, but AR is agnostic about it.
    You have a point with regard to the danger of accidentally exposing potentially dangerous mass assignments: being able to turn this feature on and off in the controller would be very nice.

  • http://www.smartlogicsolutions.com/wiki/John_Trupiano John Trupiano

    Mike, ste, thanks for the notes. Yes, I suppose I was a little off in describing mass assignment as out of place in ActiveRecord. I believe early on it was added specifically to cater to ActionPack, but as ste pointed out, it is clearly a generic interface at this point.

    As such, I’ve stricken that paragraph or two from this post. My primary points stand on their own, and I don’t want other viewers/commenters to get tripped up on that not-so-relevant detail.

  • Manfred Stienstra

    # Wait…. I’ve just turned this feature on globally…

    You can use attr_accessible and attr_protected to protect your attributes (even the nested accessors) from mass assignment.

    # Your team will forget this, and someone is going to forget to “close the hole” when adding that new “quick edit profile” form that only shows their email address and display name.

    No, because your team members write tests to make sure that non-admins can’t change protected attributes.

    # One of the biggest things that worries me is that no one is talking about this.

    We’ve discussed the feature quite extensively:

    http://groups.google.com/group/rubyonrails-core/browse_thread/thread/3c61e00916c365e5/40d7df5d5d04ba7f
    http://rails.lighthouseapp.com/projects/8994/tickets/1202-add-attributes-writer-method-for-an-association

    After the public discussion we’ve discussed the code even further with a small group of active contributers to the patch, including a few core members.

    If you think certain things need to be implemented differently, we’re happy to review your patches.

  • http://mina.naguib.ca/ Mina Naguib

    I’ve never felt comfortable with how rails does mass assignment. I believe that the problem you’re describing with nested assignment is the flip side of the same coin.

    The problem I see is that authorization is very frequently more granular than “an entire record”. By that, I mean if you have an instance of a User there’s a good chance the user who owns that record can update most, but very often not all, of the attributes in that record.

    For trivial models, the user’s attributes are name, email address, age etc. You trust the user to edit their own data appropriately, and mass assignment does the rest.

    However, as soon as there’s any business rules that apply to attributes instead of entire records, the AR attr_protected/accessible implementation breaks badly. Examples of that are, for example, a “customer_loyalty” score on the user. You definitely don’t want the user to update that field, but you want an admin to do it. The ActiveRecord implementation shoots you in the foot in that scenario. You’re forced to avoid mass assignment, or filter it manually, or some other verbose implementation in each action in the controller.

    The same problem extends to nested assignment as you’ve described. In essence we can treat the nested entities as yet another attribute, and whenever we want authorization to influence its accessibility the problem rears its head.

    Exactly as you’ve mentioned, I’ve been worried that there isn’t much discussion about this problem. I feel that either rails developers are not aware of the problem (bad), ignoring it (bad), or nobody is building anything complex enough that requires attribute-granular authorization.

    I’ve come across a plugin ( http://blog.stochasticbytes.com/2008/03/paramaccessible.html ) that attempts to address this by implementing in the controllers a param_protected/param_accessible interface similar to attr_protected/attr_accessible in AR, however I don’t think it has picked up any steam.

  • http://www.smartlogicsolutions.com/wiki/John_Trupiano John Trupiano

    Manfred, using attr_accessible and attr_protected in conjunction is a great tip, and does go a ways toward assuaging my concerns.

    The last bit about “no one is talking about it”…..I was more concerned about where/how the feature was announced. Most people are going to find Eloy’s and Ryan Bates’ tutorial (his is the one I linked to at the top), and these don’t appear to cover any of these security concerns (and your valid workarounds). Obviously, the feature has been well-discussed by the contributors, but my concerns are not discussed in the places most likely to used (by application developers) as introductions to the feature.

    Lastly, it was not my intent to condemn this feature by any means. I really just wanted to start a discussion about it.

  • http://mina.naguib.ca/ Mina Naguib

    @Manfred

    See my reply just under yours.

    I’d be happy to be enlightened regarding my gripes with the granularity of AR’s attr_accessible/protected

  • Manfred Stienstra

    # However, as soon as there’s any business rules that apply to attributes instead of entire records, the AR attr_protected/accessible implementation breaks badly.

    Yes, it just addresses the simple cases.

    # Exactly as you’ve mentioned, I’ve been worried that there isn’t much discussion about this problem. I feel that either rails developers are not aware of the problem (bad), ignoring it (bad), or nobody is building anything complex enough that requires attribute-granular authorization.

    Well, the code for these solutions tends to get really application specific so there is not much sense in publishing about it. Also, not everyone is in the spirit of sharing.

    We usually just protect attributes that should not be written by visitors of the site, after that we allow authenticated users to write to certain attributes by slicing the params hash in combination with a very simple plugin:

    http://github.com/Fingertips/attribute-permissions/tree/master

    # Most people are going to find Eloy’s and Ryan Bates’ tutorial (his is the one I linked to at the top), and these don’t appear to cover any of these security concerns

    Right, that’s because they’re meant to be introductory articles for the new feature. The security issues aren’t very different from the ones you always face on the web so my guess is that most same programmers will know they should protect their attributes. The rest is doomed anyway.

    If you think people need this kind of warning a good write-up would be great.

  • http://www.smartlogicsolutions.com/wiki/John_Trupiano John Trupiano

    Mina, Manfred, thanks for the references and discussion. Both fingertips and paramaccessible look to be potentially useful libraries for me.

    Out of curiosity, where would this write-up fit into the existing raisl documentation strategy? Is there an existing rails guide that this would be particularly suited for?

  • Eloy Duran

    I think t would fit with http://guides.rubyonrails.org/security.html#_mass_assignment.

    As you can see it already contains some documentation about this subject, but neglects to mention that the same holds true for any setter methods, _not_ for only database columns.

    See the docrails project for a way to contribute: http://github.com/lifo/docrails/tree

  • Eloy Duran

    Ouch, the text formatting got screwed a bit… :)

  • http://www.smartlogicsolutions.com/wiki/John_Trupiano John Trupiano

    Thanks for the pointers Eloy. Section 6 of the security guide definitely looks like it could be beefed up a bit…..I’ll follow up in a separate forum (is the google group the best place? IRC?) regarding actually contributing detail back to the guide(s).

    If there are further thoughts on merits/demerits of nested mass assignment, I’d love to continue the discussion.

  • Eloy Duran

    I think that if you want to have input from anyone, the group would be indeed best. There’s also #docrails on the Freenode IRC network.

  • Pingback: Double Shot #400 « A Fresh Cup

  • http://josevalim.blogspot.com/ José Valim

    You’ve exposed some breaches in the current nested forms but I would propose a slightly different solution. accepts_nested_attributes_for deals with :reject_if option (at least the last time I checked), so you could use it to specify when it’s acceptable or not.

    Then you could create a save_with_nested method that would set a flag which would make the :reject_if proc (or method) fail and your security problems would be gone with less than 10 lines of code. :)

    This sounds better than exposing this functionality as a class method in the controller (imho, this is not a controller concern).

  • Ryan Sobol

    @John – Thanks for bringing this security concern to the forefront of the developer community.

    +1 for a good write-up in the rails guides

  • http://www.smartlogicsolutions.com/wiki/John_Trupiano John Trupiano

    @Jose, that’s an interesting take– I suppose I’m not a fan of it from a “style” point of view….writing special save() methods for each set of restricted permissions will get cumbersome….then again, any fine-grained access control is going to get nasty, so maybe mine is not really a valid point.

    It’s clear at this point that we do have suitable workarounds. My two primary concerns at this point are: (1) can we make this safer out of the box? (2) regardless of (1), let’s make sure we’re making this information available.

    I think Mina captured the shortcomings very well in his responses farther up. A/R’s attr_accessible and attr_protected are fantastic for the simple cases, but don’t scale very well when you want finer access control.

    A colleague of mine has been working on an AASM fork (http://github.com/mikowitz/aasm) that includes (among other things) the ability to restrict a transition to a specific set of “roles.” While still early in development, I wonder if some of the ideas from that project might apply here….seems to share several similarities to this discussion, particularly as it relates to fine-grained access control.

    @Ryan I’m hoping to submit a doc patch this weekend for that guide.

  • http://www.smartlogicsolutions.com/wiki/John_Trupiano John Trupiano

    As a followup, I’ve beefed up the mass assignment section of the Securing Rails Applications guide: http://guides.rubyonrails.org/security.html#_mass_assignment

    Mina, it’s possible you may have more to contribute, so check it out if you have an opportunity.

  • http://zenadmin.org Gaspard Bucher

    All these scenarios and awful risks can be easily avoided if we accept that the security level should live in the model, not in the controller.

    This simple assertion means that models know who is doing what to them when they are saved.

    For example, you would have a validation that verifies that only admins can change the “credit”:

    validate :only_admins_can_change_credit

    def only_admins_can_change_credit
    errors.add(‘credit’, ‘cannot be changed’) if credit_changed? && !visitor.is_admin?
    end

  • http://mina.naguib.ca/ Mina Naguib

    @John
    Will take a look

    @Gaspard
    I agree regarding doing the authorization in the model, though not entirely with the approach you suggest.

    I’d feel more comfortable if the setter method is never called in the first place, as opposed to having it called and then try to do authorization later when .valid? is invoked – this is in case the setter has any side effects (hand-rolled setters, or nested mass assignment helpers for instance).

    If we agree on the above, and if my understanding is correct, all mass-assignment methods (new/update_attributes) end up delegating to #attributes= at one point. Therefore, this is the place to inject the authorization logic to not invoke setters the visitor doesn’t have access to invoke.

    Regarding your exact suggested approach though, which already does a decent job attacking the problem: do you hand-roll the notion of “visitor” in your example in every rails app ? or are you aware of plugins to do this ? Also, how do you communicate the visitor from the controller to the model ?

  • http://www.smartlogicsolutions.com/wiki/John_Trupiano John Trupiano

    @Gaspard, @Mina,

    I’ve long held the belief that models should be entirely session-independent. In other words, they should not know who is logged in, or really that they (the models) are being consumed by a rails application. I wrote my code such that controllers would orchestrate the permissions/authorization, and that the models were malleable pieces, unaware of roles/permissions/etc.

    That said, the more I think about this, the more I’m convinced that there isn’t a golden rule for all of these cases. Still, in the majority of my apps, authorization is best (and more appropriately I’d argue) handled at the controller level. However, in extreme cases where ACL’s and the likes need to be implemented, there’s no way around pushing authorization down to the model level.

    As I noted earlier, perhaps we can take a few ideas from how roles/authorization are handled in my Michael Berkowitz’s fork of the AASM gem: http://github.com/mikowitz/aasm

    Though his implementation only deals with state transitions, it’s easy to consider this idea being applied to the concept of editing a single field.

    @Gaspard, I’m also curious as to your approach for passing the “current logged in user or access level” to the model….this always felt very dirty to me (hence why I always advocated applying authorization at the controller level).

    Also @Mina, I started a discussion on the core group about perhaps tweaking rails to protect against mass assignment by default. Feel free to chime in there as well: http://groups.google.com/group/rubyonrails-core/browse_thread/thread/4e312a3de4c3579f

  • http://zenadmin.org Gaspard Bucher

    @John, you are right, there is no golden rule.

    In zena, access to users is controlled with a before filter in the controller since it’s a very simple grant/refuse authorization scheme.

    On the contrary, Node access (pages,notes,contacts,etc) is done with groups (like in UNIX). This means that when we follow links between nodes (children, other relations), we need to move with the visitor along and the authorization is then moved into the model.

    I did not decide to go off the tracks and have an explicit visitor available in models just for fun. It makes testing harder and everyone says it’s wrong. Apart from all these negative aspects, it’s a very elegant solution when you have complexe workflows (publication/proposition/redaction) in a multiversion, group based authorisation scheme.

    Another advantage is that you can write very clean authorization rules (http://tinyurl.com/bfvpks) that produce nice error messages in the forms (ajax) where the user tries to post forbidden data.

    You could move such rules in the controller, but then it would need to know so much of the node’s internals that it makes things worse.

    As a rule of thumb, I would say that we need to move authorization to the part that knows the most about the necessary context. Sometimes it might be the controller because the session is the important part. Sometimes it should be the model because that’s where the complexity lies.

  • http://zenadmin.org Gaspard Bucher

    It seems the url did not show correctly in the previous post: http://tinyurl.com/bfvpks

  • http://www.smartlogicsolutions.com/wiki/John_Trupiano John Trupiano

    @Gaspard, I think you’ve summed it up very nicely. Because of the complexity of the domain of authorization, we don’t have it built into rails (which is likely a good thing). I’d think we’d have a tough time building a generic authorization layer that actually provided any utility.

    That said, the question turns back to whether or not we think that this mass assignment vulnerability is worth addressing. Out of curiosity, what did you think of my suggestion on the rails-core list? Specifically turning off mass assignment by default, and requiring developers to open it up with attr_accessible declarations (I also suggested that the generators could be updated to include this attr_accessible declaration, bringing more visibility to the vulernability). Do you think there’s value in this, and if not, why?

  • http://zenadmin.org Gaspard Bucher

    I really like the idea to force the use of attr_accessible (maybe with something like routable_attributes http://tinyurl.com/b6t3zv in case you use dynamic attributes that you could not list).

    In the case of nested_attributes, it’s very good to explicitly allow them so that you can allow setting the preferences of a user through user but disallow the reverse.

    class User
    attr_accessible :preferences_attributes
    end

    class Preference
    # nested disallowed from here
    end

John Trupiano co-founded SmartLogic with Yair Flicker in May 2005 and was co-president through 2011. Check out his GitHub Projects or follow @jtrupiano on Twitter.

John Trupiano's posts