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.

Sign up for GitHub

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

Jump to bottom

[GAPRINDASHVILI] Allow tenant admins to see all groups within the scope of their tenant #17817

Merged
merged 1 commit into from
Aug 13, 2018
Merged

[GAPRINDASHVILI] Allow tenant admins to see all groups within the scope of their tenant #17817

merged 1 commit into from
Aug 13, 2018

Conversation

gtanzillo
Copy link
Member

@gtanzillo gtanzillo commented Aug 8, 2018 •
edited
Loading

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

/cc @jrafanie @kbrock

@simaishi simaishi requested review from lpichler and kbrock August 8, 2018 13:25
gtanzillo
gtanzillo commented Aug 8, 2018
View reviewed changes
spec/lib/rbac/filterer_spec.rb

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])
Copy link
Member Author

Choose a reason for hiding this comment

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

@jrafanie @kbrock Notice that there is a difference from the same test on master here (https://github.com/ManageIQ/manageiq/pull/17768/files?utf8=%E2%9C%93&diff=unified#diff-5a9a344d1cbb6a063ed0fb111938778bR1022) On master the tenant admin is able to see the administrator role. In Gaprindashvili he can't. I think it should be consistent but I'm not sure whether it should be changed here or on master.

Copy link
Member

Choose a reason for hiding this comment

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

@kbrock what do you think of this? Also, is it ok to do this PR for gaprindashvili or do we need to bring back the dependent PRs?

Copy link
Member

@kbrock kbrock Aug 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

the PR in question changed admin_user? from a string compare to a role compare.

I think keeping the string compare in Gaprindashvili is good.

G's admin is kinda broken. the use case that was introduced was not fully thought through. The admin doesn't fully have more privs that the tenant admin - so I'm not sure if it matters

@gtanzillo gtanzillo requested a review from jrafanie August 8, 2018 13:31
@gtanzillo gtanzillo force-pushed the tenant-admin-groups-59 branch from 8911892 to c1c047a Compare August 8, 2018 13:53
kbrock
kbrock approved these changes Aug 8, 2018
View reviewed changes
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.

ship it

spec/lib/rbac/filterer_spec.rb

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])
Copy link
Member

@kbrock kbrock Aug 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

the PR in question changed admin_user? from a string compare to a role compare.

I think keeping the string compare in Gaprindashvili is good.

G's admin is kinda broken. the use case that was introduced was not fully thought through. The admin doesn't fully have more privs that the tenant admin - so I'm not sure if it matters

@gtanzillo
Allow tenant admins to see all groups within the scope of their tenant
87cafbd 
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 gtanzillo force-pushed the tenant-admin-groups-59 branch from c1c047a to 87cafbd Compare August 8, 2018 22:26
@miq-bot
Copy link
Member

miq-bot commented Aug 8, 2018

Checked commit gtanzillo@87cafbd with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 🏆

kbrock
kbrock approved these changes Aug 8, 2018
View reviewed changes
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.

Looks good - and is now green

jrafanie
jrafanie approved these changes Aug 13, 2018
View reviewed changes
Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

We discussed this the other day and this is the easiest fix to bring back to gaprindashvili.

@simaishi simaishi merged commit 80878c7 into ManageIQ:gaprindashvili Aug 13, 2018
@simaishi simaishi modified the milestones: Roadmap, Sprint 92 Ending Aug 13, 2018 Aug 13, 2018
@simaishi
Copy link
Contributor

@gtanzillo Is it ok to backport this to Fine branch, or a separate PR will be needed there?

@gtanzillo
Copy link
Member Author

@simaishi

Is it ok to backport this to Fine branch, or a separate PR will be needed there?

Yes, this should backport cleanly to the fine branch

simaishi added a commit that referenced this pull request Aug 14, 2018
@simaishi
Merge pull request #17817 from gtanzillo/tenant-admin-groups-59
a027ed5 
[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
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit a027ed50319276376a82b785e36b7c9a2feb89fa
Author: Satoe Imaishi <[email protected]>
Date:   Tue Aug 14 00:58:11 2018 +0900

    Merge pull request #17817 from gtanzillo/tenant-admin-groups-59
    
    [GAPRINDASHVILI] Allow tenant admins to see all groups within the scope of their tenant
    (cherry picked from commit 80878c7725613eece48a6e12e4407e1de6a96551)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1613388

tumido pushed a commit to tumido/manageiq that referenced this pull request Aug 31, 2018
@simaishi @tumido
Merge pull request ManageIQ#17817 from gtanzillo/tenant-admin-groups-59
074cf89 
[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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpichler lpichler lpichler approved these changes

@jrafanie jrafanie jrafanie approved these changes

@kbrock kbrock kbrock approved these changes

Assignees

@simaishi simaishi

Labels
blocker bug core/rbac fine/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gtanzillo @miq-bot @simaishi @kbrock @jrafanie @lpichler