STI support in 3.2.0 is a breaking change

[Fryie]

Steps to reproduce

So, here is some fun change in behaviour in the 3.2.0 release, related to the STI support.

The following tests fail in 3.2.0, but they pass in the previous 3.1.0 version: https://gist.github.com/Fryie/46b759ca19efa28e1dfdb41a54742e1b

Basically, calling index on some resource may lead to an authorization failure if access to a subclass of that resource is disallowed. Changing the order of the two ability rules also affects the code in a somewhat unintuitive(?) way: the previously failing assertion passes, but the one that used to pass doesn't anymore. (This seems due to the fact that matching rules are selected in reverse order).

Expected behavior

I'm not sure what the expected behaviour should be here. Inheritance generally makes reasoning about something like this harder: should I be able to index all parents even if some of them might actually be children, which I'm not technically able to access? Maybe not (but then again, what if my controller automatically filters the children out?)? But probably this change is also unexpected in a minor release. At least a gotcha/workaround could be added somewhere in the documentation.

The problem is unfortunately compounded in my case by the fact that ActiveAdmin doesn't distinguish between index and show. So saying something like "the user should be able to list all parents (including, or excluding children), but the show action should be forbidden for children" doesn't actually seem to be possible. As far as I am aware, this used to work in 3.1.0.

Actual behavior

See above

System configuration

Rails version:
see gist

Ruby version:
ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin19]

CanCanCan version
see gist

[coorasse]

The test on line https://gist.github.com/Fryie/46b759ca19efa28e1dfdb41a54742e1b#file-cancan_bug-rb-L62 should definitely return true in my opinion, so we have a regression.
can :index, ModelClass should return true since you can index the class itself. I don't think is possible to use STI here in a smart way.

@Liberatys what is your opinion on this issue?

[tagliala]

Hi,

I'm experiencing a similar regression with breaking change when permissions is only assigned to a subclass, propagating to the parent class.

Git bisect indicates that the breaking change has been introduced in this commit: 5d39575

class Motorbike < ApplicationRecord
end

class Suzuki < Motorbike
end

class Ability
  include CanCan::Ability

  def initialize(user = nil)
    can :manage, Suzuki
  end
end

3.1

> CanCan::VERSION
=> "3.1.0"
> Ability.new.can? :manage, Suzuki
=> true
> Ability.new.can? :manage, Motorbike
=> false

3.2

> CanCan::VERSION
=> "3.2.1"
> Ability.new.can? :manage, Suzuki
=> true
> Ability.new.can? :manage, Motorbike
=> true

[axlekb]

@Liberatys, would you be able to assist here as it appears that your commit introduced a regression?

[ghost]

@axlekb, of course, ^^ I'll look into finding a solution for the issue over the weekend as it has caused quite some issues and it will take a bit more time as it affects multiple operations :D

[tagliala]

Any news about this?

I can see there was an attempt to fix at #688, but the PR is now closed

[ghost]

@tagliala The initial solution was somewhat difficult to extend and I tried to extract it into an easier format. #689 - Should fix the issue.

[tagliala]

@Liberatys thanks. Didn't see that PR linked to this issue

[georf]

What is the argument against merging the PR? #689

[coorasse]

This should be closed in 3.4.0 now.