inossidabile / protector

Comfortable (seriously) white-list security restrictions for models on a field level
MIT License
270 stars 31 forks source link

Built-in strong_parameters integration #12

Closed inossidabile closed 11 years ago

inossidabile commented 11 years ago

Protector should automatically permit available parameters for protected entities:

Foo.new(params)                         # ForbiddenAttributes
Foo.restrict!(current_user).new(params) # Assigns well if all of the attributes are allowed withion protection block

@cj @fdeschenes how do you deal with strong_parameters at the moment? Do you think this could be a reasonable feature of core or should I move it to separate gem?

fdeschenes commented 11 years ago

I'm not sure I like the idea of protector automatically handling strong parameters. Not all controller actions are made equal and in certain cases, I may not want to allow certain parameters from being written. How would that be handled by Protector? I'm assuming I'd need several if statements and use different can and cannot?

inossidabile commented 11 years ago

I believe what you really don't want is to spread your security meta across models AND controllers. Protector is all about security scope centralization. Why would you want to avoid that at the end?

If you insist I can make this thing optional though.

cj commented 11 years ago

I agree with @fdeschenes, also strong parameters doesn't throw an error if a field is passed someone doesn't have access to, it simply just ignores it and if that field is a requirement your activerecord validation will handle any errors. Having protector throw an error whenever a field is passed you don't have access to will be a nightmare and you'll have to write a bunch of extra code to filter out params.

inossidabile commented 11 years ago

No no, @cj, it's not how it works. The only thing this integration does is automatically permitting fields. So if a field was listed within, say, can :edit it will be automatically permitted for any incoming params. It will not throw exception or do anything. The goal is to NOT duplicate security logic at, first, Protector and then strong_parameters.

inossidabile commented 11 years ago

So here is the sample:

class Foo
  protect
    can :create, :foo
  end
end

For the case when params = {foo: 'test'} if I call Foo.new(params) strong_parameters will act and prevent the assignment. But if I call Foo.restrict!.new(params) it will be assigned properly cause I have already listed foo field as an allowed one.

cj commented 11 years ago

@inossidabile ah ok, I understand. You definitely need to add a config option to disable and a manual method to disable too, because if someone is just wanting to do a few things in rails console they will run into a bunch of issues.

inossidabile commented 11 years ago

What kind of issues are we talking about? I don't understand. Protector here acts as a thing that increases your abilities, not decreases it. I will make an option to turn this off, but really, I don't get it.

cj commented 11 years ago

I shouldn't have used the word issue, more like annoyance. Lets say you had a bunch of roles in the database with descriptions and you wanted to update the description, you'd probably just rails c into your app and do a quick Role.find(:id).update_attributes(description: 'new description'). You'd no longer be able to do that, so I think you should just add #unrestrict! or something similar. That way you'd be able to just do Role.unrestrict!.find(:id).update_attributes(description: 'new description').

fdeschenes commented 11 years ago

@inossidabile I love the idea and as I said, my only concern with this (and it probably shouldn't be one), is that I'll need to be very explicit when assigning what fields can be created and and updated. I guess an option to disable the strong parameter integration isn't really necessary.

fdeschenes commented 11 years ago

@cj Protector already has an unrestrict! method. But in the use case you're describing, that's a non-issue because a protection subject isn't explicitly set using restrict! and as far as I know, strong_parameters isn't used if you're manually creating a hash in the console.

cj commented 11 years ago

@fdeschenes thanks for clearing that up :) Then this gets a +1 from me then.