From fd0d78cec1daf15d756fc9c9e167f915b78e9ab0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 24 Jun 2026 12:53:30 +0000 Subject: [PATCH 1/3] Initial plan From 0f788aab7ce88c35870dcfcb19a9f47ea7d054ae Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 24 Jun 2026 12:58:32 +0000 Subject: [PATCH 2/3] Fix NPE when memberOf attribute is missing in LDAP user lookup - Make memberOf attribute access null-safe in resultToUserEntry - Log a warning when memberOf is absent (e.g. service accounts) - Expose resultToUserEntry as private[search] for testability - Add unit tests covering: groups present, no memberOf (service account), empty memberOf, and optional attributes with absent memberOf --- .../service/search/LdapUserRepository.scala | 14 ++- .../search/LdapUserRepositoryTest.scala | 95 ++++++++++++++++++- 2 files changed, 104 insertions(+), 5 deletions(-) diff --git a/api/src/main/scala/za/co/absa/loginsvc/rest/service/search/LdapUserRepository.scala b/api/src/main/scala/za/co/absa/loginsvc/rest/service/search/LdapUserRepository.scala index 0204e759..5f7a47cb 100644 --- a/api/src/main/scala/za/co/absa/loginsvc/rest/service/search/LdapUserRepository.scala +++ b/api/src/main/scala/za/co/absa/loginsvc/rest/service/search/LdapUserRepository.scala @@ -97,14 +97,20 @@ class LdapUserRepository(activeDirectoryLDAPConfig: ActiveDirectoryLDAPConfig) } else true } - private def resultToUserEntry(result: SearchResult): User = { + private[search] def resultToUserEntry(result: SearchResult): User = { val attrs: Attributes = result.getAttributes val optionalAttr = activeDirectoryLDAPConfig.attributes.getOrElse(Map.empty) val username = attrs.get("sAMAccountName").get.toString - val groups = attrs.get("memberOf").getAll.asScala.map(group => { - group.toString.substring(3, group.toString.indexOf(",")) - }).toSeq + val groups = Option(attrs.get("memberOf")) match { + case None => + logger.warn(s"No memberOf attribute found for user: $username - defaulting to empty groups") + Seq.empty[String] + case Some(memberOfAttr) => + memberOfAttr.getAll.asScala.map(group => { + group.toString.substring(3, group.toString.indexOf(",")) + }).toSeq + } val extraAttributes = optionalAttr.map { case (fieldName, claimName) => val value = Option(attrs.get(fieldName)).map(_.get()) claimName -> value diff --git a/api/src/test/scala/za/co/absa/loginsvc/rest/service/search/LdapUserRepositoryTest.scala b/api/src/test/scala/za/co/absa/loginsvc/rest/service/search/LdapUserRepositoryTest.scala index dfa80b01..c3fff721 100644 --- a/api/src/test/scala/za/co/absa/loginsvc/rest/service/search/LdapUserRepositoryTest.scala +++ b/api/src/test/scala/za/co/absa/loginsvc/rest/service/search/LdapUserRepositoryTest.scala @@ -16,12 +16,16 @@ package za.co.absa.loginsvc.rest.service.search +import org.scalamock.scalatest.MockFactory import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers import za.co.absa.loginsvc.model.User import za.co.absa.loginsvc.rest.config.auth.{ActiveDirectoryLDAPConfig, LdapRetryConfig, LdapUserCredentialsConfig, ServiceAccountConfig} -class LdapUserRepositoryTest extends AnyFlatSpec with Matchers { +import javax.naming.NamingEnumeration +import javax.naming.directory.{Attribute, Attributes, SearchResult} + +class LdapUserRepositoryTest extends AnyFlatSpec with Matchers with MockFactory { private val integratedCfg = LdapUserCredentialsConfig("svc-ldap", "password") private val serviceAccountCfg = ServiceAccountConfig( @@ -88,4 +92,93 @@ class LdapUserRepositoryTest extends AnyFlatSpec with Matchers { } assert(testLdapUserRepository.counter == 3) } + + // Helper to build a mock SearchResult with the given attributes + private def mockSearchResult(username: String, memberOfDNs: Option[Seq[String]], extraFields: Map[String, String] = Map.empty): SearchResult = { + val attrs = mock[Attributes] + val samAttr = mock[Attribute] + (samAttr.get _).expects().returns(username) + (attrs.get _).expects("sAMAccountName").returns(samAttr) + + memberOfDNs match { + case None => + (attrs.get _).expects("memberOf").returns(null) + case Some(dns) => + val memberOfAttr = mock[Attribute] + val namingEnum = mock[NamingEnumeration[Object]] + val iterator = dns.iterator + (namingEnum.hasMoreElements _).expects().onCall(() => iterator.hasNext).anyNumberOfTimes() + (namingEnum.nextElement _).expects().onCall(() => iterator.next().asInstanceOf[Object]).anyNumberOfTimes() + (memberOfAttr.getAll _).expects().returns(namingEnum) + (attrs.get _).expects("memberOf").returns(memberOfAttr) + } + + extraFields.foreach { case (fieldName, _) => + (attrs.get _).expects(fieldName).returns(null) + } + + val searchResult = mock[SearchResult] + (searchResult.getAttributes _).expects().returns(attrs) + searchResult + } + + "resultToUserEntry" should "parse groups correctly when memberOf is present" in { + val repo = new LdapUserRepository(ldapCfgNoRetries) + val memberOfDNs = Seq( + "CN=group1,OU=Groups,DC=domain,DC=com", + "CN=group2,OU=Groups,DC=domain,DC=com" + ) + val result = mockSearchResult("testuser", Some(memberOfDNs)) + + val user = repo.resultToUserEntry(result) + + user.name shouldBe "testuser" + user.groups shouldBe Seq("group1", "group2") + user.optionalAttributes shouldBe Map.empty + } + + "resultToUserEntry" should "return empty groups when memberOf attribute is absent (service account)" in { + val repo = new LdapUserRepository(ldapCfgNoRetries) + val result = mockSearchResult("SVC-eventgate-e2e", None) + + val user = repo.resultToUserEntry(result) + + user.name shouldBe "SVC-eventgate-e2e" + user.groups shouldBe Seq.empty + user.optionalAttributes shouldBe Map.empty + } + + "resultToUserEntry" should "return empty groups when memberOf is empty" in { + val repo = new LdapUserRepository(ldapCfgNoRetries) + val result = mockSearchResult("testuser", Some(Seq.empty)) + + val user = repo.resultToUserEntry(result) + + user.name shouldBe "testuser" + user.groups shouldBe Seq.empty + } + + "resultToUserEntry" should "populate optional attributes when config specifies them" in { + val ldapCfgWithAttrs = ldapCfgNoRetries.copy(attributes = Some(Map("mail" -> "email"))) + val repo = new LdapUserRepository(ldapCfgWithAttrs) + + val attrs = mock[Attributes] + val samAttr = mock[Attribute] + (samAttr.get _).expects().returns("testuser") + (attrs.get _).expects("sAMAccountName").returns(samAttr) + (attrs.get _).expects("memberOf").returns(null) + + val mailAttr = mock[Attribute] + (mailAttr.get _).expects().returns("testuser@example.com") + (attrs.get _).expects("mail").returns(mailAttr) + + val searchResult = mock[SearchResult] + (searchResult.getAttributes _).expects().returns(attrs) + + val user = repo.resultToUserEntry(searchResult) + + user.name shouldBe "testuser" + user.groups shouldBe Seq.empty + user.optionalAttributes shouldBe Map("email" -> Some("testuser@example.com")) + } } From de0ef1891f3994d2c649589828d28fe1981b342b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 24 Jun 2026 12:59:46 +0000 Subject: [PATCH 3/3] Add missing optionalAttributes assertion in empty memberOf test --- .../loginsvc/rest/service/search/LdapUserRepositoryTest.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/api/src/test/scala/za/co/absa/loginsvc/rest/service/search/LdapUserRepositoryTest.scala b/api/src/test/scala/za/co/absa/loginsvc/rest/service/search/LdapUserRepositoryTest.scala index c3fff721..fef0d03a 100644 --- a/api/src/test/scala/za/co/absa/loginsvc/rest/service/search/LdapUserRepositoryTest.scala +++ b/api/src/test/scala/za/co/absa/loginsvc/rest/service/search/LdapUserRepositoryTest.scala @@ -156,6 +156,7 @@ class LdapUserRepositoryTest extends AnyFlatSpec with Matchers with MockFactory user.name shouldBe "testuser" user.groups shouldBe Seq.empty + user.optionalAttributes shouldBe Map.empty } "resultToUserEntry" should "populate optional attributes when config specifies them" in {