varvet / pundit

Minimal authorization through OO design and pure Ruby classes
MIT License
8.28k stars 630 forks source link

Getting a scope for a specific action #368

Open AndrewSwerlick opened 8 years ago

AndrewSwerlick commented 8 years ago

Is there a mechanism for having a policy with different scopes for different kinds of actions. For example, I have a license table that stores all the licenses for our application that an organization has purchased. When users login, they have a page where they can select a license to operate under. Company administrators have pages where they can view a license, and all its associated details. "select" vs "view" are two very different permissions. Users may be able to select licenses from their entire company, but may only be responsible for the administration of licenses in their department, and so can only view those. However in both cases I have reasons to want to quickly create a scope of the records the user is authorized to access. For "select" I need to populate drop downs, for "view", I need to build out tables on index pages. I don't see how I can do this in pundit with the current scope setup.

nbekirov commented 8 years ago

Is there a mechanism for having a policy with different scopes for different kinds of actions.

If I'm getting this right maybe you'll be interested in this discussion: Allows Scope to receive additional arguments

AndrewSwerlick commented 8 years ago

@nbekirov I have seen that, but it seemed a little different in goals, even if the implementation ends up being similar. From the example included, it looks like that user it trying to pass in somewhat arbitrary arguments, and for that reason @jnicklas suggests wrapping in a domain object. I'm interested only in passing additional "verbs" into scope, generally the same "verbs" that correspond to the policy's method names.

davidstosik commented 8 years ago

:+1:

It would be nice to have different scopes for different actions, such as:

Would there be any red flag in implementing multiple Scopes in the same Policy?

# user_policy.rb
class UserPolicy < ApplicationPolicy
  class Scope < Scope  # equivalent to `< ApplicationPolicy::Scope`
    def resolve
      if user.admin?
        scope
      else
        scope.confirmed.enabled  # filter unconfirmed, and disabled accounts
      end
    end
  end

  class EmailScope < Scope  # equivalent to `< UserPolicy::Scope`
    def resolve
      super.where.not(users: {id: user.id})  # Based on the UserPolicy's main Scope
    end
  end
end

# in my views/controllers
# ...
mailable_users = UserPolicy::EmailScope.new(current_user, User).resolve
# ...
jnicklas commented 8 years ago

Yes, I think that's a nice pattern. Nothing wrong with that in my book! :+1:

jnicklas commented 8 years ago

@AndrewSwerlick this was actually something I thought about when I designed the interface. My thinking then was that resolve would work somewhat similarly to the query methods on policies, that is one could do different verbs instead of resolve. I couldn't come up with any way to tie that API together though. It always felt a bit odd to have to specify the method and there wasn't really a good way around that.

I think for backward-compatibility reasons it'll be hard to switch to an API which allows different verbs to be passed in somehow, but it would definitely be a nice feature, IMO.

Envek commented 8 years ago

+1 on having some method like policy_scope but allowing to retrieve exact scope that you want for this model at time. Writing something like this in every action in, say, CallsJournalController isn't so pleasant:

CallPolicy::JournalScope.new(pundit_user, Call.all).resolve

@davidstosik, thank you, I'm currently using your example, it's working fine for me.

eLod commented 8 years ago

what about resolve having arguments? i think that is clean (e.g. we don't pollute the init arguments), and you can have additional arguments. i think it has the nice benefit letting the developer choose whatever pattern he likes (a simple case, multiple methods, multiple subclasses, etc.).

caryfitzhugh commented 8 years ago

I sortof think that

policy_scope(Post, :visible_users)

where the second argument defaults to :resolve would be very useful. Is that something people are interested in supporting if I implemented it in a PR?

So, I would add additional methods into the Scope class, and reference them by the symbol I pass into policy_scope

formigarafa commented 8 years ago

Hello all,

I was taking a look into this and I realized something. there is a kind of misalignment between record policy methods and scope policy controller helper methods.

record scope
fetch policy policy(record) --none--
apply authorization authorize(record, query = nil) policy_scope(scope)

What has been done so far, is just fetching the policy and calling a method on it. Same as you would with policy(post).update?

I think we could add a method like authorize_scope(scope, query = nil).

def authorize_scope(scope, query = nil)
  query ||= params[:action].to_s
  @_pundit_policy_scoped = true
  scope_policy(scope).public_send(query)
end

and obviously, complement with the scope_policy(scope) method for symmetry with policy(record).

def scope_policy(scope)
  scope_policies[scope] ||= PolicyFinder.new(scope).scope!.new(pundit_user, scope)
end

def scope_policies
  @_pundit_scope_policies ||= {}
end

This would be even backward-compatible as the any existing use of policy_scope would still work.

policy scope could then be defined as just:

def policy_scope(scope)
  authorized_scope(scope, :resolve)
end

This would result in a more symmetric set of features that would look like this:

record scope
fetch policy policy(record) scope_policy(scope)
apply authorization authorize(record, query = nil) authorized_scope(scope)

And policy_scope(scope) would be just a legacy specific case.

jnicklas commented 7 years ago

Sorry it took me so long to respond to this, I think @formigarafa's comment is really interesting and well reasoned.

I really like that it preserves backward compatibility, but it seems a bit confusing to me that we'd still have policy_scope (though deprecated) and scope_policy which have different semantics. Also it should probably be authorized_policy_scope for consistency's sake, though that's rather long.

bradgessler commented 5 years ago

About a year ago our company was reading this exact thread trying to figure out if we could use Pundit or make our own gem that solves this problem. We loved Pundit so much we knew we wanted to take a PORO approach, so we built Moat. Moat’s “opinion” is that security should be controlled primarily at the scope level (and support lots of scopes) instead of at the object level.

We’ve been using it in production now for over a year so it’s been battle tested. We also get our source code audited every year by a security firm and they were quite pleased to see our authorization logic organized into policy folders with corresponding specs.

Envek commented 5 years ago

Also there is action_policy gem (it is inspired by pundit too) which supports scoping among other features.

thedumbtechguy commented 11 months ago

I came looking for the solution @formigarafa outlined. It is clean and should be supported. I also prefer a scope first approach since it prevents the application from loading invalid records and wasting resources. It's also critical for some regulatory sectors. Loading the record from the database can be considered access in some industries, even if it is not rendered.

Combining this with record level authorisations gives you a very flexible and powerful authorisation system.

However, I would be careful with the naming here.

authorized_scope implies a noun, which is the mistake made with policy_scope. A noun should return an object, just like policy does. The ship has sailed with policy_scope (it was ideal), so naming here is critical to prevent a future occurrence of this.

I would propose something like

Thankfully, Pundit is plain objects (thank the stars) and it is easy to monkey-patch/refine this while we wait for it to be added.

formigarafa commented 11 months ago

Those helper methods on controllers are really handy bit to be honest I gave up on waiting for them in the gem. This gem is awesome, don't get me wrong, I still use it, but I rolled out my own and even redefined some others on application controller. My perception today is that the idea, the design pattern, the way to separate the responsibility is the best part of pundit. And like you mention, it can be just PORO. I made an ApplicationPolicy to inherit from in most Rails style where I add some basic reusable methods and made a more well defined initializer so all the policies can be instantiated without surprises. The simplicity of the interface is a white canvas on your app waiting for your creativity

thedumbtechguy commented 11 months ago

Totally agree!

Also, I thought about this a bit and authorized_scope does make sense since it is returning a scope.