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..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,9 +58,13 @@ 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; +import com.cloud.utils.db.TransactionCallback; import com.cloud.utils.exception.CloudRuntimeException; @Component @@ -75,6 +79,9 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag @Inject private AccountDao accountDao; + @Inject + private UserDao userDao; + @Inject private LdapContextFactory _ldapContextFactory; @@ -425,7 +432,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 +499,64 @@ 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) { + 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 + * {@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..58af0b2e785a --- /dev/null +++ b/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java @@ -0,0 +1,223 @@ +// 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.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; +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.Collections; +import java.util.Date; +import java.util.List; + +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; + + @Mock + private UserDao userDaoMock; + + @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); + 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 + 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()); + } + + @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); + ReflectionTestUtils.setField(cmd, "type", "GROUP"); + ReflectionTestUtils.setField(cmd, "ldapDomain", ldapDomain); + ReflectionTestUtils.setField(cmd, "accountType", Account.Type.NORMAL.ordinal()); + return cmd; + } +}