github-actions[bot] commented on code in PR #61440:
URL: https://github.com/apache/doris/pull/61440#discussion_r3601057992
##########
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.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java:
##########
@@ -237,8 +237,14 @@ public void checkPlainPassword(String remoteUser, String
remoteHost, String remo
// Check the LDAP password when the user exists in the LDAP service.
if (ldapManager.doesUserExist(remoteUser)) {
if (!ldapManager.checkUserPasswd(remoteUser, remotePasswd,
remoteHost, currentUser)) {
- throw new
AuthenticationException(ErrorCode.ERR_ACCESS_DENIED_ERROR, remoteUser + "@" +
remoteHost,
+ if (Strings.isNullOrEmpty(remotePasswd)) {
+ // extra check for login with empty LDAP password was
added in checkUserPasswd
+ throw new
AuthenticationException(ErrorCode.ERR_EMPTY_PASSWORD, remoteUser + "@" +
remoteHost);
+ } else {
+ throw new
AuthenticationException(ErrorCode.ERR_ACCESS_DENIED_ERROR,
+ remoteUser + "@" + remoteHost + " via LDAP",
Review Comment:
**[P1] Keep failed authentication responses backend-neutral**
This branch is reached only after `doesUserExist(remoteUser)` resolved the
name in LDAP. Adding ` via LDAP` therefore makes every nonempty bad password
return a different client-visible message from a nonexistent or native user.
`BaseController` returns `formatErrMsg()` in the HTTP unauthorized response
body, and Arrow Flight includes `e.getMessage()` in its `UNAUTHENTICATED`
description, so an unauthenticated caller can enumerate LDAP-backed accounts.
Please keep the client response identical to the generic access-denied form and
put the selected backend only in server-side debug/audit logs; the LDAP-only
empty-password response needs the same neutrality.
##########
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.
--
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]
