Rails core - May 2009 - redesigning mass attribute assignment (activerecord 3.0?)

Hello!

I''d like to refactor the code for mass assignment to support a new
security
paradigm, but figured that I might save some time getting feedback from the
gatekeepers of the code. :-)

We''re all familiar with attr_accessible and attr_protected. These
methods
provide rudimentary support from malicious mass assignment ... when people
remember to use them. I usually remember to set them myself, except that I
really wouldn''t be surprised if someone auditing my code found one or
two
spots that I''ve forgotten. But this isn''t about just changing
the default to
a hardcore blacklist instead of an all-encompassing whitelist. This is about
when attr_accessible and attr_protected aren''t enough.

They''re not enough when user roles enter into the picture. Just to take
the
simplest possible example, let''s suppose that an application has normal
registered users and it has staff. The staff, very trustable sorts (crossed
fingers), are allowed to change attributes that normal users are not.
It''s a
problem that attr_accessible and attr_protected, being very static lists,
are not prepared to handle. But let''s not stop there. We''ve
all coded spots
where we wanted to directly assign a couple of attributes ourselves to
trusted values, but we couldn''t use mass assignment because we (the
devs)
have essentially told the code to not even trust ourselves.

And yet ... it''s such a pretty syntax. Who wants to give that up?

I do! Well, just a little.

My idea (most likely not original) is to let the controller specify which
attributes may be assigned. In its briefest form, my proposal is be to
remove attr_accessible/attr_protected and change the mass assignment API as
follows:

* User.new(params[:user]) becomes User.new(params[:user], whitelist)
* @user.attributes = params[:user] becomes @user.assign(params[:user],
whitelist)

Overall I think that this makes things a whole lot cleaner! Which may or may
not be the same thing as fewer LOC. Consider:

* The whitelist would be optional, and if absent, would default to most
every column. Which makes it usable for normal coding, and doesn''t get
in
the way when you''re just getting started.
* The whitelist makes it very obvious how the code works. Obvious is better
than hidden. I normally don''t realize that my attr_accessible
isn''t
configured right until I happen to scan logs and see that "can''t
mass
assign" message.
* The whitelist can be a nested hash of any depth, offering excellent
support for the nascent nested forms functionality.
* The whitelist can be different in a controller that enforces broader
security and requires admin users in the first place.
* It moves security where it belongs, into the context of a user. Security
is business logic, in my mind, and I do like that in my (still-skinny)
controllers.
* The whitelist can be generated on-demand according to the current user.
Which would offer an actual extension point for permission-management
plugins and their User.current class/thread variable hacks!
* I like the assign() syntax better than the attributes= syntax. But that
could be just me.

I think that Rails 3.0 would be the right time for an API change like this.
It would provide a much better extension point for permission management
plugins, and, well, there are no holy cows, right? So ... should I put in
the time and code it up? What do you all think?

-Lance

-- 
rails blog: http://codelevy.com

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

I like it. 
Sent from my Verizon Wireless BlackBerry

-----Original Message-----
From: cainlevy <cainlevy@gmail.com>

Date: Fri, 22 May 2009 19:42:03 
To: <rubyonrails-core@googlegroups.com>
Subject: [Rails-core] redesigning mass attribute assignment (activerecord
 3.0?)


Hello!

I'd like to refactor the code for mass assignment to support a new security
paradigm, but figured that I might save some time getting feedback from the
gatekeepers of the code. :-)

We're all familiar with attr_accessible and attr_protected. These methods
provide rudimentary support from malicious mass assignment ... when people
remember to use them. I usually remember to set them myself, except that I
really wouldn't be surprised if someone auditing my code found one or two
spots that I've forgotten. But this isn't about just changing the
default to
a hardcore blacklist instead of an all-encompassing whitelist. This is about
when attr_accessible and attr_protected aren't enough.

They're not enough when user roles enter into the picture. Just to take the
simplest possible example, let's suppose that an application has normal
registered users and it has staff. The staff, very trustable sorts (crossed
fingers), are allowed to change attributes that normal users are not. It's a
problem that attr_accessible and attr_protected, being very static lists,
are not prepared to handle. But let's not stop there. We've all coded
spots
where we wanted to directly assign a couple of attributes ourselves to
trusted values, but we couldn't use mass assignment because we (the devs)
have essentially told the code to not even trust ourselves.

And yet ... it's such a pretty syntax. Who wants to give that up?

I do! Well, just a little.

My idea (most likely not original) is to let the controller specify which
attributes may be assigned. In its briefest form, my proposal is be to
remove attr_accessible/attr_protected and change the mass assignment API as
follows:

* User.new(params[:user]) becomes User.new(params[:user], whitelist)
* @user.attributes = params[:user] becomes @user.assign(params[:user],
whitelist)

Overall I think that this makes things a whole lot cleaner! Which may or may
not be the same thing as fewer LOC. Consider:

* The whitelist would be optional, and if absent, would default to most
every column. Which makes it usable for normal coding, and doesn't get in
the way when you're just getting started.
* The whitelist makes it very obvious how the code works. Obvious is better
than hidden. I normally don't realize that my attr_accessible isn't
configured right until I happen to scan logs and see that "can't mass
assign" message.
* The whitelist can be a nested hash of any depth, offering excellent
support for the nascent nested forms functionality.
* The whitelist can be different in a controller that enforces broader
security and requires admin users in the first place.
* It moves security where it belongs, into the context of a user. Security
is business logic, in my mind, and I do like that in my (still-skinny)
controllers.
* The whitelist can be generated on-demand according to the current user.
Which would offer an actual extension point for permission-management
plugins and their User.current class/thread variable hacks!
* I like the assign() syntax better than the attributes= syntax. But that
could be just me.

I think that Rails 3.0 would be the right time for an API change like this.
It would provide a much better extension point for permission management
plugins, and, well, there are no holy cows, right? So ... should I put in
the time and code it up? What do you all think?

-Lance

-- 
rails blog: http://codelevy.com




