Allows additional methods to be used with Scopes

[davidbiehl]

A proposal for issue #368.

Adds an additional argument to the policy_scope methods that specifies the method on the scope that should be called. Defaults to :resolve for backward compatibility.

This makes Scopes more flexible by allowing multiple scopes to be defined on a Scope class and making them available for use in controllers or views with existing helper methods.

[ce07c3]

Nice addition. Might wish to call it scope instead of method?

[davidbiehl]

The methods I'm adding the argument to already have an argument called scope ... I'm not a huge fan of method but it seemed to be the most straightforward name considering scope was already taken. Do you have any other suggestions?

[davidbiehl]

Maybe resolver? It might be too semantically similar to the default :resolve tho.

[ce07c3]

@davidbiehl I have little hope that this will ever be merged so I am forking and working from there.

[davidbiehl]

For anyone that stumbles on this, you can also just include the following method in your ApplicationController. This does completely override the default helper method, so proceed with caution.

def policy_scope(scope, method = :resolve)
  policy_scope = PolicyFinder.new(scope).scope
  policy_scope.new(current_user, scope).public_send(method) if policy_scope
end

[masciugo]

@davidbiehl thanks for your workaround. I had to add @_pundit_policy_scoped = true in order to have it working with verify_policy_scoped. See https://github.com/varvet/pundit/blob/master/lib/pundit.rb#L210-L213

  def policy_scope(scope, method = :resolve)
    @_pundit_policy_scoped = true
    policy_scope = PolicyFinder.new(scope).scope
    policy_scope.new(current_user, scope).public_send(method) if policy_scope
  end

[Linuus]

@davidbiehl Sorry for this PR being left alone for soo long.

I'm a bit curious if you have some more specific use cases for this?

