From 89cbc80b5123af49ee7a3a7c968c9baf6e6d6694 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Tue, 21 May 2024 02:16:17 +0000 Subject: [PATCH 01/14] improve --- 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 | 7 +++++++ 5 files changed, 28 insertions(+) diff --git a/models/issues/pull.go b/models/issues/pull.go index 4194df2e3d535..734919f4464cd 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 @@ -556,6 +558,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 eef4f5696a7f1..c797f6e52a358 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 38a32a73c78e7..2b5916d0fcf7e 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 bbdc6ca631aed..2a0c84107fbca 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 5c0ea42d77525..bfff0b9fbb4f5 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -48,6 +48,13 @@ 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 + if canCreate, err := repo_model.IsOwnerMemberCollaborator(ctx, repo, issue.Poster.ID); err != nil { + return err + } else if !canCreate { + return issues_model.ErrMustCollaborator + } + prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) if err != nil { if !git_model.IsErrBranchNotExist(err) { From 8b28a6dff1d65bb1c92686441521e033972e43bb Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Tue, 21 May 2024 02:46:10 +0000 Subject: [PATCH 02/14] add test --- tests/integration/api_pull_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 9bf0d3d745179..c0b07ec769763 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" @@ -122,6 +123,19 @@ func TestAPICreatePullSuccess(t *testing.T) { Base: "master", Title: "create a failure pr", }).AddTokenAuth(token) + // user should be a collaborator + MakeRequest(t, req, http.StatusForbidden) + + // add owner11 to be a collaborator + testCtx := NewAPITestContext(t, repo10.OwnerName, repo10.Name, auth_model.AccessTokenScopeWriteRepository) + t.Run("AddOwner11AsCollaborator", doAPIAddCollaborator(testCtx, owner11.Name, perm.AccessModeRead)) + + // create again + req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &api.CreatePullRequestOption{ + Head: fmt.Sprintf("%s:master", owner11.Name), + Base: "master", + Title: "create a failure pr", + }).AddTokenAuth(token) MakeRequest(t, req, http.StatusCreated) MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail } From 3bf5a7605391268c115fd09766be92077a797800 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Tue, 21 May 2024 06:25:05 +0000 Subject: [PATCH 03/14] improve test --- models/fixtures/collaboration.yml | 6 ++++++ services/pull/pull.go | 10 ++++++---- tests/integration/api_pull_test.go | 29 ++++++++++++++++++++--------- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/models/fixtures/collaboration.yml b/models/fixtures/collaboration.yml index 4c3ac367f6b59..16447c9607ac5 100644 --- a/models/fixtures/collaboration.yml +++ b/models/fixtures/collaboration.yml @@ -63,3 +63,9 @@ repo_id: 32 user_id: 10 mode: 2 # write + +- + id: 12 + repo_id: 10 + user_id: 13 + mode: 2 # write diff --git a/services/pull/pull.go b/services/pull/pull.go index bfff0b9fbb4f5..108a846ee04bb 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -49,10 +49,12 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss } // user should be a collaborator or a member of the organization - if canCreate, err := repo_model.IsOwnerMemberCollaborator(ctx, repo, issue.Poster.ID); err != nil { - return err - } else if !canCreate { - return issues_model.ErrMustCollaborator + if !issue.Poster.IsAdmin { + if canCreate, err := repo_model.IsOwnerMemberCollaborator(ctx, repo, issue.Poster.ID); err != nil { + return err + } else if !canCreate { + return issues_model.ErrMustCollaborator + } } prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index c0b07ec769763..d7f1c31e64f2f 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -123,21 +123,32 @@ func TestAPICreatePullSuccess(t *testing.T) { Base: "master", Title: "create a failure pr", }).AddTokenAuth(token) - // user should be a collaborator + MakeRequest(t, req, http.StatusCreated) + MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail +} + +func TestAPICreatePullCollaboratorSuccess(t *testing.T) { + defer tests.PrepareTestEnv(t)() + repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) + owner10 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo10.OwnerID}) + + session := loginUser(t, "user2") + opts := &api.CreatePullRequestOption{ + Head: "user13:master", + Base: "master", + Title: "create a collaborator pr", + } + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + 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 owner11 to be a collaborator + // add user2 to be a collaborator testCtx := NewAPITestContext(t, repo10.OwnerName, repo10.Name, auth_model.AccessTokenScopeWriteRepository) - t.Run("AddOwner11AsCollaborator", doAPIAddCollaborator(testCtx, owner11.Name, perm.AccessModeRead)) + t.Run("AddUser2AsCollaborator", doAPIAddCollaborator(testCtx, "user2", perm.AccessModeRead)) // create again - req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &api.CreatePullRequestOption{ - Head: fmt.Sprintf("%s:master", owner11.Name), - Base: "master", - Title: "create a failure pr", - }).AddTokenAuth(token) + 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) - MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail } func TestAPICreatePullSameRepoSuccess(t *testing.T) { From 02afab3da4e72d1a06d76b0fc8d4c99128b322c4 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Tue, 21 May 2024 06:27:12 +0000 Subject: [PATCH 04/14] fix access --- models/fixtures/access.yml | 44 ++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/models/fixtures/access.yml b/models/fixtures/access.yml index 4171e31fef777..0a90b97288a6a 100644 --- a/models/fixtures/access.yml +++ b/models/fixtures/access.yml @@ -54,120 +54,126 @@ - id: 10 + user_id: 13 + repo_id: 10 + mode: 2 + +- + id: 11 user_id: 15 repo_id: 21 mode: 2 - - id: 11 + id: 12 user_id: 15 repo_id: 22 mode: 2 - - id: 12 + id: 13 user_id: 15 repo_id: 23 mode: 4 - - id: 13 + id: 14 user_id: 15 repo_id: 24 mode: 4 - - id: 14 + id: 15 user_id: 15 repo_id: 32 mode: 2 - - id: 15 + id: 16 user_id: 18 repo_id: 21 mode: 2 - - id: 16 + id: 17 user_id: 18 repo_id: 22 mode: 2 - - id: 17 + id: 18 user_id: 18 repo_id: 23 mode: 4 - - id: 18 + id: 19 user_id: 18 repo_id: 24 mode: 4 - - id: 19 + id: 20 user_id: 20 repo_id: 24 mode: 1 - - id: 20 + id: 21 user_id: 20 repo_id: 27 mode: 4 - - id: 21 + id: 22 user_id: 20 repo_id: 28 mode: 4 - - id: 22 + id: 23 user_id: 29 repo_id: 4 mode: 2 - - id: 23 + id: 24 user_id: 29 repo_id: 24 mode: 1 - - id: 24 + id: 25 user_id: 31 repo_id: 27 mode: 4 - - id: 25 + id: 26 user_id: 31 repo_id: 28 mode: 4 - - id: 26 + id: 27 user_id: 38 repo_id: 60 mode: 2 - - id: 27 + id: 28 user_id: 38 repo_id: 61 mode: 1 - - id: 28 + id: 29 user_id: 39 repo_id: 61 mode: 1 - - id: 29 + id: 30 user_id: 40 repo_id: 61 mode: 4 From c6cd9c6308b85b1d7eff406507a38a03c9f08434 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Wed, 22 May 2024 01:19:48 +0000 Subject: [PATCH 05/14] fix TestPullRequestTargetEvent --- tests/integration/actions_trigger_test.go | 38 +++++++++++++---------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index 2a2fdceb61e90..ed0c607374374 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{ From c37be6eda79730f5fa32ffb8de48178045b5f17e Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Wed, 22 May 2024 01:23:30 +0000 Subject: [PATCH 06/14] fix TestPullView_CodeOwner --- tests/integration/pull_review_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 273332a36b416..e64966f0ad18e 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -11,8 +11,10 @@ import ( "strings" "testing" + 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" @@ -150,6 +152,10 @@ func TestPullView_CodeOwner(t *testing.T) { }) assert.NoError(t, err) + // add user5 as the collaborator + ctx := NewAPITestContext(t, repo.OwnerName, repo.Name, auth_model.AccessTokenScopeWriteRepository) + t.Run("AddUser5AsCollaboratorWithReadAccess", doAPIAddCollaborator(ctx, "user5", perm.AccessModeRead)) + // create a new branch to prepare for pull request _, err = files_service.ChangeRepoFiles(db.DefaultContext, forkedRepo, user5, &files_service.ChangeRepoFilesOptions{ NewBranch: "codeowner-basebranch-forked", @@ -164,7 +170,6 @@ func TestPullView_CodeOwner(t *testing.T) { assert.NoError(t, err) session := loginUser(t, "user5") - // create a pull request on the forked repository, code reviewers should not be mentioned testPullCreateDirectly(t, session, "user5", "test_codeowner", forkedRepo.DefaultBranch, "", "", "codeowner-basebranch-forked", "Test Pull Request on Forked Repository") From cfb88d2b3f70944f55662b4f68ba05cac93cf876 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Wed, 22 May 2024 01:27:29 +0000 Subject: [PATCH 07/14] Revert "fix access" This reverts commit 02afab3da4e72d1a06d76b0fc8d4c99128b322c4. --- models/fixtures/access.yml | 44 ++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/models/fixtures/access.yml b/models/fixtures/access.yml index 0a90b97288a6a..4171e31fef777 100644 --- a/models/fixtures/access.yml +++ b/models/fixtures/access.yml @@ -54,126 +54,120 @@ - id: 10 - user_id: 13 - repo_id: 10 - mode: 2 - -- - id: 11 user_id: 15 repo_id: 21 mode: 2 - - id: 12 + id: 11 user_id: 15 repo_id: 22 mode: 2 - - id: 13 + id: 12 user_id: 15 repo_id: 23 mode: 4 - - id: 14 + id: 13 user_id: 15 repo_id: 24 mode: 4 - - id: 15 + id: 14 user_id: 15 repo_id: 32 mode: 2 - - id: 16 + id: 15 user_id: 18 repo_id: 21 mode: 2 - - id: 17 + id: 16 user_id: 18 repo_id: 22 mode: 2 - - id: 18 + id: 17 user_id: 18 repo_id: 23 mode: 4 - - id: 19 + id: 18 user_id: 18 repo_id: 24 mode: 4 - - id: 20 + id: 19 user_id: 20 repo_id: 24 mode: 1 - - id: 21 + id: 20 user_id: 20 repo_id: 27 mode: 4 - - id: 22 + id: 21 user_id: 20 repo_id: 28 mode: 4 - - id: 23 + id: 22 user_id: 29 repo_id: 4 mode: 2 - - id: 24 + id: 23 user_id: 29 repo_id: 24 mode: 1 - - id: 25 + id: 24 user_id: 31 repo_id: 27 mode: 4 - - id: 26 + id: 25 user_id: 31 repo_id: 28 mode: 4 - - id: 27 + id: 26 user_id: 38 repo_id: 60 mode: 2 - - id: 28 + id: 27 user_id: 38 repo_id: 61 mode: 1 - - id: 29 + id: 28 user_id: 39 repo_id: 61 mode: 1 - - id: 30 + id: 29 user_id: 40 repo_id: 61 mode: 4 From e0ba2f74df5e137341b50c2487d51b6d4826d7f7 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Wed, 22 May 2024 01:28:06 +0000 Subject: [PATCH 08/14] revert change fixture --- models/fixtures/collaboration.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/models/fixtures/collaboration.yml b/models/fixtures/collaboration.yml index 16447c9607ac5..4c3ac367f6b59 100644 --- a/models/fixtures/collaboration.yml +++ b/models/fixtures/collaboration.yml @@ -63,9 +63,3 @@ repo_id: 32 user_id: 10 mode: 2 # write - -- - id: 12 - repo_id: 10 - user_id: 13 - mode: 2 # write From 783f61486bd5659a4f169aac26a44c894a097310 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Wed, 22 May 2024 01:30:22 +0000 Subject: [PATCH 09/14] fix TestAPICreatePullWithFieldsSuccess --- tests/integration/api_pull_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index d7f1c31e64f2f..42db467568f0a 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -177,6 +177,10 @@ func TestAPICreatePullWithFieldsSuccess(t *testing.T) { repo11 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11}) owner11 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo11.OwnerID}) + // add user3 as the collaborator + ctx := NewAPITestContext(t, repo10.OwnerName, repo10.Name, auth_model.AccessTokenScopeWriteRepository) + t.Run("AddOwner11AsCollaboratorWithReadAccess", doAPIAddCollaborator(ctx, owner11.Name, perm.AccessModeRead)) + session := loginUser(t, owner11.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) From 1ea0f1e27915277d49879fa8e08f8e6b62b247d4 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Wed, 22 May 2024 02:09:05 +0000 Subject: [PATCH 10/14] fix TestAPICreatePullSuccess --- tests/integration/api_pull_test.go | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 42db467568f0a..597c870dba5a8 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -118,37 +118,22 @@ func TestAPICreatePullSuccess(t *testing.T) { session := loginUser(t, owner11.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) - req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &api.CreatePullRequestOption{ + opts := &api.CreatePullRequestOption{ Head: fmt.Sprintf("%s:master", owner11.Name), Base: "master", Title: "create a failure pr", - }).AddTokenAuth(token) - MakeRequest(t, req, http.StatusCreated) - MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail -} - -func TestAPICreatePullCollaboratorSuccess(t *testing.T) { - defer tests.PrepareTestEnv(t)() - repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) - owner10 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo10.OwnerID}) - - session := loginUser(t, "user2") - opts := &api.CreatePullRequestOption{ - Head: "user13:master", - Base: "master", - Title: "create a collaborator pr", } - token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) - req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), opts).AddTokenAuth(token) + 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 user2 to be a collaborator - testCtx := NewAPITestContext(t, repo10.OwnerName, repo10.Name, auth_model.AccessTokenScopeWriteRepository) - t.Run("AddUser2AsCollaborator", doAPIAddCollaborator(testCtx, "user2", perm.AccessModeRead)) + // add owner11 to be a collaborator + ctx := NewAPITestContext(t, repo10.OwnerName, repo10.Name, auth_model.AccessTokenScopeWriteRepository) + t.Run("AddOwner11AsCollaborator", doAPIAddCollaborator(ctx, owner11.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) + 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) + MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail } func TestAPICreatePullSameRepoSuccess(t *testing.T) { From e9638dc0de608eac7298baaac41689721e48a1a4 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Mon, 27 May 2024 02:47:29 +0000 Subject: [PATCH 11/14] add another condition --- services/pull/pull.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 108a846ee04bb..c5945fdd30987 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,14 +50,26 @@ 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 + // user should be a collaborator or a member of the organization for base repo + var err error + canCreate := false if !issue.Poster.IsAdmin { - if canCreate, err := repo_model.IsOwnerMemberCollaborator(ctx, repo, issue.Poster.ID); err != nil { + canCreate, err = repo_model.IsOwnerMemberCollaborator(ctx, repo, issue.Poster.ID) + if err != nil { return err - } else if !canCreate { - return issues_model.ErrMustCollaborator } } + // 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 !canCreate && !perm.CanWrite(unit.TypeCode) { + return issues_model.ErrMustCollaborator + } prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) if err != nil { From 28b43b5e632a223821412f1b1245d5845b444e75 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Mon, 27 May 2024 03:00:11 +0000 Subject: [PATCH 12/14] fix test --- tests/integration/api_pull_test.go | 62 +++++++++++++++++++++++---- tests/integration/pull_review_test.go | 7 --- 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 597c870dba5a8..8239878d2b789 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -118,22 +118,72 @@ func TestAPICreatePullSuccess(t *testing.T) { session := loginUser(t, owner11.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) - opts := &api.CreatePullRequestOption{ + req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &api.CreatePullRequestOption{ Head: fmt.Sprintf("%s:master", owner11.Name), Base: "master", Title: "create a failure pr", + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + 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 owner11 to be a collaborator + // add user4 to be a collaborator to base repo ctx := NewAPITestContext(t, repo10.OwnerName, repo10.Name, auth_model.AccessTokenScopeWriteRepository) - t.Run("AddOwner11AsCollaborator", doAPIAddCollaborator(ctx, owner11.Name, perm.AccessModeRead)) + 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) - MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail +} + +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) { @@ -162,10 +212,6 @@ func TestAPICreatePullWithFieldsSuccess(t *testing.T) { repo11 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11}) owner11 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo11.OwnerID}) - // add user3 as the collaborator - ctx := NewAPITestContext(t, repo10.OwnerName, repo10.Name, auth_model.AccessTokenScopeWriteRepository) - t.Run("AddOwner11AsCollaboratorWithReadAccess", doAPIAddCollaborator(ctx, owner11.Name, perm.AccessModeRead)) - session := loginUser(t, owner11.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index e64966f0ad18e..61220a672d31e 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -11,10 +11,8 @@ import ( "strings" "testing" - 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" @@ -152,10 +150,6 @@ func TestPullView_CodeOwner(t *testing.T) { }) assert.NoError(t, err) - // add user5 as the collaborator - ctx := NewAPITestContext(t, repo.OwnerName, repo.Name, auth_model.AccessTokenScopeWriteRepository) - t.Run("AddUser5AsCollaboratorWithReadAccess", doAPIAddCollaborator(ctx, "user5", perm.AccessModeRead)) - // create a new branch to prepare for pull request _, err = files_service.ChangeRepoFiles(db.DefaultContext, forkedRepo, user5, &files_service.ChangeRepoFilesOptions{ NewBranch: "codeowner-basebranch-forked", @@ -168,7 +162,6 @@ func TestPullView_CodeOwner(t *testing.T) { }, }) assert.NoError(t, err) - session := loginUser(t, "user5") // create a pull request on the forked repository, code reviewers should not be mentioned testPullCreateDirectly(t, session, "user5", "test_codeowner", forkedRepo.DefaultBranch, "", "", "codeowner-basebranch-forked", "Test Pull Request on Forked Repository") From 3a19a3c8338d5c7339b4706a7363c21e13aea13a Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Mon, 27 May 2024 03:01:02 +0000 Subject: [PATCH 13/14] remove --- tests/integration/pull_review_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 61220a672d31e..273332a36b416 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -162,7 +162,9 @@ func TestPullView_CodeOwner(t *testing.T) { }, }) assert.NoError(t, err) + session := loginUser(t, "user5") + // create a pull request on the forked repository, code reviewers should not be mentioned testPullCreateDirectly(t, session, "user5", "test_codeowner", forkedRepo.DefaultBranch, "", "", "codeowner-basebranch-forked", "Test Pull Request on Forked Repository") From 386546931b2d5d0c3a75e852625d5445489b0892 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Mon, 27 May 2024 06:48:59 +0000 Subject: [PATCH 14/14] fix logic --- services/pull/pull.go | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index c5945fdd30987..e69c842a2d4b5 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -51,24 +51,25 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss } // user should be a collaborator or a member of the organization for base repo - var err error - canCreate := false if !issue.Poster.IsAdmin { - canCreate, err = repo_model.IsOwnerMemberCollaborator(ctx, repo, issue.Poster.ID) + canCreate, err := repo_model.IsOwnerMemberCollaborator(ctx, repo, issue.Poster.ID) if err != nil { return err } - } - // 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 !canCreate && !perm.CanWrite(unit.TypeCode) { - return issues_model.ErrMustCollaborator + + 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)