diff --git a/modelmigration/migrations.go b/modelmigration/migrations.go index 32ab0f9586..d7eda29fb1 100644 --- a/modelmigration/migrations.go +++ b/modelmigration/migrations.go @@ -425,6 +425,7 @@ func prepareMigrationTasks() []*migration { newMigration(343, "Add max_parallel column to action_run_job", v1_28.AddMaxParallelToActionRunJob), newMigration(344, "Add deferred-matrix columns to ActionRunJob", v1_28.AddDeferredMatrixColumnsToActionRunJob), + newMigration(345, "Add block on CODEOWNERS reviews branch protection", v1_28.AddBlockOnCodeownerReviews), } return preparedMigrations } diff --git a/modelmigration/v1_28/v345.go b/modelmigration/v1_28/v345.go new file mode 100644 index 0000000000..914e2940b0 --- /dev/null +++ b/modelmigration/v1_28/v345.go @@ -0,0 +1,23 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_28 + +import ( + "gitea.dev/modelmigration/base" + + "xorm.io/xorm" +) + +// AddBlockOnCodeownerReviews adds block on codeowner reviews branch protection +func AddBlockOnCodeownerReviews(x base.EngineMigration) error { + type ProtectedBranch struct { + BlockOnCodeownerReviews bool `xorm:"NOT NULL DEFAULT false"` + } + + _, err := x.SyncWithOptions(xorm.SyncOptions{ + IgnoreConstrains: true, + IgnoreDropIndices: true, + }, new(ProtectedBranch)) + return err +} diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 5f5ab13f9d..9eed8bb013 100644 --- a/models/git/protected_branch.go +++ b/models/git/protected_branch.go @@ -59,6 +59,7 @@ type ProtectedBranch struct { RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"` BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"` BlockOnOfficialReviewRequests bool `xorm:"NOT NULL DEFAULT false"` + BlockOnCodeownerReviews bool `xorm:"NOT NULL DEFAULT false"` BlockOnOutdatedBranch bool `xorm:"NOT NULL DEFAULT false"` DismissStaleApprovals bool `xorm:"NOT NULL DEFAULT false"` IgnoreStaleApprovals bool `xorm:"NOT NULL DEFAULT false"` diff --git a/modules/structs/repo_branch.go b/modules/structs/repo_branch.go index fc84ab364e..2b931c032b 100644 --- a/modules/structs/repo_branch.go +++ b/modules/structs/repo_branch.go @@ -61,6 +61,7 @@ type BranchProtection struct { ApprovalsWhitelistTeams []string `json:"approvals_whitelist_teams"` BlockOnRejectedReviews bool `json:"block_on_rejected_reviews"` BlockOnOfficialReviewRequests bool `json:"block_on_official_review_requests"` + BlockOnCodeownerReviews bool `json:"block_on_codeowner_reviews"` BlockOnOutdatedBranch bool `json:"block_on_outdated_branch"` DismissStaleApprovals bool `json:"dismiss_stale_approvals"` IgnoreStaleApprovals bool `json:"ignore_stale_approvals"` @@ -104,6 +105,7 @@ type CreateBranchProtectionOption struct { ApprovalsWhitelistTeams []string `json:"approvals_whitelist_teams"` BlockOnRejectedReviews bool `json:"block_on_rejected_reviews"` BlockOnOfficialReviewRequests bool `json:"block_on_official_review_requests"` + BlockOnCodeownerReviews bool `json:"block_on_codeowner_reviews"` BlockOnOutdatedBranch bool `json:"block_on_outdated_branch"` DismissStaleApprovals bool `json:"dismiss_stale_approvals"` IgnoreStaleApprovals bool `json:"ignore_stale_approvals"` @@ -140,6 +142,7 @@ type EditBranchProtectionOption struct { ApprovalsWhitelistTeams []string `json:"approvals_whitelist_teams"` BlockOnRejectedReviews *bool `json:"block_on_rejected_reviews"` BlockOnOfficialReviewRequests *bool `json:"block_on_official_review_requests"` + BlockOnCodeownerReviews *bool `json:"block_on_codeowner_reviews"` BlockOnOutdatedBranch *bool `json:"block_on_outdated_branch"` DismissStaleApprovals *bool `json:"dismiss_stale_approvals"` IgnoreStaleApprovals *bool `json:"ignore_stale_approvals"` diff --git a/options/locale/locale_en-US.json b/options/locale/locale_en-US.json index df4086bd5c..6ac76d8d45 100644 --- a/options/locale/locale_en-US.json +++ b/options/locale/locale_en-US.json @@ -1830,6 +1830,7 @@ "repo.pulls.blocked_by_outdated_branch": "This pull request is blocked because it's outdated.", "repo.pulls.blocked_by_changed_protected_files_1": "This pull request is blocked because it changes a protected file:", "repo.pulls.blocked_by_changed_protected_files_n": "This pull request is blocked because it changes protected files:", + "repo.pulls.blocked_by_codeowners": "This pull request is missing approval from one or more code owners.", "repo.pulls.can_auto_merge_desc": "This pull request can be merged automatically.", "repo.pulls.cannot_auto_merge_desc": "This pull request cannot be merged automatically due to conflicts.", "repo.pulls.cannot_auto_merge_helper": "Merge manually to resolve the conflicts.", @@ -2463,6 +2464,8 @@ "repo.settings.block_on_official_review_requests_desc": "Merging will not be possible when it has official review requests, even if there are enough approvals.", "repo.settings.block_outdated_branch": "Block merge if pull request is outdated", "repo.settings.block_outdated_branch_desc": "Merging will not be possible when head branch is behind base branch.", + "repo.settings.block_on_codeowner_reviews": "Require approval from code owners", + "repo.settings.block_on_codeowner_reviews_desc": "Merging will only be possible if at least one code owner per code owner rule has given an approving review.", "repo.settings.block_admin_merge_override": "Administrators must follow branch protection rules", "repo.settings.block_admin_merge_override_desc": "Administrators must follow branch protection rules and cannot circumvent it. Users or teams in the bypass allowlist can still bypass these rules if bypass allowlist is enabled.", "repo.settings.default_branch_desc": "Select a default branch for code commits.", diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 08a838f265..521013541e 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -797,6 +797,7 @@ func CreateBranchProtection(ctx *context.APIContext) { RequiredApprovals: requiredApprovals, BlockOnRejectedReviews: form.BlockOnRejectedReviews, BlockOnOfficialReviewRequests: form.BlockOnOfficialReviewRequests, + BlockOnCodeownerReviews: form.BlockOnCodeownerReviews, DismissStaleApprovals: form.DismissStaleApprovals, IgnoreStaleApprovals: form.IgnoreStaleApprovals, RequireSignedCommits: form.RequireSignedCommits, @@ -965,6 +966,10 @@ func EditBranchProtection(ctx *context.APIContext) { protectBranch.BlockOnOfficialReviewRequests = *form.BlockOnOfficialReviewRequests } + if form.BlockOnCodeownerReviews != nil { + protectBranch.BlockOnCodeownerReviews = *form.BlockOnCodeownerReviews + } + if form.DismissStaleApprovals != nil { protectBranch.DismissStaleApprovals = *form.DismissStaleApprovals } diff --git a/routers/web/repo/issue_view.go b/routers/web/repo/issue_view.go index db9830d450..84c501c017 100644 --- a/routers/web/repo/issue_view.go +++ b/routers/web/repo/issue_view.go @@ -956,11 +956,9 @@ func (prInfo *pullRequestViewInfo) prepareMergeBox(ctx *context.Context, issue * // so block on any required status context, not only when enableStatusCheck is on. data.hasStatusCheckBlocker = (data.enableStatusCheck || data.hasRequiredStatusContexts) && !data.StatusCheckData.RequiredChecksState.IsSuccess() - // this logic is from: - // {{$notAllOverridableChecksOk := or .IsBlockedByApprovals .IsBlockedByRejection .IsBlockedByOfficialReviewRequests .IsBlockedByOutdatedBranch .IsBlockedByChangedProtectedFiles (and .EnableStatusCheck (not $requiredStatusCheckState.IsSuccess))}} - // HINT: if a PR's status is not mergeable, then it is a non-overridable blocker, such logic is handled separately (see IsStatusMergeable) data.hasOverridableBlockers = data.isBlockedByApprovals || data.isBlockedByRejection || - data.isBlockedByOfficialReviewRequests || data.isBlockedByOutdatedBranch || data.isBlockedByChangedProtectedFiles || + data.isBlockedByOfficialReviewRequests || data.isBlockedByCodeowners || + data.isBlockedByOutdatedBranch || data.isBlockedByChangedProtectedFiles || data.hasStatusCheckBlocker data.canBypassProtection = isRepoAdmin @@ -1071,6 +1069,11 @@ func (prInfo *pullRequestViewInfo) prepareMergeBoxProtectedRules(ctx *context.Co data.infoProtectionBlockers.AddErrorItem(ctx.Locale.Tr("repo.pulls.blocked_by_official_review_requests")) } + data.isBlockedByCodeowners = !issue_service.HasAllRequiredCodeownerReviews(ctx, pb, pull) + if data.isBlockedByCodeowners { + data.infoProtectionBlockers.AddErrorItem(ctx.Locale.Tr("repo.pulls.blocked_by_codeowners")) + } + data.isBlockedByOutdatedBranch = issues_model.MergeBlockedByOutdatedBranch(pb, pull) if data.isBlockedByOutdatedBranch { data.infoProtectionBlockers.AddErrorItem(ctx.Locale.Tr("repo.pulls.blocked_by_outdated_branch")) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 4d8bfeb7d1..70e6e99d0e 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -297,6 +297,7 @@ type pullMergeBoxData struct { isBlockedByApprovals bool isBlockedByRejection bool isBlockedByOfficialReviewRequests bool + isBlockedByCodeowners bool isBlockedByOutdatedBranch bool isBlockedByChangedProtectedFiles bool requireSigned, willSign bool diff --git a/routers/web/repo/setting/protected_branch.go b/routers/web/repo/setting/protected_branch.go index e6a9506c93..ea51b476a1 100644 --- a/routers/web/repo/setting/protected_branch.go +++ b/routers/web/repo/setting/protected_branch.go @@ -266,6 +266,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) { } protectBranch.BlockOnRejectedReviews = f.BlockOnRejectedReviews protectBranch.BlockOnOfficialReviewRequests = f.BlockOnOfficialReviewRequests + protectBranch.BlockOnCodeownerReviews = f.BlockOnCodeownerReviews protectBranch.DismissStaleApprovals = f.DismissStaleApprovals protectBranch.IgnoreStaleApprovals = f.IgnoreStaleApprovals protectBranch.RequireSignedCommits = f.RequireSignedCommits diff --git a/services/convert/convert.go b/services/convert/convert.go index fa61dbe7e5..80a4cdcdeb 100644 --- a/services/convert/convert.go +++ b/services/convert/convert.go @@ -196,6 +196,7 @@ func ToBranchProtection(ctx context.Context, bp *git_model.ProtectedBranch, repo ApprovalsWhitelistTeams: approvalsWhitelistTeams, BlockOnRejectedReviews: bp.BlockOnRejectedReviews, BlockOnOfficialReviewRequests: bp.BlockOnOfficialReviewRequests, + BlockOnCodeownerReviews: bp.BlockOnCodeownerReviews, BlockOnOutdatedBranch: bp.BlockOnOutdatedBranch, DismissStaleApprovals: bp.DismissStaleApprovals, IgnoreStaleApprovals: bp.IgnoreStaleApprovals, diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 5b65369dd6..72aec26d4c 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -192,6 +192,7 @@ type ProtectBranchForm struct { ApprovalsWhitelistTeams string BlockOnRejectedReviews bool BlockOnOfficialReviewRequests bool + BlockOnCodeownerReviews bool BlockOnOutdatedBranch bool DismissStaleApprovals bool IgnoreStaleApprovals bool diff --git a/services/issue/pull.go b/services/issue/pull.go index 95f14ec367..a7d3ca1b22 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -9,11 +9,13 @@ import ( "slices" "time" + git_model "gitea.dev/models/git" issues_model "gitea.dev/models/issues" org_model "gitea.dev/models/organization" user_model "gitea.dev/models/user" "gitea.dev/modules/git" "gitea.dev/modules/log" + "gitea.dev/modules/optional" "gitea.dev/modules/setting" ) @@ -34,69 +36,92 @@ func IsCodeOwnerFile(f string) bool { return slices.Contains(codeOwnerFiles, f) } -func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { - if err := pr.LoadIssue(ctx); err != nil { - return nil, err - } - issue := pr.Issue - if pr.IsWorkInProgress(ctx) { - return nil, nil - } - if err := pr.LoadHeadRepo(ctx); err != nil { - return nil, err - } +// Get all code owner rules for a given pr + repo combination. +func getCodeOwnerRules(ctx context.Context, repo *git.Repository, pr *issues_model.PullRequest) ([]*issues_model.CodeOwnerRule, error) { if err := pr.LoadBaseRepo(ctx); err != nil { return nil, err } + + if err := pr.LoadIssue(ctx); err != nil { + return nil, err + } + pr.Issue.Repo = pr.BaseRepo if pr.BaseRepo.IsFork { return nil, nil } - repo, err := git.OpenRepository(ctx, pr.BaseRepo) - if err != nil { - return nil, err - } - defer repo.Close() - - commit, err := repo.GetBranchCommit(ctx, pr.BaseRepo.DefaultBranch) + commit, err := repo.GetBranchCommit(ctx, pr.BaseBranch) if err != nil { return nil, err } var data string + for _, file := range codeOwnerFiles { - if blob, err := commit.GetBlobByPath(ctx, repo, file); err == nil { - data, err = blob.GetBlobContent(ctx, setting.UI.MaxDisplayFileSize) - if err == nil { - break - } + blob, err := commit.GetBlobByPath(ctx, repo, file) + if err != nil { + continue // no CODEOWNERS at this path, try the next candidate } + // A truncated CODEOWNERS would silently drop rules, so fail closed rather + // than evaluate an incomplete file (the gate must not under-enforce). + if blob.Size(ctx) > setting.UI.MaxDisplayFileSize { + return nil, fmt.Errorf("CODEOWNERS file %q exceeds the maximum readable size", file) + } + // The file exists but is unreadable: propagate instead of swallowing, so + // callers can fail closed instead of treating it as "no code owners". + data, err = blob.GetBlobContent(ctx, setting.UI.MaxDisplayFileSize) + if err != nil { + return nil, err + } + break } + + // no code owner file = no one to approve if data == "" { return nil, nil } - rules, _ := issues_model.GetCodeOwnersFromContent(ctx, data) + rules, warnings := issues_model.GetCodeOwnersFromContent(ctx, data) + for _, w := range warnings { + log.Warn("CODEOWNERS parsing for PR %s#%d: %s", pr.BaseRepo.FullName(), pr.ID, w) + } if len(rules) == 0 { return nil, nil } + return rules, nil +} + +// Get the matching code owner rules for a given pr + repo combination. The returned +// complete flag is false when rule matching was cut short by the match budget, so +// the returned slice is only a partial set of the matching rules. +func getMatchingCodeOwnerRules(ctx context.Context, repo *git.Repository, pr *issues_model.PullRequest) (matchingRules []*issues_model.CodeOwnerRule, complete bool, err error) { + rules, err := getCodeOwnerRules(ctx, repo, pr) + if err != nil { + return nil, false, err + } + if len(rules) == 0 { + return nil, true, nil + } + // get the mergebase mergeBase, err := git.MergeBase(ctx, pr.BaseRepo, git.BranchPrefix+pr.BaseBranch, pr.GetGitHeadRefName()) if err != nil { - return nil, err + return nil, false, err } + // https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed // between the merge base and the head commit but not the base branch and the head commit changedFiles, err := repo.GetFilesChangedBetween(ctx, mergeBase, pr.GetGitHeadRefName()) if err != nil { - return nil, err + return nil, false, err } - uniqUsers := make(map[int64]*user_model.User) - uniqTeams := make(map[string]*org_model.Team) + matchingRules = make([]*issues_model.CodeOwnerRule, 0) + complete = true + // Bound the total time spent matching rules×files. The per-rule MatchTimeout // only caps a single match; without an aggregate budget a crafted CODEOWNERS // plus a PR touching many files could still exhaust CPU inside this loop. @@ -106,19 +131,166 @@ ruleLoop: for _, f := range changedFiles { if time.Now().After(matchDeadline) { log.Warn("CODEOWNERS matching for PR %s#%d exceeded its time budget; some rules were not evaluated", pr.BaseRepo.FullName(), pr.ID) + complete = false break ruleLoop } - shouldMatch := !rule.Negative matched, _ := rule.Rule.MatchString(f) // err only happens when timeouts, any error can be considered as not matched - if matched == shouldMatch { - for _, u := range rule.Users { - uniqUsers[u.ID] = u + if matched != rule.Negative { + matchingRules = append(matchingRules, rule) + break + } + } + } + + return matchingRules, complete, nil +} + +func HasAllRequiredCodeownerReviews(ctx context.Context, pb *git_model.ProtectedBranch, pr *issues_model.PullRequest) bool { + if !pb.BlockOnCodeownerReviews { + return true + } + + if err := pr.LoadBaseRepo(ctx); err != nil { + log.Error("HasAllRequiredCodeownerReviews: failed to load base repository for PR %d: %v", pr.ID, err) + return false + } + + repo, closer, err := git.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) + if err != nil { + log.Error("HasAllRequiredCodeownerReviews: failed to open base repository for PR %d: %v", pr.ID, err) + return false + } + + defer closer.Close() + + matchingRules, complete, err := getMatchingCodeOwnerRules(ctx, repo, pr) + if err != nil { + log.Error("HasAllRequiredCodeownerReviews: failed to match code owner rules for PR %d: %v", pr.ID, err) + return false + } + // Rule matching was truncated by the match budget, so matchingRules is only a + // partial set. Fail closed rather than let an un-evaluated rule pass the gate. + if !complete { + return false + } + if len(matchingRules) == 0 { + return true + } + + // OfficialOnly is intentionally false here: a code owner's approval satisfies this gate + // even if it wouldn't count as an "official" review toward RequiredApprovals (e.g. the + // owner lacks write access, or isn't on the approvals whitelist). Unlike the other + // branch-protection checks below, this one only cares whether a listed code owner approved. + approvingReviews, err := issues_model.FindLatestReviews(ctx, issues_model.FindReviewOptions{ + Types: []issues_model.ReviewType{issues_model.ReviewTypeApprove, issues_model.ReviewTypeReject}, + IssueID: pr.IssueID, + OfficialOnly: false, + Dismissed: optional.Some(false), + }) + if err != nil { + log.Warn("Failed to get approving reviews for PR review %d, error: %v", pr.ID, err) + return false + } + + if pb.IgnoreStaleApprovals { + validApprovingReviews := make(issues_model.ReviewList, 0, len(approvingReviews)) + for _, review := range approvingReviews { + if !review.Stale { + validApprovingReviews = append(validApprovingReviews, review) + } + } + approvingReviews = validApprovingReviews + } + + teamMembersByID := make(map[int64][]*user_model.User) + + for _, rule := range matchingRules { + ruleReviewers := make([]*user_model.User, 0, len(rule.Users)) + for _, u := range rule.Users { + if u.ID != pr.Issue.PosterID { + ruleReviewers = append(ruleReviewers, u) + } + } + for _, t := range rule.Teams { + members, ok := teamMembersByID[t.ID] + if !ok { + if err := t.LoadMembers(ctx); err != nil { + log.Error("HasAllRequiredCodeownerReviews: failed to load members of team %d for PR %d: %v", t.ID, pr.ID, err) + return false } - for _, t := range rule.Teams { - uniqTeams[fmt.Sprintf("%d/%d", t.OrgID, t.ID)] = t + members = t.Members + teamMembersByID[t.ID] = members + } + for _, m := range members { + if m.ID != pr.Issue.PosterID { + ruleReviewers = append(ruleReviewers, m) } } } + + // A rule whose only code owner is the PR author can never be satisfied (an + // author cannot approve their own PR), so it is intentionally waived rather + // than left permanently unmergeable. + if len(ruleReviewers) == 0 { + continue + } + + // and at least 1 approving review from one of them + hasRuleApproval := slices.ContainsFunc(ruleReviewers, func(elem *user_model.User) bool { + return slices.ContainsFunc(approvingReviews, func(review *issues_model.Review) bool { + return review.ReviewerID == elem.ID && review.Type == issues_model.ReviewTypeApprove + }) + }) + + if !hasRuleApproval { + return false + } + } + + return true +} + +func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { + if err := pr.LoadIssue(ctx); err != nil { + return nil, err + } + issue := pr.Issue + if pr.IsWorkInProgress(ctx) { + return nil, nil + } + + if err := pr.LoadBaseRepo(ctx); err != nil { + return nil, err + } + + pr.Issue.Repo = pr.BaseRepo + + repo, closer, err := git.RepositoryFromContextOrOpen(ctx, pr.BaseRepo) + if err != nil { + return nil, err + } + + defer closer.Close() + + // The notifier only requests reviews, so a partial (budget-truncated) rule set is + // acceptable here; the complete flag matters only for the merge gate. + matchingRules, _, err := getMatchingCodeOwnerRules(ctx, repo, pr) + if err != nil { + return nil, err + } + if len(matchingRules) == 0 { + return nil, nil + } + + uniqUsers := make(map[int64]*user_model.User) + uniqTeams := make(map[string]*org_model.Team) + for _, rule := range matchingRules { + for _, u := range rule.Users { + uniqUsers[u.ID] = u + } + for _, t := range rule.Teams { + uniqTeams[fmt.Sprintf("%d/%d", t.OrgID, t.ID)] = t + } } notifiers := make([]*ReviewRequestNotifier, 0, len(uniqUsers)+len(uniqTeams)) diff --git a/services/pull/merge.go b/services/pull/merge.go index 0ffbc37d05..9a52eb1954 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -602,6 +602,10 @@ func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullReques return util.ErrorWrap(ErrNotReadyToMerge, "The head branch is behind the base branch") } + if !issue_service.HasAllRequiredCodeownerReviews(ctx, pb, pr) { + return util.ErrorWrap(ErrNotReadyToMerge, "There are missing code owner reviews") + } + if skipProtectedFilesCheck { return nil } diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index ae262c9500..7b6aa60ad0 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -362,6 +362,13 @@

