Copilot commented on code in PR #11220:
URL: https://github.com/apache/cloudstack/pull/11220#discussion_r2209869129
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java:
##########
@@ -140,9 +140,10 @@ public void execute() throws ServerApiException {
try {
final List<LdapUser> users = _ldapManager.getUsers(domainId);
ldapResponses = createLdapUserResponse(users);
-// now filter and annotate
+ // now filter and annotate
ldapResponses = applyUserFilter(ldapResponses);
} catch (final NoLdapUserMatchingQueryException ex) {
+ logger.debug(ex.getMessage());
// ok, we'll make do with the empty list ldapResponses = new
ArrayList<LdapUserResponse>();
Review Comment:
The catch block logs the exception but never actually resets `ldapResponses`
to a new empty list. Add `ldapResponses = new ArrayList<>();` to ensure you
don't return stale or null data.
```suggestion
// ok, we'll make do with the empty list
ldapResponses = new ArrayList<>();
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -303,8 +303,8 @@ public LdapUser getUser(final String username, Long
domainId) throws NoLdapUserM
return
_ldapUserManagerFactory.getInstance(_ldapConfiguration.getLdapProvider(null)).getUser(escapedUsername,
context, domainId);
} catch (NamingException | IOException e) {
- logger.debug("ldap Exception: ",e);
- throw new NoLdapUserMatchingQueryException("No Ldap User found for
username: "+username);
+ logger.debug("LDAP Exception: ", e);
+ throw new NoLdapUserMatchingQueryException("Unable to find LDAP
User for username: " + username + ", due to " + e.getMessage());
Review Comment:
Wrap the caught exception as the cause when rethrowing to preserve stack
trace, for example: `new NoLdapUserMatchingQueryException(message, e);`.
```suggestion
throw new NoLdapUserMatchingQueryException("Unable to find LDAP
User for username: " + username + ", due to " + e.getMessage(), e);
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapAuthenticator.java:
##########
@@ -174,7 +176,11 @@ Pair<Boolean, ActionOnFailedAuthentication>
authenticate(String username, String
}
} catch (NoLdapUserMatchingQueryException e) {
logger.debug(e.getMessage());
- disableUserInCloudStack(userAccount);
+ if (e.getMessage().contains(LDAP_READ_TIMED_OUT_MESSAGE) &&
!rc.first()) {
Review Comment:
Parsing exception messages with `contains(...)` is brittle and may break if
the message text changes or is localized. Consider using a dedicated exception
subclass or error code to detect timeouts reliably.
--
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]
