pass arbitrary arguments to #authorize

[lanej]

this facilitates the usecase where an query method would want to conditionally authorize based on the content of the request parameters

[lanej]

I need something that allows

def update?(params)
  return true if subject.staff?

  if params.has_key?(:accepted) && record.email != subject.email
    forbidden!("You cannot accept an invitation that you do not own.")
  end
end

[thomasklemm]

Thanks for your PR, Josh!

What you're reasoning behind pulling this whitelisting based on attributes into the policy?
Couldn't you instead slice the permitted attributes and pass only them to the update / update_attributes statement?

# app/controllers/invitations_controller.rb
class InvitationsController < ApplicationController
  def update
    load_invitation

    # IMO `authorize` should only determine whether the user has the right 
    # to update the invitation at all
    authorize @invitation

    if @invitation.update(invitation_params)
      # success
    else
      # failure
    end
  end

  private

  def load_invitation
    @invitation ||= Invitation.find_by!(code: params[:invitation_code])
  end

  # The permitted attributes for this user can be sliced as needed
  # White-listing attributes here to me feels more secure 
  # than blacklisting individual keys elsewhere
  def invitation_params
    params.require(:invitation).permit(*policy(Invitation).permitted_attributes)
  end
end

# app/policies/invitation_policy.rb
class InvitationPolicy < ApplicationPolicy
  alias_method :invitation, :record

  def update?
    user
  end

  def permitted_attributes
    fields = []
    fields << :accepted if user.email == invitation.email
    fields
  end
end

Pundit with Strong Parameters (or just regular params_hash slicing): https://github.com/elabs/pundit#strong-parameters

I feel that one might easily forget to blacklist a single key, so I'd strongly opt for a white-listing approach.

In your example of accepting an invitation, one might also introduce a custom authorize @invitation, :accept? and an invitations#accept action.

What do you think?

[lanej]

@thomasklemm in my case, I would like to provide custom feedback based on the value of the permitted attribute given.

my mistake, i should mentioned that forbidden! in original example raises Pundit::NotAuthorizedError

[thomasklemm]

What happens if someone introduces another sensitive flag to the model and forgets to blacklist the attribute?

[thomasklemm]

It honestly doesn't feel right to me to leave an authorization layer that open to the change that someone forgets to include a certain statement. I do understand though that you might want to provide custom feedback on why the something is forbidden. A two-tiered approach would be my choice then: 1) The white-listing of only permitted attributes and users who may perform certain actions. 2) A cosmetic layer in front sitting in the controller or some kind of custom class slicing the params and raising custom error messages, rendering different errors or the like in each scenario. This might lead to some duplication, though I guess worth it when dealing with security.

[lanej]

We have strict whitelisting being performed in the controller, being passed directly and tested separately.

Something like:

update_params = params.require(:membership).permit(:accepted, :role)
authorize @membership, :update?, update_params

You cannot deny that some authorization cases, especially in the update case, are complex beyond the capabilities of strong_parameters. I believe #50 is attempting to say something similar.

[thomasklemm]

Slicing params can certainly become complex, but I don't perceive a PostPolicy to be responsible for slicing incoming params. You can raise a Pundit::NotAuthorizedError from anywhere and even use the built-in way for customizing error messages. I can feel your need for handling complex parameter permissions, however I don't perceive pundit to be the tool for this job. One of the regular contributors here created arcane for this matter, which might be a good starting point for rolling a custom solution.

[lanej]

I do agree that the policy should not be responsible for slicing. That is why I provided an example where that happens in the controller.

However, it seems well within the policy's purview to determine which parameter values are not authorized and to express that reason.

[thomasklemm]

Does this mean that editors are allowed to publish posts? Native pundit might be

def publish?
  user.editor?
end

Are these params to be posted in by the user, or just for the pundit implementor to customize the flow of authorization?

[lanej]

A bit of a contrived example I admit. They were meant to be params posted by the user.

[jnicklas]

