Allow authorize to pass arguments to policy methods.

[coop]

authorize currently assumes that each policy has enough information to make a decision. This isn't always the case as described in the policy below.

class ProjectPolicy
  # Determine if user is allowed to remove other_user from the project.
  #
  # @return [Boolean]
  def remove_user?(other_user)
    user != other_user && project.owned_by?(user)
  end
end

authorize has been patched to send any extra arguments to the policy which
makes the above possible.

class ProjectsController < ApplicationController
  include Pundit

   # This isn't RESTful but shows intent.
  def remove_user
    authorize project, :remove_user?, other_user

    project.remove_user other_user
    respond_with project
  end

   private

  def project
    @project ||= current_user.find_project params[:id]
  end

  def other_user
    @other_user ||= project.find_user params[:user_id]
  end
end

[coop]

I have just realised https://github.com/elabs/pundit/pull/48/files implements similar functionality. When I started on the pull request there was no open pull requests. I feel that there is some uncertainty around the scoping implementation in #48 which means the authorize change cannot come in. This pull request only aims to change the behaviour of authorize.

[thomasklemm]

Thanks for this PR too, especially the great explanation and for including the specs, Tim (@coop)! With #48 and #50 now both making good cases, I agree that there should be the option to pass additional information into the authorize call.

@coop Could you do me a favor and explain this addition in the Readme?

[coop]

README updated. Thanks @thomasklemm for looking.

[tatey]

Would love to see this merged in. What's happening?

[coop]

@thomasklemm is there anything else you need from me to get this merged?

[thomasklemm]

@coop This PR looks great to me.
@jnicklas Could you take a look / merge / release a new gem version?

[jnicklas]

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.

[coop]

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.

[coop]

@jnicklas have you had a chance to go through my explanation? I'm really interested to know your thoughts.

[coop]

@jnicklas or @thomasklemm any movement on this?

[jnicklas]

I think your explanation makes sense, I'd like to see discussion about this and #38, since they conflict. I think that this is probably a more important use case, and there are other ways to solve #38. what do you guys think?

[tatey]

I'm just an observer, but I like the trailing arguments in this PR. It feels like send which is already a familiar API.

[coop]

I think #38 would be better served with i18n as @jnicklas mentioned. For the example from the PR the translation would look something like I18n.t('create', scope: 'pundit.posts', default: 'pundit.not_authorized') (I'm not sure if default takes another translation but the idea is there).

[jnicklas]

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.

[coop]

@jnicklas what's holding this up from being merged? Is there anything that needs clarification or further work?

[thomasklemm]

@coop Here's another approach possible today:

# tug(master)$ cat app/policies/user_policy.rb 
class UserPolicy < ApplicationPolicy
  # In the case of a UserPolicy, the other_user is the record
  # while the user acting is the still the user
  alias_method :other_user, :record

  # Determine if the user has permission to remove the specified user.
  #
  # @return [Boolean]
  def remove?
    user != other_user && project_owned_by_user?
  end

  private

  def project_owned_by_user?
   # EDIT: I'm wrong here, sorry. A user can have may projects, 
   # and we don't know which one is referenced without additional information
    user.project.owned_by? 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(user).remove? %>  <!-- changed -->
          <%= 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>

@coop Could you assess if this works with / conflicts with your current UserPolicy? Today, I'm the one thinking we should go with associations...

EDIT: I'm wrong here, sometimes associations may lead to one particular record, but not in this case and not in all cases. Indeed, we need the current_user, project and user_to_be_removed records for permission checks here.

[thomasklemm]

I've edited my statement above, it was incorrect. We need three pieces of information for permission checks here.

What you should be able to do without any changes to Pundit is manually retrieving policies and applying .

class ProjectsController < ApplicationController
  def remove_collaborator
    user = @project.users.find(params[:id])

    # Version 1 (edited, see below): Retrieving a policy manually 
    # and perform a query on it will work today
    unless ProjectPolicy.new(current_user, @project).remove_collaborator?(user)
      raise Pundit::NotAuthorizedError
    end

    # Version 2: Requires this PR, won't work right now
    authorize @project, :remove_collaborator?, user

    # Version 3: Additional arguments are incorrectly passed 
    # as the query argument, will never work
    authorize @project, user
    ...
  end
end

