morningman commented on PR #61440:
URL: https://github.com/apache/doris/pull/61440#issuecomment-4636863827
## 1. Clean up the log statement in `Auth.java` (high priority)
The new log line prints the password variable and reads like leftover
debugging code:
```java
LOG.info("empty pass branch was activated: for user {}, pass {}, mode {}",
remoteUser, remotePasswd, LdapConfig.ldap_allow_empty_pass);
```
Problems:
- **It logs the password field (`remotePasswd`).** The branch condition
guarantees the password is empty/`null` here, so no real secret leaks — but
logging a `password` variable at all is a bad pattern that shouldn't be
introduced. Drop `pass {}` entirely.
- **The wording looks like debug residue** ("empty pass branch was
activated", "mode {}") rather than an operator-facing production log.
- **The first letter is not capitalized**, which the logging conventions
require.
- It should mirror the cleaner message already used in `LdapAuthenticator`.
Suggested change:
```diff
- LOG.info("empty pass branch was activated: for user {}, pass {}, mode {}",
- remoteUser, remotePasswd, LdapConfig.ldap_allow_empty_pass);
+ LOG.info("User {}@{} login rejected: empty LDAP password is prohibited
(ldap_allow_empty_pass=false)",
+ remoteUser, remoteHost);
```
(For reference, the `LdapAuthenticator` message is fine except its first
letter `user` should also be capitalized.)
## 2. Remove the leftover `System.out.println` in the test (high priority)
`PlainAuthWithEmptyPasswordAndLdapTest.tearDown` contains a debug print:
```java
System.out.println("4.0 [" + LdapConfig.ldap_allow_empty_pass + "]");
```
This should be deleted. The `4.0` literal is also misleading, since the PR
targets `master` (4.1).
## 3. Use SQLSTATE `28000` for consistency (medium priority)
`ERR_EMPTY_PASSWORD` is an authentication denial, but it uses SQLSTATE
`42000` (syntax error / access-rule violation). The other auth-denial codes in
the codebase (`ERR_ACCESS_DENIED_ERROR`, `ERR_ACCESS_DENIED_NO_PASSWORD_ERROR`)
consistently use `28000` (invalid authorization specification). Align with them:
```diff
- ERR_EMPTY_PASSWORD(6001, new byte[]{'4', '2', '0', '0', '0'},
+ ERR_EMPTY_PASSWORD(6001, new byte[]{'2', '8', '0', '0', '0'},
"Access with empty password is prohibited for LDAP user '%s'.
...");
```
--
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]
