Skip to content
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

Strip namespace when using policy_class #697

Merged
merged 3 commits into from
Jan 4, 2022
Merged

Conversation

Burgestrand
Copy link
Member

@Burgestrand Burgestrand commented Jan 4, 2022

Closes #689
Closes #694
Closes #666

Using policy_class and namespacing are two different ways of doing the same thing (specifying which policy to use). When both are given, we ignore the namespacing and rely on policy class since it's more specific — when we do so, we need to strip the namespace when passing the record to the policy class.

This is a refactor of #694.

TODO:

  • Update the changelog.

@Burgestrand Burgestrand marked this pull request as ready for review January 4, 2022 13:56
@Burgestrand Burgestrand merged commit 46af662 into master Jan 4, 2022
@Burgestrand Burgestrand deleted the kbs/refactor-694 branch January 4, 2022 14:06
@sedubois
Copy link

NB: I used to override def policy(record) (to add a namespace to the records and thus load namespaced policies when in an admin context) but that method is not called by def authorize any more, so it makes the overriding more difficult. Now I have to override both def pundit and def authorize.

@dgmstuart
Copy link
Collaborator

@sedubois have you tried the suggestion laid out in https://github.com/varvet/pundit#policy-namespacing?

If it doesn't work, please raise an issue.

policy = if policy_class
policy_class.new(user, record)
else
cache[possibly_namespaced_record] ||= policy!(user, possibly_namespaced_record)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be a breaking change, and I'm curious whether or not that was intentional. Our code uses the authorize(record) method below which used to find the policy_class with policy but this now uses policy! in that situation.

Can you please verify that you meant to force a more explicit definition of policy_class? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Knee-jerk reaction is that's indeed unintended! We had some other things that were affected by this move that might also be unintended, so I'd wager it's very likely we'll make some adjustments to this code in the near future.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reply and opening a new issue. I'll stick to 2.1.1 until this is worked out. Let me know if there's anything I can do to help.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @adherr! This was a long time ago, but I'm here again for other reasons.

While the previous implementation used to use policy(record), it still relied on the method returning an actual policy because the next call would be policy.public_send(query) which would fail with a NoMethodError if nil was returned for the policy.

The main change is that after this we instead raise Pundit::NotDefinedError as opposed to the NoMethodError.

What part of this behaviour were you relying on that prevented the upgrade?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants