fix: Allow Organisations to remove the Email Address (#5517)

It is possible to set a Email for a Organization. This Email is optional and only used to be displayed on the profile page. However, once you set an EMail, you can no longer remove it. This PR fixes that.

While working on the tests, I found out, that the API returns a 500 when trying to set an invalid EMail. I fixed that too. It returns a 422 now.

Fixes #4567

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/5517
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Reviewed-by: Otto <otto@codeberg.org>
Co-authored-by: JakobDev <jakobdev@gmx.de>
Co-committed-by: JakobDev <jakobdev@gmx.de>
(cherry picked from commit 45fa9e5ae9)

models/user/email_address.go

@@ -200,6 +200,38 @@ func GetPrimaryEmailAddressOfUser(ctx context.Context, uid int64) (*EmailAddress
 	return ea, nil
 }
 
+// Deletes the primary email address of the user
+// This is only allowed if the user is a organization
+func DeletePrimaryEmailAddressOfUser(ctx context.Context, uid int64) error {
+	user, err := GetUserByID(ctx, uid)
+	if err != nil {
+		return err
+	}
+
+	if user.Type != UserTypeOrganization {
+		return fmt.Errorf("%s is not a organization", user.Name)
+	}
+
+	ctx, committer, err := db.TxContext(ctx)
+	if err != nil {
+		return err
+	}
+	defer committer.Close()
+
+	_, err = db.GetEngine(ctx).Exec("DELETE FROM email_address WHERE uid = ? AND is_primary = true", uid)
+	if err != nil {
+		return err
+	}
+
+	user.Email = ""
+	err = UpdateUserCols(ctx, user, "email")
+	if err != nil {
+		return err
+	}
+
+	return committer.Commit()
+}
+
 // GetEmailAddresses returns all email addresses belongs to given user.
 func GetEmailAddresses(ctx context.Context, uid int64) ([]*EmailAddress, error) {
 	emails := make([]*EmailAddress, 0, 5)

models/user/email_address_test.go

@@ -220,3 +220,21 @@ func TestGetActivatedEmailAddresses(t *testing.T) {
 		})
 	}
 }
+
+func TestDeletePrimaryEmailAddressOfUser(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	user, err := user_model.GetUserByName(db.DefaultContext, "org3")
+	require.NoError(t, err)
+	assert.Equal(t, "org3@example.com", user.Email)
+
+	require.NoError(t, user_model.DeletePrimaryEmailAddressOfUser(db.DefaultContext, user.ID))
+
+	user, err = user_model.GetUserByName(db.DefaultContext, "org3")
+	require.NoError(t, err)
+	assert.Empty(t, user.Email)
+
+	email, err := user_model.GetPrimaryEmailAddressOfUser(db.DefaultContext, user.ID)
+	assert.True(t, user_model.IsErrEmailAddressNotExist(err))
+	assert.Nil(t, email)
+}

modules/structs/org.go

