diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index aa9a06d70c..76201f3f6e 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -11,7 +11,7 @@ import ( issues_model "forgejo.org/models/issues" "forgejo.org/models/organization" - access_model "forgejo.org/models/perm/access" + "forgejo.org/models/perm" user_model "forgejo.org/models/user" "forgejo.org/modules/gitrepo" api "forgejo.org/modules/structs" @@ -746,6 +746,8 @@ func CreateReviewRequests(ctx *context.APIContext) { // "$ref": "#/responses/PullReviewListWithoutPagination" // "422": // "$ref": "#/responses/validationError" + // "403": + // "$ref": "#/responses/forbidden" // "404": // "$ref": "#/responses/notFound" @@ -813,9 +815,18 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions reviewers := make([]*user_model.User, 0, len(opts.Reviewers)) - permDoer, err := access_model.GetUserRepoPermission(ctx, pr.Issue.Repo, ctx.Doer) + // Ability to add review requests is relatively complex logic that will be in `CanDoerChangeReviewRequests`. But on + // the API side, it's possible that an auth reducer may want to interfere with this logic -- so we'll run a write + // AccessMode through the reducer and if we don't get write back, then the reducer is preventing us from adding this + // review request. (eg. a repo-scoped access token, when accessing a public repo, can't write review requests) + reducerAccessMode, err := ctx.Reducer.ReduceRepoAccess(ctx, ctx.Repo.Repository, perm.AccessModeWrite) if err != nil { - ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) + ctx.Error(http.StatusInternalServerError, "ReduceRepoAccess", err) + return + } else if reducerAccessMode != perm.AccessModeWrite { + // Forbidden (rather than NotFound) is used for this perm check because the middleware on the APIs already + // guarantees we have read access to this repo, so this doesn't leak any existence information. + ctx.Error(http.StatusForbidden, "apiReviewRequest", "permission reduction prevented review request change") return } @@ -836,7 +847,7 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions return } - err = issue_service.IsValidReviewRequest(ctx, reviewer, ctx.Doer, isAdd, pr.Issue, &permDoer) + err = issue_service.IsValidReviewRequest(ctx, reviewer, ctx.Doer, isAdd, pr.Issue) if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { ctx.Error(http.StatusUnprocessableEntity, "NotValidReviewRequest", err) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 1fef838516..10d3e6651b 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -2605,7 +2605,7 @@ func UpdatePullReviewRequest(ctx *context.Context) { return } - err = issue_service.IsValidReviewRequest(ctx, reviewer, ctx.Doer, action == "attach", issue, nil) + err = issue_service.IsValidReviewRequest(ctx, reviewer, ctx.Doer, action == "attach", issue) if err != nil { if issues_model.IsErrNotValidReviewRequest(err) { log.Warn( diff --git a/services/issue/assignee.go b/services/issue/assignee.go index 2d8700213e..5ec351b411 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -81,7 +81,7 @@ func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer, reviewe } // IsValidReviewRequest Check permission for ReviewRequest -func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, isAdd bool, issue *issues_model.Issue, permDoer *access_model.Permission) error { +func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, isAdd bool, issue *issues_model.Issue) error { if reviewer.IsOrganization() { return issues_model.ErrNotValidReviewRequest{ Reason: "Organization can't be added as reviewer", @@ -102,14 +102,6 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, return err } - if permDoer == nil { - permDoer = new(access_model.Permission) - *permDoer, err = access_model.GetUserRepoPermission(ctx, issue.Repo, doer) - if err != nil { - return err - } - } - lastreview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID) if err != nil && !issues_model.IsErrReviewNotExist(err) { return err diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 7e83814a2d..f155e91f3c 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -14576,6 +14576,9 @@ "201": { "$ref": "#/responses/PullReviewListWithoutPagination" }, + "403": { + "$ref": "#/responses/forbidden" + }, "404": { "$ref": "#/responses/notFound" }, diff --git a/tests/integration/api_pull_review_test.go b/tests/integration/api_pull_review_test.go index 9f00686b57..5ff8bd4adf 100644 --- a/tests/integration/api_pull_review_test.go +++ b/tests/integration/api_pull_review_test.go @@ -6,6 +6,7 @@ package integration import ( "fmt" "net/http" + "net/url" "testing" auth_model "forgejo.org/models/auth" @@ -486,6 +487,101 @@ func TestAPIPullReviewRequest(t *testing.T) { MakeRequest(t, req, http.StatusNoContent) } +func TestAPIPullReviewRequestAccessTokenResources(t *testing.T) { + onApplicationRun(t, func(t *testing.T, u *url.URL) { + session := loginUser(t, "user2") + writeToken := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue, auth_model.AccessTokenScopeWriteRepository) + + for _, repo := range []string{"user2/repo1", "user2/repo2", "org3/repo3"} { + // For our three target repos, we'll need to enable pull requests for this test case. + trueBool := true + req := NewRequestWithJSON(t, "PATCH", fmt.Sprintf("/api/v1/repos/%s", repo), &api.EditRepoOption{ + HasPullRequests: &trueBool, + }).AddTokenAuth(writeToken) + MakeRequest(t, req, http.StatusOK) + + // Add user `user5` as a collaborator on all the target repos as well, so that we can successfully request + // review from them. + write := "write" + req = NewRequestWithJSON(t, "PUT", fmt.Sprintf("/api/v1/repos/%s/collaborators/user5", repo), &api.AddCollaboratorOption{ + Permission: &write, + }).AddTokenAuth(writeToken) + MakeRequest(t, req, http.StatusNoContent) + } + + // Create a pull request on each of the target test repos. + var repo1PullRequest, repo2PullRequest, repo3PullRequest api.PullRequest + createPullRequest := func(repoFullname string, pullRequest *api.PullRequest) { + req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/contents", repoFullname), + &api.ChangeFilesOptions{ + FileOptions: api.FileOptions{ + NewBranchName: "prtest", + }, + Files: []*api.ChangeFileOperation{}, + }).AddTokenAuth(writeToken) + MakeRequest(t, req, http.StatusCreated) + + req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/pulls", repoFullname), &api.CreatePullRequestOption{ + Body: "repo1 issue dependency", + Title: "important dependency", + Base: "master", + Head: "prtest", + }).AddTokenAuth(writeToken) + resp := MakeRequest(t, req, http.StatusCreated) + DecodeJSON(t, resp, pullRequest) + } + createPullRequest("user2/repo1", &repo1PullRequest) + createPullRequest("user2/repo2", &repo2PullRequest) + createPullRequest("org3/repo3", &repo3PullRequest) + + // The core of the test is to see whether we can add pull request reviewers to the repos, using access tokens + // with different permission scopes (all, public-only, fine-grained access tokens). Define the test: + testCase := func(t *testing.T, repo string, pullRequest *api.PullRequest, token string, expectedStatus int) { + req := NewRequestWithJSON(t, + "POST", + fmt.Sprintf("/api/v1/repos/%s/pulls/%d/requested_reviewers", repo, pullRequest.Index), + &api.PullReviewRequestOptions{ + Reviewers: []string{"user5"}, + }). + AddTokenAuth(token) + MakeRequest(t, req, expectedStatus) + } + + t.Run("all access token", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + allToken := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + + testCase(t, "user2/repo1", &repo1PullRequest, allToken, http.StatusCreated) // public user2/repo1 + testCase(t, "user2/repo2", &repo2PullRequest, allToken, http.StatusCreated) // private user2/repo2 + testCase(t, "org3/repo3", &repo3PullRequest, allToken, http.StatusCreated) // private org3/repo3 + }) + + t.Run("public-only access token", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + publicOnlyToken := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopePublicOnly, auth_model.AccessTokenScopeWriteRepository) + + testCase(t, "user2/repo1", &repo1PullRequest, publicOnlyToken, http.StatusCreated) // public user2/repo1 + testCase(t, "user2/repo2", &repo2PullRequest, publicOnlyToken, http.StatusNotFound) // private user2/repo2 + testCase(t, "org3/repo3", &repo3PullRequest, publicOnlyToken, http.StatusNotFound) // private org3/repo3 + }) + + t.Run("specific repo access token", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + repo2OnlyToken := createFineGrainedRepoAccessToken(t, "user2", + []auth_model.AccessTokenScope{auth_model.AccessTokenScopeWriteRepository}, + []int64{3}, + ) + + testCase(t, "user2/repo1", &repo1PullRequest, repo2OnlyToken, http.StatusForbidden) // public user2/repo1, read-only outside of the auth'd repos + testCase(t, "user2/repo2", &repo2PullRequest, repo2OnlyToken, http.StatusNotFound) // private user2/repo2, outside of fine-grain + testCase(t, "org3/repo3", &repo3PullRequest, repo2OnlyToken, http.StatusCreated) // private org3/repo3 + }) + }) +} + func TestAPIPullReviewStayDismissed(t *testing.T) { // This test against issue https://github.com/go-gitea/gitea/issues/28542 // where old reviews surface after a review request got dismissed.