Feat(Auth): Authorization by Env Cluster #5302
Conversation
dosubot
bot
added
the
size:XXL
WalkthroughThis pull request introduces comprehensive enhancements to the Apollo configuration portal's permission management system, focusing on cluster-level authorization across different environments. The changes enable more granular control over namespace permissions within clusters, allowing administrators to assign modify and release permissions at the cluster level. New methods, constants, and controllers have been added to facilitate this functionality, alongside updates to existing permission checks across various components. Changes
Assessment against linked issues
Suggested Labels
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.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Outdated
Show resolved
Hide resolved
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)
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
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/CommitController.java
Show resolved
Hide resolved
...ain/java/com/ctrip/framework/apollo/portal/entity/vo/ClusterNamespaceRolesAssignedUsers.java
Outdated
Show resolved
Hide resolved
...tal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java
Show resolved
Hide resolved
...tal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseHistoryController.java
Show resolved
Hide resolved
...o-portal/src/main/resources/static/scripts/controller/role/ClusterNamespaceRoleController.js
Outdated
Show resolved
Hide resolved
Converge namespace permission related interfaces into the same; Full trace name change ClusterRole to NamespaceInClusterRole likes.
fix unit test
fix hound js style check
nobodyiam
force-pushed
the
feat-cluster-authorize-by-env
branch
from
a37dc2f to
071b199
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (13)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RoleInitializationServiceTest.java (3)
149-167: LGTM! Comprehensive test for role initialization.The test effectively covers the base case where both roles need to be created. Consider adding a verification for the correct permission target ID format to ensure cluster-specific permissions are created correctly.
Add verification for permission target ID:
verify(rolePermissionService, times(2)).createPermission(any()); +verify(rolePermissionService, times(1)).createPermission(argThat(permission -> + permission.getTargetId().equals(RoleUtils.buildNamespaceTargetId(APP_ID, ENV, CLUSTER)) +));
209-227: LGTM! Complete test coverage matrix.The test effectively covers the opposite partial initialization scenario. Consider adding a brief Javadoc comment to document the test matrix coverage across these four test methods.
Add documentation to clarify test coverage:
+/** + * Test matrix for cluster namespace role initialization: + * 1. testInitClusterNsRole: Both roles don't exist + * 2. testInitClusterNsRoleHasExisted: Both roles exist + * 3. testInitClusterNsRoleModifyNamespaceInClusterExisted: Only modify role exists + * 4. testInitClusterNsRoleReleaseNamespaceInClusterExisted: Only release role exists + */ @Test public void testInitClusterNsRoleReleaseNamespaceInClusterExisted() {
148-227: Excellent test coverage for cluster-based authorization feature.The test suite comprehensively covers all possible scenarios for cluster namespace role initialization, which is crucial for the new environment cluster-based authorization feature. The systematic approach to testing different role combinations ensures robust permission management.
Consider adding integration tests to verify the interaction between
RoleInitializationServiceandPermissionControllerfor the complete authorization flow.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java (2)
177-189: Add input validation and documentation for the new cluster roles initialization.The method implementation looks good but could benefit from some improvements:
- Add JavaDoc to explain the purpose and behavior of this method
- Add parameter validation for appId, env, and clusterName
Consider adding the following improvements:
+ /** + * Initializes roles for managing namespaces within a specific cluster. + * Creates two roles if they don't exist: + * 1. Modify namespace in cluster role + * 2. Release namespace in cluster role + * + * @param appId The application id + * @param env The environment name + * @param clusterName The cluster name + * @param operator The user who performs this operation + * @throws IllegalArgumentException if any parameter is null or empty + */ @Transactional @Override public void initClusterNamespaceRoles(String appId, String env, String clusterName, String operator) { + Preconditions.checkArgument(StringUtils.isNotBlank(appId), "AppId cannot be blank"); + Preconditions.checkArgument(StringUtils.isNotBlank(env), "Env cannot be blank"); + Preconditions.checkArgument(StringUtils.isNotBlank(clusterName), "ClusterName cannot be blank"); + Preconditions.checkArgument(StringUtils.isNotBlank(operator), "Operator cannot be blank"); + String modifyNamespaceInClusterRoleName = RoleUtils.buildModifyNamespaceInClusterRoleName(appId, env, clusterName);
246-255: Enhance error handling in createClusterRole method.The implementation follows the established pattern but could benefit from better error handling:
- Handle potential failures during permission creation
- Consider adding logging for debugging purposes
Consider enhancing the error handling:
private void createClusterRole(String appId, String env, String clusterName, String permissionType, String roleName, String operator) { + try { Permission permission = createPermission(RoleUtils.buildClusterTargetId(appId, env, clusterName), permissionType, operator); Permission createdPermission = rolePermissionService.createPermission(permission); + if (createdPermission == null) { + throw new ServiceException("Failed to create permission for cluster role: " + roleName); + } Role role = createRole(roleName, operator); rolePermissionService .createRoleWithPermissions(role, Sets.newHashSet(createdPermission.getId())); + } catch (Exception e) { + logger.error("Failed to create cluster role: {}", roleName, e); + throw new ServiceException("Failed to create cluster role: " + roleName, e); + } }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.java (1)
94-110: Consider reducing code duplication in permission checks.The modify and release permission check implementations share identical structure. Consider extracting the common logic into a reusable method.
+private boolean hasPermissionInHierarchy(String appId, String env, String clusterName, String namespaceName, + Function<String, Boolean> namespaceCheck, + Function<String, Boolean> envCheck, + Function<String, Boolean> clusterCheck) { + if (namespaceCheck.apply(namespaceName)) { + return true; + } + if (envCheck.apply(env)) { + return true; + } + if (clusterCheck.apply(clusterName)) { + return true; + } + return false; +}apollo-portal/src/main/resources/static/scripts/services/PermissionService.js (1)
272-303: Consider reducing promise handling duplication.The role management functions contain repeated promise handling code. Consider extracting a utility function to reduce duplication.
+function wrapPromise(resourceCall) { + var d = $q.defer(); + resourceCall( + function (result) { + d.resolve(result); + }, + function (result) { + d.reject(result); + } + ); + return d.promise; +}apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java (2)
186-189: Consider enhancing the error message clarityWhile the permission check is correct, the error message could be more specific about which permission is missing (cluster vs. namespace level).
- diff.setExtInfo("You are not this project's administrator, nor you have edit or release permission for the namespace:" + namespace); + diff.setExtInfo("You do not have sufficient permissions for the namespace in cluster " + namespace.getClusterName() + ": " + namespace);
200-220: Consider optimizing permission check loopWhile the permission checking logic is correct, it could be more efficient using Java streams and early return pattern.
- NamespaceIdentifier noPermissionNamespace = null; - // check if user has every namespace's ModifyNamespace permission - boolean hasPermission = true; - for (NamespaceIdentifier namespaceIdentifier : model.getSyncToNamespaces()) { - // once user has not one of the namespace's ModifyNamespace permission, then break the loop - hasPermission = permissionValidator.hasModifyNamespacePermission( - namespaceIdentifier.getAppId(), - namespaceIdentifier.getEnv().getName(), - namespaceIdentifier.getClusterName(), - namespaceIdentifier.getNamespaceName() - ); - if (!hasPermission) { - noPermissionNamespace = namespaceIdentifier; - break; - } - } - if (hasPermission) { - configService.syncItems(model.getSyncToNamespaces(), model.getSyncItems()); - return ResponseEntity.status(HttpStatus.OK).build(); - } - throw new AccessDeniedException(String.format("You don't have the permission to modify namespace: %s", noPermissionNamespace)); + Optional<NamespaceIdentifier> noPermissionNamespace = model.getSyncToNamespaces().stream() + .filter(namespace -> !permissionValidator.hasModifyNamespacePermission( + namespace.getAppId(), + namespace.getEnv().getName(), + namespace.getClusterName(), + namespace.getNamespaceName())) + .findFirst(); + + if (noPermissionNamespace.isPresent()) { + throw new AccessDeniedException( + String.format("You don't have the permission to modify namespace: %s", noPermissionNamespace.get())); + } + + configService.syncItems(model.getSyncToNamespaces(), model.getSyncItems()); + return ResponseEntity.status(HttpStatus.OK).build();apollo-portal/src/main/resources/static/cluster/role.html (4)
21-21: Consider adding additional security headersWhile the character encoding is properly declared, consider adding security-related meta tags to enhance protection against common web vulnerabilities:
+ <meta http-equiv="X-Content-Type-Options" content="nosniff"> + <meta http-equiv="X-Frame-Options" content="DENY"> + <meta http-equiv="Content-Security-Policy" content="default-src 'self';">
77-87: Improve user list accessibility and maintainabilityThe user list buttons lack proper accessibility attributes and have duplicated code. Consider:
- Adding ARIA labels and roles
- Extracting the common structure into a reusable component
- Adding confirmation dialog for role removal
Here's a suggested improvement:
+ <div class="item-container" role="list" aria-label="{{roleTitle}} {{'Common.AssignedUsers' | translate}}"> + <div class="btn-group item-info" role="listitem" + ng-repeat="user in users" + aria-label="{{'Common.User' | translate}}: {{user.userId}}"> + <button type="button" class="btn btn-default" + ng-bind="user.userId" + aria-label="{{user.userId}}"> + </button> + <button type="button" class="btn btn-default dropdown-toggle" + data-toggle="dropdown" + aria-label="{{'Common.Remove' | translate}} {{user.userId}}" + ng-click="confirmRemoval(roleType, user.userId)"> + <span class="glyphicon glyphicon-remove" aria-hidden="true"></span> + </button> + </div> + </div>Also applies to: 108-118
128-130: Enhance the no-permission messageThe current message could be more helpful by providing guidance on how to obtain the necessary permissions.
- <div class="panel-body text-center" ng-show="!hasAssignUserPermission"> - <h2>{{'Cluster.Role.NoPermission' | translate }}</h2> - </div> + <div class="panel-body text-center" ng-show="!hasAssignUserPermission"> + <h2>{{'Cluster.Role.NoPermission' | translate }}</h2> + <p class="text-muted"> + {{'Cluster.Role.NoPermission.Help' | translate }} + <a href="{{helpDocUrl}}">{{'Common.LearnMore' | translate }}</a> + </p> + </div>
137-170: Optimize script loading for better performanceConsider the following improvements for script loading:
- Add integrity hashes for third-party scripts
- Use async/defer attributes where appropriate
- Consider bundling and minifying custom scripts
Example for third-party scripts:
- <script src="../vendor/angular/angular.min.js"></script> + <script src="../vendor/angular/angular.min.js" + integrity="sha384-[hash]" + crossorigin="anonymous"></script>For custom scripts:
- <script type="application/javascript" src="../scripts/app.js"></script> - <script type="application/javascript" src="../scripts/services/AppService.js"></script> + <script type="application/javascript" src="../scripts/bundle.min.js" defer></script>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/integration/ConfigControllerIntegrationTest.java(1 hunks)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/ClusterController.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/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/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/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/service/AppServiceTest.java(2 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 (28)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java
- apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/integration/ConfigControllerIntegrationTest.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RoleInitializationService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/PermissionType.java
- apollo-portal/src/main/resources/static/styles/common-style.css
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/AppServiceTest.java
- apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/ControllableAuthorizationConfiguration.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseHistoryController.java
- apollo-portal/src/main/resources/static/config.html
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/CommitController.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/RoleType.java
- apollo-portal/src/main/resources/static/scripts/controller/role/ClusterNamespaceRoleController.js
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ClusterService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/ClusterNamespaceRolesAssignedUsers.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.java
- apollo-portal/src/main/resources/static/i18n/en.json
- apollo-portal/src/main/resources/static/i18n/zh-CN.json
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java
🔇 Additional comments (16)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RoleInitializationServiceTest.java (3)
45-45: LGTM! Well-structured constant declaration.The constant follows the established pattern and is effectively used across the new test methods.
170-185: LGTM! Well-structured negative test case.The test effectively verifies that no unnecessary role or permission creation occurs when roles already exist.
188-206: LGTM! Good edge case coverage.The test effectively verifies the partial initialization scenario where only the release role needs to be created.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java (1)
Line range hint
177-255: Verify integration with supporting components.The new cluster-based permission model requires support from other components. Let's verify their existence:
✅ Verification successful
Integration with supporting components is properly implemented
The verification shows comprehensive integration with the required components:
- Permission types
MODIFY_NAMESPACE_IN_CLUSTERandRELEASE_NAMESPACE_IN_CLUSTERare properly defined inPermissionType.javaand used consistently across the codebaseRoleUtilsimplements all required cluster-related methods:
buildModifyNamespaceInClusterRoleNamebuildReleaseNamespaceInClusterRoleNamebuildClusterTargetId- The functionality is properly integrated in
PermissionValidatorandPermissionController- Test coverage exists in
RoleInitializationServiceTest🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of supporting code for cluster-based permissions # Check for required permission types echo "Checking for cluster permission types..." rg "MODIFY_NAMESPACE_IN_CLUSTER|RELEASE_NAMESPACE_IN_CLUSTER" --type java # Check for RoleUtils methods echo "Checking for RoleUtils cluster methods..." rg "buildModifyNamespaceInClusterRoleName|buildReleaseNamespaceInClusterRoleName|buildClusterTargetId" --type java # Check for corresponding test coverage echo "Checking for test coverage..." fd -e java -e groovy -e scala test.*DefaultRoleInitializationServiceLength of output: 6608
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.java (3)
69-80: Well-structured hierarchical permission check implementation.The implementation follows the principle of least privilege while maintaining a clear hierarchy of permissions. The early returns optimize performance by avoiding unnecessary checks.
Line range hint
160-174: Security checks properly updated for cluster-based permissions.The method has been correctly updated to incorporate cluster-based permissions while maintaining all existing security checks.
63-67: Implementation follows established permission check patterns.The new cluster-level permission check is well-implemented, following the existing permission validation patterns. The method correctly uses cluster-specific permission types and target IDs.
Let's verify the consistency of permission types:
✅ Verification successful
Permission type is consistently defined and used across the codebase
The verification shows that
MODIFY_NAMESPACE_IN_CLUSTERis properly defined inPermissionType.javaandRoleType.java, and is consistently used across the codebase:
- Defined as a constant in both permission and role type interfaces
- Used in
DefaultRoleInitializationServicefor role creation- Used in
RoleUtilsfor target ID generation- Used in the new permission validator method
The implementation aligns perfectly with the existing permission framework.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the permission type constant exists and is used consistently rg "MODIFY_NAMESPACE_IN_CLUSTER" --type javaLength of output: 1154
apollo-portal/src/main/resources/static/scripts/services/PermissionService.js (3)
26-32: API endpoints follow RESTful design principles.The new cluster namespace permission endpoints maintain consistency with existing patterns and properly implement RESTful principles.
Also applies to: 45-48
192-206: Permission check implementation follows established patterns.The new cluster namespace permission check properly implements the promise-based pattern and includes appropriate error handling.
448-476: Service interface maintains consistency and clarity.The new cluster namespace permission methods follow established naming conventions and properly map to their implementations.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java (6)
90-91: LGTM - Consistent authorization patternThe authorization check follows the same pattern as other methods, maintaining consistency in the permission model.
109-110: LGTM - Authorization check properly implementedThe permission check is consistent with the new model and properly validates cluster-level access.
Line range hint
123-134: Well-implemented authorization with additional security validationThe method properly implements cluster-level permission checks and includes an important security validation to prevent potential attack scenarios through namespace mismatch.
145-145: LGTM - Visibility check enhanced with cluster contextThe visibility check has been properly updated to consider cluster-level permissions.
Line range hint
223-239: LGTM - Consistent permission checksThe permission checks in syntaxCheckText and revokeItems methods are properly implemented and consistent with the new cluster-based permission model.
76-77: Authorization check properly enhanced with cluster-level permissionThe updated permission check now correctly includes cluster-level authorization, aligning with the new security model.
Let's verify the permission validator implementation:
✅ Verification successful
Authorization check implementation verified as correct and complete
The verification confirms that the permission check is properly implemented:
The
PermissionValidatorclass correctly implements thehasModifyNamespacePermission(appId, env, clusterName, namespaceName)method with a hierarchical permission check:
- Checks app-level namespace permission
- Checks environment-specific namespace permission
- Checks cluster-level permission
The permission check is consistently used across all item modification endpoints in both:
- Regular web controllers (
ItemController,NamespaceBranchController,ConfigsImportController)- OpenAPI controllers with their own consumer-specific validator
The implementation aligns with the security model by enforcing proper authorization at all required levels (app, environment, cluster, namespace).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the hasModifyNamespacePermission implementation ast-grep --pattern 'hasModifyNamespacePermission($appId, $env, $clusterName, $namespaceName)'Length of output: 95
Script:
#!/bin/bash # Let's try a different approach to find the permission validator implementation rg -l "class.*PermissionValidator"Length of output: 234
Script:
#!/bin/bash # Search for hasModifyNamespacePermission method definition rg "hasModifyNamespacePermission.*\(" -A 5Length of output: 25847
Script:
#!/bin/bash # Look for other similar permission checks in the codebase rg "@PreAuthorize.*hasModifyNamespacePermission" -A 1Length of output: 6974
...va/com/ctrip/framework/apollo/configservice/integration/ConfigControllerIntegrationTest.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/PermissionType.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/RoleType.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java
Outdated
Show resolved
Hide resolved
...ain/java/com/ctrip/framework/apollo/portal/entity/vo/ClusterNamespaceRolesAssignedUsers.java
Outdated
Show resolved
Hide resolved
...o-portal/src/main/resources/static/scripts/controller/role/ClusterNamespaceRoleController.js
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Outdated
Show resolved
Hide resolved
| {{'Config.CurrentlyOperatorEnv' | translate }}:{{pageContext.env}}， | ||
| {{'Common.Cluster' | translate }}:{{pageContext.clusterName}} | ||
| </h4> | ||
| <section class="panel cluster-info-panel"> |
There was a problem hiding this comment.
Displaying cluster permission management at the top of the configuration page feels a bit abrupt. Would it be better to add a new tab for cluster management within the application management section?
There was a problem hiding this comment.
This banner actually already existed, I think this banner still has a certain effect. Other Cluster-related functions can be added later, so that different Clusters correspond to the same set of pages through here, and these pages' design and code implementation will be more concise.
There was a problem hiding this comment.
The banner is now functional but only appears when the page width is very narrow.
Since the cluster authorization feature is not frequently used, I suggest adding a cluster management option to the left sidebar or placing it on the Manager Project page.
There was a problem hiding this comment.
Sounds good and reasonable!
Though, this could be a big complex to existing codes, because it may require a redesign of the Cluster Management page. Maybe I can open a PR later to make this change?
There was a problem hiding this comment.
Which UI modification option do you prefer? Adding a new cluster management entry or adding a cluster management tab in application management?
There was a problem hiding this comment.
From a lazy point of view, it is of course appropriate to add a banner directly,
while from a long-term perspective, I think it is better to add a cluster management interface.
After all, it is just not good to invade the original UI.
There was a problem hiding this comment.
How about
- Add a
Manage Clusterentry in the left panel.
- Clicking the entry navigates to the cluster list page, similar to the
Manage Access Keypage.
- Include an
Authorizebutton for each cluster.
The main additional work involves the cluster list page, which can follow existing implementation logic.
There was a problem hiding this comment.
Sounds vvvery good!
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RoleInitializationServiceTest.java (1)
Line range hint
261-266: Update mockPermission helper method for cluster permissionsThe current mockPermission helper method only supports namespace-level permissions. Consider updating it to support cluster-level permissions:
private Permission mockPermission(){ Permission permission = new Permission(); - permission.setPermissionType(PermissionType.MODIFY_NAMESPACE); - permission.setTargetId(RoleUtils.buildNamespaceTargetId(APP_ID, NAMESPACE)); + permission.setPermissionType(PermissionType.MODIFY_NAMESPACE_IN_CLUSTER); + permission.setTargetId(RoleUtils.buildClusterTargetId(APP_ID, ENV, CLUSTER)); return permission; }Or make it more flexible:
- private Permission mockPermission(){ + private Permission mockPermission(PermissionType type, String targetId){ Permission permission = new Permission(); - permission.setPermissionType(PermissionType.MODIFY_NAMESPACE); - permission.setTargetId(RoleUtils.buildNamespaceTargetId(APP_ID, NAMESPACE)); + permission.setPermissionType(type); + permission.setTargetId(targetId); return permission; }
🧹 Nitpick comments (19)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RoleInitializationServiceTest.java (4)
45-45: Consider parameterizing environment testsThe hardcoded "DEV" environment constant might not cover all scenarios. Consider adding test cases for other environments (e.g., FAT) to ensure the role initialization works correctly across different environments.
148-167: Enhance test coverage with specific verificationsWhile the test verifies the number of method calls, it would be more robust to also verify:
- The exact role names being created
- The specific permission types being assigned
- The target IDs of the created permissions
Add these verifications:
verify(rolePermissionService, times(2)).findRoleByRoleName(anyString()); verify(rolePermissionService, times(2)).createPermission(any()); verify(rolePermissionService, times(2)).createRoleWithPermissions(any(), anySet()); + verify(rolePermissionService).createPermission(argThat(permission -> + permission.getPermissionType() == PermissionType.MODIFY_NAMESPACE_IN_CLUSTER && + permission.getTargetId().equals(RoleUtils.buildClusterTargetId(APP_ID, ENV, CLUSTER)) + )); + verify(rolePermissionService).createPermission(argThat(permission -> + permission.getPermissionType() == PermissionType.RELEASE_NAMESPACE_IN_CLUSTER && + permission.getTargetId().equals(RoleUtils.buildClusterTargetId(APP_ID, ENV, CLUSTER)) + ));
187-206: Add specific verifications for release permissionConsider adding explicit verifications for the release permission creation:
verify(rolePermissionService, times(2)).findRoleByRoleName(anyString()); verify(rolePermissionService, times(1)).createPermission(any()); verify(rolePermissionService, times(1)).createRoleWithPermissions(any(), anySet()); + verify(rolePermissionService).createPermission(argThat(permission -> + permission.getPermissionType() == PermissionType.RELEASE_NAMESPACE_IN_CLUSTER && + permission.getTargetId().equals(RoleUtils.buildClusterTargetId(APP_ID, ENV, CLUSTER)) + ));
208-227: Add specific verifications for modify permissionConsider adding explicit verifications for the modify permission creation:
verify(rolePermissionService, times(2)).findRoleByRoleName(anyString()); verify(rolePermissionService, times(1)).createPermission(any()); verify(rolePermissionService, times(1)).createRoleWithPermissions(any(), anySet()); + verify(rolePermissionService).createPermission(argThat(permission -> + permission.getPermissionType() == PermissionType.MODIFY_NAMESPACE_IN_CLUSTER && + permission.getTargetId().equals(RoleUtils.buildClusterTargetId(APP_ID, ENV, CLUSTER)) + ));apollo-portal/src/main/resources/static/cluster/ns_role.html (5)
20-30: Add responsive design and SEO meta tagsThe document head is missing important meta tags for proper mobile support and SEO.
Add these meta tags:
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> + <meta name="viewport" content="width=device-width, initial-scale=1"> + <meta name="description" content="Apollo Configuration Center - Cluster Namespace Role Management"> <link rel="icon" href="../img/config.png">
69-75: Standardize button disable variable namingThere's an inconsistency in the naming convention of button disable variables:
modifyRoleSubmitBtnDisabledReleaseRoleSubmitBtnDisabled(incorrect Pascal case)- ng-disabled="ReleaseRoleSubmitBtnDisabled" + ng-disabled="releaseRoleSubmitBtnDisabled"Also applies to: 100-106
63-123: Reduce code duplication in role assignment formsThe modify and release role forms share almost identical structure. Consider creating a reusable directive or component.
Create a new directive like:
<role-assignment-form role-type="ModifyNamespacesInCluster" title="{{ 'Cluster.Role.GrantModifyTo' | translate }}" subtitle="{{ 'Cluster.Role.GrantModifyTo2' | translate }}" assigned-users="rolesAssignedUsers.modifyRoleUsers" on-assign="assignRoleToUser" on-remove="removeUserRole"> </role-assignment-form>
62-62: Consider granular permission handlingThe current implementation uses a single
hasAssignUserPermissioncheck. Consider implementing separate permissions for modify and release role management.This would allow for more flexible access control where users might have permission to manage modify roles but not release roles, or vice versa.
Also applies to: 128-130
137-170: Optimize script loading and add integrity checksThe current script loading can be optimized for better page load performance and security:
- Consider using async/defer attributes for non-critical scripts
- Add integrity checks for external resources
Example implementation:
- <script src="../vendor/jquery.min.js" type="text/javascript"></script> + <script src="../vendor/jquery.min.js" + integrity="sha384-[hash]" + crossorigin="anonymous" + defer></script>Also consider bundling and minifying the application scripts to reduce HTTP requests.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java (1)
200-220: Consider enhancing the permission check implementation.While the current implementation is functional, here are some suggestions for improvement:
- Consider collecting all namespaces that the user lacks permission for, rather than breaking at the first one. This would help users fix all permission issues at once.
- The variable name
hasPermissioncould be more specific, e.g.,hasModifyPermissionForAllNamespaces.- The comment on line 204 is redundant with the code logic.
Here's a suggested implementation:
- NamespaceIdentifier noPermissionNamespace = null; - // check if user has every namespace's ModifyNamespace permission - boolean hasPermission = true; - for (NamespaceIdentifier namespaceIdentifier : model.getSyncToNamespaces()) { - // once user has not one of the namespace's ModifyNamespace permission, then break the loop - hasPermission = permissionValidator.hasModifyNamespacePermission( - namespaceIdentifier.getAppId(), - namespaceIdentifier.getEnv().getName(), - namespaceIdentifier.getClusterName(), - namespaceIdentifier.getNamespaceName() - ); - if (!hasPermission) { - noPermissionNamespace = namespaceIdentifier; - break; - } - } - if (hasPermission) { + List<NamespaceIdentifier> noPermissionNamespaces = model.getSyncToNamespaces().stream() + .filter(namespace -> !permissionValidator.hasModifyNamespacePermission( + namespace.getAppId(), + namespace.getEnv().getName(), + namespace.getClusterName(), + namespace.getNamespaceName())) + .collect(Collectors.toList()); + + if (noPermissionNamespaces.isEmpty()) { configService.syncItems(model.getSyncToNamespaces(), model.getSyncItems()); return ResponseEntity.status(HttpStatus.OK).build(); } - throw new AccessDeniedException(String.format("You don't have the permission to modify namespace: %s", noPermissionNamespace)); + throw new AccessDeniedException(String.format("You don't have permission to modify the following namespaces: %s", + noPermissionNamespaces.stream() + .map(NamespaceIdentifier::toString) + .collect(Collectors.joining(", "))));apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java (3)
177-189: Add input validation and documentation for the new method.The method implementation looks good but could benefit from some improvements:
- Add input validation for parameters (appId, env, clusterName, operator)
- Add JavaDoc to document the method's purpose and parameters
- Consider adding debug logging for role creation events
Add parameter validation and documentation:
+ /** + * Initializes roles for managing namespaces within a specific cluster. + * Creates roles for modifying and releasing namespaces if they don't exist. + * + * @param appId The application ID + * @param env The environment name + * @param clusterName The cluster name + * @param operator The user performing the operation + * @throws IllegalArgumentException if any parameter is null or empty + */ @Transactional @Override public void initClusterNamespaceRoles(String appId, String env, String clusterName, String operator) { + Preconditions.checkArgument(StringUtils.isNotBlank(appId), "AppId must not be blank"); + Preconditions.checkArgument(StringUtils.isNotBlank(env), "Env must not be blank"); + Preconditions.checkArgument(StringUtils.isNotBlank(clusterName), "ClusterName must not be blank"); + Preconditions.checkArgument(StringUtils.isNotBlank(operator), "Operator must not be blank"); + + logger.debug("Initializing cluster namespace roles for app: {}, env: {}, cluster: {}", + appId, env, clusterName);
246-255: Consider refactoring role creation methods to reduce duplication.The implementation of
createClusterRolefollows a similar pattern tocreateNamespaceRoleandcreateNamespaceEnvRole. Consider extracting the common logic into a more generic method.Example refactoring:
+ private void createRoleWithPermission(String targetId, String permissionType, + String roleName, String operator) { + Permission permission = createPermission(targetId, permissionType, operator); + Permission createdPermission = rolePermissionService.createPermission(permission); + + Role role = createRole(roleName, operator); + rolePermissionService + .createRoleWithPermissions(role, Sets.newHashSet(createdPermission.getId())); + } private void createClusterRole(String appId, String env, String clusterName, String permissionType, String roleName, String 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())); + createRoleWithPermission(RoleUtils.buildClusterTargetId(appId, env, clusterName), + permissionType, roleName, operator); }This refactoring can be applied to
createNamespaceRoleandcreateNamespaceEnvRoleas well.
177-189: Document the new cluster-level permission model.The implementation introduces a new level of permission granularity at the cluster level. This architectural change should be properly documented to help users and developers understand:
- The hierarchy of permissions (app > cluster > namespace)
- When to use cluster-level permissions vs namespace-level permissions
- Migration guide for existing applications
Consider adding this information to:
- Architecture documentation
- User guide
- Developer guide
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
97-99: Refactor specific role builders to use this generic method.This generic method could be used by both
buildModifyNamespacesInClusterRoleNameandbuildReleaseNamespacesInClusterRoleNameto reduce code duplication. Also needs input validation.public static String buildClusterRoleName(String appId, String env, String clusterName, String roleType) { + if (appId == null || env == null || clusterName == null || roleType == null) { + throw new IllegalArgumentException("All parameters must not be null"); + } + if (appId.isEmpty() || env.isEmpty() || clusterName.isEmpty() || roleType.isEmpty()) { + throw new IllegalArgumentException("All parameters must not be empty"); + } return STRING_JOINER.join(roleType, appId, env, clusterName); } public static String buildModifyNamespacesInClusterRoleName(String appId, String env, String clusterName) { - return STRING_JOINER.join(RoleType.MODIFY_NAMESPACES_IN_CLUSTER, appId, env, clusterName); + return buildClusterRoleName(appId, env, clusterName, RoleType.MODIFY_NAMESPACES_IN_CLUSTER); } public static String buildReleaseNamespacesInClusterRoleName(String appId, String env, String clusterName) { - return STRING_JOINER.join(RoleType.RELEASE_NAMESPACES_IN_CLUSTER, appId, env, clusterName); + return buildClusterRoleName(appId, env, clusterName, RoleType.RELEASE_NAMESPACES_IN_CLUSTER); }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.java (2)
69-80: Add JavaDoc to explain the permission hierarchy.These methods implement a hierarchical permission check system, but the logic flow isn't immediately obvious. Consider adding documentation to explain:
- The order of permission checks
- The implications of each level of permission
- Why the checks are short-circuited with return true
+/** + * Checks if the current user has permission to modify the namespace. + * The check follows this hierarchy: + * 1. Global namespace permission + * 2. Environment-specific namespace permission + * 3. Cluster-level permission + * + * @param appId The application ID + * @param env The environment + * @param clusterName The cluster name + * @param namespaceName The namespace name + * @return true if the user has any of the required permissions + */ public boolean hasModifyNamespacePermission(String appId, String env, String clusterName, String namespaceName) {Also applies to: 100-110
Line range hint
160-174: LGTM! Well-documented and secure implementation.The method implements a secure-by-default approach with clear step-by-step validation. Consider extracting the validation steps into private methods for better testability.
public boolean shouldHideConfigToCurrentUser(String appId, String env, String clusterName, String namespaceName) { + if (!isEnvironmentConfigViewRestricted(env)) { + return false; + } + if (isPublicNamespace(appId, namespaceName)) { + return false; + } + return !hasRequiredPermissions(appId, env, clusterName, namespaceName); } + +private boolean isEnvironmentConfigViewRestricted(String env) { + return portalConfig.isConfigViewMemberOnly(env); +} + +private boolean isPublicNamespace(String appId, String namespaceName) { + AppNamespace appNamespace = appNamespaceService.findByAppIdAndName(appId, namespaceName); + return appNamespace != null && appNamespace.isPublic(); +} + +private boolean hasRequiredPermissions(String appId, String env, String clusterName, String namespaceName) { + return isAppAdmin(appId) || hasOperateNamespacePermission(appId, env, clusterName, namespaceName); +}apollo-portal/src/main/resources/static/scripts/services/PermissionService.js (3)
272-303: Add input validation for role management functions.Consider adding input validation to ensure required parameters are not null/undefined before making the API calls. This would prevent unnecessary server calls and provide better error messages.
function assignClusterNsRoleToUser(appId, env, clusterName, roleType, user) { var d = $q.defer(); + if (!appId || !env || !clusterName || !roleType || !user) { + d.reject('Missing required parameters'); + return d.promise; + } permission_resource.assign_cluster_ns_role_to_user({
448-476: Consider standardizing error handling across permission checks.While the implementation is correct, consider standardizing error handling across all permission check methods. This would ensure consistent error reporting and make it easier to handle permission-related errors in the UI.
has_modify_cluster_ns_permission: function (appId, env, clusterName) { - return hasClusterNsPermission(appId, env, clusterName, 'ModifyNamespacesInCluster'); + return hasClusterNsPermission(appId, env, clusterName, 'ModifyNamespacesInCluster') + .catch(function(error) { + console.error('Failed to check modify cluster namespace permission:', error); + return false; + }); },
Line range hint
26-476: Consider adding JSDoc documentation for new permission types.The implementation is solid, but adding JSDoc documentation for the new cluster namespace permission types and their usage would improve maintainability and help other developers understand when to use cluster namespace permissions vs. regular namespace permissions.
Example documentation:
/** * Checks if the current user has permission to modify namespaces within a cluster. * This differs from namespace-level permissions as it grants access to all namespaces * within the specified cluster. * * @param {string} appId - The application ID * @param {string} env - The environment name * @param {string} clusterName - The cluster name * @returns {Promise<boolean>} - Resolves to true if user has permission */ has_modify_cluster_ns_permission: function (appId, env, clusterName) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
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/ItemController.java(8 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/entity/vo/ClusterNamespaceRolesAssignedUsers.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/ns_role.html(1 hunks)apollo-portal/src/main/resources/static/config.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 (8)
- apollo-portal/src/main/resources/static/config.html
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/RoleType.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/PermissionType.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/vo/ClusterNamespaceRolesAssignedUsers.java
- apollo-portal/src/main/resources/static/scripts/directive/namespace-panel-directive.js
- apollo-portal/src/main/resources/static/scripts/controller/role/ClusterNamespaceRoleController.js
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java
🔇 Additional comments (9)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RoleInitializationServiceTest.java (1)
169-185: LGTM!The test effectively verifies that no unnecessary role or permission creations occur when both roles already exist.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java (2)
145-145: LGTM! Improved permission checks and error messaging.The permission checks have been properly updated to include cluster-level verification, and the error message has been enhanced to provide clearer context about the access denial.
The error message improvement aligns with the previous suggestion from reviewer nobodyiam.
Also applies to: 186-189
76-76: LGTM! Consistent implementation of cluster-level authorization.The
@PreAuthorizeannotations have been consistently updated across all methods to include cluster-level permission checks. This aligns well with the PR objectives.Let's verify that all callers have been updated to handle the new permission checks:
Also applies to: 90-90, 109-109, 123-123, 223-223, 234-234
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java (1)
Line range hint
177-255: Verify cluster role initialization flow.The implementation looks solid, but we should verify:
- When and where cluster roles are initialized in the application lifecycle
- Integration with existing permission checks
Let's verify the initialization flow:
✅ Verification successful
Let's gather more information about the initialization flow and permission checks.
Let's get one more piece of information about the permission checks and role utils.
Cluster role initialization is properly integrated and secure
Based on the verification results, the cluster role initialization implementation is well-integrated and secure:
Initialization is triggered at appropriate lifecycle points:
- During cluster creation via
ClusterService- During app creation via
AppServicefor default cluster- Protected by
@PreAuthorizepermissions in controllersPermission model integration is complete:
- Proper role naming conventions in
RoleUtils- Permission checks implemented in
PermissionValidator- OpenAPI permission validation via
ConsumerPermissionValidator- Comprehensive test coverage in
RoleInitializationServiceTestThe implementation follows the established pattern:
- Consistent role and permission creation
- Proper transaction handling
- Proper permission type enforcement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check where initClusterNamespaceRoles is called rg -l "initClusterNamespaceRoles" # Check for any existing cluster permission checks ast-grep --pattern 'class $_ implements $_ { $$$ buildClusterTargetId($_, $_, $_) $$$ }'Length of output: 838
Script:
#!/bin/bash # Check cluster role initialization in ClusterService rg -A 10 "initClusterNamespaceRoles" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ClusterService.java # Check cluster role initialization in AppService rg -A 10 "initClusterNamespaceRoles" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppService.java # Check RoleUtils for cluster permission handling ast-grep --pattern 'class RoleUtils { $$$ buildClusterTargetId($_, $_, $_) { $$$ } $$$ }' # Check permission checks in PermissionController rg -A 10 "hasCreateClusterPermission|hasModifyClusterPermission" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.javaLength of output: 1702
Script:
#!/bin/bash # Check RoleUtils implementation rg -A 5 "buildClusterTargetId|buildModifyNamespacesInClusterRoleName|buildReleaseNamespacesInClusterRoleName" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java # Check permission verification in tests rg -A 10 "initClusterNamespaceRoles" apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RoleInitializationServiceTest.java # Check for cluster permission checks in the codebase rg "hasCreateClusterPermission|hasModifyClusterPermission|hasReleaseNamespacesPermission" -A 5Length of output: 8930
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.java (2)
63-67: LGTM! Well-structured permission checks.The cluster permission check methods are well-implemented with:
- Consistent pattern across modify and release permissions
- Proper encapsulation (private methods)
- Clear separation of concerns
Also applies to: 94-97
117-119: LGTM! Clean implementation of operation permission check.The method provides a clear and concise way to check both modify and release permissions, following the single responsibility principle.
apollo-portal/src/main/resources/static/scripts/services/PermissionService.js (3)
26-32: LGTM! Well-structured resource definitions.The new resource definitions follow consistent patterns and REST best practices:
- Proper HTTP methods (GET/POST/DELETE) for respective operations
- Consistent URL structure with existing endpoints
- Appropriate Content-Type headers for POST requests
Also applies to: 45-48, 102-115
192-206: LGTM! Consistent implementation of permission check.The
hasClusterNsPermissionfunction follows the established patterns:
- Proper promise handling
- Consistent parameter passing
- Error handling aligned with other functions
132-145: Verify if empty request body is intentional.Unlike
initAppNamespacePermissionwhich passes a namespace parameter,initClusterNsPermissionpasses an empty object{}as the request body. Please verify if this is intentional or if cluster-specific data should be passed.✅ Verification successful
Let me try a different approach to verify this by checking the permission initialization patterns in the codebase.
These scripts will help us understand:
- How other permission initialization methods are implemented
- The permission_resource configuration
- The backend API endpoint implementation to verify the expected request body
Let me try one more search to find the backend controller and related files.
Let me try one final search to find the complete permission resource definition.
Empty request body is correct for cluster namespace permission initialization
Based on the complete service definition, the empty request body
{}ininitClusterNsPermissionis intentional and correct. Here's why:
- The endpoint
/apps/:appId/envs/:env/clusters/:clusterName/initNsPermissionalready receives all necessary information through URL parameters (appId, env, clusterName)- The function follows the same pattern as other permission initialization methods where the actual data needed for initialization is passed through URL parameters
- The
Content-Typeheader is set totext/plain;charset=UTF-8, indicating that the endpoint is not expecting a JSON payload🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other initialization endpoints accept empty bodies rg -A 5 "init.*Permission.*\{\}" --type jsLength of output: 44
Script:
#!/bin/bash # Search for permission initialization patterns rg "permission_resource\.init" --type js -A 5Length of output: 1408
Script:
#!/bin/bash # Look for the permission_resource definition ast-grep --pattern 'permission_resource = $rest_resource($url, $config)'Length of output: 74
Script:
#!/bin/bash # Check the API endpoint definition fd -e java -e kt -x rg "initClusterNsPermission" {} \;Length of output: 57
Script:
#!/bin/bash # Search for cluster permission related endpoints in backend rg "cluster.*permission" --type java -A 5 # Also check for the REST resource definition rg "permission_resource\s*=" --type js -A 5Length of output: 4308
Script:
#!/bin/bash # Get the complete permission_resource definition with all endpoints cat apollo-portal/src/main/resources/static/scripts/services/PermissionService.jsLength of output: 20846
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Copilot reviewed 17 out of 32 changed files in this pull request and generated no comments.
Files not reviewed (15)
- apollo-portal/src/main/resources/static/cluster/ns_role.html: Language not supported
- apollo-portal/src/main/resources/static/config.html: Language not supported
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/CommitController.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseHistoryController.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RoleInitializationService.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.java: Evaluated as low risk
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/RoleType.java: Evaluated as low risk
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apollo-portal/src/main/resources/static/i18n/zh-CN.json (1)
364-372: LGTM! The translations are clear and consistent.The new translations for cluster-level permission management follow the same pattern as existing namespace permission translations, making the UI consistent across different permission management screens.
Consider adding a period at the end of
Cluster.Role.NoPermissionfor consistency with other similar messages in the file:- "Cluster.Role.NoPermission": "您没有权限哟!", + "Cluster.Role.NoPermission": "您没有权限哟！",
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 updated release notes:
New Features
Improvements
Bug Fixes
Documentation
These changes significantly improve the access control and permission management capabilities of the Apollo configuration management platform.