Fix org contact email not clearable once set (#36975)

When the email field was submitted as empty in org settings (web and
API), the previous guard `if form.Email != ""` silently skipped the
update, making it impossible to remove a contact email after it was set.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

Author: bircni

modules/structs/org.go

@@ -66,23 +66,21 @@ type CreateOrgOption struct {
 	RepoAdminChangeTeamAccess bool `json:"repo_admin_change_team_access"`
 }
 
-// TODO: make EditOrgOption fields optional after https://gitea.com/go-chi/binding/pulls/5 got merged
-
 // EditOrgOption options for editing an organization
 type EditOrgOption struct {
 	// The full display name of the organization
-	FullName string `json:"full_name" binding:"MaxSize(100)"`
-	// The email address of the organization
-	Email string `json:"email" binding:"MaxSize(255)"`
+	FullName *string `json:"full_name" binding:"MaxSize(100)"`
+	// The email address of the organization; use empty string to clear
+	Email *string `json:"email" binding:"MaxSize(255)"`
 	// The description of the organization
-	Description string `json:"description" binding:"MaxSize(255)"`
+	Description *string `json:"description" binding:"MaxSize(255)"`
 	// The website URL of the organization
-	Website string `json:"website" binding:"ValidUrl;MaxSize(255)"`
+	Website *string `json:"website" binding:"ValidUrl;MaxSize(255)"`
 	// The location of the organization
-	Location string `json:"location" binding:"MaxSize(50)"`
+	Location *string `json:"location" binding:"MaxSize(50)"`
 	// possible values are `public`, `limited` or `private`
 	// enum: public,limited,private
-	Visibility string `json:"visibility" binding:"In(,public,limited,private)"`
+	Visibility *string `json:"visibility" binding:"In(,public,limited,private)"`
 	// Whether repository administrators can change team access
 	RepoAdminChangeTeamAccess *bool `json:"repo_admin_change_team_access"`
 }

routers/api/v1/org/org.go

@@ -5,6 +5,7 @@
 package org
 
 import (
+	"errors"
 	"net/http"
 
 	activities_model "code.gitea.io/gitea/models/activities"
@@ -14,6 +15,7 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/optional"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/api/v1/user"
 	"code.gitea.io/gitea/routers/api/v1/utils"
@@ -379,19 +381,21 @@ func Edit(ctx *context.APIContext) {
 
 	form := web.GetForm(ctx).(*api.EditOrgOption)
 
-	if form.Email != "" {
-		if err := user_service.ReplacePrimaryEmailAddress(ctx, ctx.Org.Organization.AsUser(), form.Email); err != nil {
-			ctx.APIErrorInternal(err)
+	if err := org.UpdateOrgEmailAddress(ctx, ctx.Org.Organization, form.Email); err != nil {
+		if errors.Is(err, util.ErrInvalidArgument) {
+			ctx.APIError(http.StatusUnprocessableEntity, err)
 			return
 		}
+		ctx.APIErrorInternal(err)
+		return
 	}
 
 	opts := &user_service.UpdateOptions{
-		FullName:                  optional.Some(form.FullName),
-		Description:               optional.Some(form.Description),
-		Website:                   optional.Some(form.Website),
-		Location:                  optional.Some(form.Location),
-		Visibility:                optional.FromMapLookup(api.VisibilityModes, form.Visibility),
+		FullName:                  optional.FromPtr(form.FullName),
+		Description:               optional.FromPtr(form.Description),
+		Website:                   optional.FromPtr(form.Website),
+		Location:                  optional.FromPtr(form.Location),
+		Visibility:                optional.FromMapLookup(api.VisibilityModes, optional.FromPtr(form.Visibility).Value()),
 		RepoAdminChangeTeamAccess: optional.FromPtr(form.RepoAdminChangeTeamAccess),
 	}
 	if err := user_service.UpdateUser(ctx, ctx.Org.Organization.AsUser(), opts); err != nil {

routers/web/org/setting.go

@@ -5,6 +5,7 @@
 package org
 
 import (
+	"errors"
 	"net/http"
 	"net/url"
 
@@ -69,24 +70,25 @@ func SettingsPost(ctx *context.Context) {
 	}
 
 	org := ctx.Org.Organization
-
-	if form.Email != "" {
-		if err := user_service.ReplacePrimaryEmailAddress(ctx, org.AsUser(), form.Email); err != nil {
+	if err := org_service.UpdateOrgEmailAddress(ctx, org, form.Email); err != nil {
+		if errors.Is(err, util.ErrInvalidArgument) {
 			ctx.Data["Err_Email"] = true
 			ctx.RenderWithErrDeprecated(ctx.Tr("form.email_invalid"), tplSettingsOptions, &form)
 			return
 		}
+		ctx.ServerError("UpdateOrgEmailAddress", err)
+		return
 	}
 
 	opts := &user_service.UpdateOptions{
-		FullName:                  optional.Some(form.FullName),
-		Description:               optional.Some(form.Description),
-		Website:                   optional.Some(form.Website),
-		Location:                  optional.Some(form.Location),
-		RepoAdminChangeTeamAccess: optional.Some(form.RepoAdminChangeTeamAccess),
+		FullName:                  optional.FromPtr(form.FullName),
+		Description:               optional.FromPtr(form.Description),
+		Website:                   optional.FromPtr(form.Website),
+		Location:                  optional.FromPtr(form.Location),
+		RepoAdminChangeTeamAccess: optional.FromPtr(form.RepoAdminChangeTeamAccess),
 	}
 	if ctx.Doer.IsAdmin {
-		opts.MaxRepoCreation = optional.Some(form.MaxRepoCreation)
+		opts.MaxRepoCreation = optional.FromPtr(form.MaxRepoCreation)
 	}
 
 	if err := user_service.UpdateUser(ctx, org.AsUser(), opts); err != nil {

services/forms/org.go

@@ -36,13 +36,13 @@ func (f *CreateOrgForm) Validate(req *http.Request, errs binding.Errors) binding
 
 // UpdateOrgSettingForm form for updating organization settings
 type UpdateOrgSettingForm struct {
-	FullName                  string `binding:"MaxSize(100)"`
-	Email                     string `binding:"MaxSize(255)"`
-	Description               string `binding:"MaxSize(255)"`
-	Website                   string `binding:"ValidUrl;MaxSize(255)"`
-	Location                  string `binding:"MaxSize(50)"`
-	MaxRepoCreation           int
-	RepoAdminChangeTeamAccess bool
+	FullName                  *string `binding:"MaxSize(100)"`
+	Email                     *string `binding:"MaxSize(255)"`
+	Description               *string `binding:"MaxSize(255)"`
+	Website                   *string `binding:"ValidUrl;MaxSize(255)"`
+	Location                  *string `binding:"MaxSize(50)"`
+	MaxRepoCreation           *int
+	RepoAdminChangeTeamAccess *bool
 }
 
 // Validate validates the fields

services/org/org.go

@@ -168,3 +168,20 @@ func ChangeOrganizationVisibility(ctx context.Context, org *org_model.Organizati
 		return nil
 	})
 }
+
+// UpdateOrgEmailAddress validates and updates the organization's contact email.
+// A nil email means no change.
+func UpdateOrgEmailAddress(ctx context.Context, org *org_model.Organization, email *string) error {
+	if email == nil {
+		return nil
+	}
+
+	if *email != "" {
+		if err := user_model.ValidateEmail(*email); err != nil {
+			return err
+		}
+	}
+
+	org.Email = *email
+	return user_model.UpdateUserCols(ctx, org.AsUser(), "email")
+}

services/org/org_test.go

@@ -10,28 +10,54 @@ import (
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestMain(m *testing.M) {
 	unittest.MainTest(m)
 }
 
-func TestDeleteOrganization(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-	org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 6})
-	assert.NoError(t, DeleteOrganization(t.Context(), org, false))
-	unittest.AssertNotExistsBean(t, &organization.Organization{ID: 6})
-	unittest.AssertNotExistsBean(t, &organization.OrgUser{OrgID: 6})
-	unittest.AssertNotExistsBean(t, &organization.Team{OrgID: 6})
-
-	org = unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
-	err := DeleteOrganization(t.Context(), org, false)
-	assert.Error(t, err)
-	assert.True(t, repo_model.IsErrUserOwnRepos(err))
-
-	user := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 5})
-	assert.Error(t, DeleteOrganization(t.Context(), user, false))
-	unittest.CheckConsistencyFor(t, &user_model.User{}, &organization.Team{})
+func TestOrg(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	t.Run("UpdateOrgEmailAddress", func(t *testing.T) {
+		org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
+		originalEmail := org.Email
+
+		require.NoError(t, UpdateOrgEmailAddress(t.Context(), org, nil))
+		unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3, Email: originalEmail})
+
+		newEmail := "contact@org3.example.com"
+		require.NoError(t, UpdateOrgEmailAddress(t.Context(), org, &newEmail))
+		unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3, Email: newEmail})
+
+		invalidEmail := "invalid email"
+		err := UpdateOrgEmailAddress(t.Context(), org, &invalidEmail)
+		require.ErrorIs(t, err, util.ErrInvalidArgument)
+		unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3, Email: newEmail})
+
+		require.NoError(t, UpdateOrgEmailAddress(t.Context(), org, new("")))
+		org = unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3, Email: ""})
+		assert.Empty(t, org.Email)
+	})
+
+	t.Run("DeleteOrganization", func(t *testing.T) {
+		org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 6})
+		assert.NoError(t, DeleteOrganization(t.Context(), org, false))
+		unittest.AssertNotExistsBean(t, &organization.Organization{ID: 6})
+		unittest.AssertNotExistsBean(t, &organization.OrgUser{OrgID: 6})
+		unittest.AssertNotExistsBean(t, &organization.Team{OrgID: 6})
+
+		org = unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
+		err := DeleteOrganization(t.Context(), org, false)
+		assert.Error(t, err)
+		assert.True(t, repo_model.IsErrUserOwnRepos(err))
+
+		user := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 5})
+		assert.Error(t, DeleteOrganization(t.Context(), user, false))
+		unittest.CheckConsistencyFor(t, &user_model.User{}, &organization.Team{})
+	})
 }

