feat: implement fine-grained access tokens in /repos/{owner}/{repo}/pulls/{index}/requested_reviewers
This commit is contained in:
committed by
Mathieu Fenniak
parent
f9a2167105
commit
bbb7d52fc0
@@ -11,7 +11,7 @@ import (
|
|||||||
|
|
||||||
issues_model "forgejo.org/models/issues"
|
issues_model "forgejo.org/models/issues"
|
||||||
"forgejo.org/models/organization"
|
"forgejo.org/models/organization"
|
||||||
access_model "forgejo.org/models/perm/access"
|
"forgejo.org/models/perm"
|
||||||
user_model "forgejo.org/models/user"
|
user_model "forgejo.org/models/user"
|
||||||
"forgejo.org/modules/gitrepo"
|
"forgejo.org/modules/gitrepo"
|
||||||
api "forgejo.org/modules/structs"
|
api "forgejo.org/modules/structs"
|
||||||
@@ -746,6 +746,8 @@ func CreateReviewRequests(ctx *context.APIContext) {
|
|||||||
// "$ref": "#/responses/PullReviewListWithoutPagination"
|
// "$ref": "#/responses/PullReviewListWithoutPagination"
|
||||||
// "422":
|
// "422":
|
||||||
// "$ref": "#/responses/validationError"
|
// "$ref": "#/responses/validationError"
|
||||||
|
// "403":
|
||||||
|
// "$ref": "#/responses/forbidden"
|
||||||
// "404":
|
// "404":
|
||||||
// "$ref": "#/responses/notFound"
|
// "$ref": "#/responses/notFound"
|
||||||
|
|
||||||
@@ -813,9 +815,18 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions
|
|||||||
|
|
||||||
reviewers := make([]*user_model.User, 0, len(opts.Reviewers))
|
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 {
|
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
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -836,7 +847,7 @@ func apiReviewRequest(ctx *context.APIContext, opts api.PullReviewRequestOptions
|
|||||||
return
|
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 err != nil {
|
||||||
if issues_model.IsErrNotValidReviewRequest(err) {
|
if issues_model.IsErrNotValidReviewRequest(err) {
|
||||||
ctx.Error(http.StatusUnprocessableEntity, "NotValidReviewRequest", err)
|
ctx.Error(http.StatusUnprocessableEntity, "NotValidReviewRequest", err)
|
||||||
|
|||||||
@@ -2605,7 +2605,7 @@ func UpdatePullReviewRequest(ctx *context.Context) {
|
|||||||
return
|
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 err != nil {
|
||||||
if issues_model.IsErrNotValidReviewRequest(err) {
|
if issues_model.IsErrNotValidReviewRequest(err) {
|
||||||
log.Warn(
|
log.Warn(
|
||||||
|
|||||||
@@ -81,7 +81,7 @@ func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer, reviewe
|
|||||||
}
|
}
|
||||||
|
|
||||||
// IsValidReviewRequest Check permission for ReviewRequest
|
// 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() {
|
if reviewer.IsOrganization() {
|
||||||
return issues_model.ErrNotValidReviewRequest{
|
return issues_model.ErrNotValidReviewRequest{
|
||||||
Reason: "Organization can't be added as reviewer",
|
Reason: "Organization can't be added as reviewer",
|
||||||
@@ -102,14 +102,6 @@ func IsValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User,
|
|||||||
return err
|
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)
|
lastreview, err := issues_model.GetReviewByIssueIDAndUserID(ctx, issue.ID, reviewer.ID)
|
||||||
if err != nil && !issues_model.IsErrReviewNotExist(err) {
|
if err != nil && !issues_model.IsErrReviewNotExist(err) {
|
||||||
return err
|
return err
|
||||||
|
|||||||
Generated
+3
@@ -14576,6 +14576,9 @@
|
|||||||
"201": {
|
"201": {
|
||||||
"$ref": "#/responses/PullReviewListWithoutPagination"
|
"$ref": "#/responses/PullReviewListWithoutPagination"
|
||||||
},
|
},
|
||||||
|
"403": {
|
||||||
|
"$ref": "#/responses/forbidden"
|
||||||
|
},
|
||||||
"404": {
|
"404": {
|
||||||
"$ref": "#/responses/notFound"
|
"$ref": "#/responses/notFound"
|
||||||
},
|
},
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ package integration
|
|||||||
import (
|
import (
|
||||||
"fmt"
|
"fmt"
|
||||||
"net/http"
|
"net/http"
|
||||||
|
"net/url"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
auth_model "forgejo.org/models/auth"
|
auth_model "forgejo.org/models/auth"
|
||||||
@@ -486,6 +487,101 @@ func TestAPIPullReviewRequest(t *testing.T) {
|
|||||||
MakeRequest(t, req, http.StatusNoContent)
|
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) {
|
func TestAPIPullReviewStayDismissed(t *testing.T) {
|
||||||
// This test against issue https://github.com/go-gitea/gitea/issues/28542
|
// This test against issue https://github.com/go-gitea/gitea/issues/28542
|
||||||
// where old reviews surface after a review request got dismissed.
|
// where old reviews surface after a review request got dismissed.
|
||||||
|
|||||||
Reference in New Issue
Block a user