Copilot commented on code in PR #13954:
URL: https://github.com/apache/cloudstack/pull/13954#discussion_r3947626117
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:
##########
@@ -173,30 +174,47 @@ private LdapConfigurationResponse
addConfigurationInternal(final String hostname
// hostname:port is unique for domain binding
LdapConfigurationVO configuration =
_ldapConfigurationDao.find(hostname, port, domainId);
if (configuration == null) {
- LdapContext context = null;
- try {
- final String providerUrl = "ldap://"; + hostname + ":" + port;
- context =
_ldapContextFactory.createBindContext(providerUrl,domainId);
- configuration = new LdapConfigurationVO(hostname, port,
domainId);
- _ldapConfigurationDao.persist(configuration);
- logger.info("Added a new LDAP server with URL: {}{}",
providerUrl, domainId == null ? "" : " for domain " + domainId);
- return createLdapConfigurationResponse(configuration);
- } catch (NamingException | IOException e) {
- logger.debug("NamingException while doing an LDAP bind", e);
- throw new InvalidParameterValueException("Unable to bind to
the given LDAP server");
- } catch (RuntimeException e) {
- if (e.getMessage().contains("Invalid truststore")) {
- throw new InvalidParameterValueException("Invalid
truststore or truststore password");
- }
- throw e;
- } finally {
- closeContext(context);
- }
+ testBind(hostname, port, domainId);
+ configuration = new LdapConfigurationVO(hostname, port, domainId);
+ _ldapConfigurationDao.persist(configuration);
+ logger.info("Added a new LDAP server with URL: ldap://{}:{}{}";,
hostname, port, domainId == null ? "" : " for domain " + domainId);
+ return createLdapConfigurationResponse(configuration);
} else {
throw new InvalidParameterValueException("Duplicate
configuration");
}
}
+ @Override
+ public void testConnection(LdapTestConfigurationCmd cmd) throws
InvalidParameterValueException {
+ int port = cmd.getPort();
+ if (port <= 0) {
+ port = 389;
+ }
+ testBind(cmd.getHostname(), port, cmd.getDomainId());
+ }
+
+ /**
+ * Binds to the given LDAP server without persisting a configuration, so
both adding a new
+ * configuration and {@link #testConnection} can share the same
connectivity check.
+ */
+ private void testBind(final String hostname, final int port, final Long
domainId) throws InvalidParameterValueException {
+ LdapContext context = null;
+ try {
+ final String providerUrl = "ldap://"; + hostname + ":" + port;
+ context = _ldapContextFactory.createBindContext(providerUrl,
domainId);
+ } catch (NamingException | IOException e) {
+ logger.debug("NamingException while doing an LDAP bind", e);
+ throw new InvalidParameterValueException("Unable to bind to the
given LDAP server");
+ } catch (RuntimeException e) {
+ if (e.getMessage().contains("Invalid truststore")) {
+ throw new InvalidParameterValueException("Invalid truststore
or truststore password");
+ }
+ throw e;
+ } finally {
Review Comment:
The LDAP bind exception handling can throw a NullPointerException when
`e.getMessage()` is null (e.g., for some RuntimeException types), which would
mask the real failure and prevent returning a clean API error message.
##########
ui/src/config/section/config.js:
##########
@@ -52,6 +52,34 @@ export default {
'hostname', 'port', 'domainid'
]
},
+ {
+ api: 'testLdapConfiguration',
+ icon: 'ExperimentOutlined',
+ label: 'label.test.ldap.configuration',
+ docHelp:
'adminguide/accounts.html#using-an-ldap-server-for-user-authentication',
+ listView: true,
Review Comment:
`testLdapConfiguration` is defined twice (one `listView` action and one
`dataView` action) with largely duplicated metadata (api/icon/label). Other
sections typically model multi-view actions as a single object with both
`listView` and `dataView` flags, which avoids divergence (e.g., `docHelp` is
currently present only on the list-view action).
--
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]
