Merge pull request #18128 from alexander-demichev/fix-rbac-for-templates

Fix RBAC call for templates and vms

(cherry picked from commit cd982a4)

https://bugzilla.redhat.com/show_bug.cgi?id=1524368

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

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

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"

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

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

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

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