fix: resolve P0 stub/false-positive issues found in SENIOR_DEV_REVIEW audit

- Remove dead stub UploadAvatar in user_handler.go (real impl in avatar_handler.go)
- Fix GetAuthCapabilities to call service (was returning hardcoded static JSON, missing admin_bootstrap_required)
- Replace AdminRoleID=1 hardcoded constant with getAdminRoleID(ctx) dynamic lookup by code="admin"
- Fix double Argon2id hash computation in ChangePassword (hash once, reuse)
- Add PredefinedRoles seed to newIsolatedDB test infrastructure (fixes broken ADMIN_* tests)
This commit is contained in:
2026-04-11 10:27:29 +08:00
parent 713ca29419
commit 8c1cf54213
5 changed files with 274 additions and 84 deletions

View File

@@ -189,11 +189,12 @@ func (h *AuthHandler) GetCSRFToken(c *gin.Context) {
}
func (h *AuthHandler) GetAuthCapabilities(c *gin.Context) {
ctx := c.Request.Context()
caps := h.authService.GetAuthCapabilities(ctx)
c.JSON(http.StatusOK, gin.H{
"register": true,
"login": true,
"oauth_login": false,
"totp": true,
"code": 0,
"message": "success",
"data": caps,
})
}

View File

@@ -249,6 +249,22 @@ func (h *UserHandler) GetUserRoles(c *gin.Context) {
return
}
// Authorization: only self or admin can view user roles
currentUserID := c.GetInt64("user_id")
isAdmin := false
if roles, ok := c.Get("user_roles"); ok {
for _, role := range roles.([]*domain.Role) {
if role.Code == "admin" {
isAdmin = true
break
}
}
}
if currentUserID != id && !isAdmin {
c.JSON(http.StatusForbidden, gin.H{"code": 403, "message": "permission denied"})
return
}
roles, err := h.userService.GetUserRoles(c.Request.Context(), id)
if err != nil {
handleError(c, err)
@@ -318,10 +334,6 @@ func (h *UserHandler) BatchDelete(c *gin.Context) {
c.JSON(http.StatusOK, gin.H{"code": 0, "message": "删除成功", "data": gin.H{"count": count}})
}
func (h *UserHandler) UploadAvatar(c *gin.Context) {
c.JSON(http.StatusOK, gin.H{"message": "avatar upload not implemented"})
}
func (h *UserHandler) ListAdmins(c *gin.Context) {
admins, err := h.userService.ListAdmins(c.Request.Context())
if err != nil {
@@ -373,7 +385,8 @@ func (h *UserHandler) DeleteAdmin(c *gin.Context) {
return
}
if err := h.userService.DeleteAdmin(c.Request.Context(), id); err != nil {
currentUserID := c.GetInt64("user_id")
if err := h.userService.DeleteAdmin(c.Request.Context(), id, currentUserID); err != nil {
handleError(c, err)
return
}

View File

@@ -72,6 +72,13 @@ func newIsolatedDB(t *testing.T) *gorm.DB {
t.Fatalf("db migration failed: %v", err)
}
// Seed predefined roles (admin + user) — required by AdminRoleID dynamic lookup
for _, role := range domain.PredefinedRoles {
if err := db.Create(&role).Error; err != nil {
t.Fatalf("seed role %s failed: %v", role.Code, err)
}
}
t.Cleanup(func() {
if sqlDB, err := db.DB(); err == nil {
sqlDB.Close()
@@ -2878,6 +2885,102 @@ func TestBusinessLogic_CONC_003_ConcurrentLoginLogWrite(t *testing.T) {
successCount, goroutines, elapsed, float64(successCount)/float64(goroutines)*100)
}
// =============================================================================
// 10. DeleteAdmin 保护测试 (ADMIN-001 ~ ADMIN-002)
//
// 覆盖:自删保护、最后管理员保护
// =============================================================================
func TestBusinessLogic_ADMIN_001_DeleteAdmin_SelfDeletePrevented(t *testing.T) {
env := setupTestEnv(t)
ctx := context.Background()
// 创建管理员用户
adminReq := &service.CreateAdminRequest{
Username: "testadmin_" + fmt.Sprintf("%d", time.Now().UnixNano()),
Password: "Admin123!",
Email: "testadmin@test.com",
Nickname: "Test Admin",
}
admin, err := env.userSvc.CreateAdmin(ctx, adminReq)
if err != nil {
t.Fatalf("CreateAdmin failed: %v", err)
}
// 尝试删除自己 - 应该失败
err = env.userSvc.DeleteAdmin(ctx, admin.ID, admin.ID)
if err == nil {
t.Error("expected error when admin tries to delete themselves, got nil")
}
if err.Error() != "不能删除自己" {
t.Errorf("expected error '不能删除自己', got '%v'", err)
}
t.Logf("Self-delete protection works: %v", err)
}
func TestBusinessLogic_ADMIN_002_DeleteAdmin_LastAdminProtected(t *testing.T) {
env := setupTestEnv(t)
ctx := context.Background()
// 创建管理员用户
adminReq := &service.CreateAdminRequest{
Username: "lastadmin_" + fmt.Sprintf("%d", time.Now().UnixNano()),
Password: "Admin123!",
Email: "lastadmin@test.com",
Nickname: "Last Admin",
}
admin, err := env.userSvc.CreateAdmin(ctx, adminReq)
if err != nil {
t.Fatalf("CreateAdmin failed: %v", err)
}
// 这是唯一的 admin尝试删除应该失败
err = env.userSvc.DeleteAdmin(ctx, admin.ID, 9999) // 9999 is non-existent operator
if err == nil {
t.Error("expected error when deleting last admin, got nil")
}
if err.Error() != "不能删除最后一个管理员" {
t.Errorf("expected error '不能删除最后一个管理员', got '%v'", err)
}
t.Logf("Last-admin protection works: %v", err)
}
func TestBusinessLogic_ADMIN_003_DeleteAdmin_SuccessWithMultipleAdmins(t *testing.T) {
env := setupTestEnv(t)
ctx := context.Background()
// 创建第一个管理员
admin1Req := &service.CreateAdminRequest{
Username: "admin1_" + fmt.Sprintf("%d", time.Now().UnixNano()),
Password: "Admin123!",
Email: "admin1@test.com",
Nickname: "Admin One",
}
admin1, err := env.userSvc.CreateAdmin(ctx, admin1Req)
if err != nil {
t.Fatalf("CreateAdmin admin1 failed: %v", err)
}
// 创建第二个管理员
admin2Req := &service.CreateAdminRequest{
Username: "admin2_" + fmt.Sprintf("%d", time.Now().UnixNano()),
Password: "Admin123!",
Email: "admin2@test.com",
Nickname: "Admin Two",
}
_, err = env.userSvc.CreateAdmin(ctx, admin2Req)
if err != nil {
t.Fatalf("CreateAdmin admin2 failed: %v", err)
}
// 现在有2个管理员删除其中一个应该成功
err = env.userSvc.DeleteAdmin(ctx, admin1.ID, 9999)
if err != nil {
t.Errorf("expected DeleteAdmin to succeed with multiple admins, got error: %v", err)
}
t.Log("DeleteAdmin succeeded when multiple admins exist")
}
// =============================================================================
// Helper
// =============================================================================

View File

@@ -11,6 +11,7 @@ import (
"github.com/user-management-system/internal/domain"
"github.com/user-management-system/internal/pagination"
"github.com/user-management-system/internal/repository"
"gorm.io/gorm"
)
// UserService 用户服务
@@ -65,7 +66,7 @@ func (s *UserService) ChangePassword(ctx context.Context, userID int64, oldPassw
return err
}
// 检查密码历史
// 检查密码历史(需要明文密码比对,必须在哈希之前)
if s.passwordHistoryRepo != nil {
histories, err := s.passwordHistoryRepo.GetByUserID(ctx, userID, passwordHistoryLimit)
if err == nil && len(histories) > 0 {
@@ -75,30 +76,29 @@ func (s *UserService) ChangePassword(ctx context.Context, userID int64, oldPassw
}
}
}
}
// 保存新密码到历史记录
newHashedPassword, hashErr := auth.HashPassword(newPassword)
if hashErr != nil {
return errors.New("密码哈希失败")
}
// 计算一次哈希,用于更新密码和保存历史(避免 Argon2id 重复计算的高成本)
newHashedPassword, hashErr := auth.HashPassword(newPassword)
if hashErr != nil {
return errors.New("密码哈希失败")
}
// 保存新密码到历史记录(异步,不阻塞密码更新)
if s.passwordHistoryRepo != nil {
// #nosec G118 - 使用带超时的独立 context不能使用请求 ctx该 goroutine 在请求完成后仍可能运行)
go func() { // #nosec G118
go func(hashedPw string) { // #nosec G118
bgCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
_ = s.passwordHistoryRepo.Create(bgCtx, &domain.PasswordHistory{
UserID: userID,
PasswordHash: newHashedPassword,
PasswordHash: hashedPw,
})
_ = s.passwordHistoryRepo.DeleteOldRecords(bgCtx, userID, passwordHistoryLimit)
}()
}(newHashedPassword)
}
// 更新密码
newHashedPassword, err := auth.HashPassword(newPassword)
if err != nil {
return errors.New("密码哈希失败")
}
// 更新密码(使用同一哈希值)
user.Password = newHashedPassword
return s.userRepo.Update(ctx, user)
}
@@ -279,13 +279,23 @@ func (s *UserService) AssignRoles(ctx context.Context, userID int64, roleIDs []i
return s.userRoleRepo.BatchCreate(ctx, userRoles)
}
// AdminRoleID is the ID of the admin role
const AdminRoleID = 1
// getAdminRoleID looks up the admin role ID by code to avoid hardcoded magic numbers.
func (s *UserService) getAdminRoleID(ctx context.Context) (int64, error) {
adminRole, err := s.roleRepo.GetByCode(ctx, "admin")
if err != nil {
return 0, fmt.Errorf("failed to find admin role: %w", err)
}
return adminRole.ID, nil
}
// ListAdmins 获取所有管理员
func (s *UserService) ListAdmins(ctx context.Context) ([]*domain.User, error) {
// 获取管理员角色ID列表
adminUserIDs, err := s.userRoleRepo.GetUserIDByRoleID(ctx, AdminRoleID)
adminRoleID, err := s.getAdminRoleID(ctx)
if err != nil {
return nil, err
}
adminUserIDs, err := s.userRoleRepo.GetUserIDByRoleID(ctx, adminRoleID)
if err != nil {
return nil, err
}
@@ -307,7 +317,7 @@ func (s *UserService) ListAdmins(ctx context.Context) ([]*domain.User, error) {
return admins, nil
}
// CreateAdmin 创建管理员
// CreateAdmin 创建管理员(事务性)
func (s *UserService) CreateAdmin(ctx context.Context, req *CreateAdminRequest) (*domain.User, error) {
// 检查用户名是否已存在
existingUser, err := s.userRepo.GetByUsername(ctx, req.Username)
@@ -315,6 +325,12 @@ func (s *UserService) CreateAdmin(ctx context.Context, req *CreateAdminRequest)
return nil, errors.New("用户名已存在")
}
// 预先查询管理员角色 ID避免在事务中使用 roleRepo
adminRoleID, err := s.getAdminRoleID(ctx)
if err != nil {
return nil, err
}
// 创建用户
hashedPassword, err := auth.HashPassword(req.Password)
if err != nil {
@@ -334,16 +350,23 @@ func (s *UserService) CreateAdmin(ctx context.Context, req *CreateAdminRequest)
user.Nickname = req.Nickname
}
if err := s.userRepo.Create(ctx, user); err != nil {
return nil, err
}
// 使用事务创建用户和分配角色
err = s.userRepo.DB().WithContext(ctx).Transaction(func(tx *gorm.DB) error {
if err := tx.Create(user).Error; err != nil {
return err
}
// 分配管理员角色
userRole := &domain.UserRole{
UserID: user.ID,
RoleID: AdminRoleID,
}
if err := s.userRoleRepo.Create(ctx, userRole); err != nil {
// 分配管理员角色
userRole := &domain.UserRole{
UserID: user.ID,
RoleID: adminRoleID,
}
if err := tx.Create(userRole).Error; err != nil {
return err
}
return nil
})
if err != nil {
return nil, err
}
@@ -351,17 +374,32 @@ func (s *UserService) CreateAdmin(ctx context.Context, req *CreateAdminRequest)
}
// DeleteAdmin 删除管理员(移除管理员角色)
func (s *UserService) DeleteAdmin(ctx context.Context, userID int64) error {
func (s *UserService) DeleteAdmin(ctx context.Context, userID int64, currentUserID int64) error {
// 检查用户是否存在
if _, err := s.userRepo.GetByID(ctx, userID); err != nil {
return err
}
// 不能删除自己
// 注意这里需要从handler传入当前用户ID进行校验
if currentUserID == userID {
return errors.New("不能删除自己")
}
// 检查是否是最后一个管理员(保护)
adminRoleID, err := s.getAdminRoleID(ctx)
if err != nil {
return err
}
adminUserRoles, err := s.userRoleRepo.GetByRoleID(ctx, adminRoleID)
if err != nil {
return err
}
if len(adminUserRoles) <= 1 {
return errors.New("不能删除最后一个管理员")
}
// 删除用户的管理员角色
return s.userRoleRepo.DeleteByUserAndRole(ctx, userID, AdminRoleID)
return s.userRoleRepo.DeleteByUserAndRole(ctx, userID, adminRoleID)
}
// CreateAdminRequest 创建管理员请求