Skip to content

Commit 9137d4d

Browse files
Meanozeripath
authored and
AbdulrhmnGhanem
committed
Fix activation of primary email addresses (go-gitea#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>
1 parent 0a2631e commit 9137d4d

File tree

5 files changed

+56
-49
lines changed

5 files changed

+56
-49
lines changed

Diff for: ‎models/user_mail.go

+21-21
Original file line numberDiff line numberDiff line change
@@ -74,14 +74,15 @@ func GetEmailAddressByID(uid, id int64) (*EmailAddress, error) {
7474
return email, nil
7575
}
7676

77-
func isEmailActive(e Engine, email string, userID, emailID int64) (bool, error) {
77+
// isEmailActive check if email is activated with a different emailID
78+
func isEmailActive(e Engine, email string, excludeEmailID int64) (bool, error) {
7879
if len(email) == 0 {
7980
return true, nil
8081
}
8182

8283
// Can't filter by boolean field unless it's explicit
8384
cond := builder.NewCond()
84-
cond = cond.And(builder.Eq{"lower_email": strings.ToLower(email)}, builder.Neq{"id": emailID})
85+
cond = cond.And(builder.Eq{"lower_email": strings.ToLower(email)}, builder.Neq{"id": excludeEmailID})
8586
if setting.Service.RegisterEmailConfirm {
8687
// Inactive (unvalidated) addresses don't count as active if email validation is required
8788
cond = cond.And(builder.Eq{"is_activated": true})
@@ -90,7 +91,7 @@ func isEmailActive(e Engine, email string, userID, emailID int64) (bool, error)
9091
var em EmailAddress
9192
if has, err := e.Where(cond).Get(&em); has || err != nil {
9293
if has {
93-
log.Info("isEmailActive('%s',%d,%d) found duplicate in email ID %d", email, userID, emailID, em.ID)
94+
log.Info("isEmailActive(%q, %d) found duplicate in email ID %d", email, excludeEmailID, em.ID)
9495
}
9596
return has, err
9697
}
@@ -366,8 +367,8 @@ func SearchEmails(opts *SearchEmailOptions) ([]*SearchEmailResult, int64, error)
366367
}
367368

368369
// ActivateUserEmail will change the activated state of an email address,
369-
// either primary (in the user table) or secondary (in the email_address table)
370-
func ActivateUserEmail(userID int64, email string, primary, activate bool) (err error) {
370+
// either primary or secondary (all in the email_address table)
371+
func ActivateUserEmail(userID int64, email string, activate bool) (err error) {
371372
sess := x.NewSession()
372373
defer sess.Close()
373374
if err = sess.Begin(); err != nil {
@@ -387,34 +388,33 @@ func ActivateUserEmail(userID int64, email string, primary, activate bool) (err
387388
return nil
388389
}
389390
if activate {
390-
if used, err := isEmailActive(sess, email, 0, addr.ID); err != nil {
391-
return fmt.Errorf("isEmailActive(): %v", err)
391+
if used, err := isEmailActive(sess, email, addr.ID); err != nil {
392+
return fmt.Errorf("unable to check isEmailActive() for %s: %v", email, err)
392393
} else if used {
393394
return ErrEmailAlreadyUsed{Email: email}
394395
}
395396
}
396397
if err = addr.updateActivation(sess, activate); err != nil {
397-
return fmt.Errorf("updateActivation(): %v", err)
398+
return fmt.Errorf("unable to updateActivation() for %d:%s: %w", addr.ID, addr.Email, err)
398399
}
399400

400-
if primary {
401-
// Activate/deactivate a user's primary email address
401+
// Activate/deactivate a user's primary email address and account
402+
if addr.IsPrimary {
402403
user := User{ID: userID, Email: email}
403404
if has, err := sess.Get(&user); err != nil {
404405
return err
405406
} else if !has {
406-
return fmt.Errorf("no such user: %d (%s)", userID, email)
407+
return fmt.Errorf("no user with ID: %d and Email: %s", userID, email)
407408
}
408-
if user.IsActive == activate {
409-
// Already in the desired state; no action
410-
return nil
411-
}
412-
user.IsActive = activate
413-
if user.Rands, err = GetUserSalt(); err != nil {
414-
return fmt.Errorf("generate salt: %v", err)
415-
}
416-
if err = updateUserCols(sess, &user, "is_active", "rands"); err != nil {
417-
return fmt.Errorf("updateUserCols(): %v", err)
409+
// The user's activation state should be synchronized with the primary email
410+
if user.IsActive != activate {
411+
user.IsActive = activate
412+
if user.Rands, err = GetUserSalt(); err != nil {
413+
return fmt.Errorf("unable to generate salt: %v", err)
414+
}
415+
if err = updateUserCols(sess, &user, "is_active", "rands"); err != nil {
416+
return fmt.Errorf("unable to updateUserCols() for user ID: %d: %v", userID, err)
417+
}
418418
}
419419
}
420420

Diff for: ‎routers/web/admin/emails.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,8 @@ func ActivateEmail(ctx *context.Context) {
125125

126126
log.Info("Changing activation for User ID: %d, email: %s, primary: %v to %v", uid, email, primary, activate)
127127

128-
if err := models.ActivateUserEmail(uid, email, primary, activate); err != nil {
129-
log.Error("ActivateUserEmail(%v,%v,%v,%v): %v", uid, email, primary, activate, err)
128+
if err := models.ActivateUserEmail(uid, email, activate); err != nil {
129+
log.Error("ActivateUserEmail(%v,%v,%v): %v", uid, email, activate, err)
130130
if models.IsErrEmailAlreadyUsed(err) {
131131
ctx.Flash.Error(ctx.Tr("admin.emails.duplicate_active"))
132132
} else {

Diff for: ‎routers/web/user/auth.go

+9-3
Original file line numberDiff line numberDiff line change
@@ -1429,16 +1429,22 @@ func handleAccountActivation(ctx *context.Context, user *models.User) {
14291429
return
14301430
}
14311431

1432+
if err := models.ActivateUserEmail(user.ID, user.Email, true); err != nil {
1433+
log.Error("Unable to activate email for user: %-v with email: %s: %v", user, user.Email, err)
1434+
ctx.ServerError("ActivateUserEmail", err)
1435+
return
1436+
}
1437+
14321438
log.Trace("User activated: %s", user.Name)
14331439

14341440
if err := ctx.Session.Set("uid", user.ID); err != nil {
1435-
log.Error(fmt.Sprintf("Error setting uid in session: %v", err))
1441+
log.Error("Error setting uid in session[%s]: %v", ctx.Session.ID(), err)
14361442
}
14371443
if err := ctx.Session.Set("uname", user.Name); err != nil {
1438-
log.Error(fmt.Sprintf("Error setting uname in session: %v", err))
1444+
log.Error("Error setting uname in session[%s]: %v", ctx.Session.ID(), err)
14391445
}
14401446
if err := ctx.Session.Release(); err != nil {
1441-
log.Error("Error storing session: %v", err)
1447+
log.Error("Error storing session[%s]: %v", ctx.Session.ID(), err)
14421448
}
14431449

14441450
ctx.Flash.Success(ctx.Tr("auth.account_activated"))

Diff for: ‎routers/web/user/setting/account.go

+23-22
Original file line numberDiff line numberDiff line change
@@ -107,35 +107,36 @@ func EmailPost(ctx *context.Context) {
107107
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
108108
return
109109
}
110-
if ctx.Query("id") == "PRIMARY" {
111-
if ctx.User.IsActive {
112-
log.Error("Send activation: email not set for activation")
110+
111+
id := ctx.QueryInt64("id")
112+
email, err := models.GetEmailAddressByID(ctx.User.ID, id)
113+
if err != nil {
114+
log.Error("GetEmailAddressByID(%d,%d) error: %v", ctx.User.ID, id, err)
115+
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
116+
return
117+
}
118+
if email == nil {
119+
log.Warn("Send activation failed: EmailAddress[%d] not found for user: %-v", id, ctx.User)
120+
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
121+
return
122+
}
123+
if email.IsActivated {
124+
log.Debug("Send activation failed: email %s is already activated for user: %-v", email.Email, ctx.User)
125+
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
126+
return
127+
}
128+
if email.IsPrimary {
129+
if ctx.User.IsActive && !setting.Service.RegisterEmailConfirm {
130+
log.Debug("Send activation failed: email %s is already activated for user: %-v", email.Email, ctx.User)
113131
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
114132
return
115133
}
134+
// Only fired when the primary email is inactive (Wrong state)
116135
mailer.SendActivateAccountMail(ctx.Locale, ctx.User)
117-
address = ctx.User.Email
118136
} else {
119-
id := ctx.QueryInt64("id")
120-
email, err := models.GetEmailAddressByID(ctx.User.ID, id)
121-
if err != nil {
122-
log.Error("GetEmailAddressByID(%d,%d) error: %v", ctx.User.ID, id, err)
123-
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
124-
return
125-
}
126-
if email == nil {
127-
log.Error("Send activation: EmailAddress not found; user:%d, id: %d", ctx.User.ID, id)
128-
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
129-
return
130-
}
131-
if email.IsActivated {
132-
log.Error("Send activation: email not set for activation")
133-
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
134-
return
135-
}
136137
mailer.SendActivateEmailMail(ctx.User, email)
137-
address = email.Email
138138
}
139+
address = email.Email
139140

140141
if err := ctx.Cache.Put("MailResendLimit_"+ctx.User.LowerName, ctx.User.LowerName, 180); err != nil {
141142
log.Error("Set cache(MailResendLimit) fail: %v", err)

Diff for: ‎templates/user/settings/account.tmpl

+1-1
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@
9292
<form action="{{AppSubUrl}}/user/settings/account/email" method="post">
9393
{{$.CsrfTokenHtml}}
9494
<input name="_method" type="hidden" value="SENDACTIVATION">
95-
<input name="id" type="hidden" value="{{if .IsPrimary}}PRIMARY{{else}}{{.ID}}{{end}}">
95+
<input name="id" type="hidden" value="{{.ID}}">
9696
{{if $.ActivationsPending}}
9797
<button disabled class="ui blue tiny button">{{$.i18n.Tr "settings.activations_pending"}}</button>
9898
{{else}}

0 commit comments

Comments
 (0)