fix SEV 16399 (#765)

lyft · Jul 31, 2024 · 2f43bc9 · 2f43bc9
1 parent 31f1f02
commit 2f43bc9
Show file tree

Hide file tree

Showing 10 changed files with 652 additions and 26 deletions.
diff --git a/server/legacy/events/policy_filter.go b/server/legacy/events/policy_filter.go
@@ -58,6 +58,14 @@ func (p *ApprovedPolicyFilter) Filter(ctx context.Context, installationToken int
 		return failedPolicies, nil
 	}
 
+	// Dismiss PR reviews when event came from pull request change/atlantis plan comment
+	if trigger == command.AutoTrigger || trigger == command.CommentTrigger {
+		err := p.dismissStalePRReviews(ctx, installationToken, repo, prNum)
+		if err != nil {
+			return failedPolicies, errors.Wrap(err, "failed to dismiss stale PR reviews")
+		}
+		return failedPolicies, nil
+	}
 	// Fetch reviewers who approved the PR
 	approvedReviewers, err := p.prReviewFetcher.ListLatestApprovalUsernames(ctx, installationToken, repo, prNum)
 	if err != nil {
@@ -77,6 +85,55 @@ func (p *ApprovedPolicyFilter) Filter(ctx context.Context, installationToken int
 	}
 	return filteredFailedPolicies, nil
 }
+func (p *ApprovedPolicyFilter) dismissStalePRReviews(ctx context.Context, installationToken int64, repo models.Repo, prNum int) error {
+	shouldAllocate, err := p.allocator.ShouldAllocate(feature.LegacyDeprecation, feature.FeatureContext{
+		RepoName: repo.FullName,
+	})
+	if err != nil {
+		return errors.Wrap(err, "unable to allocate legacy deprecation feature flag")
+	}
+	// if legacy deprecation is enabled, don't dismiss stale PR reviews in legacy workflow
+	if shouldAllocate {
+		p.logger.InfoContext(ctx, "legacy deprecation feature flag enabled, not dismissing stale PR reviews")
+		return nil
+	}
+
+	approvalReviews, err := p.prReviewFetcher.ListApprovalReviews(ctx, installationToken, repo, prNum)
+	if err != nil {
+		return errors.Wrap(err, "failed to fetch GH PR reviews")
+	}
+
+	for _, approval := range approvalReviews {
+		isOwner, err := p.approverIsOwner(ctx, installationToken, approval)
+		if err != nil {
+			return errors.Wrap(err, "failed to validate approver is owner")
+		}
+		if isOwner {
+			err = p.prReviewDismisser.Dismiss(ctx, installationToken, repo, prNum, approval.GetID())
+			if err != nil {
+				return errors.Wrap(err, "failed to dismiss GH PR reviews")
+			}
+		}
+	}
+	return nil
+}
+
+func (p *ApprovedPolicyFilter) approverIsOwner(ctx context.Context, installationToken int64, approval *gh.PullRequestReview) (bool, error) {
+	if approval.GetUser() == nil {
+		return false, errors.New("failed to identify approver")
+	}
+	reviewers := []string{approval.GetUser().GetLogin()}
+	for _, policy := range p.policies {
+		isOwner, err := p.reviewersContainsPolicyOwner(ctx, installationToken, reviewers, policy)
+		if err != nil {
+			return false, errors.Wrap(err, "validating policy approval")
+		}
+		if isOwner {
+			return true, nil
+		}
+	}
+	return false, nil
+}
 
 func (p *ApprovedPolicyFilter) reviewersContainsPolicyOwner(ctx context.Context, installationToken int64, reviewers []string, policy valid.PolicySet) (bool, error) {
 	// fetch owners from GH team

diff --git a/server/legacy/events/policy_filter_test.go b/server/legacy/events/policy_filter_test.go
@@ -45,6 +45,87 @@ func TestFilter_Approved(t *testing.T) {
 	assert.Empty(t, filteredPolicies)
 }
 
+func TestFilter_NotApproved(t *testing.T) {
+	reviewFetcher := &mockReviewFetcher{
+		reviews: []*github.PullRequestReview{
+			{
+				User: &github.User{Login: github.String(ownerA)},
+			},
+			{
+				User: &github.User{Login: github.String(ownerB)},
+			},
+		},
+	}
+	teamFetcher := &mockTeamMemberFetcher{
+		members: []string{ownerC},
+	}
+	reviewDismisser := &mockReviewDismisser{}
+	failedPolicies := []valid.PolicySet{
+		{Name: policyName, Owner: policyOwner},
+	}
+
+	policyFilter := NewApprovedPolicyFilter(reviewFetcher, reviewDismisser, teamFetcher, &testFeatureAllocator{}, failedPolicies, logging.NewNoopCtxLogger(t))
+	filteredPolicies, err := policyFilter.Filter(context.Background(), 0, models.Repo{}, 0, command.AutoTrigger, failedPolicies)
+	assert.NoError(t, err)
+	assert.False(t, reviewFetcher.listUsernamesIsCalled)
+	assert.True(t, reviewFetcher.listApprovalsIsCalled)
+	assert.True(t, teamFetcher.isCalled)
+	assert.False(t, reviewDismisser.isCalled)
+	assert.Equal(t, failedPolicies, filteredPolicies)
+}
+
+func TestFilter_DismissalBlockedByFeatureAllocator(t *testing.T) {
+	reviewFetcher := &mockReviewFetcher{
+		reviews: []*github.PullRequestReview{
+			{
+				User: &github.User{Login: github.String(ownerA)},
+			},
+		},
+	}
+	teamFetcher := &mockTeamMemberFetcher{
+		members: []string{ownerA},
+	}
+	reviewDismisser := &mockReviewDismisser{}
+	failedPolicies := []valid.PolicySet{
+		{Name: policyName, Owner: policyOwner},
+	}
+
+	policyFilter := NewApprovedPolicyFilter(reviewFetcher, reviewDismisser, teamFetcher, &testFeatureAllocator{Enabled: true}, failedPolicies, logging.NewNoopCtxLogger(t))
+	filteredPolicies, err := policyFilter.Filter(context.Background(), 0, models.Repo{}, 0, command.AutoTrigger, failedPolicies)
+	assert.NoError(t, err)
+	assert.False(t, reviewFetcher.listUsernamesIsCalled)
+	assert.False(t, reviewFetcher.listApprovalsIsCalled)
+	assert.False(t, teamFetcher.isCalled)
+	assert.False(t, reviewDismisser.isCalled)
+	assert.Equal(t, failedPolicies, filteredPolicies)
+}
+
+func TestFilter_NotApproved_Dismissal(t *testing.T) {
+	reviewFetcher := &mockReviewFetcher{
+		reviews: []*github.PullRequestReview{
+			{
+				User: &github.User{Login: github.String(ownerA)},
+			},
+		},
+	}
+	teamFetcher := &mockTeamMemberFetcher{
+		members: []string{ownerA},
+	}
+	reviewDismisser := &mockReviewDismisser{}
+	failedPolicies := []valid.PolicySet{
+		{Name: policyName, Owner: policyOwner},
+	}
+
+	policyFilter := NewApprovedPolicyFilter(reviewFetcher, reviewDismisser, teamFetcher, &testFeatureAllocator{}, failedPolicies, logging.NewNoopCtxLogger(t))
+	filteredPolicies, err := policyFilter.Filter(context.Background(), 0, models.Repo{}, 0, command.AutoTrigger, failedPolicies)
+	assert.NoError(t, err)
+	assert.False(t, reviewFetcher.listUsernamesIsCalled)
+	assert.True(t, reviewFetcher.listApprovalsIsCalled)
+	assert.True(t, teamFetcher.isCalled)
+	assert.True(t, reviewDismisser.isCalled)
+	assert.Equal(t, failedPolicies, filteredPolicies)
+}
+
 func TestFilter_NoFailedPolicies(t *testing.T) {
 	reviewFetcher := &mockReviewFetcher{
 		approvers: []string{ownerB},
@@ -85,6 +166,26 @@ func TestFilter_FailedListLatestApprovalUsernames(t *testing.T) {
 	assert.Equal(t, failedPolicies, filteredPolicies)
 }
 
+func TestFilter_FailedListApprovalReviews(t *testing.T) {
+	reviewFetcher := &mockReviewFetcher{
+		listApprovalsError: assert.AnError,
+	}
+	teamFetcher := &mockTeamMemberFetcher{}
+	reviewDismisser := &mockReviewDismisser{}
+	failedPolicies := []valid.PolicySet{
+		{Name: policyName, Owner: policyOwner},
+	}
+
+	policyFilter := NewApprovedPolicyFilter(reviewFetcher, reviewDismisser, teamFetcher, &testFeatureAllocator{}, failedPolicies, logging.NewNoopCtxLogger(t))
+	filteredPolicies, err := policyFilter.Filter(context.Background(), 0, models.Repo{}, 0, command.CommentTrigger, failedPolicies)
+	assert.Error(t, err)
+	assert.False(t, reviewFetcher.listUsernamesIsCalled)
+	assert.True(t, reviewFetcher.listApprovalsIsCalled)
+	assert.False(t, reviewDismisser.isCalled)
+	assert.False(t, teamFetcher.isCalled)
+	assert.Equal(t, failedPolicies, filteredPolicies)
+}
+
 func TestFilter_FailedTeamMemberFetch(t *testing.T) {
 	reviewFetcher := &mockReviewFetcher{
 		approvers: []string{ownerB},
@@ -107,6 +208,34 @@ func TestFilter_FailedTeamMemberFetch(t *testing.T) {
 	assert.Equal(t, failedPolicies, filteredPolicies)
 }
 
+func TestFilter_FailedDismiss(t *testing.T) {
+	reviewFetcher := &mockReviewFetcher{
+		reviews: []*github.PullRequestReview{
+			{
+				User: &github.User{Login: github.String(ownerB)},
+			},
+		},
+	}
+	reviewDismisser := &mockReviewDismisser{
+		error: assert.AnError,
+	}
+	teamFetcher := &mockTeamMemberFetcher{
+		members: []string{ownerB},
+	}
+	failedPolicies := []valid.PolicySet{
+		{Name: policyName, Owner: policyOwner},
+	}
+
+	policyFilter := NewApprovedPolicyFilter(reviewFetcher, reviewDismisser, teamFetcher, &testFeatureAllocator{}, failedPolicies, logging.NewNoopCtxLogger(t))
+	filteredPolicies, err := policyFilter.Filter(context.Background(), 0, models.Repo{}, 0, command.AutoTrigger, failedPolicies)
+	assert.Error(t, err)
+	assert.False(t, reviewFetcher.listUsernamesIsCalled)
+	assert.True(t, reviewFetcher.listApprovalsIsCalled)
+	assert.True(t, teamFetcher.isCalled)
+	assert.True(t, reviewDismisser.isCalled)
+	assert.Equal(t, failedPolicies, filteredPolicies)
+}
+
 type mockReviewFetcher struct {
 	approvers             []string
 	listUsernamesIsCalled bool

diff --git a/server/legacy/events/pr_project_context_builder.go b/server/legacy/events/pr_project_context_builder.go
@@ -1,6 +1,8 @@
 package events
 
 import (
+	"fmt"
+
 	"github.com/runatlantis/atlantis/server/config/valid"
 	"github.com/runatlantis/atlantis/server/legacy/events/command"
 	"github.com/runatlantis/atlantis/server/logging"
@@ -36,14 +38,23 @@ func (p *PlatformModeProjectContextBuilder) BuildProjectContext(
 	repoDir string,
 	contextFlags *command.ContextFlags,
 ) []command.ProjectContext {
-	return buildContext(
-		ctx,
-		cmdName,
-		getSteps(cmdName, prjCfg.PullRequestWorkflow, contextFlags.LogLevel),
-		p.CommentBuilder,
-		prjCfg,
-		commentArgs,
-		repoDir,
-		contextFlags,
-	)
+	shouldAllocate, err := p.allocator.ShouldAllocate(feature.PlatformMode, feature.FeatureContext{RepoName: ctx.HeadRepo.FullName})
+	if err != nil {
+		p.Logger.ErrorContext(ctx.RequestCtx, fmt.Sprintf("unable to allocate for feature: %s, error: %s", feature.PlatformMode, err))
+	}
+
+	if shouldAllocate {
+		return buildContext(
+			ctx,
+			cmdName,
+			getSteps(cmdName, prjCfg.PullRequestWorkflow, contextFlags.LogLevel),
+			p.CommentBuilder,
+			prjCfg,
+			commentArgs,
+			repoDir,
+			contextFlags,
+		)
+	}
+
+	return p.delegate.BuildProjectContext(ctx, cmdName, prjCfg, commentArgs, repoDir, contextFlags)
 }