-
Notifications
You must be signed in to change notification settings - Fork 22.1k
Integrate strong_parameters in Rails 4 #7251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
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. |
|
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! |
|
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. |
|
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? |
|
Good point. 👍 for this Rafael Mendonça França
|
|
Besides from all that is looking great ❤️ ❤️ ❤️ |
|
@spastorino @josevalim @rafaelfranca thanks for you feedback guys, I'm starting to change the code with your suggestions |
|
❤️ |
|
Could you explain how does the |
|
@kirs: there is documentation in the strong parameters gem. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
|
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? |
|
The plugin is ready and all tests passing, this is ready now /cc @dhh |
There was a problem hiding this comment.
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: :aThere was a problem hiding this comment.
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
There was a problem hiding this comment.
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
…psulate permissible params
…eference from abstract_unit
…if attributes list isn't given
…rameters protection
… plugin or new protection model
…ed than IndexError
Integrate strong_parameters in Rails 4
|
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 One form can require Another problem with StrongParameters - they ara introduce duplication of responsibilities: and What is difference between these approaches? ResolutionChecking parameters (validation, requirements etc) must be extracted to the dedicated class. |
|
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. |
|
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:
David Heinemeier Hansson |
|
I'm talking not about view's. In real life Forms contains two parts. The list of required and permitted parameters is linked to forms, not to Example.
In every form specific list of fields, specific list of required and In controller we must have something like this: RegistrationFormVlidator.new.validate params Every FormValidator requires and permits and also validates parameters in Yes we can have default UserFormValidator or specific role of current_user The key question is validation, not just requires and permits. And On Mon, Sep 24, 2012 at 8:14 PM, David Heinemeier Hansson <
äÁÎÉÌ ðÉÓØÍÅÎÎÙÊ [email protected] [email protected] | www.investcafe.ru |
|
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:
David Heinemeier Hansson |
This integrate strong_parameters plugin in Rails core and remove attr_accessible/attr_protected.