diff --git a/CHANGELOG.md b/CHANGELOG.md index cc34c198..e10ac627 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## Unreleased + +* [#876](https://github.com/CanCanCommunity/cancancan/issues/876): Fix `accessible_by` permanently overwriting shared rule conditions, causing `can?` to fail for models sharing the same rule. ([@say-apm35][]) + ## 3.6.0 * [#849](https://github.com/CanCanCommunity/cancancan/pull/849): Update tests matrix. ([@coorasse][]) diff --git a/lib/cancan/model_adapters/active_record_adapter.rb b/lib/cancan/model_adapters/active_record_adapter.rb index 33766c0a..39a03ed8 100644 --- a/lib/cancan/model_adapters/active_record_adapter.rb +++ b/lib/cancan/model_adapters/active_record_adapter.rb @@ -18,11 +18,18 @@ def self.version_lower?(version) def initialize(model_class, rules) super - @compressed_rules = if CanCan.rules_compressor_enabled - RulesCompressor.new(@rules.reverse).rules_collapsed.reverse - else - @rules - end + base_rules = if CanCan.rules_compressor_enabled + RulesCompressor.new(@rules.reverse).rules_collapsed.reverse + else + @rules + end + # Dup each rule so that normalization for this model class does not permanently + # alter the shared rule objects. ConditionsNormalizer replaces rule.conditions with + # a newly computed hash, which overwrites the shared rule's conditions and corrupts + # subsequent can? checks for other models sharing the same rule. Duping the rule + # gives each adapter its own Rule shell so the conditions= assignment is local. + # See GitHub issue #876. + @compressed_rules = base_rules.map(&:dup) StiNormalizer.normalize(@compressed_rules) ConditionsNormalizer.normalize(model_class, @compressed_rules) end diff --git a/spec/cancan/model_adapters/conditions_normalizer_spec.rb b/spec/cancan/model_adapters/conditions_normalizer_spec.rb index 9368ae65..754ad8bd 100644 --- a/spec/cancan/model_adapters/conditions_normalizer_spec.rb +++ b/spec/cancan/model_adapters/conditions_normalizer_spec.rb @@ -5,36 +5,38 @@ connect_db ActiveRecord::Migration.verbose = false ActiveRecord::Schema.define do - create_table(:articles) do |t| + create_table(:articles, force: true) do |t| end - create_table(:users) do |t| + create_table(:users, force: true) do |t| t.string :name end - create_table(:comments) do |t| + create_table(:comments, force: true) do |t| end - create_table(:spread_comments) do |t| + create_table(:spread_comments, force: true) do |t| t.integer :article_id t.integer :comment_id end - create_table(:legacy_mentions) do |t| + create_table(:legacy_mentions, force: true) do |t| t.integer :user_id t.integer :article_id end - create_table(:attachments) do |t| + create_table(:attachments, force: true) do |t| t.references :record, polymorphic: true t.integer :blob_id end - create_table(:blob) do |t| + create_table(:blob, force: true) do |t| end end class Article < ActiveRecord::Base + self.record_timestamps = false + has_many :spread_comments has_many :comments, through: :spread_comments has_many :mentions @@ -43,11 +45,15 @@ class Article < ActiveRecord::Base end class Comment < ActiveRecord::Base + self.record_timestamps = false + has_many :spread_comments has_many :articles, through: :spread_comments end class SpreadComment < ActiveRecord::Base + self.record_timestamps = false + belongs_to :comment belongs_to :article end @@ -105,4 +111,32 @@ class AccountHistory < ActiveRecord::Base CanCan::ModelAdapters::ConditionsNormalizer.normalize(Supplier, [rule]) expect(rule.conditions).to eq(accountant: { account_history: { name: 'pippo' } }) end + + context 'with accessible_by using has_many-through conditions' do + let(:ability) { double.extend(CanCan::Ability) } + + before do + @article1 = Article.create! + @comment1 = Comment.create! + SpreadComment.create!(article: @article1, comment: @comment1) + end + + it 'does not overwrite shared rule conditions after accessible_by is called' do + # Regression test for issue #876. + # accessible_by triggers ConditionsNormalizer which expands has_many-through conditions + # and writes the result back to rule.conditions on the shared Rule object. + # Without the adapter fix, the shared Rule's conditions are permanently replaced, + # breaking future can? checks that rely on the original un-normalized conditions. + ability.can :read, Comment, articles: { id: @article1.id } + rule = ability.send(:rules).last + conditions_before_accessible_by = rule.conditions + + Comment.accessible_by(ability) + + # The shared Rule object must not have its conditions replaced. + # Without the fix: rule.conditions becomes { spread_comments: { article: { id: ... } } }. + # With the fix: rule.conditions is still the original hash object. + expect(rule.conditions).to be(conditions_before_accessible_by) + end + end end