diff --git a/lib/cancan.rb b/lib/cancan.rb index 86d419c4..8b77b767 100644 --- a/lib/cancan.rb +++ b/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' diff --git a/lib/cancan/model_adapters/active_record_adapter.rb b/lib/cancan/model_adapters/active_record_adapter.rb index fd7aefae..16737e37 100644 --- a/lib/cancan/model_adapters/active_record_adapter.rb +++ b/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 diff --git a/lib/cancan/model_adapters/conditions_extractor.rb b/lib/cancan/model_adapters/conditions_extractor.rb index bfe8e872..e0662937 100644 --- a/lib/cancan/model_adapters/conditions_extractor.rb +++ b/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}") diff --git a/lib/cancan/model_adapters/conditions_normalizer.rb b/lib/cancan/model_adapters/conditions_normalizer.rb new file mode 100644 index 00000000..46ceab71 --- /dev/null +++ b/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 diff --git a/lib/cancan/rule.rb b/lib/cancan/rule.rb index a6780615..82fe916f 100644 --- a/lib/cancan/rule.rb +++ b/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 diff --git a/spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb b/spec/cancan/model_adapters/accessible_by_has_many_through_spec.rb new file mode 100644 index 00000000..28e2d836 --- /dev/null +++ b/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 diff --git a/spec/cancan/model_adapters/active_record_adapter_spec.rb b/spec/cancan/model_adapters/active_record_adapter_spec.rb index 8a090aee..809e8c29 100644 --- a/spec/cancan/model_adapters/active_record_adapter_spec.rb +++ b/spec/cancan/model_adapters/active_record_adapter_spec.rb @@ -25,6 +25,10 @@ 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 @@ -32,12 +36,14 @@ 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 diff --git a/spec/cancan/model_adapters/conditions_normalizer_spec.rb b/spec/cancan/model_adapters/conditions_normalizer_spec.rb new file mode 100644 index 00000000..fdfb7f52 --- /dev/null +++ b/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