I'm all for using rails built-ins where possible.

Commenting here, since I was redirected from #282

I cannot say much about the proposal, since my knowledge about the chili sources is rather limited. So the following is just based to my experience after one or two days of working with safe_attributes.

I dislike the need to differentiate between general mass assignment and safe mass assignment. I think this is error-prone and should be avoided. Using attr_protected would fix that.

I like the optional :if parameter available for safe_attributes. This is something, that is not supported by the default Rails API - AFAIK. I am not sure, if it really worth it, to fully avoid having such an API. It seems to be rather useful. So we might end up with implement something similar to safe_attributes, just naming it attr_protected.

Since attr_protected is white list based -- just like safe_attributes -- plugin developers would still need to extend the core class when trying to add attributes to existing models. The current API of safe_attributes seems to be just as good as the one for attr_protected in this regard. Please correct me, if I'm wrong.

I agree that using core functionality is better than implementing things on our own version of the same thing. The rest needs to evaluated by somebody with deeper knowledge about the Chili code base.

Gregor Schmidt wrote:

I like the optional :if parameter available for safe_attributes. This is something, that is not supported by the default Rails API - AFAIK. I am not sure, if it really worth it, to fully avoid having such an API. It seems to be rather useful. So we might end up with implement something similar to safe_attributes, just naming it attr_protected.

This could be used as a security feature of the upcoming rewrite of the permissions system later on. It fits nicely into my train of thought here.

Since attr_protected is white list based -- just like safe_attributes -- plugin developers would still need to extend the core class when trying to add attributes to existing models. The current API of safe_attributes seems to be just as good as the one for attr_protected in this regard. Please correct me, if I'm wrong.

I personally like to have an API where I just can declare things. As far as I understood it, the save_attributes API would allow me to define it without having to monkey patch the class. Which is a good thing in my book.

Regarding the :if option:

You can also declare a setter for the field and have more control over it.

1  def isbn=(v)
2    write_attribute(:isbn, v) if @book.author == @user
3  end

Holger Just wrote:

I personally like to have an API where I just can declare things. As far as I understood it, the save_attributes API would allow me to define it without having to monkey patch the class. Which is a good thing in my book.

If you don't want to reopen a class we can define a class level method to add to the attr_protected/attr_accessible attributes. The main thing is I want to use the "protection" part of the code from Rails, instead of from a custom library. Otherwise we will have to maintain the library and all of the bugs along with it (like #282).

Eric Davis

Gregor just posted a link to Michael Koziarski's security recommendation to use attr_protected or attr_accessible to prevent a potential DoS weakness.

Unless there is a compelling reason that hasn't been stated yet, I think we should proceed on replacing safe_attributes.

Eric Davis

I like the idea of going with core rails stuff. Do we have a ticket for this already?

#655

E