diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index ddb813cf44..38325e181f 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -63,7 +63,7 @@ func GetUnmergedPullRequestsByHeadInfoMax(ctx context.Context, repoID, olderThan } // GetUnmergedPullRequestsByHeadInfo returns all pull requests that are open and has not been merged -func GetUnmergedPullRequestsByHeadInfo(ctx context.Context, repoID int64, branch string) ([]*PullRequest, error) { +func GetUnmergedPullRequestsByHeadInfo(ctx context.Context, repoID int64, branch string) (PullRequestList, error) { prs := make([]*PullRequest, 0, 2) sess := db.GetEngine(ctx). Join("INNER", "issue", "issue.id = pull_request.issue_id"). @@ -82,18 +82,44 @@ func CanMaintainerWriteToBranch(ctx context.Context, p access_model.Permission, } prs, err := GetUnmergedPullRequestsByHeadInfo(ctx, p.Units[0].RepoID, branch) + // All these error cases return `false` to defer to the safer choice of not allowing write access on an error. if err != nil { + log.Error("GetUnmergedPullRequestsByHeadInfo failed: %s", err) + return false + } else if issues, err := prs.LoadIssues(ctx); err != nil { + log.Error("LoadIssues failed: %s", err) + return false + } else if err := issues.LoadPosters(ctx); err != nil { + log.Error("LoadPosters failed: %s", err) + return false + } else if err := prs.LoadHeadRepos(ctx); err != nil { + log.Error("LoadHeadRepos failed: %s", err) return false } for _, pr := range prs { if pr.AllowMaintainerEdit { + // PR Poster must have write access to the head, so that when they turned on "AllowMaintainerEdit" they + // delegated that write access to the maintainers of the PR base. If they don't currently have write + // access, they can't delegate that access. + poster := pr.Issue.Poster + posterHeadPerm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, poster) + if err != nil { + log.Error("GetUserRepoPermission failed: %s", err) + continue + } + if !posterHeadPerm.CanWrite(unit.TypeCode) { + continue + } + err = pr.LoadBaseRepo(ctx) if err != nil { + log.Error("LoadBaseRepo failed: %s", err) continue } prPerm, err := access_model.GetUserRepoPermission(ctx, pr.BaseRepo, user) if err != nil { + log.Error("GetUserRepoPermission failed: %s", err) continue } if prPerm.CanWrite(unit.TypeCode) { @@ -240,6 +266,25 @@ func (prs PullRequestList) LoadIssues(ctx context.Context) (IssueList, error) { return issueList, nil } +func (prs PullRequestList) LoadHeadRepos(ctx context.Context) error { + repoIDs := []int64{} + for _, pr := range prs { + repoIDs = append(repoIDs, pr.HeadRepoID) + } + repos, err := db.GetByIDs(ctx, "id", repoIDs, &repo_model.Repository{}) + if err != nil { + return err + } + for _, pr := range prs { + repo, ok := repos[pr.HeadRepoID] + if !ok { + return fmt.Errorf("unable to find repo %d", pr.HeadRepoID) + } + pr.HeadRepo = repo + } + return nil +} + // GetIssueIDs returns all issue ids func (prs PullRequestList) GetIssueIDs() []int64 { return container.FilterSlice(prs, func(pr *PullRequest) (int64, bool) { diff --git a/services/pull/pull.go b/services/pull/pull.go index 9cb957d021..70013d326d 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -539,7 +539,7 @@ func CloseBranchPulls(ctx context.Context, doer *user_model.User, repoID int64, } prs = append(prs, prs2...) - if err := issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil { + if err := prs.LoadAttributes(ctx); err != nil { return err } @@ -569,7 +569,7 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re return err } - if err = issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil { + if err = prs.LoadAttributes(ctx); err != nil { return err } diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index 42f9e841e1..d013dfeb95 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -4,6 +4,7 @@ package integration import ( + "encoding/base64" "fmt" "net/http" "net/url" @@ -130,6 +131,107 @@ func TestAPIPullUpdateBranchProtection(t *testing.T) { }) } +func TestAPIPullAllowMaintainerEditRestrictedHead(t *testing.T) { + onApplicationRun(t, func(t *testing.T, giteaURL *url.URL) { + baseRepoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + + realBaseRepo, _, cleanup := tests.CreateDeclarativeRepo(t, baseRepoOwner, "base-repo", nil, nil, nil) + defer cleanup() + + forkUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + forkRepo, err := repo_service.ForkRepositoryAndUpdates(t.Context(), forkUser, forkUser, repo_service.ForkRepoOptions{ + BaseRepo: realBaseRepo, + Name: "repo-pr-update", + Description: "desc", + }) + require.NoError(t, err) + assert.NotNil(t, forkRepo) + + _, err = files_service.ChangeRepoFiles(git.DefaultContext, forkRepo, forkUser, &files_service.ChangeRepoFilesOptions{ + Files: []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: "File_B", + ContentReader: strings.NewReader("File B"), + }, + }, + Message: "Add File on PR branch", + OldBranch: "main", + NewBranch: "main", + Author: &files_service.IdentityOptions{ + Name: forkUser.Name, + Email: forkUser.Email, + }, + Committer: &files_service.IdentityOptions{ + Name: forkUser.Name, + Email: forkUser.Email, + }, + Dates: &files_service.CommitDateOptions{ + Author: time.Now(), + Committer: time.Now(), + }, + }) + require.NoError(t, err) + + // Create a pull request that is unexpectedly backwards -- normally we'd request a branch from the fork to be + // merged into the original base repo. But there's nothing preventing us from creating a pull request for the + // base to be pulled into the fork: + + pullIssue := &issues_model.Issue{ + RepoID: forkRepo.ID, + Title: "Pull Base into Fork", + PosterID: forkUser.ID, + Poster: forkUser, + IsPull: true, + } + pullRequest := &issues_model.PullRequest{ + HeadRepoID: realBaseRepo.ID, + BaseRepoID: forkRepo.ID, + HeadBranch: "main", + BaseBranch: "main", + HeadRepo: realBaseRepo, + BaseRepo: forkRepo, + Type: issues_model.PullRequestGitea, + AllowMaintainerEdit: true, + } + err = pull_service.NewPullRequest(git.DefaultContext, forkRepo, pullIssue, nil, nil, pullRequest, nil) + require.NoError(t, err) + + // forkUser is the owner of forkRepo, and therefore a maintainer of forkRepo. `AllowMaintainerEdit` should + // allow forkUser to edit the head of the PR... except that, in this case, the owner of the PR delegated that + // edit access but they never had that edit access. Try to modify the head repo & branch to see. + session := loginUser(t, forkUser.LoginName) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + + // Attempt modification by editing the realBase's main branch via API: + req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/contents/File_A", realBaseRepo.OwnerName, realBaseRepo.Name), + &api.CreateFileOptions{ + FileOptions: api.FileOptions{ + BranchName: "main", + NewBranchName: "main", + Message: "illegal change", + Author: api.Identity{ + Name: forkUser.FullName, + Email: forkUser.Email, + }, + Committer: api.Identity{ + Name: forkUser.FullName, + Email: forkUser.Email, + }, + }, + ContentBase64: base64.StdEncoding.EncodeToString([]byte("Some content.")), + }). + AddTokenAuth(token) + MakeRequest(t, req, http.StatusForbidden) + + // Modify by "updating" the PR, which would pull the base into the head, even though we don't have access to + // write the head: + req = NewRequestf(t, "POST", "/api/v1/repos/%s/%s/pulls/%d/update", forkRepo.OwnerName, forkRepo.Name, pullIssue.Index). + AddTokenAuth(token) + MakeRequest(t, req, http.StatusInternalServerError) // probably should be Forbidden + }) +} + func TestAPIViewUpdateSettings(t *testing.T) { onApplicationRun(t, func(t *testing.T, giteaURL *url.URL) { // Create PR to test