This is an automated email from the ASF dual-hosted git repository. DaanHoogland pushed a commit to branch ghi11185-update-ldap-domain-on-account in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit 4825b6d001ae2b1a2a398d94aac255d9e1ed99fd Author: Daan Hoogland <[email protected]> AuthorDate: Sat Aug 22 18:18:57 2026 +0200 allow update of ldap linked account --- .../apache/cloudstack/ldap/LdapManagerImpl.java | 26 ++- .../cloudstack/ldap/LdapManagerImplTest.java | 183 +++++++++++++++++++++ 2 files changed, 205 insertions(+), 4 deletions(-) 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 a93b7a9e133..52bb12a2772 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 @@ -63,6 +63,8 @@ import com.cloud.user.DomainManager; 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 @@ -473,8 +475,11 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag } long accountId = account.getAccountId(); - clearOldAccountMapping(cmd); - LdapTrustMapVO vo = _ldapTrustMapDao.persist(new LdapTrustMapVO(cmd.getDomainId(), linkType, cmd.getLdapDomain(), cmd.getAccountType(), accountId)); + LdapTrustMapVO vo = Transaction.execute((TransactionCallback<LdapTrustMapVO>) status -> { + clearOldAccountMapping(cmd, accountId); + clearAccountsOwnMapping(cmd.getDomainId(), accountId); + return _ldapTrustMapDao.persist(new LdapTrustMapVO(cmd.getDomainId(), linkType, cmd.getLdapDomain(), cmd.getAccountType(), accountId)); + }); return new LinkAccountToLdapResponse(domain.getUuid(), vo.getType().toString(), vo.getName(), vo.getAccountType().ordinal(), account.getUuid(), cmd.getAccountName()); } @@ -515,10 +520,23 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag return linkAccountToLdapAndGetResponse(cmd); } - private void clearOldAccountMapping(LinkAccountToLdapCmd cmd) { + /** + * Replaces the account's existing LDAP mapping, if any, so {@link #linkAccountToLdap} + * can update the ldapDomain/type of an existing link instead of failing on the + * domain_id/account_id unique key. + */ + private void clearAccountsOwnMapping(Long domainId, long accountId) { + LdapTrustMapVO ownVo = _ldapTrustMapDao.findByAccount(domainId, accountId); + if (ownVo != null) { + logger.warn(String.format("account %d in domain %d is already linked to ldap %s '%s'; replacing with the new mapping", accountId, domainId, ownVo.getType(), ownVo.getName())); + _ldapTrustMapDao.expunge(ownVo.getId()); + } + } + + private void clearOldAccountMapping(LinkAccountToLdapCmd cmd, long accountId) { // first find if exists log warning and update LdapTrustMapVO oldVo = _ldapTrustMapDao.findGroupInDomain(cmd.getDomainId(), cmd.getLdapDomain()); - if (oldVo != null) { + if (oldVo != null && oldVo.getAccountId() != accountId) { // deal with edge cases, i.e. check if the old account is indeed deleted etc. if (oldVo.getAccountId() != 0L) { AccountVO oldAcount = accountDao.findByIdIncludingRemoved(oldVo.getAccountId()); 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 00000000000..7c729d604ed --- /dev/null +++ b/plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java @@ -0,0 +1,183 @@ +// 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.acl.RoleService; +import org.apache.cloudstack.api.command.LinkAccountToLdapCmd; +import org.apache.cloudstack.api.response.LinkAccountToLdapResponse; +import org.apache.cloudstack.ldap.dao.LdapTrustMapDao; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.MockedStatic; +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 for #11185: linkAccountToLdap must be usable to update the + * ldapDomain/type of an account's existing LDAP mapping, instead of failing on the + * domain_id/account_id unique key, 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 ACCOUNT_ID = 24L; + private static final long OLD_MAPPING_ID = 5L; + + private LdapManagerImpl ldapManager; + + private MockedStatic<LdapConfiguration> ldapConfigurationMockedStatic; + + @Mock + private LdapTrustMapDao ldapTrustMapDaoMock; + + @Mock + private DomainDao domainDaoMock; + + @Mock + private AccountDao accountDaoMock; + + @Mock + private RoleService roleServiceMock; + + @Before + public void setup() { + ldapConfigurationMockedStatic = Mockito.mockStatic(LdapConfiguration.class, Mockito.CALLS_REAL_METHODS); + when(LdapConfiguration.getBaseDn(DOMAIN_ID)).thenReturn("dc=my,dc=domain,dc=com"); + + ldapManager = new LdapManagerImpl(); + ldapManager._ldapTrustMapDao = ldapTrustMapDaoMock; + ReflectionTestUtils.setField(ldapManager, "domainDao", domainDaoMock); + ReflectionTestUtils.setField(ldapManager, "accountDao", accountDaoMock); + when(domainDaoMock.findById(DOMAIN_ID)).thenReturn(new DomainVO()); + + AccountVO existingAccount = new AccountVO("jdoe", DOMAIN_ID, null, Account.Type.NORMAL, null, "acct-uuid"); + ReflectionTestUtils.setField(existingAccount, "id", ACCOUNT_ID); + when(accountDaoMock.findActiveAccount("jdoe", DOMAIN_ID)).thenReturn(existingAccount); + when(ldapTrustMapDaoMock.persist(any())).thenAnswer(invocation -> invocation.getArgument(0)); + } + + @After + public void tearDown() { + ldapConfigurationMockedStatic.close(); + } + + @Test + public void relinkingAccountReplacesItsOwnExistingMapping() { + LdapTrustMapVO ownMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=old,dc=my,dc=domain,dc=com", Account.Type.NORMAL, ACCOUNT_ID); + ReflectionTestUtils.setField(ownMapping, "id", OLD_MAPPING_ID); + when(ldapTrustMapDaoMock.findByAccount(DOMAIN_ID, ACCOUNT_ID)).thenReturn(ownMapping); + + LinkAccountToLdapResponse response = ldapManager.linkAccountToLdap(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 firstLinkOfAccountDoesNotExpungeAnything() { + when(ldapTrustMapDaoMock.findByAccount(DOMAIN_ID, ACCOUNT_ID)).thenReturn(null); + + ldapManager.linkAccountToLdap(buildCmd("cn=first,dc=my,dc=domain,dc=com")); + + verify(ldapTrustMapDaoMock, never()).expunge(any(Long.class)); + } + + @Test + public void relinkingAccountToItsCurrentGroupDoesNotThrow() { + LdapTrustMapVO ownMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=same,dc=my,dc=domain,dc=com", Account.Type.NORMAL, ACCOUNT_ID); + ReflectionTestUtils.setField(ownMapping, "id", OLD_MAPPING_ID); + when(ldapTrustMapDaoMock.findGroupInDomain(DOMAIN_ID, "cn=same,dc=my,dc=domain,dc=com")).thenReturn(ownMapping); + when(ldapTrustMapDaoMock.findByAccount(DOMAIN_ID, ACCOUNT_ID)).thenReturn(ownMapping); + + LinkAccountToLdapResponse response = ldapManager.linkAccountToLdap(buildCmd("cn=same,dc=my,dc=domain,dc=com")); + + assertEquals("cn=same,dc=my,dc=domain,dc=com", response.getLdapDomain()); + } + + @Test + public void relinkingAccountRefusesGroupClaimedByAnotherLiveAccount() { + long otherAccountId = 99L; + LdapTrustMapVO otherMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=claimed,dc=my,dc=domain,dc=com", Account.Type.NORMAL, otherAccountId); + when(ldapTrustMapDaoMock.findGroupInDomain(DOMAIN_ID, "cn=claimed,dc=my,dc=domain,dc=com")).thenReturn(otherMapping); + AccountVO otherAccount = new AccountVO(); + when(accountDaoMock.findByIdIncludingRemoved(otherAccountId)).thenReturn(otherAccount); + + assertThrows(CloudRuntimeException.class, () -> ldapManager.linkAccountToLdap(buildCmd("cn=claimed,dc=my,dc=domain,dc=com"))); + + verify(ldapTrustMapDaoMock, never()).persist(any()); + } + + @Test + public void relinkingAccountAllowsGroupOnceOtherClaimingAccountIsRemoved() { + long removedAccountId = 99L; + LdapTrustMapVO otherMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=stale,dc=my,dc=domain,dc=com", Account.Type.NORMAL, removedAccountId); + ReflectionTestUtils.setField(otherMapping, "id", OLD_MAPPING_ID); + when(ldapTrustMapDaoMock.findGroupInDomain(DOMAIN_ID, "cn=stale,dc=my,dc=domain,dc=com")).thenReturn(otherMapping); + AccountVO removedAccount = new AccountVO(); + ReflectionTestUtils.setField(removedAccount, "removed", new Date()); + when(accountDaoMock.findByIdIncludingRemoved(removedAccountId)).thenReturn(removedAccount); + + LinkAccountToLdapResponse response = ldapManager.linkAccountToLdap(buildCmd("cn=stale,dc=my,dc=domain,dc=com")); + + assertEquals("cn=stale,dc=my,dc=domain,dc=com", response.getLdapDomain()); + } + + @Test + public void relinkingAccountDoesNotPersistWhenClearingOldMappingFails() { + LdapTrustMapVO ownMapping = new LdapTrustMapVO(DOMAIN_ID, LdapManager.LinkType.GROUP, "cn=old,dc=my,dc=domain,dc=com", Account.Type.NORMAL, ACCOUNT_ID); + ReflectionTestUtils.setField(ownMapping, "id", OLD_MAPPING_ID); + when(ldapTrustMapDaoMock.findByAccount(DOMAIN_ID, ACCOUNT_ID)).thenReturn(ownMapping); + Mockito.doThrow(new CloudRuntimeException("db blip")).when(ldapTrustMapDaoMock).expunge(Long.valueOf(OLD_MAPPING_ID)); + + assertThrows(CloudRuntimeException.class, () -> ldapManager.linkAccountToLdap(buildCmd("cn=new,dc=my,dc=domain,dc=com"))); + + verify(ldapTrustMapDaoMock, never()).persist(any()); + } + + private LinkAccountToLdapCmd buildCmd(String ldapDomain) { + LinkAccountToLdapCmd cmd = new LinkAccountToLdapCmd(); + cmd.roleService = roleServiceMock; + ReflectionTestUtils.setField(cmd, "domainId", DOMAIN_ID); + ReflectionTestUtils.setField(cmd, "type", "GROUP"); + ReflectionTestUtils.setField(cmd, "ldapDomain", ldapDomain); + ReflectionTestUtils.setField(cmd, "accountName", "jdoe"); + ReflectionTestUtils.setField(cmd, "accountType", Account.Type.NORMAL.ordinal()); + return cmd; + } +}
