Skip to content

API pull's head/base have correct permission(#17214) #17245

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 7, 2021
Merged

API pull's head/base have correct permission(#17214) #17245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c4c7cae
API pull's head/base have correct permission
pricly-yellow Oct 4, 2021
6a0bf94
Fix incorrect error handler
pricly-yellow Oct 4, 2021
a6994b8
Fix wrong assumption in tests
pricly-yellow Oct 5, 2021
9253ef9
Change paramenter name to doer to indicate source
pricly-yellow Oct 6, 2021
3566e36
Merge branch 'release/v1.15' into api_pull_incorrect
6543 Oct 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions modules/convert/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions modules/convert/pull_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down
24 changes: 12 additions & 12 deletions modules/notification/webhook/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
})
Expand Down Expand Up @@ -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),
}
Expand Down Expand Up @@ -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),
})
Expand Down Expand Up @@ -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),
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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),
})
Expand Down Expand Up @@ -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),
})
Expand Down Expand Up @@ -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),
})
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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),
})
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 4 additions & 4 deletions routers/api/v1/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down