From 2c9ed47509a2aec7b3e4caf7fb9dcedbc8f3b03a Mon Sep 17 00:00:00 2001 From: fusagiko/takayamaki Date: Wed, 13 Mar 2024 02:06:58 +0000 Subject: [PATCH 1/5] add missing test case about accesible_by when STI is in use --- .../active_record_adapter_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/spec/cancan/model_adapters/active_record_adapter_spec.rb b/spec/cancan/model_adapters/active_record_adapter_spec.rb index 810af592..82cd0f62 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/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,20 @@ 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 end end From 970c7f333a554e340f0256d45e58b46b247c5d84 Mon Sep 17 00:00:00 2001 From: fusagiko/takayamaki Date: Wed, 13 Mar 2024 03:15:34 +0000 Subject: [PATCH 2/5] add test cases about CanCan::Rule#catch_all? --- spec/cancan/rule_spec.rb | 91 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/spec/cancan/rule_spec.rb b/spec/cancan/rule_spec.rb index 3f789fd1..59d549d0 100644 --- a/spec/cancan/rule_spec.rb +++ b/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 From 4c3a184ee65e943450c5ad85e4dcbbb66855ef21 Mon Sep 17 00:00:00 2001 From: fusagiko/takayamaki Date: Wed, 13 Mar 2024 03:44:12 +0000 Subject: [PATCH 3/5] fix CanCan::Rule#catch_all? to care STI subclasses --- lib/cancan/rule.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/cancan/rule.rb b/lib/cancan/rule.rb index cdab1643..3a74cc53 100644 --- a/lib/cancan/rule.rb +++ b/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? From 2d9080850d0cb9decfa6d3dbf6b872281e3500a1 Mon Sep 17 00:00:00 2001 From: takayamaki / fusagiko <24884114+takayamaki@users.noreply.github.com> Date: Thu, 24 Apr 2025 20:19:04 +0900 Subject: [PATCH 4/5] fix CanCan::RulesCompressor#simplify to care STI subclasses --- lib/cancan/rules_compressor.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/cancan/rules_compressor.rb b/lib/cancan/rules_compressor.rb index 46f5a4cb..77ccb599 100644 --- a/lib/cancan/rules_compressor.rb +++ b/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 From 53e575e45fb08da3acb421eb655604ee9937e9ee Mon Sep 17 00:00:00 2001 From: takayamaki / fusagiko <24884114+takayamaki@users.noreply.github.com> Date: Thu, 24 Apr 2025 20:47:40 +0900 Subject: [PATCH 5/5] add test case between base class and sub class --- .../model_adapters/active_record_adapter_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/cancan/model_adapters/active_record_adapter_spec.rb b/spec/cancan/model_adapters/active_record_adapter_spec.rb index 82cd0f62..8bafe5fa 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_adapter_spec.rb @@ -1514,5 +1514,17 @@ class Suzuki < Motorbike 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