class ProjectPolicy < ApplicationPolicy
  def remove_collaborator?(collaborator)
     project.owned_by?(user) && collaborator != user
  end
end

How about using ProjectPolicy.new(current_user, @project).remove_collaborator?(user) in cases where additional arguments are necessary? We could advertise this more prominently in the Readme with an example.

[thomasklemm]

Actually retrieving the policy and querying manually doesn't raise a Pundit::NotAuthorizedError, so a minimal implementation of Version 1) above could be:

unless ProjectPolicy.new(current_user, @project).remove_collaborator?(user)
  raise Pundit::NotAuthorizedError
end

Using the authorize call would additionally mark authorization as performed and use whatever error message retrieval we might come up, e.g. I18n error messages as proposed in #68.

[equivalent]

+1 for Coop pull request, had the same problem #88 ...although I was passing args differently

[sankage]

I just ran into another possible use case for this. I have time-limited access to certain actions, so I need to pass a time variable and check if we are in the currently allowable window.

[phlegx]

@coop nice work! 👍

[aried3r]

Any news on if this or #133 is going to get merged? I'm pretty much in the same boat as the others here, needing additional information for the authorization (much like @phlegx).

[jnicklas]

I've yet to see someone describe a sensible use case for these things. To me @lanej's example with checking the params for update actions is the only example where this makes sense to me, but the README example in #133 isn't awesome, and I'd personally rather solve that problem than adding a generic, easily misused feature.

@sankage's time sensitivity thing is only really an issue if you buy into the whole dependency injection thing, otherwise, why not just use Time.now or even ActiveSupport's Time#past?.

@phlegx's parent object should already be accessible through the object itself. If an object is a parent of another object, IMO that relationship should be expressed at the domain model layer. There should be a record.parent, so no need to pass that in.

So I don't get it, I've never needed this feature, I've never seen a compelling use case for this feature. Yet everyone wants it, but can't describe what for.

Then we also have this pattern https://github.com/elabs/pundit#additional-context which can be used if authorization should be based on something other than the currently logged in user. To be honest I had to think for a long time before I came up with the use-case documented in the README, and I still don't think it's a very good one. I have a feeling this will be liberally abused for all kinds of things as well.

Pundit is about encapsulating and encouraging a specific design pattern. It's opinionated. If we change the design pattern we're encouraging, I want there to be a reason for it, at least.

[aried3r]

I've yet to see someone describe a sensible use case for these things. To me @lanej's example with checking the params for update actions is the only example where this makes sense to me

If I understood #133 correct it would work in my case.

class Organization < ActiveRecord::Base
  has_many :users
  has_many :projects, dependent: :destroy
end

Now in my projects controller, I want to check for #index if the current_user is in the same organization as the @projects he's trying to access (the actual content of @projects is handled via scopes).

class ProjectsController < ApplicationController
  def index
    @organization = Organization.find(params[:organization_id])
    authorize @organization
    @projects = policy_scope(Project)
  end
end

With the following policy for organizations:

class OrganizationPolicy < ApplicationPolicy
  def index?
    user.organization_id == record.id
  end
end

If I could pass params[:organization_id] to the project policy, I could handle #index? there, where I feel that it belongs. The alternative I see is doing something like @projects.first.organization == user.organization or checking if all projects in @projects are of the same org, which of course is stupid.

I could be missing something, maybe the @projects.first solution is the preferred one, I just don't want to make things too convoluted.

[jnicklas]

I'd go for authorize @organization, :list_projects? or something similar in that case. index actions always tend to be a bit awkward to express, I'll agree to that, and I haven't found a great solution either.

Another alternative is to instantiate the policy class manually.

[aried3r]

Thanks for the reply. Yeah, the index actions feel awkward, I'm not sure
how I'd approach it myself.

Could you provide an example of how you'd do it instantiating the policy
class manually? I think I have a clue on how to do it, but it doesn't hurt
asking the creator. ;)

On 25 September 2014 09:21, Jonas Nicklas notifications@github.com wrote:

I'd go for authorize @organization, :list_projects? or something similar
in that case. index actions always tend to be a bit awkward to express,
I'll agree to that, and I haven't found a great solution either.

Another alternative is to instantiate the policy class manually.

—
Reply to this email directly or view it on GitHub
#50 (comment).

[jnicklas]

I'm closing this due to the same reason as #133. Please see my explanation there. Please respect that this is my final decision on the matter.