Repository: palantir/policy-bot
Pull request #357 (2021-12-22 22:47:28)

feat: ignore edited comments with new option
Closes https://github.com/palantir/policy-bot/issues/356

As discussed in the attached issue, this feature would allow a new boolean option `ignore_edited_comments:` that when set to `true` would ignore both issue comments and review comments that have been edited when evaluating the approval rules. This helps guard against potential situations where a user could edit a comment to gain approval outside of the intended approval rules construct.

I went with adding the `updated_at` field we get back from the graphql response because that makes it easy to determine quickly if a comment has been edited irrespective of whom the comment was edited by (which was also an intentional choice).

I went with putting it in the approval part of the code because it seemed like that was where it best fit naturally given we already have access to the rule options and the candidates and this is the place we seem to already be doing other types of candidate filtering. This would have been harder to implement over in the server handlers as we don't have access to the options yet. Let me know what you think of this approach, open to other ideas, but this seemed the easiest to implement.


Patch:
diff --git a/README.md b/README.md
index 2e3e44b..30c9acc 100644
--- a/README.md
+++ b/README.md
@@ -293,6 +293,10 @@ options:
   # approvals for this rule. False by default.
   invalidate_on_push: false
 
+  # If true, comments on PRs and review comments that have been edited in any way
+  # will be ignored when evaluating approval rules. Default is false.
+  ignore_edited_comments: false
+
   # If true, "update merges" do not invalidate approval (if invalidate_on_push
   # is enabled) and their authors/committers do not count as contributors. An
   # "update merge" is a merge commit that was created in the UI or via the API
@@ -639,11 +643,12 @@ contributions from forks.
 ### Comment Edits
 
 GitHub users with sufficient permissions can edit the comments of other users,
-possibly chaning an unrelated comment into one that enables approval.
+possibly changing an unrelated comment into one that enables approval.
 `policy-bot` also contains audting for this event, but as with statuses, a
 well-timed edit can approve and merge a pull request before `policy-bot` can
-detect the problem. Organizations concerned about this case should monitor and
-alert on the relevant audit logs.
+detect the problem. Organizations concerned about this case can use the
+`ignore_edited_comments` option or can monitor and alert on the relevant audit
+logs.
 
 This issue can also be minimized by only using GitHub reviews for approval, at
 the expense of removing the ability to self-approve pull requests.
diff --git a/policy/approval/approve.go b/policy/approval/approve.go
index 13ad9ee..1ac32da 100644
--- a/policy/approval/approve.go
+++ b/policy/approval/approve.go
@@ -41,8 +41,9 @@ type Options struct {
 	AllowContributor bool `yaml:"allow_contributor"`
 	InvalidateOnPush bool `yaml:"invalidate_on_push"`
 
-	IgnoreUpdateMerges bool          `yaml:"ignore_update_merges"`
-	IgnoreCommitsBy    common.Actors `yaml:"ignore_commits_by"`
+	IgnoreEditedComments bool          `yaml:"ignore_edited_comments"`
+	IgnoreUpdateMerges   bool          `yaml:"ignore_update_merges"`
+	IgnoreCommitsBy      common.Actors `yaml:"ignore_commits_by"`
 
 	RequestReview RequestReview `yaml:"request_review"`
 
@@ -248,14 +249,55 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string
 }
 
 func (r *Rule) filteredCandidates(ctx context.Context, prctx pull.Context) ([]*common.Candidate, error) {
-	log := zerolog.Ctx(ctx)
-
 	candidates, err := r.Options.GetMethods().Candidates(ctx, prctx)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to get approval candidates")
 	}
+
 	sort.Stable(common.CandidatesByCreationTime(candidates))
 
