varvet / pundit

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

Considering integrating the Pundit::Context even more #801

Closed mattzollinhofer closed 2 months ago

mattzollinhofer commented 3 months ago

Prefer policy_cache if available to policy_finder.

I'm unsure if there are other implications for this, but for the use case of sharing context between the typical controller use (authorize) vs the view typical use (policy) this adds value. Without this the authorize and policy methods are still disconnected as far as I can tell.

It looks like you were thoughtful about only choosing the else clause to be stored in the cache, so I might be missing a consideration you were making. But, I'd argue that if the client specifies a policy_class for a given record then the goal is for that policy_class to apply to all records through the pundit-based interactions. It was our intention at least.

Burgestrand commented 3 months ago

Thanks!

Yes, I've been thinking about this. I believe that the policy and scope cache should apply when you supply a policy_class.

However, changing the current behaviour is not backwards-compatible: adding caching behaviour to a method where there used to be none.

I believe there's a way forward where caching like this can be opt-in behaviour, while keeping backwards-compatibility by default. Probably by adjusting the dependencies injected into Pundit::Context.

Something that also needs to be considered is the user and policy_class, they both should probably be considered for being part of the cache key. user could maybe be omitted, but policy_class probably can't.