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"})
+}