Improve ability checks with nested resources (hash checks)

[Juleffel]

When we use rails nested resources, cancancan does ability checks with can? :action, { parent => Class }. In this case, depending on how abilities are defined, we can have bugs and unexpected behaviours. I believe these are important issues that should be solved quickly.

I created an example application listing the problems arising depending on different ability configurations. You just need to clone it, seed it, and launch it. The explanations are on the home page.

I also created a gist to show up the exact examples of can? that are failing.

I also believe issue #661 is related to this.

[coorasse]

I am looking at the gist. In particular at this part:

class AbilityCanID
  include CanCan::Ability

  def initialize(user)
    can :read, Book, user_id: [0, user.id]
  end
end

what I understand from here is "the user can read books with user_id 0 or with my user_id (they belong to me). So afterwards you do:

user1 = User.create!
user2 = User.create!
Book.create([{user: user1}, {user: user2}])

ability = AbilityCanID.new(user1)
assert_equal true, ability.can?(:read, {user1 => Book})
assert_equal false, ability.can?(:read, {user2 => Book})

I am not entirely sure what you are checking with these assertions. Shouldn't you rather check:

user1 = User.create!
user2 = User.create!
book1, book2  = Book.create([{user: user1}, {user: user2}])

ability = AbilityCanID.new(user1)
assert_equal true, ability.can?(:read, book1)
assert_equal false, ability.can?(:read, book2)

?

[coorasse]

I also looked into /companies/3/project1s/custom_collection from your Rails app.
What you say is:

As it's not an owned company, we should not be able to see any projects.
We should not be able to access this action.

The permissions are:

can :read, Company
can :manage, Company, id: user.company_ids

can :read, Project1
can :manage, Project1, company_id: user.company_ids

1. what I read from these permissions is: "you can read companies, manage only yours".
2. the controller checks against your "read" permissions for the company (default) so it passes this check
3. the controller then checks the action :custom_collection (https://github.com/CanCanCommunity/cancancan/blob/develop/docs/changing_defaults.md#non-restful-controllers) but is not RESTful and therefore treats it as a collection and say: "give me all the project1 which I can custom_collection on", which is none due to can :manage, Project1, company_id: user.company_ids

If you want to have this action accessible only if you can manage a company, then you should check this manually in the action with authorize! :manage, @company.

[coorasse]

On the page /companies/3/project1s/ it states: "As it's not an owned company, we should not be able to see any projects.", but the ability says can :read, Project1, so you can actually read them

[Juleffel]

Concerning your first comment, the reason I check can?(:read, {user1 => Book}) instead of can?(:read, book1) is because that's what, it seems, is checked by cancancan when we use load_and_authorize_resource through: ..., for a controller. Or at least that's what it seemed to do with debugger

For the second point:

1. agreed
2. agreed
3. the problem is here, depending on the definition of the can :manage, ProjectX, company_id: user.company_ids, we'll pass, or not.

The check done will actually be can? :custom_collection, {company => Project1s}
If the ability is defined with can :manage, ProjectX, company: {id: user.company_ids} , then the rule will be checked against the ID of the company, as it will compare the parent (company) against id: user.company_ids and enforce this rule, therefore only allowing :custom_collection when the company id is in user.company_ids, and preventing access when it's not.

However, if the rule is defined with can :manage, ProjectX, company_id: user.company_ids, the rule cannot be automatically checked against company, the way it is coded today, therefore it will just consider it OK, all the time, for a can? :custom_collection, {company => Project1s} (difference between access to pages in Project 1 VS Project 3). The accessible_by will then reduce the dataset by filtering on accessible_by, so we won't have access to the data but we are still able to access the page.

It's even more flagrant when the rule is defined with cannot (Project 4 VS 6), because then the rule cannot :manage, ProjectX, company_id: user.company_ids will be recognized as a rule for :custom_collection, but just denied all the time, as it isn't checked against company, even is we are in a path and nested resource were our company id is in user.company_ids.

Concerning your last comment, indeed, there is a problem with the sentence I display at the top of the page for the index. You're right, we have read access to everything, I had the custom actions in mind when writing this. Everything always works in index, as it's a read access, the most important things is what's happening in the custom actions, as that's where we are trying to hit the can? :manage, ProjectX, ...... rule.

The other rules about read, or about companies, are not really relevant here.

The best explanations about what we should be able to/not be able to access are on the root page.

Thanks for considering the PR, and don't hesitate to ask me if it's still not clear

[coorasse]

Hi @Juleffel , thanks for the call today. I merged the first part of your changes on #772. Can you please rebase this PR so that it gets updated with the latest develop? Thanks

[Juleffel]

Thanks, done :)

[coorasse]

I propose to improve the current solution by considering also cases where the foreign key is not following a standard ParentName_id.
We can use:

_child.reflect_on_all_associations(:belongs_to).find { |association| association.klass == parent.class }.foreign_key

to identify the correct name for the foreign key. What do you think?

[Juleffel]

Nice thought indeed :)

[coorasse]

I added this in a separate branch