I'm in favour of putting this in there. This PR is the least contrived for this feature I've seen, and it is the most requested feature, by far. I've always been worried that people would rampantly abuse this feature and totally miss the point of Pundit. So the question becomes, is Pundit really only a set of best practices, or is it a library? My feeling though it that we should yield to popular demand, and that there really are valid use cases out there. The update? use case that @lanej pointed out is pretty good. I've been racking my brain around how to handle update actions properly, since authorization needs to occur based on both the current state of the world, and the proposed change. Often the difference doesn't matter, but when it does, it's a pretty annoying problem with the way Pundit is now.

I do have a concern though, and that's the notion of exception-based workflow in the query methods this introduces. I think this is a huge mistake. The whole point of policy objects is that they can be used, not just in controllers, but also in views, or wherever else they might be needed. Always having to check for exceptions whenever they are used is problematic. I would strongly discourage throwing exceptions in the query methods.

I think it goes without saying that the example that @lanej posted could easily be refactored into something where exceptions are not needed. For example, you could have added a new query method called accept_invitation? or something, and moved the accept error message logic into the controller.

At the very least, this is a completely separate change, so it violates the single responsibility principle of pull requests ;) @lanej please remove the change to the permit matcher from the PR, and I would be in favour of merging this.

[lanej]

thanks @jnicklas. I've removed the commit containing the change to the permit matcher

[lanej]

@jnicklas i see your point about the exception-based workflows.

Simplest way to work within the framework and facilitate the view usecase would be to build in some convention around update? and update!, which individual users can do on their own.

Slightly more complicated way would be to make the return value an object that accumulates error messages (more or less like ActiveModel::Errors). That way the 'why' can be expressed without exception throwing.

[thomasklemm]

@jnicklas I feel highly skeptical about passing user-supplied params into the policy action. Since when should you gain admin privileges by adding the magical key admin=true into the query string?

IMO authorization should be decided on the basis of the user and the record and their relationship.

authorize @post, :publish?

def publish?
  user.editor?
end

