diff --git a/.rubocop.yml b/.rubocop.yml index 7456684e..8585e75c 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -6,6 +6,9 @@ Style/Documentation: Style/NonNilCheck: IncludeSemanticChanges: true +Style/RedundantInitialize: + Enabled: false + Style/EmptyMethod: Enabled: false diff --git a/CHANGELOG.md b/CHANGELOG.md index 98fb2e7d..265e7f54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,9 @@ -## Unreleased +## 3.5.0 + +* [#653](https://github.com/CanCanCommunity/cancancan/pull/653): Add support for using an nil relation as a condition. ([@ghiculescu][]) +* [#702](https://github.com/CanCanCommunity/cancancan/pull/702): Support scopes of STI classes as ability conditions. ([@honigc][]) +* [#798](https://github.com/CanCanCommunity/cancancan/pull/798): Allow disabling of rules compressor via `CanCan.rules_compressor_enabled = false`. ([@coorasse][]) +* [#814](https://github.com/CanCanCommunity/cancancan/pull/814): Fix issue with polymorphic associations. ([@WriterZephos][]) ## 3.4.0 @@ -700,3 +705,5 @@ Please read the [guide on migrating from CanCanCan 2.x to 3.0](https://github.co [@ghiculescu]: https://github.com/ghiculescu [@mtoneil]: https://github.com/mtoneil [@Juleffel]: https://github.com/Juleffel +[@honigc]: https://github.com/honigc +[@WriterZephos]: https://github.com/WriterZephos diff --git a/README.md b/README.md index 47a5fcc6..04fa2b68 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ [![Code Climate Badge](https://codeclimate.com/github/CanCanCommunity/cancancan.svg)](https://codeclimate.com/github/CanCanCommunity/cancancan) [Developer guide](./docs/README.md) | -[RDocs](http://rdoc.info/projects/CanCanCommunity/cancancan) | +[RDocs](https://www.rubydoc.info/github/CanCanCommunity/cancancan) | [Screencast 1](http://railscasts.com/episodes/192-authorization-with-cancan) | [Screencast 2](https://www.youtube.com/watch?v=cTYu-OjUgDw) @@ -179,6 +179,8 @@ When first developing, you need to run `bundle install` and then `bundle exec ap You can then run all appraisal files (like CI does), with `appraisal rake` or just run a specific set `DB='sqlite' bundle exec appraisal activerecord_5.2.2 rake`. +If you'd like to run a specific set of tests within a specific file or folder you can use `DB='sqlite' SPEC=path/to/file/or/folder bundle exec appraisal activerecord_5.2.2 rake`. + If you use RubyMine, you can run RSpec tests by configuring the RSpec configuration template like this: ![rubymine_rspec.png](rubymine_rspec.png) diff --git a/cancancan.gemspec b/cancancan.gemspec index 91e0c612..2302e1f1 100644 --- a/cancancan.gemspec +++ b/cancancan.gemspec @@ -25,5 +25,5 @@ Gem::Specification.new do |s| s.add_development_dependency 'bundler', '~> 2.0' s.add_development_dependency 'rake', '~> 10.1', '>= 10.1.1' s.add_development_dependency 'rspec', '~> 3.2', '>= 3.2.0' - s.add_development_dependency 'rubocop', '~> 1.26' + s.add_development_dependency 'rubocop', '~> 1.31.1' end diff --git a/docs/friendly_id.md b/docs/friendly_id.md index 8f2f92a3..b20c03eb 100644 --- a/docs/friendly_id.md +++ b/docs/friendly_id.md @@ -4,7 +4,7 @@ If you are using [FriendlyId](https://github.com/norman/friendly_id) you will pr You do not have to write `find_by :slug` or something like that, that is always error prone. -You just need to create a `config/initizializers/cancancan.rb` file with: +You just need to create a `config/initializers/cancancan.rb` file with: ```ruby if defined?(CanCanCan) diff --git a/docs/hash_of_conditions.md b/docs/hash_of_conditions.md index 6ec8d424..7375d81c 100644 --- a/docs/hash_of_conditions.md +++ b/docs/hash_of_conditions.md @@ -46,6 +46,13 @@ An array or range can be passed to match multiple values. Here the user can only can :read, Project, priority: 1..3 ``` +If you want to a negative match, you can pass in `nil`. + +```ruby +# Can read projects that don't have any members. +can :read, Project, members: { id: nil } +``` + Almost anything that you can pass to a hash of conditions in ActiveRecord will work here as well. ## Traverse associations diff --git a/docs/rules_compression.md b/docs/rules_compression.md index 077decec..a535dbf3 100644 --- a/docs/rules_compression.md +++ b/docs/rules_compression.md @@ -1,6 +1,12 @@ # Rules compressions -Your rules are optimized automatically at runtime. There are a set of "rules" to optimize your rules definition and they are implemented in the `RulesCompressor` class. Here you can see how this works: +Database are great on optimizing queries, but sometimes cancancan builds `joins` that might lead to slow performance. +This is why your rules are optimized automatically at runtime. +There are a set of "rules" to optimize your rules definition and they are implemented in the `RulesCompressor` class. +You can always disable the rules compressor by setting `CanCan.rules_compressor_enabled = false` in your initializer. +You can also enable/disable it on a specific check by using: `with_rules_compressor_enabled(false) { ... }` + +Here you can see how this works: A rule without conditions is defined as `catch_all`. diff --git a/lib/cancan/ability/rules.rb b/lib/cancan/ability/rules.rb index c7491a6e..8bee574c 100644 --- a/lib/cancan/ability/rules.rb +++ b/lib/cancan/ability/rules.rb @@ -61,8 +61,8 @@ def relevant_rules_for_match(action, subject) next unless rule.only_raw_sql? raise Error, - "The can? and cannot? call cannot be used with a raw sql 'can' definition."\ - " The checking code cannot be determined for #{action.inspect} #{subject.inspect}" + "The can? and cannot? call cannot be used with a raw sql 'can' definition. " \ + "The checking code cannot be determined for #{action.inspect} #{subject.inspect}" end end @@ -72,7 +72,7 @@ def relevant_rules_for_query(action, subject) rule.base_behavior == false && rule.attributes.present? end if rules.any?(&:only_block?) - raise Error, "The accessible_by call cannot be used with a block 'can' definition."\ + raise Error, "The accessible_by call cannot be used with a block 'can' definition." \ "The SQL cannot be determined for #{action.inspect} #{subject.inspect}" end rules diff --git a/lib/cancan/conditions_matcher.rb b/lib/cancan/conditions_matcher.rb index b30d3e2a..c1a97b96 100644 --- a/lib/cancan/conditions_matcher.rb +++ b/lib/cancan/conditions_matcher.rb @@ -18,10 +18,14 @@ def subject_class?(subject) [Class, Module].include? klass end - def matches_block_conditions(subject, *extra_args) + def matches_block_conditions(subject, attribute, *extra_args) return @base_behavior if subject_class?(subject) - @block.call(subject, *extra_args.compact) + if attribute + @block.call(subject, attribute, *extra_args) + else + @block.call(subject, *extra_args) + end end def matches_non_block_conditions(subject) @@ -35,11 +39,13 @@ def matches_non_block_conditions(subject) def nested_subject_matches_conditions?(subject_hash) parent, child = subject_hash.first - matches_base_parent_conditions = matches_conditions_hash?(parent, - @conditions[parent.class.name.downcase.to_sym] || {}) - adapter = model_adapter(parent) + parent_condition_name = adapter.parent_condition_name(parent, child) + + matches_base_parent_conditions = matches_conditions_hash?(parent, + @conditions[parent_condition_name] || {}) + matches_base_parent_conditions && (!adapter.override_nested_subject_conditions_matching?(parent, child, @conditions) || adapter.nested_subject_matches_conditions?(parent, child, @conditions)) @@ -63,16 +69,15 @@ def matches_conditions_hash?(subject, conditions = @conditions) def matches_all_conditions?(adapter, subject, conditions) if conditions.is_a?(Hash) - matches_hash_conditions(adapter, subject, conditions) + matches_hash_conditions?(adapter, subject, conditions) elsif conditions.respond_to?(:include?) conditions.include?(subject) else - puts "does #{subject} match #{conditions}?" subject == conditions end end - def matches_hash_conditions(adapter, subject, conditions) + def matches_hash_conditions?(adapter, subject, conditions) conditions.all? do |name, value| if adapter.override_condition_matching?(subject, name, value) adapter.matches_condition?(subject, name, value) @@ -97,12 +102,29 @@ def condition_match?(attribute, value) def hash_condition_match?(attribute, value) if attribute.is_a?(Array) || (defined?(ActiveRecord) && attribute.is_a?(ActiveRecord::Relation)) - attribute.to_a.any? { |element| matches_conditions_hash?(element, value) } + array_like_matches_condition_hash?(attribute, value) else attribute && matches_conditions_hash?(attribute, value) end end + def array_like_matches_condition_hash?(attribute, value) + if attribute.any? + attribute.any? { |element| matches_conditions_hash?(element, value) } + else + # you can use `nil`s in your ability definition to tell cancancan to find + # objects that *don't* have any children in a has_many relationship. + # + # for example, given ability: + # => can :read, Article, comments: { id: nil } + # cancancan will return articles where `article.comments == []` + # + # this is implemented here. `attribute` is `article.comments`, and it's an empty array. + # the expression below returns true if this was expected. + !value.values.empty? && value.values.all?(&:nil?) + end + end + def call_block_with_all(action, subject, *extra_args) if subject.class == Class @block.call(action, subject, nil, *extra_args) diff --git a/lib/cancan/config.rb b/lib/cancan/config.rb index 75e68bad..0fd8893f 100644 --- a/lib/cancan/config.rb +++ b/lib/cancan/config.rb @@ -11,6 +11,29 @@ def self.valid_accessible_by_strategies strategies end + # You can disable the rules compressor if it's causing unexpected issues. + def self.rules_compressor_enabled + return @rules_compressor_enabled if defined?(@rules_compressor_enabled) + + @rules_compressor_enabled = true + end + + def self.rules_compressor_enabled=(value) + @rules_compressor_enabled = value + end + + def self.with_rules_compressor_enabled(value) + return yield if value == rules_compressor_enabled + + begin + rules_compressor_enabled_was = rules_compressor_enabled + @rules_compressor_enabled = value + yield + ensure + @rules_compressor_enabled = rules_compressor_enabled_was + end + end + # Determines how CanCan should build queries when calling accessible_by, # if the query will contain a join. The default strategy is `:subquery`. # diff --git a/lib/cancan/controller_additions.rb b/lib/cancan/controller_additions.rb index 68f949dc..0c84f83e 100644 --- a/lib/cancan/controller_additions.rb +++ b/lib/cancan/controller_additions.rb @@ -264,7 +264,7 @@ def check_authorization(options = {}) next if options[:unless] && controller.send(options[:unless]) raise AuthorizationNotPerformed, - 'This action failed the check_authorization because it does not authorize_resource. '\ + 'This action failed the check_authorization because it does not authorize_resource. ' \ 'Add skip_authorization_check to bypass this check.' end diff --git a/lib/cancan/model_adapters/abstract_adapter.rb b/lib/cancan/model_adapters/abstract_adapter.rb index 4041dcb4..d934b596 100644 --- a/lib/cancan/model_adapters/abstract_adapter.rb +++ b/lib/cancan/model_adapters/abstract_adapter.rb @@ -35,6 +35,11 @@ def self.matches_conditions_hash?(_subject, _conditions) raise NotImplemented, 'This model adapter does not support matching on a conditions hash.' end + # Override if parent condition could be under a different key in conditions + def self.parent_condition_name(parent, _child) + parent.class.name.downcase.to_sym + end + # Used above override_conditions_hash_matching to determine if this model adapter will override the # matching behavior for nested subject. # If this returns true then nested_subject_matches_conditions? will be called. diff --git a/lib/cancan/model_adapters/active_record_adapter.rb b/lib/cancan/model_adapters/active_record_adapter.rb index e3e84c06..6d17f863 100644 --- a/lib/cancan/model_adapters/active_record_adapter.rb +++ b/lib/cancan/model_adapters/active_record_adapter.rb @@ -15,7 +15,11 @@ def self.version_lower?(version) def initialize(model_class, rules) super - @compressed_rules = RulesCompressor.new(@rules.reverse).rules_collapsed.reverse + @compressed_rules = if CanCan.rules_compressor_enabled + RulesCompressor.new(@rules.reverse).rules_collapsed.reverse + else + @rules + end StiNormalizer.normalize(@compressed_rules) ConditionsNormalizer.normalize(model_class, @compressed_rules) end @@ -38,11 +42,53 @@ def nested_subject_matches_conditions?(parent, child, all_conditions) def parent_child_conditions(parent, child, all_conditions) child_class = child.is_a?(Class) ? child : child.class + parent_class = parent.is_a?(Class) ? parent : parent.class + foreign_key = child_class.reflect_on_all_associations(:belongs_to).find do |association| - association.klass == parent.class - end&.foreign_key&.to_sym + # Do not match on polymorphic associations or it will throw an error (klass cannot be determined) + !association.polymorphic? && association.klass == parent.class + end&.foreign_key&.to_sym + + # Search again in case of polymorphic associations, this time matching on the :has_many side + # via the :as option, as well as klass + foreign_key ||= parent_class.reflect_on_all_associations(:has_many).find do |has_many_assoc| + !matching_parent_child_polymorphic_association(has_many_assoc, child_class).nil? + end&.foreign_key&.to_sym + foreign_key.nil? ? nil : all_conditions[foreign_key] end + + def matching_parent_child_polymorphic_association(parent_assoc, child_class) + return nil unless parent_assoc.klass == child_class + return nil if parent_assoc&.options[:as].nil? + + child_class.reflect_on_all_associations(:belongs_to).find do |child_assoc| + # Only match this way for polymorphic associations + child_assoc.polymorphic? && child_assoc.name == parent_assoc.options[:as] + end + end + + def child_association_to_parent(parent, child) + child_class = child.is_a?(Class) ? child : child.class + parent_class = parent.is_a?(Class) ? parent : parent.class + + association = child_class.reflect_on_all_associations(:belongs_to).find do |association| + # Do not match on polymorphic associations or it will throw an error (klass cannot be determined) + !association.polymorphic? && association.klass == parent.class + end + + return association unless association.nil? + + parent_class.reflect_on_all_associations(:has_many).each do |has_many_assoc| + association ||= matching_parent_child_polymorphic_association(has_many_assoc, child_class) + end + + association + end + + def parent_condition_name(parent, child) + child_association_to_parent(parent, child)&.name || parent.class.name.downcase.to_sym + end end # Returns conditions intended to be used inside a database query. Normally you will not call this @@ -133,7 +179,7 @@ def override_scope def raise_override_scope_error rule_found = @compressed_rules.detect { |rule| rule.conditions.is_a?(ActiveRecord::Relation) } raise Error, - 'Unable to merge an Active Record scope with other conditions. '\ + 'Unable to merge an Active Record scope with other conditions. ' \ "Instead use a hash or SQL for #{rule_found.actions.first} #{rule_found.subjects.first} ability." end diff --git a/lib/cancan/model_adapters/sti_normalizer.rb b/lib/cancan/model_adapters/sti_normalizer.rb index 4e656f01..e6cb3142 100644 --- a/lib/cancan/model_adapters/sti_normalizer.rb +++ b/lib/cancan/model_adapters/sti_normalizer.rb @@ -30,8 +30,16 @@ def update_rule(subject, rule, rules_cache) # create a new rule for the subclasses that links on the inheritance_column def build_rule_for_subclass(rule, subject) + sti_conditions = { subject.inheritance_column => subject.sti_name } + new_rule_conditions = + if rule.with_scope? + rule.conditions.where(sti_conditions) + else + rule.conditions.merge(sti_conditions) + end + CanCan::Rule.new(rule.base_behavior, rule.actions, subject.superclass, - rule.conditions.merge(subject.inheritance_column => subject.sti_name), rule.block) + new_rule_conditions, rule.block) end end end diff --git a/lib/cancan/rule.rb b/lib/cancan/rule.rb index 18df3895..cdab1643 100644 --- a/lib/cancan/rule.rb +++ b/lib/cancan/rule.rb @@ -123,7 +123,7 @@ def parse_attributes_from_extra_args(args) def condition_and_block_check(conditions, block, action, subject) return unless conditions.is_a?(Hash) && block - raise BlockAndConditionsError, 'A hash of conditions is mutually exclusive with a block. '\ + raise BlockAndConditionsError, 'A hash of conditions is mutually exclusive with a block. ' \ "Check \":#{action} #{subject}\" ability." end diff --git a/lib/cancan/version.rb b/lib/cancan/version.rb index d5cf6e1f..7477402a 100644 --- a/lib/cancan/version.rb +++ b/lib/cancan/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module CanCan - VERSION = '3.4.0'.freeze + VERSION = '3.5.0'.freeze end diff --git a/spec/cancan/ability_spec.rb b/spec/cancan/ability_spec.rb index 8bfade06..486cbc42 100644 --- a/spec/cancan/ability_spec.rb +++ b/spec/cancan/ability_spec.rb @@ -127,6 +127,17 @@ expect(@block_called).to be(true) end + it 'allows passing nil as extra arguments' do + @ability.can :to_s, Integer do |integer, arg1, arg2| + expect(integer).to eq(42) + expect(arg1).to eq(nil) + expect(arg2).to eq(:foo) + @block_called = true + end + @ability.can?(:to_s, 42, nil, nil, :foo) + expect(@block_called).to be(true) + end + it 'passes nil to object when comparing class with can check' do @ability.can do |action, object_class, object| expect(action).to eq(:foo) diff --git a/spec/cancan/model_adapters/active_record_adapter_spec.rb b/spec/cancan/model_adapters/active_record_adapter_spec.rb index 90065108..810af592 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_adapter_spec.rb @@ -60,6 +60,12 @@ t.timestamps null: false end + create_table(:attachments) do |t| + t.integer :attachable_id + t.string :attachable_type + t.timestamps null: false + end + create_table(:users) do |t| t.string :name t.timestamps null: false @@ -86,6 +92,7 @@ class Article < ActiveRecord::Base has_many :mentioned_users, through: :mentions, source: :user belongs_to :user belongs_to :project + has_many :attachments, as: :attachable scope :unpopular, lambda { joins('LEFT OUTER JOIN comments ON (comments.post_id = posts.id)') @@ -103,6 +110,7 @@ class Mention < ActiveRecord::Base class Comment < ActiveRecord::Base belongs_to :article belongs_to :project + has_many :attachments, as: :attachable end class LegacyComment < ActiveRecord::Base @@ -110,10 +118,15 @@ class LegacyComment < ActiveRecord::Base belongs_to :project end + class Attachment < ActiveRecord::Base + belongs_to :attachable, polymorphic: true + end + class User < ActiveRecord::Base has_many :articles has_many :mentions has_many :mentioned_articles, through: :mentions, source: :article + has_many :comments, through: :articles end (@ability = double).extend(CanCan::Ability) @@ -277,7 +290,7 @@ class User < ActiveRecord::Base @ability.can :read, Article, Article.where(secret: true) expect { Article.accessible_by(@ability) } .to raise_error(CanCan::Error, - 'Unable to merge an Active Record scope with other conditions. '\ + 'Unable to merge an Active Record scope with other conditions. ' \ 'Instead use a hash or SQL for read Article ability.') end @@ -393,12 +406,26 @@ class User < ActiveRecord::Base expect(@ability.model_adapter(Article, :read).joins).to be_nil end - it 'has nil joins if rules got compressed' do - @ability.can :read, Comment, article: { category: { visible: true } } - @ability.can :read, Comment - expect(@ability.model_adapter(Comment, :read)) - .to generate_sql("SELECT \"#{@comment_table}\".* FROM \"#{@comment_table}\"") - expect(@ability.model_adapter(Comment, :read).joins).to be_nil + context 'if rules got compressed' do + it 'has nil joins' do + @ability.can :read, Comment, article: { category: { visible: true } } + @ability.can :read, Comment + expect(@ability.model_adapter(Comment, :read)) + .to generate_sql("SELECT \"#{@comment_table}\".* FROM \"#{@comment_table}\"") + expect(@ability.model_adapter(Comment, :read).joins).to be_nil + end + end + + context 'if rules did not get compressed' do + before :each do + CanCan.rules_compressor_enabled = false + end + + it 'has joins' do + @ability.can :read, Comment, article: { category: { visible: true } } + @ability.can :read, Comment + expect(@ability.model_adapter(Comment, :read).joins).to be_present + end end it 'has nil joins if no nested hashes specified in conditions' do @@ -497,6 +524,10 @@ class User < ActiveRecord::Base @comment2 = Comment.create!(article: @article2) @legacy_comment1 = LegacyComment.create!(article: @article1) @legacy_comment2 = LegacyComment.create!(article: @article2) + @attachment1 = Attachment.create!(attachable: @article1) + @comment_attachment1 = Attachment.create!(attachable: @comment1) + @attachment2 = Attachment.create!(attachable: @article2) + @comment_attachment2 = Attachment.create!(attachable: @comment2) end context 'when conditions are defined using the parent model' do @@ -506,6 +537,8 @@ class User < ActiveRecord::Base ability.can :manage, Article, user: user1 ability.can :manage, Comment, article: user1.articles ability.can :manage, LegacyComment, article: user1.articles + ability.can :manage, Attachment, attachable: user1.articles + ability.can :manage, Attachment, attachable: user1.comments end end @@ -519,6 +552,20 @@ class User < ActiveRecord::Base expect(ability.can?(:manage, { @article2 => LegacyComment })).to eq(false) expect(ability.can?(:manage, { @article2 => @legacy_comment2 })).to eq(false) end + + context 'when the association is polymorphic' do + it 'verifies parent equality correctly' do + expect(ability.can?(:manage, { @article1 => Attachment })).to eq(true) + expect(ability.can?(:manage, { @article1 => @attachment1 })).to eq(true) + expect(ability.can?(:manage, { @comment1 => Attachment })).to eq(true) + expect(ability.can?(:manage, { @comment1 => @comment_attachment1 })).to eq(true) + + expect(ability.can?(:manage, { @article2 => Attachment })).to eq(false) + expect(ability.can?(:manage, { @article2 => @attachment2 })).to eq(false) + expect(ability.can?(:manage, { @comment2 => Attachment })).to eq(false) + expect(ability.can?(:manage, { @comment2 => @comment_attachment2 })).to eq(false) + end + end end context 'when conditions are defined using the parent id' do @@ -528,6 +575,7 @@ class User < ActiveRecord::Base ability.can :manage, Article, user_id: user1.id ability.can :manage, Comment, article_id: user1.article_ids ability.can :manage, LegacyComment, post_id: user1.article_ids + ability.can :manage, Attachment, attachable_id: user1.article_ids end end @@ -541,6 +589,20 @@ class User < ActiveRecord::Base expect(ability.can?(:manage, { @article2 => LegacyComment })).to eq(false) expect(ability.can?(:manage, { @article2 => @legacy_comment2 })).to eq(false) end + + context 'when the association is polymorphic' do + it 'verifies parent equality correctly' do + expect(ability.can?(:manage, { @article1 => Attachment })).to eq(true) + expect(ability.can?(:manage, { @article1 => @attachment1 })).to eq(true) + expect(ability.can?(:manage, { @comment1 => Attachment })).to eq(true) + expect(ability.can?(:manage, { @comment1 => @comment_attachment1 })).to eq(true) + + expect(ability.can?(:manage, { @article2 => Attachment })).to eq(false) + expect(ability.can?(:manage, { @article2 => @attachment2 })).to eq(false) + expect(ability.can?(:manage, { @comment2 => Attachment })).to eq(false) + expect(ability.can?(:manage, { @comment2 => @comment_attachment2 })).to eq(false) + end + end end end @@ -626,6 +688,199 @@ class User < ActiveRecord::Base end end + it 'allows an empty array to be used as a condition for a has_many, but this is never a passing condition' do + a1 = Article.create! + a2 = Article.create! + a2.comments = [Comment.create!] + + @ability.can :read, Article, comment_ids: [] + + expect(@ability.can?(:read, a1)).to eq(false) + expect(@ability.can?(:read, a2)).to eq(false) + + expect(Article.accessible_by(@ability)).to eq([]) + + if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + expect(@ability.model_adapter(Article, :read)).to generate_sql(%( + SELECT "articles".* + FROM "articles" + WHERE 1=0)) + end + end + + it 'allows a nil to be used as a condition for a has_many - with join' do + a1 = Article.create! + a2 = Article.create! + a2.comments = [Comment.create!] + + @ability.can :read, Article, comments: { id: nil } + + expect(@ability.can?(:read, a1)).to eq(true) + expect(@ability.can?(:read, a2)).to eq(false) + + expect(Article.accessible_by(@ability)).to eq([a1]) + + if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + expect(@ability.model_adapter(Article, :read)).to generate_sql(%( + SELECT "articles".* + FROM "articles" + WHERE "articles"."id" IN (SELECT "articles"."id" FROM "articles" + LEFT OUTER JOIN "comments" ON "comments"."article_id" = "articles"."id" + WHERE "comments"."id" IS NULL))) + end + end + + it 'allows several nils to be used as a condition for a has_many - with join' do + a1 = Article.create! + a2 = Article.create! + a2.comments = [Comment.create!] + + @ability.can :read, Article, comments: { id: nil, spam: nil } + + expect(@ability.can?(:read, a1)).to eq(true) + expect(@ability.can?(:read, a2)).to eq(false) + + expect(Article.accessible_by(@ability)).to eq([a1]) + + if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + expect(@ability.model_adapter(Article, :read)).to generate_sql(%( + SELECT "articles".* + FROM "articles" + WHERE "articles"."id" IN (SELECT "articles"."id" FROM "articles" + LEFT OUTER JOIN "comments" ON "comments"."article_id" = "articles"."id" + WHERE "comments"."id" IS NULL AND "comments"."spam" IS NULL))) + end + end + + it 'doesn\'t permit anything if a nil is used as a condition for a has_many alongside other attributes' do + a1 = Article.create! + a2 = Article.create! + a2.comments = [Comment.create!(spam: true)] + a3 = Article.create! + a3.comments = [Comment.create!(spam: false)] + + # if we are checking for `id: nil` and any other criteria, we should never return any Article. + # either the Article has Comments, which means `id: nil` fails. + # or the Article has no Comments, which means `spam: true` fails. + @ability.can :read, Article, comments: { id: nil, spam: true } + + expect(@ability.can?(:read, a1)).to eq(false) + expect(@ability.can?(:read, a2)).to eq(false) + expect(@ability.can?(:read, a3)).to eq(false) + + expect(Article.accessible_by(@ability)).to eq([]) + + if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + expect(@ability.model_adapter(Article, :read)).to generate_sql(%( + SELECT "articles".* + FROM "articles" + WHERE "articles"."id" IN (SELECT "articles"."id" FROM "articles" + LEFT OUTER JOIN "comments" ON "comments"."article_id" = "articles"."id" + WHERE "comments"."id" IS NULL AND "comments"."spam" = #{true_v}))) + end + end + + it 'doesn\'t permit if a nil is used as a condition for a has_many alongside other attributes - false case' do + a1 = Article.create! + a2 = Article.create! + a2.comments = [Comment.create!(spam: true)] + a3 = Article.create! + a3.comments = [Comment.create!(spam: false)] + + # if we are checking for `id: nil` and any other criteria, we should never return any Article. + # either the Article has Comments, which means `id: nil` fails. + # or the Article has no Comments, which means `spam: false` fails. + @ability.can :read, Article, comments: { id: nil, spam: false } + + expect(@ability.can?(:read, a1)).to eq(false) + expect(@ability.can?(:read, a2)).to eq(false) + expect(@ability.can?(:read, a3)).to eq(false) + + expect(Article.accessible_by(@ability)).to eq([]) + + if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + expect(@ability.model_adapter(Article, :read)).to generate_sql(%( + SELECT "articles".* + FROM "articles" + WHERE "articles"."id" IN (SELECT "articles"."id" FROM "articles" + LEFT OUTER JOIN "comments" ON "comments"."article_id" = "articles"."id" + WHERE "comments"."id" IS NULL AND "comments"."spam" = #{false_v}))) + end + end + + it 'allows a nil to be used as a condition for a has_many when combined with other conditions' do + a1 = Article.create! + a2 = Article.create! + a2.comments = [Comment.create!(spam: true)] + a3 = Article.create! + a3.comments = [Comment.create!(spam: false)] + + @ability.can :read, Article, comments: { spam: true } + @ability.can :read, Article, comments: { id: nil } + + expect(@ability.can?(:read, a1)).to eq(true) # true because no comments + expect(@ability.can?(:read, a2)).to eq(true) # true because has comments but they have spam=true + expect(@ability.can?(:read, a3)).to eq(false) # false because has comments but none with spam=true + + expect(Article.accessible_by(@ability).sort_by(&:id)).to eq([a1, a2].sort_by(&:id)) + + if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + expect(@ability.model_adapter(Article, :read)).to generate_sql(%( + SELECT "articles".* + FROM "articles" + WHERE "articles"."id" IN (SELECT "articles"."id" FROM "articles" + LEFT OUTER JOIN "comments" ON "comments"."article_id" = "articles"."id" + WHERE (("comments"."id" IS NULL) OR ("comments"."spam" = #{true_v}))))) + end + end + + it 'allows a nil to be used as a condition for a has_many alongside other attributes on the parent' do + a1 = Article.create!(secret: true) + a2 = Article.create!(secret: true) + a2.comments = [Comment.create!] + a3 = Article.create!(secret: false) + a3.comments = [Comment.create!] + a4 = Article.create!(secret: false) + + @ability.can :read, Article, secret: true, comments: { id: nil } + + expect(@ability.can?(:read, a1)).to eq(true) + expect(@ability.can?(:read, a2)).to eq(false) + expect(@ability.can?(:read, a3)).to eq(false) + expect(@ability.can?(:read, a4)).to eq(false) + + expect(Article.accessible_by(@ability)).to eq([a1]) + + if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + expect(@ability.model_adapter(Article, :read)).to generate_sql(%( + SELECT "articles".* + FROM "articles" + WHERE "articles"."id" IN (SELECT "articles"."id" FROM "articles" + LEFT OUTER JOIN "comments" ON "comments"."article_id" = "articles"."id" + WHERE "articles"."secret" = #{true_v} AND "comments"."id" IS NULL))) + end + end + + it 'allows an empty array to be used as a condition for a belongs_to; this never returns true' do + a1 = Article.create! + a2 = Article.create! + a2.project = Project.create! + + @ability.can :read, Article, project_id: [] + + expect(@ability.can?(:read, a1)).to eq(false) + expect(@ability.can?(:read, a2)).to eq(false) + + expect(Article.accessible_by(@ability)).to eq([]) + + if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0') + expect(@ability.model_adapter(Article, :read)).to generate_sql(%( + SELECT "articles".* + FROM "articles" + WHERE 1=0)) + end + end + context 'with namespaced models' do before :each do ActiveRecord::Schema.define do @@ -1150,6 +1405,7 @@ class Child < Parent create_table(:vehicles) do |t| t.string :type + t.integer :capacity end end @@ -1227,5 +1483,18 @@ class Suzuki < Motorbike expect(ability).to be_able_to(:read, motorbike) expect(ability).to be_able_to(:read, Suzuki.new) end + + it 'allows a scope of a subclass for conditions' do + u1 = User.create!(name: 'pippo') + car = Car.create!(capacity: 2) + Car.create!(capacity: 4) + Motorbike.create!(capacity: 2) + + ability = Ability.new(u1) + ability.can :read, [Car], Car.where(capacity: 2) + expect(Vehicle.accessible_by(ability)).to match_array([car]) + expect(Car.accessible_by(ability)).to eq([car]) + expect(Motorbike.accessible_by(ability)).to eq([]) + end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 7f2a4ab8..72b4ac23 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -30,6 +30,7 @@ config.after :each do CanCan.accessible_by_strategy = CanCan.default_accessible_by_strategy + CanCan.rules_compressor_enabled = true end end