mirror of https://github.com/go-gitea/gitea.git
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).pull/33249/head^2
parent
4b21a6c792
commit
2483a93fbc
|
@ -171,3 +171,9 @@
|
|||
user_id: 40
|
||||
repo_id: 61
|
||||
mode: 4
|
||||
|
||||
-
|
||||
id: 30
|
||||
user_id: 40
|
||||
repo_id: 1
|
||||
mode: 2
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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" {
|
||||
|
|
|
@ -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)
|
||||
}
|
||||
|
|
|
@ -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 {
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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)
|
||||
}
|
||||
|
|
Loading…
Reference in New Issue