0

Add presubmit coverage for C++ service manifests

Extends PRESUBMIT.py's IPC coverage checks to be concerned with C++
source and header files which match the filename patterns
*manifest.(cc|h) and *manifests.(cc|h), making an exception only for
*test_manifest.(cc|h) and *test_manifests.(cc|h).

Files which match according to filename are then additionally filtered
by contents, expecting at least one qualified reference to
service_manager::Manifest to be treated as a service manifest.

Bug: 895616
Change-Id: I7aae5a7535fb004e370569cc4672ad49c4c3752f
Reviewed-on: https://chromium-review.googlesource.com/c/1381248
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618563}
This commit is contained in:
Ken Rockot
2018-12-21 18:56:36 +00:00
committed by Commit Bot
parent 59ec80f939
commit 9f668264e8
2 changed files with 42 additions and 4 deletions

@ -2082,7 +2082,8 @@ def _GetOwnersFilesToCheckForIpcOwners(input_api):
# *.mojom files.
for f in input_api.AffectedFiles(include_deletes=False):
# Manifest files don't have a strong naming convention. Instead, scan
# affected files for .json files and see if they look like a manifest.
# affected files for .json, .cc, and .h files which look like they contain
# a manifest definition.
if (f.LocalPath().endswith('.json') and
not _MatchesFile(input_api, _KNOWN_INVALID_JSON_FILE_PATTERNS,
f.LocalPath())):
@ -2098,6 +2099,15 @@ def _GetOwnersFilesToCheckForIpcOwners(input_api):
continue
if 'interface_provider_specs' in json_content:
AddPatternToCheck(f, input_api.os_path.basename(f.LocalPath()))
else:
manifest_pattern = input_api.re.compile('manifests?\.(cc|h)$')
test_manifest_pattern = input_api.re.compile('test_manifests?\.(cc|h)')
if (manifest_pattern.search(f.LocalPath()) and not
test_manifest_pattern.search(f.LocalPath())):
# We expect all actual service manifest files to contain at least one
# qualified reference to service_manager::Manifest.
if 'service_manager::Manifest' in '\n'.join(f.NewContents()):
AddPatternToCheck(f, input_api.os_path.basename(f.LocalPath()))
for pattern in file_patterns:
if input_api.fnmatch.fnmatch(
input_api.os_path.basename(f.LocalPath()), pattern):

@ -1682,8 +1682,8 @@ class CorrectProductNameInMessagesTest(unittest.TestCase):
'chrome/app/chromium_strings.grd:8' in warnings[1].items[1])
class MojoManifestOwnerTest(unittest.TestCase):
def testMojoManifestChangeNeedsSecurityOwner(self):
class ServiceManifestOwnerTest(unittest.TestCase):
def testServiceManifestJsonChangeNeedsSecurityOwner(self):
mock_input_api = MockInputApi()
mock_input_api.files = [
MockAffectedFile('services/goat/manifest.json',
@ -1706,7 +1706,7 @@ class MojoManifestOwnerTest(unittest.TestCase):
# No warning if already covered by an OWNERS rule.
def testNonManifestChangesDoNotRequireSecurityOwner(self):
def testNonManifestJsonChangesDoNotRequireSecurityOwner(self):
mock_input_api = MockInputApi()
mock_input_api.files = [
MockAffectedFile('services/goat/species.json',
@ -1722,6 +1722,34 @@ class MojoManifestOwnerTest(unittest.TestCase):
mock_input_api, mock_output_api)
self.assertEqual([], errors)
def testServiceManifestChangeNeedsSecurityOwner(self):
mock_input_api = MockInputApi()
mock_input_api.files = [
MockAffectedFile('services/goat/public/cpp/manifest.cc',
[
'#include "services/goat/public/cpp/manifest.h"',
'const service_manager::Manifest& GetManifest() {}',
])]
mock_output_api = MockOutputApi()
errors = PRESUBMIT._CheckIpcOwners(
mock_input_api, mock_output_api)
self.assertEqual(1, len(errors))
self.assertEqual(
'Found OWNERS files that need to be updated for IPC security review ' +
'coverage.\nPlease update the OWNERS files below:', errors[0].message)
def testNonServiceManifestSourceChangesDoNotRequireSecurityOwner(self):
mock_input_api = MockInputApi()
mock_input_api.files = [
MockAffectedFile('some/non/service/thing/foo_manifest.cc',
[
'const char kNoEnforcement[] = "not a manifest!";',
])]
mock_output_api = MockOutputApi()
errors = PRESUBMIT._CheckIpcOwners(
mock_input_api, mock_output_api)
self.assertEqual([], errors)
class BannedFunctionCheckTest(unittest.TestCase):