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>

diff --git a/models/user_mail.go b/models/user_mail.go
index 89cbdf12a488f340635fd5c3e9d844462cf11c59..f8b084a0064b57d85fcc89fc6584d2c7d1299320 100644 (file)
--- a/models/user_mail.go
+++ b/models/user_mail.go
@@ -74,14 +74,15 @@ func GetEmailAddressByID(uid, id int64) (*EmailAddress, error) {
        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 {
                return true, nil
        }
 
        // Can't filter by boolean field unless it's explicit
        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 {
                // Inactive (unvalidated) addresses don't count as active if email validation is required
                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
        if has, err := e.Where(cond).Get(&em); has || err != nil {
                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
        }
@@ -366,8 +367,8 @@ func SearchEmails(opts *SearchEmailOptions) ([]*SearchEmailResult, int64, error)
 }
 
 // ActivateUserEmail will change the activated state of an email address,
-// either primary (in the user table) or secondary (in the email_address table)
-func ActivateUserEmail(userID int64, email string, primary, activate bool) (err error) {
+// either primary or secondary (all in the email_address table)
+func ActivateUserEmail(userID int64, email string, activate bool) (err error) {
        sess := x.NewSession()
        defer sess.Close()
        if err = sess.Begin(); err != nil {
@@ -387,34 +388,33 @@ func ActivateUserEmail(userID int64, email string, primary, activate bool) (err
                return nil
        }
        if activate {
-               if used, err := isEmailActive(sess, email, 0, addr.ID); err != nil {
-                       return fmt.Errorf("isEmailActive(): %v", err)
+               if used, err := isEmailActive(sess, email, addr.ID); err != nil {
+                       return fmt.Errorf("unable to check isEmailActive() for %s: %v", email, err)
                } else if used {
                        return ErrEmailAlreadyUsed{Email: email}
                }
        }
        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
+       // Activate/deactivate a user's primary email address and account
+       if addr.IsPrimary {
                user := User{ID: userID, Email: email}
                if has, err := sess.Get(&user); err != nil {
                        return err
                } 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 {
-                       // Already in the desired state; no action
-                       return nil
-               }
-               user.IsActive = activate
-               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("updateUserCols(): %v", err)
+               // The user's activation state should be synchronized with the primary email
+               if user.IsActive != activate {
+                       user.IsActive = activate
+                       if user.Rands, err = GetUserSalt(); err != nil {
+                               return fmt.Errorf("unable to 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)
+                       }
                }
        }
 

diff --git a/routers/web/admin/emails.go b/routers/web/admin/emails.go
index f7e8c97fb6b958472a9de339cc12adb73ea77257..704cb88c640d9bfd06f67929a3e76a4367ded5a6 100644 (file)
--- a/routers/web/admin/emails.go
+++ b/routers/web/admin/emails.go
@@ -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)
 
-       if err := models.ActivateUserEmail(uid, email, primary, activate); err != nil {
-               log.Error("ActivateUserEmail(%v,%v,%v,%v): %v", uid, email, primary, activate, err)
+       if err := models.ActivateUserEmail(uid, email, activate); err != nil {
+               log.Error("ActivateUserEmail(%v,%v,%v): %v", uid, email, activate, err)
                if models.IsErrEmailAlreadyUsed(err) {
                        ctx.Flash.Error(ctx.Tr("admin.emails.duplicate_active"))
                } else {

diff --git a/routers/web/user/auth.go b/routers/web/user/auth.go
index 9458bf5c95a9cef8c9dffdeabdded5f1be7bdeee..4095d2956e3a59e53536f54d126a5e937955b5ee 100644 (file)
--- a/routers/web/user/auth.go
+++ b/routers/web/user/auth.go
@@ -1429,16 +1429,22 @@ func handleAccountActivation(ctx *context.Context, user *models.User) {
                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)
 
        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 {
-               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 {
-               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"))

diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go
index 48ab37d9369e660f651a5a6e34c1d1e68946b0b0..b805db620083b44bbc5ad063f5808c01cdbddd73 100644 (file)
--- a/routers/web/user/setting/account.go
+++ b/routers/web/user/setting/account.go
@@ -107,35 +107,36 @@ func EmailPost(ctx *context.Context) {
                        ctx.Redirect(setting.AppSubURL + "/user/settings/account")
                        return
                }
-               if ctx.Query("id") == "PRIMARY" {
-                       if ctx.User.IsActive {
-                               log.Error("Send activation: email not set for activation")
+
+               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.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)
-                       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
                }
+               address = email.Email
 
                if err := ctx.Cache.Put("MailResendLimit_"+ctx.User.LowerName, ctx.User.LowerName, 180); err != nil {
                        log.Error("Set cache(MailResendLimit) fail: %v", err)

diff --git a/templates/user/settings/account.tmpl b/templates/user/settings/account.tmpl
index 1a74c64d74ba2574380347f61fa02913e6e52b29..2e1976aaa95771d515a172a66ba1f093b6c6a92e 100644 (file)
--- a/templates/user/settings/account.tmpl
+++ b/templates/user/settings/account.tmpl
@@ -92,7 +92,7 @@
                                                                <form action="{{AppSubUrl}}/user/settings/account/email" method="post">
                                                                        {{$.CsrfTokenHtml}}
                                                                        <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}}
                                                                                <button disabled class="ui blue tiny button">{{$.i18n.Tr "settings.activations_pending"}}</button>
                                                                        {{else}}