Add ability to use different error messages for same policy rule

[holyketzer]

Deep error messages customisation

There is no way to show different error messages for one policy rule now, but is matters (see example below).

When you have different authorization deny reasons you want to inform user with descriptive message then:

Extend your ApplicationPolicy with message attr and deny! method:

class ApplicationPolicy
  attr_reader :message
  
  def deny!(message)
    @message = message
    false
  end
end

In your policy class specify custom error message in deny! parameter:

class ProjectPolicy < ApplicationPolicy
  def create?
    if user.has_paid_subscription?
      if user.project_limit_reached?
        deny!(I18n.t('errors.user.project_limit_reached'))
      else
        true
      end
    else
      deny!(I18n.t('errors.user.paid_subscription_required'))
    end
  end
end

Then you can get this error message in exception handler:

rescue_from Pundit::NotAuthorizedError do |e|
  # Cutom error will be in e.message
  # Also it will bein e.policy.message
  # If there was no deny! call e.policy.message will be nil
end

[dgmstuart]

@holyketzer Interesting suggestion. I guess you're referring to https://github.com/varvet/pundit#creating-custom-error-messages, which shows an approach allowing a single message to be assigned to each action.

I'm not sure about the implementation in this PR: adding these methods to the ApplicationPolicy and passing the message using an instance variable feels a bit too obscure for me.

Since the Pundit::NotAuthorizedError already supports a :message option, how would you feel about this:

class ProjectPolicy < ApplicationPolicy
  def create?
    if user.has_paid_subscription?
      if user.project_limit_reached?
        raise(Pundit::NotAuthorizedError, message: I18n.t('errors.user.project_limit_reached'))
      else
        true
      end
    else
      raise(Pundit::NotAuthorizedError, message: I18n.t('errors.user.paid_subscription_required'))
    end
  end
end

Or even just eg.

raise(Pundit::NotAuthorizedError, I18n.t('errors.user.paid_subscription_required'))

...since I don't see how passing the message as part of the options is any different to just passing it as a string.

Maybe this is bad since it's failing using an error rather than false 🤷‍♂. Looking at the implementations of .authorize and #authorize it looks like this is almost exactly the same behaviour, since false immediately results in an error getting raised, though I guess that could change in the future.

I'd be open to adding a method to Pundit (maybe deny!, I can't think of a better name!), which wraps this up so the user doesn't have to refer to the error class directly.

[dgmstuart]

Or maybe my suggestion doesn't work since Pundit::NotAuthorizedError isn't available on policies?

[holyketzer]

@holyketzer Interesting suggestion. I guess you're referring to https://github.com/varvet/pundit#creating-custom-error-messages, which shows an approach allowing a single message to be assigned to each action.

Yes I started from it, but later application evolved and we faced with requirement to have different messages for one policy

I'm not sure about the implementation in this PR: adding these methods to the ApplicationPolicy and passing the message using an instance variable feels a bit too obscure for me.

Yes, I understand it, including Pundit philosophy to keep things very simple.

Maybe this is bad since it's failing using an error rather than false 🤷‍♂. Looking at the implementations of .authorize and #authorize it looks like this is almost exactly the same behaviour, since false immediately results in an error getting raised, though I guess that could change in the future.

IMHO from policy point of view returning false or raise exception in different cases looks inconsistent.

I'd be open to adding a method to Pundit (maybe deny!, I can't think of a better name!), which wraps this up so the user doesn't have to refer to the error class directly.

Cool, sounds like a good compromise. Let me investigate it a little bit more.

Or maybe my suggestion doesn't work since Pundit::NotAuthorizedError isn't available on policies?

It should work, yes it works

[holyketzer]

Since the Pundit::NotAuthorizedError already supports a :message option, how would you feel about this:

Right now I inherited custom error class to split errors in controller, like this.

class CutomNotAuthorizedError < Pundit::NotAuthorizedError
  def initialize(custom_message)
    super(custom_message)
  end
end

But this also works:

# inside my policy
raise Pundit::NotAuthorizedError, custom_message

rescue_from Pundit::NotAuthorizedError do |e|
  message =
    if e.policy 
      policy_name = e.policy.class.to_s.underscore
      I18n.t("#{policy_name}.#{e.query}", scope: "pundit", default: :default)
    else
      # if there is no policy in exception options it means custom error was raised
      e.message
    end

  # render with message ...
end

Which options I can propose:

1. I can remove this messy message attr stuff and leave only deny! method which raises error with custom message + some description in readme how to use it.
2. I can just extend readme with this solution and don't touch codebase at all.
3. Take the blue pill (close PR, no changes, I solved my problem already) - the story ends, and you can forget about this very soon )
4. ... Maybe your suggestions?

[dgmstuart]

Right now I inherited custom error class to split errors in controller, like this.

class CustomNotAuthorizedError < Pundit::NotAuthorizedError
  def initialize(custom_message)
    super(custom_message)
  end
end

If what you want is to sometimes have a single message and sometimes have multiple messages, then this approach of using a separate class feels more explicit: the rescue_from/if approach basically involves having two separate behaviours in the same class, and asking the class which one it is. This feels like a violation of Tell-Don't-Ask.

