Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add support for strong_parameters #1731

Closed
garysweaver opened this Issue Oct 18, 2012 · 60 comments

Comments

Projects
None yet
6 participants
Contributor

garysweaver commented Oct 18, 2012

as noted here: https://groups.google.com/forum/?fromgroups=#!topic/activeadmin/XD3W9QNbB8I

patch: https://gist.github.com/3718571
patch noting can require patch file(s) at bottom of initializer: https://gist.github.com/3907216

Tested (lightly) and now I can edit records again. Proposing temporarily adding optional support as DSL method until formal support can be added. Will submit a pull in a minute for consideration.

garysweaver added a commit to garysweaver/active_admin that referenced this issue Oct 18, 2012

Contributor

garysweaver commented Oct 18, 2012

Wrote a note to @dpmccabe in the forum to ensure he is ok with this.

garysweaver added a commit to garysweaver/active_admin that referenced this issue Oct 18, 2012

mrbrdo commented Oct 18, 2012

This fix is not ok when using virtual attributes. One example is when using a User model with has_secure_password.
It would be better to permit allow everything.

Either this or issue #1732 should be closed I'd say, they are both about the same issue, and @mrbrdo has an interesting point here. Leaving it here we risk failing to discus it.

However, are you sure strong_parameters doesn't play nicely with virtual attributes? I believe that virtual attributes can be easily be added to any array of allowed attributes.

mrbrdo commented Oct 19, 2012

It's not a problem with strong parameters, only a problem with this patch... This patch only permits real attributes of the model. So for example if you use has_secure_password and add a password and password_confirmation input in ActiveAdmin, it will not work when using this patch, because this patch will only permit password_digest (which is the attribute that is in DB). So this patch is no good, I suggest to patch so that everything is allowed.

Ah, yes that is true. I'd say that's a great suggestion. .permit! allows all attributes. I've done a quick change to the older gist, pay attention to rule 25 and 37, I believe this should allow all attributes.

garysweaver added a commit to garysweaver/active_admin that referenced this issue Oct 19, 2012

mrbrdo commented Oct 19, 2012

BUT because ActiveAdmin can also be used to provide an interface to users where you still want to sanitize user input (meaning that not always you want to allow all attributes to be mass assigned), I think this kind of feature should not be enabled by default, but I think it makes sense to provide such an option inside the gem itself because most of the time you do actually want to allow everything to be assigned. But you also need documentation on how to actually use strong_parameters with AA if you want to only permit certain attributes.

I will test your change, because I have a case where I have virtual attributes so it will be easy to test. But I also had problems with a model AdminUser, which caused problems in "active_admin_config.resource_name.constantize" (it returns a constant named "Admin User" - note the space! so it does not work). I will try to come up with a good solution, right now I just gsub(/\s+/, "")

garysweaver added a commit to garysweaver/active_admin that referenced this issue Oct 19, 2012

#1731 using Jasper's changes for strong_parameters patch. readding re…
…moval of id, created_at, updated_at from @columns but using Jasper's changes to permit! without columns specified

@mrbrdo Have a look at tableize for the latter manner, that might help.

For the first point, I think simply allowing all params might be a good first step for now, optionally of course. We can always add more complexity later.

mrbrdo commented Oct 19, 2012

Sure, I just mean it should be configurable and disabled by default, I think best would be an option in initializers/active_admin in the setup block, already there but commented by default. Something like

# config.strong_parameters_permit_all = true

mrbrdo commented Oct 19, 2012

I suggest to change the implementation like this:
https://gist.github.com/3918135
Much better to use resource_class instead of resource_name in this case.

Also note in your gist you have a typo "extention" - it should be "extension" (folder name aswell). But this is probably not relevant if you will make a pull request to merge into the gem anyway.
I think this would be good enough, and also with a configuration option like I said above, to automatically include the module in all AA controllers when enabled.

garysweaver added a commit to garysweaver/active_admin that referenced this issue Oct 19, 2012

#1731 incorporating changes from mrbrdo and implementing suggestion t…
…o make permitting all configurable, leaving in jasperkennis's long controller name fix.
Contributor

garysweaver commented Oct 19, 2012

@mrbrdo We're not using the extention thing. Take a look at the patch. Thanks for your help!

Contributor

garysweaver commented Oct 19, 2012

@jasperkennis I left in your long classname fix and @mrbrdo I made it configurable as you said and tested. By default config.strong_parameters_permit_all is false if commented out, but by default the generated config makes it true and uncommented, so people will see it but won't have to turn it on to get it to work. This way it is the user's local config that specifies that it should permit all or not.

Contributor

garysweaver commented Oct 19, 2012

