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(ESSNTL-5253): Change required permissions for tabs #2003

Merged
merged 3 commits into from
Aug 30, 2023

Conversation

gkarat
Copy link
Contributor

@gkarat gkarat commented Aug 28, 2023

Fixes https://issues.redhat.com/browse/ESSNTL-5253. Should be merged after RedHatInsights/frontend-components#1900 is released.

With this PR, Inventory fetches permissions for all applications, not only from the inventory scope. This way, we can read all the required permissions before rendering contents of each tab on the System details page (Advisor, Vulnerability, etc.). This also updates the list of permissions according to the stage, not prod, RBAC config, and makes exception for org. admins to always show the tab contents.

How to test

Have an account for which you can alter roles/permissions. Go to /inventory and any system's details page. Leave only the Inventory Read role, and verify that all tabs except General and Advisor are unavailable (should write no access). Try to add some viewer roles for each app and check that the tabs are now viewable.

@gkarat gkarat marked this pull request as ready for review August 29, 2023 15:53
@gkarat gkarat requested a review from a team as a code owner August 29, 2023 15:53
@gkarat gkarat self-assigned this Aug 29, 2023
@gkarat gkarat added the bug Something isn't working label Aug 29, 2023
Copy link
Contributor

@Fewwy Fewwy left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -9,7 +9,10 @@ const App = () => {
return (
<div className="inventory">
<NotificationsPortal />
<RBACProvider appName="inventory" checkResourceDefinitions>
<RBACProvider
appName={null /* fetch permissions from all scopes */}
Copy link
Member

Choose a reason for hiding this comment

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

What if we extend the RBACProvider to support something like scopes, with * being all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was an author of the recent RBACProvider changes actually, and yeah, this is the good suggestion. There were some suggestions from Karel to keep it simple for now though. So there are three values accepted ATM: null for fetching all scopes, undefined for none, and string that represents the name of app and also scope.

@gkarat gkarat merged commit d0904bd into RedHatInsights:master Aug 30, 2023
gkarat added a commit to gkarat/insights-inventory-frontend that referenced this pull request Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants