accessible_by may crash when a default_scope is used

CHANGELOG.md

@@ -1,7 +1,9 @@
 ## 3.0.2
 
 * [#590](https://github.com/CanCanCommunity/cancancan/pull/590): Fix Rule#inspect when rule is created through a SQL array. ([@frostblooded][])
-* [#592](https://github.com/CanCanCommunity/cancancan/pull/592): Prevent normalization of through polymorphic associations.([@eloyesp][])
+* [#592](https://github.com/CanCanCommunity/cancancan/pull/592): Prevent normalization of through polymorphic associations. ([@eloyesp][])
+* [#600](https://github.com/CanCanCommunity/cancancan/pull/600): Switch back to eagerly loading associations for ActiveRecord 5.0.x and 5.1.x. 5.2+ will still not automatically eager load. ([@ghiculescu][])
+* [#600](https://github.com/CanCanCommunity/cancancan/pull/600): Fix regression when using accessibl_by with default_scopes that set an order. ([@ghiculescu][])
 
 ## 3.0.1
 
@@ -650,3 +652,4 @@ Please read the [guide on migrating from CanCanCan 2.x to 3.0](https://github.co
 [@kaspernj]: https://github.com/kaspernj
 [@frostblooded]: https://github.com/frostblooded
 [@eloyesp]: https://github.com/eloyesp
+[@ghiculescu]: https://github.com/ghiculescu

lib/cancan/model_adapters/active_record_5_adapter.rb

@@ -22,9 +22,46 @@ def self.matches_condition?(subject, name, value)
       private
 
       def build_relation(*where_conditions)
-        relation = @model_class.where(*where_conditions)
-        relation = relation.left_joins(joins).distinct if joins.present?
-        relation
+        relation = @model_class.all
+        return relation unless where_conditions.any?(&:present?) || joins.present?
+
+        relation = relation.where(*where_conditions)
+        relation = add_joins_to_relation(relation)
+        extract_ids_and_requery(relation)
+      end
+
+      def extract_ids_and_requery(relation)
+        need_to_extract_ids = relation.values[:distinct].present? || relation.values[:order].present?
+
+        # we need to extract IDs if the query has a builtin `distinct` or `order`. an example of when
+        # would be a `default_scope` that adds either. this is because otherwise you'll call
+        # SELECT DISTINCT @model_class.* ORDER BY ___, but there's no guarantee that the columns you
+        # order by will be included in the SELECT. a previously attempted solution was to add
+        # SELECTs for all the ORDER depends on (see https://github.com/CanCanCommunity/cancancan/pull/600)
+        # but this breaks COUNT queries. the simplest general purpose solution is to get IDs for all
+        # records that match the cancan criteria, then do *another* query that re-adds the default scope.
+        # this is what we do here.
+        # the main downside is some queries cancan generates will now look a bit uglier.
+        if need_to_extract_ids
+          @model_class.where(id: relation.reorder(nil).pluck(:id).uniq)
+        else
+          # if we don't already have a `distinct` (relation.values[:distinct].blank?)
+          # but we have added a join, we need to add our own `distinct`.
+          relation = relation.distinct if joins.present?
+          relation
+        end
+      end
+
+      def add_joins_to_relation(relation)
+        return relation unless joins.present?
+
+        # AR#left_joins doesn't play nicely in AR 5.0 and 5.1
+        # see https://github.com/CanCanCommunity/cancancan/pull/600#issuecomment-524672268
+        if self.class.version_greater_or_equal?('5.2')
+          relation.left_joins(joins)
+        else
+          relation.includes(joins).references(joins)
+        end
       end
 
       # Rails 4.2 deprecates `sanitize_sql_hash_for_conditions`

spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb

@@ -87,7 +87,7 @@ class Editor < ActiveRecord::Base
     end
   end
 
-  if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0')
+  if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.2.0')
     describe 'selecting custom columns' do
       it 'extracts custom columns correctly' do
         posts = Post.accessible_by(ability).select('title as mytitle')

spec/cancan/model_adapters/accessible_by_integration_spec.rb

@@ -89,7 +89,7 @@ class Editor < ActiveRecord::Base
     end
   end
 
-  if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0')
+  if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.2.0')
     describe 'selecting custom columns' do
       it 'extracts custom columns correctly' do
         posts = Post.accessible_by(ability).select('title as mytitle')

spec/cancan/model_adapters/active_record_5_adapter_with_default_scopes_spec.rb

@@ -0,0 +1,156 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0')
+  describe CanCan::ModelAdapters::ActiveRecord5Adapter do
+    context 'with postgresql' do
+      before :each do
+        connect_db
+        ActiveRecord::Migration.verbose = false
+
+        ActiveRecord::Schema.define do
+          create_table(:blog_authors) do |t|
+            t.string :name, null: false
+            t.timestamps null: false
+          end
+
+          create_table(:blog_posts) do |t|
+            t.references :blog_author
+            t.string :title, null: false
+            t.timestamps null: false
+          end
+
+          create_table(:blog_post_comments) do |t|
+            t.references :blog_post
+            t.string :body, null: false
+            t.timestamps null: false
+          end
+        end
+
+        unless defined?(BlogAuthor)
+          class BlogAuthor < ActiveRecord::Base
+            has_many :blog_posts
+            has_many :blog_post_comments, through: :blog_posts
+          end
+        end
+
+        unless defined?(BlogPost)
+          class BlogPost < ActiveRecord::Base
+            has_many :blog_post_comments
+            belongs_to :blog_author
+
+            default_scope -> { order(:title) }
+          end
+        end
+
+        unless defined?(BlogPostComment)
+          class BlogPostComment < ActiveRecord::Base
+            belongs_to :blog_post
+
+            default_scope -> { order(created_at: :desc) }
+          end
+        end
+      end
+
+      subject(:ability) { Ability.new(nil) }
+
+      let(:alex) { BlogAuthor.create!(name: 'Alex') }
+      let(:josh) { BlogAuthor.create!(name: 'Josh') }
+
+      let(:p1) { josh.blog_posts.create!(title: 'p1') }
+      let(:p2) { alex.blog_posts.create!(title: 'p2') }
+
+      let(:p1c1) { p1.blog_post_comments.create!(body: 'p1c1', created_at: Time.new(2019, 8, 25, 1)) }
+      let(:p1c2) { p1.blog_post_comments.create!(body: 'p1c2', created_at: Time.new(2019, 8, 25, 2)) }
+
+      let(:p2c1) { p2.blog_post_comments.create!(body: 'p2c1', created_at: Time.new(2019, 8, 25, 3)) }
+      let(:p2c2) { p2.blog_post_comments.create!(body: 'p2c2', created_at: Time.new(2019, 8, 25, 4)) }
+
+      context 'when default scope sets an order, and abilities dont have extra checks' do
+        before do
+          ability.can :read, BlogPost
+          ability.can :read, BlogPostComment
+        end
+
+        it 'can get accessible records' do
+          expected_bodies_in_order = [p2c2, p2c1, p1c2, p1c1].map(&:body)
+          accessible = BlogPostComment.accessible_by(ability)
+          expect(accessible.map(&:body)).to eq(expected_bodies_in_order)
+          expect(accessible.count).to eq(4)
+        end
+
+        it 'can get accessible records from a has_many' do
+          expected_bodies_in_order = [p2c2, p2c1].map(&:body)
+          accessible = p2.blog_post_comments.accessible_by(ability)
+          expect(accessible.map(&:body)).to eq(expected_bodies_in_order)
+          expect(accessible.count).to eq(2)
+        end
+
+        it 'can get accessible records from a has_many - other post' do
+          expected_bodies_in_order = [p1c2, p1c1].map(&:body)
+          accessible = p1.blog_post_comments.accessible_by(ability)
+          expect(accessible.map(&:body)).to eq(expected_bodies_in_order)
+          expect(accessible.count).to eq(2)
+        end
+
+        it 'can get accessible records from a has_many :through' do
+          expected_bodies_in_order = [p2c2, p2c1].map(&:body)
+          accessible = alex.blog_post_comments.accessible_by(ability)
+          expect(accessible.map(&:body)).to eq(expected_bodies_in_order)
+          expect(accessible.count).to eq(2)
+        end
+
+        it 'can get accessible records from a has_many :through - other user' do
+          expected_bodies_in_order = [p1c2, p1c1].map(&:body)
+          accessible = josh.blog_post_comments.accessible_by(ability)
+          expect(accessible.map(&:body)).to eq(expected_bodies_in_order)
+          expect(accessible.count).to eq(2)
+        end
+      end
+
+      context 'when default scope sets an order, and abilities have extra checks' do
+        before do
+          ability.can :read, BlogPost
+          # this is the only change vs. the above context -- in this context we can *only* see alex's posts
+          ability.can :read, BlogPostComment, blog_post: { blog_author_id: alex.id }
+        end
+
+        it 'can get accessible records' do
+          expected_bodies_in_order = [p2c2, p2c1].map(&:body)
+          accessible = BlogPostComment.accessible_by(ability)
+          expect(accessible.map(&:body)).to eq(expected_bodies_in_order)
+          expect(accessible.count).to eq(2)
+        end
+
+        it 'can get accessible records from a has_many' do
+          expected_bodies_in_order = [p2c2, p2c1].map(&:body)
+          accessible = p2.blog_post_comments.accessible_by(ability)
+          expect(accessible.map(&:body)).to eq(expected_bodies_in_order)
+          expect(accessible.count).to eq(2)
+        end
+
+        it 'can get accessible records from a has_many - none returned' do
+          expected_bodies_in_order = []
+          accessible = p1.blog_post_comments.accessible_by(ability)
+          expect(accessible.map(&:body)).to eq(expected_bodies_in_order)
+          expect(accessible.count).to eq(0)
+        end
+
+        it 'can get accessible records from a has_many :through' do
+          expected_bodies_in_order = [p2c2, p2c1].map(&:body)
+          accessible = alex.blog_post_comments.accessible_by(ability)
+          expect(accessible.map(&:body)).to eq(expected_bodies_in_order)
+          expect(accessible.count).to eq(2)
+        end
+
+        it 'can get accessible records from a has_many :through - none returned' do
+          expected_bodies_in_order = []
+          accessible = josh.blog_post_comments.accessible_by(ability)
+          expect(accessible.map(&:body)).to eq(expected_bodies_in_order)
+          expect(accessible.count).to eq(0)
+        end
+      end
+    end
+  end
+end

spec/cancan/model_adapters/active_record_adapter_spec.rb

@@ -194,6 +194,7 @@ class User < ActiveRecord::Base
     Article.create!(published: true, category: category)
     Article.create!(published: true, category: category)
     expect(Category.accessible_by(@ability)).to match_array([category])
+    expect(Category.accessible_by(@ability).count).to eq(1)
   end
 
   it 'only reads comments for visible categories through articles' do
@@ -529,7 +530,8 @@ class Transaction < ActiveRecord::Base
       ability.can :read, Article, mentioned_users: { name: u1.name }
       ability.can :read, Article, mentions: { user: { name: u2.name } }
       expect(Article.accessible_by(ability)).to match_array([a1, a2])
-      if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0')
+
+      if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.2.0')
         expect(ability.model_adapter(Article, :read)).to generate_sql(%(
   SELECT DISTINCT "articles".*
   FROM "articles"

spec/cancan/model_adapters/has_and_belongs_to_many_spec.rb

@@ -51,7 +51,7 @@ class House < ActiveRecord::Base
       expect(houses).to match_array [@house2, @house1]
     end
 
-    if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0')
+    if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.2.0')
       it 'generates the correct query' do
         expect(ability.model_adapter(House, :read))
           .to generate_sql("SELECT DISTINCT \"houses\".*

spec/changelog_spec.rb

@@ -11,6 +11,8 @@
     expect(changelog.end_with?("\n")).to be true
   end
 
+  # if this test fails it means you probably haven't added a link
+  # for your GH username at the bottom of the CHANGELOG
   it 'has link definitions for all implicit links' do
     implicit_link_names = changelog.scan(/\[([^\]]+)\]\[\]/).flatten.uniq
     implicit_link_names.each do |name|