My first thought is that Pundit should do the base scoping and not cater for different views. Like, if I'm allowed to see my organization's articles then that's the base scope. Whether I only can see published articles in some views and unpublished in another is not Pundit's responsibility (I'm open to be convinced about this :) ).

I mean, something like this seems appropriate for me:

# In ArticlePolicy
def resolve
  scope.where(organization_id: user.organization_id)
end

# In FooController
policy_scope(Article).published

# In BarController
policy_scope(Article).unpublished

# In BazController
policy_scope(Article)

I don't see the benefit of passing another argument which controls the extra scoping for different scenarios.

What do you think?

[immerrr]

@Linuus consider the following example.

DISCLAIMER: I have only recently started working with Rails and Pundit was shown to me literally less than an hour ago, so my code might have odds and quirks, but should be enough to convey the idea.

Suppose in your example a user can have roles assigned on per-article basis, e.g. 'READER', 'EDITOR', 'AUTHOR' and so on. It is handled by an ArticleUser many-to-many association with a role attribute. A scopeless policy would look something like this:

class ArticlePolicy
  def show?
    user.admin? || article.article_users.any?(user: user)
  end

  def update?
    user.admin? || article.article_users.find_by?(user: user, role: ['EDITOR', 'AUTHOR']) 
  end

  def delete?
    user.admin? || article.article_users.find_by?(user: user, role: ['AUTHOR']) 
  end
end

Then the "base scope" would be something like

  class Scope
    attr_reader :user, :articles

    def initialize(user, articles)
      @user  = user
      @articles = articles
    end

    def resolve
      if user.admin?
        articles.all
      else
        articles.where('EXISTS(SELECT 1 FROM article_users WHERE article_id = articles.id AND user_id = ?)', user.id)
      end
    end
  end

With that base scope,

readable_articles = policy_scope(Article)

will return a list of readable articles, no problem.

The problem is that to go from readable_articles to deletable_articles for non-admin users you need to refer to article_users table again, but now filtering by AUTHOR role. It can be remedied somewhat by doing an INNER JOIN instead of EXISTS in resolve method and then doing

readable_articles.where('article_users.role = ?', 'AUTHOR')

but that could be more expensive performance-wise and that would mean we would also have to pay the JOIN performance penalty for the base scope. And you also need to take care if a user can have multiple roles for an article, because then you need to deduplicate the scope too.

UPD: In addition to that, now the article_users logic is no longer neatly tucked inside just one policy class. If you are lazy, it now lives everywhere in your code, if you are not lazy and add a scope to the base model, well, it is two places, but still two places that have to be updated in lock-step.

Now, if only at the moment of resolve's execution we would know what action is expected to be run on the objects in scope, we could do the filtering right away with the second scope method:

    def resolve_for_delete
      if user.admin?
        articles.all
      else
        articles.where('EXISTS(SELECT 1 FROM article_users WHERE article_id = articles.id AND user_id = ? AND role = ?)', user.id, 'AUTHOR')
      end
    end

And avoid any performance penalties or deduplication problems.

[Linuus]

@immerrr why use a scope for the delete action? You can just authorize that specific article in the destroy? method as you do in your example?

[immerrr]

@immerrr why use a scope for the delete action? You can just authorize that specific article in the destroy? method as you do in your example?

For example, I'm implementing a multi-choice form to batch-delete articles, then I want a scope to be able to display the list of articles that a user can destroy and maybe paginate through it.

And I mean, I can check objects one by one with destroy? method, but it's the same thing as the "basic" scope: you can go through a Rails relation and authorize objects one by one for show? too, it's just that it is wildly inefficient, so there is a better way that offloads filtering to the database, Policy::Scope.

[vcavallo]

@Linuus

I'm a bit curious if you have some more specific use cases for this?
I don't see the benefit of passing another argument which controls the extra scoping for different scenarios.

What do you think?

Here's a potential simple example: date range params passed in from the request.

So you want to limit the results based on the date range first and foremost and then apply some Pundit-controlled policy restrictions as well - but every user type will definitely be scoped down by those date params. You'd have to apply a sort of "where dates in" query to the model first (in the controller) and then pass that subset of results into the Pundit scope. If I understand correctly, that would be (at least one) additional separate query, whereas if those params could be passed along into Pundit it could possibly all be handled in one larger query.

Incidentally, this happens to be exactly what I'm trying to do and how I found this issue :)

[dmitry]

What about add scopes similarly to the https://actionpolicy.evilmartians.io/#/scoping ?
Scopes are not just about activerecord relation scopes, but also could be about filtering request params, or even mutating an attribute data of the model, like the name of the user, which could be abbreviated in case when other users might not see his real full name.

[Linuus]

@vcavallo

If I understand correctly, that would be (at least one) additional separate query, whereas if those params could be passed along into Pundit it could possibly all be handled in one larger query.

No. Well, it depends on your ORM, but most ORMs are lazy so there will only be one query executed in the end.
Also, what you want is to make it possible to pass additional params to the scopes, which is not what this PR is about.

@dmitry

What about add scopes similarly to the https://actionpolicy.evilmartians.io/#/scoping ?
Scopes are not just about activerecord relation scopes, but also could be about filtering request params, or even mutating an attribute data of the model, like the name of the user, which could be abbreviated in case when other users might not see his real full name.

IMO this is too complex of a feature to be in Pundit. It kind of goes against the core philosophy of the library. However, I don't think I'm the lead maintainer anymore so ultimately I guess it's up to @dgmstuart to decide.

[dgmstuart]

I'm just starting to get my head around this and I have a couple of small thoughts for starters:

• I believe naming a method method overrides Object#method, which can lead to some pretty funky situations. I've been burned by that before
• I'm still not clear on what sort of use case there is for having a single Scope class with two methods on it, rather two Scope classes with one method. If there are two scopes which share some logic then I wonder if a better solution would be to extract that logic to a third class?

The fact that we're passing class names rather than instances perhaps makes that less clean than it might otherwise be (eg. I don't see how we could have one Scope decorating another), but that's perhaps a bigger discussion.

[dgmstuart]

Ah wait, the suggestion is around having a variable called method not a method.

[dgmstuart]

Possibly dumb question:

