Skip to content

Commit

Permalink
Merge pull request ManageIQ#17817 from gtanzillo/tenant-admin-groups-59
Browse files Browse the repository at this point in the history
[GAPRINDASHVILI] Allow tenant admins to see all groups within the scope of their tenant
(cherry picked from commit 80878c7)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1613388
  • Loading branch information
simaishi authored and tumido committed Aug 31, 2018
1 parent a488a68 commit 074cf89
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 15 deletions.
2 changes: 1 addition & 1 deletion app/models/miq_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,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?
Expand Down
10 changes: 7 additions & 3 deletions app/models/miq_user_role.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,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
Expand Down Expand Up @@ -264,7 +264,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.seed
Expand Down
3 changes: 2 additions & 1 deletion lib/rbac/filterer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,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)
Expand Down
39 changes: 31 additions & 8 deletions spec/lib/rbac/filterer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -695,14 +695,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
Expand All @@ -711,7 +734,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
Expand Down Expand Up @@ -741,7 +764,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
Expand Down
18 changes: 18 additions & 0 deletions spec/models/miq_user_role_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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") }
Expand Down

0 comments on commit 074cf89

Please sign in to comment.