+	if r.Options.IgnoreEditedComments {
+		candidates, err = r.filterEditedCandidates(ctx, prctx, candidates)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	if r.Options.InvalidateOnPush {
+		candidates, err = r.filterInvalidCandidates(ctx, prctx, candidates)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	return candidates, nil
+}
+
+func (r *Rule) filterEditedCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, error) {
+	log := zerolog.Ctx(ctx)
+
+	if !r.Options.IgnoreEditedComments {
+		return candidates, nil
+	}
+
+	var allowedCandidates []*common.Candidate
+	for _, candidate := range candidates {
+		if r.Options.IgnoreEditedComments {
+			if candidate.UpdatedAt == candidate.CreatedAt {
+				allowedCandidates = append(allowedCandidates, candidate)
+			}
+		}
+	}
+
+	log.Debug().Msgf("discarded %d candidates with edited comments",
+		len(candidates)-len(allowedCandidates))
+
+	return allowedCandidates, nil
+}
+
+func (r *Rule) filterInvalidCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, error) {
+	log := zerolog.Ctx(ctx)
+
 	if !r.Options.InvalidateOnPush {
 		return candidates, nil
 	}
diff --git a/policy/common/methods.go b/policy/common/methods.go
index 0299ac1..05baec4 100644
--- a/policy/common/methods.go
+++ b/policy/common/methods.go
@@ -36,6 +36,7 @@ type Methods struct {
 type Candidate struct {
 	User      string
 	CreatedAt time.Time
+	UpdatedAt time.Time
 }
 
 type CandidatesByCreationTime []*Candidate
@@ -64,6 +65,7 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid
 				candidates = append(candidates, &Candidate{
 					User:      c.Author,
 					CreatedAt: c.CreatedAt,
+					UpdatedAt: c.UpdatedAt,
 				})
 			}
 		}
@@ -80,6 +82,7 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid
 				candidates = append(candidates, &Candidate{
 					User:      r.Author,
 					CreatedAt: r.CreatedAt,
+					UpdatedAt: r.UpdatedAt,
 				})
 			}
 		}
