Skip to content

Commit e0a408e

Browse files
yp05327yp05327
authored
Add permission check when creating PR (#31033)
user should be a collaborator of the base repo to create a PR
1 parent d109923 commit e0a408e

File tree

7 files changed

+127
-16
lines changed

7 files changed

+127
-16
lines changed

‎models/issues/pull.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ import (
2727
"xorm.io/builder"
2828
)
2929

30+
var ErrMustCollaborator = util.NewPermissionDeniedErrorf("user must be a collaborator")
31+
3032
// ErrPullRequestNotExist represents a "PullRequestNotExist" kind of error.
3133
type ErrPullRequestNotExist struct {
3234
ID int64
@@ -572,6 +574,12 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *Iss
572574
return nil
573575
}
574576

577+
// ErrUserMustCollaborator represents an error that the user must be a collaborator to a given repo.
578+
type ErrUserMustCollaborator struct {
579+
UserID int64
580+
RepoName string
581+
}
582+
575583
// GetUnmergedPullRequest returns a pull request that is open and has not been merged
576584
// by given head/base and repo/branch.
577585
func GetUnmergedPullRequest(ctx context.Context, headRepoID, baseRepoID int64, headBranch, baseBranch string, flow PullRequestFlow) (*PullRequest, error) {

‎options/locale/locale_en-US.ini‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1765,6 +1765,7 @@ compare.compare_head = compare
17651765
pulls.desc = Enable pull requests and code reviews.
17661766
pulls.new = New Pull Request
17671767
pulls.new.blocked_user = Cannot create pull request because you are blocked by the repository owner.
1768+
pulls.new.must_collaborator = You must be a collaborator to create pull request.
17681769
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
17691770
pulls.view = View Pull Request
17701771
pulls.compare_changes = New Pull Request

‎routers/api/v1/repo/pull.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -535,6 +535,8 @@ func CreatePullRequest(ctx *context.APIContext) {
535535
ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err)
536536
} else if errors.Is(err, user_model.ErrBlockedUser) {
537537
ctx.Error(http.StatusForbidden, "BlockedUser", err)
538+
} else if errors.Is(err, issues_model.ErrMustCollaborator) {
539+
ctx.Error(http.StatusForbidden, "MustCollaborator", err)
538540
} else {
539541
ctx.Error(http.StatusInternalServerError, "NewPullRequest", err)
540542
}

‎routers/web/repo/pull.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1337,6 +1337,16 @@ func CompareAndPullRequestPost(ctx *context.Context) {
13371337
return
13381338
}
13391339
ctx.JSONError(flashError)
1340+
} else if errors.Is(err, issues_model.ErrMustCollaborator) {
1341+
flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
1342+
"Message": ctx.Tr("repo.pulls.push_rejected"),
1343+
"Summary": ctx.Tr("repo.pulls.new.must_collaborator"),
1344+
})
1345+
if err != nil {
1346+
ctx.ServerError("CompareAndPullRequest.HTMLString", err)
1347+
return
1348+
}
1349+
ctx.JSONError(flashError)
13401350
}
13411351
return
13421352
}

‎services/pull/pull.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@ import (
1717
"code.gitea.io/gitea/models/db"
1818
git_model "code.gitea.io/gitea/models/git"
1919
issues_model "code.gitea.io/gitea/models/issues"
20+
access_model "code.gitea.io/gitea/models/perm/access"
2021
repo_model "code.gitea.io/gitea/models/repo"
22+
"code.gitea.io/gitea/models/unit"
2123
user_model "code.gitea.io/gitea/models/user"
2224
"code.gitea.io/gitea/modules/base"
2325
"code.gitea.io/gitea/modules/container"
@@ -48,6 +50,28 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss
4850
return user_model.ErrBlockedUser
4951
}
5052

