From e0a408e6f374352a4b4ed06c727b1d5777ca6ea8 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Mon, 29 Jul 2024 11:21:22 +0900 Subject: [PATCH 1/5] Add permission check when creating PR (#31033) user should be a collaborator of the base repo to create a PR --- models/issues/pull.go | 8 +++ options/locale/locale_en-US.ini | 1 + routers/api/v1/repo/pull.go | 2 + routers/web/repo/pull.go | 10 ++++ services/pull/pull.go | 24 +++++++++ tests/integration/actions_trigger_test.go | 38 ++++++++------ tests/integration/api_pull_test.go | 60 +++++++++++++++++++++++ 7 files changed, 127 insertions(+), 16 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index ef49a510458b..a4e61476198d 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -27,6 +27,8 @@ import ( "xorm.io/builder" ) +var ErrMustCollaborator = util.NewPermissionDeniedErrorf("user must be a collaborator") + // ErrPullRequestNotExist represents a "PullRequestNotExist" kind of error. type ErrPullRequestNotExist struct { ID int64 @@ -572,6 +574,12 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *Iss 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 // by given head/base and repo/branch. func GetUnmergedPullRequest(ctx context.Context, headRepoID, baseRepoID int64, headBranch, baseBranch string, flow PullRequestFlow) (*PullRequest, error) { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 6a748aed0022..53d746ef1256 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1765,6 +1765,7 @@ compare.compare_head = compare pulls.desc = Enable pull requests and code reviews. pulls.new = New Pull Request 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.view = View Pull Request pulls.compare_changes = New Pull Request diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index ebe876da6460..148b6ed637f0 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -535,6 +535,8 @@ func CreatePullRequest(ctx *context.APIContext) { ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err) } else if errors.Is(err, user_model.ErrBlockedUser) { ctx.Error(http.StatusForbidden, "BlockedUser", err) + } else if errors.Is(err, issues_model.ErrMustCollaborator) { + ctx.Error(http.StatusForbidden, "MustCollaborator", err) } else { ctx.Error(http.StatusInternalServerError, "NewPullRequest", err) } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index f9642d44d4a3..9531482bee76 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1337,6 +1337,16 @@ func CompareAndPullRequestPost(ctx *context.Context) { return } 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 } diff --git a/services/pull/pull.go b/services/pull/pull.go index 5c0ea42d7752..e69c842a2d4b 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -17,7 +17,9 @@ import ( "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" 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" + "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/base" "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 } + // 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) if err != nil { if !git_model.IsErrBranchNotExist(err) { diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index 2a2fdceb61e9..ed0c60737437 100644 --- a/tests/integration/actions_trigger_test.go +++ b/tests/integration/actions_trigger_test.go @@ -11,9 +11,11 @@ import ( "time" actions_model "code.gitea.io/gitea/models/actions" + auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" unit_model "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" @@ -34,7 +36,7 @@ import ( func TestPullRequestTargetEvent(t *testing.T) { onGiteaRun(t, func(t *testing.T, u *url.URL) { 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 baseRepo, err := repo_service.CreateRepository(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{ @@ -57,8 +59,12 @@ func TestPullRequestTargetEvent(t *testing.T) { }}, nil) 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 - 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, Name: "forked-repo-pull-request-target", Description: "test pull-request-target event", @@ -95,7 +101,7 @@ func TestPullRequestTargetEvent(t *testing.T) { assert.NotEmpty(t, addWorkflowToBaseResp) // 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{ { Operation: "create", @@ -107,12 +113,12 @@ func TestPullRequestTargetEvent(t *testing.T) { OldBranch: "main", NewBranch: "fork-branch-1", Author: &files_service.IdentityOptions{ - Name: org3.Name, - Email: org3.Email, + Name: user4.Name, + Email: user4.Email, }, Committer: &files_service.IdentityOptions{ - Name: org3.Name, - Email: org3.Email, + Name: user4.Name, + Email: user4.Email, }, Dates: &files_service.CommitDateOptions{ Author: time.Now(), @@ -126,8 +132,8 @@ func TestPullRequestTargetEvent(t *testing.T) { pullIssue := &issues_model.Issue{ RepoID: baseRepo.ID, Title: "Test pull-request-target-event", - PosterID: org3.ID, - Poster: org3, + PosterID: user4.ID, + Poster: user4, IsPull: true, } pullRequest := &issues_model.PullRequest{ @@ -149,7 +155,7 @@ func TestPullRequestTargetEvent(t *testing.T) { assert.Equal(t, actions_module.GithubEventPullRequestTarget, actionRun.TriggerEvent) // 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{ { Operation: "create", @@ -161,12 +167,12 @@ func TestPullRequestTargetEvent(t *testing.T) { OldBranch: "main", NewBranch: "fork-branch-2", Author: &files_service.IdentityOptions{ - Name: org3.Name, - Email: org3.Email, + Name: user4.Name, + Email: user4.Email, }, Committer: &files_service.IdentityOptions{ - Name: org3.Name, - Email: org3.Email, + Name: user4.Name, + Email: user4.Email, }, Dates: &files_service.CommitDateOptions{ Author: time.Now(), @@ -180,8 +186,8 @@ func TestPullRequestTargetEvent(t *testing.T) { pullIssue = &issues_model.Issue{ RepoID: baseRepo.ID, Title: "A mismatched path cannot trigger pull-request-target-event", - PosterID: org3.ID, - Poster: org3, + PosterID: user4.ID, + Poster: user4, IsPull: true, } pullRequest = &issues_model.PullRequest{ diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 9bf0d3d74517..8239878d2b78 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -12,6 +12,7 @@ import ( auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" 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 } +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) { defer tests.PrepareTestEnv(t)() repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) From 7b388630ecb4537f9bb04e55cbb10eb7cf83b9c5 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Mon, 29 Jul 2024 15:51:02 +0900 Subject: [PATCH 2/5] Fix loadRepository error when access user dashboard (#31719) --- models/git/commit_status.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/models/git/commit_status.go b/models/git/commit_status.go index f80de9679f3a..deec21e38b70 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -211,6 +211,10 @@ func (status *CommitStatus) LocaleString(lang translation.Locale) string { // HideActionsURL set `TargetURL` to an empty string if the status comes from Gitea Actions func (status *CommitStatus) HideActionsURL(ctx context.Context) { + if status.RepoID == 0 { + return + } + if status.Repo == nil { if err := status.loadRepository(ctx); err != nil { log.Error("loadRepository: %v", err) From bf5ae79c5163b8dd6a3185711ad11893b1270f62 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Tue, 30 Jul 2024 00:45:24 +0800 Subject: [PATCH 3/5] Fix API endpoint for registration-token (#31722) Partially fix #31707. Related to #30656 --- routers/api/v1/repo/action.go | 2 +- templates/swagger/v1_json.tmpl | 66 +++++++++++++++++----------------- 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/routers/api/v1/repo/action.go b/routers/api/v1/repo/action.go index 48ba35ac2117..a777779b248e 100644 --- a/routers/api/v1/repo/action.go +++ b/routers/api/v1/repo/action.go @@ -486,7 +486,7 @@ func (Action) ListVariables(ctx *context.APIContext) { // GetRegistrationToken returns the token to register repo runners func (Action) GetRegistrationToken(ctx *context.APIContext) { - // swagger:operation GET /repos/{owner}/{repo}/runners/registration-token repository repoGetRunnerRegistrationToken + // swagger:operation GET /repos/{owner}/{repo}/actions/runners/registration-token repository repoGetRunnerRegistrationToken // --- // summary: Get a repository's actions runner registration token // produces: diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index e7c67616068c..18b9cdab3a3f 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -3843,6 +3843,39 @@ } } }, + "/repos/{owner}/{repo}/actions/runners/registration-token": { + "get": { + "produces": [ + "application/json" + ], + "tags": [ + "repository" + ], + "summary": "Get a repository's actions runner registration token", + "operationId": "repoGetRunnerRegistrationToken", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "$ref": "#/responses/RegistrationToken" + } + } + } + }, "/repos/{owner}/{repo}/actions/secrets": { "get": { "produces": [ @@ -13408,39 +13441,6 @@ } } }, - "/repos/{owner}/{repo}/runners/registration-token": { - "get": { - "produces": [ - "application/json" - ], - "tags": [ - "repository" - ], - "summary": "Get a repository's actions runner registration token", - "operationId": "repoGetRunnerRegistrationToken", - "parameters": [ - { - "type": "string", - "description": "owner of the repo", - "name": "owner", - "in": "path", - "required": true - }, - { - "type": "string", - "description": "name of the repo", - "name": "repo", - "in": "path", - "required": true - } - ], - "responses": { - "200": { - "$ref": "#/responses/RegistrationToken" - } - } - } - }, "/repos/{owner}/{repo}/signing-key.gpg": { "get": { "produces": [ From d39bce7f003cf2137a5a561ed488c7b638e52275 Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Tue, 30 Jul 2024 01:15:02 +0800 Subject: [PATCH 4/5] fix(api): owner ID should be zero when created repo secret (#31715) - Change condition to include `RepoID` equal to 0 for organization secrets --------- Signed-off-by: Bo-Yi Wu Co-authored-by: Giteabot --- routers/api/v1/repo/action.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/routers/api/v1/repo/action.go b/routers/api/v1/repo/action.go index a777779b248e..8add076c0e64 100644 --- a/routers/api/v1/repo/action.go +++ b/routers/api/v1/repo/action.go @@ -117,12 +117,11 @@ func (Action) CreateOrUpdateSecret(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - owner := ctx.Repo.Owner repo := ctx.Repo.Repository opt := web.GetForm(ctx).(*api.CreateOrUpdateSecretOption) - _, created, err := secret_service.CreateOrUpdateSecret(ctx, owner.ID, repo.ID, ctx.PathParam("secretname"), opt.Data) + _, created, err := secret_service.CreateOrUpdateSecret(ctx, 0, repo.ID, ctx.PathParam("secretname"), opt.Data) if err != nil { if errors.Is(err, util.ErrInvalidArgument) { ctx.Error(http.StatusBadRequest, "CreateOrUpdateSecret", err) @@ -174,10 +173,9 @@ func (Action) DeleteSecret(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - owner := ctx.Repo.Owner repo := ctx.Repo.Repository - err := secret_service.DeleteSecretByName(ctx, owner.ID, repo.ID, ctx.PathParam("secretname")) + err := secret_service.DeleteSecretByName(ctx, 0, repo.ID, ctx.PathParam("secretname")) if err != nil { if errors.Is(err, util.ErrInvalidArgument) { ctx.Error(http.StatusBadRequest, "DeleteSecret", err) From 81fa471119a6733d257f63f8c2c1f4acc583d21b Mon Sep 17 00:00:00 2001 From: Jason Song Date: Tue, 30 Jul 2024 02:46:45 +0800 Subject: [PATCH 5/5] Set owner id to zero when GetRegistrationToken for repo (#31725) Fix #31707. It's split from #31724. Although #31724 could also fix #31707, it has change a lot so it's not a good idea to backport it. --- routers/api/v1/repo/action.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/repo/action.go b/routers/api/v1/repo/action.go index 8add076c0e64..d27e8d2427b3 100644 --- a/routers/api/v1/repo/action.go +++ b/routers/api/v1/repo/action.go @@ -504,7 +504,7 @@ func (Action) GetRegistrationToken(ctx *context.APIContext) { // "200": // "$ref": "#/responses/RegistrationToken" - shared.GetRegistrationToken(ctx, ctx.Repo.Repository.OwnerID, ctx.Repo.Repository.ID) + shared.GetRegistrationToken(ctx, 0, ctx.Repo.Repository.ID) } var _ actions_service.API = new(Action)