Merge pull request #13535 from rwsu/cloud_tenancy_mixin

Introduces CloudTenancyMixin to fix RBAC for cloud_tenant based models

app/models/cloud_volume.rb

@@ -6,6 +6,7 @@ class CloudVolume < ApplicationRecord
   include AsyncDeleteMixin
   include AvailabilityMixin
   include SupportsFeatureMixin
+  include CloudTenancyMixin
 
   belongs_to :ext_management_system, :foreign_key => :ems_id, :class_name => "ExtManagementSystem"
   belongs_to :availability_zone

app/models/manageiq/providers/openstack/cloud_manager.rb

@@ -66,6 +66,12 @@ def ensure_swift_manager
     true
   end
 
+  after_save :save_on_other_managers
+
+  def save_on_other_managers
+    storage_managers.update_all(:tenant_mapping_enabled => tenant_mapping_enabled)
+  end
+
   def supports_cloud_tenants?
     true
   end

app/models/mixins/cloud_tenancy_mixin.rb

@@ -0,0 +1,19 @@
+module CloudTenancyMixin
+  extend ActiveSupport::Concern
+
+  module ClassMethods
+    include TenancyCommonMixin
+
+    def scope_by_cloud_tenant?
+      true
+    end
+
+    def tenant_id_clause_format(tenant_ids)
+      ["(tenants.id IN (?) AND ext_management_systems.tenant_mapping_enabled IS TRUE) OR ext_management_systems.tenant_mapping_enabled IS FALSE OR ext_management_systems.tenant_mapping_enabled IS NULL", tenant_ids]
+    end
+
+    def tenant_joins_clause(scope)
+      scope.includes(:cloud_tenant => "source_tenant").includes(:ext_management_system)
+    end
+  end
+end

app/models/mixins/tenancy_common_mixin.rb

@@ -0,0 +1,19 @@
+module TenancyCommonMixin
+  def accessible_tenant_ids(user_or_group, strategy)
+    tenant = user_or_group.try(:current_tenant)
+    return [] if tenant.nil? || tenant.root?
+
+    tenant.accessible_tenant_ids(strategy)
+  end
+
+  def tenant_id_clause(user_or_group)
+    tenant_ids = accessible_tenant_ids(user_or_group, Rbac.accessible_tenant_ids_strategy(self))
+    return if tenant_ids.empty?
+
+    tenant_id_clause_format(tenant_ids)
+  end
+
+  def tenant_id_clause_format(tenant_ids)
+    {table_name => {:tenant_id => tenant_ids}}
+  end
+end

app/models/mixins/tenancy_mixin.rb

@@ -6,23 +6,11 @@ module TenancyMixin
   end
 
   module ClassMethods
+    include TenancyCommonMixin
+
     def scope_by_tenant?
       true
     end
-
-    def accessible_tenant_ids(user_or_group, strategy)
-      tenant = user_or_group.try(:current_tenant)
-      return [] if tenant.nil? || tenant.root?
-
-      tenant.accessible_tenant_ids(strategy)
-    end
-
-    def tenant_id_clause(user_or_group)
-      tenant_ids = accessible_tenant_ids(user_or_group, Rbac.accessible_tenant_ids_strategy(self))
-      return if tenant_ids.empty?
-
-      {table_name => {:tenant_id => tenant_ids}}
-    end
   end
 
   def set_tenant

lib/rbac/filterer.rb

@@ -427,10 +427,16 @@ def scope_to_tenant(scope, user, miq_group)
       klass = scope.respond_to?(:klass) ? scope.klass : scope
       user_or_group = user || miq_group
       tenant_id_clause = klass.tenant_id_clause(user_or_group)
-
       tenant_id_clause ? scope.where(tenant_id_clause) : scope
     end
 
+    def scope_to_cloud_tenant(scope, user, miq_group)
+      klass = scope.respond_to?(:klass) ? scope.klass : scope
+      user_or_group = user || miq_group
+      tenant_id_clause = klass.tenant_id_clause(user_or_group)
+      klass.tenant_joins_clause(scope).where(tenant_id_clause)
+    end
+
     ##
     # Main scoping method
     #
