From 297b4f701e85b8431e5175273dbc4a9e3b676ce6 Mon Sep 17 00:00:00 2001 From: Gregg Tanzillo Date: Tue, 18 Dec 2018 11:58:25 -0500 Subject: [PATCH] Merge pull request #18128 from alexander-demichev/fix-rbac-for-templates Fix RBAC call for templates and vms (cherry picked from commit cd982a488b48a189e4aabf2f1a21ae7007dda5fb) https://bugzilla.redhat.com/show_bug.cgi?id=1524368 --- .../providers/cloud_manager/template.rb | 11 --- app/models/vm_or_template.rb | 16 +++- spec/factories/vm_or_template.rb | 4 + spec/lib/rbac/filterer_spec.rb | 74 +++++++++++++++---- .../providers/cloud_manager/template_spec.rb | 28 ------- .../cloud_manager/vm_or_template_spec.rb | 28 +++++++ .../models/mixins/cloud_tenancy_mixin_spec.rb | 2 +- 7 files changed, 105 insertions(+), 58 deletions(-) diff --git a/app/models/manageiq/providers/cloud_manager/template.rb b/app/models/manageiq/providers/cloud_manager/template.rb index 3af0dee3f06..8a06ea98f21 100644 --- a/app/models/manageiq/providers/cloud_manager/template.rb +++ b/app/models/manageiq/providers/cloud_manager/template.rb @@ -122,17 +122,6 @@ def self.display_name(number = 1) n_('Image', 'Images', number) end - def self.tenant_id_clause(user_or_group) - template_tenant_ids = MiqTemplate.accessible_tenant_ids(user_or_group, Rbac.accessible_tenant_ids_strategy(self)) - tenant = user_or_group.current_tenant - - if tenant.source_id - ["(vms.template = true AND (vms.tenant_id = (?) AND vms.publicly_available = false OR vms.publicly_available = true))", tenant.id] - else - ["(vms.template = true AND (vms.tenant_id IN (?) OR vms.publicly_available = true))", template_tenant_ids] unless template_tenant_ids.empty? - end - end - private def raise_created_event diff --git a/app/models/vm_or_template.rb b/app/models/vm_or_template.rb index dadf946190e..eb01154ca1b 100644 --- a/app/models/vm_or_template.rb +++ b/app/models/vm_or_template.rb @@ -1747,13 +1747,25 @@ def self.reconfigurable?(ids) vms.all?(&:reconfigurable?) end + PUBLIC_TEMPLATE_CLASSES = %w(ManageIQ::Providers::Openstack::CloudManager::Template).freeze + def self.tenant_id_clause(user_or_group) template_tenant_ids = MiqTemplate.accessible_tenant_ids(user_or_group, Rbac.accessible_tenant_ids_strategy(MiqTemplate)) vm_tenant_ids = Vm.accessible_tenant_ids(user_or_group, Rbac.accessible_tenant_ids_strategy(Vm)) return if template_tenant_ids.empty? && vm_tenant_ids.empty? - ["(vms.template = true AND vms.tenant_id IN (?)) OR (vms.template = false AND vms.tenant_id IN (?))", - template_tenant_ids, vm_tenant_ids] + tenant = user_or_group.current_tenant + tenant_vms = "vms.template = false AND vms.tenant_id IN (?)" + public_templates = "vms.template = true AND vms.publicly_available = true AND vms.type IN (?)" + tenant_templates = "vms.template = true AND vms.tenant_id IN (?)" + + if tenant.source_id + private_tenant_templates = "vms.template = true AND vms.tenant_id = (?) AND vms.publicly_available = false" + tenant_templates += " AND vms.type NOT IN (?)" + ["#{private_tenant_templates} OR #{tenant_vms} OR #{tenant_templates} OR #{public_templates}", tenant.id, vm_tenant_ids, template_tenant_ids, PUBLIC_TEMPLATE_CLASSES, PUBLIC_TEMPLATE_CLASSES] + else + ["#{tenant_templates} OR #{public_templates} OR #{tenant_vms}", template_tenant_ids, PUBLIC_TEMPLATE_CLASSES, vm_tenant_ids] + end end def self.with_ownership diff --git a/spec/factories/vm_or_template.rb b/spec/factories/vm_or_template.rb index 502f3c2f7e0..bb3f6b196d3 100644 --- a/spec/factories/vm_or_template.rb +++ b/spec/factories/vm_or_template.rb @@ -31,6 +31,10 @@ vendor "openstack" end + factory :volume_template_openstack, :class => "ManageIQ::Providers::Openstack::CloudManager::VolumeTemplate", :parent => :template_cloud do + vendor "openstack" + end + factory :miq_template do name "ubuntu-16.04-stable" location "Minneapolis, MN" diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb index 69bbf4bd134..b6db467af25 100644 --- a/spec/lib/rbac/filterer_spec.rb +++ b/spec/lib/rbac/filterer_spec.rb @@ -799,7 +799,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt context "searching CloudTemplate" do let(:group) { FactoryGirl.create(:miq_group, :tenant => default_tenant) } # T1 let(:admin_user) { FactoryGirl.create(:user, :role => "super_administrator") } - let!(:cloud_template_root) { FactoryGirl.create(:template_cloud, :publicly_available => false) } + let!(:cloud_template_root) { FactoryBot.create(:template_openstack, :publicly_available => false) } let(:tenant_2) { FactoryGirl.create(:tenant, :parent => default_tenant, :source_type => 'CloudTenant') } # T2 let(:group_2) { FactoryGirl.create(:miq_group, :tenant => tenant_2) } # T1 @@ -814,20 +814,26 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt context "when user is restricted user" do let(:tenant_3) { FactoryGirl.create(:tenant, :parent => tenant_2) } # T3 - let!(:cloud_template) { FactoryGirl.create(:template_cloud, :tenant => tenant_3, :publicly_available => true) } + let!(:cloud_template) { FactoryBot.create(:template_openstack, :tenant => tenant_3, :publicly_available => true) } + let!(:volume_template_openstack_1) { FactoryBot.create(:template_openstack, :tenant => tenant_3, :publicly_available => true) } - it "returns all public cloud templates" do + it "returns all public cloud templates and its descendants" do User.current_user = user_2 + + results = described_class.filtered(VmOrTemplate, :user => user_2) + expect(results).to match_array([cloud_template, cloud_template_root, volume_template_openstack_1]) + results = described_class.filtered(TemplateCloud, :user => user_2) - expect(results).to match_array([cloud_template, cloud_template_root]) + expect(results).to match_array([cloud_template, cloud_template_root, volume_template_openstack_1]) end context "should ignore other tenant's private cloud templates" do - let!(:cloud_template) { FactoryGirl.create(:template_cloud, :tenant => tenant_3, :publicly_available => false) } + let!(:cloud_template) { FactoryBot.create(:template_openstack, :tenant => tenant_3, :publicly_available => false) } + let!(:volume_template_openstack_2) { FactoryBot.create(:template_openstack, :tenant => tenant_3, :publicly_available => false) } it "returns public templates" do User.current_user = user_2 results = described_class.filtered(TemplateCloud, :user => user_2) - expect(results).to match_array([cloud_template_root]) + expect(results).to match_array([cloud_template_root, volume_template_openstack_1]) end end end @@ -837,34 +843,34 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt let(:tenant_2) { FactoryGirl.create(:tenant, :parent => default_tenant, :source_type => 'CloudTenant', :source_id => 1) } it "finds tenant's private cloud templates" do - cloud_template2 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => false) + cloud_template2 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => false) User.current_user = user_2 results = described_class.filtered(TemplateCloud, :user => user_2) expect(results).to match_array([cloud_template2]) end it "finds tenant's private and public cloud templates" do - cloud_template2 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => false) - cloud_template3 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => true) + cloud_template2 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => false) + cloud_template3 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => true) User.current_user = user_2 results = described_class.filtered(TemplateCloud, :user => user_2) expect(results).to match_array([cloud_template2, cloud_template3]) end it "ignores other tenant's private templates" do - cloud_template2 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => false) - cloud_template3 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => true) - FactoryGirl.create(:template_cloud, :tenant => default_tenant, :publicly_available => false) + cloud_template2 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => false) + cloud_template3 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => true) + FactoryBot.create(:template_openstack, :tenant => default_tenant, :publicly_available => false) User.current_user = user_2 results = described_class.filtered(TemplateCloud, :user => user_2) expect(results).to match_array([cloud_template2, cloud_template3]) end it "finds other tenant's public templates" do - cloud_template2 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => false) - cloud_template3 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => true) - cloud_template4 = FactoryGirl.create(:template_cloud, :tenant => default_tenant, :publicly_available => true) - FactoryGirl.create(:template_cloud, :tenant => default_tenant, :publicly_available => false) + cloud_template2 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => false) + cloud_template3 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => true) + cloud_template4 = FactoryBot.create(:template_openstack, :tenant => default_tenant, :publicly_available => true) + FactoryBot.create(:template_openstack, :tenant => default_tenant, :publicly_available => false) User.current_user = user_2 results = described_class.filtered(TemplateCloud, :user => user_2) expect(results).to match_array([cloud_template2, cloud_template3, cloud_template4]) @@ -2288,6 +2294,42 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) end end + describe "using right RBAC rules to VM and Templates" do + let(:ems_openstack) { FactoryBot.create(:ems_cloud) } + let(:tenant_1) { FactoryBot.create(:tenant, :source => project1_cloud_tenant) } + let(:project1_cloud_tenant) { FactoryBot.create(:cloud_tenant, :ext_management_system => ems_openstack) } + + let(:tenant_2) { FactoryBot.create(:tenant, :source => project2_cloud_tenant, :parent => tenant_1) } + let(:project2_cloud_tenant) { FactoryBot.create(:cloud_tenant, :ext_management_system => ems_openstack) } + + let(:project2_group) { FactoryBot.create(:miq_group, :tenant => tenant_2) } + let(:user_2) { FactoryBot.create(:user, :miq_groups => [project2_group]) } + + let(:tenant_2_without_mapping) { FactoryBot.create(:tenant, :parent => tenant_1) } + let(:project2_group_without_mapping) { FactoryBot.create(:miq_group, :tenant => tenant_2_without_mapping) } + let(:user_2_without_mapping) { FactoryBot.create(:user, :miq_groups => [project2_group_without_mapping]) } + + let(:tenant_3) { FactoryBot.create(:tenant, :source => project3_cloud_tenant, :parent => tenant_2) } + let(:project3_cloud_tenant) { FactoryBot.create(:cloud_tenant, :ext_management_system => ems_openstack) } + + let(:ems_infra) { FactoryBot.create(:ems_vmware) } + let!(:vm_tenant_1) { FactoryBot.create(:vm_infra, :ext_management_system => ems_infra, :tenant => tenant_1) } + let!(:vm_tenant_2) { FactoryBot.create(:vm_infra, :ext_management_system => ems_infra, :tenant => tenant_2) } + let!(:vm_tenant_3) { FactoryBot.create(:vm_infra, :ext_management_system => ems_infra, :tenant => tenant_3) } + + let!(:template_tenant_1) { FactoryBot.create(:template_infra, :ext_management_system => ems_infra, :tenant => tenant_1) } + let!(:template_tenant_2) { FactoryBot.create(:template_infra, :ext_management_system => ems_infra, :tenant => tenant_2) } + let!(:template_tenant_3) { FactoryBot.create(:template_infra, :ext_management_system => ems_infra, :tenant => tenant_3) } + + it "finds records according to RBAC" do + results = described_class.filtered(Vm, :user => user_2) + expect(results.ids).to match_array([vm_tenant_2.id, vm_tenant_3.id]) + + results = described_class.filtered(MiqTemplate, :user => user_2) + expect(results.ids).to match_array([template_tenant_1.id, template_tenant_2.id]) + end + end + it ".apply_rbac_directly?" do expect(described_class.new.send(:apply_rbac_directly?, Vm)).to be_truthy expect(described_class.new.send(:apply_rbac_directly?, Rbac)).not_to be diff --git a/spec/models/manageiq/providers/cloud_manager/template_spec.rb b/spec/models/manageiq/providers/cloud_manager/template_spec.rb index 414c254e55c..ecdc5a0818f 100644 --- a/spec/models/manageiq/providers/cloud_manager/template_spec.rb +++ b/spec/models/manageiq/providers/cloud_manager/template_spec.rb @@ -8,32 +8,4 @@ subject.post_create_actions end end - - let(:root_tenant) do - Tenant.seed - end - - let(:default_tenant) do - root_tenant - Tenant.default_tenant - end - - describe "miq_group" do - let(:user) { FactoryGirl.create(:user, :userid => 'user', :miq_groups => [tenant_group]) } - let(:tenant) { FactoryGirl.build(:tenant, :parent => default_tenant) } - let(:tenant_users) { FactoryGirl.create(:miq_user_role, :name => "tenant-users") } - let(:tenant_group) { FactoryGirl.create(:miq_group, :miq_user_role => tenant_users, :tenant => tenant) } - let(:cloud_template_1) { FactoryGirl.create(:class => "TemplateCloud") } - - it "finds correct tenant id clause when tenant doesn't have source_id" do - User.current_user = user - expect(TemplateCloud.tenant_id_clause(user)).to eql ["(vms.template = true AND (vms.tenant_id IN (?) OR vms.publicly_available = true))", [default_tenant.id, tenant.id]] - end - - it "finds correct tenant id clause when tenant has source_id" do - User.current_user = user - tenant.source_id = 1 - expect(TemplateCloud.tenant_id_clause(user)).to eql ["(vms.template = true AND (vms.tenant_id = (?) AND vms.publicly_available = false OR vms.publicly_available = true))", tenant.id] - end - end end diff --git a/spec/models/manageiq/providers/cloud_manager/vm_or_template_spec.rb b/spec/models/manageiq/providers/cloud_manager/vm_or_template_spec.rb index 7a681b702d3..5b4239761e4 100644 --- a/spec/models/manageiq/providers/cloud_manager/vm_or_template_spec.rb +++ b/spec/models/manageiq/providers/cloud_manager/vm_or_template_spec.rb @@ -25,4 +25,32 @@ expect(described_class.archived).to match_array([vm, t]) end end + + let(:root_tenant) do + Tenant.seed + end + + let(:default_tenant) do + root_tenant + Tenant.default_tenant + end + + describe "miq_group" do + let(:user) { FactoryGirl.create(:user, :userid => 'user', :miq_groups => [tenant_group]) } + let(:tenant) { FactoryGirl.build(:tenant, :parent => default_tenant) } + let(:tenant_users) { FactoryGirl.create(:miq_user_role, :name => "tenant-users") } + let(:tenant_group) { FactoryGirl.create(:miq_group, :miq_user_role => tenant_users, :tenant => tenant) } + let(:cloud_template_1) { FactoryGirl.create(:class => "TemplateCloud") } + + it "finds correct tenant id clause when tenant has source_id" do + User.current_user = user + tenant.source_id = 1 + expect(VmOrTemplate.tenant_id_clause(user)).to eql ["vms.template = true AND vms.tenant_id = (?) AND vms.publicly_available = false OR vms.template = false AND vms.tenant_id IN (?) OR vms.template = true AND vms.tenant_id IN (?) AND vms.type NOT IN (?) OR vms.template = true AND vms.publicly_available = true AND vms.type IN (?)", tenant.id, [tenant.id], [root_tenant.id, tenant.id], ["ManageIQ::Providers::Openstack::CloudManager::Template"], ["ManageIQ::Providers::Openstack::CloudManager::Template"]] + end + + it "finds correct tenant id clause when tenant doesn't have source_id" do + User.current_user = user + expect(VmOrTemplate.tenant_id_clause(user)).to eql ["vms.template = true AND vms.tenant_id IN (?) OR vms.template = true AND vms.publicly_available = true AND vms.type IN (?) OR vms.template = false AND vms.tenant_id IN (?)", [root_tenant.id, tenant.id], ["ManageIQ::Providers::Openstack::CloudManager::Template"], [tenant.id]] + end + end end diff --git a/spec/models/mixins/cloud_tenancy_mixin_spec.rb b/spec/models/mixins/cloud_tenancy_mixin_spec.rb index 9026ffe411d..35379c67822 100644 --- a/spec/models/mixins/cloud_tenancy_mixin_spec.rb +++ b/spec/models/mixins/cloud_tenancy_mixin_spec.rb @@ -15,7 +15,7 @@ let(:tenant_group) { FactoryGirl.create(:miq_group, :miq_user_role => tenant_users, :tenant => tenant) } it "finds correct tenant id clause for regular tenants" do - expect(VmOrTemplate.tenant_id_clause(user)).to eql ["(vms.template = true AND vms.tenant_id IN (?)) OR (vms.template = false AND vms.tenant_id IN (?))", [default_tenant.id, tenant.id], [tenant.id]] + expect(VmOrTemplate.tenant_id_clause(user)).to eql ["vms.template = true AND vms.tenant_id IN (?) OR vms.template = true AND vms.publicly_available = true AND vms.type IN (?) OR vms.template = false AND vms.tenant_id IN (?)", [default_tenant.id, tenant.id], ["ManageIQ::Providers::Openstack::CloudManager::Template"], [tenant.id]] query = VmOrTemplate.where(VmOrTemplate.tenant_id_clause(user)) expect { query.load }.not_to raise_error