For @immerrr's example does the following make any sense? What are the tradeoffs?

 class ReadScope
    attr_reader :user, :articles

    def initialize(user, articles)
      @user  = user
      @articles = articles
    end

    def resolve
      if user.admin?
        articles.all
      else
        articles.where('EXISTS(SELECT 1 FROM article_users WHERE article_id = articles.id AND user_id = ?)', user.id)
      end
    end
  end

 class DeleteScope
    attr_reader :user, :articles

    def initialize(user, articles)
      @user  = user
      @articles = articles
    end

    def resolve
      if user.admin?
        articles.all
      else
        articles.where('EXISTS(SELECT 1 FROM article_users WHERE article_id = articles.id AND user_id = ? AND role = ?)', user.id, 'AUTHOR')
      end
    end
  end

readable_articles = policy_scope(Article, policy_scope_class: ArticlePolicy::ReadScope)
deletable_articles = policy_scope(Article, policy_scope_class: ArticlePolicy::DeleteScope)

...although there seems to be some mismatch between the README which includes the example:

@publications = policy_scope(publication_class, policy_scope_class: PublicationPolicy::Scope)

...and the actual source which says:

 def policy_scope(user, scope)

(

pundit/lib/pundit.rb

Line 84 in 5b0d709

def policy_scope(user, scope)

)

...so maybe this doesn't exist anymore 😞

[immerrr]

It's not stupid at all, it could work, although there's an immediate code duplication optimization, the

   attr_reader :user, :articles

    def initialize(user, articles)
      @user  = user
      @articles = articles
    end

part is asking to be shared between the policy classes. Or if it is just one class, to be shared between methods with no extra actions :)

[dgmstuart]

@immerrr is there more duplication between these two classes than between two Scope classes relating to different resources? I'm not sure there is, but I'm ready to be convinced.

[immerrr]

I guess it depends on the policy itself, I mean the policy in question obviously shares this snippet between two resolve methods:

      if user.admin?
        articles.all
      else

But none of this is a big deal really.

The way you emphasize "different" resources could be the reason why we went for different approaches intuitively. I think it's because I don't see them as different resources, the resource for me here is the same, the list of objects passed in as the input scope. Then the policies look like filters rather than new subresources to me. And the policy scopes are a natural optimization of what otherwise would look something like

policy = Policy.new(...)
scope.select { |obj| policy.show?(obj) }
# or 
scope.select { |obj| policy.update?(obj) }

But I don't insist. For me personally, it would be great to see this feature land, the way it is implemented doesn't matter that much.

[dgmstuart]

@immerrr I think I've caused some confusion: when I say different resources I do literally mean different - eg. UserPolicy::Scope vs ArticlePolicy::Scope.

Wouldn't this bit be duplicated regardless of whether this was implemented in the same class or two different classes?

if user.admin?
  articles.all
 else

