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.

Sign up for GitHub

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

Jump to bottom

fix activation of primary email addresses #16385

Merged
merged 5 commits into from
Jul 13, 2021
Merged

fix activation of primary email addresses #16385

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7669ff8
fix: primary email cannot be activated
Meano Jul 8, 2021
8ae1fd6
Merge branch 'main' into develop
zeripath Jul 13, 2021
4ce9c80
Improve logging and refactor isEmailActive
zeripath Jul 13, 2021
ad3c0d5
Merge branch 'main' into develop
zeripath Jul 13, 2021
9c092f6
Merge branch 'main' into develop
zeripath Jul 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 21 additions & 21 deletions models/user_mail.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand All @@ -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
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions routers/web/admin/emails.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
12 changes: 9 additions & 3 deletions routers/web/user/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down
45 changes: 23 additions & 22 deletions routers/web/user/setting/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
6543 marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down
2 changes: 1 addition & 1 deletion templates/user/settings/account.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
Expand Down