Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Apollo 2.5.0
* [Refactor: Exception handler adds root cause information](https://github.com/apolloconfig/apollo/pull/5367)
* [Feature: Enhanced parameter verification for edit item](https://github.com/apolloconfig/apollo/pull/5376)
* [Feature: Added a new feature to get instance count by namespace.](https://github.com/apolloconfig/apollo/pull/5381)
* [Bugfix: Remove cluster-related roles and permissions upon deletion](https://github.com/apolloconfig/apollo/pull/5395)
* [Security: Prevent unauthorized access to other users' apps in /apps/by-owner endpoint](https://github.com/apolloconfig/apollo/pull/5396)

------------------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,19 @@ List<Permission> findByPermissionTypeInAndTargetId(Collection<String> permission
@Query("SELECT p.id from Permission p where p.targetId like ?1 or p.targetId like CONCAT(?1, '+%')")
List<Long> findPermissionIdsByAppId(String appId);

@Query("SELECT p.id from Permission p where p.targetId like CONCAT(?1, '+', ?2) "
+ "OR p.targetId like CONCAT(?1, '+', ?2, '+%')")
@Query("SELECT p.id from Permission p "
+ "where ("
+ "p.targetId like CONCAT(?1, '+', ?2) OR p.targetId like CONCAT(?1, '+', ?2, '+%')"
+ ") AND ( "
+ "p.permissionType = 'ModifyNamespace' OR p.permissionType = 'ReleaseNamespace'"
+ ")")
List<Long> findPermissionIdsByAppIdAndNamespace(String appId, String namespaceName);

@Modifying
@Query("UPDATE Permission SET IsDeleted = true, DeletedAt = ROUND(UNIX_TIMESTAMP(NOW(4))*1000), DataChange_LastModifiedBy = ?2 WHERE Id in ?1 and IsDeleted = false")
Integer batchDelete(List<Long> permissionIds, String operator);

@Query("SELECT p.id from Permission p where p.targetId = CONCAT(?1, '+', ?2, '+', ?3)"
+ " AND ( p.permissionType = 'ModifyNamespacesInCluster' OR p.permissionType = 'ReleaseNamespacesInCluster')")
List<Long> findPermissionIdsByAppIdAndEnvAndCluster(String appId, String env, String clusterName);
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ public interface RoleRepository extends PagingAndSortingRepository<Role, Long> {
@Query("SELECT r.id from Role r where r.roleName like CONCAT('Master+', ?1) "
+ "OR r.roleName like CONCAT('ModifyNamespace+', ?1, '+%') "
+ "OR r.roleName like CONCAT('ReleaseNamespace+', ?1, '+%') "
+ "OR r.roleName like CONCAT('ManageAppMaster+', ?1)")
+ "OR r.roleName like CONCAT('ManageAppMaster+', ?1) "
+ "OR r.roleName like CONCAT('ModifyNamespacesInCluster+', ?1, '+%')"
+ "OR r.roleName like CONCAT('ReleaseNamespacesInCluster+', ?1, '+%')")
List<Long> findRoleIdsByAppId(String appId);

@Query("SELECT r.id from Role r where r.roleName like CONCAT('ModifyNamespace+', ?1, '+', ?2) "
Expand All @@ -47,4 +49,8 @@ public interface RoleRepository extends PagingAndSortingRepository<Role, Long> {
@Modifying
@Query("UPDATE Role SET IsDeleted = true, DeletedAt = ROUND(UNIX_TIMESTAMP(NOW(4))*1000), DataChange_LastModifiedBy = ?2 WHERE Id in ?1 and IsDeleted = false")
Integer batchDelete(List<Long> roleIds, String operator);

@Query("SELECT r.id from Role r where r.roleName = CONCAT('ModifyNamespacesInCluster+', ?1, '+', ?2, '+', ?3) "
+ "OR r.roleName = CONCAT('ReleaseNamespacesInCluster+', ?1, '+', ?2, '+', ?3)")
List<Long> findRoleIdsByCluster(String appId, String env, String clusterName);
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.ctrip.framework.apollo.portal.constant.TracerEventType;
import com.ctrip.framework.apollo.portal.spi.UserInfoHolder;
import com.ctrip.framework.apollo.tracer.Tracer;
import javax.transaction.Transactional;
import org.springframework.stereotype.Service;

import java.util.List;
Expand All @@ -33,12 +34,15 @@ public class ClusterService {
private final UserInfoHolder userInfoHolder;
private final AdminServiceAPI.ClusterAPI clusterAPI;
private final RoleInitializationService roleInitializationService;
private final RolePermissionService rolePermissionService;

public ClusterService(final UserInfoHolder userInfoHolder, final AdminServiceAPI.ClusterAPI clusterAPI,
RoleInitializationService roleInitializationService) {
RoleInitializationService roleInitializationService,
RolePermissionService rolePermissionService) {
this.userInfoHolder = userInfoHolder;
this.clusterAPI = clusterAPI;
this.roleInitializationService = roleInitializationService;
this.rolePermissionService = rolePermissionService;
}

public List<ClusterDTO> findClusters(Env env, String appId) {
Expand All @@ -61,6 +65,9 @@ public ClusterDTO createCluster(Env env, ClusterDTO cluster) {

public void deleteCluster(Env env, String appId, String clusterName){
clusterAPI.delete(env, appId, clusterName, userInfoHolder.getUser().getUserId());

rolePermissionService.deleteRolePermissionsByCluster(appId, env.getName(), clusterName,
userInfoHolder.getUser().getUserId());
}

public ClusterDTO loadCluster(String appId, Env env, String clusterName){
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,9 @@ Set<String> assignRoleToUsers(String roleName, Set<String> userIds,
* delete permissions when delete app namespace.
*/
void deleteRolePermissionsByAppIdAndNamespace(String appId, String namespaceName, String operator);

/**
* delete permissions when delete cluster.
*/
void deleteRolePermissionsByCluster(String appId, String env, String clusterName, String operator);
}
Original file line number Diff line number Diff line change
Expand Up @@ -348,4 +348,32 @@ public void deleteRolePermissionsByAppIdAndNamespace(String appId, String namesp
consumerRoleRepository.batchDeleteByRoleIds(roleIds, operator);
}
}

@Transactional
@Override
public void deleteRolePermissionsByCluster(String appId, String env, String clusterName, String operator) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could move the Codex-generated test to this PR.

appId = EscapeCharacter.DEFAULT.escape(appId);
List<Long> permissionIds = permissionRepository.findPermissionIdsByAppIdAndEnvAndCluster(appId, env, clusterName);
if (!permissionIds.isEmpty()) {
// 1. delete Permission
permissionRepository.batchDelete(permissionIds, operator);

// 2. delete Role Permission
rolePermissionRepository.batchDeleteByPermissionIds(permissionIds, operator);
}

List<Long> roleIds = roleRepository.findRoleIdsByCluster(appId, env, clusterName);
if (!roleIds.isEmpty()) {
// 3. delete Role
roleRepository.batchDelete(roleIds, operator);

// 4. delete User Role
userRoleRepository.batchDeleteByRoleIds(roleIds, operator);

// 5. delete Consumer Role
consumerRoleRepository.batchDeleteByRoleIds(roleIds, operator);
}
}


}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNull;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix inconsistent testing framework imports.

The test class uses JUnit 4 throughout (imports on lines 19-21, @Test annotations), but line 22 imports assertNull from JUnit 5. This creates inconsistency and potential conflicts.

Use the JUnit 4 equivalent instead:

-import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.Assert.assertNull;
🤖 Prompt for AI Agents
In
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/spi/defaultImpl/RolePermissionServiceTest.java
at line 22, replace the import of assertNull from JUnit 5 with the JUnit 4
equivalent to maintain consistency with the rest of the test class that uses
JUnit 4. Change the import statement to use org.junit.Assert.assertNull instead
of org.junit.jupiter.api.Assertions.assertNull.


import com.ctrip.framework.apollo.common.entity.BaseEntity;
import com.ctrip.framework.apollo.portal.AbstractIntegrationTest;
Expand All @@ -32,6 +33,7 @@
import com.ctrip.framework.apollo.portal.repository.RoleRepository;
import com.ctrip.framework.apollo.portal.repository.UserRoleRepository;
import com.ctrip.framework.apollo.portal.service.RolePermissionService;
import com.ctrip.framework.apollo.portal.util.RoleUtils;
import com.google.common.collect.Sets;
import java.util.List;
import java.util.Set;
Expand Down Expand Up @@ -320,6 +322,23 @@ public void testUserHasPermission() throws Exception {

}

@Test
@Sql(scripts = "/sql/permission/RolePermissionServiceTest.deleteRolePermissionsByAppIdWithClusterRoles.sql",
executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD)
@Sql(scripts = "/sql/cleanup.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD)
public void testDeleteRolePermissionsByAppIdWithClusterRoles() {
String appId = "clusterApp";
String operator = "test";

rolePermissionService.deleteRolePermissionsByAppId(appId, operator);

String modifyRoleName = RoleUtils.buildModifyNamespacesInClusterRoleName(appId, "DEV", "default");
String releaseRoleName = RoleUtils.buildReleaseNamespacesInClusterRoleName(appId, "DEV", "default");

assertNull(roleRepository.findTopByRoleName(modifyRoleName));
assertNull(roleRepository.findTopByRoleName(releaseRoleName));
}

private Role assembleRole(String roleName) {
Role role = new Role();
role.setRoleName(roleName);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
-- Copyright 2025 Apollo Authors
--
-- Licensed under the Apache License, Version 2.0 (the "License");
-- you may not use this file except in compliance with the License.
-- You may obtain a copy of the License at
--
-- http://www.apache.org/licenses/LICENSE-2.0
--
-- Unless required by applicable law or agreed to in writing, software
-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
INSERT INTO "Permission" (`Id`, `PermissionType`, `TargetId`, `DataChange_CreatedBy`,
`DataChange_LastModifiedBy`)
VALUES (1500, 'ModifyNamespacesInCluster', 'clusterApp+DEV+default', 'someOperator',
'someOperator'),
(1501, 'ReleaseNamespacesInCluster', 'clusterApp+DEV+default', 'someOperator',
'someOperator');

INSERT INTO "Role" (`Id`, `RoleName`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`)
VALUES (1500, 'ModifyNamespacesInCluster+clusterApp+DEV+default', 'someOperator', 'someOperator'),
(1501, 'ReleaseNamespacesInCluster+clusterApp+DEV+default', 'someOperator', 'someOperator');

INSERT INTO "RolePermission" (`Id`, `RoleId`, `PermissionId`)
VALUES (1500, 1500, 1500),
(1501, 1501, 1501);