Fix evaluation of disapproval reviews and comments (#567)

A previous optimization to minimize evaluations and reduce status updates incorrectly ignored disapproval policies. This meant that a disapproval would only appear in GitHub if some other event happened after and triggered an evaluation.
palantir · Apr 19, 2023 · 00420ac · 00420ac
1 parent 861953f
commit 00420ac
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 12 deletions.
diff --git a/server/handler/issue_comment.go b/server/handler/issue_comment.go
@@ -21,7 +21,7 @@ import (
 
 	"github.com/google/go-github/v50/github"
 	"github.com/palantir/go-githubapp/githubapp"
-	"github.com/palantir/policy-bot/policy/approval"
+	"github.com/palantir/policy-bot/policy"
 	"github.com/palantir/policy-bot/policy/common"
 	"github.com/palantir/policy-bot/pull"
 	"github.com/pkg/errors"
@@ -92,7 +92,7 @@ func (h *IssueComment) Handle(ctx context.Context, eventType, deliveryID string,
 		return nil
 	}
 
-	if !h.affectsApproval(event, evalCtx.Config.Config.ApprovalRules) {
+	if !h.affectsApproval(event, evalCtx.Config.Config) {
 		logger.Debug().Msg("Skipping evaluation because this comment does not impact approval")
 		return nil
 	}
@@ -119,7 +119,7 @@ func (h *IssueComment) detectAndLogTampering(ctx context.Context, evalCtx *EvalC
 		return false
 	}
 
-	if h.affectsApproval(event, evalCtx.Config.Config.ApprovalRules) {
+	if h.affectsApproval(event, evalCtx.Config.Config) {
 		msg := fmt.Sprintf("Entity %s edited approval comment by %s", eventAuthor, commentAuthor)
 		logger.Warn().Str(LogKeyAudit, "issue_comment").Msg(msg)
 
@@ -131,7 +131,7 @@ func (h *IssueComment) detectAndLogTampering(ctx context.Context, evalCtx *EvalC
 	return true
 }
 
-func (h *IssueComment) affectsApproval(event github.IssueCommentEvent, rules []*approval.Rule) bool {
+func (h *IssueComment) affectsApproval(event github.IssueCommentEvent, config *policy.Config) bool {
 	var body, originalBody string
 	switch event.GetAction() {
 	case "edited":
@@ -142,9 +142,17 @@ func (h *IssueComment) affectsApproval(event github.IssueCommentEvent, rules []*
 		originalBody = body
 	}
 
-	for _, rule := range rules {
-		methods := rule.Options.GetMethods()
-		if methods.CommentMatches(body) || (body != originalBody && methods.CommentMatches(originalBody)) {
+	var methods []*common.Methods
+	for _, rule := range config.ApprovalRules {
+		methods = append(methods, rule.Options.GetMethods())
+	}
+	if disapproval := config.Policy.Disapproval; disapproval != nil {
+		methods = append(methods, disapproval.Options.GetDisapproveMethods())
+		methods = append(methods, disapproval.Options.GetRevokeMethods())
+	}
+
+	for _, m := range methods {
+		if m.CommentMatches(body) || (body != originalBody && m.CommentMatches(originalBody)) {
 			return true
 		}
 	}

diff --git a/server/handler/pull_request_review.go b/server/handler/pull_request_review.go
@@ -20,7 +20,7 @@ import (
 
 	"github.com/google/go-github/v50/github"
 	"github.com/palantir/go-githubapp/githubapp"
-	"github.com/palantir/policy-bot/policy/approval"
+	"github.com/palantir/policy-bot/policy"
 	"github.com/palantir/policy-bot/policy/common"
 	"github.com/palantir/policy-bot/pull"
 	"github.com/pkg/errors"
@@ -72,7 +72,7 @@ func (h *PullRequestReview) Handle(ctx context.Context, eventType, deliveryID st
 	}
 
 	reviewState := pull.ReviewState(event.GetReview().GetState())
-	if !h.affectsApproval(reviewState, evalCtx.Config.Config.ApprovalRules) {
+	if !h.affectsApproval(reviewState, evalCtx.Config.Config) {
 		logger.Debug().Msg("Skipping evaluation because this review does not impact approval")
 		return nil
 	}
@@ -86,9 +86,18 @@ func (h *PullRequestReview) Handle(ctx context.Context, eventType, deliveryID st
 	return nil
 }
 
-func (h *PullRequestReview) affectsApproval(reviewState pull.ReviewState, rules []*approval.Rule) bool {
-	for _, rule := range rules {
-		if reviewState == rule.Options.GetMethods().GithubReviewState {
+func (h *PullRequestReview) affectsApproval(reviewState pull.ReviewState, config *policy.Config) bool {
+	states := make(map[pull.ReviewState]struct{})
+	for _, rule := range config.ApprovalRules {
+		states[rule.Options.GetMethods().GithubReviewState] = struct{}{}
+	}
+	if disapproval := config.Policy.Disapproval; disapproval != nil {
+		states[disapproval.Options.GetDisapproveMethods().GithubReviewState] = struct{}{}
+		states[disapproval.Options.GetRevokeMethods().GithubReviewState] = struct{}{}
+	}
+
+	for state := range states {
+		if state == reviewState {
 			return true
 		}
 	}