Introduce a conditions normalizer

Author: coorasse

lib/cancan.rb

@@ -13,6 +13,7 @@
 
 if defined? ActiveRecord
   require 'cancan/model_adapters/conditions_extractor'
+  require 'cancan/model_adapters/conditions_normalizer'
   require 'cancan/model_adapters/active_record_adapter'
   require 'cancan/model_adapters/active_record_4_adapter'
   require 'cancan/model_adapters/active_record_5_adapter'

lib/cancan/model_adapters/active_record_adapter.rb

@@ -14,6 +14,7 @@ def self.version_lower?(version)
       def initialize(model_class, rules)
         super
         @compressed_rules = RulesCompressor.new(@rules.reverse).rules_collapsed.reverse
+        ConditionsNormalizer.normalize(model_class, @compressed_rules)
       end
 
       # Returns conditions intended to be used inside a database query. Normally you will not call this

lib/cancan/model_adapters/conditions_extractor.rb

@@ -27,8 +27,6 @@ def tableize_conditions(conditions, model_class = @root_model_class, path_to_key
 
       def calculate_result_hash(key, model_class, path_to_key, result_hash, value)
         reflection = model_class.reflect_on_association(key)
-        raise WrongAssociationName, "association #{key} not defined in model #{model_class.name}" unless reflection
-
         nested_resulted = calculate_nested(model_class, result_hash, key, value.dup, path_to_key)
         association_class = reflection.klass.name.constantize
         tableize_conditions(nested_resulted, association_class, "#{path_to_key}_#{key}")

lib/cancan/model_adapters/conditions_normalizer.rb

@@ -0,0 +1,45 @@
+# this class is responsible of normalizing the hash of conditions
+# by exploding has_many through associations
+# when a condition is defined with an has_many thorugh association this is exploded in all its parts
+# TODO: it could identify STI and normalize it
+module CanCan
+  module ModelAdapters
+    class ConditionsNormalizer
+      class << self
+        def normalize(model_class, rules)
+          rules.each { |rule| rule.conditions = normalize_conditions(model_class, rule.conditions) }
+        end
+
+        def normalize_conditions(model_class, conditions)
+          return conditions unless conditions.is_a? Hash
+
+          conditions.each_with_object({}) do |(key, value), result_hash|
+            if value.is_a? Hash
+              result_hash.merge!(calculate_result_hash(model_class, key, value))
+            else
+              result_hash[key] = value
+            end
+            result_hash
+          end
+        end
+
+        private
+
+        def calculate_result_hash(model_class, key, value)
+          reflection = model_class.reflect_on_association(key)
+          unless reflection
+            raise WrongAssociationName, "Association '#{key}' not defined in model '#{model_class.name}'"
+          end
+
+          if reflection.options[:through].present?
+            key = reflection.options[:through]
+            value = { reflection.source_reflection_name => value }
+            reflection = model_class.reflect_on_association(key)
+          end
+
+          { key => normalize_conditions(reflection.klass.name.constantize, value) }
+        end
+      end
+    end
+  end
+end

lib/cancan/rule.rb

@@ -7,7 +7,7 @@ class Rule # :nodoc:
     include ConditionsMatcher
     include ParameterValidators
     attr_reader :base_behavior, :subjects, :actions, :conditions, :attributes
-    attr_writer :expanded_actions
+    attr_writer :expanded_actions, :conditions
 
     # The first argument when initializing is the base_behavior which is a true/false
     # value. True for "can" and false for "cannot". The next two arguments are the action

spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb

@@ -0,0 +1,98 @@
+require 'spec_helper'
+
+# integration tests for latest ActiveRecord version.
+RSpec.describe CanCan::ModelAdapters::ActiveRecord5Adapter do
+  let(:ability) { double.extend(CanCan::Ability) }
+  let(:users_table) { Post.table_name }
+  let(:posts_table) { Post.table_name }
+  let(:likes_table) { Like.table_name }
+  before :each do
+    connect_db
+    ActiveRecord::Migration.verbose = false
+
+    ActiveRecord::Schema.define do
+      create_table(:users) do |t|
+        t.string :name
+        t.timestamps null: false
+      end
+
+      create_table(:posts) do |t|
+        t.string :title
+        t.boolean :published, default: true
+        t.integer :user_id
+        t.timestamps null: false
+      end
+
+      create_table(:likes) do |t|
+        t.integer :post_id
+        t.integer :user_id
+        t.timestamps null: false
+      end
+
+      create_table(:editors) do |t|
+        t.integer :post_id
+        t.integer :user_id
+        t.timestamps null: false
+      end
+    end
+
+    class User < ActiveRecord::Base
+      has_many :posts
+      has_many :likes
+      has_many :editors
+    end
+
+    class Post < ActiveRecord::Base
+      belongs_to :user
+      has_many :likes
+      has_many :editors
+    end
+
+    class Like < ActiveRecord::Base
+      belongs_to :user
+      belongs_to :post
+    end
+
+    class Editor < ActiveRecord::Base
+      belongs_to :user
+      belongs_to :post
+    end
+  end
+
+  before do
+    @user1 = User.create!
+    @user2 = User.create!
+    @post1 = Post.create!(title: 'post1', user: @user1)
+    @post2 = Post.create!(user: @user1, published: false)
+    @post3 = Post.create!(user: @user2)
+    @like1 = Like.create!(post: @post1, user: @user1)
+    @like2 = Like.create!(post: @post1, user: @user2)
+    @editor1 = Editor.create(user: @user1, post: @post2)
+    ability.can :read, Post, user_id: @user1
+    ability.can :read, Post, editors: { user_id: @user1 }
+  end
+
+  describe 'preloading of associatons' do
+    it 'preloads associations correctly' do
+      posts = Post.accessible_by(ability).includes(likes: :user)
+      expect(posts[0].association(:likes)).to be_loaded
+      expect(posts[0].likes[0].association(:user)).to be_loaded
+    end
+  end
+
+  describe 'filtering of results' do
+    it 'adds the where clause correctly' do
+      posts = Post.accessible_by(ability).where(published: true)
+      expect(posts.length).to eq 1
+    end
+  end
+
+  if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0')
+    describe 'selecting custom columns' do
+      it 'extracts custom columns correctly' do
+        posts = Post.accessible_by(ability).select('title as mytitle')
+        expect(posts[0].mytitle).to eq 'post1'
+      end
+    end
+  end
+end

spec/cancan/model_adapters/active_record_adapter_spec.rb

@@ -25,19 +25,25 @@
         t.timestamps null: false
       end
 
+      create_table(:companies) do |t|
+        t.boolean :admin
+      end
+
       create_table(:articles) do |t|
         t.string :name
         t.timestamps null: false
         t.boolean :published
         t.boolean :secret
         t.integer :priority
         t.integer :category_id
+        t.integer :project_id
         t.integer :user_id
       end
 
       create_table(:comments) do |t|
         t.boolean :spam
         t.integer :article_id
+        t.integer :project_id
         t.timestamps null: false
       end
 
@@ -54,18 +60,24 @@
     end
 
     class Project < ActiveRecord::Base
+      has_many :comments
     end
 
     class Category < ActiveRecord::Base
       has_many :articles
     end
 
+    class Company < ActiveRecord::Base
+    end
+
     class Article < ActiveRecord::Base
       belongs_to :category
+      belongs_to :company
       has_many :comments
       has_many :mentions
       has_many :mentioned_users, through: :mentions, source: :user
       belongs_to :user
+      belongs_to :project
     end
 
     class Mention < ActiveRecord::Base
@@ -502,4 +514,27 @@ class Transaction < ActiveRecord::Base
       expect(Article.accessible_by(ability)).to match_array([a1])
     end
   end
+
+  context 'has_many through is defined and referenced differently' do
+    it 'recognises it and simplifies the query' do
+      u1 = User.create!(name: 'pippo')
+      u2 = User.create!(name: 'paperino')
+
+      a1 = Article.create!(mentioned_users: [u1])
+      a2 = Article.create!(mentioned_users: [u2])
+
+      ability = Ability.new(u1)
+      ability.can :read, Article, mentioned_users: { name: u1.name }
+      ability.can :read, Article, mentions: { user: { name: u2.name } }
+      expect(Article.accessible_by(ability)).to match_array([a1, a2])
+      if CanCan::ModelAdapters::ActiveRecordAdapter.version_greater_or_equal?('5.0.0')
+        expect(ability.model_adapter(Article, :read)).to generate_sql(%(
+  SELECT DISTINCT "articles".*
+  FROM "articles"
+  LEFT OUTER JOIN "legacy_mentions" ON "legacy_mentions"."article_id" = "articles"."id"
+  LEFT OUTER JOIN "users" ON "users"."id" = "legacy_mentions"."user_id"
+  WHERE (("users"."name" = 'paperino') OR ("users"."name" = 'pippo'))))
+      end
+    end
+  end
 end

spec/cancan/model_adapters/conditions_normalizer_spec.rb

@@ -0,0 +1,83 @@
+require 'spec_helper'
+
+RSpec.describe CanCan::ModelAdapters::ConditionsNormalizer do
+  before do
+    connect_db
+    ActiveRecord::Migration.verbose = false
+    ActiveRecord::Schema.define do
+      create_table(:articles) do |t|
+      end
+
+      create_table(:users) do |t|
+        t.string :name
+      end
+
+      create_table(:comments) do |t|
+      end
+
+      create_table(:spread_comments) do |t|
+        t.integer :article_id
+        t.integer :comment_id
+      end
+
+      create_table(:legacy_mentions) do |t|
+        t.integer :user_id
+        t.integer :article_id
+      end
+    end
+
+    class Article < ActiveRecord::Base
+      has_many :spread_comments
+      has_many :comments, through: :spread_comments
+      has_many :mentions
+      has_many :mentioned_users, through: :mentions, source: :user
+    end
+
+    class Comment < ActiveRecord::Base
+      has_many :spread_comments
+      has_many :articles, through: :spread_comments
+    end
+
+    class SpreadComment < ActiveRecord::Base
+      belongs_to :comment
+      belongs_to :article
+    end
+
+    class Mention < ActiveRecord::Base
+      self.table_name = 'legacy_mentions'
+      belongs_to :article
+      belongs_to :user
+    end
+
+    class User < ActiveRecord::Base
+      has_many :mentions
+      has_many :mentioned_articles, through: :mentions, source: :article
+    end
+  end
+
+  it 'simplifies has_many through associations' do
+    rule = CanCan::Rule.new(true, :read, Comment, articles: { mentioned_users: { name: 'pippo' } })
+    CanCan::ModelAdapters::ConditionsNormalizer.normalize(Comment, [rule])
+    expect(rule.conditions).to eq(spread_comments: { article: { mentions: { user: { name: 'pippo' } } } })
+  end
+
+  it 'normalizes the has_one through associations' do
+    class Supplier < ActiveRecord::Base
+      has_one :accountant
+      has_one :account_history, through: :accountant
+    end
+
+    class Accountant < ActiveRecord::Base
+      belongs_to :supplier
+      has_one :account_history
+    end
+
+    class AccountHistory < ActiveRecord::Base
+      belongs_to :accountant
+    end
+
+    rule = CanCan::Rule.new(true, :read, Supplier, account_history: { name: 'pippo' })
+    CanCan::ModelAdapters::ConditionsNormalizer.normalize(Supplier, [rule])
+    expect(rule.conditions).to eq(accountant: { account_history: { name: 'pippo' } })
+  end
+end