allow linking a domain to a LDAP when it was already used before - #13948
DaanHoogland wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #13948 +/- ##
============================================
+ Coverage 16.32% 16.45% +0.13%
- Complexity 13556 13723 +167
============================================
Files 5669 5669
Lines 501399 504403 +3004
Branches 60902 62149 +1247
============================================
+ Hits 81847 83014 +1167
- Misses 410390 412126 +1736
- Partials 9162 9263 +101
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:
|
…new one to be linked instead
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19175 |
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 re-linking a domain to LDAP by safely replacing any existing domain-wide mapping, while preventing accidental takeover of account-linked groups and protecting LDAP-provisioned accounts that still depend on the old domain mapping.
Changes:
- Wrap domain-to-LDAP linking in a DB transaction that checks for conflicts, clears old domain mappings, and persists the new mapping atomically.
- Add guards to (a) prevent domains from claiming a group/OU already mapped to a live account and (b) prevent removing a domain mapping still required by LDAP-sourced accounts.
- Add regression/unit tests covering re-linking, conflict detection, and failure/rollback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java | Adds transactional re-linking flow plus dependency/conflict checks for domain LDAP mappings. |
| plugins/user-authenticators/ldap/src/test/java/org/apache/cloudstack/ldap/LdapManagerImplTest.java | Adds regression tests validating re-linking behavior and the new safety checks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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()); | ||
| } | ||
| } |
| LdapTrustMapVO vo = Transaction.execute((TransactionCallback<LdapTrustMapVO>) status -> { | ||
| ensureGroupNotClaimedByLiveAccount(domainId, name); | ||
| clearOldDomainMapping(domainId); | ||
| return _ldapTrustMapDao.persist(new LdapTrustMapVO(domainId, linkType, name, accountType, 0)); |
|
|
@blueorangutan package |
Description
This PR...
Fixes: #11471
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?