Fix activation of primary email addresses (#16385)

* fix: primary email cannot be activated

* Primary email should be activated together with user account when
'RegisterEmailConfirm' is enabled.

* To fix the existing error state. When 'RegisterEmailConfirm' is enabled, the
admin should have permission to modify the activations status of user email.
And the user should be allowed to send activation to primary email.

* Only judge whether email is primary from email_address table.

* Improve logging and refactor isEmailActive

Co-authored-by: zeripath <art27@cantab.net>
release/v1.15
Meano 2021-07-14 04:59:27 +08:00 committed by GitHub
parent 56b7f53329
commit 423a0fccb6
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 62 additions and 55 deletions

View File

@ -74,14 +74,15 @@ func GetEmailAddressByID(uid, id int64) (*EmailAddress, error) {
return email, nil return email, nil
} }
func isEmailActive(e Engine, email string, userID, emailID int64) (bool, error) { // isEmailActive check if email is activated with a different emailID
func isEmailActive(e Engine, email string, excludeEmailID int64) (bool, error) {
if len(email) == 0 { if len(email) == 0 {
return true, nil return true, nil
} }
// Can't filter by boolean field unless it's explicit // Can't filter by boolean field unless it's explicit
cond := builder.NewCond() cond := builder.NewCond()
cond = cond.And(builder.Eq{"lower_email": strings.ToLower(email)}, builder.Neq{"id": emailID}) cond = cond.And(builder.Eq{"lower_email": strings.ToLower(email)}, builder.Neq{"id": excludeEmailID})
if setting.Service.RegisterEmailConfirm { if setting.Service.RegisterEmailConfirm {
// Inactive (unvalidated) addresses don't count as active if email validation is required // Inactive (unvalidated) addresses don't count as active if email validation is required
cond = cond.And(builder.Eq{"is_activated": true}) cond = cond.And(builder.Eq{"is_activated": true})
@ -90,7 +91,7 @@ func isEmailActive(e Engine, email string, userID, emailID int64) (bool, error)
var em EmailAddress var em EmailAddress
if has, err := e.Where(cond).Get(&em); has || err != nil { if has, err := e.Where(cond).Get(&em); has || err != nil {
if has { if has {
log.Info("isEmailActive('%s',%d,%d) found duplicate in email ID %d", email, userID, emailID, em.ID) log.Info("isEmailActive(%q, %d) found duplicate in email ID %d", email, excludeEmailID, em.ID)
} }
return has, err return has, err
} }
@ -366,8 +367,8 @@ func SearchEmails(opts *SearchEmailOptions) ([]*SearchEmailResult, int64, error)
} }
// ActivateUserEmail will change the activated state of an email address, // ActivateUserEmail will change the activated state of an email address,
// either primary (in the user table) or secondary (in the email_address table) // either primary or secondary (all in the email_address table)
func ActivateUserEmail(userID int64, email string, primary, activate bool) (err error) { func ActivateUserEmail(userID int64, email string, activate bool) (err error) {
sess := x.NewSession() sess := x.NewSession()
defer sess.Close() defer sess.Close()
if err = sess.Begin(); err != nil { if err = sess.Begin(); err != nil {
@ -387,34 +388,33 @@ func ActivateUserEmail(userID int64, email string, primary, activate bool) (err
return nil return nil
} }
if activate { if activate {
if used, err := isEmailActive(sess, email, 0, addr.ID); err != nil { if used, err := isEmailActive(sess, email, addr.ID); err != nil {
return fmt.Errorf("isEmailActive(): %v", err) return fmt.Errorf("unable to check isEmailActive() for %s: %v", email, err)
} else if used { } else if used {
return ErrEmailAlreadyUsed{Email: email} return ErrEmailAlreadyUsed{Email: email}
} }
} }
if err = addr.updateActivation(sess, activate); err != nil { if err = addr.updateActivation(sess, activate); err != nil {
return fmt.Errorf("updateActivation(): %v", err) return fmt.Errorf("unable to updateActivation() for %d:%s: %w", addr.ID, addr.Email, err)
} }
if primary { // Activate/deactivate a user's primary email address and account
// Activate/deactivate a user's primary email address if addr.IsPrimary {
user := User{ID: userID, Email: email} user := User{ID: userID, Email: email}
if has, err := sess.Get(&user); err != nil { if has, err := sess.Get(&user); err != nil {
return err return err
} else if !has { } else if !has {
return fmt.Errorf("no such user: %d (%s)", userID, email) return fmt.Errorf("no user with ID: %d and Email: %s", userID, email)
} }
if user.IsActive == activate { // The user's activation state should be synchronized with the primary email
// Already in the desired state; no action if user.IsActive != activate {
return nil user.IsActive = activate
} if user.Rands, err = GetUserSalt(); err != nil {
user.IsActive = activate return fmt.Errorf("unable to generate salt: %v", err)
if user.Rands, err = GetUserSalt(); err != nil { }
return fmt.Errorf("generate salt: %v", err) if err = updateUserCols(sess, &user, "is_active", "rands"); err != nil {
} return fmt.Errorf("unable to updateUserCols() for user ID: %d: %v", userID, err)
if err = updateUserCols(sess, &user, "is_active", "rands"); err != nil { }
return fmt.Errorf("updateUserCols(): %v", err)
} }
} }

View File

@ -125,8 +125,8 @@ func ActivateEmail(ctx *context.Context) {
log.Info("Changing activation for User ID: %d, email: %s, primary: %v to %v", uid, email, primary, activate) log.Info("Changing activation for User ID: %d, email: %s, primary: %v to %v", uid, email, primary, activate)
if err := models.ActivateUserEmail(uid, email, primary, activate); err != nil { if err := models.ActivateUserEmail(uid, email, activate); err != nil {
log.Error("ActivateUserEmail(%v,%v,%v,%v): %v", uid, email, primary, activate, err) log.Error("ActivateUserEmail(%v,%v,%v): %v", uid, email, activate, err)
if models.IsErrEmailAlreadyUsed(err) { if models.IsErrEmailAlreadyUsed(err) {
ctx.Flash.Error(ctx.Tr("admin.emails.duplicate_active")) ctx.Flash.Error(ctx.Tr("admin.emails.duplicate_active"))
} else { } else {

View File

@ -1429,16 +1429,22 @@ func handleAccountActivation(ctx *context.Context, user *models.User) {
return return
} }
if err := models.ActivateUserEmail(user.ID, user.Email, true); err != nil {
log.Error("Unable to activate email for user: %-v with email: %s: %v", user, user.Email, err)
ctx.ServerError("ActivateUserEmail", err)
return
}
log.Trace("User activated: %s", user.Name) log.Trace("User activated: %s", user.Name)
if err := ctx.Session.Set("uid", user.ID); err != nil { if err := ctx.Session.Set("uid", user.ID); err != nil {
log.Error(fmt.Sprintf("Error setting uid in session: %v", err)) log.Error("Error setting uid in session[%s]: %v", ctx.Session.ID(), err)
} }
if err := ctx.Session.Set("uname", user.Name); err != nil { if err := ctx.Session.Set("uname", user.Name); err != nil {
log.Error(fmt.Sprintf("Error setting uname in session: %v", err)) log.Error("Error setting uname in session[%s]: %v", ctx.Session.ID(), err)
} }
if err := ctx.Session.Release(); err != nil { if err := ctx.Session.Release(); err != nil {
log.Error("Error storing session: %v", err) log.Error("Error storing session[%s]: %v", ctx.Session.ID(), err)
} }
ctx.Flash.Success(ctx.Tr("auth.account_activated")) ctx.Flash.Success(ctx.Tr("auth.account_activated"))

View File

@ -107,35 +107,36 @@ func EmailPost(ctx *context.Context) {
ctx.Redirect(setting.AppSubURL + "/user/settings/account") ctx.Redirect(setting.AppSubURL + "/user/settings/account")
return return
} }
if ctx.Query("id") == "PRIMARY" {
if ctx.User.IsActive { id := ctx.QueryInt64("id")
log.Error("Send activation: email not set for activation") email, err := models.GetEmailAddressByID(ctx.User.ID, id)
ctx.Redirect(setting.AppSubURL + "/user/settings/account") if err != nil {
return log.Error("GetEmailAddressByID(%d,%d) error: %v", ctx.User.ID, id, err)
} ctx.Redirect(setting.AppSubURL + "/user/settings/account")
mailer.SendActivateAccountMail(ctx.Locale, ctx.User) return
address = ctx.User.Email
} else {
id := ctx.QueryInt64("id")
email, err := models.GetEmailAddressByID(ctx.User.ID, id)
if err != nil {
log.Error("GetEmailAddressByID(%d,%d) error: %v", ctx.User.ID, id, err)
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
return
}
if email == nil {
log.Error("Send activation: EmailAddress not found; user:%d, id: %d", ctx.User.ID, id)
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
return
}
if email.IsActivated {
log.Error("Send activation: email not set for activation")
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
return
}
mailer.SendActivateEmailMail(ctx.User, email)
address = email.Email
} }
if email == nil {
log.Warn("Send activation failed: EmailAddress[%d] not found for user: %-v", id, ctx.User)
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
return
}
if email.IsActivated {
log.Debug("Send activation failed: email %s is already activated for user: %-v", email.Email, ctx.User)
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
return
}
if email.IsPrimary {
if ctx.User.IsActive && !setting.Service.RegisterEmailConfirm {
log.Debug("Send activation failed: email %s is already activated for user: %-v", email.Email, ctx.User)
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
return
}
// Only fired when the primary email is inactive (Wrong state)
mailer.SendActivateAccountMail(ctx.Locale, ctx.User)
} else {
mailer.SendActivateEmailMail(ctx.User, email)
}
address = email.Email
if err := ctx.Cache.Put("MailResendLimit_"+ctx.User.LowerName, ctx.User.LowerName, 180); err != nil { if err := ctx.Cache.Put("MailResendLimit_"+ctx.User.LowerName, ctx.User.LowerName, 180); err != nil {
log.Error("Set cache(MailResendLimit) fail: %v", err) log.Error("Set cache(MailResendLimit) fail: %v", err)

View File

@ -92,7 +92,7 @@
<form action="{{AppSubUrl}}/user/settings/account/email" method="post"> <form action="{{AppSubUrl}}/user/settings/account/email" method="post">
{{$.CsrfTokenHtml}} {{$.CsrfTokenHtml}}
<input name="_method" type="hidden" value="SENDACTIVATION"> <input name="_method" type="hidden" value="SENDACTIVATION">
<input name="id" type="hidden" value="{{if .IsPrimary}}PRIMARY{{else}}{{.ID}}{{end}}"> <input name="id" type="hidden" value="{{.ID}}">
{{if $.ActivationsPending}} {{if $.ActivationsPending}}
<button disabled class="ui blue tiny button">{{$.i18n.Tr "settings.activations_pending"}}</button> <button disabled class="ui blue tiny button">{{$.i18n.Tr "settings.activations_pending"}}</button>
{{else}} {{else}}