From 7669ff86ed8d24fc5771eea6028ca6b8f0fb9cce Mon Sep 17 00:00:00 2001 From: Meano Date: Thu, 8 Jul 2021 15:19:21 +0800 Subject: [PATCH 1/2] 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. Signed-off-by: Meano --- models/user_mail.go | 27 +++++++++-------- routers/web/admin/emails.go | 4 +-- routers/web/user/auth.go | 4 +++ routers/web/user/setting/account.go | 43 ++++++++++++++-------------- templates/user/settings/account.tmpl | 2 +- 5 files changed, 42 insertions(+), 38 deletions(-) diff --git a/models/user_mail.go b/models/user_mail.go index 89cbdf12a488f..e8cb3ae6c3c6d 100644 --- a/models/user_mail.go +++ b/models/user_mail.go @@ -366,8 +366,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 { @@ -397,24 +397,23 @@ func ActivateUserEmail(userID int64, email string, primary, activate bool) (err return fmt.Errorf("updateActivation(): %v", 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) } - 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 synchronized with the primary email + if user.IsActive != activate { + 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) + } } } diff --git a/routers/web/admin/emails.go b/routers/web/admin/emails.go index f7e8c97fb6b95..704cb88c640d9 100644 --- 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 9458bf5c95a9c..636d8492ac813 100644 --- a/routers/web/user/auth.go +++ b/routers/web/user/auth.go @@ -1429,6 +1429,10 @@ func handleAccountActivation(ctx *context.Context, user *models.User) { return } + if err := models.ActivateUserEmail(user.ID, user.Email, true); err != nil { + log.Error(fmt.Sprintf("Error active user mail: %v", err)) + } + log.Trace("User activated: %s", user.Name) if err := ctx.Session.Set("uid", user.ID); err != nil { diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go index 48ab37d9369e6..e0159f6e63f8e 100644 --- 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 { + + 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 + } + if email.IsPrimary { + if ctx.User.IsActive && !setting.Service.RegisterEmailConfirm { log.Error("Send activation: email not set for activation") 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 1a74c64d74ba2..2e1976aaa9577 100644 --- a/templates/user/settings/account.tmpl +++ b/templates/user/settings/account.tmpl @@ -92,7 +92,7 @@
{{$.CsrfTokenHtml}} - + {{if $.ActivationsPending}} {{else}} From 4ce9c80ba90328d7890924d3c7a41535972a6dee Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 13 Jul 2021 19:51:57 +0100 Subject: [PATCH 2/2] Improve logging and refactor isEmailActive Signed-off-by: Andrew Thornton --- models/user_mail.go | 21 +++++++++++---------- routers/web/user/auth.go | 10 ++++++---- routers/web/user/setting/account.go | 6 +++--- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/models/user_mail.go b/models/user_mail.go index e8cb3ae6c3c6d..f8b084a0064b5 100644 --- 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 } @@ -387,14 +388,14 @@ func ActivateUserEmail(userID int64, email string, activate bool) (err error) { 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) } // Activate/deactivate a user's primary email address and account @@ -403,16 +404,16 @@ func ActivateUserEmail(userID int64, email string, activate bool) (err error) { 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) } - // The user's activation state should synchronized with the primary email + // 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("generate salt: %v", err) + return fmt.Errorf("unable to generate salt: %v", err) } if err = updateUserCols(sess, &user, "is_active", "rands"); err != nil { - return fmt.Errorf("updateUserCols(): %v", err) + return fmt.Errorf("unable to updateUserCols() for user ID: %d: %v", userID, err) } } } diff --git a/routers/web/user/auth.go b/routers/web/user/auth.go index 636d8492ac813..4095d2956e3a5 100644 --- a/routers/web/user/auth.go +++ b/routers/web/user/auth.go @@ -1430,19 +1430,21 @@ func handleAccountActivation(ctx *context.Context, user *models.User) { } if err := models.ActivateUserEmail(user.ID, user.Email, true); err != nil { - log.Error(fmt.Sprintf("Error active user mail: %v", err)) + 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 e0159f6e63f8e..b805db620083b 100644 --- a/routers/web/user/setting/account.go +++ b/routers/web/user/setting/account.go @@ -116,18 +116,18 @@ func EmailPost(ctx *context.Context) { return } if email == nil { - log.Error("Send activation: EmailAddress not found; user:%d, id: %d", ctx.User.ID, id) + 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.Error("Send activation: email not set for activation") + 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.Error("Send activation: email not set for activation") + 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 }