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

Already on GitHub? Sign in to your account

Add #query and #record properties to Pundit::NotAuthorizedError. #114

Merged
merged 1 commit into from Mar 5, 2014

Conversation

Projects
None yet
3 participants
Contributor

ucarion commented Feb 26, 2014

I don't see why Pundit::NotAuthorizedError doesn't provide the user with the query or record that "caused" the error to be raised. This PR adds these two properties.

I think using the NotAuthorizedError could resolve the still-open question of handling custom error messages (see #68, #38, and #66 for earlier discussion). With this change, I as a user of this gem could choose to (but am not obligated to) use the query and record property to create my error message:

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

  def user_not_authorized(exception)
    record_name = exception.record.class.to_s.downcase
    flash[:error] = I18n.t "pundit.#{record_name}.#{exception.query}"
  end
end
en:
  pundit:
    post:
      update?: 'You cannot edit this post!'

I think one of the strong points about Pundit is that it is extremely transparent -- there's almost nothing magical about it. The fact that there's little consensus about how errors should be handled is a sign that we can't predict what developers will expect Pundit to do w.r.t. errors. Rather than surprise them, why not let developers choose how to handle errors on their own?

Let me know what you think!

Collaborator

jnicklas commented Feb 26, 2014

I like this. This is much simpler and nicer than the other suggestions. @thomasklemm, what do you think?

Collaborator

thomasklemm commented Feb 26, 2014

I like it. Feels like pundit.

You'll need to implement lookup chain for the error messages yourself, so we should suggest a sensible default with a fallback in the Readme. How about:

class ApplicationController < ActionController::Base
  include Pundit

  rescue_from Pundit::NotAuthorizedError, with: :user_not_authorized

  def user_not_authorized(exception)
    record_name = exception.record.class.to_s.downcase
    flash[:error] = I18n.t "pundit.#{record_name}.#{exception.query}", 
      default: 'You cannot perform this action.'
    redirect_to request.headers['Referer'] || root_path
  end
end

@ulyssecarion Can you explain this change in the Readme? If you wish, you could override or extend the rescuing a denied authorization bit.

Contributor

ucarion commented Feb 26, 2014

@thomasklemm I've adapted your code and opened #116 to change the README. If there's disagreement on the details of custom error messages that discussion could happen there, since my comment about error messages was really just a suggestion and not part of this PR.

Collaborator

jnicklas commented Feb 27, 2014

What do you guys think about adding #policy which would return the policy instance? I think that would almost be more useful than #record. Also, maybe we shouldn't change the constructor of the error, and instead simply add these as attr_accessors and set them. NotAuthorizedError is kind of public API, so it might be wise to not break it unnecessarily.

Collaborator

thomasklemm commented Feb 27, 2014

@jnicklas You mean #policy should return policy(record) / RecordPolicy.new(record) or RecordPolicy?

Contributor

ucarion commented Feb 27, 2014

@jnicklas while I'm totally not opposed to keeping the constructor the same, these changes don't break backward compatibility because the arguments to the constructor have default values.

Collaborator

jnicklas commented Feb 27, 2014

@ulyssecarion the default for StandardError is to take message as a parameter. So it does break it in a sense.

@thomasklemm we already have an instance of the policy (we just called a query method on it which failed), so I'd say policy returns this instance. Since the policy instance contains the record and user, we probably don't need to store it separately in the error, I think.

Collaborator

thomasklemm commented Feb 27, 2014

@ulyssecarion How about going the attr_accessor :record, :query route, so we can keep the message argument?

@jnicklas From the perspective of our own Readme and specs, the record argument in a policy can be named differently:

PostPolicy = Struct.new(:user, :post) do
  def update?
    user.admin? or not post.published?
  end
end

With the policy, policy.record would break, we would need to call policy.post to access it. That's why I favor the more generic record and query accessors.

Would there be a point in adding an attr_accessor :user, in cases the error is handled in a setting without access to the pundit_user / current_user? If nescessary, you can instantiate the policy once again anywhere with Pundit.policy(error.user, error.record).

Collaborator

thomasklemm commented Feb 27, 2014

Is there a nicer way to assign the values than this one?

Collaborator

jnicklas commented Feb 27, 2014

@thomasklemm I think that's fine. Does it really work to raise an instance of an error with a message like that though, doesn't the message need to be sent in to the constructor?

Collaborator

thomasklemm commented Feb 27, 2014

@jnicklas In Ruby 2.0 it works, haven't checked other runtimes. I've also added it to the specs, I'll open a PR just for seeing what Travis says about other Ruby implementations.

Might be that raise e, 'message' does an e.message = 'message' behind the scenes when passing in an instantiated e = Exception.new. Kernel#raise

Collaborator

thomasklemm commented Feb 27, 2014

@jnicklas Travis says the Ruby implementations behave consistently 👍

Contributor

ucarion commented Mar 1, 2014

@thomasklemm I like how you're doing it. Shall I merge your approach into this PR?

Contributor

ucarion commented Mar 3, 2014

@thomasklemm @jnicklas I've updated the PR to add a user property to NotAuthorizedError. I didn't think of this before because I've never yet needed to use anything else than the default. I switched to using an attr_accessor-based approach, and eschewed any policy properties for the reason mentioned here.

Since the exception has not has its constructor modified at all, you can still generate a NotAuthorizedError using the default StandardError string-based constructor.

Collaborator

jnicklas commented Mar 4, 2014

As a counterpoint to @thomasklemm's earlier comment: if you're implementing a policy, you are also the one responsible for rescuing exceptions from it. If you then want to extract data from the policy, it makes sense that you have to follow your own API. If the policy doesn't respond to #record then don't can #record on it.

IMO, there's something a little problematic about authorize directly fetching pundit_user for example, rather than grabbing it from the policy. I realize this is a bit far fetched, but someone could be overriding policy to do something different, in that case the user we'd return might not match the user inside the policy. Maybe I'm just being paranoid.

Contributor

ucarion commented Mar 4, 2014

@jnicklas The issue is that I think many policies (including those I write) do opt to use more descriptive names (post, comment) rather than plain old record, so not providing #record would force users to use a case or something.

Perhaps an acceptable compromise would be to provide both #record and #policy? Both are pieces of information Pundit is pulling in.

As for the user property calling pundit_user, I don't it's worth trying to handle the case of #policy being overridden, but I'm open to something else. One possible issue related to the problem with fetching #record from a policy is that Pundit does not hard-code that the first parameter must be called user, right?

Collaborator

thomasklemm commented Mar 4, 2014

@jnicklas I've updated #117 to use policy(record).user.

I like having a general error.record reader, e.g. to allow for different errors based on the record class.

Contributor

ucarion commented Mar 4, 2014

@thomasklemm Does your modification to #117 mean that you support using policy(record).user over pundit_user? What if someone had instead done:

# Note that it's 'moderator', not 'user'
PostPolicy = Struct.new(:moderator, :post) do
  def update?
    moderator.sysop? or not post.published?
  end
end

Or something? I admit that it seems weird, but otherwise we're adding the requirement that all policies must call their first argument user. Or is this already a requirement?

Collaborator

thomasklemm commented Mar 5, 2014

@ulyssecarion Good point. While I assume most people call the user as is, there might be policies in which you choose to go a different route. @jnicklas Should we continue with pundit_user then?

Contributor

ucarion commented Mar 5, 2014

@thomasklemm Must we really provide #user? Would it be okay to say "if you want to get the offending user, call error.policy.user"?

Collaborator

thomasklemm commented Mar 5, 2014

So we just provide the instantiated error.policy and a quick accessor forerror.record? Sounds good, too, covering #116. I'll update #117.

Contributor

ucarion commented Mar 5, 2014

@thomasklemm Actually, could we just provide error.query and error.policy? This seems like the simplest solution yet.

Collaborator

thomasklemm commented Mar 5, 2014

@ulyssecarion I really favor adding a error.record as I'd like to have a common accessor for it, no matter whether it's called post or blog or whatever in the policy itself. I've put it in #117.

Add #query, #record, and #policy properties to Pundit::NotAuthorizedE…
…rror.

Exceptions raised by #authorize now provide the query (e.g. 'create?') and
record (e.g. an instance of 'Post') that caused the exception to be raised, as
well as the relevant policy (e.g. an instance of 'PostPolicy').

NotAuthorizedError is modified to continue to inherit from StandardError, but
now also has attr_accessor values for :query, :record, and :policy.
Contributor

ucarion commented Mar 5, 2014

@thomasklemm Good point about policy.record. I've updated this PR to use #policy too, and dropped #user. What do you think?

Collaborator

thomasklemm commented Mar 5, 2014

@ulyssecarion Looks good. Setting the error message when calling Exception.new is even nicer than passing it in with the raise as I did. I'm in favor of merging this PR.

jnicklas added a commit that referenced this pull request Mar 5, 2014

Merge pull request #114 from ulyssecarion/detailed_exception
Add #query and #record properties to Pundit::NotAuthorizedError.

@jnicklas jnicklas merged commit f19e8c1 into elabs:master Mar 5, 2014

1 check passed

default The Travis CI build passed
Details
Collaborator

jnicklas commented Mar 5, 2014

Let's put an end to this! Awesome work everyone, really like this way of handling errors!

Contributor

ucarion commented Mar 5, 2014

Thanks for merging, @jnicklas!

I'll try to work on #116 tomorrow.

Collaborator

thomasklemm commented Mar 5, 2014

@ulyssecarion Great PR!

@ucarion ucarion deleted the ucarion:detailed_exception branch Mar 5, 2014

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