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

Getting a scope for a specific action #368

Open
AndrewSwerlick opened this issue Feb 26, 2016 · 15 comments
Open

Getting a scope for a specific action #368

AndrewSwerlick opened this issue Feb 26, 2016 · 15 comments

Comments

@AndrewSwerlick
Copy link

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
Copy link

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
Copy link
Author

@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
Copy link

👍

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

  • Users I'm allowed to see
  • Users I'm allowed to manage (me only, unless I'm admin)
  • Users I'm allowed to mail (all but me if I'm admin, only my friends otherwise)

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
Copy link
Collaborator

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

@jnicklas
Copy link
Collaborator

@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
Copy link
Contributor

Envek commented Mar 16, 2016

+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
Copy link

eLod commented May 25, 2016

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
Copy link

caryfitzhugh commented Jun 20, 2016

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
Copy link

formigarafa commented Jul 5, 2016

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
Copy link
Collaborator

jnicklas commented Nov 4, 2016

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
Copy link

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
Copy link
Contributor

Envek commented Aug 30, 2019

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

@thedumbtechguy
Copy link

thedumbtechguy commented Oct 26, 2023

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

  • scope: returns the policy scope object. analogous to policy. I agree with @formigarafa here.
  • scoped: perform the actual scoping, similar to authorize, but returns the scoped resources.

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
Copy link

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
Copy link

Totally agree!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants