Allow custom error messages to be passed to authorize call

[thomasklemm]

• Add Readme section on rescuing from Pundit::NotAuthorizedError in Rails.
• Allow custom error messages to be set in controller actions.

Regarding the custom error messages, I think that passing in a custom error message in the controller action should suffice.

authorize @post, :create?, "You cannot create new posts."

#20 and #32 proposed custom error_message(query) methods on the policy, but I'm unsure whether this will bring us any further. Would you check the implementation and docs section @jnicklas and add in any other ways you might be rescuing a Pundit::NotAuthorizedError?

Closes #20 and #32.

[jnicklas]

We should at least give some thought into how this might integrate with i18n. I'm not sure if this proposal would fit well with i18n, or worse. I'm not entirely sure whose concern the error message should be either. Difficult.

[thomasklemm]

Didn't need i18n in a project up to now, so I don't know. Would you have any improvements for the Readme section on rescuing Pundit::NotAuthorizedError? I could split it out and merge it separately?

[thomasklemm]

@Zeeraw @kidpollo Your motivation behind PR's #20 and #32 is - I assume - that you want to provide a descriptive error message to your users, which is actually very good practice and a courtesy towards your app's users.

Taking this motivation even further, one could say that you can improve your app's user experience even further by simply not rendering the links to actions that the user is not authorized to access or execute.

<% if policy(@post).create? %>
  <%= link_to "New post", new_post_path %>
<% end %>

My conclusion would be that the error message passed to a Pundit::NotAuthorizedError can help the user to further understand what he may not do, but it would be even better if he wouldn't see a flash message stating "You are not authorized to perform this action." when clicking on a visible link. For users trying to force an action by explicitly typing a URL a generic message might suffice.

Therefore, I'm closing this PR. Would you agree?

[jnicklas]

Yeah, I think you're right. If you build your app's UX right, authorize should be a last line of defense, the user should never be able to "accidentally" trigger an authorization error. So I think it's good practice for the authorization error message to be quite generic.

[kidpollo]

That is not true if you are building an API meant for public consumption.

[thomasklemm]

@kidpollo How do you rescue the Pundit::NotAuthorizedError in your API and build the response? How would setting a custom error message in Pundit help?

[zeevallin]

More times than not, I've found catching the authorisation errors useful to provide good explanation to users as well as API consumers. It has solved issues before they appear as questions in my mail inbox.

Though simplified, this is how I usually go about it:

def show
  @post = Post.find(params[:id])
  authorize @post, :read?, "you're not allowed to do this"
rescue Pundit::NotAuthorizedError => message
  respond_with { error: message }, status: 403
end

or even more often, have a generic catchall for Pundit::NotAuthorizedError using rails rescue_from, like so:

class ApplicationController < Actioncontroller::Base

  rescue_from Pundit::NotAuthorizedError do |message|
   respond_with { error: message }, status: 403
  end

end

I would argue my codebase will contain a magnitude more lines of code to always raise and catch errors per instance of authorisation through pundit happens. Making use of the built in exception parameters through authorise might increase the gem complexity, but at the same time save quite a significant amount of trouble.

Thoughts?

[thomasklemm]

@Zeeraw Your API is English only I assume? Is there a straight-forward way we could localize the error messages?

[jnicklas]

Once thing which has gone unmentioned is that there is an ambiguity with the arguments, since the name of the query method to check is actually optional:

authorize @post, "you're not allowed to do this"
authorize @post, :read?

Of course we could check if the second parameter is_a?(String), but that seems a bit iffy. Thoughts?

[jnicklas]

Let's reopen this for now. @kidpollo makes a good point. Pundit could well be used for public APIs and in that case it does matter.

[zeevallin]

@thomasklemm Let's say we're setting up some defaults for the most common methods; read, delete, update etc together with the most common alias. This way the consumer of pundit can just interact with error messages through their translation backend and I18n itself. Then one could extend it to to use the parameterised name of the record class to define record specific messages for those same methods.

Default:

en:
  pundit:
    default: "not allowed to %{query} this %{record}"

Consumer Extended:

en:
  pundit:
    default: "not allowed to %{query} this %{record}"
    records:
      post: "post"
    queries:
      read: &en_pundit_queries_read "read"
      show: *en_pundit_queries_read

    read: &en_pundit_read "not allowed read do this %{record}"
    show: *en_pundit_read

    post:
      create: "not allowed to post this"
sv:
  pundit:
    default: "inget tillstånd att %{query} this %{record}"
    read: &sv_pundit_read "du har inte tillstånd att läsa detta"
    show: *sv_pundit_read
    records:
      post: "posten"
    queries:
      read: &sv_pundit_queries_read "läsa"
      show: *sv_pundit_queries_read
    post:
      create: "får inte posta detta"

And through pundit source having a fallback on a more generic message something along the lines of this.

def authorize(record, query=nil, error_message=nil)
  query ||= params[:action].to_s + "?"
  error_message ||= extract_translation_key(record,query)
  @_policy_authorized = true
  unless policy(record).public_send(query)
    raise NotAuthorizedError, error_message
  end
  true
end

private

def extract_translation_key(record,query)
  # insert mumbojumbo to extract query and record translations here
  I18n.t("pundit.#{record.class.to_s.parameterize}.#{query.to_s}") ||
  I18n.t("pundit.#{query}", :record => record) ||
  I18n.t("pundit.default", :query => query, :record => record)
end

Quite frankly, while I like Pundit because it's slim, what do you actually think of adding I18n to the mix, both for managing the default behaviour and giving the consumer a standard interface to work with. To me it sounds rather compelling.

[zeevallin]

@jnicklas Iffy indeed, either you insert magic to determine if it's an error message or have the constraint that you need to specify the query if you want an instance specific error message.

[thomasklemm]

@Zeeraw Good work, great suggestion!

[kidpollo]

I was asleep during this conversation but Philip made all the points I
would and actually that is exactly the case of what we are doing! Thanks

{ :sent_from => iPhone }

On Jun 25, 2013, at 8:13 AM, Thomas Klemm notifications@github.com wrote:

@Zeeraw https://github.com/Zeeraw Good work, great suggestion!

—
Reply to this email directly or view it on
GitHubhttps://github.com//pull/38#issuecomment-19983300
.

[jnicklas]

I'd like to point out that this PR conflicts with the gajillion misguided issues and PRs trying to add extra params to policy query methods and passing them through authorize.

[thomasklemm]

The issue evolved and is discussed in other PRs (see links above).

[thomasklemm]

Custom error messages have been implemented in #114.