From 5acc28967257104c9a0656fb74f8ffc204681339 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 10 Jan 2025 21:11:26 -0800 Subject: [PATCH 01/13] refactor transfer related code --- models/repo/transfer.go | 4 +- routers/api/v1/repo/transfer.go | 33 +---- routers/web/repo/repo.go | 50 ++------ routers/web/repo/setting/setting.go | 2 +- services/context/repo.go | 2 +- services/repository/transfer.go | 174 +++++++++++++++------------ services/repository/transfer_test.go | 6 +- services/user/block.go | 8 +- 8 files changed, 123 insertions(+), 156 deletions(-) diff --git a/models/repo/transfer.go b/models/repo/transfer.go index 43e15b33bcfc1..36fbd42377319 100644 --- a/models/repo/transfer.go +++ b/models/repo/transfer.go @@ -117,10 +117,10 @@ func (r *RepoTransfer) LoadAttributes(ctx context.Context) error { return nil } -// CanUserAcceptTransfer checks if the user has the rights to accept/decline a repo transfer. +// CanUserAcceptOrRejectTransfer checks if the user has the rights to accept/decline a repo transfer. // For user, it checks if it's himself // For organizations, it checks if the user is able to create repos -func (r *RepoTransfer) CanUserAcceptTransfer(ctx context.Context, u *user_model.User) bool { +func (r *RepoTransfer) CanUserAcceptOrRejectTransfer(ctx context.Context, u *user_model.User) bool { if err := r.LoadAttributes(ctx); err != nil { log.Error("LoadAttributes: %v", err) return false diff --git a/routers/api/v1/repo/transfer.go b/routers/api/v1/repo/transfer.go index b2090cac41cad..01f3a4d2c9bf9 100644 --- a/routers/api/v1/repo/transfer.go +++ b/routers/api/v1/repo/transfer.go @@ -161,7 +161,7 @@ func AcceptTransfer(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - err := acceptOrRejectRepoTransfer(ctx, true) + err := repo_service.AcceptTransferOwnership(ctx, ctx.Repo.Repository, ctx.Doer) if ctx.Written() { return } @@ -199,10 +199,7 @@ func RejectTransfer(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - err := acceptOrRejectRepoTransfer(ctx, false) - if ctx.Written() { - return - } + err := repo_service.RejectRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer) if err != nil { ctx.Error(http.StatusInternalServerError, "acceptOrRejectRepoTransfer", err) return @@ -210,29 +207,3 @@ func RejectTransfer(ctx *context.APIContext) { ctx.JSON(http.StatusOK, convert.ToRepo(ctx, ctx.Repo.Repository, ctx.Repo.Permission)) } - -func acceptOrRejectRepoTransfer(ctx *context.APIContext, accept bool) error { - repoTransfer, err := repo_model.GetPendingRepositoryTransfer(ctx, ctx.Repo.Repository) - if err != nil { - if repo_model.IsErrNoPendingTransfer(err) { - ctx.NotFound() - return nil - } - return err - } - - if err := repoTransfer.LoadAttributes(ctx); err != nil { - return err - } - - if !repoTransfer.CanUserAcceptTransfer(ctx, ctx.Doer) { - ctx.Error(http.StatusForbidden, "CanUserAcceptTransfer", nil) - return fmt.Errorf("user does not have permissions to do this") - } - - if accept { - return repo_service.TransferOwnership(ctx, repoTransfer.Doer, repoTransfer.Recipient, ctx.Repo.Repository, repoTransfer.Teams) - } - - return repo_service.CancelRepositoryTransfer(ctx, ctx.Repo.Repository) -} diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index e5c397ec5415e..066643302300d 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -322,9 +322,17 @@ func Action(ctx *context.Context) { case "unstar": err = repo_model.StarRepo(ctx, ctx.Doer, ctx.Repo.Repository, false) case "accept_transfer": - err = acceptOrRejectRepoTransfer(ctx, true) + err = repo_service.AcceptTransferOwnership(ctx, ctx.Repo.Repository, ctx.Doer) + if err == nil { + ctx.Flash.Success(ctx.Tr("repo.settings.transfer.success")) + } + ctx.Redirect(ctx.Repo.Repository.Link()) case "reject_transfer": - err = acceptOrRejectRepoTransfer(ctx, false) + err = repo_service.RejectRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer) + if err == nil { + ctx.Flash.Success(ctx.Tr("repo.settings.transfer.rejected")) + } + ctx.Redirect(ctx.Repo.Repository.Link()) case "desc": // FIXME: this is not used if !ctx.Repo.IsOwner() { ctx.Error(http.StatusNotFound) @@ -339,6 +347,9 @@ func Action(ctx *context.Context) { if err != nil { if errors.Is(err, user_model.ErrBlockedUser) { ctx.Flash.Error(ctx.Tr("repo.action.blocked_user")) + } else if errors.Is(err, util.ErrPermissionDenied) { + ctx.Error(http.StatusNotFound) + return } else { ctx.ServerError(fmt.Sprintf("Action (%s)", ctx.PathParam("action")), err) return @@ -377,41 +388,6 @@ func Action(ctx *context.Context) { ctx.RedirectToCurrentSite(ctx.FormString("redirect_to"), ctx.Repo.RepoLink) } -func acceptOrRejectRepoTransfer(ctx *context.Context, accept bool) error { - repoTransfer, err := repo_model.GetPendingRepositoryTransfer(ctx, ctx.Repo.Repository) - if err != nil { - return err - } - - if err := repoTransfer.LoadAttributes(ctx); err != nil { - return err - } - - if !repoTransfer.CanUserAcceptTransfer(ctx, ctx.Doer) { - return errors.New("user does not have enough permissions") - } - - if accept { - if ctx.Repo.GitRepo != nil { - ctx.Repo.GitRepo.Close() - ctx.Repo.GitRepo = nil - } - - if err := repo_service.TransferOwnership(ctx, repoTransfer.Doer, repoTransfer.Recipient, ctx.Repo.Repository, repoTransfer.Teams); err != nil { - return err - } - ctx.Flash.Success(ctx.Tr("repo.settings.transfer.success")) - } else { - if err := repo_service.CancelRepositoryTransfer(ctx, ctx.Repo.Repository); err != nil { - return err - } - ctx.Flash.Success(ctx.Tr("repo.settings.transfer.rejected")) - } - - ctx.Redirect(ctx.Repo.Repository.Link()) - return nil -} - // RedirectDownload return a file based on the following infos: func RedirectDownload(ctx *context.Context) { var ( diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index 7399c681e2e9d..20c5b5478be58 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -830,7 +830,7 @@ func SettingsPost(ctx *context.Context) { return } - if err := repo_service.CancelRepositoryTransfer(ctx, ctx.Repo.Repository); err != nil { + if err := repo_service.RejectRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer); err != nil { ctx.ServerError("CancelRepositoryTransfer", err) return } diff --git a/services/context/repo.go b/services/context/repo.go index 05f6bb40f94ae..bed6e4c7004a0 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -674,7 +674,7 @@ func RepoAssignment(ctx *Context) { ctx.Data["RepoTransfer"] = repoTransfer if ctx.Doer != nil { - ctx.Data["CanUserAcceptTransfer"] = repoTransfer.CanUserAcceptTransfer(ctx, ctx.Doer) + ctx.Data["CanUserAcceptTransfer"] = repoTransfer.CanUserAcceptOrRejectTransfer(ctx, ctx.Doer) } } diff --git a/services/repository/transfer.go b/services/repository/transfer.go index 9ef28ddeb9fa6..ad12073cc9677 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -27,19 +27,8 @@ func getRepoWorkingLockKey(repoID int64) string { return fmt.Sprintf("repo_working_%d", repoID) } -// TransferOwnership transfers all corresponding setting from old user to new one. -func TransferOwnership(ctx context.Context, doer, newOwner *user_model.User, repo *repo_model.Repository, teams []*organization.Team) error { - if err := repo.LoadOwner(ctx); err != nil { - return err - } - for _, team := range teams { - if newOwner.ID != team.OrgID { - return fmt.Errorf("team %d does not belong to organization", team.ID) - } - } - - oldOwner := repo.Owner - +// AcceptTransferOwnership transfers all corresponding setting from old user to new one. +func AcceptTransferOwnership(ctx context.Context, repo *repo_model.Repository, doer *user_model.User) error { releaser, err := globallock.Lock(ctx, getRepoWorkingLockKey(repo.ID)) if err != nil { log.Error("lock.Lock(): %v", err) @@ -47,29 +36,42 @@ func TransferOwnership(ctx context.Context, doer, newOwner *user_model.User, rep } defer releaser() - if err := transferOwnership(ctx, doer, newOwner.Name, repo); err != nil { - return err - } - releaser() - - newRepo, err := repo_model.GetRepositoryByID(ctx, repo.ID) + repoTransfer, err := repo_model.GetPendingRepositoryTransfer(ctx, repo) if err != nil { return err } - for _, team := range teams { - if err := addRepositoryToTeam(ctx, team, newRepo); err != nil { + if err := db.WithTx(ctx, func(ctx context.Context) error { + if err := repoTransfer.LoadAttributes(ctx); err != nil { return err } + + if !repoTransfer.CanUserAcceptOrRejectTransfer(ctx, doer) { + return util.ErrPermissionDenied + } + + if err := repo.LoadOwner(ctx); err != nil { + return err + } + for _, team := range repoTransfer.Teams { + if repoTransfer.Recipient.ID != team.OrgID { + return fmt.Errorf("team %d does not belong to organization", team.ID) + } + } + + return transferOwnership(ctx, repoTransfer.Doer, repoTransfer.Recipient.Name, repo, repoTransfer.Teams) + }); err != nil { + return err } + releaser() - notify_service.TransferRepository(ctx, doer, repo, oldOwner.Name) + notify_service.TransferRepository(ctx, doer, repo, repoTransfer.Recipient.Name) return nil } // transferOwnership transfers all corresponding repository items from old user to new one. -func transferOwnership(ctx context.Context, doer *user_model.User, newOwnerName string, repo *repo_model.Repository) (err error) { +func transferOwnership(ctx context.Context, doer *user_model.User, newOwnerName string, repo *repo_model.Repository, teams []*organization.Team) (err error) { repoRenamed := false wikiRenamed := false oldOwnerName := doer.Name @@ -301,6 +303,17 @@ func transferOwnership(ctx context.Context, doer *user_model.User, newOwnerName return fmt.Errorf("repo_model.NewRedirect: %w", err) } + newRepo, err := repo_model.GetRepositoryByID(ctx, repo.ID) + if err != nil { + return err + } + + for _, team := range teams { + if err := addRepositoryToTeam(ctx, team, newRepo); err != nil { + return err + } + } + return committer.Commit() } @@ -343,17 +356,9 @@ func changeRepositoryName(ctx context.Context, repo *repo_model.Repository, newR } } - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - - if err := repo_model.NewRedirect(ctx, repo.Owner.ID, repo.ID, oldRepoName, newRepoName); err != nil { - return err - } - - return committer.Commit() + return db.WithTx(ctx, func(ctx context.Context) error { + return repo_model.NewRedirect(ctx, repo.Owner.ID, repo.ID, oldRepoName, newRepoName) + }) } // ChangeRepositoryName changes all corresponding setting from old repository name to new one. @@ -391,66 +396,81 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use return err } - // Admin is always allowed to transfer || user transfer repo back to his account - if doer.IsAdmin || doer.ID == newOwner.ID { - return TransferOwnership(ctx, doer, newOwner, repo, teams) - } + var isTransfer bool - if user_model.IsUserBlockedBy(ctx, doer, newOwner.ID) { - return user_model.ErrBlockedUser - } + if err := db.WithTx(ctx, func(ctx context.Context) error { + // Admin is always allowed to transfer || user transfer repo back to his account + if doer.IsAdmin || doer.ID == newOwner.ID { + isTransfer = true + return transferOwnership(ctx, doer, newOwner.Name, repo, teams) + } - // If new owner is an org and user can create repos he can transfer directly too - if newOwner.IsOrganization() { - allowed, err := organization.CanCreateOrgRepo(ctx, newOwner.ID, doer.ID) - if err != nil { - return err + if user_model.IsUserBlockedBy(ctx, doer, newOwner.ID) { + return user_model.ErrBlockedUser } - if allowed { - return TransferOwnership(ctx, doer, newOwner, repo, teams) + + // If new owner is an org and user can create repos he can transfer directly too + if newOwner.IsOrganization() { + allowed, err := organization.CanCreateOrgRepo(ctx, newOwner.ID, doer.ID) + if err != nil { + return err + } + if allowed { + isTransfer = true + return transferOwnership(ctx, doer, newOwner.Name, repo, teams) + } } - } - // In case the new owner would not have sufficient access to the repo, give access rights for read - hasAccess, err := access_model.HasAnyUnitAccess(ctx, newOwner.ID, repo) - if err != nil { - return err - } - if !hasAccess { - if err := AddOrUpdateCollaborator(ctx, repo, newOwner, perm.AccessModeRead); err != nil { + // In case the new owner would not have sufficient access to the repo, give access rights for read + hasAccess, err := access_model.HasAnyUnitAccess(ctx, newOwner.ID, repo) + if err != nil { return err } - } + if !hasAccess { + if err := AddOrUpdateCollaborator(ctx, repo, newOwner, perm.AccessModeRead); err != nil { + return err + } + } - // Make repo as pending for transfer - repo.Status = repo_model.RepositoryPendingTransfer - if err := repo_model.CreatePendingRepositoryTransfer(ctx, doer, newOwner, repo.ID, teams); err != nil { + // Make repo as pending for transfer + repo.Status = repo_model.RepositoryPendingTransfer + return repo_model.CreatePendingRepositoryTransfer(ctx, doer, newOwner, repo.ID, teams) + }); err != nil { return err } - // notify users who are able to accept / reject transfer - notify_service.RepoPendingTransfer(ctx, doer, newOwner, repo) + if isTransfer { + notify_service.TransferRepository(ctx, doer, repo, newOwner.Name) + } else { + // notify users who are able to accept / reject transfer + notify_service.RepoPendingTransfer(ctx, doer, newOwner, repo) + } return nil } -// CancelRepositoryTransfer marks the repository as ready and remove pending transfer entry, +// RejectRepositoryTransfer marks the repository as ready and remove pending transfer entry, // thus cancel the transfer process. -func CancelRepositoryTransfer(ctx context.Context, repo *repo_model.Repository) error { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() +func RejectRepositoryTransfer(ctx context.Context, repo *repo_model.Repository, doer *user_model.User) error { + return db.WithTx(ctx, func(ctx context.Context) error { + repoTransfer, err := repo_model.GetPendingRepositoryTransfer(ctx, repo) + if err != nil { + return err + } - repo.Status = repo_model.RepositoryReady - if err := repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil { - return err - } + if err := repoTransfer.LoadAttributes(ctx); err != nil { + return err + } - if err := repo_model.DeleteRepositoryTransfer(ctx, repo.ID); err != nil { - return err - } + if !repoTransfer.CanUserAcceptOrRejectTransfer(ctx, doer) { + return util.ErrPermissionDenied + } - return committer.Commit() + repo.Status = repo_model.RepositoryReady + if err := repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil { + return err + } + + return repo_model.DeleteRepositoryTransfer(ctx, repo.ID) + }) } diff --git a/services/repository/transfer_test.go b/services/repository/transfer_test.go index 91722fb8ae14b..94d4351f55a79 100644 --- a/services/repository/transfer_test.go +++ b/services/repository/transfer_test.go @@ -37,7 +37,7 @@ func TestTransferOwnership(t *testing.T) { doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) repo.Owner = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) - assert.NoError(t, TransferOwnership(db.DefaultContext, doer, doer, repo, nil)) + assert.NoError(t, AcceptTransferOwnership(db.DefaultContext, repo, doer)) transferredRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) assert.EqualValues(t, 2, transferredRepo.OwnerID) @@ -90,7 +90,7 @@ func TestRepositoryTransfer(t *testing.T) { assert.NotNil(t, transfer) // Cancel transfer - assert.NoError(t, CancelRepositoryTransfer(db.DefaultContext, repo)) + assert.NoError(t, RejectRepositoryTransfer(db.DefaultContext, repo, doer)) transfer, err = repo_model.GetPendingRepositoryTransfer(db.DefaultContext, repo) assert.Error(t, err) @@ -118,5 +118,5 @@ func TestRepositoryTransfer(t *testing.T) { assert.Error(t, err) // Cancel transfer - assert.NoError(t, CancelRepositoryTransfer(db.DefaultContext, repo)) + assert.NoError(t, RejectRepositoryTransfer(db.DefaultContext, repo, doer)) } diff --git a/services/user/block.go b/services/user/block.go index c24ce5273ca69..418123cae0fb7 100644 --- a/services/user/block.go +++ b/services/user/block.go @@ -117,10 +117,10 @@ func BlockUser(ctx context.Context, doer, blocker, blockee *user_model.User, not } // cancel each other repository transfers - if err := cancelRepositoryTransfers(ctx, blocker, blockee); err != nil { + if err := cancelRepositoryTransfers(ctx, doer, blocker, blockee); err != nil { return err } - if err := cancelRepositoryTransfers(ctx, blockee, blocker); err != nil { + if err := cancelRepositoryTransfers(ctx, doer, blockee, blocker); err != nil { return err } @@ -192,7 +192,7 @@ func unwatchRepos(ctx context.Context, watcher, repoOwner *user_model.User) erro } } -func cancelRepositoryTransfers(ctx context.Context, sender, recipient *user_model.User) error { +func cancelRepositoryTransfers(ctx context.Context, doer, sender, recipient *user_model.User) error { transfers, err := repo_model.GetPendingRepositoryTransfers(ctx, &repo_model.PendingRepositoryTransferOptions{ SenderID: sender.ID, RecipientID: recipient.ID, @@ -207,7 +207,7 @@ func cancelRepositoryTransfers(ctx context.Context, sender, recipient *user_mode return err } - if err := repo_service.CancelRepositoryTransfer(ctx, repo); err != nil { + if err := repo_service.RejectRepositoryTransfer(ctx, repo, doer); err != nil { return err } } From 237a09c0ccdabc9ab2c9433dee15c8ceffe25c2d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 10 Jan 2025 23:14:35 -0800 Subject: [PATCH 02/13] Fix bugs --- services/notify/notify.go | 4 ++-- services/repository/transfer.go | 7 +++++-- services/repository/transfer_test.go | 13 ++++++++----- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/services/notify/notify.go b/services/notify/notify.go index 3b5f24340bb86..c97d0fcbaf0af 100644 --- a/services/notify/notify.go +++ b/services/notify/notify.go @@ -272,9 +272,9 @@ func MigrateRepository(ctx context.Context, doer, u *user_model.User, repo *repo } // TransferRepository notifies create repository to notifiers -func TransferRepository(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, newOwnerName string) { +func TransferRepository(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, oldOwnerName string) { for _, notifier := range notifiers { - notifier.TransferRepository(ctx, doer, repo, newOwnerName) + notifier.TransferRepository(ctx, doer, repo, oldOwnerName) } } diff --git a/services/repository/transfer.go b/services/repository/transfer.go index ad12073cc9677..a0ad8501022e9 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -41,6 +41,8 @@ func AcceptTransferOwnership(ctx context.Context, repo *repo_model.Repository, d return err } + oldOwnerName := repo.OwnerName + if err := db.WithTx(ctx, func(ctx context.Context) error { if err := repoTransfer.LoadAttributes(ctx); err != nil { return err @@ -65,7 +67,7 @@ func AcceptTransferOwnership(ctx context.Context, repo *repo_model.Repository, d } releaser() - notify_service.TransferRepository(ctx, doer, repo, repoTransfer.Recipient.Name) + notify_service.TransferRepository(ctx, doer, repo, oldOwnerName) return nil } @@ -397,6 +399,7 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use } var isTransfer bool + oldOwnerName := repo.OwnerName if err := db.WithTx(ctx, func(ctx context.Context) error { // Admin is always allowed to transfer || user transfer repo back to his account @@ -440,7 +443,7 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use } if isTransfer { - notify_service.TransferRepository(ctx, doer, repo, newOwner.Name) + notify_service.TransferRepository(ctx, doer, repo, oldOwnerName) } else { // notify users who are able to accept / reject transfer notify_service.RepoPendingTransfer(ctx, doer, newOwner, repo) diff --git a/services/repository/transfer_test.go b/services/repository/transfer_test.go index 94d4351f55a79..9441ac105b170 100644 --- a/services/repository/transfer_test.go +++ b/services/repository/transfer_test.go @@ -34,23 +34,26 @@ func TestTransferOwnership(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) - repo.Owner = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + assert.NoError(t, repo.LoadOwner(db.DefaultContext)) + repoTransfer := unittest.AssertExistsAndLoadBean(t, &repo_model.RepoTransfer{ID: 1}) + assert.NoError(t, repoTransfer.LoadAttributes(db.DefaultContext)) assert.NoError(t, AcceptTransferOwnership(db.DefaultContext, repo, doer)) transferredRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) - assert.EqualValues(t, 2, transferredRepo.OwnerID) + assert.EqualValues(t, 1, transferredRepo.OwnerID) // repo_transfer.yml id=1 + unittest.AssertNotExistsBean(t, &repo_model.RepoTransfer{ID: 1}) exist, err := util.IsExist(repo_model.RepoPath("org3", "repo3")) assert.NoError(t, err) assert.False(t, exist) - exist, err = util.IsExist(repo_model.RepoPath("user2", "repo3")) + exist, err = util.IsExist(repo_model.RepoPath("user1", "repo3")) assert.NoError(t, err) assert.True(t, exist) unittest.AssertExistsAndLoadBean(t, &activities_model.Action{ OpType: activities_model.ActionTransferRepo, - ActUserID: 2, + ActUserID: 1, RepoID: 3, Content: "org3/repo3", }) From 94caa396ba8ca916d07d97d85810386c0f478222 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 11 Jan 2025 00:01:47 -0800 Subject: [PATCH 03/13] Fix test --- models/repo/transfer.go | 14 ++++++++++++++ services/repository/transfer_test.go | 16 +++++++++------- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/models/repo/transfer.go b/models/repo/transfer.go index 36fbd42377319..a499acb3d8d24 100644 --- a/models/repo/transfer.go +++ b/models/repo/transfer.go @@ -206,11 +206,25 @@ func CreatePendingRepositoryTransfer(ctx context.Context, doer, newOwner *user_m return err } + if _, err := user_model.GetUserByID(ctx, newOwner.ID); err != nil { + return err + } + // Make sure repo is ready to transfer if err := TestRepositoryReadyForTransfer(repo.Status); err != nil { return err } + _, err = GetPendingRepositoryTransfer(ctx, repo) + if err == nil { + return ErrRepoTransferInProgress{ + Uname: repo.Owner.LowerName, + Name: repo.Name, + } + } else if !IsErrNoPendingTransfer(err) { + return err + } + repo.Status = RepositoryPendingTransfer if err := UpdateRepositoryCols(ctx, repo, "status"); err != nil { return err diff --git a/services/repository/transfer_test.go b/services/repository/transfer_test.go index 9441ac105b170..ccdff0ff3ad4a 100644 --- a/services/repository/transfer_test.go +++ b/services/repository/transfer_test.go @@ -64,10 +64,10 @@ func TestTransferOwnership(t *testing.T) { func TestStartRepositoryTransferSetPermission(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) recipient := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) - repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) - repo.Owner = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + assert.NoError(t, repo.LoadOwner(db.DefaultContext)) hasAccess, err := access_model.HasAnyUnitAccess(db.DefaultContext, recipient.ID, repo) assert.NoError(t, err) @@ -85,7 +85,7 @@ func TestStartRepositoryTransferSetPermission(t *testing.T) { func TestRepositoryTransfer(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) transfer, err := repo_model.GetPendingRepositoryTransfer(db.DefaultContext, repo) @@ -116,10 +116,12 @@ func TestRepositoryTransfer(t *testing.T) { assert.Error(t, err) assert.True(t, repo_model.IsErrRepoTransferInProgress(err)) - // Unknown user - err = repo_model.CreatePendingRepositoryTransfer(db.DefaultContext, doer, &user_model.User{ID: 1000, LowerName: "user1000"}, repo.ID, nil) + repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + // Unknown user, transfer non-existent transfer repo id = 2 + err = repo_model.CreatePendingRepositoryTransfer(db.DefaultContext, doer, &user_model.User{ID: 1000, LowerName: "user1000"}, repo2.ID, nil) assert.Error(t, err) // Cancel transfer - assert.NoError(t, RejectRepositoryTransfer(db.DefaultContext, repo, doer)) + err = RejectRepositoryTransfer(db.DefaultContext, repo2, doer) + assert.True(t, repo_model.IsErrNoPendingTransfer(err)) } From 07d1a378d0de8976375db7bb8f6d2037ea95664c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 11 Jan 2025 22:10:15 -0800 Subject: [PATCH 04/13] Fix bugs --- models/repo/transfer.go | 74 ++++++++++++++++++++++------ routers/api/v1/repo/transfer.go | 24 ++++++--- routers/web/repo/repo.go | 2 +- routers/web/repo/setting/setting.go | 3 +- services/context/repo.go | 2 +- services/repository/transfer.go | 9 ++-- services/repository/transfer_test.go | 4 +- services/user/block.go | 2 +- 8 files changed, 88 insertions(+), 32 deletions(-) diff --git a/models/repo/transfer.go b/models/repo/transfer.go index a499acb3d8d24..2d7762aa23f23 100644 --- a/models/repo/transfer.go +++ b/models/repo/transfer.go @@ -68,6 +68,7 @@ type RepoTransfer struct { //nolint RecipientID int64 Recipient *user_model.User `xorm:"-"` RepoID int64 + Repo *Repository `xorm:"-"` TeamIDs []int64 Teams []*organization.Team `xorm:"-"` @@ -79,48 +80,65 @@ func init() { db.RegisterModel(new(RepoTransfer)) } -// LoadAttributes fetches the transfer recipient from the database -func (r *RepoTransfer) LoadAttributes(ctx context.Context) error { +func (r *RepoTransfer) LoadRecipient(ctx context.Context) error { if r.Recipient == nil { u, err := user_model.GetUserByID(ctx, r.RecipientID) if err != nil { return err } - r.Recipient = u } - if r.Recipient.IsOrganization() && len(r.TeamIDs) != len(r.Teams) { - for _, v := range r.TeamIDs { - team, err := organization.GetTeamByID(ctx, v) - if err != nil { - return err - } + return nil +} - if team.OrgID != r.Recipient.ID { - return fmt.Errorf("team %d belongs not to org %d", v, r.Recipient.ID) - } +func (r *RepoTransfer) LoadRepo(ctx context.Context) error { + if r.Repo == nil { + repo, err := GetRepositoryByID(ctx, r.RepoID) + if err != nil { + return err + } + r.Repo = repo + } + + return nil +} + +// LoadAttributes fetches the transfer recipient from the database +func (r *RepoTransfer) LoadAttributes(ctx context.Context) error { + if err := r.LoadRecipient(ctx); err != nil { + return err + } + if r.Recipient.IsOrganization() && r.Teams == nil { + teamsMap, err := organization.GetTeamsByIDs(ctx, r.TeamIDs) + if err != nil { + return err + } + for _, team := range teamsMap { r.Teams = append(r.Teams, team) } } + if err := r.LoadRepo(ctx); err != nil { + return err + } + if r.Doer == nil { u, err := user_model.GetUserByID(ctx, r.DoerID) if err != nil { return err } - r.Doer = u } return nil } -// CanUserAcceptOrRejectTransfer checks if the user has the rights to accept/decline a repo transfer. +// CanUserAcceptTransfer checks if the user has the rights to accept/decline a repo transfer. // For user, it checks if it's himself // For organizations, it checks if the user is able to create repos -func (r *RepoTransfer) CanUserAcceptOrRejectTransfer(ctx context.Context, u *user_model.User) bool { +func (r *RepoTransfer) CanUserAcceptTransfer(ctx context.Context, u *user_model.User) bool { if err := r.LoadAttributes(ctx); err != nil { log.Error("LoadAttributes: %v", err) return false @@ -139,6 +157,32 @@ func (r *RepoTransfer) CanUserAcceptOrRejectTransfer(ctx context.Context, u *use return allowed } +func (r *RepoTransfer) CanUserCancelTransfer(ctx context.Context, u *user_model.User) bool { + if r.DoerID == u.ID { // sender can cancel the transfer + return true + } + if err := r.Repo.LoadOwner(ctx); err != nil { + log.Error("LoadOwner: %v", err) + return false + } + if !r.Repo.Owner.IsOrganization() { + if u.ID == r.Repo.OwnerID { // owner can cancel the transfer + return true + } + } else { + allowed, err := organization.CanCreateOrgRepo(ctx, r.Repo.OwnerID, u.ID) + if err != nil { + log.Error("CanCreateOrgRepo: %v", err) + return false + } + if allowed { + return true + } + } + + return r.CanUserAcceptTransfer(ctx, u) // the user who can accept the transfer can also reject it +} + type PendingRepositoryTransferOptions struct { RepoID int64 SenderID int64 diff --git a/routers/api/v1/repo/transfer.go b/routers/api/v1/repo/transfer.go index 01f3a4d2c9bf9..883c14ac9a88f 100644 --- a/routers/api/v1/repo/transfer.go +++ b/routers/api/v1/repo/transfer.go @@ -15,6 +15,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/log" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" @@ -162,11 +163,15 @@ func AcceptTransfer(ctx *context.APIContext) { // "$ref": "#/responses/notFound" err := repo_service.AcceptTransferOwnership(ctx, ctx.Repo.Repository, ctx.Doer) - if ctx.Written() { - return - } if err != nil { - ctx.Error(http.StatusInternalServerError, "acceptOrRejectRepoTransfer", err) + switch { + case repo_model.IsErrNoPendingTransfer(err): + ctx.Error(http.StatusNotFound, "AcceptTransferOwnership", err) + case errors.Is(err, util.ErrPermissionDenied): + ctx.Error(http.StatusForbidden, "AcceptTransferOwnership", err) + default: + ctx.Error(http.StatusInternalServerError, "AcceptTransferOwnership", err) + } return } @@ -199,9 +204,16 @@ func RejectTransfer(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - err := repo_service.RejectRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer) + err := repo_service.CancelRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer) if err != nil { - ctx.Error(http.StatusInternalServerError, "acceptOrRejectRepoTransfer", err) + switch { + case repo_model.IsErrNoPendingTransfer(err): + ctx.Error(http.StatusNotFound, "CancelRepositoryTransfer", err) + case errors.Is(err, util.ErrPermissionDenied): + ctx.Error(http.StatusForbidden, "CancelRepositoryTransfer", err) + default: + ctx.Error(http.StatusInternalServerError, "CancelRepositoryTransfer", err) + } return } diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index 066643302300d..f2158b07b0997 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -328,7 +328,7 @@ func Action(ctx *context.Context) { } ctx.Redirect(ctx.Repo.Repository.Link()) case "reject_transfer": - err = repo_service.RejectRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer) + err = repo_service.CancelRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer) if err == nil { ctx.Flash.Success(ctx.Tr("repo.settings.transfer.rejected")) } diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index 20c5b5478be58..0fc87a89fcff0 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -821,7 +821,6 @@ func SettingsPost(ctx *context.Context) { } else { ctx.ServerError("GetPendingRepositoryTransfer", err) } - return } @@ -830,7 +829,7 @@ func SettingsPost(ctx *context.Context) { return } - if err := repo_service.RejectRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer); err != nil { + if err := repo_service.CancelRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer); err != nil { ctx.ServerError("CancelRepositoryTransfer", err) return } diff --git a/services/context/repo.go b/services/context/repo.go index bed6e4c7004a0..05f6bb40f94ae 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -674,7 +674,7 @@ func RepoAssignment(ctx *Context) { ctx.Data["RepoTransfer"] = repoTransfer if ctx.Doer != nil { - ctx.Data["CanUserAcceptTransfer"] = repoTransfer.CanUserAcceptOrRejectTransfer(ctx, ctx.Doer) + ctx.Data["CanUserAcceptTransfer"] = repoTransfer.CanUserAcceptTransfer(ctx, ctx.Doer) } } diff --git a/services/repository/transfer.go b/services/repository/transfer.go index a0ad8501022e9..70c70f7fcf080 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -48,7 +48,7 @@ func AcceptTransferOwnership(ctx context.Context, repo *repo_model.Repository, d return err } - if !repoTransfer.CanUserAcceptOrRejectTransfer(ctx, doer) { + if !repoTransfer.CanUserAcceptTransfer(ctx, doer) { return util.ErrPermissionDenied } @@ -452,9 +452,10 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use return nil } -// RejectRepositoryTransfer marks the repository as ready and remove pending transfer entry, +// CancelRepositoryTransfer marks the repository as ready and remove pending transfer entry, // thus cancel the transfer process. -func RejectRepositoryTransfer(ctx context.Context, repo *repo_model.Repository, doer *user_model.User) error { +// Both the sender and the accepter can cancel the transfer. +func CancelRepositoryTransfer(ctx context.Context, repo *repo_model.Repository, doer *user_model.User) error { return db.WithTx(ctx, func(ctx context.Context) error { repoTransfer, err := repo_model.GetPendingRepositoryTransfer(ctx, repo) if err != nil { @@ -465,7 +466,7 @@ func RejectRepositoryTransfer(ctx context.Context, repo *repo_model.Repository, return err } - if !repoTransfer.CanUserAcceptOrRejectTransfer(ctx, doer) { + if !repoTransfer.CanUserCancelTransfer(ctx, doer) { return util.ErrPermissionDenied } diff --git a/services/repository/transfer_test.go b/services/repository/transfer_test.go index ccdff0ff3ad4a..9fb46c57614ad 100644 --- a/services/repository/transfer_test.go +++ b/services/repository/transfer_test.go @@ -93,7 +93,7 @@ func TestRepositoryTransfer(t *testing.T) { assert.NotNil(t, transfer) // Cancel transfer - assert.NoError(t, RejectRepositoryTransfer(db.DefaultContext, repo, doer)) + assert.NoError(t, CancelRepositoryTransfer(db.DefaultContext, repo, doer)) transfer, err = repo_model.GetPendingRepositoryTransfer(db.DefaultContext, repo) assert.Error(t, err) @@ -122,6 +122,6 @@ func TestRepositoryTransfer(t *testing.T) { assert.Error(t, err) // Cancel transfer - err = RejectRepositoryTransfer(db.DefaultContext, repo2, doer) + err = CancelRepositoryTransfer(db.DefaultContext, repo2, doer) assert.True(t, repo_model.IsErrNoPendingTransfer(err)) } diff --git a/services/user/block.go b/services/user/block.go index 418123cae0fb7..0a2254b0132e9 100644 --- a/services/user/block.go +++ b/services/user/block.go @@ -207,7 +207,7 @@ func cancelRepositoryTransfers(ctx context.Context, doer, sender, recipient *use return err } - if err := repo_service.RejectRepositoryTransfer(ctx, repo, doer); err != nil { + if err := repo_service.CancelRepositoryTransfer(ctx, repo, doer); err != nil { return err } } From 45ffedd3050cb2334432d72d9e673ec424c2b371 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 12 Jan 2025 20:08:42 -0800 Subject: [PATCH 05/13] Fix bug --- models/repo/transfer.go | 30 +------------- routers/api/v1/repo/transfer.go | 2 +- routers/web/repo/repo.go | 2 +- routers/web/repo/setting/setting.go | 2 +- services/context/repo.go | 3 +- services/repository/transfer.go | 58 +++++++++++++++++++++++++--- services/repository/transfer_test.go | 6 +-- services/user/block.go | 7 +--- templates/repo/header.tmpl | 4 +- 9 files changed, 66 insertions(+), 48 deletions(-) diff --git a/models/repo/transfer.go b/models/repo/transfer.go index 2d7762aa23f23..a03cee05b1fe7 100644 --- a/models/repo/transfer.go +++ b/models/repo/transfer.go @@ -135,10 +135,10 @@ func (r *RepoTransfer) LoadAttributes(ctx context.Context) error { return nil } -// CanUserAcceptTransfer checks if the user has the rights to accept/decline a repo transfer. +// CanUserAcceptOrRejectTransfer checks if the user has the rights to accept/decline a repo transfer. // For user, it checks if it's himself // For organizations, it checks if the user is able to create repos -func (r *RepoTransfer) CanUserAcceptTransfer(ctx context.Context, u *user_model.User) bool { +func (r *RepoTransfer) CanUserAcceptOrRejectTransfer(ctx context.Context, u *user_model.User) bool { if err := r.LoadAttributes(ctx); err != nil { log.Error("LoadAttributes: %v", err) return false @@ -157,32 +157,6 @@ func (r *RepoTransfer) CanUserAcceptTransfer(ctx context.Context, u *user_model. return allowed } -func (r *RepoTransfer) CanUserCancelTransfer(ctx context.Context, u *user_model.User) bool { - if r.DoerID == u.ID { // sender can cancel the transfer - return true - } - if err := r.Repo.LoadOwner(ctx); err != nil { - log.Error("LoadOwner: %v", err) - return false - } - if !r.Repo.Owner.IsOrganization() { - if u.ID == r.Repo.OwnerID { // owner can cancel the transfer - return true - } - } else { - allowed, err := organization.CanCreateOrgRepo(ctx, r.Repo.OwnerID, u.ID) - if err != nil { - log.Error("CanCreateOrgRepo: %v", err) - return false - } - if allowed { - return true - } - } - - return r.CanUserAcceptTransfer(ctx, u) // the user who can accept the transfer can also reject it -} - type PendingRepositoryTransferOptions struct { RepoID int64 SenderID int64 diff --git a/routers/api/v1/repo/transfer.go b/routers/api/v1/repo/transfer.go index 883c14ac9a88f..8a258f30b303d 100644 --- a/routers/api/v1/repo/transfer.go +++ b/routers/api/v1/repo/transfer.go @@ -204,7 +204,7 @@ func RejectTransfer(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - err := repo_service.CancelRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer) + err := repo_service.RejectRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer) if err != nil { switch { case repo_model.IsErrNoPendingTransfer(err): diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index f2158b07b0997..066643302300d 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -328,7 +328,7 @@ func Action(ctx *context.Context) { } ctx.Redirect(ctx.Repo.Repository.Link()) case "reject_transfer": - err = repo_service.CancelRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer) + err = repo_service.RejectRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer) if err == nil { ctx.Flash.Success(ctx.Tr("repo.settings.transfer.rejected")) } diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index 0fc87a89fcff0..fa3c567b34dbf 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -829,7 +829,7 @@ func SettingsPost(ctx *context.Context) { return } - if err := repo_service.CancelRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer); err != nil { + if err := repo_service.CancelRepositoryTransfer(ctx, repoTransfer, ctx.Doer); err != nil { ctx.ServerError("CancelRepositoryTransfer", err) return } diff --git a/services/context/repo.go b/services/context/repo.go index 05f6bb40f94ae..6db042bef7563 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -674,7 +674,8 @@ func RepoAssignment(ctx *Context) { ctx.Data["RepoTransfer"] = repoTransfer if ctx.Doer != nil { - ctx.Data["CanUserAcceptTransfer"] = repoTransfer.CanUserAcceptTransfer(ctx, ctx.Doer) + ctx.Data["CanUserAcceptTransfer"] = repoTransfer.CanUserAcceptOrRejectTransfer(ctx, ctx.Doer) + ctx.Data["CanUserRejectTransfer"] = ctx.Data["CanUserAcceptTransfer"] } } diff --git a/services/repository/transfer.go b/services/repository/transfer.go index 70c70f7fcf080..2f9b1d16b1d3d 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -48,7 +48,7 @@ func AcceptTransferOwnership(ctx context.Context, repo *repo_model.Repository, d return err } - if !repoTransfer.CanUserAcceptTransfer(ctx, doer) { + if !repoTransfer.CanUserAcceptOrRejectTransfer(ctx, doer) { return util.ErrPermissionDenied } @@ -452,10 +452,10 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use return nil } -// CancelRepositoryTransfer marks the repository as ready and remove pending transfer entry, +// RejectRepositoryTransfer marks the repository as ready and remove pending transfer entry, // thus cancel the transfer process. -// Both the sender and the accepter can cancel the transfer. -func CancelRepositoryTransfer(ctx context.Context, repo *repo_model.Repository, doer *user_model.User) error { +// The accepter can reject the transfer. +func RejectRepositoryTransfer(ctx context.Context, repo *repo_model.Repository, doer *user_model.User) error { return db.WithTx(ctx, func(ctx context.Context) error { repoTransfer, err := repo_model.GetPendingRepositoryTransfer(ctx, repo) if err != nil { @@ -466,7 +466,7 @@ func CancelRepositoryTransfer(ctx context.Context, repo *repo_model.Repository, return err } - if !repoTransfer.CanUserCancelTransfer(ctx, doer) { + if !repoTransfer.CanUserAcceptOrRejectTransfer(ctx, doer) { return util.ErrPermissionDenied } @@ -478,3 +478,51 @@ func CancelRepositoryTransfer(ctx context.Context, repo *repo_model.Repository, return repo_model.DeleteRepositoryTransfer(ctx, repo.ID) }) } + +func canUserCancelTransfer(ctx context.Context, r *repo_model.RepoTransfer, u *user_model.User) bool { + if u.ID == r.DoerID { + return true + } + + if err := r.LoadAttributes(ctx); err != nil { + log.Error("LoadAttributes: %v", err) + return false + } + + if err := r.Repo.LoadOwner(ctx); err != nil { + log.Error("LoadOwner: %v", err) + return false + } + + if !r.Repo.Owner.IsOrganization() { + return r.Repo.OwnerID == u.ID + } + + perm, err := access_model.GetUserRepoPermission(ctx, r.Repo, u) + if err != nil { + log.Error("GetUserRepoPermission: %v", err) + return false + } + return perm.IsAdmin() +} + +// CancelRepositoryTransfer cancels the repository transfer process. The sender or +// the users who have admin permission of the original repository can cancel the transfer +func CancelRepositoryTransfer(ctx context.Context, repoTransfer *repo_model.RepoTransfer, doer *user_model.User) error { + return db.WithTx(ctx, func(ctx context.Context) error { + if err := repoTransfer.LoadAttributes(ctx); err != nil { + return err + } + + if !canUserCancelTransfer(ctx, repoTransfer, doer) { + return util.ErrPermissionDenied + } + + repoTransfer.Repo.Status = repo_model.RepositoryReady + if err := repo_model.UpdateRepositoryCols(ctx, repoTransfer.Repo, "status"); err != nil { + return err + } + + return repo_model.DeleteRepositoryTransfer(ctx, repoTransfer.RepoID) + }) +} diff --git a/services/repository/transfer_test.go b/services/repository/transfer_test.go index 9fb46c57614ad..16a8fb6e1eed2 100644 --- a/services/repository/transfer_test.go +++ b/services/repository/transfer_test.go @@ -93,7 +93,7 @@ func TestRepositoryTransfer(t *testing.T) { assert.NotNil(t, transfer) // Cancel transfer - assert.NoError(t, CancelRepositoryTransfer(db.DefaultContext, repo, doer)) + assert.NoError(t, CancelRepositoryTransfer(db.DefaultContext, transfer, doer)) transfer, err = repo_model.GetPendingRepositoryTransfer(db.DefaultContext, repo) assert.Error(t, err) @@ -121,7 +121,7 @@ func TestRepositoryTransfer(t *testing.T) { err = repo_model.CreatePendingRepositoryTransfer(db.DefaultContext, doer, &user_model.User{ID: 1000, LowerName: "user1000"}, repo2.ID, nil) assert.Error(t, err) - // Cancel transfer - err = CancelRepositoryTransfer(db.DefaultContext, repo2, doer) + // Reject transfer + err = RejectRepositoryTransfer(db.DefaultContext, repo2, doer) assert.True(t, repo_model.IsErrNoPendingTransfer(err)) } diff --git a/services/user/block.go b/services/user/block.go index 0a2254b0132e9..7727780dfc919 100644 --- a/services/user/block.go +++ b/services/user/block.go @@ -202,12 +202,7 @@ func cancelRepositoryTransfers(ctx context.Context, doer, sender, recipient *use } for _, transfer := range transfers { - repo, err := repo_model.GetRepositoryByID(ctx, transfer.RepoID) - if err != nil { - return err - } - - if err := repo_service.CancelRepositoryTransfer(ctx, repo, doer); err != nil { + if err := repo_service.CancelRepositoryTransfer(ctx, transfer, doer); err != nil { return err } } diff --git a/templates/repo/header.tmpl b/templates/repo/header.tmpl index 3e7214f48cf7f..c3edbcba2dd17 100644 --- a/templates/repo/header.tmpl +++ b/templates/repo/header.tmpl @@ -47,8 +47,8 @@
{{$.CsrfTokenHtml}} -
-
From 2f1fc758ade6be02828bb5715ae37d546fa3ec84 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 13 Jan 2025 13:05:39 -0800 Subject: [PATCH 06/13] Some improvements --- routers/web/repo/repo.go | 61 +++++++++++++++++++---------- routers/web/repo/setting/setting.go | 5 --- services/repository/transfer.go | 23 +++++------ 3 files changed, 53 insertions(+), 36 deletions(-) diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index 066643302300d..e7b63d70fbff3 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -309,6 +309,40 @@ const ( tplStarUnstar templates.TplName = "repo/star_unstar" ) +func acceptTransfer(ctx *context.Context) { + err := repo_service.AcceptTransferOwnership(ctx, ctx.Repo.Repository, ctx.Doer) + if err == nil { + ctx.Flash.Success(ctx.Tr("repo.settings.transfer.success")) + ctx.Redirect(ctx.Repo.Repository.Link()) + return + } + handleActionError(ctx, err) +} + +func rejectTransfer(ctx *context.Context) { + err := repo_service.RejectRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer) + if err == nil { + ctx.Flash.Success(ctx.Tr("repo.settings.transfer.rejected")) + ctx.Redirect(ctx.Repo.Repository.Link()) + return + } + handleActionError(ctx, err) +} + +func handleActionError(ctx *context.Context, err error) { + if err != nil { + if errors.Is(err, user_model.ErrBlockedUser) { + ctx.Flash.Error(ctx.Tr("repo.action.blocked_user")) + } else if errors.Is(err, util.ErrPermissionDenied) { + ctx.Error(http.StatusNotFound) + return + } else { + ctx.ServerError(fmt.Sprintf("Action (%s)", ctx.PathParam("action")), err) + return + } + } +} + // Action response for actions to a repository func Action(ctx *context.Context) { var err error @@ -322,17 +356,11 @@ func Action(ctx *context.Context) { case "unstar": err = repo_model.StarRepo(ctx, ctx.Doer, ctx.Repo.Repository, false) case "accept_transfer": - err = repo_service.AcceptTransferOwnership(ctx, ctx.Repo.Repository, ctx.Doer) - if err == nil { - ctx.Flash.Success(ctx.Tr("repo.settings.transfer.success")) - } - ctx.Redirect(ctx.Repo.Repository.Link()) + acceptTransfer(ctx) + return case "reject_transfer": - err = repo_service.RejectRepositoryTransfer(ctx, ctx.Repo.Repository, ctx.Doer) - if err == nil { - ctx.Flash.Success(ctx.Tr("repo.settings.transfer.rejected")) - } - ctx.Redirect(ctx.Repo.Repository.Link()) + rejectTransfer(ctx) + return case "desc": // FIXME: this is not used if !ctx.Repo.IsOwner() { ctx.Error(http.StatusNotFound) @@ -344,16 +372,9 @@ func Action(ctx *context.Context) { err = repo_service.UpdateRepository(ctx, ctx.Repo.Repository, false) } - if err != nil { - if errors.Is(err, user_model.ErrBlockedUser) { - ctx.Flash.Error(ctx.Tr("repo.action.blocked_user")) - } else if errors.Is(err, util.ErrPermissionDenied) { - ctx.Error(http.StatusNotFound) - return - } else { - ctx.ServerError(fmt.Sprintf("Action (%s)", ctx.PathParam("action")), err) - return - } + handleActionError(ctx, err) + if ctx.Written() { + return } switch ctx.PathParam("action") { diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index fa3c567b34dbf..360e46f1e4b6f 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -824,11 +824,6 @@ func SettingsPost(ctx *context.Context) { return } - if err := repoTransfer.LoadAttributes(ctx); err != nil { - ctx.ServerError("LoadRecipient", err) - return - } - if err := repo_service.CancelRepositoryTransfer(ctx, repoTransfer, ctx.Doer); err != nil { ctx.ServerError("CancelRepositoryTransfer", err) return diff --git a/services/repository/transfer.go b/services/repository/transfer.go index 2f9b1d16b1d3d..c18d31add3705 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -142,7 +142,7 @@ func transferOwnership(ctx context.Context, doer *user_model.User, newOwnerName repo.OwnerName = newOwner.Name // Update repository. - if _, err := sess.ID(repo.ID).Update(repo); err != nil { + if err := repo_model.UpdateRepositoryCols(ctx, repo, "owner_id", "owner_name"); err != nil { return fmt.Errorf("update owner: %w", err) } @@ -178,15 +178,13 @@ func transferOwnership(ctx context.Context, doer *user_model.User, newOwnerName collaboration.UserID = 0 } - // Remove old team-repository relations. if oldOwner.IsOrganization() { + // Remove old team-repository relations. if err := organization.RemoveOrgRepo(ctx, oldOwner.ID, repo.ID); err != nil { return fmt.Errorf("removeOrgRepo: %w", err) } - } - // Remove project's issues that belong to old organization's projects - if oldOwner.IsOrganization() { + // Remove project's issues that belong to old organization's projects projects, err := project_model.GetAllProjectsIDsByOwnerIDAndType(ctx, oldOwner.ID, project_model.TypeOrganization) if err != nil { return fmt.Errorf("Unable to find old org projects: %w", err) @@ -229,15 +227,13 @@ func transferOwnership(ctx context.Context, doer *user_model.User, newOwnerName return fmt.Errorf("watchRepo: %w", err) } - // Remove watch for organization. if oldOwner.IsOrganization() { + // Remove watch for organization. if err := repo_model.WatchRepo(ctx, oldOwner, repo, false); err != nil { return fmt.Errorf("watchRepo [false]: %w", err) } - } - // Delete labels that belong to the old organization and comments that added these labels - if oldOwner.IsOrganization() { + // Delete labels that belong to the old organization and comments that added these labels if _, err := sess.Exec(`DELETE FROM issue_label WHERE issue_label.id IN ( SELECT il_too.id FROM ( SELECT il_too_too.id @@ -265,7 +261,6 @@ func transferOwnership(ctx context.Context, doer *user_model.User, newOwnerName // Rename remote repository to new path and delete local copy. dir := user_model.UserPath(newOwner.Name) - if err := os.MkdirAll(dir, os.ModePerm); err != nil { return fmt.Errorf("Failed to create dir %s: %w", dir, err) } @@ -277,7 +272,6 @@ func transferOwnership(ctx context.Context, doer *user_model.User, newOwnerName // Rename remote wiki repository to new path and delete local copy. wikiPath := repo_model.WikiPath(oldOwner.Name, repo.Name) - if isExist, err := util.IsExist(wikiPath); err != nil { log.Error("Unable to check if %s exists. Error: %v", wikiPath, err) return err @@ -394,6 +388,13 @@ 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 { + releaser, err := globallock.Lock(ctx, getRepoWorkingLockKey(repo.ID)) + if err != nil { + log.Error("lock.Lock(): %v", err) + return fmt.Errorf("lock.Lock: %w", err) + } + defer releaser() + if err := repo_model.TestRepositoryReadyForTransfer(repo.Status); err != nil { return err } From 6bd16bd79f7c35d35506b78f7103248429750b34 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 17 Jan 2025 20:42:09 -0800 Subject: [PATCH 07/13] Follow reviews --- routers/api/v1/repo/transfer.go | 6 +++--- services/context/repo.go | 3 +-- templates/repo/header.tmpl | 8 ++++---- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/routers/api/v1/repo/transfer.go b/routers/api/v1/repo/transfer.go index 8a258f30b303d..bb666f6487ed3 100644 --- a/routers/api/v1/repo/transfer.go +++ b/routers/api/v1/repo/transfer.go @@ -208,11 +208,11 @@ func RejectTransfer(ctx *context.APIContext) { if err != nil { switch { case repo_model.IsErrNoPendingTransfer(err): - ctx.Error(http.StatusNotFound, "CancelRepositoryTransfer", err) + ctx.Error(http.StatusNotFound, "RejectRepositoryTransfer", err) case errors.Is(err, util.ErrPermissionDenied): - ctx.Error(http.StatusForbidden, "CancelRepositoryTransfer", err) + ctx.Error(http.StatusForbidden, "RejectRepositoryTransfer", err) default: - ctx.Error(http.StatusInternalServerError, "CancelRepositoryTransfer", err) + ctx.Error(http.StatusInternalServerError, "RejectRepositoryTransfer", err) } return } diff --git a/services/context/repo.go b/services/context/repo.go index 0440ca781bc7f..258a71d6e76e8 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -665,8 +665,7 @@ func RepoAssignment(ctx *Context) { ctx.Data["RepoTransfer"] = repoTransfer if ctx.Doer != nil { - ctx.Data["CanUserAcceptTransfer"] = repoTransfer.CanUserAcceptOrRejectTransfer(ctx, ctx.Doer) - ctx.Data["CanUserRejectTransfer"] = ctx.Data["CanUserAcceptTransfer"] + ctx.Data["CanUserAcceptOrRejectTransfer"] = repoTransfer.CanUserAcceptOrRejectTransfer(ctx, ctx.Doer) } } diff --git a/templates/repo/header.tmpl b/templates/repo/header.tmpl index c3edbcba2dd17..8c2a0da8d0362 100644 --- a/templates/repo/header.tmpl +++ b/templates/repo/header.tmpl @@ -39,16 +39,16 @@ {{if $.RepoTransfer}} {{$.CsrfTokenHtml}} -
-
{{$.CsrfTokenHtml}} -
-
From 78b1cb95fbb63a46cbc5dab52763145357ee24c9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 22 Jan 2025 23:24:37 -0800 Subject: [PATCH 08/13] Some improvements --- models/repo/transfer.go | 13 +++++++++---- routers/web/repo/repo.go | 2 -- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/models/repo/transfer.go b/models/repo/transfer.go index a03cee05b1fe7..b669145d68af8 100644 --- a/models/repo/transfer.go +++ b/models/repo/transfer.go @@ -184,6 +184,10 @@ func GetPendingRepositoryTransfers(ctx context.Context, opts *PendingRepositoryT Find(&transfers) } +func IsRepositoryTransferExist(ctx context.Context, repoID int64) (bool, error) { + return db.GetEngine(ctx).Where("repo_id = ?", repoID).Exist(new(RepoTransfer)) +} + // GetPendingRepositoryTransfer fetches the most recent and ongoing transfer // process for the repository func GetPendingRepositoryTransfer(ctx context.Context, repo *Repository) (*RepoTransfer, error) { @@ -233,14 +237,15 @@ func CreatePendingRepositoryTransfer(ctx context.Context, doer, newOwner *user_m return err } - _, err = GetPendingRepositoryTransfer(ctx, repo) - if err == nil { + exist, err := IsRepositoryTransferExist(ctx, repo.ID) + if err != nil { + return err + } + if exist { return ErrRepoTransferInProgress{ Uname: repo.Owner.LowerName, Name: repo.Name, } - } else if !IsErrNoPendingTransfer(err) { - return err } repo.Status = RepositoryPendingTransfer diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index da06e2895a808..914e146e01ad4 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -335,10 +335,8 @@ func handleActionError(ctx *context.Context, err error) { ctx.Flash.Error(ctx.Tr("repo.action.blocked_user")) } else if errors.Is(err, util.ErrPermissionDenied) { ctx.Error(http.StatusNotFound) - return } else { ctx.ServerError(fmt.Sprintf("Action (%s)", ctx.PathParam("action")), err) - return } } } From 7edb840eedf65e2b737dc4afcc57d6eaa9f47abb Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 22 Jan 2025 23:43:57 -0800 Subject: [PATCH 09/13] Fix the permission check --- services/repository/transfer.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/repository/transfer.go b/services/repository/transfer.go index c18d31add3705..0b0e132badadf 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -481,7 +481,7 @@ func RejectRepositoryTransfer(ctx context.Context, repo *repo_model.Repository, } func canUserCancelTransfer(ctx context.Context, r *repo_model.RepoTransfer, u *user_model.User) bool { - if u.ID == r.DoerID { + if u.IsAdmin || u.ID == r.DoerID { return true } @@ -504,7 +504,7 @@ func canUserCancelTransfer(ctx context.Context, r *repo_model.RepoTransfer, u *u log.Error("GetUserRepoPermission: %v", err) return false } - return perm.IsAdmin() + return perm.IsOwner() } // CancelRepositoryTransfer cancels the repository transfer process. The sender or From cb24f729e3a14d3075bc3d1240388c31b025a81e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 29 Jan 2025 15:41:38 -0800 Subject: [PATCH 10/13] Follow @wxiaoguang's suggestion --- routers/web/repo/repo.go | 14 ++++++-------- services/repository/transfer.go | 11 ++++++----- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index 914e146e01ad4..9154437080197 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -330,14 +330,12 @@ func rejectTransfer(ctx *context.Context) { } func handleActionError(ctx *context.Context, err error) { - if err != nil { - if errors.Is(err, user_model.ErrBlockedUser) { - ctx.Flash.Error(ctx.Tr("repo.action.blocked_user")) - } else if errors.Is(err, util.ErrPermissionDenied) { - ctx.Error(http.StatusNotFound) - } else { - ctx.ServerError(fmt.Sprintf("Action (%s)", ctx.PathParam("action")), err) - } + if errors.Is(err, user_model.ErrBlockedUser) { + ctx.Flash.Error(ctx.Tr("repo.action.blocked_user")) + } else if errors.Is(err, util.ErrPermissionDenied) { + ctx.Error(http.StatusNotFound) + } else { + ctx.ServerError(fmt.Sprintf("Action (%s)", ctx.PathParam("action")), err) } } diff --git a/services/repository/transfer.go b/services/repository/transfer.go index 0b0e132badadf..61613b332ecdb 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -399,13 +399,14 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use return err } - var isTransfer bool + var isDirectTransfer bool oldOwnerName := repo.OwnerName if err := db.WithTx(ctx, func(ctx context.Context) error { - // Admin is always allowed to transfer || user transfer repo back to his account + // Admin is always allowed to transfer || user transfer repo back to his account, + // then it will transfer directly without acceptance. if doer.IsAdmin || doer.ID == newOwner.ID { - isTransfer = true + isDirectTransfer = true return transferOwnership(ctx, doer, newOwner.Name, repo, teams) } @@ -420,7 +421,7 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use return err } if allowed { - isTransfer = true + isDirectTransfer = true return transferOwnership(ctx, doer, newOwner.Name, repo, teams) } } @@ -443,7 +444,7 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use return err } - if isTransfer { + if isDirectTransfer { notify_service.TransferRepository(ctx, doer, repo, oldOwnerName) } else { // notify users who are able to accept / reject transfer From 0a51b67dc1fb1761b8616cc3d80cda11c575d776 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 30 Jan 2025 09:03:48 +0800 Subject: [PATCH 11/13] Update repo.go --- routers/web/repo/repo.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index 9154437080197..cba3a3a7e6945 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -368,9 +368,11 @@ func Action(ctx *context.Context) { err = repo_service.UpdateRepository(ctx, ctx.Repo.Repository, false) } - handleActionError(ctx, err) - if ctx.Written() { - return + if err != nil { + handleActionError(ctx, err) + if ctx.Written() { + return + } } switch ctx.PathParam("action") { From 515ee5ff6fb8ffb6df8d085567708a95ec1bb7bb Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 30 Jan 2025 09:04:17 +0800 Subject: [PATCH 12/13] Update repo.go --- routers/web/repo/repo.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index cba3a3a7e6945..8ebf5bcf3903b 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -370,9 +370,7 @@ func Action(ctx *context.Context) { if err != nil { handleActionError(ctx, err) - if ctx.Written() { - return - } + return } switch ctx.PathParam("action") { From b80f349a2b88a8d5c297249080b736ebab70340a Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 30 Jan 2025 09:05:57 +0800 Subject: [PATCH 13/13] Update services/repository/transfer.go --- services/repository/transfer.go | 1 - 1 file changed, 1 deletion(-) diff --git a/services/repository/transfer.go b/services/repository/transfer.go index 61613b332ecdb..bd3bf326b4f54 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -390,7 +390,6 @@ func ChangeRepositoryName(ctx context.Context, doer *user_model.User, repo *repo func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.User, repo *repo_model.Repository, teams []*organization.Team) error { releaser, err := globallock.Lock(ctx, getRepoWorkingLockKey(repo.ID)) if err != nil { - log.Error("lock.Lock(): %v", err) return fmt.Errorf("lock.Lock: %w", err) } defer releaser()