diff --git a/doc/code-review-report.md b/doc/code-review-report.md new file mode 100644 index 0000000..26c2c1a --- /dev/null +++ b/doc/code-review-report.md @@ -0,0 +1,218 @@ +# 考试管理系统代码审查报告 + +## 📊 总体评估 +- 代码质量评分:6.5/10 +- 安全风险等级:高 +- 主要问题数量:12个 + +## 🔴 严重问题(必须修复) + +### 问题1:密码明文存储,存在严重安全风险 +- **位置**:com/system/realm/LoginRealm.java:81-83 +- **描述**:系统中所有密码均以明文形式存储和比较,包括LoginRealm中的密码验证和RestPasswordController中的密码重置功能。 +- **风险**:一旦数据库泄露,所有用户密码直接暴露。攻击者获取数据库访问权限后可直接获取所有账户密码。 +- **修复建议**:使用BCrypt或SHA-256等强哈希算法加密密码,示例: +```java +// 添加Shiro密码匹配器配置 +@Bean +public HashedCredentialsMatcher hashedCredentialsMatcher() { + HashedCredentialsMatcher matcher = new HashedCredentialsMatcher(); + matcher.setHashAlgorithmName("SHA-256"); + matcher.setHashIterations(1024); + return matcher; +} + +// 注册时加密密码 +String encryptedPassword = new SimpleHash("SHA-256", password, salt, 1024).toString(); +``` + +### 问题2:Shiro URL权限配置拼写错误导致教师权限拦截失效 +- **位置**:spring/applicationContext-shiro.xml:34 +- **描述**:配置中误将`/teacher/**`写为`/techer/**`,导致教师路径的权限拦截完全失效。 +- **风险**:未认证或未授权用户可直接访问教师模块所有功能,造成越权访问。 +- **修复建议**:修正拼写错误: +```xml +/teacher/** = authc, roles[teacher] +``` + +### 问题3:课程删除约束未正确处理,用户无法感知删除失败 +- **位置**:com/system/controller/AdminController.java:347-356 +- **描述**:CourseServiceImpl.removeById方法在有学生选课时返回false,但AdminController未检查该返回值,直接进行重定向,用户无法知道课程删除是否成功。 +- **风险**:用户体验极差,可能造成数据不一致或误解。 +- **修复建议**:在Controller中检查返回值并给出相应提示: +```java +public String removeCourse(Integer id, Model model) throws Exception { + Boolean result = courseService.removeById(id); + if (!result) { + model.addAttribute("message", "该课程已有学生选课,无法删除"); + return "error"; + } + return "redirect:/admin/showCourse"; +} +``` + +### 问题4:教师打分无防重复机制,可重复覆盖成绩 +- **位置**:com/system/controller/TeacherController.java:72-78 +- **描述**:教师打分功能直接更新分数,未检查是否已打过分数,也未限制打分权限。 +- **风险**:教师可随意修改已打分数,造成成绩管理混乱;也可能被恶意利用重复提交覆盖成绩。 +- **修复建议**:在Service层添加分数重复检查: +```java +public void updataOne(SelectedCourseCustom selectedCourseCustom) throws Exception { + SelectedCourseCustom existing = findOne(selectedCourseCustom); + if (existing != null && existing.getMark() != null) { + throw new CustomException("该学生已打分,如需修改请联系管理员"); + } + selectedcourseMapper.updateByExample(selectedCourseCustom, example); +} +``` + +### 问题5:异常处理不规范,直接打印栈追踪信息 +- **位置**:com/system/realm/LoginRealm.java:48,75 +- **描述**:多处使用e.printStackTrace()处理异常,未记录到日志系统,生产环境无法追踪问题。 +- **风险**:生产环境故障无法定位,问题难以排查。 +- **修复建议**:使用SLF4J记录日志: +```java +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +private static final Logger logger = LoggerFactory.getLogger(LoginRealm.class); + +// 替换e.printStackTrace() +logger.error("查询用户失败", e); +``` + +### 问题6:管理员密码重置功能无权限校验,存在越权风险 +- **位置**:com/system/controller/AdminController.java:377-393 +- **描述**:密码重置功能仅检查了用户角色,但未验证当前用户是否为管理员,理论上存在越权访问风险。 +- **风险**:若Shiro配置有漏洞,普通用户可能重置其他用户密码。 +- **修复建议**:添加编程式权限校验: +```java +Subject subject = SecurityUtils.getSubject(); +if (!subject.hasRole("admin")) { + throw new CustomException("无权限进行此操作"); +} +``` + +## 🟡 中等问题(建议修复) + +### 问题1:事务配置不完整,select方法使用REQUIRED而非SUPPORTS +- **位置**:spring/applicationContext-trsaction.xml:36 +- **描述**:事务通知中select*方法使用了REQUIRED传播行为,查询方法应使用SUPPORTS以提高性能。 +- **建议**:修改事务配置: +```xml + +``` + +### 问题2:用户登录查询在无结果时抛出IndexOutOfBoundsException +- **位置**:com/system/service/impl/UserloginServiceImpl.java:30 +- **描述**:当查询不到用户时直接调用list.get(0)会抛出数组越界异常,而非优雅处理。 +- **建议**:添加空列表检查: +```java +if (list.isEmpty()) { + return null; +} +return list.get(0); +``` + +### 问题3:教师删除时抛出异常但Controller未捕获处理 +- **位置**:com/system/service/impl/TeacherServiceImpl.java:47 +- **描述**:removeById方法在教师有课程时抛出CustomException,但AdminController中未捕获该异常。 +- **建议**:在Controller中添加try-catch处理: +```java +try { + teacherService.removeById(id); + userloginService.removeByName(id.toString()); +} catch (CustomException e) { + model.addAttribute("message", e.getMessage()); + return "error"; +} +``` + +### 问题4:自定义分页逻辑偏移量计算错误 +- **位置**:mapper/CourseMapperCustom.xml:10, StudentMapperCustom.xml:46 +- **描述**:LIMIT参数直接使用toPageNo作为偏移量,应改为(toPageNo - 1) * pageSize。 +- **建议**:在Service层计算正确的偏移量: +```java +// PagingVO中添加 +public int getOffset() { + return (toPageNo - 1) * pageSize; +} +``` + +### 问题5:学生和教师删除时未处理关联的选课记录 +- **位置**:com/system/controller/AdminController.java:134-135, 244-245 +- **描述**:删除学生和教师时未清理关联的选课记录,可能造成数据不一致。 +- **建议**:删除前先清理关联数据: +```java +// 删除学生前先删除选课记录 +SelectedcourseExample example = new SelectedcourseExample(); +example.createCriteria().andStudentidEqualTo(id); +selectedcourseMapper.deleteByExample(example); +``` + +### 问题6:CourseCustom类拷贝时BeanUtils参数顺序错误 +- **位置**:com/system/service/impl/CourseServiceImpl.java:85, 134 +- **描述**:使用Apache Commons BeanUtils时参数顺序错误(应为目标对象在前)。 +- **建议**:统一使用Spring的BeanUtils,确保参数顺序正确: +```java +// 改为 +BeanUtils.copyProperties(c, courseCustom); +``` + +## 🟢 优化建议 + +### 建议1:使用DTO/VO模式分离持久层对象和视图对象 +- **描述**:目前直接使用PO对象在各层传递,建议引入DTO(数据传输对象)和VO(视图对象)模式,分离各层数据模型。 +- **收益**:解耦内部数据模型和外部接口,提高灵活性,便于维护。 + +### 建议2:统一异常处理和错误码定义 +- **描述**:建议定义统一的错误码枚举和异常体系,而非依赖字符串消息。 +- **收益**:错误处理更规范,便于国际化和前端统一处理。 + +### 建议3:添加方法级别的权限注解 +- **描述**:除了URL级别的Shiro拦截,建议在Controller方法上添加@RequiresRoles等注解。 +- **收益**:双重安全保障,配置更加灵活。 + +### 建议4:使用分页插件替代手动分页 +- **描述**:建议使用MyBatis PageHelper等成熟分页插件替代手动编写分页SQL。 +- **收益**:减少重复代码,支持多种数据库,分页逻辑更可靠。 + +### 建议5:添加输入参数校验 +- **描述**:建议使用JSR-303注解(如@NotNull, @Size等)对Controller输入参数进行校验。 +- **收益**:提前拦截非法参数,减少业务层校验代码。 + +## ✅ 优秀实践 + +1. **分层架构清晰**:Controller/Service/Mapper/PO分层明确,职责分离基本合理。 +2. **统一异常处理**:通过CustomExceptionResolver实现了全局异常处理,架构设计合理。 +3. **Shiro权限框架集成**:正确集成了Shiro安全框架,实现了基本的认证和授权功能。 +4. **事务管理配置**:基于AOP的事务声明式配置,符合Spring最佳实践。 +5. **MyBatis整合规范**:Mapper接口与XML对应,使用Example查询避免硬编码SQL。 +6. **自定义类型转换器**:实现了CustomDateConverter日期转换器,符合Spring MVC最佳实践。 + +## 📋 修复优先级清单 + +1. **[P0] 严重安全问题**: + - 密码明文存储问题(问题1) + - Shiro权限配置拼写错误(问题2) + +2. **[P1] 功能缺陷**: + - 课程删除约束未正确处理(问题3) + - 教师打分防重复机制缺失(问题4) + - 用户查询空列表异常(中等问题2) + - 教师删除异常未处理(中等问题3) + - 分页偏移量计算错误(中等问题4) + +3. **[P2] 代码质量优化**: + - 异常处理规范化(问题5) + - 事务配置优化(中等问题1) + - 统一日志框架使用(问题5) + - BeanUtils参数顺序修复(中等问题6) + +4. **[P3] 架构和设计改进**: + - 关联数据完整性处理(中等问题5) + - DTO/VO模式引入(优化建议1) + - 统一错误码定义(优化建议2) + - 方法级权限注解(优化建议3) + - 分页插件使用(优化建议4) + - 参数校验注解(优化建议5)