From 901e58a9a07617227b341a4c3b4860b306d87230 Mon Sep 17 00:00:00 2001 From: Sai Asish Y Date: Sun, 24 May 2026 19:58:16 -0700 Subject: [PATCH] fix: dup rules in ActiveRecordAdapter to prevent accessible_by mutating shared conditions ConditionsNormalizer replaces rule.conditions with a newly computed hash for each model class. When rules_compressor_enabled is false (the default fallback), @compressed_rules references the same Rule objects stored in the ability's shared @rules. Calling accessible_by for one model class permanently overwrites conditions on those shared Rule objects, so subsequent can? checks for other models using the same rule fail or raise NoMethodError. Fix by mapping @compressed_rules through Rule#dup before passing to the normalizers. The dup gives each adapter its own copy of the Rule shell so that conditions= writes are local and never touch the originals. Fixes #876 Signed-off-by: Sai Asish Y --- CHANGELOG.md | 4 ++ .../model_adapters/active_record_adapter.rb | 17 +++++-- .../conditions_normalizer_spec.rb | 48 ++++++++++++++++--- 3 files changed, 57 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc34c1981..e10ac627a 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 33766c0a4..39a03ed81 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 9368ae655..754ad8bdc 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