PI-2562 Check if role already exists before attempting to add/remove (#…

…4275)
ministryofjustice · Oct 3, 2024 · e623152 · e623152
1 parent 678f3a5
commit e623152
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 24 deletions.
diff --git a/libs/commons/src/main/kotlin/uk/gov/justice/digital/hmpps/ldap/LdapTemplateExtensions.kt b/libs/commons/src/main/kotlin/uk/gov/justice/digital/hmpps/ldap/LdapTemplateExtensions.kt
@@ -1,7 +1,9 @@
 package uk.gov.justice.digital.hmpps.ldap
 
+import io.opentelemetry.api.trace.Span
 import io.opentelemetry.instrumentation.annotations.SpanAttribute
 import io.opentelemetry.instrumentation.annotations.WithSpan
+import org.springframework.ldap.NameAlreadyBoundException
 import org.springframework.ldap.NameNotFoundException
 import org.springframework.ldap.core.AttributesMapper
 import org.springframework.ldap.core.LdapTemplate
@@ -11,6 +13,7 @@ import org.springframework.ldap.query.LdapQueryBuilder.query
 import org.springframework.ldap.query.SearchScope
 import org.springframework.ldap.support.LdapNameBuilder
 import uk.gov.justice.digital.hmpps.exception.NotFoundException
+import javax.naming.Name
 import javax.naming.directory.Attributes
 import javax.naming.directory.BasicAttribute
 import javax.naming.directory.BasicAttributes
@@ -36,7 +39,7 @@ fun LdapTemplate.findAttributeByUsername(@SpanAttribute username: String, @SpanA
         AttributesMapper { it[attribute]?.get()?.toString() }
     ).singleOrNull()
 } catch (_: NameNotFoundException) {
-    throw NotFoundException("NDeliusUser of $username not found")
+    throw NotFoundException("User", "username", username)
 }
 
 @WithSpan
