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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Fixed

- Using `policy_class` and a namespaced record now passes only the record when instantiating the policy. (#697, #689, #694, #666)

## 2.1.1 (2021-08-13)

Friday 13th-release!
Expand Down
22 changes: 12 additions & 10 deletions lib/pundit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,23 @@ class << self
# authorized to perform the given action.
#
# @param user [Object] the user that initiated the action
# @param record [Object] the object we're checking permissions of
# @param possibly_namespaced_record [Object, Array] the object we're checking permissions of
# @param query [Symbol, String] the predicate method to check on the policy (e.g. `:show?`)
# @param policy_class [Class] the policy class we want to force use of
# @param cache [#[], #[]=] a Hash-like object to cache the found policy instance in
# @raise [NotAuthorizedError] if the given query method returned false
# @return [Object] Always returns the passed object record
def authorize(user, record, query, policy_class: nil)
policy = policy_class ? policy_class.new(user, record) : policy!(user, record)
def authorize(user, possibly_namespaced_record, query, policy_class: nil, cache: {})
record = pundit_model(possibly_namespaced_record)
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?

end

raise NotAuthorizedError, query: query, record: record, policy: policy unless policy.public_send(query)

record.is_a?(Array) ? record.last : record
record
end

# Retrieves the policy scope for the given record.
Expand Down Expand Up @@ -207,7 +213,7 @@ def verify_policy_scoped
# and current user and finally throwing an error if the user is not
# authorized to perform the given action.
#
# @param record [Object] the object we're checking permissions of
# @param record [Object, Array] the object we're checking permissions of
# @param query [Symbol, String] the predicate method to check on the policy (e.g. `:show?`).
# If omitted then this defaults to the Rails controller action name.
# @param policy_class [Class] the policy class we want to force use of
Expand All @@ -218,11 +224,7 @@ def authorize(record, query = nil, policy_class: nil)

@_pundit_policy_authorized = true

policy = policy_class ? policy_class.new(pundit_user, record) : policy(record)

raise NotAuthorizedError, query: query, record: record, policy: policy unless policy.public_send(query)

record.is_a?(Array) ? record.last : record
Pundit.authorize(pundit_user, record, query, policy_class: policy_class, cache: policies)
end

# Allow this action not to perform authorization.
Expand Down
22 changes: 22 additions & 0 deletions spec/pundit_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@
expect(Pundit.authorize(user, post, :create?, policy_class: PublicationPolicy)).to be_truthy
end

it "can be given a different policy class using namespaces" do
expect(PublicationPolicy).to receive(:new).with(user, comment).and_call_original
expect(Pundit.authorize(user, [:project, comment], :create?, policy_class: PublicationPolicy)).to be_truthy
end

it "works with anonymous class policies" do
expect(Pundit.authorize(user, article_tag, :show?)).to be_truthy
expect { Pundit.authorize(user, article_tag, :destroy?) }.to raise_error(Pundit::NotAuthorizedError)
Expand All @@ -66,6 +71,18 @@
# rubocop:enable Style/MultilineBlockChain
end

it "raises an error with a the record, query and action when the record is namespaced" do
# rubocop:disable Style/MultilineBlockChain
expect do
Pundit.authorize(user, [:project, :admin, comment], :destroy?)
end.to raise_error(Pundit::NotAuthorizedError, "not allowed to destroy? this Comment") do |error|
expect(error.query).to eq :destroy?
expect(error.record).to eq comment
expect(error.policy).to eq Pundit.policy(user, [:project, :admin, comment])
end
# rubocop:enable Style/MultilineBlockChain
end

it "raises an error with a invalid policy constructor" do
expect do
Pundit.authorize(user, wiki, :update?)
Expand Down Expand Up @@ -458,6 +475,11 @@
expect(controller.authorize(post, :create?, policy_class: PublicationPolicy)).to be_truthy
end

it "can be given a different policy class using namespaces" do
expect(PublicationPolicy).to receive(:new).with(user, comment).and_call_original
expect(controller.authorize([:project, comment], :create?, policy_class: PublicationPolicy)).to eql(comment)
end

it "works with anonymous class policies" do
expect(controller.authorize(article_tag, :show?)).to be_truthy
expect { controller.authorize(article_tag, :destroy?) }.to raise_error(Pundit::NotAuthorizedError)
Expand Down
4 changes: 4 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,10 @@ class CommentPolicy < Struct.new(:user, :comment)
def update?
true
end

def destroy?
false
end
end
end
end
Expand Down