Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow tenant admins to see all groups within the scope of their tenant #17768

Merged
merged 3 commits into from
Aug 3, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/models/miq_product_feature.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ class MiqProductFeature < ApplicationRecord
REPORT_ADMIN_FEATURE = "miq_report_superadmin".freeze
REQUEST_ADMIN_FEATURE = "miq_request_superadmin".freeze
ADMIN_FEATURE = REPORT_ADMIN_FEATURE
TENANT_ADMIN_FEATURE = "rbac_tenant".freeze

acts_as_tree

has_and_belongs_to_many :miq_user_roles, :join_table => :miq_roles_features
Expand Down
4 changes: 4 additions & 0 deletions app/models/miq_user_role.rb
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ def super_admin_user?
allows?(:identifier => MiqProductFeature::SUPER_ADMIN_FEATURE)
end

def tenant_admin_user?
allows?(:identifier => MiqProductFeature::TENANT_ADMIN_FEATURE)
end

def report_admin_user?
allows?(:identifier => MiqProductFeature::REPORT_ADMIN_FEATURE)
end
Expand Down
7 changes: 4 additions & 3 deletions lib/rbac/filterer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -514,15 +514,16 @@ def scope_for_user_role_group(klass, scope, miq_group, user, managed_filters)
if user_or_group.try!(:self_service?) && MiqUserRole != klass
scope.where(:id => klass == User ? user.id : miq_group.id)
else
role = user_or_group.miq_user_role
# hide creating admin group / roles from non-super administrators
unless user_or_group.miq_user_role&.super_admin_user?
unless role&.super_admin_user?
scope = scope.with_roles_excluding(MiqProductFeature::SUPER_ADMIN_FEATURE)
end

if MiqUserRole != klass
filtered_ids = pluck_ids(get_managed_filter_object_ids(scope, managed_filters))
# Non admins can only see their own groups
scope = scope.with_groups(user.miq_group_ids) unless user_or_group.miq_user_role&.super_admin_user?
# Non tenant admins can only see their own groups. Note - a super admin is also a tenant admin
scope = scope.with_groups(user.miq_group_ids) unless role&.tenant_admin_user?
end

scope_by_ids(scope, filtered_ids)
Expand Down
41 changes: 32 additions & 9 deletions spec/lib/rbac/filterer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -987,8 +987,12 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
end

context 'with EvmRole-tenant_administrator' do
let(:rbac_tenant) do
FactoryGirl.create(:miq_product_feature, :identifier => MiqProductFeature::TENANT_ADMIN_FEATURE)
end

let(:tenant_administrator_user_role) do
FactoryGirl.create(:miq_user_role, :name => MiqUserRole::DEFAULT_TENANT_ROLE_NAME)
FactoryGirl.create(:miq_user_role, :name => MiqUserRole::DEFAULT_TENANT_ROLE_NAME, :miq_product_features => [rbac_tenant])
end

let!(:super_administrator_user_role) do
Expand All @@ -1003,16 +1007,35 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
FactoryGirl.create(:miq_group, :tenant => default_tenant, :miq_user_role => tenant_administrator_user_role)
end

let!(:user_role) do
FactoryGirl.create(:miq_user_role, :role => "user")
end

let!(:other_group) do
FactoryGirl.create(:miq_group, :tenant => default_tenant, :miq_user_role => user_role)
end

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, administrator_user_role])
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, administrator_user_role, user_role])
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])
it 'can see all groups except for group with role EvmRole-super_administrator' do
expect(MiqUserRole.count).to eq(4)
default_group_for_tenant = user.current_tenant.miq_groups.where(:group_type => "tenant").first
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, :tenant => another_tenant)
group.tenant = another_tenant

default_group_for_tenant = user.current_tenant.miq_groups.where(:group_type => "tenant").first
get_rbac_results_for_and_expect_objects(MiqGroup, [another_tenant_group, default_group_for_tenant])
end

let(:super_admin_group) do
Expand All @@ -1021,7 +1044,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 @@ -1051,7 +1074,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
15 changes: 15 additions & 0 deletions spec/models/miq_user_role_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@
end

let(:super_admin_role) { FactoryGirl.create(:miq_user_role, :features => MiqProductFeature::SUPER_ADMIN_FEATURE) }
let(:tenant_admin_role) { FactoryGirl.create(:miq_user_role, :features => MiqProductFeature::TENANT_ADMIN_FEATURE) }
let(:report_admin_role) { FactoryGirl.create(:miq_user_role, :features => MiqProductFeature::REPORT_ADMIN_FEATURE) }
let(:request_admin_role) { FactoryGirl.create(:miq_user_role, :features => MiqProductFeature::REQUEST_ADMIN_FEATURE) }
let(:regular_role) { FactoryGirl.create(:miq_user_role) }
Expand Down Expand Up @@ -208,6 +209,20 @@
end
end

describe "#tenant_admin" do
it "detects tenant_admin" do
expect(tenant_admin_role).to be_tenant_admin_user
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add test with the super_admin_role (should be true) and regular_role (should be false)


it "detects super_admin" do
expect(super_admin_role).to be_tenant_admin_user
end

it "does not detect regular role" do
expect(regular_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