services/user/email.go

@@ -14,7 +14,14 @@ import (
 	"code.gitea.io/gitea/modules/util"
 )
 
+// ReplacePrimaryEmailAddress replaces the user's primary email address with the given email address.
+// It also updates the user's email field to match the new primary email address.
 func ReplacePrimaryEmailAddress(ctx context.Context, u *user_model.User, emailStr string) error {
+	// FIXME: this check is from old logic, but it is not right, there are far more user types, not only "organization"
+	if u.IsOrganization() {
+		return util.NewInvalidArgumentErrorf("user %s is an organization", u.Name)
+	}
+
 	if strings.EqualFold(u.Email, emailStr) {
 		return nil
 	}
@@ -24,41 +31,38 @@ func ReplacePrimaryEmailAddress(ctx context.Context, u *user_model.User, emailSt
 	}
 
 	return db.WithTx(ctx, func(ctx context.Context) error {
-		if !u.IsOrganization() {
-			// 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 {
-				if email.IsPrimary && email.UID == u.ID {
-					return nil
-				}
-				return user_model.ErrEmailAlreadyUsed{Email: emailStr}
+		// 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 {
+			if email.IsPrimary && email.UID == u.ID {
+				return nil
 			}
+			return user_model.ErrEmailAlreadyUsed{Email: emailStr}
+		}
 
-			// Remove old primary address
-			primary, err := user_model.GetPrimaryEmailAddressOfUser(ctx, u.ID)
-			if err != nil {
-				return err
-			}
-			if _, err := db.DeleteByID[user_model.EmailAddress](ctx, primary.ID); err != nil {
-				return err
-			}
+		// Remove old primary address
+		primary, err := user_model.GetPrimaryEmailAddressOfUser(ctx, u.ID)
+		if err != nil {
+			return err
+		}
+		if _, err := db.DeleteByID[user_model.EmailAddress](ctx, primary.ID); err != nil {
+			return err
+		}
 
-			// Insert new primary address
-			if _, err := user_model.InsertEmailAddress(ctx, &user_model.EmailAddress{
-				UID:         u.ID,
-				Email:       emailStr,
-				IsActivated: true,
-				IsPrimary:   true,
-			}); err != nil {
-				return err
-			}
+		// Insert new primary address
+		if _, err := user_model.InsertEmailAddress(ctx, &user_model.EmailAddress{
+			UID:         u.ID,
+			Email:       emailStr,
+			IsActivated: true,
+			IsPrimary:   true,
+		}); err != nil {
+			return err
 		}
 
 		u.Email = emailStr
-
 		return user_model.UpdateUserCols(ctx, u, "email")
 	})
 }