conflicting STI rules

[tomasc]

This PR addresses an issue where conditions are not correctly generated for conflicting STI rules.

See (currently failing) test here:

https:/tomasc/cancancan/blob/conflicting-sti-rules/spec/cancan/model_adapters/active_record_adapter_spec.rb#L735-L753

I tried to come up with a way to fix this, but no luck so far. Help would be appreciated!

[coorasse]

Thank you very much! I'd ask @Liberatys for help, since he took care of the STI implementation. Any idea how we could tackle this Nick?

[ghost]

Looking into it ^^

[tomasc]

@coorasse @Liberatys thanks guys!

[ghost]

I had a look during the previous days and was able to narrow the point that causes the issue. Will look into a fix this evening and tomorrow ^^

[tomasc]

@Liberatys that would be fantastic, thanks!

[coorasse]

Thanks for the update @Liberatys!

[tomasc]

Hi @Liberatys and @coorasse , any news on this? Or suggestions how to go about fixing this? Thank you!

[tomasc]

Hi all,

I ended up writing my own ActiveRecord adapter to support STI.

The code is available here: https://gist.github.com/tomasc/52652bd6fcacdf0d03ecc5ad3c6c298d

(I have a decent test suite for it as well.)

The adapter has a bit different (and maybe more naïve) approach when compared to the one that ships with CanCan, but, at the same time, perhaps more readable (at least to my eyes).

I am open to turning it into a gem etc., if you are interested and willing to help out with it.

Tomas

[ghost]

@tomasc hey ^^ I have to apologise, this pr somewhat slipped my mind. I meant to post my findings but came up somewhat short. The issue I was facing is that the STI check at the moment somewhat goes against the default loading of other abilities / rules. Your adapter looks really promising and I think it would be a better solution for the problem. 👍 .

[tomasc]

No worries @Liberatys , the current adapter code is very hard to follow for me. So I can understand it might have been difficult to come up with a solution within that context.

[fmang]

For the record, I’m getting similar surprising behaviors with the following test I wrote:

    it 'combines rules' do
      u1 = User.create!(name: 'pippo')
      suzuki = Suzuki.create!
      ability = Ability.new(u1)
      ability.can :read, Suzuki
      ability.can :read, Motorbike
      expect(Suzuki.accessible_by(ability)).to include suzuki
    end 

When run with DB='sqlite' bundle exec appraisal activerecord_6.1.0 rake under Ruby 3.0.0, I get:

  1) CanCan::ModelAdapters::ActiveRecordAdapter when STI is in use combines rules
     Failure/Error: expect(Suzuki.accessible_by(ability)).to include suzuki
     
       expected #<ActiveRecord::Relation []> to include #<Suzuki id: 1, type: "Suzuki">
       Diff:
       @@ -1 +1 @@
       -[#<Suzuki id: 1, type: "Suzuki">]
       +[]

If I swap the two can, or remove the can on Motorbike, the test passes. If I leave only the can on Motorbike, it fails too and should not.

[JohnSmall]

Any movement on this problem? It's stopping me upgrading Rails from 5.x to 5.1.x. In cancancan 1.16.0 adding extra restrictions to a subclass did not affect the parent class, but in cancancan 3.3.0 it does.
I have;-
class StaffUser < User etc

 if user.has_role?("account_manager")
      can :manage, User 
      cannot :manage, StaffUser

In 1.16.0 it used to prevent an account manager from managing other StaffUsers and allow an account manager to manage instances of User, i.e customers. but now in 3.3.0 it also prevents an account_manager managing instances of User (i.e customers).

My temporary workaround is to regress to 1.16.0

[c960657]

AFAICT this bug is not specific to STI but occurs during rule compression – see #783.

[armellarcier]

@c960657 I confirm that making CanCan::RulesCompressor.compress(array) just return the array fixes the problem and would fix the failing tests also.

[coorasse]

"Fixed" by #798