--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Fri, May 22, 2009 at 7:42 PM, cainlevy<cainlevy@gmail.com>
wrote:
> Hello!
>
> I''d like to refactor the code for mass assignment to support a new
security
> paradigm, but figured that I might save some time getting feedback from the
> gatekeepers of the code. :-)
>
> We''re all familiar with attr_accessible and attr_protected. These
methods
> provide rudimentary support from malicious mass assignment ... when people
> remember to use them. I usually remember to set them myself, except that I
> really wouldn''t be surprised if someone auditing my code found one
or two
> spots that I''ve forgotten. But this isn''t about just
changing the default to
> a hardcore blacklist instead of an all-encompassing whitelist. This is
about
> when attr_accessible and attr_protected aren''t enough.
>
> They''re not enough when user roles enter into the picture. Just to
take the
> simplest possible example, let''s suppose that an application has
normal
> registered users and it has staff. The staff, very trustable sorts (crossed
> fingers), are allowed to change attributes that normal users are not.
It''s a
> problem that attr_accessible and attr_protected, being very static lists,
> are not prepared to handle. But let''s not stop there.
We''ve all coded spots
> where we wanted to directly assign a couple of attributes ourselves to
> trusted values, but we couldn''t use mass assignment because we
(the devs)
> have essentially told the code to not even trust ourselves.
>
> And yet ... it''s such a pretty syntax. Who wants to give that up?
>
> I do! Well, just a little.
>
> My idea (most likely not original) is to let the controller specify which
> attributes may be assigned. In its briefest form, my proposal is be to
> remove attr_accessible/attr_protected and change the mass assignment API as
> follows:
>
> * User.new(params[:user]) becomes User.new(params[:user], whitelist)
> * @user.attributes = params[:user] becomes @user.assign(params[:user],
> whitelist)
>
> Overall I think that this makes things a whole lot cleaner! Which may or
may
> not be the same thing as fewer LOC. Consider:
>
> * The whitelist would be optional, and if absent, would default to most
> every column. Which makes it usable for normal coding, and doesn''t
get in
> the way when you''re just getting started.
> * The whitelist makes it very obvious how the code works. Obvious is better
> than hidden. I normally don''t realize that my attr_accessible
isn''t
> configured right until I happen to scan logs and see that
"can''t mass
> assign" message.
> * The whitelist can be a nested hash of any depth, offering excellent
> support for the nascent nested forms functionality.
> * The whitelist can be different in a controller that enforces broader
> security and requires admin users in the first place.
> * It moves security where it belongs, into the context of a user. Security
> is business logic, in my mind, and I do like that in my (still-skinny)
> controllers.
> * The whitelist can be generated on-demand according to the current user.
> Which would offer an actual extension point for permission-management
> plugins and their User.current class/thread variable hacks!
> * I like the assign() syntax better than the attributes= syntax. But that
> could be just me.
>
> I think that Rails 3.0 would be the right time for an API change like this.
> It would provide a much better extension point for permission management
> plugins, and, well, there are no holy cows, right? So ... should I put in
> the time and code it up? What do you all think?
FWIW, Sequel has had something similar to what you propose for about a
year.  You can set accessible/protected attributes at a class level
and you can use the set_(only|except) instance methods to only
allow/reject specific attributes per method call.   I added support
for it for pretty much the same reasons you gave.  The all-or-nothing
approach of ActiveRecord''s mass assignment is not flexible enough to
handle different security contexts.

I''m -1 on this being added to ActiveRecord, so Sequel keeps its
advantage in this area. ;p

Jeremy

On Fri, May 22, 2009 at 11:42 PM, cainlevy <cainlevy@gmail.com>
wrote:
> We''re all familiar with attr_accessible and attr_protected. These
methods
> provide rudimentary support from malicious mass assignment ... when people
> remember to use them. I usually remember to set them myself, except that I
> really wouldn''t be surprised if someone auditing my code found one
or two
> spots that I''ve forgotten. But this isn''t about just
changing the default to
> a hardcore blacklist instead of an all-encompassing whitelist. This is
about
> when attr_accessible and attr_protected aren''t enough.
>
> They''re not enough when user roles enter into the picture. Just to
take the
> simplest possible example, let''s suppose that an application has
normal
> registered users and it has staff. The staff, very trustable sorts (crossed
> fingers), are allowed to change attributes that normal users are not.
It''s a
> problem that attr_accessible and attr_protected, being very static lists,
> are not prepared to handle. But let''s not stop there.
We''ve all coded spots
> where we wanted to directly assign a couple of attributes ourselves to
> trusted values, but we couldn''t use mass assignment because we
(the devs)
> have essentially told the code to not even trust ourselves.
I completely agree with you. Which attributes are allowed to be
mass-assigned is almost always context-sensitive, and thus the
whitelist doesn''t belong in the class level.
> And yet ... it''s such a pretty syntax. Who wants to give that up?
>
> I do! Well, just a little.
>
> My idea (most likely not original) is to let the controller specify which
> attributes may be assigned. In its briefest form, my proposal is be to
> remove attr_accessible/attr_protected and change the mass assignment API as
> follows:
>
> * User.new(params[:user]) becomes User.new(params[:user], whitelist)
> * @user.attributes = params[:user] becomes @user.assign(params[:user],
> whitelist)
Agreed.

How about a simple extension to Hash (somebody let me know if this
exists already) that allows you to do:

user.attributes = params[:user].pick(:first_name, :last_name)

