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

Conversation

gtanzillo
Copy link
Member

@gtanzillo gtanzillo commented Jul 26, 2018

Tenant admin users were only able to see groups in which they had membership. That meant that a tenant admin could create a new group but not have the ability to see it.

This change allows tenant admin users visibility to all groups with in the scope of their tenant. That means they can see all groups in their immediate tenant and all child tenants of that tenant. The only exception to this is that they are not able to see any groups that have a super admin role. That condition existed before this change.

/cc @jrafanie @kbrock

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1596639
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1596266

@@ -522,7 +522,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))
# 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?
scope = scope.with_groups(user.miq_group_ids) unless user_or_group.miq_user_role&.super_admin_user? ||
user_or_group.miq_user_role&.tenant_admin_user?
Copy link
Member

Choose a reason for hiding this comment

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

hmm, looking at this again, unless with a || always make me think more to grok it...

maybe this is more obvious:

scope = scope.with_groups(user.miq_group_ids) if !user_or_group.miq_user_role&.super_admin_user? &&
!user_or_group.miq_user_role&.tenant_admin_user?

since the conditional is so long, maybe use a block:

if !user_or_group.miq_user_role&.super_admin_user? && !user_or_group.miq_user_role&.tenant_admin_user?
  scope = scope.with_groups(user.miq_group_ids)
end

Copy link
Member

Choose a reason for hiding this comment

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

maybe we can simplify this further by assigning role at the beginning of this else and use it in the 3 places that all do user_or_group.miq_user_role

else
  role = user_or_group.miq_user_role
  ...

    if !role&.super_admin_user? && !role&.tenant_admin_user?
      scope = scope.with_groups(user.miq_group_ids)
    end
end

@jrafanie
Copy link
Member

LGTM, @kbrock can you take a look?

@jrafanie
Copy link
Member

cc @lpichler (since you were in this code too)

Copy link
Contributor

@lpichler lpichler left a comment

Choose a reason for hiding this comment

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

👍 thanks!

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Thank you so much for adding this.

I also found this behavior odd/broken.

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)

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?
if !role&.super_admin_user? && !role&.tenant_admin_user?
Copy link
Member

Choose a reason for hiding this comment

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

Since a super admin is also a tenant admin, I think you want:

scope = scope.with_groups(user.miq_group_ids) unless role&.tenant_admin_user?

The test I suggested ensures that super_user.tenant_admin_user? == true

Copy link
Member

Choose a reason for hiding this comment

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

Interesting... great point, it's true. Do we lose clarity by removing this though? I'm good with using this suggestion.

@miq-bot
Copy link
Member

miq-bot commented Aug 3, 2018

Checked commits gtanzillo/manageiq@4d996af~...8934759 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. ⭐

@kbrock kbrock self-assigned this Aug 3, 2018
@kbrock kbrock added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 3, 2018
@kbrock kbrock merged commit d23810d into ManageIQ:master Aug 3, 2018
@JPrause
Copy link
Member

JPrause commented Aug 7, 2018

@miq-bot add_label blocker

@JPrause
Copy link
Member

JPrause commented Aug 7, 2018

@gtanzillo if this can be backported, can you add the gaprindashvili/yes label.

@miq-bot miq-bot added the blocker label Aug 7, 2018
gtanzillo added a commit to gtanzillo/manageiq that referenced this pull request Aug 8, 2018
Manual back port of ManageIQ#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
gtanzillo added a commit to gtanzillo/manageiq that referenced this pull request Aug 8, 2018
Manual back port of ManageIQ#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
@kbrock
Copy link
Member

kbrock commented Aug 8, 2018

This is dependent upon #17444
Will need to gaprindashvili/yes that one.

That PR is a little invasive, but with a little work could be backported.
I defer to @gtanzillo on that one

gtanzillo added a commit to gtanzillo/manageiq that referenced this pull request Aug 8, 2018
Manual back port of ManageIQ#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants