Create a gist now

Instantly share code, notes, and snippets.

Embed
What would you like to do?
class PostsController < ActionController::Base
def create
Post.create(post_params)
end
def update
Post.find(params[:id]).update_attributes!(post_params)
end
private
def post_params
params[:post].slice(:title, :content)
end
end
@mahemoff

This comment has been minimized.

Show comment
Hide comment
@mahemoff

mahemoff Mar 5, 2012

emphasis on slice()

mahemoff commented Mar 5, 2012

emphasis on slice()

@larzconwell

This comment has been minimized.

Show comment
Hide comment
@larzconwell

larzconwell Mar 5, 2012

I really like this.

I really like this.

@Najaf

This comment has been minimized.

Show comment
Hide comment
@Najaf

Najaf Mar 5, 2012

We've been using slice as well as attr_accessible.

Najaf commented Mar 5, 2012

We've been using slice as well as attr_accessible.

@vidriloco

This comment has been minimized.

Show comment
Hide comment
@vidriloco

vidriloco Mar 5, 2012

Sweet approach!

Sweet approach!

@raggi

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi Mar 5, 2012

When in a pure ruby project I do similar things with Hash#values_at. I particularly like this for decomposing hash parameters too:

a,b,c,d = hash.values_at(:a, :b, :c, :d)

raggi commented Mar 5, 2012

When in a pure ruby project I do similar things with Hash#values_at. I particularly like this for decomposing hash parameters too:

a,b,c,d = hash.values_at(:a, :b, :c, :d)
@JakubOboza

This comment has been minimized.

Show comment
Hide comment
@JakubOboza

JakubOboza Mar 5, 2012

I'm still not sure if controller is the right place.

I'm still not sure if controller is the right place.

@Najaf

This comment has been minimized.

Show comment
Hide comment
@Najaf

Najaf Mar 5, 2012

@JakubOboza In a script you might want update_attributes to actually update all attributes. Further, in a controller your job is to handle a web request, and part of that might be filtering query parameters.

If this is more complex than the above then you may want to factor it out into a conductor or something, but whatever that is would still be called from the controller.

Najaf commented Mar 5, 2012

@JakubOboza In a script you might want update_attributes to actually update all attributes. Further, in a controller your job is to handle a web request, and part of that might be filtering query parameters.

If this is more complex than the above then you may want to factor it out into a conductor or something, but whatever that is would still be called from the controller.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 5, 2012

The great thing about post_params is that you can make it dependent on controller state as well:

def post_params
  if current_user.admin?
    params[:post].slice(:title, :content, :published)
  else
    params[:post].slice(:title, :content)
  end
end
Owner

dhh commented Mar 5, 2012

The great thing about post_params is that you can make it dependent on controller state as well:

def post_params
  if current_user.admin?
    params[:post].slice(:title, :content, :published)
  else
    params[:post].slice(:title, :content)
  end
end
@znbailey

This comment has been minimized.

Show comment
Hide comment
@znbailey

znbailey Mar 5, 2012

Doesn't this bolster support for @wycats' argument/solution [1] that this belongs mainly at the controller level?

[1] https://gist.github.com/1974187

znbailey commented Mar 5, 2012

Doesn't this bolster support for @wycats' argument/solution [1] that this belongs mainly at the controller level?

[1] https://gist.github.com/1974187

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 5, 2012

@znbailey, yes, I completely agree with @wycats that this is a controller concern. Whether it actually needs dsl sugar like attr_accessible in the controller is more up for debate, though.

Owner

dhh commented Mar 5, 2012

@znbailey, yes, I completely agree with @wycats that this is a controller concern. Whether it actually needs dsl sugar like attr_accessible in the controller is more up for debate, though.

@t27duck

This comment has been minimized.

Show comment
Hide comment

t27duck commented Mar 5, 2012

👍

@manavid

This comment has been minimized.

Show comment
Hide comment
@manavid

manavid Mar 5, 2012

How about nested attributes?

manavid commented Mar 5, 2012

How about nested attributes?

@juanm55

This comment has been minimized.

Show comment
Hide comment
@juanm55

