allow update of ldap linked account - #13949
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13949 +/- ##
=========================================
Coverage 19.74% 19.74%
- Complexity 19960 19976 +16
=========================================
Files 6371 6371
Lines 575784 575792 +8
Branches 70478 70479 +1
=========================================
+ Hits 113665 113710 +45
+ Misses 449765 449730 -35
+ Partials 12354 12352 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.62% |
| Branch coverage | 18.82% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
4825b6d to
cc543f0
Compare
4ca51b7 to
012307c
Compare
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18984 |
|
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16859)
|
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
LGTM Tested manually
cmk create account accounttype=0 username=qa-admin password=x email=a@b.c firstname=q lastname=a account=qa-team-acct domainid=7d685c93-0d65-4cbb-9afe-223d1000aa7d roleid=4
cmk link accounttoldap domainid=7d685c93-0d65-4cbb-9afe-223d1000aa7d account=qa-team-acct type=GROUP ldapdomain='cn=qa-team,ou=Telco-Bng,dc=example,dc=in' accounttype=0
mysql> SELECT id, domain_id, account_id, name, type FROM cloud.ldap_trust_map;
+----+-----------+------------+------------------------------------------+-------+
| id | domain_id | account_id | name | type |
+----+-----------+------------+------------------------------------------+-------+
| 3 | 2 | 8 | cn=qa-team,ou=Telco-Bng,dc=example,dc=in | GROUP |
+----+-----------+------------+------------------------------------------+-------+
1 row in set (0.00 sec)
Able to login with the user
update the link accounttoldap setting
cmk link accounttoldap domainid=7d685c93-0d65-4cbb-9afe-223d1000aa7d account=qa-team-acct type=GROUP ldapdomain='cn=dev-team,ou=Telco-Bng,dc=example,dc=in' accounttype=0
mysql> SELECT id, domain_id, account_id, name, type FROM cloud.ldap_trust_map;
+----+-----------+------------+-------------------------------------------+-------+
| id | domain_id | account_id | name | type |
+----+-----------+------------+-------------------------------------------+-------+
| 4 | 2 | 8 | cn=dev-team,ou=Telco-Bng,dc=example,dc=in | GROUP |
+----+-----------+------------+-------------------------------------------+-------+
1 row in set (0.00 sec)
Able to login with a new account
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Enables updating an existing LDAP-linked account by making the link operation replace the account’s prior mapping (instead of failing on uniqueness constraints), and adds unit tests around relinking behavior.
Changes:
- Wrap LDAP link update in a transaction and explicitly clear the account’s existing mapping before persisting the new one.
- Adjust conflict checks so relinking to the same group doesn’t error, while still blocking groups claimed by other active accounts.
- Add JUnit tests covering relinking, conflicts, removed accounts, and failure rollback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java | Make relinking atomic and allow replacing an account’s existing LDAP mapping; refine conflict detection. |
| plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java | Add coverage for relinking scenarios and failure modes introduced/changed by the new behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /** | ||
| * 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()); |
| 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)); | ||
| }); |
| 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()); |
|
|
||
| private static final Long DOMAIN_ID = 1L; | ||
| private static final long ACCOUNT_ID = 24L; | ||
| private static final long OLD_MAPPING_ID = 5L; |
| 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(); |
| 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); |
| @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); |
Description
This PR...
Fixes: #11185
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?