diff --git a/models/issues/review.go b/models/issues/review.go index e1f522be75d..a051440290e 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -324,6 +324,59 @@ func IsOfficialReviewerTeam(ctx context.Context, issue *Issue, team *organizatio return slices.Contains(pb.ApprovalsWhitelistTeamIDs, team.ID), nil } +// RecalculateReviewsOfficial re-evaluates the "official" flag of the latest approve +// and reject reviews of an issue against its pull request's current base branch. +// It must be called whenever the target branch changes, otherwise an approval that +// was official on the previous (possibly unprotected) branch would keep satisfying +// the new branch's protection rules. +func RecalculateReviewsOfficial(ctx context.Context, issue *Issue) error { + if err := issue.LoadPullRequest(ctx); err != nil { + return err + } + + // Clearing and restoring the official flags must happen atomically, otherwise a + // failure in between would leave the reviews without any official flag set. + return db.WithTx(ctx, func(ctx context.Context) error { + // Only the latest approve/reject review of each reviewer counts as official, so + // clear the flag on all of them first and restore it only where it still applies. + if _, err := db.GetEngine(ctx). + Where("issue_id = ?", issue.ID). + In("type", ReviewTypeApprove, ReviewTypeReject). + Cols("official"). + Update(&Review{Official: false}); err != nil { + return err + } + + reviews, err := FindLatestReviews(ctx, FindReviewOptions{ + Types: []ReviewType{ReviewTypeApprove, ReviewTypeReject}, + IssueID: issue.ID, + }) + if err != nil { + return err + } + + for _, review := range reviews { + if err := review.LoadReviewer(ctx); err != nil { + return err + } + if review.Reviewer == nil { + continue + } + official, err := IsOfficialReviewer(ctx, issue, review.Reviewer) + if err != nil { + return err + } + if official { + if _, err := db.GetEngine(ctx).ID(review.ID).Cols("official").Update(&Review{Official: true}); err != nil { + return err + } + } + } + + return nil + }) +} + // CreateReview creates a new review based on opts func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error) { return db.WithTx2(ctx, func(ctx context.Context) (*Review, error) { diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 3d2672c4036..6010f74c415 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -6,6 +6,8 @@ package issues_test import ( "testing" + "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" @@ -386,3 +388,45 @@ func TestAddReviewRequest(t *testing.T) { assert.NotNil(t, comment.CommentMetaData) assert.Equal(t, issues_model.SpecialDoerNameCodeOwners, comment.CommentMetaData.SpecialDoerName) } + +func TestRecalculateReviewsOfficial(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + // PR #2 targets repo1's "master" branch. Simulate an approval that became + // official while the PR targeted an unprotected branch. + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 3}) + reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + review, err := issues_model.CreateReview(t.Context(), issues_model.CreateReviewOptions{ + Type: issues_model.ReviewTypeApprove, + Issue: issue, + Reviewer: reviewer, + Official: true, + }) + assert.NoError(t, err) + + // Protect the (now current) target branch with an approvals whitelist that + // does not include the reviewer, mirroring a retarget onto a protected branch. + rule := &git_model.ProtectedBranch{ + RepoID: issue.RepoID, + RuleName: "master", + EnableApprovalsWhitelist: true, + ApprovalsWhitelistUserIDs: []int64{2}, + RequiredApprovals: 1, + } + assert.NoError(t, db.Insert(t.Context(), rule)) + + // Re-evaluating must strip the stale official flag, otherwise the approval + // would still satisfy the protected branch's required approvals. + assert.NoError(t, issues_model.RecalculateReviewsOfficial(t.Context(), issue)) + review = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: review.ID}) + assert.False(t, review.Official) + + // Once the reviewer is whitelisted, re-evaluating restores the official flag. + rule.ApprovalsWhitelistUserIDs = []int64{2, reviewer.ID} + _, err = db.GetEngine(t.Context()).ID(rule.ID).Cols("approvals_whitelist_user_i_ds").Update(rule) + assert.NoError(t, err) + + assert.NoError(t, issues_model.RecalculateReviewsOfficial(t.Context(), issue)) + review = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: review.ID}) + assert.True(t, review.Official) +} diff --git a/services/pull/pull.go b/services/pull/pull.go index 13f8ffa8df6..79a07a9348e 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -326,6 +326,14 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer return fmt.Errorf("syncCommitDivergence: %w", err) } + // The "official" flag of existing reviews was computed against the previous + // target branch's protection rules, so re-evaluate it against the new branch. + // Otherwise a stale official approval could bypass the new branch's protection. + pr.Issue.PullRequest = pr + if err := issues_model.RecalculateReviewsOfficial(ctx, pr.Issue); err != nil { + return fmt.Errorf("RecalculateReviewsOfficial: %w", err) + } + // Create comment options := &issues_model.CreateCommentOptions{ Type: issues_model.CommentTypeChangeTargetBranch,