Commit c1d0a3afb0
Verified · cmc ci/build: success ci/test: success
.gitbay/wiki/Users.org +8
| @@ -192,6 +192,7 @@ Access and settings (owner or =admin= grant): | ||
| 192 | 192 | gitbay repo access grant you/project alice write # read | write | admin |
| 193 | 193 | gitbay repo access revoke you/project alice |
| 194 | 194 | gitbay repo settings protect you/project main # no force-push, no delete |
| 195 | gitbay repo settings require-mr you/project on # protected branches: merge requests only | |
| 195 | 196 | gitbay repo settings default-branch you/project trunk # HEAD, and what the web shows |
| 196 | 197 | gitbay repo settings require-signed you/project on # every commit must verify |
| 197 | 198 | gitbay repo settings git-daemon you/project on # expose over git:// |
| @@ -397,6 +398,13 @@ to ask without it gating merges; with it on and no file on the target | ||
| 397 | 398 | branch, the merge is refused and says so. It does not wait on |
| 398 | 399 | =require-approvals=. |
| 399 | 400 | |
| 401 | Those gates apply to =mr merge=. A direct push to a protected branch | |
| 402 | passes none of them until =require-mr on=: then an existing protected | |
| 403 | branch refuses every push, including =repo commit-file= and the web | |
| 404 | editor, and the server's merge is its only writer. Creating the branch | |
| 405 | is still a push, since there is nothing to open a merge request against | |
| 406 | yet. | |
| 407 | ||
| 400 | 408 | A merge request whose target is another open merge request's source |
| 401 | 409 | branch is stacked on it: =mr create= says so, =mr show= carries |
| 402 | 410 | =stacked_on= and =stacked=, and merging the lower one retargets the |
cmd/gitbay/main.go +1
| @@ -479,6 +479,7 @@ func repoCmd() *cobra.Command { | ||
| 479 | 479 | pass("require-checks", "gate merges on green statuses: ... on|off", passOpts{server: []string{"repo", "settings", "require-checks"}, needsRepo: true}), |
| 480 | 480 | pass("visibility", "set repository visibility: public|private", passOpts{server: []string{"repo", "settings", "visibility"}, needsRepo: true}), |
| 481 | 481 | pass("require-signed", "require verified commit signatures: ... on|off", passOpts{server: []string{"repo", "settings", "require-signed"}, needsRepo: true}), |
| 482 | pass("require-mr", "protected branches take changes through merge requests only: on|off", passOpts{server: []string{"repo", "settings", "require-mr"}, needsRepo: true}), | |
| 482 | 483 | pass("description", "set the repository description: <text>", passOpts{server: []string{"repo", "settings", "description"}, needsRepo: true}), |
| 483 | 484 | pass("website", "set the repository website: <url> ('' clears)", passOpts{server: []string{"repo", "settings", "website"}, needsRepo: true}), |
| 484 | 485 | pass("git-daemon", "expose over git://: ... on|off", passOpts{server: []string{"repo", "settings", "git-daemon"}, needsRepo: true}), |
e2e/requiremr_test.go added +83
| @@ -0,0 +1,83 @@ | ||
| 1 | package e2e | |
| 2 | ||
| 3 | import ( | |
| 4 | "os" | |
| 5 | "path/filepath" | |
| 6 | "strings" | |
| 7 | "testing" | |
| 8 | ) | |
| 9 | ||
| 10 | // With require-mr on, a protected branch takes changes through mr merge | |
| 11 | // only: direct pushes and commit-file are refused, a branch that does | |
| 12 | // not exist yet can still be created, and unprotected branches are | |
| 13 | // unaffected (#197). | |
| 14 | func TestRequireMR(t *testing.T) { | |
| 15 | inst := startInstance(t) | |
| 16 | aliceKey := inst.newKey(t, "alice") | |
| 17 | inst.admin(t, "admin", "user", "create", "alice", | |
| 18 | "--key", aliceKey+".pub", "--email", "alice@example.test", "--verified") | |
| 19 | if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "create", "alice/lib"); code != 0 { | |
| 20 | t.Fatalf("repo create: %s", errOut) | |
| 21 | } | |
| 22 | env := inst.gitEnv(aliceKey) | |
| 23 | work := t.TempDir() | |
| 24 | mustGit(t, work, env, "clone", "-q", inst.sshURL("alice/lib"), "lib") | |
| 25 | dir := filepath.Join(work, "lib") | |
| 26 | write := func(name, content string) { | |
| 27 | t.Helper() | |
| 28 | if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil { | |
| 29 | t.Fatal(err) | |
| 30 | } | |
| 31 | mustGit(t, dir, env, "add", name) | |
| 32 | mustGit(t, dir, env, "commit", "-q", "-m", name) | |
| 33 | } | |
| 34 | mustGit(t, dir, env, "checkout", "-q", "-b", "main") | |
| 35 | write("README", "one\n") | |
| 36 | mustGit(t, dir, env, "push", "-q", "origin", "main") | |
| 37 | ||
| 38 | for _, args := range [][]string{ | |
| 39 | {"repo", "settings", "protect", "alice/lib", "main"}, | |
| 40 | {"repo", "settings", "protect", "alice/lib", "release"}, | |
| 41 | {"repo", "settings", "require-mr", "alice/lib", "on"}, | |
| 42 | } { | |
| 43 | if _, errOut, code := inst.ssh(t, aliceKey, "", args...); code != 0 { | |
| 44 | t.Fatalf("%v: %s", args, errOut) | |
| 45 | } | |
| 46 | } | |
| 47 | out, _, _ := inst.ssh(t, aliceKey, "", "repo", "settings", "show", "alice/lib", "--json") | |
| 48 | if !strings.Contains(out, `"require_mr":true`) { | |
| 49 | t.Fatalf("settings show: %s", out) | |
| 50 | } | |
| 51 | ||
| 52 | // Direct push to main: refused, with the reason. | |
| 53 | write("two", "two\n") | |
| 54 | if out, code := gitRun(t, dir, env, "push", "origin", "main"); code == 0 || !strings.Contains(out, "merge requests only") { | |
| 55 | t.Fatalf("direct push to protected branch: %d\n%s", code, out) | |
| 56 | } | |
| 57 | if _, errOut, code := inst.ssh(t, aliceKey, "x\n", "repo", "commit-file", "alice/lib", "notes.txt", | |
| 58 | "--ref", "main", "--file", "-"); code != 4 || !strings.Contains(errOut, "merge requests only") { | |
| 59 | t.Fatalf("commit-file to protected branch: %d %s", code, errOut) | |
| 60 | } | |
| 61 | // Creating a protected branch is still a push; so is an unprotected one. | |
| 62 | mustGit(t, dir, env, "push", "-q", "origin", "main:release") | |
| 63 | mustGit(t, dir, env, "push", "-q", "origin", "main:feature") | |
| 64 | ||
| 65 | // The same change lands through a merge request. | |
| 66 | if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "create", "alice/lib", | |
| 67 | "--source", "feature", "--target", "main", "--title", "'two'"); code != 0 { | |
| 68 | t.Fatalf("mr create: %s", errOut) | |
| 69 | } | |
| 70 | if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "merge", "alice/lib", "1", "--strategy", "ff"); code != 0 { | |
| 71 | t.Fatalf("mr merge: %s", errOut) | |
| 72 | } | |
| 73 | if out, _, _ := inst.ssh(t, aliceKey, "", "repo", "log", "alice/lib", "--limit", "1"); !strings.Contains(out, "two") { | |
| 74 | t.Fatalf("merge did not move main: %s", out) | |
| 75 | } | |
| 76 | ||
| 77 | // Off again, and the push goes through. | |
| 78 | if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "settings", "require-mr", "alice/lib", "off"); code != 0 { | |
| 79 | t.Fatalf("require-mr off: %s", errOut) | |
| 80 | } | |
| 81 | write("three", "three\n") | |
| 82 | mustGit(t, dir, env, "push", "-q", "origin", "main") | |
| 83 | } | |
internal/control/commitfile.go +4
| @@ -3,6 +3,7 @@ package control | ||
| 3 | 3 | import ( |
| 4 | 4 | "fmt" |
| 5 | 5 | "io" |
| 6 | "slices" | |
| 6 | 7 | "strings" |
| 7 | 8 | |
| 8 | 9 | "gitbay.org/gitbay/internal/gitutil" |
| @@ -55,6 +56,9 @@ func runCommitFile(c *Ctx, args []string) int { | ||
| 55 | 56 | if !ok || filePath == "" { |
| 56 | 57 | return c.fail(protocol.ExitUsage, "path must stay inside the repository") |
| 57 | 58 | } |
| 59 | if repo.Settings.RequireMR && slices.Contains(repo.Settings.ProtectedBranches, ref) { | |
| 60 | return c.fail(protocol.ExitDenied, "branch %s accepts changes through merge requests only", ref) | |
| 61 | } | |
| 58 | 62 | // The server authors this commit, so it cannot sign it. |
| 59 | 63 | if repo.Settings.RequireSignedCommits { |
| 60 | 64 | return c.fail(protocol.ExitDenied, |
internal/control/mr.go +20
| @@ -32,6 +32,9 @@ func init() { | ||
| 32 | 32 | register(Command{Path: []string{"repo", "settings", "require-checks"}, |
| 33 | 33 | Summary: "gate merges on green statuses", |
| 34 | 34 | Usage: "repo settings require-checks <owner/name> on|off", Run: runRequireChecks}) |
| 35 | register(Command{Path: []string{"repo", "settings", "require-mr"}, | |
| 36 | Summary: "protected branches take changes through merge requests only", | |
| 37 | Usage: "repo settings require-mr <owner/name> on|off", Run: runRequireMR}) | |
| 35 | 38 | register(Command{Path: []string{"repo", "settings", "require-signed"}, |
| 36 | 39 | Summary: "require verified commit signatures", |
| 37 | 40 | Usage: "repo settings require-signed <owner/name> on|off", Run: runRequireSigned}) |
| @@ -217,6 +220,23 @@ func runRequireChecks(c *Ctx, args []string) int { | ||
| 217 | 220 | }) |
| 218 | 221 | } |
| 219 | 222 | |
| 223 | func runRequireMR(c *Ctx, args []string) int { | |
| 224 | if len(args) != 2 || (args[1] != "on" && args[1] != "off") { | |
| 225 | return c.fail(protocol.ExitUsage, "usage: repo settings require-mr <owner/name> on|off") | |
| 226 | } | |
| 227 | repo, code := resolveRepo(c, args[0], policy.CanAdmin) | |
| 228 | if code >= 0 { | |
| 229 | return code | |
| 230 | } | |
| 231 | s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.RequireMR = args[1] == "on" }) | |
| 232 | if err != nil { | |
| 233 | return c.fail(protocol.ExitFailure, "%v", err) | |
| 234 | } | |
| 235 | return c.emit(s, func(w io.Writer) { | |
| 236 | fmt.Fprintf(w, "require_mr %s on %s\n", args[1], repo.Path()) | |
| 237 | }) | |
| 238 | } | |
| 239 | ||
| 220 | 240 | func runRequireSigned(c *Ctx, args []string) int { |
| 221 | 241 | if len(args) != 2 || (args[1] != "on" && args[1] != "off") { |
| 222 | 242 | return c.fail(protocol.ExitUsage, "usage: repo settings require-signed <owner/name> on|off") |
internal/control/repo.go +2 −2
| @@ -606,8 +606,8 @@ func runSettingsShow(c *Ctx, args []string) int { | ||
| 606 | 606 | return code |
| 607 | 607 | } |
| 608 | 608 | return c.emit(repo.Settings, func(w io.Writer) { |
| 609 | fmt.Fprintf(w, "protected_branches: %s\nrequire_signed_commits: %v\ngit_daemon: %v\narchived: %v\n", | |
| 610 | strings.Join(repo.Settings.ProtectedBranches, ", "), repo.Settings.RequireSignedCommits, repo.Settings.GitDaemon, repo.Settings.Archived) | |
| 609 | fmt.Fprintf(w, "protected_branches: %s\nrequire_mr: %v\nrequire_signed_commits: %v\ngit_daemon: %v\narchived: %v\n", | |
| 610 | strings.Join(repo.Settings.ProtectedBranches, ", "), repo.Settings.RequireMR, repo.Settings.RequireSignedCommits, repo.Settings.GitDaemon, repo.Settings.Archived) | |
| 611 | 611 | }) |
| 612 | 612 | } |
| 613 | 613 | |
internal/httpd/settings.go +2
| @@ -78,6 +78,8 @@ func (s *Server) settingsSubmit(w http.ResponseWriter, r *http.Request, u store. | ||
| 78 | 78 | argv = []string{"repo", "settings", "require-resolved", repo, onOff(v("require-resolved"))} |
| 79 | 79 | case "require-codeowners": |
| 80 | 80 | argv = []string{"repo", "settings", "require-codeowners", repo, onOff(v("require-codeowners"))} |
| 81 | case "require-mr": | |
| 82 | argv = []string{"repo", "settings", "require-mr", repo, onOff(v("require-mr"))} | |
| 81 | 83 | case "require-signed": |
| 82 | 84 | argv = []string{"repo", "settings", "require-signed", repo, onOff(v("require-signed"))} |
| 83 | 85 | case "require-approvals": |
internal/policy/access.go +8
| @@ -106,7 +106,15 @@ func CheckPush(repo store.Repo, updates []RefUpdate) string { | ||
| 106 | 106 | if u.IsForce { |
| 107 | 107 | return "branch " + branch + " is protected: force-push refused" |
| 108 | 108 | } |
| 109 | // Under require_mr the server's merge is the only writer of an | |
| 110 | // existing protected branch. Creating one is still a push: | |
| 111 | // there is nothing to route a merge request into yet. | |
| 112 | if repo.Settings.RequireMR && !isZeroSHA(u.Old) { | |
| 113 | return "branch " + branch + " accepts changes through merge requests only" | |
| 114 | } | |
| 109 | 115 | } |
| 110 | 116 | } |
| 111 | 117 | return "" |
| 112 | 118 | } |
| 119 | ||
| 120 | func isZeroSHA(sha string) bool { return sha != "" && strings.Trim(sha, "0") == "" } | |
internal/policy/access_test.go +29
| @@ -1,6 +1,7 @@ | ||
| 1 | 1 | package policy |
| 2 | 2 | |
| 3 | 3 | import ( |
| 4 | "strings" | |
| 4 | 5 | "testing" |
| 5 | 6 | |
| 6 | 7 | "gitbay.org/gitbay/internal/store" |
| @@ -110,3 +111,31 @@ func TestCheckPush(t *testing.T) { | ||
| 110 | 111 | }) |
| 111 | 112 | } |
| 112 | 113 | } |
| 114 | ||
| 115 | // With require_mr, a protected branch takes no direct push once it | |
| 116 | // exists; creating it and pushing elsewhere are unaffected (#197). | |
| 117 | func TestCheckPushRequireMR(t *testing.T) { | |
| 118 | repo := store.Repo{Settings: store.RepoSettings{ProtectedBranches: []string{"main"}, RequireMR: true}} | |
| 119 | const zero = "0000000000000000000000000000000000000000" | |
| 120 | cases := []struct { | |
| 121 | name string | |
| 122 | updates []RefUpdate | |
| 123 | denied bool | |
| 124 | }{ | |
| 125 | {"update protected", []RefUpdate{{Ref: "refs/heads/main", Old: "abc", New: "def"}}, true}, | |
| 126 | {"create protected", []RefUpdate{{Ref: "refs/heads/main", Old: zero, New: "def"}}, false}, | |
| 127 | {"delete protected", []RefUpdate{{Ref: "refs/heads/main", Old: "abc", New: zero, IsDelete: true}}, true}, | |
| 128 | {"update unprotected", []RefUpdate{{Ref: "refs/heads/dev", Old: "abc", New: "def"}}, false}, | |
| 129 | } | |
| 130 | for _, tc := range cases { | |
| 131 | t.Run(tc.name, func(t *testing.T) { | |
| 132 | msg := CheckPush(repo, tc.updates) | |
| 133 | if (msg != "") != tc.denied { | |
| 134 | t.Errorf("CheckPush = %q, denied should be %v", msg, tc.denied) | |
| 135 | } | |
| 136 | }) | |
| 137 | } | |
| 138 | if msg := CheckPush(repo, cases[0].updates); !strings.Contains(msg, "merge requests only") { | |
| 139 | t.Errorf("message %q", msg) | |
| 140 | } | |
| 141 | } | |
internal/store/repos.go +1
| @@ -28,6 +28,7 @@ type RepoSettings struct { | ||
| 28 | 28 | RequireApprovals int `json:"require_approvals,omitempty"` |
| 29 | 29 | RequireResolved bool `json:"require_resolved,omitempty"` |
| 30 | 30 | RequireCodeowners bool `json:"require_codeowners,omitempty"` |
| 31 | RequireMR bool `json:"require_mr,omitempty"` | |
| 31 | 32 | GitDaemon bool `json:"git_daemon,omitempty"` |
| 32 | 33 | Archived bool `json:"archived,omitempty"` |
| 33 | 34 | Website string `json:"website,omitempty"` |
internal/web/templates/settings.html +7
| @@ -105,6 +105,13 @@ | ||
| 105 | 105 | </select> |
| 106 | 106 | <button type="submit">Protect</button> |
| 107 | 107 | </form> |
| 108 | <form method="post" action="{{$base}}" class="setform"> | |
| 109 | <input type="hidden" name="field" value="require-mr"> | |
| 110 | <label for="require-mr">Merge requests only</label> | |
| 111 | <input type="checkbox" id="require-mr" name="require-mr" value="on"{{if .Repo.Settings.RequireMR}} checked{{end}}> | |
| 112 | <button type="submit">Save</button> | |
| 113 | </form> | |
| 114 | <p class="meta">With merge requests only, a protected branch refuses every direct push once it exists; the merge gates above are then what a change has to pass.</p> | |
| 108 | 115 | |
| 109 | 116 | <h2>Dependencies</h2> |
| 110 | 117 | <form method="post" action="{{$base}}" class="setform"> |