CanCanCommunity / cancancan

The authorization Gem for Ruby on Rails.
MIT License
5.57k stars 638 forks source link

Selective permissions on STI sub-classes not respected by accessible_by #809

Open owst opened 1 year ago

owst commented 1 year ago

Steps to reproduce

Granting read permission on a subset of STI subclass doesn't lead to relevant records being returned by accessible_by. With the following STI hierarchy:

class Vehicle < ActiveRecord::Base
class Car < Vehicle
class Motorbike < Vehicle
class Ferrari < Car
class Suzuki < Motorbike

Given can :read, Ferrari we expect Vehicle.accessible_by(ability, :index) to include any instances of Ferrari (but none of Suzuki), but instead no Vehicle records are returned.

Reproduction:

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'
  gem 'rails', '= 6.1.7'
  gem 'cancancan', '= 3.4.0', 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 Motorbike < Vehicle; end

class Ferrari < Car; end
class Suzuki < Motorbike; end

class Ability
  include CanCan::Ability

  def initialize
    can :read, Ferrari
  end
end

class BugTest < Minitest::Test
  def test_bug
    ferrari = Ferrari.create!
    suzuki = Suzuki.create!

    ability = Ability.new

    # These assertions pass
    assert_equal true, ability.can?(:index, Ferrari)
    assert_equal false, ability.can?(:index, Suzuki)

    # This assertion passes
    assert_equal [ferrari], Ferrari.accessible_by(ability, :index).to_a
    # This assertion also passes
    assert_equal [ferrari], Car.accessible_by(ability, :index).to_a
    # This assertion fails (the actual array is empty)
    assert_equal [ferrari], Vehicle.accessible_by(ability, :index).to_a
  end
end

This feels like an extension of #677

Expected behavior

The allowed subclass instances should be returned by klass.accessible_by for all klasses in the STI hierarchy.

Actual behavior

The allowed subclass instances are not returned by accessible_by when invoked on the base class.

System configuration

Rails version: 6.1.7

Ruby version: 2.7.6

CanCanCan version 3.4.0

owst commented 1 year ago

This seems like intentional behaviour according to this suggested test: https://github.com/CanCanCommunity/cancancan/pull/689#discussion_r638352513 My expectation also seems to be somewhat at odds with the expectation set out in another issue: https://github.com/CanCanCommunity/cancancan/issues/771

My expectation was that since all Ferraris and Suzukis are Vehicles (both intuitively and in the assert_equal true, ferrari.is_a?(Vehicle) sense), asking for the set of accessible Vehicles (assuming that Ferraris but not Suzukis have been marked as accessible) should include only Ferraris.

In my specific example, I would expect:

Vehicle.accessible_by(ability, :index).to_a   # => [ferrari]
Car.accessible_by(ability, :index).to_a       # => [ferrari]
Ferrari.accessible_by(ability, :index).to_a   # => [ferrari]
Motorbike.accessible_by(ability, :index).to_a # => []
Suzuki.accessible_by(ability, :index).to_a    # => []

but what we get is

Vehicle.accessible_by(ability, :index).to_a   # => []
Car.accessible_by(ability, :index).to_a       # => [ferrari]
Ferrari.accessible_by(ability, :index).to_a   # => [ferrari]
Motorbike.accessible_by(ability, :index).to_a # => []
Suzuki.accessible_by(ability, :index).to_a    # => []