diff --git a/pull/context.go b/pull/context.go
index b259111..82e8a30 100644
--- a/pull/context.go
+++ b/pull/context.go
@@ -182,6 +182,7 @@ type Signature struct {
 
 type Comment struct {
 	CreatedAt time.Time
+	UpdatedAt time.Time
 	Author    string
 	Body      string
 }
@@ -198,6 +199,7 @@ const (
 
 type Review struct {
 	CreatedAt time.Time
+	UpdatedAt time.Time
 	Author    string
 	State     ReviewState
 	Body      string
diff --git a/pull/github.go b/pull/github.go
index 126f696..7abedf3 100644
--- a/pull/github.go
+++ b/pull/github.go
@@ -983,6 +983,7 @@ type v4PullRequestReview struct {
 	State       string
 	Body        string
 	SubmittedAt time.Time
+	UpdatedAt   time.Time
 
 	Commit struct {
 		OID string
@@ -1005,6 +1006,7 @@ func (r *v4PullRequestReview) ToReview() *Review {
 
 	return &Review{
 		CreatedAt: r.SubmittedAt,
+		UpdatedAt: r.UpdatedAt,
 		Author:    r.Author.GetV3Login(),
 		State:     ReviewState(strings.ToLower(r.State)),
 		Body:      r.Body,
@@ -1016,6 +1018,7 @@ func (r *v4PullRequestReview) ToReview() *Review {
 func (r *v4PullRequestReview) ToComment() *Comment {
 	return &Comment{
 		CreatedAt: r.SubmittedAt,
+		UpdatedAt: r.UpdatedAt,
 		Author:    r.Author.GetV3Login(),
 		Body:      r.Body,
 	}
@@ -1025,11 +1028,13 @@ type v4IssueComment struct {
 	Author    v4Actor
 	Body      string
 	CreatedAt time.Time
+	UpdatedAt time.Time
 }
 
 func (c *v4IssueComment) ToComment() *Comment {
 	return &Comment{
 		CreatedAt: c.CreatedAt,
+		UpdatedAt: c.UpdatedAt,
 		Author:    c.Author.GetV3Login(),
 		Body:      c.Body,
 	}


Test patch:
diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go
index 0914d95..d6ff78c 100644
--- a/policy/approval/approve_test.go
+++ b/policy/approval/approve_test.go
@@ -39,48 +39,69 @@ func TestIsApproved(t *testing.T) {
 			CommentsValue: []*pull.Comment{
 				{
 					CreatedAt: now.Add(10 * time.Second),
+					UpdatedAt: now.Add(10 * time.Second),
 					Author:    "other-user",
 					Body:      "Why did you do this?",
 				},
 				{
 					CreatedAt: now.Add(20 * time.Second),
+					UpdatedAt: now.Add(20 * time.Second),
 					Author:    "comment-approver",
 					Body:      "LGTM :+1: :shipit:",
 				},
 				{
 					CreatedAt: now.Add(30 * time.Second),
+					UpdatedAt: now.Add(30 * time.Second),
 					Author:    "disapprover",
 					Body:      "I don't like things! :-1:",
 				},
 				{
 					CreatedAt: now.Add(40 * time.Second),
+					UpdatedAt: now.Add(40 * time.Second),
 					Author:    "mhaypenny",
 					Body:      ":+1: my stuff is cool",
 				},
 				{
 					CreatedAt: now.Add(50 * time.Second),
+					UpdatedAt: now.Add(50 * time.Second),
 					Author:    "contributor-author",
 					Body:      ":+1: I added to this PR",
 				},
 				{
 					CreatedAt: now.Add(60 * time.Second),
+					UpdatedAt: now.Add(60 * time.Second),
 					Author:    "contributor-committer",
 					Body:      ":+1: I also added to this PR",
 				},
+				{
+					CreatedAt: now.Add(70 * time.Second),
+					UpdatedAt: now.Add(71 * time.Second),
+					Author:    "comment-editor",
+					Body:      "LGTM :+1: :shipit:",
+				},
 			},
 			ReviewsValue: []*pull.Review{
 				{
 					CreatedAt: now.Add(70 * time.Second),
+					UpdatedAt: now.Add(70 * time.Second),
 					Author:    "disapprover",
 					State:     pull.ReviewChangesRequested,
 					Body:      "I _really_ don't like things!",
 				},
 				{
 					CreatedAt: now.Add(80 * time.Second),
+					UpdatedAt: now.Add(80 * time.Second),
 					Author:    "review-approver",
 					State:     pull.ReviewApproved,
 					Body:      "I LIKE THIS",
 				},
+				{
+					CreatedAt: now.Add(90 * time.Second),
+					UpdatedAt: now.Add(91 * time.Second),
+					Author:    "review-comment-editor",
+					State:     pull.ReviewApproved,
+					Body:      "I LIKE THIS",
+				},
 			},
 			CommitsValue: []*pull.Commit{
 				{
@@ -143,7 +164,7 @@ func TestIsApproved(t *testing.T) {
 				Count: 1,
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users")
 	})
 
 	t.Run("authorCannotApprove", func(t *testing.T) {
@@ -214,7 +235,7 @@ func TestIsApproved(t *testing.T) {
 				},
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users")
 	})
 
 	t.Run("specificOrgApproves", func(t *testing.T) {
@@ -237,7 +258,7 @@ func TestIsApproved(t *testing.T) {
 				},
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users")
 	})
 
 	t.Run("specificOrgsOrUserApproves", func(t *testing.T) {
@@ -276,7 +297,7 @@ func TestIsApproved(t *testing.T) {
 		assertApproved(t, prctx, r, "Approved by comment-approver")
 
 		r.Options.InvalidateOnPush = true
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 4 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 approvals required. Ignored 6 approvals from disqualified users")
 	})
 
 	t.Run("invalidateReviewOnPush", func(t *testing.T) {
@@ -301,7 +322,7 @@ func TestIsApproved(t *testing.T) {
 		assertApproved(t, prctx, r, "Approved by review-approver")
 
 		r.Options.InvalidateOnPush = true
-		assertPending(t, prctx, r, "0/1 approvals required")
+		assertPending(t, prctx, r, "0/1 approvals required. Ignored 1 approval from disqualified users")
 	})
 
 	t.Run("ignoreUpdateMergeAfterReview", func(t *testing.T) {
@@ -328,7 +349,7 @@ func TestIsApproved(t *testing.T) {
 				InvalidateOnPush: true,
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 4 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 approvals required. Ignored 6 approvals from disqualified users")
 
 		r.Options.IgnoreUpdateMerges = true
 		assertApproved(t, prctx, r, "Approved by comment-approver")
@@ -360,7 +381,7 @@ func TestIsApproved(t *testing.T) {
 				},
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 6 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 approvals required. Ignored 8 approvals from disqualified users")
 
 		r.Options.IgnoreUpdateMerges = true
 		assertApproved(t, prctx, r, "Approved by merge-committer")
@@ -382,7 +403,7 @@ func TestIsApproved(t *testing.T) {
 				},
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users")
 
 		r.Options.IgnoreCommitsBy = common.Actors{
 			Users: []string{"comment-approver"},
@@ -406,7 +427,7 @@ func TestIsApproved(t *testing.T) {
 				},
 			},
 		}
-		assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users")
+		assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users")
 	})
 
 	t.Run("ignoreCommitsInvalidateOnPush", func(t *testing.T) {
@@ -436,6 +457,44 @@ func TestIsApproved(t *testing.T) {
 		}
 		assertApproved(t, prctx, r, "Approved by comment-approver")
 	})
+
+	t.Run("ignoreEditedReviewComments", func(t *testing.T) {
+		prctx := basePullContext()
+
+		r := &Rule{
+			Requires: Requires{
+				Count: 1,
+				Actors: common.Actors{
+					Users: []string{"review-comment-editor"},
+				},
+			},
+		}
+
+		assertApproved(t, prctx, r, "Approved by review-comment-editor")
+
+		r.Options.IgnoreEditedComments = true
+
+		assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users")
+	})
+
+	t.Run("ignoreEditedComments", func(t *testing.T) {
+		prctx := basePullContext()
+
+		r := &Rule{
+			Requires: Requires{
+				Count: 1,
+				Actors: common.Actors{
+					Users: []string{"comment-editor"},
+				},
+			},
+		}
+
+		assertApproved(t, prctx, r, "Approved by comment-editor")
+
+		r.Options.IgnoreEditedComments = true
+
+		assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users")
+	})
 }
 
 func newTime(t time.Time) *time.Time {
diff --git a/pull/github_test.go b/pull/github_test.go
index 64dfc93..0a190db 100644
--- a/pull/github_test.go
+++ b/pull/github_test.go
@@ -201,16 +201,19 @@ func TestReviews(t *testing.T) {
 
 	assert.Equal(t, "mhaypenny", reviews[0].Author)
 	assert.Equal(t, expectedTime, reviews[0].CreatedAt)
+	assert.Equal(t, expectedTime, reviews[0].UpdatedAt)
 	assert.Equal(t, ReviewChangesRequested, reviews[0].State)
 	assert.Equal(t, "", reviews[0].Body)
 
 	assert.Equal(t, "bkeyes", reviews[1].Author)
 	assert.Equal(t, expectedTime.Add(time.Second), reviews[1].CreatedAt)
+	assert.Equal(t, expectedTime.Add(time.Second), reviews[1].UpdatedAt)
 	assert.Equal(t, ReviewApproved, reviews[1].State)
 	assert.Equal(t, "the body", reviews[1].Body)
 
 	assert.Equal(t, "jgiannuzzi", reviews[2].Author)
 	assert.Equal(t, expectedTime.Add(-4*time.Second).Add(5*time.Minute), reviews[2].CreatedAt)
+	assert.Equal(t, expectedTime.Add(-4*time.Second).Add(5*time.Minute), reviews[2].UpdatedAt)
 	assert.Equal(t, ReviewCommented, reviews[2].State)
 	assert.Equal(t, "A review comment", reviews[2].Body)
 
@@ -263,14 +266,17 @@ func TestComments(t *testing.T) {
 
 	assert.Equal(t, "bkeyes", comments[0].Author)
 	assert.Equal(t, expectedTime, comments[0].CreatedAt)
+	assert.Equal(t, expectedTime, comments[0].UpdatedAt)
 	assert.Equal(t, ":+1:", comments[0].Body)
 
 	assert.Equal(t, "bulldozer[bot]", comments[1].Author)
 	assert.Equal(t, expectedTime.Add(time.Minute), comments[1].CreatedAt)
+	assert.Equal(t, expectedTime.Add(time.Minute), comments[1].UpdatedAt)
 	assert.Equal(t, "I merge!", comments[1].Body)
 
 	assert.Equal(t, "jgiannuzzi", comments[2].Author)
 	assert.Equal(t, expectedTime.Add(10*time.Minute), comments[2].CreatedAt)
+	assert.Equal(t, expectedTime.Add(10*time.Minute), comments[2].UpdatedAt)
 	assert.Equal(t, "A review comment", comments[2].Body)
 
 	// verify that the commit list is cached
diff --git a/pull/testdata/responses/pull_comments.yml b/pull/testdata/responses/pull_comments.yml
index 44871b4..bbca887 100644
--- a/pull/testdata/responses/pull_comments.yml
+++ b/pull/testdata/responses/pull_comments.yml
@@ -17,7 +17,8 @@
                     "login": "bkeyes"
                   },
                   "body": ":+1:",
-                  "createdAt": "2018-06-27T20:28:22Z"
+                  "createdAt": "2018-06-27T20:28:22Z",
+                  "updatedAt": "2018-06-27T20:28:22Z"
                 }
               ]
             },
@@ -33,7 +34,8 @@
                   },
                   "state": "CHANGES_REQUESTED",
                   "body": "",
-                  "submittedAt": "2018-06-27T20:33:26Z"
+                  "submittedAt": "2018-06-27T20:33:26Z",
+                  "updatedAt": "2018-06-27T20:33:26Z"
                 }
               ]
             }
@@ -60,7 +62,8 @@
                     "login": "bulldozer"
                   },
                   "body": "I merge!",
-                  "createdAt": "2018-06-27T20:29:22Z"
+                  "createdAt": "2018-06-27T20:29:22Z",
+                  "updatedAt": "2018-06-27T20:29:22Z"
                 }
               ]
             },
@@ -76,7 +79,8 @@
                   },
                   "state": "APPROVED",
                   "body": "the body",
-                  "submittedAt": "2018-06-27T20:33:27Z"
+                  "submittedAt": "2018-06-27T20:33:27Z",
+                  "updatedAt": "2018-06-27T20:33:27Z"
                 },
                 {
                   "author": {
@@ -84,7 +88,8 @@
                   },
                   "state": "COMMENTED",
                   "body": "A review comment",
-                  "submittedAt": "2018-06-27T20:38:22Z"
+                  "submittedAt": "2018-06-27T20:38:22Z",
+                  "updatedAt": "2018-06-27T20:38:22Z"
                 }
               ]
             }
