Modified NotAuthorizedError default message

[langsharpe]

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]

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]

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]

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]

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:

pundit/lib/pundit.rb

Line 39 in 7e59b98

message = options.fetch(:message) { "not allowed to #{query} this #{record.class}" }

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!