From 6f48f767703ad337ce8edfd9f0c80c3f077869d7 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Sat, 22 Aug 2026 17:33:14 +0200 Subject: [PATCH 1/5] allow linking a domain to a LDAP when it was already used before --- .../cloudstack/ldap/LdapManagerImpl.java | 39 ++++- .../cloudstack/ldap/LdapManagerImplTest.java | 154 ++++++++++++++++++ 2 files changed, 192 insertions(+), 1 deletion(-) create mode 100644 plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java diff --git a/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java b/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java index dbb4eeb4fdc7..73bab72a3149 100644 --- a/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java +++ b/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java @@ -61,6 +61,8 @@ import com.cloud.user.dao.AccountDao; import com.cloud.utils.Pair; import com.cloud.utils.component.ComponentLifecycleBase; +import com.cloud.utils.db.Transaction; +import com.cloud.utils.db.TransactionCallback; import com.cloud.utils.exception.CloudRuntimeException; @Component @@ -425,7 +427,11 @@ private LinkDomainToLdapResponse linkDomainToLdap(Long domainId, String type, St //Account type should be 0 or 2. check the constants in com.cloud.user.Account Validate.isTrue(accountType== Account.Type.NORMAL || accountType== Account.Type.DOMAIN_ADMIN, "accountype should be either 0(normal user) or 2(domain admin)"); LinkType linkType = LdapManager.LinkType.valueOf(type.toUpperCase()); - LdapTrustMapVO vo = _ldapTrustMapDao.persist(new LdapTrustMapVO(domainId, linkType, name, accountType, 0)); + LdapTrustMapVO vo = Transaction.execute((TransactionCallback) status -> { + ensureGroupNotClaimedByLiveAccount(domainId, name); + clearOldDomainMapping(domainId); + return _ldapTrustMapDao.persist(new LdapTrustMapVO(domainId, linkType, name, accountType, 0)); + }); DomainVO domain = domainDao.findById(vo.getDomainId()); String domainUuid = ""; if (domain == null) { @@ -488,6 +494,37 @@ public LinkAccountToLdapResponse linkAccountToLdap(LinkAccountToLdapCmd cmd) { return response; } + /** + * Replaces a domain's existing LDAP mapping, if any, instead of leaving a second + * {@link #linkDomainToLdap} call to fail on the domain_id/account_id unique key. + */ + private void clearOldDomainMapping(Long domainId) { + LdapTrustMapVO oldVo = _ldapTrustMapDao.findByDomainId(domainId); + if (oldVo != null) { + logger.warn(String.format("domain %d is already linked to ldap %s '%s'; replacing with the new mapping", domainId, oldVo.getType(), oldVo.getName())); + _ldapTrustMapDao.expunge(oldVo.getId()); + } + } + + /** + * Refuses to hand a GROUP/OU to the domain-wide mapping while a live account still + * claims it via {@link #linkAccountToLdap}, mirroring the reverse check in + * {@link #clearOldAccountMapping}. + */ + private void ensureGroupNotClaimedByLiveAccount(Long domainId, String ldapDomain) { + LdapTrustMapVO existing = _ldapTrustMapDao.findGroupInDomain(domainId, ldapDomain); + if (existing == null || existing.getAccountId() == 0L) { + return; + } + AccountVO existingAccount = accountDao.findByIdIncludingRemoved(existing.getAccountId()); + if (existingAccount.getRemoved() == null) { + String msg = String.format("group/OU %s is already mapped to account %d in domain %d; unlink that account before linking the domain to it.", + ldapDomain, existing.getAccountId(), domainId); + logger.error(msg); + throw new CloudRuntimeException(msg); + } + } + private void clearOldAccountMapping(LinkAccountToLdapCmd cmd) { // first find if exists log warning and update LdapTrustMapVO oldVo = _ldapTrustMapDao.findGroupInDomain(cmd.getDomainId(), cmd.getLdapDomain()); diff --git a/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java b/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java new file mode 100644 index 000000000000..740f88ddbd0e --- /dev/null +++ b/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java @@ -0,0 +1,154 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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. +package org.apache.cloudstack.ldap; + +import com.cloud.domain.DomainVO; +import com.cloud.domain.dao.DomainDao; +import com.cloud.user.Account; +import com.cloud.user.AccountVO; +import com.cloud.user.dao.AccountDao; +import com.cloud.utils.exception.CloudRuntimeException; +import org.apache.cloudstack.api.command.LinkDomainToLdapCmd; +import org.apache.cloudstack.api.response.LinkDomainToLdapResponse; +import org.apache.cloudstack.ldap.dao.LdapTrustMapDao; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; +import org.springframework.test.util.ReflectionTestUtils; + +import java.util.Date; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Regression tests: re-linking a domain to LDAP must replace its existing mapping + * instead of failing on the domain_id/account_id unique key, must not silently steal + * a group still claimed by a live account, and must not persist a new mapping if + * clearing the old one fails. + */ +@RunWith(MockitoJUnitRunner.class) +public class LdapManagerImplTest { + + private static final long DOMAIN_ID = 1L; + private static final long OLD_MAPPING_ID = 5L; + + private LdapManagerImpl ldapManager; + + @Mock + private LdapTrustMapDao ldapTrustMapDaoMock; + + @Mock + private LdapConfiguration ldapConfigurationMock; + + @Mock + private DomainDao domainDaoMock; + + @Mock + private AccountDao accountDaoMock; + + @Before + public void setup() { + ldapManager = new LdapManagerImpl(); + ldapManager._ldapTrustMapDao = ldapTrustMapDaoMock; + ReflectionTestUtils.setField(ldapManager, "_ldapConfiguration", ldapConfigurationMock); + ReflectionTestUtils.setField(ldapManager, "domainDao", domainDaoMock); + ReflectionTestUtils.setField(ldapManager, "accountDao", accountDaoMock); + when(ldapConfigurationMock.getBaseDn(DOMAIN_ID)).thenReturn("dc=my,dc=domain,dc=com"); + when(domainDaoMock.findById(DOMAIN_ID)).thenReturn(new DomainVO()); + } + + @Test + public void relinkingDomainReplacesExistingMapping() { + LdapTrustMapVO oldMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=old,dc=my,dc=domain,dc=com", Account.Type.NORMAL, 0); + ReflectionTestUtils.setField(oldMapping, "id", OLD_MAPPING_ID); + when(ldapTrustMapDaoMock.findByDomainId(DOMAIN_ID)).thenReturn(oldMapping); + when(ldapTrustMapDaoMock.persist(any())).thenAnswer(invocation -> invocation.getArgument(0)); + + LinkDomainToLdapResponse response = ldapManager.linkDomainToLdap(buildCmd("cn=new,dc=my,dc=domain,dc=com")); + + verify(ldapTrustMapDaoMock, times(1)).expunge(Long.valueOf(OLD_MAPPING_ID)); + assertEquals("cn=new,dc=my,dc=domain,dc=com", response.getLdapDomain()); + } + + @Test + public void firstLinkOfDomainDoesNotExpungeAnything() { + when(ldapTrustMapDaoMock.findByDomainId(DOMAIN_ID)).thenReturn(null); + when(ldapTrustMapDaoMock.persist(any())).thenAnswer(invocation -> invocation.getArgument(0)); + + ldapManager.linkDomainToLdap(buildCmd("cn=first,dc=my,dc=domain,dc=com")); + + verify(ldapTrustMapDaoMock, never()).expunge(any(Long.class)); + } + + @Test + public void linkingDomainRefusesGroupClaimedByLiveAccount() { + long liveAccountId = 42L; + LdapTrustMapVO accountMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=claimed,dc=my,dc=domain,dc=com", Account.Type.NORMAL, liveAccountId); + when(ldapTrustMapDaoMock.findGroupInDomain(DOMAIN_ID, "cn=claimed,dc=my,dc=domain,dc=com")).thenReturn(accountMapping); + AccountVO liveAccount = new AccountVO(); + when(accountDaoMock.findByIdIncludingRemoved(liveAccountId)).thenReturn(liveAccount); + + assertThrows(CloudRuntimeException.class, () -> ldapManager.linkDomainToLdap(buildCmd("cn=claimed,dc=my,dc=domain,dc=com"))); + + verify(ldapTrustMapDaoMock, never()).persist(any()); + } + + @Test + public void linkingDomainAllowsGroupOnceClaimingAccountIsRemoved() { + long removedAccountId = 42L; + LdapTrustMapVO accountMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=stale,dc=my,dc=domain,dc=com", Account.Type.NORMAL, removedAccountId); + when(ldapTrustMapDaoMock.findGroupInDomain(DOMAIN_ID, "cn=stale,dc=my,dc=domain,dc=com")).thenReturn(accountMapping); + AccountVO removedAccount = new AccountVO(); + ReflectionTestUtils.setField(removedAccount, "removed", new Date()); + when(accountDaoMock.findByIdIncludingRemoved(removedAccountId)).thenReturn(removedAccount); + when(ldapTrustMapDaoMock.persist(any())).thenAnswer(invocation -> invocation.getArgument(0)); + + LinkDomainToLdapResponse response = ldapManager.linkDomainToLdap(buildCmd("cn=stale,dc=my,dc=domain,dc=com")); + + assertEquals("cn=stale,dc=my,dc=domain,dc=com", response.getLdapDomain()); + } + + @Test + public void linkingDomainDoesNotPersistWhenClearingOldMappingFails() { + LdapTrustMapVO oldMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=old,dc=my,dc=domain,dc=com", Account.Type.NORMAL, 0); + ReflectionTestUtils.setField(oldMapping, "id", OLD_MAPPING_ID); + when(ldapTrustMapDaoMock.findByDomainId(DOMAIN_ID)).thenReturn(oldMapping); + Mockito.doThrow(new CloudRuntimeException("db blip")).when(ldapTrustMapDaoMock).expunge(Long.valueOf(OLD_MAPPING_ID)); + + assertThrows(CloudRuntimeException.class, () -> ldapManager.linkDomainToLdap(buildCmd("cn=new,dc=my,dc=domain,dc=com"))); + + verify(ldapTrustMapDaoMock, never()).persist(any()); + } + + private LinkDomainToLdapCmd buildCmd(String ldapDomain) { + LinkDomainToLdapCmd cmd = new LinkDomainToLdapCmd(); + ReflectionTestUtils.setField(cmd, "domainId", DOMAIN_ID); + ReflectionTestUtils.setField(cmd, "type", "GROUP"); + ReflectionTestUtils.setField(cmd, "ldapDomain", ldapDomain); + ReflectionTestUtils.setField(cmd, "accountType", Account.Type.NORMAL.ordinal()); + return cmd; + } +} From 291f662544f44ada6af54e444a4d85ac9679f7b9 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Sat, 22 Aug 2026 17:46:30 +0200 Subject: [PATCH 2/5] check accounts/users of the old domain to unlink before allowing the new one to be linked instead --- .../cloudstack/ldap/LdapManagerImpl.java | 32 +++++++++ .../cloudstack/ldap/LdapManagerImplTest.java | 69 +++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java b/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java index 73bab72a3149..2f493e4cd62a 100644 --- a/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java +++ b/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java @@ -58,7 +58,9 @@ import com.cloud.user.AccountManager; import com.cloud.user.AccountVO; import com.cloud.user.DomainManager; +import com.cloud.user.User; import com.cloud.user.dao.AccountDao; +import com.cloud.user.dao.UserDao; import com.cloud.utils.Pair; import com.cloud.utils.component.ComponentLifecycleBase; import com.cloud.utils.db.Transaction; @@ -77,6 +79,9 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag @Inject private AccountDao accountDao; + @Inject + private UserDao userDao; + @Inject private LdapContextFactory _ldapContextFactory; @@ -501,11 +506,38 @@ public LinkAccountToLdapResponse linkAccountToLdap(LinkAccountToLdapCmd cmd) { private void clearOldDomainMapping(Long domainId) { LdapTrustMapVO oldVo = _ldapTrustMapDao.findByDomainId(domainId); if (oldVo != null) { + ensureOldDomainMappingNotInUse(domainId, oldVo); logger.warn(String.format("domain %d is already linked to ldap %s '%s'; replacing with the new mapping", domainId, oldVo.getType(), oldVo.getName())); _ldapTrustMapDao.expunge(oldVo.getId()); } } + /** + * Refuses to drop the domain's current LDAP mapping while a live account still relies + * on it: an LDAP-sourced account with no per-account mapping of its own (see + * {@link #linkAccountToLdap}) can only have been provisioned through this domain-wide + * mapping, so dropping it would silently orphan that provisioning link. + */ + private void ensureOldDomainMappingNotInUse(Long domainId, LdapTrustMapVO oldMapping) { + List dependentAccountNames = new ArrayList<>(); + for (AccountVO account : accountDao.findActiveAccountsForDomain(domainId)) { + if (_ldapTrustMapDao.findByAccount(domainId, account.getAccountId()) != null) { + continue; + } + boolean hasLdapUser = userDao.listByAccount(account.getAccountId()).stream() + .anyMatch(user -> User.Source.LDAP.equals(user.getSource())); + if (hasLdapUser) { + dependentAccountNames.add(account.getAccountName()); + } + } + if (!dependentAccountNames.isEmpty()) { + String msg = String.format("domain %d has account(s) %s relying on its current ldap mapping %s '%s'; unlink or migrate them before linking the domain to a different GROUP or OU.", + domainId, String.join(", ", dependentAccountNames), oldMapping.getType(), oldMapping.getName()); + logger.error(msg); + throw new CloudRuntimeException(msg); + } + } + /** * Refuses to hand a GROUP/OU to the domain-wide mapping while a live account still * claims it via {@link #linkAccountToLdap}, mirroring the reverse check in diff --git a/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java b/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java index 740f88ddbd0e..58af0b2e785a 100644 --- a/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java +++ b/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java @@ -20,7 +20,10 @@ import com.cloud.domain.dao.DomainDao; import com.cloud.user.Account; import com.cloud.user.AccountVO; +import com.cloud.user.User; +import com.cloud.user.UserVO; import com.cloud.user.dao.AccountDao; +import com.cloud.user.dao.UserDao; import com.cloud.utils.exception.CloudRuntimeException; import org.apache.cloudstack.api.command.LinkDomainToLdapCmd; import org.apache.cloudstack.api.response.LinkDomainToLdapResponse; @@ -33,7 +36,9 @@ import org.mockito.junit.MockitoJUnitRunner; import org.springframework.test.util.ReflectionTestUtils; +import java.util.Collections; import java.util.Date; +import java.util.List; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; @@ -69,6 +74,9 @@ public class LdapManagerImplTest { @Mock private AccountDao accountDaoMock; + @Mock + private UserDao userDaoMock; + @Before public void setup() { ldapManager = new LdapManagerImpl(); @@ -76,8 +84,10 @@ public void setup() { ReflectionTestUtils.setField(ldapManager, "_ldapConfiguration", ldapConfigurationMock); ReflectionTestUtils.setField(ldapManager, "domainDao", domainDaoMock); ReflectionTestUtils.setField(ldapManager, "accountDao", accountDaoMock); + ReflectionTestUtils.setField(ldapManager, "userDao", userDaoMock); when(ldapConfigurationMock.getBaseDn(DOMAIN_ID)).thenReturn("dc=my,dc=domain,dc=com"); when(domainDaoMock.findById(DOMAIN_ID)).thenReturn(new DomainVO()); + when(accountDaoMock.findActiveAccountsForDomain(DOMAIN_ID)).thenReturn(Collections.emptyList()); } @Test @@ -143,6 +153,65 @@ public void linkingDomainDoesNotPersistWhenClearingOldMappingFails() { verify(ldapTrustMapDaoMock, never()).persist(any()); } + @Test + public void relinkingDomainRefusesWhenLdapAccountDependsOnOldMapping() { + LdapTrustMapVO oldMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=old,dc=my,dc=domain,dc=com", Account.Type.NORMAL, 0); + ReflectionTestUtils.setField(oldMapping, "id", OLD_MAPPING_ID); + when(ldapTrustMapDaoMock.findByDomainId(DOMAIN_ID)).thenReturn(oldMapping); + + AccountVO dependentAccount = new AccountVO("imported-user", DOMAIN_ID, null, Account.Type.NORMAL, null, "acct-uuid"); + ReflectionTestUtils.setField(dependentAccount, "id", 99L); + when(accountDaoMock.findActiveAccountsForDomain(DOMAIN_ID)).thenReturn(List.of(dependentAccount)); + when(ldapTrustMapDaoMock.findByAccount(DOMAIN_ID, 99L)).thenReturn(null); + UserVO ldapUser = new UserVO(); + ReflectionTestUtils.setField(ldapUser, "source", User.Source.LDAP); + when(userDaoMock.listByAccount(99L)).thenReturn(List.of(ldapUser)); + + assertThrows(CloudRuntimeException.class, () -> ldapManager.linkDomainToLdap(buildCmd("cn=new,dc=my,dc=domain,dc=com"))); + + verify(ldapTrustMapDaoMock, never()).expunge(any(Long.class)); + verify(ldapTrustMapDaoMock, never()).persist(any()); + } + + @Test + public void relinkingDomainAllowsDependentAccountWithItsOwnMapping() { + LdapTrustMapVO oldMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=old,dc=my,dc=domain,dc=com", Account.Type.NORMAL, 0); + ReflectionTestUtils.setField(oldMapping, "id", OLD_MAPPING_ID); + when(ldapTrustMapDaoMock.findByDomainId(DOMAIN_ID)).thenReturn(oldMapping); + when(ldapTrustMapDaoMock.persist(any())).thenAnswer(invocation -> invocation.getArgument(0)); + + AccountVO explicitlyLinkedAccount = new AccountVO("explicit-user", DOMAIN_ID, null, Account.Type.NORMAL, null, "acct-uuid"); + ReflectionTestUtils.setField(explicitlyLinkedAccount, "id", 99L); + when(accountDaoMock.findActiveAccountsForDomain(DOMAIN_ID)).thenReturn(List.of(explicitlyLinkedAccount)); + when(ldapTrustMapDaoMock.findByAccount(DOMAIN_ID, 99L)) + .thenReturn(new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=own,dc=my,dc=domain,dc=com", Account.Type.NORMAL, 99L)); + + LinkDomainToLdapResponse response = ldapManager.linkDomainToLdap(buildCmd("cn=new,dc=my,dc=domain,dc=com")); + + assertEquals("cn=new,dc=my,dc=domain,dc=com", response.getLdapDomain()); + verify(userDaoMock, never()).listByAccount(99L); + } + + @Test + public void relinkingDomainAllowsAccountThatIsNotLdapSourced() { + LdapTrustMapVO oldMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=old,dc=my,dc=domain,dc=com", Account.Type.NORMAL, 0); + ReflectionTestUtils.setField(oldMapping, "id", OLD_MAPPING_ID); + when(ldapTrustMapDaoMock.findByDomainId(DOMAIN_ID)).thenReturn(oldMapping); + when(ldapTrustMapDaoMock.persist(any())).thenAnswer(invocation -> invocation.getArgument(0)); + + AccountVO localAccount = new AccountVO("local-user", DOMAIN_ID, null, Account.Type.NORMAL, null, "acct-uuid"); + ReflectionTestUtils.setField(localAccount, "id", 99L); + when(accountDaoMock.findActiveAccountsForDomain(DOMAIN_ID)).thenReturn(List.of(localAccount)); + when(ldapTrustMapDaoMock.findByAccount(DOMAIN_ID, 99L)).thenReturn(null); + UserVO localUser = new UserVO(); + ReflectionTestUtils.setField(localUser, "source", User.Source.UNKNOWN); + when(userDaoMock.listByAccount(99L)).thenReturn(List.of(localUser)); + + LinkDomainToLdapResponse response = ldapManager.linkDomainToLdap(buildCmd("cn=new,dc=my,dc=domain,dc=com")); + + assertEquals("cn=new,dc=my,dc=domain,dc=com", response.getLdapDomain()); + } + private LinkDomainToLdapCmd buildCmd(String ldapDomain) { LinkDomainToLdapCmd cmd = new LinkDomainToLdapCmd(); ReflectionTestUtils.setField(cmd, "domainId", DOMAIN_ID); From da8b2f4f9d58d0e38a358bae372aa61366363cbd Mon Sep 17 00:00:00 2001 From: dahn Date: Wed, 9 Sep 2026 12:35:14 +0200 Subject: [PATCH 3/5] change log formatting --- .../main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java b/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java index 2f493e4cd62a..9f4e6d8cbd17 100644 --- a/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java +++ b/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java @@ -507,7 +507,7 @@ private void clearOldDomainMapping(Long domainId) { LdapTrustMapVO oldVo = _ldapTrustMapDao.findByDomainId(domainId); if (oldVo != null) { ensureOldDomainMappingNotInUse(domainId, oldVo); - logger.warn(String.format("domain %d is already linked to ldap %s '%s'; replacing with the new mapping", domainId, oldVo.getType(), oldVo.getName())); + logger.warn("domain {} is already linked to ldap {} ‘{}'; replacing with the new mapping", domainId, oldVo.getType(), oldVo.getName()); _ldapTrustMapDao.expunge(oldVo.getId()); } } From f5ad9bf6fec755832ccd01af4bf916f7d2e24a71 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 10 Sep 2026 09:03:25 +0200 Subject: [PATCH 4/5] sonar --- .../apache/cloudstack/ldap/LdapManagerImplTest.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java b/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java index 58af0b2e785a..aed805350cc6 100644 --- a/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java +++ b/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java @@ -32,7 +32,6 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; -import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; import org.springframework.test.util.ReflectionTestUtils; @@ -43,6 +42,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -121,7 +121,8 @@ public void linkingDomainRefusesGroupClaimedByLiveAccount() { AccountVO liveAccount = new AccountVO(); when(accountDaoMock.findByIdIncludingRemoved(liveAccountId)).thenReturn(liveAccount); - assertThrows(CloudRuntimeException.class, () -> ldapManager.linkDomainToLdap(buildCmd("cn=claimed,dc=my,dc=domain,dc=com"))); + LinkDomainToLdapCmd cmd = buildCmd("cn=claimed,dc=my,dc=domain,dc=com"); + assertThrows(CloudRuntimeException.class, () -> ldapManager.linkDomainToLdap(cmd)); verify(ldapTrustMapDaoMock, never()).persist(any()); } @@ -146,9 +147,10 @@ public void linkingDomainDoesNotPersistWhenClearingOldMappingFails() { LdapTrustMapVO oldMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=old,dc=my,dc=domain,dc=com", Account.Type.NORMAL, 0); ReflectionTestUtils.setField(oldMapping, "id", OLD_MAPPING_ID); when(ldapTrustMapDaoMock.findByDomainId(DOMAIN_ID)).thenReturn(oldMapping); - Mockito.doThrow(new CloudRuntimeException("db blip")).when(ldapTrustMapDaoMock).expunge(Long.valueOf(OLD_MAPPING_ID)); + doThrow(new CloudRuntimeException("db blip")).when(ldapTrustMapDaoMock).expunge(Long.valueOf(OLD_MAPPING_ID)); - assertThrows(CloudRuntimeException.class, () -> ldapManager.linkDomainToLdap(buildCmd("cn=new,dc=my,dc=domain,dc=com"))); + LinkDomainToLdapCmd cmd = buildCmd("cn=new,dc=my,dc=domain,dc=com"); + assertThrows(CloudRuntimeException.class, () -> ldapManager.linkDomainToLdap(cmd)); verify(ldapTrustMapDaoMock, never()).persist(any()); } @@ -167,7 +169,8 @@ public void relinkingDomainRefusesWhenLdapAccountDependsOnOldMapping() { ReflectionTestUtils.setField(ldapUser, "source", User.Source.LDAP); when(userDaoMock.listByAccount(99L)).thenReturn(List.of(ldapUser)); - assertThrows(CloudRuntimeException.class, () -> ldapManager.linkDomainToLdap(buildCmd("cn=new,dc=my,dc=domain,dc=com"))); + LinkDomainToLdapCmd cmd = buildCmd("cn=new,dc=my,dc=domain,dc=com"); + assertThrows(CloudRuntimeException.class, () -> ldapManager.linkDomainToLdap(cmd)); verify(ldapTrustMapDaoMock, never()).expunge(any(Long.class)); verify(ldapTrustMapDaoMock, never()).persist(any()); From 5e8cde6d7305ac712990e4a4cb2e161458b1b534 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 10 Sep 2026 13:28:27 +0200 Subject: [PATCH 5/5] co-pilot suggestions --- .../main/java/com/cloud/user/dao/UserDao.java | 8 +++++ .../java/com/cloud/user/dao/UserDaoImpl.java | 21 +++++++++++++ .../cloudstack/ldap/LdapManagerImpl.java | 31 ++++++++++++------- .../cloudstack/ldap/LdapManagerImplTest.java | 19 +++++------- 4 files changed, 56 insertions(+), 23 deletions(-) diff --git a/engine/schema/src/main/java/com/cloud/user/dao/UserDao.java b/engine/schema/src/main/java/com/cloud/user/dao/UserDao.java index 14b074251508..bcf55eb72dac 100644 --- a/engine/schema/src/main/java/com/cloud/user/dao/UserDao.java +++ b/engine/schema/src/main/java/com/cloud/user/dao/UserDao.java @@ -18,6 +18,7 @@ import java.util.List; +import com.cloud.user.User; import com.cloud.user.UserVO; import com.cloud.utils.db.GenericDao; @@ -37,6 +38,13 @@ public interface UserDao extends GenericDao { List listByAccount(long accountId); + /** + * Bulk-fetches, in a single query, the ids of every account in {@code accountIds} that has + * at least one user with the given {@code source}; avoids one {@link #listByAccount(long)} + * call per account when checking many accounts at once. + */ + List listAccountIdsBySource(List accountIds, User.Source source); + /** * Finds a user based on the secret key provided. * @param secretKey diff --git a/engine/schema/src/main/java/com/cloud/user/dao/UserDaoImpl.java b/engine/schema/src/main/java/com/cloud/user/dao/UserDaoImpl.java index 8baf732c2406..27d9f0bfa893 100644 --- a/engine/schema/src/main/java/com/cloud/user/dao/UserDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/user/dao/UserDaoImpl.java @@ -16,6 +16,7 @@ // under the License. package com.cloud.user.dao; +import java.util.Collections; import java.util.List; @@ -23,9 +24,11 @@ import org.springframework.stereotype.Component; +import com.cloud.user.User; import com.cloud.user.UserVO; import com.cloud.utils.db.DB; import com.cloud.utils.db.GenericDaoBase; +import com.cloud.utils.db.GenericSearchBuilder; import com.cloud.utils.db.SearchBuilder; import com.cloud.utils.db.SearchCriteria; @@ -39,6 +42,7 @@ public class UserDaoImpl extends GenericDaoBase implements UserDao protected SearchBuilder AccountIdSearch; protected SearchBuilder SecretKeySearch; protected SearchBuilder RegistrationTokenSearch; + protected GenericSearchBuilder AccountIdsBySourceSearch; @Inject private AccountDao accountDao; @@ -72,6 +76,12 @@ protected UserDaoImpl() { RegistrationTokenSearch = createSearchBuilder(); RegistrationTokenSearch.and("registrationToken", RegistrationTokenSearch.entity().getRegistrationToken(), SearchCriteria.Op.EQ); RegistrationTokenSearch.done(); + + AccountIdsBySourceSearch = createSearchBuilder(Long.class); + AccountIdsBySourceSearch.selectFields(AccountIdsBySourceSearch.entity().getAccountId()); + AccountIdsBySourceSearch.and("accountIds", AccountIdsBySourceSearch.entity().getAccountId(), SearchCriteria.Op.IN); + AccountIdsBySourceSearch.and("source", AccountIdsBySourceSearch.entity().getSource(), SearchCriteria.Op.EQ); + AccountIdsBySourceSearch.done(); } @Override @@ -142,4 +152,15 @@ public List findUsersByName(String username) { return listBy(sc); } + @Override + public List listAccountIdsBySource(List accountIds, User.Source source) { + if (accountIds == null || accountIds.isEmpty()) { + return Collections.emptyList(); + } + SearchCriteria sc = AccountIdsBySourceSearch.create(); + sc.setParameters("accountIds", accountIds.toArray()); + sc.setParameters("source", source); + return customSearch(sc, null); + } + } diff --git a/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java b/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java index 9f4e6d8cbd17..29f2027a45c5 100644 --- a/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java +++ b/plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java @@ -18,9 +18,12 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.UUID; +import java.util.stream.Collectors; import javax.inject.Inject; import javax.naming.ConfigurationException; @@ -507,7 +510,7 @@ private void clearOldDomainMapping(Long domainId) { LdapTrustMapVO oldVo = _ldapTrustMapDao.findByDomainId(domainId); if (oldVo != null) { ensureOldDomainMappingNotInUse(domainId, oldVo); - logger.warn("domain {} is already linked to ldap {} ‘{}'; replacing with the new mapping", domainId, oldVo.getType(), oldVo.getName()); + logger.warn("domain {} is already linked to ldap {} '{}'; replacing with the new mapping", domainId, oldVo.getType(), oldVo.getName()); _ldapTrustMapDao.expunge(oldVo.getId()); } } @@ -519,17 +522,23 @@ private void clearOldDomainMapping(Long domainId) { * mapping, so dropping it would silently orphan that provisioning link. */ private void ensureOldDomainMappingNotInUse(Long domainId, LdapTrustMapVO oldMapping) { - List dependentAccountNames = new ArrayList<>(); - for (AccountVO account : accountDao.findActiveAccountsForDomain(domainId)) { - if (_ldapTrustMapDao.findByAccount(domainId, account.getAccountId()) != null) { - continue; - } - boolean hasLdapUser = userDao.listByAccount(account.getAccountId()).stream() - .anyMatch(user -> User.Source.LDAP.equals(user.getSource())); - if (hasLdapUser) { - dependentAccountNames.add(account.getAccountName()); - } + List activeAccounts = accountDao.findActiveAccountsForDomain(domainId); + if (activeAccounts.isEmpty()) { + return; } + Set accountsWithOwnMapping = _ldapTrustMapDao.searchByDomainId(domainId).stream() + .map(LdapTrustMapVO::getAccountId) + .filter(accountId -> accountId != 0L) + .collect(Collectors.toSet()); + List candidateAccountIds = activeAccounts.stream() + .map(AccountVO::getAccountId) + .filter(accountId -> !accountsWithOwnMapping.contains(accountId)) + .collect(Collectors.toList()); + Set accountIdsWithLdapUser = new HashSet<>(userDao.listAccountIdsBySource(candidateAccountIds, User.Source.LDAP)); + List dependentAccountNames = activeAccounts.stream() + .filter(account -> accountIdsWithLdapUser.contains(account.getAccountId())) + .map(AccountVO::getAccountName) + .collect(Collectors.toList()); if (!dependentAccountNames.isEmpty()) { String msg = String.format("domain %d has account(s) %s relying on its current ldap mapping %s '%s'; unlink or migrate them before linking the domain to a different GROUP or OU.", domainId, String.join(", ", dependentAccountNames), oldMapping.getType(), oldMapping.getName()); diff --git a/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java b/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java index aed805350cc6..b223f0252c71 100644 --- a/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java +++ b/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java @@ -21,7 +21,6 @@ import com.cloud.user.Account; import com.cloud.user.AccountVO; import com.cloud.user.User; -import com.cloud.user.UserVO; import com.cloud.user.dao.AccountDao; import com.cloud.user.dao.UserDao; import com.cloud.utils.exception.CloudRuntimeException; @@ -164,10 +163,8 @@ public void relinkingDomainRefusesWhenLdapAccountDependsOnOldMapping() { AccountVO dependentAccount = new AccountVO("imported-user", DOMAIN_ID, null, Account.Type.NORMAL, null, "acct-uuid"); ReflectionTestUtils.setField(dependentAccount, "id", 99L); when(accountDaoMock.findActiveAccountsForDomain(DOMAIN_ID)).thenReturn(List.of(dependentAccount)); - when(ldapTrustMapDaoMock.findByAccount(DOMAIN_ID, 99L)).thenReturn(null); - UserVO ldapUser = new UserVO(); - ReflectionTestUtils.setField(ldapUser, "source", User.Source.LDAP); - when(userDaoMock.listByAccount(99L)).thenReturn(List.of(ldapUser)); + when(ldapTrustMapDaoMock.searchByDomainId(DOMAIN_ID)).thenReturn(List.of(oldMapping)); + when(userDaoMock.listAccountIdsBySource(List.of(99L), User.Source.LDAP)).thenReturn(List.of(99L)); LinkDomainToLdapCmd cmd = buildCmd("cn=new,dc=my,dc=domain,dc=com"); assertThrows(CloudRuntimeException.class, () -> ldapManager.linkDomainToLdap(cmd)); @@ -186,13 +183,13 @@ public void relinkingDomainAllowsDependentAccountWithItsOwnMapping() { AccountVO explicitlyLinkedAccount = new AccountVO("explicit-user", DOMAIN_ID, null, Account.Type.NORMAL, null, "acct-uuid"); ReflectionTestUtils.setField(explicitlyLinkedAccount, "id", 99L); when(accountDaoMock.findActiveAccountsForDomain(DOMAIN_ID)).thenReturn(List.of(explicitlyLinkedAccount)); - when(ldapTrustMapDaoMock.findByAccount(DOMAIN_ID, 99L)) - .thenReturn(new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=own,dc=my,dc=domain,dc=com", Account.Type.NORMAL, 99L)); + when(ldapTrustMapDaoMock.searchByDomainId(DOMAIN_ID)).thenReturn(List.of(oldMapping, + new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=own,dc=my,dc=domain,dc=com", Account.Type.NORMAL, 99L))); LinkDomainToLdapResponse response = ldapManager.linkDomainToLdap(buildCmd("cn=new,dc=my,dc=domain,dc=com")); assertEquals("cn=new,dc=my,dc=domain,dc=com", response.getLdapDomain()); - verify(userDaoMock, never()).listByAccount(99L); + verify(userDaoMock).listAccountIdsBySource(Collections.emptyList(), User.Source.LDAP); } @Test @@ -205,10 +202,8 @@ public void relinkingDomainAllowsAccountThatIsNotLdapSourced() { AccountVO localAccount = new AccountVO("local-user", DOMAIN_ID, null, Account.Type.NORMAL, null, "acct-uuid"); ReflectionTestUtils.setField(localAccount, "id", 99L); when(accountDaoMock.findActiveAccountsForDomain(DOMAIN_ID)).thenReturn(List.of(localAccount)); - when(ldapTrustMapDaoMock.findByAccount(DOMAIN_ID, 99L)).thenReturn(null); - UserVO localUser = new UserVO(); - ReflectionTestUtils.setField(localUser, "source", User.Source.UNKNOWN); - when(userDaoMock.listByAccount(99L)).thenReturn(List.of(localUser)); + when(ldapTrustMapDaoMock.searchByDomainId(DOMAIN_ID)).thenReturn(List.of(oldMapping)); + when(userDaoMock.listAccountIdsBySource(List.of(99L), User.Source.LDAP)).thenReturn(Collections.emptyList()); LinkDomainToLdapResponse response = ldapManager.linkDomainToLdap(buildCmd("cn=new,dc=my,dc=domain,dc=com"));