cancancan icon indicating copy to clipboard operation
cancancan copied to clipboard

Use `relation.and` instead of `relation.merge` whenever possible.

Open albb0920 opened this issue 4 years ago • 4 comments

Currently cancancan uses relation.merge to apply scope in ability rule to the scope of load_resource through. However, since relation.merge intends to mimic Hash#merge, (i.e. later condition on the same column replaces the previous one), this causes the load_resource through become ineffective when the scope in ability also specifics conditions on the same column.

This PR adds model adapter for ActiveRecord >= 6.1 and overrides database_records to use relation.and instead. It also tries to keep the query simple by only wrapping scopes to sub queries when necessary, which relies on private ActiveRecord API structurally_incompatible_values_for

Example:

ability file:

can :manage, Group

can :manage, Post, Post.where(group_id: user.groups) do |post|
  post.gorup.users.exists?(user.id)
end

controller:

class PostsController < ApplicationController
  load_and_authorize_resource :group
  load_and_authorize_resource through: :group
  
  def index
    # @posts will be from all groups,
    # as the condition for Post#group_id setted on `load_and_authorize_resource through`
    # is replaced by the condition specified in ability rule during relation.merge 
  end
end

References

For more information on the behavior of relation.merge and relation.and that was introduced in rails 6.1, please refer to rails/rails#39250 & rails/rails#40944

albb0920 avatar Feb 24 '21 04:02 albb0920

I'd like to tackle this by the next release that focuses on nested associations. Would it be possible for you to add the test for this case and rebase on the latest develop? If you don't have time, I'll take over. Thanks for your feedback and work

coorasse avatar Mar 23 '22 08:03 coorasse

@coorasse I won't have a lot of free time until mid April, plus I'm not very familiar with the test suite of this project. Please take this over, thanks!

albb0920 avatar Mar 23 '22 08:03 albb0920

Would love to see this one go through :). Rails 7 forced us to monkeypatch with @albb0920 solution.

kwent avatar Dec 17 '23 10:12 kwent