From b532c0e2aad5a3b6898e7a9cbf1e57ad0f97b935 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 d0bc8beccc0c..cd3fb56d0642 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 59ac07e5da4b..8b32942da410 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 1dca5272736f..dd65762cfc30 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.unscope(:select).find(id).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 c0415d99f9c4..e38e800fdbe4 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 a5adc27eb9a4..1cce85d11d19 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") + b = FactoryGirl.create(:miq_group, :features => %w(good everything)) + c = 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 f8058014c948..a65de463abcd 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") + b = FactoryGirl.create(:miq_user_role, :features => %w(good everything)) + c = 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 4a54ff83f9ff..d9f6f26b2462 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 unsopes" 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]) # good + u2 = FactoryGirl.create(:user, :miq_groups => [a1, a2]) # good + FactoryGirl.create(:user, :miq_groups => [a1, b]) # nope + FactoryGirl.create(:user, :miq_groups => [c]) # nope + + expect(User.with_roles_excluding("everything").select(:id, :name)).to match_array([u1, u2]) + end + end end