Fix CanCan::RulesCompressor for STI subclasses

lib/cancan/rule.rb

@@ -58,7 +58,8 @@ def cannot_catch_all?
 
     def catch_all?
       (with_scope? && @conditions.where_values_hash.empty?) ||
-        (!with_scope? && [nil, false, [], {}, '', ' '].include?(@conditions))
+        ((@subjects.all? { |subject| !StiDetector.sti_class?(subject) || subject.base_class? }) &&
+          (!with_scope? && [nil, false, [], {}, '', ' '].include?(@conditions)))
     end
 
     def only_block?

lib/cancan/rules_compressor.rb

@@ -31,9 +31,10 @@ def compress(array)
     def simplify(rules)
       seen = Set.new
       rules.reverse_each.filter_map do |rule|
-        next if seen.include?(rule.conditions)
+        subjects_and_conditions = [rule.subjects, rule.conditions]
+        next if seen.include?(subjects_and_conditions)
 
-        seen.add(rule.conditions)
+        seen.add(subjects_and_conditions)
         rule
       end.reverse
     end

spec/cancan/model_adapters/active_record_adapter_spec.rb

@@ -1416,6 +1416,9 @@ class ApplicationRecord < ActiveRecord::Base
       class Vehicle < ApplicationRecord
       end
 
+      class Airplane < Vehicle
+      end
+
       class Car < Vehicle
       end
 
@@ -1496,5 +1499,32 @@ class Suzuki < Motorbike
       expect(Car.accessible_by(ability)).to eq([car])
       expect(Motorbike.accessible_by(ability)).to eq([])
     end
+
+    it 'allows a selectable scope when permit by two subclasses' do
+      u1 = User.create!(name: 'pippo')
+      Airplane.create!(capacity: 1)
+      car = Car.create!(capacity: 4)
+      mortorbike = Motorbike.create!(capacity: 2)
+
+      ability = Ability.new(u1)
+      ability.can :read, Car
+      ability.can :read, Motorbike
+      expect(Vehicle.accessible_by(ability)).to contain_exactly(car, mortorbike)
+      expect(Airplane.accessible_by(ability)).to be_empty
+      expect(Car.accessible_by(ability)).to contain_exactly(car)
+      expect(Motorbike.accessible_by(ability)).to contain_exactly(mortorbike)
+    end
+
+    it 'allows access to both base class and subclass when permissions are defined for both' do
+      u1 = User.create!(name: 'pippo')
+      vehicle = Vehicle.create!(capacity: 1)
+      car = Car.create!(capacity: 4)
+
+      ability = Ability.new(u1)
+      ability.can :read, Vehicle
+      ability.can :read, Car
+      expect(Vehicle.accessible_by(ability)).to contain_exactly(vehicle, car)
+      expect(Car.accessible_by(ability)).to contain_exactly(car)
+    end
   end
 end

spec/cancan/rule_spec.rb

@@ -86,4 +86,95 @@ class Watermelon < ActiveRecord::Base
       end
     end
   end
+
+  describe '#catch_all?' do
+    it 'is true when no conditions are specified' do
+      rule = CanCan::Rule.new(true, :read, Integer, nil)
+      expect(rule).to be_catch_all
+    end
+
+    it 'is false when conditions are specified' do
+      rule = CanCan::Rule.new(true, :read, Integer, foo: :bar)
+      expect(rule).not_to be_catch_all
+    end
+
+    describe 'when subject is a ActiveRecord class' do
+      around do |example|
+        connect_db
+        ActiveRecord::Migration.verbose = false
+
+        ActiveRecord::Base.transaction do
+          ActiveRecord::Schema.define do
+            create_table(:vehicles) do |t|
+              t.string :name
+            end
+          end
+
+          class Vehicle < ApplicationRecord; end
+
+          example.run
+        end
+      end
+
+      it 'is true when no conditions are specified' do
+        rule = CanCan::Rule.new(true, :read, Vehicle)
+        expect(rule).to be_catch_all
+      end
+
+      it 'is false when conditions are specified' do
+        rule = CanCan::Rule.new(true, :read, Vehicle, name: 'foo')
+        expect(rule).not_to be_catch_all
+      end
+
+      it 'is false when conditions are ActiveRecord Scope' do
+        rule = CanCan::Rule.new(true, :read, Vehicle, Vehicle.where(name: 'foo'))
+        expect(rule).not_to be_catch_all
+      end
+    end
+
+    describe 'when STI is used' do
+      around do |example|
+        connect_db
+        ActiveRecord::Migration.verbose = false
+
+        ActiveRecord::Base.transaction do
+          ActiveRecord::Schema.define do
+            create_table(:vehicles) do |t|
+              t.string :type
+            end
+          end
+
+          class ApplicationRecord < ActiveRecord::Base
+            self.abstract_class = true
+          end
+
+          class Vehicle < ApplicationRecord; end
+          class Airplane < Vehicle; end
+          class Car < Vehicle; end
+          class MotorBike < Vehicle; end
+          example.run
+        end
+      end
+
+      it 'is true when subject is base class and no conditions are specified' do
+        rule = CanCan::Rule.new(true, :read, Vehicle)
+        expect(rule).to be_catch_all
+      end
+
+      it 'is true when subject is base class and conditions are specified' do
+        rule = CanCan::Rule.new(true, :read, Vehicle, foo: :bar)
+        expect(rule).not_to be_catch_all
+      end
+
+      it 'is false when subject is subclass even if no conditions are specified' do
+        rule = CanCan::Rule.new(true, :read, Car)
+        expect(rule).not_to be_catch_all
+      end
+
+      it 'is false when subjects includes subclass even if no conditions are specified' do
+        rule = CanCan::Rule.new(true, :read, [Vehicle, Car])
+        expect(rule).not_to be_catch_all
+      end
+    end
+  end
 end