optimize: Implement unified permission verification logic and Optimize the implementation of permission verification#5456
Conversation
dosubot
bot
added
the
size:XXL
WalkthroughIntroduces AbstractPermissionValidator and UnifiedPermissionValidator, adds per-request auth-type resolution via a request filter and thread-local, extends role/consumer APIs with hasAnyPermission and repository queries, implements Permission equality, and replaces User/Consumer validators with the unified validator across controllers, services, tests, and SQL fixtures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Filter as UserTypeResolverFilter
participant Ctx as UserIdentityContextHolder
participant Controller
participant Unified as UnifiedPermissionValidator
participant UserVal as UserPermissionValidator
participant ConsumerVal as ConsumerPermissionValidator
Client->>Filter: HTTP request
Filter->>Ctx: setAuthType(USER/CONSUMER/ANONYMOUS)
Filter->>Controller: continue request
Controller->>Unified: hasXPermission(args)
Unified->>Ctx: getAuthType()
alt auth = CONSUMER
Unified->>ConsumerVal: delegate hasXPermission(args)
ConsumerVal->>ConsumerVal: evaluate via hasPermissions(list)
else auth = USER
Unified->>UserVal: delegate hasXPermission(args)
UserVal->>UserVal: evaluate via hasPermissions(list)
else auth = ANONYMOUS
Unified-->>Controller: throw IllegalStateException / deny
end
Unified-->>Controller: boolean
Controller->>Client: OK / 403
Filter->>Ctx: clear()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java (1)
177-189: Remove extra whitespace in constant reference
In DefaultRoleInitializationService.java at line 182, change the invocation fromPermissionType. MODIFY_NAMESPACES_IN_CLUSTERto
PermissionType.MODIFY_NAMESPACES_IN_CLUSTER- createClusterRole(appId, env, clusterName, PermissionType. MODIFY_NAMESPACES_IN_CLUSTER, modifyNamespacesInClusterRoleName, operator); + createClusterRole(appId, env, clusterName, PermissionType.MODIFY_NAMESPACES_IN_CLUSTER, modifyNamespacesInClusterRoleName, operator);apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ItemController.java (1)
122-136: deleteItem: null-check item to prevent NPEloadItemById may return null; subsequent item.getNamespaceId() deref risks NPE.
Apply this diff:
public void deleteItem(@PathVariable String appId, @PathVariable String env, @PathVariable String clusterName, @PathVariable String namespaceName, @PathVariable long itemId) { ItemDTO item = configService.loadItemById(Env.valueOf(env), itemId); NamespaceDTO namespace = namespaceService.loadNamespaceBaseInfo(appId, Env.valueOf(env), clusterName, namespaceName); // In case someone constructs an attack scenario - if (namespace == null || item.getNamespaceId() != namespace.getId()) { + if (namespace == null || item == null || item.getNamespaceId() != namespace.getId()) { throw BadRequestException.namespaceNotMatch(); } configService.deleteItem(Env.valueOf(env), itemId, userInfoHolder.getUser().getUserId()); }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java (1)
101-106: Fix guard: use branchName in gray-release PreAuthorize (security correctness)The permission guard for gray release should validate against the branch, not the parent cluster; otherwise a user with parent-cluster permission could publish to a branch without branch-level permission (or be blocked incorrectly). Update as below.
- @PreAuthorize(value = "@unifiedPermissionValidator.hasReleaseNamespacePermission(#appId, #env, #clusterName, #namespaceName)") + @PreAuthorize(value = "@unifiedPermissionValidator.hasReleaseNamespacePermission(#appId, #env, #branchName, #namespaceName)")apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java (1)
100-107: Fix Env.valueOf case-sensitivity (runtime 500 risk)At Line 116, Env.valueOf(env) will throw for lowercase envs; elsewhere in this file you use toUpperCase(). Align it here.
Apply:
- releaseService.loadLatestRelease(appId, Env.valueOf(env), branchName, namespaceName) == null); + releaseService.loadLatestRelease(appId, Env.valueOf(env.toUpperCase()), branchName, namespaceName) == null);Also applies to: 114-116
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java (2)
98-99: Make Env.valueOf() robust to lowercase input across endpointsSeveral calls use Env.valueOf(env) which will throw for lowercase env values; other places in this PR already use toUpperCase().
Apply:
- .setEnv(Env.valueOf(env)); + .setEnv(Env.valueOf(env.toUpperCase())); @@ - .setEnv(Env.valueOf(env)); + .setEnv(Env.valueOf(env.toUpperCase())); @@ - releaseModel.setEnv(Env.valueOf(env).toString()); + releaseModel.setEnv(Env.valueOf(env.toUpperCase()).toString()); @@ - .setEnv(Env.valueOf(env)); + .setEnv(Env.valueOf(env.toUpperCase())); @@ - ReleaseDTO release = releaseService.findReleaseById(Env.valueOf(env), releaseId); + ReleaseDTO release = releaseService.findReleaseById(Env.valueOf(env.toUpperCase()), releaseId); @@ - .setEnv(Env.valueOf(env)); + .setEnv(Env.valueOf(env.toUpperCase()));Also applies to: 132-132, 154-154, 162-162, 204-204, 220-220
176-178: Validate grayDelKeys is not empty, not just non-nullPrevent accidental empty deletions and improve error messaging.
Apply:
- RequestPrecondition.checkArguments(model.getGrayDelKeys() != null, - "Params(grayDelKeys) can not be null"); + RequestPrecondition.checkArguments(model.getGrayDelKeys() != null && !model.getGrayDelKeys().isEmpty(), + "Params(grayDelKeys) can not be null or empty");
🧹 Nitpick comments (21)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java (1)
182-182: Remove stray whitespace after the dot (likely Checkstyle failure).
PermissionType. MODIFY_NAMESPACES_IN_CLUSTERhas double spaces after.. This compiles but typically violates NoWhitespaceAfter(DOT) rules and is inconsistent with the rest of the file.- createClusterRole(appId, env, clusterName, PermissionType. MODIFY_NAMESPACES_IN_CLUSTER, modifyNamespacesInClusterRoleName, operator); + createClusterRole(appId, env, clusterName, PermissionType.MODIFY_NAMESPACES_IN_CLUSTER, modifyNamespacesInClusterRoleName, operator);apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AuthContextHolder.java (2)
21-22: Avoid nulls: provide a default value via withInitial or normalize on readReturning null invites NPEs/branching. Prefer a non-null default.
Option A (preferred): initialize with default
- private static final ThreadLocal<String> AUTH_TYPE_HOLDER = new ThreadLocal<>(); + private static final ThreadLocal<String> AUTH_TYPE_HOLDER = + ThreadLocal.withInitial(() -> com.ctrip.framework.apollo.portal.constant.AuthConstants.UNKNOWN);Option B: normalize on read
- public static String getAuthType() { - return AUTH_TYPE_HOLDER.get(); - } + public static String getAuthType() { + String v = AUTH_TYPE_HOLDER.get(); + return v != null ? v : com.ctrip.framework.apollo.portal.constant.AuthConstants.UNKNOWN; + }
27-40: Document cleanup contract in AuthContextHolder — AuthTypeResolverFilter already wraps setAuthType/clear in a finally block and tests clear the thread-local in @AfterEach/@beforeeach; add class-level Javadoc in AuthContextHolder stating that callers must always invoke clear() (e.g. in a finally block) to prevent ThreadLocal leaks.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AuthContextHolderTest.java (1)
27-33: Simplify setup; remove redundant setAuthType(null)clear() is sufficient; the explicit set to null is redundant.
Apply:
@BeforeEach public void setUp() { - // Clear ThreadLocal before each test - AuthContextHolder.setAuthType(null); - // Clear any existing values before each test + // Clear any existing values before each test AuthContextHolder.clear(); }apollo-portal/src/test/resources/sql/permission/insert-test-getUserPermissionSet.sql (1)
25-25: Clarify comment wording for readability.“Nobody has no roles” is ambiguous. Prefer “In this dataset, only user 'apollo' has roles; no other users are inserted.”
- -- User-Role associations (apollo has two permissions, nobody has no roles) + -- User-Role associations (only user 'apollo' has roles in this dataset)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java (1)
398-402: Consider a typed descriptor instead of "type:targetId" strings.Strings are brittle if targetId ever contains “:”. A small immutable value object (e.g., PermissionDescriptor { type, targetId }) avoids delimiter issues. If string form is required for transport, serialize at the edge.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/AuthConstants.java (1)
1-9: Consider using an enum for auth types to gain type-safety.Constants work, but an enum prevents typos and eases switch logic.
package com.ctrip.framework.apollo.portal.constant; public enum AuthType { USER, CONSUMER, UNKNOWN }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RolePermissionRepository.java (1)
24-24: Remove unused import if custom query is dropped.-import org.springframework.data.repository.query.Param;apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RolePermissionServiceTest.java (1)
349-356: Tighten assertions to assert the exact set (order-agnostic)Use a single assertion to express intent clearly and avoid size+contains coupling.
Apply this diff:
- assertEquals(2, permissions.size()); - assertTrue(permissions.contains("ModifyNamespace:someApp+someNamespace")); - assertTrue(permissions.contains("ReleaseNamespace:someApp+someNamespace")); + org.assertj.core.api.Assertions.assertThat(permissions) + .containsExactlyInAnyOrder( + "ModifyNamespace:someApp+someNamespace", + "ReleaseNamespace:someApp+someNamespace");apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfigurationTest.java (2)
37-40: Remove unnecessary Mockito initializationopenMocks() is unused (no @mock fields). Drop it to reduce noise.
- @BeforeEach - public void setUp() { - MockitoAnnotations.openMocks(this); - authFilterConfiguration = new AuthFilterConfiguration(); - } + @BeforeEach + public void setUp() { + authFilterConfiguration = new AuthFilterConfiguration(); + }
65-76: Strengthen assertions and avoid brittle interaction checksAlso assert the filter instance type. Prefer verifyNoInteractions over verifyNoMoreInteractions to better express intent.
@@ -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; @@ - // Verify that URL pattern configuration is correct + // Verify that URL pattern configuration is correct assertEquals(1, result.getUrlPatterns().size()); assertEquals("/openapi/*", result.getUrlPatterns().iterator().next()); + // Verify filter instance type + ConsumerAuthenticationFilter filter = result.getFilter(); + assertNotNull(filter, "Filter should not be null"); + assertInstanceOf(ConsumerAuthenticationFilter.class, filter); @@ - // Verify that there are no unexpected interactions - verifyNoMoreInteractions(mockConsumerAuthUtil, mockConsumerAuditUtil); + // Verify that there are no interactions during bean construction + verifyNoInteractions(mockConsumerAuthUtil, mockConsumerAuditUtil);Also applies to: 29-31
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java (2)
88-94: Use constant instead of string literal for UNKNOWNAvoid typos and keep consistency with AuthConstants.
- AuthContextHolder.setAuthType("UNKNOWN"); + AuthContextHolder.setAuthType(AuthConstants.UNKNOWN); @@ - AuthContextHolder.setAuthType("UNKNOWN"); + AuthContextHolder.setAuthType(AuthConstants.UNKNOWN);Also applies to: 128-132
56-68: Assert non-selected validator is not usedStrengthen tests by verifying no calls hit the non-delegated validator.
@@ boolean result = unifiedPermissionValidator.hasManageAppMasterPermission(appId); assertTrue(result); + org.mockito.Mockito.verifyNoInteractions(consumerPermissionValidator); @@ boolean result = unifiedPermissionValidator.hasManageAppMasterPermission(appId); assertFalse(result); + org.mockito.Mockito.verifyNoInteractions(userPermissionValidator); @@ boolean result = unifiedPermissionValidator.hasCreateNamespacePermission(appId); assertTrue(result); + org.mockito.Mockito.verifyNoInteractions(consumerPermissionValidator); @@ boolean result = unifiedPermissionValidator.hasCreateNamespacePermission(appId); assertTrue(result); + org.mockito.Mockito.verifyNoInteractions(userPermissionValidator);Also applies to: 70-82, 96-108, 110-122
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.java (3)
55-66: Constructor param consistency and readabilityKeep the last parameter final and on a new line for consistency.
public NamespaceBranchController( - final ReleaseService releaseService, - final NamespaceBranchService namespaceBranchService, - final ApplicationEventPublisher publisher, - final PortalConfig portalConfig, UnifiedPermissionValidator unifiedPermissionValidator) { + final ReleaseService releaseService, + final NamespaceBranchService namespaceBranchService, + final ApplicationEventPublisher publisher, + final PortalConfig portalConfig, + final UnifiedPermissionValidator unifiedPermissionValidator) {
93-116: Centralize delete authorization logic (optional)Consider a unified canDeleteBranch(...) method in UnifiedPermissionValidator to encapsulate the combined permission logic used here, improving auditability and reuse.
68-80: Input validation for envEnv.valueOf(env) throws on bad input. If this endpoint is externally called, consider validating or mapping case-insensitively to avoid 500s.
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionServiceTest.java (1)
112-115: Simplify set constructionUse Guava Sets.newHashSet for brevity.
+import com.google.common.collect.Sets; @@ - Set<String> expected = new HashSet<>(Lists.newArrayList("app:app1", "namespace:ns1", "cluster:cluster1")); + Set<String> expected = Sets.newHashSet("app:app1", "namespace:ns1", "cluster:cluster1");Also applies to: 21-21
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
19-28: Ensure auth type is always set or provide a safe fallbackgetDelegate() throws for null/unknown auth type. If the resolver filter doesn’t run or misses a path, this will surface as 500s (including in PreAuthorize evaluation).
Please confirm AuthTypeResolverFilter is registered for both portal and OpenAPI paths and clears the ThreadLocal in a finally block. If not guaranteed, consider:
- throw new IllegalStateException("Unknown authentication type"); + // Default to USER for portal routes if missing; log/trace unknowns + return userPermissionValidator;I can add targeted tests for filter coverage and ThreadLocal cleanup.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (2)
78-81: Avoid repeated context lookup in hasPermission (minor)Cache consumerId locally for consistency and micro-efficiency.
- protected boolean hasPermission(String targetId, String permissionType) { - return permissionService.consumerHasPermission(consumerAuthUtil.retrieveConsumerIdFromCtx(), - permissionType, targetId); - } + protected boolean hasPermission(String targetId, String permissionType) { + final long consumerId = consumerAuthUtil.retrieveConsumerIdFromCtx(); + return permissionService.consumerHasPermission(consumerId, permissionType, targetId); + }
84-87: Guard empty requiredPerms (minor)Short-circuit empty lists to false to match intent.
- protected boolean hasPermissions(List<String> requiredPerms) { - if (requiredPerms == null){ + protected boolean hasPermissions(List<String> requiredPerms) { + if (requiredPerms == null || requiredPerms.isEmpty()){ return false; }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java (1)
28-31: Document “any-of” semantics to match subclass implementationsBoth UserPermissionValidator and ConsumerPermissionValidator implement hasPermissions as “any match”. Add Javadoc to avoid future inconsistencies.
- @Override + /** + * Grants if the principal has any of the listed permissions: + * - Global namespace-level + * - Env-scoped namespace-level + * - Cluster-scoped + */ + @Override public boolean hasModifyNamespacePermission(String appId, String env, String clusterName, String namespaceName) { … - @Override + /** + * Grants if the principal has any of the listed permissions: + * - Global namespace-level + * - Env-scoped namespace-level + * - Cluster-scoped + */ + @Override public boolean hasReleaseNamespacePermission(String appId, String env, String clusterName, String namespaceName) {Also applies to: 41-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (39)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java(6 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/AbstractPermissionValidator.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AuthContextHolder.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/AuthConstants.java(1 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/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(8 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java(16 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java(7 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/filter/AuthTypeResolverFilter.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RolePermissionRepository.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RolePermissionService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionServiceTest.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AuthContextHolderTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.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/spi/configuration/AuthFilterConfigurationTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RolePermissionServiceTest.java(1 hunks)apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_different_users.sql(1 hunks)apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_no_roles.sql(1 hunks)apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_roles_without_permissions.sql(1 hunks)apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_with_permissions.sql(1 hunks)apollo-portal/src/test/resources/sql/permission/insert-test-getUserPermissionSet.sql(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (5)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/AuthConstants.java (1)
AuthConstants(3-9)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (1)
Component(33-95)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (1)
Component(32-121)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/audit/ApolloAuditLogQueryApiPortalPreAuthorizer.java (1)
Component(24-38)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AuthContextHolder.java (1)
AuthContextHolder(19-41)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/AuthTypeResolverFilter.java (1)
AuthTypeResolverFilter(29-53)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/AuthConstants.java (1)
AuthConstants(3-9)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AuthContextHolder.java (1)
AuthContextHolder(19-41)
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java (1)
ExtendWith(32-133)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java (1)
ExtendWith(45-261)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/audit/ApolloAuditLogQueryApiPortalPreAuthorizer.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (1)
Component(32-121)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
Component(9-79)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AuthContextHolderTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AuthContextHolder.java (1)
AuthContextHolder(19-41)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/AuthTypeResolverFilter.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AuthContextHolder.java (1)
AuthContextHolder(19-41)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/AuthConstants.java (1)
AuthConstants(3-9)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (1)
Component(33-95)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
Component(9-79)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidatorTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-124)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-124)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java (1)
AbstractPermissionValidator(26-97)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (1)
Component(32-121)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
Component(9-79)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfigurationTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/AuthTypeResolverFilter.java (1)
AuthTypeResolverFilter(29-53)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/filter/ConsumerAuthenticationFilter.java (1)
ConsumerAuthenticationFilter(44-121)
🔇 Additional comments (41)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RolePermissionService.java (1)
96-98: Convert to English Javadoc and decide on default vs abstract
- Replace the Chinese inline comment on
getUserPermissionSet(String userId)inRolePermissionServicewith proper English Javadoc.- Internally, only
DefaultRolePermissionServiceimplements and already overrides this method, so adding it as abstract won’t break existing code.- To preserve binary compatibility for any external/third-party SPI implementors, either provide a
defaultimplementation returningCollections.emptySet(), or confirm that all downstream implementations override this method.apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_different_users.sql (1)
1-25: Drop the cleanup diff suggestion
cleanup.sql (invoked via @SQL(…, executionPhase = AFTER_TEST_METHOD)) already deletes “ConsumerRole”, “Permission” and “RolePermission” after each test, preventing ID clashes; quoted identifiers match the schema.Likely an incorrect or invalid review comment.
apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_with_permissions.sql (1)
1-19: Verify FK constraints and add seeding/cleanup if needed
Confirm whether the test schema enforces foreign-key constraints on ConsumerRole/RolePermission/Permission. If it does, you’ll need to:
- Seed the Role table for IDs 200 and 201 before inserting ConsumerRole/RolePermission rows.
- Add DELETE statements after the test to clean up Permission, RolePermission, and ConsumerRole entries and avoid PK collisions on reruns.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AuthContextHolderTest.java (1)
35-46: No changes needed: tests correctly expect null default and cleanup is handled in @beforeeach
AuthContextHolder#getAuthType returns null by default, and each test clears the ThreadLocal in @beforeeach; no UNKNOWN default alignment or additional @AfterEach is required.apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_no_roles.sql (1)
1-3: Ignore quoted identifiers in SQL fixtures Quoting table and column names with double quotes is consistent across all existing test SQL files; replacing them with unquoted identifiers would conflict with the established style.Likely an incorrect or invalid review comment.
apollo-portal/src/test/resources/sql/permission/insert-test-getUserPermissionSet.sql (1)
1-29: Fixture data looks correct and idempotent for the tests.Cleans first, inserts minimal rows, and aligns with the "permissionType:targetId" descriptor used in services. LGTM.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java (1)
378-402: Soft-delete filtering already enforced via @where
UserRole(line 36) andRolePermission(line 34) entities declare@Where(clause = "IsDeleted= false"), sofindByUserId(...)andfindByRoleIdIn(...)automatically exclude soft-deleted records.apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java (2)
28-28: Import addition is correct.Collections.emptySet() is used below; import is needed.
82-105: Soft-delete filtering confirmed—no changes required Entities ConsumerRole, RolePermission, and Permission each declare@Where(clause = "IsDeleted= false"), so the derivedfindByConsumerId,findByRoleIdIn, andfindByIdsmethods already exclude deleted rows.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RolePermissionRepository.java (1)
37-41: Remove unnecessary soft-delete suggestion
RolePermission entity is annotated with @SQLDelete and @where(clause = "IsDeleted= false"), so both the derived findByRoleIdIn and the custom batchDeleteByPermissionIds automatically enforce soft-delete filtering.Likely an incorrect or invalid review comment.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RolePermissionServiceTest.java (1)
342-357: Solid additive coverage for getUserPermissionSet()Test setup/teardown and assertions look correct and map to the string representation contract.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseHistoryController.java (1)
20-41: Ensure fallback to USER in UnifiedPermissionValidator.getDelegate()
Unrecognized or missingAuthContextHolder.getAuthType()currently triggers an exception. Add a default branch to return the USER validator to preserve existing portal behavior:private PermissionValidator getDelegate() { String type = AuthContextHolder.getAuthType(); if (AuthConstants.CONSUMER.equals(type)) { return consumerPermissionValidator; } // fallback to USER for portal endpoints return userPermissionValidator; }Also mark the second constructor parameter in ReleaseHistoryController (the UnifiedPermissionValidator) as
finalfor consistency.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/CommitController.java (1)
21-46: LGTM on validator migrationConstructor, field, and guard updated correctly to unified validator; behavior remains consistent.
Also applies to: 54-56
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfigurationTest.java (1)
42-56: Assert filter ordering to prevent auth-type spoofing via headerAuthTypeResolverFilter should run after ConsumerAuthenticationFilter (or resolve via path) to avoid trusting a client-supplied header. Consider asserting/getting FilterRegistrationBean order here after setting explicit orders in AuthFilterConfiguration.
Do you want me to add an order assertion test once orders are defined in AuthFilterConfiguration?
Also applies to: 60-76
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java (1)
32-43: Consider adding a test for unset authType semanticsIf unset should default to USER, add a test documenting that. If not, ensure UnifiedPermissionValidator throws with a clear message.
Do you want me to draft that additional test?
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceBranchController.java (1)
82-92: SpEL bean name and method signatures are correct. The@Component("unifiedPermissionValidator")bean is named correctly, and it exposeshasModifyNamespacePermission(String,String,String,String),hasOperateNamespacePermission(...)(default on thePermissionValidatorinterface), andhasReleaseNamespacePermission(...)matching the SpEL references.apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionServiceTest.java (2)
69-139: Good coverage across edge casesThe added tests validate empty roles, roles without permissions, populated permissions, and isolation across users. Assertions are clear.
22-24: Unify JUnit usage
ConsumerRolePermissionServiceTest imports JUnit 4 (org.junit.Before/Test) while the surrounding suite uses JUnit 5 (org.junit.jupiter). Verify the build includes the Vintage engine for JUnit 4 or migrate this test to JUnit 5 for consistency.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java (1)
173-173: PreAuthorize migrated to unifiedPermissionValidator — LGTMSpEL guards consistently target UnifiedPermissionValidator as intended.
Also applies to: 198-198, 241-241, 265-265, 301-301, 321-321, 347-347, 367-367, 388-388, 400-400, 412-412, 439-439
apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java (4)
53-59: Test fixture setup is solidConstructor wiring and lenient default for consumerId retrieval keep tests stable.
107-125: Coverage for hasCreateApplicationPermission — LGTMPositive path assertions and interaction verifications are correct.
191-197: Unsupported manage-app-master behavior validatedGood to assert UnsupportedOperationException explicitly.
268-276: hasPermissions() edge cases comprehensively coveredIntersection, empty/null inputs, and empty/null user-permission set are all exercised.
Also applies to: 285-294, 300-308, 314-317, 323-331, 335-343
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java (2)
19-19: Swap to UnifiedPermissionValidator — LGTMConstructor and field updates look correct; downstream usage compiles to the unified API.
Also applies to: 69-69, 71-83
142-147: Permission check delegation — LGTMDelegating isAppAdmin to UnifiedPermissionValidator aligns with the unified architecture.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java (3)
44-61: Fixture and mocks — LGTMInjected mocks and lenient user stub are appropriate.
Also applies to: 68-76
79-101: CreateAppNamespace permission tests — LGTMPublic/private branches and config gating are validated well.
Also applies to: 103-123
127-137: Broad coverage for admin/super-admin and permission-set helpers — LGTMAssertions match expected behavior of the validator contract.
Also applies to: 139-178, 182-192, 196-221, 225-237, 241-261
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ReleaseController.java (1)
22-22: UnifiedPermissionValidator adoption — LGTMConstructor wiring, PreAuthorize on createRelease, visibility checks, and rollback guard are consistent.
Also applies to: 56-57, 60-70, 72-99, 139-143, 153-158, 168-173, 194-196
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java (3)
52-66: Constructor and field migration to UnifiedPermissionValidator looks goodDependency injection and field rename are consistent with the unified permission strategy.
80-86: Confirm correct permission for creating a branchUsing hasCreateNamespacePermission(#appId) may be broader than needed. Historically, branch creation typically aligns with modify/release permissions on a specific namespace/environment.
Would you like to switch to hasModifyNamespacePermission(#appId, #env, #clusterName, #namespaceName) or confirm the product decision behind app-scoped create permission here?
140-146: LGTM: permission guard for updating branch rulesUsing hasModifyNamespacePermission(...) matches the operation scope.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java (2)
75-76: LGTM: centralized @PreAuthorize guardsSwitching to @unifiedPermissionValidator across these endpoints is consistent with the new architecture.
Also applies to: 111-112, 138-139, 168-169
210-212: LGTM: runtime release permission check before rollbackInline permission validation with unified validator is correct.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (2)
32-34: Class refactor aligns with the new abstractionExtending AbstractPermissionValidator and implementing PermissionValidator is appropriate for unification.
74-89: LGTM: visibility logic defers to operate/role checksshouldHideConfigToCurrentUser(...) correctly composes member-only config, public namespace, and operate permission.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java (3)
79-99: Constructor migration to UnifiedPermissionValidator is consistentWiring and assignment are correct.
114-115: LGTM: reuse unified shouldHideConfigToCurrentUserCentralizing the visibility logic through the unified validator matches the new design.
Also applies to: 128-129
144-146: LGTM: create namespace guardApp-scoped create permission is appropriate here.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (1)
88-91: No action needed: API matches usage. getUserPermissionSet(long) is defined on ConsumerRolePermissionService and returns strings in the “permissionType:targetId” format, so the call in ConsumerPermissionValidator is correct.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java (1)
28-39: Shared namespace permission composition looks goodThe targetId construction and delegation are centralized cleanly.
Also applies to: 41-52
...ortal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java
Show resolved
Hide resolved
.../src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java
Outdated
Show resolved
Hide resolved
.../java/com/ctrip/framework/apollo/portal/audit/ApolloAuditLogQueryApiPortalPreAuthorizer.java
Show resolved
Hide resolved
...al/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java
Show resolved
Hide resolved
...ortal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java
Outdated
Show resolved
Hide resolved
...c/test/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidatorTest.java
Show resolved
Hide resolved
...c/test/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidatorTest.java
Show resolved
Hide resolved
...lo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerTest.java
Show resolved
Hide resolved
...st/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfigurationTest.java
Outdated
Show resolved
Hide resolved
.../consumer_role_permission_service/test_get_user_permission_set_roles_without_permissions.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
91-96: Add missing delegator: hasDeleteNamespacePermission(String appId)Controllers use @unifiedPermissionValidator.hasDeleteNamespacePermission(...). Without this, SpEL evaluation will fail at startup.
@Override public boolean hasManageAppMasterPermission(String appId) { return getDelegate().hasManageAppMasterPermission(appId); } + + // Required by controllers using @PreAuthorize on namespace delete operations + public boolean hasDeleteNamespacePermission(String appId) { + return getDelegate().hasDeleteNamespacePermission(appId); + } }
🧹 Nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/AuthConstants.java (1)
21-25: Centralize header key to avoid string duplicationExpose the ApolloConsumerId header name here so filters/controllers don’t hardcode it.
public final class AuthConstants { public static final String USER = "USER"; public static final String CONSUMER= "CONSUMER"; public static final String UNKNOWN = "UNKNOWN"; + /** HTTP header used to indicate OpenAPI consumer context */ + public static final String HEADER_APOLLO_CONSUMER_ID = "ApolloConsumerId";apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
36-45: Improve failure diagnostics for unknown auth typeInclude the actual auth type in the exception to aid debugging; keep fail-closed behavior.
- throw new IllegalStateException("Unknown authentication type"); + throw new IllegalStateException("Unknown authentication type: " + type);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
CHANGES.md(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/AuthConstants.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_different_users.sql(1 hunks)apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_no_roles.sql(1 hunks)apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_roles_without_permissions.sql(1 hunks)apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_with_permissions.sql(1 hunks)apollo-portal/src/test/resources/sql/permission/insert-test-getUserPermissionSet.sql(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- CHANGES.md
- apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_roles_without_permissions.sql
🚧 Files skipped from review as they are similar to previous changes (5)
- apollo-portal/src/test/resources/sql/permission/insert-test-getUserPermissionSet.sql
- apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_no_roles.sql
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java
- apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_different_users.sql
- apollo-portal/src/test/resources/sql/permission/consumer_role_permission_service/test_get_user_permission_set_with_permissions.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/AuthConstants.java (1)
26-26: Utility class pattern correctly appliedPrivate constructor prevents instantiation. LGTM.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
26-34: Constructor injection and bean naming look goodBean name "unifiedPermissionValidator" matches typical SpEL usage in controllers. No issues.
...al/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (1)
115-126: Fix NPE: null-safe requiredPerms in hasPermissions
requiredPerms.isEmpty()will NPE ifrequiredPermsis null (noted in a prior review). Also minor brace/indent tidy.@Override protected boolean hasPermissions(List<String> requiredPerms) { - String userId = userInfoHolder.getUser().getUserId(); - if (requiredPerms.isEmpty()) { + if (requiredPerms == null || requiredPerms.isEmpty()) { return false; } + String userId = userInfoHolder.getUser().getUserId(); Set<String> permissions = rolePermissionService.getUserPermissionSet(userId); if (permissions == null) { - permissions = Collections.emptySet(); - } + permissions = Collections.emptySet(); + } return requiredPerms.stream().anyMatch(permissions::contains); }Also consider mirroring this null/empty handling in ConsumerPermissionValidator for symmetry.
🧹 Nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (2)
29-32: Imports look goodCollections/List/Set are used below. If you adopt the null-precondition suggestion for hasPermission, also add
import java.util.Objects;.
110-114: Add preconditions to avoid silent misuse in hasPermissionGuard
targetIdandpermissionType(fail-fast) to keep invariants tight.+import java.util.Objects; @@ @Override protected boolean hasPermission(String targetId, String permissionType) { - return rolePermissionService.userHasPermission(userInfoHolder.getUser().getUserId(), permissionType, targetId); + Objects.requireNonNull(targetId, "targetId"); + Objects.requireNonNull(permissionType, "permissionType"); + return rolePermissionService.userHasPermission( + userInfoHolder.getUser().getUserId(), permissionType, targetId); }Please confirm callers never pass nulls; otherwise keep the guards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidatorTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidatorTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
Component(26-96)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (1)
Component(33-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (1)
34-34: Class wiring LGTMExtends the new base and implements PermissionValidator; matches UnifiedPermissionValidator expectations.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java (1)
170-174: Resource leak and swallowed failures: close streams and fail the test on exceptions.
FileOutputStream, FileInputStream, and ZipInputStream aren’t closed; and exceptions are printed then ignored. This can cause flakiness (e.g., file locks on Windows) and hide real failures.Apply:
- FileOutputStream fileOutputStream = new FileOutputStream(filePath); - - configsExportService.exportData(fileOutputStream, Lists.newArrayList(Env.DEV)); + try (FileOutputStream fileOutputStream = new FileOutputStream(filePath)) { + configsExportService.exportData(fileOutputStream, Lists.newArrayList(Env.DEV)); + } @@ - FileInputStream fileInputStream = new FileInputStream(filePath); - ZipInputStream zipInputStream = new ZipInputStream(fileInputStream); - - try { - configsImportService.importDataFromZipFile(Lists.newArrayList(Env.DEV), zipInputStream, false); - } catch (Exception e) { - e.printStackTrace(); - } + try (FileInputStream fileInputStream = new FileInputStream(filePath); + ZipInputStream zipInputStream = new ZipInputStream(fileInputStream)) { + configsImportService.importDataFromZipFile(Lists.newArrayList(Env.DEV), zipInputStream, false); + } catch (Exception e) { + org.junit.Assert.fail("importDataFromZipFile threw an unexpected exception: " + e.getMessage()); + }Also applies to: 192-199
🧹 Nitpick comments (3)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java (2)
116-116: Avoid hard-coded permission target; build it from existing test constants.Reduces duplication and keeps the test resilient to appId/namespaceName changes.
-when(rolePermissionService.getUserPermissionSet("luke")).thenReturn(Collections.singleton("ModifyNamespace:testApp+application")); +when(rolePermissionService.getUserPermissionSet("luke")) + .thenReturn(Collections.singleton("ModifyNamespace:" + appId + "+" + namespaceName));Optional: if the validator expects a non-null set for unknown users, add a default stub to prevent NPEs:
when(rolePermissionService.getUserPermissionSet(org.mockito.ArgumentMatchers.anyString())) .thenReturn(java.util.Collections.emptySet());Please confirm the unified permission descriptor is exactly "ModifyNamespace:+" for create-item.
51-51: LGTM on the Collections import.Needed for the singleton set usage. If the project baseline is Java 9+, consider Set.of(...) as a minor cleanup.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java (1)
220-223: Redundant assignment to orgName.
setOrgName is called twice; drop the first to reduce noise.- app.setOrgName("apollo"); app.setOrgId(orgId); - app.setOrgName(orgName); + app.setOrgName(orgName);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java(4 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (4)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java (1)
32-32: LGTM on introducing RolePermissionService to the test.This aligns the test with the unified permission flow.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/ConfigsExportServiceTest.java (3)
26-26: LGTM: import aligns tests with unified permission layer.
188-191: Scope permission stubs to specific inputs or add negative‐case tests
Over-broadwhen(...).thenReturn(true)stubs can mask permission regressions—restrict them to expected appId/namespace values or add a test case asserting failure when permissions are denied.Request manual verification that
UnifiedPermissionValidatoractually defines anisAppAdmin(...)method matching your test stub, since its signature wasn’t located in the codebase.
85-87: Remove unused UserPermissionValidator mock
The test neither ConfigsExportService (which uses UnifiedPermissionValidator) nor ConfigsImportService (which has no validator dependency) ever uses UserPermissionValidator—delete the@Mock private UserPermissionValidator userPermissionValidator;.Likely an incorrect or invalid review comment.
| @Autowired | ||
| private RolePermissionService rolePermissionService; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Mock RolePermissionService instead of autowiring; otherwise Mockito when() will throw.
You call when(rolePermissionService.getUserPermissionSet("luke")), which requires a Mockito mock. With @Autowired this is a real bean and will cause NotAMockException at runtime. Use @MockBean and add its import.
@@
-import com.ctrip.framework.apollo.portal.service.RolePermissionService;
+import com.ctrip.framework.apollo.portal.service.RolePermissionService;
+import org.springframework.boot.test.mock.mockito.MockBean;
@@
- @Autowired
- private RolePermissionService rolePermissionService;
+ @MockBean
+ private RolePermissionService rolePermissionService;Also ensure ItemService is a mock (e.g., @MockBean) wherever you use verify(itemService)...; otherwise verification on a real bean will fail.
Also applies to: 32-32
🏁 Script executed:
#!/bin/bash
# Display the first 120 lines of the test class to inspect annotations and fields
sed -n '1,120p' apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.javaLength of output: 4932
🏁 Script executed:
#!/bin/bash
# Display lines 121-250 to check for verify(itemService) usage
sed -n '121,250p' apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.javaLength of output: 517
Use @MockBean for RolePermissionService and ItemService
Replace @Autowired with @MockBean so that Mockito when() and verify() operate on mocks, not real beans.
+ import org.springframework.boot.test.mock.mockito.MockBean;
@@
- @Autowired
- private RolePermissionService rolePermissionService;
+ @MockBean
+ private RolePermissionService rolePermissionService;
@@
- @Autowired
- private ItemService itemService;
+ @MockBean
+ private ItemService itemService;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Autowired | |
| private RolePermissionService rolePermissionService; | |
| // add this import alongside your other test imports | |
| import org.springframework.boot.test.mock.mockito.MockBean; | |
| ... | |
| public class ItemControllerAuthIntegrationTest { | |
| // replace @Autowired with @MockBean for RolePermissionService | |
| @MockBean | |
| private RolePermissionService rolePermissionService; | |
| // replace @Autowired with @MockBean for ItemService | |
| @MockBean | |
| private ItemService itemService; | |
| // ... rest of your test class ... | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java
around lines 73-75, the test currently uses @Autowired for RolePermissionService
(and should also for ItemService) which gives real beans instead of Mockito
mocks; replace the @Autowired annotations with @MockBean for
RolePermissionService and for ItemService, add the corresponding
org.springframework.boot.test.mock.mockito.MockBean import if missing, and keep
the existing field names so Mockito.when(...) and Mockito.verify(...) operate on
those mocks in the test setup and assertions.
|
cc @spaceluke |
spaceluke
left a comment
spaceluke
left a comment
There was a problem hiding this comment.
Congrats, most of the work has been done, but there are still some areas that need to be modified~
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/AuthConstants.java
Show resolved
Hide resolved
.../src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java
Outdated
Show resolved
Hide resolved
...l/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AuthContextHolder.java
Show resolved
Hide resolved
...lo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/AuthTypeResolverFilter.java
Outdated
Show resolved
Hide resolved
...-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java
Outdated
Show resolved
Hide resolved
...java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRoleInitializationService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java (1)
54-80: Reduce DB roundtrips: reuse permission set in consumerHasPermissionYou can implement this check via the new getUserPermissionSet(consumerId), collapsing 3 queries to 1.
- public boolean consumerHasPermission(long consumerId, String permissionType, String targetId) { - Permission permission = - permissionRepository.findTopByPermissionTypeAndTargetId(permissionType, targetId); - if (permission == null) { - return false; - } - - List<ConsumerRole> consumerRoles = consumerRoleRepository.findByConsumerId(consumerId); - if (CollectionUtils.isEmpty(consumerRoles)) { - return false; - } - - Set<Long> roleIds = - consumerRoles.stream().map(ConsumerRole::getRoleId).collect(Collectors.toSet()); - List<RolePermission> rolePermissions = rolePermissionRepository.findByRoleIdIn(roleIds); - if (CollectionUtils.isEmpty(rolePermissions)) { - return false; - } - - for (RolePermission rolePermission : rolePermissions) { - if (rolePermission.getPermissionId() == permission.getId()) { - return true; - } - } - - return false; - } + public boolean consumerHasPermission(long consumerId, String permissionType, String targetId) { + String key = permissionType + ":" + targetId; + return getUserPermissionSet(consumerId).contains(key); + }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (1)
72-88: Fix: missing hasOperateNamespacePermission leads to compile errorThis calls hasOperateNamespacePermission(...), but AbstractPermissionValidator doesn’t define it. Either add it to AbstractPermissionValidator (preferred) or inline “modify || release” here.
Apply in AbstractPermissionValidator (see patch in that file’s comment).
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (2)
60-64: Same: avoid throwing in shouldHideConfigToCurrentUserReturn false to maintain non-breaking behavior.
Apply:
@Override public boolean shouldHideConfigToCurrentUser(String appId, String env, String clusterName, String namespaceName) { - throw new UnsupportedOperationException("Not supported operation"); + return false; }
72-75: Same: avoid throwing in hasManageAppMasterPermissionReturn false rather than throwing.
Apply:
@Override public boolean hasManageAppMasterPermission(String appId) { - throw new UnsupportedOperationException("Not supported operation"); + return false; }
♻️ Duplicate comments (3)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java (1)
76-81: Use @MockBean for beans you stub/verify
when(rolePermissionService...)andverify(itemService...)require Mockito mocks, not real beans. Replace @Autowired with @MockBean.-import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.mock.mockito.MockBean; @@ - @Autowired - private ItemService itemService; + @MockBean + private ItemService itemService; @@ - @Autowired - private RolePermissionService rolePermissionService; + @MockBean + private RolePermissionService rolePermissionService;apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (1)
108-119: Guard requiredPerms against null; minor formattingrequiredPerms can be null from upstream; also fix indentation in the null-to-empty conversion.
Apply:
@Override protected boolean hasPermissions(List<String> requiredPerms) { - String userId = userInfoHolder.getUser().getUserId(); - if (requiredPerms.isEmpty()) { + if (requiredPerms == null || requiredPerms.isEmpty()) { return false; } + String userId = userInfoHolder.getUser().getUserId(); Set<String> permissions = rolePermissionService.getUserPermissionSet(userId); - if (permissions == null) { - permissions = Collections.emptySet(); - } + if (permissions == null) { + permissions = Collections.emptySet(); + } return requiredPerms.stream().anyMatch(permissions::contains); }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (1)
47-50: Don’t throw in permission checks; return falseThrowing UnsupportedOperationException will surface as 500s via UnifiedPermissionValidator. Return a safe default instead.
Apply:
@Override public boolean hasCreateAppNamespacePermission(String appId, AppNamespace appNamespace) { - throw new UnsupportedOperationException("Not supported operation"); + return false; }
🧹 Nitpick comments (13)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java (1)
56-59: Clear SecurityContext after the test; also verify necessity with skipAuthorization.
- With @activeprofiles("skipAuthorization"), this Authentication won’t propagate to server threads handling RestTemplate calls and is likely unused. Keep it only if you plan to enable auth here.
- Add teardown to avoid context leakage across tests.
Add after-method cleanup and its import:
import org.junit.After; @After public void tearDown() { SecurityContextHolder.clearContext(); }Optional: for clarity, consider aligning the principal with the test’s
user(“apollo”) or drop the injection if authorization remains skipped.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstans.java (2)
22-24: Prefer enum for identity over raw StringsEnums (or a sealed type) avoid typos and enable switch exhaustiveness. Keep String constants for backward compatibility if needed.
Example:
public enum UserIdentity { USER, CONSUMER, ANONYMOUS }
22-24: Minor style consistencyAlign spacing around '=' for constants per project styleguide.
- public static final String USER = "USER"; - public static final String CONSUMER= "CONSUMER"; + public static final String USER = "USER"; + public static final String CONSUMER = "CONSUMER";apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
21-21: Consider withInitial for cleaner defaultingAlternative to the explicit null-check: initialize ThreadLocal with ANONYMOUS.
- private static final ThreadLocal<String> AUTH_TYPE_HOLDER = new ThreadLocal<>(); + private static final ThreadLocal<String> AUTH_TYPE_HOLDER = + ThreadLocal.withInitial(() -> UserIdentityConstants.ANONYMOUS);apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RolePermissionService.java (1)
96-97: Clarify contract: non-null result and key formatDocument that the method never returns null and specify the delimiter to avoid ambiguity and drift between implementations.
- // Get all permissions of the user (set of permissionType + targetId) - Set<String> getUserPermissionSet(String userId); + /** + * Returns all permissions of the user as a Set of keys formatted "permissionType:targetId". + * Must never return null; return Collections.emptySet() when none. + */ + Set<String> getUserPermissionSet(String userId);apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java (1)
382-384: Centralize permission key formattingAvoid duplicating the "type:targetId" convention across services; extract a small util or a value object to prevent delimiter drift and collisions.
Example:
public final class PermissionKey { public static String of(Permission p) { return p.getPermissionType() + ":" + p.getTargetId(); } }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java (2)
82-92: Method naming: clarify that this is for consumersRename for clarity and symmetry with the user-side API.
-public Set<String> getUserPermissionSet(long consumerId) { +public Set<String> getConsumerPermissionSet(long consumerId) {I can prep a follow-up patch to update call sites.
89-91: Deduplicate key formatting with shared helperSame suggestion as in DefaultRolePermissionService to avoid drift.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolderTest.java (1)
27-33: Simplify setup; rely on clear() and add @AfterEach for symmetrysetAuthType(null) is redundant before clear(). Add @AfterEach to guarantee cleanup even if a test fails early.
import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.AfterEach; @@ public void setUp() { - // Clear ThreadLocal before each test - UserIdentityContextHolder.setAuthType(null); - // Clear any existing values before each test - UserIdentityContextHolder.clear(); + UserIdentityContextHolder.clear(); } + + @AfterEach + public void tearDown() { + UserIdentityContextHolder.clear(); + }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java (1)
64-82: Return only required columns via projection to reduce I/OIf callers only build “permissionType:targetId”, project those fields instead of full entities.
Example:
public interface PermissionBrief { String getPermissionType(); String getTargetId(); } @Query("SELECT DISTINCT p.permissionType AS permissionType, p.targetId AS targetId ...") List<PermissionBrief> findUserPermissionsBrief(@Param("userId") String userId);apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java (1)
97-134: Namespace permission delegation tests are solid; consider ANONYMOUS caseOptional: add an explicit ANONYMOUS test mirroring UNKNOWN to lock behavior.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (1)
49-52: Guard against empty consumer headerTreat blank header as absent.
- if (req.getHeader(CONSUMER_ID) != null) { + String consumerId = req.getHeader(CONSUMER_ID); + if (consumerId != null && !consumerId.isEmpty()) { return UserIdentityConstans.CONSUMER; }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (1)
78-89: Minor: short-circuit on empty requiredPermsSkip service lookup when requiredPerms is empty.
Apply:
@Override protected boolean hasPermissions(List<String> requiredPerms) { - if (requiredPerms == null){ + if (requiredPerms == null || requiredPerms.isEmpty()){ return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java(6 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstans.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RolePermissionService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolderTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java(4 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java(2 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfigurationTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfigurationTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidatorTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java
🧰 Additional context used
🧬 Code graph analysis (5)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolderTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(19-41)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-124)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(19-41)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstans.java (1)
UserIdentityConstans(21-27)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstans.java (1)
UserIdentityConstans(21-27)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(19-41)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
Component(26-96)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/audit/ApolloAuditLogQueryApiPortalPreAuthorizer.java (1)
Component(24-38)
🔇 Additional comments (12)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/PermissionControllerTest.java (2)
32-35: Security test imports are appropriate for establishing an authenticated context.These additions align with the unified permission flow. No issues spotted.
39-39: Collections import is fine.Used appropriately for a singleton authority list.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
21-21: setAuthType and clear are already paired in a try/finally. The filter wrapsfilterChain.doFilterand callsUserIdentityContextHolder.clear()in itsfinallyblock, so no changes are needed.apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RolePermissionService.java (1)
96-97: All RolePermissionService implementors provide getUserPermissionSet
DefaultRolePermissionService implements the new API; no missing implementations found.apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolderTest.java (1)
35-74: Good coverage of ThreadLocal lifecycleCovers set/get/clear (null and non-null). LGTM.
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java (1)
142-146: Ensure UserInfoHolder is also a mockYou stub
userInfoHolder.getUser(). If it’s not already a@MockBeanelsewhere, convert it similarly to avoidNotAMockException.- @Autowired - private UserInfoHolder userInfoHolder; + @MockBean + private UserInfoHolder userInfoHolder;apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java (1)
57-95: Delegation for manage master permission looks correctUSER → userPermissionValidator, CONSUMER → consumerPermissionValidator, UNKNOWN → throws. LGTM.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (1)
99-106: Sanity-check semantics of “manage app master” gatingThe additional hasAssignRolePermission(appId) gate changes behavior unless it existed before. Please confirm no regressions for app admins who previously could manage masters without ASSIGN_ROLE.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java (3)
29-41: LGTM: modify namespace permission compositionThe three-path composition (namespace-global, env-specific, cluster-scope) aligns with expected any-of semantics.
42-53: LGTM: release namespace permission compositionMirrors modify logic correctly.
87-107: Default-false stubs are good; ensure UnifiedPermissionValidator delegates everywhereWith false defaults here, delegation must override where needed (user/consumer). Confirm all PermissionValidator methods are implemented in delegates to prevent unexpected false negatives.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (1)
33-43: Ignore typo alert in UnifiedPermissionValidator wiring
No occurrences ofconsumerPermission_validatorwere found; the field is correctly namedconsumerPermissionValidatorand delegation is wired properly.Likely an incorrect or invalid review comment.
| @Override | ||
| public boolean hasCreateAppNamespacePermission(String appId, AppNamespace appNamespace) { | ||
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean hasCreateClusterPermission(String appId) { | ||
| List<String> requiredPerms = Collections.singletonList( | ||
| PermissionType.CREATE_CLUSTER + ":" + | ||
| appId | ||
| ); | ||
| return hasPermissions(requiredPerms); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add operate permission aggregator to keep API parity
UserPermissionValidator references hasOperateNamespacePermission(...). Provide a default that composes modify OR release to avoid duplication and keep compatibility.
Apply:
@Override
public boolean hasCreateClusterPermission(String appId) {
List<String> requiredPerms = Collections.singletonList(
PermissionType.CREATE_CLUSTER + ":" +
appId
);
return hasPermissions(requiredPerms);
}
+ @Override
+ public boolean hasOperateNamespacePermission(String appId, String env, String clusterName, String namespaceName) {
+ // Operate = modify OR release
+ return hasModifyNamespacePermission(appId, env, clusterName, namespaceName)
+ || hasReleaseNamespacePermission(appId, env, clusterName, namespaceName);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public boolean hasCreateAppNamespacePermission(String appId, AppNamespace appNamespace) { | |
| return false; | |
| } | |
| @Override | |
| public boolean hasCreateClusterPermission(String appId) { | |
| List<String> requiredPerms = Collections.singletonList( | |
| PermissionType.CREATE_CLUSTER + ":" + | |
| appId | |
| ); | |
| return hasPermissions(requiredPerms); | |
| } | |
| @Override | |
| public boolean hasCreateClusterPermission(String appId) { | |
| List<String> requiredPerms = Collections.singletonList( | |
| PermissionType.CREATE_CLUSTER + ":" + | |
| appId | |
| ); | |
| return hasPermissions(requiredPerms); | |
| } | |
| @Override | |
| public boolean hasOperateNamespacePermission(String appId, String env, String clusterName, String namespaceName) { | |
| // Operate = modify OR release | |
| return hasModifyNamespacePermission(appId, env, clusterName, namespaceName) | |
| || hasReleaseNamespacePermission(appId, env, clusterName, namespaceName); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java
around lines 73–86, add a default implementation for
hasOperateNamespacePermission(...) to preserve API parity: implement it to
return true if the caller has either MODIFY_NAMESPACE or RELEASE_NAMESPACE
permission for the given appId/namespace (compose an OR of the two checks). Use
the existing hasPermissions(...) helper (or calls to it) to check the MODIFY and
RELEASE permission strings for the provided appId and namespace and return the
logical OR of those checks.
...tal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java
Outdated
Show resolved
Hide resolved
...lo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstans.java
Outdated
Show resolved
Hide resolved
...lo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java
Outdated
Show resolved
Hide resolved
...-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java
Outdated
Show resolved
Hide resolved
...-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java
Outdated
Show resolved
Hide resolved
...ain/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (1)
53-56: Good fix: exclude AnonymousAuthenticationTokenThe additional check avoids classifying anonymous requests as USER. This resolves the earlier concern.
🧹 Nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (2)
49-51: Header-only CONSUMER detection: verify trust boundary and filter orderingClassifying as CONSUMER solely by presence of
ApolloConsumerIdis spoofable. Please confirm:
- A preceding auth filter validates consumer credentials and sets a verified signal; or
- Permission checks do not grant access based only on this classification.
Consider tightening:
- Check header has text and a verification attribute set by the consumer auth filter.
Example:
- if (req.getHeader(CONSUMER_ID) != null) { + if (org.springframework.util.StringUtils.hasText(req.getHeader(CONSUMER_ID)) + && Boolean.TRUE.equals(req.getAttribute("ApolloConsumerVerified"))) { return UserIdentityConstans.CONSUMER; }
33-33: Encapsulate constantMake the constant explicitly private.
-static final String CONSUMER_ID = "ApolloConsumerId"; +private static final String CONSUMER_ID = "ApolloConsumerId";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java(4 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(19-41)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstans.java (1)
UserIdentityConstans(21-27)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (1)
UserTypeResolverFilter(32-61)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(19-41)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstans.java (1)
UserIdentityConstans(21-27)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (1)
UserTypeResolverFilter(29-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
...lo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java
Show resolved
Hide resolved
| @Override | ||
| protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { | ||
| UserIdentityContextHolder.setAuthType(UserIdentityConstans.USER); | ||
| filterChain.doFilter(request, response); | ||
| } |
There was a problem hiding this comment.
Fix ThreadLocal leak: clear in finally
UserIdentityContextHolder is never cleared, risking cross-test contamination under thread reuse.
Apply:
- protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException {
- UserIdentityContextHolder.setAuthType(UserIdentityConstans.USER);
- filterChain.doFilter(request, response);
- }
+ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
+ throws ServletException, IOException {
+ UserIdentityContextHolder.setAuthType(UserIdentityConstans.USER);
+ try {
+ filterChain.doFilter(request, response);
+ } finally {
+ UserIdentityContextHolder.clear();
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { | |
| UserIdentityContextHolder.setAuthType(UserIdentityConstans.USER); | |
| filterChain.doFilter(request, response); | |
| } | |
| @Override | |
| protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) | |
| throws ServletException, IOException { | |
| UserIdentityContextHolder.setAuthType(UserIdentityConstans.USER); | |
| try { | |
| filterChain.doFilter(request, response); | |
| } finally { | |
| UserIdentityContextHolder.clear(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java
around lines 31 to 35, the filter sets
UserIdentityContextHolder.setAuthType(...) but never clears it, risking
ThreadLocal leakage across reused threads; change the method to set the auth
type, call filterChain.doFilter(...) inside a try block and always clear the
ThreadLocal in a finally block (e.g., call UserIdentityContextHolder.clear() or
equivalent) so the context is removed regardless of success or exception.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (2)
29-35: Do not shadow the production filter with the same FQCN in test sourcesThis masks the real implementation on the test classpath; tests won’t exercise production logic.
Rename and relocate the test stub, then wire it via @TestConfiguration if needed:
-package com.ctrip.framework.apollo.portal.filter; +package com.ctrip.framework.apollo.portal.filter.test; @@ -public class UserTypeResolverFilter extends OncePerRequestFilter { +public class TestUserTypeResolverFilter extends OncePerRequestFilter {
31-35: Fix ThreadLocal leak: clear context in finallyAlways clear UserIdentityContextHolder to avoid cross-test contamination.
- protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { - UserIdentityContextHolder.setAuthType(UserIdentityConstants.USER); - filterChain.doFilter(request, response); - } + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) + throws ServletException, IOException { + UserIdentityContextHolder.setAuthType(UserIdentityConstants.USER); + try { + filterChain.doFilter(request, response); + } finally { + UserIdentityContextHolder.clear(); + } + }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java (1)
74-82: Mirror change looks good; apply same refinements if you accept them above.
Portable JPQL form is correct. Optionally switch to EXISTS and drop DISTINCT; also ensure matching indexes (ConsumerRole[consumerId, isDeleted, roleId]).Suggested rewrite:
- @Query("SELECT DISTINCT p " + - "FROM ConsumerRole cr, RolePermission rp, Permission p " + - "WHERE cr.roleId = rp.roleId " + - " AND rp.permissionId = p.id " + - " AND cr.consumerId = :consumerId " + - " AND cr.isDeleted = false " + - " AND rp.isDeleted = false " + - " AND p.isDeleted = false") + @Query("SELECT p " + + "FROM Permission p " + + "WHERE p.isDeleted = false " + + " AND EXISTS ( " + + " SELECT 1 FROM RolePermission rp " + + " WHERE rp.permissionId = p.id " + + " AND rp.isDeleted = false " + + " AND EXISTS ( " + + " SELECT 1 FROM ConsumerRole cr " + + " WHERE cr.roleId = rp.roleId " + + " AND cr.consumerId = :consumerId " + + " AND cr.isDeleted = false " + + " ) " + + " )") List<Permission> findConsumerPermissions(@Param("consumerId") long consumerId);
🧹 Nitpick comments (9)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstants.java (2)
21-27: Prefer a type-safe identity over raw stringsConsider an enum (or sealed hierarchy) for USER/CONSUMER/ANONYMOUS to prevent typos and enable switches. If string interop is required, keep constants but expose a parse/validate helper.
22-24: Normalize spacing for consistencyMinor style nit to align assignments.
- public static final String USER = "USER"; - public static final String CONSUMER= "CONSUMER"; + public static final String USER = "USER"; + public static final String CONSUMER = "CONSUMER";apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java (7)
45-49: Remove no-op @beforeeachIt adds noise without value.
- @BeforeEach - public void setUp() { - // No operation needed, UserIdentityContextHolder state will be set separately in each test - } + // setUp not needed
30-31: Enable interaction verificationsUse Mockito wildcard import to allow verify/never/noInteractions.
-import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*;
57-70: Verify delegation and absence of cross-calls (USER -> user validator only)Add interaction checks to harden the test intent.
boolean result = unifiedPermissionValidator.hasManageAppMasterPermission(appId); assertTrue(result); + verify(userPermissionValidator).hasManageAppMasterPermission(appId); + verifyNoInteractions(consumerPermissionValidator);
71-84: Verify delegation and absence of cross-calls (CONSUMER -> consumer validator only)boolean result = unifiedPermissionValidator.hasManageAppMasterPermission(appId); assertFalse(result); + verify(consumerPermissionValidator).hasManageAppMasterPermission(appId); + verifyNoInteractions(userPermissionValidator);
97-109: Verify delegation for create-namespace (USER) and assert no cross-callboolean result = unifiedPermissionValidator.hasCreateNamespacePermission(appId); assertTrue(result); + verify(userPermissionValidator).hasCreateNamespacePermission(appId); + verifyNoInteractions(consumerPermissionValidator);
111-123: Verify delegation for create-namespace (CONSUMER) and assert no cross-callboolean result = unifiedPermissionValidator.hasCreateNamespacePermission(appId); assertTrue(result); + verify(consumerPermissionValidator).hasCreateNamespacePermission(appId); + verifyNoInteractions(userPermissionValidator);
125-133: Add tests for ANONYMOUS and null authTypeDocument expected behavior when authType is ANONYMOUS or unset (null): deny vs throw. Add tests accordingly to lock contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstants.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java
🧰 Additional context used
🧬 Code graph analysis (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(19-41)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstants.java (1)
UserIdentityConstants(21-27)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (1)
UserTypeResolverFilter(32-61)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstants.java (1)
UserIdentityConstants(21-27)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(19-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (5)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidatorTest.java (2)
51-55: Good: context cleanup after each testClearing UserIdentityContextHolder avoids state bleed across tests.
85-96: Good: unknown auth type path is asserted to throwCovers the exceptional branch.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java (3)
24-24: Import is correct and necessary for named parameters.
The added Param import is required by the new repository methods.
62-63: No-op change.
Whitespace-only; nothing to review.
64-72: Prefer EXISTS over DISTINCT semi-join
Replace the cross-join +DISTINCTwith anEXISTS-based JPQL (as in the prior diff) to express the semi-join more clearly and dropDISTINCT.Add/verify composite indexes for performance
Ensure your schema (via migration scripts or JPA @table(indexes=…)) defines composite indexes on:
- UserRole(user_id, is_deleted, role_id)
- RolePermission(role_id, is_deleted, permission_id)
- Permission(id, is_deleted)
spaceluke
left a comment
spaceluke
left a comment
There was a problem hiding this comment.
Something needs to be fixed~
...ortal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java
Outdated
Show resolved
Hide resolved
...lo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java
Show resolved
Hide resolved
...-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/po/Permission.java (1)
37-49: Avoid equals/hashCode on mutable fields; make columns immutableequals/hashCode rely on permissionType/targetId, but setters allow mutation, which can corrupt HashSet/Map membership. Mark columns immutable at the DB layer to reduce risk.
- @Column(name = "`PermissionType`", nullable = false) + @Column(name = "`PermissionType`", nullable = false, updatable = false) private String permissionType; - @Column(name = "`TargetId`", nullable = false) + @Column(name = "`TargetId`", nullable = false, updatable = false) private String targetId;If these fields must be mutable, consider not using entity instances for Set membership and compare via a dedicated value object instead.
♻️ Duplicate comments (7)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java (2)
71-78: Use @MockBean instead of @Autowired for stubbing and verificationMockito when()/verify() on real Spring beans will throw. Mock RolePermissionService, ItemService, and UserInfoHolder.
- @Autowired - private UserInfoHolder userInfoHolder; - @Autowired - private ItemService itemService; + @org.springframework.boot.test.mock.mockito.MockBean + private UserInfoHolder userInfoHolder; + @org.springframework.boot.test.mock.mockito.MockBean + private ItemService itemService; - @Autowired - private RolePermissionService rolePermissionService; + @org.springframework.boot.test.mock.mockito.MockBean + private RolePermissionService rolePermissionService;Also add the missing import:
+import org.springframework.boot.test.mock.mockito.MockBean;
118-120: Stubbing requires mocksThis when(...) requires rolePermissionService to be a @MockBean; same for userInfoHolder used in setUserId(...).
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java (2)
64-73: Prefer JPQL that’s portable across providers (no JOIN ... ON across unrelated entities)Hibernate supports JOIN ... ON here, but portability is poor. Rewrite as cross joins with WHERE conditions (or nativeQuery).
- @Query("SELECT DISTINCT p " + - "FROM UserRole ur " + - "JOIN RolePermission rp ON ur.roleId = rp.roleId " + - "JOIN Permission p ON rp.permissionId = p.id " + - "WHERE ur.userId = :userId " + - "AND ur.isDeleted = false " + - "AND rp.isDeleted = false " + - "AND p.isDeleted = false") + @Query("SELECT DISTINCT p " + + "FROM UserRole ur, RolePermission rp, Permission p " + + "WHERE ur.roleId = rp.roleId " + + " AND rp.permissionId = p.id " + + " AND ur.userId = :userId " + + " AND ur.isDeleted = false " + + " AND rp.isDeleted = false " + + " AND p.isDeleted = false") List<Permission> findUserPermissions(@Param("userId") String userId);
74-83: Mirror the portable JPQL for consumer queryApply the same rewrite for the consumer variant.
- @Query("SELECT DISTINCT p " + - "FROM ConsumerRole cr " + - "JOIN RolePermission rp ON cr.roleId = rp.roleId " + - "JOIN Permission p ON rp.permissionId = p.id " + - "WHERE cr.consumerId = :consumerId " + - "AND cr.isDeleted = false " + - "AND rp.isDeleted = false " + - "AND p.isDeleted = false") + @Query("SELECT DISTINCT p " + + "FROM ConsumerRole cr, RolePermission rp, Permission p " + + "WHERE cr.roleId = rp.roleId " + + " AND rp.permissionId = p.id " + + " AND cr.consumerId = :consumerId " + + " AND cr.isDeleted = false " + + " AND rp.isDeleted = false " + + " AND p.isDeleted = false") List<Permission> findConsumerPermissions(@Param("consumerId") long consumerId);apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (3)
61-65: Don’t throw from visibility checks; return false (no hide) insteadThrowing here will surface as a 500 when UnifiedPermissionValidator delegates to consumer; defaulting to false preserves backward compatibility.
@Override public boolean shouldHideConfigToCurrentUser(String appId, String env, String clusterName, String namespaceName) { - throw new UnsupportedOperationException("Not supported operation"); + return false; }(Alternatively, remove the override and inherit the base false.)
73-76: Return false instead of throwing for manage-app-master permissionKeep behavior non-breaking for OpenAPI consumers; throwing will cause unnecessary 500s.
@Override public boolean hasManageAppMasterPermission(String appId) { - throw new UnsupportedOperationException("Not supported operation"); + return false; }(Or drop the override to inherit base false.)
49-51: Fail-closed without throwing to avoid 500s via UnifiedPermissionValidatorReturning false keeps semantics consistent with AbstractPermissionValidator defaults and prevents runtime 500s when the consumer path is hit.
Apply either minimal or cleaner fix:
@Override public boolean hasCreateAppNamespacePermission(String appId, AppNamespace appNamespace) { - throw new UnsupportedOperationException("Not supported operation"); + return false; }Or delete this override entirely to inherit the safe default (false) from AbstractPermissionValidator.
🧹 Nitpick comments (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RolePermissionService.java (1)
95-97: Clarify ANY-of semantics and null contract for new APIDocument that this returns true if the user has any of the provided permissions, and define behavior for null/empty input to avoid ambiguity with existing userHasPermission(...).
void deleteRolePermissionsByCluster(String appId, String env, String clusterName, String operator); - boolean checkUserHasPermission(String userId, List<Permission> permissions); + /** + * Returns true iff the user has any permission in the given list. + * Semantics: ANY-of. For null/empty input, implementations should return false. + */ + boolean checkUserHasPermission(String userId, List<Permission> permissions);apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java (1)
54-55: Remove unused importjava.util.Collections is unused.
-import java.util.Collections;apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidatorTest.java (1)
126-166: Add a test proving ANY-of semantics (single permission suffices)Strengthen coverage with a case where only one required permission is granted.
@Test public void testHasModifyNamespacePermission_WithSingleGrantedPermission() { String appId = "testApp", env = "DEV", clusterName = "default", ns = "application"; List<Permission> granted = java.util.Collections.singletonList( new Permission(PermissionType.MODIFY_NAMESPACE, RoleUtils.buildNamespaceTargetId(appId, ns)) ); AbstractPermissionValidator validator = new AbstractPermissionValidatorWithPermissionsImpl(granted); org.junit.Assert.assertTrue(validator.hasModifyNamespacePermission(appId, env, clusterName, ns)); }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (2)
28-31: Remove unused importsjava.util.Collections and java.util.Set are unused.
-import java.util.Collections; import java.util.List; -import java.util.Set;
109-117: Defensive null check on requiredPermsGuard against null to prevent NPE if future callers pass null.
@Override protected boolean hasPermissions(List<Permission> requiredPerms) { String userId = userInfoHolder.getUser().getUserId(); - if (requiredPerms.isEmpty()) { + if (requiredPerms == null || requiredPerms.isEmpty()) { return false; } return rolePermissionService.checkUserHasPermission(userId, requiredPerms); }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java (1)
79-87: Provide operate permission aggregator (modify OR release)Centralize the common “operate namespace” check to reduce duplication and keep API parity with callers.
@Override public boolean isSuperAdmin() { return false; } + @Override + public boolean hasOperateNamespacePermission(String appId, String env, String clusterName, String namespaceName) { + return hasModifyNamespacePermission(appId, env, clusterName, namespaceName) + || hasReleaseNamespacePermission(appId, env, clusterName, namespaceName); + } + @Override public boolean shouldHideConfigToCurrentUser(String appId, String env, String clusterName, String namespaceName) { return false; }apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (1)
68-71: Unify permission check via hasPermissions
Replace the directconsumerHasPermissioncall inhasCreateApplicationPermission()with a single call tohasPermissions(...)to leverage the shared permission‐check path:@Override public boolean hasCreateApplicationPermission() { - long consumerId = consumerAuthUtil.retrieveConsumerIdFromCtx(); - return permissionService.consumerHasPermission(consumerId, PermissionType.CREATE_APPLICATION, SYSTEM_PERMISSION_TARGET_ID); + return hasPermissions(Collections.singletonList( + new Permission(PermissionType.CREATE_APPLICATION, SYSTEM_PERMISSION_TARGET_ID) + )); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/po/Permission.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/PermissionRepository.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java(3 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RolePermissionService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionServiceTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java(5 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RolePermissionServiceTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RolePermissionServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionServiceTest.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java
🧰 Additional context used
🧬 Code graph analysis (4)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidatorTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-124)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (1)
Component(34-87)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
Component(26-96)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java (1)
AbstractPermissionValidator(28-100)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (1)
Component(32-118)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
Component(26-96)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RoleUtils.java (1)
RoleUtils(25-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/po/Permission.java (1)
67-79: LGTM on value-based equalityequals/hashCode based on (permissionType, targetId) matches the documented uniqueness constraint and enables fast Set lookups.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (2)
24-35: LGTM: shift to AbstractPermissionValidator is the right unification hookImports and class extension align this validator with the new unified permission flow. No issues.
80-86: LGTM: centralized permission set checkNull/empty guard + context-driven consumerId + service-level check is sound and matches the unified design.
| public boolean checkUserHasPermission(String userId, List<Permission> permissions) { | ||
| List<Permission> userPermissions = permissionRepository.findUserPermissions(userId); | ||
|
|
||
| if (CollectionUtils.isEmpty(userPermissions)) { | ||
| return false; | ||
| } | ||
|
|
||
| Set<Permission> userPermissionSet = Sets.newHashSet(userPermissions); | ||
|
|
||
| return permissions.stream().anyMatch(userPermissionSet::contains); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add @OverRide and guard null/empty input
Prevent NPE when callers pass null/empty perms; add @OverRide for clarity.
- public boolean checkUserHasPermission(String userId, List<Permission> permissions) {
- List<Permission> userPermissions = permissionRepository.findUserPermissions(userId);
+ @Override
+ public boolean checkUserHasPermission(String userId, List<Permission> permissions) {
+ if (org.springframework.util.CollectionUtils.isEmpty(permissions)) {
+ return false;
+ }
+ List<Permission> userPermissions = permissionRepository.findUserPermissions(userId);
if (CollectionUtils.isEmpty(userPermissions)) {
return false;
}
Set<Permission> userPermissionSet = Sets.newHashSet(userPermissions);
return permissions.stream().anyMatch(userPermissionSet::contains);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public boolean checkUserHasPermission(String userId, List<Permission> permissions) { | |
| List<Permission> userPermissions = permissionRepository.findUserPermissions(userId); | |
| if (CollectionUtils.isEmpty(userPermissions)) { | |
| return false; | |
| } | |
| Set<Permission> userPermissionSet = Sets.newHashSet(userPermissions); | |
| return permissions.stream().anyMatch(userPermissionSet::contains); | |
| } | |
| @Override | |
| public boolean checkUserHasPermission(String userId, List<Permission> permissions) { | |
| if (org.springframework.util.CollectionUtils.isEmpty(permissions)) { | |
| return false; | |
| } | |
| List<Permission> userPermissions = permissionRepository.findUserPermissions(userId); | |
| if (CollectionUtils.isEmpty(userPermissions)) { | |
| return false; | |
| } | |
| Set<Permission> userPermissionSet = Sets.newHashSet(userPermissions); | |
| return permissions.stream().anyMatch(userPermissionSet::contains); | |
| } |
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java
around lines 379 to 389, add the @Override annotation to the
checkUserHasPermission method and defensively handle a null or empty permissions
argument by returning false early (i.e., if permissions is null or
permissions.isEmpty() return false) before querying user permissions; keep the
existing check for userPermissions and the final anyMatch logic unchanged.
spaceluke
left a comment
spaceluke
left a comment
There was a problem hiding this comment.
Still have some minor issues to be fix~
Besides, I think it is good to do some benchmark to prove that your optimization of permission verification is effective!
...ortal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java
Show resolved
Hide resolved
|
|
||
|
|
||
| @Query("SELECT rp FROM RolePermission rp WHERE rp.roleId IN :roleIds") | ||
| List<RolePermission> findByRoleIds(@Param("roleIds") List<Long> roleIds); |
There was a problem hiding this comment.
What is the difference between this method and findByRoleIdIn above?
| } | ||
|
|
||
|
|
||
| public boolean checkUserHasPermission(String userId, List<Permission> permissions) { |
There was a problem hiding this comment.
Suggest naming：hasAnyPermission
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java (2)
79-83: Prefer @activeprofiles("test") over setting system property in @PostConstruct.Avoid cross-test leakage and make profile explicit.
+import org.springframework.test.context.ActiveProfiles; @@ -@RunWith(SpringJUnit4ClassRunner.class) -@SpringBootTest(classes = {PortalApplication.class, - ControllableAuthorizationConfiguration.class}, webEnvironment = WebEnvironment.RANDOM_PORT) +@RunWith(SpringJUnit4ClassRunner.class) +@SpringBootTest(classes = {PortalApplication.class, + ControllableAuthorizationConfiguration.class}, webEnvironment = WebEnvironment.RANDOM_PORT) +@ActiveProfiles("test") public class ItemControllerAuthIntegrationTest { @@ - @PostConstruct - private void postConstruct() { - System.setProperty("spring.profiles.active", "test"); - restTemplate.setErrorHandler(new DefaultResponseErrorHandler()); - } + @PostConstruct + private void postConstruct() { + restTemplate.setErrorHandler(new DefaultResponseErrorHandler()); + }
119-119: Keep anyList() — method expects ListRolePermissionService.hasAnyPermission(String, List) (apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RolePermissionService.java:96) takes a List, so anyList() is correct; optionally add an ArgumentCaptor<List> to assert the actual permissions passed.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (1)
59-63: Route create-application check through hasPermissions for consistency/DRYReuse the unified path so identity retrieval and evaluation stay in one place.
+import java.util.Collections; @@ @Override public boolean hasCreateApplicationPermission() { - long consumerId = consumerAuthUtil.retrieveConsumerIdFromCtx(); - return permissionService.consumerHasPermission(consumerId, PermissionType.CREATE_APPLICATION, SYSTEM_PERMISSION_TARGET_ID); + return hasPermissions(Collections.singletonList( + new Permission(PermissionType.CREATE_APPLICATION, SYSTEM_PERMISSION_TARGET_ID) + )); }apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (2)
56-61: Defensive null check for appNamespaceIf a caller passes null,
appNamespace.isPublic()will NPE. Cheap guard keeps behavior predictable.@Override public boolean hasCreateAppNamespacePermission(String appId, AppNamespace appNamespace) { + if (appNamespace == null) { + return false; + } boolean isPublicAppNamespace = appNamespace.isPublic();
74-89: Skip DB read when caller is privileged (micro‑opt)You can avoid
appNamespaceService.findByAppIdAndNamefor app admins/operators by short‑circuiting earlier.public boolean shouldHideConfigToCurrentUser(String appId, String env, String clusterName, String namespaceName) { // 1. check whether the current environment enables member only function if (!portalConfig.isConfigViewMemberOnly(env)) { return false; } + // 1.5. privileged users should always see configs + if (isAppAdmin(appId) || hasOperateNamespacePermission(appId, env, clusterName, namespaceName)) { + return false; + } + // 2. public namespace is open to every one AppNamespace appNamespace = appNamespaceService.findByAppIdAndName(appId, namespaceName); if (appNamespace != null && appNamespace.isPublic()) { return false; } // 3. check app admin and operate permissions - return !isAppAdmin(appId) && !hasOperateNamespacePermission(appId, env, clusterName, namespaceName); + return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
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/service/ConsumerRolePermissionService.java(5 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RolePermissionRepository.java(2 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RolePermissionService.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java(1 hunks)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/repository/RolePermissionRepository.java
🚧 Files skipped from review as they are similar to previous changes (5)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/RolePermissionService.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/defaultimpl/DefaultRolePermissionService.java
- apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/AbstractPermissionValidator.java (1)
AbstractPermissionValidator(28-100)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (1)
Component(32-118)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
Component(26-96)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (1)
Component(30-78)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UnifiedPermissionValidator.java (1)
Component(26-96)
🔇 Additional comments (6)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/controller/ItemControllerAuthIntegrationTest.java (2)
33-33: Confirm correct RolePermissionService type (portal vs openapi).Ensure this import matches the service used by the controller under test; there may also be an OpenAPI variant with the same name.
76-78: Use @MockBean for RolePermissionService, ItemService, and UserInfoHolder (avoid NotAMockException).
when(rolePermissionService...),verify(itemService), andwhen(userInfoHolder.getUser())require Mockito mocks, not real beans.Apply:
@@ -import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.mock.mockito.MockBean; @@ - @Autowired - private UserInfoHolder userInfoHolder; - @Autowired - private ItemService itemService; + @MockBean + private UserInfoHolder userInfoHolder; + @MockBean + private ItemService itemService; @@ - @Autowired - private RolePermissionService rolePermissionService; + @MockBean + private RolePermissionService rolePermissionService;Also applies to: 71-75, 130-134
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java (3)
23-31: Good move: inherit common checks via AbstractPermissionValidatorExtending the shared base centralizes permission construction and reduces duplication. Imports look correct.
42-45: Don’t throw in permission methods; return safe defaults to avoid 500s via unified delegationThrowing UnsupportedOperationException here will bubble up as 5xx when UnifiedPermissionValidator routes to the consumer path. Keep base semantics (false) or drop the overrides to inherit the parent’s false.
Apply one of the following:
@@ @Override public boolean hasCreateAppNamespacePermission(String appId, AppNamespace appNamespace) { - throw new UnsupportedOperationException("Not supported operation"); + return false; } @@ @Override public boolean shouldHideConfigToCurrentUser(String appId, String env, String clusterName, String namespaceName) { - throw new UnsupportedOperationException("Not supported operation"); + return false; } @@ @Override public boolean hasManageAppMasterPermission(String appId) { - throw new UnsupportedOperationException("Not supported operation"); + return false; }Alternatively, delete these overrides entirely to inherit the base-class false.
Also applies to: 53-57, 65-68
70-77: Optional hardening — confirm consumer-context invariant or handle missing consumer idConsumerPermissionValidator calls consumerAuthUtil.retrieveConsumerIdFromCtx (apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidator.java ~lines 61–77). ConsumerAuthUtil implements retrieveConsumerIdFromCtx (apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java) and tests (apollo-portal/src/test/java/com/ctrip/framework/apollo/openapi/auth/ConsumerPermissionValidatorTest.java, TC007) mock it to throw RuntimeException("Consumer ID not found") and expect propagation.
Either (A) keep propagating the exception and document the invariant that consumer context must be present, or (B) if you harden this, catch the concrete exception type(s) ConsumerAuthUtil actually throws (or explicitly check context before calling) and update tests — do not just catch IllegalStateException if the code/tests use RuntimeException.
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java (1)
109-117: Harden hasPermissions: null-safety + super-admin bypass
- requiredPerms can be null from upstream; current
requiredPerms.isEmpty()risks NPE.- Super admins should pass regardless of specific grants; add a short‑circuit.
- Guard against missing user in context to avoid NPEs.
Apply:
@Override protected boolean hasPermissions(List<Permission> requiredPerms) { - String userId = userInfoHolder.getUser().getUserId(); - if (requiredPerms.isEmpty()) { + if (requiredPerms == null || requiredPerms.isEmpty()) { return false; } - - return rolePermissionService.hasAnyPermission(userId, requiredPerms); + // Ensure user context exists + if (userInfoHolder.getUser() == null) { + return false; + } + final String userId = userInfoHolder.getUser().getUserId(); + // Super admin has all permissions + if (rolePermissionService.isSuperAdmin(userId)) { + return true; + } + return rolePermissionService.hasAnyPermission(userId, requiredPerms); }Consider adding unit tests for: requiredPerms == null, empty list, missing user context, and super‑admin bypass.
...ortal/src/main/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidator.java
Outdated
Show resolved
Hide resolved
|
@nobodyiam PTAL Meanwhile, @wjwang00 is performing performance testing to verify the effectiveness of his optimization on auth checking. |
Performance Validation
We conducted a lightweight benchmark on the “update config item” endpoint: @PreAuthorize(value = "@consumerPermissionValidator.hasModifyNamespacePermission(#appId, #env, #clusterName, #namespaceName)")
@PutMapping(value = "/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/items/{key:.+}")
public void updateItem(@PathVariable String appId,
@PathVariable String env,
@PathVariable String clusterName,
@PathVariable String namespaceName,
@PathVariable String key,
@RequestBody OpenItemDTO item,
@RequestParam(defaultValue = "false") boolean createIfNotExists)
ResultsAverage latency comparison 
P95 & P99 latency show similar improvements. 
|
nobodyiam
left a comment
nobodyiam
left a comment
There was a problem hiding this comment.
The refactoring and optimization look good, but there are several code style issues. I’ve highlighted some, but please review all changes and address them.
.../src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/ctrip/framework/apollo/openapi/service/ConsumerRolePermissionService.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/openapi/v1/controller/NamespaceBranchController.java
Outdated
Show resolved
Hide resolved
...o-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java
Show resolved
Hide resolved
...-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java
Outdated
Show resolved
Hide resolved
...-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/PermissionController.java
Outdated
Show resolved
Hide resolved
...tal/src/main/java/com/ctrip/framework/apollo/portal/repository/RolePermissionRepository.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java (1)
71-80: Add javadoc to document the dual-behavior design.The method correctly implements null-safe logic by falling back to
RequestContextHolderwhen the inputrequestis null. However, this dual behavior (explicit request parameter vs. context retrieval) should be documented with javadoc to help future maintainers understand when to pass null vs. an explicit request.Consider adding documentation like:
+ /** + * Checks if a consumer ID exists in the request attributes. + * + * @param request the HttpServletRequest to check; if null, retrieves the current request from RequestContextHolder + * @return true if CONSUMER_ID attribute is present, false otherwise (including when no request context is available) + */ public boolean checkConsumerIdExist(HttpServletRequest request) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java(1 hunks)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java
🧰 Additional context used
🧬 Code graph analysis (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (1)
UserTypeResolverFilter(32-65)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthConfiguration.java (4)
Configuration(81-469)Configuration(91-161)Configuration(191-250)Configuration(428-455)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java (2)
22-22: LGTM: Import added for new filter.The import is necessary for the new
authTypeResolverFilterbean and is correctly placed.
45-52: Filter ordering is correct; verify that setName() was truly requested in a prior review.The filter chain ordering is properly configured:
ConsumerAuthenticationFilterat order-99executes beforeUserTypeResolverFilterat order-98. However, the review comment claims a "previous review explicitly requested" addingsetName(), which cannot be verified in the codebase. No existingFilterRegistrationBeanconfigurations includesetName()calls, and no tests assert on filter names. Confirm whether this requirement exists before proceeding.
...c/main/java/com/ctrip/framework/apollo/portal/spi/configuration/AuthFilterConfiguration.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java (2)
200-209: Previous feedback appears addressed.The test setup is now consistent with its name. The test correctly verifies that a normal user with
ASSIGN_ROLEpermission but withoutManageAppMasterpermission cannot manage app masters.
153-158: Misleading test name remains unfixed.As discussed in previous reviews, this test name suggests it verifies app admin status, but it actually tests the case when the namespace is not found (
appNamespaceService.findByAppIdAndNamereturns null). The agreed-upon rename toshouldHideConfigToCurrentUser_namespaceNotFoundhas not been applied.Apply this diff to rename the test:
- void shouldHideConfigToCurrentUser_userIsNotAppAdmin() { + void shouldHideConfigToCurrentUser_namespaceNotFound() {
🧹 Nitpick comments (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java (1)
212-226: Consider using explicit permission objects for clarity.The tests use
new Permission()without setting any fields. While this works for verifying delegation torolePermissionService, using explicit permission types would make the tests more readable and maintainable.For example:
@Test void hasPermissions_match() { - List<Permission> requiredPerms = Lists.newArrayList(new Permission(), new Permission()); + List<Permission> requiredPerms = Lists.newArrayList( + new Permission(PermissionType.MODIFY_NAMESPACE, APP_ID), + new Permission(PermissionType.RELEASE_NAMESPACE, APP_ID)); when(rolePermissionService.hasAnyPermission(USER_ID, requiredPerms)).thenReturn(true); assertThat(validator.hasPermissions(requiredPerms)).isTrue(); }
📜 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/component/UserPermissionValidatorTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
...l/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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/component/UserPermissionValidatorTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (6)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java (6)
1-70: LGTM! Well-structured test setup.The test class setup is clean and follows standard Mockito patterns. The use of
lenient()in thesetUpmethod is appropriate for shared stub configuration across tests.
72-117: LGTM! Comprehensive coverage of namespace creation permission logic.The tests cover all branches: public namespaces, private namespaces with different portal config settings, and the super admin override. The test names clearly indicate the scenario being tested.
119-131: LGTM! Clean delegation tests.The super admin tests correctly verify the delegation to
rolePermissionService.isSuperAdmin.
166-178: LGTM! Clean delegation tests.The create application permission tests correctly verify the delegation to
systemRoleManagerService.hasCreateApplicationPermission.
180-209: LGTM! Past review feedback addressed.The
hasManageAppMasterPermissiontests now have accurate naming and complete mock setup. The test at line 201 correctly verifies the case where a user hasASSIGN_ROLEpermission but lacksManageAppMasterpermission, and appropriately expectsfalse.
211-231: LGTM! Good coverage of the generic permission check.The tests cover the match, no-match, and empty-list cases for the generic
hasPermissionsmethod. Using emptyPermissionobjects is acceptable here since these tests focus on delegation logic rather than permission content.
...l/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java (1)
170-199: Consider adding test for user without ASSIGN_ROLE permission.Current tests cover super admin and normal users with ASSIGN_ROLE. Consider adding a test for the case where a normal user lacks ASSIGN_ROLE permission entirely to ensure the method correctly returns
false.Add this test:
@Test void hasManageAppMasterPermission_normalUser_withoutAssignRole() { when(rolePermissionService.isSuperAdmin(USER_ID)).thenReturn(false); List<Permission> requiredPermissions = Collections.singletonList( new Permission(PermissionType.ASSIGN_ROLE, APP_ID)); when(rolePermissionService.hasAnyPermission(USER_ID, requiredPermissions)).thenReturn(false); assertThat(validator.hasManageAppMasterPermission(APP_ID)).isFalse(); }
📜 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/component/UserPermissionValidatorTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/entity/bo/UserInfo.java (1)
UserInfo(19-79)
🔇 Additional comments (2)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java (2)
1-70: LGTM! Well-structured test setup.The test class follows standard Mockito conventions with proper mock injection and a comprehensive setUp method. The use of
lenient()at line 69 is appropriate since not all test methods will invokeuserInfoHolder.getUser().
201-221: Verify if empty Permission objects are intentional.Lines 204 and 212 create
Permissionobjects without setting any properties (e.g.,PermissionType,targetId). In production, permissions typically have these fields populated. Using empty objects might not accurately test the permission-matching logic if it relies on these properties.If these tests are meant to verify the validator's delegation to
rolePermissionService.hasAnyPermission, consider explicitly documenting this intent. Otherwise, populate the Permission objects with realistic data:-List<Permission> requiredPerms = Lists.newArrayList(new Permission(), new Permission()); +List<Permission> requiredPerms = Lists.newArrayList( + new Permission(PermissionType.READ_NAMESPACE, APP_ID + "+" + NAMESPACE), + new Permission(PermissionType.MODIFY_NAMESPACE, APP_ID + "+" + NAMESPACE) +);
...l/src/test/java/com/ctrip/framework/apollo/portal/component/UserPermissionValidatorTest.java
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java
Outdated
Show resolved
Hide resolved
|
@wjwang00 please fix the unit test errors. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java (1)
71-73: Consider a more idiomatic method name.The method name
checkConsumerIdExistis grammatically awkward. More idiomatic alternatives:
hasConsumerId(HttpServletRequest request)– concise, follows the "has" boolean patternisConsumerIdPresent(HttpServletRequest request)– follows the "is" boolean patternAdditionally, consider adding JavaDoc to describe the method's purpose and return value for consistency with the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java(1 hunks)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (1)
39-39: Consider removing the excess blank line.The blank line after the method signature is unnecessary.
Apply this diff:
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { - String authType = resolve(request);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (3)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/component/UserIdentityContextHolder.java (1)
UserIdentityContextHolder(19-47)apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/constant/UserIdentityConstants.java (1)
UserIdentityConstants(19-27)apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (1)
UserTypeResolverFilter(29-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (8)
🔇 Additional comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java (2)
42-46: LGTM: ThreadLocal cleanup pattern is correct.The try-finally block ensures
UserIdentityContextHolder.clear()is always called, preventing memory leaks.
54-57: LGTM: USER authentication check correctly excludes anonymous tokens.The logic properly distinguishes authenticated users from anonymous authentication by checking
!(auth instanceof AnonymousAuthenticationToken), addressing the issue from previous reviews.
| import org.springframework.security.core.context.SecurityContextHolder; | ||
| import org.springframework.web.filter.OncePerRequestFilter; | ||
|
|
||
| import static com.ctrip.framework.apollo.openapi.util.ConsumerAuthUtil.CONSUMER_ID; |
There was a problem hiding this comment.
Critical: Use ConsumerAuthUtil.checkConsumerIdExist instead of getAttribute.
Per multiple past reviews and team agreement, consumer authentication should be detected via a utility method in ConsumerAuthUtil rather than directly checking req.getAttribute(CONSUMER_ID). The current implementation will misclassify consumer requests as ANONYMOUS because:
- The OpenAPI documentation states that the consumer ID is passed via HTTP header, not request attribute
- Multiple reviewers (nobodyiam, spaceluke) flagged this as a critical bug
- You and spaceluke agreed to centralize consumer detection in
ConsumerAuthUtil.checkConsumerIdExistto avoid duplicating theCONSUMER_IDconstant
Apply this diff:
-import static com.ctrip.framework.apollo.openapi.util.ConsumerAuthUtil.CONSUMER_ID;
+import com.ctrip.framework.apollo.openapi.util.ConsumerAuthUtil; private String resolve(HttpServletRequest req) {
- if (req.getAttribute(CONSUMER_ID) != null) {
+ if (ConsumerAuthUtil.checkConsumerIdExist(req)) {
return UserIdentityConstants.CONSUMER;
}Also applies to: 50-51
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/filter/UserTypeResolverFilter.java
around line 33 (and also apply the same change at lines 50-51), replace the
direct request attribute check using req.getAttribute(CONSUMER_ID) with the
centralized utility ConsumerAuthUtil.checkConsumerIdExist(req) so consumer
detection uses the agreed helper; update imports to remove the static
CONSUMER_ID usage if no longer needed and call
ConsumerAuthUtil.checkConsumerIdExist(req) wherever the code currently inspects
req.getAttribute(CONSUMER_ID) to correctly classify consumer requests.
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/util/ConsumerAuthUtil.java
Show resolved
Hide resolved
|
@spaceluke This PR looks good now. Please help to do a final review. |
Thank you for your time and effort! |
|
LGTM |
3 participants
What's the purpose of this PR
2025 OSPP Project 1: Main Code for Apollo Permission System Architecture Iterative Upgrade
Which issue(s) this PR fixes:
Implement the unified architecture upgrade of the permission system for both OpenApi and Portal ends, and write corresponding test code to verify.
Optimize the implementation of permission verification, and write corresponding test code to verify.
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
New Features
Tests
Documentation