From 8843b8b986bd1f098cd1d07750270c3797ee76bd Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Tue, 14 Jan 2025 14:50:58 -0800 Subject: [PATCH 1/5] Only allow admins to rename default/protected branches --- models/fixtures/access.yml | 6 ++++ options/locale/locale_en-US.ini | 1 + routers/api/v1/repo/branch.go | 8 +++++- routers/web/repo/setting/protected_branch.go | 4 +++ services/repository/branch.go | 24 ++++++++++++++++ tests/integration/api_branch_test.go | 29 +++++++++++++++----- 6 files changed, 64 insertions(+), 8 deletions(-) diff --git a/models/fixtures/access.yml b/models/fixtures/access.yml index 4171e31fef777..171b6234192ad 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: 4 + repo_id: 1 + mode: 2 diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 93155caa109f4..1982b12e26b78 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2713,6 +2713,7 @@ 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. 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 592879235cda0..98ab8da84368a 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,12 @@ 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.") + 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 022a24a9ad465..3aeae43638bed 100644 --- a/routers/web/repo/setting/protected_branch.go +++ b/routers/web/repo/setting/protected_branch.go @@ -14,6 +14,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,6 +352,9 @@ 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)) diff --git a/services/repository/branch.go b/services/repository/branch.go index 302cfff62e4b7..2cf3ba85daddd 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -416,6 +416,30 @@ 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, + } + } + + isProtected, err := git_model.IsBranchProtected(ctx, repo.ID, from) + if err != nil { + return "", err + } + if isProtected && !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 8a0bd2e4ffa4b..e2e7046cb257a 100644 --- a/tests/integration/api_branch_test.go +++ b/tests/integration/api_branch_test.go @@ -190,28 +190,43 @@ 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("UpdateBranchWithNonAdminDoer", func(t *testing.T) { + // don't allow default branch renaming + resp := testAPIUpdateBranch(t, "user4", "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, "user4", "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("RenameBranchNormalScenario", func(t *testing.T) { - testAPIUpdateBranch(t, "user2", "repo1", "branch2", "new-branch-name", http.StatusNoContent) + 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) From 2e5471d701ef4aa260cc0312962e337ebce4937c Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Tue, 14 Jan 2025 16:09:02 -0800 Subject: [PATCH 2/5] Try changing doer user in test to avoid conflicts with other tests --- models/fixtures/access.yml | 2 +- tests/integration/api_branch_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/models/fixtures/access.yml b/models/fixtures/access.yml index 171b6234192ad..596046e95022e 100644 --- a/models/fixtures/access.yml +++ b/models/fixtures/access.yml @@ -174,6 +174,6 @@ - id: 30 - user_id: 4 + user_id: 40 repo_id: 1 mode: 2 diff --git a/tests/integration/api_branch_test.go b/tests/integration/api_branch_test.go index e2e7046cb257a..c419f3c26b12d 100644 --- a/tests/integration/api_branch_test.go +++ b/tests/integration/api_branch_test.go @@ -206,7 +206,7 @@ func TestAPIUpdateBranch(t *testing.T) { }) t.Run("UpdateBranchWithNonAdminDoer", func(t *testing.T) { // don't allow default branch renaming - resp := testAPIUpdateBranch(t, "user4", "user2", "repo1", "master", "new-branch-name", http.StatusForbidden) + 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 @@ -216,7 +216,7 @@ func TestAPIUpdateBranch(t *testing.T) { }).AddTokenAuth(token) MakeRequest(t, req, http.StatusCreated) testAPICreateBranchProtection(t, "protected-branch", 1, http.StatusCreated) - resp = testAPIUpdateBranch(t, "user4", "user2", "repo1", "protected-branch", "new-branch-name", http.StatusForbidden) + 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("RenameBranchNormalScenario", func(t *testing.T) { From 5064d8312a83da568d005208260bcae7e4f2e209 Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Tue, 14 Jan 2025 16:49:52 -0800 Subject: [PATCH 3/5] Modify existing test to account for new access rules in repo1 --- tests/integration/api_repo_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index 122afbfa08fb1..13dc90f8a7dab 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) } From 22e1ee589d4f9b5930d95261a36e774dcc494653 Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Tue, 14 Jan 2025 18:55:39 -0800 Subject: [PATCH 4/5] Account for protected branches design --- options/locale/locale_en-US.ini | 1 + routers/api/v1/repo/branch.go | 2 ++ routers/web/repo/setting/protected_branch.go | 4 ++++ services/repository/branch.go | 15 ++++++++------- tests/integration/api_branch_test.go | 20 +++++++++++++++++++- 5 files changed, 34 insertions(+), 8 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 1982b12e26b78..b6bae6dfdd618 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2714,6 +2714,7 @@ 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 98ab8da84368a..a5ea752cf10e1 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -447,6 +447,8 @@ func UpdateBranch(ctx *context.APIContext) { 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) } diff --git a/routers/web/repo/setting/protected_branch.go b/routers/web/repo/setting/protected_branch.go index 3aeae43638bed..06a9e69507193 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" @@ -358,6 +359,9 @@ func RenameBranchPost(ctx *context.Context) { 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 2cf3ba85daddd..b906298692638 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -429,14 +429,15 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m } } - isProtected, err := git_model.IsBranchProtected(ctx, repo.ID, from) - if err != nil { + // If from == rule name, admins are allowed to modify them. + if protectedBranch, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, from); err != nil { return "", err - } - if isProtected && !perm.IsAdmin() { - return "", repo_model.ErrUserDoesNotHaveAccessToRepo{ - UserID: doer.ID, - RepoName: repo.LowerName, + } else { + if protectedBranch != nil && !perm.IsAdmin() { + return "", repo_model.ErrUserDoesNotHaveAccessToRepo{ + UserID: doer.ID, + RepoName: repo.LowerName, + } } } diff --git a/tests/integration/api_branch_test.go b/tests/integration/api_branch_test.go index c419f3c26b12d..d64dd97f93f0f 100644 --- a/tests/integration/api_branch_test.go +++ b/tests/integration/api_branch_test.go @@ -219,7 +219,25 @@ func TestAPIUpdateBranch(t *testing.T) { 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("RenameBranchNormalScenario", func(t *testing.T) { + 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) }) }) From cacfdda08f185e46f38cdd0e3d026001b42287c5 Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Tue, 14 Jan 2025 19:05:47 -0800 Subject: [PATCH 5/5] Resolve lint CI failure --- services/repository/branch.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index b906298692638..a1065f5641108 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -432,12 +432,10 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m // 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, - } + } else if protectedBranch != nil && !perm.IsAdmin() { + return "", repo_model.ErrUserDoesNotHaveAccessToRepo{ + UserID: doer.ID, + RepoName: repo.LowerName, } }