search

inossidabile / protector

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

Should Protector make `scope { null }` when scope was not set explicitly? #9

Closed inossidabile closed 11 years ago

inossidabile commented 11 years ago

Protector utilizes white-list strategy when it comes to fields. That however is not true for scopes. It, by default, gives you an open scope which might be confusing. Shouldn't we change the behaviour to make it scope to an empty set by default?

/cc @cj @grindars @fdeschenes

cj commented 11 years ago

@inossidabile yes, I think you should actually remove https://github.com/inossidabile/protector/commit/62566d01de453e5249ac4acbf0cf37a03d1405ec#L3R92 as it goes against the new rails white-list approach. The user should have to define their own scopes and make sure a scope is defined for all users. It should just be made clearer in the docs.

inossidabile commented 11 years ago

@cj it will not only return false from visible? but will also return an empty set for restricted model having no scope defined. I'm not sure if that would be comfortable in fact. And I bet it will be confusing too. Just not sure which approach is more confusing here %). But ok, your vote accepted.

cj commented 11 years ago

@inossidabile I agree as that is why I got confused when I was getting undefined method 'where' for false:FalseClass, I didn't realize I needed a scope defined for all users. I think if we put in bold somewhere that a scope has to be defined and if you just wanted to inherit your other scopes just use scope { scoped }, I think people will be less confused when the run into the issue I did. I know if I re-read of the docs and saw I must be returning a scope I might not have opened a ticket about it :)

cj commented 11 years ago

For now I've just reverted back to https://github.com/inossidabile/protector/commit/06a02ed5230a0a1b02126b7b9e10b29964cd1582

inossidabile commented 11 years ago

Why? It doesn't make sense. Protector still doesn't make null scopes. You only have visible? broken at that point. visible? is just a checker, the real logic is hidden way deeper. It was initial approach to give you open scope unless you specify another one so downgrading to any point will not change anything.

cj commented 11 years ago

Ah, sorry I got confused. In that case I think you should keep the 'open scope' approach as changing it would create a lot of extra work for people and like you pointed out cause a lot of extra confusion :)

fdeschenes commented 11 years ago

I agree that we should keep the "open scope" approach – and for the same reasons.

inossidabile commented 11 years ago

I'm thinking about implementing a 'paranoid' mode that would make it default to nullified scopes and also raise exceptions for methods like pluck. So you could be sure you are not making something bad. It will be off by default.