From 7b1153e943b62b20b102210b6a41ccf4854b374f Mon Sep 17 00:00:00 2001 From: pricly-yellow <79628427+pricly-yellow@users.noreply.github.com> Date: Thu, 7 Oct 2021 16:39:23 +0700 Subject: [PATCH] API pull's head/base have correct permission(#17214) (#17245) * for all pull requests API return permissions of caller * for all webhook return empty permissions Signed-off-by: Danila Kryukov * Fix incorrect error handler Co-authored-by: delvh * Fix wrong assumption in tests * Change paramenter name to doer to indicate source Co-authored-by: 6543 <6543@obermui.de> Co-authored-by: delvh Co-authored-by: 6543 <6543@obermui.de> --- modules/convert/pull.go | 18 +++++++++++++++--- modules/convert/pull_test.go | 6 +++--- modules/notification/webhook/webhook.go | 24 ++++++++++++------------ routers/api/v1/repo/pull.go | 8 ++++---- 4 files changed, 34 insertions(+), 22 deletions(-) diff --git a/modules/convert/pull.go b/modules/convert/pull.go index 8bdf17a04..ad70eb5ca 100644 --- a/modules/convert/pull.go +++ b/modules/convert/pull.go @@ -17,7 +17,7 @@ import ( // ToAPIPullRequest assumes following fields have been assigned with valid values: // Required - Issue // Optional - Merger -func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest { +func ToAPIPullRequest(pr *models.PullRequest, doer *models.User) *api.PullRequest { var ( baseBranch *git.Branch headBranch *git.Branch @@ -41,6 +41,12 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest { return nil } + perm, err := models.GetUserRepoPermission(pr.BaseRepo, doer) + if err != nil { + log.Error("GetUserRepoPermission[%d]: %v", pr.BaseRepoID, err) + perm.AccessMode = models.AccessModeNone + } + apiPullRequest := &api.PullRequest{ ID: pr.ID, URL: pr.Issue.HTMLURL(), @@ -68,7 +74,7 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest { Name: pr.BaseBranch, Ref: pr.BaseBranch, RepoID: pr.BaseRepoID, - Repository: ToRepo(pr.BaseRepo, models.AccessModeNone), + Repository: ToRepo(pr.BaseRepo, perm.AccessMode), }, Head: &api.PRBranchInfo{ Name: pr.HeadBranch, @@ -96,8 +102,14 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest { } if pr.HeadRepo != nil { + perm, err := models.GetUserRepoPermission(pr.HeadRepo, doer) + if err != nil { + log.Error("GetUserRepoPermission[%d]: %v", pr.HeadRepoID, err) + perm.AccessMode = models.AccessModeNone + } + apiPullRequest.Head.RepoID = pr.HeadRepo.ID - apiPullRequest.Head.Repository = ToRepo(pr.HeadRepo, models.AccessModeNone) + apiPullRequest.Head.Repository = ToRepo(pr.HeadRepo, perm.AccessMode) headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath()) if err != nil { diff --git a/modules/convert/pull_test.go b/modules/convert/pull_test.go index adf42d8ca..a88135780 100644 --- a/modules/convert/pull_test.go +++ b/modules/convert/pull_test.go @@ -20,14 +20,14 @@ func TestPullRequest_APIFormat(t *testing.T) { pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest) assert.NoError(t, pr.LoadAttributes()) assert.NoError(t, pr.LoadIssue()) - apiPullRequest := ToAPIPullRequest(pr) + apiPullRequest := ToAPIPullRequest(pr, nil) assert.NotNil(t, apiPullRequest) assert.EqualValues(t, &structs.PRBranchInfo{ Name: "branch1", Ref: "refs/pull/2/head", Sha: "4a357436d925b5c974181ff12a994538ddc5a269", RepoID: 1, - Repository: ToRepo(headRepo, models.AccessModeNone), + Repository: ToRepo(headRepo, models.AccessModeRead), }, apiPullRequest.Head) //withOut HeadRepo @@ -37,7 +37,7 @@ func TestPullRequest_APIFormat(t *testing.T) { // simulate fork deletion pr.HeadRepo = nil pr.HeadRepoID = 100000 - apiPullRequest = ToAPIPullRequest(pr) + apiPullRequest = ToAPIPullRequest(pr, nil) assert.NotNil(t, apiPullRequest) assert.Nil(t, apiPullRequest.Head.Repository) assert.EqualValues(t, -1, apiPullRequest.Head.RepoID) diff --git a/modules/notification/webhook/webhook.go b/modules/notification/webhook/webhook.go index acdb91efe..c767acf13 100644 --- a/modules/notification/webhook/webhook.go +++ b/modules/notification/webhook/webhook.go @@ -51,7 +51,7 @@ func (m *webhookNotifier) NotifyIssueClearLabels(doer *models.User, issue *model err = webhook_services.PrepareWebhooks(issue.Repo, models.HookEventPullRequestLabel, &api.PullRequestPayload{ Action: api.HookIssueLabelCleared, Index: issue.Index, - PullRequest: convert.ToAPIPullRequest(issue.PullRequest), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil), Repository: convert.ToRepo(issue.Repo, mode), Sender: convert.ToUser(doer, nil), }) @@ -145,7 +145,7 @@ func (m *webhookNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *mo issue.PullRequest.Issue = issue apiPullRequest := &api.PullRequestPayload{ Index: issue.Index, - PullRequest: convert.ToAPIPullRequest(issue.PullRequest), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil), Repository: convert.ToRepo(issue.Repo, mode), Sender: convert.ToUser(doer, nil), } @@ -197,7 +197,7 @@ func (m *webhookNotifier) NotifyIssueChangeTitle(doer *models.User, issue *model From: oldTitle, }, }, - PullRequest: convert.ToAPIPullRequest(issue.PullRequest), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil), Repository: convert.ToRepo(issue.Repo, mode), Sender: convert.ToUser(doer, nil), }) @@ -232,7 +232,7 @@ func (m *webhookNotifier) NotifyIssueChangeStatus(doer *models.User, issue *mode // Merge pull request calls issue.changeStatus so we need to handle separately. apiPullRequest := &api.PullRequestPayload{ Index: issue.Index, - PullRequest: convert.ToAPIPullRequest(issue.PullRequest), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil), Repository: convert.ToRepo(issue.Repo, mode), Sender: convert.ToUser(doer, nil), } @@ -301,7 +301,7 @@ func (m *webhookNotifier) NotifyNewPullRequest(pull *models.PullRequest, mention if err := webhook_services.PrepareWebhooks(pull.Issue.Repo, models.HookEventPullRequest, &api.PullRequestPayload{ Action: api.HookIssueOpened, Index: pull.Issue.Index, - PullRequest: convert.ToAPIPullRequest(pull), + PullRequest: convert.ToAPIPullRequest(pull, nil), Repository: convert.ToRepo(pull.Issue.Repo, mode), Sender: convert.ToUser(pull.Issue.Poster, nil), }); err != nil { @@ -322,7 +322,7 @@ func (m *webhookNotifier) NotifyIssueChangeContent(doer *models.User, issue *mod From: oldContent, }, }, - PullRequest: convert.ToAPIPullRequest(issue.PullRequest), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil), Repository: convert.ToRepo(issue.Repo, mode), Sender: convert.ToUser(doer, nil), }) @@ -500,7 +500,7 @@ func (m *webhookNotifier) NotifyIssueChangeLabels(doer *models.User, issue *mode err = webhook_services.PrepareWebhooks(issue.Repo, models.HookEventPullRequestLabel, &api.PullRequestPayload{ Action: api.HookIssueLabelUpdated, Index: issue.Index, - PullRequest: convert.ToAPIPullRequest(issue.PullRequest), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil), Repository: convert.ToRepo(issue.Repo, models.AccessModeNone), Sender: convert.ToUser(doer, nil), }) @@ -542,7 +542,7 @@ func (m *webhookNotifier) NotifyIssueChangeMilestone(doer *models.User, issue *m err = webhook_services.PrepareWebhooks(issue.Repo, models.HookEventPullRequestMilestone, &api.PullRequestPayload{ Action: hookAction, Index: issue.Index, - PullRequest: convert.ToAPIPullRequest(issue.PullRequest), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil), Repository: convert.ToRepo(issue.Repo, mode), Sender: convert.ToUser(doer, nil), }) @@ -609,7 +609,7 @@ func (*webhookNotifier) NotifyMergePullRequest(pr *models.PullRequest, doer *mod // Merge pull request calls issue.changeStatus so we need to handle separately. apiPullRequest := &api.PullRequestPayload{ Index: pr.Issue.Index, - PullRequest: convert.ToAPIPullRequest(pr), + PullRequest: convert.ToAPIPullRequest(pr, nil), Repository: convert.ToRepo(pr.Issue.Repo, mode), Sender: convert.ToUser(doer, nil), Action: api.HookIssueClosed, @@ -642,7 +642,7 @@ func (m *webhookNotifier) NotifyPullRequestChangeTargetBranch(doer *models.User, From: oldBranch, }, }, - PullRequest: convert.ToAPIPullRequest(issue.PullRequest), + PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil), Repository: convert.ToRepo(issue.Repo, mode), Sender: convert.ToUser(doer, nil), }) @@ -681,7 +681,7 @@ func (m *webhookNotifier) NotifyPullRequestReview(pr *models.PullRequest, review if err := webhook_services.PrepareWebhooks(review.Issue.Repo, reviewHookType, &api.PullRequestPayload{ Action: api.HookIssueReviewed, Index: review.Issue.Index, - PullRequest: convert.ToAPIPullRequest(pr), + PullRequest: convert.ToAPIPullRequest(pr, nil), Repository: convert.ToRepo(review.Issue.Repo, mode), Sender: convert.ToUser(review.Reviewer, nil), Review: &api.ReviewPayload{ @@ -736,7 +736,7 @@ func (m *webhookNotifier) NotifyPullRequestSynchronized(doer *models.User, pr *m if err := webhook_services.PrepareWebhooks(pr.Issue.Repo, models.HookEventPullRequestSync, &api.PullRequestPayload{ Action: api.HookIssueSynchronized, Index: pr.Issue.Index, - PullRequest: convert.ToAPIPullRequest(pr), + PullRequest: convert.ToAPIPullRequest(pr, nil), Repository: convert.ToRepo(pr.Issue.Repo, models.AccessModeNone), Sender: convert.ToUser(doer, nil), }); err != nil { diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index cb0c15a05..2e9b2c691 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -115,7 +115,7 @@ func ListPullRequests(ctx *context.APIContext) { ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err) return } - apiPrs[i] = convert.ToAPIPullRequest(prs[i]) + apiPrs[i] = convert.ToAPIPullRequest(prs[i], ctx.User) } ctx.SetLinkHeader(int(maxResults), listOptions.PageSize) @@ -172,7 +172,7 @@ func GetPullRequest(ctx *context.APIContext) { ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err) return } - ctx.JSON(http.StatusOK, convert.ToAPIPullRequest(pr)) + ctx.JSON(http.StatusOK, convert.ToAPIPullRequest(pr, ctx.User)) } // DownloadPullDiff render a pull's raw diff @@ -437,7 +437,7 @@ func CreatePullRequest(ctx *context.APIContext) { } log.Trace("Pull request created: %d/%d", repo.ID, prIssue.ID) - ctx.JSON(http.StatusCreated, convert.ToAPIPullRequest(pr)) + ctx.JSON(http.StatusCreated, convert.ToAPIPullRequest(pr, ctx.User)) } // EditPullRequest does what it says @@ -640,7 +640,7 @@ func EditPullRequest(ctx *context.APIContext) { } // TODO this should be 200, not 201 - ctx.JSON(http.StatusCreated, convert.ToAPIPullRequest(pr)) + ctx.JSON(http.StatusCreated, convert.ToAPIPullRequest(pr, ctx.User)) } // IsPullRequestMerged checks if a PR exists given an index