fix: prevent accessible_by from permanently overwriting shared rule conditions

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][])

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

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