juanm55 Mar 5, 2012

was having a little trouble realizing why those should go in the controller.... but now I'm on the same page,

and yes there should be some kind of module (sorry if that is not the word) to define several ways of updating the attributes based on roles and/or controller state..... BUT isn't that the job of some authorization mechanism? ,,, but now I have red @wycats' gist....

so now this post is only to vote up the moving of attr_accessible logic to controller.

juanm55 commented Mar 5, 2012

was having a little trouble realizing why those should go in the controller.... but now I'm on the same page,

and yes there should be some kind of module (sorry if that is not the word) to define several ways of updating the attributes based on roles and/or controller state..... BUT isn't that the job of some authorization mechanism? ,,, but now I have red @wycats' gist....

so now this post is only to vote up the moving of attr_accessible logic to controller.

@larzconwell

This comment has been minimized.

Show comment
Hide comment
@larzconwell

larzconwell Mar 5, 2012

I think we should leave attr_accessible in the model(It's been fine so far, other than lazy developers forgetting it). And instead include a 'helper' method that will sanitze all attributes in a param. Good idea?

I think we should leave attr_accessible in the model(It's been fine so far, other than lazy developers forgetting it). And instead include a 'helper' method that will sanitze all attributes in a param. Good idea?

@znbailey

This comment has been minimized.

Show comment
Hide comment
@znbailey

znbailey Mar 5, 2012

@larzconwell the problem with attr_accessible only being in the model is that if you want to stick with the update_attributes convention in most of your forms, attr_accessible ends up being the set union of all of those attributes you want updateable, even if they SHOULDN'T be updateable by some forms.

A good example of this would be a webapp that allows different roles the ability to update more/less attributes of a model depending on their privileges. Since which attributes are acceptable is often dependent on permissions AND where the request is coming from (what form), it makes much more sense to do this in the controller, which is where the rest of that sort of logic lives as well in most well-structured rails apps.

znbailey commented Mar 5, 2012

@larzconwell the problem with attr_accessible only being in the model is that if you want to stick with the update_attributes convention in most of your forms, attr_accessible ends up being the set union of all of those attributes you want updateable, even if they SHOULDN'T be updateable by some forms.

A good example of this would be a webapp that allows different roles the ability to update more/less attributes of a model depending on their privileges. Since which attributes are acceptable is often dependent on permissions AND where the request is coming from (what form), it makes much more sense to do this in the controller, which is where the rest of that sort of logic lives as well in most well-structured rails apps.

@databyte

This comment has been minimized.

Show comment
Hide comment
@databyte

databyte Mar 5, 2012

Do you really double indent your methods under private? 😞 :wink2:

databyte commented Mar 5, 2012

Do you really double indent your methods under private? 😞 :wink2:

@sobrinho

This comment has been minimized.

Show comment
Hide comment
@sobrinho

sobrinho Mar 5, 2012

@dhh what about nested forms?

sobrinho commented Mar 5, 2012

@dhh what about nested forms?

@larzconwell

This comment has been minimized.

Show comment
Hide comment
@larzconwell

larzconwell Mar 5, 2012

@znbailey Yeah that is an extremely good point, I do run into errors quite often about mass assignment, and needing to update a particular attribute at a certain time.

@databyte I indent my private methods as well (: It helps me see which methods are private better!

@znbailey Yeah that is an extremely good point, I do run into errors quite often about mass assignment, and needing to update a particular attribute at a certain time.

@databyte I indent my private methods as well (: It helps me see which methods are private better!

@sentientmonkey

This comment has been minimized.

Show comment
Hide comment
@sentientmonkey

sentientmonkey Mar 5, 2012

I thought the proper way of doing this was using presenters & delegating fields you want to change on a per-use case basis. Is this not common practice??

I thought the proper way of doing this was using presenters & delegating fields you want to change on a per-use case basis. Is this not common practice??

@alassek

This comment has been minimized.

Show comment
Hide comment
@alassek

alassek Mar 5, 2012

Am I the only one who does this?

class PostsController < ActionController::Base
  def create
    Post.create(params[:post], :as => current_role)
  end

  def update
    @post = Post.find(params[:id])
    @post.assign_attributes(params[:post], :as => current_role).save!
  end
end

The post_params approach wouldn't work well for me, because a lot of my forms have two or three levels of nesting. That would mean a lot of repetition of parameter lists.

alassek commented Mar 5, 2012

Am I the only one who does this?

class PostsController < ActionController::Base
  def create
    Post.create(params[:post], :as => current_role)
  end

  def update
    @post = Post.find(params[:id])
    @post.assign_attributes(params[:post], :as => current_role).save!
  end
end

The post_params approach wouldn't work well for me, because a lot of my forms have two or three levels of nesting. That would mean a lot of repetition of parameter lists.

@larzconwell

This comment has been minimized.

Show comment
Hide comment
@larzconwell

larzconwell Mar 5, 2012

@alassek I use that for approach when checking to use admin attribute or not. But I don't think it is that flexible.

@alassek I use that for approach when checking to use admin attribute or not. But I don't think it is that flexible.

@sheldonh

This comment has been minimized.

Show comment
Hide comment
@sheldonh

sheldonh Mar 5, 2012

For me, this tickles an itch. I have a nagging suspicion that this rule would drive deeper domain model design: never call AR methods from outside your AR objects. I'm not sure yet that it's a useful rule to adhere to religiously, but I think it's a useful thought experiment.

sheldonh commented Mar 5, 2012

For me, this tickles an itch. I have a nagging suspicion that this rule would drive deeper domain model design: never call AR methods from outside your AR objects. I'm not sure yet that it's a useful rule to adhere to religiously, but I think it's a useful thought experiment.

@petervandenabeele

This comment has been minimized.

Show comment
Hide comment
@petervandenabeele

petervandenabeele Mar 5, 2012

Another use case for this approach (of slicing in the controller) I had is that the HTML and XML interface for the same model have slightly different "access rights" to the fields. So that is also a "controller driven" slicing of the access to the fields. On the other hand, the model tests should be able to access all fields freely.

Another use case for this approach (of slicing in the controller) I had is that the HTML and XML interface for the same model have slightly different "access rights" to the fields. So that is also a "controller driven" slicing of the access to the fields. On the other hand, the model tests should be able to access all fields freely.

@gma

This comment has been minimized.

Show comment
Hide comment
@gma

gma Mar 5, 2012

I use slice too. But what's this comment about, from the Rails source?

Note that using Hash#except or Hash#slice in place of attr_accessible to sanitize attributes won't provide sufficient protection.

https://github.com/rails/rails/blob/master/activemodel/lib/active_model/mass_assignment_security.rb#L171

I can only imagine they're thinking of nested attributes (which I slice up as well, on the rare occasions that I use them).

gma commented Mar 5, 2012

I use slice too. But what's this comment about, from the Rails source?

Note that using Hash#except or Hash#slice in place of attr_accessible to sanitize attributes won't provide sufficient protection.

https://github.com/rails/rails/blob/master/activemodel/lib/active_model/mass_assignment_security.rb#L171

I can only imagine they're thinking of nested attributes (which I slice up as well, on the rare occasions that I use them).

@adnanced

This comment has been minimized.

Show comment
Hide comment
@adnanced

adnanced Mar 5, 2012

Or you could use principles from some other frameworks (which I wont mention here) and that is:

"You cannot mass assign attribute that have no validation rule assigned to it"

It is perfectly logical and works like a charm.
This way you are making whitelist for security hole instead of blacklisting it. And if you want to mass assign attribute that have no validation (rare cases) you can assign "safe" validator which is just dummy validation rule.

adnanced commented Mar 5, 2012

Or you could use principles from some other frameworks (which I wont mention here) and that is:

"You cannot mass assign attribute that have no validation rule assigned to it"

It is perfectly logical and works like a charm.
This way you are making whitelist for security hole instead of blacklisting it. And if you want to mass assign attribute that have no validation (rare cases) you can assign "safe" validator which is just dummy validation rule.

@diegorv

This comment has been minimized.

Show comment
Hide comment

diegorv commented Mar 5, 2012

attr_accessible is already available in controller isn't? http://api.rubyonrails.org/classes/ActiveModel/MassAssignmentSecurity/ClassMethods.html

@sobrinho

This comment has been minimized.

Show comment
Hide comment
@sobrinho

sobrinho Mar 5, 2012

@diegorv it only works for controller that handles a single resource.

nested resources will be a problem :)

sobrinho commented Mar 5, 2012

@diegorv it only works for controller that handles a single resource.

nested resources will be a problem :)

@wxianfeng

This comment has been minimized.

Show comment
Hide comment
@wxianfeng

wxianfeng Mar 5, 2012

no anyone use attr_protected?

no anyone use attr_protected?

@filipeamoreira

This comment has been minimized.

Show comment
Hide comment
@filipeamoreira

filipeamoreira Mar 5, 2012

I really liked this approach. Especially with the added option to do a permission based selection (see follow up comment by @dhh)

I really liked this approach. Especially with the added option to do a permission based selection (see follow up comment by @dhh)

@jasonroelofs

This comment has been minimized.

Show comment
Hide comment
@jasonroelofs

jasonroelofs Mar 5, 2012

@wxianfeng attr_protected, IMO, should be removed from ActiveRecord entirely. Blacklisting is almost always a bad idea, and with the nested form parameters stuff Rails already handles, it will hide very devious security holes. Always use attr_accessible to whitelist what people can change.

@wxianfeng attr_protected, IMO, should be removed from ActiveRecord entirely. Blacklisting is almost always a bad idea, and with the nested form parameters stuff Rails already handles, it will hide very devious security holes. Always use attr_accessible to whitelist what people can change.

@larzconwell

This comment has been minimized.

Show comment
Hide comment
@larzconwell

larzconwell Mar 5, 2012

@jameskilton Agreed attr_protected will make you assume too much about what attributes aren't available.

@jameskilton Agreed attr_protected will make you assume too much about what attributes aren't available.

@norv

This comment has been minimized.

Show comment
Hide comment
@norv

norv Mar 7, 2012

Access control, in particular access from a web form, to the model/database layer, is not a model matter. It's better that attr_accessible needs to be specified from now on, but it's not the end of this issue.

+1 for adnanced, znbailey, original gist.

norv commented Mar 7, 2012

Access control, in particular access from a web form, to the model/database layer, is not a model matter. It's better that attr_accessible needs to be specified from now on, but it's not the end of this issue.

+1 for adnanced, znbailey, original gist.

@atwam

This comment has been minimized.

Show comment
Hide comment
@atwam

atwam Mar 8, 2012

I've always wondered if rails should add a Form layer, like symfony does.
Having a separate layer that handles validation, nesting, display, assignment security (aware of current user/role) and update of models isn't too bad, and removes a lot of clutter from models...
The way symfony used to do it : http://www.symfony-project.org/forms/1_4/en/01-Form-Creation

atwam commented Mar 8, 2012

I've always wondered if rails should add a Form layer, like symfony does.
Having a separate layer that handles validation, nesting, display, assignment security (aware of current user/role) and update of models isn't too bad, and removes a lot of clutter from models...
The way symfony used to do it : http://www.symfony-project.org/forms/1_4/en/01-Form-Creation

@sbeam

This comment has been minimized.

Show comment
Hide comment
@sbeam

sbeam Mar 10, 2012

this is great for a simple case, but sometimes slice isn't enough, e.g. nested attributes, and also date/time fields that come in like "posted_on(1i)"=>"2010", "posted_on(2i)"=>"3", "posted_on(3i)"=>"3"

What about a way to have form_for send to the controller which fields actually exist in the form (how to do this in a tamper-proof fashion is a good question, an encrypted serialized hash of the legitimate keys would be needed) - any params that are not actually in the form_for block would be ignored. Of course there would need to be a way to whitelist other fields in case they were legitimately added client-side via JS.

That way you wouldn't have the model concerned about current user roles and authz, or even the controller. If a field exists in the form, then it seems safe to presume the corresponding param is meant to be changed via that form instance, and v/v.

sbeam commented Mar 10, 2012

this is great for a simple case, but sometimes slice isn't enough, e.g. nested attributes, and also date/time fields that come in like "posted_on(1i)"=>"2010", "posted_on(2i)"=>"3", "posted_on(3i)"=>"3"

What about a way to have form_for send to the controller which fields actually exist in the form (how to do this in a tamper-proof fashion is a good question, an encrypted serialized hash of the legitimate keys would be needed) - any params that are not actually in the form_for block would be ignored. Of course there would need to be a way to whitelist other fields in case they were legitimately added client-side via JS.

That way you wouldn't have the model concerned about current user roles and authz, or even the controller. If a field exists in the form, then it seems safe to presume the corresponding param is meant to be changed via that form instance, and v/v.

@bkimble

This comment has been minimized.

Show comment
Hide comment
@bkimble

bkimble Mar 23, 2012

Without the introduction of a deep slice implementation, this approach won't work well with nested attributes/forms. The model will still be the goto spot. A deep slice method in all its recursive glory will be a cpu hog as well :/

bkimble commented Mar 23, 2012

Without the introduction of a deep slice implementation, this approach won't work well with nested attributes/forms. The model will still be the goto spot. A deep slice method in all its recursive glory will be a cpu hog as well :/

@halloffame

This comment has been minimized.

Show comment
Hide comment
@halloffame

halloffame Apr 4, 2012

I'm not sure if slice supports it, but there are ways to support nested attributes using this technique

params[:post].slice(:title, :content, friends: [ :name, { family: [ :name ] }])

source https://github.com/rails/strong_parameters

I'm not sure if slice supports it, but there are ways to support nested attributes using this technique

params[:post].slice(:title, :content, friends: [ :name, { family: [ :name ] }])

source https://github.com/rails/strong_parameters

@JoshCheek

This comment has been minimized.

Show comment
Hide comment
@JoshCheek

JoshCheek Apr 7, 2012

I'd love to see validations also move out of the model.

I'd love to see validations also move out of the model.

@sobrinho

This comment has been minimized.

Show comment
Hide comment
@sobrinho

sobrinho Apr 7, 2012

@JoshCheek you can do that using active model ;)

sobrinho commented Apr 7, 2012

@JoshCheek you can do that using active model ;)

