Copilot commented on code in PR #13949:
URL: https://github.com/apache/cloudstack/pull/13949#discussion_r3967490121
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -515,10 +520,23 @@ public LinkAccountToLdapResponse
linkAccountToLdap(LinkAccountToLdapCmd cmd) {
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.
+ */
Review Comment:
The PR description appears to still contain template placeholders and none
of the 'Types of changes' / severity checkboxes are selected. Since your
automation relies on this metadata, please update the PR description to
accurately reflect the change type (bug fix/enhancement/etc.) and how it was
tested.
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -515,10 +520,23 @@ public LinkAccountToLdapResponse
linkAccountToLdap(LinkAccountToLdapCmd cmd) {
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("account {} in domain {} is already linked to ldap {}
'{}'; replacing with the new mapping", accountId, domainId, ownVo.getType(),
ownVo.getName());
Review Comment:
When relinking to the same target (same type/name), this always expunges and
re-inserts the mapping. That creates unnecessary DB writes and a warn log for a
no-op update. Consider short-circuiting when the existing mapping matches the
requested mapping (skip expunge/persist and return the existing mapping),
and/or reduce the log level for benign relinks.
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -473,8 +475,11 @@ private LinkAccountToLdapResponse
linkAccountToLdapAndGetResponse(LinkAccountToL
}
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));
+ });
Review Comment:
The explicit cast to TransactionCallback reduces readability and can be
avoided in most cases by using generic method qualification (e.g.,
Transaction.<LdapTrustMapVO>execute(...)) or an explicit anonymous
TransactionCallback implementation. If overload resolution is the reason for
the cast, consider refactoring to make the intended overload unambiguous.
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -515,10 +520,23 @@ public LinkAccountToLdapResponse
linkAccountToLdap(LinkAccountToLdapCmd cmd) {
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("account {} in domain {} is already linked to ldap {}
'{}'; 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());
Review Comment:
Correct the spelling of variable name 'oldAcount' to 'oldAccount' for
clarity and consistency.
##########
plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java:
##########
@@ -0,0 +1,185 @@
+// 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.doThrow;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Tests {@link LdapManagerImpl#linkAccountToLdap}: re-linking an account
replaces its own
+ * mapping, refuses a group already claimed by another live account, and
leaves no mapping
+ * persisted 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();
Review Comment:
Static mocks can leak if `@Before` fails before `@After` runs. To make
cleanup more robust, consider scoping the static mock with try-with-resources
inside each test, or using a pattern that guarantees closure even when setup
throws.
##########
plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java:
##########
@@ -0,0 +1,185 @@
+// 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.doThrow;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Tests {@link LdapManagerImpl#linkAccountToLdap}: re-linking an account
replaces its own
+ * mapping, refuses a group already claimed by another live account, and
leaves no mapping
+ * persisted 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);
+
+ LinkAccountToLdapCmd cmd =
buildCmd("cn=claimed,dc=my,dc=domain,dc=com");
+ assertThrows(CloudRuntimeException.class, () ->
ldapManager.linkAccountToLdap(cmd));
+
+ 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);
Review Comment:
The same constant (OLD_MAPPING_ID) is reused for different logical mappings
(account’s own mapping vs another account’s mapping). Using distinct constants
(e.g., OWN_MAPPING_ID and OTHER_MAPPING_ID) would make the tests easier to read
and reduce confusion when adding verifications around expunge calls.
##########
plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java:
##########
@@ -0,0 +1,185 @@
+// 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.doThrow;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Tests {@link LdapManagerImpl#linkAccountToLdap}: re-linking an account
replaces its own
+ * mapping, refuses a group already claimed by another live account, and
leaves no mapping
+ * persisted 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;
Review Comment:
The same constant (OLD_MAPPING_ID) is reused for different logical mappings
(account’s own mapping vs another account’s mapping). Using distinct constants
(e.g., OWN_MAPPING_ID and OTHER_MAPPING_ID) would make the tests easier to read
and reduce confusion when adding verifications around expunge calls.
##########
plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java:
##########
@@ -0,0 +1,185 @@
+// 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.doThrow;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Tests {@link LdapManagerImpl#linkAccountToLdap}: re-linking an account
replaces its own
+ * mapping, refuses a group already claimed by another live account, and
leaves no mapping
+ * persisted 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);
+
+ LinkAccountToLdapCmd cmd =
buildCmd("cn=claimed,dc=my,dc=domain,dc=com");
+ assertThrows(CloudRuntimeException.class, () ->
ldapManager.linkAccountToLdap(cmd));
+
+ 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);
Review Comment:
The same constant (OLD_MAPPING_ID) is reused for different logical mappings
(account’s own mapping vs another account’s mapping). Using distinct constants
(e.g., OWN_MAPPING_ID and OTHER_MAPPING_ID) would make the tests easier to read
and reduce confusion when adding verifications around expunge calls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
