I’ve chimed in previously regarding my preference to see the equivalent of attr_accessible be the default for AR::Base…in other words, you have to explicitly define those attributes that you want to expose to mass assignment.
As such, by placing this logic in the model, I think you’re polluting the model in such a way that the model now only has relevance to the business rules of your specific application. The model is no longer a generic object abstraction of a database table but now also encapsulates business rules.
I firmly believe this logic should be applied at the controller level. In fact, what I think we’re really looking at here is the other side of the MVPC layer (where P = presenter). Basically, we’re looking for the inverse of a presenter, something that sits between the controller layer and the model lay to properly apply the business rules for this application.
Really, I do not want my models to know anything about session or authorization (unless I’m implementing an ACL, in which case authorization is stored in the database).
Unless I'm mistaken, I think that both of the methods discussed in
this thread satisfy your requirements? The only approach that tries to
define the filter completely in the model is attr_protected/
attr_accessible, which seems to fall short for just the reason you
name -- the logic is placed in the wrong layer.
The problem with Hash#slice/except is that it does not make security
the default. If one forgets to do this in one request then it leaves a
security hole which is difficult to see because the app behaves
normally.
With attr_accessible security is the default and one must explicitly
bypass it - I believe that is the way it should be. The problem is
there is no easy way to do that. This discourages the use of
attr_accessible - not a good thing at all.
Rails 3 is already moving into this idea of default security with HTML
injection. One must explicitly specify if they don't want the HTML to
be escaped. I think we can take the same concept and apply it here.
What if params was a special subclass of Hash and had some security
built into it. By default it would assume all content is dangerous but
one can specify that directly.
User.new(params[:user].trusted)
Or like this:
params[:user].trusted! if admin?
User.new(params[:user])
The method could take a list of arguments defining which attributes
are trusted to handle more complex scenarios:
params[:user].trusted!(:username, :email) if moderator?
The mass assignment can look for this type of hash and bypass
attribute protection on trusted values. I could see this concept of
params security being applied to other methods too (such as AR finds).
Also, I want to point out a bigger issue here is that attr_accessible
is WAY underused leaving so many apps vulnerable when they don't even
know it. Anything we can do to make it more friendly is a good thing.
See the section titled "The Reality" on this excellent writup:
http://railspikes.com/2008/9/22/is-your-rails-application-safe-from-mass-assignment
I agree with Ryan that model security should be pessimistic by
default. Doing a bit of extra work to use the elegant mass assignment
syntax in exchange for security is a good compromise.
I think of mass_assignment more in a context-driven way.
I have in one of my apps a user model which has a lot of attributes.
What is allowed to be edited/assigned is dependent on the context I'm
approaching the model, is it from the frontend, is it from the
backoffice by a support user, is it from the backoffice from an admin
user, is it from the API, ...
For this I think it's best to work with pre-defined contexts in your
user model. For example:
And in your controller you can say
user.update_attributes(params[:user ], :context => :support)
I've implemented this as a rails plugin for you guys to give it a
spin, see what you think. The code isn't what I want it to be but it's
quite hard overriding create / new / attributes= / ... for these kind
of things.
Assumptions:
- attr_accessible/attr_protected aren't used because coders often
forget about those macros, and also because they are annoying to work
with. The current API doesn't encourage them.
- attr_protected is dangerous (rails-spikes.com)
- Protection should be at class level
- Controllers should decide which attributes should be passed
- The API should be pessimistic by default
- A warning isn't enough, since security issues are involved, the
communication's semantic should be stronger (ie an exception)
- Ease the assignment of non-accessible attributes
Design decisions:
- attr_protected remotion
- All the attributes are unaccessible by default, this encourage the
usage of attr_accessible
- Add the attr_accessible :all facility
- Add a global flag (ActiveRecord::Base.protect_from_mass_assigment,
true by default) for backward compatibility and as context facility.
As instance, when you hack with IRb or the application console,
probably you don't want to be annoyed by security concerns.
- Raise an exception when try to mass assign non-accessible
attributes. This should make the code more robust, and provide a
stronger pointer to the problem.
- Introduction of ActiveRecord::Base.assign_attributes as non-
accessible attributes assignment facility.
Example:
before:
def create
@comment = @commentable.comments.build(params[:reply])
@comment.user = current_user
@comment.request = request
It shouldn't be hard to migrate legacy code:
- Existing code with attr_protected, should be changed, with the
opposite semantic.
- Existing code with attr_accessible, is already OK.
- Existing code without any protection, should introduce
attr_accessible in each class.
- Existing code with a large codebase or more complex examples can
turn off the protection in first instance, then re-enable when the
migration is done.
- Optional: coders can use #assign_attributes, as replacement of
manual assignment (@comment.user = current_user)
Excellent! I agree with the majority of what you propose. The only
thing is I don't think assign_attributes is an adequate solution. Many
times what one needs to assign is in the params hash, and this needs
to happen conditionally. I suppose one can use slice/except like
this...
def create
@comment = @commentable.comments.build(params[:reply].except
(:spam, :important, :troll))
@comment.assign_attributes(params[:reply].slice
(:spam, :important, :troll)) if admin?
end
This has some duplication and seems a bit more convoluted than it
needs to be. I prefer my original proposal of defining if the params
hash is trusted.
def create
params[:reply].trusted! if admin?
@comment = @commentable.comments.build(params[:reply])
end
Much cleaner. But other than that I agree with everything else.
Thanks!
I implemented all the changes mentioned in my last email, only #assign_attributes is pending. http://bit.ly/1wSmS
Few considerations:
* I implemented #protect_from_mass_assignment at class level, instead of a top-level ActiveRecord flag, because gives a fine grained control on the protection.
* Associations attribute writers should be explicitly declared as accessible.
* #accepts_nested_attributes_for, automatically adds the attribute to the accessible list
class Member < ActiveRecord::Base
has_many :posts
accepts_nested_attributes_for :posts
end
It just doesn't seem very ruby-esque to shield the 'forbidden'
attributes with attr_accessors. Since on one form you might be allowed
to change it, yet on a different one you wont have that field
supplied.
You obviously dont want to hard code your data entry restrictions on
controller level. That violates the DRY principle. When I change the
form to allow someone to edit an extra field, I also have to 'open up'
this field in the controller.
The form fields I specify in the form are the only fields the user is
allowed to change on that particular entry point. Why dont we take
this given as leading and mould our controllers and models to this set
of allowed fields?
I am thinking about an idea very similar to the authenticity token
from protect_from_forgery. Create a hash based on all the fields in a
form and some serverside secret. Whenever the post params come in I
know which fields are posted so I can recreate this hash and compare.
That way only the fields that I initially supplied in the form are
allowed. No more and no less.