What might be ok (and the source of discussion in #48 and #50) is passing options into a policy.

authorize @post, :publish?, collaborator: true
authorize @post, :publish?, collaborator: false

def publish?(options = {})
  user.editor? || user.collaborator_on?(post.blog) if options[:collaborator]
end

But again, we've had lots of discussion on this issue and never seen a code example that says: I would benefit with some options being passed in. I'll reconsider anytime there is a good example, but I oppose this PR, for two reasons:

• No user-supplied magic key should IMO open your authorization layer (...?admin=true)
• The example is weak. If we cannot include a proper example in the Pundit specs, we should not include the feature.

[thomasklemm]

@lanej Sorry for my exceptionally strong opinion and infavorable opinion in this PR, I really feel you're trying to do something with Pundit that it is currently not designed for.

If I understand you correctly, you are looking for a good way to authorize values that certain people may set on certain attributes. Again, not only slicing the params, but making sure that an admin can set the state field to [:under_review, :accepted, :rejected, :published], while a reviewer may only set it to [:accepted, :reviewed]. Is that your intention?

In the world of strong params there is no concept of a user, therefore we need to roll it ourselves

def article_params
  # If the user is a reviewer and tries to set a value for `state`
  # that he is not allowed to set, we raise a NotAuthorizedError.
  unless user.admin?
    unless params[:article][:state].in? [:accepted, :rejected] # Not entirely failsafe :)
      raise Pundit::NotAuthorizedError, "No permission to set the state you're trying to set"
    end
  end

  # The usual
  params.require(:article).permit(:state)
end

I suggest we either discuss first class support for permissible values for certain attributes here and make this feature a first class citizen, or build a new library aroud that approch (that can also raise a Pundit::NotAuthorizedError or a custom error modeled alongside Pundit's now I18n-friendly error messages).

[lanej]

@thomasklemm I understand what you're seeing and I can appreciate your opinion. The thing that grinds me about the above example is that all that takes place in the controller and is outside of the testable boundaries of the policy.

I would support either permissible values as first class citizens and/or bolting on an additional library.

[jezstephens]

How about this example: I have a resource which should be accessible if the authenticated user is the owner of the resource or a key parameter matches the resource's secret key. I could do this:

def show
  authorize resource, :show?, key: params[:key]
  respond_with resource
end

class ResourcePolicy < BasePolicy
  def show?(params={})
    record.owner == user || params[:key] == record.key
  end
end

Another example: "A video can be viewed only if the user has a paid subscription and the video is accessed from Europe." In this case you might pass in the request origin as a parameter to the policy.

def show
  authorize video, :show?, region: request_region
  respond_with video
end

class VideoPolicy < BasePolicy
  def show?(options={})
    return false unless user.subscriber?
    options[:region] && record.available_in_region?(options[:region])
  end
end

In these examples I want to make an authorization decision based on contextual information besides the authenticated user and the resource being acted on. @thomasklemm, do you consider such decisions to be beyond the scope of Pundit?

I would add that I wonder whether it might be better to enable somehow passing the additional data to the policy constructor rather than to the query methods, to allow a policy to be constructed once in the controller, then used for example in the view without having to handle the additional parameters in there.

[thomasklemm]

@jezstephens Two good examples that passing options into the authorize call make sense. The idea of passing them into the policy instance instead of the query methods might be worth investigating, as policy(@video).show? in the view is much nicer to handle than policy(@video).show?(region: request_region).

[Cominch]

Another use case where options are really useful:

I need to...

authorize @resource, :show?, @project

... to decide, if current_user can show the resource, which can be assigned to different projects, and the user/project association can not be clearly derived.

EDIT: I like #48

[thomasklemm]

@Cominch You could authorize the join model (with a ProjectResourcePolicy), where you have access to user, project and resource. Example Gist. Hope that helps.

[seku]

+1 for the pull request (i'm not sure if i should copy my comment message into #50)
Place explain if i'm wrong that this pull request was already approved and only Readme example is missing ?? Or there are still some doubts ??

[polianych]

+1 for the pull request. My story is that user role and mostly for policy_scope at index actions depends on other model current_site, that depends on request.subdomain. How i should handle that better? My solution is, but it is ugly.

def pundit_user
  {
    user: current_user,
    site: current_site
  }
end

[thomasklemm]

@polianych Is a user always connected to exactly one site?

[polianych]

@thomasklemm the problem is that users have many sites as admin. But I can solve it because every content belongs_to site. Other problem is that policy_scope for index actions should know about site_id. It depends on request.subdomain.

[jnicklas]

So this PR has been open for a long time, and activity here has died down a bit. This is probably the most requested feature for Pundit, and so I realize that this will be a bit of an unpopular move, but I have decided to close this PR without merging it.

Here are my reasons for doing this:

• Currently using policies is straightforward and obvious. The query method dictates what we are asking, and the object dictates what we are asking from. In the cases where multiple objects are involved in whether a particular action should be allowed, they should be wrapped up in something, for example by utilizing the form object pattern. This is good practice anyway, and Pundit is all about encouraging good practice.
• Adding parameters to the query methods makes them harder to use in views, adding parameters to the policy constructor makes the caching situation of policies (introduced after this PR iirc) much more non-obvious.
• The usecase that @lanej has described, which I think is valid, is much less relevant given the establishment of the permitted_attributes pattern. I have an upcoming PR which expands this functionality and should solve @lanej's usecase in a much more elegant way.
• The context pattern established in the README since this PR solves a lot of what people want to use this kind of pattern for, and it's a much better, much more convenient solution. We use this very successfully in a multi-tenant app, and it works very nicely. If we would have to pass in the tenant everywhere, a lot of the convenience of Pundit would be lost.
• This is a feature which is very, very easily abused, and by Murphy's law will be abused. The original intent behind Pundit was establishing a simple good-practice pattern for authorization, we should not be encouraging bad patterns.

I want to clarify that I have thought about this a lot. Please respect that this is my final decision on the matter.

[lanej]

Proper explanation. Thanks for the time @jnicklas