varvet / pundit

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

Manually specifying policy class via an instance method does not always work #743

Closed ryanvall closed 6 months ago

ryanvall commented 2 years ago

This portion of the documentation states that you can define a custom policy_class via either a class method or an instance method, but the latter doesn't always work. Specifically, it fails when you want to policy scope the entire class, eg:

class Post
  def policy_class
    PostablePolicy
  end
end

class PostablePolicy
  class Scope < Scope
    def resolve
      if user.admin?
        scope.all
      else
        scope.where(published: true)
      end
    end
  end
end

# controller
def index
  @posts = policy_scope(Post)
end

With this setup, @posts ends up evaluating to nil, rather than resolving to an ActiveRecord::Relation. The problem seems to be in the find method of the PolicyFinder:

https://github.com/varvet/pundit/blob/6513071aa790f061dec72e676b99996fa007c9ff/lib/pundit/policy_finder.rb#L73-L87

In the case where you are policy scoping the entire class, the subject is the class. This ends up falling into the else branch and returns PostPolicy instead of PostablePolicy, which in the above example doesn't exist. If the policy can't be found, nil is returned.

Assuming that's all correct, I see two solutions:

  1. Add another elsif branch - if subject is a class, attempt to instantiate an arbitrary instance of it and check if it has policy_class defined
  2. Deprecate/remove the instance-method version entirely

2 is obviously a breaking change for anyone currently using the library, but long-term it seems like a better solution to me - besides backwards compatibility I don't really see a benefit/reason to keep the instance-method version around.

Burgestrand commented 6 months ago

Thank you, this is well written! The provided example is very helpful.

I believe the use-case for policy_scope(Post), i.e. calling it with a class, is specifically when you don't have an instance to call it on, which is mostly only ever in index. For all the other resource actions (new, create, show, edit, update, destroy) you do have an instance, where the instance method would then be used.

The instance method is arguably more useful because you can alter the returned policy based off of record data, which you can't do on the class-method. I'm not saying you should do this, but you could.

class User
  def policy_class
    if admin?
      AdminUserPolicy
    else
      UserPolicy
    end
  end
end

So while your assessment of what the code does is correct, I believe this is a failure of documentation and not implementation. If something needs fixing it's maybe clarifying this difference in the README!