diff --git a/pull/testdata/responses/pull_reviews.yml b/pull/testdata/responses/pull_reviews.yml
index 173372e..3bee176 100644
--- a/pull/testdata/responses/pull_reviews.yml
+++ b/pull/testdata/responses/pull_reviews.yml
@@ -17,7 +17,8 @@
                   },
                   "state": "CHANGES_REQUESTED",
                   "body": "",
-                  "submittedAt": "2018-06-27T20:33:26Z"
+                  "submittedAt": "2018-06-27T20:33:26Z",
+                  "updatedAt": "2018-06-27T20:33:26Z"
                 }
               ]
             }
@@ -44,7 +45,8 @@
                   },
                   "state": "APPROVED",
                   "body": "the body",
-                  "submittedAt": "2018-06-27T20:33:27Z"
+                  "submittedAt": "2018-06-27T20:33:27Z",
+                  "updatedAt": "2018-06-27T20:33:27Z"
                 },
                 {
                   "author": {
@@ -52,7 +54,8 @@
                   },
                   "state": "COMMENTED",
                   "body": "A review comment",
-                  "submittedAt": "2018-06-27T20:38:22Z"
+                  "submittedAt": "2018-06-27T20:38:22Z",
+                  "updatedAt": "2018-06-27T20:38:22Z"
                 }
               ]
             }
