Refactor: align permission validator api #5337
Conversation
dosubot
bot
added
the
size:XL
WalkthroughThis pull request refactors the permission validation logic across both the openapi and portal modules. The changes remove the direct dependency on the HttpServletRequest object by introducing a context-based retrieval method. In openapi, method signatures in controllers and the ConsumerPermissionValidator have been updated to use the new consumer ID retrieval method. In portal, the existing PermissionValidator is refactored into an interface and replaced with UserPermissionValidator, with corresponding updates to annotations and dependency injections in controllers, services, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AppController as OpenAPI AppController
participant AuthUtil as ConsumerAuthUtil
participant PermValidator as ConsumerPermissionValidator
Client->>AppController: POST /createApp (payload)
AppController->>AuthUtil: retrieveConsumerIdFromCtx()
AuthUtil-->>AppController: consumerId
AppController->>PermValidator: hasCreateApplicationPermission()
PermValidator-->>AppController: permission granted/denied
AppController-->>Client: Response
sequenceDiagram
participant Client
participant PortalController
participant UserPermValidator as UserPermissionValidator
Client->>PortalController: API Request (e.g., deleteApp)
PortalController->>UserPermValidator: isSuperAdmin()
UserPermValidator-->>PortalController: permission granted/denied
PortalController-->>Client: API Response
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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 (13)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java (1)
1-455: Consider adding integration tests.While the code changes look good, it would be beneficial to add integration tests to verify the permission validation behavior across different scenarios.
Consider adding tests for:
- Permission inheritance
- Role-based access control
- Edge cases like non-existent users or roles
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceControllerTest.java (1)
42-42: Enhance test coverage for permission validation.While the permission check has been updated to match the new API signature, passing
nulldoesn't effectively test the permission validation logic.Consider adding these test cases:
// Test with valid consumer ID Assert.assertTrue(consumerPermissionValidator.hasCreateNamespacePermission("validConsumerId")); // Test with invalid consumer ID Assert.assertFalse(consumerPermissionValidator.hasCreateNamespacePermission("invalidConsumerId"));apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/GlobalSearchController.java (1)
43-44: Consider adding Javadoc for better API documentation.Since this is a public API endpoint with super admin privileges, consider adding Javadoc to document:
- The purpose of the endpoint
- Required permissions
- Parameter descriptions
- Response format
Example:
/** * Search for configuration items across all environments by key or value. * Requires super admin privileges. * * @param key The configuration key to search for (optional) * @param value The configuration value to search for (optional) * @return A SearchResponseEntity containing matching ItemInfo objects * @throws BadRequestException if both key and value are empty */apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java (1)
114-114: Consider adding null checks for method parameters.While the permission check calls are correctly updated, consider adding null checks for the parameters to prevent potential NullPointerExceptions.
- if (userPermissionValidator.shouldHideConfigToCurrentUser(appId, env, clusterName, namespaceBO.getBaseInfo().getNamespaceName())) { + if (namespaceBO != null && namespaceBO.getBaseInfo() != null && + userPermissionValidator.shouldHideConfigToCurrentUser(appId, env, clusterName, namespaceBO.getBaseInfo().getNamespaceName())) {Also applies to: 128-128
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java (1)
59-59: LGTM! Consider adding request validation for critical operations.The permission validator update maintains the super admin requirement for cluster deletion, which is appropriate for this critical operation.
Consider adding the following security enhancements:
- Request validation to prevent accidental deletions (e.g., confirmation token)
- Rate limiting for delete operations
- Audit logging of the actual changes being made (which clusters were affected)
Example enhancement:
@PreAuthorize(value = "@userPermissionValidator.isSuperAdmin()") @DeleteMapping(value = "apps/{appId}/envs/{env}/clusters/{clusterName:.+}") @ApolloAuditLog(type = OpType.DELETE, name = "Cluster.delete") + @RateLimiter(name = "clusterDelete") public ResponseEntity<Void> deleteCluster(@PathVariable String appId, @PathVariable String env, - @PathVariable String clusterName){ + @PathVariable String clusterName, + @RequestHeader(required = true) String confirmationToken){ + validateConfirmationToken(appId, env, clusterName, confirmationToken); clusterService.deleteCluster(Env.valueOf(env), appId, clusterName); return ResponseEntity.ok().build(); }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.java (1)
100-111: Consider enhancing error message clarity.While the permission checks are correctly implemented, the error message could be more user-friendly by providing specific details about which permission is missing.
Consider updating the error message to be more specific:
- throw new AccessDeniedException("Forbidden operation. " - + "Caused by: 1.you don't have release permission " - + "or 2. you don't have modification permission " - + "or 3. you have modification permission but branch has been released"); + throw new AccessDeniedException(String.format( + "Forbidden operation on branch '%s' in namespace '%s'. Reason: %s", + branchName, + namespaceName, + !hasReleasePermission && !hasModifyPermission ? "missing both release and modify permissions" : + !hasReleasePermission ? "missing release permission" : + "branch has already been released" + ));apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/SystemInfoController.java (1)
91-120: Consider optimizing the health check implementation.The current implementation has a few areas for improvement:
- Linear search through services is inefficient. Consider using a map for O(1) lookup.
- Health check requests lack timeout handling.
Here's a suggested optimization:
@PreAuthorize(value = "@userPermissionValidator.isSuperAdmin()") @GetMapping(value = "/health") public Health checkHealth(@RequestParam String instanceId) { List<Env> allEnvs = portalSettings.getAllEnvs(); + ServiceDTO service = findServiceById(instanceId); + if (service == null) { + throw new IllegalArgumentException("No such instance of instanceId: " + instanceId); + } + return restTemplate.getForObject(service.getHomepageUrl() + "/health", Health.class); } + private ServiceDTO findServiceById(String instanceId) { + return portalSettings.getAllEnvs().stream() + .map(this::adaptEnv2EnvironmentInfo) + .flatMap(envInfo -> Stream.concat( + Optional.ofNullable(envInfo.getAdminServices()).stream().flatMap(Arrays::stream), + Optional.ofNullable(envInfo.getConfigServices()).stream().flatMap(Arrays::stream) + )) + .filter(service -> instanceId.equals(service.getInstanceId())) + .findFirst() + .orElse(null); + }Additionally, consider adding timeout handling:
@Bean public RestTemplate restTemplate(RestTemplateBuilder builder) { return builder .setConnectTimeout(Duration.ofSeconds(5)) .setReadTimeout(Duration.ofSeconds(5)) .build(); }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java (1)
94-94: LGTM! Consider reducing duplication withimportConfigByZipWithCoverConflictNamespace.The authorization annotation has been correctly updated. However, there's significant code duplication between this method and
importConfigByZipWithCoverConflictNamespace. Consider refactoring these methods to share common logic.Here's a suggested refactor to reduce duplication:
+ private void importConfigByZip(String envs, MultipartFile file, boolean ignoreConflict) throws IOException { + List<Env> importEnvs = + Splitter.on(ENV_SEPARATOR).splitToList(envs).stream() + .map(env -> Env.valueOf(env)) + .collect(Collectors.toList()); + + byte[] bytes = file.getBytes(); + try (ZipInputStream zipInputStream = new ZipInputStream(new ByteArrayInputStream(bytes))) { + configsImportService.importDataFromZipFile(importEnvs, zipInputStream, ignoreConflict); + } + } @PreAuthorize(value = "@userPermissionValidator.isSuperAdmin()") @PostMapping(value = "/configs/import", params = "conflictAction=cover") public void importConfigByZipWithCoverConflictNamespace(@RequestParam(value = "envs") String envs, @RequestParam("file") MultipartFile file) throws IOException { - List<Env> - importEnvs = - Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(env -> Env.valueOf(env)).collect(Collectors.toList()); - - byte[] bytes = file.getBytes(); - try (ZipInputStream zipInputStream = new ZipInputStream(new ByteArrayInputStream(bytes))) { - configsImportService.importDataFromZipFile(importEnvs, zipInputStream, false); - } + importConfigByZip(envs, file, false); } @PreAuthorize(value = "@userPermissionValidator.isSuperAdmin()") @PostMapping(value = "/configs/import", params = "conflictAction=ignore") public void importConfigByZipWithIgnoreConflictNamespace(@RequestParam(value = "envs") String envs, @RequestParam("file") MultipartFile file) throws IOException { - List<Env> - importEnvs = - Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(env -> Env.valueOf(env)).collect(Collectors.toList()); - - byte[] bytes = file.getBytes(); - try (ZipInputStream zipInputStream = new ZipInputStream(new ByteArrayInputStream(bytes))) { - configsImportService.importDataFromZipFile(importEnvs, zipInputStream, true); - } + importConfigByZip(envs, file, true); }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.java (1)
33-37: Clarify “operate” meaning.
Since this aggregates “modify” and “release,” consider documenting or renaming for clarity.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (3)
80-84: TODO notice for app namespace creation.
This is currently a stub. Would you like assistance implementing the alignment withOpenApiConfigandPortalConfig?
98-104: Visibility control logic is unimplemented.
Another TODO suggests it requires further alignment with the portal configuration.
111-116: Manage App Master permission check.
Partially unimplemented. This references a TODO regardingOpenApiConfigandPortalConfigalignment.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java (1)
61-74: Extract duplicated error handling logic.The error handling logic in
retrieveConsumerIdByCtx()is duplicated fromretrieveConsumerId(request). Consider extracting the common logic into a private method.private long retrieveConsumerId(HttpServletRequest request) { Object value = request.getAttribute(CONSUMER_ID); + return parseConsumerId(value); + } + private long parseConsumerId(Object value) { try { return Long.parseLong(value.toString()); } catch (Throwable ex) { throw new IllegalStateException("No consumer id!", ex); } } public long retrieveConsumerIdByCtx() { ServletRequestAttributes attributes = (ServletRequestAttributes) RequestContextHolder.getRequestAttributes(); if (attributes == null) { throw new IllegalStateException("No consumer id!"); } HttpServletRequest request = attributes.getRequest(); Object value = request.getAttribute(CONSUMER_ID); + return parseConsumerId(value); - try { - return Long.parseLong(value.toString()); - } catch (Throwable ex) { - throw new IllegalStateException("No consumer id!", ex); - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
CHANGES.md(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/audit/ApolloAuditLogQueryApiPortalPreAuthorizer.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AccessKeyController.java(4 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/CommitController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/GlobalSearchController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java(10 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java(9 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java(15 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseHistoryController.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ServerConfigController.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/SystemInfoController.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceControllerTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerTest.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGES.md
🧰 Additional context used
🧠 Learnings (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java (1)
Learnt from: BlackBear2003
PR: apolloconfig/apollo#5324
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java:171-197
Timestamp: 2025-01-24T14:43:58.631Z
Learning: The `createGrayDelRelease` method in Portal's ReleaseController should emit a ConfigPublishEvent, similar to other release-related methods (`createRelease`, `merge`, `createGrayRelease`), to maintain consistent event-driven behavior.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
🔇 Additional comments (106)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ClusterController.java (2)
57-57: LGTM! Clean permission validation refactor.The
@PreAuthorizeannotation has been simplified to use context-based permission validation, aligning with the broader API refactoring initiative.
59-60: LGTM! Streamlined method signature.The removal of the
HttpServletRequestparameter simplifies the API while maintaining all necessary business logic parameters. This change is consistent with the move towards context-based request handling.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java (2)
26-26: LGTM! Consistent renaming of permission validator.The update from
PermissionValidatortoUserPermissionValidatoris consistent with the PR's objective to align the permission validator API.Also applies to: 68-68
158-158: LGTM! Mock behavior preserved.The mock usage is correctly updated while maintaining the same behavior, ensuring test integrity.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java (3)
23-23: LGTM! Clean dependency injection.The refactoring from
PermissionValidatortoUserPermissionValidatoris implemented correctly with proper constructor injection and final field declaration.Also applies to: 60-60, 68-74
169-169: LGTM! Consistent permission validation.The
@PreAuthorizeannotations have been consistently updated across all endpoints to use the newuserPermissionValidatorbean, maintaining proper access control.Also applies to: 194-194, 237-237, 261-261, 297-297, 317-317, 343-343, 363-363, 384-384, 396-396, 408-408, 422-422, 435-435
418-418: LGTM! Proper validator usage.The
hasCreateApplicationPermissioncheck is correctly delegated to theuserPermissionValidator.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/GlobalSearchController.java (1)
42-42: LGTM! Security annotation updated correctly.The change from
@permissionValidatorto@userPermissionValidatoraligns with the broader permission validation refactoring effort.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ItemController.java (2)
81-85: LGTM! Improved method signature by removing unnecessary HttpServletRequest parameter.The removal of the HttpServletRequest parameter simplifies the API while maintaining the required functionality.
102-107: LGTM! Consistent permission validation using ConsumerPermissionValidator.The @PreAuthorize annotation correctly uses the consumerPermissionValidator for checking modify namespace permissions.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java (4)
26-26: LGTM! Improved dependency injection with UserPermissionValidator.The constructor properly initializes the UserPermissionValidator, maintaining consistent permission validation across the application.
Also applies to: 66-66, 68-73
145-147: LGTM! Consistent permission check for hiding configurations.The permission check uses the new UserPermissionValidator to determine if configurations should be hidden from the current user.
185-191: LGTM! Clear error message for unauthorized access.The permission check provides a descriptive message when hiding configurations from unauthorized users.
205-210: LGTM! Improved permission check readability.The permission check is now more readable with proper parameter alignment.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java (4)
27-27: LGTM!The import statement correctly reflects the transition to the new
UserPermissionValidatorclass.
69-69: LGTM!The field declaration correctly uses the new
UserPermissionValidatortype while maintaining immutability with thefinalmodifier.
77-77: LGTM!The constructor parameter and field assignment correctly use the new
UserPermissionValidatortype, maintaining proper dependency injection.Also applies to: 83-83
144-152: LGTM!The permission check correctly uses the new
UserPermissionValidatorwhile maintaining proper error handling within the predicate.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java (3)
33-33: LGTM! Clean refactoring of permission validator.The import and field declaration changes align well with the permission validator API refactoring.
Also applies to: 79-79
89-90: LGTM! Constructor injection properly updated.The constructor parameter and field assignment changes maintain clean dependency injection.
Also applies to: 97-97
144-144: LGTM! Consistent updates to permission annotations.The
@PreAuthorizeannotations are consistently updated across all methods to use the newuserPermissionValidator, maintaining proper access control.Also applies to: 175-175, 198-198, 221-221
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java (6)
80-80: LGTM! Permission check properly refactored.The simplified permission check still maintains security by validating create namespace permission based on appId.
86-86: LGTM! Method signature simplified while maintaining operator tracking.The removal of HttpServletRequest parameter reduces coupling while preserving the ability to track operations.
100-100: LGTM! Enhanced permission check with environment context.The permission check now properly includes the environment parameter, providing more granular access control.
107-107: LGTM! Consistent method signature refactoring.The change maintains consistency with createBranch method while preserving operation tracking.
114-115: LGTM! Permission validation logic properly updated.The permission checks maintain the same security level while properly incorporating the environment context.
140-140: LGTM! Consistent refactoring of updateBranchRules method.The changes align with the refactoring pattern seen in other methods, maintaining both security and operation tracking.
Also applies to: 145-145
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java (5)
23-23: LGTM! Consistent dependency injection of UserPermissionValidator.The refactoring from
PermissionValidatortoUserPermissionValidatoris implemented correctly with proper dependency injection.Also applies to: 56-56, 63-63, 68-68
72-72: LGTM! Proper permission validation and event publishing.The
@PreAuthorizeannotation correctly uses the newuserPermissionValidator, and the method maintains proper event publishing after successful release creation.Also applies to: 101-101, 102-102
149-149: LGTM! Proper empty results handling for unauthorized access.The implementation correctly returns an empty list when the user doesn't have permission to view the configuration.
Also applies to: 150-150, 151-151, 152-152, 153-153
164-164: LGTM! Consistent permission check for active releases.The implementation maintains consistency with the
findAllReleasesmethod by using the same permission check.Also applies to: 165-165, 166-166, 167-167, 168-168
190-190: LGTM! Proper permission validation for rollback operations.The rollback operation correctly validates user permissions before proceeding.
Also applies to: 191-191, 192-192
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java (2)
76-76: LGTM! Proper permission validation for release creation.The
@PreAuthorizeannotation correctly uses theconsumerPermissionValidator, and the method signature has been simplified by removing theHttpServletRequestparameter.Also applies to: 77-77, 78-78, 79-79, 80-80, 81-81
211-211: LGTM! Proper permission validation for rollback operations.The rollback operation correctly validates consumer permissions before proceeding.
Also applies to: 212-212, 213-213
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseHistoryController.java (4)
21-21: LGTM!The import of UserPermissionValidator aligns with the PR's objective to refactor the permission validation mechanism.
36-36: LGTM!The field declaration is properly updated to use UserPermissionValidator and maintains immutability with the final modifier.
38-41: LGTM!The constructor is properly updated to use UserPermissionValidator and follows dependency injection best practices.
51-51: Verify the security implications of the permission check.While the change aligns with the refactoring, please ensure that the UserPermissionValidator's implementation of
shouldHideConfigToCurrentUsermaintains or enhances the security guarantees of the original PermissionValidator.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for the implementation of shouldHideConfigToCurrentUser in UserPermissionValidator ast-grep --pattern 'class UserPermissionValidator { $$$ shouldHideConfigToCurrentUser($$$) { $$$ } $$$ }'Length of output: 119
Let's further verify the implementation by searching for the method definition using a different search tool. Please run the following script to search for any references to
shouldHideConfigToCurrentUserin the repository:#!/bin/bash # Search for any references of shouldHideConfigToCurrentUser to locate its implementation details rg "shouldHideConfigToCurrentUser" -nCould you please provide the output from this script? This will help confirm whether the method's implementation is in place and evaluate its security implications.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ClusterController.java (1)
47-47: LGTM! Verify consistent usage across the codebase.The permission validator update aligns with the PR objectives to standardize the permission validation mechanism.
Let's verify that this pattern is consistently applied across other controllers:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining usage of the old permissionValidator rg "@permissionValidator\." "apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/" # Search for the new userPermissionValidator usage pattern rg "@userPermissionValidator\." "apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/"Length of output: 10860
Consistent Permission Validator Update Verified
The search confirms that all controllers now consistently use
@userPermissionValidatorwith no remaining references to the old@permissionValidator. This update is correctly applied and aligns with the PR objectives across the codebase.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.java (3)
26-26: LGTM! Import and field changes align with the permission validator API refactoring.The switch from
PermissionValidatortoUserPermissionValidatoris consistent with the PR's objective to align the permission validator API.Also applies to: 48-48
54-65: LGTM! Constructor changes maintain consistency with the field updates.The constructor parameter and field assignment have been correctly updated to use
UserPermissionValidator.
81-81: LGTM! PreAuthorize annotations correctly updated.The annotations have been properly updated to use
userPermissionValidatorand removeHttpServletRequestparameters.Let's verify that all PreAuthorize annotations in the codebase have been consistently updated:
Also applies to: 119-119, 159-159
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining PreAuthorize annotations that might still use the old validator # Search for PreAuthorize annotations rg -l '@PreAuthorize.*PermissionValidator' | while read -r file; do echo "=== $file ===" rg '@PreAuthorize.*PermissionValidator' "$file" doneLength of output: 14771
PreAuthorize Annotations Update Verified
All controllers are now correctly using the updated validators—both for
@userPermissionValidatorand@consumerPermissionValidator—with no sign of the old patterns. The scan confirms that the NamespaceBranchController and other controllers have the intended, consistent updates.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/CommitController.java (4)
22-22: LGTM!The import of
UserPermissionValidatoraligns with the PR objective of refactoring the permission validation mechanism.
41-41: LGTM!The field declaration change from
PermissionValidatortoUserPermissionValidatormaintains immutability and follows consistent naming conventions.
43-46: LGTM!The constructor changes correctly implement dependency injection for the new
UserPermissionValidatortype.
54-54: Verify consistent implementation of the new permission check method.The change to
shouldHideConfigToCurrentUserimproves clarity and removes HttpServletRequest dependency, aligning with the PR objective.Let's verify the consistent implementation of this method across the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for all implementations of shouldHideConfigToCurrentUser ast-grep --pattern 'shouldHideConfigToCurrentUser($$$)' # Search for any remaining direct HttpServletRequest usage in permission checks rg -l "HttpServletRequest.*Permission"Length of output: 4836
Consistent Permission Check Implementation Verified
- The new method
shouldHideConfigToCurrentUserappears to be used as intended in CommitController.- No remaining instances of direct
HttpServletRequestusage in permission validations were found in the codebase.- The few matches from the SQL files are unrelated to the Java implementation and can be disregarded.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/SystemInfoController.java (2)
89-89: LGTM! Permission level is appropriate.The change aligns with the permission validator API refactoring effort. Super admin access is appropriate for health check endpoints.
68-68: LGTM! Verify the validator bean configuration.The change aligns with the permission validator API refactoring effort.
Let's verify that the
userPermissionValidatorbean is properly configured:❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for UserPermissionValidator bean configuration rg -l "UserPermissionValidator" | xargs ast-grep --pattern 'class $_ implements PermissionValidator'Length of output: 100
It appears that our initial script did not locate any bean definition or class named "UserPermissionValidator." Could you please verify manually (or run additional searches) to confirm that the
userPermissionValidatorbean is configured and registered in the Spring context? This is essential for ensuring that the updated annotation functions as intended.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java (2)
81-81: LGTM! Verify integration with UserPermissionValidator.The change aligns with the PR objectives to refactor the permission validator API. The security check remains functionally equivalent.
Let's verify the integration with UserPermissionValidator:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Verify the UserPermissionValidator implementation and its usage # Check if UserPermissionValidator implements the required method ast-grep --pattern $'class UserPermissionValidator { $$$ shouldHideConfigToCurrentUser($_, $_, $_, $_) { $$$ } $$$ }' # Check for consistent usage across other controllers rg -A 2 '@userPermissionValidator.shouldHideConfigToCurrentUser'Length of output: 813
The usage of
@userPermissionValidator.shouldHideConfigToCurrentUserinConfigsExportControlleris correctly updated according to the PR objectives. We did find that the controller now uses the validator as intended. However, our initial check viaast-grepdid not return an implementation match forUserPermissionValidatorwith the expected method signature, so further investigation would be needed to confirm that the class actually implementsshouldHideConfigToCurrentUser. If that implementation exists (perhaps with different formatting or in a different module), then everything is in order.For now, as the integration in the controller is verified and assuming the implementation exists elsewhere (or with a slightly different code style), the change in the controller is correct.
LGTM provided the
UserPermissionValidatorimplementation is confirmed separately.
[approve]
114-114: LGTM! Verify integration with UserPermissionValidator.The change aligns with the PR objectives to refactor the permission validator API. The security check remains functionally equivalent, and the method maintains good audit logging.
Let's verify the integration with UserPermissionValidator:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the UserPermissionValidator implementation and its usage # Check if UserPermissionValidator implements the required method ast-grep --pattern $'class UserPermissionValidator { $$$ isSuperAdmin() { $$$ } $$$ }' # Check for consistent usage across other controllers rg -A 2 '@userPermissionValidator.isSuperAdmin'Length of output: 12361
Verified: The integration with UserPermissionValidator is consistent across controllers.
The usage of
@userPermissionValidator.isSuperAdmin()in theConfigsExportControlleris consistent with its usage in multiple other controllers (e.g.,ServerConfigController,PermissionController, etc.). This indicates that the refactor from the original validator to the new UserPermissionValidator has been applied uniformly, and the critical security check remains intact.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java (2)
79-79: LGTM! Authorization check properly updated.The super admin permission check has been correctly migrated to use the new validator.
64-64: LGTM! Verify the validator bean configuration.The authorization annotation change aligns with the broader permission validator API refactoring effort.
Let's verify that the
userPermissionValidatorbean is properly configured:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for UserPermissionValidator bean configuration ast-grep --pattern 'class $_ implements $_ { $$$ hasModifyNamespacePermission($_, $_, $_, $_) { $$$ } $$$ }'Length of output: 122
I'll run an additional search to locate any mentions of
userPermissionValidatorin the repository.Once you run this script, please check if there is any configuration or definition for the
userPermissionValidatorbean. If the search still produces no output, then it's likely that the bean is configured indirectly (for example, via component scanning or XML configuration) or defined using an alternative naming convention. In that case, manual verification of the Spring configuration (or bean definitions) is advised to ensure that theuserPermissionValidatorbean is available in the application context.Action needed: Please manually verify the bean configuration for
userPermissionValidatorif the automated search doesn't yield any results.
🏁 Script executed:
#!/bin/bash # Search for all occurrences of 'userPermissionValidator' in the codebase rg -n "userPermissionValidator" .Length of output: 20074
Validator bean confirmed!
The search results show thatuserPermissionValidatoris defined as a Spring component inapollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java, which verifies that the authorization annotation change inConfigsImportController.javais correct and aligned with the refactoring effort.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceController.java (2)
53-53: LGTM! Clean permission validation refactor.The simplified permission check that focuses solely on
appIdreduces coupling with the servlet API and follows good design principles.
55-56: LGTM! Method signature simplified.The removal of
HttpServletRequestparameter aligns with the permission validation refactoring and doesn't affect the core business logic.Let's verify that this pattern is consistently applied across other controllers:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining HttpServletRequest parameters in permission validation # across other controllers in the openapi module ast-grep --pattern 'class $_ { $$$ @PreAuthorize($_) $$$ public $_ $_($_, HttpServletRequest $_) { $$$ } $$$ }'Length of output: 129
I'll run a broader search for any occurrences of
HttpServletRequestin the openapi module controllers to double-check that no similar signatures remain.
🏁 Script executed:
#!/bin/bash echo "Searching for 'HttpServletRequest' in openapi controllers..." rg "HttpServletRequest" -n -C 2 apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/Length of output: 10544
Approval: Method Signature Simplification Confirmed
The removal of theHttpServletRequestparameter from thecreateNamespacemethod inNamespaceControlleraligns with the overall refactoring. Our targeted search for methods annotated with@PreAuthorizehaving anHttpServletRequestparameter in the openapi controllers returned no results, confirming that this pattern has been consistently updated. Note that occurrences ofHttpServletRequestin utility and filter classes are expected and unrelated to this change.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.java (13)
21-21: Refactor from class to interface.
Adopting an interface decouples permission checks from a concrete implementation, allowing multiple implementers with different logic.
23-24: Method signature is straightforward.
The parameters(appId, env, clusterName, namespaceName)provide a clear context for permission checks here.
26-27: Aligned naming for release permission.
This naming is consistent with the established permission patterns in the codebase.
29-31: Default implementation may need environment checks.
ReturninghasAssignRolePermission(appId) || isSuperAdmin()might not suffice if there's a need to validate a specific environment or cluster for deletion. Please confirm that environment-based checks aren't required for delete permissions.
39-39: No issues with role assignment check.
Clear and succinct.
41-41: Create namespace permission check.
No concerns. Method name is self-explanatory.
43-43: Parameter type is well-defined.
AppNamespace appNamespaceusage is direct, but ensure implementers handle any null or invalid cases.
45-45: Create cluster permission.
Method aligns with the rest of the interface.
47-49: Default method for app admin check.
Falling back onisSuperAdmin()or role assignment is logical.
51-51: Super admin check.
Used frequently in other default methods. No issues.
53-54: Visibility control method.
Essential for ensuring that sensitive configs are hidden when appropriate.
56-56: Application creation permission.
Straightforward.
58-58: Manage App Master permission.
Method name is consistent and descriptive.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (9)
21-24: New imports align with refactored interface.
They ensure access toAppNamespaceandPermissionValidatorfor the updated permission logic.
30-31: Bean definition & interface implementation.
Using the@Componentannotation and implementingPermissionValidatoris consistent with the new architecture.
42-53: Security check: “Create” implies “Modify.”
Granting modification rights if the consumer can create namespaces may be intended, but confirm it doesn’t introduce an unintended escalation of privileges.
55-66: Release permission logic.
Similar approach to modify permission—no immediate issues.
68-72: Assign role permission check.
Straightforward usage ofconsumerHasPermissionwithASSIGN_ROLE.
74-79: Namespace creation permission.
No concerns, the logic is direct and consistent withConsumerRolePermissionService.
86-90: Create cluster permission.
Consistent with the rest of the permission checks.
92-96: isSuperAdmin returns false.
This ensures openAPI consumers only have designated permissions.
105-110: Create application permission.
Implementation uses theSYSTEM_PERMISSION_TARGET_ID, consistent with portal logic.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (13)
1-16: License and file header.
No technical issues identified here.
17-49: Constructor injection and field declarations.
The dependencies—UserInfoHolder,RolePermissionService,PortalConfig,AppNamespaceService, andSystemRoleManagerService—are properly injected for permission checks.
51-80: Modify namespace checks.
Private helper methods ensure clarity. The multi-layer approach (env,clusterName, broader cluster perms) is comprehensive but watch for potential coverage in tests to ensure all paths are validated.
83-113: Release namespace permission checks.
Mirrors the approach from modify, with each layer tested in a private method. This pattern fosters maintainability.
115-120: Assign role permission.
Direct check usinguserHasPermissionwith the relevantPermissionType.
122-128: Create namespace permission.
Straightforward mapping to the role permission service. No concerns.
129-139: Create app namespace permission logic.
Includes handling for public namespaces vs. private ones, gating onportalConfig.canAppAdminCreatePrivateNamespace(). Falls back on super admin. The logic covers the typical scenarios well.
141-146: Create cluster permission.
No concerns—similar to other checks.
148-152: Super admin check.
Delegates to the rolePermissionService. This is standard.
153-169: shouldHideConfigToCurrentUser
Nicely breaks down steps: environment setting, public namespace check, then verifying admin or operate perms. Make sure to cover negative test cases in unit tests.
171-174: Create application permission (no arguments).
Delegates to the user-specific method. Straightforward.
176-178: Create application permission (userId).
Directly callssystemRoleManagerService.hasCreateApplicationPermission(userId). Clear separation of concerns.
180-187: Manage app master permission.
Ensures super admin or a combination of role permissions for the current user. Thorough approach that guards the feature effectively.apollo-portal/src/test/java/com/ctrip/framework/apollo/SkipAuthorizationConfiguration.java (2)
45-45: LGTM! Method signature updated correctly.The mock configuration has been updated to match the new signature of
hasCreateNamespacePermissionwhich no longer requires the request parameter.
64-67: LGTM! Bean configuration updated correctly.The bean name and type have been updated to use
UserPermissionValidator, maintaining consistency with the refactored permission validation system.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ServerConfigController.java (1)
45-45: LGTM! PreAuthorize annotations updated consistently.All @PreAuthorize annotations have been updated to use
@userPermissionValidatorconsistently across the controller methods, maintaining the same permission check logic.Also applies to: 52-52, 59-59, 65-65
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerTest.java (1)
47-47: LGTM! Test class updated correctly.The test class has been properly updated to use
UserPermissionValidator, with consistent changes to both the mock field and constructor parameter.Also applies to: 54-55
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java (2)
59-60: LGTM! Improved design by removing direct HttpServletRequest dependency.The changes enhance maintainability by using a context-based approach for consumer ID retrieval instead of directly passing the HttpServletRequest object.
Also applies to: 62-63, 74-75
97-98: LGTM! Consistent with the refactoring pattern.The changes follow the same pattern of using context-based consumer ID retrieval, maintaining consistency across the codebase.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java (1)
61-61: LGTM! Consistent permission validator update.The changes align with the refactoring goal by updating the permission validator references from
@permissionValidatorto@userPermissionValidator.Run the following script to verify consistent usage across the codebase:
Also applies to: 86-87
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify consistent usage of userPermissionValidator # Test: Search for any remaining usage of @permissionValidator. Expect: No results. rg -l '@permissionValidator\.'Length of output: 32
I'm going to run an expanded search script to verify that there are no remaining occurrences of the old annotation (
@permissionValidator) and to confirm that the updated validator (@userPermissionValidator) is present across the codebase. This extra context helps ensure our inference is robust.Please run the above script to provide additional verification output. Once we confirm that:
- There are zero occurrences of
@permissionValidator- The new pattern
@userPermissionValidatorappears where expected (including in UserInfoController.java at lines 61 and 86-87)we can conclude that the consistent permission validator update has been successfully applied.
If the output confirms these findings, we can consider the changes verified.
🏁 Script executed:
#!/bin/bash # Verify that there are no remaining references to the deprecated "@permissionValidator" echo "=== Searching for deprecated '@permissionValidator' references ===" rg -n '@permissionValidator\.' || echo "No deprecated usage found." echo "=== Searching for updated '@userPermissionValidator' references ===" rg -n '@userPermissionValidator\.' || echo "No updated usage found."Length of output: 11288
Consistent usage of @userPermissionValidator verified.
- The search found no remaining references to the deprecated @permissionValidator.
- Updated validator usage (including the expected references in UserInfoController at lines 61 and 86-87) is consistently applied across the codebase.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConsumerController.java (1)
64-65: LGTM! Consistent permission validator updates across all methods.The changes systematically update all
@PreAuthorizeannotations from@permissionValidatorto@userPermissionValidator, maintaining consistency across the controller.Also applies to: 105-106, 111-112, 117-118, 166-167, 172-173
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AppController.java (1)
122-123: LGTM! Consistent permission validator updates across all methods.The changes systematically update all
@PreAuthorizeannotations from@permissionValidatorto@userPermissionValidator, maintaining consistency across the controller.Also applies to: 131-132, 184-185
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/audit/ApolloAuditLogQueryApiPortalPreAuthorizer.java (3)
20-20: LGTM!The import statement correctly reflects the dependency on the new
UserPermissionValidatorclass, aligning with the PR's objective to refactor the permission validator API.
28-32: LGTM!The field declaration and constructor changes properly implement the dependency injection of
UserPermissionValidator. The field is correctly marked as final, ensuring immutability.
36-36: Verify security implications of the validator change.While the change aligns with the refactoring, please ensure that
UserPermissionValidator.isSuperAdmin()maintains the same security guarantees as the previous implementation.Let's verify the implementation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for the UserPermissionValidator implementation to verify the isSuperAdmin method ast-grep --pattern 'class $_ implements UserPermissionValidator { $$$ isSuperAdmin() { $$$ } $$$ }'Length of output: 113
Let's gather some additional context. The previous AST search didn't return any results, so we need to locate the implementation details of the
isSuperAdminmethod more broadly. In particular, we want to search for where the method is defined in the entire repository to analyze its security implications. You can run the following script to search for any occurrence of the method definition:Once you have the results, we can further verify that the logic in
UserPermissionValidator.isSuperAdmin()provides equivalent security guarantees as the previous implementation.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for the public boolean isSuperAdmin method definition in all .java files rg "public boolean isSuperAdmin" -n .Length of output: 609
Let's inspect the actual implementation of the
isSuperAdmin()method from theUserPermissionValidatorto verify that the security checks remain intact. Please run the following script to extract the surrounding code (lines 140–170) from the file:The output will provide context on the logic used in the
isSuperAdmin()method so that we can confirm it maintains the same security guarantees as the previous implementation. Once we review the snippet, we can determine if further action is needed.
🏁 Script executed:
[run_scripts] #!/bin/bash sed -n '140,170p' apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.javaLength of output: 1278
Security Check on the Validator Update Verified
The new call to
userPermissionValidator.isSuperAdmin()inApolloAuditLogQueryApiPortalPreAuthorizer.javadelegates the check toreturn rolePermissionService.isSuperAdmin(userInfoHolder.getUser().getUserId());in
UserPermissionValidator.java. Given that:
- The
isSuperAdminmethod is implemented inDefaultRolePermissionService(and similarly inConsumerPermissionValidator), and- It consistently performs the super-admin check based on the current user’s ID,
the change maintains equivalent security guarantees. No deviation in the security check is observed with this refactoring.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/AccessKeyController.java (5)
60-65: LGTM!The permission validator change maintains the same security check while aligning with the refactoring objectives.
67-75: LGTM!The permission validator change maintains the same security check while aligning with the refactoring objectives.
77-86: LGTM!The permission validator change maintains the same security check while aligning with the refactoring objectives.
88-96: LGTM!The permission validator change maintains the same security check while aligning with the refactoring objectives.
49-58: LGTM! Verify the validator implementation.The permission validator change aligns with the PR objectives while maintaining the same security check for app admin status.
Let's verify the implementation of
UserPermissionValidator:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the UserPermissionValidator implementation # Search for the UserPermissionValidator class definition ast-grep --pattern 'class UserPermissionValidator implements $_ { $$$ isAppAdmin($_) { $$$ } $$$ }'Length of output: 113
Let's run a new search using
rgto locate the definition ofUserPermissionValidatorin the repository:
🏁 Script executed:
#!/bin/bash # Search for the UserPermissionValidator class definition using ripgrep rg "class\s+UserPermissionValidator" -n .Length of output: 219
Let's further verify by checking if the
isAppAdminmethod is implemented inUserPermissionValidator.java:
🏁 Script executed:
#!/bin/bash # Searching for the isAppAdmin method implementation in the UserPermissionValidator class rg "public boolean isAppAdmin" -n apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.javaLength of output: 136
Let's perform a broader search for the
isAppAdminsubstring in theUserPermissionValidatorfile:
🏁 Script executed:
#!/bin/bash # Searching for any occurrence of 'isAppAdmin(' in the UserPermissionValidator file rg "isAppAdmin(" -n apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.javaLength of output: 195
Let's run a modified search with the parenthesis properly escaped:
🏁 Script executed:
#!/bin/bash # Searching for any occurrence of 'isAppAdmin(' in the UserPermissionValidator file with escaped parenthesis rg "isAppAdmin\(" -n apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.javaLength of output: 232
Let's perform an additional search in the
UserPermissionValidator.javafile to locate the declaration of theisAppAdminmethod (with the "boolean" keyword):
🏁 Script executed:
#!/bin/bash # Searching for the isAppAdmin method definition using "boolean isAppAdmin" pattern rg "boolean\s+isAppAdmin" -n apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.javaLength of output: 131
Let's run one more search using a more general regex to retrieve the method declaration of
isAppAdminin theUserPermissionValidatorclass:
🏁 Script executed:
#!/bin/bash # Search for any access modifier followed by a method declaration containing 'isAppAdmin' rg -nE "(public|protected|private)\s+.*isAppAdmin\(" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.javaLength of output: 236
Let's search for the
PermissionValidatorinterface to check if it declares theisAppAdminmethod:
🏁 Script executed:
#!/bin/bash # Search for the PermissionValidator interface definition rg "interface PermissionValidator" -n .Length of output: 182
Let's search for any declaration of
isAppAdminin thePermissionValidatorinterface to see if it's defined there:
🏁 Script executed:
#!/bin/bash rg "isAppAdmin" -n apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/PermissionValidator.javaLength of output: 163
LGTM – Permission Validator Implementation Verified
- The
UserPermissionValidatorclass implementsPermissionValidator, which provides a default implementation forisAppAdmin(String appId)(verified by the search in the interface).- The change in the
@PreAuthorizeannotation in theAccessKeyControllernow references@userPermissionValidator.isAppAdmin(#appId), which will invoke the inherited default method.- This refactor aligns with the PR objectives and satisfies the required permission checks.
...portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java
Show resolved
Hide resolved
...portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java
Outdated
Show resolved
Hide resolved
...ortal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (2)
47-51: Optimize duplicate consumer ID retrieval.Methods like
hasModifyNamespacePermissionandhasReleaseNamespacePermissionmake multiple calls toretrieveConsumerIdFromCtx()which could be optimized by storing the result in a local variable.Example optimization for
hasModifyNamespacePermission:public boolean hasModifyNamespacePermission(String appId, String env, String clusterName, String namespaceName) { if (hasCreateNamespacePermission(appId)) { return true; } - return permissionService.consumerHasPermission(consumerAuthUtil.retrieveConsumerIdFromCtx(), + long consumerId = consumerAuthUtil.retrieveConsumerIdFromCtx(); + return permissionService.consumerHasPermission(consumerId, PermissionType.MODIFY_NAMESPACE, RoleUtils.buildNamespaceTargetId(appId, namespaceName)) - || permissionService.consumerHasPermission(consumerAuthUtil.retrieveConsumerIdFromCtx(), + || permissionService.consumerHasPermission(consumerId, PermissionType.MODIFY_NAMESPACE, RoleUtils.buildNamespaceTargetId(appId, namespaceName, env)); }Also applies to: 60-64
80-82: Add documentation for unsupported operations.While throwing UnsupportedOperationException is the agreed-upon approach (based on past review comments), it would be helpful to add Javadoc comments explaining why these operations are not supported in the OpenAPI context.
Example documentation:
+ /** + * This operation is not supported in OpenAPI context as it requires Portal-specific configurations + * that are not available in OpenApiConfig. + * + * @throws UnsupportedOperationException always + */ @Override public boolean hasCreateAppNamespacePermission(String appId, AppNamespace appNamespace) { throw new UnsupportedOperationException("Not supported operation"); }Also applies to: 97-100, 109-111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java(3 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AppController.java
🔇 Additional comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (2)
21-21: LGTM! Class now properly implements PermissionValidator interface.The changes align well with the PR objective to refactor the permission validator API.
Also applies to: 24-24, 29-30
32-39: LGTM! Well-structured constructor with proper dependency injection.The implementation follows good practices by using final parameters for immutable dependencies.
What's the purpose of this PR
align permission validator api
Which issue(s) this PR fixes:
Fixes #5331
Brief changelog
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
Refactor
PermissionValidatorwithUserPermissionValidatorin multiple components, ensuring consistency in permission checks.Tests