Skip to content

Commit

Permalink
fix: with_role_excluding has bad subquery
Browse files Browse the repository at this point in the history
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
  • Loading branch information
kbrock committed Sep 5, 2018
1 parent b23230d commit b532c0e
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 7 deletions.
2 changes: 1 addition & 1 deletion app/models/miq_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/models/miq_user_role.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions spec/lib/rbac/filterer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
10 changes: 10 additions & 0 deletions spec/models/miq_group_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
10 changes: 10 additions & 0 deletions spec/models/miq_user_role_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
24 changes: 24 additions & 0 deletions spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit b532c0e

Please sign in to comment.