Skip to content

Conversation

@guilleiguaran
Copy link
Member

This integrate strong_parameters plugin in Rails core and remove attr_accessible/attr_protected.

@guilleiguaran
Copy link
Member Author

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is not better call this exception ForbiddenAttrbibutesError or something like this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, sounds better, we should also change it in the gem to keep both synced

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Note to myself: This change is pending on gem

@rafaelfranca
Copy link
Member

I think we will need a hidden option to disable strong_parameters that should be set by the attr_accessible plugin.

Also guides and docs should be updated/written.

@josevalim
Copy link
Contributor

Thanks @guilleiguaran for the PR and @rafaelfranca for the review. I want to take a look at this as well, but I will be able to do it just later this week!

@spastorino
Copy link
Contributor

Agree with @rafaelfranca about disabling strong_parameters, should be a hidden option to do that to allow attr_accessible plugin to work without strong_parameters.

@spastorino
Copy link
Contributor

What if people attr_accessible or attr_protected in 4.0 without the plugin?. Couldn't we leave those methods and raise an error saying "Add the plugin to the Gemfile or move to strong_parameters. @josevalim what do you think?

@rafaelfranca
Copy link
Member

Good point. 👍 for this

Rafael Mendonça França
On Aug 4, 2012 11:15 AM, "Santiago Pastorino" <
[email protected]>
wrote:

What if people attr_accessible or attr_protected in 4.0 without the
plugin?. Couldn't we leave those methods and raise an error saying "Add the
plugin to the Gemfile or move to strong_parameters. @josevalim what do you
think?


Reply to this email directly or view it on GitHub:
#7251 (comment)

@spastorino
Copy link
Contributor

I'd also squash some commits, like bfac9c4 25c2fe8 and 678137e in the future this will make much easier to understand why the code was added

@spastorino
Copy link
Contributor

Besides from all that is looking great ❤️ ❤️ ❤️

@guilleiguaran
Copy link
Member Author

@spastorino @josevalim @rafaelfranca thanks for you feedback guys, I'm starting to change the code with your suggestions

@homakov
Copy link
Contributor

homakov commented Aug 13, 2012

❤️

@kirs
Copy link
Member

kirs commented Aug 13, 2012

Could you explain how does the strong_parameters work or maybe write some docs?

@dmathieu
Copy link
Contributor

@kirs: there is documentation in the strong parameters gem.
But indeed, an update of the official documentation will be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better if ActionController::Parameters wrapped a Hash rather than inherited from ActiveSupport::HashWithIndifferentAccess? For example CookieJar used to inherit from Hash but was changed in 0ca69ca. @tenderlove, what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca can you work on this, I'm no having much luck with this and @spastorino won't have time to check it on the next weeks

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though my opinion is to do so, I think the final decision was to not change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember some discussion about this one but I didn't read yet. I'll try to read now.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca it'll be great you post a link :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuraga I can't. It is a private discussion.

@guilleiguaran
Copy link
Member Author

I've extracted ActiveModel::MassAssignmentSecurity and ActiveRecord/AC::ParamsWrapper integration to the protected_attributes plugin:

https://github.com/rails/protected_attributes

Right now everything is working fine except integration with AR deprecated finders (related tests), can someone help me fixing it?

/cc @jonleighton @tenderlove

@guilleiguaran
Copy link
Member Author

The plugin is ready and all tests passing, this is ready now

/cc @dhh

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're dealing with parameters that emulate a hash, it seems to be more appropriate to use KeyError, not? (I know it's a subclass, just thought it might be a better error to inherit from)

>> [].fetch 1
IndexError: index 1 outside of array bounds: 0...0

>> {}.fetch :a
KeyError: key not found: :a

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree 100% on this, I will change it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

This will be moved out to protected_attributes gem
…Hash instead of monkey patch permitted? method in regular hashes
dhh pushed a commit that referenced this pull request Sep 18, 2012
Integrate strong_parameters in Rails 4
@dhh dhh merged commit c49d959 into master Sep 18, 2012
@dapi
Copy link

dapi commented Sep 24, 2012

Strong Prameters does not solve the problem of checking parameters in Rails. The problem is model-based validation.

Require or not require, permit or not permit, validate and how to validate - these are questions of the Form, not of a model and not of a controller.

