Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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)

------------------
All issues and pull requests are [here](https://github.com/apolloconfig/apollo/milestone/16?closed=1)
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);