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 <[email protected]>
go-gitea · Jan 30, 2025 · f88dbf8 · f88dbf8
1 parent 48183d2
commit f88dbf8
Show file tree

Hide file tree

Showing 10 changed files with 297 additions and 217 deletions.
diff --git 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
 }
 
-// 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
@@ -166,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) {
@@ -206,11 +228,26 @@ 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
 		}
 
+		exist, err := IsRepositoryTransferExist(ctx, repo.ID)
+		if err != nil {
+			return err
+		}
+		if exist {
+			return ErrRepoTransferInProgress{
+				Uname: repo.Owner.LowerName,
+				Name:  repo.Name,
+			}
+		}
+
 		repo.Status = RepositoryPendingTransfer
 		if err := UpdateRepositoryCols(ctx, repo, "status"); err != nil {
 			return err

diff --git 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"
@@ -161,12 +162,16 @@ func AcceptTransfer(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 
-	err := acceptOrRejectRepoTransfer(ctx, true)
-	if ctx.Written() {
-		return
-	}
+	err := repo_service.AcceptTransferOwnership(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, "AcceptTransferOwnership", err)
+		case errors.Is(err, util.ErrPermissionDenied):
+			ctx.Error(http.StatusForbidden, "AcceptTransferOwnership", err)
+		default:
+			ctx.Error(http.StatusInternalServerError, "AcceptTransferOwnership", err)
+		}
 		return
 	}
 
@@ -199,40 +204,18 @@ 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)
+		switch {
+		case repo_model.IsErrNoPendingTransfer(err):
+			ctx.Error(http.StatusNotFound, "RejectRepositoryTransfer", err)
+		case errors.Is(err, util.ErrPermissionDenied):
+			ctx.Error(http.StatusForbidden, "RejectRepositoryTransfer", err)
+		default:
+			ctx.Error(http.StatusInternalServerError, "RejectRepositoryTransfer", err)
+		}
 		return
 	}
 
 	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
@@ -309,6 +309,36 @@ 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 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)
+	}
+}
+
 // Action response for actions to a repository
 func Action(ctx *context.Context) {
 	var err error
@@ -322,9 +352,11 @@ 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)
+		acceptTransfer(ctx)
+		return
 	case "reject_transfer":
-		err = acceptOrRejectRepoTransfer(ctx, false)
+		rejectTransfer(ctx)
+		return
 	case "desc": // FIXME: this is not used
 		if !ctx.Repo.IsOwner() {
 			ctx.Error(http.StatusNotFound)
@@ -337,12 +369,8 @@ 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 {
-			ctx.ServerError(fmt.Sprintf("Action (%s)", ctx.PathParam("action")), err)
-			return
-		}
+		handleActionError(ctx, err)
+		return
 	}
 
 	switch ctx.PathParam("action") {
@@ -377,41 +405,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
@@ -832,16 +832,10 @@ func SettingsPost(ctx *context.Context) {
 			} else {
 				ctx.ServerError("GetPendingRepositoryTransfer", err)
 			}
-
-			return
-		}
-
-		if err := repoTransfer.LoadAttributes(ctx); err != nil {
-			ctx.ServerError("LoadRecipient", err)
 			return
 		}
 
-		if err := repo_service.CancelRepositoryTransfer(ctx, ctx.Repo.Repository); 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
@@ -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)
 		}
 	}
 

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