(Naive implementation here: http://gist.github.com/116880)

The RHS could be extracted to a separate method in the relevant
controller, which would do the context checking.

Regardless of the syntax I think the use of attr_accessible at the
class level should not be encouraged. I''m +1 on this.

On May 23, 2009, at 7:40 PM, Damian Janowski wrote:
> How about a simple extension to Hash (somebody let me know if this  
> exists already) that allows you to do:
>
> user.attributes = params[:user].pick(:first_name, :last_name)
This is what active support #slice and #slice! hash extensions do and  
I have used this in my controllers already for mass attribute white  
lists per controller action "when needed" for a few months now. I  
think since 2.3.2 (perhaps earlier) the #slice method even do as  
expected for indifferent access hashes.

 >> {''a'' => ''fff'',
''b'' =>
''ccc''}.with_indifferent_access.slice(:a)
=> {"a"=>"fff"}


  - Ken

Which points to an interesting question -- should the model or the
controller be responsible for filtering the attributes? That is,
should the burden be on the model to only assign allowed parameters,
or the controller to only pass allowed parameters? It certainly seems
simple to do it from the controller using something like your
Hash#pick method, but I think it''s safer to do it from the model. For
example, if the model is responsible for filtering assignable
attributes, it may create an intelligent default blacklist for cases
where the developer has paid no attention.

I''ve just about finished a patch to implement AR::Base#assign
(attributes, allowed_attributes). In the process I''ve realized that
allowed_attributes can simply be an override to attr_accessible/
attr_protected, which makes for an easily backwards compatible API
update. So that''ll be my first ticket.

I''d really prefer to remove attr_accessible/attr_protected altogether
as I believe they are in all ways inferior to the new approach and
would only serve to clutter the API in the name of backwards
compatibility. But that''s a secondary concern, and will be in a second
ticket that may be evaluated independently.

On May 23, 4:40 pm, Damian Janowski <damian.janow...@gmail.com>
wrote:
> On Fri, May 22, 2009 at 11:42 PM, cainlevy <cainl...@gmail.com>
wrote:
> > We''re all familiar with attr_accessible and attr_protected.
These methods
> > provide rudimentary support from malicious mass assignment ... when
people
> > remember to use them. I usually remember to set them myself, except
that I
> > really wouldn''t be surprised if someone auditing my code
found one or two
> > spots that I''ve forgotten. But this isn''t about just
changing the default to
> > a hardcore blacklist instead of an all-encompassing whitelist. This is
about
> > when attr_accessible and attr_protected aren''t enough.
>
> > They''re not enough when user roles enter into the picture.
Just to take the
> > simplest possible example, let''s suppose that an application
has normal
> > registered users and it has staff. The staff, very trustable sorts
(crossed
> > fingers), are allowed to change attributes that normal users are not.
It''s a
> > problem that attr_accessible and attr_protected, being very static
lists,
> > are not prepared to handle. But let''s not stop there.
We''ve all coded spots
> > where we wanted to directly assign a couple of attributes ourselves to
> > trusted values, but we couldn''t use mass assignment because
we (the devs)
> > have essentially told the code to not even trust ourselves.
>
> I completely agree with you. Which attributes are allowed to be
> mass-assigned is almost always context-sensitive, and thus the
> whitelist doesn''t belong in the class level.
>
> > And yet ... it''s such a pretty syntax. Who wants to give that
up?
>
> > I do! Well, just a little.
>
> > My idea (most likely not original) is to let the controller specify
which
> > attributes may be assigned. In its briefest form, my proposal is be to
> > remove attr_accessible/attr_protected and change the mass assignment
API as
> > follows:
>
> > * User.new(params[:user]) becomes User.new(params[:user], whitelist)
> > * @user.attributes = params[:user] becomes @user.assign(params[:user],
> > whitelist)
>
> Agreed.
>
> How about a simple extension to Hash (somebody let me know if this
> exists already) that allows you to do:
>
> user.attributes = params[:user].pick(:first_name, :last_name)
>
> (Naive implementation here:http://gist.github.com/116880)
>
> The RHS could be extracted to a separate method in the relevant
> controller, which would do the context checking.
>
> Regardless of the syntax I think the use of attr_accessible at the
> class level should not be encouraged. I''m +1 on this.

Ok, primary ticket:
https://rails.lighthouseapp.com/projects/8994/tickets/2704-allow-calltime-list-of-allowed-attributes-for-mass-assignment

And the bonus round:
https://rails.lighthouseapp.com/projects/8994/tickets/2705-deprecate-attr_accessible-attr_protected

On May 23, 5:40 pm, cainlevy <cainl...@gmail.com>
wrote:
> Which points to an interesting question -- should the model or the
> controller be responsible for filtering the attributes? That is,
> should the burden be on the model to only assign allowed parameters,
> or the controller to only pass allowed parameters? It certainly seems
> simple to do it from the controller using something like your
> Hash#pick method, but I think it''s safer to do it from the model.
For
> example, if the model is responsible for filtering assignable
> attributes, it may create an intelligent default blacklist for cases
> where the developer has paid no attention.
>
> I''ve just about finished a patch to implement AR::Base#assign
> (attributes, allowed_attributes). In the process I''ve realized
that
> allowed_attributes can simply be an override to attr_accessible/
> attr_protected, which makes for an easily backwards compatible API
> update. So that''ll be my first ticket.
>
> I''d really prefer to remove attr_accessible/attr_protected
altogether
> as I believe they are in all ways inferior to the new approach and
> would only serve to clutter the API in the name of backwards
> compatibility. But that''s a secondary concern, and will be in a
second
> ticket that may be evaluated independently.
>
> On May 23, 4:40 pm, Damian Janowski <damian.janow...@gmail.com>
wrote:
>
> > On Fri, May 22, 2009 at 11:42 PM, cainlevy <cainl...@gmail.com>
wrote:
> > > We''re all familiar with attr_accessible and
attr_protected. These methods
> > > provide rudimentary support from malicious mass assignment ...
when people
> > > remember to use them. I usually remember to set them myself,
except that I
> > > really wouldn''t be surprised if someone auditing my code
found one or two
> > > spots that I''ve forgotten. But this isn''t about
just changing the default to
> > > a hardcore blacklist instead of an all-encompassing whitelist.
This is about
> > > when attr_accessible and attr_protected aren''t enough.
>
> > > They''re not enough when user roles enter into the
picture. Just to take the
> > > simplest possible example, let''s suppose that an
application has normal
> > > registered users and it has staff. The staff, very trustable
sorts (crossed
> > > fingers), are allowed to change attributes that normal users are
not. It''s a
> > > problem that attr_accessible and attr_protected, being very
static lists,
> > > are not prepared to handle. But let''s not stop there.
We''ve all coded spots
> > > where we wanted to directly assign a couple of attributes
ourselves to
> > > trusted values, but we couldn''t use mass assignment
because we (the devs)
> > > have essentially told the code to not even trust ourselves.
>
> > I completely agree with you. Which attributes are allowed to be
> > mass-assigned is almost always context-sensitive, and thus the
> > whitelist doesn''t belong in the class level.
>
> > > And yet ... it''s such a pretty syntax. Who wants to give
that up?
>
> > > I do! Well, just a little.
>
> > > My idea (most likely not original) is to let the controller
specify which
> > > attributes may be assigned. In its briefest form, my proposal is
be to
> > > remove attr_accessible/attr_protected and change the mass
assignment API as
> > > follows:
>
> > > * User.new(params[:user]) becomes User.new(params[:user],
whitelist)
> > > * @user.attributes = params[:user] becomes
@user.assign(params[:user],
> > > whitelist)
>
> > Agreed.
>
> > How about a simple extension to Hash (somebody let me know if this
> > exists already) that allows you to do:
>
> > user.attributes = params[:user].pick(:first_name, :last_name)
>
> > (Naive implementation here:http://gist.github.com/116880)
>
> > The RHS could be extracted to a separate method in the relevant
> > controller, which would do the context checking.
>
> > Regardless of the syntax I think the use of attr_accessible at the
> > class level should not be encouraged. I''m +1 on this.
>
>

The second one seems like a really, really bad idea. Specifying the  
allowed attributes at the call is great, but what about the simpler  
use cases? Sometimes you just want to prohibit (or allow) mass  
assignment, and requiring users to specify the list every time is the  
opposite of DRY. Not to mention the maintenance headache when adding a  
new field...

--Matt Jones

On May 24, 2009, at 12:16 AM, cainlevy wrote:
>
> Ok, primary ticket:
>
https://rails.lighthouseapp.com/projects/8994/tickets/2704-allow-calltime-list-of-allowed-attributes-for-mass-assignment
>
> And the bonus round:
>
https://rails.lighthouseapp.com/projects/8994/tickets/2705-deprecate-attr_accessible-attr_protected

cainlevy wrote:
> Which points to an interesting question -- should the model or the
> controller be responsible for filtering the attributes? That is,
> should the burden be on the model to only assign allowed parameters,
> or the controller to only pass allowed parameters? It certainly seems
> simple to do it from the controller using something like your
> Hash#pick method, but I think it''s safer to do it from the model.
For
> example, if the model is responsible for filtering assignable
> attributes, it may create an intelligent default blacklist for cases
> where the developer has paid no attention.
> 
> I''ve just about finished a patch to implement AR::Base#assign
> (attributes, allowed_attributes). In the process I''ve realized
that
> allowed_attributes can simply be an override to attr_accessible/
> attr_protected, which makes for an easily backwards compatible API
> update. So that''ll be my first ticket.
I don''t think this massive change to the api is justified. 
You''re
introducing complexity for all users to support a few cases which, while
hardly rare, aren''t 100% of user''s requirements.

It should be trivial for you to implement this as a plugin to see if
people prefer this approach to specifying assignable attributes.  If
that picks up momentum we can look at pulling it in to rails.

In the meantime users can already do:

@user.attributes = params[:user].slice(:email, :password,
:password_confirmation)

or

@user.attributes = params[:user].except(:admin)

> I''d really prefer to remove attr_accessible/attr_protected
altogether
> as I believe they are in all ways inferior to the new approach and
> would only serve to clutter the API in the name of backwards
> compatibility. But that''s a secondary concern, and will be in a
second
> ticket that may be evaluated independently.
attr_accessible and friends are a great simple solution for a really
common case.  We shouldn''t lose sight of that just because there are
some cases where they''re not perfect.



-- 
Cheers,

Koz

Hey Matt, I did some thinkng about this and I''m not sure if the
simpler use cases are actually simpler with attr_accessible, or if
they''re just different? For any given model I generally only have one
resource controller, so in that case I''d simply be moving the
attribute list from the model to the controller. And when a model is
represented by more than one resource controller, it''s probably
because of an entirely different context with different permissions.
And of course there are easy ways to DRY up a list of attributes that
are commonly used without needing official API support.

Do you have any examples where this would be a bad idea? Perhaps I''m
missing something obvious here.

But yes, I split #2705 into a separate ticket because it''s more about
opinion than functionality.

On May 23, 10:05 pm, Matt Jones <al2o...@gmail.com>
wrote:
> The second one seems like a really, really bad idea. Specifying the  
> allowed attributes at the call is great, but what about the simpler  
> use cases? Sometimes you just want to prohibit (or allow) mass  
> assignment, and requiring users to specify the list every time is the  
> opposite of DRY. Not to mention the maintenance headache when adding a  
> new field...
>
> --Matt Jones
>
> On May 24, 2009, at 12:16 AM, cainlevy wrote:
>
>
>
> > Ok, primary ticket:
>
>https://rails.lighthouseapp.com/projects/8994/tickets/2704-allow-call...
>
> > And the bonus round:
>
>https://rails.lighthouseapp.com/projects/8994/tickets/2705-deprecate-...
>
>

I''ve put the core assign() method into a plugin. Feedback is always
desired.

http://github.com/cainlevy/mass_assignment

On May 24, 1:33 am, cainlevy <cainl...@gmail.com>
wrote:
> Hey Matt, I did some thinkng about this and I''m not sure if the
> simpler use cases are actually simpler with attr_accessible, or if
> they''re just different? For any given model I generally only have
one
> resource controller, so in that case I''d simply be moving the
> attribute list from the model to the controller. And when a model is
> represented by more than one resource controller, it''s probably
> because of an entirely different context with different permissions.
> And of course there are easy ways to DRY up a list of attributes that
> are commonly used without needing official API support.
>
> Do you have any examples where this would be a bad idea? Perhaps
I''m
> missing something obvious here.
>
> But yes, I split #2705 into a separate ticket because it''s more
about
> opinion than functionality.
>
> On May 23, 10:05 pm, Matt Jones <al2o...@gmail.com> wrote:
>
> > The second one seems like a really, really bad idea. Specifying the  
> > allowed attributes at the call is great, but what about the simpler  
> > use cases? Sometimes you just want to prohibit (or allow) mass  
> > assignment, and requiring users to specify the list every time is the
 
> > opposite of DRY. Not to mention the maintenance headache when adding a
 
> > new field...
>
> > --Matt Jones
>
> > On May 24, 2009, at 12:16 AM, cainlevy wrote:
>
> > > Ok, primary ticket:
> >
>https://rails.lighthouseapp.com/projects/8994/tickets/2704-allow-call...
>
> > > And the bonus round:
> >
>https://rails.lighthouseapp.com/projects/8994/tickets/2705-deprecate-...
>
>

class UsersController < ApplicationController
  def create
    @user = User.new
    # during signup a user may pick a username
    @user.assign(params[:user], [:username, :email, :password,
:password_confirmation])
    @user.save!
    ...
  end

I still don''t see why #splice or #except wouldn''t be preferred
here?
@user.attributes = params[:user].splice(:username, :email, :password,
:password_confirmation)

-Brennan


On Mon, May 25, 2009 at 7:36 PM, cainlevy <cainlevy@gmail.com> wrote:
>
> I''ve put the core assign() method into a plugin. Feedback is
always
> desired.
>
> http://github.com/cainlevy/mass_assignment
>
> On May 24, 1:33 am, cainlevy <cainl...@gmail.com> wrote:
> > Hey Matt, I did some thinkng about this and I''m not sure if
the
> > simpler use cases are actually simpler with attr_accessible, or if
> > they''re just different? For any given model I generally only
have one
> > resource controller, so in that case I''d simply be moving the
> > attribute list from the model to the controller. And when a model is
> > represented by more than one resource controller, it''s
probably
> > because of an entirely different context with different permissions.
> > And of course there are easy ways to DRY up a list of attributes that
> > are commonly used without needing official API support.
> >
> > Do you have any examples where this would be a bad idea? Perhaps
I''m
> > missing something obvious here.
> >
> > But yes, I split #2705 into a separate ticket because it''s
more about
> > opinion than functionality.
> >
> > On May 23, 10:05 pm, Matt Jones <al2o...@gmail.com> wrote:
> >
> > > The second one seems like a really, really bad idea. Specifying
the
> > > allowed attributes at the call is great, but what about the
simpler
> > > use cases? Sometimes you just want to prohibit (or allow) mass
> > > assignment, and requiring users to specify the list every time is
the
> > > opposite of DRY. Not to mention the maintenance headache when
adding a
>
> > > new field...
> >
> > > --Matt Jones
> >
> > > On May 24, 2009, at 12:16 AM, cainlevy wrote:
> >
> > > > Ok, primary ticket:
> > > >
> https://rails.lighthouseapp.com/projects/8994/tickets/2704-allow-call...
> >
> > > > And the bonus round:
> > > >
> https://rails.lighthouseapp.com/projects/8994/tickets/2705-deprecate-...
> >
> >
> >
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On May 24, 8:25 am, Michael Koziarski <mich...@koziarski.com>
wrote:
> cainlevy wrote:
> > Which points to an interesting question -- should the model or the
> > controller be responsible for filtering the attributes? That is,
> > should the burden be on the model to only assign allowed parameters,
> > or the controller to only pass allowed parameters? It certainly seems
> > simple to do it from the controller using something like your
> > Hash#pick method, but I think it''s safer to do it from the
model. For
> > example, if the model is responsible for filtering assignable
> > attributes, it may create an intelligent default blacklist for cases
> > where the developer has paid no attention.
>
> > I''ve just about finished a patch to implement AR::Base#assign
> > (attributes, allowed_attributes). In the process I''ve
realized that
> > allowed_attributes can simply be an override to attr_accessible/
> > attr_protected, which makes for an easily backwards compatible API
> > update. So that''ll be my first ticket.
>
> I don''t think this massive change to the api is justified.
 You''re
> introducing complexity for all users to support a few cases which, while
> hardly rare, aren''t 100% of user''s requirements.
>
> It should be trivial for you to implement this as a plugin to see if
> people prefer this approach to specifying assignable attributes.  If
> that picks up momentum we can look at pulling it in to rails.
>
> In the meantime users can already do:
>
> @user.attributes = params[:user].slice(:email, :password,
> :password_confirmation)
>
> or
>
> @user.attributes = params[:user].except(:admin)
>
> > I''d really prefer to remove attr_accessible/attr_protected
altogether
> > as I believe they are in all ways inferior to the new approach and
> > would only serve to clutter the API in the name of backwards
> > compatibility. But that''s a secondary concern, and will be in
a second
> > ticket that may be evaluated independently.
>
> attr_accessible and friends are a great simple solution for a really
> common case.  We shouldn''t lose sight of that just because there
are
> some cases where they''re not perfect.
I agree. Lance raises very valid points, but what would what''s wrong
with using Hash#slice and Hash#except to solve these problems?
Especially if the documentation for attr_accessible/attr_protected can
be updated to refer to the usage of Hash#slice and Hash#except for
more complex use cases?

I''m still struggling a bit for clear reasons why it might be better to
filter the attributes controller-side or model-side. The only
functional advantage I can think of either way is that if you want a
system with overridable defaults, you want to build the filtering into
the model. For example, if you had a baseline attr_accessible
declaration and overrode it at calltime with a different set. But eh,
combining declarations like that sounds messy and prone to failure.

Or perhaps some security plugin for people who want to start with
fewer (or no) assignable fields could create default blacklists for
any /is_.*/ and /.*_id/ fields, to be overridden as needed. Hmm, that
actually sounds interesting. Perhaps I''ll add that optional module
into my plugin. :-)

Other than that, is the difference entirely aesthetic? I think the
"assign(params[:user], ...)" syntax reads better than "attributes
params[:user].slice(...)", but that alone isn''t enough reason to
change an API.

On May 26, 3:42 am, Hongli Lai <hon...@phusion.nl>
wrote:
> On May 24, 8:25 am, Michael Koziarski <mich...@koziarski.com> wrote:
>
>
>
> > cainlevy wrote:
> > > Which points to an interesting question -- should the model or
the
> > > controller be responsible for filtering the attributes? That is,
> > > should the burden be on the model to only assign allowed
parameters,
> > > or the controller to only pass allowed parameters? It certainly
seems
> > > simple to do it from the controller using something like your
> > > Hash#pick method, but I think it''s safer to do it from
the model. For
> > > example, if the model is responsible for filtering assignable
> > > attributes, it may create an intelligent default blacklist for
cases
> > > where the developer has paid no attention.
>
> > > I''ve just about finished a patch to implement
AR::Base#assign
> > > (attributes, allowed_attributes). In the process I''ve
realized that
> > > allowed_attributes can simply be an override to attr_accessible/
> > > attr_protected, which makes for an easily backwards compatible
API
> > > update. So that''ll be my first ticket.
>
> > I don''t think this massive change to the api is justified.
 You''re
> > introducing complexity for all users to support a few cases which,
while
> > hardly rare, aren''t 100% of user''s requirements.
>
> > It should be trivial for you to implement this as a plugin to see if
> > people prefer this approach to specifying assignable attributes.  If
> > that picks up momentum we can look at pulling it in to rails.
>
> > In the meantime users can already do:
>
> > @user.attributes = params[:user].slice(:email, :password,
> > :password_confirmation)
>
> > or
>
> > @user.attributes = params[:user].except(:admin)
>
> > > I''d really prefer to remove
attr_accessible/attr_protected altogether
> > > as I believe they are in all ways inferior to the new approach
and
> > > would only serve to clutter the API in the name of backwards
> > > compatibility. But that''s a secondary concern, and will
be in a second
> > > ticket that may be evaluated independently.
>
> > attr_accessible and friends are a great simple solution for a really
> > common case.  We shouldn''t lose sight of that just because
there are
> > some cases where they''re not perfect.
>
> I agree. Lance raises very valid points, but what would what''s
wrong
> with using Hash#slice and Hash#except to solve these problems?
> Especially if the documentation for attr_accessible/attr_protected can
> be updated to refer to the usage of Hash#slice and Hash#except for
> more complex use cases?

Based on the example given for #assign, it does appear to be just
a syntactical difference, and I wouldn''t think anything about AR core
should
be changed.
When mass assigning attributes, one could always do: @user.attributes
params[:user].slice(*@user.writeable_attributes), where
User#writeable_attributes would tie in to your ACL system and return an
array of settable attributes for that user type.

-Brennan


On Tue, May 26, 2009 at 1:42 PM, cainlevy <cainlevy@gmail.com> wrote:
>
> I''m still struggling a bit for clear reasons why it might be
better to
> filter the attributes controller-side or model-side. The only
> functional advantage I can think of either way is that if you want a
> system with overridable defaults, you want to build the filtering into
> the model. For example, if you had a baseline attr_accessible
> declaration and overrode it at calltime with a different set. But eh,
> combining declarations like that sounds messy and prone to failure.
>
> Or perhaps some security plugin for people who want to start with
> fewer (or no) assignable fields could create default blacklists for
> any /is_.*/ and /.*_id/ fields, to be overridden as needed. Hmm, that
> actually sounds interesting. Perhaps I''ll add that optional module
> into my plugin. :-)
>
> Other than that, is the difference entirely aesthetic? I think the
> "assign(params[:user], ...)" syntax reads better than
"attributes > params[:user].slice(...)", but that alone
isn''t enough reason to
> change an API.
>
> On May 26, 3:42 am, Hongli Lai <hon...@phusion.nl> wrote:
> > On May 24, 8:25 am, Michael Koziarski <mich...@koziarski.com>
wrote:
> >
> >
> >
> > > cainlevy wrote:
> > > > Which points to an interesting question -- should the model
or the
> > > > controller be responsible for filtering the attributes? That
is,
> > > > should the burden be on the model to only assign allowed
parameters,
> > > > or the controller to only pass allowed parameters? It
certainly seems
> > > > simple to do it from the controller using something like
your
> > > > Hash#pick method, but I think it''s safer to do it
from the model. For
> > > > example, if the model is responsible for filtering
assignable
> > > > attributes, it may create an intelligent default blacklist
for cases
> > > > where the developer has paid no attention.
> >
> > > > I''ve just about finished a patch to implement
AR::Base#assign
> > > > (attributes, allowed_attributes). In the process
I''ve realized that
> > > > allowed_attributes can simply be an override to
attr_accessible/
> > > > attr_protected, which makes for an easily backwards
compatible API
> > > > update. So that''ll be my first ticket.
> >
> > > I don''t think this massive change to the api is
justified.  You''re
> > > introducing complexity for all users to support a few cases
which,
> while
> > > hardly rare, aren''t 100% of user''s
requirements.
> >
> > > It should be trivial for you to implement this as a plugin to see
if
> > > people prefer this approach to specifying assignable attributes. 
If
> > > that picks up momentum we can look at pulling it in to rails.
> >
> > > In the meantime users can already do:
> >
> > > @user.attributes = params[:user].slice(:email, :password,
> > > :password_confirmation)
> >
> > > or
> >
> > > @user.attributes = params[:user].except(:admin)
> >
> > > > I''d really prefer to remove
attr_accessible/attr_protected altogether
> > > > as I believe they are in all ways inferior to the new
approach and
> > > > would only serve to clutter the API in the name of backwards
> > > > compatibility. But that''s a secondary concern, and
will be in a
> second
> > > > ticket that may be evaluated independently.
> >
> > > attr_accessible and friends are a great simple solution for a
really
> > > common case.  We shouldn''t lose sight of that just
because there are
> > > some cases where they''re not perfect.
> >
> > I agree. Lance raises very valid points, but what would
what''s wrong
> > with using Hash#slice and Hash#except to solve these problems?
> > Especially if the documentation for attr_accessible/attr_protected can
> > be updated to refer to the usage of Hash#slice and Hash#except for
> > more complex use cases?
> >
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Tue, May 26, 2009 at 19:42, cainlevy <cainlevy@gmail.com> wrote:
>
> I''m still struggling a bit for clear reasons why it might be
better to
> filter the attributes controller-side or model-side.

To me it sounds like a responsibility of the controller. It is the one
receiving the parameters, selecting the relevant ones and passing them down
to models.

On the other hand, having attribute filtering in the controllers would
require a bit of sync between model and controller code. I like to avoid
controller having too much knowledge of model attributes, myself.

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Tue, May 26, 2009 at 12:42 PM, Hongli Lai <hongli@phusion.nl> wrote:
> I agree. Lance raises very valid points, but what would what''s
wrong
> with using Hash#slice and Hash#except to solve these problems?
> Especially if the documentation for attr_accessible/attr_protected can
> be updated to refer to the usage of Hash#slice and Hash#except for
> more complex use cases?
Yeah I''ve added it

  
http://github.com/lifo/docrails/commit/6197606588674bd16e17899e0df15adf2a482ba0

I think a concrete application will probably need some controller
macro to be able to express those tweaks at a higher level. But the
basic technique to build upon is documented there anyway.

One other thought - going back to the original example (admin user can  
mass-assign fields that are normally protected), what about an extra  
parameter to update_attributes (and possibly create)? ie:

@model.update_attributes(params[:whatever],  
[:stuff_non_admins_cant_change])

So essentially a, "no, really, you can mass-assign these attributes  
just this once" parameter. That would still allow regular code to work  
correctly while permitting the context-sensitive stuff you''re looking  
for.

--Matt Jones

On Tue, May 26, 2009 at 10:30 PM, Matt Jones <al2o3cr@gmail.com> wrote:
> One other thought - going back to the original example (admin user can
> mass-assign fields that are normally protected), what about an extra
> parameter to update_attributes (and possibly create)? ie:
>
> @model.update_attributes(params[:whatever],
> [:stuff_non_admins_cant_change])
>
> So essentially a, "no, really, you can mass-assign these attributes
> just this once" parameter. That would still allow regular code to work
> correctly while permitting the context-sensitive stuff you''re
looking
> for.
Certainly the Hash#except idiom requires you whitelist (or
not-blacklist) sensitive data, because attr_(accessible|protected) are
of course applied to whatever sanitized hash you pass. So in
particular you can only narrow accessible aattributes (or extend
protected attributes)

