From 583dfe369757c30a13a9b144519a5bee54c816e9 Mon Sep 17 00:00:00 2001
From: "tom.fitzpatrick" <tom.fitzpatrick@r3.com>
Date: Thu, 25 Jul 2024 09:10:23 +0100
Subject: [PATCH] CORE-20803: Add validation to prevent changeParentGroup
 creating circular group hierarchies (#6304)

---
 .../impl/PermissionGroupManagerImpl.kt        | 20 ++++++++++++++
 .../impl/PermissionGroupManagerImplTest.kt    | 27 +++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/libs/permissions/permission-manager-impl/src/main/kotlin/net/corda/libs/permissions/manager/impl/PermissionGroupManagerImpl.kt b/libs/permissions/permission-manager-impl/src/main/kotlin/net/corda/libs/permissions/manager/impl/PermissionGroupManagerImpl.kt
index 632079406d3..db70719f0e5 100644
--- a/libs/permissions/permission-manager-impl/src/main/kotlin/net/corda/libs/permissions/manager/impl/PermissionGroupManagerImpl.kt
+++ b/libs/permissions/permission-manager-impl/src/main/kotlin/net/corda/libs/permissions/manager/impl/PermissionGroupManagerImpl.kt
@@ -55,6 +55,7 @@ class PermissionGroupManagerImpl(
         }
         val cachedGroup = permissionManagementCache.getGroup(changeGroupParentIdDto.groupId)!!
         val groupEntityVersion = cachedGroup.version
+        validateNewGroupIsNotChildOfGroup(changeGroupParentIdDto.groupId, changeGroupParentIdDto.newParentGroupId)
 
         val result = sendPermissionWriteRequest<Group>(
             rpcSender,
@@ -140,4 +141,23 @@ class PermissionGroupManagerImpl(
         )
         return result.convertToResponseDto()
     }
+
+    private fun validateNewGroupIsNotChildOfGroup(groupToModifyId: String, newParentGroupId: String?) {
+        if (groupToModifyId == newParentGroupId) {
+            throw IllegalArgumentException("Group '$groupToModifyId' cannot be its own parent.")
+        }
+        val permissionManagementCache = checkNotNull(permissionManagementCacheRef.get()) {
+            "Permission management cache is null."
+        }
+        var currentGroup: String? = newParentGroupId
+        while (currentGroup != null) {
+            if (currentGroup == groupToModifyId) {
+                throw IllegalArgumentException(
+                    "Cannot set Group '$newParentGroupId' as the parent of " +
+                        "Group '$groupToModifyId' because it would create a cycle in the group hierarchy."
+                )
+            }
+            currentGroup = permissionManagementCache.getGroup(currentGroup)?.parentGroupId
+        }
+    }
 }
diff --git a/libs/permissions/permission-manager-impl/src/test/kotlin/net/corda/libs/permissions/manager/impl/PermissionGroupManagerImplTest.kt b/libs/permissions/permission-manager-impl/src/test/kotlin/net/corda/libs/permissions/manager/impl/PermissionGroupManagerImplTest.kt
index 4a9139a0414..2f49a6cf706 100644
--- a/libs/permissions/permission-manager-impl/src/test/kotlin/net/corda/libs/permissions/manager/impl/PermissionGroupManagerImplTest.kt
+++ b/libs/permissions/permission-manager-impl/src/test/kotlin/net/corda/libs/permissions/manager/impl/PermissionGroupManagerImplTest.kt
@@ -130,6 +130,33 @@ class PermissionGroupManagerImplTest {
         assertTrue(result.roleAssociations.isEmpty())
     }
 
+    @Test
+    fun `change parent group throws exception if new parent group is a child of the group`() {
+        val groupId = UUID.randomUUID().toString()
+        val childGroupId = UUID.randomUUID().toString()
+        val avroGroup = Group(groupId, 0, ChangeDetails(Instant.now()), "groupName", null, emptyList(), emptyList())
+        val childGroup = Group(childGroupId, 0, ChangeDetails(Instant.now()), "childGroupName", groupId, emptyList(), emptyList())
+
+        val future = mock<CompletableFuture<PermissionManagementResponse>>()
+        whenever(future.getOrThrow(defaultTimeout)).thenReturn(PermissionManagementResponse(avroGroup))
+        whenever(permissionManagementCache.getGroup(groupId)).thenReturn(avroGroup)
+        whenever(permissionManagementCache.getGroup(childGroupId)).thenReturn(childGroup)
+
+        val requestCaptor = argumentCaptor<PermissionManagementRequest>()
+        whenever(rpcSender.sendRequest(requestCaptor.capture())).thenReturn(future)
+
+        val changeGroupParentIdDto = ChangeGroupParentIdDto("requestedBy", groupId, childGroupId)
+
+        val exception = assertThrows<IllegalArgumentException> {
+            manager.changeParentGroup(changeGroupParentIdDto)
+        }
+
+        assertEquals(
+            "Cannot set Group '$childGroupId' as the parent of Group '$groupId' because it would create a cycle in the group hierarchy.",
+            exception.message
+        )
+    }
+
     @Test
     fun `add role to group sends rpc request and converts result to response dto`() {
         val groupId = UUID.randomUUID().toString()