Closed xanagi closed 2 years ago
@xanagi Thanks for the PR. I believe this will be fixed by https://github.com/varvet/pundit/pull/626 (merged but not yet released) which passes the actual record to the authorize
method which raises this error.
@xanagi Ah actually it's not fixed by that PR, but I think the authorize
method is the right place to fix this - if you'd like to submit a new pr fixing the issue there (and remove the unrelated jruby change) then I can have another look.
@dgmstuart Sorry for my late reply and thanks for your review! I removed the change to .travis.yml.
I agree that authorize
method has the same code, but I believe record
passed to NotAuthorizedError
should not lose the namespace information.
I think authorize
method is fine and just want to fix the message of NotAuthorizedError
...
(or maybe the message should contain the namespace information)
Could you give me some advice?
I believe
record
passed toNotAuthorizedError
should not lose the namespace information.
I don't understand what you mean by that - could you explain in different words? What's the impact?
Doesn't the code in the authorize
method have the exact same purpose as the code here? I'd prefer not to have duplicated knowledge in multiple different places in the code if we can help it.
I guess your intention is to strip namespace from record
in the authorize
method and pass it to NotAuthorizedError
initializer like this.
def authorize(user, record, query, policy_class: nil)
policy = policy_class ? policy_class.new(user, record) : policy!(user, record)
record = record.is_a?(Array) ? record.last : record
raise NotAuthorizedError, query: query, record: record, policy: policy unless policy.public_send(query)
record
end
I'm afraid this can make debugging difficult because NotAuthorizedError
's record
attribute don't have the namespace information .
The authorize
method is expected to return record not namespase array as fixed in #626.
On the other hand, I think NotAuthorizedError
is expected to have the original record
(namespace array).
Anyway, I totally agree that duplicated codes are not preferable. If you think I care too much about this, I'm happy to remove the duplicated code .
@xanagi ah I see your point 🤔
I think the larger design issue here is that we're passing around these arrays, but an array isn't really an appropriate object: maybe we'd ideally be passing around an object which responds to .model_name
rather than an array.
But that's a much bigger change than we can accommodate here.
@dgmstuart Thanks for your reply.
I think making record
an instance of a custom class (rather than Array
) is a great idea that resolves the issue here naturally!
For the time being, I think the change in this PR can help uses getting better error messages. Or please close this if introducing another duplicate does more harm than good.
Hi :wave:! I'm afraid other changes made this PR obsolete. Namespace information is now stripped before it's passed to the error class, which does lose namespace information from the record
.
However, the namespacing information in record
was only used for finding the policy class. The relevant namespace information is still available in the policy
accessor of the error and so the information is still available! :tada:
I also believe that the final default error message (in main
) now has the same format as it would have if this PR would have been merged, so hopefully the end-result is similarly useful.
Thanks for the PR though! 🎊
Pundit::NotAuthorizedError
provides the default error message like"not allowed to destroy? this Array"
when namespaced policies are used. This is because the record passed toPundit::NotAuthorizedError
is an Array.This PR modifies the error message so that it provides clearer information on the model. (e.g.
"not allowed to destroy? this Comment"
whenauthorize([:project, comment], :destroy?)
is called).