STI ability overwrites previously defined STI rules

[23tux]

Steps to reproduce

The order of can statements affects the SQL that is generated:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  gem "rails", "= 7.1.5.1"
  gem "cancancan", "= 3.6.1", require: false # require false to force rails to be required first
  gem "sqlite3"
end

require "active_record"
require "cancancan"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :vehicles, force: true do |t|
    t.string :type
  end
end

class Vehicle < ActiveRecord::Base; end
class Car < Vehicle; end

class Ability
  include CanCan::Ability

  def initialize
    can :index, Vehicle
    can :index, Car
  end
end

class BugTest < Minitest::Test
  def test_bug
    vehicle = Vehicle.create!
    car = Car.create!

    ability = Ability.new

    # These assertions pass
    assert_equal true, ability.can?(:index, Vehicle)
    assert_equal true, ability.can?(:index, Car)
    assert_equal [vehicle, car], Vehicle.accessible_by(ability, :index).to_a
  end
end

D, [2025-01-14T13:10:25.471243 #1978] DEBUG -- :   Vehicle Load (0.1ms)  SELECT "vehicles".* FROM "vehicles" WHERE "vehicles"."type" = ?  [["type", "Car"]]
F

Failure:
BugTest#test_bug [bug.rb:46]:
--- expected
+++ actual
@@ -1 +1 @@
-[#<Vehicle id: 1, type: nil>, #<Car id: 2, type: "Car">]
+[#<Car id: 2, type: "Car">]

Expected behavior

When using Vehicle.accessible_by(ability, :index) I would expect to have access to all vehicles, the generated SQL produces an OR statement in cancancan 3.5.0 but truncates the statement to only include the last defined can :index, XXXX rule.

Actual behavior

Demonstrated best by looking at the SQL:

class Ability
  include CanCan::Ability

  def initialize
    can :index, Vehicle
    can :index, Car
  end
end

Vehicle.accessible_by(Ability.new, :index).to_sql
=> SELECT "vehicles".* FROM "vehicles" WHERE "vehicles"."type" = 'Car'

Switching the order of the can statements:

class Ability
  include CanCan::Ability

  def initialize
    can :index, Car
    can :index, Vehicle
  end
end

Vehicle.accessible_by(Ability.new, :index).to_sql
=> SELECT "vehicles".* FROM "vehicles"

IMO it should not matter, in which order the can statements are written.

System configuration

Rails version: Tested in 7.1 and 7.2

Ruby version: 3.3.3

CanCanCan version 3.6.1

[kevinluo201]

I looked into this issue today, and it's a big rabbit hole 😅 I think there are 2 places which do not consider STI, and they are important when using accessible_by

1. RulesCompressor https://github.com/CanCanCommunity/cancancan/blob/develop/lib/cancan/model_adapters/active_record_adapter.rb#L22 It doesn't check STI classes and will throw away rules incorrectly, that's why the SQL changed when you switch the orders of 2 can rules
2. build_relations https://github.com/kevinluo201/cancancan/blob/develop/lib/cancan/model_adapters/active_record_adapter.rb#L132 It only builds the where clauses by rules' conditions. This doesn't reflected by your example code. However, if we change can :index, Car to cannot :index, Car, you'll find incorrect behaviour

I think the rules' subjects need to be normalized in some way so the rules on STI classes can be handled better

[kevinluo201]

I read some older issues and I found this is an existing issue: #771, #809, etc.

[23tux]

Cool, thanks for looking into it. I'm not super deep into CanCanCan, but let me know if I can be of any help

[kevinluo201]

Me, neither, lol. I tried to solve the simpler issue first for STI. #877 but seem like it will be a very long waiting time before it gets merge (because they're too busy). After it's merged, I have an idea how to solve this issue 😁