fix: use transactions when operations involve multiple database queries (#392)

Co-authored-by: Kyle Mendell <kmendell@ofkm.us>
2026-02-16 03:46:12 +00:00 · 2025-04-06 06:04:08 -07:00
parent c810fec8c4
commit ec626ee797
33 changed files with 1401 additions and 501 deletions
--- a/backend/internal/service/user_service.go
+++ b/backend/internal/service/user_service.go
@@ -2,6 +2,7 @@ package service

 import (
 	"bytes"
+	"context"
 	"errors"
 	"fmt"
 	"io"
@@ -12,7 +13,7 @@ import (
 	"time"

 	"github.com/google/uuid"
-	profilepicture "github.com/pocket-id/pocket-id/backend/internal/utils/image"
+	"gorm.io/gorm"

 	"github.com/pocket-id/pocket-id/backend/internal/common"
 	"github.com/pocket-id/pocket-id/backend/internal/dto"
@@ -20,7 +21,7 @@ import (
 	datatype "github.com/pocket-id/pocket-id/backend/internal/model/types"
 	"github.com/pocket-id/pocket-id/backend/internal/utils"
 	"github.com/pocket-id/pocket-id/backend/internal/utils/email"
-	"gorm.io/gorm"
+	profilepicture "github.com/pocket-id/pocket-id/backend/internal/utils/image"
 )

 type UserService struct {
@@ -35,9 +36,9 @@ func NewUserService(db *gorm.DB, jwtService *JwtService, auditLogService *AuditL
 	return &UserService{db: db, jwtService: jwtService, auditLogService: auditLogService, emailService: emailService, appConfigService: appConfigService}
 }

-func (s *UserService) ListUsers(searchTerm string, sortedPaginationRequest utils.SortedPaginationRequest) ([]model.User, utils.PaginationResponse, error) {
+func (s *UserService) ListUsers(ctx context.Context, searchTerm string, sortedPaginationRequest utils.SortedPaginationRequest) ([]model.User, utils.PaginationResponse, error) {
 	var users []model.User
-	query := s.db.Model(&model.User{})
+	query := s.db.WithContext(ctx).Model(&model.User{})

 	if searchTerm != "" {
 		searchPattern := "%" + searchTerm + "%"
@@ -48,13 +49,23 @@ func (s *UserService) ListUsers(searchTerm string, sortedPaginationRequest utils
 	return users, pagination, err
 }

-func (s *UserService) GetUser(userID string) (model.User, error) {
+func (s *UserService) GetUser(ctx context.Context, userID string) (model.User, error) {
+	return s.getUserInternal(ctx, userID, s.db)
+}
+
+func (s *UserService) getUserInternal(ctx context.Context, userID string, tx *gorm.DB) (model.User, error) {
 	var user model.User
-	err := s.db.Preload("UserGroups").Preload("CustomClaims").Where("id = ?", userID).First(&user).Error
+	err := tx.
+		WithContext(ctx).
+		Preload("UserGroups").
+		Preload("CustomClaims").
+		Where("id = ?", userID).
+		First(&user).
+		Error
 	return user, err
 }

-func (s *UserService) GetProfilePicture(userID string) (io.ReadCloser, int64, error) {
+func (s *UserService) GetProfilePicture(ctx context.Context, userID string) (io.ReadCloser, int64, error) {
 	// Validate the user ID to prevent directory traversal
 	if err := uuid.Validate(userID); err != nil {
 		return nil, 0, &common.InvalidUUIDError{}
@@ -74,7 +85,7 @@ func (s *UserService) GetProfilePicture(userID string) (io.ReadCloser, int64, er
 	}

 	// If no custom picture exists, get the user's data for creating initials
-	user, err := s.GetUser(userID)
+	user, err := s.GetUser(ctx, userID)
 	if err != nil {
 		return nil, 0, err
 	}
@@ -115,9 +126,15 @@ func (s *UserService) GetProfilePicture(userID string) (io.ReadCloser, int64, er
 	return io.NopCloser(bytes.NewReader(defaultPictureBytes)), int64(defaultPicture.Len()), nil
 }

-func (s *UserService) GetUserGroups(userID string) ([]model.UserGroup, error) {
+func (s *UserService) GetUserGroups(ctx context.Context, userID string) ([]model.UserGroup, error) {
 	var user model.User
-	if err := s.db.Preload("UserGroups").Where("id = ?", userID).First(&user).Error; err != nil {
+	err := s.db.
+		WithContext(ctx).
+		Preload("UserGroups").
+		Where("id = ?", userID).
+		First(&user).
+		Error
+	if err != nil {
 		return nil, err
 	}
 	return user.UserGroups, nil
@@ -152,9 +169,21 @@ func (s *UserService) UpdateProfilePicture(userID string, file io.Reader) error
 	return nil
 }

-func (s *UserService) DeleteUser(userID string, allowLdapDelete bool) error {
+func (s *UserService) DeleteUser(ctx context.Context, userID string, allowLdapDelete bool) error {
+	return s.db.Transaction(func(tx *gorm.DB) error {
+		return s.deleteUserInternal(ctx, userID, allowLdapDelete, tx)
+	})
+}
+
+func (s *UserService) deleteUserInternal(ctx context.Context, userID string, allowLdapDelete bool, tx *gorm.DB) error {
 	var user model.User
-	if err := s.db.Where("id = ?", userID).First(&user).Error; err != nil {
+
+	err := tx.
+		WithContext(ctx).
+		Where("id = ?", userID).
+		First(&user).
+		Error
+	if err != nil {
 		return err
 	}

@@ -165,14 +194,35 @@ func (s *UserService) DeleteUser(userID string, allowLdapDelete bool) error {

 	// Delete the profile picture
 	profilePicturePath := common.EnvConfig.UploadPath + "/profile-pictures/" + userID + ".png"
-	if err := os.Remove(profilePicturePath); err != nil && !os.IsNotExist(err) {
+	err = os.Remove(profilePicturePath)
+	if err != nil && !os.IsNotExist(err) {
 		return err
 	}

-	return s.db.Delete(&user).Error
+	return tx.WithContext(ctx).Delete(&user).Error
 }

-func (s *UserService) CreateUser(input dto.UserCreateDto) (model.User, error) {
+func (s *UserService) CreateUser(ctx context.Context, input dto.UserCreateDto) (model.User, error) {
+	tx := s.db.Begin()
+	defer func() {
+		// This is a no-op if the transaction has been committed already
+		tx.Rollback()
+	}()
+
+	user, err := s.createUserInternal(ctx, input, tx)
+	if err != nil {
+		return model.User{}, err
+	}
+
+	err = tx.Commit().Error
+	if err != nil {
+		return model.User{}, err
+	}
+
+	return user, nil
+}
+
+func (s *UserService) createUserInternal(ctx context.Context, input dto.UserCreateDto, tx *gorm.DB) (model.User, error) {
 	user := model.User{
 		FirstName: input.FirstName,
 		LastName:  input.LastName,
@@ -185,18 +235,47 @@ func (s *UserService) CreateUser(input dto.UserCreateDto) (model.User, error) {
 		user.LdapID = &input.LdapID
 	}

-	if err := s.db.Create(&user).Error; err != nil {
-		if errors.Is(err, gorm.ErrDuplicatedKey) {
-			return model.User{}, s.checkDuplicatedFields(user)
-		}
+	err := tx.WithContext(ctx).Create(&user).Error
+	if errors.Is(err, gorm.ErrDuplicatedKey) {
+		tx.Rollback()
+
+		// If we are here, the transaction is already aborted due to an error, so we pass s.db
+		err = s.checkDuplicatedFields(ctx, user, s.db)
+		return model.User{}, err
+	} else if err != nil {
 		return model.User{}, err
 	}
 	return user, nil
 }

-func (s *UserService) UpdateUser(userID string, updatedUser dto.UserCreateDto, updateOwnUser bool, allowLdapUpdate bool) (model.User, error) {
+func (s *UserService) UpdateUser(ctx context.Context, userID string, updatedUser dto.UserCreateDto, updateOwnUser bool, allowLdapUpdate bool) (model.User, error) {
+	tx := s.db.Begin()
+	defer func() {
+		// This is a no-op if the transaction has been committed already
+		tx.Rollback()
+	}()
+
+	user, err := s.updateUserInternal(ctx, userID, updatedUser, updateOwnUser, allowLdapUpdate, tx)
+	if err != nil {
+		return model.User{}, err
+	}
+
+	err = tx.Commit().Error
+	if err != nil {
+		return model.User{}, err
+	}
+
+	return user, nil
+}
+
+func (s *UserService) updateUserInternal(ctx context.Context, userID string, updatedUser dto.UserCreateDto, updateOwnUser bool, allowLdapUpdate bool, tx *gorm.DB) (model.User, error) {
 	var user model.User
-	if err := s.db.Where("id = ?", userID).First(&user).Error; err != nil {
+	err := tx.
+		WithContext(ctx).
+		Where("id = ?", userID).
+		First(&user).
+		Error
+	if err != nil {
 		return model.User{}, err
 	}

@@ -214,24 +293,42 @@ func (s *UserService) UpdateUser(userID string, updatedUser dto.UserCreateDto, u
 		user.IsAdmin = updatedUser.IsAdmin
 	}

-	if err := s.db.Save(&user).Error; err != nil {
-		if errors.Is(err, gorm.ErrDuplicatedKey) {
-			return user, s.checkDuplicatedFields(user)
-		}
+	err = tx.
+		WithContext(ctx).
+		Save(&user).
+		Error
+	if errors.Is(err, gorm.ErrDuplicatedKey) {
+		tx.Rollback()
+
+		// If we are here, the transaction is already aborted due to an error, so we pass s.db
+		err = s.checkDuplicatedFields(ctx, user, s.db)
+		return user, err
+	} else if err != nil {
 		return user, err
 	}

 	return user, nil
 }

-func (s *UserService) RequestOneTimeAccessEmail(emailAddress, redirectPath string) error {
+func (s *UserService) RequestOneTimeAccessEmail(ctx context.Context, emailAddress, redirectPath string) error {
+	tx := s.db.Begin()
+	defer func() {
+		// This is a no-op if the transaction has been committed already
+		tx.Rollback()
+	}()
+
 	isDisabled := !s.appConfigService.DbConfig.EmailOneTimeAccessEnabled.IsTrue()
 	if isDisabled {
 		return &common.OneTimeAccessDisabledError{}
 	}

 	var user model.User
-	if err := s.db.Where("email = ?", emailAddress).First(&user).Error; err != nil {
+	err := tx.
+		WithContext(ctx).
+		Where("email = ?", emailAddress).
+		First(&user).
+		Error
+	if err != nil {
 		// Do not return error if user not found to prevent email enumeration
 		if errors.Is(err, gorm.ErrRecordNotFound) {
 			return nil
@@ -240,22 +337,31 @@ func (s *UserService) RequestOneTimeAccessEmail(emailAddress, redirectPath strin
 		}
 	}

-	oneTimeAccessToken, err := s.CreateOneTimeAccessToken(user.ID, time.Now().Add(15*time.Minute))
+	oneTimeAccessToken, err := s.createOneTimeAccessTokenInternal(ctx, user.ID, time.Now().Add(15*time.Minute), tx)
 	if err != nil {
 		return err
 	}

-	link := fmt.Sprintf("%s/lc", common.EnvConfig.AppURL)
-	linkWithCode := fmt.Sprintf("%s/%s", link, oneTimeAccessToken)
-
-	// Add redirect path to the link
-	if strings.HasPrefix(redirectPath, "/") {
-		encodedRedirectPath := url.QueryEscape(redirectPath)
-		linkWithCode = fmt.Sprintf("%s?redirect=%s", linkWithCode, encodedRedirectPath)
+	err = tx.Commit().Error
+	if err != nil {
+		return err
 	}

+	// We use a background context here as this is running in a goroutine
+	//nolint:contextcheck
 	go func() {
-		err := SendEmail(s.emailService, email.Address{
+		innerCtx := context.Background()
+
+		link := common.EnvConfig.AppURL + "/lc"
+		linkWithCode := link + "/" + oneTimeAccessToken
+
+		// Add redirect path to the link
+		if strings.HasPrefix(redirectPath, "/") {
+			encodedRedirectPath := url.QueryEscape(redirectPath)
+			linkWithCode = linkWithCode + "?redirect=" + encodedRedirectPath
+		}
+
+		errInternal := SendEmail(innerCtx, s.emailService, email.Address{
 			Name:  user.Username,
 			Email: user.Email,
 		}, OneTimeAccessTemplate, &OneTimeAccessTemplateData{
@@ -263,18 +369,21 @@ func (s *UserService) RequestOneTimeAccessEmail(emailAddress, redirectPath strin
 			LoginLink:         link,
 			LoginLinkWithCode: linkWithCode,
 		})
-		if err != nil {
-			log.Printf("Failed to send email to '%s': %v\n", user.Email, err)
+		if errInternal != nil {
+			log.Printf("Failed to send email to '%s': %v\n", user.Email, errInternal)
 		}
 	}()

 	return nil
 }

-func (s *UserService) CreateOneTimeAccessToken(userID string, expiresAt time.Time) (string, error) {
-	tokenLength := 16
+func (s *UserService) CreateOneTimeAccessToken(ctx context.Context, userID string, expiresAt time.Time) (string, error) {
+	return s.createOneTimeAccessTokenInternal(ctx, userID, expiresAt, s.db)
+}

+func (s *UserService) createOneTimeAccessTokenInternal(ctx context.Context, userID string, expiresAt time.Time, tx *gorm.DB) (string, error) {
 	// If expires at is less than 15 minutes, use an 6 character token instead of 16
+	tokenLength := 16
 	if time.Until(expiresAt) <= 15*time.Minute {
 		tokenLength = 6
 	}
@@ -290,16 +399,27 @@ func (s *UserService) CreateOneTimeAccessToken(userID string, expiresAt time.Tim
 		Token:     randomString,
 	}

-	if err := s.db.Create(&oneTimeAccessToken).Error; err != nil {
+	if err := tx.WithContext(ctx).Create(&oneTimeAccessToken).Error; err != nil {
 		return "", err
 	}

 	return oneTimeAccessToken.Token, nil
 }

-func (s *UserService) ExchangeOneTimeAccessToken(token string, ipAddress, userAgent string) (model.User, string, error) {
+func (s *UserService) ExchangeOneTimeAccessToken(ctx context.Context, token string, ipAddress, userAgent string) (model.User, string, error) {
+	tx := s.db.Begin()
+	defer func() {
+		// This is a no-op if the transaction has been committed already
+		tx.Rollback()
+	}()
+
 	var oneTimeAccessToken model.OneTimeAccessToken
-	if err := s.db.Where("token = ? AND expires_at > ?", token, datatype.DateTime(time.Now())).Preload("User").First(&oneTimeAccessToken).Error; err != nil {
+	err := tx.
+		WithContext(ctx).
+		Where("token = ? AND expires_at > ?", token, datatype.DateTime(time.Now())).Preload("User").
+		First(&oneTimeAccessToken).
+		Error
+	if err != nil {
 		if errors.Is(err, gorm.ErrRecordNotFound) {
 			return model.User{}, "", &common.TokenInvalidOrExpiredError{}
 		}
@@ -310,19 +430,34 @@ func (s *UserService) ExchangeOneTimeAccessToken(token string, ipAddress, userAg
 		return model.User{}, "", err
 	}

-	if err := s.db.Delete(&oneTimeAccessToken).Error; err != nil {
+	err = tx.
+		WithContext(ctx).
+		Delete(&oneTimeAccessToken).
+		Error
+	if err != nil {
 		return model.User{}, "", err
 	}

 	if ipAddress != "" && userAgent != "" {
-		s.auditLogService.Create(model.AuditLogEventOneTimeAccessTokenSignIn, ipAddress, userAgent, oneTimeAccessToken.User.ID, model.AuditLogData{})
+		s.auditLogService.Create(ctx, model.AuditLogEventOneTimeAccessTokenSignIn, ipAddress, userAgent, oneTimeAccessToken.User.ID, model.AuditLogData{}, tx)
+	}
+
+	err = tx.Commit().Error
+	if err != nil {
+		return model.User{}, "", err
 	}

 	return oneTimeAccessToken.User, accessToken, nil
 }

-func (s *UserService) UpdateUserGroups(id string, userGroupIds []string) (user model.User, err error) {
-	user, err = s.GetUser(id)
+func (s *UserService) UpdateUserGroups(ctx context.Context, id string, userGroupIds []string) (user model.User, err error) {
+	tx := s.db.Begin()
+	defer func() {
+		// This is a no-op if the transaction has been committed already
+		tx.Rollback()
+	}()
+
+	user, err = s.getUserInternal(ctx, id, tx)
 	if err != nil {
 		return model.User{}, err
 	}
@@ -330,27 +465,49 @@ func (s *UserService) UpdateUserGroups(id string, userGroupIds []string) (user m
 	// Fetch the groups based on userGroupIds
 	var groups []model.UserGroup
 	if len(userGroupIds) > 0 {
-		if err := s.db.Where("id IN (?)", userGroupIds).Find(&groups).Error; err != nil {
+		err = tx.
+			WithContext(ctx).
+			Where("id IN (?)", userGroupIds).
+			Find(&groups).
+			Error
+		if err != nil {
 			return model.User{}, err
 		}
 	}

 	// Replace the current groups with the new set of groups
-	if err := s.db.Model(&user).Association("UserGroups").Replace(groups); err != nil {
+	err = tx.
+		WithContext(ctx).
+		Model(&user).
+		Association("UserGroups").
+		Replace(groups)
+	if err != nil {
 		return model.User{}, err
 	}

 	// Save the updated user
-	if err := s.db.Save(&user).Error; err != nil {
+	err = tx.WithContext(ctx).Save(&user).Error
+	if err != nil {
+		return model.User{}, err
+	}
+
+	err = tx.Commit().Error
+	if err != nil {
 		return model.User{}, err
 	}

 	return user, nil
 }

-func (s *UserService) SetupInitialAdmin() (model.User, string, error) {
+func (s *UserService) SetupInitialAdmin(ctx context.Context) (model.User, string, error) {
+	tx := s.db.Begin()
+	defer func() {
+		// This is a no-op if the transaction has been committed already
+		tx.Rollback()
+	}()
+
 	var userCount int64
-	if err := s.db.Model(&model.User{}).Count(&userCount).Error; err != nil {
+	if err := tx.WithContext(ctx).Model(&model.User{}).Count(&userCount).Error; err != nil {
 		return model.User{}, "", err
 	}
 	if userCount > 1 {
@@ -365,7 +522,7 @@ func (s *UserService) SetupInitialAdmin() (model.User, string, error) {
 		IsAdmin:   true,
 	}

-	if err := s.db.Model(&model.User{}).Preload("Credentials").FirstOrCreate(&user).Error; err != nil {
+	if err := tx.WithContext(ctx).Model(&model.User{}).Preload("Credentials").FirstOrCreate(&user).Error; err != nil {
 		return model.User{}, "", err
 	}

@@ -378,16 +535,39 @@ func (s *UserService) SetupInitialAdmin() (model.User, string, error) {
 		return model.User{}, "", err
 	}

+	err = tx.Commit().Error
+	if err != nil {
+		return model.User{}, "", err
+	}
+
 	return user, token, nil
 }

-func (s *UserService) checkDuplicatedFields(user model.User) error {
-	var existingUser model.User
-	if s.db.Where("id != ? AND email = ?", user.ID, user.Email).First(&existingUser).Error == nil {
+func (s *UserService) checkDuplicatedFields(ctx context.Context, user model.User, tx *gorm.DB) error {
+	var result struct {
+		Found bool
+	}
+	err := tx.
+		WithContext(ctx).
+		Raw(`SELECT EXISTS(SELECT 1 FROM users WHERE id != ? AND email = ?) AS found`, user.ID, user.Email).
+		First(&result).
+		Error
+	if err != nil {
+		return err
+	}
+	if result.Found {
 		return &common.AlreadyInUseError{Property: "email"}
 	}

-	if s.db.Where("id != ? AND username = ?", user.ID, user.Username).First(&existingUser).Error == nil {
+	err = tx.
+		WithContext(ctx).
+		Raw(`SELECT EXISTS(SELECT 1 FROM users WHERE id != ? AND username = ?) AS found`, user.ID, user.Username).
+		First(&result).
+		Error
+	if err != nil {
+		return err
+	}
+	if result.Found {
 		return &common.AlreadyInUseError{Property: "username"}
 	}