Feat(Auth): Authorization by Env Cluster #5302
Conversation
dosubot
bot
added
the
size:XXL
WalkthroughThis pull request introduces comprehensive changes to the Apollo Configuration Center's permission management system, focusing on enhancing cluster-level namespace permissions. The modifications span multiple components, including controllers, services, and utility classes, to provide more granular access control for users across different environments and clusters. Changes
Sequence DiagramsequenceDiagram
participant User
participant PermissionController
participant RoleInitializationService
participant RolePermissionService
User->>PermissionController: Request cluster namespace role
PermissionController->>RoleInitializationService: Initialize cluster roles
RoleInitializationService-->>RolePermissionService: Create/Verify roles
PermissionController->>RolePermissionService: Check/Assign/Remove roles
RolePermissionService-->>PermissionController: Return role status
PermissionController-->>User: Respond with role details
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (20)
apollo-portal/src/test/java/com/ctrip/framework/apollo/ControllableAuthorizationConfiguration.java (1)
44-45: Hard-coded username for permission checks.
Relying on"luke"as a special-cased user is fine for a single scenario, but consider parameterizing or documenting this for clarity and future extension to different test scenarios.- when(mock.userHasPermission(eq("luke"), any(), any())).thenReturn(true); + // Optionally, read from a property or variable, allowing different user test scenariosapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/ClusterRolesAssignedUsers.java (3)
24-28: Consider usingfinalor a constructor for immutability
Currently, fields can be freely reassigned. Making themfinaland initializing them through a constructor can reinforce immutability. This is a minor suggestion that can help avoid unintended reassignments.
54-60: Getter/Setter formodifyRoleUsers
If you want to avoid accidental external modifications, consider returning and storing unmodifiable sets or defensive copies.- public void setModifyRoleUsers(Set<UserInfo> modifyRoleUsers) { - this.modifyRoleUsers = modifyRoleUsers; - } + public void setModifyRoleUsers(Set<UserInfo> modifyRoleUsers) { + this.modifyRoleUsers = modifyRoleUsers == null + ? Collections.emptySet() + : Collections.unmodifiableSet(new HashSet<>(modifyRoleUsers)); + }
62-68: Getter/Setter forreleaseRoleUsers
The same consideration for immutability applies here if that aligns with the design approach.apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js (1)
280-290: Consolidate repeated permission-checking logicWithin these lines, the permission checks for both
namespaceandnamespace.branchuse nearly identical expressions for settinghasModifyPermission. Consider extracting this repetition into a helper function or a shared utility to improve code maintainability.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java (1)
71-75: **Consider externalizing test profile activation **Setting the active Spring profile programmatically within the test may mask potential environment inconsistencies. It's often preferable to configure the test profile via configuration files or command-line properties for clearer separation of concerns.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java (2)
84-88: Validate environment for consistency.
Unlike some other methods in this controller (e.g.,getClusterRoles),initClusterPermissiondoes not explicitly validateenv. Consider adding a similar check forEnv.UNKNOWNor invalid environment formats for consistency and safety.
215-237: Cluster role retrieval logic is clear.
This endpoint neatly validatesenvand fetches release/modify role users. The approach is consistent with existing naming and usage patterns. Consider extracting the environment validation into a small helper function to reduce duplicate code blocks across multiple endpoints.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RoleInitializationService.java (1)
36-37: Consider adding Javadoc.
TheinitClusterRolesmethod is clear in intent but lacks any Javadoc/explanatory comments describing preconditions, side effects, or error handling. Adding brief documentation can help future maintainers.apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java (1)
68-68: Expand test coverage for negative scenarios.
MockinghasAssignRolePermission(...)to always returntruecan mask potential authorization bugs. While this configuration may be intentional for integration tests, consider adding complementary tests to verify scenarios when permissions are absent or restricted.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ClusterService.java (1)
54-56: Confirm role initialization for newly created clusters.Calling
roleInitializationService.initClusterRolesis crucial for setting up cluster-level permissions upon creation. Consider adding error handling or logging if role initialization fails, so administrators can investigate any issues.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java (1)
52-106: Comprehensive test for cluster role lifecycle.
- Validation: The test checks the role assignment, retrieval, and removal thoroughly, ensuring coverage of main CRUD operations.
- Clarity: Each step is clearly separated, making it easy to follow the test logic.
- Potential Improvement: Include a negative test case where attempts to modify the cluster fail when the user lacks the needed role.
apollo-portal/src/main/resources/static/scripts/controller/role/ClusterRoleController.js (1)
36-41: Initialize permissions gracefully.Initializing cluster permissions on controller load ensures the UI can conditionally show or hide role management functionality. The promise handlers properly show a warning if initialization fails. Consider adding debug logs for better troubleshooting.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (1)
172-176: Handle possible exceptions in environment iteration
When retrieving active environments viaportalSettings.getActiveEnvs(), consider adding error-handling or logging if one or more target environments fail to initialize cluster roles (e.g., network or permission availability issues). This is especially important for robust production usage.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RoleInitializationServiceTest.java (1)
45-45: Test environment should be configurable
private final String ENV = "DEV";is hardcoded. Making this configurable (e.g., from a properties file or test parameter) would allow environment-specific test variations without code changes.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java (2)
211-212: Loop-based validation
When syncing items to multiple namespaces, each iteration checks:hasModifyNamespacePermission(...) || hasModifyClusterPermission(...)If any environment requires extra constraints, consider customizing the logic or restricting the OR condition to reduce vulnerabilities.
226-227: Syntax check permission
Running syntax checks can be less restrictive than actual item modification, but is currently governed by the same permission check. If the design requires a less strict check, consider adjusting.apollo-portal/src/main/resources/static/scripts/services/PermissionService.js (2)
26-32: Ensure consistent error-handling approach.
This newinit_cluster_permissionresource is consistent with the existing pattern; however, if the server response contains unexpected error details, consider logging them to help with debugging and ensuring the UI is notified properly.
101-105: Add user-notification logic if request fails.
Retrieving cluster role users is straightforward here but might benefit from more explicit error-handling or UI updates (e.g., toaster error messages) on failure.apollo-portal/src/main/resources/static/cluster/role.html (1)
1-173: Well-structured new file for cluster-level role management.
- The layout is intuitive, using AngularJS forms and repeaters to manage “ModifyCluster” and “ReleaseCluster” roles.
- It's good to see the
hasAssignUserPermissioncondition guard the role assignment UI. This prevents showing sensitive options to unauthorized users.- Consider adding a loading indicator for slow network cases when retrieving assigned users or assigning roles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/PermissionType.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/RoleType.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java(7 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/ClusterRolesAssignedUsers.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ClusterService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RoleInitializationService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java(4 hunks)apollo-portal/src/main/resources/static/cluster/role.html(1 hunks)apollo-portal/src/main/resources/static/config.html(1 hunks)apollo-portal/src/main/resources/static/i18n/en.json(1 hunks)apollo-portal/src/main/resources/static/i18n/zh-CN.json(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/role/ClusterRoleController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js(2 hunks)apollo-portal/src/main/resources/static/scripts/services/PermissionService.js(7 hunks)apollo-portal/src/main/resources/static/styles/common-style.css(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/ControllableAuthorizationConfiguration.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RoleInitializationServiceTest.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java
🧰 Additional context used
🪛 Biome (1.9.4)
apollo-portal/src/main/resources/static/scripts/controller/role/ClusterRoleController.js
[error] 88-88: Shouldn't redeclare 'user'. Consider to delete it or rename it.
'user' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (77)
apollo-portal/src/test/java/com/ctrip/framework/apollo/ControllableAuthorizationConfiguration.java (4)
1-16: License header continuity check.
The license header follows the Apache 2.0 format consistently, matching the project’s standard. No action needed.
31-33: Clear class naming and annotations.
Naming reflects the intent of providing a controllable authorization context for tests, and usage of @Configuration is appropriate.
37-38: Mocked userInfoHolder bean.
Using a @Primary bean overrides any default beans in the context. This is a good approach for test isolation. Make sure that user details are correctly set via further stubbing if needed in actual tests.
50-51: Mocked itemService bean.
Again, overriding with a primary bean mock for the test environment is consistent with the overall design. Ensure that dependent test cases provide the necessary stubbing logic for item operations if required.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/ClusterRolesAssignedUsers.java (8)
1-16: License header is well-structured
This file is correctly including the Apache License 2.0 header, which is consistent with project requirements.
17-17: Package declaration looks good
The package path follows logical grouping under com.ctrip.framework.apollo.portal.entity.vo, matching the project's code organization.
19-20: Imports are straightforward and minimal
You’re only importing the UserInfo class and Set, which is appropriate. No extraneous imports are present.
22-22: Class name is descriptive
The name ClusterRolesAssignedUsers clearly indicates the purpose of this class, reflecting its role in storing user-role assignments.
30-36: Getter/Setter for env
Straightforward implementation. The semantics are clear, and the naming is consistent with the field name.
38-44: Getter/Setter for appId
Same pattern: minimal, readable, and consistent.
46-52: Getter/Setter for cluster
Matches the earlier style and ensures clarity for cluster information.
70-80: toString() is concise and adequate
It includes all fields, making debugging and logging easier.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java (1)
128-128: Verify null or empty admin list handling
If appModel.getAdmins() is empty or null, the method might silently skip assigning admins. Ensure the app creation logic handles edge cases or returns an informative error.
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js (2)
295-306: Validate fallback logic for cluster-level modify permission
This block enriches the permission model by setting namespace.hasModifyPermission (and branch) if the user has cluster-level modify permission. Ensure that permission results do not inadvertently override existing narrower-scope permissions and that future logic properly accounts for cluster-level overrides.
335-347: Ensure consistency for cluster-level release permission
This block mirrors the modify-permission calls with cluster-level release permission checks. Verify that setting hasReleasePermission does not conflict with narrower namespace permissions. Additionally, consider unifying permission checks into a single routine for improved maintainability.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java (5)
1-16: **License Header Check **
The Apache 2.0 License header is properly included, adhering to standard open-source licensing requirements.
81-101: **Robust negative test coverage **
The test method testCreateItemPermissionDenied correctly validates that a user without the required permission receives a 403 FORBIDDEN response. This improves confidence in the authorization checks for item creation.
103-120: **Positive test coverage and service interaction **
The test method testCreateItemPermissionAccessed verifies that an authorized user can create items, and the verify call ensures the ItemService is invoked with correct arguments. This is a well-structured integration test.
121-125: **Mocking user identity **
The setUserId helper method appropriately mocks UserInfoHolder to simulate different user identities, supporting thorough permission testing scenarios.
103-105: **Clean-up strategy after tests **
The usage of @Sql with the AFTER_TEST_METHOD phase is a good approach for cleaning up database changes. Confirm that this script reliably reverts data, especially if multiple tests interact with the same database state.
✅ Verification successful
Let me analyze the test class and cleanup script further to ensure we have a complete picture.
Let me check one more aspect to see if there are other tests in this class that might affect shared state.
Clean-up strategy is comprehensive and reliable
The cleanup.sql script is well-designed and thoroughly cleans up all relevant database tables after test execution:
- It includes cleanup for all core tables: Item, Namespace, AppNamespace, Cluster, App, etc.
- It also covers authentication-related tables: Users, Authorities, Permission, Role, etc.
- The script is executed in the AFTER_TEST_METHOD phase, ensuring proper cleanup regardless of test outcome
The cleanup strategy is reliable for this test case as:
- The script removes all data from tables that could be affected by item creation tests
- It's comprehensive enough to handle shared state between different test methods
- The DELETE statements are unconditional, ensuring complete cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the cleanup.sql is present and confirm references to the relevant tables or data
fd cleanup.sql --exec cat
Length of output: 2539
Script:
#!/bin/bash
# Check the test class for database interactions and setup
rg -B 5 -A 5 "testCreateItemPermissionAccessed"
# Look for other tests in the same class that might affect shared state
ast-grep --pattern 'class ItemControllerAuthIntegrationTest {
$$$
}'
Length of output: 1717
Script:
#!/bin/bash
# Check for all test methods and their cleanup annotations in this class
rg -l "ItemControllerAuthIntegrationTest" | xargs rg "@Test|@Sql.*cleanup"
Length of output: 182
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java (4)
28-28: Import usage looks fine.
This import statement for ClusterRolesAssignedUsers indicates a new VO class for cluster-level role assignments. It's clearly used downstream for retrieving and displaying cluster roles.
124-135: Might need environment validation.
Here, too, the code retrieves cluster permissions without an explicit environment format check. Ensure consistency by validating env just as other methods (e.g., lines 218–222) do.
239-263: Role assignment usage is consistent.
Assignments follow the same pattern used throughout the controller: verifying user existence, validating env, checking roleType, etc. The logic is straightforward and consistent with established patterns for namespace roles.
265-281: Removal of cluster role is correct.
The removal approach parallels the assignment logic, providing a symmetrical API design. The environment checks, role checks, and user checks all align well with the rest of the code.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/RoleType.java (2)
27-30: New cluster role types introduced.
The new constants MODIFY_CLUSTER and RELEASE_CLUSTER follow existing naming conventions, which promotes clarity.
32-33: Updated isValidRoleType logic.
Extending the check to include cluster roles ensures consistency. The approach is straightforward and leverages existing comparisons appropriately.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/PermissionType.java (1)
48-54: Cluster-level permission constants.
Defining MODIFY_CLUSTER and RELEASE_CLUSTER aligns with the newly introduced cluster role types in RoleType.java. This extension cleanly supports the new cluster-level permissions.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ClusterService.java (2)
35-35: Field injection is properly defined.
Declaring the RoleInitializationService as final is a good practice, preventing unintended reassignment.
37-41: Constructor injection looks consistent.
The new constructor parameter roleInitializationService follows established best practices for dependency injection. Ensure that any calling code in other parts of the application is updated accordingly to avoid NoSuchMethodError.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java (1)
64-65: Expanded permission check is aligned with cluster-level security.
Including hasModifyClusterPermission in the @PreAuthorize expression ensures tighter access control. Verify that the logic remains understandable to maintainers and consider documenting the relationship between namespace- and cluster-level permissions.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java (2)
44-45: Autowired service usage is correct.
Injecting RoleInitializationService is consistent. Confirm that related tests utilize mocks if needed to isolate behavior.
47-50: Set-up method ensures cluster roles are initialized before tests.
Invoking roleInitializationService.initClusterRoles prior to tests is beneficial for a clean and predictable test environment. If roles require unique states for different tests, ensure proper teardown as well.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (4)
69-71: Method name aligns with purpose.
buildModifyClusterRoleName is self-explanatory. The usage of STRING_JOINER for consistent role naming is good for maintainability.
85-87: Consistent naming for release cluster roles.
buildReleaseClusterRoleName parallels the existing naming patterns, ensuring consistency. No additional concerns here.
97-99: Generalized builder for cluster role names.
The buildClusterRoleName method is a flexible addition, making it easier to handle future role types without cluttering the utility class.
113-115: Target ID generation is straightforward.
buildClusterTargetId correctly aggregates the user-specified parameters, helping maintain clarity in permission checks. Consider verifying that this target ID formatting remains consistent across all permission/role checks in usage.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.java (2)
63-67: Good addition for cluster modification checks.
This method effectively queries whether the current user has the new MODIFY_CLUSTER permission. The logic is consistent with the existing permission checks for namespaces and appears to align well with the cluster-level permission paradigm introduced in this PR.
81-85: Good addition for cluster release checks.
Similar to the hasModifyClusterPermission method, this method straightforwardly verifies whether the user has the RELEASE_CLUSTER permission, further expanding the portal’s fine-grained permission model.
apollo-portal/src/main/resources/static/scripts/controller/role/ClusterRoleController.js (5)
43-48: Check if user has role assignment permissions.
This call to PermissionService.has_assign_user_permission is a good practice before allowing role assignment. The result is used to toggle $scope.hasAssignUserPermission. Consider verifying if further role assignment methods are disabled accordingly in the UI when the user lacks permissions.
50-57: Handle user roles retrieval robustly.
Fetching the current cluster’s assigned users and storing them in $scope.rolesAssignedUsers is well-implemented. The error handling gracefully alerts the user if the retrieval fails. This provides clarity on assignment statuses.
58-87: Cluster role assignment logic is solid.
The assignRoleToUser function correctly handles both "ReleaseCluster" and "ModifyCluster" scenarios by calling the appropriate PermissionService methods. The use of select2 to retrieve the user ID is neat. Error handling is also well-structured, showing success or failure toasts.
88-88: 'user' variable redeclaration.
The user variable is redeclared in another branch, which may shadow or confuse references to the same identifier. This has been flagged before in a previous review.
🧰 Tools
🪛 Biome (1.9.4)
[error] 88-88: Shouldn't redeclare 'user'. Consider to delete it or rename it.
'user' is defined here:
(lint/suspicious/noRedeclare)
153-162: Clean removal from array.
The removeUserFromList utility function properly identifies and removes the user by matching userId. This is straightforward yet effective. Beware of potential off-by-one errors when splicing.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.java (3)
81-82: Broadened pre-authorization condition.
By combining hasModifyNamespacePermission with hasModifyClusterPermission, the code now supports either permission to create a branch. This change is consistent with the cluster-level authorization expansion, ensuring more flexibility.
101-106: Refined delete permission checks.
Splitting permissions into modification (hasModifyPermission) and release (hasReleasePermission) clarifies the conditions under which a branch may be deleted. The logic to allow deletion if there’s no release or if the user also has release rights is well-structured.
122-123: Extended pre-authorization for merge.
Allowing a merge with either release namespace permission or cluster permission is logical. This aligns nicely with the newly introduced hasReleaseClusterPermission API, preserving consistent authorization across different operations.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java (3)
72-73: New pre-authorization for normal releases.
Requiring either namespace release permission or cluster release permission is consistent with the cluster-focused enhancements. It empowers more granular control over who can initiate releases.
102-103: Appropriate for gray release checks.
Similarly, broadening gray release checks to consider cluster-level permissions is consistent with the changes in the rest of the codebase. Ensures that only authorized users can initiate a gray release.
192-198: Rollback permission refinement.
Separating the checks into releaseNamespacePermission and releaseClusterPermission clearly indicates that either permission suffices to roll back a release. The subsequent AccessDeniedException is concise and appropriate.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java (4)
28-31: Use PortalSettings with caution across modules
The newly imported PortalSettings is used to retrieve the active environments. Ensure that any future module dependencies remain consistent with portalSettings usage to avoid potential environment mismatches or stale data if caching is involved.
68-68: Consistency in final fields
Declaring portalSettings as final is a good practice that enforces immutability. Continue applying this principle for other newly introduced dependencies to preserve consistent constructor-based dependency injection.
Line range hint 82-94: Constructor updates require test updates
The constructor signature change to include PortalSettings might break code in downstream modules or tests. Ensure all test suites and integration points are updated accordingly to pass the PortalSettings parameter.
145-147: Initialize cluster roles with caution
roleInitializationService.initClusterRoles(...) depends on env.getName() and ConfigConsts.CLUSTER_NAME_DEFAULT. Make sure env is non-null and recognized by the system. Consider validating or logging in case env or cluster name is invalid.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RoleInitializationServiceTest.java (4)
148-167: Positive path test coverage
The testInitClusterRole test method convincingly verifies a scenario where neither the modify nor release cluster role exists. This solid coverage ensures correct role creation.
168-185: Verify no extraneous calls
testInitClusterRoleHasExisted correctly checks that no new roles or permissions are created if they already exist. This covers an important idempotent scenario.
187-206: Mixed existence scenarios
testInitClusterRoleModifyClusterExisted verifies the modify cluster role pre-exists but not the release role. This partial existence scenario is well tested, ensuring graceful handling.
208-227: Release cluster pre-exists scenario
testInitClusterRoleReleaseClusterExisted ensures role creation logic is partial, addressing the final major scenario. The test suite is comprehensive.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java (2)
177-189: Transaction boundary
@Transactional on initClusterRoles ensures consistent role creation. However, if partial failures occur (e.g., cluster role creation succeeds but release role creation fails), re-check that the transaction is rolled back as expected.
246-255: Cluster role creation and permission association
createClusterRole(...) properly sets up the permission and role within a single method. This is good for maintainability. Should further permission checks be needed, keep everything within this method or consider a dedicated helper method.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java (5)
91-92: Create item permission scope
Similar to line 76, newly created items in a namespace can be changed by anyone with cluster-level modify privileges. Confirm that this is by design.
111-112: Update item permission
Same OR-based approach applies for updating items. Confirm there are no conflicts between cluster-level roles and granular namespace-level roles that might cause unexpected modifications.
126-127: Delete item permission
Deleting an item typically requires at least the same permissions required for creation or update. The OR-based approach may be too permissive if the business logic expects stricter controls.
238-239: Revoke items permission
Revoking items also uses the OR condition. If the product intentionally merges the cluster and namespace permissions, this is validated. Otherwise, confirm the correct method of enforcement.
76-77: OR-based permission is broad
The OR logic:
@permissionValidator.hasModifyNamespacePermission(...) OR
@permissionValidator.hasModifyClusterPermission(...)
allows either cluster or namespace modify permissions. Verify this doesn't inadvertently grant excessive privileges to cluster-level operators who lack namespace-level permissions, and vice versa.
✅ Verification successful
Let me gather information about the permission validation implementation to verify the concern.
Let me try to find the actual implementation details using ripgrep for better context.
Let me search for more context about the permission types and role utils to understand the permission hierarchy.
Let me try one final search to understand the ItemController's protected operation.
Based on the code analysis so far, I can provide a final response:
OR-based permission is appropriately scoped
The permission check combines two specific, well-defined permissions:
hasModifyNamespacePermission- verifies if the user has permission to modify the specific namespacehasModifyClusterPermission- verifies if the user has permission to modify the specific cluster
The implementation in PermissionValidator shows that these permissions are properly scoped:
- Namespace permission is checked against a specific namespace target ID (
appId + namespaceName) - Cluster permission is checked against a specific cluster target ID (
appId + env + clusterName)
Each permission type is independently validated against specific resource targets, making the OR condition a valid design choice that maintains proper access control granularity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for the PermissionValidator class implementation
ast-grep --pattern 'class PermissionValidator {
$$$
hasModifyNamespacePermission($$$) {
$$$
}
$$$
}'
# Also search for hasModifyClusterPermission
ast-grep --pattern 'class PermissionValidator {
$$$
hasModifyClusterPermission($$$) {
$$$
}
$$$
}'
# Look for the ItemController class to understand the context
ast-grep --pattern 'class ItemController {
$$$
}'
# Search for related test files
fd "PermissionValidatorTest|ItemControllerTest" --type f
Length of output: 546
Script:
#!/bin/bash
# Search for PermissionValidator implementation with context
rg -l "class PermissionValidator"
# Search for hasModifyNamespacePermission implementation
rg "hasModifyNamespacePermission.*\{" -A 10
# Search for hasModifyClusterPermission implementation
rg "hasModifyClusterPermission.*\{" -A 10
# Search for ItemController's method using this permission
rg -B 2 -A 5 "@PreAuthorize.*hasModifyNamespacePermission.*hasModifyClusterPermission"
Length of output: 4536
Script:
#!/bin/bash
# Search for PermissionType enum
rg "enum PermissionType" -A 10
# Search for RoleUtils implementation
rg "class RoleUtils" -A 20
# Search for the ItemController method using this permission to understand what operation it protects
rg -B 5 -A 10 "@PreAuthorize.*hasModifyNamespacePermission.*hasModifyClusterPermission" --type java
Length of output: 4910
Script:
#!/bin/bash
# Search for the ItemController class implementation focusing on the method with the permission
rg -B 10 -A 10 "class ItemController" --type java
# Search for PermissionType enum in a different way
find . -type f -exec grep -l "enum.*PermissionType" {} \;
# Search for the specific method in ItemController that uses this permission
rg -B 10 -A 10 "class ItemController" --type java | rg -B 10 -A 10 "@PreAuthorize.*hasModifyNamespacePermission"
Length of output: 17823
apollo-portal/src/main/resources/static/scripts/services/PermissionService.js (9)
45-48: Adequate usage alignment with existing naming.
has_cluster_permission follows a naming pattern similar to has_app_permission and has_namespace_permission, ensuring consistency throughout the service. Good job!
106-112: Role assignment is well structured.
Assigning cluster roles shares the same approach as assigning namespace roles, which aids maintainability. Good job referencing the same pattern.
113-115: Remove cluster role – usage caution.
The removal logic is sound. Double-check that front-end components have appropriate confirmation flows if these roles are business-critical.
132-145: Promise-based approach is consistent with existing code.
initClusterPermission neatly fits the existing promise pattern. Keep an eye out for potential usage of async/await in the future if the codebase transitions away from AngularJS conventions.
192-206: Maintain parallel structure with hasAppPermission.
hasClusterPermission fosters code reusability and standardization. Nicely done!
272-286: Cluster role assignment function.
This block aligns with established naming conventions. Great job on code clarity.
288-303: Symmetry between assign/remove cluster role.
It's excellent that you preserved symmetry in methods. This improves maintainability and developer onboarding.
309-311: Expose init_cluster_permission as a top-level method.
Including init_cluster_permission in the returned object is consistent with your existing approach for app/namespace permissions. Looks good!
447-476: Comprehensive methods for cluster-level checks.
Adding has_modify_cluster_permission, has_release_cluster_permission, etc., ensures your service covers all cluster operations thoroughly.
apollo-portal/src/main/resources/static/styles/common-style.css (1)
1192-1195: New .cluster-info-panel class.
Adding extra spacing around cluster information is a simple and effective visual enhancement. The choice of margin: 20px 0px aligns well with the existing layout.
apollo-portal/src/main/resources/static/config.html (1)
190-210: Improved cluster visibility with a dedicated panel.
- The newly added
<section class="panel cluster-info-panel">clarifies which environment and cluster the user is operating on. - The “Grant” button linking to
role.htmlis a convenient way for users to manage roles without navigating through multiple pages. - Verify that permission checks are consistently enforced downstream when your backend processes these cluster-based actions.
apollo-portal/src/main/resources/static/i18n/zh-CN.json (1)
364-372: LGTM! The translations for cluster permission management are clear and consistent.
The added translations accurately reflect the cluster permission management functionality and maintain consistency with the English version.
apollo-portal/src/main/resources/static/i18n/en.json (1)
364-372: LGTM! The translations for cluster permission management are clear and consistent.
The added translations accurately reflect the cluster permission management functionality and maintain consistency with the Chinese version.
| @@ -125,6 +125,7 @@ public List<App> findAppsByOwner(@RequestParam("owner") String owner, Pageable p | |||
| public App create(@Valid @RequestBody AppModel appModel) { | |||
|
|
|||
| App app = transformToApp(appModel); | |||
|
|
|||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider transactional boundaries
Depending on how createAppAndAddRolePermission is implemented, you may want to ensure that both app creation and role/permission assignment remain consistent within a single transactional boundary, preventing partial failures (e.g., app is created but permissions are not assigned).
| namespace.hasReleasePermission |= result.hasPermission; | ||
| if (namespace.branch) { | ||
| namespace.branch.hasReleasePermission = result.hasPermission; | ||
| namespace.branch.hasReleasePermission |= result.hasPermission; | ||
| } | ||
| }); | ||
| } | ||
| else { | ||
| //branch has same permission | ||
| namespace.hasReleasePermission = result.hasPermission; | ||
| namespace.hasReleasePermission |= result.hasPermission; | ||
| if (namespace.branch) { | ||
| namespace.branch.hasReleasePermission = result.hasPermission; | ||
| namespace.branch.hasReleasePermission |= result.hasPermission; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Confirm usage of bitwise-or shorthand for booleans
Using |= for boolean flags can be error-prone in JavaScript because bitwise operations convert booleans to integers internally. While this works as expected in many cases, consider using logical or (||=) or a standard assignment (= ... || ...) to avoid unintended side effects and enhance readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java (1)
81-82: Consider removing unused mock
PortalSettings portalSettingsis introduced as a mock but is currently unused. If you don't have immediate plans to test code usingportalSettings, consider removing it to keep the test suite lean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java(2 hunks)
🔇 Additional comments (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java (1)
26-26: Import looks good
The new import for PortalSettings is consistent with the introduction of the mock bean below.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/integration/ConfigControllerIntegrationTest.java (1)
352-352: Consider using a more robust synchronization approach instead of a fixed 2-second sleep.Relying on a fixed delay can make tests slower than necessary and may mask possible timing-related issues. A more robust approach is to use a synchronization mechanism (e.g.,
CountDownLatch) or a polling approach that confirms the messages have been fully processed before proceeding.Here is an example of how you could refactor the code to avoid an arbitrary sleep:
- TimeUnit.MILLISECONDS.sleep(2000); + // Example approach using CountDownLatch: + // CountDownLatch latch = new CountDownLatch(1); + // periodicSendMessage(..., stop, latch); + // latch.await(5, TimeUnit.SECONDS); // Wait for message processing or timeout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/integration/ConfigControllerIntegrationTest.java(1 hunks)
Converge namespace permission related interfaces into the same; Full trace name change ClusterRole to NamespaceInClusterRole likes.
fix unit test
fix hound js style check
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (19)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/CommitController.java (1)
54-54: Consider returning an HTTP 403 instead of an empty list for unauthorized access.Returning an empty list may obscure permission errors and lead to confusion. Instead, returning a 403 error (Forbidden) or some proper status code would more accurately indicate that the requested resource is disallowed.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java (1)
114-114: Consider unifying the repeated permission check.You have a repeated pattern:
if (permissionValidator.shouldHideConfigToCurrentUser(appId, env, clusterName, ...)) { namespaceBO.hideItems(); }It might be beneficial to extract this into a dedicated helper method to reduce duplication, improve clarity, and ensure consistency across the codebase.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/ClusterNamespaceRolesAssignedUsers.java (2)
22-22: Add class-level Javadoc for clarity.
A brief class-level Javadoc would be beneficial to explain the role of this data structure in the permission management system, enhancing understanding for future maintainers.
24-28: Consider using an enum or a well-defined constant for environment strings.
Using an enum for environments (DEV, TEST, PROD, etc.) or a known constant class eliminates potential for typos and consolidates environment-related checks.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java (1)
Line range hint
72-190: Document the new cluster-based permission model.The changes consistently implement cluster-level authorization across all release operations. To ensure proper usage and maintenance:
- Consider adding Javadoc explaining the cluster-based permission model
- Update API documentation to reflect the new authorization requirements
- Consider creating a migration guide for existing clients
Would you like me to help draft the documentation for the new permission model?
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java (2)
47-50: Enhance test setup reliabilityThe setup method could be more robust:
- Add verification that role initialization was successful
- Consider adding pre-test cleanup to ensure a clean state
- Consider using
@Sqlfor both before and after test execution@Before public void setUp() { + // Clean up any existing roles first + roleInitializationService.cleanupRoles(appId, env, clusterName); roleInitializationService.initClusterNamespaceRoles(appId, env, clusterName, "apollo"); + // Verify initialization + assertTrue(roleInitializationService.isClusterNamespaceRoleInitialized(appId, env, clusterName)); }
56-58: Extract HTTP headers to constantsConsider extracting the content-type header to a constant or utility method for reusability and maintainability.
+ private static final String CONTENT_TYPE = "Content-Type"; + private static final String APPLICATION_JSON = "application/json"; + HttpHeaders headers = new HttpHeaders(); - headers.set("Content-Type", "application/json"); + headers.set(CONTENT_TYPE, APPLICATION_JSON);apollo-portal/src/main/resources/static/scripts/controller/role/ClusterNamespaceRoleController.js (3)
17-21: Ensure consistent naming convention.The controller name
ClusterNamespaceRoleControlleris clear. However, confirm that similar controllers across the codebase follow the same naming pattern (e.g.,<Context>Controller) for consistency.
36-41: Handle initialization errors comprehensively.Currently, the error callback only displays a warning. Consider adding a more detailed error-handling routine, such as automatically retrying, redirecting, or logging for observability.
153-162: Consider early return or short-circuit logic.
removeUserFromListiterates over the entire list to find an index. If the user is discovered early, consider returning immediately. This reduces time complexity when the user is found at earlier indices.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/RoleType.java (1)
32-33: KeepisValidRoleTypeupdated.Be mindful of future role additions and update
isValidRoleTypeaccordingly. Consider documenting the need to maintain this method if new role types are introduced.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
97-99: Generalize role name construction if needed.
buildClusterRoleNameis straightforward. If further cluster-based role strings are introduced, consider centralizing them in a single place to reduce repetitive patterns.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java (2)
177-189: LGTM! Consider adding audit logging.The implementation correctly follows the established pattern for role initialization and maintains transactional integrity. The method properly checks for existing roles before creation to avoid duplicates.
Consider adding audit logging similar to other role initialization methods to track who creates these roles and when.
246-255: Add input parameter validation.The implementation is consistent with other role creation methods, but could benefit from explicit parameter validation.
Consider adding
RequestPrecondition.checkArgumentsNotEmpty()checks for all parameters at the beginning of the method:private void createClusterRole(String appId, String env, String clusterName, String permissionType, String roleName, String operator) { + RequestPrecondition.checkArgumentsNotEmpty(appId, env, clusterName, permissionType, roleName, operator); Permission permission = createPermission(RoleUtils.buildClusterTargetId(appId, env, clusterName), permissionType, operator); Permission createdPermission = rolePermissionService.createPermission(permission); Role role = createRole(roleName, operator); rolePermissionService .createRoleWithPermissions(role, Sets.newHashSet(createdPermission.getId())); }apollo-portal/src/main/resources/static/scripts/services/PermissionService.js (1)
272-303: Standardize error handling messages.The implementation follows the established patterns for role management, but could benefit from standardized error messages.
Consider creating constants for error messages to maintain consistency across the application:
const ERROR_MESSAGES = { ROLE_ASSIGNMENT_FAILED: 'Failed to assign role to user', ROLE_REMOVAL_FAILED: 'Failed to remove role from user' };apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java (4)
83-87: Add API documentation.The endpoint implementation is correct but lacks documentation.
Add Swagger/OpenAPI documentation to describe the endpoint, its parameters, and possible responses:
+ /** + * Initialize permissions for a cluster namespace. + * + * @param appId The application ID + * @param env The environment + * @param clusterName The cluster name + * @return ResponseEntity<Void> with status 200 on success + */ @PostMapping("/apps/{appId}/envs/{env}/clusters/{clusterName}/initNsPermission") public ResponseEntity<Void> initClusterNamespacePermission(@PathVariable String appId, @PathVariable String env, @PathVariable String clusterName) {
123-133: Consider adding caching for permission checks.The implementation is correct and consistent with existing patterns.
Consider adding caching to improve performance of frequent permission checks:
+ @Cacheable(value = "cluster-namespace-permissions", key = "#appId + '-' + #env + '-' + #clusterName + '-' + #permissionType") @GetMapping("/apps/{appId}/envs/{env}/clusters/{clusterName}/ns_permissions/{permissionType}") public ResponseEntity<PermissionCondition> hasClusterNamespacePermission(@PathVariable String appId, @PathVariable String env, @PathVariable String clusterName, @PathVariable String permissionType) {
237-259: Add audit logging for role management operations.The implementation has proper security controls and input validation.
Add audit logging annotations to track role changes:
@PreAuthorize(value = "@permissionValidator.hasAssignRolePermission(#appId)") @PostMapping("/apps/{appId}/envs/{env}/clusters/{clusterName}/ns_roles/{roleType}") + @ApolloAuditLog(type = OpType.CREATE, name = "Auth.assignClusterNamespaceRoleToUser") public ResponseEntity<Void> assignClusterNamespaceRoleToUser(...) @PreAuthorize(value = "@permissionValidator.hasAssignRolePermission(#appId)") @DeleteMapping("/apps/{appId}/envs/{env}/clusters/{clusterName}/ns_roles/{roleType}") + @ApolloAuditLog(type = OpType.DELETE, name = "Auth.removeClusterNamespaceRoleFromUser") public ResponseEntity<Void> removeClusterNamespaceRoleFromUser(...)Also applies to: 261-277
213-235: Add pagination support for role user retrieval.The implementation correctly retrieves and validates role assignments.
Consider adding pagination support to handle large numbers of users efficiently:
@GetMapping("/apps/{appId}/envs/{env}/clusters/{clusterName}/ns_role_users") - public ClusterNamespaceRolesAssignedUsers getClusterNamespaceRoles(...) + public ClusterNamespaceRolesAssignedUsers getClusterNamespaceRoles( + @PathVariable String appId, + @PathVariable String env, + @PathVariable String clusterName, + @RequestParam(defaultValue = "0") int page, + @RequestParam(defaultValue = "20") int size)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/PermissionType.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/RoleType.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/CommitController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java(8 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseHistoryController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/ClusterNamespaceRolesAssignedUsers.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ClusterService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RoleInitializationService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java(4 hunks)apollo-portal/src/main/resources/static/cluster/role.html(1 hunks)apollo-portal/src/main/resources/static/scripts/controller/role/ClusterNamespaceRoleController.js(1 hunks)apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js(2 hunks)apollo-portal/src/main/resources/static/scripts/services/PermissionService.js(7 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RoleInitializationServiceTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RoleInitializationService.java
- apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/PermissionType.java
- apollo-portal/src/main/resources/static/cluster/role.html
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ClusterService.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RoleInitializationServiceTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java
🔇 Additional comments (23)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java (1)
128-128: Double-check null-safety for namespaceBO and its base info.
While you already check that namespaceBO != null, consider whether namespaceBO.getBaseInfo() might be null under any edge cases. If that's a possibility, you may need an additional check to avoid a potential NullPointerException.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java (1)
81-81: Good addition but ensure consistent application in other permission checks
Including #clusterName in the @PreAuthorize annotation allows for more granular access control. However, please confirm that all relevant permission checks in related controllers (e.g., import or modify operations) are updated similarly to maintain consistency throughout the codebase.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java (4)
101-101: Verify cluster vs branch name usage in permission check.
While the permission check is correctly updated, note that the method uses branchName as the clusterName in the model (line 108). Ensure this aligns with the intended permission model for gray releases.
Consider documenting the relationship between cluster and branch names in the method's documentation to clarify the permission model for gray releases.
149-149: LGTM! Consistent permission check implementation.
Both read methods correctly implement the new cluster-aware permission model while maintaining the secure approach of returning empty lists for unauthorized access.
Also applies to: 164-164
190-190: LGTM! Verify rollback permission checks in integration tests.
The permission check correctly uses cluster information from the release object. Since rollback is a critical operation, ensure comprehensive integration testing of the new permission model.
Run the following script to check for existing integration tests:
72-72: LGTM! Verify permission validator implementation.
The permission check now correctly includes cluster-level authorization, aligning with the feature requirements.
Run the following script to verify the permission validator implementation:
✅ Verification successful
Permission validation implementation is correctly handling cluster-level authorization
The implementation in PermissionValidator properly validates cluster-level permissions through a hierarchical check:
- Checks app-level namespace release permission
- Checks environment-specific namespace release permission
- Checks cluster-specific release permission using
hasReleaseNamespaceInClusterPermission
The permission target IDs are correctly built using RoleUtils:
- Namespace permission:
appId:namespaceName - Environment-specific:
appId:namespaceName:env - Cluster-specific:
appId:env:clusterName
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of hasReleaseNamespacePermission method
# to ensure it properly handles the new cluster parameter
ast-grep --pattern 'hasReleaseNamespacePermission($appId, $env, $clusterName, $namespaceName)'
Length of output: 96
Script:
#!/bin/bash
# Let's try a different approach to find the permission validator implementation
# First, let's find the validator class
fd "PermissionValidator" --type f
# Then let's search for the method implementation using ripgrep for better context
rg "hasReleaseNamespacePermission" -A 5
Length of output: 16240
Script:
#!/bin/bash
# Let's check the RoleUtils to understand how the cluster-based permission target is built
rg "buildNamespaceTargetId|buildClusterTargetId" -A 3
# Also check for any cluster-specific permission types
rg "PermissionType\." -A 1
Length of output: 23146
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java (2)
1-107: Verify test uniqueness in the codebase
Let's ensure this test implementation doesn't duplicate existing test coverage.
✅ Verification successful
Based on the search results, I can provide the final response:
Test implementation is unique and well-placed
The search results confirm this test implementation is unique in the codebase:
- While there are other tests involving
RoleInitializationService(inRoleInitializationServiceTest.java), they focus on unit testing the service itself - The
testClusterNamespaceRoleLifeCyclemethod name is unique and no similar role lifecycle tests exist - This is the only integration test focusing on the permission controller's role management endpoints
This test provides valuable coverage of the role assignment workflow through the REST API layer, complementing the unit tests in RoleInitializationServiceTest.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar role lifecycle tests
rg -l "RoleInitializationService|ClusterNamespaceRoles" | xargs rg "class.*Test" -C 5
# Search for similar test method names
rg "test.*Role.*Cycle" -C 5
Length of output: 7444
35-36: Verify the necessity of skipping authorization in this test
The @ActiveProfiles("skipAuthorization") annotation seems counterintuitive for testing permission management functionality. This might bypass critical authorization checks that should be validated.
Let's check how other auth-related tests handle this:
✅ Verification successful
Let me gather more information about the authorization setup and test configuration.
Let me check the configuration for the skipAuthorization profile.
Let me check the test base class and the skipAuthorization configuration.
Let me check the actual test implementation and what permissions are being tested.
The skipAuthorization profile is correctly used in this test
The @ActiveProfiles("skipAuthorization") annotation is actually appropriate here because:
- The test is focused on verifying the lifecycle of cluster namespace roles (assignment, verification, and removal) through the REST API endpoints, not the authorization mechanism itself.
- The
SkipAuthorizationConfigurationprovides mock implementations that:- Grant superadmin access to bypass pre-authorization checks
- Allow the test to focus on the core role management functionality
- The test properly validates the complete role management flow:
- Initial state (no roles)
- Role assignment
- Role verification
- Role removal
The authorization skipping is intentional and doesn't compromise the test's validity, as it's testing the role management functionality, not the authorization system itself.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other auth-related test files and their profile configurations
rg -l "class.*Test.*Auth" | xargs rg "@ActiveProfiles" -C 5
Length of output: 6120
Script:
#!/bin/bash
# Check for skipAuthorization profile configuration
rg -l "skipAuthorization" | xargs rg "skipAuthorization|@Profile" -C 5
# Check for permission-related test configurations
rg -l "Permission.*Test" | xargs rg "@ActiveProfiles|@Test" -C 5
Length of output: 33400
Script:
#!/bin/bash
# Check the skipAuthorization configuration and its purpose
rg -l "SkipAuthorizationConfiguration" | xargs rg -C 10
# Check the actual test implementation to understand what's being tested
cat apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java
Length of output: 4721
Script:
#!/bin/bash
# Check the AbstractIntegrationTest class
cat apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/AbstractIntegrationTest.java
# Check for any skipAuthorization configuration files
rg -g "*.java" -l "skipAuthorization" | xargs rg "class.*Configuration" -C 5
Length of output: 3436
Script:
#!/bin/bash
# Check the SkipAuthorizationConfiguration implementation
cat apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java
# Check what's being tested in the permission controller
rg -g "*.java" "class.*PermissionController" -A 10
Length of output: 5749
apollo-portal/src/main/resources/static/scripts/controller/role/ClusterNamespaceRoleController.js (1)
58-87: Check for null references and validate user input.
Within assignRoleToUser, ensure that roleType, env, and clusterName are non-null and valid before proceeding with the role assignment. While you do check user, you might also want to handle unexpected or undefined role types gracefully.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/RoleType.java (1)
27-30: New constants look good.
The addition of MODIFY_NAMESPACE_IN_CLUSTER and RELEASE_NAMESPACE_IN_CLUSTER aligns well with the existing naming conventions.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (3)
69-71: Naming consistency.
buildModifyNamespaceInClusterRoleName follows the patterns of existing methods. This approach is consistent and clear for building role names.
85-87: Ensure release role naming alignment.
buildReleaseNamespaceInClusterRoleName is consistent with the buildModifyNamespaceInClusterRoleName approach. No issues noted.
113-115: Reconfirm delimiter usage.
buildClusterTargetId uses ConfigConsts.CLUSTER_NAMESPACE_SEPARATOR. Ensure that any external references to these role/target strings also use the same delimiter to prevent mismatches.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.java (9)
51-60: Visibility changes for hasModifyNamespacePermission.
Turning this method private is appropriate to limit external usage. Confirm any existing references outside this class have been updated accordingly.
63-67: Implementation fulfills new cluster namespace permission requirement.
hasModifyNamespaceInClusterPermission is a good addition. Ensure that an equivalent unit test suite checks whether these new permission checks function correctly.
69-80: Layered checks are effective.
hasModifyNamespacePermission(...) unifies three different permission checks in a short-circuit manner, which is efficient. Keep an eye on potential performance concerns if further checks are added in the future.
82-93: Visibility changes for hasReleaseNamespacePermission.
Similar to hasModifyNamespacePermission, ensure external references are updated for these newly private methods.
94-97: Consistent approach for release permissions.
hasReleaseNamespaceInClusterPermission parallels hasModifyNamespaceInClusterPermission.
100-110: Comprehensive release checks.
hasReleaseNamespacePermission uses short-circuit logic as well. Current approach is readable and maintainable.
113-114: Potential confusion in hasDeleteNamespacePermission.
hasDeleteNamespacePermission simply returns hasAssignRolePermission(appId) || isSuperAdmin(). Confirm that no dedicated "delete" role is required.
117-119: Combined operation check.
hasOperateNamespacePermission merges modify and release checks. This may help reduce duplication. It appears consistent with the new cluster-based checks.
160-161: Maintain clarity in shouldHideConfigToCurrentUser.
The extra parameters (env, clusterName) match new cluster permission logic. Confirm that all callers of shouldHideConfigToCurrentUser are updated to supply the cluster name.
Also applies to: 174-174
apollo-portal/src/main/resources/static/scripts/services/PermissionService.js (1)
26-32: LGTM! Resource configurations are well-defined.
The new resource configurations follow RESTful conventions and maintain consistency with existing patterns.
Also applies to: 45-48
| @@ -51,7 +51,7 @@ public List<CommitDTO> find(@PathVariable String appId, @PathVariable String env | |||
| @RequestParam(required = false) String key, | |||
| @Valid @PositiveOrZero(message = "page should be positive or 0") @RequestParam(defaultValue = "0") int page, | |||
| @Valid @Positive(message = "size should be positive number") @RequestParam(defaultValue = "10") int size) { | |||
| if (permissionValidator.shouldHideConfigToCurrentUser(appId, env, namespaceName)) { | |||
| if (permissionValidator.shouldHideConfigToCurrentUser(appId, env, clusterName, namespaceName)) { | |||
There was a problem hiding this comment.
Validate clusterName to ensure it follows expected naming conventions.
If clusterName can be user-provided, add validation to avoid potential naming conflicts or injection issues. This can be done either in the @PathVariable binding (e.g., using a pattern constraint) or at the service/validator layer.
| @Override | ||
| public String toString() { | ||
| return "ClusterRolesAssignedUsers{" + | ||
| "appId='" + appId + '\'' + | ||
| ", env='" + env + '\'' + | ||
| ", cluster='" + cluster + '\'' + | ||
| ", modifyRoleUsers=" + modifyRoleUsers + | ||
| ", releaseRoleUsers=" + releaseRoleUsers + | ||
| '}'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the mismatch in the toString method.
The returned string includes "ClusterRolesAssignedUsers", whereas the class name is ClusterNamespaceRolesAssignedUsers.
Apply this diff to resolve the mismatch:
- return "ClusterRolesAssignedUsers{" +
+ return "ClusterNamespaceRolesAssignedUsers{" +📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public String toString() { | |
| return "ClusterRolesAssignedUsers{" + | |
| "appId='" + appId + '\'' + | |
| ", env='" + env + '\'' + | |
| ", cluster='" + cluster + '\'' + | |
| ", modifyRoleUsers=" + modifyRoleUsers + | |
| ", releaseRoleUsers=" + releaseRoleUsers + | |
| '}'; | |
| } | |
| @Override | |
| public String toString() { | |
| return "ClusterNamespaceRolesAssignedUsers{" + | |
| "appId='" + appId + '\'' + | |
| ", env='" + env + '\'' + | |
| ", cluster='" + cluster + '\'' + | |
| ", modifyRoleUsers=" + modifyRoleUsers + | |
| ", releaseRoleUsers=" + releaseRoleUsers + | |
| '}'; | |
| } |
| @Test | ||
| @Sql(scripts = "/sql/cleanup.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD) | ||
| public void testClusterNamespaceRoleLifeCycle() { | ||
|
|
||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers.set("Content-Type", "application/json"); | ||
| HttpEntity<String> entity = new HttpEntity<>(user, headers); | ||
|
|
||
| // check role not assigned | ||
| ResponseEntity<ClusterNamespaceRolesAssignedUsers> beforeAssign = restTemplate.getForEntity( | ||
| url("/apps/{appId}/envs/{env}/clusters/{clusterName}/ns_role_users"), | ||
| ClusterNamespaceRolesAssignedUsers.class, appId, env, clusterName); | ||
| assertEquals(200, beforeAssign.getStatusCodeValue()); | ||
| ClusterNamespaceRolesAssignedUsers body = beforeAssign.getBody(); | ||
| assertNotNull(body); | ||
| assertEquals(appId, body.getAppId()); | ||
| assertEquals(env, body.getEnv()); | ||
| assertEquals(clusterName, body.getCluster()); | ||
| assertTrue(body.getModifyRoleUsers() == null || body.getModifyRoleUsers().isEmpty()); | ||
|
|
||
| // assign role to user | ||
| restTemplate.postForEntity( | ||
| url("/apps/{appId}/envs/{env}/clusters/{clusterName}/ns_roles/{roleType}"), entity, Void.class, | ||
| appId, env, clusterName, roleType); | ||
|
|
||
| // check role assigned | ||
| ResponseEntity<ClusterNamespaceRolesAssignedUsers> afterAssign = restTemplate.getForEntity( | ||
| url("/apps/{appId}/envs/{env}/clusters/{clusterName}/ns_role_users"), | ||
| ClusterNamespaceRolesAssignedUsers.class, appId, env, clusterName); | ||
| assertEquals(200, afterAssign.getStatusCodeValue()); | ||
| body = afterAssign.getBody(); | ||
| assertNotNull(body); | ||
| assertEquals(appId, body.getAppId()); | ||
| assertEquals(env, body.getEnv()); | ||
| assertEquals(clusterName, body.getCluster()); | ||
| assertTrue( | ||
| body.getModifyRoleUsers().stream().anyMatch(userInfo -> userInfo.getUserId().equals(user))); | ||
|
|
||
| // remove role from user | ||
| restTemplate.delete( | ||
| url("/apps/{appId}/envs/{env}/clusters/{clusterName}/ns_roles/{roleType}?user={user}"), appId, | ||
| env, clusterName, roleType, user); | ||
|
|
||
| // check role removed | ||
| ResponseEntity<ClusterNamespaceRolesAssignedUsers> afterRemove = restTemplate.getForEntity( | ||
| url("/apps/{appId}/envs/{env}/clusters/{clusterName}/ns_role_users"), | ||
| ClusterNamespaceRolesAssignedUsers.class, appId, env, clusterName); | ||
| assertEquals(200, afterRemove.getStatusCodeValue()); | ||
| body = afterRemove.getBody(); | ||
| assertNotNull(body); | ||
| assertEquals(appId, body.getAppId()); | ||
| assertEquals(env, body.getEnv()); | ||
| assertEquals(clusterName, body.getCluster()); | ||
| assertTrue(body.getModifyRoleUsers() == null || body.getModifyRoleUsers().isEmpty()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional scenarios
The test covers the happy path but should also verify:
- Error scenarios (e.g., invalid role types, non-existent users)
- Response headers and content types
- Concurrent role assignments
- Edge cases (e.g., removing non-existent roles)
Consider adding these test methods:
@Test
public void testInvalidRoleType() {
// Test assigning invalid role type
}
@Test
public void testNonExistentUser() {
// Test operations with non-existent user
}
@Test
public void testConcurrentRoleAssignment() {
// Test concurrent role assignments
}Also, enhance the existing assertions:
ResponseEntity<ClusterNamespaceRolesAssignedUsers> afterAssign = restTemplate.getForEntity(...);
assertEquals(200, afterAssign.getStatusCodeValue());
+assertEquals(APPLICATION_JSON, afterAssign.getHeaders().getContentType().toString());
body = afterAssign.getBody();
assertNotNull(body);| @@ -48,7 +48,7 @@ public List<ReleaseHistoryBO> findReleaseHistoriesByNamespace(@PathVariable Stri | |||
| @RequestParam(value = "page", defaultValue = "0") int page, | |||
| @RequestParam(value = "size", defaultValue = "10") int size) { | |||
|
|
|||
| if (permissionValidator.shouldHideConfigToCurrentUser(appId, env, namespaceName)) { | |||
| if (permissionValidator.shouldHideConfigToCurrentUser(appId, env, clusterName, namespaceName)) { | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the code analysis, I can now provide a final response about the environment validation concern.
The environment validation is already properly handled in the codebase
The codebase has a robust environment validation mechanism through the Env.transformEnv() method which safely handles invalid environment strings by returning Env.UNKNOWN. This method is the recommended way to validate environments instead of using Env.valueOf() directly.
- In
ReleaseHistoryController.java, consider replacing:
if (permissionValidator.shouldHideConfigToCurrentUser(appId, env, clusterName, namespaceName))with:
Env targetEnv = Env.transformEnv(env);
if (permissionValidator.shouldHideConfigToCurrentUser(appId, targetEnv.toString(), clusterName, namespaceName))This change would ensure invalid environment values are handled gracefully without throwing exceptions.
🔗 Analysis chain
Verify environment validity and handle potential errors.
This call to Env.valueOf(env) might throw an IllegalArgumentException if the env string is invalid. Consider validating the environment string or handling exceptions to mitigate unexpected errors.
Below is a script to track occurrences of Env.valueOf calls in the codebase and confirm they are safeguarded with proper checks:
Let me analyze the Env.valueOf usage in the codebase to verify the concern about environment validity and error handling.
Let me try a different approach to analyze the Env class implementation.
Let me try one more search to see the implementation of the transformEnv method which seems to handle environment validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all Env.valueOf calls and display surrounding lines for context
rg -A 5 $'Env.valueOf'
Length of output: 73974
Script:
#!/bin/bash
# Search for the Env class implementation to understand error handling
ast-grep --pattern 'class Env {
$$$
}'
Length of output: 42
Script:
#!/bin/bash
# Search for the Env class implementation using ripgrep
rg -A 20 "^public.*class Env"
Length of output: 15758
Script:
#!/bin/bash
# Search for transformEnv implementation
rg -A 10 "transformEnv.*String.*\{"
Length of output: 3207
| var removeModifyNamespaceInClusterRoleFunc = function (appId, namespaceName, user) { | ||
| return PermissionService.remove_modify_cluster_ns_role(appId, namespaceName, user); | ||
| }; |
There was a problem hiding this comment.
Fix mismatch in function signature vs. invocation.
The function removeModifyNamespaceInClusterRoleFunc accepts (appId, namespaceName, user) but is called with (appId, env, clusterName, user). This will break the role removal logic. Update the function signature or the invocation to match the required parameters.
- var removeModifyNamespaceInClusterRoleFunc = function (appId, namespaceName, user) {
- return PermissionService.remove_modify_cluster_ns_role(appId, namespaceName, user);
- };
+ var removeModifyNamespaceInClusterRoleFunc = function (appId, env, clusterName, user) {
+ return PermissionService.remove_modify_cluster_ns_role(appId, env, clusterName, user);
+ };Also applies to: 139-144
What's the purpose of this PR
支持按集群给用户授予管理权限（修改、发布配置）
Which issue(s) this PR fixes:
Fixes:
Related
Brief changelog
see git commits
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
Based on the comprehensive changes, here are the release notes:
New Features
Improvements
UI Updates
Security Enhancements