Org-level branch protection is stored but never enforced (no enforcement/materialization) #727
Priority
Medium
Type
Feature
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: MokoConsulting/MokoGIT#727
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
The org-level branch protection feature is half-implemented: it has a DB model, migration, and full CRUD API, but nothing consumes the rules, so creating an org-level rule has zero effect on pushes or merges.
What exists
models/git/org_protected_branch.go(OrgProtectedBranch, team-based whitelists)models/migrations/v1_27/v332.go/orgs/{org}/branch_protections— list/create/get/edit/delete (routers/api/v1/org/branch_protection.go, routes inrouters/api/v1/api.gounder/orgs/{org})OrgProtectedBranch.Match()(:75),OrgProtectedBranch.ToProtectedBranch()(:86),FindOrgBranchRuleForBranch()(:186)The gap (evidence)
grep -rl OrgProtectedBranch --include=*.goreturns only 4 files — the model, the migration (×2), and the API handler. Nothing else references it.Meanwhile, all actual enforcement reads per-repo rules only:
routers/private/hook_pre_receive.go:194→GetFirstMatchProtectedBranchRule(ctx, repo.ID, branchName)services/pull/merge.go:557,575,check.go:199,240,patch.go:430,commit_status.go:70,129→ allGetFirstMatchProtectedBranchRule(ctx, repo.ID/pr.BaseRepoID, ...)None of these consult org rules, and
ToProtectedBranch()/FindOrgBranchRuleForBranch()are defined but never called from any enforcement or sync path.Result: org-level rules are inert. Org-wide protection is currently only achievable by the external
branch-protection.ymlprovisioning workflow (mokocli), which writes real per-repo rules and re-asserts them weekly.Fix options
Option 1 — Enforce at check time (highest fidelity):
In the pre-receive hook and
services/pullchecks, after fetching the repo rule, also look up the matching org rule (FindOrgBranchRuleForBranch(orgID, branch)) and merge them (repo rule overrides, or most-restrictive wins). Requires touching every enforcement site and defining precedence.Option 2 — Materialize org rules into per-repo records (simpler, recommended):
On org-rule create/update/delete (and on repo creation / org membership change), call the existing
ToProtectedBranch()to upsert matching per-repoProtectedBranchrows. Enforcement then works unchanged because it already reads per-repo rules. This effectively makes the native org feature replace the external provisioning workflow, with the same "materialized per-repo" model but a native trigger instead of a weekly cron.Recommendation
Ship Option 2 — least invasive, reuses
ToProtectedBranch(), and lets thebranch-protection.ymlcron be retired once it's live. Add a one-time backfill that materializes existing org rules across all repos.Filed from a workflow/dev-cycle audit; related: mcp-mokogitea-api#30 (bot allowlists), Template-Generic#53 (workflow correctness).
Branch created:
feature/727-org-level-branch-protection-is-stored-buImplemented Option 2 (materialize) in PR #728 (
fix/727-materialize-org-branch-protection→main).Each org rule is written into every repo of the org as a per-repo
ProtectedBranchrow tagged with a newOrgRuleIDcolumn, so the existing enforcement path (pre-receive hook,services/pullchecks) picks it up unchanged.OrgRuleIDcolumn + migration 362; helpersDeleteMaterializedProtectedBranches/HasMaterializedOrgRule/FindAllOrgProtectedBranchRules.services/org/branch_protection.go):SyncOrgProtectedBranch(delete-then-rematerialize, handles pattern renames),RemoveOrgProtectedBranch,SyncOrgRulesToRepo, guarded idempotentBackfillOrgProtectedBranches.services/repository → services/orgimport cycle); one-time startup backfill for pre-existing rules.Once live this can retire the external
branch-protection.ymlprovisioning cron.⚠️ Not compiled/tested locally (no Go toolchain in the authoring env) — CI must validate build +
gofmt+ tests before merge; integration testing of actual enforcement recommended.Approach revised — reworked PR #728 to layering (most-restrictive wins).
Two findings changed the plan:
Org rules are not fully inert. The single enforcement choke point
GetFirstMatchProtectedBranchRule(models/git/protected_branch_list.go) already consults org rules viaFindOrgBranchRuleForBranch/ToProtectedBranch. Thegrep OrgProtectedBranchin the issue missed it because those are function calls that never name the type. So org protection already worked — but only as a fallback: any matching repo rule shadowed the org rule entirely, letting a repo opt out.Layering is therefore a one-function change, not materialization. The materialization approach (per-repo rows, notifier, backfill,
OrgRuleIDcolumn) was discarded.Implemented:
GetFirstMatchProtectedBranchRulenow merges the repo rule and org rule when both match, via a new fail-closedmergeMostRestrictive(models/git/protected_branch_merge.go): allow flags AND'd, gate/require/block flags OR'd, required approvals max'd, status contexts + protected files unioned, whitelists intersected. The org rule becomes a floor a repo can only tighten. Because all ~15 enforcement sites funnel through this choke point, none needed editing.Diff is now 2 files (+207/−7). ⚠️ Still not compiled/tested locally (no Go toolchain) — CI must validate; integration testing of layered enforcement recommended.
Note: the issue's stated Option 2 (materialize) is superseded; this is effectively Option 1 (enforce-at-check-time) done at the pre-existing choke point.