mirror of
https://github.com/go-gitea/gitea
synced 2024-08-26 17:23:57 +02:00
Add permission check when creating PR (#31033)
user should be a collaborator of the base repo to create a PR
This commit is contained in:
parent
d109923ed8
commit
e0a408e6f3
@ -27,6 +27,8 @@ import (
|
|||||||
"xorm.io/builder"
|
"xorm.io/builder"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
var ErrMustCollaborator = util.NewPermissionDeniedErrorf("user must be a collaborator")
|
||||||
|
|
||||||
// ErrPullRequestNotExist represents a "PullRequestNotExist" kind of error.
|
// ErrPullRequestNotExist represents a "PullRequestNotExist" kind of error.
|
||||||
type ErrPullRequestNotExist struct {
|
type ErrPullRequestNotExist struct {
|
||||||
ID int64
|
ID int64
|
||||||
@ -572,6 +574,12 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *Iss
|
|||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ErrUserMustCollaborator represents an error that the user must be a collaborator to a given repo.
|
||||||
|
type ErrUserMustCollaborator struct {
|
||||||
|
UserID int64
|
||||||
|
RepoName string
|
||||||
|
}
|
||||||
|
|
||||||
// GetUnmergedPullRequest returns a pull request that is open and has not been merged
|
// GetUnmergedPullRequest returns a pull request that is open and has not been merged
|
||||||
// by given head/base and repo/branch.
|
// by given head/base and repo/branch.
|
||||||
func GetUnmergedPullRequest(ctx context.Context, headRepoID, baseRepoID int64, headBranch, baseBranch string, flow PullRequestFlow) (*PullRequest, error) {
|
func GetUnmergedPullRequest(ctx context.Context, headRepoID, baseRepoID int64, headBranch, baseBranch string, flow PullRequestFlow) (*PullRequest, error) {
|
||||||
|
@ -1765,6 +1765,7 @@ compare.compare_head = compare
|
|||||||
pulls.desc = Enable pull requests and code reviews.
|
pulls.desc = Enable pull requests and code reviews.
|
||||||
pulls.new = New Pull Request
|
pulls.new = New Pull Request
|
||||||
pulls.new.blocked_user = Cannot create pull request because you are blocked by the repository owner.
|
pulls.new.blocked_user = Cannot create pull request because you are blocked by the repository owner.
|
||||||
|
pulls.new.must_collaborator = You must be a collaborator to create pull request.
|
||||||
pulls.edit.already_changed = Unable to save changes to the pull request. It appears the content has already been changed by another user. Please refresh the page and try editing again to avoid overwriting their changes
|
pulls.edit.already_changed = Unable to save changes to the pull request. It appears the content has already been changed by another user. Please refresh the page and try editing again to avoid overwriting their changes
|
||||||
pulls.view = View Pull Request
|
pulls.view = View Pull Request
|
||||||
pulls.compare_changes = New Pull Request
|
pulls.compare_changes = New Pull Request
|
||||||
|
@ -535,6 +535,8 @@ func CreatePullRequest(ctx *context.APIContext) {
|
|||||||
ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err)
|
ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err)
|
||||||
} else if errors.Is(err, user_model.ErrBlockedUser) {
|
} else if errors.Is(err, user_model.ErrBlockedUser) {
|
||||||
ctx.Error(http.StatusForbidden, "BlockedUser", err)
|
ctx.Error(http.StatusForbidden, "BlockedUser", err)
|
||||||
|
} else if errors.Is(err, issues_model.ErrMustCollaborator) {
|
||||||
|
ctx.Error(http.StatusForbidden, "MustCollaborator", err)
|
||||||
} else {
|
} else {
|
||||||
ctx.Error(http.StatusInternalServerError, "NewPullRequest", err)
|
ctx.Error(http.StatusInternalServerError, "NewPullRequest", err)
|
||||||
}
|
}
|
||||||
|
@ -1337,6 +1337,16 @@ func CompareAndPullRequestPost(ctx *context.Context) {
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
ctx.JSONError(flashError)
|
ctx.JSONError(flashError)
|
||||||
|
} else if errors.Is(err, issues_model.ErrMustCollaborator) {
|
||||||
|
flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
|
||||||
|
"Message": ctx.Tr("repo.pulls.push_rejected"),
|
||||||
|
"Summary": ctx.Tr("repo.pulls.new.must_collaborator"),
|
||||||
|
})
|
||||||
|
if err != nil {
|
||||||
|
ctx.ServerError("CompareAndPullRequest.HTMLString", err)
|
||||||
|
return
|
||||||
|
}
|
||||||
|
ctx.JSONError(flashError)
|
||||||
}
|
}
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
@ -17,7 +17,9 @@ import (
|
|||||||
"code.gitea.io/gitea/models/db"
|
"code.gitea.io/gitea/models/db"
|
||||||
git_model "code.gitea.io/gitea/models/git"
|
git_model "code.gitea.io/gitea/models/git"
|
||||||
issues_model "code.gitea.io/gitea/models/issues"
|
issues_model "code.gitea.io/gitea/models/issues"
|
||||||
|
access_model "code.gitea.io/gitea/models/perm/access"
|
||||||
repo_model "code.gitea.io/gitea/models/repo"
|
repo_model "code.gitea.io/gitea/models/repo"
|
||||||
|
"code.gitea.io/gitea/models/unit"
|
||||||
user_model "code.gitea.io/gitea/models/user"
|
user_model "code.gitea.io/gitea/models/user"
|
||||||
"code.gitea.io/gitea/modules/base"
|
"code.gitea.io/gitea/modules/base"
|
||||||
"code.gitea.io/gitea/modules/container"
|
"code.gitea.io/gitea/modules/container"
|
||||||
@ -48,6 +50,28 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss
|
|||||||
return user_model.ErrBlockedUser
|
return user_model.ErrBlockedUser
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// user should be a collaborator or a member of the organization for base repo
|
||||||
|
if !issue.Poster.IsAdmin {
|
||||||
|
canCreate, err := repo_model.IsOwnerMemberCollaborator(ctx, repo, issue.Poster.ID)
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
|
||||||
|
if !canCreate {
|
||||||
|
// or user should have write permission in the head repo
|
||||||
|
if err := pr.LoadHeadRepo(ctx); err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, issue.Poster)
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
if !perm.CanWrite(unit.TypeCode) {
|
||||||
|
return issues_model.ErrMustCollaborator
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
|
prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
if !git_model.IsErrBranchNotExist(err) {
|
if !git_model.IsErrBranchNotExist(err) {
|
||||||
|
@ -11,9 +11,11 @@ import (
|
|||||||
"time"
|
"time"
|
||||||
|
|
||||||
actions_model "code.gitea.io/gitea/models/actions"
|
actions_model "code.gitea.io/gitea/models/actions"
|
||||||
|
auth_model "code.gitea.io/gitea/models/auth"
|
||||||
"code.gitea.io/gitea/models/db"
|
"code.gitea.io/gitea/models/db"
|
||||||
git_model "code.gitea.io/gitea/models/git"
|
git_model "code.gitea.io/gitea/models/git"
|
||||||
issues_model "code.gitea.io/gitea/models/issues"
|
issues_model "code.gitea.io/gitea/models/issues"
|
||||||
|
"code.gitea.io/gitea/models/perm"
|
||||||
repo_model "code.gitea.io/gitea/models/repo"
|
repo_model "code.gitea.io/gitea/models/repo"
|
||||||
unit_model "code.gitea.io/gitea/models/unit"
|
unit_model "code.gitea.io/gitea/models/unit"
|
||||||
"code.gitea.io/gitea/models/unittest"
|
"code.gitea.io/gitea/models/unittest"
|
||||||
@ -34,7 +36,7 @@ import (
|
|||||||
func TestPullRequestTargetEvent(t *testing.T) {
|
func TestPullRequestTargetEvent(t *testing.T) {
|
||||||
onGiteaRun(t, func(t *testing.T, u *url.URL) {
|
onGiteaRun(t, func(t *testing.T, u *url.URL) {
|
||||||
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // owner of the base repo
|
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) // owner of the base repo
|
||||||
org3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) // owner of the forked repo
|
user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) // owner of the forked repo
|
||||||
|
|
||||||
// create the base repo
|
// create the base repo
|
||||||
baseRepo, err := repo_service.CreateRepository(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{
|
baseRepo, err := repo_service.CreateRepository(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{
|
||||||
@ -57,8 +59,12 @@ func TestPullRequestTargetEvent(t *testing.T) {
|
|||||||
}}, nil)
|
}}, nil)
|
||||||
assert.NoError(t, err)
|
assert.NoError(t, err)
|
||||||
|
|
||||||
|
// add user4 as the collaborator
|
||||||
|
ctx := NewAPITestContext(t, baseRepo.OwnerName, baseRepo.Name, auth_model.AccessTokenScopeWriteRepository)
|
||||||
|
t.Run("AddUser4AsCollaboratorWithReadAccess", doAPIAddCollaborator(ctx, "user4", perm.AccessModeRead))
|
||||||
|
|
||||||
// create the forked repo
|
// create the forked repo
|
||||||
forkedRepo, err := repo_service.ForkRepository(git.DefaultContext, user2, org3, repo_service.ForkRepoOptions{
|
forkedRepo, err := repo_service.ForkRepository(git.DefaultContext, user2, user4, repo_service.ForkRepoOptions{
|
||||||
BaseRepo: baseRepo,
|
BaseRepo: baseRepo,
|
||||||
Name: "forked-repo-pull-request-target",
|
Name: "forked-repo-pull-request-target",
|
||||||
Description: "test pull-request-target event",
|
Description: "test pull-request-target event",
|
||||||
@ -95,7 +101,7 @@ func TestPullRequestTargetEvent(t *testing.T) {
|
|||||||
assert.NotEmpty(t, addWorkflowToBaseResp)
|
assert.NotEmpty(t, addWorkflowToBaseResp)
|
||||||
|
|
||||||
// add a new file to the forked repo
|
// add a new file to the forked repo
|
||||||
addFileToForkedResp, err := files_service.ChangeRepoFiles(git.DefaultContext, forkedRepo, org3, &files_service.ChangeRepoFilesOptions{
|
addFileToForkedResp, err := files_service.ChangeRepoFiles(git.DefaultContext, forkedRepo, user4, &files_service.ChangeRepoFilesOptions{
|
||||||
Files: []*files_service.ChangeRepoFile{
|
Files: []*files_service.ChangeRepoFile{
|
||||||
{
|
{
|
||||||
Operation: "create",
|
Operation: "create",
|
||||||
@ -107,12 +113,12 @@ func TestPullRequestTargetEvent(t *testing.T) {
|
|||||||
OldBranch: "main",
|
OldBranch: "main",
|
||||||
NewBranch: "fork-branch-1",
|
NewBranch: "fork-branch-1",
|
||||||
Author: &files_service.IdentityOptions{
|
Author: &files_service.IdentityOptions{
|
||||||
Name: org3.Name,
|
Name: user4.Name,
|
||||||
Email: org3.Email,
|
Email: user4.Email,
|
||||||
},
|
},
|
||||||
Committer: &files_service.IdentityOptions{
|
Committer: &files_service.IdentityOptions{
|
||||||
Name: org3.Name,
|
Name: user4.Name,
|
||||||
Email: org3.Email,
|
Email: user4.Email,
|
||||||
},
|
},
|
||||||
Dates: &files_service.CommitDateOptions{
|
Dates: &files_service.CommitDateOptions{
|
||||||
Author: time.Now(),
|
Author: time.Now(),
|
||||||
@ -126,8 +132,8 @@ func TestPullRequestTargetEvent(t *testing.T) {
|
|||||||
pullIssue := &issues_model.Issue{
|
pullIssue := &issues_model.Issue{
|
||||||
RepoID: baseRepo.ID,
|
RepoID: baseRepo.ID,
|
||||||
Title: "Test pull-request-target-event",
|
Title: "Test pull-request-target-event",
|
||||||
PosterID: org3.ID,
|
PosterID: user4.ID,
|
||||||
Poster: org3,
|
Poster: user4,
|
||||||
IsPull: true,
|
IsPull: true,
|
||||||
}
|
}
|
||||||
pullRequest := &issues_model.PullRequest{
|
pullRequest := &issues_model.PullRequest{
|
||||||
@ -149,7 +155,7 @@ func TestPullRequestTargetEvent(t *testing.T) {
|
|||||||
assert.Equal(t, actions_module.GithubEventPullRequestTarget, actionRun.TriggerEvent)
|
assert.Equal(t, actions_module.GithubEventPullRequestTarget, actionRun.TriggerEvent)
|
||||||
|
|
||||||
// add another file whose name cannot match the specified path
|
// add another file whose name cannot match the specified path
|
||||||
addFileToForkedResp, err = files_service.ChangeRepoFiles(git.DefaultContext, forkedRepo, org3, &files_service.ChangeRepoFilesOptions{
|
addFileToForkedResp, err = files_service.ChangeRepoFiles(git.DefaultContext, forkedRepo, user4, &files_service.ChangeRepoFilesOptions{
|
||||||
Files: []*files_service.ChangeRepoFile{
|
Files: []*files_service.ChangeRepoFile{
|
||||||
{
|
{
|
||||||
Operation: "create",
|
Operation: "create",
|
||||||
@ -161,12 +167,12 @@ func TestPullRequestTargetEvent(t *testing.T) {
|
|||||||
OldBranch: "main",
|
OldBranch: "main",
|
||||||
NewBranch: "fork-branch-2",
|
NewBranch: "fork-branch-2",
|
||||||
Author: &files_service.IdentityOptions{
|
Author: &files_service.IdentityOptions{
|
||||||
Name: org3.Name,
|
Name: user4.Name,
|
||||||
Email: org3.Email,
|
Email: user4.Email,
|
||||||
},
|
},
|
||||||
Committer: &files_service.IdentityOptions{
|
Committer: &files_service.IdentityOptions{
|
||||||
Name: org3.Name,
|
Name: user4.Name,
|
||||||
Email: org3.Email,
|
Email: user4.Email,
|
||||||
},
|
},
|
||||||
Dates: &files_service.CommitDateOptions{
|
Dates: &files_service.CommitDateOptions{
|
||||||
Author: time.Now(),
|
Author: time.Now(),
|
||||||
@ -180,8 +186,8 @@ func TestPullRequestTargetEvent(t *testing.T) {
|
|||||||
pullIssue = &issues_model.Issue{
|
pullIssue = &issues_model.Issue{
|
||||||
RepoID: baseRepo.ID,
|
RepoID: baseRepo.ID,
|
||||||
Title: "A mismatched path cannot trigger pull-request-target-event",
|
Title: "A mismatched path cannot trigger pull-request-target-event",
|
||||||
PosterID: org3.ID,
|
PosterID: user4.ID,
|
||||||
Poster: org3,
|
Poster: user4,
|
||||||
IsPull: true,
|
IsPull: true,
|
||||||
}
|
}
|
||||||
pullRequest = &issues_model.PullRequest{
|
pullRequest = &issues_model.PullRequest{
|
||||||
|
@ -12,6 +12,7 @@ import (
|
|||||||
auth_model "code.gitea.io/gitea/models/auth"
|
auth_model "code.gitea.io/gitea/models/auth"
|
||||||
"code.gitea.io/gitea/models/db"
|
"code.gitea.io/gitea/models/db"
|
||||||
issues_model "code.gitea.io/gitea/models/issues"
|
issues_model "code.gitea.io/gitea/models/issues"
|
||||||
|
"code.gitea.io/gitea/models/perm"
|
||||||
repo_model "code.gitea.io/gitea/models/repo"
|
repo_model "code.gitea.io/gitea/models/repo"
|
||||||
"code.gitea.io/gitea/models/unittest"
|
"code.gitea.io/gitea/models/unittest"
|
||||||
user_model "code.gitea.io/gitea/models/user"
|
user_model "code.gitea.io/gitea/models/user"
|
||||||
@ -126,6 +127,65 @@ func TestAPICreatePullSuccess(t *testing.T) {
|
|||||||
MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail
|
MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestAPICreatePullBasePermission(t *testing.T) {
|
||||||
|
defer tests.PrepareTestEnv(t)()
|
||||||
|
repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10})
|
||||||
|
// repo10 have code, pulls units.
|
||||||
|
repo11 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11})
|
||||||
|
// repo11 only have code unit but should still create pulls
|
||||||
|
owner10 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo10.OwnerID})
|
||||||
|
user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
|
||||||
|
|
||||||
|
session := loginUser(t, user4.Name)
|
||||||
|
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
|
||||||
|
opts := &api.CreatePullRequestOption{
|
||||||
|
Head: fmt.Sprintf("%s:master", repo11.OwnerName),
|
||||||
|
Base: "master",
|
||||||
|
Title: "create a failure pr",
|
||||||
|
}
|
||||||
|
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
|
||||||
|
MakeRequest(t, req, http.StatusForbidden)
|
||||||
|
|
||||||
|
// add user4 to be a collaborator to base repo
|
||||||
|
ctx := NewAPITestContext(t, repo10.OwnerName, repo10.Name, auth_model.AccessTokenScopeWriteRepository)
|
||||||
|
t.Run("AddUser4AsCollaborator", doAPIAddCollaborator(ctx, user4.Name, perm.AccessModeRead))
|
||||||
|
|
||||||
|
// create again
|
||||||
|
req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
|
||||||
|
MakeRequest(t, req, http.StatusCreated)
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestAPICreatePullHeadPermission(t *testing.T) {
|
||||||
|
defer tests.PrepareTestEnv(t)()
|
||||||
|
repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10})
|
||||||
|
// repo10 have code, pulls units.
|
||||||
|
repo11 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11})
|
||||||
|
// repo11 only have code unit but should still create pulls
|
||||||
|
owner10 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo10.OwnerID})
|
||||||
|
user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
|
||||||
|
|
||||||
|
session := loginUser(t, user4.Name)
|
||||||
|
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
|
||||||
|
opts := &api.CreatePullRequestOption{
|
||||||
|
Head: fmt.Sprintf("%s:master", repo11.OwnerName),
|
||||||
|
Base: "master",
|
||||||
|
Title: "create a failure pr",
|
||||||
|
}
|
||||||
|
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
|
||||||
|
MakeRequest(t, req, http.StatusForbidden)
|
||||||
|
|
||||||
|
// add user4 to be a collaborator to head repo with read permission
|
||||||
|
ctx := NewAPITestContext(t, repo11.OwnerName, repo11.Name, auth_model.AccessTokenScopeWriteRepository)
|
||||||
|
t.Run("AddUser4AsCollaboratorWithRead", doAPIAddCollaborator(ctx, user4.Name, perm.AccessModeRead))
|
||||||
|
req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
|
||||||
|
MakeRequest(t, req, http.StatusForbidden)
|
||||||
|
|
||||||
|
// add user4 to be a collaborator to head repo with write permission
|
||||||
|
t.Run("AddUser4AsCollaboratorWithWrite", doAPIAddCollaborator(ctx, user4.Name, perm.AccessModeWrite))
|
||||||
|
req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
|
||||||
|
MakeRequest(t, req, http.StatusCreated)
|
||||||
|
}
|
||||||
|
|
||||||
func TestAPICreatePullSameRepoSuccess(t *testing.T) {
|
func TestAPICreatePullSameRepoSuccess(t *testing.T) {
|
||||||
defer tests.PrepareTestEnv(t)()
|
defer tests.PrepareTestEnv(t)()
|
||||||
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
|
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
|
||||||
|
Loading…
Reference in New Issue
Block a user