From 3a80534d4db523efe56b368489f81dc1cb2c99f7 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Thu, 7 Dec 2023 12:57:16 +0100 Subject: [PATCH] [GITEA] Allow changing the email address before activation During registration, one may be required to give their email address, to be verified and activated later. However, if one makes a mistake, a typo, they may end up with an account that cannot be activated due to having a wrong email address. They can still log in, but not change the email address, thus, no way to activate it without help from an administrator. To remedy this issue, lets allow changing the email address for logged in, but not activated users. This fixes gitea#17785. Signed-off-by: Gergely Nagy (cherry picked from commit aaaece28e4c6a8980cef932e224e84933d7c9262) (cherry picked from commit 639dafabec0a5c1f943b44ca02f72c5ba2fc5e10) (cherry picked from commit d699c12cebea7dbbda950ae257a46d53c39f22ea) [GITEA] Allow changing the email address before activation (squash) cache is always active This needs to be revisited because the MailResendLimit is not enforced and turns out to not be tested. See e7cb8da2a8 * Always enable caches (#28527) (cherry picked from commit 43ded8ee30ab5c7a40a456600cdaa8a0fbdccec2) Rate limit pre-activation email change separately Changing the email address before any email address is activated should be subject to a different rate limit than the normal activation email resending. If there's only one rate limit for both, then if a newly signed up quickly discovers they gave a wrong email address, they'd have to wait three minutes to change it. With the two separate limits, they don't - but they'll have to wait three minutes before they can change the email address again. The downside of this setup is that a malicious actor can alternate between resending and changing the email address (to something like `user+$idx@domain`, delivered to the same inbox) to effectively halving the rate limit. I do not think there's a better solution, and this feels like such a small attack surface that I'd deem it acceptable. The way the code works after this change is that `ActivatePost` will now check the `MailChangeLimit_user` key rather than `MailResendLimit_user`, and if we're within the limit, it will set `MailChangedJustNow_user`. The `Activate` method - which sends the activation email, whether it is a normal resend, or one following an email change - will check `MailChangedJustNow_user`, and if it is set, it will check the rate limit against `MailChangedLimit_user`, otherwise against `MailResendLimit_user`, and then will delete the `MailChangedJustNow_user` key from the cache. Fixes #2040. Signed-off-by: Gergely Nagy (cherry picked from commit e35d2af2e56f4ecb3a4f6d1109d02c8aa1a6d182) (cherry picked from commit 03989418a70d3445e0edada7fbe5a4151d7836b1) (cherry picked from commit f50e0dfe5e90d6a31c5b59e687580e8b2725c22b) (cherry picked from commit cad9184a3653e6c80de2e006a0d699b816980987) (cherry picked from commit e2da5d7fe13a685606913a131687a94f9f5fcfeb) --- models/user/email_address.go | 77 +++++++++++++++++++++---------- models/user/email_address_test.go | 22 +++++++++ routers/web/auth/auth.go | 50 +++++++++++++++++++- templates/user/auth/activate.tmpl | 10 ++++ tests/integration/signup_test.go | 76 ++++++++++++++++++++++++++++++ 5 files changed, 208 insertions(+), 27 deletions(-) diff --git a/models/user/email_address.go b/models/user/email_address.go index b795a7e94b..986f12e900 100644 --- a/models/user/email_address.go +++ b/models/user/email_address.go @@ -375,31 +375,7 @@ func updateActivation(ctx context.Context, email *EmailAddress, activate bool) e return UpdateUserCols(ctx, user, "rands") } -// MakeEmailPrimary sets primary email address of given user. -func MakeEmailPrimary(ctx context.Context, email *EmailAddress) error { - has, err := db.GetEngine(ctx).Get(email) - if err != nil { - return err - } else if !has { - return ErrEmailAddressNotExist{Email: email.Email} - } - - if !email.IsActivated { - return ErrEmailNotActivated - } - - user := &User{} - has, err = db.GetEngine(ctx).ID(email.UID).Get(user) - if err != nil { - return err - } else if !has { - return ErrUserNotExist{ - UID: email.UID, - Name: "", - KeyID: 0, - } - } - +func makeEmailPrimary(ctx context.Context, user *User, email *EmailAddress) error { ctx, committer, err := db.TxContext(ctx) if err != nil { return err @@ -429,6 +405,57 @@ func MakeEmailPrimary(ctx context.Context, email *EmailAddress) error { return committer.Commit() } +// ReplaceInactivePrimaryEmail replaces the primary email of a given user, even if the primary is not yet activated. +func ReplaceInactivePrimaryEmail(ctx context.Context, oldEmail string, email *EmailAddress) error { + user := &User{} + has, err := db.GetEngine(ctx).ID(email.UID).Get(user) + if err != nil { + return err + } else if !has { + return ErrUserNotExist{ + UID: email.UID, + Name: "", + KeyID: 0, + } + } + + err = AddEmailAddress(ctx, email) + if err != nil { + return err + } + + err = makeEmailPrimary(ctx, user, email) + if err != nil { + return err + } + + return DeleteEmailAddress(ctx, &EmailAddress{UID: email.UID, Email: oldEmail}) +} + +// MakeEmailPrimary sets primary email address of given user. +func MakeEmailPrimary(ctx context.Context, email *EmailAddress) error { + has, err := db.GetEngine(ctx).Get(email) + if err != nil { + return err + } else if !has { + return ErrEmailAddressNotExist{Email: email.Email} + } + + if !email.IsActivated { + return ErrEmailNotActivated + } + + user := &User{} + has, err = db.GetEngine(ctx).ID(email.UID).Get(user) + if err != nil { + return err + } else if !has { + return ErrUserNotExist{UID: email.UID} + } + + return makeEmailPrimary(ctx, user, email) +} + // VerifyActiveEmailCode verifies active email code when active account func VerifyActiveEmailCode(ctx context.Context, code, email string) *EmailAddress { minutes := setting.Service.ActiveCodeLives diff --git a/models/user/email_address_test.go b/models/user/email_address_test.go index b20797d700..c2c0378061 100644 --- a/models/user/email_address_test.go +++ b/models/user/email_address_test.go @@ -167,6 +167,28 @@ func TestMakeEmailPrimary(t *testing.T) { assert.Equal(t, "user101@example.com", user.Email) } +func TestReplaceInactivePrimaryEmail(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + email := &user_model.EmailAddress{ + Email: "user9999999@example.com", + UID: 9999999, + } + err := user_model.ReplaceInactivePrimaryEmail(db.DefaultContext, "user10@example.com", email) + assert.Error(t, err) + assert.True(t, user_model.IsErrUserNotExist(err)) + + email = &user_model.EmailAddress{ + Email: "user201@example.com", + UID: 10, + } + err = user_model.ReplaceInactivePrimaryEmail(db.DefaultContext, "user10@example.com", email) + assert.NoError(t, err) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 10}) + assert.Equal(t, "user201@example.com", user.Email) +} + func TestActivate(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index e86c5da3eb..83ff03a33c 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -647,13 +647,22 @@ func Activate(ctx *context.Context) { } // Resend confirmation email. if setting.Service.RegisterEmailConfirm { - if ctx.Cache.IsExist("MailResendLimit_" + ctx.Doer.LowerName) { + var cacheKey string + if ctx.Cache.IsExist("MailChangedJustNow_" + ctx.Doer.LowerName) { + cacheKey = "MailChangedLimit_" + if err := ctx.Cache.Delete("MailChangedJustNow_" + ctx.Doer.LowerName); err != nil { + log.Error("Delete cache(MailChangedJustNow) fail: %v", err) + } + } else { + cacheKey = "MailResendLimit_" + } + if ctx.Cache.IsExist(cacheKey + ctx.Doer.LowerName) { ctx.Data["ResendLimited"] = true } else { ctx.Data["ActiveCodeLives"] = timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale) mailer.SendActivateAccountMail(ctx.Locale, ctx.Doer) - if err := ctx.Cache.Put("MailResendLimit_"+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil { + if err := ctx.Cache.Put(cacheKey+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil { log.Error("Set cache(MailResendLimit) fail: %v", err) } } @@ -687,6 +696,43 @@ func Activate(ctx *context.Context) { func ActivatePost(ctx *context.Context) { code := ctx.FormString("code") if len(code) == 0 { + email := ctx.FormString("email") + if len(email) > 0 { + ctx.Data["IsActivatePage"] = true + if ctx.Doer == nil || ctx.Doer.IsActive { + ctx.NotFound("invalid user", nil) + return + } + // Change the primary email + if setting.Service.RegisterEmailConfirm { + if ctx.Cache.IsExist("MailChangeLimit_" + ctx.Doer.LowerName) { + ctx.Data["ResendLimited"] = true + } else { + ctx.Data["ActiveCodeLives"] = timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale) + err := user_model.ReplaceInactivePrimaryEmail(ctx, ctx.Doer.Email, &user_model.EmailAddress{ + UID: ctx.Doer.ID, + Email: email, + }) + if err != nil { + ctx.Data["IsActivatePage"] = false + log.Error("Couldn't replace inactive primary email of user %d: %v", ctx.Doer.ID, err) + ctx.RenderWithErr(ctx.Tr("auth.change_unconfirmed_email_error", err), TplActivate, nil) + return + } + if err := ctx.Cache.Put("MailChangeLimit_"+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil { + log.Error("Set cache(MailChangeLimit) fail: %v", err) + } + if err := ctx.Cache.Put("MailChangedJustNow_"+ctx.Doer.LowerName, ctx.Doer.LowerName, 180); err != nil { + log.Error("Set cache(MailChangedJustNow) fail: %v", err) + } + + // Confirmation mail will be re-sent after the redirect to `/user/activate` below. + } + } else { + ctx.Data["ServiceNotEnabled"] = true + } + } + ctx.Redirect(setting.AppSubURL + "/user/activate") return } diff --git a/templates/user/auth/activate.tmpl b/templates/user/auth/activate.tmpl index 1b06719753..d8a737e996 100644 --- a/templates/user/auth/activate.tmpl +++ b/templates/user/auth/activate.tmpl @@ -39,6 +39,16 @@ {{else}}

