github-actions[bot] commented on code in PR #61440:
URL: https://github.com/apache/doris/pull/61440#discussion_r3601057982
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/ldap/LdapAuthenticator.java:
##########
@@ -107,12 +107,17 @@ private AuthenticateResponse internalAuthenticate(String
password, String qualif
}
// check user password by ldap server.
+ // extra check for login with empty LDAP password was added in
checkUserPasswd.
try {
if
(!Env.getCurrentEnv().getAuth().getLdapManager().checkUserPasswd(qualifiedUser,
password)) {
if (LOG.isDebugEnabled()) {
LOG.debug("internalAuthenticate: user={}, success=false",
userName);
}
- ErrorReport.report(ErrorCode.ERR_ACCESS_DENIED_ERROR,
qualifiedUser, remoteIp, usePasswd);
Review Comment:
**[P2] Report the policy error only when the policy rejected the login**
`checkUserPasswd` returns `false` for the null precondition, missing users,
LDAP bind rejection, and operational LDAP failures as well as for the new
policy gate. This branch looks only at the supplied password, so with the
default `ldap_allow_empty_pass=true` an LDAP server that rejects an empty bind
still produces error 6001 saying empty passwords are prohibited and advising
the operator to enable an already-enabled flag. `Auth.checkPlainPassword` has
the same branch (and Arrow Flight explicitly converts its empty-password
sentinel to `null`). Please preserve a typed failure reason, or at minimum
require `!LdapConfig.ldap_allow_empty_pass` before emitting
`ERR_EMPTY_PASSWORD`; ordinary failures should retain their existing error.
##########
fe/fe-core/src/test/java/org/apache/doris/mysql/authenticate/ldap/LdapManagerTest.java:
##########
@@ -108,6 +115,32 @@ public void testCheckUserPasswd() {
Assert.assertFalse(ldapManager.checkUserPasswd(USER2, "123"));
}
+ @Test
+ public void testUserLoginWithEmptyLDAPPasswordDefault() {
Review Comment:
**[P1] Exercise the production empty-password chokepoint**
These new tests call the helper directly, so they still pass if its
invocation is deleted from `LdapManager.checkUserPasswd` or moved after the
cached-password/LDAP-bind success paths. The existing manager test uses only
nonempty passwords, `LdapAuthenticatorTest` mocks `checkUserPasswd`, and no
`Auth.checkPlainPassword` test is changed despite the PR description claiming
both caller suites. Please test an empty credential through `checkUserPasswd`
with cached/client success under both flag values, assert the disabled case
returns before those success paths, and cover both callers. Those caller
assertions should distinguish a flag-disabled policy rejection from a
flag-enabled empty-bind failure and keep LDAP-present/nonempty failures
message-neutral with LDAP-absent/native failures.
--
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]