53+
// user should be a collaborator or a member of the organization for base repo
54+
if !issue.Poster.IsAdmin {
55+
canCreate, err := repo_model.IsOwnerMemberCollaborator(ctx, repo, issue.Poster.ID)
56+
if err != nil {
57+
return err
58+
}
59+
60+
if !canCreate {
61+
// or user should have write permission in the head repo
62+
if err := pr.LoadHeadRepo(ctx); err != nil {
63+
return err
64+
}
65+
perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, issue.Poster)
66+
if err != nil {
67+
return err
68+
}
69+
if !perm.CanWrite(unit.TypeCode) {
70+
return issues_model.ErrMustCollaborator
71+
}
72+
}
73+
}
74+
5175
prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
5276
if err != nil {
5377
if !git_model.IsErrBranchNotExist(err) {

‎tests/integration/actions_trigger_test.go‎

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,11 @@ import (
1111
"time"
1212

1313
actions_model "code.gitea.io/gitea/models/actions"
14+
auth_model "code.gitea.io/gitea/models/auth"
1415
"code.gitea.io/gitea/models/db"
1516
git_model "code.gitea.io/gitea/models/git"
1617
issues_model "code.gitea.io/gitea/models/issues"
18+
"code.gitea.io/gitea/models/perm"
1719
repo_model "code.gitea.io/gitea/models/repo"
1820
unit_model "code.gitea.io/gitea/models/unit"
1921
"code.gitea.io/gitea/models/unittest"
@@ -34,7 +36,7 @@ import (
3436
func TestPullRequestTargetEvent(t *testing.T) {
3537
onGiteaRun(t, func(t *testing.T, u *url.URL) {
3638
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // owner of the base repo
37-
org3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) // owner of the forked repo
39+
user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) // owner of the forked repo
3840

3941
// create the base repo
4042
baseRepo, err := repo_service.CreateRepository(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{
@@ -57,8 +59,12 @@ func TestPullRequestTargetEvent(t *testing.T) {
5759
}}, nil)
5860
assert.NoError(t, err)
5961

62+
// add user4 as the collaborator
63+
ctx := NewAPITestContext(t, baseRepo.OwnerName, baseRepo.Name, auth_model.AccessTokenScopeWriteRepository)
64+
t.Run("AddUser4AsCollaboratorWithReadAccess", doAPIAddCollaborator(ctx, "user4", perm.AccessModeRead))
65+
6066
// create the forked repo
61-
forkedRepo, err := repo_service.ForkRepository(git.DefaultContext, user2, org3, repo_service.ForkRepoOptions{
67+
forkedRepo, err := repo_service.ForkRepository(git.DefaultContext, user2, user4, repo_service.ForkRepoOptions{
6268
BaseRepo: baseRepo,
6369
Name: "forked-repo-pull-request-target",
6470
Description: "test pull-request-target event",
@@ -95,7 +101,7 @@ func TestPullRequestTargetEvent(t *testing.T) {
95101
assert.NotEmpty(t, addWorkflowToBaseResp)
96102

97103
// add a new file to the forked repo
98-
addFileToForkedResp, err := files_service.ChangeRepoFiles(git.DefaultContext, forkedRepo, org3, &files_service.ChangeRepoFilesOptions{
104+
addFileToForkedResp, err := files_service.ChangeRepoFiles(git.DefaultContext, forkedRepo, user4, &files_service.ChangeRepoFilesOptions{
99105
Files: []*files_service.ChangeRepoFile{
100106
{
101107
Operation: "create",
@@ -107,12 +113,12 @@ func TestPullRequestTargetEvent(t *testing.T) {
107113
OldBranch: "main",
108114
NewBranch: "fork-branch-1",
109115
Author: &files_service.IdentityOptions{
110-
Name: org3.Name,
111-
Email: org3.Email,
116+
Name: user4.Name,
117+
Email: user4.Email,
112118
},
113119
Committer: &files_service.IdentityOptions{
114-
Name: org3.Name,
115-
Email: org3.Email,
120+
Name: user4.Name,
121+
Email: user4.Email,
116122
},
117123
Dates: &files_service.CommitDateOptions{
118124
Author: time.Now(),
@@ -126,8 +132,8 @@ func TestPullRequestTargetEvent(t *testing.T) {
126132
pullIssue := &issues_model.Issue{
127133
RepoID: baseRepo.ID,
128134
Title: "Test pull-request-target-event",
129-
PosterID: org3.ID,
130-
Poster: org3,
135+
PosterID: user4.ID,
136+
Poster: user4,
131137
IsPull: true,
132138
}
133139
pullRequest := &issues_model.PullRequest{
@@ -149,7 +155,7 @@ func TestPullRequestTargetEvent(t *testing.T) {
149155
assert.Equal(t, actions_module.GithubEventPullRequestTarget, actionRun.TriggerEvent)
150156

151157
// add another file whose name cannot match the specified path
152-
addFileToForkedResp, err = files_service.ChangeRepoFiles(git.DefaultContext, forkedRepo, org3, &files_service.ChangeRepoFilesOptions{
158+
addFileToForkedResp, err = files_service.ChangeRepoFiles(git.DefaultContext, forkedRepo, user4, &files_service.ChangeRepoFilesOptions{
153159
Files: []*files_service.ChangeRepoFile{
154160
{
155161
Operation: "create",
@@ -161,12 +167,12 @@ func TestPullRequestTargetEvent(t *testing.T) {
161167
OldBranch: "main",
162168
NewBranch: "fork-branch-2",
163169
Author: &files_service.IdentityOptions{
164-
Name: org3.Name,
165-
Email: org3.Email,
170+
Name: user4.Name,
171+
Email: user4.Email,
166172
},
167173
Committer: &files_service.IdentityOptions{
168-
Name: org3.Name,
169-
Email: org3.Email,
174+
Name: user4.Name,
175+
Email: user4.Email,
170176
},
171177
Dates: &files_service.CommitDateOptions{
172178
Author: time.Now(),
@@ -180,8 +186,8 @@ func TestPullRequestTargetEvent(t *testing.T) {
180186
pullIssue = &issues_model.Issue{
181187
RepoID: baseRepo.ID,
182188
Title: "A mismatched path cannot trigger pull-request-target-event",
183-
PosterID: org3.ID,
184-
Poster: org3,
189+
PosterID: user4.ID,
190+
Poster: user4,
185191
IsPull: true,
186192
}
187193
pullRequest = &issues_model.PullRequest{

‎tests/integration/api_pull_test.go‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
auth_model "code.gitea.io/gitea/models/auth"
1313
"code.gitea.io/gitea/models/db"
1414
issues_model "code.gitea.io/gitea/models/issues"
15+
"code.gitea.io/gitea/models/perm"
1516
repo_model "code.gitea.io/gitea/models/repo"
1617
"code.gitea.io/gitea/models/unittest"
1718
user_model "code.gitea.io/gitea/models/user"
@@ -126,6 +127,65 @@ func TestAPICreatePullSuccess(t *testing.T) {
126127
MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail
127128
}
128129

130+
func TestAPICreatePullBasePermission(t *testing.T) {
131+
defer tests.PrepareTestEnv(t)()
132+
repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10})
133+
// repo10 have code, pulls units.
134+
repo11 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11})
135+
// repo11 only have code unit but should still create pulls
136+
owner10 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo10.OwnerID})
137+
user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
138+
139+
session := loginUser(t, user4.Name)
140+
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
141+
opts := &api.CreatePullRequestOption{
142+
Head: fmt.Sprintf("%s:master", repo11.OwnerName),
143+
Base: "master",
144+
Title: "create a failure pr",
145+
}
146+
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
147+
MakeRequest(t, req, http.StatusForbidden)
148+
149+
// add user4 to be a collaborator to base repo
150+
ctx := NewAPITestContext(t, repo10.OwnerName, repo10.Name, auth_model.AccessTokenScopeWriteRepository)
151+
t.Run("AddUser4AsCollaborator", doAPIAddCollaborator(ctx, user4.Name, perm.AccessModeRead))
152+
153+
// create again
154+
req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
155+
MakeRequest(t, req, http.StatusCreated)
156+
}
157+
158+
func TestAPICreatePullHeadPermission(t *testing.T) {
159+
defer tests.PrepareTestEnv(t)()
160+
repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10})
161+
// repo10 have code, pulls units.
162+
repo11 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11})
163+
// repo11 only have code unit but should still create pulls
164+
owner10 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo10.OwnerID})
165+
user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
166+
167+
session := loginUser(t, user4.Name)
168+
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
169+
opts := &api.CreatePullRequestOption{
170+
Head: fmt.Sprintf("%s:master", repo11.OwnerName),
171+
Base: "master",
172+
Title: "create a failure pr",
173+
}
174+
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
175+
MakeRequest(t, req, http.StatusForbidden)
176+
177+
// add user4 to be a collaborator to head repo with read permission
178+
ctx := NewAPITestContext(t, repo11.OwnerName, repo11.Name, auth_model.AccessTokenScopeWriteRepository)
179+
t.Run("AddUser4AsCollaboratorWithRead", doAPIAddCollaborator(ctx, user4.Name, perm.AccessModeRead))
180+
req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
181+
MakeRequest(t, req, http.StatusForbidden)
182+
183+
// add user4 to be a collaborator to head repo with write permission
184+
t.Run("AddUser4AsCollaboratorWithWrite", doAPIAddCollaborator(ctx, user4.Name, perm.AccessModeWrite))
185+
req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
186+
MakeRequest(t, req, http.StatusCreated)
187+
}
188+
129189
func TestAPICreatePullSameRepoSuccess(t *testing.T) {
130190
defer tests.PrepareTestEnv(t)()
131191
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})

0 commit comments

Comments
 (0)