Going the other way around sounds better to me, not sure about the API
though. I think it requires the same amount of configuration and
exceptions, but looks like a safer default.

After playing with a reimagined version of attr_accessible/
attr_protected in my plugin, I''m much happier with the model-side
filtering approach. I think it allows for more interesting and useful
defaults.

Since this API is to live only as a plugin for a bit, I''m unsure
whether this thread is the place to continue discussion? I think that
Xavier''s improved documentation is probably all that can/should be
done to core at this time. Anyone interested in playing with the mass
assignment API is welcome to contact me directly or through GitHub.

On May 26, 1:47 pm, Xavier Noria <f...@hashref.com>
wrote:
> On Tue, May 26, 2009 at 10:30 PM, Matt Jones <al2o...@gmail.com>
wrote:
> > One other thought - going back to the original example (admin user can
> > mass-assign fields that are normally protected), what about an extra
> > parameter to update_attributes (and possibly create)? ie:
>
> > @model.update_attributes(params[:whatever],
> > [:stuff_non_admins_cant_change])
>
> > So essentially a, "no, really, you can mass-assign these
attributes
> > just this once" parameter. That would still allow regular code to
work
> > correctly while permitting the context-sensitive stuff you''re
looking
> > for.
>
> Certainly the Hash#except idiom requires you whitelist (or
> not-blacklist) sensitive data, because attr_(accessible|protected) are
> of course applied to whatever sanitized hash you pass. So in
> particular you can only narrow accessible aattributes (or extend
> protected attributes)
>
> Going the other way around sounds better to me, not sure about the API
> though. I think it requires the same amount of configuration and
> exceptions, but looks like a safer default.

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).

