From 45fa9e5ae9115df63313b2b46192c5a588557908 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 --- 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 54667986ac..fdb3d64904 100644 --- a/models/user/email_address.go +++ b/models/user/email_address.go @@ -139,6 +139,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 b00b91c5f2..5653111fff 100644 --- a/models/user/email_address_test.go +++ b/models/user/email_address_test.go @@ -163,3 +163,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 3e3838ccc2..fd147f44a2 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 3a7c81ca3a..c26cf196de 100644 --- a/tests/integration/api_org_test.go +++ b/tests/integration/api_org_test.go @@ -218,3 +218,57 @@ func TestAPIOrgSearchEmptyTeam(t *testing.T) { assert.EqualValues(t, "Empty", data.Data[0].Name) } } + +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) + }) +}