From da7e85c8191a19a7f68e8e0b8ba91f7cf3298c9a Mon Sep 17 00:00:00 2001 From: JakobDev Date: Wed, 20 Nov 2024 12:31:34 +0000 Subject: [PATCH] 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 Reviewed-by: Otto Co-authored-by: JakobDev Co-committed-by: JakobDev (cherry picked from commit 45fa9e5ae9115df63313b2b46192c5a588557908) --- models/user/email_address.go | 32 +++++++++ models/user/email_address_test.go | 18 ++++++ modules/structs/org.go | 10 +-- routers/api/v1/org/org.go | 24 +++++-- routers/web/org/setting.go | 8 ++- templates/swagger/v1_json.tmpl | 3 + tests/integration/api_org_test.go | 54 ++++++++++++++++ tests/integration/org_settings_test.go | 89 ++++++++++++++++++++++++++ 8 files changed, 228 insertions(+), 10 deletions(-) create mode 100644 tests/integration/org_settings_test.go diff --git a/models/user/email_address.go b/models/user/email_address.go index 011c3edfa6..f7c9460f16 100644 --- a/models/user/email_address.go +++ b/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) diff --git a/models/user/email_address_test.go b/models/user/email_address_test.go index b918f21018..886e1715c3 100644 --- a/models/user/email_address_test.go +++ b/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) +} diff --git a/modules/structs/org.go b/modules/structs/org.go index b2b2c61a01..686345b2c3 100644 --- a/modules/structs/org.go +++ b/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)"` diff --git a/routers/api/v1/org/org.go b/routers/api/v1/org/org.go index 95c8c25d8e..b278281c35 100644 --- a/routers/api/v1/org/org.go +++ b/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 + } } } diff --git a/routers/web/org/setting.go b/routers/web/org/setting.go index 0be734abaf..8bd8ae6126 100644 --- a/routers/web/org/setting.go +++ b/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) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index d47f965286..218505d019 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -2263,6 +2263,9 @@ }, "404": { "$ref": "#/responses/notFound" + }, + "422": { + "$ref": "#/responses/error" } } } diff --git a/tests/integration/api_org_test.go b/tests/integration/api_org_test.go index 70d3a446f7..57bc6b5d7c 100644 --- a/tests/integration/api_org_test.go +++ b/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) + }) +} diff --git a/tests/integration/org_settings_test.go b/tests/integration/org_settings_test.go new file mode 100644 index 0000000000..56b7b02271 --- /dev/null +++ b/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) + }) +}