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

Add policy_class to permitted_attributes #789

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/pundit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ def authorize(user, possibly_namespaced_record, query, policy_class: nil, cache:
policy = if policy_class
policy_class.new(user, record)
else
cache[possibly_namespaced_record] ||= policy!(user, possibly_namespaced_record)
cache[{ policy_class: policy_class,
Copy link
Member

Choose a reason for hiding this comment

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

This is always nil here. What's the intention?

record: possibly_namespaced_record }] ||= policy!(user, possibly_namespaced_record)
end

raise NotAuthorizedError, query: query, record: record, policy: policy unless policy.public_send(query)
Expand Down
14 changes: 10 additions & 4 deletions lib/pundit/authorization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,14 @@ def policy_scope(scope, policy_scope_class: nil)
#
# @see https://github.com/varvet/pundit#policies
# @param record [Object] the object we're retrieving the policy for
# @param policy_class [Class] the policy class we want to force use of
# @return [Object, nil] instance of policy class with query methods
def policy(record)
policies[record] ||= Pundit.policy!(pundit_user, record)
def policy(record, policy_class: nil)
policies[{ policy_class: policy_class, record: record }] ||= if policy_class
Copy link
Member

Choose a reason for hiding this comment

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

While policies is technically marked as private, I'm cautious against modifying its behaviour/content.

We maybe should cache when policy_class is passed, but that's a bigger consideration than adding the feature to permitted_attributes.

I believe that the cache behaviour of authorize(..., policy_class: SomePolicy) should be the same for permitted_attributes(..., policy_class: SomePolicy), and today the behaviour is to entirely bypass the cache.

policy_class.new(pundit_user, record)
else
Pundit.policy!(pundit_user, record)
end
end

# Retrieves a set of permitted attributes from the policy by instantiating
Expand All @@ -113,9 +118,10 @@ def policy(record)
# @param record [Object] the object we're retrieving permitted attributes for
# @param action [Symbol, String] the name of the action being performed on the record (e.g. `:update`).
# If omitted then this defaults to the Rails controller action name.
# @param policy_class [Class] the policy class we want to force use of
# @return [Hash{String => Object}] the permitted attributes
def permitted_attributes(record, action = action_name)
policy = policy(record)
def permitted_attributes(record, action = action_name, policy_class: nil)
policy = policy(record, policy_class: policy_class)
method_name = if policy.respond_to?("permitted_attributes_for_#{action}")
"permitted_attributes_for_#{action}"
else
Expand Down
6 changes: 3 additions & 3 deletions spec/authorization_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,9 @@
end

it "caches the policy" do
expect(controller.policies[post]).to be_nil
expect(controller.policies[{ policy_class: nil, record: post }]).to be_nil
controller.authorize(post)
expect(controller.policies[post]).not_to be_nil
expect(controller.policies[{ policy_class: nil, record: post }]).not_to be_nil
end

it "raises an error when the given record is nil" do
Expand Down Expand Up @@ -155,7 +155,7 @@

it "allows policy to be injected" do
new_policy = OpenStruct.new
controller.policies[post] = new_policy
controller.policies[{ policy_class: nil, record: post }] = new_policy

expect(controller.policy(post)).to eq new_policy
end
Expand Down