From aa0bb74e0c6c0176b8aa9c17a35f0cf9f7702805 Mon Sep 17 00:00:00 2001 From: Oliver Braun Date: Sat, 25 Apr 2026 18:45:45 +0200 Subject: [PATCH] fix: branch protection for mergeOnly Co-authored-by: Copilot --- gitlab/branches.go | 1 + gitlab/branches_contract_test.go | 38 +++++++++++++++++++++++++++++++ gitlab/protect.go | 32 ++++++++++++++++++++++++++ gitlab/protect_contract_test.go | 39 ++++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+) create mode 100644 gitlab/branches_contract_test.go diff --git a/gitlab/branches.go b/gitlab/branches.go index 0b367ef..0ee8dd1 100644 --- a/gitlab/branches.go +++ b/gitlab/branches.go @@ -41,6 +41,7 @@ func (c *Client) syncConfiguredBranches(assignmentCfg *config.AssignmentConfig, if err := c.protectBranch(assignmentCfg, project, false); err != nil { log.Debug().Err(err).Str("project", project.Name).Msg("cannot protect configured branches") + return fmt.Errorf("error while protecting configured branches for %s: %w", project.Name, err) } return nil diff --git a/gitlab/branches_contract_test.go b/gitlab/branches_contract_test.go new file mode 100644 index 0000000..0f83b3d --- /dev/null +++ b/gitlab/branches_contract_test.go @@ -0,0 +1,38 @@ +package gitlab + +import ( + "net/http" + "strings" + "testing" + + "github.com/obcode/glabs/config" + gitlabapi "gitlab.com/gitlab-org/api/client-go/v2" +) + +func TestSyncConfiguredBranches_ReturnsErrorWhenProtectionFails(t *testing.T) { + client := newContractClient(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPatch && r.URL.Path == "/api/v4/projects/1": + _, _ = w.Write([]byte(`{"id":1,"name":"repo"}`)) + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/api/v4/projects/1/protected_branches/main"): + _, _ = w.Write([]byte(`{"id":1,"name":"main","push_access_levels":[{"id":10,"access_level":40}],"merge_access_levels":[{"id":11,"access_level":40}],"unprotect_access_levels":[{"id":12,"access_level":40}]}`)) + case r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "/api/v4/projects/1/protected_branches/main"): + w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/api/v4/projects/1/protected_branches"): + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"500 Internal Server Error"}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + }) + + cfg := &config.AssignmentConfig{ + Branches: []config.BranchRule{{Name: "main", MergeOnly: true, Default: true}}, + } + project := &gitlabapi.Project{ID: 1, Name: "repo"} + + err := client.syncConfiguredBranches(cfg, project, "main") + if err == nil { + t.Fatal("syncConfiguredBranches() expected error when branch protection fails, got nil") + } +} diff --git a/gitlab/protect.go b/gitlab/protect.go index 91522bd..7c56efc 100644 --- a/gitlab/protect.go +++ b/gitlab/protect.go @@ -119,6 +119,15 @@ func (c *Client) protectSingleBranch( ) error { existing, _, err := c.ProtectedBranches.GetProtectedBranch(project.ID, branch.Name) if err == nil { + // GitLab can keep stale push permissions when updating existing rules to + // "No one" (merge-only). Recreate the rule to enforce the access levels. + if pushAccessLevel == gitlab.NoPermissions { + if err := c.recreateProtectedBranch(project, branch, pushAccessLevel, mergeAccessLevel); err != nil { + return err + } + return nil + } + updateOpts := &gitlab.UpdateProtectedBranchOptions{ AllowedToPush: replaceBranchPermissions(existing.PushAccessLevels, pushAccessLevel), AllowedToMerge: replaceBranchPermissions(existing.MergeAccessLevels, mergeAccessLevel), @@ -149,6 +158,29 @@ func (c *Client) protectSingleBranch( return fmt.Errorf("error while trying to read protected branch %s: %w", branch.Name, err) } + if err := c.recreateProtectedBranch(project, branch, pushAccessLevel, mergeAccessLevel); err != nil { + return err + } + + return nil +} + +func (c *Client) recreateProtectedBranch( + project *gitlab.Project, + branch config.BranchRule, + pushAccessLevel gitlab.AccessLevelValue, + mergeAccessLevel gitlab.AccessLevelValue, +) error { + _, err := c.ProtectedBranches.UnprotectRepositoryBranches(project.ID, branch.Name) + if err != nil && !isProtectedBranchNotFoundError(err) { + log.Debug().Err(err). + Str("name", project.Name). + Str("toURL", project.SSHURLToRepo). + Str("branch", branch.Name). + Msg("cannot unprotect branch") + return fmt.Errorf("error while trying to unprotect branch %s: %w", branch.Name, err) + } + opts := &gitlab.ProtectRepositoryBranchesOptions{ Name: gitlab.Ptr(branch.Name), PushAccessLevel: gitlab.Ptr(pushAccessLevel), diff --git a/gitlab/protect_contract_test.go b/gitlab/protect_contract_test.go index 2264736..7ed7aee 100644 --- a/gitlab/protect_contract_test.go +++ b/gitlab/protect_contract_test.go @@ -367,6 +367,45 @@ func TestProtectSingleBranch_Success(t *testing.T) { } } +func TestProtectSingleBranch_MergeOnly_RecreatesExistingRule(t *testing.T) { + deleteCalled := false + postCalled := false + patchCalled := false + + client := newContractClient(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "protected_branches"): + _, _ = w.Write([]byte(`{"id":1,"name":"main","push_access_levels":[{"id":10,"access_level":40}],"merge_access_levels":[{"id":11,"access_level":40}],"unprotect_access_levels":[{"id":12,"access_level":40}]}`)) + case r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "protected_branches"): + deleteCalled = true + w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "protected_branches"): + postCalled = true + _, _ = w.Write([]byte(`{"id":1,"name":"main"}`)) + case r.Method == http.MethodPatch && strings.Contains(r.URL.Path, "protected_branches"): + patchCalled = true + _, _ = w.Write([]byte(`{"id":1,"name":"main"}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + }) + + project := &gitlabapi.Project{ID: 1, Name: "myrepo"} + err := client.protectSingleBranch(project, config.BranchRule{Name: "main", MergeOnly: true}, gitlabapi.NoPermissions, gitlabapi.DeveloperPermissions) + if err != nil { + t.Fatalf("protectSingleBranch() error = %v", err) + } + if !deleteCalled { + t.Fatal("protectSingleBranch() did not unprotect existing branch before recreating merge-only rule") + } + if !postCalled { + t.Fatal("protectSingleBranch() did not create merge-only protected branch rule") + } + if patchCalled { + t.Fatal("protectSingleBranch() should not patch existing protected branch for merge-only rule") + } +} + func TestProtectSingleBranch_GetNotFound_ProtectStillCalled(t *testing.T) { // Get returns 404 (branch not yet protected) -> protectSingleBranch creates the rule. protectCalled := false