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

Custom Error Messages (different from #38) #66

Closed
andrewpthorp opened this Issue Nov 5, 2013 · 3 comments

Comments

Projects
None yet
3 participants
@andrewpthorp

andrewpthorp commented Nov 5, 2013

I have read through the open issues, and I wanted to see if there is potential for this functionality. There are some cases in which the policies fail for different reasons.

If I have a User who has had their account suspended, I may want to tell them that they cannot create a new Record because their account is suspended. If their account is not suspended, I may tell them they cannot create a new Record because they have exceeded their limit.

Both of these happen from the same action:

def create
  @record = Record.new
  authorize @record
end

I don't believe passing the third argument of a custom message here (#38) solves the problem. I do, however, believe giving the exception more details and letting the rescue handle it gracefully solves this and #38.

If I could do this (over simplified for example):

rescue_from Pundit::NotAuthorizedError do |exception|
  if exception.query == 'create?' && current_user.limit_reached?(exception.record)
    flash[:error] = 'You have reached your limit, consider upgrading your account.'
  else
    flash[:error] = 'You are not authorized to do that.'
  end
  redirect_to root_path
end

Thoughts? I could work on a feature like this if it was a desire.

@thomasklemm

This comment has been minimized.

Show comment
Hide comment
@thomasklemm

thomasklemm Nov 5, 2013

Collaborator

Hi Andrew, that's an interesting idea, especially that an authorization can fail for different reasons, which should be reflected in the error message. Error messages certainly can become more user friendly. However, I feel that your approach might lead to a very deeply branched logic in the ApplicationController and also duplication of the authorization logic between RecordPolicy and rescue_from statement. Agreed?

Maybe we could put the error messages right in the policy, where people could plug in I18n if nescessary. Authorization logic and error messages in one place.

Collaborator

thomasklemm commented Nov 5, 2013

Hi Andrew, that's an interesting idea, especially that an authorization can fail for different reasons, which should be reflected in the error message. Error messages certainly can become more user friendly. However, I feel that your approach might lead to a very deeply branched logic in the ApplicationController and also duplication of the authorization logic between RecordPolicy and rescue_from statement. Agreed?

Maybe we could put the error messages right in the policy, where people could plug in I18n if nescessary. Authorization logic and error messages in one place.

@whatthewhat

This comment has been minimized.

Show comment
Hide comment
@whatthewhat

whatthewhat Feb 12, 2014

Hi!
I like the exception.query syntax for a somewhat different use case.
Let's say I have an API server and I want to respond with different http statuses depending on the type of action, the code might look like this:

rescue_from Pundit::NotAuthorizedError do |exception|
  if exception.query == 'read?' # or even exception.read?
    head :not_found             # obscure the fact that the resource actually exists
  else
    head :unauthorized
  end
end

@thomasklemm do you think this is a valid use case or maybe this logic should be handled at a different level? Thanks!

whatthewhat commented Feb 12, 2014

Hi!
I like the exception.query syntax for a somewhat different use case.
Let's say I have an API server and I want to respond with different http statuses depending on the type of action, the code might look like this:

rescue_from Pundit::NotAuthorizedError do |exception|
  if exception.query == 'read?' # or even exception.read?
    head :not_found             # obscure the fact that the resource actually exists
  else
    head :unauthorized
  end
end

@thomasklemm do you think this is a valid use case or maybe this logic should be handled at a different level? Thanks!

@thomasklemm

This comment has been minimized.

Show comment
Hide comment
@thomasklemm

thomasklemm Feb 26, 2014

Collaborator

@andrewpthorp @whatthewhat Good point. We're merging #114 which allows you to do that.

Collaborator

thomasklemm commented Feb 26, 2014

@andrewpthorp @whatthewhat Good point. We're merging #114 which allows you to do that.

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