From e9d638d0756ee20b6bf1eb999c988533a5066a68 Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 7 Aug 2023 16:00:55 +0200 Subject: [PATCH] [MODERATION] QoL improvements (squash) - Ensure that organisations cannot be blocked. It currently has no effect, as all blocked operations cannot be executed from an organisation standpoint. - Refactored the API route to make use of the `UserAssignmentAPI` middleware. - Make more use of `t.Run` so that the test code is more clear about which block of code belongs to which test case. - Added more integration testing (to ensure the organisations cannot be blocked and some authorization/permission checks). --- routers/api/v1/api.go | 14 ++- routers/api/v1/org/org.go | 17 ++- routers/api/v1/user/user.go | 17 ++- routers/web/org/setting/blocked_users.go | 21 +++- routers/web/user/profile.go | 10 +- templates/swagger/v1_json.tmpl | 12 ++ tests/integration/api_block_test.go | 87 ++++++++++++++ tests/integration/block_test.go | 144 +++++++++++++++++------ 8 files changed, 266 insertions(+), 56 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 3cedbfd708..0bdfeb7a2b 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -902,8 +902,10 @@ func Routes() *web.Route { m.Group("", func() { m.Get("/list_blocked", user.ListBlockedUsers) - m.Put("/block/{username}", user.BlockUser) - m.Put("/unblock/{username}", user.UnblockUser) + m.Group("", func() { + m.Put("/block/{username}", user.BlockUser) + m.Put("/unblock/{username}", user.UnblockUser) + }, context_service.UserAssignmentAPI()) }) m.Group("/avatar", func() { @@ -1336,9 +1338,11 @@ func Routes() *web.Route { m.Get("/activities/feeds", org.ListOrgActivityFeeds) m.Group("", func() { - m.Get("/list_blocked", reqToken(), reqOrgOwnership(), org.ListBlockedUsers) - m.Put("/block/{username}", reqToken(), reqOrgOwnership(), org.BlockUser) - m.Put("/unblock/{username}", reqToken(), reqOrgOwnership(), org.UnblockUser) + m.Get("/list_blocked", org.ListBlockedUsers) + m.Group("", func() { + m.Put("/block/{username}", org.BlockUser) + m.Put("/unblock/{username}", org.UnblockUser) + }, context_service.UserAssignmentAPI()) }, reqToken(), reqOrgOwnership()) }, tokenRequiresScopes(auth_model.AccessTokenScopeCategoryOrganization), orgAssignment(true)) m.Group("/teams/{teamid}", func() { diff --git a/routers/api/v1/org/org.go b/routers/api/v1/org/org.go index 28afc8fc02..da3b5c6be5 100644 --- a/routers/api/v1/org/org.go +++ b/routers/api/v1/org/org.go @@ -5,6 +5,7 @@ package org import ( + "fmt" "net/http" activities_model "code.gitea.io/gitea/models/activities" @@ -499,13 +500,15 @@ func BlockUser(ctx *context.APIContext) { // "$ref": "#/responses/empty" // "404": // "$ref": "#/responses/notFound" + // "422": + // "$ref": "#/responses/validationError" - user := user.GetUserByParams(ctx) - if ctx.Written() { + if ctx.ContextUser.IsOrganization() { + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("%s is an organization not a user", ctx.ContextUser.Name)) return } - utils.BlockUser(ctx, ctx.Org.Organization.AsUser(), user) + utils.BlockUser(ctx, ctx.Org.Organization.AsUser(), ctx.ContextUser) } // UnblockUser unblocks a user from the organization. @@ -531,11 +534,13 @@ func UnblockUser(ctx *context.APIContext) { // "$ref": "#/responses/empty" // "404": // "$ref": "#/responses/notFound" + // "422": + // "$ref": "#/responses/validationError" - user := user.GetUserByParams(ctx) - if ctx.Written() { + if ctx.ContextUser.IsOrganization() { + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("%s is an organization not a user", ctx.ContextUser.Name)) return } - utils.UnblockUser(ctx, ctx.Org.Organization.AsUser(), user) + utils.UnblockUser(ctx, ctx.Org.Organization.AsUser(), ctx.ContextUser) } diff --git a/routers/api/v1/user/user.go b/routers/api/v1/user/user.go index 595ca04d62..48f8e9037a 100644 --- a/routers/api/v1/user/user.go +++ b/routers/api/v1/user/user.go @@ -5,6 +5,7 @@ package user import ( + "fmt" "net/http" activities_model "code.gitea.io/gitea/models/activities" @@ -244,13 +245,15 @@ func BlockUser(ctx *context.APIContext) { // "$ref": "#/responses/empty" // "404": // "$ref": "#/responses/notFound" + // "422": + // "$ref": "#/responses/validationError" - user := GetUserByParams(ctx) - if ctx.Written() { + if ctx.ContextUser.IsOrganization() { + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("%s is an organization not a user", ctx.ContextUser.Name)) return } - utils.BlockUser(ctx, ctx.Doer, user) + utils.BlockUser(ctx, ctx.Doer, ctx.ContextUser) } // UnblockUser unblocks a user from the doer. @@ -271,11 +274,13 @@ func UnblockUser(ctx *context.APIContext) { // "$ref": "#/responses/empty" // "404": // "$ref": "#/responses/notFound" + // "422": + // "$ref": "#/responses/validationError" - user := GetUserByParams(ctx) - if ctx.Written() { + if ctx.ContextUser.IsOrganization() { + ctx.Error(http.StatusUnprocessableEntity, "", fmt.Errorf("%s is an organization not a user", ctx.ContextUser.Name)) return } - utils.UnblockUser(ctx, ctx.Doer, user) + utils.UnblockUser(ctx, ctx.Doer, ctx.ContextUser) } diff --git a/routers/web/org/setting/blocked_users.go b/routers/web/org/setting/blocked_users.go index ebec1f1df5..9f0c868aa2 100644 --- a/routers/web/org/setting/blocked_users.go +++ b/routers/web/org/setting/blocked_users.go @@ -4,6 +4,7 @@ package setting import ( + "fmt" "net/http" "strings" @@ -41,6 +42,11 @@ func BlockedUsersBlock(ctx *context.Context) { return } + if u.IsOrganization() { + ctx.ServerError("IsOrganization", fmt.Errorf("%s is an organization not a user", u.Name)) + return + } + if err := user_service.BlockUser(ctx, ctx.Org.Organization.ID, u.ID); err != nil { ctx.ServerError("BlockUser", err) return @@ -52,8 +58,19 @@ func BlockedUsersBlock(ctx *context.Context) { // BlockedUsersUnblock unblocks a particular user from the organization. func BlockedUsersUnblock(ctx *context.Context) { - if err := user_model.UnblockUser(ctx, ctx.Org.Organization.ID, ctx.FormInt64("user_id")); err != nil { - ctx.ServerError("BlockUser", err) + u, err := user_model.GetUserByID(ctx, ctx.FormInt64("user_id")) + if err != nil { + ctx.ServerError("GetUserByID", err) + return + } + + if u.IsOrganization() { + ctx.ServerError("IsOrganization", fmt.Errorf("%s is an organization not a user", u.Name)) + return + } + + if err := user_model.UnblockUser(ctx, ctx.Org.Organization.ID, u.ID); err != nil { + ctx.ServerError("UnblockUser", err) return } diff --git a/routers/web/user/profile.go b/routers/web/user/profile.go index d165a63396..f7dd9df477 100644 --- a/routers/web/user/profile.go +++ b/routers/web/user/profile.go @@ -287,7 +287,15 @@ func prepareUserProfileTabData(ctx *context.Context, showPrivate bool, profileGi func Action(ctx *context.Context) { var err error var redirectViaJSON bool - switch ctx.FormString("action") { + action := ctx.FormString("action") + + if ctx.ContextUser.IsOrganization() && (action == "block" || action == "unblock") { + log.Error("Cannot perform this action on an organization %q", ctx.FormString("action")) + ctx.JSONError(fmt.Sprintf("Action %q failed", ctx.FormString("action"))) + return + } + + switch action { case "follow": err = user_model.FollowUser(ctx, ctx.Doer.ID, ctx.ContextUser.ID) case "unfollow": diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 5dcae18d95..b459827c8e 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -1687,6 +1687,9 @@ }, "404": { "$ref": "#/responses/notFound" + }, + "422": { + "$ref": "#/responses/validationError" } } } @@ -2589,6 +2592,9 @@ }, "404": { "$ref": "#/responses/notFound" + }, + "422": { + "$ref": "#/responses/validationError" } } } @@ -14100,6 +14106,9 @@ }, "404": { "$ref": "#/responses/notFound" + }, + "422": { + "$ref": "#/responses/validationError" } } } @@ -15210,6 +15219,9 @@ }, "404": { "$ref": "#/responses/notFound" + }, + "422": { + "$ref": "#/responses/validationError" } } } diff --git a/tests/integration/api_block_test.go b/tests/integration/api_block_test.go index b07dd2455a..48ee51bffa 100644 --- a/tests/integration/api_block_test.go +++ b/tests/integration/api_block_test.go @@ -58,6 +58,26 @@ func TestAPIUserBlock(t *testing.T) { unittest.AssertNotExistsBean(t, &user_model.BlockedUser{UserID: 4, BlockID: 2}) }) + + t.Run("Organization as target", func(t *testing.T) { + org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 26, Type: user_model.UserTypeOrganization}) + + t.Run("Block", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "PUT", fmt.Sprintf("/api/v1/user/block/%s?token=%s", org.Name, token)) + MakeRequest(t, req, http.StatusUnprocessableEntity) + + unittest.AssertNotExistsBean(t, &user_model.BlockedUser{UserID: 4, BlockID: org.ID}) + }) + + t.Run("Unblock", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "PUT", fmt.Sprintf("/api/v1/user/unblock/%s?token=%s", org.Name, token)) + MakeRequest(t, req, http.StatusUnprocessableEntity) + }) + }) } func TestAPIOrgBlock(t *testing.T) { @@ -99,6 +119,73 @@ func TestAPIOrgBlock(t *testing.T) { unittest.AssertNotExistsBean(t, &user_model.BlockedUser{UserID: 6, BlockID: 2}) }) + + t.Run("Organization as target", func(t *testing.T) { + targetOrg := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 26, Type: user_model.UserTypeOrganization}) + + t.Run("Block", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "PUT", fmt.Sprintf("/api/v1/orgs/%s/block/%s?token=%s", org, targetOrg.Name, token)) + MakeRequest(t, req, http.StatusUnprocessableEntity) + + unittest.AssertNotExistsBean(t, &user_model.BlockedUser{UserID: 4, BlockID: targetOrg.ID}) + }) + + t.Run("Unblock", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "PUT", fmt.Sprintf("/api/v1/orgs/%s/unblock/%s?token=%s", org, targetOrg.Name, token)) + MakeRequest(t, req, http.StatusUnprocessableEntity) + }) + }) + + t.Run("Read scope token", func(t *testing.T) { + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadOrganization) + + t.Run("Write action", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "PUT", fmt.Sprintf("/api/v1/orgs/%s/block/user2?token=%s", org, token)) + MakeRequest(t, req, http.StatusForbidden) + + unittest.AssertNotExistsBean(t, &user_model.BlockedUser{UserID: 6, BlockID: 2}) + }) + + t.Run("Read action", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/orgs/%s/list_blocked?token=%s", org, token)) + MakeRequest(t, req, http.StatusOK) + }) + }) + + t.Run("Not as owner", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + org := "user3" + user := "user4" // Part of org team with write perms. + + session := loginUser(t, user) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteOrganization) + + t.Run("Block user", func(t *testing.T) { + req := NewRequest(t, "PUT", fmt.Sprintf("/api/v1/orgs/%s/block/user2?token=%s", org, token)) + MakeRequest(t, req, http.StatusForbidden) + + unittest.AssertNotExistsBean(t, &user_model.BlockedUser{UserID: 3, BlockID: 2}) + }) + + t.Run("Unblock user", func(t *testing.T) { + req := NewRequest(t, "PUT", fmt.Sprintf("/api/v1/orgs/%s/unblock/user2?token=%s", org, token)) + MakeRequest(t, req, http.StatusForbidden) + }) + + t.Run("List blocked users", func(t *testing.T) { + req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/orgs/%s/list_blocked?token=%s", org, token)) + MakeRequest(t, req, http.StatusForbidden) + }) + }) } // TestAPIBlock_AddCollaborator ensures that the doer and blocked user cannot diff --git a/tests/integration/block_test.go b/tests/integration/block_test.go index be1097f0f8..1d36a99ef6 100644 --- a/tests/integration/block_test.go +++ b/tests/integration/block_test.go @@ -50,19 +50,51 @@ func TestBlockUser(t *testing.T) { doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 8}) blockedUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - BlockUser(t, doer, blockedUser) + session := loginUser(t, doer.Name) + + t.Run("Block", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + BlockUser(t, doer, blockedUser) + }) // Unblock user. - session := loginUser(t, doer.Name) - req := NewRequestWithValues(t, "POST", "/"+blockedUser.Name, map[string]string{ - "_csrf": GetCSRF(t, session, "/"+blockedUser.Name), - "action": "unblock", - }) - resp := session.MakeRequest(t, req, http.StatusSeeOther) + t.Run("Unblock", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequestWithValues(t, "POST", "/"+blockedUser.Name, map[string]string{ + "_csrf": GetCSRF(t, session, "/"+blockedUser.Name), + "action": "unblock", + }) + resp := session.MakeRequest(t, req, http.StatusSeeOther) - loc := resp.Header().Get("Location") - assert.EqualValues(t, "/"+blockedUser.Name, loc) - unittest.AssertNotExistsBean(t, &user_model.BlockedUser{BlockID: blockedUser.ID, UserID: doer.ID}) + loc := resp.Header().Get("Location") + assert.EqualValues(t, "/"+blockedUser.Name, loc) + unittest.AssertNotExistsBean(t, &user_model.BlockedUser{BlockID: blockedUser.ID, UserID: doer.ID}) + }) + + t.Run("Organization as target", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + targetOrg := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3, Type: user_model.UserTypeOrganization}) + + t.Run("Block", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", "/"+targetOrg.Name, map[string]string{ + "_csrf": GetCSRF(t, session, "/"+targetOrg.Name), + "action": "block", + }) + resp := session.MakeRequest(t, req, http.StatusBadRequest) + + assert.Contains(t, resp.Body.String(), "Action \\\"block\\\" failed") + }) + + t.Run("Unblock", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", "/"+targetOrg.Name, map[string]string{ + "_csrf": GetCSRF(t, session, "/"+targetOrg.Name), + "action": "unblock", + }) + resp := session.MakeRequest(t, req, http.StatusBadRequest) + + assert.Contains(t, resp.Body.String(), "Action \\\"unblock\\\" failed") + }) + }) } func TestBlockIssueCreation(t *testing.T) { @@ -167,24 +199,31 @@ func TestBlockFollow(t *testing.T) { BlockUser(t, doer, blockedUser) // Doer cannot follow blocked user. - session := loginUser(t, doer.Name) - req := NewRequestWithValues(t, "POST", "/"+blockedUser.Name, map[string]string{ - "_csrf": GetCSRF(t, session, "/"+blockedUser.Name), - "action": "follow", - }) - session.MakeRequest(t, req, http.StatusSeeOther) + t.Run("Doer follow blocked user", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + session := loginUser(t, doer.Name) - unittest.AssertNotExistsBean(t, &user_model.Follow{UserID: doer.ID, FollowID: blockedUser.ID}) + req := NewRequestWithValues(t, "POST", "/"+blockedUser.Name, map[string]string{ + "_csrf": GetCSRF(t, session, "/"+blockedUser.Name), + "action": "follow", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + unittest.AssertNotExistsBean(t, &user_model.Follow{UserID: doer.ID, FollowID: blockedUser.ID}) + }) // Blocked user cannot follow doer. - session = loginUser(t, blockedUser.Name) - req = NewRequestWithValues(t, "POST", "/"+doer.Name, map[string]string{ - "_csrf": GetCSRF(t, session, "/"+doer.Name), - "action": "follow", - }) - session.MakeRequest(t, req, http.StatusSeeOther) + t.Run("Blocked user follow doer", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + session := loginUser(t, blockedUser.Name) - unittest.AssertNotExistsBean(t, &user_model.Follow{UserID: blockedUser.ID, FollowID: doer.ID}) + req := NewRequestWithValues(t, "POST", "/"+doer.Name, map[string]string{ + "_csrf": GetCSRF(t, session, "/"+doer.Name), + "action": "follow", + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + unittest.AssertNotExistsBean(t, &user_model.Follow{UserID: blockedUser.ID, FollowID: doer.ID}) + }) } // TestBlockUserFromOrganization ensures that an organisation can block and unblock an user. @@ -195,21 +234,54 @@ func TestBlockUserFromOrganization(t *testing.T) { blockedUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 17, Type: user_model.UserTypeOrganization}) unittest.AssertNotExistsBean(t, &user_model.BlockedUser{BlockID: blockedUser.ID, UserID: org.ID}) - session := loginUser(t, doer.Name) - req := NewRequestWithValues(t, "POST", org.OrganisationLink()+"/settings/blocked_users/block", map[string]string{ - "_csrf": GetCSRF(t, session, org.OrganisationLink()+"/settings/blocked_users"), - "uname": blockedUser.Name, - }) - session.MakeRequest(t, req, http.StatusSeeOther) - assert.True(t, unittest.BeanExists(t, &user_model.BlockedUser{BlockID: blockedUser.ID, UserID: org.ID})) - req = NewRequestWithValues(t, "POST", org.OrganisationLink()+"/settings/blocked_users/unblock", map[string]string{ - "_csrf": GetCSRF(t, session, org.OrganisationLink()+"/settings/blocked_users"), - "user_id": strconv.FormatInt(blockedUser.ID, 10), + t.Run("Block user", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequestWithValues(t, "POST", org.OrganisationLink()+"/settings/blocked_users/block", map[string]string{ + "_csrf": GetCSRF(t, session, org.OrganisationLink()+"/settings/blocked_users"), + "uname": blockedUser.Name, + }) + session.MakeRequest(t, req, http.StatusSeeOther) + assert.True(t, unittest.BeanExists(t, &user_model.BlockedUser{BlockID: blockedUser.ID, UserID: org.ID})) + }) + + t.Run("Unblock user", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequestWithValues(t, "POST", org.OrganisationLink()+"/settings/blocked_users/unblock", map[string]string{ + "_csrf": GetCSRF(t, session, org.OrganisationLink()+"/settings/blocked_users"), + "user_id": strconv.FormatInt(blockedUser.ID, 10), + }) + session.MakeRequest(t, req, http.StatusSeeOther) + unittest.AssertNotExistsBean(t, &user_model.BlockedUser{BlockID: blockedUser.ID, UserID: org.ID}) + }) + + t.Run("Organization as target", func(t *testing.T) { + targetOrg := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3, Type: user_model.UserTypeOrganization}) + + t.Run("Block", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequestWithValues(t, "POST", org.OrganisationLink()+"/settings/blocked_users/block", map[string]string{ + "_csrf": GetCSRF(t, session, org.OrganisationLink()+"/settings/blocked_users"), + "uname": targetOrg.Name, + }) + session.MakeRequest(t, req, http.StatusInternalServerError) + unittest.AssertNotExistsBean(t, &user_model.BlockedUser{BlockID: blockedUser.ID, UserID: targetOrg.ID}) + }) + + t.Run("Unblock", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequestWithValues(t, "POST", org.OrganisationLink()+"/settings/blocked_users/unblock", map[string]string{ + "_csrf": GetCSRF(t, session, org.OrganisationLink()+"/settings/blocked_users"), + "user_id": strconv.FormatInt(targetOrg.ID, 10), + }) + session.MakeRequest(t, req, http.StatusInternalServerError) + }) }) - session.MakeRequest(t, req, http.StatusSeeOther) - unittest.AssertNotExistsBean(t, &user_model.BlockedUser{BlockID: blockedUser.ID, UserID: org.ID}) } // TestBlockAddCollaborator ensures that the doer and blocked user cannot add each each other as collaborators.