Here we go again: Custom error messages?

[rlue]

Sorry to beat a dead horse. People have been asking about custom error messages for over six years now (#66, #143, #212, #599). Clearly, there have been some improvements in the API since then (#114), and some great suggestions for how to implement this at the controller level (#503), but the approach currently recommended in the README still doesn't feel right to me.

Official Approach

The README suggests one of two options:

• Use i18n to generate error messages based on query, record, and policy.
• Raise errors inside your query methods.

What's wrong with this?

The first approach limits you to one error message per resource-query. Queries can fail for many reasons, and it's nice to be able to pass this information along in the exception message.

The second approach breaks the pattern of query methods returning a boolean. Pundit's own README recommends using them in views as follows:

<% if policy(@post).update? %>
  <%= link_to "Edit post", edit_post_path(@post) %>
<% end %>

Raising an error in #update? is not compatible with this approach.

What I'm not asking for

A lot of people have asked to specify custom exceptions as an argument to #authorize. I am 100% against this idea: if you're using policies in the first place, then that's the only class that should know about why authorization failed. If you spread that responsibility between the policy and the controller, then the policy just becomes a bucket for tossing helper methods into, which violates SRP and really defeats the purpose.

It appears that @jnicklas shares this sentiment.

A proposal

#503 came very close to what I'd like to see, but it accomplishes custom error messages in rescue_from, via a combination of policies, controllers, and the I18n gem. As a library, I think Pundit should make minimal assumptions about external dependencies, and so I'd suggest something that lives entirely within Pundit itself—i.e., depending only on policy classes and Pundit::NotAuthorizedError:

# lib/pundit.rb

class NotAuthorizedError < Error
  def initialize(options = {})
    ...
    message = options.[:message] || policy.try(:error_message) || "not allowed to #{query} this #{record.inspect}" }
    ...
  end
end

Usage

class ApplicationPolicy
  attr_accessor :error_message
end

class PostPolicy < ApplicationPolicy
  def update?
    self.error_message = if record.author != user
                           'Keep your grubby hands off other people’s stuff!'
                         elsif record.archived?
                           'That’s old news, baby!'
                         end

    error_message.nil?
  end
end

Risks

Any existing users who have defined an #error_message method (or getter) on their policies would be affected by this change.

EDIT: To address this, we could choose a method name that Pundit users are less likely to have already taken, like #denial. This has the added benefit of (conceptually) decoupling this attribute with the concept of errors/exceptions. After all, policies shouldn't "know about" errors in the first place—they just know about users, resources, and queries. (It's #authorize that handles raising errors.)

---

@Linuus & co., what do you think about this proposal? It's a small one, so I will prepare a PR anyway.

[rlue]

[raising exceptions in queries] breaks the pattern of query methods returning a boolean.

I just discovered that 973b63b (relatively recent—just six months old) builds on (established?) this pattern of raising exceptions in queries. I still don't like it; as with my previous example, I think a getter/setter on the policy itself is a conceptually cleaner way to pass this parameter to the exception.

I don't want to step on anyone's toes, but does the exceptions-in-queries approach bother anyone else?

[Burgestrand]

I believe the reasoning in this issue makes sense, and agree with most everything except for possibly the issue with the implementation as mentioned in #655 (comment)

Either way, as mentioned in #656 (comment) we can keep this issue and the PR (#655) for the discussion about this particular feature, regardless of what solution we end up with.

[phikes]

Here's how I solved it using dry-monads (someone else mentioned it on another issue already):

class ThingPolicy < ApplicationPolicy
  include Dry::Monads[:result]

  def user_completed_thing?
    user.completed_things.include?(record.thing)
  end

  def user_bought_thing?
    user.orders.find_by(orderable: record).present?
  end

  def show?
    show_result.success?
  end

  def show_result
    return Success() if record.user.admin?

    return Failure(:incomplete_thing) unless user_completed_thing?
    return Failure(:missing_order) unless user_bought_thing?

    Success()
  end
end

This way the original convention of returning booleans in policy methods is still kept and all of the pundit helpers still work. Using this approach doesn't require any monkey patching/changes to pundit and is entirely optional in application code as well. The downside of course is a small dependency. If any of the methods called in show_result are expensive it's up to the user to cache these. I only return a single failure, but of course it's also possible to wrap an array of reasons in failure.