Add permission check when creating PR (#31033) (#31720)

Backport #31033

user should be a collaborator of the base repo to create a PR

models/issues/pull.go

@@ -27,6 +27,8 @@ import (
 	"xorm.io/builder"
 )
 
+var ErrMustCollaborator = util.NewPermissionDeniedErrorf("user must be a collaborator")
+
 // ErrPullRequestNotExist represents a "PullRequestNotExist" kind of error.
 type ErrPullRequestNotExist struct {
 	ID         int64
@@ -571,6 +573,12 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *Iss
 	return nil
 }
 
+// ErrUserMustCollaborator represents an error that the user must be a collaborator to a given repo.
+type ErrUserMustCollaborator struct {
+	UserID   int64
+	RepoName string
+}
+
 // GetUnmergedPullRequest returns a pull request that is open and has not been merged
 // by given head/base and repo/branch.
 func GetUnmergedPullRequest(ctx context.Context, headRepoID, baseRepoID int64, headBranch, baseBranch string, flow PullRequestFlow) (*PullRequest, error) {

options/locale/locale_en-US.ini

@@ -1758,6 +1758,7 @@ compare.compare_head = compare
 pulls.desc = Enable pull requests and code reviews.
 pulls.new = New Pull Request
 pulls.new.blocked_user = Cannot create pull request because you are blocked by the repository owner.
+pulls.new.must_collaborator = You must be a collaborator to create pull request.
 pulls.view = View Pull Request
 pulls.compare_changes = New Pull Request
 pulls.allow_edits_from_maintainers = Allow edits from maintainers

routers/api/v1/repo/pull.go

@@ -519,6 +519,8 @@ func CreatePullRequest(ctx *context.APIContext) {
 			ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err)
 		} else if errors.Is(err, user_model.ErrBlockedUser) {
 			ctx.Error(http.StatusForbidden, "BlockedUser", err)
+		} else if errors.Is(err, issues_model.ErrMustCollaborator) {
+			ctx.Error(http.StatusForbidden, "MustCollaborator", err)
 		} else {
 			ctx.Error(http.StatusInternalServerError, "NewPullRequest", err)
 		}

routers/web/repo/pull.go

@@ -1325,6 +1325,16 @@ func CompareAndPullRequestPost(ctx *context.Context) {
 				return
 			}
 			ctx.JSONError(flashError)
+		} else if errors.Is(err, issues_model.ErrMustCollaborator) {
+			flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
+				"Message": ctx.Tr("repo.pulls.push_rejected"),
+				"Summary": ctx.Tr("repo.pulls.new.must_collaborator"),
+			})
+			if err != nil {
+				ctx.ServerError("CompareAndPullRequest.HTMLString", err)
+				return
+			}
+			ctx.JSONError(flashError)
 		}
 		return
 	}

services/pull/pull.go

@@ -17,7 +17,9 @@ import (
 	"code.gitea.io/gitea/models/db"
 	git_model "code.gitea.io/gitea/models/git"
 	issues_model "code.gitea.io/gitea/models/issues"
+	access_model "code.gitea.io/gitea/models/perm/access"
 	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/container"
@@ -48,6 +50,28 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss
 		return user_model.ErrBlockedUser
 	}
 
