PolicyFinder Loads Association Data In Rails 3.2.x

[matt-glover]

In Rails 3.2.x some associations are wrapped with a CollectionProxy object. Regrettably Rails 3.2.x redefines respond_to? on this object such that it will query/load the data behind the association in some cases.

This behavior was removed in Rails 4: https://github.com/rails/rails/commit/ecef5a9f224ad3ac74c0466b33cef0226ec0787c

If you try to pass associations directly into Pundit for scoping in Rails 3.2 it will trigger a full data load.

class Blog < ActiveRecord::Base
  has_many :posts
end

### Inside a controller somewhere
blog = Blog.find(1)

# This line will load every post for that blog from the db into memory
only_authorized_data = policy_scope(blog.posts)

@something_to_render = only_authorized_data.limit(5)

In the example above the policy_scope line loads all of the posts for the blog. Most people probably expect it to simply pass the association through to filter further in their policy file. The data load is triggered by the PolicyFinder find method using respond_to?.

As noted above this boils down to a problem with Rails 3.2 behavior and goes away in Rails 4.x. I wanted to call it out here in case others run into this issue on projects that are still running Rails 3.x. I am also happy to issue a pull with a patch if the maintainers are interested in it.

It boils down to something like this (probably with version guards since Rails 4 does not seem to experience this issue):

# Certain respond_to? calls will not load the data behind the association
if object.respond_to?(:proxy_association)
  # Perform most of the rest of the pundit finder checks normally against the relation class
  relation_class = object.proxy_association.klass

[thomasklemm]

@matt-glover Please post your PostPolicy::Scope.

Incorrect guess: You might be using scope.all in there, which loads the records in Rails 3.2. In Rails 4. .all will not load the records, that's why you might experience the difference. If that's the case, use scope.scoped for Rails 3.2 and update to scope.all when upgrading to Rails 4.

[matt-glover]

It does not matter what is inside the policy/scope class. It's the policy finder resolution process that triggers the data load by triggering what I would argue is bad behavior in Rails 3.2.

For example this will still trigger the data load:

class PostPolicy < ApplicationPolicy
  class Scope
    attr_reader :user, :scope

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

    def resolve
      scope.scoped
    end
  end
end

Note the extraneous first query that occurs in this case:

1.9.1 :022 > result = Pundit.policy_scope(user, blog.posts);
1.9.1 :023 > result.limit(5).to_a;
  Posts Load (59.2ms)  SELECT "posts".* FROM "posts" WHERE "posts"."blog_id" = 1
  Posts Load (1.3ms)  SELECT "posts".* FROM "posts" WHERE "posts"."blog_id" = 1 LIMIT 5

This assumes the blog class definition from the body of this issue and includes a semi-colon on the first line to avoid irb printing out the return value and resolving the query then.

If you look in a debugger this query: Posts Load (59.2ms) SELECT "posts".* FROM "posts" WHERE "posts"."blog_id" = 1 is triggered when this line is executed.

[jnicklas]

This is nasty. I guess we could check methods to see if it includes policy_class but that seems like such an ugly workaround for an upstream issue. Pundit is doing the right thing, Rails is clearly wrong in this one. It's especially annoying since it's fixed in Rails 4. @matt-glover, do you have any suggestions on how we could fix this?

[matt-glover]

Because Pundit is doing the right thing, Rails 3.2 is not, and 4.x will do the right thing I am inclined to leave Pundit alone. I logged this primarily so others who hit the issue could see it and find a workaround.

In my case I re-opened the PolicyFinder class in a Rails initializer to work around it until the project running Rails 3.2 can be upgraded.

require 'pundit'

module Pundit
  class PolicyFinder
    def find_with_activerecord_workaround
      if object.respond_to?(:proxy_association)
        relation_class = object.proxy_association.klass
        if relation_class.respond_to?(:policy_class)
          relation_class.policy_class
        else
          klass = if relation_class.respond_to?(:model_name)
                    relation_class.model_name
                  else
                    relation_class
                  end
          "#{klass}Policy"
        end
      else
        find_without_activerecord_workaround
      end
    end

    alias_method_chain :find, :activerecord_workaround
  end
end

If you want to patch Pundit you could do something similar to that check. You can eliminate some duplicated code from the patch above by conditionally extracting the relation class and passing that through the existing find definition.

Something like this might work:

def find
  # Replaces all `object` references with `policy_object`
  # A clearer variable name than `policy_object` is probably warranted...
  policy_object = if object.respond_to?(:proxy_association)
    object.proxy_association.klass
  else
    object
  end

  if policy_object.respond_to?(:policy_class)
    policy_object.policy_class
  elsif policy_object.class.respond_to?(:policy_class)
    policy_object.class.policy_class
  else
    klass = if policy_object.respond_to?(:model_name)
      policy_object.model_name
    elsif policy_object.class.respond_to?(:model_name) 
      policy_object.class.model_name
    elsif policy_object.is_a?(Class)
      policy_object
    elsif policy_object.is_a?(Symbol)
      policy_object.to_s.classify
    else
      policy_object.class
    end
    "#{klass}Policy"
  end
end

If you go down this road:

1. The check for the proxy_association must occur prior to any other respond_to? calls or else you risk resolving the query
2. You probably need Rails version guards around the new behavior because it may be wrong (and probably is wrong) in Rails 4
3. I made no effort to test out the second code example so YMMV

[jnicklas]

Alright. Seems like there's nothing we can do about this. The workaround isn't really acceptable IMO, so let's just leave it as is.