{{ctx.Locale.Tr "auth.has_unconfirmed_mail" (.SignedUser.Name|Escape) (.SignedUser.Email|Escape) | Str2html}}

+
+ {{ctx.Locale.Tr "auth.change_unconfirmed_email_summary"}} + +

{{ctx.Locale.Tr "auth.change_unconfirmed_email"}}

+
+ + +
+
+
diff --git a/tests/integration/signup_test.go b/tests/integration/signup_test.go index f983f98ad8..8b0013419f 100644 --- a/tests/integration/signup_test.go +++ b/tests/integration/signup_test.go @@ -12,6 +12,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/test" "code.gitea.io/gitea/modules/translation" "code.gitea.io/gitea/tests" @@ -91,3 +92,78 @@ func TestSignupEmail(t *testing.T) { } } } + +func TestSignupEmailChangeForInactiveUser(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // Disable the captcha & enable email confirmation for registrations + defer test.MockVariableValue(&setting.Service.EnableCaptcha, false)() + defer test.MockVariableValue(&setting.Service.RegisterEmailConfirm, true)() + + // Create user + req := NewRequestWithValues(t, "POST", "/user/sign_up", map[string]string{ + "user_name": "exampleUserX", + "email": "wrong-email@example.com", + "password": "examplePassword!1", + "retype": "examplePassword!1", + }) + MakeRequest(t, req, http.StatusOK) + + session := loginUserWithPassword(t, "exampleUserX", "examplePassword!1") + + // Verify that the initial e-mail is the wrong one. + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "exampleUserX"}) + assert.Equal(t, "wrong-email@example.com", user.Email) + + // Change the email address + req = NewRequestWithValues(t, "POST", "/user/activate", map[string]string{ + "email": "fine-email@example.com", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + // Verify that the email was updated + user = unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "exampleUserX"}) + assert.Equal(t, "fine-email@example.com", user.Email) + + // Try to change the email again + req = NewRequestWithValues(t, "POST", "/user/activate", map[string]string{ + "email": "wrong-again@example.com", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + // Verify that the email was NOT updated + user = unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "exampleUserX"}) + assert.Equal(t, "fine-email@example.com", user.Email) +} + +func TestSignupEmailChangeForActiveUser(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // Disable the captcha & enable email confirmation for registrations + defer test.MockVariableValue(&setting.Service.EnableCaptcha, false)() + defer test.MockVariableValue(&setting.Service.RegisterEmailConfirm, false)() + + // Create user + req := NewRequestWithValues(t, "POST", "/user/sign_up", map[string]string{ + "user_name": "exampleUserY", + "email": "wrong-email-2@example.com", + "password": "examplePassword!1", + "retype": "examplePassword!1", + }) + MakeRequest(t, req, http.StatusSeeOther) + + session := loginUserWithPassword(t, "exampleUserY", "examplePassword!1") + + // Verify that the initial e-mail is the wrong one. + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "exampleUserY"}) + assert.Equal(t, "wrong-email-2@example.com", user.Email) + + // Changing the email for a validated address is not available + req = NewRequestWithValues(t, "POST", "/user/activate", map[string]string{ + "email": "fine-email-2@example.com", + }) + session.MakeRequest(t, req, http.StatusNotFound) + + // Verify that the email remained unchanged + user = unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "exampleUserY"}) + assert.Equal(t, "wrong-email-2@example.com", user.Email) +}