Skip to content

Commit

Permalink
Don't require approval during CQ+1 for security owners check.
Browse files Browse the repository at this point in the history 
CQ+1 runs the presubmit commit hooks, but it is not useful for the
missing security reviewers check to require approval, since there may be
reviewers that have yet to respond. Only CQ+2 should generate an error
for a missing approval.

Bug: 801315
Change-Id: I76ed35d515987a1efb9a8a2344210bc601a22849
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3646825
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Giovanni Ortuno Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1002990}
  • Loading branch information
@zetafunction
zetafunction authored and Chromium LUCI CQ committed May 13, 2022
1 parent af0fc4d commit 3008dc1
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 29 deletions.
23 changes: 17 additions & 6 deletions PRESUBMIT.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2246,6 +2246,8 @@ def CheckAddedDepsHaveTargetApprovals(input_api, output_api):
'--tbr was specified, skipping OWNERS check for DEPS additions'
)
]
# TODO(dcheng): Make this generate an error on dry runs if the reviewer
# is not added, to prevent review serialization.
if input_api.dry_run:
return [
output_api.PresubmitNotifyResult(
Expand Down Expand Up @@ -2768,7 +2770,9 @@ def _ChangeHasSecurityReviewer(input_api, owners_file):
"""
owner_email, reviewers = (
input_api.canned_checks.GetCodereviewOwnerAndReviewers(
input_api, None, approval_needed=input_api.is_committing))
input_api,
None,
approval_needed=input_api.is_committing and not input_api.dry_run))

security_owners = input_api.owners_client.ListOwners(owners_file)
return any(owner in reviewers for owner in security_owners)
Expand Down Expand Up @@ -3010,10 +3014,6 @@ def CheckSecurityOwners(input_api, output_api):
output_api.AppendCC('ipc-security-reviews@chromium.org')

results = []
if input_api.is_committing:
make_presubmit_message = output_api.PresubmitError
else:
make_presubmit_message = output_api.PresubmitPromptWarning

# Ensure that a security reviewer is included as a CL reviewer. This is a
# hack, but is needed because the OWNERS check (by design) ignores new
Expand All @@ -3024,6 +3024,12 @@ def CheckSecurityOwners(input_api, output_api):
missing_reviewer_errors.extend(fuchsia_results.missing_reviewer_errors)

if missing_reviewer_errors:
# Missing reviewers are only a warning at upload time; otherwise, it'd
# be impossible to upload a change.
if input_api.is_committing:
make_presubmit_message = output_api.PresubmitError
else:
make_presubmit_message = output_api.PresubmitPromptWarning
results.append(
make_presubmit_message(
'Found missing security reviewers:',
Expand All @@ -3034,8 +3040,13 @@ def CheckSecurityOwners(input_api, output_api):
owners_file_errors.extend(fuchsia_results.owners_file_errors)

if owners_file_errors:
# Missing per-file rules are always an error. While swarming and caching
# means that uploading a patchset with updated OWNERS files and sending
# it to the CQ again should not have a large incremental cost, it is
# still frustrating to discover the error only after the change has
# already been uploaded.
results.append(
make_presubmit_message(
output_api.PresubmitError(
'Found OWNERS files with missing per-file rules for '
'security-sensitive files.\nPlease update the OWNERS files '
'below to add the missing rules:',
Expand Down
121 changes: 98 additions & 23 deletions PRESUBMIT_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2323,6 +2323,71 @@ class IpcSecurityOwnerTest(_SecurityOwnersTestCase):

# TODO(dcheng): add tests for when there are no missing per-file rules. These
# are currently missing because `open()` is not injected.
def testMissingSecurityReviewerAtUpload(self):
mock_input_api = MockInputApi()
mock_input_api.files = [
MockAffectedFile(f'services/goat/public/goat.mojom',
['// Scary contents.'])]
self._injectFakeOwnersClient(
mock_input_api,
['apple@chromium.org', 'orange@chromium.org'])
self._injectFakeChangeOwnerAndReviewers(
mock_input_api, 'owner@chromium.org', ['banana@chromium.org'])
mock_input_api.is_committing = False
mock_input_api.dry_run = False
mock_output_api = MockOutputApi()
results = PRESUBMIT.CheckSecurityOwners(
mock_input_api, mock_output_api)
# TODO(dcheng): This should be 1, but the PRESUBMIT currently opens the
# OWNERS file in an unmockable way.
self.assertEqual(2, len(results))
self.assertEqual('warning', results[0].type)
self.assertEqual(
'Found missing security reviewers:', results[0].message)

def testMissingSecurityReviewerAtDryRunCommit(self):
mock_input_api = MockInputApi()
mock_input_api.files = [
MockAffectedFile(f'services/goat/public/goat.mojom',
['// Scary contents.'])]
self._injectFakeOwnersClient(
mock_input_api,
['apple@chromium.org', 'orange@chromium.org'])
self._injectFakeChangeOwnerAndReviewers(
mock_input_api, 'owner@chromium.org', ['banana@chromium.org'])
mock_input_api.is_committing = True
mock_input_api.dry_run = True
mock_output_api = MockOutputApi()
results = PRESUBMIT.CheckSecurityOwners(
mock_input_api, mock_output_api)
# TODO(dcheng): This should be 1, but the PRESUBMIT currently opens the
# OWNERS file in an unmockable way.
self.assertEqual(2, len(results))
self.assertEqual('error', results[0].type)
self.assertEqual(
'Found missing security reviewers:', results[0].message)

def testmissingSecurityApprovalAtRealCommit(self):
mock_input_api = MockInputApi()
mock_input_api.files = [
MockAffectedFile(f'services/goat/public/goat.mojom',
['// Scary contents.'])]
self._injectFakeOwnersClient(
mock_input_api,
['apple@chromium.org', 'orange@chromium.org'])
self._injectFakeChangeOwnerAndReviewers(
mock_input_api, 'owner@chromium.org', ['banana@chromium.org'])
mock_input_api.is_committing = True
mock_input_api.dry_run = False
mock_output_api = MockOutputApi()
results = PRESUBMIT.CheckSecurityOwners(
mock_input_api, mock_output_api)
# TODO(dcheng): This should be 1, but the PRESUBMIT currently opens the
# OWNERS file in an unmockable way.
self.assertEqual(2, len(results))
self.assertEqual('error', results[0].type)
self.assertEqual(
'Found missing security reviewers:', results[0].message)

def testMissingPerFileRulesButNotSecurityReviewer(self):
for pattern, filename in self._test_cases:
Expand All @@ -2348,29 +2413,38 @@ def testMissingPerFileRulesButNotSecurityReviewer(self):
mock_output_api.more_cc)

def testIpcChangeNeedsSecurityOwner(self):
for pattern, filename in self._test_cases:
with self.subTest(line=filename):
mock_input_api = MockInputApi()
mock_input_api.files = [
MockAffectedFile(f'services/goat/public/{filename}',
['// Scary contents.'])]
self._injectFakeOwnersClient(
mock_input_api,
['apple@chromium.org', 'orange@chromium.org'])
self._injectFakeChangeOwnerAndReviewers(
mock_input_api, 'owner@chromium.org', ['banana@chromium.org'])
mock_output_api = MockOutputApi()
errors = PRESUBMIT.CheckSecurityOwners(
mock_input_api, mock_output_api)
self.assertEqual(2, len(errors))
self.assertEqual(
'Found missing security reviewers:', errors[0].message)
self.assertEqual(
'Found OWNERS files with missing per-file rules for '
'security-sensitive files.\nPlease update the OWNERS files below '
'to add the missing rules:', errors[1].message)
self.assertEqual(['ipc-security-reviews@chromium.org'],
mock_output_api.more_cc)
for is_committing in [True, False]:
for pattern, filename in self._test_cases:
with self.subTest(
line=f'is_committing={is_committing}, filename={filename}'):
mock_input_api = MockInputApi()
mock_input_api.files = [
MockAffectedFile(f'services/goat/public/{filename}',
['// Scary contents.'])]
self._injectFakeOwnersClient(
mock_input_api,
['apple@chromium.org', 'orange@chromium.org'])
self._injectFakeChangeOwnerAndReviewers(
mock_input_api, 'owner@chromium.org', ['banana@chromium.org'])
mock_input_api.is_committing = is_committing
mock_input_api.dry_run = False
mock_output_api = MockOutputApi()
results = PRESUBMIT.CheckSecurityOwners(
mock_input_api, mock_output_api)
self.assertEqual(2, len(results))
if is_committing:
self.assertEqual('error', results[0].type)
else:
self.assertEqual('warning', results[0].type)
self.assertEqual(
'Found missing security reviewers:', results[0].message)
self.assertEqual('error', results[1].type)
self.assertEqual(
'Found OWNERS files with missing per-file rules for '
'security-sensitive files.\nPlease update the OWNERS files below '
'to add the missing rules:', results[1].message)
self.assertEqual(['ipc-security-reviews@chromium.org'],
mock_output_api.more_cc)


def testServiceManifestChangeNeedsSecurityOwner(self):
Expand Down Expand Up @@ -2590,6 +2664,7 @@ def testChangeOwnersMissingAtCommit(self):
self._injectFakeChangeOwnerAndReviewers(
mock_input_api, 'owner@chromium.org', ['banana@chromium.org'])
mock_input_api.is_committing = True
mock_input_api.dry_run = False
mock_input_api.files = [
MockAffectedFile('file.cc', ['GetServiceSandboxType<mojom::Goat>()'])
]
Expand Down

0 comments on commit 3008dc1

Please sign in to comment.