Observe that at this point all we're using the error class for is to pass information from the policy up to the controller, so it doesn't really matter what the error is: I'm not even sure it needs to inherit from Pundit::NotAuthorizedError, and I'm not sure it needs a special definition?

But is this really what you want? This means that in some cases the message is configured in the controller and in some places in the policy - would it be easier to manage if all the messages were at the same level (in the policies)?

[dgmstuart]

Here's some thinking aloud:

There are a number of different ways that we've discussed building messages at various points here:

1. as an explicit string
2. fetched from I18n with a custom key
3. fetched from I18n with a key based on the policy and query
4. a default message based on the resource class and query

[dgmstuart]

@holyketzer question: do the messages need to be different per controller? Or should the same authorisation failure have the same message regardless of where it happens?

[holyketzer]

@holyketzer question: do the messages need to be different per controller? Or should the same authorisation failure have the same message regardless of where it happens?

Yes, for example for some policy actions we have couple of checks can user access this resource (does it belong to this user) and does use have paid subscription which allows to do it.

Observe that at this point all we're using the error class for is to pass information from the policy up to the controller, so it doesn't really matter what the error is: I'm not even sure it needs to inherit from Pundit::NotAuthorizedError, and I'm not sure it needs a special definition?

Correct, the way how I implemented it now, it doesn't.

But is this really what you want? This means that in some cases the message is configured in the controller and in some places in the policy - would it be easier to manage if all the messages were at the same level (in the policies)?

Hm, yes, I would prefer to move this logic into one place - policy and just consume exception message in rescue controller block. I can generalise the way how base ApplicationPolicy makes messages, and for special cases throw custom message, but in all cases policy will throw an exception and it will be the one class.

[dgmstuart]

@holyketzer What do you think the way forward is?

Reading back, it looks like the solution that we arrived on is to have pundit policies explicitly raise errors with specific messages, and then rescue these in the controller to display the message.

That solution required no changes to Pundit, but looking at it now I'm not sure it works: is I18n available in policies?

If not then maybe an idea could be to store a code for the type of authorisation on the error message and use that to look up in I18n - that way the pundit policy stays nice and isolated.

One way to do this might be to add an additional option onto Pundit::NotAuthorizedError, or a class inheriting from it:

class ProjectPolicy < ApplicationPolicy
  def create?
    if user.has_paid_subscription?
      if user.project_limit_reached?
        raise(Pundit::NotAuthorizedError, reason: 'user.project_limit_reached')
      else
        true
      end
    else
      raise(Pundit::NotAuthorizedError, reason: 'user.paid_subscription_required')
    end
  end
end

class ApplicationController < ActionController::Base
 rescue_from Pundit::NotAuthorizedError, with: :user_not_authorized

 private

 def user_not_authorized(exception)
   message = exception.reason ? t "errors.#{exception.reason}" : exception.message
   flash[:error] = message, scope: "pundit", default: :default
   redirect_to(request.referrer || root_path)
 end
end

A nice property of this is that :reason makes sense in its own right, not just as an I18n Key.

[holyketzer]

That solution required no changes to Pundit, but looking at it now I'm not sure it works: is I18n available in policies?

I18n is available in policies if you refer it with full module name I18n.t("key")

One way to do this might be to add an additional option onto Pundit::NotAuthorizedError, or a class inheriting from it:

I like this approach, it looks simplest one, I implemented it, and check on my my project, looks good.

[dgmstuart]

@holyketzer great - I guess we can close this now?

[holyketzer]

I changed PR did you see it?
Ii you like it you can merge, else it's ok my issue is solved, just wanted to share with a community.

[dgmstuart]

Good work 👍 - a couple of tweaks maybe.

[dgmstuart]

"deny reason" feels like a new bit of terminology: I'd prefer to avoid that if possible. Consider:

Suggested change

	If you have different authorization deny reasons you want to inform user with descriptive message then:
	If there are multiple reasons that authorization can be denied, you can show different messages by raising exceptions in your policy:

[dgmstuart]

I'm not sure "Deep" feels like a familiar term for me. Maybe something like "Multiple error messages".

Also consider nesting this under the previous section.

[dgmstuart]

Is it worth including an example locale file like in the previous section, or is that unnecessary?

[dgmstuart]

@Linuus any thoughts on the PR as it now stands? (no need to understand the discussion: this is now just documentation).

[holyketzer]

Good work 👍 - a couple of tweaks maybe.

Ok, done

[danielolivaresd]

This is definitely a feature that I appreciate and will be using.

Are there any plans to release a new version of the gem soon including this? (just asking, otherwise I can just point to this PR or master for now). Thanks in advance.

[dgmstuart]

@K1N5L4Y3R this is largely just documentation and there's probably nothing stopping you from applying the approach now. To be honest I'm less and less convinced that extending Pundit::NotAuthorizedError was a good idea: you can most likely get the same result by making your own class that inherits from Pundit::NotAuthorizedError and includes a 'reason' key.