From 87cafbdfbec630ed8ccb8172dc033eba6b944aa3 Mon Sep 17 00:00:00 2001 From: Gregg Tanzillo Date: Wed, 8 Aug 2018 09:22:01 -0400 Subject: [PATCH] Allow tenant admins to see all groups within the scope of their tenant Manual back port of #17768 to gaprindashvili This had to be done because of the change in master to rely on product features instead of role name to determine whether a user is an admin, tenant admin or super admin Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1613387 --- app/models/miq_group.rb | 2 +- app/models/miq_user_role.rb | 10 +++++--- app/models/user.rb | 4 ++-- lib/rbac/filterer.rb | 3 ++- spec/lib/rbac/filterer_spec.rb | 39 ++++++++++++++++++++++++------- spec/models/miq_user_role_spec.rb | 18 ++++++++++++++ 6 files changed, 61 insertions(+), 15 deletions(-) diff --git a/app/models/miq_group.rb b/app/models/miq_group.rb index 1c6b1dedc83..e8c57bdf82b 100644 --- a/app/models/miq_group.rb +++ b/app/models/miq_group.rb @@ -251,7 +251,7 @@ def self.non_tenant_groups_in_my_region def self.with_current_user_groups(user = nil) current_user = user || User.current_user - current_user.admin_user? ? all : where(:id => current_user.miq_group_ids) + current_user.tenant_admin_user? ? all : where(:id => current_user.miq_group_ids) end def single_group_users? diff --git a/app/models/miq_user_role.rb b/app/models/miq_user_role.rb index 037fcd33285..817fb9143a6 100644 --- a/app/models/miq_user_role.rb +++ b/app/models/miq_user_role.rb @@ -1,7 +1,7 @@ class MiqUserRole < ApplicationRecord - SUPER_ADMIN_ROLE_NAME = "EvmRole-super_administrator" - ADMIN_ROLE_NAME = "EvmRole-administrator" - DEFAULT_TENANT_ROLE_NAME = "EvmRole-tenant_administrator" + SUPER_ADMIN_ROLE_NAME = "EvmRole-super_administrator".freeze + ADMIN_ROLE_NAME = "EvmRole-administrator".freeze + DEFAULT_TENANT_ROLE_NAME = "EvmRole-tenant_administrator".freeze has_many :entitlements, :dependent => :restrict_with_exception has_many :miq_groups, :through => :entitlements @@ -112,6 +112,10 @@ def admin_user? name == SUPER_ADMIN_ROLE_NAME || name == ADMIN_ROLE_NAME end + def tenant_admin_user? + name == SUPER_ADMIN_ROLE_NAME || name == ADMIN_ROLE_NAME || name == DEFAULT_TENANT_ROLE_NAME + end + def self.default_tenant_role find_by(:name => DEFAULT_TENANT_ROLE_NAME) end diff --git a/app/models/user.rb b/app/models/user.rb index 4d2929f6728..7ad2801f280 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -32,7 +32,7 @@ class User < ApplicationRecord delegate :miq_user_role, :current_tenant, :get_filters, :has_filters?, :get_managed_filters, :get_belongsto_filters, :to => :current_group, :allow_nil => true - delegate :super_admin_user?, :admin_user?, :self_service?, :limited_self_service?, :disallowed_roles, + delegate :super_admin_user?, :admin_user?, :tenant_admin_user?, :self_service?, :limited_self_service?, :disallowed_roles, :to => :miq_user_role, :allow_nil => true validates_presence_of :name, :userid @@ -285,7 +285,7 @@ def self.current_user def self.with_current_user_groups(user = nil) user ||= current_user - user.admin_user? ? all : includes(:miq_groups).where(:miq_groups => {:id => user.miq_group_ids}) + user.tenant_admin_user? ? all : includes(:miq_groups).where(:miq_groups => {:id => user.miq_group_ids}) end def self.missing_user_features(db_user) diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb index 77d1d7b8ab0..0670578ef8c 100644 --- a/lib/rbac/filterer.rb +++ b/lib/rbac/filterer.rb @@ -519,7 +519,8 @@ def scope_for_user_role_group(klass, scope, miq_group, user, managed_filters) if MiqUserRole != klass filtered_ids = pluck_ids(get_managed_filter_object_ids(scope, managed_filters)) - scope = scope.with_current_user_groups(user) + # Non tenant admins can only see their own groups. Note - a super admin is also a tenant admin + scope = scope.with_current_user_groups(user) unless user.tenant_admin_user? end scope_by_ids(scope, filtered_ids) diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb index 92bf38edcda..b157725a194 100644 --- a/spec/lib/rbac/filterer_spec.rb +++ b/spec/lib/rbac/filterer_spec.rb @@ -937,14 +937,37 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) let!(:user) { FactoryGirl.create(:user, :miq_groups => [group]) } - it 'can see all roles expect to EvmRole-super_administrator' do - expect(MiqUserRole.count).to eq(3) - get_rbac_results_for_and_expect_objects(MiqUserRole, [tenant_administrator_user_role]) + let!(:user_role) do + FactoryGirl.create(:miq_user_role, :role => "user") end - it 'can see all groups expect to group with role EvmRole-super_administrator' do - expect(MiqUserRole.count).to eq(3) - get_rbac_results_for_and_expect_objects(MiqGroup, [group]) + let!(:other_group) do + FactoryGirl.create(:miq_group, :tenant => default_tenant, :miq_user_role => user_role) + end + + it 'can see all roles except for EvmRole-super_administrator' do + expect(MiqUserRole.count).to eq(4) + get_rbac_results_for_and_expect_objects(MiqUserRole, [tenant_administrator_user_role, user_role]) + end + + it 'can see all groups except for group with roles EvmRole-super_administrator amd EvmRole-administrator' do + expect(MiqUserRole.count).to eq(4) + default_group_for_tenant = user.current_tenant.miq_groups.where(:group_type => "tenant").first + default_group_for_tenant.miq_user_role = MiqUserRole.default_tenant_role + default_group_for_tenant.save! + super_admin_group + get_rbac_results_for_and_expect_objects(MiqGroup, [group, other_group, default_group_for_tenant]) + end + + it 'can see all groups in the current tenant only' do + another_tenant = FactoryGirl.create(:tenant) + another_tenant_group = FactoryGirl.create(:miq_group, :miq_user_role => user_role, :tenant => another_tenant) + group.tenant = another_tenant + + default_group_for_tenant = user.current_tenant.miq_groups.where(:group_type => "tenant").first + default_group_for_tenant.miq_user_role = MiqUserRole.default_tenant_role + default_group_for_tenant.save! + get_rbac_results_for_and_expect_objects(MiqGroup, [another_tenant_group, default_group_for_tenant]) end let(:super_admin_group) do @@ -953,7 +976,7 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) let!(:super_admin_user) { FactoryGirl.create(:user, :miq_groups => [super_admin_group]) } - it 'can see all users expect to user with group with role EvmRole-super_administrator' do + it 'can see all users except for user with group with role EvmRole-super_administrator' do expect(User.count).to eq(2) get_rbac_results_for_and_expect_objects(User, [user]) end @@ -983,7 +1006,7 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) h.metric_rollups << FactoryGirl.create(:metric_rollup_host_hr, :timestamp => t, :cpu_usage_rate_average => v, - :cpu_ready_delta_summation => v * 1000, # Multiply by a factor of 1000 to maake it more realistic and enable testing virtual col v_pct_cpu_ready_delta_summation + :cpu_ready_delta_summation => v * 1000, # Multiply by a factor of 1000 to make it more realistic and enable testing virtual col v_pct_cpu_ready_delta_summation :sys_uptime_absolute_latest => v ) end diff --git a/spec/models/miq_user_role_spec.rb b/spec/models/miq_user_role_spec.rb index e4e14b34dcc..b2059938784 100644 --- a/spec/models/miq_user_role_spec.rb +++ b/spec/models/miq_user_role_spec.rb @@ -200,6 +200,24 @@ end end + describe "#tenant_admin" do + it "detects tenant_admin" do + expect(FactoryGirl.build(:miq_user_role, :role => "tenant_administrator")).to be_tenant_admin_user + end + + it "detects admin" do + expect(FactoryGirl.build(:miq_user_role, :role => "administrator")).to be_tenant_admin_user + end + + it "detects super_admin" do + expect(FactoryGirl.build(:miq_user_role, :role => "super_administrator")).to be_tenant_admin_user + end + + it "does not detect regular role" do + expect(FactoryGirl.build(:miq_user_role)).not_to be_tenant_admin_user + end + end + describe "#destroy" do subject { miq_group.entitlement.miq_user_role } let!(:miq_group) { FactoryGirl.create(:miq_group, :role => "EvmRole-administrator") }