inossidabile / protector

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

multiple scope blocks not chaining #20

Closed anazar closed 11 years ago

anazar commented 11 years ago

I noted that if I do...

protect do |user|

    if user.client?
      scope { where(permission: :client) }
      scope { where(user_id: user.id) }
    end

end

It only adds the last scope to the query. Shouldn't it be applying both scoped where conditions?

inossidabile commented 11 years ago

There's a huge difference between fields and scopes. Fields rules are reversible. You can make can, then cannot and that's why it's safe and easy to merge such definitions.

Scopes are from other world. While merging two scopes is easy, the opposite is simply impossible. This is the main reason why you can only have one scope active and the next one will override it.

If you want to collect things, you can use variables (or instance variables if you have several protect calls). On the other side – this is interesting thinking. I agree this might be tricky and unpredictable.

Maybe we could have another method, say, filter that would mean "part of the scope". For every filter call – your scope extends with given things. But any direct scope call drops accumulated values and sets single value. With this we will save backward interface compatibility and make things look more reasonable.

/cc @fdeschenes

inossidabile commented 11 years ago

Another thing we can do is to make filter(:name) { ... }. Then we can make unfilter :name later. And scope would become a low-level approach. I think I like this new filter more and more.

anazar commented 11 years ago

I would think that scopes should be chained by default. If certain conditions are met you would apply a scope. I can't think of a situation in which you would want to remove a scope. That login would be accomplished by doing an if... else... scenario.

Rails chains scopes by default... I don't see why this should be any different. Only applying the last scope doesn't seem like the "rails way" of doing things.

If you wanted to filter and unfilter things I think the filter approach would be good. But if we're using scope it should chain in my opinion.

inossidabile commented 11 years ago

We've discussed it a bit together internally and came up to the conclusion you were right. To keep things settled I've released this as 0.6.0.beta.1. I'm going to release 0.6.0 in a week if nobody complains on BC changes.

anazar commented 11 years ago

Awesome! Thanks so much!!