To clarify, with this patch you need to add support_strong_parameters to your controller definition and if you have existing config, you have to add this to your config:

# == Strong Parameters options
#
# Permit all attributes. This is the only option currently
#
config.strong_parameters_permit_all = true

garysweaver added a commit to garysweaver/active_admin that referenced this issue Oct 19, 2012

#1731 clarifying strong parameters usage for existing users that need…
… to add config.strong_parameters_permit_all = true to config

Hehe, thanks @mrbrdo , I hadn't noticed that:p

I'm not sure if I understand correctly, what will happen if a user doesn't use the strong_parameters gem?

mrbrdo commented Oct 19, 2012

@garysweaver If you need to call support_strong_parameters in the active admin model, then I don't see the need for the configuration option.
When I was talking about the config option I thought it would magically include the module in all active admin models, so no further code modification would be necessary. But I like your solution better I think.

However I think you should remove the config option then, because it seems to me like a violation of DRY and it doesn't seem necessary. If you need to check if the user is using strong parameters, it would be better to just check if the StrongParameters module is defined. Or if you are concerned about loading unnecessary code when user will not use it I don't think it's a concern, but you could always lazy load the module the first time support_strong_parameters is called.

I also would prefer permit_all_parameters or permit_all_strong_parameters instead of support_strong_parameters, since I think that name is misleading (all ActionControllers support strong parameters once you are using the gem, it's just that no params are permitted by default). I think permit_all_strong_parameters is more clear about what is going on.

Contributor

garysweaver commented Oct 19, 2012

@mrbrdo how about just permit_all?

mrbrdo commented Oct 19, 2012

Yes permit_all sounds very good.

I also suggest you change (like I had in my gist)

@instance_name = active_admin_config.resource_name.gsub(/(.)([A-Z])/,'\1_\2').downcase

to

@instance_name = active_admin_config.resource_class.name.underscore.to_sym

(just seems more solid to me)
Also since I added .to_sym here, you can remove it from @instance_name.to_sym further down in the code (so just do @instance_name). In the strings you are using it in, it's automatically converted to string so no worries there.

mrbrdo commented Oct 19, 2012

I like permit_all_parameters the most.

garysweaver added a commit to garysweaver/active_admin that referenced this issue Oct 19, 2012

Contributor

garysweaver commented Oct 19, 2012

Ok will fix.

garysweaver added a commit to garysweaver/active_admin that referenced this issue Oct 19, 2012

Contributor

garysweaver commented Oct 19, 2012

@jasperkennis I tested and works in a new Rails project with clean gemset and ruby 1.9.3p194 with and without the strong_parameters gem.

To summarize, there is now no configuration change from the current version in master and you use permit_all_parameters in the controller to enable strong_parameters support, which includes ActiveAdmin::StrongParametersPatch that overrides methods to permit! the instance prior to new/update_attributes.

garysweaver added a commit to garysweaver/active_admin that referenced this issue Oct 19, 2012

mrbrdo commented Oct 19, 2012

Looks good to me :) Good job.

Contributor

garysweaver commented Oct 19, 2012

@mrbrdo :) Thanks, but that was all @dpmccabe, @jasperkennis, and you. Now I guess the question is whether it is good enough to be incorporated.

mrbrdo commented Oct 19, 2012

I think you need to write tests for it to be merged.

Contributor

garysweaver commented Oct 19, 2012

Yep. Was looking at that earlier. If you want to help, let me know. Assuming maybe a mock to ensure that permit! is called after that method is used would be good enough.

Contributor

garysweaver commented Oct 25, 2012

Added ability to disable strong_parameters via configuration so that it can be included as a runtime_dependency perhaps in the AA gemspec and can be included in the test scope in Gemfile:
rails/strong_parameters#55

Will try to get test added to AA as part of the AA patch soon.

Contributor

garysweaver commented Oct 25, 2012

And... request denied by @dhh.

mrbrdo commented Oct 25, 2012

For valid reason I think. Like he said you can just check if StrongParameters constant is defined.
Add it as dependency only for testing but not for the gem itself. You don't even need runtime_dependency.

