Merge pull request '[BUG] Don't overwrite protected branch accidentally' (#2473) from gusted/forgejo-error-duplicate into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2473
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

options/locale/locale_en-US.ini

@@ -2425,7 +2425,7 @@ settings.choose_branch = Choose a branch…
 settings.no_protected_branch = There are no protected branches.
 settings.edit_protected_branch = Edit
 settings.protected_branch_required_rule_name = Required rule name
-settings.protected_branch_duplicate_rule_name = Duplicate rule name
+settings.protected_branch_duplicate_rule_name = There is already a rule for this set of branches
 settings.protected_branch_required_approvals_min = Required approvals cannot be negative.
 settings.tags = Tags
 settings.tags.protection = Tag Protection

routers/web/repo/setting/protected_branch.go

@@ -132,12 +132,15 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
 			}
 		}
 	} else {
-		// FIXME: If a new ProtectBranch has a duplicate RuleName, an error should be returned.
+		// Check if a rule already exists with this rulename, if so redirect to it.
-		// Currently, if a new ProtectBranch with a duplicate RuleName is created, the existing ProtectBranch will be updated.
-		// But we cannot modify this logic now because many unit tests rely on it.
 		protectBranch, err = git_model.GetProtectedBranchRuleByName(ctx, ctx.Repo.Repository.ID, f.RuleName)
 		if err != nil {
-			ctx.ServerError("GetProtectBranchOfRepoByName", err)
+			ctx.ServerError("GetProtectedBranchRuleByName", err)
+			return
+		}
+		if protectBranch != nil {
+			ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_duplicate_rule_name"))
+			ctx.Redirect(fmt.Sprintf("%s/settings/branches/edit?rule_name=%s", ctx.Repo.RepoLink, protectBranch.RuleName))
 			return
 		}
 	}

tests/integration/git_test.go

@@ -18,6 +18,7 @@ import (
 
 	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"
@@ -413,12 +414,17 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes
 func doProtectBranch(ctx APITestContext, branch, userToWhitelist, unprotectedFilePatterns string) func(t *testing.T) {
 	// We are going to just use the owner to set the protection.
 	return func(t *testing.T) {
+		repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: ctx.Reponame, OwnerName: ctx.Username})
+		rule := &git_model.ProtectedBranch{RuleName: branch, RepoID: repo.ID}
+		unittest.LoadBeanIfExists(rule)
+
 		csrf := GetCSRF(t, ctx.Session, fmt.Sprintf("/%s/%s/settings/branches", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame)))
 
 		if userToWhitelist == "" {
 			// Change branch to protected
 			req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame)), map[string]string{
 				"_csrf":                     csrf,
+				"rule_id":                   strconv.FormatInt(rule.ID, 10),
 				"rule_name":                 branch,
 				"unprotected_file_patterns": unprotectedFilePatterns,
 			})
@@ -430,6 +436,7 @@ func doProtectBranch(ctx APITestContext, branch, userToWhitelist, unprotectedFil
 			req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/edit", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame)), map[string]string{
 				"_csrf":                     csrf,
 				"rule_name":                 branch,
+				"rule_id":                   strconv.FormatInt(rule.ID, 10),
 				"enable_push":               "whitelist",
 				"enable_whitelist":          "on",
 				"whitelist_users":           strconv.FormatInt(user.ID, 10),

tests/integration/repo_settings_test.go

@@ -9,10 +9,12 @@ import (
 	"testing"
 
 	"code.gitea.io/gitea/models/db"
+	git_model "code.gitea.io/gitea/models/git"
 	repo_model "code.gitea.io/gitea/models/repo"
 	unit_model "code.gitea.io/gitea/models/unit"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
+	gitea_context "code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/setting"
 	repo_service "code.gitea.io/gitea/services/repository"
 	"code.gitea.io/gitea/tests"
@@ -128,3 +130,43 @@ func TestRepoAddMoreUnits(t *testing.T) {
 		assertAddMore(t, false)
 	})
 }
+
+func TestProtectedBranch(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1, OwnerID: user.ID})
+	session := loginUser(t, user.Name)
+
+	t.Run("Add", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+		link := fmt.Sprintf("/%s/settings/branches/edit", repo.FullName())
+
+		req := NewRequestWithValues(t, "POST", link, map[string]string{
+			"_csrf":       GetCSRF(t, session, link),
+			"rule_name":   "master",
+			"enable_push": "true",
+		})
+		session.MakeRequest(t, req, http.StatusSeeOther)
+
+		// Verify it was added.
+		unittest.AssertExistsIf(t, true, &git_model.ProtectedBranch{RuleName: "master", RepoID: repo.ID})
+	})
+
+	t.Run("Add duplicate", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+		link := fmt.Sprintf("/%s/settings/branches/edit", repo.FullName())
+
+		req := NewRequestWithValues(t, "POST", link, map[string]string{
+			"_csrf":           GetCSRF(t, session, link),
+			"rule_name":       "master",
+			"require_signed_": "true",
+		})
+		session.MakeRequest(t, req, http.StatusSeeOther)
+		flashCookie := session.GetCookie(gitea_context.CookieNameFlash)
+		assert.NotNil(t, flashCookie)
+		assert.EqualValues(t, "error%3DThere%2Bis%2Balready%2Ba%2Brule%2Bfor%2Bthis%2Bset%2Bof%2Bbranches", flashCookie.Value)
+
+		// Verify it wasn't added.
+		unittest.AssertCount(t, &git_model.ProtectedBranch{RuleName: "master", RepoID: repo.ID}, 1)
+	})
+}