From 190383dbf82d9fa3e1c5a89078604bc04b29ae75 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 29 Mar 2024 14:53:56 +0100 Subject: [PATCH] [BUG] Don't delete inactive emails explicitly - `user_model.DeleteInactiveEmailAddresses` related code was added in Gogs as part to delete inactive users, however since then the related code to delete users has changed and this code now already delete email addresses of the user, it's therefore not needed anymore to `DeleteInactiveEmailAddresses`. - The call to `DeleteInactiveEmailAddresses` can actually cause issues. As the associated user might not have been deleted, because it was not older than the specified `olderThan` argument. Therefore causing a database inconsistency and lead to internal server errors if the user tries to activate their account. - Adds unit test to verify correct behavior (fails without this patch). --- models/user/email_address.go | 8 -------- services/user/user.go | 2 +- services/user/user_test.go | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/models/user/email_address.go b/models/user/email_address.go index 45bcc54aa..85824fcdc 100644 --- a/models/user/email_address.go +++ b/models/user/email_address.go @@ -278,14 +278,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 f2648db40..9dc4f6fe6 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -304,5 +304,5 @@ func DeleteInactiveUsers(ctx context.Context, olderThan time.Duration) error { } } - return user_model.DeleteInactiveEmailAddresses(ctx) + return nil } diff --git a/services/user/user_test.go b/services/user/user_test.go index 2ebcded92..9013208ed 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" ) @@ -184,3 +186,33 @@ func TestCreateUser_Issue5882(t *testing.T) { assert.NoError(t, DeleteUser(db.DefaultContext, v.user, false)) } } + +func TestDeleteInactiveUsers(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + // Add an inactive user older than a minute, with an associated email_address record. + oldUser := &user_model.User{Name: "OldInactive", LowerName: "oldinactive", Email: "old@example.com", CreatedUnix: timeutil.TimeStampNow().Add(-120)} + _, err := db.GetEngine(db.DefaultContext).NoAutoTime().Insert(oldUser) + assert.NoError(t, err) + oldEmail := &user_model.EmailAddress{UID: oldUser.ID, IsPrimary: true, Email: "old@example.com", LowerEmail: "old@example.com"} + err = db.Insert(db.DefaultContext, oldEmail) + assert.NoError(t, err) + + // Add an inactive user that's not older than a minute, with an associated email_address record. + newUser := &user_model.User{Name: "NewInactive", LowerName: "newinactive", Email: "new@example.com"} + err = db.Insert(db.DefaultContext, newUser) + assert.NoError(t, err) + newEmail := &user_model.EmailAddress{UID: newUser.ID, IsPrimary: true, Email: "new@example.com", LowerEmail: "new@example.com"} + err = db.Insert(db.DefaultContext, newEmail) + assert.NoError(t, err) + + err = DeleteInactiveUsers(db.DefaultContext, time.Minute) + assert.NoError(t, err) + + // User older than a minute should be deleted along with their email address. + unittest.AssertExistsIf(t, false, oldUser) + unittest.AssertExistsIf(t, false, oldEmail) + + // User not older than a minute shouldn't be deleted and their emaill address should still exist. + unittest.AssertExistsIf(t, true, newUser) + unittest.AssertExistsIf(t, true, newEmail) +}