varvet / pundit

Minimal authorization through OO design and pure Ruby classes
MIT License
8.29k stars 632 forks source link

Pundit.policy_scope class method should support new policy_scope_class option #537

Closed CharlesMcMillan closed 6 years ago

CharlesMcMillan commented 6 years ago

We noticed that the newest release includes this policy_scope_class option (which is nice!) for the policy_scope method that is included as an instance method on controllers here, but it was not added as an option on the class method here.

Since we use a mixture of both the instance and class method, depending on the context in which the policy is used, we would like to be consistent in our usage of policy_scope_class. Do you think it makes sense to support this feature in both of these code paths?

Linuus commented 6 years ago

Why not just call the correct scope directly instead of using the class method if you know what class you want? The class method literally just calls the resolve method on the class.

CharlesMcMillan commented 6 years ago

@Linuus Your suggestion makes tons of sense! In our case, we decided to leverage the PolicyFinder rather than invoking the policy class directly because we wanted to encourage one way to do things, whenever possible. Doing so has led to a pretty consistent set of practices for everybody to follow whether you're in a controller (where you have the instance method) or in another context (where you use the class method). The consistency is nice to have, but whether or not this was the right way to go is definitely up for debate! 😄

That being said, is there any downside to having matching APIs for the case of the class method and the instance method? If the prescription/solution is to instantiate the policy class directly, why not make that the recommended way to get policies outside of controllers? In other words, is there any reason for the Pundit class method 'policy_scope' to exist at all?

Linuus commented 6 years ago

Yes, there is a reason to have the class method. Either as a convenience method to fetch the policy scope from a model or maybe you can have different kind of models (in case of polymorphism).

However, if you override the class you always know the class name, so just invoke it directly. The difference in the instance method is that it also has some other logic behind the scenes (flagging request as scoped).

Linuus commented 6 years ago

I don’t think we need to add this for the reasons I stated above.