@keithtom

This comment has been minimized.

Show comment
Hide comment
@keithtom

keithtom Dec 11, 2012

Sweet pattern, and I like the simplicity. One thing though:

If someone forgets, or is unaware of the pattern, then you can potentially expose unwanted mass-assignment.

Perhaps, we can combine the strategies so that you can restrict assignment of certain attributes at the model layer, and then have this pattern act more as a 'mark as safe to mass assign'. This seems like it allows controller endpoints to dictate what inputs are acceptable, while letting the model dictate what attributes require additional checks.

As far as actual implementation, it might look more like

@resource.allow_assignment(:password, :password_confirmation)
@resource.update_attributes(params[:resource])

This of course doesn't have to be in a controller. It is really just the idea of forcing developers to explicitly say, "I really want to mass-assign these attributes here".

Thoughts?

Sweet pattern, and I like the simplicity. One thing though:

If someone forgets, or is unaware of the pattern, then you can potentially expose unwanted mass-assignment.

Perhaps, we can combine the strategies so that you can restrict assignment of certain attributes at the model layer, and then have this pattern act more as a 'mark as safe to mass assign'. This seems like it allows controller endpoints to dictate what inputs are acceptable, while letting the model dictate what attributes require additional checks.

As far as actual implementation, it might look more like

@resource.allow_assignment(:password, :password_confirmation)
@resource.update_attributes(params[:resource])

This of course doesn't have to be in a controller. It is really just the idea of forcing developers to explicitly say, "I really want to mass-assign these attributes here".

Thoughts?

@keithtom

This comment has been minimized.

Show comment
Hide comment
@keithtom

keithtom Dec 11, 2012

Ah just read the strong_params gem. nevermind...

Ah just read the strong_params gem. nevermind...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment