Refactor repository transfer (#33211)

- Both have `RejectTransfer` and `CancelTransfer` because the permission
checks are not the same. `CancelTransfer` can be done by the doer or
those who have admin permission to access this repository.
`RejectTransfer` can be done by the receiver user if it's an individual
or those who can create repositories if it's an organization.

- Some tests are wrong, this PR corrects them.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

services/context/repo.go

@@ -653,7 +653,7 @@ func RepoAssignment(ctx *Context) {
 
 		ctx.Data["RepoTransfer"] = repoTransfer
 		if ctx.Doer != nil {
-			ctx.Data["CanUserAcceptTransfer"] = repoTransfer.CanUserAcceptTransfer(ctx, ctx.Doer)
+			ctx.Data["CanUserAcceptOrRejectTransfer"] = repoTransfer.CanUserAcceptOrRejectTransfer(ctx, ctx.Doer)
 		}
 	}
 

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

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,44 @@ 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 {
+	oldOwnerName := repo.OwnerName
+
+	if err := db.WithTx(ctx, func(ctx context.Context) error {
+		if err := repoTransfer.LoadAttributes(ctx); err != nil {
 			return err
 		}
-	}
 
-	notify_service.TransferRepository(ctx, doer, repo, oldOwner.Name)
+		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, oldOwnerName)
 
 	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
@@ -138,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)
 	}
 
@@ -174,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)
@@ -225,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
@@ -261,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)
 	}
@@ -273,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
@@ -301,6 +299,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 +352,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.
@@ -387,70 +388,142 @@ 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 {
+		return fmt.Errorf("lock.Lock: %w", err)
+	}
+	defer releaser()
+
 	if err := repo_model.TestRepositoryReadyForTransfer(repo.Status); err != nil {
 		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 isDirectTransfer bool
+	oldOwnerName := repo.OwnerName
 
-	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,
+		// then it will transfer directly without acceptance.
+		if doer.IsAdmin || doer.ID == newOwner.ID {
+			isDirectTransfer = 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 user_model.IsUserBlockedBy(ctx, doer, newOwner.ID) {
+			return user_model.ErrBlockedUser
+		}
+
+		// 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 {
+				isDirectTransfer = 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 allowed {
-			return TransferOwnership(ctx, doer, newOwner, repo, teams)
+		if !hasAccess {
+			if err := AddOrUpdateCollaborator(ctx, repo, newOwner, perm.AccessModeRead); err != nil {
+				return err
+			}
 		}
-	}
 
-	// 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 isDirectTransfer {
+		notify_service.TransferRepository(ctx, doer, repo, oldOwnerName)
+	} 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()
+// 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 {
+			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)
+	})
+}
+
+func canUserCancelTransfer(ctx context.Context, r *repo_model.RepoTransfer, u *user_model.User) bool {
+	if u.IsAdmin || 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.IsOwner()
+}
+
+// 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)
+	})
 }

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, TransferOwnership(db.DefaultContext, doer, doer, repo, nil))
+	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",
 	})
@@ -61,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)
@@ -82,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)
@@ -90,7 +93,7 @@ func TestRepositoryTransfer(t *testing.T) {
 	assert.NotNil(t, transfer)
 
 	// Cancel transfer
-	assert.NoError(t, CancelRepositoryTransfer(db.DefaultContext, repo))
+	assert.NoError(t, CancelRepositoryTransfer(db.DefaultContext, transfer, doer))
 
 	transfer, err = repo_model.GetPendingRepositoryTransfer(db.DefaultContext, repo)
 	assert.Error(t, err)
@@ -113,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, CancelRepositoryTransfer(db.DefaultContext, repo))
+	// Reject transfer
+	err = RejectRepositoryTransfer(db.DefaultContext, repo2, doer)
+	assert.True(t, repo_model.IsErrNoPendingTransfer(err))
 }

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,
@@ -202,12 +202,7 @@ func cancelRepositoryTransfers(ctx context.Context, sender, recipient *user_mode
 	}
 
 	for _, transfer := range transfers {
-		repo, err := repo_model.GetRepositoryByID(ctx, transfer.RepoID)
-		if err != nil {
-			return err
-		}
-
-		if err := repo_service.CancelRepositoryTransfer(ctx, repo); err != nil {
+		if err := repo_service.CancelRepositoryTransfer(ctx, transfer, doer); err != nil {
 			return err
 		}
 	}