allow linking a domain to a LDAP when it was already used before

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 @@
     @Inject
     private AccountDao accountDao;
 
+    @Inject
+    private UserDao userDao;
+
     @Inject
     private LdapContextFactory _ldapContextFactory;
 
@@ -425,7 +432,11 @@
         //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<LdapTrustMapVO>) status -> {
+            ensureGroupNotClaimedByLiveAccount(domainId, name);
+            clearOldDomainMapping(domainId);
+            return _ldapTrustMapDao.persist(new LdapTrustMapVO(domainId, linkType, name, accountType, 0));
+        });
         DomainVO domain = domainDao.findById(vo.getDomainId());
         String domainUuid = "<unknown>";
         if (domain == null) {
@@ -488,6 +499,64 @@
         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<String> 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());

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;
+    }
+}