Support custom error messages in policies

[rlue]

This commit allows policies to define the error messages set on Pundit::NotAuthorizedError exceptions when #authorize fails. The rationale is described in detail in GitHub issue #654, and summarized below.

Some queries can fail in multiple ways; for instance,

class PostPolicy
  def update?
    if record.author != user
      ... # failure case 1
    elsif record.archived
      ... # failure case 2
    end

    true
  end
end

In their controllers, users might wish to handle different failure modes in different ways, but prior to this commit, there was only one way to tell the difference—namely, by raising errors inside the query method:

def update?
  if record.author != user
    raise Pundit::NotAuthorizedError, 'You’re not the author!'
  elsif record.archived
    raise Pundit::NotAuthorizedError, 'This post is old news!'
  end

  true
end

This breaks the expectation that query methods should return booleans, which in turn breaks a pattern for using query methods in views:

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

973b63b added a reason option to the NotAuthorizedError initializer, but ultimately required the same approach of raising errors in queries.

---

This commit enables a cleaner method of passing a custom error message to exceptions from within policies, without violating the expectations of where exceptions are raised from.

class PostPolicy
  attr_accessor :error_message

  def update?
    self.error_message = if record.author != user
                           'You’re not the author!'
                         elsif record.archived
                           'This post is old news!'
                         end

    error_message.nil?
  end
end

[take]

👍 for this

I think this approach is better than the current one included in master branch - 973b63b

[matteeyah]

Maybe the policy method could just return a failure reason or a custom query symbol? Generating error messages should be handled at the presentation layer.

[rlue]

Generating error messages should be handled at the presentation layer.

Can you elaborate? Applications with no presentation layer at all (e.g., CLI apps) raise errors and set messages on them all the time; why should it be the sole province of the presentation layer in a web app?

The presentation layer can decide what is shown to the user, and as the developer, you can decide whether that is Pundit::NotAuthorizedError#message or something else. IMO, setting a "failure reason" in the policy and then using that reason to generate a message in the controller is unnecessary indirection and complexity.

[matteeyah]

I'm not saying it needs to be on the controller level, API-only applications have presentation layers as well. In a web application with a frontend it can be in a Presenter or a Decorator, in API applications it can be in the serializer and/or entity object.

Bubbling up messages along with the error is fine as well. As you said, some applications might not actually have a presentation layer. The library should be able to send a failure reason or an error message and leave the decision about how error messages are displayed to the application.

[rlue]

The library should be able to send a failure reason or an error message and leave the decision about how error messages are displayed to the application.

100% agree, but I also don't see why the error message displayed by the application has to be the same as the value of Pundit::NotAuthorizedError#message. Why not just set that to your "failure reason" and handle it accordingly in your presentation layer?

def foo
  authorize(@post)
rescue Pundit::NotAuthorizedError => e
  case e.message
  when 'not author'
    # display one error message...
  when 'archived
    # display another error message...
  end
end

Maybe I am misunderstanding your original question:

Maybe the policy method could just return a failure reason or a custom query symbol?

Can you provide an example of how this would be implemented, so I can see what you're talking aobut?

[matteeyah]

Your example is almost exactly what I had in mind

The NotAuthorizedError also has reason and query fields that get bubbled up to the rescue block. (https://github.com/varvet/pundit/blob/master/lib/pundit.rb#L25)

I figured the policy method could return a symbol that the error can be initialized with.

Policy

def foo?
  :no_user unless user
  :archived if record.archived?

  true
end

Application Code

def foo
  authorize(@post)
rescue Pundit::NotAuthorizedError => e
  case e.reason
  when :not_author
    # display one error message...
  when :archived
    # display another error message...
  end
end

[rlue]

Interesting! I assume you actually meant the following:

def foo?
  return :no_user unless user
  return :archived if record.archived?

  true
end

because without return, the symbols are invoked in a null context and don't actually do anything.

The problem with this is that #authorize expects the query method (#foo?) to return a boolean:

raise NotAuthorizedError, ... unless policy.public_send(query)

and symbols (like :no_user and :archived) are truthy. Thus, by returning a failure symbol, the query method is actually telling Pundit that authorization succeeded.

This can be resolved by setting an attribute on the policy instead, which #authorize then uses to populate the error object:

def foo?
  self.reason = :no_user unless user
  self.reason = :archived if record.archived?

  reason.nil?
end

which is basically what my original PR proposes, but with #reason instead of #error_message.

I don't know how the Pundit authors feel about adding even more new attributes to the policy class; personally, I think #message is flexible enough to do what you're asking, without violating any serious conceptual principles.

[flvrone]

One way to deal with custom failures is right here, I use it a lot in my code.
https://dry-rb.org/gems/dry-monads/1.3/result/

My comment is barely relevant though, just an idea, don't mind me. :)

[Burgestrand]

Me and @dgmstuart had a chat about this, and I believe we both agreed that query methods should be encouraged to return a boolean (as opposed to raising an exception).

A possible problem with the approach suggested in this PR is that we've got memoized policy instances:

pundit/lib/pundit.rb

Lines 256 to 263 in 2714875

	# Retrieves the policy for the given record.
	#
	# @see https://github.com/varvet/pundit#policies
	# @param record [Object] the object we're retrieving the policy for
	# @return [Object, nil] instance of policy class with query methods
	def policy(record)
	policies[record] ||= Pundit.policy!(pundit_user, record)
	end

If I'm not wrong this means that the error reason here could become very confusing in case you query the same policy multiple times, e.g. in a controller action:

def do_something_in_a_controller
  if policy(record).can_do_this? # .error_message is now set
    # …
  elsif policy(record).can_do_that? # if this is true, .error_message is still going to be set from previous statement due to memoized instance
    # …
  else
     render policy(record).error_message # error message from _second_ query, the first one is lost
  end
end

I haven't verified this in actual code/test-case yet.

[Burgestrand]

Revisiting this today, and I don't think that having stateful policies as the default recommended approach is a good idea. Our main state keeping today is the policy/scope cache, and that's about it.

If it works for you, that's great, keep doing it! I'm mainly mindful of recommending it for all.

There's an approach posted in #654 that has a slightly different approach: #654 (comment)

I'd still love to hear how y'all end up solving this problem in your apps. Keep that in #654 though!