CanCanCommunity / cancancan

The authorization Gem for Ruby on Rails.
MIT License
5.56k stars 637 forks source link

Enumerable condition with Range #613

Open 23tux opened 4 years ago

23tux commented 4 years ago

Hi, consider the following ability:

class MyAbility
  include CanCan::Ability

  def initialize
    can %i[index show], Product, count_on_hand: [1..Float::INFINITY, nil]
  end
end

ability = MyAbility.new

The user should have access to products that have count_on_hand set to nil or at least 1. This works when using accessible_by:

puts Product.accessible_by(ability).to_sql
# => SELECT `products`.* FROM `products` WHERE (`products`.`count_on_hand` IS NULL OR `products`.`count_on_hand` >= 1)

However, when using it via can? it doesn't work:

puts ability.can?(:show, Product.new(count_on_hand: 1))
# => false # WRONG

I digged into the source code and found out, that it simply uses a value.include?(attribute) when an Enumerable is used for a matcher

def condition_match?(attribute, value)
  case value
  when Hash
    hash_condition_match?(attribute, value)
  when Range
    value.cover?(attribute)
  when Enumerable
    value.include?(attribute) # <---- her it asks [1..Float::INFINITY, nil].include?(1) which is false
  else
    attribute == value
  end
end

It would be great, if the behaviour for hash conditions used for scopes and on a per object basis would be the same.

Unfortunately, I can't use

cannot %i[index show], Product, count_on_hand: 0

because this would exclude rows where count_on_hand is NULL

For now, I'm using a monkeypatch like this

def condition_match?(attribute, value)
  case value
  when Hash
    hash_condition_match?(attribute, value)
  when Range
    value.cover?(attribute)
  when Enumerable
    value.any? { |v| condition_match?(attribute, v) }
  else
    attribute == value
  end
end

But I'm unsure if this is a good / the right solution.

coorasse commented 4 years ago

Would

can %i[index show], Product, count_on_hand: [1..Float::INFINITY]
can %i[index show], Product, count_on_hand: nil

work? If so, I'd close this because the semantic seems legit.