Skip to content

fix: branch protection for mergeOnly #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
obcode merged 1 commit into from
Apr 25, 2026
Merged

fix: branch protection for mergeOnly #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions gitlab/branches.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
38 changes: 38 additions & 0 deletions gitlab/branches_contract_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"):
Comment on lines +15 to +17
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mock server currently only treats PATCH /api/v4/projects/1 as a success. Since this test’s goal is to reach the protection step, consider making this handler accept whatever HTTP method Projects.EditProject actually uses (or at least fail the test if an unexpected method hits this endpoint) so the test can’t silently pass/fail due to a mock mismatch.

Copilot uses AI. Check for mistakes.
_, _ = 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")
}
Comment on lines +12 to +37
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test only asserts err != nil, so it can pass even if syncConfiguredBranches fails earlier (e.g., default-branch update) and never reaches the branch-protection code path you intend to validate. Tighten the assertion to ensure the error is coming from the protection step (e.g., check that the error message includes the new "error while protecting configured branches" wrapper and/or that the POST /protected_branches endpoint was actually hit).

Copilot uses AI. Check for mistakes.
}
32 changes: 32 additions & 0 deletions gitlab/protect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment on lines +122 to +129
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the merge-only path, protectSingleBranch always unprotects and re-protects whenever pushAccessLevel == gitlab.NoPermissions, even if the existing protected-branch rule is already correctly configured. This introduces a small window where the branch is briefly unprotected (race risk) and adds extra API calls. Consider only recreating when the existing rule’s push access levels don’t already represent “no one”, or try an update first and fall back to recreate only if GitLab leaves stale push permissions.

Copilot uses AI. Check for mistakes.

updateOpts := &gitlab.UpdateProtectedBranchOptions{
AllowedToPush: replaceBranchPermissions(existing.PushAccessLevels, pushAccessLevel),
AllowedToMerge: replaceBranchPermissions(existing.MergeAccessLevels, mergeAccessLevel),
Expand Down Expand Up @@ -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
Comment on lines +161 to +165
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the GET-404 path, protectSingleBranch calls recreateProtectedBranch, which performs a DELETE (unprotect) before POST (protect). When the branch isn’t protected yet, this DELETE is unnecessary and adds an extra API round-trip. Consider calling ProtectRepositoryBranches directly in the not-found case and reserving the delete+recreate logic for the “existing rule but needs reset” scenario.

Copilot uses AI. Check for mistakes.
}

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),
Expand Down
39 changes: 39 additions & 0 deletions gitlab/protect_contract_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading