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.

Sign up for GitHub

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

Jump to bottom

Allow authorize to pass arguments to policy methods. #50

Closed
wants to merge 2 commits into from
Closed

Allow authorize to pass arguments to policy methods. #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70c798c
Allow authorize to pass arguments to policy methods.
coop Aug 12, 2013
b11129e
Update README with `authorize` example.
coop Aug 12, 2013
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
27 changes: 26 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ Pundit makes the following assumptions about this class:
you want to check. This does not need to be an ActiveRecord or even
an ActiveModel object, it can be anything really.
- The class implements some kind of query method, in this case `create?`.
Usually, this will map to the name of a particular controller action.
Usually, this will map to the name of a particular controller action. The
query method may take zero or more arguments.

That's it really.

Expand Down Expand Up @@ -111,6 +112,30 @@ def publish
end
```

Any further arguments passed to `authorize` will be passed onto the policy as
arguments to the query method. This is most useful when the policy needs more
information in order to make a decision. For example:

``` ruby
class PostPolicy < Struct.new(:user, :post)
def remove_tag?(tag)
record.tags.include?(tag) and record.authored_by?(user)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This example is weak. Checking if the record already has this tag is not an authorization concern, it's business logic. Yes, I shouldn't be able to remove a tag that does not exist, but not because I could ever gain this ability. There is no possible case where someone else should be able to do this. Authorization should be solely for the purpose of who is allowed to do what. Adding in stuff like this which can easily be determined to always be applicable to everyone does not make sense.

Consider this: could there ever be a scenario where an admin would be allowed to remove a tag that doesn't exist? Obviously no, because that does not make any sense.

In a lot of our applications we have stuff like this creeping into authorization, and it quickly escalates and makes authorization completely unmanageable.

So once we skip that part, we no longer have a cause to send in the tag.

Aside from all of this, we could have just created a policy for the join model instead, so if we have a TaggingPolicy we would have access to both the post and the tag and we could have verified both of these things, thus there wouldn't have been a need to pass in extra arguments. I think this is what pretty much every case of wanting extra arguments boils down to. Convince me that there is something which cannot be covered simply by choosing the correct object to write a policy for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the biggest problem that I introduced was not using real code - I attempted to continue using the objects you created. I'm showing application code below which I hope will make the case clear. I'll address these out of order:

Aside from all of this, we could have just created a policy for the join model instead

For our application the API that we developed backgrounded the fact that there is a join model. We actually wrote two separate implementations, the one we decided to go with and one that brought the join model to the foreground (significantly more code and less clear intent). Our current API looks like:

# tug(master)$ cat app/models/project.rb 
class Project < ActiveRecord::Base
  has_many :collaborations, dependent: :destroy

  # Add the user to the project and transfer project ownership if they are the
  # only collaborator. If the user already belongs to the project, this method
  # does nothing.
  #
  # @return [User] User that was added.
  def add_user user
    if users.empty?
      users << user
      transfer_ownership_to_user user
    else
      users << user unless users.include? user
    end

    user
  end

  # Removes the user from the project by destroying the collaboration.
  #
  # @note After the collaboration has been removed the in memory collaborators
  #   is not updated. An explicit call to reload seems to be the only way to
  #   force an update.
  # @param user [User] User to be removed.
  # @return [User] User that was removed.
  def remove_user user
    raise NotCollaboratingError unless users.include?(user)

    collaborations.remove_user_belonging_to_project user, self
    reload
    user
  end

  # Transfers project ownership from an existing user to another user.
  #
  # @param user [User] User to become project owner.
  # @return [User] User that has received project ownership.
  def transfer_ownership_to_user user
    raise NotCollaboratingError unless users.include?(user)

    collaborations.transfer_ownership_of_project_to_user user, self
    user
  end
end

# tug(master)$ cat app/views/projects/collaborators.html.erb 
<ul class="list-group">
  <% project.users.each do |user| %>
    <li class="list-group-item media pinboard">
        <% if policy(project).remove_user?(user) %>
          <%= link_to 'Remove', project_collaborator_path(project, user), method: 'delete', class: 'btn btn-danger btn-small pin-right', style: 'top: 20px; right: 15px' %>
        <% end %>
      </div>
    </li>
  <% end %>
</ul>

From these snippets you can see that we don't have access to the collaboration at this point, nor is there really a requirement to. In fact adding it would make this code a lot less readable IMO. I'm happy to understand how you would have approached this differently.

This example is weak.

Fair point. I wanted to write documentation that was consistent with the objects that you'd constructed. My implementation for policy(project).remove_user?(user) shown above:

# tug(master)$ cat app/policies/project_policy.rb 
class ProjectPolicy < ApplicationPolicy
  # Determine if the user has permission to remove the specified user.
  #
  # @return [Boolean]
  def remove_user? other_user
    user != other_user && project_owned_by_user?
  end

  private

  def project_owned_by_user?
    record.owned_by? user
  end
end

I believe this policy method cleanly fits inside your description of Authorization should be solely for the purpose of who is allowed to do what. I can't think of another way to write this - again I'd be happy to be shown a better / different implementation.

end
end

class PostsController < ApplicationController
include Pundit

def remove_tag
@post = Post.find(params[:id])
@tag = @post.tags.find(params[:tag_id])
authorize @post, :remove_tag?, @tag
@post.remove_tag(@tag)
respond_with(@post)
end
end
```

You can easily get a hold of an instance of the policy through the `policy`
method in both the view and controller. This is especially useful for
conditionally showing links or buttons in the view:
Expand Down
4 changes: 2 additions & 2 deletions lib/pundit.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,10 @@ def verify_policy_scoped
raise NotAuthorizedError unless @_policy_scoped
end

def authorize(record, query=nil)
def authorize(record, query=nil, *args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still not super enthusiastic about the mixture of default arguments and "rest" arguments. I guess there isn't any way to work around this.

query ||= params[:action].to_s + "?"
@_policy_authorized = true
unless policy(record).public_send(query)
unless policy(record).public_send(query, *args)
raise NotAuthorizedError, "not allowed to #{query} this #{record}"
end
true
Expand Down
7 changes: 7 additions & 0 deletions spec/pundit_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ def destroy?
def show?
true
end
def known_author?(other_user)
true
end
end
class PostPolicy::Scope < Struct.new(:user, :scope)
def resolve
Expand Down Expand Up @@ -224,6 +227,10 @@ def destroy?
it "raises an error when the permission check fails" do
expect { controller.authorize(Post.new) }.to raise_error(Pundit::NotAuthorizedError)
end

it "passes extra attributes through to policy methods" do
controller.authorize(post, :known_author?, OpenStruct.new).should be_true
end
end

describe "#pundit_user" do
Expand Down