search

varvet / pundit

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

Include policy name in NotAuthorizedError message #681

Closed langsharpe closed 3 months ago

langsharpe commented 3 years ago

record is sometimes an Object, sometimes a Class and sometimes an Array. Use the policy instead in the error message.

e.g.

authorize(Post.first)
before: not allowed to destroy? this Post
after : not allowed to destroy? with PostPolicy
authorize([:project, Post.first])
before: not allowed to destroy? this Array
after : not allowed to destroy? with Project::PostPolicy

This is a slightly different solution to the problem raised by #670. This may not the right solution but it was easy to create without refactoring what 'record' means when it is an Array.

Please let me know if there is anything I can do to help this go through.

Burgestrand commented 3 years ago

Hi! Thanks for the PR!

I personally like this and believe it makes sense. I'd like to get another set of eyes on this, e.g. by @dgmstuart before merging.

Burgestrand commented 2 years ago

Hi again!

Due to some recent changes (not yet released but available in main) the record is no longer passed as an array, so hopefully most of this problem is resolved.

However, I still believe having the policy name in the default error message would be useful. Would it be overkill to have both? What do you think?

For example (I'm open to suggestions):

not allowed to Project::PostPolicy#destroy? this Post
langsharpe commented 2 years ago

not allowed to Project::PostPolicy#destroy? this Post

This error message would be fine and fix the issue we have.

The most challenging place to debug is when the error is discovered in production. It would help if you had visibility when viewing the error in an error tracker (e.g. Bugsnag). Knowing which policy is selected is beneficial. The policy can be chosen dynamically, so an error message is the only chance you get to know what was selected.

If you wanted to take it a step further, you could::

This User is not allowed to Project::PostPolicy#destroy? this Post

That would ensure at least the type of user and record is recorded. Which user and record should be determined by other information in the tracked error like URL and logged in user.

Looking forward to the new version!

Burgestrand commented 5 months ago

Taking a look at some old issues. Commenting for documentation's sake.

This PR is still relevant, even though the underlying code has changed. We're still not including the policy name in the error message: https://github.com/varvet/pundit/blob/7e59b98a72850a02c0a10ec17fff425cff18d31a/lib/pundit.rb#L39

However, the information is available so it's mostly a matter of updating the message to also include the policy used.

So the PR does need updating, and the underlying idea/approach is good!

Burgestrand commented 4 months ago

647 is also related to this!