[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

@@ -902,8 +902,10 @@ func Routes() *web.Route {
 
 			m.Group("", func() {
 				m.Get("/list_blocked", user.ListBlockedUsers)
-				m.Put("/block/{username}", user.BlockUser)
+				m.Group("", func() {
-				m.Put("/unblock/{username}", user.UnblockUser)
+					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.Get("/list_blocked", org.ListBlockedUsers)
-				m.Put("/block/{username}", reqToken(), reqOrgOwnership(), org.BlockUser)
+				m.Group("", func() {
-				m.Put("/unblock/{username}", reqToken(), reqOrgOwnership(), org.UnblockUser)
+					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() {

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.ContextUser.IsOrganization() {
-	if ctx.Written() {
+		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.ContextUser.IsOrganization() {
-	if ctx.Written() {
+		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)
 }

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.ContextUser.IsOrganization() {
-	if ctx.Written() {
+		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.ContextUser.IsOrganization() {
-	if ctx.Written() {
+		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)
 }

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 {
+	u, err := user_model.GetUserByID(ctx, ctx.FormInt64("user_id"))
-		ctx.ServerError("BlockUser", err)
+	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
 	}
 

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":

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"
           }
         }
       }

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

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)
+	t.Run("Unblock", func(t *testing.T) {
-	req := NewRequestWithValues(t, "POST", "/"+blockedUser.Name, map[string]string{
+		defer tests.PrintCurrentTest(t)()
-		"_csrf":  GetCSRF(t, session, "/"+blockedUser.Name),
+		req := NewRequestWithValues(t, "POST", "/"+blockedUser.Name, map[string]string{
-		"action": "unblock",
+			"_csrf":  GetCSRF(t, session, "/"+blockedUser.Name),
-	})
+			"action": "unblock",
-	resp := session.MakeRequest(t, req, http.StatusSeeOther)
+		})
+		resp := session.MakeRequest(t, req, http.StatusSeeOther)
 
-	loc := resp.Header().Get("Location")
+		loc := resp.Header().Get("Location")
-	assert.EqualValues(t, "/"+blockedUser.Name, loc)
+		assert.EqualValues(t, "/"+blockedUser.Name, loc)
-	unittest.AssertNotExistsBean(t, &user_model.BlockedUser{BlockID: blockedUser.ID, UserID: doer.ID})
+		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)
+	t.Run("Doer follow blocked user", func(t *testing.T) {
-	req := NewRequestWithValues(t, "POST", "/"+blockedUser.Name, map[string]string{
+		defer tests.PrintCurrentTest(t)()
-		"_csrf":  GetCSRF(t, session, "/"+blockedUser.Name),
+		session := loginUser(t, doer.Name)
-		"action": "follow",
-	})
-	session.MakeRequest(t, req, http.StatusSeeOther)
 
-	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)
+	t.Run("Blocked user follow doer", func(t *testing.T) {
-	req = NewRequestWithValues(t, "POST", "/"+doer.Name, map[string]string{
+		defer tests.PrintCurrentTest(t)()
-		"_csrf":  GetCSRF(t, session, "/"+doer.Name),
+		session := loginUser(t, blockedUser.Name)
-		"action": "follow",
-	})
-	session.MakeRequest(t, req, http.StatusSeeOther)
 
-	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{
+	t.Run("Block user", func(t *testing.T) {
-		"_csrf":   GetCSRF(t, session, org.OrganisationLink()+"/settings/blocked_users"),
+		defer tests.PrintCurrentTest(t)()
-		"user_id": strconv.FormatInt(blockedUser.ID, 10),
+
+		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.