According to the documentation, the solution I outlined (which is actually the same as this, described in the original issue: #368 (comment)), should already be possible in pundit (but whether the documentation is correct is another matter).

[immerrr]

Ah, ok, sorry.

Wouldn't this bit be duplicated regardless of whether this was implemented in the same class or two different classes?

Originally I had it in my head as something like

def resolve_for_action(action: :show?)
  if user.admin?
    articles.all
  else
    ...
  end
end 

And in that case there would probably less duplication, but like I said, I'm fine with separate methods or separate classes too, you would split the single method if/when it would grow anyway.

Re: the solution you outlined: I get how from the maintainer's perspective sometimes less features is better than more features, both in terms of effort and interface complexity. As the example from #368 shows, your approach can be achieved without any pundit interference at all:

  @publications = policy_scope(publication_class, policy_scope_class: PublicationPolicy::Scope)
  # with
  @publications = PublicationPolicy::Scope.new(current_user, publication_class).resolve

That approach does not require any extra pundit code, and oddly enough it ends up being a few characters shorter.

AFAICS this PR is more about a way to bring feature parity between authorize and policy_scope in terms of the interface, to get them closer to something like:

authorize post, :update?
# vs
policy_scope Post, :update?

The way it is implemented IMHO doesn't really matter as long as the implementation details, like policy class names, don't leak.

[Linuus]

readable_articles = policy_scope(Article, policy_scope_class: ArticlePolicy::ReadScope)
deletable_articles = policy_scope(Article, policy_scope_class: ArticlePolicy::DeleteScope)

...although there seems to be some mismatch between the README which includes the example:

@publications = policy_scope(publication_class, policy_scope_class: PublicationPolicy::Scope)

...and the actual source which says:

 def policy_scope(user, scope)

(

pundit/lib/pundit.rb

Line 84 in 5b0d709

def policy_scope(user, scope)

)

...so maybe this doesn't exist anymore 😞

There is no mismatch. That's not the correct method you are looking at. Here is the source:

https://github.com/varvet/pundit/blob/master/lib/pundit.rb#L250-L253

[dgmstuart]

Ah I see - there's a class method and an instance method with the same name.

[Linuus]

Ah I see - there's a class method and an instance method with the same name.

Yes. So you can do Pundit.policy_scope(....) in other places if you want. See #537 for more info why overriding the policy class is not available on the class method.

[dgmstuart]

OK - I think I'm getting there with understanding this - thanks everyone for your patience. Here's my understanding of the issue:

1. In principle a Pundit Policy can have different rules for each of the 7 RESTful controller actions [i], as well as arbitrary other methods

2. By default these policy rules get applied in the corresponding controller action.

3. For some of the RESTful controller actions [ii], we may want to define which list of existing records that action can act on for a given user. This is what Scope is intending to do.

4. In addition: if we have our own non-RESTful policy rules we may also want to specify the list of records for which those rules are true

5. In general there is a relationship between a policy rule and the corresponding scope:

   • the policy rule asks a true/false question of a single user+record combination
   • the scope should return the list of things for which that same question is true

6. In many cases, these lists will be identical for all policy rules [iii]. This is why the current approach (which supports a single scope) is sufficient for many use cases.

7. Unlike policy rules, scopes will not generally be applied in the corresponding controller action [iv]

Conclusion 1: Different scopes corresponding to different policy rules is a well-formed concept. Let's refer to these as 'scope rules'

Conclusion 2: Scope rules should only relate to specific policy rules (otherwise there's no reason for them to live within Pundit rather than in the model)

Conclusion 3: The behaviour of policy_scope isn't really the same shape as authorize, for two reasons:

• authorize post, :create? is meaningful, but policy_scope Post, :create? is not a well-formed concept ("the list of Posts which can be created")
• authorize post has default behaviour based on the context: it looks up the corresponding policy rule for the current controller action, which defaults to false. With policy_scope Post we can't in general infer anything from the context, so the default behaviour is something we have to define.

I wonder if it would be misleading to aim to give these such similar interfaces since they don't actually behave the same way? (cc @immerrr)

Conclusion 4: Since a specific scope rule and policy rule are actually two representations of the same rule, ideally we would infer the scope rule from the policy rule rather than defining it explicitly, but I don't see how that's possible since pundit rules are arbitrary ruby and we can't assume much about what's backing the policy scope either.

Given some data we could perhaps check that the policy and scope rules match up, but I'm not sure if that's a useful/practical thing to do:

post = policy_scope(Posts, :updateable).first
authorize(post, :update?) # should always be true

---

• [i] ...though in practice it's hard to imagine a case for new/create and edit/update having different rules
• [ii] If we assume that the RESTful actions correspond to records backed by a database then this is probably index, show, edit, update and destroy since they're actions which relate to existing records. However it's hard to imagine that the scope for edit would be different to update.
• [iii] Example: Blog Posts.
  • everyone can see published things
  • users can see, edit, and delete all posts which they have authored
  • admins can see and do everything that any user can do.
• [iv] Example: We might have a screen showing a list of editable records (index) in which we use policy_scope Record, :editable. Each record in the list has a button which toggles some value on that record (update), but there is no dedicated page for modifying individual records (no edit action)

[immerrr]

Great summary, thanks @dgmstuart ! Sorry for the delay.

authorize post, :create? is meaningful, but policy_scope Post, :create? is not a well-formed concept ("the list of Posts which can be created")

Yeah, the "new"/"create" pair (C of CRUD) is a strange thing to do for a Scope that's supposed to operate on persisted objects, but the rest should fit quite well.

authorize post has default behaviour based on the context: it looks up the corresponding policy rule for the current controller action, which defaults to false. With policy_scope Post we can't in general infer anything from the context, so the default behaviour is something we have to define.

In that sense authorize and policy_scope methods are not exactly equivalent and might not need the same API. It would be nice to make the two APIs close at least in the part where the operation is specified explicitly.

Since a specific scope rule and policy rule are actually two representations of the same rule, ideally we would infer the scope rule from the policy rule rather than defining it explicitly, but I don't see how that's possible since pundit rules are arbitrary ruby and we can't assume much about what's backing the policy scope either

Perhaps it could it be done the other way around? E.g. auto-generating a policy rule from a scope rule by doing something like Scope.resolve(Post.where(id: post.id)).exists? at a cost of an extra query? Or maybe there's a library to provide query methods over a collection of pre-fetched model instances so that Scope.resolve([post]).exists? would work...

Happy Holidays!

[dgmstuart]

@Eric-Guo

Yes, agree, the result of authorization is do filtering in tables,

No, that's not what I'm saying.

"filtering" is an ambiguous word. This is what I mean when I'm using that word:
There are two different concepts here:

1. Authorization: Listing all the records that a user is allowed to interact with in any context
2. Filtering: Listing a subset of the records that a user is allowed to interact with, in a specific context

Pundit is designed to help with 1:

Often, you will want to have some kind of view listing records which a particular user has access to.

Pundit should probably not get involved in 2:
Instead you can use the policy scope (Pundit) in combination with regular scopes (Rails) on your models.

To put it another way, the intention is that the definition of resolve should list exactly the set of records for which show? is true. That doesn't seem to be the case in your code.

[Eric-Guo]

Thank your kindly reply and patient to explain your design decision.

I'm afraid I still hope Pundit can help rails developer resolve case 2. because in case 2, in a specific context, the control variable is probably only model class and user, if I can not put such logic in policy class. I only can allow User::ALL_EXCEPT_OTHER_COMPANY_DETAILS access all record in every usage and do a rails regular scopes in every usage place at same time, which more repeat and no a prevent mistakes design.

So this PR, IMHO merged to the master will be helpful. 😄

[dmitry]

Why not to add filtering to the pundit as it would be helpful to filter-out unauthorized attributes posted via HTTP using strong_parameters?

[dgmstuart]

@Eric-Guo

because in case 2, in a specific context, the control variable is probably only model class and user

I'm not sure I understand what you mean:

Let's say we have records A, B, C and D, and we have a user who is authorised in general to see A, B and C.

Let's say we have a page on which we only want the user to be able to see A and B. They're authorised to see C, but we don't want to include it on this page for some other reason.

How can that reason be only based on the user? Don't we need to use some information about the records in order to decide which ones to show?

[Eric-Guo]

How can that reason be only based on the user? Don't we need to use some information about the records in order to decide which ones to show?

We always control access based on user and his access tables. we do need some information about the user, like his title, job_level, company or departments, but it can be calculated on the User#calculate_operation_access_code

I need change the example a little bit: we have records A, B, C and D in the one table, and we have a user who is authorised in general to see A, B only and it used in many place

Now we having a little special page, he should be able to see A,B and C. (notice he still can not access D.)

Then can not using the general resolve in pundit because C is already filled out in general, and if we not using policy_scope in special page, it works, but have to put the user control logic in controller and if another special page coming, it need to repeat in that controller again. (maybe using rails scope can help minimal the logic, but still need to repeat using that scope again), let it even make it more worse, if user having different access level, then such logic will become very long.

That's the one reason I want to merge this PR, another reason is some what optioned, pundit as authorization gem should help developer put all their authorization logic in the policy class, although it will probably will change the meaning of pundit slogan (Minimal authorization through OO design and pure Ruby classes)

[dgmstuart]

@Eric-Guo If they're authorized to see record C in any context, then by definition they're authorized to see it.

If you want to hide that record in some contexts then that's not authorization and it's not something Pundit should be concerned with.

Your concept of "authorized in general" doesn't make sense to me - sorry.

[dgmstuart]

Thanks for your input everyone, but I'm closing this PR.

I'll very happily re-open if someone is able to present (or construct) a simple example of a plausible use case that I can understand.

[dmitry]

@dgmstuart please look to the activepolicy scoping examples: https://actionpolicy.evilmartians.io/#/scoping

[dgmstuart]

@dmitry thanks, I've read them. Currently the reason for closing is that I haven't seen a use case that I can understand, for which this feature seems like a good solution. I don't see one in those docs either: it's all pretty abstract no?

All I'm asking for is a simple plausible example where two different scopes required in the same policy.

All I can say for certain about the actionpolicy feature is that it seems pretty complex, and as Linus said above, against the design approach of this gem. Bear in mind that asking for new features is asking for me to be part of maintaining those features for the foreseeable future.

[dmitry]

@dgmstuart few examples:

• You may filter out parameters using scopes for the strong_parameters, and not just filtering (based on the user role) records using AR (or any other ORM).
• Sometimes there are different contexts to fetch the data (with filter based on the user role), for example you may have two types of documents in the user model (ID and business license). Both documents have one class (Document), but a bit different context. Means you have to differentiate between them.

[dgmstuart]

@dmitry I'm sorry I'm afraid I don't understand the first point. Could you write a simple code example?

In the second case, it sounds like ID and business license are treated differently for the purposes of authorization, in which case shouldn't they have different policies? Or if I've misunderstood, please write a simple code example: it's much easier to understand by looking at code.

[dmitry]

First case.

Strong parameters scope usage could look like:

def create
  attributes = policy_scope(params, :params, class: PostPolicy)
  @post = Post.create(attributes)
end

Definition:

 class ParamsScope
    def initialize(user, params)
      @user  = user
      @params  = params
    end

    # could be reused as nested class
    def resolve
      params.permit(permitted)
    end

    protected

    def permitted
      if user.admin?
        [:user_id, :title, :text]
      else
        [:text]
      end
    end
  end

---

Second case

I agree you can create shared class (DocumentPolicy) which can be reused as nested class for IDPolicy and BusinessLicensePolicy classes. One of the real examples, is when you are logged in as a super user and you want to control over subuser's documents. In that case you need to have special context scope, as you are not accessing directly from the user's account to the it's documents, but from the super account.

[dgmstuart]

@dmitry
First case:
That's not a scope though: a scope returns a list of records.
If you want to be able to support arbitrary functions like this which return different results based on the user role, that's a whole other discussion.

[dgmstuart]

@dmitry Second case sounds completely implementable in Pundit currently. I'm showing an example with different policies, but I don't think you actually need these: it seems much simpler to just pass the relevant scope into the policy_scope method.

class Admin::DocumentsController < ApplicationController
  def index
    @documents = policy_scope(Document)
  end
end

class DocumentsController < ApplicationController
  def index
    @documents = policy_scope(Document)
  end
end

class IDCardsController < ApplicationController
  def index
    @documents = policy_scope(Document, policy_scope_class: IDCardPolicy::Scope)
    # but actually this isn't even necessary and you can just do this for exactly the same result:
    @documents = policy_scope(Document.id_cards)
  end
end

class DocumentPolicy < ApplicationPolicy
  class Scope < Scope
    def resolve
      if user.admin?
        scope.all
      else
        scope.where(owner_id: user.id)
      end
    end
  end
  
  def show?
    record.type = "licence" || user.admin? || record.owner_id = user.id
  end
end

class LicencePolicy < ApplicationPolicy
  class Scope < Scope
    def resolve
      scope.licences # assuming that this is a scope on Documents that returns all the licences.
    end
  end
  
  def show?
    true
  end
end

class IDCardPolicy < ApplicationPolicy
  class Scope < Scope
    def resolve
      if user.admin?
        scope.id_cards # assuming that the Document model has a scope which returns all the ID cards
      else
        scope.id_cards.where(owner_id: user.id)
      end
    end
  end
  
  def show?
    user.admin? || record.owner_id = user.id
  end
end