From 8bd5169c5fe181e4a70e5987bc73930ddbd84370 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 3 Oct 2025 07:16:24 +0200 Subject: [PATCH] fix: allow unactivated users to send recovery mails (#9504) With forgejo/forgejo#9075 the `GetUserByEmail` now actually only used activated emails. This however broke sending recovery mails to unactivated users, as their email are not yet activated. Use the newly introduced function `GetUserByEmailSimple` to not care about this activated email requirement and also avoid the no-reply address being a valid email address for this functionality. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9504 Reviewed-by: Otto Co-authored-by: Gusted Co-committed-by: Gusted --- models/user/user.go | 24 +++++++++++- models/user/user_test.go | 30 ++++++++++++++ routers/web/auth/password.go | 2 +- tests/integration/user_recovery_test.go | 52 +++++++++++++++++++++++++ 4 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 tests/integration/user_recovery_test.go diff --git a/models/user/user.go b/models/user/user.go index 8cdecc06e4..819199607d 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -1194,7 +1194,9 @@ func ValidateCommitsWithEmails(ctx context.Context, oldCommits []*git.Commit) [] return newCommits } -// GetUserByEmail returns the user object by given e-mail if exists. +// GetUserByEmail returns the user associated with the email, if it exists +// and is activated. If the email is a no-reply address, then the user +// associated with that no-reply address is returned. func GetUserByEmail(ctx context.Context, email string) (*User, error) { if len(email) == 0 { return nil, ErrUserNotExist{Name: email} @@ -1227,6 +1229,26 @@ func GetUserByEmail(ctx context.Context, email string) (*User, error) { return nil, ErrUserNotExist{Name: email} } +// GetUserByEmailSimple returns the user associated with the email, if it exists. +// +// NOTE: You likely should use `GetUserByEmail`, which handles the no-reply +// address and only uses activated emails to get the user. +func GetUserByEmailSimple(ctx context.Context, email string) (*User, error) { + if len(email) == 0 { + return nil, ErrUserNotExist{Name: email} + } + + emailAddress := &EmailAddress{} + has, err := db.GetEngine(ctx).Where("lower_email = ?", strings.ToLower(email)).Get(emailAddress) + if err != nil { + return nil, err + } else if !has { + return nil, ErrUserNotExist{Name: email} + } + + return GetUserByID(ctx, emailAddress.UID) +} + // GetUser checks if a user already exists func GetUser(ctx context.Context, user *User) (bool, error) { return db.GetEngine(ctx).Get(user) diff --git a/models/user/user_test.go b/models/user/user_test.go index 20665eb813..0a1802ba6a 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -999,6 +999,7 @@ func TestPronounsPrivacy(t *testing.T) { func TestGetUserByEmail(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) + defer test.MockVariableValue(&setting.Service.NoReplyAddress, "noreply.example.org")() t.Run("Normal", func(t *testing.T) { u, err := user_model.GetUserByEmail(t.Context(), "user2@example.com") @@ -1017,4 +1018,33 @@ func TestGetUserByEmail(t *testing.T) { require.NoError(t, err) assert.EqualValues(t, 1, u.ID) }) + + t.Run("No-reply", func(t *testing.T) { + u, err := user_model.GetUserByEmail(t.Context(), "user1@noreply.example.org") + require.NoError(t, err) + assert.EqualValues(t, 1, u.ID) + }) +} + +func TestGetUserByEmailSimple(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + defer test.MockVariableValue(&setting.Service.NoReplyAddress, "noreply.example.org")() + + t.Run("Normal", func(t *testing.T) { + u, err := user_model.GetUserByEmailSimple(t.Context(), "user2@example.com") + require.NoError(t, err) + assert.EqualValues(t, 2, u.ID) + }) + + t.Run("Not activated", func(t *testing.T) { + u, err := user_model.GetUserByEmailSimple(t.Context(), "user11@example.com") + require.NoError(t, err) + assert.EqualValues(t, 11, u.ID) + }) + + t.Run("No-reply", func(t *testing.T) { + u, err := user_model.GetUserByEmailSimple(t.Context(), "user1@noreply.example.org") + require.ErrorIs(t, err, user_model.ErrUserNotExist{Name: "user1@noreply.example.org"}) + assert.Nil(t, u) + }) } diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go index 4f31f372b4..5d1da59ac5 100644 --- a/routers/web/auth/password.go +++ b/routers/web/auth/password.go @@ -61,7 +61,7 @@ func ForgotPasswdPost(ctx *context.Context) { email := ctx.FormString("email") ctx.Data["Email"] = email - u, err := user_model.GetUserByEmail(ctx, email) + u, err := user_model.GetUserByEmailSimple(ctx, email) if err != nil { if user_model.IsErrUserNotExist(err) { ctx.Data["ResetPwdCodeLives"] = timeutil.MinutesToFriendly(setting.Service.ResetPwdCodeLives, ctx.Locale) diff --git a/tests/integration/user_recovery_test.go b/tests/integration/user_recovery_test.go new file mode 100644 index 0000000000..94114c6b38 --- /dev/null +++ b/tests/integration/user_recovery_test.go @@ -0,0 +1,52 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "net/http" + "testing" + + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + "forgejo.org/modules/test" + "forgejo.org/modules/translation" + "forgejo.org/services/mailer" + "forgejo.org/tests" + + "github.com/stretchr/testify/assert" +) + +func TestForgotPassword(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + test := func(t *testing.T, user *user_model.User, email *user_model.EmailAddress) { + t.Helper() + + called := false + defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) { + assert.Len(t, msgs, 1) + assert.Equal(t, user.EmailTo(), msgs[0].To) + assert.EqualValues(t, translation.NewLocale("en-US").Tr("mail.reset_password"), msgs[0].Subject) + assert.Contains(t, msgs[0].Body, translation.NewLocale("en-US").Tr("mail.reset_password.text", "3 hours")) + called = true + })() + + req := NewRequestWithValues(t, "POST", "/user/forgot_password", map[string]string{ + "_csrf": GetCSRF(t, emptyTestSession(t), "/user/forgot_password"), + "email": email.Email, + }) + MakeRequest(t, req, http.StatusOK) + + assert.True(t, called) + } + t.Run("Unactivated email address", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + test(t, unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 11}), unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{UID: 11}, "is_activated = false")) + }) + + t.Run("Activated email address", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + test(t, unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 12}), unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{UID: 12}, "is_activated = true")) + }) +}