Copilot commented on code in PR #16512:
URL: https://github.com/apache/iotdb/pull/16512#discussion_r2385296325
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/user/BasicUserManager.java:
##########
@@ -229,6 +229,30 @@ public boolean updateUserPassword(String username, String
newPassword, boolean b
}
}
+ public void renameUser(String username, String newUsername) throws
AuthException {
+ User user = this.getEntity(username);
+ if (user == null) {
+ throw new AuthException(
+ getEntityNotExistErrorCode(), String.format(getNoSuchEntityError(),
username));
+ }
+ User tmpUser = this.getEntity(newUsername);
+ if (tmpUser != null) {
+ throw new AuthException(
+ TSStatusCode.USER_ALREADY_EXIST,
+ String.format(
+ "Cannot rename user %s to %s, because the target username is
already existed.",
+ username, newUsername));
+ }
+ lock.writeLock(username);
+ try {
+ User newUser = (User) entityMap.remove(username);
+ newUser.setName(newUsername);
+ entityMap.put(newUsername, newUser);
+ } finally {
+ lock.writeUnlock(username);
Review Comment:
The lock is acquired on the old username, but the user is moved to a new key
in the entityMap. This creates a potential race condition where another thread
could access the user under the new username before the lock is released.
Consider acquiring locks on both usernames or using a global lock for rename
operations.
```suggestion
// Acquire locks on both usernames in lexicographical order to avoid
deadlocks
String firstLock, secondLock;
if (username.compareTo(newUsername) < 0) {
firstLock = username;
secondLock = newUsername;
} else {
firstLock = newUsername;
secondLock = username;
}
lock.writeLock(firstLock);
lock.writeLock(secondLock);
try {
User newUser = (User) entityMap.remove(username);
newUser.setName(newUsername);
entityMap.put(newUsername, newUser);
} finally {
lock.writeUnlock(secondLock);
lock.writeUnlock(firstLock);
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/security/AccessControlImpl.java:
##########
@@ -262,6 +261,18 @@ public void checkUserCanRunRelationalAuthorStatement(
}
authChecker.checkGlobalPrivilege(userName,
TableModelPrivilege.MANAGE_USER, auditEntity);
return;
+ case RENAME_USER:
+ case UPDATE_USER:
+ auditEntity.setAuditLogOperation(AuditLogOperation.DDL);
+ // users can change the username and password of themselves
+ // the superuser can affect anyone
Review Comment:
The comment mentions 'username and password' but this code block handles
both RENAME_USER and UPDATE_USER cases. For clarity, the comment should specify
that users can change their own username (RENAME_USER) and password
(UPDATE_USER).
```suggestion
// users can change their own username (RENAME_USER) and password
(UPDATE_USER)
// the superuser can change the username or password of any user
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/security/TreeAccessCheckVisitor.java:
##########
@@ -517,18 +517,18 @@ public TSStatus visitAuthor(AuthorStatement statement,
TreeAccessCheckContext co
PrivilegeType.MANAGE_USER,
statement::getUserName);
case UPDATE_USER:
- // users can change passwords of themselves
+ case RENAME_USER:
+ context.setAuditLogOperation(AuditLogOperation.DDL);
+ // users can change the username and password of themselves
if (statement.getUserName().equals(context.getUsername())) {
+ recordObjectAuthenticationAuditLog(context.setResult(true),
context::getUsername);
return RpcUtils.SUCCESS_STATUS;
}
Review Comment:
The comment mentions 'username and password' but the RENAME_USER case only
handles username changes, not password changes. The comment should read 'users
can change the username of themselves' to be accurate.
--
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]
