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

Fix the pre-defined Auditor role's permissions. #16394

Merged
merged 1 commit into from
Jan 26, 2018

Conversation

martinpovolny
Copy link
Member

@martinpovolny martinpovolny commented Nov 4, 2017

Auditor: Able to see virtual infrastructure for auditing purposes. Can view all infrastructure items. Cannot perform actions on them.

http://talk.manageiq.org/t/product-features-and-roles/2855

https://bugzilla.redhat.com/show_bug.cgi?id=1479583

This fixes just the Auditor role. A more systematic check and fix is needed. A detailed review is very welcome.

@martinpovolny martinpovolny changed the title Fix the pre-defined Auditor role's permissins. Fix the pre-defined Auditor role's permissions. Nov 6, 2017
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

I'm ok with this change. However, it would be good for someone else on the UI team to review because I am not familiar with how the changed identifiers are used in the UI code and cannot asses the impact of the changes.

@martinpovolny
Copy link
Member Author

@h-kataria : can you, please review this?

I'ld also like to discuss how to fix all the roles, not just this one. Thx!

- ems_infra_timeline
- ems_physical_infra_console
- ems_physical_infra_show
- ems_physical_infra_show_list
- ems_physical_infra_tag
- ems_physical_infra_timeline
- infra_networking_view
- infra_networking_tag
- instance_view
Copy link
Contributor

Choose a reason for hiding this comment

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

@martinpovolny since you already have instance_view, vm_view, image_view, you shouldn't need instance_compare, instance_drift, instance_timeline, instance_perf and same for image_* & vm_*

- image_check_compliance
- image_compare
- image_drift
- image_petf
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

@martinpovolny
Copy link
Member Author

martinpovolny commented Dec 2, 2017

@h-kataria : thank you for the detailed review!

Fixed the issues you found.

I also added host_view removing the children.

@martinpovolny
Copy link
Member Author

Added some edits:

  • Add miq_template_view and remove it's children.
  • Replace miq_template_snapshot with miq_template_snapshot_view.

@miq-bot
Copy link
Member

miq-bot commented Dec 21, 2017

This pull request is not mergeable. Please rebase and repush.

@martinpovolny
Copy link
Member Author

Ping @h-kataria, @gtanzillo. Had to rebase this again.

@miq-bot
Copy link
Member

miq-bot commented Jan 9, 2018

This pull request is not mergeable. Please rebase and repush.

@martinpovolny
Copy link
Member Author

Ping @h-kataria, @gtanzillo.

Is there any point in me rebasing this over and over again or should I close the BZ as "won't fix"?

@h-kataria
Copy link
Contributor

@martinpovolny please rebase it one more time, i will merge once Travis turns green.

 * Add miq_template_view and remove it's children.
 * Replace miq_template_snapshot with miq_template_snapshot_view.
 * Replace ems_cluster_show and other with ems_cluster_view
 * Add infra_networking_view and other
 * Add image_view and other
 * Add instance_view and other
 * Add host_view and other
@martinpovolny
Copy link
Member Author

Rebased.

@miq-bot
Copy link
Member

miq-bot commented Jan 26, 2018

Checked commit martinpovolny@e704564 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🏆

@h-kataria h-kataria added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 26, 2018
@h-kataria h-kataria merged commit fcc654f into ManageIQ:master Jan 26, 2018
simaishi pushed a commit that referenced this pull request Mar 8, 2018
Fix the pre-defined Auditor role's permissions.
(cherry picked from commit fcc654f)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1553392
@simaishi
Copy link
Contributor

simaishi commented Mar 8, 2018

Gaprindashvili backport details:

$ git log -1
commit 555435b44d629d182c554f3303ede83bad525fac
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Fri Jan 26 09:17:45 2018 -0500

    Merge pull request #16394 from martinpovolny/role_fix
    
    Fix the pre-defined Auditor role's permissions.
    (cherry picked from commit fcc654f3a77d9e843b200d38ff07922333c540c2)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1553392

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