You can use add_development_dependency (see http://docs.rubygems.org/read/chapter/20).

mrbrdo commented Oct 25, 2012

Can you squash your commits together so this can be reviewed and merged easier? And also add tests.

Contributor

garysweaver commented Oct 25, 2012

Jan, the reason I was asking for that was to disable the gem for all tests except those that I wanted to write, because otherwise SP would be enabled for everything and break all other tests, so I didn't need it to check if SP is defined or not. That wasn't the problem. Basically, @dhh doesn't want to make it be disabled at runtime, but that is fine and I understand why dhh didn't accept it, also, but considering that mass assignment security has a similar setting (for whitelisting) it seems that an on/off switch for strong_parameters is not far from that, but ... whatever. I'll let that be.

It looks like the appraisal gem is a good way around the being able to test with and without SP, but will have to find time to do that now.

I will rebase and squash also, but that doesn't clean up the comments in issue pages like this, only the pull request page, just so you know. It is really better to do that locally vs. change history in a pushed repo, but I totally get the readability thing. I wish that github would solve that though and provide a way to view as if it were squashed. Again- not a big deal.

In short- I've been working on this, but if you want to fork off my repo's branch where I did this, write the tests, and do a pull request, then I'll merge that and it will be in this request. That might be faster if you have time.

Contributor

garysweaver commented Oct 25, 2012

This is the one that was mentioned on the core list to assist with using SP and not using SP (as two appraisals) in tests via https://github.com/thoughtbot/appraisal

Contributor

garysweaver commented Oct 25, 2012

Also, good point about development dependency, but it would still (I think) break a lot of existing AA tests unless we used Appraisals. I might be wrong though. I've not spend much time on this issue yet.

Contributor

garysweaver commented Oct 25, 2012

One last thing, Pedro Nascimento said on the core list:

Following AA's DSL, you could have something like this for allowing everything:

allowed_params do |params|
  params.permit(User.column_names.map(&:to_sym)
end

If a allowed_params call is not defined in AA's controller, assume
everything is allowed or nothing is allowed, up to what's best for
everyone.

We could do that, but I also think it would be shorter to have nothing at all and permit all or use the permit_all_parameters method name we came up with before to cause the override/call to permit on everything. At this point, I'm starting not to care as much about it as long as there is some solution that will be accepted. I don't have a good feeling that the existing patch is a long term fix, and I see the benefit of Pedro's longer-term thinking, but I also think it may be more code than necessary as I would see people having to put things into this block as well as migrations, etc. and then AA is not nearly as easy to use for those using SP/Rails 4.

mrbrdo commented Oct 25, 2012

First of all:

params.permit(User.column_names.map(&:to_sym)

Is not good, because it doesn't permit virtual attributes and so on. We've been through this. The current solution works fine, what's the problem with it?

Secondly, you don't need to permit anything in tests because strong parameters are not enabled until both this criteria are met:

  • model includes

    include ActiveModel::ForbiddenAttributesProtection
    
  • the hash you pass into create/update_attributes etc. is properly marked. This marking by default only happens when using params hash available in controllers. So when you pass a normal Hash object strong parameters does nothing.

So your arguments are void I'm afraid, there is no problem in testing this.

Contributor

garysweaver commented Oct 25, 2012

Jan,

Pedro's point was not so much the actual code in the block, but that we'd provide the ability for the user to set params via the block, and it would be the user's responsibility to add those virtual attributes in the block (that code would be copied in by the user and altered). I am on your side though. I think that is overkill for now and we should use the existing patch and add tests.

That is a very good point about the model. I think that will eliminate the need for the appraisals gem, except that if we were to use the appraisals gem then we could test everything with SP if we had something (like a railtie or initializer) in the test that would include that on every model and call the method on every controller. But, just to test a few tests, then we could just use a different model that includes ActiveModel::ForbiddenAttributesProtection and its controller that uses the permit_all_parameters method for testing the patch. Thanks! Good catch. I had forgotten about that.

You didn't need to void my arguments. ;) But thanks for the advice on the models needing that, as that what will make it easy, and now I understand why you were just saying we needed a development_dependency in the gemspec (although could include in Gemfile in this case, I think).

Thanks again,

Gary

Contributor

latortuga commented Oct 25, 2012

Just want to jump in real quick here: InheritedResources has an open PR that adds support for StrongParamters in a pretty simple way, basically you just add a method resource_name_params to your controller block and it just works. Perhaps we should just wait until it has been merged and then update the dependency version?

mrbrdo commented Oct 25, 2012

Sounds reasonable. I can't go through that patch right now, but does it support whitelisting everything? Meaning including arbitrary virtual attributes that are not real column names? By support I mean giving the option to whitelist everything, no matter what it is (without specifying what to whitelist exactly).

Contributor

garysweaver commented Oct 26, 2012

Jan, it looks like it doesn't do that by default, although I don't think it would be difficult to patch.

Drew, take a look at @mrbrdo's comment way above about has_secure_password. That is what he is talking about. AA has some functionality to support params coming in that may not be gleanable from the resource, and perhaps there needs to be a way to have AA either permit everything as Jan said, or maybe easily define additional params to tack on to what it perceives the params should be?

If everyone is ok with that, let's close this ticket and create another to just update the InheritedResources dependency and add something to the AA readme that states that you just need to add that to the controller block for SP.

Contributor

garysweaver commented Oct 26, 2012

@latortuga btw - Thanks!

mrbrdo commented Oct 26, 2012

Honestly, I still want a one-liner for doing this. I think it's common enough in AA that you know exactly who has access and you trust them that they won't hack anything (or you give them permission to everything anyway).
I don't think it should be on by default, but I think it should be there and very simple to turn on. Something like we said "permit_all".
Besides calling permit_all it would be nice to additionally offer a configuration option that would automatically "do" permit_all for all AA controllers. Because in most of the cases in my projects I just want to permit everything for all the models in AA (for reasons I described above).

So I wouldn't close this issue, although it would make sense to wait for the patch to InheritedResources if that is gonna get accepted any time soon.

Contributor

garysweaver commented Oct 26, 2012

Looking at the IR pull request josevalim/inherited_resources#237 I don't see any calls to permit in there, so I don't think that it is adding support for SP? I've commented there to try to get some assistance.

Contributor

garysweaver commented Oct 26, 2012

Ok, just saw: josevalim/inherited_resources#236 I think that defining the resource_params method and having it permit all params similar to what Drew said may be the current IR way of doing it. Drew, feel free to correct me.

Contributor

garysweaver commented Oct 26, 2012

Looks like this is all that is needed for you, Jan? (without a patch):

controller do
  def resource_params
    return [] if request.get?
    [ params[active_admin_config.resource_class.name.underscore.to_sym].permit! ]
  end
end

That is long-winded, but it keeps us from overriding create and update, etc.

That doesn't even require waiting on anything from IR. It works in current AA, 0.5.0.

This probably works, too:

controller do
  def resource_params
    return [] if request.get?
    [ params.require(:name_of_model).permit(:each,:param,:goes,:here,:if,:you,:want) ]
  end
end

This looks like it might hold over people using AA with SP until SP is updated for Rails 4.

Contributor

garysweaver commented Oct 26, 2012

Jan, also- I understand that you'd rather this be done as a patch so that it can be a short statement instead of all the code, but I think this is less invasive.

Contributor

garysweaver commented Oct 26, 2012

Let me know if I can close this.

mrbrdo commented Oct 26, 2012

I think it would still be useful to have a configuration option (disabled by default) that would by default add that method to all AA controllers. Then you can overwrite it if you need finer control over the params in some of your controllers.
I say this would be useful only because we are talking about AA, which at least I usually use for only specific admins that I know and can trust.

I don't think we need a shorter way of doing it in each controller separately, but I do think an option like I propose would be very useful in AA. It means writing 1 line instead of 6 lines * controllers_count. Which for me is a big saving.

Contributor

garysweaver commented Oct 26, 2012

Jan, ok, I'll see what I can do.

Contributor

garysweaver commented Oct 26, 2012

Having some problems with the tests, and even if the tests didn't fail, I can't leave SP in the gemspec even as a development dependency because then the required step (according to CONTRIBUTING.md) of:

rake test:major_supported_rails

fails because it is supposed to support Rails 3.0.12:

Bundler could not find compatible versions for gem "railties":
  In Gemfile:
    strong_parameters (>= 0.1.4) ruby depends on
      railties (>= 3.2.0) ruby

    rails (= 3.0.12) ruby depends on
      railties (3.0.12)

I looked at the appraisals gem but it failed to install, so created an issue for that. Stuck for now, so I am leaving as-is.

mrbrdo commented Oct 26, 2012

@garysweaver would it be possible to leave out the gem from the gemspec entirely, and just require it in the test and fail the test if it can't be required? Just a thought, don't know if it would work.
I guess it would make sense to otherwise bump the rails version, people with lower rails versions should just use the older version of AA. Since Rails 4 is coming soon and rails 3.0 will not be supported anymore anyway.

PS: I see now that appraisals enables something like that, so I guess that's your best bet then.

Contributor

garysweaver commented Oct 26, 2012

@mrbrdo Yep, I think the solution probably involves AA switching to using appraisals. Right now it is using script/use_rails in the AA gem to switch between rails versions. Then- I wonder whether we could just have it run a script that switches out some files so that all models contain:

include ::ActiveModel::ForbiddenAttributesProtection

(or maybe via railtie, or maybe via a class_eval at some point) and add the line to the controller configs in spec/rails/ (not sure how else to do that)... Then everything would be tested the same way. There is probably a better way though.

mrbrdo commented Oct 26, 2012

Sounds like a lot of work. Would be nice to have a fork until that without support for rails < 3.2.0. But definitely before Rails 4 release.

Contributor

latortuga commented Oct 26, 2012

The intention of StrongParameter is to force developers to whitelist attributes when using mass-assignment from controllers. AA is used, for the most part, as an administration interface; therefore, the sane default would be to allow mass-assignment everywhere. This is the current behavior and IMO it should continue to be the default. @mrbrdo proposed creating a config variable, I think something like this would be good:

# unset/false - call permit! on resource_params if the strong_parameters gem can be found
# true - don't call anything, the strong_parameters gem will cause exceptions to be raised
#        without resource_params implemented with require/permit
config.enforce_strong_parameters

If you want parameter filtering at the controller level in AA, you can override the resource_params method on a controller-by-controller basis. We could add a DSL method that would allow you to do something like:

ActiveAdmin.register Post do
  # Allow static list
  permitted_params :a, :b, :c

  # Conditionally control access at runtime
  permitted_params do
    [:a, :b, :c]
  end
end

I've read the entire thread so I'm aware of the virtual attributes concerns.

Regarding running the tests, isn't there a way to use separate bundles for different test runs?

mrbrdo commented Oct 26, 2012

I think overwriting resource_params to override on per-controller-basis is short enough (and also will not be used so much), so I don't think an additional DSL is necessary as much. It's also not a big hack or anything so I think it's okay.

For configuration variable I think it is still better to leave it off by default, and rather very clearly point it out in the docs. Because we want to protect new users from thinking they are safe with the defaults but they really are not if this is enabled (so anyone who enables the option should know what it means). If it's enabled by default we risk people not knowing the security implications. It's the same reason why attr_accessible went from blacklisting approach to whitelisting approach in the past.

Contributor

latortuga commented Oct 26, 2012

The more I think about it the more I agree. Rails default will be to use strong_parameters in all controllers so we should follow suit.

Contributor

garysweaver commented Oct 26, 2012

@latortuga with the appraisals gem there is an Appraisals file where you can define those, but with AA currently it is using a custom rails version switching that can look at RAILS env variable, etc. I think that should be a separate ticket to switch that because it will affect enough on its own, it would seem.

@mrbrdo @latortuga I think if we add #permitted_params :add, :your, :attributes, :here or similar in the controller generator and add it to the docs that might be enough without additional controller config similar to what Jan is saying. It actually wasn't bad adding a new config option, though, so whatever everyone wants is fine.

The main thing was that writing tests for this as it is now is non-trivial.

Maybe it would make since to have a rails3_2-active_admin-with-strong_parameters (renamed) fork of AA until Rails 4 is released. That way it could have its own gemspec, go into rubygems.org. I don't love the idea at all, but the alternative is to have a branch of AA just for rails 3.2 and SP that would have to be referred to via a git url in the Gemfile like:

gem 'activeadmin', git: 'https://github.com/garysweaver/active_admin.git', branch: 'fix-1731-add-support-for-strong-parameters'

Something like that. ... or we have to work out the test nastiness to get it into master.

mrbrdo commented Oct 26, 2012

I think both options are acceptable, but I like the config option more just because in 90% of my AA I want to premit everything. We should keep in mind we are not normal Rails controllers, we are AA :-)

Contributor

garysweaver commented Oct 26, 2012

Ok, starting to implement.

Contributor

garysweaver commented Oct 26, 2012

Note: what I said above was bogus. The Gemfile had a way to specify gems for different rails version and I missed it. :( Doesn't make things that much less complex though, so I still think the new branch is the way to go.

Contributor

garysweaver commented Oct 29, 2012

Ok, if someone has time to add tests for this implementation and figure out whether there is a working way to use the permitted_params without having to permit! and discard pairs manually, let me know and I can give you rights to this repo, or feel free to copy my work and just give me mention in the commit if you use any of it. Testing with our app and this works, but took a look through tests and it looked like it was going to take longer than the time I had allotted for it. Thanks!
https://github.com/garysweaver/active_admin/tree/strong_parameters_support
garysweaver/active_admin@9dc6644

brendon commented Mar 27, 2013

Seems that now if you just update to the latest inherited_resources gem you can use:

def permitted_params
  params.permit(:blog => [:name, :description])
end

in your controller block.

Contributor

garysweaver commented Mar 27, 2013

@brendon Thanks! If it works, I'm ok with closing this. Other solutions along with these here for those that can't update for whatever reason: http://stackoverflow.com/q/13091011/178651

Contributor

garysweaver commented Apr 11, 2013

Just closed and killed the branch of the related patch provided earlier so as not to be a point of confusion. Can we close this, too?

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