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

Add Set Ownership to Key Pairs #5973

Merged
merged 2 commits into from
Aug 13, 2019

Conversation

PanSpagetka
Copy link
Contributor

@PanSpagetka PanSpagetka commented Aug 8, 2019

@PanSpagetka
Copy link
Contributor Author

@h-kataria @lpichler

@h-kataria
Copy link
Contributor

@PanSpagetka we will need to add this in the Product features as well.

@h-kataria
Copy link
Contributor

@PanSpagetka one more thing, can we add ownership information on the summary screen as well. Same for the Hammer PR.

@PanSpagetka
Copy link
Contributor Author

@miq-bot add_label pending core
ManageIQ/manageiq#19124

@andyvesel
Copy link
Contributor

Tested on master. The PR partly solves the problem: Keypairs now have correct evm_owner_id and miq_group_id assigned.
I create keypair as an Admin. Then I set ownership to it to TestUser with TestGroup (and its role is a TestRole, where keypairs are visible). When I log in as a testuser, I don't see any kps
I tried to do it both with tenant mapping enabled and disabled.
So, the filtering still needs to be implemented on a backend side (in a different PR maybe).

@PanSpagetka PanSpagetka force-pushed the add-ownership-keypair branch from b879cba to 31a00b4 Compare August 12, 2019 13:48
@lpichler
Copy link
Contributor

Tested on master. The PR partly solves the problem: Keypairs now have correct evm_owner_id and miq_group_id assigned.
I create keypair as an Admin. Then I set ownership to it to TestUser with TestGroup (and its role is a TestRole, where keypairs are visible). When I log in as a testuser, I don't see any kps
I tried to do it both with tenant mapping enabled and disabled.
So, the filtering still needs to be implemented on a backend side (in a different PR maybe).

@andyvesel thanks for testing we had similar result with @PanSpagetka
but we realised that user's role needs to have set up options"Access Restriction for Services, VMs, and Templates" to user or group.

if you have still this setup , can you try it ?

thanks!

@andyvesel
Copy link
Contributor

@lpichler yes, I'll try it

@andyvesel
Copy link
Contributor

Checked, LGTM 👍

@PanSpagetka PanSpagetka force-pushed the add-ownership-keypair branch from 31a00b4 to 5accb14 Compare August 13, 2019 13:07
@miq-bot
Copy link
Member

miq-bot commented Aug 13, 2019

Checked commits PanSpagetka/manageiq-ui-classic@5b6f8e7~...5accb14 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 4 offenses detected

app/helpers/application_helper/toolbar/auth_key_pair_cloud_center.rb

app/helpers/application_helper/toolbar/auth_key_pair_clouds_center.rb

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 all ! LGTM & tested 👍 👍 so I am also confirming funcionality:

  • if user belongs to root tenant (My Company) he will see all Key Pairs
  • if user doesn't belongs to root tenant (My Company) he will see only Key Pairs with the user's tenant (Key Pairs can have tenant derived from assigned group in set ownership screen)

role's restriction :

  • if user's role "Access Restriction for Services, VMs, and Templates" is set to Owned Group then user will see Key Pairs with this user's group(group is set in set ownership screen of Key Pairs)

  • if user's role "Access Restriction for Services, VMs, and Templates" is set to Owned Group or User then user will see Key Pairs with this user's group or user (group or user is set in set ownership screen of Key Pairs)

@h-kataria h-kataria self-assigned this Aug 13, 2019
@h-kataria h-kataria added this to the Sprint 118 Ending Aug 19, 2019 milestone Aug 13, 2019
@h-kataria h-kataria merged commit 5692b46 into ManageIQ:master Aug 13, 2019
@skateman
Copy link
Member

I'm pretty sure there are collisions with #5863

@lpichler
Copy link
Contributor

I'm pretty sure there are collisions with #5863

@PanSpagetka 🍝 got conflict and he did rebase.

@skateman
Copy link
Member

Yeah, because rebasing always solves the problem 😆 the page is broken

@lpichler
Copy link
Contributor

Yeah, because rebasing always solves the problem 😆 the page is broken
page is not broken in Ivanchuk just in master

@simaishi

@PanSpagetka
Copy link
Contributor Author

ManageIQ/manageiq-api#650 this PR solves the problem

@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit f0b1b500f1a029429b58716af3cf64a31bc79fa6
Author: Harpreet Kataria <[email protected]>
Date:   Tue Aug 13 10:25:55 2019 -0400

    Merge pull request #5973 from PanSpagetka/add-ownership-keypair
    
    Add Set Ownership to Key Pairs
    
    (cherry picked from commit 5692b46ca8720ede8deb67c343f92aba39bd3456)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1589766
    https://bugzilla.redhat.com/show_bug.cgi?id=1730066

@simaishi
Copy link
Contributor

Backported to Hammer via #5967

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.

7 participants