Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to change primary email before account activation #29412

Merged
merged 3 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 29 additions & 17 deletions models/user/email_address.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ import (
"xorm.io/builder"
)

// ErrEmailNotActivated e-mail address has not been activated error
var ErrEmailNotActivated = util.NewInvalidArgumentErrorf("e-mail address has not been activated")

// ErrEmailCharIsNotSupported e-mail address contains unsupported character
type ErrEmailCharIsNotSupported struct {
Email string
Expand Down Expand Up @@ -313,27 +310,27 @@ 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 {
func MakeActiveEmailPrimary(ctx context.Context, emailID int64) error {
return makeEmailPrimaryInternal(ctx, emailID, true)
}

func MakeInactiveEmailPrimary(ctx context.Context, emailID int64) error {
return makeEmailPrimaryInternal(ctx, emailID, false)
}

func makeEmailPrimaryInternal(ctx context.Context, emailID int64, isActive bool) error {
email := &EmailAddress{}
if has, err := db.GetEngine(ctx).ID(emailID).Where(builder.Eq{"is_activated": isActive}).Get(email); err != nil {
return err
} else if !has {
return ErrEmailAddressNotExist{Email: email.Email}
}

if !email.IsActivated {
return ErrEmailNotActivated
return ErrEmailAddressNotExist{}
}

user := &User{}
has, err = db.GetEngine(ctx).ID(email.UID).Get(user)
if err != nil {
if has, err := db.GetEngine(ctx).ID(email.UID).Get(user); err != nil {
return err
} else if !has {
return ErrUserNotExist{
UID: email.UID,
}
return ErrUserNotExist{UID: email.UID}
}

ctx, committer, err := db.TxContext(ctx)
Expand Down Expand Up @@ -365,6 +362,21 @@ func MakeEmailPrimary(ctx context.Context, email *EmailAddress) error {
return committer.Commit()
}

// ChangeInactivePrimaryEmail replaces the inactive primary email of a given user
func ChangeInactivePrimaryEmail(ctx context.Context, uid int64, oldEmailAddr, newEmailAddr string) error {
return db.WithTx(ctx, func(ctx context.Context) error {
_, err := db.GetEngine(ctx).Where(builder.Eq{"uid": uid, "lower_email": strings.ToLower(oldEmailAddr)}).Delete(&EmailAddress{})
if err != nil {
return err
}
newEmail, err := InsertEmailAddress(ctx, &EmailAddress{UID: uid, Email: newEmailAddr})
if err != nil {
return err
}
return MakeInactiveEmailPrimary(ctx, newEmail.ID)
})
}

// VerifyActiveEmailCode verifies active email code when active account
func VerifyActiveEmailCode(ctx context.Context, code, email string) *EmailAddress {
minutes := setting.Service.ActiveCodeLives
Expand Down
27 changes: 9 additions & 18 deletions models/user/email_address_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,31 +45,22 @@ func TestIsEmailUsed(t *testing.T) {
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)
err := user_model.MakeActiveEmailPrimary(db.DefaultContext, 9999999)
assert.Error(t, err)
assert.EqualError(t, err, user_model.ErrEmailAddressNotExist{Email: email.Email}.Error())
assert.ErrorIs(t, err, user_model.ErrEmailAddressNotExist{})

email = &user_model.EmailAddress{
Email: "user11@example.com",
}
err = user_model.MakeEmailPrimary(db.DefaultContext, email)
email := unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{Email: "user11@example.com"})
err = user_model.MakeActiveEmailPrimary(db.DefaultContext, email.ID)
assert.Error(t, err)
assert.EqualError(t, err, user_model.ErrEmailNotActivated.Error())
assert.ErrorIs(t, err, user_model.ErrEmailAddressNotExist{}) // inactive email is considered as not exist for "MakeActiveEmailPrimary"

email = &user_model.EmailAddress{
Email: "user9999999@example.com",
}
err = user_model.MakeEmailPrimary(db.DefaultContext, email)
email = unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{Email: "user9999999@example.com"})
err = user_model.MakeActiveEmailPrimary(db.DefaultContext, email.ID)
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)
email = unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{Email: "user101@example.com"})
err = user_model.MakeActiveEmailPrimary(db.DefaultContext, email.ID)
assert.NoError(t, err)

user, _ := user_model.GetUserByID(db.DefaultContext, int64(10))
Expand Down
3 changes: 2 additions & 1 deletion options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ forgot_password_title= Forgot Password
forgot_password = Forgot password?
sign_up_now = Need an account? Register now.
sign_up_successful = Account was successfully created. Welcome!
confirmation_mail_sent_prompt = A new confirmation email has been sent to <b>%s</b>. Please check your inbox within the next %s to complete the registration process.
confirmation_mail_sent_prompt_ex = A new confirmation email has been sent to <b>%s</b>. Please check your inbox within the next %s to complete the registration process. If your registration email address is incorrect, you can sign in again and change it.
must_change_password = Update your password
allow_password_change = Require user to change password (recommended)
reset_password_mail_sent_prompt = A confirmation email has been sent to <b>%s</b>. Please check your inbox within the next %s to complete the account recovery process.
Expand All @@ -378,6 +378,7 @@ prohibit_login = Sign In Prohibited
prohibit_login_desc = Your account is prohibited from signing in, please contact your site administrator.
resent_limit_prompt = You have already requested an activation email recently. Please wait 3 minutes and try again.
has_unconfirmed_mail = Hi %s, you have an unconfirmed email address (<b>%s</b>). If you haven't received a confirmation email or need to resend a new one, please click on the button below.
change_unconfirmed_mail_address = If your registration email address is incorrect, you can change it here and resend a new confirmation email.
resend_mail = Click here to resend your activation email
email_not_associate = The email address is not associated with any account.
send_reset_mail = Send Account Recovery Email
Expand Down
31 changes: 28 additions & 3 deletions routers/web/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ func sendActivateEmail(ctx *context.Context, u *user_model.User) {
mailer.SendActivateAccountMail(ctx.Locale, u)

activeCodeLives := timeutil.MinutesToFriendly(setting.Service.ActiveCodeLives, ctx.Locale)
msgHTML := ctx.Locale.Tr("auth.confirmation_mail_sent_prompt", u.Email, activeCodeLives)
msgHTML := ctx.Locale.Tr("auth.confirmation_mail_sent_prompt_ex", u.Email, activeCodeLives)
renderActivationPromptMessage(ctx, msgHTML)
}

Expand All @@ -656,6 +656,10 @@ func renderActivationVerifyPassword(ctx *context.Context, code string) {
ctx.HTML(http.StatusOK, TplActivate)
}

func renderActivationChangeEmail(ctx *context.Context) {
ctx.HTML(http.StatusOK, TplActivate)
}

// Activate render activate user page
func Activate(ctx *context.Context) {
code := ctx.FormString("code")
Expand All @@ -674,7 +678,7 @@ func Activate(ctx *context.Context) {
return
}

// Resend confirmation email.
// Resend confirmation email. FIXME: ideally this should be in a POST request
sendActivateEmail(ctx, ctx.Doer)
return
}
Expand All @@ -698,7 +702,28 @@ func Activate(ctx *context.Context) {
// ActivatePost handles account activation with password check
func ActivatePost(ctx *context.Context) {
code := ctx.FormString("code")
if code == "" || (ctx.Doer != nil && ctx.Doer.IsActive) {
if ctx.Doer != nil && ctx.Doer.IsActive {
ctx.Redirect(setting.AppSubURL + "/user/activate") // it will redirect again to the correct page
return
}

if code == "" {
newEmail := strings.TrimSpace(ctx.FormString("change_email"))
if ctx.Doer != nil && newEmail != "" && !strings.EqualFold(ctx.Doer.Email, newEmail) {
if user_model.ValidateEmail(newEmail) != nil {
ctx.Flash.Error(ctx.Locale.Tr("form.email_invalid"), true)
renderActivationChangeEmail(ctx)
return
}
err := user_model.ChangeInactivePrimaryEmail(ctx, ctx.Doer.ID, ctx.Doer.Email, newEmail)
if err != nil {
ctx.Flash.Error(ctx.Locale.Tr("admin.emails.not_updated", newEmail), true)
renderActivationChangeEmail(ctx)
return
}
ctx.Doer.Email = newEmail
}
// FIXME: at the moment, GET request handles the "send confirmation email" action. But the old code does this redirect and then send a confirmation email.
ctx.Redirect(setting.AppSubURL + "/user/activate")
return
}
Expand Down
4 changes: 2 additions & 2 deletions routers/web/user/setting/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,9 @@ func EmailPost(ctx *context.Context) {
ctx.Data["Title"] = ctx.Tr("settings")
ctx.Data["PageIsSettingsAccount"] = true

// Make emailaddress primary.
// Make email address primary.
if ctx.FormString("_method") == "PRIMARY" {
if err := user_model.MakeEmailPrimary(ctx, &user_model.EmailAddress{ID: ctx.FormInt64("id")}); err != nil {
if err := user_model.MakeActiveEmailPrimary(ctx, ctx.FormInt64("id")); err != nil {
ctx.ServerError("MakeEmailPrimary", err)
return
}
Expand Down
7 changes: 7 additions & 0 deletions templates/user/auth/activate.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@
<input name="code" type="hidden" value="{{.ActivationCode}}">
{{else}}
<p>{{ctx.Locale.Tr "auth.has_unconfirmed_mail" .SignedUser.Name .SignedUser.Email}}</p>
<details>
<summary>{{ctx.Locale.Tr "auth.change_unconfirmed_mail_address"}}</summary>
<div class="tw-py-2">
<label for="change-email">{{ctx.Locale.Tr "email"}}</label>
<input id="change-email" name="change_email" type="email" value="{{.SignedUser.Email}}">
</div>
</details>
<div class="divider"></div>
<div class="text right">
<button class="ui primary button">{{ctx.Locale.Tr "auth.resend_mail"}}</button>
Expand Down
16 changes: 14 additions & 2 deletions tests/integration/signup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,25 @@ func TestSignupEmailActive(t *testing.T) {
resp := MakeRequest(t, req, http.StatusOK)
assert.Contains(t, resp.Body.String(), `A new confirmation email has been sent to <b>email-1@example.com</b>.`)

// access "user/active" means trying to re-send the activation email
// access "user/activate" means trying to re-send the activation email
session := loginUserWithPassword(t, "test-user-1", "password1")
resp = session.MakeRequest(t, NewRequest(t, "GET", "/user/activate"), http.StatusOK)
assert.Contains(t, resp.Body.String(), "You have already requested an activation email recently")

// access "user/active" with a valid activation code, then get the "verify password" page
// access anywhere else will see a "Activate Your Account" prompt, and there is a chance to change email
resp = session.MakeRequest(t, NewRequest(t, "GET", "/user/issues"), http.StatusOK)
assert.Contains(t, resp.Body.String(), `<input id="change-email" name="change_email" `)

// post to "user/activate" with a new email
session.MakeRequest(t, NewRequestWithValues(t, "POST", "/user/activate", map[string]string{"change_email": "email-changed@example.com"}), http.StatusSeeOther)
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "test-user-1"})
assert.Equal(t, "email-changed@example.com", user.Email)
email := unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{Email: "email-changed@example.com"})
assert.False(t, email.IsActivated)
assert.True(t, email.IsPrimary)

// access "user/activate" with a valid activation code, then get the "verify password" page
user = unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "test-user-1"})
activationCode := user.GenerateEmailActivateCode(user.Email)
resp = session.MakeRequest(t, NewRequest(t, "GET", "/user/activate?code="+activationCode), http.StatusOK)
assert.Contains(t, resp.Body.String(), `<input id="verify-password"`)
Expand Down
Loading