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>
2026-03-25 08:23:11 +01:00
parent 943ff75233
commit e24c3f7a40
12 changed files with 232 additions and 164 deletions
--- a/services/forms/org.go
+++ b/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
--- a/services/org/org.go
+++ b/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")
+}
--- a/services/org/org_test.go
+++ b/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})
+func TestOrg(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())

-	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))
+	t.Run("UpdateOrgEmailAddress", func(t *testing.T) {
+		org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
+		originalEmail := org.Email

-	user := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 5})
-	assert.Error(t, DeleteOrganization(t.Context(), user, false))
-	unittest.CheckConsistencyFor(t, &user_model.User{}, &organization.Team{})
+		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{})
+	})
 }
--- a/services/user/email.go
+++ b/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")
 	})
 }
--- a/services/user/email_test.go
+++ b/services/user/email_test.go
@@ -6,17 +6,16 @@ package user
 import (
 	"testing"

-	organization_model "code.gitea.io/gitea/models/organization"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"

 	"github.com/stretchr/testify/assert"
 )

-func TestReplacePrimaryEmailAddress(t *testing.T) {
+func TestUserEmail(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())

-	t.Run("User", func(t *testing.T) {
+	t.Run("PrimaryEmailAddress", func(t *testing.T) {
 		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 13})

 		emails, err := user_model.GetEmailAddresses(t.Context(), user.ID)
@@ -42,50 +41,36 @@ func TestReplacePrimaryEmailAddress(t *testing.T) {
 		assert.NoError(t, ReplacePrimaryEmailAddress(t.Context(), user, "primary-13@example.com"))
 	})

-	t.Run("Organization", func(t *testing.T) {
-		org := unittest.AssertExistsAndLoadBean(t, &organization_model.Organization{ID: 3})
+	t.Run("AddEmailAddresses", func(t *testing.T) {
+		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})

-		assert.Equal(t, "org3@example.com", org.Email)
+		assert.Error(t, AddEmailAddresses(t.Context(), user, []string{" invalid email "}))

-		assert.NoError(t, ReplacePrimaryEmailAddress(t.Context(), org.AsUser(), "primary-org@example.com"))
+		emails := []string{"user1234@example.com", "user5678@example.com"}

-		assert.Equal(t, "primary-org@example.com", org.Email)
+		assert.NoError(t, AddEmailAddresses(t.Context(), user, emails))
+
+		err := AddEmailAddresses(t.Context(), user, emails)
+		assert.Error(t, err)
+		assert.True(t, user_model.IsErrEmailAlreadyUsed(err))
+	})
+
+	t.Run("DeleteEmailAddresses", func(t *testing.T) {
+		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+		emails := []string{"user2-2@example.com"}
+
+		err := DeleteEmailAddresses(t.Context(), user, emails)
+		assert.NoError(t, err)
+
+		err = DeleteEmailAddresses(t.Context(), user, emails)
+		assert.Error(t, err)
+		assert.True(t, user_model.IsErrEmailAddressNotExist(err))
+
+		emails = []string{"user2@example.com"}
+
+		err = DeleteEmailAddresses(t.Context(), user, emails)
+		assert.Error(t, err)
+		assert.True(t, user_model.IsErrPrimaryEmailCannotDelete(err))
 	})
 }
-
-func TestAddEmailAddresses(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-
-	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
-
-	assert.Error(t, AddEmailAddresses(t.Context(), user, []string{" invalid email "}))
-
-	emails := []string{"user1234@example.com", "user5678@example.com"}
-
-	assert.NoError(t, AddEmailAddresses(t.Context(), user, emails))
-
-	err := AddEmailAddresses(t.Context(), user, emails)
-	assert.Error(t, err)
-	assert.True(t, user_model.IsErrEmailAlreadyUsed(err))
-}
-
-func TestDeleteEmailAddresses(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-
-	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
-
-	emails := []string{"user2-2@example.com"}
-
-	err := DeleteEmailAddresses(t.Context(), user, emails)
-	assert.NoError(t, err)
-
-	err = DeleteEmailAddresses(t.Context(), user, emails)
-	assert.Error(t, err)
-	assert.True(t, user_model.IsErrEmailAddressNotExist(err))
-
-	emails = []string{"user2@example.com"}
-
-	err = DeleteEmailAddresses(t.Context(), user, emails)
-	assert.Error(t, err)
-	assert.True(t, user_model.IsErrPrimaryEmailCannotDelete(err))
-}