-John

On Tue, May 26, 2009 at 8:57 PM, cainlevy <cainlevy@gmail.com> wrote:
>
> After playing with a reimagined version of attr_accessible/
> attr_protected in my plugin, I''m much happier with the model-side
> filtering approach. I think it allows for more interesting and useful
> defaults.
>
> Since this API is to live only as a plugin for a bit, I''m unsure
> whether this thread is the place to continue discussion? I think that
> Xavier''s improved documentation is probably all that can/should be
> done to core at this time. Anyone interested in playing with the mass
> assignment API is welcome to contact me directly or through GitHub.
>
> On May 26, 1:47 pm, Xavier Noria <f...@hashref.com> wrote:
> > On Tue, May 26, 2009 at 10:30 PM, Matt Jones <al2o...@gmail.com>
wrote:
> > > One other thought - going back to the original example (admin
user can
> > > mass-assign fields that are normally protected), what about an
extra
> > > parameter to update_attributes (and possibly create)? ie:
> >
> > > @model.update_attributes(params[:whatever],
> > > [:stuff_non_admins_cant_change])
> >
> > > So essentially a, "no, really, you can mass-assign these
attributes
> > > just this once" parameter. That would still allow regular
code to work
> > > correctly while permitting the context-sensitive stuff
you''re looking
> > > for.
> >
> > Certainly the Hash#except idiom requires you whitelist (or
> > not-blacklist) sensitive data, because attr_(accessible|protected) are
> > of course applied to whatever sanitized hash you pass. So in
> > particular you can only narrow accessible aattributes (or extend
> > protected attributes)
> >
> > Going the other way around sounds better to me, not sure about the API
> > though. I think it requires the same amount of configuration and
> > exceptions, but looks like a safer default.
> >
>

