[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).
This commit is contained in:
Gusted 2024-03-29 14:53:56 +01:00
parent 033c7ce685
commit fb4c42deb2
No known key found for this signature in database
GPG key ID: FD821B732837125F
3 changed files with 33 additions and 9 deletions

View file

@ -320,14 +320,6 @@ func DeleteEmailAddresses(ctx context.Context, emails []*EmailAddress) (err erro
return nil
}
// 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)

View file

@ -307,5 +307,5 @@ func DeleteInactiveUsers(ctx context.Context, olderThan time.Duration) error {
}
}
return user_model.DeleteInactiveEmailAddresses(ctx)
return nil
}

View file

@ -8,6 +8,7 @@ import (
"path/filepath"
"strings"
"testing"
"time"
"code.gitea.io/gitea/models"
"code.gitea.io/gitea/models/auth"
@ -17,6 +18,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"
)
@ -187,3 +189,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)
}