From 959eee14e132b3f5e22b9e29649327eaf46efb37 Mon Sep 17 00:00:00 2001 From: Liberatys Date: Thu, 23 Jun 2022 12:12:14 +0000 Subject: [PATCH 01/14] Update README.md (#779) --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 47a5fcc6..83243748 100644 --- a/README.md +++ b/README.md @@ -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) From 65a85210bb7ed54d244e3c31ce401a73beb9bb09 Mon Sep 17 00:00:00 2001 From: Alessandro Rodi Date: Wed, 6 Jul 2022 14:36:22 +0200 Subject: [PATCH 02/14] Fix most recent rubocop issues (#797) --- .rubocop.yml | 3 +++ cancancan.gemspec | 2 +- lib/cancan/ability/rules.rb | 6 +++--- lib/cancan/controller_additions.rb | 2 +- lib/cancan/model_adapters/active_record_adapter.rb | 2 +- lib/cancan/rule.rb | 2 +- spec/cancan/model_adapters/active_record_adapter_spec.rb | 2 +- 7 files changed, 11 insertions(+), 8 deletions(-) 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/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/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/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/active_record_adapter.rb b/lib/cancan/model_adapters/active_record_adapter.rb index e3e84c06..2bf2941d 100644 --- a/lib/cancan/model_adapters/active_record_adapter.rb +++ b/lib/cancan/model_adapters/active_record_adapter.rb @@ -133,7 +133,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/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/spec/cancan/model_adapters/active_record_adapter_spec.rb b/spec/cancan/model_adapters/active_record_adapter_spec.rb index 90065108..d9e8bd40 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_adapter_spec.rb @@ -277,7 +277,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 From f7a6055756f77d27840cc6746447ec61e4cd4cea Mon Sep 17 00:00:00 2001 From: CH <44315+honigc@users.noreply.github.com> Date: Wed, 6 Jul 2022 08:38:54 -0400 Subject: [PATCH 03/14] Support scopes of STI classes as ability conditions (#702) Co-authored-by: Charlie Honig --- lib/cancan/model_adapters/sti_normalizer.rb | 10 +++++++++- .../model_adapters/active_record_adapter_spec.rb | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) 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/spec/cancan/model_adapters/active_record_adapter_spec.rb b/spec/cancan/model_adapters/active_record_adapter_spec.rb index d9e8bd40..c50f18bb 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_adapter_spec.rb @@ -1150,6 +1150,7 @@ class Child < Parent create_table(:vehicles) do |t| t.string :type + t.integer :capacity end end @@ -1227,5 +1228,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 From c426bb2759893eb7670438e1f60b5843acbd6b30 Mon Sep 17 00:00:00 2001 From: Alessandro Rodi Date: Wed, 6 Jul 2022 14:40:30 +0200 Subject: [PATCH 04/14] Update CHANGELOG --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98fb2e7d..82c18eda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## Unreleased +* [#702](https://github.com/CanCanCommunity/cancancan/pull/702): Support scopes of STI classes as ability conditions. ([@honigc][])) + ## 3.4.0 * [#691](https://github.com/CanCanCommunity/cancancan/pull/691): Add two new subquery strategies: `joined_alias_exists_subquery`, `joined_alias_each_rule_as_exists_subquery`. ([@kaspernj][]) @@ -700,3 +702,4 @@ 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 From 65ada12c601d8dcd27627baffd317db1804772e1 Mon Sep 17 00:00:00 2001 From: Alessandro Rodi Date: Wed, 6 Jul 2022 15:38:44 +0200 Subject: [PATCH 05/14] Correct typo in CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82c18eda..78fe81b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ ## Unreleased -* [#702](https://github.com/CanCanCommunity/cancancan/pull/702): Support scopes of STI classes as ability conditions. ([@honigc][])) +* [#702](https://github.com/CanCanCommunity/cancancan/pull/702): Support scopes of STI classes as ability conditions. ([@honigc][]) ## 3.4.0 From cc66e6347c127e6324546f90b6a6f0e0a008a9e5 Mon Sep 17 00:00:00 2001 From: Remo Fritzsche Date: Tue, 9 Feb 2021 12:02:47 +0100 Subject: [PATCH 06/14] parent c426bb2759893eb7670438e1f60b5843acbd6b30 author Remo Fritzsche 1612868567 +0100 committer Alessandro Rodi 1657114188 +0200 # This is a combination of 4 commits. # This is the 1st commit message: Preserve nil values as extra arguments # This is the commit message #2: Fix link (#744) # This is the commit message #3: Documentation fixes and improvements - fix typo, punctuations, and grammar - fix broken and misformatted links - fix code sample # This is the commit message #4: Format documentation/guides and fix linting issues The motivation of the changes is to make these documents neater when reading offline using a text or code editor. Reading the documentation offline is faster, and the source code is readily available for further learning. The changes do not affect the meaning of each documentation or instruction. The following changes were made: - remove extra and trailing spaces - add new lines to separate the title, explanation, code samples, etc - fix headings, links - specify code block language (bash, ruby) Updated Devise.md as exception is changed CanCan::Unathorized exception not exists anymore, CanCan::AccessDenied is used. Also added responses for different content types as a recommendation. Add support for non-hash conditions Co-authored-by: Juleffel Improve ability checks with Hashes Add Ruby 3.0 and Ruby 3.1 to CI Drop ActiveRecord4 from CI (#778) --- lib/cancan/conditions_matcher.rb | 8 ++++++-- spec/cancan/ability_spec.rb | 11 +++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/cancan/conditions_matcher.rb b/lib/cancan/conditions_matcher.rb index b30d3e2a..bb26937a 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) 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) From 8efb52207b370cc3c916bb34db1fcf6ff67832d1 Mon Sep 17 00:00:00 2001 From: Alex Ghiculescu Date: Wed, 6 Jul 2022 10:10:46 -0500 Subject: [PATCH 07/14] Support using a nil relation as a condition (#653) --- CHANGELOG.md | 1 + docs/hash_of_conditions.md | 7 + lib/cancan/conditions_matcher.rb | 19 +- .../active_record_adapter_spec.rb | 193 ++++++++++++++++++ 4 files changed, 219 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78fe81b5..6178c6ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased +* [#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][]) ## 3.4.0 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/lib/cancan/conditions_matcher.rb b/lib/cancan/conditions_matcher.rb index bb26937a..009c3f6a 100644 --- a/lib/cancan/conditions_matcher.rb +++ b/lib/cancan/conditions_matcher.rb @@ -101,12 +101,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/spec/cancan/model_adapters/active_record_adapter_spec.rb b/spec/cancan/model_adapters/active_record_adapter_spec.rb index c50f18bb..b1a76ebd 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_adapter_spec.rb @@ -626,6 +626,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 From 7c99c593dd55127db16b0920167814a296c38e0d Mon Sep 17 00:00:00 2001 From: Alessandro Rodi Date: Wed, 6 Jul 2022 17:18:04 +0200 Subject: [PATCH 08/14] Allow to disable rules compressor (#798) --- CHANGELOG.md | 1 + docs/rules_compression.md | 8 +++++- lib/cancan/config.rb | 23 ++++++++++++++++ .../model_adapters/active_record_adapter.rb | 10 ++++--- .../active_record_adapter_spec.rb | 26 ++++++++++++++----- spec/spec_helper.rb | 1 + 6 files changed, 59 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6178c6ec..5dece0e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ * [#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][]) ## 3.4.0 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/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/model_adapters/active_record_adapter.rb b/lib/cancan/model_adapters/active_record_adapter.rb index 2bf2941d..9eeefdcb 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 @@ -39,8 +43,8 @@ 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 foreign_key = child_class.reflect_on_all_associations(:belongs_to).find do |association| - association.klass == parent.class - end&.foreign_key&.to_sym + association.klass == parent.class + end&.foreign_key&.to_sym foreign_key.nil? ? nil : all_conditions[foreign_key] end end diff --git a/spec/cancan/model_adapters/active_record_adapter_spec.rb b/spec/cancan/model_adapters/active_record_adapter_spec.rb index b1a76ebd..efb03c62 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_adapter_spec.rb @@ -393,12 +393,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 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 From 7fe91791c8d51b1a9345e310aa3733edeb121ecc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Avenel?= Date: Sun, 5 Mar 2023 15:15:13 +0100 Subject: [PATCH 09/14] Fix a typo in friendly_id.rb (#820) The correct path is `config/initializers/cancancan.rb` --- docs/friendly_id.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) From 36b1e0f04fa7cdc07fc7135b82aae281720b2939 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serge=20H=C3=A4nni?= Date: Sun, 5 Mar 2023 15:15:35 +0100 Subject: [PATCH 10/14] Remove puts statement (#818) --- lib/cancan/conditions_matcher.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/cancan/conditions_matcher.rb b/lib/cancan/conditions_matcher.rb index 009c3f6a..818dba7e 100644 --- a/lib/cancan/conditions_matcher.rb +++ b/lib/cancan/conditions_matcher.rb @@ -71,7 +71,6 @@ def matches_all_conditions?(adapter, subject, conditions) elsif conditions.respond_to?(:include?) conditions.include?(subject) else - puts "does #{subject} match #{conditions}?" subject == conditions end end From 08ced0d2495650a8eaaec27efe95935d975eaa36 Mon Sep 17 00:00:00 2001 From: Greg Joyce <108758686+gjpsquare@users.noreply.github.com> Date: Sun, 5 Mar 2023 06:16:18 -0800 Subject: [PATCH 11/14] Fix rdoc link (#817) "too many redirects" at rdoc.info --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 83243748..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) From eaddc378514727533ef13b82c756a7af27de85f9 Mon Sep 17 00:00:00 2001 From: Pieter Date: Sun, 5 Mar 2023 15:16:48 +0100 Subject: [PATCH 12/14] Renames method name in ConditionsMatcher (#815) --- lib/cancan/conditions_matcher.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cancan/conditions_matcher.rb b/lib/cancan/conditions_matcher.rb index 818dba7e..5cac4c73 100644 --- a/lib/cancan/conditions_matcher.rb +++ b/lib/cancan/conditions_matcher.rb @@ -67,7 +67,7 @@ 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 @@ -75,7 +75,7 @@ def matches_all_conditions?(adapter, 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) From fa19c5ce31e0713558a54faf3690427067dc8b11 Mon Sep 17 00:00:00 2001 From: WriterZephos Date: Sun, 5 Mar 2023 07:20:47 -0700 Subject: [PATCH 13/14] add support for polymorphic associations in active record (#814) * add support for polymorphic associations in active record * make tests pass for polymorphic support and ensure backwards compatibility * remove unnecessary and noisy puts statement --------- Co-authored-by: Bryant Morrill --- lib/cancan/conditions_matcher.rb | 8 ++-- lib/cancan/model_adapters/abstract_adapter.rb | 5 ++ .../model_adapters/active_record_adapter.rb | 44 ++++++++++++++++- .../active_record_adapter_spec.rb | 48 +++++++++++++++++++ 4 files changed, 101 insertions(+), 4 deletions(-) diff --git a/lib/cancan/conditions_matcher.rb b/lib/cancan/conditions_matcher.rb index 5cac4c73..c1a97b96 100644 --- a/lib/cancan/conditions_matcher.rb +++ b/lib/cancan/conditions_matcher.rb @@ -39,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)) 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 9eeefdcb..6d17f863 100644 --- a/lib/cancan/model_adapters/active_record_adapter.rb +++ b/lib/cancan/model_adapters/active_record_adapter.rb @@ -42,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 + # 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 diff --git a/spec/cancan/model_adapters/active_record_adapter_spec.rb b/spec/cancan/model_adapters/active_record_adapter_spec.rb index efb03c62..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) @@ -511,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 @@ -520,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 @@ -533,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 @@ -542,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 @@ -555,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 From bd7158cb9f98ea581095b4149dc99f76fd9e696b Mon Sep 17 00:00:00 2001 From: Alessandro Rodi Date: Sun, 5 Mar 2023 15:33:42 +0100 Subject: [PATCH 14/14] Bump version and update CHANGELOG --- CHANGELOG.md | 4 +++- lib/cancan/version.rb | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5dece0e0..265e7f54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +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 @@ -705,3 +706,4 @@ Please read the [guide on migrating from CanCanCan 2.x to 3.0](https://github.co [@mtoneil]: https://github.com/mtoneil [@Juleffel]: https://github.com/Juleffel [@honigc]: https://github.com/honigc +[@WriterZephos]: https://github.com/WriterZephos 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