@@ -50,33 +53,50 @@ fun LdapTemplate.getRoles(@SpanAttribute username: String) = try {
         AttributesMapper { it["cn"]?.get()?.toString() }
     ).filterNotNull()
 } catch (_: NameNotFoundException) {
-    throw NotFoundException("NDeliusUser of $username not found")
+    throw NotFoundException("User", "username", username)
 }
 
 @WithSpan
 fun LdapTemplate.addRole(@SpanAttribute username: String, @SpanAttribute role: DeliusRole) {
-    val roleContext = lookupContext(role.context())
-        ?: throw NotFoundException("NDeliusRole of ${role.name} not found")
+    val roleContext = lookupContext(role.context()) ?: throw NotFoundException("Role", "name", role.name)
     val attributes: Attributes = BasicAttributes(true).apply {
         put(roleContext.nameInNamespace.asAttribute("aliasedObjectName"))
         put(role.name.asAttribute("cn"))
         put(listOf("NDRoleAssociation", "alias", "top").asAttribute("objectclass"))
     }
     val userRole = role.context(username)
-    try {
-        rebind(userRole, null, attributes)
-    } catch (_: NameNotFoundException) {
-        throw NotFoundException("NDeliusUser of $username not found")
+    if (!exists(userRole)) {
+        try {
+            rebind(userRole, null, attributes)
+        } catch (_: NameNotFoundException) {
+            throw NotFoundException("User", "username", username)
+        } catch (_: NameAlreadyBoundException) {
+            // role already assigned to user
+        }
     }
 }
 
 @WithSpan
-fun LdapTemplate.removeRole(@SpanAttribute username: String, @SpanAttribute role: DeliusRole) =
-    try {
-        unbind(role.context(username))
-    } catch (_: NameNotFoundException) {
-        throw NotFoundException("NDeliusUser of $username not found")
+fun LdapTemplate.removeRole(@SpanAttribute username: String, @SpanAttribute role: DeliusRole) {
+    val userRole = role.context(username)
+    if (exists(userRole)) {
+        try {
+            unbind(role.context(username))
+        } catch (_: NameNotFoundException) {
+            throw NotFoundException("User", "username", username)
+        }
     }
+}
+
+@WithSpan
+fun LdapTemplate.exists(name: Name) = try {
+    lookup(name)
+    true
+} catch (_: NameNotFoundException) {
+    false
+}.also {
+    Span.current().setAttribute("exists", it)
+}
 
 private fun DeliusRole.context(username: String? = null) =
     LdapNameBuilder.newInstance()

diff --git a/libs/commons/src/test/kotlin/uk/gov/justice/digital/hmpps/ldap/LdapTemplateExtensionsTest.kt b/libs/commons/src/test/kotlin/uk/gov/justice/digital/hmpps/ldap/LdapTemplateExtensionsTest.kt
@@ -5,9 +5,14 @@ import org.hamcrest.Matchers.equalTo
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.api.extension.ExtendWith
+import org.mockito.ArgumentMatchers.any
+import org.mockito.ArgumentMatchers.eq
 import org.mockito.Mock
 import org.mockito.junit.jupiter.MockitoExtension
-import org.mockito.kotlin.*
+import org.mockito.kotlin.anyOrNull
+import org.mockito.kotlin.check
+import org.mockito.kotlin.verify
+import org.mockito.kotlin.whenever
 import org.springframework.ldap.NameNotFoundException
 import org.springframework.ldap.core.AttributesMapper
 import org.springframework.ldap.core.DirContextOperations
@@ -58,6 +63,7 @@ class LdapTemplateExtensionsTest {
 
     @Test
     fun `add role successfully`() {
+        whenever(ldapTemplate.lookup(any<LdapName>())).thenThrow(NameNotFoundException("no existing role"))
         whenever(ldapTemplate.lookupContext(any<LdapName>()))
             .thenReturn(dirContextOperations)
         whenever(dirContextOperations.nameInNamespace)
@@ -100,7 +106,7 @@ class LdapTemplateExtensionsTest {
             )
         }
 
-        assertThat(res.message, equalTo("NDeliusRole of UNKNOWN not found"))
+        assertThat(res.message, equalTo("Role with name of UNKNOWN not found"))
     }
 
     @Test
@@ -139,17 +145,13 @@ class LdapTemplateExtensionsTest {
 
     @Test
     fun `unknown username throws NotFoundException when adding roles`() {
-        whenever(ldapTemplate.lookupContext(any<LdapName>()))
-            .thenReturn(dirContextOperations)
+        whenever(ldapTemplate.lookupContext(any<LdapName>())).thenReturn(dirContextOperations)
         whenever(dirContextOperations.nameInNamespace)
             .thenReturn("cn=ROLE1,cn=ndRoleCatalogue,ou=Users,dc=moj,dc=com")
 
-        whenever(ldapTemplate.rebind(any<Name>(), anyOrNull(), any<Attributes>())).thenThrow(
-            NameNotFoundException("No Such Object")
-        )
-        whenever(ldapTemplate.unbind(any<Name>())).thenThrow(
-            NameNotFoundException("No Such Object")
-        )
+        whenever(ldapTemplate.lookup(any<LdapName>())).thenThrow(NameNotFoundException("no existing role"))
+        whenever(ldapTemplate.rebind(any<Name>(), anyOrNull(), any<Attributes>()))
+            .thenThrow(NameNotFoundException("no user"))
 
         assertThrows<NotFoundException> {
             ldapTemplate.addRole(
@@ -161,6 +163,12 @@ class LdapTemplateExtensionsTest {
                 }
             )
         }
+    }
+
+    @Test
+    fun `unknown username throws NotFoundException when removing roles`() {
+        whenever(ldapTemplate.lookup(any<LdapName>())).thenReturn("existing role")
+        whenever(ldapTemplate.unbind(any<Name>())).thenThrow(NameNotFoundException("no user"))
 
         assertThrows<NotFoundException> {
             ldapTemplate.removeRole(

diff --git a/...and-delius/src/integrationTest/kotlin/uk/gov/justice/digital/hmpps/UserIntegrationTest.kt b/...and-delius/src/integrationTest/kotlin/uk/gov/justice/digital/hmpps/UserIntegrationTest.kt
@@ -66,7 +66,7 @@ internal class UserIntegrationTest {
             .andExpect(status().isNotFound)
         mockMvc
             .perform(delete("/users/nonexistent.user/roles/LHDCBT003").withToken())
-            .andExpect(status().isNotFound)
+            .andExpect(status().isOk)
         mockMvc
             .perform(get("/users/nonexistent.user/details").withToken())
             .andExpect(status().isNotFound)