Copilot commented on code in PR #13948:
URL: https://github.com/apache/cloudstack/pull/13948#discussion_r3971779730
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -488,6 +499,64 @@ public LinkAccountToLdapResponse
linkAccountToLdap(LinkAccountToLdapCmd cmd) {
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("domain {} is already linked to ldap {} ‘{}';
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());
+ }
+ }
Review Comment:
This introduces an N+1 query pattern: for each active account, it calls
`findByAccount(...)` and `userDao.listByAccount(...)`. In large domains this
can be expensive and can hold the transaction open longer than necessary.
Consider fetching required data in bulk (e.g., a DAO method that returns
accountIds with per-account LDAP mappings for the domain, and a query that
returns accountIds with LDAP-sourced users), then evaluate dependencies
in-memory with sets/maps.
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -425,7 +432,11 @@ private LinkDomainToLdapResponse linkDomainToLdap(Long
domainId, String type, St
//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));
Review Comment:
The explicit cast to `TransactionCallback<LdapTrustMapVO>` reduces
readability and can mask type inference issues. If possible, adjust the call to
avoid the cast (e.g., by using a typed callback instance/anonymous class, or by
choosing/adding an overload that preserves generics cleanly).
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -488,6 +499,64 @@ public LinkAccountToLdapResponse
linkAccountToLdap(LinkAccountToLdapCmd cmd) {
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("domain {} is already linked to ldap {} ‘{}';
replacing with the new mapping", domainId, oldVo.getType(), oldVo.getName());
Review Comment:
This log message contains a non-ASCII typographic quote character (‘), which
can lead to inconsistent rendering/searching in logs and potential encoding
issues across environments. Replace it with plain ASCII quotes (e.g., ' or \").
--
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]
