From 747be949a1b3cd06f6586512f1af4630e55d7ad4 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 21 Jul 2023 00:51:08 +0200 Subject: [PATCH] [MODERATION] Block adding collaborators - Ensure that the doer and blocked user cannot add each other as collaborators to repositories. - The Web UI gets an detailed message of the specific situation, the API gets an generic Forbidden code. - Unit tests has been added. - Integration testing for Web and API has been added. - This commit doesn't introduce removing each other as collaborators on the block action, due to the complexity of database calls that needs to be figured out. That deserves its own commit and test code. --- modules/repository/collaborator.go | 4 ++ modules/repository/collaborator_test.go | 27 ++++++++++++++ options/locale/locale_en-US.ini | 3 ++ routers/api/v1/repo/collaborators.go | 8 +++- routers/web/repo/setting/collaboration.go | 14 ++++++- routers/web/repo/setting/settings_test.go | 12 ++++-- templates/swagger/v1_json.tmpl | 3 ++ templates/user/profile.tmpl | 1 + tests/integration/api_block_test.go | 40 ++++++++++++++++++++ tests/integration/block_test.go | 45 +++++++++++++++++++++++ 10 files changed, 151 insertions(+), 6 deletions(-) diff --git a/modules/repository/collaborator.go b/modules/repository/collaborator.go index f5cdc35045..4099a178f2 100644 --- a/modules/repository/collaborator.go +++ b/modules/repository/collaborator.go @@ -14,6 +14,10 @@ import ( ) func AddCollaborator(ctx context.Context, repo *repo_model.Repository, u *user_model.User) error { + if user_model.IsBlocked(ctx, repo.OwnerID, u.ID) || user_model.IsBlocked(ctx, u.ID, repo.OwnerID) { + return user_model.ErrBlockedByUser + } + return db.WithTx(ctx, func(ctx context.Context) error { collaboration := &repo_model.Collaboration{ RepoID: repo.ID, diff --git a/modules/repository/collaborator_test.go b/modules/repository/collaborator_test.go index c772806819..b051aa2ac7 100644 --- a/modules/repository/collaborator_test.go +++ b/modules/repository/collaborator_test.go @@ -33,6 +33,33 @@ func TestRepository_AddCollaborator(t *testing.T) { testSuccess(3, 4) } +func TestRepository_AddCollaborator_IsBlocked(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + testSuccess := func(repoID, userID int64) { + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: repoID}) + assert.NoError(t, repo.LoadOwner(db.DefaultContext)) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: userID}) + + // Owner blocked user. + unittest.AssertSuccessfulInsert(t, &user_model.BlockedUser{UserID: repo.OwnerID, BlockID: userID}) + assert.ErrorIs(t, AddCollaborator(db.DefaultContext, repo, user), user_model.ErrBlockedByUser) + unittest.CheckConsistencyFor(t, &repo_model.Repository{ID: repoID}, &user_model.User{ID: userID}) + _, err := db.DeleteByBean(db.DefaultContext, &user_model.BlockedUser{UserID: repo.OwnerID, BlockID: userID}) + assert.NoError(t, err) + + // User has owner blocked. + unittest.AssertSuccessfulInsert(t, &user_model.BlockedUser{UserID: userID, BlockID: repo.OwnerID}) + assert.ErrorIs(t, AddCollaborator(db.DefaultContext, repo, user), user_model.ErrBlockedByUser) + unittest.CheckConsistencyFor(t, &repo_model.Repository{ID: repoID}, &user_model.User{ID: userID}) + } + // Ensure idempotency (public repository). + testSuccess(1, 4) + testSuccess(1, 4) + // Add collaborator to private repository. + testSuccess(3, 4) +} + func TestRepoPermissionPublicNonOrgRepo(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 997169632a..6bc9b5b7d3 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -607,6 +607,7 @@ block_user = Block User block_user.detail = Please understand that if you block this user, other actions will be taken. Such as: block_user.detail_1 = You are being unfollowed from this user. block_user.detail_2 = This user cannot interact with your repositories, created issues and comments. +block_user.detail_3 = This user cannot add you as a collaborator, nor can you add them as a collaborator. follow_blocked_user = You cannot follow this user because you have blocked this user or this user has blocked you. form.name_reserved = The username "%s" is reserved. @@ -2080,6 +2081,8 @@ settings.add_collaborator_success = The collaborator has been added. settings.add_collaborator_inactive_user = Cannot add an inactive user as a collaborator. settings.add_collaborator_owner = Cannot add an owner as a collaborator. settings.add_collaborator_duplicate = The collaborator is already added to this repository. +settings.add_collaborator_blocked_our = Cannot add the collaborator, because the repository owner has blocked them. +settings.add_collaborator_blocked_them = Cannot add the collaborator, because they have blocked the repository owner. settings.delete_collaborator = Remove settings.collaborator_deletion = Remove Collaborator settings.collaborator_deletion_desc = Removing a collaborator will revoke their access to this repository. Continue? diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index 942d4c799f..cfa45e9995 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -156,6 +156,8 @@ func AddCollaborator(ctx *context.APIContext) { // "$ref": "#/responses/empty" // "422": // "$ref": "#/responses/validationError" + // "403": + // "$ref": "#/responses/forbidden" form := web.GetForm(ctx).(*api.AddCollaboratorOption) @@ -175,7 +177,11 @@ func AddCollaborator(ctx *context.APIContext) { } if err := repo_module.AddCollaborator(ctx, ctx.Repo.Repository, collaborator); err != nil { - ctx.Error(http.StatusInternalServerError, "AddCollaborator", err) + if errors.Is(err, user_model.ErrBlockedByUser) { + ctx.Error(http.StatusForbidden, "AddCollaborator", err) + } else { + ctx.Error(http.StatusInternalServerError, "AddCollaborator", err) + } return } diff --git a/routers/web/repo/setting/collaboration.go b/routers/web/repo/setting/collaboration.go index 8f2d306862..a9dc47693d 100644 --- a/routers/web/repo/setting/collaboration.go +++ b/routers/web/repo/setting/collaboration.go @@ -4,6 +4,7 @@ package setting import ( + "errors" "net/http" "strings" @@ -102,7 +103,18 @@ func CollaborationPost(ctx *context.Context) { } if err = repo_module.AddCollaborator(ctx, ctx.Repo.Repository, u); err != nil { - ctx.ServerError("AddCollaborator", err) + if !errors.Is(err, user_model.ErrBlockedByUser) { + ctx.ServerError("AddCollaborator", err) + return + } + + // To give an good error message, be precise on who has blocked who. + if blockedOurs := user_model.IsBlocked(ctx, ctx.Repo.Repository.OwnerID, u.ID); blockedOurs { + ctx.Flash.Error(ctx.Tr("repo.settings.add_collaborator_blocked_our")) + } else { + ctx.Flash.Error(ctx.Tr("repo.settings.add_collaborator_blocked_them")) + } + ctx.Redirect(ctx.Repo.RepoLink + "/settings/collaboration") return } diff --git a/routers/web/repo/setting/settings_test.go b/routers/web/repo/setting/settings_test.go index 6f7c844ce7..46180ac372 100644 --- a/routers/web/repo/setting/settings_test.go +++ b/routers/web/repo/setting/settings_test.go @@ -102,13 +102,15 @@ func TestCollaborationPost(t *testing.T) { ctx.Req.Form.Set("collaborator", "user4") u := &user_model.User{ + ID: 2, LowerName: "user2", Type: user_model.UserTypeIndividual, } re := &repo_model.Repository{ - ID: 2, - Owner: u, + ID: 2, + Owner: u, + OwnerID: u.ID, } repo := &context.Repository{ @@ -160,13 +162,15 @@ func TestCollaborationPost_AddCollaboratorTwice(t *testing.T) { ctx.Req.Form.Set("collaborator", "user4") u := &user_model.User{ + ID: 2, LowerName: "user2", Type: user_model.UserTypeIndividual, } re := &repo_model.Repository{ - ID: 2, - Owner: u, + ID: 2, + Owner: u, + OwnerID: u.ID, } repo := &context.Repository{ diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 6f27cd9c76..eefe3b2c3d 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -3948,6 +3948,9 @@ "204": { "$ref": "#/responses/empty" }, + "403": { + "$ref": "#/responses/forbidden" + }, "422": { "$ref": "#/responses/validationError" } diff --git a/templates/user/profile.tmpl b/templates/user/profile.tmpl index 6a2f719a34..05e95a1a34 100644 --- a/templates/user/profile.tmpl +++ b/templates/user/profile.tmpl @@ -52,6 +52,7 @@ {{template "base/modal_actions_confirm" .}} diff --git a/tests/integration/api_block_test.go b/tests/integration/api_block_test.go index 2118080c7a..b07dd2455a 100644 --- a/tests/integration/api_block_test.go +++ b/tests/integration/api_block_test.go @@ -9,6 +9,7 @@ import ( "testing" auth_model "code.gitea.io/gitea/models/auth" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" api "code.gitea.io/gitea/modules/structs" @@ -99,3 +100,42 @@ func TestAPIOrgBlock(t *testing.T) { unittest.AssertNotExistsBean(t, &user_model.BlockedUser{UserID: 6, BlockID: 2}) }) } + +// TestAPIBlock_AddCollaborator ensures that the doer and blocked user cannot +// add each others as collaborators via the API. +func TestAPIBlock_AddCollaborator(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user1 := "user10" + user2 := "user2" + perm := "write" + collabOption := &api.AddCollaboratorOption{Permission: &perm} + + // User1 blocks User2. + session := loginUser(t, user1) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteUser, auth_model.AccessTokenScopeWriteRepository) + + req := NewRequest(t, "PUT", fmt.Sprintf("/api/v1/user/block/%s?token=%s", user2, token)) + MakeRequest(t, req, http.StatusNoContent) + unittest.AssertExistsAndLoadBean(t, &user_model.BlockedUser{UserID: 10, BlockID: 2}) + + t.Run("BlockedUser Add Doer", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2, OwnerID: 2}) + session := loginUser(t, user2) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + + req := NewRequestWithJSON(t, "PUT", fmt.Sprintf("/api/v1/repos/%s/%s/collaborators/%s?token=%s", user2, repo.Name, user1, token), collabOption) + session.MakeRequest(t, req, http.StatusForbidden) + }) + + t.Run("Doer Add BlockedUser", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 7, OwnerID: 10}) + session := loginUser(t, user1) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + + req := NewRequestWithJSON(t, "PUT", fmt.Sprintf("/api/v1/repos/%s/%s/collaborators/%s?token=%s", user1, repo.Name, user2, token), collabOption) + session.MakeRequest(t, req, http.StatusForbidden) + }) +} diff --git a/tests/integration/block_test.go b/tests/integration/block_test.go index b8001f4968..be1097f0f8 100644 --- a/tests/integration/block_test.go +++ b/tests/integration/block_test.go @@ -16,6 +16,7 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + forgejo_context "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/translation" "code.gitea.io/gitea/tests" @@ -210,3 +211,47 @@ func TestBlockUserFromOrganization(t *testing.T) { 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. +func TestBlockAddCollaborator(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 10}) + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + BlockUser(t, user1, user2) + + t.Run("BlockedUser Add Doer", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2, OwnerID: user2.ID}) + + session := loginUser(t, user2.Name) + req := NewRequestWithValues(t, "POST", path.Join(repo.Link(), "/settings/collaboration"), map[string]string{ + "_csrf": GetCSRF(t, session, path.Join(repo.Link(), "/settings/collaboration")), + "collaborator": user1.Name, + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + flashCookie := session.GetCookie(forgejo_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.EqualValues(t, "error%3DCannot%2Badd%2Bthe%2Bcollaborator%252C%2Bbecause%2Bthey%2Bhave%2Bblocked%2Bthe%2Brepository%2Bowner.", flashCookie.Value) + }) + + t.Run("Doer Add BlockedUser", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 7, OwnerID: user1.ID}) + + session := loginUser(t, user1.Name) + req := NewRequestWithValues(t, "POST", path.Join(repo.Link(), "/settings/collaboration"), map[string]string{ + "_csrf": GetCSRF(t, session, path.Join(repo.Link(), "/settings/collaboration")), + "collaborator": user2.Name, + }) + session.MakeRequest(t, req, http.StatusSeeOther) + + flashCookie := session.GetCookie(forgejo_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.EqualValues(t, "error%3DCannot%2Badd%2Bthe%2Bcollaborator%252C%2Bbecause%2Bthe%2Brepository%2Bowner%2Bhas%2Bblocked%2Bthem.", flashCookie.Value) + }) +}