Add permission check when creating PR (#31033)

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

diff --git a/models/issues/pull.go b/models/issues/pull.go
index ef49a510458b139113d8a9aa19f641d672e1745b..a4e61476198d7e65c2ab96c4ace5481e1082ff3d 100644 (file)
--- a/models/issues/pull.go
+++ b/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
@@ -572,6 +574,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) {

diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
index 6a748aed0022ff61ba71eb5eec5dc52c5341eb2f..53d746ef1256c87740fbdadda250c5a2f16e98ef 100644 (file)
--- a/options/locale/locale_en-US.ini
+++ b/options/locale/locale_en-US.ini
@@ -1765,6 +1765,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.edit.already_changed = Unable to save changes to the pull request. It appears the content has already been changed by another user. Please refresh the page and try editing again to avoid overwriting their changes
 pulls.view = View Pull Request
 pulls.compare_changes = New Pull Request

diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go
index ebe876da646093df34eb7df1226232050643767d..148b6ed637f073b419d41c7ddf8931d9a9a675bd 100644 (file)
--- a/routers/api/v1/repo/pull.go
+++ b/routers/api/v1/repo/pull.go
@@ -535,6 +535,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)
                }

diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go
index f9642d44d4a383beadfd9308571e0331158ca826..9531482bee76e19e2f71bac8a439288cd5dffadf 100644 (file)
--- a/routers/web/repo/pull.go
+++ b/routers/web/repo/pull.go
@@ -1337,6 +1337,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
        }

diff --git a/services/pull/pull.go b/services/pull/pull.go
index 5c0ea42d77525a05ae799a8a611290c59ed82342..e69c842a2d4b54e6f7522067be7538c287b6584b 100644 (file)
--- a/services/pull/pull.go
+++ b/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) {

diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go
index 2a2fdceb61e903b08990cfe9b61ee14f02b38a6f..ed0c607374374fcb6e69fa6dc7094991eed15e44 100644 (file)
--- a/tests/integration/actions_trigger_test.go
+++ b/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,
-                               Email: org3.Email,
+                               Name:  user4.Name,
+                               Email: user4.Email,
                        },
                        Committer: &files_service.IdentityOptions{
-                               Name:  org3.Name,
-                               Email: org3.Email,
+                               Name:  user4.Name,
+                               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,
-                       Poster:   org3,
+                       PosterID: user4.ID,
+                       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,
-                               Email: org3.Email,
+                               Name:  user4.Name,
+                               Email: user4.Email,
                        },
                        Committer: &files_service.IdentityOptions{
-                               Name:  org3.Name,
-                               Email: org3.Email,
+                               Name:  user4.Name,
+                               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,
-                       Poster:   org3,
+                       PosterID: user4.ID,
+                       Poster:   user4,
                        IsPull:   true,
                }
                pullRequest = &issues_model.PullRequest{

diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go
index 9bf0d3d745179333869389c28500b51dee7d5257..8239878d2b789cd93aa03bf1b6aecaa6cdf6c623 100644 (file)
--- a/tests/integration/api_pull_test.go
+++ b/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})