From 9a9b5ddc47ff8baae49ed1a7cba37732d609df11 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 30 Aug 2018 14:18:21 -0400 Subject: [PATCH] fix: with_role_excluding has bad subquery Rbac.filtered/search uses this method to filter by tenant admin Unfortunately, the subquery was using the select from the main query and producing bad queries It is now properly isolating the queries and tests have been added to protect against regressions https://bugzilla.redhat.com/show_bug.cgi?id=1623464 --- app/models/miq_group.rb | 2 +- app/models/miq_user_role.rb | 2 +- app/models/user.rb | 4 ++-- spec/lib/rbac/filterer_spec.rb | 6 +++--- spec/models/miq_group_spec.rb | 10 ++++++++++ spec/models/miq_user_role_spec.rb | 10 ++++++++++ spec/models/user_spec.rb | 24 ++++++++++++++++++++++++ 7 files changed, 51 insertions(+), 7 deletions(-) diff --git a/app/models/miq_group.rb b/app/models/miq_group.rb index d0bc8beccc0..cd3fb56d064 100644 --- a/app/models/miq_group.rb +++ b/app/models/miq_group.rb @@ -61,7 +61,7 @@ def settings=(new_settings) end def self.with_roles_excluding(identifier) - where.not(:id => MiqGroup.joins(:miq_product_features) + where.not(:id => MiqGroup.unscope(:select).joins(:miq_product_features) .where(:miq_product_features => {:identifier => identifier}) .select(:id)) end diff --git a/app/models/miq_user_role.rb b/app/models/miq_user_role.rb index 59ac07e5da4..8b32942da41 100644 --- a/app/models/miq_user_role.rb +++ b/app/models/miq_user_role.rb @@ -65,7 +65,7 @@ def limited_self_service? end def self.with_roles_excluding(identifier) - where.not(:id => MiqUserRole.joins(:miq_product_features) + where.not(:id => MiqUserRole.unscope(:select).joins(:miq_product_features) .where(:miq_product_features => {:identifier => identifier}) .select(:id)) end diff --git a/app/models/user.rb b/app/models/user.rb index 1dca5272736..f63791d1665 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -49,10 +49,10 @@ class User < ApplicationRecord serialize :settings, Hash # Implement settings column as a hash default_value_for(:settings) { Hash.new } - scope :with_same_userid, ->(id) { where(:userid => User.find(id).userid) } + scope :with_same_userid, ->(id) { where(:userid => User.where(:id => id).pluck(:userid)) } def self.with_roles_excluding(identifier) - where.not(:id => User.joins(:miq_groups => :miq_product_features) + where.not(:id => User.unscope(:select).joins(:miq_groups => :miq_product_features) .where(:miq_product_features => {:identifier => identifier}) .select(:id)) end diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb index c0415d99f9c..e38e800fdbe 100644 --- a/spec/lib/rbac/filterer_spec.rb +++ b/spec/lib/rbac/filterer_spec.rb @@ -888,7 +888,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt def get_rbac_results_for_and_expect_objects(klass, expected_objects) User.current_user = user - results = described_class.search(:class => klass).first + results = described_class.search(:targets => klass).first expect(results).to match_array(expected_objects) end @@ -1015,14 +1015,14 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) it 'can see all roles except for EvmRole-super_administrator' do expect(MiqUserRole.count).to eq(3) - get_rbac_results_for_and_expect_objects(MiqUserRole, [tenant_administrator_user_role, user_role]) + get_rbac_results_for_and_expect_objects(MiqUserRole.select(:id, :name), [tenant_administrator_user_role, user_role]) end it 'can see all groups except for group with role EvmRole-super_administrator' do expect(MiqUserRole.count).to eq(3) default_group_for_tenant = user.current_tenant.miq_groups.where(:group_type => "tenant").first super_admin_group - get_rbac_results_for_and_expect_objects(MiqGroup, [group, other_group, default_group_for_tenant]) + get_rbac_results_for_and_expect_objects(MiqGroup.select(:id, :description), [group, other_group, default_group_for_tenant]) end it 'can see all groups in the current tenant only' do diff --git a/spec/models/miq_group_spec.rb b/spec/models/miq_group_spec.rb index a5adc27eb9a..98f7a2b7a36 100644 --- a/spec/models/miq_group_spec.rb +++ b/spec/models/miq_group_spec.rb @@ -583,4 +583,14 @@ expect(virtual_column_sql_value(MiqGroup.where(:id => group.id), "miq_user_role_name")).to eq("EvmRole-the_role") end end + + describe ".with_roles_excluding" do + it "handles multiple columns" do + a = FactoryGirl.create(:miq_group, :features => "good") + FactoryGirl.create(:miq_group, :features => %w(good everything)) + FactoryGirl.create(:miq_group, :features => "everything") + + expect(MiqGroup.select(:id, :description).with_roles_excluding("everything")).to match_array([a]) + end + end end diff --git a/spec/models/miq_user_role_spec.rb b/spec/models/miq_user_role_spec.rb index f8058014c94..70a6cc83759 100644 --- a/spec/models/miq_user_role_spec.rb +++ b/spec/models/miq_user_role_spec.rb @@ -261,4 +261,14 @@ expect(role.group_count).to eq(2) end end + + describe ".with_roles_excluding" do + it "handles multiple columns" do + a = FactoryGirl.create(:miq_user_role, :features => "good") + FactoryGirl.create(:miq_user_role, :features => %w(good everything)) + FactoryGirl.create(:miq_user_role, :features => "everything") + + expect(MiqUserRole.select(:id, :name).with_roles_excluding("everything")).to match_array([a]) + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 4a54ff83f9f..607125ba13a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -585,4 +585,28 @@ expect(User.authorize_user("admin")).to be_nil end end + + describe ".with_same_userid" do + # this is testing the select does not break and in general, the scope works + it "properly handles select clause" do + u = FactoryGirl.create(:user) + expect(User.select(:id, :email).with_same_userid(u.id)).to eq([u]) + end + end + + describe ".with_roles_excluding" do + it "handles multiple columns" do + a1 = FactoryGirl.create(:miq_group, :features => "good") + a2 = FactoryGirl.create(:miq_group, :features => "something") + b = FactoryGirl.create(:miq_group, :features => %w(good everything)) + c = FactoryGirl.create(:miq_group, :features => "everything") + + u1 = FactoryGirl.create(:user, :miq_groups => [a1]) + u2 = FactoryGirl.create(:user, :miq_groups => [a1, a2]) + FactoryGirl.create(:user, :miq_groups => [a1, b]) + FactoryGirl.create(:user, :miq_groups => [c]) + + expect(User.with_roles_excluding("everything").select(:id, :name)).to match_array([u1, u2]) + end + end end