Copilot commented on code in PR #60275:
URL: https://github.com/apache/doris/pull/60275#discussion_r2768989860
##########
fe/fe-common/src/main/java/org/apache/doris/common/LdapConfig.java:
##########
@@ -157,4 +157,22 @@ public class LdapConfig extends ConfigBase {
*/
@ConfigBase.ConfField
public static boolean ldap_pool_test_while_idle = true;
+
+ /**
+ * Flag to enable usage of LDAPS.
+ */
+ @ConfigBase.ConfField
+ public static boolean ldap_use_ssl = false;
+
+ /**
+ * The method constructs correct URL connection string for specified host
and port depending on
+ * value of ldap_use_ssl property.
+ * If ldap_use_ssl property is true - LDAPS is used as protocol
+ * If ldap_use_ssl_property is false or not specified - LDAP is used as
protocol
+ * @param hostPortInAccessibleFormat
+ * @return
Review Comment:
The JavaDoc comment is missing proper parameter and return value
documentation. According to Java conventions, the @param tag should be used to
document the hostPortInAccessibleFormat parameter, and the @return tag should
be used to describe what the method returns. Consider updating to:
/**
* The method constructs correct URL connection string for specified host
and port depending on
* value of ldap_use_ssl property.
* If ldap_use_ssl property is true - LDAPS is used as protocol
* If ldap_use_ssl is false or not specified - LDAP is used as protocol
* @param hostPortInAccessibleFormat the host and port in accessible format
* @return the LDAP or LDAPS connection URL string
*/
```suggestion
* The method constructs the correct URL connection string for the
specified host and port depending on
* the value of the {@code ldap_use_ssl} property.
* If {@code ldap_use_ssl} is true, LDAPS is used as the protocol.
* If {@code ldap_use_ssl} is false or not specified, LDAP is used as
the protocol.
* @param hostPortInAccessibleFormat the host and port in accessible
format (for example, "host:port")
* @return the LDAP or LDAPS connection URL string
```
##########
conf/ldap.conf:
##########
@@ -44,6 +44,9 @@ ldap_group_basedn = ou=group,dc=domain,dc=com
# ldap_user_cache_timeout_s = 5 * 60;
+## ldap_use_ssl - use secured connection to LDAP server if required (disabled
by default)
Review Comment:
The comment should include a note about the port being used. When
ldap_use_ssl is set to true, users typically need to change the port from 389
(LDAP default) to 636 (LDAPS default). Consider updating the comment to mention
that users should also update ldap_port when enabling SSL. For example: "use
secured connection to LDAP server if required (disabled by default). Note: When
enabling SSL, ensure ldap_port is set appropriately (typically 636 for LDAPS
instead of 389 for LDAP)."
```suggestion
## ldap_use_ssl - use secured connection to LDAP server if required
(disabled by default). Note: When enabling SSL, ensure ldap_port is set
appropriately (typically 636 for LDAPS instead of 389 for LDAP).
```
##########
fe/fe-core/src/test/java/org/apache/doris/mysql/authenticate/ldap/LdapClientTest.java:
##########
@@ -95,4 +97,28 @@ public void testGetGroups() {
};
Assert.assertEquals(1, ldapClient.getGroups("zhangsan").size());
}
+
+ @Test
+ public void testSecuredProtocolIsUsed() {
+ //testing default case with not specified property ldap_use_ssl or it
is specified as false
+ String insecureUrl = LdapConfig.getConnectionURL(
+ NetUtils.getHostPortInAccessibleFormat(LdapConfig.ldap_host,
LdapConfig.ldap_port));
+
+ Assert.assertNotNull("connection URL should not be null", insecureUrl);
+ Assert.assertTrue("with ldap_use_ssl = false or not specified URL
should start with ldap, but received: " + insecureUrl,
+ insecureUrl.startsWith("ldap://";));
+
+ //testing new case with specified property ldap_use_ssl as true
+ LdapConfig.ldap_use_ssl = true;
+ String secureUrl = LdapConfig.getConnectionURL(
+ NetUtils.getHostPortInAccessibleFormat(LdapConfig.ldap_host,
LdapConfig.ldap_port));
+ Assert.assertNotNull("connection URL should not be null", secureUrl);
+ Assert.assertTrue("with ldap_use_ssl = true URL should start with
ldaps, but received: " + secureUrl,
+ secureUrl.startsWith("ldaps://"));
+ }
Review Comment:
The test modifies the static field LdapConfig.ldap_use_ssl, which could
potentially cause issues with test isolation if other tests run concurrently or
if this test fails before reaching the tearDown method. Consider also resetting
ldap_use_ssl in the setUp method to ensure consistent test state, or use a
try-finally block within the test method itself to guarantee cleanup even if
assertions fail.
##########
fe/fe-common/src/main/java/org/apache/doris/common/LdapConfig.java:
##########
@@ -157,4 +157,22 @@ public class LdapConfig extends ConfigBase {
*/
@ConfigBase.ConfField
public static boolean ldap_pool_test_while_idle = true;
+
+ /**
+ * Flag to enable usage of LDAPS.
+ */
+ @ConfigBase.ConfField
+ public static boolean ldap_use_ssl = false;
+
+ /**
+ * The method constructs correct URL connection string for specified host
and port depending on
+ * value of ldap_use_ssl property.
+ * If ldap_use_ssl property is true - LDAPS is used as protocol
+ * If ldap_use_ssl_property is false or not specified - LDAP is used as
protocol
Review Comment:
The JavaDoc comment has a typo in line 171: "ldap_use_ssl_property" should
be "ldap_use_ssl". The extra "_property" suffix is incorrect and inconsistent
with the actual field name.
```suggestion
* If ldap_use_ssl is false or not specified - LDAP is used as protocol
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
