Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Here we go again: Custom error messages? #654

Open
rlue opened this issue May 14, 2020 · 3 comments
Open

Here we go again: Custom error messages? #654

rlue opened this issue May 14, 2020 · 3 comments

Comments

@rlue
Copy link
Contributor

rlue commented May 14, 2020

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:

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
Copy link
Contributor Author

rlue commented May 14, 2020

[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?

rlue added a commit to rlue/pundit that referenced this issue May 14, 2020
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 varvet#654[0],
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 ultimatly 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

[0]: varvet#654
rlue added a commit to rlue/pundit that referenced this issue May 14, 2020
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 varvet#654[0],
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 ultimatly 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

[0]: varvet#654
@rlue rlue mentioned this issue May 27, 2020
@Burgestrand
Copy link
Member

Burgestrand commented Aug 10, 2021

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
Copy link

phikes commented May 26, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants