From 2483a93fbc2111bac8f53a0411cbddfa4395b8a9 Mon Sep 17 00:00:00 2001 From: Kemal Zebari <60799661+kemzeb@users.noreply.github.com> Date: Wed, 15 Jan 2025 12:51:49 -0800 Subject: Only allow admins to rename default/protected branches (#33276) Currently, anyone with write permissions to a repo are able to rename default or protected branches. This change follows [GitHub's](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-branches-in-your-repository/renaming-a-branch) design by only allowing repo/site admins to change these branches. However, it also follows are current design for protected branches and only allows admins to modify branch names == branch protection rule names. Glob-based rules cannot be renamed by anyone (as was already the case, but we now catch `ErrBranchIsProtected` which we previously did not catch, throwing a 500). --- models/fixtures/access.yml | 6 ++++ options/locale/locale_en-US.ini | 2 ++ routers/api/v1/repo/branch.go | 10 +++++- routers/web/repo/setting/protected_branch.go | 8 +++++ services/repository/branch.go | 23 +++++++++++++ tests/integration/api_branch_test.go | 49 +++++++++++++++++++++++----- tests/integration/api_repo_test.go | 2 +- 7 files changed, 90 insertions(+), 10 deletions(-) diff --git a/models/fixtures/access.yml b/models/fixtures/access.yml index 4171e31fef..596046e950 100644 --- a/models/fixtures/access.yml +++ b/models/fixtures/access.yml @@ -171,3 +171,9 @@ user_id: 40 repo_id: 61 mode: 4 + +- + id: 30 + user_id: 40 + repo_id: 1 + mode: 2 diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index e5fab5911a..8d9dd69b3d 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2714,6 +2714,8 @@ branch.create_branch_operation = Create branch branch.new_branch = Create new branch branch.new_branch_from = Create new branch from "%s" branch.renamed = Branch %s was renamed to %s. +branch.rename_default_or_protected_branch_error = Only admins can rename default or protected branches. +branch.rename_protected_branch_failed = This branch is protected by glob-based protection rules. tag.create_tag = Create tag %s tag.create_tag_operation = Create tag diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 592879235c..a5ea752cf1 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" "code.gitea.io/gitea/models/organization" + repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" @@ -443,7 +444,14 @@ func UpdateBranch(ctx *context.APIContext) { msg, err := repo_service.RenameBranch(ctx, repo, ctx.Doer, ctx.Repo.GitRepo, oldName, opt.Name) if err != nil { - ctx.Error(http.StatusInternalServerError, "RenameBranch", err) + switch { + case repo_model.IsErrUserDoesNotHaveAccessToRepo(err): + ctx.Error(http.StatusForbidden, "", "User must be a repo or site admin to rename default or protected branches.") + case errors.Is(err, git_model.ErrBranchIsProtected): + ctx.Error(http.StatusForbidden, "", "Branch is protected by glob-based protection rules.") + default: + ctx.Error(http.StatusInternalServerError, "RenameBranch", err) + } return } if msg == "target_exist" { diff --git a/routers/web/repo/setting/protected_branch.go b/routers/web/repo/setting/protected_branch.go index 022a24a9ad..06a9e69507 100644 --- a/routers/web/repo/setting/protected_branch.go +++ b/routers/web/repo/setting/protected_branch.go @@ -4,6 +4,7 @@ package setting import ( + "errors" "fmt" "net/http" "net/url" @@ -14,6 +15,7 @@ import ( "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/templates" "code.gitea.io/gitea/modules/web" @@ -351,9 +353,15 @@ func RenameBranchPost(ctx *context.Context) { msg, err := repository.RenameBranch(ctx, ctx.Repo.Repository, ctx.Doer, ctx.Repo.GitRepo, form.From, form.To) if err != nil { switch { + case repo_model.IsErrUserDoesNotHaveAccessToRepo(err): + ctx.Flash.Error(ctx.Tr("repo.branch.rename_default_or_protected_branch_error")) + ctx.Redirect(fmt.Sprintf("%s/branches", ctx.Repo.RepoLink)) case git_model.IsErrBranchAlreadyExists(err): ctx.Flash.Error(ctx.Tr("repo.branch.branch_already_exists", form.To)) ctx.Redirect(fmt.Sprintf("%s/branches", ctx.Repo.RepoLink)) + case errors.Is(err, git_model.ErrBranchIsProtected): + ctx.Flash.Error(ctx.Tr("repo.branch.rename_protected_branch_failed")) + ctx.Redirect(fmt.Sprintf("%s/branches", ctx.Repo.RepoLink)) default: ctx.ServerError("RenameBranch", err) } diff --git a/services/repository/branch.go b/services/repository/branch.go index 302cfff62e..a1065f5641 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -416,6 +416,29 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m return "from_not_exist", nil } + perm, err := access_model.GetUserRepoPermission(ctx, repo, doer) + if err != nil { + return "", err + } + + isDefault := from == repo.DefaultBranch + if isDefault && !perm.IsAdmin() { + return "", repo_model.ErrUserDoesNotHaveAccessToRepo{ + UserID: doer.ID, + RepoName: repo.LowerName, + } + } + + // If from == rule name, admins are allowed to modify them. + if protectedBranch, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, from); err != nil { + return "", err + } else if protectedBranch != nil && !perm.IsAdmin() { + return "", repo_model.ErrUserDoesNotHaveAccessToRepo{ + UserID: doer.ID, + RepoName: repo.LowerName, + } + } + if err := git_model.RenameBranch(ctx, repo, from, to, func(ctx context.Context, isDefault bool) error { err2 := gitRepo.RenameBranch(from, to) if err2 != nil { diff --git a/tests/integration/api_branch_test.go b/tests/integration/api_branch_test.go index 8a0bd2e4ff..d64dd97f93 100644 --- a/tests/integration/api_branch_test.go +++ b/tests/integration/api_branch_test.go @@ -190,28 +190,61 @@ func testAPICreateBranch(t testing.TB, session *TestSession, user, repo, oldBran func TestAPIUpdateBranch(t *testing.T) { onGiteaRun(t, func(t *testing.T, _ *url.URL) { t.Run("UpdateBranchWithEmptyRepo", func(t *testing.T) { - testAPIUpdateBranch(t, "user10", "repo6", "master", "test", http.StatusNotFound) + testAPIUpdateBranch(t, "user10", "user10", "repo6", "master", "test", http.StatusNotFound) }) t.Run("UpdateBranchWithSameBranchNames", func(t *testing.T) { - resp := testAPIUpdateBranch(t, "user2", "repo1", "master", "master", http.StatusUnprocessableEntity) + resp := testAPIUpdateBranch(t, "user2", "user2", "repo1", "master", "master", http.StatusUnprocessableEntity) assert.Contains(t, resp.Body.String(), "Cannot rename a branch using the same name or rename to a branch that already exists.") }) t.Run("UpdateBranchThatAlreadyExists", func(t *testing.T) { - resp := testAPIUpdateBranch(t, "user2", "repo1", "master", "branch2", http.StatusUnprocessableEntity) + resp := testAPIUpdateBranch(t, "user2", "user2", "repo1", "master", "branch2", http.StatusUnprocessableEntity) assert.Contains(t, resp.Body.String(), "Cannot rename a branch using the same name or rename to a branch that already exists.") }) t.Run("UpdateBranchWithNonExistentBranch", func(t *testing.T) { - resp := testAPIUpdateBranch(t, "user2", "repo1", "i-dont-exist", "new-branch-name", http.StatusNotFound) + resp := testAPIUpdateBranch(t, "user2", "user2", "repo1", "i-dont-exist", "new-branch-name", http.StatusNotFound) assert.Contains(t, resp.Body.String(), "Branch doesn't exist.") }) - t.Run("RenameBranchNormalScenario", func(t *testing.T) { - testAPIUpdateBranch(t, "user2", "repo1", "branch2", "new-branch-name", http.StatusNoContent) + t.Run("UpdateBranchWithNonAdminDoer", func(t *testing.T) { + // don't allow default branch renaming + resp := testAPIUpdateBranch(t, "user40", "user2", "repo1", "master", "new-branch-name", http.StatusForbidden) + assert.Contains(t, resp.Body.String(), "User must be a repo or site admin to rename default or protected branches.") + + // don't allow protected branch renaming + token := getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository) + req := NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/branches", &api.CreateBranchRepoOption{ + BranchName: "protected-branch", + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + testAPICreateBranchProtection(t, "protected-branch", 1, http.StatusCreated) + resp = testAPIUpdateBranch(t, "user40", "user2", "repo1", "protected-branch", "new-branch-name", http.StatusForbidden) + assert.Contains(t, resp.Body.String(), "User must be a repo or site admin to rename default or protected branches.") + }) + t.Run("UpdateBranchWithGlobedBasedProtectionRulesAndAdminAccess", func(t *testing.T) { + // don't allow branch that falls under glob-based protection rules to be renamed + token := getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository) + req := NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/branch_protections", &api.BranchProtection{ + RuleName: "protected/**", + EnablePush: true, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + + from := "protected/1" + req = NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/branches", &api.CreateBranchRepoOption{ + BranchName: from, + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + + resp := testAPIUpdateBranch(t, "user2", "user2", "repo1", from, "new-branch-name", http.StatusForbidden) + assert.Contains(t, resp.Body.String(), "Branch is protected by glob-based protection rules.") + }) + t.Run("UpdateBranchNormalScenario", func(t *testing.T) { + testAPIUpdateBranch(t, "user2", "user2", "repo1", "branch2", "new-branch-name", http.StatusNoContent) }) }) } -func testAPIUpdateBranch(t *testing.T, ownerName, repoName, from, to string, expectedHTTPStatus int) *httptest.ResponseRecorder { - token := getUserToken(t, ownerName, auth_model.AccessTokenScopeWriteRepository) +func testAPIUpdateBranch(t *testing.T, doerName, ownerName, repoName, from, to string, expectedHTTPStatus int) *httptest.ResponseRecorder { + token := getUserToken(t, doerName, auth_model.AccessTokenScopeWriteRepository) req := NewRequestWithJSON(t, "PATCH", "api/v1/repos/"+ownerName+"/"+repoName+"/branches/"+from, &api.UpdateBranchRepoOption{ Name: to, }).AddTokenAuth(token) diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index 122afbfa08..13dc90f8a7 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -735,5 +735,5 @@ func TestAPIRepoGetAssignees(t *testing.T) { resp := MakeRequest(t, req, http.StatusOK) var assignees []*api.User DecodeJSON(t, resp, &assignees) - assert.Len(t, assignees, 1) + assert.Len(t, assignees, 2) } -- cgit v1.2.3