{{ctx.Locale.Tr "repo.settings.block_on_official_review_requests_desc"}}

+
+
+ + +

{{ctx.Locale.Tr "repo.settings.block_on_codeowner_reviews_desc"}}

+
+
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index fff20fcc48..4d7421531b 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -23059,6 +23059,10 @@ "type": "boolean", "x-go-name": "BlockAdminMergeOverride" }, + "block_on_codeowner_reviews": { + "type": "boolean", + "x-go-name": "BlockOnCodeownerReviews" + }, "block_on_official_review_requests": { "type": "boolean", "x-go-name": "BlockOnOfficialReviewRequests" @@ -23938,6 +23942,10 @@ "type": "boolean", "x-go-name": "BlockAdminMergeOverride" }, + "block_on_codeowner_reviews": { + "type": "boolean", + "x-go-name": "BlockOnCodeownerReviews" + }, "block_on_official_review_requests": { "type": "boolean", "x-go-name": "BlockOnOfficialReviewRequests" @@ -25441,6 +25449,10 @@ "type": "boolean", "x-go-name": "BlockAdminMergeOverride" }, + "block_on_codeowner_reviews": { + "type": "boolean", + "x-go-name": "BlockOnCodeownerReviews" + }, "block_on_official_review_requests": { "type": "boolean", "x-go-name": "BlockOnOfficialReviewRequests" diff --git a/templates/swagger/v1_openapi3_json.tmpl b/templates/swagger/v1_openapi3_json.tmpl index 5afa9be8cb..bb448b7218 100644 --- a/templates/swagger/v1_openapi3_json.tmpl +++ b/templates/swagger/v1_openapi3_json.tmpl @@ -2781,6 +2781,10 @@ "type": "boolean", "x-go-name": "BlockAdminMergeOverride" }, + "block_on_codeowner_reviews": { + "type": "boolean", + "x-go-name": "BlockOnCodeownerReviews" + }, "block_on_official_review_requests": { "type": "boolean", "x-go-name": "BlockOnOfficialReviewRequests" @@ -3677,6 +3681,10 @@ "type": "boolean", "x-go-name": "BlockAdminMergeOverride" }, + "block_on_codeowner_reviews": { + "type": "boolean", + "x-go-name": "BlockOnCodeownerReviews" + }, "block_on_official_review_requests": { "type": "boolean", "x-go-name": "BlockOnOfficialReviewRequests" @@ -5150,6 +5158,10 @@ "type": "boolean", "x-go-name": "BlockAdminMergeOverride" }, + "block_on_codeowner_reviews": { + "type": "boolean", + "x-go-name": "BlockOnCodeownerReviews" + }, "block_on_official_review_requests": { "type": "boolean", "x-go-name": "BlockOnOfficialReviewRequests" diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index ae59a5633a..4ca3702963 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -13,6 +13,7 @@ import ( "time" "gitea.dev/models/db" + git_model "gitea.dev/models/git" issues_model "gitea.dev/models/issues" repo_model "gitea.dev/models/repo" "gitea.dev/models/unittest" @@ -20,6 +21,7 @@ import ( "gitea.dev/modules/git" "gitea.dev/modules/test" issue_service "gitea.dev/services/issue" + pull_service "gitea.dev/services/pull" repo_service "gitea.dev/services/repository" files_service "gitea.dev/services/repository/files" "gitea.dev/tests" @@ -51,6 +53,8 @@ func TestPullView_ReviewerMissed(t *testing.T) { func TestPullView_CodeOwner(t *testing.T) { onGiteaRun(t, func(t *testing.T, u *url.URL) { user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + user8 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 8}) // Create the repo. repo, err := repo_service.CreateRepositoryDirectly(t.Context(), user2, user2, repo_service.CreateRepoOptions{ @@ -62,6 +66,17 @@ func TestPullView_CodeOwner(t *testing.T) { }, true) assert.NoError(t, err) + // create code owner branch protection + protectBranch := git_model.ProtectedBranch{ + BlockOnCodeownerReviews: true, + RepoID: repo.ID, + RuleName: "master", + CanPush: true, + } + + err = pull_service.CreateOrUpdateProtectedBranch(t.Context(), repo, &protectBranch, git_model.WhitelistOptions{}) + assert.NoError(t, err) + // add CODEOWNERS to default branch _, err = files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{ OldBranch: repo.DefaultBranch, @@ -106,7 +121,7 @@ func TestPullView_CodeOwner(t *testing.T) { require.NoError(t, err) // update the file on the pr branch - _, err = files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{ + resp, err := files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{ OldBranch: "codeowner-basebranch", Files: []*files_service.ChangeRepoFile{ { @@ -142,6 +157,73 @@ func TestPullView_CodeOwner(t *testing.T) { prUpdated2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) assert.NoError(t, prUpdated2.LoadIssue(t.Context())) assert.Equal(t, "Test Pull Request2", prUpdated2.Issue.Title) + + // ensure it cannot be merged + hasCodeownerReviews := issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr) + assert.False(t, hasCodeownerReviews) + + _, _, err = issues_model.SubmitReview(t.Context(), user5, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0)) + assert.NoError(t, err) + + // should still fail (we also need user8) + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr) + assert.False(t, hasCodeownerReviews) + + _, _, err = issues_model.SubmitReview(t.Context(), user8, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0)) + assert.NoError(t, err) + + // now we should be able to merge + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr) + assert.True(t, hasCodeownerReviews) + + // a later requested-changes review from a required code owner must override + // that user's earlier approval and no longer satisfy the requirement + _, _, err = issues_model.SubmitReview(t.Context(), user5, pr.Issue, issues_model.ReviewTypeReject, "Needs changes", resp.Commit.SHA, false, make([]string, 0)) + assert.NoError(t, err) + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr) + assert.False(t, hasCodeownerReviews) + }) + + t.Run("Dismissed Code Owner Review", func(t *testing.T) { + _, err := files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{ + NewBranch: "codeowner-dismiss-branch", + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "README.md", + ContentReader: strings.NewReader("# New README content\n"), + }, + }, + }) + assert.NoError(t, err) + + session := loginUser(t, "user2") + testPullCreate(t, session, "user2", "test_codeowner", false, repo.DefaultBranch, "codeowner-dismiss-branch", "Test Code Owner Dismissal") + + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-dismiss-branch"}) + unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5}) + + hasCodeownerReviews := issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr) + assert.False(t, hasCodeownerReviews) + + _, _, err = issues_model.SubmitReview(t.Context(), user5, pr.Issue, issues_model.ReviewTypeApprove, " LGTM", "", false, make([]string, 0)) + assert.NoError(t, err) + + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr) + assert.True(t, hasCodeownerReviews) + + review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, ReviewerID: 5, Type: issues_model.ReviewTypeApprove}) + assert.False(t, review.Dismissed) + + err = issues_model.DismissReview(t.Context(), review, true) + assert.NoError(t, err) + + review, err = issues_model.GetReviewByID(t.Context(), review.ID) + assert.NoError(t, err) + assert.True(t, review.Dismissed) + + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr) + assert.False(t, hasCodeownerReviews) }) // change the default branch CODEOWNERS file to change README.md's codeowner @@ -158,7 +240,7 @@ func TestPullView_CodeOwner(t *testing.T) { t.Run("Second Pull Request", func(t *testing.T) { // create a new branch to prepare for pull request - _, err = files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{ + resp, err := files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{ NewBranch: "codeowner-basebranch2", Files: []*files_service.ChangeRepoFile{ { @@ -176,6 +258,24 @@ func TestPullView_CodeOwner(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch2"}) unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) + + // should need user8 approval only now + hasCodeownerReviews := issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr) + assert.False(t, hasCodeownerReviews) + + // exercise the real merge-time gate, not just the helper: the persisted + // protection rule must block the merge on the missing code owner review + err = pull_service.CheckPullBranchProtections(t.Context(), pr, false) + assert.ErrorContains(t, err, "code owner") + + _, _, err = issues_model.SubmitReview(t.Context(), user8, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0)) + assert.NoError(t, err) + + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr) + assert.True(t, hasCodeownerReviews) + + // and once the code owner has approved, the merge gate no longer blocks + assert.NoError(t, pull_service.CheckPullBranchProtections(t.Context(), pr, false)) }) t.Run("Forked Repo Pull Request", func(t *testing.T) { @@ -187,7 +287,7 @@ func TestPullView_CodeOwner(t *testing.T) { assert.NoError(t, err) // create a new branch to prepare for pull request - _, err = files_service.ChangeRepoFiles(t.Context(), forkedRepo, user5, &files_service.ChangeRepoFilesOptions{ + resp, err := files_service.ChangeRepoFiles(t.Context(), forkedRepo, user5, &files_service.ChangeRepoFilesOptions{ NewBranch: "codeowner-basebranch-forked", Files: []*files_service.ChangeRepoFile{ { @@ -228,6 +328,23 @@ func TestPullView_CodeOwner(t *testing.T) { pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"}) unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) + + // will also need user8 for this + hasCodeownerReviews := issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr) + assert.False(t, hasCodeownerReviews) + + _, _, err = issues_model.SubmitReview(t.Context(), user5, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0)) + assert.NoError(t, err) + + // should still fail (user5 is not a code owner for this PR) + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr) + assert.False(t, hasCodeownerReviews) + + _, _, err = issues_model.SubmitReview(t.Context(), user8, pr.Issue, issues_model.ReviewTypeApprove, "Very good", resp.Commit.SHA, false, make([]string, 0)) + assert.NoError(t, err) + + hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr) + assert.True(t, hasCodeownerReviews) }) }) }