Fix edit user email bug in API (#36068) (#36081)

Backport #36068 by @lunny

Follow #36058 for API edit user bug when editing email.

- The Admin Edit User API includes a breaking change. Previously, when
updating a user with an email from an unallowed domain, the request
would succeed but return a warning in the response headers. Now, the
request will fail and return an error in the response body instead.
- Removed `AdminAddOrSetPrimaryEmailAddress` because it will not be used
any where.

Fix https://github.com/go-gitea/gitea/pull/36058#issuecomment-3600005186

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

routers/api/v1/admin/user.go

@@ -216,9 +216,12 @@ func EditUser(ctx *context.APIContext) {
 	}
 
 	if form.Email != nil {
-		if err := user_service.AdminAddOrSetPrimaryEmailAddress(ctx, ctx.ContextUser, *form.Email); err != nil {
+		if err := user_service.ReplacePrimaryEmailAddress(ctx, ctx.ContextUser, *form.Email); err != nil {
 			switch {
 			case user_model.IsErrEmailCharIsNotSupported(err), user_model.IsErrEmailInvalid(err):
+				if !user_model.IsEmailDomainAllowed(*form.Email) {
+					err = fmt.Errorf("the domain of user email %s conflicts with EMAIL_DOMAIN_ALLOWLIST or EMAIL_DOMAIN_BLOCKLIST", *form.Email)
+				}
 				ctx.APIError(http.StatusBadRequest, err)
 			case user_model.IsErrEmailAlreadyUsed(err):
 				ctx.APIError(http.StatusBadRequest, err)
@@ -227,10 +230,6 @@ func EditUser(ctx *context.APIContext) {
 			}
 			return
 		}
-
-		if !user_model.IsEmailDomainAllowed(*form.Email) {
-			ctx.Resp.Header().Add("X-Gitea-Warning", fmt.Sprintf("the domain of user email %s conflicts with EMAIL_DOMAIN_ALLOWLIST or EMAIL_DOMAIN_BLOCKLIST", *form.Email))
-		}
 	}
 
 	opts := &user_service.UpdateOptions{

services/user/email.go

@@ -14,60 +14,6 @@ import (
 	"code.gitea.io/gitea/modules/util"
 )
 
-// AdminAddOrSetPrimaryEmailAddress is used by admins to add or set a user's primary email address
-func AdminAddOrSetPrimaryEmailAddress(ctx context.Context, u *user_model.User, emailStr string) error {
-	if strings.EqualFold(u.Email, emailStr) {
-		return nil
-	}
-
-	if err := user_model.ValidateEmailForAdmin(emailStr); err != nil {
-		return err
-	}
-
-	// Check if address exists already
-	email, err := user_model.GetEmailAddressByEmail(ctx, emailStr)
-	if err != nil && !errors.Is(err, util.ErrNotExist) {
-		return err
-	}
-	if email != nil && email.UID != u.ID {
-		return user_model.ErrEmailAlreadyUsed{Email: emailStr}
-	}
-
-	// Update old primary address
-	primary, err := user_model.GetPrimaryEmailAddressOfUser(ctx, u.ID)
-	if err != nil {
-		return err
-	}
-
-	primary.IsPrimary = false
-	if err := user_model.UpdateEmailAddress(ctx, primary); err != nil {
-		return err
-	}
-
-	// Insert new or update existing address
-	if email != nil {
-		email.IsPrimary = true
-		email.IsActivated = true
-		if err := user_model.UpdateEmailAddress(ctx, email); err != nil {
-			return err
-		}
-	} else {
-		email = &user_model.EmailAddress{
-			UID:         u.ID,
-			Email:       emailStr,
-			IsActivated: true,
-			IsPrimary:   true,
-		}
-		if _, err := user_model.InsertEmailAddress(ctx, email); err != nil {
-			return err
-		}
-	}
-
-	u.Email = emailStr
-
-	return user_model.UpdateUserCols(ctx, u, "email")
-}
-
 func ReplacePrimaryEmailAddress(ctx context.Context, u *user_model.User, emailStr string) error {
 	if strings.EqualFold(u.Email, emailStr) {
 		return nil

services/user/email_test.go

@@ -9,61 +9,10 @@ import (
 	organization_model "code.gitea.io/gitea/models/organization"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
-	"code.gitea.io/gitea/modules/glob"
-	"code.gitea.io/gitea/modules/setting"
 
 	"github.com/stretchr/testify/assert"
 )
 
-func TestAdminAddOrSetPrimaryEmailAddress(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-
-	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 27})
-
-	emails, err := user_model.GetEmailAddresses(t.Context(), user.ID)
-	assert.NoError(t, err)
-	assert.Len(t, emails, 1)
-
-	primary, err := user_model.GetPrimaryEmailAddressOfUser(t.Context(), user.ID)
-	assert.NoError(t, err)
-	assert.NotEqual(t, "new-primary@example.com", primary.Email)
-	assert.Equal(t, user.Email, primary.Email)
-
-	assert.NoError(t, AdminAddOrSetPrimaryEmailAddress(t.Context(), user, "new-primary@example.com"))
-
-	primary, err = user_model.GetPrimaryEmailAddressOfUser(t.Context(), user.ID)
-	assert.NoError(t, err)
-	assert.Equal(t, "new-primary@example.com", primary.Email)
-	assert.Equal(t, user.Email, primary.Email)
-
-	emails, err = user_model.GetEmailAddresses(t.Context(), user.ID)
-	assert.NoError(t, err)
-	assert.Len(t, emails, 2)
-
-	setting.Service.EmailDomainAllowList = []glob.Glob{glob.MustCompile("example.org")}
-	defer func() {
-		setting.Service.EmailDomainAllowList = []glob.Glob{}
-	}()
-
-	assert.NoError(t, AdminAddOrSetPrimaryEmailAddress(t.Context(), user, "new-primary2@example2.com"))
-
-	primary, err = user_model.GetPrimaryEmailAddressOfUser(t.Context(), user.ID)
-	assert.NoError(t, err)
-	assert.Equal(t, "new-primary2@example2.com", primary.Email)
-	assert.Equal(t, user.Email, primary.Email)
-
-	assert.NoError(t, AdminAddOrSetPrimaryEmailAddress(t.Context(), user, "user27@example.com"))
-
-	primary, err = user_model.GetPrimaryEmailAddressOfUser(t.Context(), user.ID)
-	assert.NoError(t, err)
-	assert.Equal(t, "user27@example.com", primary.Email)
-	assert.Equal(t, user.Email, primary.Email)
-
-	emails, err = user_model.GetEmailAddresses(t.Context(), user.ID)
-	assert.NoError(t, err)
-	assert.Len(t, emails, 3)
-}
-
 func TestReplacePrimaryEmailAddress(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 

tests/integration/api_admin_test.go

@@ -382,10 +382,12 @@ func TestAPIEditUser_NotAllowedEmailDomain(t *testing.T) {
 		SourceID:  0,
 		Email:     &newEmail,
 	}).AddTokenAuth(token)
-	resp := MakeRequest(t, req, http.StatusOK)
+	resp := MakeRequest(t, req, http.StatusBadRequest)
-	assert.Equal(t, "the domain of user email user2@example1.com conflicts with EMAIL_DOMAIN_ALLOWLIST or EMAIL_DOMAIN_BLOCKLIST", resp.Header().Get("X-Gitea-Warning"))
+	errMap := make(map[string]string)
+	assert.NoError(t, json.Unmarshal(resp.Body.Bytes(), &errMap))
+	assert.Equal(t, "the domain of user email user2@example1.com conflicts with EMAIL_DOMAIN_ALLOWLIST or EMAIL_DOMAIN_BLOCKLIST", errMap["message"])
 
-	originalEmail := "user2@example.com"
+	originalEmail := "user2@example.org"
 	req = NewRequestWithJSON(t, "PATCH", urlStr, api.EditUserOption{
 		LoginName: "user2",
 		SourceID:  0,