Model-based validation must be like SQL constraints - simple, basic and unconditional. It's about solidity of data, it's about consistency, not roles or permissions.

And params are form-specific, not of controller. They can be used with different models, presenters and controllers.

One form can require :password, another one require :email, another one require :full_name and permit to another field.

Another problem with StrongParameters - they ara introduce duplication of responsibilities:

params.require(:user) # in controller

and

validates :user, :presence => true # model

What is difference between these approaches?

Resolution

Checking parameters (validation, requirements etc) must be extracted to the dedicated class.

@steveklabnik
Copy link
Member

Basically nobody on the rails team but me is obsessive enough to read everything in comments on commits. The official place to talk about stuff like this is the rails-core list.

@dhh
Copy link
Member

dhh commented Sep 24, 2012

I have many forms pointing to the same controllers. Desktop and mobile web views, APIs, etc. This verification also cannot be trusted to the view as you don't want to tamper with it.

I wouldn't bother with the validates :user, :presence check if you feel like you're duplicating yourself.

On Sep 24, 2012, at 10:25 AM, Danil Pismenny wrote:

Strong Prameters does not solve the problem of checking parameters in Rails. The problem is model-based validation.

Require or not require, permit or not permit, validate and how to validate - these are questions of the Form, not of a model and not of a controller.

Model-based validation must be like SQL constraints - simple, basic and unconditional. It's about solidity of data, it's about consistency, not roles or permissions.

And params are form-specific, not of controller. They can be used with different models, presenters and controllers.

One form can require :password, another one require :email, another one require :full_name and permit to another field.

Another problem with StrongParameters - they ara introduce duplication of responsibilities:

params.require(:user) # in controller

and

validates :user, :presence => true # model

What is difference between these approaches?

Resolution

Checking parameters (validation, requirements etc) must be extracted to the dedicated class.


Reply to this email directly or view it on GitHub.

David Heinemeier Hansson

@dapi
Copy link

dapi commented Sep 24, 2012

I'm talking not about view's.

