Copilot/fix admin role assignment #207
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,6 +164,10 @@ public void updatePermission(Long id, RolePermissionUpdateReq req) { | |
| public void assignToUsers(Long id, List<Long> userIds) { | ||
| RoleDO role = super.getById(id); | ||
| CheckUtils.throwIf(Boolean.TRUE.equals(role.getIsSystem()), "[{}] 是系统内置角色，不允许分配角色给其他用户", role.getName()); | ||
| // 防止将系统内置用户分配给非超级管理员角色 | ||
| if (!SystemConstants.SUPER_ADMIN_ROLE_ID.equals(id)) { | ||
| userRoleService.checkSystemUserAssignment(userIds); | ||
| } | ||
|
Comment on lines
+167
to
+170
There was a problem hiding this comment. There appears to be an inconsistency in the business logic. The assignRolesToUser method (line 94) prevents assigning the super admin role to ANY user with the check "不允许分配超级管理员角色" (Do not allow assigning super admin role). However, the new code in RoleServiceImpl.assignToUsers (lines 168-170) explicitly allows assigning system users to the super admin role by skipping the checkSystemUserAssignment validation when the role is super admin. This means:
Consider reviewing whether these two code paths should have consistent behavior, or if the difference is intentional and should be documented. |
||
| // 保存用户和角色关联 | ||
| userRoleService.assignRoleToUsers(id, userIds); | ||
| // 更新用户上下文 | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,6 +120,8 @@ public boolean assignRoleToUsers(Long roleId, List<Long> userIds) { | |
|
|
||
| @Override | ||
| public void deleteByIds(List<Long> ids) { | ||
| // 检查是否包含系统内置用户的角色关联 | ||
| this.checkSystemUserUnassignment(ids); | ||
| baseMapper.deleteByIds(ids); | ||
| } | ||
|
|
||
|
|
@@ -165,4 +167,42 @@ public boolean isRoleIdExists(List<Long> roleIds) { | |
| } | ||
| return baseMapper.lambdaQuery().in(UserRoleDO::getRoleId, roleIds).exists(); | ||
| } | ||
|
|
||
| @Override | ||
| public void checkSystemUserAssignment(List<Long> userIds) { | ||
| if (CollUtil.isEmpty(userIds)) { | ||
| return; | ||
| } | ||
| // 查询用户列表中是否包含系统内置用户 | ||
| List<UserDO> systemUsers = userService.lambdaQuery() | ||
| .select(UserDO::getId, UserDO::getNickname) | ||
| .in(UserDO::getId, userIds) | ||
| .eq(UserDO::getIsSystem, true) | ||
| .list(); | ||
| CheckUtils.throwIfNotEmpty(systemUsers, "[{}] 是系统内置用户，不允许分配给非超级管理员角色", systemUsers.get(0).getNickname()); | ||
| } | ||
|
Comment on lines
+170
to
+183
There was a problem hiding this comment.
Java 方法参数是即时求值的。第 182 行 🐛 建议修复：仅在非空时获取昵称- CheckUtils.throwIfNotEmpty(systemUsers, "[{}] 是系统内置用户，不允许分配给非超级管理员角色", systemUsers.get(0).getNickname());
+ if (CollUtil.isNotEmpty(systemUsers)) {
+ CheckUtils.throwIfNotEmpty(systemUsers, "[{}] 是系统内置用户，不允许分配给非超级管理员角色", systemUsers.get(0).getNickname());
+ }或者更简洁地： - CheckUtils.throwIfNotEmpty(systemUsers, "[{}] 是系统内置用户，不允许分配给非超级管理员角色", systemUsers.get(0).getNickname());
+ CheckUtils.throwIfNotEmpty(systemUsers, "[{}] 是系统内置用户，不允许分配给非超级管理员角色",
+ CollUtil.isEmpty(systemUsers) ? "" : systemUsers.get(0).getNickname());🤖 Prompt for AI Agents |
||
|
|
||
| @Override | ||
| public void checkSystemUserUnassignment(List<Long> userRoleIds) { | ||
| if (CollUtil.isEmpty(userRoleIds)) { | ||
| return; | ||
| } | ||
| // 查询用户角色关联列表 | ||
| List<UserRoleDO> userRoleList = baseMapper.lambdaQuery() | ||
| .select(UserRoleDO::getUserId) | ||
| .in(UserRoleDO::getId, userRoleIds) | ||
| .list(); | ||
| if (CollUtil.isEmpty(userRoleList)) { | ||
| return; | ||
| } | ||
| // 获取用户ID列表 | ||
| List<Long> userIds = userRoleList.stream().map(UserRoleDO::getUserId).distinct().toList(); | ||
| // 查询是否包含系统内置用户 | ||
| List<UserDO> systemUsers = userService.lambdaQuery() | ||
| .select(UserDO::getId, UserDO::getNickname) | ||
| .in(UserDO::getId, userIds) | ||
| .eq(UserDO::getIsSystem, true) | ||
| .list(); | ||
| CheckUtils.throwIfNotEmpty(systemUsers, "[{}] 是系统内置用户，不允许取消分配角色", systemUsers.get(0).getNickname()); | ||
| } | ||
|
Comment on lines
+185
to
+207
There was a problem hiding this comment. 同样的 第 206 行与 🐛 建议修复- CheckUtils.throwIfNotEmpty(systemUsers, "[{}] 是系统内置用户，不允许取消分配角色", systemUsers.get(0).getNickname());
+ if (CollUtil.isNotEmpty(systemUsers)) {
+ CheckUtils.throwIfNotEmpty(systemUsers, "[{}] 是系统内置用户，不允许取消分配角色", systemUsers.get(0).getNickname());
+ }🤖 Prompt for AI Agents
Comment on lines
+186
to
+207
There was a problem hiding this comment. The method prevents system built-in users from having ANY role unassigned, including super admin roles. This is inconsistent with the assignment logic in RoleServiceImpl.assignToUsers (lines 168-170) which explicitly allows system users to be assigned to super admin roles. Consider adding similar logic here to allow unassignment of super admin role associations for system users, while preventing unassignment of other roles. You could check the role IDs associated with the user-role records and only throw an exception if non-super-admin roles are being unassigned. |
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for these methods should clarify their specific purpose regarding super admin roles. The current descriptions don't mention that checkSystemUserAssignment is intended to prevent assignment to non-super-admin roles, and checkSystemUserUnassignment should (but currently doesn't) allow unassignment of super admin roles. Consider updating the documentation to be more specific about the relationship with super admin roles.