Skip to content

Commit 4afdb1e

Browse files
pricly-yellowdelvh6543
authored
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 <[email protected]> Co-authored-by: delvh <[email protected]> Co-authored-by: 6543 <[email protected]>
1 parent 67bc04f commit 4afdb1e

File tree

4 files changed

+34
-22
lines changed

4 files changed

+34
-22
lines changed

‎modules/convert/pull.go‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import (
1717
// ToAPIPullRequest assumes following fields have been assigned with valid values:
1818
// Required - Issue
1919
// Optional - Merger
20-
func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest {
20+
func ToAPIPullRequest(pr *models.PullRequest, doer *models.User) *api.PullRequest {
2121
var (
2222
baseBranch *git.Branch
2323
headBranch *git.Branch
@@ -41,6 +41,12 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest {
4141
return nil
4242
}
4343

44+
perm, err := models.GetUserRepoPermission(pr.BaseRepo, doer)
45+
if err != nil {
46+
log.Error("GetUserRepoPermission[%d]: %v", pr.BaseRepoID, err)
47+
perm.AccessMode = models.AccessModeNone
48+
}
49+
4450
apiPullRequest := &api.PullRequest{
4551
ID: pr.ID,
4652
URL: pr.Issue.HTMLURL(),
@@ -68,7 +74,7 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest {
6874
Name: pr.BaseBranch,
6975
Ref: pr.BaseBranch,
7076
RepoID: pr.BaseRepoID,
71-
Repository: ToRepo(pr.BaseRepo, models.AccessModeNone),
77+
Repository: ToRepo(pr.BaseRepo, perm.AccessMode),
7278
},
7379
Head: &api.PRBranchInfo{
7480
Name: pr.HeadBranch,
@@ -114,8 +120,14 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest {
114120
}
115121

116122
if pr.HeadRepo != nil && pr.Flow == models.PullRequestFlowGithub {
123+
perm, err := models.GetUserRepoPermission(pr.HeadRepo, doer)
124+
if err != nil {
125+
log.Error("GetUserRepoPermission[%d]: %v", pr.HeadRepoID, err)
126+
perm.AccessMode = models.AccessModeNone
127+
}
128+
117129
apiPullRequest.Head.RepoID = pr.HeadRepo.ID
118-
apiPullRequest.Head.Repository = ToRepo(pr.HeadRepo, models.AccessModeNone)
130+
apiPullRequest.Head.Repository = ToRepo(pr.HeadRepo, perm.AccessMode)
119131

120132
headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath())
121133
if err != nil {

‎modules/convert/pull_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,14 @@ func TestPullRequest_APIFormat(t *testing.T) {
2121
pr := db.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest)
2222
assert.NoError(t, pr.LoadAttributes())
2323
assert.NoError(t, pr.LoadIssue())
24-
apiPullRequest := ToAPIPullRequest(pr)
24+
apiPullRequest := ToAPIPullRequest(pr, nil)
2525
assert.NotNil(t, apiPullRequest)
2626
assert.EqualValues(t, &structs.PRBranchInfo{
2727
Name: "branch1",
2828
Ref: "refs/pull/2/head",
2929
Sha: "4a357436d925b5c974181ff12a994538ddc5a269",
3030
RepoID: 1,
31-
Repository: ToRepo(headRepo, models.AccessModeNone),
31+
Repository: ToRepo(headRepo, models.AccessModeRead),
3232
}, apiPullRequest.Head)
3333

3434
//withOut HeadRepo
@@ -38,7 +38,7 @@ func TestPullRequest_APIFormat(t *testing.T) {
3838
// simulate fork deletion
3939
pr.HeadRepo = nil
4040
pr.HeadRepoID = 100000
41-
apiPullRequest = ToAPIPullRequest(pr)
41+
apiPullRequest = ToAPIPullRequest(pr, nil)
4242
assert.NotNil(t, apiPullRequest)
4343
assert.Nil(t, apiPullRequest.Head.Repository)
4444
assert.EqualValues(t, -1, apiPullRequest.Head.RepoID)

‎modules/notification/webhook/webhook.go‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func (m *webhookNotifier) NotifyIssueClearLabels(doer *models.User, issue *model
5151
err = webhook_services.PrepareWebhooks(issue.Repo, models.HookEventPullRequestLabel, &api.PullRequestPayload{
5252
Action: api.HookIssueLabelCleared,
5353
Index: issue.Index,
54-
PullRequest: convert.ToAPIPullRequest(issue.PullRequest),
54+
PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil),
5555
Repository: convert.ToRepo(issue.Repo, mode),
5656
Sender: convert.ToUser(doer, nil),
5757
})
@@ -145,7 +145,7 @@ func (m *webhookNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *mo
145145
issue.PullRequest.Issue = issue
146146
apiPullRequest := &api.PullRequestPayload{
147147
Index: issue.Index,
148-
PullRequest: convert.ToAPIPullRequest(issue.PullRequest),
148+
PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil),
149149
Repository: convert.ToRepo(issue.Repo, mode),
150150
Sender: convert.ToUser(doer, nil),
151151
}
@@ -197,7 +197,7 @@ func (m *webhookNotifier) NotifyIssueChangeTitle(doer *models.User, issue *model
197197
From: oldTitle,
198198
},
199199
},
200-
PullRequest: convert.ToAPIPullRequest(issue.PullRequest),
200+
PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil),
201201
Repository: convert.ToRepo(issue.Repo, mode),
202202
Sender: convert.ToUser(doer, nil),
203203
})
@@ -232,7 +232,7 @@ func (m *webhookNotifier) NotifyIssueChangeStatus(doer *models.User, issue *mode
232232
// Merge pull request calls issue.changeStatus so we need to handle separately.
233233
apiPullRequest := &api.PullRequestPayload{
234234
Index: issue.Index,
235-
PullRequest: convert.ToAPIPullRequest(issue.PullRequest),
235+
PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil),
236236
Repository: convert.ToRepo(issue.Repo, mode),
237237
Sender: convert.ToUser(doer, nil),
238238
}
@@ -301,7 +301,7 @@ func (m *webhookNotifier) NotifyNewPullRequest(pull *models.PullRequest, mention
301301
if err := webhook_services.PrepareWebhooks(pull.Issue.Repo, models.HookEventPullRequest, &api.PullRequestPayload{
302302
Action: api.HookIssueOpened,
303303
Index: pull.Issue.Index,
304-
PullRequest: convert.ToAPIPullRequest(pull),
304+
PullRequest: convert.ToAPIPullRequest(pull, nil),
305305
Repository: convert.ToRepo(pull.Issue.Repo, mode),
306306
Sender: convert.ToUser(pull.Issue.Poster, nil),
307307
}); err != nil {
@@ -322,7 +322,7 @@ func (m *webhookNotifier) NotifyIssueChangeContent(doer *models.User, issue *mod
322322
From: oldContent,
323323
},
324324
},
325-
PullRequest: convert.ToAPIPullRequest(issue.PullRequest),
325+
PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil),
326326
Repository: convert.ToRepo(issue.Repo, mode),
327327
Sender: convert.ToUser(doer, nil),
328328
})
@@ -500,7 +500,7 @@ func (m *webhookNotifier) NotifyIssueChangeLabels(doer *models.User, issue *mode
500500
err = webhook_services.PrepareWebhooks(issue.Repo, models.HookEventPullRequestLabel, &api.PullRequestPayload{
501501
Action: api.HookIssueLabelUpdated,
502502
Index: issue.Index,
503-
PullRequest: convert.ToAPIPullRequest(issue.PullRequest),
503+
PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil),
504504
Repository: convert.ToRepo(issue.Repo, models.AccessModeNone),
505505
Sender: convert.ToUser(doer, nil),
506506
})
@@ -542,7 +542,7 @@ func (m *webhookNotifier) NotifyIssueChangeMilestone(doer *models.User, issue *m
542542
err = webhook_services.PrepareWebhooks(issue.Repo, models.HookEventPullRequestMilestone, &api.PullRequestPayload{
543543
Action: hookAction,
544544
Index: issue.Index,
545-
PullRequest: convert.ToAPIPullRequest(issue.PullRequest),
545+
PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil),
546546
Repository: convert.ToRepo(issue.Repo, mode),
547547
Sender: convert.ToUser(doer, nil),
548548
})
@@ -609,7 +609,7 @@ func (*webhookNotifier) NotifyMergePullRequest(pr *models.PullRequest, doer *mod
609609
// Merge pull request calls issue.changeStatus so we need to handle separately.
610610
apiPullRequest := &api.PullRequestPayload{
611611
Index: pr.Issue.Index,
612-
PullRequest: convert.ToAPIPullRequest(pr),
612+
PullRequest: convert.ToAPIPullRequest(pr, nil),
613613
Repository: convert.ToRepo(pr.Issue.Repo, mode),
614614
Sender: convert.ToUser(doer, nil),
615615
Action: api.HookIssueClosed,
@@ -642,7 +642,7 @@ func (m *webhookNotifier) NotifyPullRequestChangeTargetBranch(doer *models.User,
642642
From: oldBranch,
643643
},
644644
},
645-
PullRequest: convert.ToAPIPullRequest(issue.PullRequest),
645+
PullRequest: convert.ToAPIPullRequest(issue.PullRequest, nil),
646646
Repository: convert.ToRepo(issue.Repo, mode),
647647
Sender: convert.ToUser(doer, nil),
648648
})
@@ -681,7 +681,7 @@ func (m *webhookNotifier) NotifyPullRequestReview(pr *models.PullRequest, review
681681
if err := webhook_services.PrepareWebhooks(review.Issue.Repo, reviewHookType, &api.PullRequestPayload{
682682
Action: api.HookIssueReviewed,
683683
Index: review.Issue.Index,
684-
PullRequest: convert.ToAPIPullRequest(pr),
684+
PullRequest: convert.ToAPIPullRequest(pr, nil),
685685
Repository: convert.ToRepo(review.Issue.Repo, mode),
686686
Sender: convert.ToUser(review.Reviewer, nil),
687687
Review: &api.ReviewPayload{
@@ -736,7 +736,7 @@ func (m *webhookNotifier) NotifyPullRequestSynchronized(doer *models.User, pr *m
736736
if err := webhook_services.PrepareWebhooks(pr.Issue.Repo, models.HookEventPullRequestSync, &api.PullRequestPayload{
737737
Action: api.HookIssueSynchronized,
738738
Index: pr.Issue.Index,
739-
PullRequest: convert.ToAPIPullRequest(pr),
739+
PullRequest: convert.ToAPIPullRequest(pr, nil),
740740
Repository: convert.ToRepo(pr.Issue.Repo, models.AccessModeNone),
741741
Sender: convert.ToUser(doer, nil),
742742
}); err != nil {

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ func ListPullRequests(ctx *context.APIContext) {
115115
ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err)
116116
return
117117
}
118-
apiPrs[i] = convert.ToAPIPullRequest(prs[i])
118+
apiPrs[i] = convert.ToAPIPullRequest(prs[i], ctx.User)
119119
}
120120

121121
ctx.SetLinkHeader(int(maxResults), listOptions.PageSize)
@@ -171,7 +171,7 @@ func GetPullRequest(ctx *context.APIContext) {
171171
ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err)
172172
return
173173
}
174-
ctx.JSON(http.StatusOK, convert.ToAPIPullRequest(pr))
174+
ctx.JSON(http.StatusOK, convert.ToAPIPullRequest(pr, ctx.User))
175175
}
176176

177177
// DownloadPullDiffOrPatch render a pull's raw diff or patch
@@ -417,7 +417,7 @@ func CreatePullRequest(ctx *context.APIContext) {
417417
}
418418

419419
log.Trace("Pull request created: %d/%d", repo.ID, prIssue.ID)
420-
ctx.JSON(http.StatusCreated, convert.ToAPIPullRequest(pr))
420+
ctx.JSON(http.StatusCreated, convert.ToAPIPullRequest(pr, ctx.User))
421421
}
422422

423423
// EditPullRequest does what it says
@@ -624,7 +624,7 @@ func EditPullRequest(ctx *context.APIContext) {
624624
}
625625

626626
// TODO this should be 200, not 201
627-
ctx.JSON(http.StatusCreated, convert.ToAPIPullRequest(pr))
627+
ctx.JSON(http.StatusCreated, convert.ToAPIPullRequest(pr, ctx.User))
628628
}
629629

630630
// IsPullRequestMerged checks if a PR exists given an index

0 commit comments

Comments
 (0)