+	// user should be a collaborator or a member of the organization for base repo
+	if !issue.Poster.IsAdmin {
+		canCreate, err := repo_model.IsOwnerMemberCollaborator(ctx, repo, issue.Poster.ID)
+		if err != nil {
+			return err
+		}
+
+		if !canCreate {
+			// or user should have write permission in the head repo
+			if err := pr.LoadHeadRepo(ctx); err != nil {
+				return err
+			}
+			perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, issue.Poster)
+			if err != nil {
+				return err
+			}
+			if !perm.CanWrite(unit.TypeCode) {
+				return issues_model.ErrMustCollaborator
+			}
+		}
+	}
+
 	prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
 	if err != nil {
 		if !git_model.IsErrBranchNotExist(err) {

tests/integration/actions_trigger_test.go

@@ -11,9 +11,11 @@ import (
 	"time"
 
 	actions_model "code.gitea.io/gitea/models/actions"
+	auth_model "code.gitea.io/gitea/models/auth"
 	"code.gitea.io/gitea/models/db"
 	git_model "code.gitea.io/gitea/models/git"
 	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/models/perm"
 	repo_model "code.gitea.io/gitea/models/repo"
 	unit_model "code.gitea.io/gitea/models/unit"
 	"code.gitea.io/gitea/models/unittest"
@@ -34,7 +36,7 @@ import (
 func TestPullRequestTargetEvent(t *testing.T) {
 	onGiteaRun(t, func(t *testing.T, u *url.URL) {
 		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // owner of the base repo
-		org3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3})  // owner of the forked repo
+		user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) // owner of the forked repo
 
 		// create the base repo
 		baseRepo, err := repo_service.CreateRepository(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{
@@ -57,8 +59,12 @@ func TestPullRequestTargetEvent(t *testing.T) {
 		}}, nil)
 		assert.NoError(t, err)
 
+		// add user4 as the collaborator
+		ctx := NewAPITestContext(t, baseRepo.OwnerName, baseRepo.Name, auth_model.AccessTokenScopeWriteRepository)
+		t.Run("AddUser4AsCollaboratorWithReadAccess", doAPIAddCollaborator(ctx, "user4", perm.AccessModeRead))
+
 		// create the forked repo
-		forkedRepo, err := repo_service.ForkRepository(git.DefaultContext, user2, org3, repo_service.ForkRepoOptions{
+		forkedRepo, err := repo_service.ForkRepository(git.DefaultContext, user2, user4, repo_service.ForkRepoOptions{
 			BaseRepo:    baseRepo,
 			Name:        "forked-repo-pull-request-target",
 			Description: "test pull-request-target event",
@@ -95,7 +101,7 @@ func TestPullRequestTargetEvent(t *testing.T) {
 		assert.NotEmpty(t, addWorkflowToBaseResp)
 
 		// add a new file to the forked repo
-		addFileToForkedResp, err := files_service.ChangeRepoFiles(git.DefaultContext, forkedRepo, org3, &files_service.ChangeRepoFilesOptions{
+		addFileToForkedResp, err := files_service.ChangeRepoFiles(git.DefaultContext, forkedRepo, user4, &files_service.ChangeRepoFilesOptions{
 			Files: []*files_service.ChangeRepoFile{
 				{
 					Operation:     "create",
@@ -107,12 +113,12 @@ func TestPullRequestTargetEvent(t *testing.T) {
 			OldBranch: "main",
 			NewBranch: "fork-branch-1",
 			Author: &files_service.IdentityOptions{
-				Name:  org3.Name,
+				Name:  user4.Name,
-				Email: org3.Email,
+				Email: user4.Email,
 			},
 			Committer: &files_service.IdentityOptions{
-				Name:  org3.Name,
+				Name:  user4.Name,
-				Email: org3.Email,
+				Email: user4.Email,
 			},
 			Dates: &files_service.CommitDateOptions{
 				Author:    time.Now(),
@@ -126,8 +132,8 @@ func TestPullRequestTargetEvent(t *testing.T) {
 		pullIssue := &issues_model.Issue{
 			RepoID:   baseRepo.ID,
 			Title:    "Test pull-request-target-event",
-			PosterID: org3.ID,
+			PosterID: user4.ID,
-			Poster:   org3,
+			Poster:   user4,
 			IsPull:   true,
 		}
 		pullRequest := &issues_model.PullRequest{
@@ -149,7 +155,7 @@ func TestPullRequestTargetEvent(t *testing.T) {
 		assert.Equal(t, actions_module.GithubEventPullRequestTarget, actionRun.TriggerEvent)
 
 		// add another file whose name cannot match the specified path
-		addFileToForkedResp, err = files_service.ChangeRepoFiles(git.DefaultContext, forkedRepo, org3, &files_service.ChangeRepoFilesOptions{
+		addFileToForkedResp, err = files_service.ChangeRepoFiles(git.DefaultContext, forkedRepo, user4, &files_service.ChangeRepoFilesOptions{
 			Files: []*files_service.ChangeRepoFile{
 				{
 					Operation:     "create",
@@ -161,12 +167,12 @@ func TestPullRequestTargetEvent(t *testing.T) {
 			OldBranch: "main",
 			NewBranch: "fork-branch-2",
 			Author: &files_service.IdentityOptions{
-				Name:  org3.Name,
+				Name:  user4.Name,
-				Email: org3.Email,
+				Email: user4.Email,
 			},
 			Committer: &files_service.IdentityOptions{
-				Name:  org3.Name,
+				Name:  user4.Name,
-				Email: org3.Email,
+				Email: user4.Email,
 			},
 			Dates: &files_service.CommitDateOptions{
 				Author:    time.Now(),
@@ -180,8 +186,8 @@ func TestPullRequestTargetEvent(t *testing.T) {
 		pullIssue = &issues_model.Issue{
 			RepoID:   baseRepo.ID,
 			Title:    "A mismatched path cannot trigger pull-request-target-event",
-			PosterID: org3.ID,
+			PosterID: user4.ID,
-			Poster:   org3,
+			Poster:   user4,
 			IsPull:   true,
 		}
 		pullRequest = &issues_model.PullRequest{

tests/integration/api_pull_test.go

@@ -12,6 +12,7 @@ import (
 	auth_model "code.gitea.io/gitea/models/auth"
 	"code.gitea.io/gitea/models/db"
 	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/models/perm"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
@@ -126,6 +127,65 @@ func TestAPICreatePullSuccess(t *testing.T) {
 	MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail
 }
 
+func TestAPICreatePullBasePermission(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+	repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10})
+	// repo10 have code, pulls units.
+	repo11 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11})
+	// repo11 only have code unit but should still create pulls
+	owner10 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo10.OwnerID})
+	user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
+
+	session := loginUser(t, user4.Name)
+	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
+	opts := &api.CreatePullRequestOption{
+		Head:  fmt.Sprintf("%s:master", repo11.OwnerName),
+		Base:  "master",
+		Title: "create a failure pr",
+	}
+	req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusForbidden)
+
+	// add user4 to be a collaborator to base repo
+	ctx := NewAPITestContext(t, repo10.OwnerName, repo10.Name, auth_model.AccessTokenScopeWriteRepository)
+	t.Run("AddUser4AsCollaborator", doAPIAddCollaborator(ctx, user4.Name, perm.AccessModeRead))
+
+	// create again
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusCreated)
+}
+
+func TestAPICreatePullHeadPermission(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+	repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10})
+	// repo10 have code, pulls units.
+	repo11 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11})
+	// repo11 only have code unit but should still create pulls
+	owner10 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo10.OwnerID})
+	user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
+
+	session := loginUser(t, user4.Name)
+	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
+	opts := &api.CreatePullRequestOption{
+		Head:  fmt.Sprintf("%s:master", repo11.OwnerName),
+		Base:  "master",
+		Title: "create a failure pr",
+	}
+	req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusForbidden)
+
+	// add user4 to be a collaborator to head repo with read permission
+	ctx := NewAPITestContext(t, repo11.OwnerName, repo11.Name, auth_model.AccessTokenScopeWriteRepository)
+	t.Run("AddUser4AsCollaboratorWithRead", doAPIAddCollaborator(ctx, user4.Name, perm.AccessModeRead))
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusForbidden)
+
+	// add user4 to be a collaborator to head repo with write permission
+	t.Run("AddUser4AsCollaboratorWithWrite", doAPIAddCollaborator(ctx, user4.Name, perm.AccessModeWrite))
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
+	MakeRequest(t, req, http.StatusCreated)
+}
+
 func TestAPICreatePullSameRepoSuccess(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})