From 4afdb1eb78ce77fad8e1d2952c69b3315a836548 Mon Sep 17 00:00:00 2001
From: pricly-yellow <79628427+pricly-yellow@users.noreply.github.com>
Date: Thu, 7 Oct 2021 07:03:37 +0700
Subject: [PATCH] API pull's head/base have correct permission (#17214)
close #17181
* for all pull requests API return permissions of caller
* for all webhook return empty permissions
Signed-off-by: Danila Kryukov <pricly_yellow@dismail.de>
Co-authored-by: delvh <dev.lh@web.de>
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 6c5d15c82e..ab17c13421 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,
@@ -114,8 +120,14 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest {
}
if pr.HeadRepo != nil && pr.Flow == models.PullRequestFlowGithub {
+ 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 655fc76329..a2b1e12a37 100644
--- a/modules/convert/pull_test.go
+++ b/modules/convert/pull_test.go
@@ -21,14 +21,14 @@ func TestPullRequest_APIFormat(t *testing.T) {
pr := db.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
@@ -38,7 +38,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 acdb91efe3..c767acf134 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 6718224d7d..d8e0d8099a 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)
@@ -171,7 +171,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))
}
// DownloadPullDiffOrPatch render a pull's raw diff or patch
@@ -417,7 +417,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
@@ -624,7 +624,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