From d3f0867204f1c09f4a7a7b6f3707c0ce140c59b2 Mon Sep 17 00:00:00 2001
From: yp05327 <576951401@qq.com>
Date: Mon, 29 Jul 2024 15:11:29 +0900
Subject: [PATCH] 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 | 8 +++
options/locale/locale_en-US.ini | 1 +
routers/api/v1/repo/pull.go | 2 +
routers/web/repo/pull.go | 10 ++++
services/pull/pull.go | 24 +++++++++
tests/integration/actions_trigger_test.go | 38 ++++++++------
tests/integration/api_pull_test.go | 60 +++++++++++++++++++++++
7 files changed, 127 insertions(+), 16 deletions(-)
diff --git a/models/issues/pull.go b/models/issues/pull.go
index 014fcd9fd0..74ea73a48f 100644
--- 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
@@ -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) {
diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
index 0c3bd1bf66..f775764d23 100644
--- a/options/locale/locale_en-US.ini
+++ b/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
diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go
index 38a32a73c7..2b5916d0fc 100644
--- a/routers/api/v1/repo/pull.go
+++ b/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)
}
diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go
index 92e0a1674e..e71b7f4be2 100644
--- a/routers/web/repo/pull.go
+++ b/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
}
diff --git a/services/pull/pull.go b/services/pull/pull.go
index 5c0ea42d77..e69c842a2d 100644
--- 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 2a2fdceb61..ed0c607374 100644
--- 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 9bf0d3d745..8239878d2b 100644
--- 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})