-- 
John Trupiano
President
SmartLogic Solutions
http://www.smartlogicsolutions.com
@jtrupiano

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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.

On May 26, 6:12 pm, John Trupiano <jtrupi...@gmail.com>
wrote:
> 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).
>
> -John
>
>
>
> On Tue, May 26, 2009 at 8:57 PM, cainlevy <cainl...@gmail.com> wrote:
>
> > After playing with a reimagined version of attr_accessible/
> > attr_protected in my plugin, I''m much happier with the
model-side
> > filtering approach. I think it allows for more interesting and useful
> > defaults.
>
> > Since this API is to live only as a plugin for a bit, I''m
unsure
> > whether this thread is the place to continue discussion? I think that
> > Xavier''s improved documentation is probably all that
can/should be
> > done to core at this time. Anyone interested in playing with the mass
> > assignment API is welcome to contact me directly or through GitHub.
>
> > On May 26, 1:47 pm, Xavier Noria <f...@hashref.com> wrote:
> > > On Tue, May 26, 2009 at 10:30 PM, Matt Jones
<al2o...@gmail.com> wrote:
> > > > One other thought - going back to the original example
(admin user can
> > > > mass-assign fields that are normally protected), what about
an extra
> > > > parameter to update_attributes (and possibly create)? ie:
>
> > > > @model.update_attributes(params[:whatever],
> > > > [:stuff_non_admins_cant_change])
>
> > > > So essentially a, "no, really, you can mass-assign
these attributes
> > > > just this once" parameter. That would still allow
regular code to work
> > > > correctly while permitting the context-sensitive stuff
you''re looking
> > > > for.
>
> > > Certainly the Hash#except idiom requires you whitelist (or
> > > not-blacklist) sensitive data, because
attr_(accessible|protected) are
> > > of course applied to whatever sanitized hash you pass. So in
> > > particular you can only narrow accessible aattributes (or extend
> > > protected attributes)
>
> > > Going the other way around sounds better to me, not sure about
the API
> > > though. I think it requires the same amount of configuration and
> > > exceptions, but looks like a safer default.
>
> --
> John Trupiano
> President
> SmartLogic Solutionshttp://www.smartlogicsolutions.com
> @jtrupiano