@@ -440,6 +446,8 @@ def scope_targets(klass, scope, rbac_filters, user, miq_group)
       # TENANT_ACCESS_STRATEGY are a consolidated list of them.
       if klass.respond_to?(:scope_by_tenant?) && klass.scope_by_tenant?
         scope = scope_to_tenant(scope, user, miq_group)
+      elsif klass.respond_to?(:scope_by_cloud_tenant?) && klass.scope_by_cloud_tenant?
+        scope = scope_to_cloud_tenant(scope, user, miq_group)
       end
 
       if apply_rbac_directly?(klass)

spec/lib/rbac/filterer_spec.rb

@@ -1390,6 +1390,48 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
     end
   end
 
+  describe "cloud_tenant based search" do
+    let(:ems_openstack)         { FactoryGirl.create(:ems_cloud) }
+    let(:project1_tenant)       { FactoryGirl.create(:tenant, :source_type => 'CloudTenant') }
+    let(:project1_cloud_tenant) { FactoryGirl.create(:cloud_tenant, :source_tenant => project1_tenant) }
+    let(:project1_group)        { FactoryGirl.create(:miq_group, :tenant => project1_tenant) }
+    let(:project1_user)         { FactoryGirl.create(:user, :miq_groups => [project1_group]) }
+    let(:project1_volume)       { FactoryGirl.create(:cloud_volume, :ext_management_system => ems_openstack, :cloud_tenant => project1_cloud_tenant) }
+    let(:project2_tenant)       { FactoryGirl.create(:tenant, :source_type => 'CloudTenant') }
+    let(:project2_cloud_tenant) { FactoryGirl.create(:cloud_tenant, :source_tenant => project2_tenant) }
+    let(:project2_group)        { FactoryGirl.create(:miq_group, :tenant => project2_tenant) }
+    let(:project2_user)         { FactoryGirl.create(:user, :miq_groups => [project2_group]) }
+    let(:project2_volume)       { FactoryGirl.create(:cloud_volume, :ext_management_system => ems_openstack, :cloud_tenant => project2_cloud_tenant) }
+    let(:ems_other)             { FactoryGirl.create(:ems_cloud, :name => 'ems_other', :tenant_mapping_enabled => false) }
+    let(:volume_other)          { FactoryGirl.create(:cloud_volume, :ext_management_system => ems_other) }
+    let!(:all_volumes)          { [project1_volume, project2_volume, volume_other] }
+
+    it "lists its own cloud volumes and other volumes where tenant_mapping is not enabled" do
+      ems_openstack.tenant_mapping_enabled = true
+      ems_openstack.save!
+      results = described_class.search(:class => CloudVolume, :user => project1_user).first
+      expect(results).to match_array [project1_volume, volume_other]
+
+      results = described_class.search(:class => CloudVolume, :user => project2_user).first
+      expect(results).to match_array [project2_volume, volume_other]
+
+      results = described_class.search(:class => CloudVolume, :user => owner_user).first
+      expect(results).to match_array [volume_other]
+    end
+
+    it "all cloud volumes are visible to all users when tenant_mapping is not enabled" do
+      ems_openstack.tenant_mapping_enabled = false
+      ems_openstack.save!
+      results = described_class.search(:class => CloudVolume, :user => project1_user).first
+      expect(results).to match_array [project1_volume, project2_volume, volume_other]
+
+      results = described_class.search(:class => CloudVolume, :user => project2_user).first
+      expect(results).to match_array [project1_volume, project2_volume, volume_other]
+
+      results = described_class.search(:class => CloudVolume, :user => owner_user).first
+      expect(results).to match_array [project1_volume, project2_volume, volume_other]
+    end
+  end
 
   private
 

spec/models/mixins/cloud_tenancy_mixin_spec.rb

@@ -0,0 +1,25 @@
+describe CloudTenancyMixin do
+  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) }
+
+    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]]
+    end
+
+    it "finds correct tenant id clause for cloud tenants" do
+      expect(CloudVolume.tenant_id_clause(user)).to eql ["(tenants.id IN (?) AND ext_management_systems.tenant_mapping_enabled IS TRUE) OR ext_management_systems.tenant_mapping_enabled IS FALSE OR ext_management_systems.tenant_mapping_enabled IS NULL", [tenant.id]]
+    end
+  end
+end