In real life Forms contains two parts.
First is a view (it's Formastic or so)
Second - some server-side object that requires and validates parameters (it
is missed in Rails)

The list of required and permitted parameters is linked to forms, not to
actions of controllers and not to models.

Example.

  1. RegistrationForm:
    = f.input :login
    = f.input :password
    = f.input :password_confirm

  2. Profile update form:
    = f.input :login
    = f.input :email
    = f.input :name

  3. Form for update password:
    = f.input :password
    = f.input :password_confirm

  4. Form of editing users for admins:
    = f.input :login
    = ..etc all parameters

  5. Form for second-lever users:
    = f.input :login
    = f.input :specific_field

In every form specific list of fields, specific list of required and
permitted fields.

In controller we must have something like this:

RegistrationFormVlidator.new.validate params
ProfileFormValidator.new.validate params
PasswordFormVlidator.new.validate params
etc

Every FormValidator requires and permits and also validates parameters in
their own way.

Yes we can have default UserFormValidator or specific role of current_user
to this FormValidator.

The key question is validation, not just requires and permits. And
extraction of these functions from controller to a separate class.

On Mon, Sep 24, 2012 at 8:14 PM, David Heinemeier Hansson <
[email protected]> wrote:

I have many forms pointing to the same controllers. Desktop and mobile web
views, APIs, etc. This verification also cannot be trusted to the view as
you don't want to tamper with it.

I wouldn't bother with the validates :user, :presence check if you feel
like you're duplicating yourself.

On Sep 24, 2012, at 10:25 AM, Danil Pismenny wrote:

Strong Prameters does not solve the problem of checking parameters in
Rails. The problem is model-based validation.

Require or not require, permit or not permit, validate and how to
validate - these are questions of the Form, not of a model and not of a
controller.

Model-based validation must be like SQL constraints - simple, basic and
unconditional. It's about solidity of data, it's about consistency, not
roles or permissions.

And params are form-specific, not of controller. They can be used with
different models, presenters and controllers.

One form can require :password, another one require :email, another one
require :full_name and permit to another field.

Another problem with StrongParameters - they ara introduce duplication
of responsibilities:

params.require(:user) # in controller

and

validates :user, :presence => true # model

What is difference between these approaches?

Resolution

Checking parameters (validation, requirements etc) must be extracted to
the dedicated class.

Reply to this email directly or view it on GitHub.

David Heinemeier Hansson

Reply to this email directly or view it on GitHubhttps://github.com//pull/7251#issuecomment-8824545.

äÁÎÉÌ ðÉÓØÍÅÎÎÙÊ
ôÅÈÎÉÞÅÓËÉÊ ÄÉÒÅËÔÏÒ


[email protected] [email protected] | www.investcafe.ru
éÎ×ÅÓÔËÁÆÅ | îÅÚÁ×ÉÓÉÍÏÅ ÁÎÁÌÉÔÉÞÅÓËÏÅ ÁÇÅÎÔÓÔ×Ï
íÏÂ.: +7 903 389-12-28
Skype: danil_pismenny

@dhh
Copy link
Member

dhh commented Sep 24, 2012

That abstraction does not offer any compelling benefit to me. But feel free to wrap it up like that in your application. You can wrap these form validators around the require/permit setup.

On Sep 24, 2012, at 11:56 AM, Danil Pismenny wrote:

I'm talking not about view's.

In real life Forms contains two parts.
First is a view (it's Formastic or so)
Second - some server-side object that requires and validates parameters (it
is missed in Rails)

The list of required and permitted parameters is linked to forms, not to
actions of controllers and not to models.

Example.

  1. RegistrationForm:
    = f.input :login
    = f.input :password
    = f.input :password_confirm

  2. Profile update form:
    = f.input :login
    = f.input :email
    = f.input :name

  3. Form for update password:
    = f.input :password
    = f.input :password_confirm

  4. Form of editing users for admins:
    = f.input :login
    = ..etc all parameters

  5. Form for second-lever users:
    = f.input :login
    = f.input :specific_field

In every form specific list of fields, specific list of required and
permitted fields.

In controller we must have something like this:

RegistrationFormVlidator.new.validate params
ProfileFormValidator.new.validate params
PasswordFormVlidator.new.validate params
etc

Every FormValidator requires and permits and also validates parameters in
their own way.

Yes we can have default UserFormValidator or specific role of current_user
to this FormValidator.

The key question is validation, not just requires and permits. And
extraction of these functions from controller to a separate class.

On Mon, Sep 24, 2012 at 8:14 PM, David Heinemeier Hansson <
[email protected]> wrote:

I have many forms pointing to the same controllers. Desktop and mobile web
views, APIs, etc. This verification also cannot be trusted to the view as
you don't want to tamper with it.

I wouldn't bother with the validates :user, :presence check if you feel
like you're duplicating yourself.

On Sep 24, 2012, at 10:25 AM, Danil Pismenny wrote:

Strong Prameters does not solve the problem of checking parameters in
Rails. The problem is model-based validation.

Require or not require, permit or not permit, validate and how to
validate - these are questions of the Form, not of a model and not of a
controller.

Model-based validation must be like SQL constraints - simple, basic and
unconditional. It's about solidity of data, it's about consistency, not
roles or permissions.

And params are form-specific, not of controller. They can be used with
different models, presenters and controllers.

One form can require :password, another one require :email, another one
require :full_name and permit to another field.

Another problem with StrongParameters - they ara introduce duplication
of responsibilities:

params.require(:user) # in controller

and

validates :user, :presence => true # model

What is difference between these approaches?

Resolution

Checking parameters (validation, requirements etc) must be extracted to
the dedicated class.

Reply to this email directly or view it on GitHub.

David Heinemeier Hansson

Reply to this email directly or view it on GitHubhttps://github.com//pull/7251#issuecomment-8824545.

äÁÎÉÌ ðÉÓØÍÅÎÎÙÊ
ôÅÈÎÉÞÅÓËÉÊ ÄÉÒÅËÔÏÒ


[email protected] [email protected] | www.investcafe.ru
éÎ×ÅÓÔËÁÆÅ | îÅÚÁ×ÉÓÉÍÏÅ ÁÎÁÌÉÔÉÞÅÓËÏÅ ÁÇÅÎÔÓÔ×Ï
íÏÂ.: +7 903 389-12-28
Skype: danil_pismenny

Reply to this email directly or view it on GitHub.

David Heinemeier Hansson

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.