-
Couldn't load subscription status.
- Fork 88
fix: restrict Shared Policy Group configuration #13735
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
base: 4.7.x
Are you sure you want to change the base?
fix: restrict Shared Policy Group configuration #13735
Conversation
Summary of ChangesHello @vikrantgravitee, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security and access control mechanisms for Shared Policy Groups (SPGs) by introducing a dedicated permission for configuration. This change ensures that only authorized administrators can modify SPG settings, while other user roles are granted more restrictive access or no access at all. The updates span both the backend permission definitions and the frontend UI, providing a robust and granular permission system for SPG management. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly introduces a new permission ENVIRONMENT_SHARED_POLICY_GROUP_CONFIGURATION to restrict configuration of Shared Policy Groups (SPGs) to administrators. The changes in the UI and backend API resources consistently adopt this new permission for configuration-related actions. However, I've identified a critical issue in the default role definitions where the USER role is incorrectly granted read access to SPG configurations, which contradicts the stated goals of this PR. There are also a couple of inconsistencies in the UI routing and navigation permissions that would prevent API_PUBLISHERs from listing SPGs as intended. Please see the detailed comments for suggestions on how to resolve these issues.
| .put(EnvironmentPermission.GROUP.getName(), new char[] { READ.getId() }) | ||
| .put(EnvironmentPermission.TENANT.getName(), new char[] { READ.getId() }) | ||
| .put(EnvironmentPermission.PLATFORM.getName(), new char[] { READ.getId() }) | ||
| .put(EnvironmentPermission.SHARED_POLICY_GROUP_CONFIGURATION.getName(), new char[] { READ.getId() }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description explicitly states that the USER role should have no access to Shared Policy Groups ("USER: no access"). This change grants the USER role READ permission on SHARED_POLICY_GROUP_CONFIGURATION, which would allow them to view SPG configurations. This is a security risk as it grants unintended permissions. This line should be removed to adhere to the principle of least privilege and the PR's requirements.
| displayName: 'Shared Policy Groups', | ||
| routerLink: './shared-policy-groups', | ||
| permissions: ['environment-shared_policy_group-r'], | ||
| permissions: ['environment-shared_policy_group_configuration-r'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change restricts visibility of the 'Shared Policy Groups' navigation item to users with environment-shared_policy_group_configuration-r permission. However, the PR description states that an API_PUBLISHER should be able to "list SPG". API_PUBLISHERs have environment-shared_policy_group-r permission, but not the new _configuration one. This change will hide the navigation link from them, preventing them from listing SPGs from the settings page. To align with the stated requirements for API_PUBLISHER, this permission should likely remain environment-shared_policy_group-r.
| permissions: ['environment-shared_policy_group_configuration-r'], | |
| permissions: ['environment-shared_policy_group-r'], |
| data: { | ||
| permissions: { | ||
| anyOf: ['environment-shared_policy_group-r'], | ||
| anyOf: ['environment-shared_policy_group_configuration-r'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change restricts access to the Shared Policy Groups list page to users with environment-shared_policy_group_configuration-r permission. This prevents API_PUBLISHERs from accessing this page, which contradicts the PR description stating they should be able to "list SPG". The individual actions on the list page (add, edit, delete) are already correctly protected by the new _configuration permission. To allow API_PUBLISHERs to view the list, this route permission should be reverted to environment-shared_policy_group-r.
| anyOf: ['environment-shared_policy_group_configuration-r'], | |
| anyOf: ['environment-shared_policy_group-r'], |
77d3303 to
12e4e50
Compare
Issue
https://gravitee.atlassian.net/browse/APIM-10849
Description
Shared Policy Group (SPG) configuration should only be editable by administrators.
This change enforces proper permission separation:
SPG.mp4
Additional context
📚 View the storybook of this branch here