diff --git a/pull/testdata/responses/pull_reviews_comments.yml b/pull/testdata/responses/pull_reviews_comments.yml
index b538626..d1120da 100644
--- a/pull/testdata/responses/pull_reviews_comments.yml
+++ b/pull/testdata/responses/pull_reviews_comments.yml
@@ -17,7 +17,8 @@
                     "login": "bkeyes"
                   },
                   "body": ":+1:",
-                  "createdAt": "2018-06-27T20:28:22Z"
+                  "createdAt": "2018-06-27T20:28:22Z",
+                  "updatedAt": "2018-06-27T20:28:22Z"
                 },
                 {
                   "author": {
@@ -25,7 +26,8 @@
                     "login": "mhaypenny"
                   },
                   "body": "I comment!",
-                  "createdAt": "2018-06-27T20:28:22Z"
+                  "createdAt": "2018-06-27T20:28:22Z",
+                  "updatedAt": "2018-06-27T20:28:22Z"
                 }
               ]
             },
@@ -41,7 +43,8 @@
                   },
                   "state": "CHANGES_REQUESTED",
                   "body": "",
-                  "submittedAt": "2018-06-27T20:33:26Z"
+                  "submittedAt": "2018-06-27T20:33:26Z",
+                  "updatedAt": "2018-06-27T20:33:26Z"
                 }
               ]
             }
@@ -75,7 +78,8 @@
                   },
                   "state": "APPROVED",
                   "body": "the body",
-                  "submittedAt": "2018-06-27T20:33:27Z"
+                  "submittedAt": "2018-06-27T20:33:27Z",
+                  "updatedAt": "2018-06-27T20:33:27Z"
                 },
                 {
                   "author": {
@@ -83,7 +87,8 @@
                   },
                   "state": "COMMENTED",
                   "body": "A review comment",
-                  "submittedAt": "2018-06-27T20:38:22Z"
+                  "submittedAt": "2018-06-27T20:38:22Z",
+                  "updatedAt": "2018-06-27T20:38:22Z"
                 }
               ]
             }