On May 27, 2:57 am, cainlevy <cainl...@gmail.com>
wrote:
> After playing with a reimagined version of attr_accessible/
> attr_protected in my plugin, I''m much happier with the model-side
> filtering approach. I think it allows for more interesting and useful
> defaults.
>
> Since this API is to live only as a plugin for a bit, I''m unsure
> whether this thread is the place to continue discussion? I think that
> Xavier''s improved documentation is probably all that can/should be
> done to core at this time. Anyone interested in playing with the mass
> assignment API is welcome to contact me directly or through GitHub.
Just publish the code. Real-world experience will teach us whether it
has any advantages over Hash#slice/Hash#except.

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

Regards,

Ryan

On May 27, 2:26 am, Hongli Lai <hon...@phusion.nl>
wrote:
> On May 27, 2:57 am, cainlevy <cainl...@gmail.com> wrote:
>
> > After playing with a reimagined version of attr_accessible/
> > attr_protected in my plugin, I''m much happier with the
model-side
> > filtering approach. I think it allows for more interesting and useful
> > defaults.
>
> > Since this API is to live only as a plugin for a bit, I''m
unsure
> > whether this thread is the place to continue discussion? I think that
> > Xavier''s improved documentation is probably all that
can/should be
> > done to core at this time. Anyone interested in playing with the mass
> > assignment API is welcome to contact me directly or through GitHub.
>
> Just publish the code. Real-world experience will teach us whether it
> has any advantages over Hash#slice/Hash#except.

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.

