Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORE-20803: Add validation to prevent changeParentGroup creating circular group hierarchies #6304

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
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down