Closed rlue closed 3 months ago
👍 for this
I think this approach is better than the current one included in master branch - https://github.com/varvet/pundit/commit/973b63b396c2a98099caf5eefd1c6841416eddfa
Maybe the policy method could just return a failure reason or a custom query symbol? Generating error messages should be handled at the presentation layer.
Generating error messages should be handled at the presentation layer.
Can you elaborate? Applications with no presentation layer at all (e.g., CLI apps) raise errors and set messages on them all the time; why should it be the sole province of the presentation layer in a web app?
The presentation layer can decide what is shown to the user, and as the developer, you can decide whether that is Pundit::NotAuthorizedError#message
or something else. IMO, setting a "failure reason" in the policy and then using that reason to generate a message in the controller is unnecessary indirection and complexity.
I'm not saying it needs to be on the controller level, API-only applications have presentation layers as well. In a web application with a frontend it can be in a Presenter or a Decorator, in API applications it can be in the serializer and/or entity object.
Bubbling up messages along with the error is fine as well. As you said, some applications might not actually have a presentation layer. The library should be able to send a failure reason or an error message and leave the decision about how error messages are displayed to the application.
The library should be able to send a failure reason or an error message and leave the decision about how error messages are displayed to the application.
100% agree, but I also don't see why the error message displayed by the application has to be the same as the value of Pundit::NotAuthorizedError#message
. Why not just set that to your "failure reason" and handle it accordingly in your presentation layer?
def foo
authorize(@post)
rescue Pundit::NotAuthorizedError => e
case e.message
when 'not author'
# display one error message...
when 'archived
# display another error message...
end
end
Maybe I am misunderstanding your original question:
Maybe the policy method could just return a failure reason or a custom query symbol?
Can you provide an example of how this would be implemented, so I can see what you're talking aobut?
Your example is almost exactly what I had in mind
The NotAuthorizedError
also has reason
and query
fields that get bubbled up to the rescue
block. (https://github.com/varvet/pundit/blob/master/lib/pundit.rb#L25)
I figured the policy method could return a symbol that the error can be initialized with.
Policy
def foo?
:no_user unless user
:archived if record.archived?
true
end
Application Code
def foo
authorize(@post)
rescue Pundit::NotAuthorizedError => e
case e.reason
when :not_author
# display one error message...
when :archived
# display another error message...
end
end
Interesting! I assume you actually meant the following:
def foo?
return :no_user unless user
return :archived if record.archived?
true
end
because without return
, the symbols are invoked in a null context and don't actually do anything.
The problem with this is that #authorize
expects the query method (#foo?
) to return a boolean:
raise NotAuthorizedError, ... unless policy.public_send(query)
and symbols (like :no_user
and :archived
) are truthy. Thus, by returning a failure symbol, the query method is actually telling Pundit that authorization succeeded.
This can be resolved by setting an attribute on the policy instead, which #authorize
then uses to populate the error object:
def foo?
self.reason = :no_user unless user
self.reason = :archived if record.archived?
reason.nil?
end
which is basically what my original PR proposes, but with #reason
instead of #error_message
.
I don't know how the Pundit authors feel about adding even more new attributes to the policy class; personally, I think #message
is flexible enough to do what you're asking, without violating any serious conceptual principles.
One way to deal with custom failures is right here, I use it a lot in my code. https://dry-rb.org/gems/dry-monads/1.3/result/
My comment is barely relevant though, just an idea, don't mind me. :)
Me and @dgmstuart had a chat about this, and I believe we both agreed that query methods should be encouraged to return a boolean (as opposed to raising an exception).
A possible problem with the approach suggested in this PR is that we've got memoized policy instances:
If I'm not wrong this means that the error reason here could become very confusing in case you query the same policy multiple times, e.g. in a controller action:
def do_something_in_a_controller
if policy(record).can_do_this? # .error_message is now set
# …
elsif policy(record).can_do_that? # if this is true, .error_message is still going to be set from previous statement due to memoized instance
# …
else
render policy(record).error_message # error message from _second_ query, the first one is lost
end
end
I haven't verified this in actual code/test-case yet.
Revisiting this today, and I don't think that having stateful policies as the default recommended approach is a good idea. Our main state keeping today is the policy/scope cache, and that's about it.
If it works for you, that's great, keep doing it! I'm mainly mindful of recommending it for all.
There's an approach posted in #654 that has a slightly different approach: https://github.com/varvet/pundit/issues/654#issuecomment-1564339565
I'd still love to hear how y'all end up solving this problem in your apps. Keep that in #654 though!
This commit allows policies to define the error messages set on
Pundit::NotAuthorizedError
exceptions when#authorize
fails. The rationale is described in detail in GitHub issue #654, and summarized below.Some queries can fail in multiple ways; for instance,
In their controllers, users might wish to handle different failure modes in different ways, but prior to this commit, there was only one way to tell the difference—namely, by raising errors inside the query method:
This breaks the expectation that query methods should return booleans, which in turn breaks a pattern for using query methods in views:
973b63b added a
reason
option to the NotAuthorizedError initializer, but ultimately required the same approach of raising errors in queries.This commit enables a cleaner method of passing a custom error message to exceptions from within policies, without violating the expectations of where exceptions are raised from.