raghav-reglobe commented on code in PR #66115:
URL: https://github.com/apache/doris/pull/66115#discussion_r3967138937
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java:
##########
@@ -1051,21 +1074,72 @@ public void setPasswordInternal(UserIdentity userIdent,
byte[] password, UserIde
ErrorReport.reportDdlException(ErrorCode.ERR_CREDENTIALS_CONTRADICT_TO_HISTORY,
userIdent.getQualifiedUser(), userIdent.getHost());
}
+ if (retainCurrentPassword) {
+ // MySQL-compatible RETAIN CURRENT PASSWORD constraint:
+ // "If you specify RETAIN CURRENT PASSWORD for an account
+ // that has an empty primary password, the statement
+ // fails." Checked here (not only at analysis) so every
+ // caller is covered; skipped on replay (the journal
+ // already passed this check). NB an EMPTY NEW password is
+ // NOT an error — it empties the secondary as well (MySQL
+ // semantics, handled in UserManager.setPassword).
+ User user = userManager.getUserByUserIdentity(userIdent);
+ if (user == null || user.getPassword() == null
+ || user.getPassword().getPassword() == null
+ || user.getPassword().getPassword().length == 0) {
+ throw new DdlException(
+ "Current password cannot be retained for user
" + userIdent
+ + " because it does not exist or is
empty");
+ }
+ }
}
- userManager.setPassword(userIdent, password, errOnNonExist);
+ userManager.setPassword(userIdent, password, errOnNonExist,
retainCurrentPassword);
if (password != null) {
// save password to password history
passwdPolicyManager.updatePassword(userIdent, password);
}
if (!isReplay) {
- PrivInfo info = new PrivInfo(userIdent, null, password, null,
null);
+ PrivInfo info = new PrivInfo(userIdent, null, password, null,
null, retainCurrentPassword, false);
+ Env.getCurrentEnv().getEditLog().logSetPassword(info);
+ }
+ } finally {
+ writeUnlock();
+ }
+ LOG.info("finished to set password for {}. is replay: {}, retain
current: {}",
+ userIdent, isReplay, retainCurrentPassword);
+ }
+
+ /**
+ * MySQL-compatible "ALTER USER ... DISCARD OLD PASSWORD": drop the
+ * retained secondary password.
+ *
+ * <p>Journaled via OP_SET_PASSWORD/PrivInfo with the discard flag and
+ * passwd = the account's CURRENT primary password, NOT via a new
+ * operation value on OP_ALTER_USER: an FE binary without this feature
+ * deserializes an unknown AlterUserOpType enum name as null and fails
+ * replay, whereas it replays this entry as a plain set-password to the
+ * value the account already holds — a harmless no-op. That keeps the
+ * journal readable by pre-feature binaries.
+ */
+ private void discardOldPasswordInternal(UserIdentity userIdent, boolean
isReplay) throws DdlException {
+ writeLock();
+ try {
+ User user = userManager.getUserByUserIdentity(userIdent);
+ if (user == null) {
+ throw new DdlException("user " + userIdent + " does not
exist");
+ }
+ userManager.discardOldPassword(userIdent, true);
+ if (!isReplay) {
+ byte[] currentPrimary = user.getPassword() == null ||
user.getPassword().getPassword() == null
+ ? new byte[0] : user.getPassword().getPassword();
+ PrivInfo info = new PrivInfo(userIdent, null, currentPrimary,
null, null, false, true);
Review Comment:
Good catch, thanks. You're right: the primary value survived that replay,
but a pre-feature `setPasswordInternal` still runs
`passwdPolicyManager.updatePassword`, which appends the primary to the history
again and refreshes the creation time.
Fixed in 1a08a3c3d0 (the branch is also rebased onto current master, and the
test class moved to JUnit 5 with the rest of the tree). DISCARD now journals an
`OP_ALTER_USER` entry whose carrier op is `SET_PASSWORD_POLICY` with every
option UNSET, plus a `discardOldPasswd` marker on `AlterUserOperationLog`:
- a pre-feature binary drops the unknown field and replays a policy update
that changes nothing (`ExpirePolicy.update(UNSET)`, `HistoryPolicy.update(null,
UNSET)` and both `FailedLoginPolicy` setters return early, and nothing is
journaled on replay);
- a current binary reads the marker and drops the secondary password. The
policy is not touched on this side either, since DISCARD is not a password
change.
A new `AlterUserOpType` name is still not journaled (an unknown enum name
deserializes as null and replay fails), so this keeps the same
readable-by-older-binaries property, now without the side effect.
`PrivInfo.discardPasswd` is gone; `retainPasswd` stays for RETAIN.
Tests:
- `DualPasswordTest.testDiscardReplayLeavesPasswordPolicyUntouched` replays
the entry the way a pre-feature binary sees it (marker dropped, same
`alterUserInternal` path) and the way a current one does, asserting the history
size and `passwordCreateTime` are unchanged in both cases, with the old
`OP_SET_PASSWORD` carrier as the contrast that does change them.
- `testDiscardJournalsAsNoOpPolicyEntry` pins the journal shape and its GSON
round trip.
- New regression suite `account_p0/test_dual_password` covers rotation (both
statement forms), eviction, a plain change leaving the secondary alone,
discard, the empty-password rules, the privilege gate on the clause, the
password-history and expiration interplay, `SHOW CREATE USER`, and
`DISCARD`/`OLD` staying usable as identifiers. Run green against an FE+BE built
from the branch.
--
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]
