From 4383da91bdd6979f40730638fe6bbbeb9216765c Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 23 Jul 2024 00:17:06 +0200 Subject: [PATCH] [SECURITY] Notify users about account security changes - Currently if the password, primary mail, TOTP or security keys are changed, no notification is made of that and makes compromising an account a bit easier as it's essentially undetectable until the original person tries to log in. Although other changes should be made as well (re-authing before allowing a password change), this should go a long way of improving the account security in Forgejo. - Adds a mail notification for password and primary mail changes. For the primary mail change, a mail notification is sent to the old primary mail. - Add a mail notification when TOTP or a security keys is removed, if no other 2FA method is configured the mail will also contain that 2FA is no longer needed to log into their account. - `MakeEmailAddressPrimary` is refactored to the user service package, as it now involves calling the mailer service. - Unit tests added. - Integration tests added. --- .deadcode-out | 1 - models/user/email_address.go | 54 ------- models/user/email_address_test.go | 34 ----- models/user/user.go | 17 ++- models/user/user_test.go | 5 + options/locale/locale_en-US.ini | 19 ++- release-notes/4635.md | 1 + routers/web/user/setting/account.go | 10 +- routers/web/user/setting/security/2fa.go | 6 + routers/web/user/setting/security/webauthn.go | 17 +++ services/mailer/mail.go | 143 +++++++++++++++++- services/mailer/mail_admin_new_user_test.go | 6 +- services/mailer/mail_auth_test.go | 60 ++++++++ services/mailer/mail_test.go | 16 +- services/mailer/main_test.go | 4 +- services/user/email.go | 42 ++++- services/user/email_test.go | 13 ++ services/user/update.go | 11 +- templates/mail/auth/2fa_disabled.tmpl | 15 ++ templates/mail/auth/password_change.tmpl | 16 ++ templates/mail/auth/primary_mail_change.tmpl | 14 ++ templates/mail/auth/removed_security_key.tmpl | 15 ++ templates/mail/common/footer_simple.tmpl | 1 + tests/integration/user_test.go | 139 +++++++++++++++++ 24 files changed, 543 insertions(+), 116 deletions(-) create mode 100644 release-notes/4635.md create mode 100644 services/mailer/mail_auth_test.go create mode 100644 templates/mail/auth/2fa_disabled.tmpl create mode 100644 templates/mail/auth/password_change.tmpl create mode 100644 templates/mail/auth/primary_mail_change.tmpl create mode 100644 templates/mail/auth/removed_security_key.tmpl create mode 100644 templates/mail/common/footer_simple.tmpl diff --git a/.deadcode-out b/.deadcode-out index 18beb6ad84..29d3d7c708 100644 --- a/.deadcode-out +++ b/.deadcode-out @@ -30,7 +30,6 @@ code.gitea.io/gitea/models/asymkey code.gitea.io/gitea/models/auth GetSourceByName - GetWebAuthnCredentialByID WebAuthnCredentials code.gitea.io/gitea/models/db diff --git a/models/user/email_address.go b/models/user/email_address.go index 18bf6d0b89..50c0cfbfb1 100644 --- a/models/user/email_address.go +++ b/models/user/email_address.go @@ -307,60 +307,6 @@ func updateActivation(ctx context.Context, email *EmailAddress, activate bool) e return UpdateUserCols(ctx, user, "rands") } -func MakeEmailPrimaryWithUser(ctx context.Context, user *User, email *EmailAddress) error { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - sess := db.GetEngine(ctx) - - // 1. Update user table - user.Email = email.Email - if _, err = sess.ID(user.ID).Cols("email").Update(user); err != nil { - return err - } - - // 2. Update old primary email - if _, err = sess.Where("uid=? AND is_primary=?", email.UID, true).Cols("is_primary").Update(&EmailAddress{ - IsPrimary: false, - }); err != nil { - return err - } - - // 3. update new primary email - email.IsPrimary = true - if _, err = sess.ID(email.ID).Cols("is_primary").Update(email); err != nil { - return err - } - - return committer.Commit() -} - -// 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 MakeEmailPrimaryWithUser(ctx, user, email) -} - // VerifyActiveEmailCode verifies active email code when active account func VerifyActiveEmailCode(ctx context.Context, code, email string) *EmailAddress { if user := GetVerifyUser(ctx, code); user != nil { diff --git a/models/user/email_address_test.go b/models/user/email_address_test.go index 65befa5660..94680b6bd5 100644 --- a/models/user/email_address_test.go +++ b/models/user/email_address_test.go @@ -43,40 +43,6 @@ func TestIsEmailUsed(t *testing.T) { assert.False(t, isExist) } -func TestMakeEmailPrimary(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - - email := &user_model.EmailAddress{ - Email: "user567890@example.com", - } - err := user_model.MakeEmailPrimary(db.DefaultContext, email) - assert.Error(t, err) - assert.EqualError(t, err, user_model.ErrEmailAddressNotExist{Email: email.Email}.Error()) - - email = &user_model.EmailAddress{ - Email: "user11@example.com", - } - err = user_model.MakeEmailPrimary(db.DefaultContext, email) - assert.Error(t, err) - assert.EqualError(t, err, user_model.ErrEmailNotActivated.Error()) - - email = &user_model.EmailAddress{ - Email: "user9999999@example.com", - } - err = user_model.MakeEmailPrimary(db.DefaultContext, email) - assert.Error(t, err) - assert.True(t, user_model.IsErrUserNotExist(err)) - - email = &user_model.EmailAddress{ - Email: "user101@example.com", - } - err = user_model.MakeEmailPrimary(db.DefaultContext, email) - assert.NoError(t, err) - - user, _ := user_model.GetUserByID(db.DefaultContext, int64(10)) - assert.Equal(t, "user101@example.com", user.Email) -} - func TestActivate(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) diff --git a/models/user/user.go b/models/user/user.go index 69cd85a207..f6d649eaf3 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -451,17 +451,22 @@ var emailToReplacer = strings.NewReplacer( ) // EmailTo returns a string suitable to be put into a e-mail `To:` header. -func (u *User) EmailTo() string { +func (u *User) EmailTo(overrideMail ...string) string { sanitizedDisplayName := emailToReplacer.Replace(u.DisplayName()) - // should be an edge case but nice to have - if sanitizedDisplayName == u.Email { - return u.Email + email := u.Email + if len(overrideMail) > 0 { + email = overrideMail[0] } - address, err := mail.ParseAddress(fmt.Sprintf("%s <%s>", sanitizedDisplayName, u.Email)) + // should be an edge case but nice to have + if sanitizedDisplayName == email { + return email + } + + address, err := mail.ParseAddress(fmt.Sprintf("%s <%s>", sanitizedDisplayName, email)) if err != nil { - return u.Email + return email } return address.String() diff --git a/models/user/user_test.go b/models/user/user_test.go index bf3fc801ce..abeff078c5 100644 --- a/models/user/user_test.go +++ b/models/user/user_test.go @@ -625,6 +625,11 @@ func TestEmailTo(t *testing.T) { assert.EqualValues(t, testCase.result, testUser.EmailTo()) }) } + + t.Run("Override user's email", func(t *testing.T) { + testUser := &user_model.User{FullName: "Christine Jorgensen", Email: "christine@test.com"} + assert.EqualValues(t, `"Christine Jorgensen" `, testUser.EmailTo("christine@example.org")) + }) } func TestDisabledUserFeatures(t *testing.T) { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 2c34025bef..5a7c182b34 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -498,7 +498,24 @@ register_notify.text_2 = You can sign into your account using your username: %s register_notify.text_3 = If someone else made this account for you, you will need to set your password first. reset_password = Recover your account -reset_password.text = If this was you, please click the following link to recover your account within %s: +reset_password.text_1 = The password for your account was just changed. + +password_change.subject = Your password has been changed +password_change.text_1 = The password for your account was just changed. + +primary_mail_change.subject = Your primary mail has been changed +primary_mail_change.text_1 = The primary mail of your account was just changed to %[1]s. This means that this e-mail address will no longer receive e-mail notifications for your account. + +totp_disabled.subject = TOTP has been disabled +totp_disabled.text_1 = Time-based one-time password (TOTP) on your account was just disabled. +totp_disabled.no_2fa = There are no other 2FA methods configured anymore, meaning it is no longer necessary to log into your account with 2FA. + +removed_security_key.subject = A security key has been removed +removed_security_key.text_1 = Security key "%[1]s" has just been removed from your account. +removed_security_key.no_2fa = There are no other 2FA methods configured anymore, meaning it is no longer necessary to log into your account with 2FA. + +account_security_caution.text_1 = If this was you, then you can safely ignore this mail. +account_security_caution.text_2 = If this wasn't you, your account is compromised. Please contact the admins of this site. register_success = Registration successful diff --git a/release-notes/4635.md b/release-notes/4635.md new file mode 100644 index 0000000000..42ace0c2f9 --- /dev/null +++ b/release-notes/4635.md @@ -0,0 +1 @@ +Email notifications are now sent when account security changes are made: password changed, primary email changed (email sent to old primary mail), TOTP disabled or a security key removed. diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go index 795ee59d58..d2d2d5bc4c 100644 --- a/routers/web/user/setting/account.go +++ b/routers/web/user/setting/account.go @@ -104,7 +104,15 @@ func EmailPost(ctx *context.Context) { // Make emailaddress primary. if ctx.FormString("_method") == "PRIMARY" { - if err := user_model.MakeEmailPrimary(ctx, &user_model.EmailAddress{ID: ctx.FormInt64("id")}); err != nil { + id := ctx.FormInt64("id") + email, err := user_model.GetEmailAddressByID(ctx, ctx.Doer.ID, id) + if err != nil { + log.Error("GetEmailAddressByID(%d,%d) error: %v", ctx.Doer.ID, id, err) + ctx.Redirect(setting.AppSubURL + "/user/settings/account") + return + } + + if err := user.MakeEmailAddressPrimary(ctx, ctx.Doer, email, true); err != nil { ctx.ServerError("MakeEmailPrimary", err) return } diff --git a/routers/web/user/setting/security/2fa.go b/routers/web/user/setting/security/2fa.go index cd09102369..1b1d385af8 100644 --- a/routers/web/user/setting/security/2fa.go +++ b/routers/web/user/setting/security/2fa.go @@ -18,6 +18,7 @@ import ( "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/forms" + "code.gitea.io/gitea/services/mailer" "github.com/pquerna/otp" "github.com/pquerna/otp/totp" @@ -78,6 +79,11 @@ func DisableTwoFactor(ctx *context.Context) { return } + if err := mailer.SendDisabledTOTP(ctx, ctx.Doer); err != nil { + ctx.ServerError("SendDisabledTOTP", err) + return + } + ctx.Flash.Success(ctx.Tr("settings.twofa_disabled")) ctx.Redirect(setting.AppSubURL + "/user/settings/security") } diff --git a/routers/web/user/setting/security/webauthn.go b/routers/web/user/setting/security/webauthn.go index e382c8b9af..bfbc06c701 100644 --- a/routers/web/user/setting/security/webauthn.go +++ b/routers/web/user/setting/security/webauthn.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/forms" + "code.gitea.io/gitea/services/mailer" "github.com/go-webauthn/webauthn/protocol" "github.com/go-webauthn/webauthn/webauthn" @@ -112,9 +113,25 @@ func WebauthnRegisterPost(ctx *context.Context) { // WebauthnDelete deletes an security key by id func WebauthnDelete(ctx *context.Context) { form := web.GetForm(ctx).(*forms.WebauthnDeleteForm) + cred, err := auth.GetWebAuthnCredentialByID(ctx, form.ID) + if err != nil || cred.UserID != ctx.Doer.ID { + if err != nil && !auth.IsErrWebAuthnCredentialNotExist(err) { + log.Error("GetWebAuthnCredentialByID: %v", err) + } + + ctx.JSONRedirect(setting.AppSubURL + "/user/settings/security") + return + } + if _, err := auth.DeleteCredential(ctx, form.ID, ctx.Doer.ID); err != nil { ctx.ServerError("GetWebAuthnCredentialByID", err) return } + + if err := mailer.SendRemovedSecurityKey(ctx, ctx.Doer, cred.Name); err != nil { + ctx.ServerError("SendRemovedSecurityKey", err) + return + } + ctx.JSONRedirect(setting.AppSubURL + "/user/settings/security") } diff --git a/services/mailer/mail.go b/services/mailer/mail.go index 38e292e6a1..87df3d1397 100644 --- a/services/mailer/mail.go +++ b/services/mailer/mail.go @@ -17,6 +17,7 @@ import ( "time" activities_model "code.gitea.io/gitea/models/activities" + auth_model "code.gitea.io/gitea/models/auth" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" @@ -35,10 +36,14 @@ import ( ) const ( - mailAuthActivate base.TplName = "auth/activate" - mailAuthActivateEmail base.TplName = "auth/activate_email" - mailAuthResetPassword base.TplName = "auth/reset_passwd" - mailAuthRegisterNotify base.TplName = "auth/register_notify" + mailAuthActivate base.TplName = "auth/activate" + mailAuthActivateEmail base.TplName = "auth/activate_email" + mailAuthResetPassword base.TplName = "auth/reset_passwd" + mailAuthRegisterNotify base.TplName = "auth/register_notify" + mailAuthPasswordChange base.TplName = "auth/password_change" + mailAuthPrimaryMailChange base.TplName = "auth/primary_mail_change" + mailAuth2faDisabled base.TplName = "auth/2fa_disabled" + mailAuthRemovedSecurityKey base.TplName = "auth/removed_security_key" mailNotifyCollaborator base.TplName = "notify/collaborator" @@ -561,3 +566,133 @@ func fromDisplayName(u *user_model.User) string { } return u.GetCompleteName() } + +// SendPasswordChange informs the user on their primary email address that +// their password was changed. +func SendPasswordChange(u *user_model.User) error { + if setting.MailService == nil { + return nil + } + locale := translation.NewLocale(u.Language) + + data := map[string]any{ + "locale": locale, + "DisplayName": u.DisplayName(), + "Username": u.Name, + "Language": locale.Language(), + } + + var content bytes.Buffer + + if err := bodyTemplates.ExecuteTemplate(&content, string(mailAuthPasswordChange), data); err != nil { + return err + } + + msg := NewMessage(u.EmailTo(), locale.TrString("mail.password_change.subject"), content.String()) + msg.Info = fmt.Sprintf("UID: %d, password change notification", u.ID) + + SendAsync(msg) + return nil +} + +// SendPrimaryMailChange informs the user on their old primary email address +// that it's no longer used as primary mail and will no longer receive +// notification on that email address. +func SendPrimaryMailChange(u *user_model.User, oldPrimaryEmail string) error { + if setting.MailService == nil { + return nil + } + locale := translation.NewLocale(u.Language) + + data := map[string]any{ + "locale": locale, + "NewPrimaryMail": u.Email, + "DisplayName": u.DisplayName(), + "Username": u.Name, + "Language": locale.Language(), + } + + var content bytes.Buffer + + if err := bodyTemplates.ExecuteTemplate(&content, string(mailAuthPrimaryMailChange), data); err != nil { + return err + } + + msg := NewMessage(u.EmailTo(oldPrimaryEmail), locale.TrString("mail.primary_mail_change.subject"), content.String()) + msg.Info = fmt.Sprintf("UID: %d, primary email change notification", u.ID) + + SendAsync(msg) + return nil +} + +// SendDisabledTOTP informs the user that their totp has been disabled. +func SendDisabledTOTP(ctx context.Context, u *user_model.User) error { + if setting.MailService == nil { + return nil + } + locale := translation.NewLocale(u.Language) + + hasWebAuthn, err := auth_model.HasWebAuthnRegistrationsByUID(ctx, u.ID) + if err != nil { + return err + } + + data := map[string]any{ + "locale": locale, + "HasWebAuthn": hasWebAuthn, + "DisplayName": u.DisplayName(), + "Username": u.Name, + "Language": locale.Language(), + } + + var content bytes.Buffer + + if err := bodyTemplates.ExecuteTemplate(&content, string(mailAuth2faDisabled), data); err != nil { + return err + } + + msg := NewMessage(u.EmailTo(), locale.TrString("mail.totp_disabled.subject"), content.String()) + msg.Info = fmt.Sprintf("UID: %d, 2fa disabled notification", u.ID) + + SendAsync(msg) + return nil +} + +// SendRemovedWebAuthn informs the user that one of their security keys has been removed. +func SendRemovedSecurityKey(ctx context.Context, u *user_model.User, securityKeyName string) error { + if setting.MailService == nil { + return nil + } + locale := translation.NewLocale(u.Language) + + hasWebAuthn, err := auth_model.HasWebAuthnRegistrationsByUID(ctx, u.ID) + if err != nil { + return err + } + hasTOTP, err := auth_model.HasTwoFactorByUID(ctx, u.ID) + if err != nil { + return err + } + + data := map[string]any{ + "locale": locale, + "HasWebAuthn": hasWebAuthn, + "HasTOTP": hasTOTP, + "SecurityKeyName": securityKeyName, + "DisplayName": u.DisplayName(), + "Username": u.Name, + "Language": locale.Language(), + } + + var content bytes.Buffer + + if err := bodyTemplates.ExecuteTemplate(&content, string(mailAuthRemovedSecurityKey), data); err != nil { + return err + } + + msg := NewMessage(u.EmailTo(), locale.TrString("mail.removed_security_key.subject"), content.String()) + msg.Info = fmt.Sprintf("UID: %d, security key removed notification", u.ID) + + SendAsync(msg) + return nil +} diff --git a/services/mailer/mail_admin_new_user_test.go b/services/mailer/mail_admin_new_user_test.go index 603a8b95c9..6868e9eb20 100644 --- a/services/mailer/mail_admin_new_user_test.go +++ b/services/mailer/mail_admin_new_user_test.go @@ -55,14 +55,14 @@ func TestAdminNotificationMail_test(t *testing.T) { defer test.MockVariableValue(&setting.Admin.SendNotificationEmailOnNewUser, true)() called := false - defer mockMailSettings(func(msgs ...*Message) { + defer MockMailSettings(func(msgs ...*Message) { assert.Equal(t, len(msgs), 1, "Test provides only one admin user, so only one email must be sent") assert.Equal(t, msgs[0].To, users[0].Email, "checks if the recipient is the admin of the instance") manageUserURL := setting.AppURL + "admin/users/" + strconv.FormatInt(users[1].ID, 10) assert.Contains(t, msgs[0].Body, manageUserURL) assert.Contains(t, msgs[0].Body, users[1].HTMLURL()) assert.Contains(t, msgs[0].Body, users[1].Name, "user name of the newly created user") - assertTranslatedLocale(t, msgs[0].Body, "mail.admin", "admin.users") + AssertTranslatedLocale(t, msgs[0].Body, "mail.admin", "admin.users") called = true })() MailNewUser(ctx, users[1]) @@ -71,7 +71,7 @@ func TestAdminNotificationMail_test(t *testing.T) { t.Run("SendNotificationEmailOnNewUser_false", func(t *testing.T) { defer test.MockVariableValue(&setting.Admin.SendNotificationEmailOnNewUser, false)() - defer mockMailSettings(func(msgs ...*Message) { + defer MockMailSettings(func(msgs ...*Message) { assert.Equal(t, 1, 0, "this shouldn't execute. MailNewUser must exit early since SEND_NOTIFICATION_EMAIL_ON_NEW_USER is disabled") })() MailNewUser(ctx, users[1]) diff --git a/services/mailer/mail_auth_test.go b/services/mailer/mail_auth_test.go new file mode 100644 index 0000000000..5346ae0d49 --- /dev/null +++ b/services/mailer/mail_auth_test.go @@ -0,0 +1,60 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package mailer_test + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/optional" + "code.gitea.io/gitea/modules/translation" + "code.gitea.io/gitea/services/mailer" + user_service "code.gitea.io/gitea/services/user" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPasswordChangeMail(t *testing.T) { + defer require.NoError(t, unittest.PrepareTestDatabase()) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + called := false + defer mailer.MockMailSettings(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.password_change.subject"), msgs[0].Subject) + mailer.AssertTranslatedLocale(t, msgs[0].Body, "mail.password_change.text_1", "mail.password_change.text_2", "mail.password_change.text_3") + called = true + })() + + require.NoError(t, user_service.UpdateAuth(db.DefaultContext, user, &user_service.UpdateAuthOptions{Password: optional.Some("NewPasswordYolo!")})) + assert.True(t, called) +} + +func TestPrimaryMailChange(t *testing.T) { + defer require.NoError(t, unittest.PrepareTestDatabase()) + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + firstEmail := unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{ID: 3, UID: user.ID, IsPrimary: true}) + secondEmail := unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{ID: 35, UID: user.ID}, "is_primary = false") + + called := false + defer mailer.MockMailSettings(func(msgs ...*mailer.Message) { + assert.False(t, called) + assert.Len(t, msgs, 1) + assert.Equal(t, user.EmailTo(firstEmail.Email), msgs[0].To) + assert.EqualValues(t, translation.NewLocale("en-US").Tr("mail.primary_mail_change.subject"), msgs[0].Subject) + assert.Contains(t, msgs[0].Body, secondEmail.Email) + mailer.AssertTranslatedLocale(t, msgs[0].Body, "mail.primary_mail_change.text_1", "mail.primary_mail_change.text_2", "mail.primary_mail_change.text_3") + called = true + })() + + require.NoError(t, user_service.MakeEmailAddressPrimary(db.DefaultContext, user, secondEmail, true)) + assert.True(t, called) + + require.NoError(t, user_service.MakeEmailAddressPrimary(db.DefaultContext, user, firstEmail, false)) +} diff --git a/services/mailer/mail_test.go b/services/mailer/mail_test.go index 54b9c177e8..78798bb9f9 100644 --- a/services/mailer/mail_test.go +++ b/services/mailer/mail_test.go @@ -62,7 +62,7 @@ func prepareMailerTest(t *testing.T) (doer *user_model.User, repo *repo_model.Re } func TestComposeIssueCommentMessage(t *testing.T) { - defer mockMailSettings(nil)() + defer MockMailSettings(nil)() doer, _, issue, comment := prepareMailerTest(t) markup.Init(&markup.ProcessorHelper{ @@ -117,7 +117,7 @@ func TestComposeIssueCommentMessage(t *testing.T) { } func TestComposeIssueMessage(t *testing.T) { - defer mockMailSettings(nil)() + defer MockMailSettings(nil)() doer, _, issue, _ := prepareMailerTest(t) recipients := []*user_model.User{{Name: "Test", Email: "test@gitea.com"}, {Name: "Test2", Email: "test2@gitea.com"}} @@ -146,7 +146,7 @@ func TestComposeIssueMessage(t *testing.T) { } func TestMailerIssueTemplate(t *testing.T) { - defer mockMailSettings(nil)() + defer MockMailSettings(nil)() assert.NoError(t, unittest.PrepareTestDatabase()) doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) @@ -160,7 +160,7 @@ func TestMailerIssueTemplate(t *testing.T) { for _, s := range expected { assert.Contains(t, wholemsg, s) } - assertTranslatedLocale(t, wholemsg, "mail.issue") + AssertTranslatedLocale(t, wholemsg, "mail.issue") } testCompose := func(t *testing.T, ctx *mailCommentContext) *Message { @@ -241,7 +241,7 @@ func TestMailerIssueTemplate(t *testing.T) { } func TestTemplateSelection(t *testing.T) { - defer mockMailSettings(nil)() + defer MockMailSettings(nil)() doer, repo, issue, comment := prepareMailerTest(t) recipients := []*user_model.User{{Name: "Test", Email: "test@gitea.com"}} @@ -296,7 +296,7 @@ func TestTemplateSelection(t *testing.T) { } func TestTemplateServices(t *testing.T) { - defer mockMailSettings(nil)() + defer MockMailSettings(nil)() doer, _, issue, comment := prepareMailerTest(t) assert.NoError(t, issue.LoadRepo(db.DefaultContext)) @@ -349,7 +349,7 @@ func testComposeIssueCommentMessage(t *testing.T, ctx *mailCommentContext, recip } func TestGenerateAdditionalHeaders(t *testing.T) { - defer mockMailSettings(nil)() + defer MockMailSettings(nil)() doer, _, issue, _ := prepareMailerTest(t) ctx := &mailCommentContext{Context: context.TODO() /* TODO: use a correct context */, Issue: issue, Doer: doer} @@ -382,7 +382,7 @@ func TestGenerateAdditionalHeaders(t *testing.T) { } func Test_createReference(t *testing.T) { - defer mockMailSettings(nil)() + defer MockMailSettings(nil)() _, _, issue, comment := prepareMailerTest(t) _, _, pullIssue, _ := prepareMailerTest(t) pullIssue.IsPull = true diff --git a/services/mailer/main_test.go b/services/mailer/main_test.go index 399d05ac7b..908976e7ef 100644 --- a/services/mailer/main_test.go +++ b/services/mailer/main_test.go @@ -22,14 +22,14 @@ func TestMain(m *testing.M) { unittest.MainTest(m) } -func assertTranslatedLocale(t *testing.T, message string, prefixes ...string) { +func AssertTranslatedLocale(t *testing.T, message string, prefixes ...string) { t.Helper() for _, prefix := range prefixes { assert.NotContains(t, message, prefix, "there is an untranslated locale prefix") } } -func mockMailSettings(send func(msgs ...*Message)) func() { +func MockMailSettings(send func(msgs ...*Message)) func() { translation.InitLocales(context.Background()) subjectTemplates, bodyTemplates = templates.Mailer(context.Background()) mailService := setting.Mailer{ diff --git a/services/user/email.go b/services/user/email.go index 9dc5270842..e8725267f4 100644 --- a/services/user/email.go +++ b/services/user/email.go @@ -12,6 +12,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/services/mailer" ) // AdminAddOrSetPrimaryEmailAddress is used by admins to add or set a user's primary email address @@ -163,7 +164,7 @@ func ReplaceInactivePrimaryEmail(ctx context.Context, oldEmail string, email *us return err } - err = user_model.MakeEmailPrimaryWithUser(ctx, user, email) + err = MakeEmailAddressPrimary(ctx, user, email, false) if err != nil { return err } @@ -190,3 +191,42 @@ func DeleteEmailAddresses(ctx context.Context, u *user_model.User, emails []stri return nil } + +func MakeEmailAddressPrimary(ctx context.Context, u *user_model.User, newPrimaryEmail *user_model.EmailAddress, notify bool) error { + ctx, committer, err := db.TxContext(ctx) + if err != nil { + return err + } + defer committer.Close() + sess := db.GetEngine(ctx) + + oldPrimaryEmail := u.Email + + // 1. Update user table + u.Email = newPrimaryEmail.Email + if _, err = sess.ID(u.ID).Cols("email").Update(u); err != nil { + return err + } + + // 2. Update old primary email + if _, err = sess.Where("uid=? AND is_primary=?", u.ID, true).Cols("is_primary").Update(&user_model.EmailAddress{ + IsPrimary: false, + }); err != nil { + return err + } + + // 3. update new primary email + newPrimaryEmail.IsPrimary = true + if _, err = sess.ID(newPrimaryEmail.ID).Cols("is_primary").Update(newPrimaryEmail); err != nil { + return err + } + + if err := committer.Commit(); err != nil { + return err + } + + if notify { + return mailer.SendPrimaryMailChange(u, oldPrimaryEmail) + } + return nil +} diff --git a/services/user/email_test.go b/services/user/email_test.go index 0784b4f803..74f8b8d010 100644 --- a/services/user/email_test.go +++ b/services/user/email_test.go @@ -14,6 +14,7 @@ import ( "github.com/gobwas/glob" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAdminAddOrSetPrimaryEmailAddress(t *testing.T) { @@ -163,3 +164,15 @@ func TestDeleteEmailAddresses(t *testing.T) { assert.Error(t, err) assert.True(t, user_model.IsErrPrimaryEmailCannotDelete(err)) } + +func TestMakeEmailAddressPrimary(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + newPrimaryEmail := unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{ID: 35, UID: user.ID}, "is_primary = false") + + require.NoError(t, MakeEmailAddressPrimary(db.DefaultContext, user, newPrimaryEmail, false)) + + unittest.AssertExistsIf(t, true, &user_model.User{ID: 2, Email: newPrimaryEmail.Email}) + unittest.AssertExistsIf(t, true, &user_model.EmailAddress{ID: 3, UID: user.ID}, "is_primary = false") + unittest.AssertExistsIf(t, true, &user_model.EmailAddress{ID: 35, UID: user.ID, IsPrimary: true}) +} diff --git a/services/user/update.go b/services/user/update.go index 1bdbf13f0d..26c90505c8 100644 --- a/services/user/update.go +++ b/services/user/update.go @@ -14,6 +14,7 @@ import ( "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/services/mailer" ) type UpdateOptions struct { @@ -220,5 +221,13 @@ func UpdateAuth(ctx context.Context, u *user_model.User, opts *UpdateAuthOptions u.ProhibitLogin = opts.ProhibitLogin.Value() } - return user_model.UpdateUserCols(ctx, u, "login_type", "login_source", "login_name", "passwd", "passwd_hash_algo", "salt", "must_change_password", "prohibit_login") + if err := user_model.UpdateUserCols(ctx, u, "login_type", "login_source", "login_name", "passwd", "passwd_hash_algo", "salt", "must_change_password", "prohibit_login"); err != nil { + return err + } + + if opts.Password.Has() { + return mailer.SendPasswordChange(u) + } + + return nil } diff --git a/templates/mail/auth/2fa_disabled.tmpl b/templates/mail/auth/2fa_disabled.tmpl new file mode 100644 index 0000000000..3f9d3795c0 --- /dev/null +++ b/templates/mail/auth/2fa_disabled.tmpl @@ -0,0 +1,15 @@ + + + + + + +

{{.locale.Tr "mail.hi_user_x" (.DisplayName|DotEscape)}}


+

{{.locale.Tr "mail.totp_disabled.text_1"}}


+ {{if not .HasWebAuthn}}

{{.locale.Tr "mail.totp_disabled.no_2fa"}}


{{end}} +

{{.locale.Tr "mail.account_security_caution.text_1"}}


+

{{.locale.Tr "mail.account_security_caution.text_2"}}


+ + {{template "common/footer_simple" .}} + + diff --git a/templates/mail/auth/password_change.tmpl b/templates/mail/auth/password_change.tmpl new file mode 100644 index 0000000000..4366b8d720 --- /dev/null +++ b/templates/mail/auth/password_change.tmpl @@ -0,0 +1,16 @@ + + + + + + + + +

{{.locale.Tr "mail.hi_user_x" (.DisplayName|DotEscape)}}


+

{{.locale.Tr "mail.password_change.text_1"}}


+

{{.locale.Tr "mail.account_security_caution.text_1"}}


+

{{.locale.Tr "mail.account_security_caution.text_2"}}


+ + {{template "common/footer_simple" .}} + + diff --git a/templates/mail/auth/primary_mail_change.tmpl b/templates/mail/auth/primary_mail_change.tmpl new file mode 100644 index 0000000000..d17be19886 --- /dev/null +++ b/templates/mail/auth/primary_mail_change.tmpl @@ -0,0 +1,14 @@ + + + + + + +

{{.locale.Tr "mail.hi_user_x" (.DisplayName|DotEscape)}}


+

{{.locale.Tr "mail.primary_mail_change.text_1" .NewPrimaryMail}}


+

{{.locale.Tr "mail.account_security_caution.text_1"}}


+

{{.locale.Tr "mail.account_security_caution.text_2"}}


+ + {{template "common/footer_simple" .}} + + diff --git a/templates/mail/auth/removed_security_key.tmpl b/templates/mail/auth/removed_security_key.tmpl new file mode 100644 index 0000000000..18ae18725e --- /dev/null +++ b/templates/mail/auth/removed_security_key.tmpl @@ -0,0 +1,15 @@ + + + + + + +

{{.locale.Tr "mail.hi_user_x" (.DisplayName|DotEscape)}}


+

{{.locale.Tr "mail.removed_security_key.text_1" .SecurityKeyName}}


+ {{if and (not .HasWebAuthn) (not .HasTOTP)}}

{{.locale.Tr "mail.removed_security_key.no_2fa"}}


{{end}} +

{{.locale.Tr "mail.account_security_caution.text_1"}}


+

{{.locale.Tr "mail.account_security_caution.text_2"}}


+ + {{template "common/footer_simple" .}} + + diff --git a/templates/mail/common/footer_simple.tmpl b/templates/mail/common/footer_simple.tmpl new file mode 100644 index 0000000000..9011d69d00 --- /dev/null +++ b/templates/mail/common/footer_simple.tmpl @@ -0,0 +1 @@ +

{{AppName}}

diff --git a/tests/integration/user_test.go b/tests/integration/user_test.go index 20b1cdf975..91de3ece31 100644 --- a/tests/integration/user_test.go +++ b/tests/integration/user_test.go @@ -7,6 +7,7 @@ package integration import ( "fmt" "net/http" + "strconv" "strings" "testing" @@ -20,6 +21,7 @@ import ( api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/translation" + "code.gitea.io/gitea/services/mailer" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -608,3 +610,140 @@ func TestUserPronouns(t *testing.T) { assert.EqualValues(t, userName, "user2") }) } + +func TestUserTOTPMail(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + session := loginUser(t, user.Name) + + t.Run("No security keys", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + 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.totp_disabled.subject"), msgs[0].Subject) + assert.Contains(t, msgs[0].Body, translation.NewLocale("en-US").Tr("mail.totp_disabled.no_2fa")) + called = true + })() + + unittest.AssertSuccessfulInsert(t, &auth_model.TwoFactor{UID: user.ID}) + req := NewRequestWithValues(t, "POST", "/user/settings/security/two_factor/disable", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/settings/security"), + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + assert.True(t, called) + unittest.AssertExistsIf(t, false, &auth_model.TwoFactor{UID: user.ID}) + }) + + t.Run("with security keys", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + 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.totp_disabled.subject"), msgs[0].Subject) + assert.NotContains(t, msgs[0].Body, translation.NewLocale("en-US").Tr("mail.totp_disabled.no_2fa")) + called = true + })() + + unittest.AssertSuccessfulInsert(t, &auth_model.TwoFactor{UID: user.ID}) + unittest.AssertSuccessfulInsert(t, &auth_model.WebAuthnCredential{UserID: user.ID}) + req := NewRequestWithValues(t, "POST", "/user/settings/security/two_factor/disable", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/settings/security"), + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + assert.True(t, called) + unittest.AssertExistsIf(t, false, &auth_model.TwoFactor{UID: user.ID}) + }) +} + +func TestUserSecurityKeyMail(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + session := loginUser(t, user.Name) + + t.Run("Normal", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + 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.removed_security_key.subject"), msgs[0].Subject) + assert.Contains(t, msgs[0].Body, translation.NewLocale("en-US").Tr("mail.removed_security_key.no_2fa")) + assert.Contains(t, msgs[0].Body, "Little Bobby Tables's primary key") + called = true + })() + + unittest.AssertSuccessfulInsert(t, &auth_model.WebAuthnCredential{UserID: user.ID, Name: "Little Bobby Tables's primary key"}) + id := unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{UserID: user.ID}).ID + req := NewRequestWithValues(t, "POST", "/user/settings/security/webauthn/delete", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/settings/security"), + "id": strconv.FormatInt(id, 10), + }) + session.MakeRequest(t, req, http.StatusOK) + + assert.True(t, called) + unittest.AssertExistsIf(t, false, &auth_model.WebAuthnCredential{UserID: user.ID}) + }) + + t.Run("With TOTP", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + 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.removed_security_key.subject"), msgs[0].Subject) + assert.NotContains(t, msgs[0].Body, translation.NewLocale("en-US").Tr("mail.removed_security_key.no_2fa")) + assert.Contains(t, msgs[0].Body, "Little Bobby Tables's primary key") + called = true + })() + + unittest.AssertSuccessfulInsert(t, &auth_model.WebAuthnCredential{UserID: user.ID, Name: "Little Bobby Tables's primary key"}) + id := unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{UserID: user.ID}).ID + unittest.AssertSuccessfulInsert(t, &auth_model.TwoFactor{UID: user.ID}) + req := NewRequestWithValues(t, "POST", "/user/settings/security/webauthn/delete", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/settings/security"), + "id": strconv.FormatInt(id, 10), + }) + session.MakeRequest(t, req, http.StatusOK) + + assert.True(t, called) + unittest.AssertExistsIf(t, false, &auth_model.WebAuthnCredential{UserID: user.ID}) + }) + + t.Run("Two security keys", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + 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.removed_security_key.subject"), msgs[0].Subject) + assert.NotContains(t, msgs[0].Body, translation.NewLocale("en-US").Tr("mail.removed_security_key.no_2fa")) + assert.Contains(t, msgs[0].Body, "Little Bobby Tables's primary key") + called = true + })() + + unittest.AssertSuccessfulInsert(t, &auth_model.WebAuthnCredential{UserID: user.ID, Name: "Little Bobby Tables's primary key"}) + id := unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{UserID: user.ID}).ID + unittest.AssertSuccessfulInsert(t, &auth_model.WebAuthnCredential{UserID: user.ID, Name: "Little Bobby Tables's evil key"}) + req := NewRequestWithValues(t, "POST", "/user/settings/security/webauthn/delete", map[string]string{ + "_csrf": GetCSRF(t, session, "/user/settings/security"), + "id": strconv.FormatInt(id, 10), + }) + session.MakeRequest(t, req, http.StatusOK) + + assert.True(t, called) + unittest.AssertExistsIf(t, false, &auth_model.WebAuthnCredential{UserID: user.ID, Name: "Little Bobby Tables's primary key"}) + unittest.AssertExistsIf(t, true, &auth_model.WebAuthnCredential{UserID: user.ID, Name: "Little Bobby Tables's evil key"}) + }) +}