Skip to content

Commit d3f0867

Browse files
yp05327yp05327
authored
Add permission check when creating PR (go-gitea#31033) (go-gitea#31720)
Backport go-gitea#31033 user should be a collaborator of the base repo to create a PR
1 parent 7e9a895 commit d3f0867

File tree

7 files changed

+127
-16
lines changed

7 files changed

+127
-16
lines changed

‎models/issues/pull.go

+8
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
@@ -571,6 +573,12 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *Iss
571573
return nil
572574
}
573575

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

‎options/locale/locale_en-US.ini

+1
Original file line numberDiff line numberDiff line change
@@ -1758,6 +1758,7 @@ compare.compare_head = compare
17581758
pulls.desc = Enable pull requests and code reviews.
17591759
pulls.new = New Pull Request
17601760
pulls.new.blocked_user = Cannot create pull request because you are blocked by the repository owner.
1761+
pulls.new.must_collaborator = You must be a collaborator to create pull request.
17611762
pulls.view = View Pull Request
17621763
pulls.compare_changes = New Pull Request
17631764
pulls.allow_edits_from_maintainers = Allow edits from maintainers

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

+2
Original file line numberDiff line numberDiff line change
@@ -519,6 +519,8 @@ func CreatePullRequest(ctx *context.APIContext) {
519519
ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err)
520520
} else if errors.Is(err, user_model.ErrBlockedUser) {
521521
ctx.Error(http.StatusForbidden, "BlockedUser", err)
522+
} else if errors.Is(err, issues_model.ErrMustCollaborator) {
523+
ctx.Error(http.StatusForbidden, "MustCollaborator", err)
522524
} else {
523525
ctx.Error(http.StatusInternalServerError, "NewPullRequest", err)
524526
}

‎routers/web/repo/pull.go

+10
Original file line numberDiff line numberDiff line change
@@ -1325,6 +1325,16 @@ func CompareAndPullRequestPost(ctx *context.Context) {
13251325
return
13261326
}
13271327
ctx.JSONError(flashError)
1328+
} else if errors.Is(err, issues_model.ErrMustCollaborator) {
1329+
flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
1330+
"Message": ctx.Tr("repo.pulls.push_rejected"),
1331+
"Summary": ctx.Tr("repo.pulls.new.must_collaborator"),
1332+
})
1333+
if err != nil {
1334+
ctx.ServerError("CompareAndPullRequest.HTMLString", err)
1335+
return
1336+
}
1337+
ctx.JSONError(flashError)
13281338
}
13291339
return
13301340
}

‎services/pull/pull.go

+24
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

+22-16
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

+60
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)