Add PRESUBMIT rule to make sure IPC changes are security reviewed.

PRESUBMIT.py

Index: PRESUBMIT.py
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index 3ab9bc268681a4f3fa824f338b6a0388fe8efbe2..16d9e3ae28ef054ec1fac157002f0974ab8e2720 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -428,10 +428,10 @@ def _CheckDCHECK_IS_ONHasBraces(input_api, output_api):
       continue
     for lnum, line in f.ChangedContents():
       if input_api.re.search(pattern, line):
-          errors.append(output_api.PresubmitError(
-            ('%s:%d: Use of DCHECK_IS_ON() must be written as "#if ' +
-             'DCHECK_IS_ON()", not forgetting the braces.')
-            % (f.LocalPath(), lnum)))
+        errors.append(output_api.PresubmitError(
+          ('%s:%d: Use of DCHECK_IS_ON() must be written as "#if ' +
+           'DCHECK_IS_ON()", not forgetting the braces.')
+          % (f.LocalPath(), lnum)))
   return errors
 
 
@@ -590,11 +590,11 @@ def _CheckNoBannedFunctions(input_api, output_api):
       if input_api.re.search(regex, line):
         matched = True
     elif func_name in line:
-        matched = True
+      matched = True
     if matched:
-      problems = warnings;
+      problems = warnings
       if error:
-        problems = errors;
+        problems = errors
       problems.append('    %s:%d:' % (affected_file.LocalPath(), line_num))
       for message_line in message:
         problems.append('      %s' % message_line)
@@ -1399,6 +1399,66 @@ def _CheckJavaStyle(input_api, output_api):
       black_list=_EXCLUDED_PATHS + input_api.DEFAULT_BLACK_LIST)
 
 
+def _CheckIpcOwners(input_api, output_api):
+  file_patterns = [

[Dirk Pranke, 2016/06/17 05:45:52]

Please add a docstring or a comment here describing what this is checking for.

Also, please add some comments describing the algorithm, since I'm not really
following it and it's not clear to me that it's correct. We should probably add
some unit tests for it to PRESUBMIT_test.py, too.

[dcheng, 2016/06/17 22:01:30]

Done (and added comments throughout as well).

Not sure how to write a unit test for this, since we use open(). Is there an
example of how to do this?

+      '*_messages.cc',
+      '*_messages*.h',
+      '*_param_traits*.*',
+      '*.mojom',
+      '*_struct_traits*.*',
+      '*_type_converters*.*',
+  ]
+
+  to_check = {}
+
+  for f in input_api.change.AffectedFiles():
+    for pattern in file_patterns:
+      if input_api.fnmatch.fnmatch(
+          input_api.os_path.basename(f.LocalPath()), pattern):
+        owners_file = input_api.os_path.join(
+            input_api.os_path.dirname(f.LocalPath()), 'OWNERS')
+        if owners_file not in to_check:
+          to_check[owners_file] = {}
+        if pattern not in to_check[owners_file]:
+          to_check[owners_file][pattern] = {
+              'files': [],
+              'missing_lines': [
+                  'per-file %s=set noparent' % pattern,
+                  'per-file %s=file://ipc/SECURITY_OWNERS' % pattern,
+              ]
+          }
+        to_check[owners_file][pattern]['files'].append(f)
+        break
+
+  for owners_file, patterns in to_check.iteritems():
+    with file(owners_file) as f:

[Dirk Pranke, 2016/06/17 05:45:52]

Make sure the file exists (or at least handle a missing file cleanly).

[dcheng, 2016/06/17 22:01:30]

Done.

+      for line in f.read().splitlines():
+        for entry in patterns.itervalues():
+          entry['missing_lines'] = [missing
+                                    for missing in entry['missing_lines']
+                                    if missing != line]
+
+  errors = []
+  for owners_file, patterns in to_check.iteritems():
+    missing_lines = []
+    files = []
+    for pattern, entry in patterns.iteritems():
+      missing_lines.extend(entry['missing_lines'])
+      files.extend(['  %s' % f.LocalPath() for f in entry['files']])
+    if missing_lines:
+      errors.append(
+          '%s is missing the following lines:\n\n%s\n\nfor changed files:\n%s' %
+          (owners_file, '\n'.join(missing_lines), '\n'.join(files)))
+
+  results = []
+  if errors:
+    results.append(output_api.PresubmitError(
+        'Found changes to IPC files without a security OWNER!',
+        long_text='\n\n'.join(errors)))
+
+  return results
+
+
 def _CheckAndroidToastUsage(input_api, output_api):
   """Checks that code uses org.chromium.ui.widget.Toast instead of
      android.widget.Toast (Chromium Toast doesn't force hardware
@@ -1862,6 +1922,7 @@ def _CommonChecks(input_api, output_api):
   results.extend(_CheckNoDeprecatedCompiledResourcesGYP(input_api, output_api))
   results.extend(_CheckPydepsNeedsUpdating(input_api, output_api))
   results.extend(_CheckJavaStyle(input_api, output_api))
+  results.extend(_CheckIpcOwners(input_api, output_api))
 
   if any('PRESUBMIT.py' == f.LocalPath() for f in input_api.AffectedFiles()):
     results.extend(input_api.canned_checks.RunUnitTestsInDirectory(
@@ -1875,9 +1936,6 @@ def _CheckAuthorizedAuthor(input_api, output_api):
   """For non-googler/chromites committers, verify the author's email address is
   in AUTHORS.
   """
-  # TODO(maruel): Add it to input_api?
-  import fnmatch
-
   author = input_api.change.author_email
   if not author:
     input_api.logging.info('No author, skipping AUTHOR check')
@@ -1888,7 +1946,8 @@ def _CheckAuthorizedAuthor(input_api, output_api):
       input_api.re.match(r'[^#]+\s+\<(.+?)\>\s*$', line)
       for line in open(authors_path))
   valid_authors = [item.group(1).lower() for item in valid_authors if item]
-  if not any(fnmatch.fnmatch(author.lower(), valid) for valid in valid_authors):
+  if not any(input_api.fnmatch.fnmatch(author.lower(), valid)
+             for valid in valid_authors):
     input_api.logging.info('Valid authors are %s', ', '.join(valid_authors))
     return [output_api.PresubmitPromptWarning(
         ('%s is not in AUTHORS file. If you are a new contributor, please visit'