From 8678d329377122b86a647ae8f08c3351beeb4ba5 Mon Sep 17 00:00:00 2001
From: Aaron Gable <agable@chromium.org>
Date: Mon, 2 Apr 2018 13:28:19 -0700
Subject: [PATCH] Disallow TBRs to OWNERS files

TBRs bypass owners checks, but are also semi-suspicious: a
contributor TBRing many changes in a row to submit code to
a directory they don't own would be seen, frowned upon, and
inquired into. However, a contributor could bypass this by
simply TBRing a single change to add themselves to an OWNERS
file, and then contributing as normal from there. This CL
removes that loophole.

This will not affect sheriffs who TBR reverts for two reasons:
first, it is rare that a chance touches both code and an
OWNERS file, and therefore it is rare that OWNERS changes
get reverted; second, quick reverts (the kind sheriffs do)
bypass PRESUBMIT entirely, and therefore also skip OWNERS
checks.

Bug: 688115
Change-Id: If2b5c9d058c62caf95389287e0bb706aef721baf
Reviewed-on: https://chromium-review.googlesource.com/982601
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
---
 presubmit_canned_checks.py  |  8 ++++----
 tests/presubmit_unittest.py | 19 +++++++++++++------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py
index 4232a5915..f1f61834f 100644
--- a/presubmit_canned_checks.py
+++ b/presubmit_canned_checks.py
@@ -827,8 +827,11 @@ def CheckBuildbotPendingBuilds(input_api, output_api, url, max_pendings,
 
 
 def CheckOwners(input_api, output_api, source_file_filter=None):
+  affected_files = set([f.LocalPath() for f in
+      input_api.change.AffectedFiles(file_filter=source_file_filter)])
+
   if input_api.is_committing:
-    if input_api.tbr:
+    if input_api.tbr and not any(['OWNERS' in name for name in affected_files]):
       return [output_api.PresubmitNotifyResult(
           '--tbr was specified, skipping OWNERS check')]
     needed = 'LGTM from an OWNER'
@@ -846,9 +849,6 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
     needed = 'OWNER reviewers'
     output_fn = output_api.PresubmitNotifyResult
 
-  affected_files = set([f.LocalPath() for f in
-      input_api.change.AffectedFiles(file_filter=source_file_filter)])
-
   owners_db = input_api.owners_db
   owners_db.override_files = input_api.change.OriginalOwnersFiles()
   owner_email, reviewers = GetCodereviewOwnerAndReviewers(
diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py
index 9c10e94a6..e3894db7b 100755
--- a/tests/presubmit_unittest.py
+++ b/tests/presubmit_unittest.py
@@ -2398,9 +2398,9 @@ class CannedChecksUnittest(PresubmitTestsBase):
         presubmit.OutputApi.PresubmitNotifyResult)
 
   def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None,
-      reviewers=None, is_committing=True,
-      response=None, uncovered_files=None, expected_output='',
-      manually_specified_reviewers=None, dry_run=None):
+      reviewers=None, is_committing=True, response=None, uncovered_files=None,
+      expected_output='', manually_specified_reviewers=None, dry_run=None,
+      modified_file='foo/xyz.cc'):
     if approvers is None:
       # The set of people who lgtm'ed a change.
       approvers = set()
@@ -2438,9 +2438,9 @@ class CannedChecksUnittest(PresubmitTestsBase):
     input_api.tbr = tbr
     input_api.dry_run = dry_run
 
-    if not is_committing or (not tbr and issue):
-      affected_file.LocalPath().AndReturn('foo/xyz.cc')
-      change.AffectedFiles(file_filter=None).AndReturn([affected_file])
+    affected_file.LocalPath().AndReturn(modified_file)
+    change.AffectedFiles(file_filter=None).AndReturn([affected_file])
+    if not is_committing or (not tbr and issue) or ('OWNERS' in modified_file):
       change.OriginalOwnersFiles().AndReturn({})
       if issue and not response:
         response = {
@@ -2721,6 +2721,13 @@ class CannedChecksUnittest(PresubmitTestsBase):
         expected_output='--tbr was specified, skipping OWNERS check\n')
     self.AssertOwnersWorks(tbr=True, is_committing=False, expected_output='')
 
+  def testCannedCheckOwners_TBROWNERSFile(self):
+    self.AssertOwnersWorks(
+        tbr=True, uncovered_files=set(['foo']),
+        modified_file='foo/OWNERS',
+        expected_output='Missing LGTM from an OWNER for these files:\n'
+                        '    foo\n')
+
   def testCannedCheckOwners_WithoutOwnerLGTM(self):
     self.AssertOwnersWorks(uncovered_files=set(['foo']),
         expected_output='Missing LGTM from an OWNER for these files:\n'