Refactor DeleteInactiveUsers, fix bug and add tests (#30206) (#30222)

Backport #30206 by wxiaoguang

1. check `IsActive` before calling `IsLastAdminUser`.
2. Fix some comments and error messages.
3. Don't `return err` if "removing file" fails in `DeleteUser`.
4. Remove incorrect `DeleteInactiveEmailAddresses`. Active users could
also have inactive emails, and inactive emails do not support
"olderThan"
5. Add tests

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

models/user/email_address.go

@@ -256,14 +256,6 @@ func IsEmailUsed(ctx context.Context, email string) (bool, error) {
 	return db.GetEngine(ctx).Where("lower_email=?", strings.ToLower(email)).Get(&EmailAddress{})
 }
 
-// DeleteInactiveEmailAddresses deletes inactive email addresses
-func DeleteInactiveEmailAddresses(ctx context.Context) error {
-	_, err := db.GetEngine(ctx).
-		Where("is_activated = ?", false).
-		Delete(new(EmailAddress))
-	return err
-}
-
 // ActivateEmail activates the email address to given user.
 func ActivateEmail(ctx context.Context, email *EmailAddress) error {
 	ctx, committer, err := db.TxContext(ctx)

services/user/user.go

@@ -126,7 +126,7 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error {
 		return fmt.Errorf("%s is an organization not a user", u.Name)
 	}
 
-	if user_model.IsLastAdminUser(ctx, u) {
+	if u.IsActive && user_model.IsLastAdminUser(ctx, u) {
 		return models.ErrDeleteLastAdminUser{UID: u.ID}
 	}
 
@@ -250,7 +250,7 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error {
 	if err := committer.Commit(); err != nil {
 		return err
 	}
-	committer.Close()
+	_ = committer.Close()
 
 	if err = asymkey_service.RewriteAllPublicKeys(ctx); err != nil {
 		return err
@@ -259,50 +259,45 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error {
 		return err
 	}
 
-	// Note: There are something just cannot be roll back,
+	// Note: There are something just cannot be roll back, so just keep error logs of those operations.
-	//	so just keep error logs of those operations.
 	path := user_model.UserPath(u.Name)
-	if err := util.RemoveAll(path); err != nil {
+	if err = util.RemoveAll(path); err != nil {
-		err = fmt.Errorf("Failed to RemoveAll %s: %w", path, err)
+		err = fmt.Errorf("failed to RemoveAll %s: %w", path, err)
 		_ = system_model.CreateNotice(ctx, system_model.NoticeTask, fmt.Sprintf("delete user '%s': %v", u.Name, err))
-		return err
 	}
 
 	if u.Avatar != "" {
 		avatarPath := u.CustomAvatarRelativePath()
-		if err := storage.Avatars.Delete(avatarPath); err != nil {
+		if err = storage.Avatars.Delete(avatarPath); err != nil {
-			err = fmt.Errorf("Failed to remove %s: %w", avatarPath, err)
+			err = fmt.Errorf("failed to remove %s: %w", avatarPath, err)
 			_ = system_model.CreateNotice(ctx, system_model.NoticeTask, fmt.Sprintf("delete user '%s': %v", u.Name, err))
-			return err
 		}
 	}
 
 	return nil
 }
 
-// DeleteInactiveUsers deletes all inactive users and email addresses.
+// DeleteInactiveUsers deletes all inactive users and their email addresses.
 func DeleteInactiveUsers(ctx context.Context, olderThan time.Duration) error {
-	users, err := user_model.GetInactiveUsers(ctx, olderThan)
+	inactiveUsers, err := user_model.GetInactiveUsers(ctx, olderThan)
 	if err != nil {
 		return err
 	}
 
 	// FIXME: should only update authorized_keys file once after all deletions.
-	for _, u := range users {
+	for _, u := range inactiveUsers {
-		select {
+		if err = DeleteUser(ctx, u, false); err != nil {
-		case <-ctx.Done():
+			// Ignore inactive users that were ever active but then were set inactive by admin
-			return db.ErrCancelledf("Before delete inactive user %s", u.Name)
+			if models.IsErrUserOwnRepos(err) || models.IsErrUserHasOrgs(err) || models.IsErrUserOwnPackages(err) {
-		default:
-		}
-		if err := DeleteUser(ctx, u, false); err != nil {
-			// Ignore users that were set inactive by admin.
-			if models.IsErrUserOwnRepos(err) || models.IsErrUserHasOrgs(err) ||
-				models.IsErrUserOwnPackages(err) || models.IsErrDeleteLastAdminUser(err) {
 				continue
 			}
+			select {
+			case <-ctx.Done():
+				return db.ErrCancelledf("when deleting inactive user %q", u.Name)
+			default:
 				return err
 			}
 		}
-
+	}
-	return user_model.DeleteInactiveEmailAddresses(ctx)
+	return nil // TODO: there could be still inactive users left, and the number would increase gradually
 }

services/user/user_test.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"strings"
 	"testing"
+	"time"
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/models/auth"
@@ -16,6 +17,7 @@ import (
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/timeutil"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -185,3 +187,26 @@ func TestCreateUser_Issue5882(t *testing.T) {
 		assert.NoError(t, DeleteUser(db.DefaultContext, v.user, false))
 	}
 }
+
+func TestDeleteInactiveUsers(t *testing.T) {
+	addUser := func(name, email string, createdUnix timeutil.TimeStamp, active bool) {
+		inactiveUser := &user_model.User{Name: name, LowerName: strings.ToLower(name), Email: email, CreatedUnix: createdUnix, IsActive: active}
+		_, err := db.GetEngine(db.DefaultContext).NoAutoTime().Insert(inactiveUser)
+		assert.NoError(t, err)
+		inactiveUserEmail := &user_model.EmailAddress{UID: inactiveUser.ID, IsPrimary: true, Email: email, LowerEmail: strings.ToLower(email), IsActivated: active}
+		err = db.Insert(db.DefaultContext, inactiveUserEmail)
+		assert.NoError(t, err)
+	}
+	addUser("user-inactive-10", "user-inactive-10@test.com", timeutil.TimeStampNow().Add(-600), false)
+	addUser("user-inactive-5", "user-inactive-5@test.com", timeutil.TimeStampNow().Add(-300), false)
+	addUser("user-active-10", "user-active-10@test.com", timeutil.TimeStampNow().Add(-600), true)
+	addUser("user-active-5", "user-active-5@test.com", timeutil.TimeStampNow().Add(-300), true)
+	unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user-inactive-10"})
+	unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{Email: "user-inactive-10@test.com"})
+	assert.NoError(t, DeleteInactiveUsers(db.DefaultContext, 8*time.Minute))
+	unittest.AssertNotExistsBean(t, &user_model.User{Name: "user-inactive-10"})
+	unittest.AssertNotExistsBean(t, &user_model.EmailAddress{Email: "user-inactive-10@test.com"})
+	unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user-inactive-5"})
+	unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user-active-10"})
+	unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user-active-5"})
+}