From 735818863c1793aa6f6983afedc4bd3b36026ca5 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 13 Sep 2023 20:11:32 +0200 Subject: [PATCH] [MODERATION] Add repo transfers to blocked functionality (squash) - When someone gets blocked, remove all pending repository transfers from the blocked user to the doer. - Do not allow to start transferring repositories to the doer as blocked user. - Added unit testing. - Added integration testing. (cherry picked from commit 8a3caac33013482ddbee2fa51510c6918ba54466) (cherry picked from commit a92b4cfeb63b90eb2d90d0feb51cec62e0502d84) (cherry picked from commit acaaaf07d999974dbe5f9c5e792621c597bfb542) --- models/fixtures/repository.yml | 2 +- models/repo_transfer.go | 10 ++++++++ models/repo_transfer_test.go | 13 ++++++++++ options/locale/locale_en-US.ini | 1 + routers/api/v1/repo/transfer.go | 6 +++++ routers/web/repo/setting/setting.go | 5 +++- services/repository/transfer.go | 4 +++ services/repository/transfer_test.go | 2 +- services/user/block.go | 25 +++++++++++++++++++ services/user/block_test.go | 18 ++++++++++++++ tests/integration/block_test.go | 37 +++++++++++++++++++++------- 11 files changed, 111 insertions(+), 12 deletions(-) diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index 382d7c14ac..c74fb716bc 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -83,7 +83,7 @@ is_empty: false is_archived: false is_mirror: false - status: 0 + status: 2 is_fork: false fork_id: 0 is_template: false diff --git a/models/repo_transfer.go b/models/repo_transfer.go index 630c243c8e..69c531ff8c 100644 --- a/models/repo_transfer.go +++ b/models/repo_transfer.go @@ -417,3 +417,13 @@ func TransferOwnership(ctx context.Context, doer *user_model.User, newOwnerName return committer.Commit() } + +// GetPendingTransfers returns the pending transfers of recipient which were sent by by doer. +func GetPendingTransferIDs(ctx context.Context, reciepientID, doerID int64) ([]int64, error) { + pendingTransferIDs := make([]int64, 0, 8) + return pendingTransferIDs, db.GetEngine(ctx).Table("repo_transfer"). + Where("doer_id = ?", doerID). + And("recipient_id = ?", reciepientID). + Cols("id"). + Find(&pendingTransferIDs) +} diff --git a/models/repo_transfer_test.go b/models/repo_transfer_test.go index b55cef9473..41c6c214f9 100644 --- a/models/repo_transfer_test.go +++ b/models/repo_transfer_test.go @@ -55,3 +55,16 @@ func TestRepositoryTransfer(t *testing.T) { // Cancel transfer assert.NoError(t, CancelRepositoryTransfer(db.DefaultContext, repo)) } + +func TestGetPendingTransferIDs(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) + reciepient := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + pendingTransfer := unittest.AssertExistsAndLoadBean(t, &RepoTransfer{RecipientID: reciepient.ID, DoerID: doer.ID}) + + pendingTransferIDs, err := GetPendingTransferIDs(db.DefaultContext, reciepient.ID, doer.ID) + assert.NoError(t, err) + if assert.Len(t, pendingTransferIDs, 1) { + assert.EqualValues(t, pendingTransfer.ID, pendingTransferIDs[0]) + } +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 7f5c4570b4..eb8a18c60a 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2079,6 +2079,7 @@ settings.reindex_requested=Reindex Requested settings.admin_enable_close_issues_via_commit_in_any_branch = Close an issue via a commit made in a non default branch settings.danger_zone = Danger Zone settings.new_owner_has_same_repo = The new owner already has a repository with same name. Please choose another name. +settings.new_owner_blocked_doer = The new owner has blocked you. settings.convert = Convert to Regular Repository settings.convert_desc = You can convert this mirror into a regular repository. This cannot be undone. settings.convert_notices_1 = This operation will convert the mirror into a regular repository and cannot be undone. diff --git a/routers/api/v1/repo/transfer.go b/routers/api/v1/repo/transfer.go index 326895918e..bf0f5b1a14 100644 --- a/routers/api/v1/repo/transfer.go +++ b/routers/api/v1/repo/transfer.go @@ -4,6 +4,7 @@ package repo import ( + "errors" "fmt" "net/http" @@ -107,6 +108,11 @@ func Transfer(ctx *context.APIContext) { oldFullname := ctx.Repo.Repository.FullName() if err := repo_service.StartRepositoryTransfer(ctx, ctx.Doer, newOwner, ctx.Repo.Repository, teams); err != nil { + if errors.Is(err, user_model.ErrBlockedByUser) { + ctx.Error(http.StatusForbidden, "StartRepositoryTransfer", err) + return + } + if models.IsErrRepoTransferInProgress(err) { ctx.Error(http.StatusConflict, "StartRepositoryTransfer", err) return diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index 78028190fe..755ee341c8 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -5,6 +5,7 @@ package setting import ( + "errors" "fmt" "net/http" "strconv" @@ -775,7 +776,9 @@ func SettingsPost(ctx *context.Context) { } if err := repo_service.StartRepositoryTransfer(ctx, ctx.Doer, newOwner, repo, nil); err != nil { - if repo_model.IsErrRepoAlreadyExist(err) { + if errors.Is(err, user_model.ErrBlockedByUser) { + ctx.RenderWithErr(ctx.Tr("repo.settings.new_owner_blocked_doer"), tplSettingsOptions, nil) + } else if repo_model.IsErrRepoAlreadyExist(err) { ctx.RenderWithErr(ctx.Tr("repo.settings.new_owner_has_same_repo"), tplSettingsOptions, nil) } else if models.IsErrRepoTransferInProgress(err) { ctx.RenderWithErr(ctx.Tr("repo.settings.transfer_in_progress"), tplSettingsOptions, nil) diff --git a/services/repository/transfer.go b/services/repository/transfer.go index 574b6c6a56..f4f301994d 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -85,6 +85,10 @@ func ChangeRepositoryName(ctx context.Context, doer *user_model.User, repo *repo // StartRepositoryTransfer transfer a repo from one owner to a new one. // it make repository into pending transfer state, if doer can not create repo for new owner. func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.User, repo *repo_model.Repository, teams []*organization.Team) error { + if user_model.IsBlocked(ctx, newOwner.ID, doer.ID) { + return user_model.ErrBlockedByUser + } + if err := models.TestRepositoryReadyForTransfer(repo.Status); err != nil { return err } diff --git a/services/repository/transfer_test.go b/services/repository/transfer_test.go index d55c76ea47..c2e38e3ef1 100644 --- a/services/repository/transfer_test.go +++ b/services/repository/transfer_test.go @@ -63,7 +63,7 @@ func TestStartRepositoryTransferSetPermission(t *testing.T) { doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) recipient := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) - repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 5}) repo.Owner = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) hasAccess, err := access_model.HasAccess(db.DefaultContext, recipient.ID, repo) diff --git a/services/user/block.go b/services/user/block.go index 06cdd27176..0b31119dfb 100644 --- a/services/user/block.go +++ b/services/user/block.go @@ -5,9 +5,12 @@ package user import ( "context" + model "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" + + "xorm.io/builder" ) // BlockUser adds a blocked user entry for userID to block blockID. @@ -66,5 +69,27 @@ func BlockUser(ctx context.Context, userID, blockID int64) error { return err } + // Remove pending repository transfers, and set the status on those repository + // back to ready. + pendingTransfersIDs, err := model.GetPendingTransferIDs(ctx, userID, blockID) + if err != nil { + return err + } + + // Use a subquery instead of a JOIN, because not every database supports JOIN + // on a UPDATE query. + _, err = db.GetEngine(ctx).Table("repository"). + In("id", builder.Select("repo_id").From("repo_transfer").Where(builder.In("id", pendingTransfersIDs))). + Cols("status"). + Update(&repo_model.Repository{Status: repo_model.RepositoryReady}) + if err != nil { + return err + } + + _, err = db.GetEngine(ctx).In("id", pendingTransfersIDs).Delete(&model.RepoTransfer{}) + if err != nil { + return err + } + return committer.Commit() } diff --git a/services/user/block_test.go b/services/user/block_test.go index 245dd959b9..121c1ea8b7 100644 --- a/services/user/block_test.go +++ b/services/user/block_test.go @@ -6,6 +6,7 @@ package user import ( "testing" + model "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" @@ -70,4 +71,21 @@ func TestBlockUser(t *testing.T) { assert.False(t, isBlockedUserCollab(repo1)) assert.False(t, isBlockedUserCollab(repo2)) }) + + t.Run("Pending transfers", func(t *testing.T) { + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + blockedUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) + defer user_model.UnblockUser(db.DefaultContext, doer.ID, blockedUser.ID) + + unittest.AssertExistsIf(t, true, &repo_model.Repository{ID: 3, OwnerID: blockedUser.ID, Status: repo_model.RepositoryPendingTransfer}) + unittest.AssertExistsIf(t, true, &model.RepoTransfer{ID: 1, RecipientID: doer.ID, DoerID: blockedUser.ID}) + + assert.NoError(t, BlockUser(db.DefaultContext, doer.ID, blockedUser.ID)) + + unittest.AssertExistsIf(t, false, &model.RepoTransfer{ID: 1, RecipientID: doer.ID, DoerID: blockedUser.ID}) + + // Don't use AssertExistsIf, as it doesn't include the zero values in the condition such as `repo_model.RepositoryReady`. + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3, OwnerID: blockedUser.ID}) + assert.Equal(t, repo_model.RepositoryReady, repo.Status) + }) } diff --git a/tests/integration/block_test.go b/tests/integration/block_test.go index fee6d4b6f9..cb11420140 100644 --- a/tests/integration/block_test.go +++ b/tests/integration/block_test.go @@ -162,7 +162,9 @@ func TestBlockActions(t *testing.T) { doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) blockedUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + blockedUser2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 10}) repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2, OwnerID: doer.ID}) + repo7 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 7, OwnerID: blockedUser2.ID}) issue4 := unittest.AssertExistsAndLoadBean(t, &issue_model.Issue{ID: 4, RepoID: repo2.ID}) issue4URL := fmt.Sprintf("/%s/issues/%d", repo2.FullName(), issue4.Index) // NOTE: Sessions shouldn't be shared, because in some situations flash @@ -170,6 +172,7 @@ func TestBlockActions(t *testing.T) { // results. BlockUser(t, doer, blockedUser) + BlockUser(t, doer, blockedUser2) // Ensures that issue creation on doer's ownen repositories are blocked. t.Run("Issue creation", func(t *testing.T) { @@ -326,10 +329,6 @@ func TestBlockActions(t *testing.T) { // Ensures that the doer and blocked user cannot add each each other as collaborators. t.Run("Add collaborator", func(t *testing.T) { - blockedUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 10}) - - BlockUser(t, doer, blockedUser) - t.Run("Doer Add BlockedUser", func(t *testing.T) { defer tests.PrintCurrentTest(t)() @@ -338,7 +337,7 @@ func TestBlockActions(t *testing.T) { req := NewRequestWithValues(t, "POST", link, map[string]string{ "_csrf": GetCSRF(t, session, link), - "collaborator": blockedUser.Name, + "collaborator": blockedUser2.Name, }) session.MakeRequest(t, req, http.StatusSeeOther) @@ -350,10 +349,8 @@ func TestBlockActions(t *testing.T) { t.Run("BlockedUser Add doer", func(t *testing.T) { defer tests.PrintCurrentTest(t)() - repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 7, OwnerID: blockedUser.ID}) - - session := loginUser(t, blockedUser.Name) - link := fmt.Sprintf("/%s/settings/collaboration", repo.FullName()) + session := loginUser(t, blockedUser2.Name) + link := fmt.Sprintf("/%s/settings/collaboration", repo7.FullName()) req := NewRequestWithValues(t, "POST", link, map[string]string{ "_csrf": GetCSRF(t, session, link), @@ -366,4 +363,26 @@ func TestBlockActions(t *testing.T) { assert.EqualValues(t, "error%3DCannot%2Badd%2Bthe%2Bcollaborator%252C%2Bbecause%2Bthey%2Bhave%2Bblocked%2Bthe%2Brepository%2Bowner.", flashCookie.Value) }) }) + + // Ensures that the blocked user cannot transfer a repository to the doer. + t.Run("Repository transfer", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + session := loginUser(t, blockedUser2.Name) + link := fmt.Sprintf("%s/settings", repo7.FullName()) + + req := NewRequestWithValues(t, "POST", link, map[string]string{ + "_csrf": GetCSRF(t, session, link), + "action": "transfer", + "repo_name": repo7.Name, + "new_owner_name": doer.Name, + }) + resp := session.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + assert.Contains(t, + htmlDoc.doc.Find(".ui.negative.message").Text(), + translation.NewLocale("en-US").Tr("repo.settings.new_owner_blocked_doer"), + ) + }) }