@@ -47,11 +47,11 @@ type CreateOrgOption struct {
 
 // EditOrgOption options for editing an organization
 type EditOrgOption struct {
-	FullName    string `json:"full_name" binding:"MaxSize(100)"`
-	Email       string `json:"email" binding:"MaxSize(255)"`
-	Description string `json:"description" binding:"MaxSize(255)"`
-	Website     string `json:"website" binding:"ValidUrl;MaxSize(255)"`
-	Location    string `json:"location" binding:"MaxSize(50)"`
+	FullName    string  `json:"full_name" binding:"MaxSize(100)"`
+	Email       *string `json:"email" binding:"MaxSize(255)"`
+	Description string  `json:"description" binding:"MaxSize(255)"`
+	Website     string  `json:"website" binding:"ValidUrl;MaxSize(255)"`
+	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)"`

routers/api/v1/org/org.go

@@ -15,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/validation"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/api/v1/user"
 	"code.gitea.io/gitea/routers/api/v1/utils"
@@ -340,13 +341,28 @@ func Edit(ctx *context.APIContext) {
 	//     "$ref": "#/responses/Organization"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
+	//   "422":
+	//     "$ref": "#/responses/error"
 
 	form := web.GetForm(ctx).(*api.EditOrgOption)
 
-	if form.Email != "" {
-		if err := user_service.ReplacePrimaryEmailAddress(ctx, ctx.Org.Organization.AsUser(), form.Email); err != nil {
-			ctx.Error(http.StatusInternalServerError, "ReplacePrimaryEmailAddress", err)
-			return
+	if form.Email != nil {
+		if *form.Email == "" {
+			err := user_model.DeletePrimaryEmailAddressOfUser(ctx, ctx.Org.Organization.ID)
+			if err != nil {
+				ctx.Error(http.StatusInternalServerError, "DeletePrimaryEmailAddressOfUser", err)
+				return
+			}
+			ctx.Org.Organization.Email = ""
+		} else {
+			if err := user_service.ReplacePrimaryEmailAddress(ctx, ctx.Org.Organization.AsUser(), *form.Email); err != nil {
+				if validation.IsErrEmailInvalid(err) || validation.IsErrEmailCharIsNotSupported(err) {
+					ctx.Error(http.StatusUnprocessableEntity, "ReplacePrimaryEmailAddress", err)
+				} else {
+					ctx.Error(http.StatusInternalServerError, "ReplacePrimaryEmailAddress", err)
+				}
+				return
+			}
 		}
 	}
 

routers/web/org/setting.go

@@ -93,7 +93,13 @@ func SettingsPost(ctx *context.Context) {
 		ctx.Org.OrgLink = setting.AppSubURL + "/org/" + url.PathEscape(org.Name)
 	}
 
-	if form.Email != "" {
+	if form.Email == "" {
+		err := user_model.DeletePrimaryEmailAddressOfUser(ctx, org.ID)
+		if err != nil {
+			ctx.ServerError("DeletePrimaryEmailAddressOfUser", err)
+			return
+		}
+	} else {
 		if err := user_service.ReplacePrimaryEmailAddress(ctx, org.AsUser(), form.Email); err != nil {
 			ctx.Data["Err_Email"] = true
 			ctx.RenderWithErr(ctx.Tr("form.email_invalid"), tplSettingsOptions, &form)

templates/swagger/v1_json.tmpl

@@ -2263,6 +2263,9 @@
           },
           "404": {
             "$ref": "#/responses/notFound"
+          },
+          "422": {
+            "$ref": "#/responses/error"
           }
         }
       }

tests/integration/api_org_test.go

@@ -226,3 +226,57 @@ func TestAPIOrgSearchEmptyTeam(t *testing.T) {
 		}
 	})
 }
+
+func TestAPIOrgChangeEmail(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	session := loginUser(t, "user1")
+	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteOrganization)
+
+	t.Run("Invalid", func(t *testing.T) {
+		newMail := "invalid"
+		settings := api.EditOrgOption{Email: &newMail}
+
+		resp := MakeRequest(t, NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &settings).AddTokenAuth(token), http.StatusUnprocessableEntity)
+
+		var org *api.Organization
+		DecodeJSON(t, resp, &org)
+
+		assert.Empty(t, org.Email)
+	})
+
+	t.Run("Valid", func(t *testing.T) {
+		newMail := "example@example.com"
+		settings := api.EditOrgOption{Email: &newMail}
+
+		resp := MakeRequest(t, NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &settings).AddTokenAuth(token), http.StatusOK)
+
+		var org *api.Organization
+		DecodeJSON(t, resp, &org)
+
+		assert.Equal(t, "example@example.com", org.Email)
+	})
+
+	t.Run("NoChange", func(t *testing.T) {
+		settings := api.EditOrgOption{}
+
+		resp := MakeRequest(t, NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &settings).AddTokenAuth(token), http.StatusOK)
+
+		var org *api.Organization
+		DecodeJSON(t, resp, &org)
+
+		assert.Equal(t, "example@example.com", org.Email)
+	})
+
+	t.Run("Empty", func(t *testing.T) {
+		newMail := ""
+		settings := api.EditOrgOption{Email: &newMail}
+
+		resp := MakeRequest(t, NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &settings).AddTokenAuth(token), http.StatusOK)
+
+		var org *api.Organization
+		DecodeJSON(t, resp, &org)
+
+		assert.Empty(t, org.Email)
+	})
+}

tests/integration/org_settings_test.go

@@ -0,0 +1,89 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"fmt"
+	"net/http"
+	"testing"
+
+	auth_model "code.gitea.io/gitea/models/auth"
+	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/tests"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func getOrgSettingsFormData(t *testing.T, session *TestSession, orgName string) map[string]string {
+	return map[string]string{
+		"_csrf":                         GetCSRF(t, session, fmt.Sprintf("/org/%s/settings", orgName)),
+		"name":                          orgName,
+		"full_name":                     "",
+		"email":                         "",
+		"description":                   "",
+		"website":                       "",
+		"location":                      "",
+		"visibility":                    "0",
+		"repo_admin_change_team_access": "on",
+		"max_repo_creation":             "-1",
+	}
+}
+
+func getOrgSettings(t *testing.T, token, orgName string) *api.Organization {
+	t.Helper()
+
+	req := NewRequestf(t, "GET", "/api/v1/orgs/%s", orgName).AddTokenAuth(token)
+	resp := MakeRequest(t, req, http.StatusOK)
+
+	var org *api.Organization
+	DecodeJSON(t, resp, &org)
+
+	return org
+}
+
+func TestOrgSettingsChangeEmail(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	const orgName = "org3"
+	settingsURL := fmt.Sprintf("/org/%s/settings", orgName)
+
+	session := loginUser(t, "user1")
+	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadOrganization)
+
+	t.Run("Invalid", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		settings := getOrgSettingsFormData(t, session, orgName)
+
+		settings["email"] = "invalid"
+		session.MakeRequest(t, NewRequestWithValues(t, "POST", settingsURL, settings), http.StatusOK)
+
+		org := getOrgSettings(t, token, orgName)
+		assert.Equal(t, "org3@example.com", org.Email)
+	})
+
+	t.Run("Valid", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		settings := getOrgSettingsFormData(t, session, orgName)
+
+		settings["email"] = "example@example.com"
+		session.MakeRequest(t, NewRequestWithValues(t, "POST", settingsURL, settings), http.StatusSeeOther)
+
+		org := getOrgSettings(t, token, orgName)
+		assert.Equal(t, "example@example.com", org.Email)
+	})
+
+	t.Run("Empty", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		settings := getOrgSettingsFormData(t, session, orgName)
+
+		settings["email"] = ""
+		session.MakeRequest(t, NewRequestWithValues(t, "POST", settingsURL, settings), http.StatusSeeOther)
+
+		org := getOrgSettings(t, token, orgName)
+		assert.Empty(t, org.Email)
+	})
+}