Fix a bug user could change another user's primary email (#36586)

models/user/email_address.go

@@ -276,17 +276,22 @@ func updateActivation(ctx context.Context, email *EmailAddress, activate bool) e
 	return UpdateUserCols(ctx, user, "rands")
 }
 
-func MakeActiveEmailPrimary(ctx context.Context, emailID int64) error {
+func MakeActiveEmailPrimary(ctx context.Context, ownerID, emailID int64) error {
-	return makeEmailPrimaryInternal(ctx, emailID, true)
+	return makeEmailPrimaryInternal(ctx, ownerID, emailID, true)
 }
 
-func MakeInactiveEmailPrimary(ctx context.Context, emailID int64) error {
+func MakeInactiveEmailPrimary(ctx context.Context, ownerID, emailID int64) error {
-	return makeEmailPrimaryInternal(ctx, emailID, false)
+	return makeEmailPrimaryInternal(ctx, ownerID, emailID, false)
 }
 
-func makeEmailPrimaryInternal(ctx context.Context, emailID int64, isActive bool) error {
+func makeEmailPrimaryInternal(ctx context.Context, ownerID, emailID int64, isActive bool) error {
 	email := &EmailAddress{}
-	if has, err := db.GetEngine(ctx).ID(emailID).Where(builder.Eq{"is_activated": isActive}).Get(email); err != nil {
+	if has, err := db.GetEngine(ctx).ID(emailID).
+		Where(builder.Eq{
+			"uid":          ownerID,
+			"is_activated": isActive,
+		}).
+		Get(email); err != nil {
 		return err
 	} else if !has {
 		return ErrEmailAddressNotExist{}
@@ -336,7 +341,7 @@ func ChangeInactivePrimaryEmail(ctx context.Context, uid int64, oldEmailAddr, ne
 		if err != nil {
 			return err
 		}
-		return MakeInactiveEmailPrimary(ctx, newEmail.ID)
+		return MakeInactiveEmailPrimary(ctx, uid, newEmail.ID)
 	})
 }
 

models/user/email_address_test.go

@@ -46,22 +46,22 @@ func TestIsEmailUsed(t *testing.T) {
 func TestMakeEmailPrimary(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 
-	err := user_model.MakeActiveEmailPrimary(t.Context(), 9999999)
+	err := user_model.MakeActiveEmailPrimary(t.Context(), 1, 9999999)
 	assert.Error(t, err)
 	assert.ErrorIs(t, err, user_model.ErrEmailAddressNotExist{})
 
 	email := unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{Email: "user11@example.com"})
-	err = user_model.MakeActiveEmailPrimary(t.Context(), email.ID)
+	err = user_model.MakeActiveEmailPrimary(t.Context(), email.UID, email.ID)
 	assert.Error(t, err)
 	assert.ErrorIs(t, err, user_model.ErrEmailAddressNotExist{}) // inactive email is considered as not exist for "MakeActiveEmailPrimary"
 
 	email = unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{Email: "user9999999@example.com"})
-	err = user_model.MakeActiveEmailPrimary(t.Context(), email.ID)
+	err = user_model.MakeActiveEmailPrimary(t.Context(), email.UID, email.ID)
 	assert.Error(t, err)
 	assert.True(t, user_model.IsErrUserNotExist(err))
 
 	email = unittest.AssertExistsAndLoadBean(t, &user_model.EmailAddress{Email: "user101@example.com"})
-	err = user_model.MakeActiveEmailPrimary(t.Context(), email.ID)
+	err = user_model.MakeActiveEmailPrimary(t.Context(), email.UID, email.ID)
 	assert.NoError(t, err)
 
 	user, _ := user_model.GetUserByID(t.Context(), int64(10))

options/locale/locale_en-US.json

@@ -758,6 +758,7 @@
   "settings.add_email": "Add Email Address",
   "settings.add_openid": "Add OpenID URI",
   "settings.add_email_confirmation_sent": "A confirmation email has been sent to \"%s\". Please check your inbox within the next %s to confirm your email address.",
+  "settings.email_primary_not_found": "The selected email address could not be found.",
   "settings.add_email_success": "The new email address has been added.",
   "settings.email_preference_set_success": "Email preference has been set successfully.",
   "settings.add_openid_success": "The new OpenID address has been added.",

routers/web/user/setting/account.go

@@ -113,7 +113,12 @@ func EmailPost(ctx *context.Context) {
 
 	// Make email address primary.
 	if ctx.FormString("_method") == "PRIMARY" {
-		if err := user_model.MakeActiveEmailPrimary(ctx, ctx.FormInt64("id")); err != nil {
+		if err := user_model.MakeActiveEmailPrimary(ctx, ctx.Doer.ID, ctx.FormInt64("id")); err != nil {
+			if user_model.IsErrEmailAddressNotExist(err) {
+				ctx.Flash.Error(ctx.Tr("settings.email_primary_not_found"))
+				ctx.Redirect(setting.AppSubURL + "/user/settings/account")
+				return
+			}
 			ctx.ServerError("MakeEmailPrimary", err)
 			return
 		}

tests/integration/user_settings_test.go

@@ -158,6 +158,34 @@ func TestUserSettingsUpdateEmail(t *testing.T) {
 		req := NewRequest(t, "POST", "/user/settings/account/email")
 		session.MakeRequest(t, req, http.StatusNotFound)
 	})
+
+	t.Run("primary email not found", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		session := loginUser(t, "user2")
+		req := NewRequestWithValues(t, "POST", "/user/settings/account/email", map[string]string{
+			"_method": "PRIMARY",
+			"id":      "9999",
+		})
+		resp := session.MakeRequest(t, req, http.StatusSeeOther)
+		assert.Equal(t, "/user/settings/account", resp.Header().Get("Location"))
+		flashMsg := session.GetCookieFlashMessage()
+		assert.Equal(t, "The selected email address could not be found.", flashMsg.ErrorMsg)
+	})
+
+	t.Run("primary email not owned by user", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		session := loginUser(t, "user2")
+		req := NewRequestWithValues(t, "POST", "/user/settings/account/email", map[string]string{
+			"_method": "PRIMARY",
+			"id":      "6",
+		})
+		resp := session.MakeRequest(t, req, http.StatusSeeOther)
+		assert.Equal(t, "/user/settings/account", resp.Header().Get("Location"))
+		flashMsg := session.GetCookieFlashMessage()
+		assert.Equal(t, "The selected email address could not be found.", flashMsg.ErrorMsg)
+	})
 }
 
 func TestUserSettingsDeleteEmail(t *testing.T) {