CalvinKirs commented on code in PR #66115:
URL: https://github.com/apache/doris/pull/66115#discussion_r3663441551
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/UserManager.java:
##########
@@ -351,14 +376,59 @@ public Map<String, List<User>> getNameToUsers() {
}
public void setPassword(UserIdentity userIdentity, byte[] password,
boolean errOnNonExist) throws DdlException {
+ setPassword(userIdentity, password, errOnNonExist, false);
+ }
+
+ /**
+ * Set the user's password, with MySQL-compatible dual password semantics:
+ * with {@code retainCurrent} ("RETAIN CURRENT PASSWORD") the previous
+ * primary password becomes the secondary password and remains valid for
+ * authentication; without it an existing secondary password remains
+ * UNCHANGED (MySQL: "If an account has a secondary password and you
+ * change its primary password without specifying RETAIN CURRENT PASSWORD,
+ * the secondary password remains unchanged."). Setting an EMPTY password
+ * empties the secondary password as well, even with retain (also MySQL).
+ */
+ public void setPassword(UserIdentity userIdentity, byte[] password,
boolean errOnNonExist,
+ boolean retainCurrent) throws DdlException {
User user = getUserByUserIdentity(userIdentity);
if (user == null) {
if (errOnNonExist) {
throw new DdlException("user " + userIdentity + " does not
exist");
}
return;
}
+ Password oldPassword = user.getPassword();
+ byte[] carried;
+ if (password == null || password.length == 0) {
+ // an empty new password empties the secondary as well, even with
+ // RETAIN CURRENT PASSWORD (MySQL semantics)
+ carried = null;
+ } else if (retainCurrent) {
+ carried = oldPassword == null ? null : oldPassword.getPassword();
+ } else {
+ carried = oldPassword == null ? null :
oldPassword.getSecondaryPassword();
+ }
user.setPassword(password);
+ user.getPassword().setSecondaryPassword(carried);
Review Comment:
`user.setPassword(password)` allocates a fresh `Password`, and only the next
line fills in the secondary. In between there's a window where the account has
the new primary and no secondary, so a client still using the old password gets
rejected — during exactly the rotation this feature exists to make seamless.
`matchUserPassword` also calls `user.getPassword()` twice, and the compare
loop in `checkPasswordInternal` runs after `rlock` is released, so those two
reads can land on different `Password` objects.
Cheap fix, `User` already has a `setPassword(Password)` overload:
```java
Password newPwd = new Password(password);
newPwd.setSecondaryPassword(carried);
user.setPassword(newPwd);
```
and hoisting `Password pwd = user.getPassword()` into a local in
`matchUserPassword` makes that a consistent snapshot.
--
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]
