Open rlue opened 4 years ago
[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?
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 https://github.com/varvet/pundit/pull/655#issuecomment-895822291
Either way, as mentioned in https://github.com/varvet/pundit/issues/656#issuecomment-895827605 we can keep this issue and the PR (#655) for the discussion about this particular feature, regardless of what solution we end up with.
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.
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:
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 andPundit::NotAuthorizedError
:Usage
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.