On May 28, 12:53 pm, Ryan Bates <r...@railscasts.com>
wrote:
> 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-m...
>
> Regards,
>
> Ryan
>
> On May 27, 2:26 am, Hongli Lai <hon...@phusion.nl> wrote:
>
> > On May 27, 2:57 am, cainlevy <cainl...@gmail.com> wrote:
>
> > > After playing with a reimagined version of attr_accessible/
> > > attr_protected in my plugin, I''m much happier with the
model-side
> > > filtering approach. I think it allows for more interesting and
useful
> > > defaults.
>
> > > Since this API is to live only as a plugin for a bit,
I''m unsure
> > > whether this thread is the place to continue discussion? I think
that
> > > Xavier''s improved documentation is probably all that
can/should be
> > > done to core at this time. Anyone interested in playing with the
mass
> > > assignment API is welcome to contact me directly or through
GitHub.
>
> > Just publish the code. Real-world experience will teach us whether it
> > has any advantages over Hash#slice/Hash#except.

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:

attr_accessible :name, :first_name, :context => :default
attr_accessible :can_login, :context => :support
attr_accessible :can_login, :is_admin, :context => :admin

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.

The plugin is at http://github.com/DefV/context_assignment/tree/ 
master . Let me know what you think.

regards,
Jan De Poorter


On 28 May 2009, at 20:32, Kevin Whinnery wrote:
>
> 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.
>
> On May 28, 12:53 pm, Ryan Bates <r...@railscasts.com> wrote:
>> 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-m
>> ...
>>
>> Regards,
>>
>> Ryan
>>
>> On May 27, 2:26 am, Hongli Lai <hon...@phusion.nl> wrote:
>>
>>> On May 27, 2:57 am, cainlevy <cainl...@gmail.com> wrote:
>>
>>>> After playing with a reimagined version of attr_accessible/
>>>> attr_protected in my plugin, I''m much happier with the
model-side
>>>> filtering approach. I think it allows for more interesting and
>>>> useful
>>>> defaults.
>>
>>>> Since this API is to live only as a plugin for a bit,
I''m unsure
>>>> whether this thread is the place to continue discussion? I
think
>>>> that
>>>> Xavier''s improved documentation is probably all that
can/should be
>>>> done to core at this time. Anyone interested in playing with
the
>>>> mass
>>>> assignment API is welcome to contact me directly or through
GitHub.
>>
>>> Just publish the code. Real-world experience will teach us whether
>>> it
>>> has any advantages over Hash#slice/Hash#except.
>
> >

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

    # ...
  end

after:
  def create
    @comment = @commentable.comments.build(params[:reply])
    @comment.assign_attributes :user => current_user, :request =>
request

    # ...
  end

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)



Hope this make sense.

Cheers,
Luca.

--
blog: www.lucaguidi.com

Hi Luca,

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!

Ryan

On Jun 1, 4:23 am, Luca Guidi <guidi.l...@gmail.com>
wrote:
> 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
>
>     # ...
>   end
>
> after:
>   def create
>     @comment = @commentable.comments.build(params[:reply])
>     @comment.assign_attributes :user => current_user, :request =>
> request
>
>     # ...
>   end
>
> 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)
>
> Hope this make sense.
>
> Cheers,
> Luca.
>
> --
> blog:www.lucaguidi.com

I implemented this as a plugin. Let me know what you think:
http://github.com/ryanb/trusted-params/tree/master

Regards,

Ryan

On Jun 1, 8:09 am, Ryan Bates <r...@railscasts.com>
wrote:
> Hi Luca,
>
> 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!
>
> Ryan
>
> On Jun 1, 4:23 am, Luca Guidi <guidi.l...@gmail.com> wrote:
>
>
>
> > 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
>
> >     # ...
> >   end
>
> > after:
> >   def create
> >     @comment = @commentable.comments.build(params[:reply])
> >     @comment.assign_attributes :user => current_user, :request
=>
> > request
>
> >     # ...
> >   end
>
> > 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)
>
> > Hope this make sense.
>
> > Cheers,
> > Luca.
>
> > --
> > blog:www.lucaguidi.com

Hi Ryan, glad you like my proposal.

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

   Member.accessible_attributes # => [ ''posts_attributes'' ]

Cheers,
Luca
-- 
blog: www.lucaguidi.com

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.

On Thu, Jun 11, 2009 at 8:36 PM, Chrisis<dekkerchrisys@gmail.com> wrote:
> 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?
Imagine a multi-account invoicing application that has a customer
selector in the form for invoice creation.

That customer_id has to be protected to prevent users from injecting
customer_ids belonging to other accounts. Yet it belongs to the form.