From a008486f5c5acfe2d2acb009f41dc660ee8348eb Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 1 Apr 2024 10:06:35 +0800 Subject: [PATCH] Refactor DeleteInactiveUsers, fix bug and add tests (#30206) 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 --- models/user/email_address.go | 8 ------- services/user/user.go | 45 ++++++++++++++++-------------------- services/user/user_test.go | 25 ++++++++++++++++++++ 3 files changed, 45 insertions(+), 33 deletions(-) diff --git a/models/user/email_address.go b/models/user/email_address.go index d26549f383..08771efe99 100644 --- a/models/user/email_address.go +++ b/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) diff --git a/services/user/user.go b/services/user/user.go index 4fcb81581d..2287e36c71 100644 --- a/services/user/user.go +++ b/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, - // so just keep error logs of those operations. + // Note: There are something just cannot be roll back, so just keep error logs of those operations. path := user_model.UserPath(u.Name) - if err := util.RemoveAll(path); err != nil { - err = fmt.Errorf("Failed to RemoveAll %s: %w", path, err) + if err = util.RemoveAll(path); err != nil { + 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 { - err = fmt.Errorf("Failed to remove %s: %w", avatarPath, err) + if err = storage.Avatars.Delete(avatarPath); err != nil { + 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 { - select { - case <-ctx.Done(): - return db.ErrCancelledf("Before delete inactive user %s", u.Name) - 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) { + for _, u := range inactiveUsers { + if err = DeleteUser(ctx, u, false); err != nil { + // Ignore inactive users that were ever active but then were set inactive by admin + if models.IsErrUserOwnRepos(err) || models.IsErrUserHasOrgs(err) || models.IsErrUserOwnPackages(err) { continue } - return err + 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 } diff --git a/services/user/user_test.go b/services/user/user_test.go index f110bd26d0..bd6019a14f 100644 --- a/services/user/user_test.go +++ b/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"}) +}