Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rules for STI subclasses apply to parent classes in accessible_by #771

Open
sam-carlberg opened this issue Mar 9, 2022 · 4 comments
Open

Comments

@sam-carlberg
Copy link

Probably related to #768. Tested with the fix in #689 but behavior did not change

Steps to reproduce

Create an STI parent class and a subclass, and add ability.can :read to both classes. Calling ParentClass.accessible_by(ability) returns only instances of the subclass

Add this spec to spec/cancan/model_adapters/active_record_adapter_spec.rb, under the 'when STI is in use' context:

it 'does not apply rules for subclasses when accessing a superclass' do
  u1 = User.create!(name: 'pippo')
  ability = Ability.new(u1)
  ability.can :read, Vehicle
  ability.can :read, Motorbike
  ability.can :read, Suzuki

  v = Vehicle.create!
  m = Motorbike.create!
  s = Suzuki.create!

  aggregate_failures do
    expect(Vehicle.accessible_by(ability)).to eq [v, m, s] # fails: actually get: [m]
    expect(Motorbike.accessible_by(ability)).to eq [m, s]  # fails: actually get: [s]
    expect(Suzuki.accessible_by(ability)).to eq [s]        # passes
  end
end

Expected behavior

Parent STI models should not be affected by the rules (and their conditions) defined on subclasses

Actual behavior

Parent models are affected by conditions coming from rules defined on their subclasses, which exclude the parent class from the set of records returned by accessible_by

Additional info for the sample spec

Vehicle.accessible_by(ability).to_sql returns SELECT "vehicles".* FROM "vehicles" WHERE "vehicles"."type" = 'Motorbike', when I would expect it to return SELECT "vehicles".* FROM "vehicles"

Motorbike.accessible_by(ability).to_sql returns SELECT "vehicles".* FROM "vehicles" WHERE "vehicles"."type" IN ('Motorbike', 'Suzuki') AND "vehicles"."type" = 'Suzuki', when I would expect it to return SELECT "vehicles".* FROM "vehicles" WHERE "vehicles"."type" IN ('Motorbike', 'Suzuki')

Suzuki.accessible_by(ability).to_sql returns SELECT "vehicles".* FROM "vehicles" WHERE "vehicles"."type" IN ('Suzuki') AND "vehicles"."type" = 'Suzuki', which is a valid query to return the appropriate records, but the STI type appears twice, which is redundant

System configuration

Rails version:
Ran the above spec on 5.2.2, but the behavior is also present on 6.0.4.6 in a separate Rails application

Ruby version:
Ran the above spec on 2.6.5, but the behavior is also present on 3.0.3 in a separate Rails application

CanCanCan version
3.3.0

@sam-carlberg
Copy link
Author

Appears to be the same problem that was discussed in #663; at least, the root cause is that rules for subclasses are incorrectly pulled in as relevant rules for their parent classes.

@JohnSmall
Copy link

I'll vote for this. I've a problem with rules on sub-classes propagating up to affect the parent class.

I'm upgrading an old 5.0.2 app to 6.latest, and I've got as far as 5.1.7 but this problem has me stumped. It used to work in lower versions but not now.

I have an extra restriction on a subclass and it also puts the restriction on the parent class!!! Grrr!!. I see that #663 has been outstanding for just over a year now.

@martinstreicher
Copy link

I am seeing the same problem in my own code base.

The gist is:

  • class User < ApplicationRecord
  • class User::Member < User
  • cannot :manage, User
  • can :manage, User::Member
  • can? :manage, User => true

This is incorrect. I debugged this and it seems as though the can :manage, User::Member rule is enumerated as a potential rule match, even though it should not be. It yields true and permission is granted.

@martinstreicher
Copy link

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'
  gem 'rails', '= 6.1.7'
  gem 'cancancan', '= 3.5.0', require: false
  gem 'pry'
  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 :users, force: true do |t|
    t.string :type
  end
end

class User < ActiveRecord::Base; end
class User::Member < User; end

class Ability
  include CanCan::Ability

  def initialize
    cannot :manage, User
    can :manage, User::Member
  end
end

class BugTest < Minitest::Test
  def test_bug
    User.create!
    User::Member.create!

    ability = Ability.new
    assert_equal true, ability.can?(:manage, User::Member)
    assert_equal false, ability.can?(:manage, User) # Fails
  end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants