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
This commit is contained in:
wxiaoguang 2024-04-01 10:06:35 +08:00 committed by GitHub
parent 3607f827fb
commit a008486f5c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 45 additions and 33 deletions

View File

@ -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{}) 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. // ActivateEmail activates the email address to given user.
func ActivateEmail(ctx context.Context, email *EmailAddress) error { func ActivateEmail(ctx context.Context, email *EmailAddress) error {
ctx, committer, err := db.TxContext(ctx) ctx, committer, err := db.TxContext(ctx)

View File

@ -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) 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} 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 { if err := committer.Commit(); err != nil {
return err return err
} }
committer.Close() _ = committer.Close()
if err = asymkey_service.RewriteAllPublicKeys(ctx); err != nil { if err = asymkey_service.RewriteAllPublicKeys(ctx); err != nil {
return err return err
@ -259,50 +259,45 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error {
return err 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) 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)) _ = system_model.CreateNotice(ctx, system_model.NoticeTask, fmt.Sprintf("delete user '%s': %v", u.Name, err))
return err
} }
if u.Avatar != "" { if u.Avatar != "" {
avatarPath := u.CustomAvatarRelativePath() 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)) _ = system_model.CreateNotice(ctx, system_model.NoticeTask, fmt.Sprintf("delete user '%s': %v", u.Name, err))
return err
} }
} }
return nil 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 { 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 { if err != nil {
return err return err
} }
// FIXME: should only update authorized_keys file once after all deletions. // 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 continue
} }
select {
case <-ctx.Done():
return db.ErrCancelledf("when deleting inactive user %q", u.Name)
default:
return err return err
} }
} }
}
return user_model.DeleteInactiveEmailAddresses(ctx) return nil // TODO: there could be still inactive users left, and the number would increase gradually
} }

View File

@ -7,6 +7,7 @@ import (
"fmt" "fmt"
"strings" "strings"
"testing" "testing"
"time"
"code.gitea.io/gitea/models" "code.gitea.io/gitea/models"
"code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/auth"
@ -16,6 +17,7 @@ import (
"code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user" user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/timeutil"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
) )
@ -185,3 +187,26 @@ func TestCreateUser_Issue5882(t *testing.T) {
assert.NoError(t, DeleteUser(db.DefaultContext, v.user, false)) 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"})
}