Copilot/fix admin role assignment#207
Conversation
…in roles Co-authored-by: gopkg-dev <58848833+gopkg-dev@users.noreply.github.com>
…ents Co-authored-by: gopkg-dev <58848833+gopkg-dev@users.noreply.github.com>
|
|
📝 Walkthrough概览本次更改添加了验证逻辑，以防止对系统内置用户进行角色分配和取消分配操作。在UserRoleService接口中引入两个新的检查方法，并在UserRoleServiceImpl中实现。这些检查在RoleServiceImpl和UserRoleServiceImpl中调用以强制执行限制。同时进行了轻微的格式调整。 变更
序列图sequenceDiagram
participant Client
participant RoleServiceImpl
participant UserRoleService
participant UserRoleServiceImpl
participant DB
Client->>RoleServiceImpl: assignToUsers(roleId, userIds)
RoleServiceImpl->>RoleServiceImpl: 检查角色是否为超级管理员
alt 角色非超级管理员
RoleServiceImpl->>UserRoleService: checkSystemUserAssignment(userIds)
UserRoleService->>UserRoleServiceImpl: checkSystemUserAssignment(userIds)
UserRoleServiceImpl->>DB: 查询userIds中的系统内置用户
DB-->>UserRoleServiceImpl: 返回系统用户列表
alt 存在系统用户
UserRoleServiceImpl-->>RoleServiceImpl: 抛出验证异常
RoleServiceImpl-->>Client: 操作失败
else 无系统用户
UserRoleServiceImpl-->>RoleServiceImpl: 验证通过
RoleServiceImpl->>UserRoleServiceImpl: assignRoleToUsers(roleId, userIds)
UserRoleServiceImpl->>DB: 保存角色分配
DB-->>UserRoleServiceImpl: 成功
UserRoleServiceImpl-->>RoleServiceImpl: 返回
RoleServiceImpl-->>Client: 分配完成
end
else 角色为超级管理员
RoleServiceImpl->>UserRoleServiceImpl: assignRoleToUsers(roleId, userIds)
UserRoleServiceImpl->>DB: 保存角色分配
DB-->>UserRoleServiceImpl: 成功
UserRoleServiceImpl-->>RoleServiceImpl: 返回
RoleServiceImpl-->>Client: 分配完成
end
代码审查工作量评估🎯 3 (中等) | ⏱️ ~20 分钟 诗歌
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@continew-system/src/main/java/top/continew/admin/system/service/impl/UserRoleServiceImpl.java`:
- Around line 170-183: In checkSystemUserAssignment, calling
systemUsers.get(0).getNickname() inside the parameter list of
CheckUtils.throwIfNotEmpty causes an IndexOutOfBoundsException when systemUsers
is empty; change the logic to call CheckUtils.throwIfNotEmpty(systemUsers,
"...") first (or check if systemUsers is not empty) and only then retrieve
systemUsers.get(0).getNickname() to include the nickname in the message—update
the method around the systemUsers query and the CheckUtils.throwIfNotEmpty
invocation accordingly so nickname is fetched only when systemUsers is
non-empty.
- Around line 185-207: The call in checkSystemUserUnassignment builds the error
message using systemUsers.get(0).getNickname(), which will throw
IndexOutOfBoundsException when systemUsers is empty because argument evaluation
happens before CheckUtils.throwIfNotEmpty; fix by computing the nickname safely
(e.g. use systemUsers.stream().findFirst().map(UserDO::getNickname).orElse(""))
or first check systemUsers.isEmpty() then call
CheckUtils.throwIfNotEmpty(systemUsers, "[{}] 是系统内置用户，不允许取消分配角色", nickname)
where nickname is computed only when systemUsers is non-empty; ensure the unsafe
systemUsers.get(0).getNickname() usage is removed.
|
|
||
| @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()); | ||
| } |
There was a problem hiding this comment.
systemUsers 为空时 get(0) 会抛出 IndexOutOfBoundsException。
Java 方法参数是即时求值的。第 182 行 systemUsers.get(0).getNickname() 会在 CheckUtils.throwIfNotEmpty 执行之前被求值。当查询结果为空列表时，get(0) 将直接抛出 IndexOutOfBoundsException，而非正常通过校验。
🐛 建议修复：仅在非空时获取昵称
- 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
In
`@continew-system/src/main/java/top/continew/admin/system/service/impl/UserRoleServiceImpl.java`
around lines 170 - 183, In checkSystemUserAssignment, calling
systemUsers.get(0).getNickname() inside the parameter list of
CheckUtils.throwIfNotEmpty causes an IndexOutOfBoundsException when systemUsers
is empty; change the logic to call CheckUtils.throwIfNotEmpty(systemUsers,
"...") first (or check if systemUsers is not empty) and only then retrieve
systemUsers.get(0).getNickname() to include the nickname in the message—update
the method around the systemUsers query and the CheckUtils.throwIfNotEmpty
invocation accordingly so nickname is fetched only when systemUsers is
non-empty.
| @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()); | ||
| } |
There was a problem hiding this comment.
同样的 IndexOutOfBoundsException 问题。
第 206 行与 checkSystemUserAssignment 存在完全相同的问题：systemUsers 为空时 get(0) 会抛异常。
🐛 建议修复
- CheckUtils.throwIfNotEmpty(systemUsers, "[{}] 是系统内置用户，不允许取消分配角色", systemUsers.get(0).getNickname());
+ if (CollUtil.isNotEmpty(systemUsers)) {
+ CheckUtils.throwIfNotEmpty(systemUsers, "[{}] 是系统内置用户，不允许取消分配角色", systemUsers.get(0).getNickname());
+ }🤖 Prompt for AI Agents
In
`@continew-system/src/main/java/top/continew/admin/system/service/impl/UserRoleServiceImpl.java`
around lines 185 - 207, The call in checkSystemUserUnassignment builds the error
message using systemUsers.get(0).getNickname(), which will throw
IndexOutOfBoundsException when systemUsers is empty because argument evaluation
happens before CheckUtils.throwIfNotEmpty; fix by computing the nickname safely
(e.g. use systemUsers.stream().findFirst().map(UserDO::getNickname).orElse(""))
or first check systemUsers.isEmpty() then call
CheckUtils.throwIfNotEmpty(systemUsers, "[{}] 是系统内置用户，不允许取消分配角色", nickname)
where nickname is computed only when systemUsers is non-empty; ensure the unsafe
systemUsers.get(0).getNickname() usage is removed.
There was a problem hiding this comment.
Pull request overview
This pull request implements safeguards to prevent system built-in users from being assigned or unassigned to/from roles inappropriately, along with code formatting improvements for better readability.
Changes:
- Added validation methods
checkSystemUserAssignmentandcheckSystemUserUnassignmentto prevent improper role assignments for system built-in users - Integrated validation into role assignment flow to allow super admin role assignment to system users while blocking other roles
- Applied code formatting improvements including javadoc list formatting, array initialization spacing, and method parameter line breaks
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| continew-system/src/main/java/top/continew/admin/system/service/UserRoleService.java | Added interface methods for checking system user assignment/unassignment |
| continew-system/src/main/java/top/continew/admin/system/service/impl/UserRoleServiceImpl.java | Implemented validation methods and integrated unassignment check into deleteByIds |
| continew-system/src/main/java/top/continew/admin/system/service/impl/RoleServiceImpl.java | Added system user assignment check that allows super admin role assignments |
| continew-system/src/main/java/top/continew/admin/system/util/FileNameGenerator.java | Formatting improvements: javadoc list spacing, array initialization spacing, line breaks for readability |
| continew-system/src/main/java/top/continew/admin/system/service/impl/MultipartUploadServiceImpl.java | Line break formatting for long method call |
| continew-system/src/main/java/top/continew/admin/system/service/impl/FileServiceImpl.java | Line break formatting for long method call |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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()); | ||
| } |
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.
| * 检查系统内置用户是否在用户列表中，如果存在则抛出异常 | ||
| * | ||
| * @param userIds 用户 ID 列表 | ||
| */ | ||
| void checkSystemUserAssignment(List<Long> userIds); | ||
|
|
||
| /** | ||
| * 检查系统内置用户是否在用户角色关联列表中，如果存在则抛出异常 | ||
| * | ||
| * @param userRoleIds 用户角色关联 ID 列表 |
There was a problem hiding this comment.
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.
| * 检查系统内置用户是否在用户列表中，如果存在则抛出异常 | |
| * | |
| * @param userIds 用户 ID 列表 | |
| */ | |
| void checkSystemUserAssignment(List<Long> userIds); | |
| /** | |
| * 检查系统内置用户是否在用户角色关联列表中，如果存在则抛出异常 | |
| * | |
| * @param userRoleIds 用户角色关联 ID 列表 | |
| * 检查系统内置用户（超级管理员）在分配角色时是否包含在用户列表中。 | |
| * <p> | |
| * 用于防止给超级管理员分配非超级管理员角色或进行不符合约束的角色分配， | |
| * 如果检测到系统内置用户出现在不允许的分配场景中，则抛出异常。 | |
| * | |
| * @param userIds 用户 ID 列表（用于角色分配的目标用户） | |
| */ | |
| void checkSystemUserAssignment(List<Long> userIds); | |
| /** | |
| * 检查系统内置用户（超级管理员）在解除角色分配（角色解绑）时是否包含在用户角色关联列表中。 | |
| * <p> | |
| * 该方法的目的在于在遵守超级管理员相关约束的前提下，允许对超级管理员角色进行解绑操作； | |
| * 如果检测到本次解绑操作会违反对超级管理员的保护规则，则抛出异常。 | |
| * | |
| * @param userRoleIds 用户角色关联 ID 列表（待解绑的用户角色关系） |
| // 防止将系统内置用户分配给非超级管理员角色 | ||
| if (!SystemConstants.SUPER_ADMIN_ROLE_ID.equals(id)) { | ||
| userRoleService.checkSystemUserAssignment(userIds); | ||
| } |
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:
- Assigning users to super admin role via RoleServiceImpl.assignToUsers: Allowed for system users
- Assigning super admin role to users via UserRoleServiceImpl.assignRolesToUser: Blocked for all users
Consider reviewing whether these two code paths should have consistent behavior, or if the difference is intentional and should be documented.
PR 类型
PR 目的
This pull request introduces important enhancements to user-role management in the system, specifically adding safeguards to prevent system built-in users from being assigned or unassigned roles inappropriately. Additionally, it includes minor code formatting improvements for better readability.
User-role assignment and unassignment safeguards:
checkSystemUserAssignmentandcheckSystemUserUnassignmentmethods to theUserRoleServiceinterface and implemented them inUserRoleServiceImpl, ensuring that system built-in users cannot be assigned to non-super admin roles or have their role assignments removed. Exceptions are thrown if such actions are attempted. [1] [2] [3]RoleServiceImpl.assignToUsersto callcheckSystemUserAssignmentwhen assigning users to roles other than super admin, preventing improper assignment of system users.Code formatting improvements:
FileNameGenerator.generateUniqueName,FileNameGenerator.parseFileName, and method calls in file upload services. [1] [2] [3] [4] [5]解决方案
PR 测试
Changelog
其他信息
提交前确认
Summary by CodeRabbit
发版说明