Implement mail dispatcher app. This adds the mail python app that handles incoming mail and dispatches it to handlers, as well as a handler implementing automated commenting for changes to chrome/app/policy/policy_templates.json. R=agable@chromium.org Review URL: https://codereview.chromium.org/20518002 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/reviewbot@219005 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 28ff4db..8fe6f00 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py
@@ -8,6 +8,33 @@ details on the presubmit API built into gcl. """ +def GetUnitTests(input_api, output_api, pythonpath): + """Finds all unit tests to run within the source tree.""" + tests = [] + for root, dirs, files in input_api.os_walk(input_api.PresubmitLocalPath()): + # Don't recurse in blacklisted directories. + blacklist = ['.svn', '.git', 'third_party'] + dirs[:] = filter(lambda d : d not in blacklist, dirs) + + # Add all files ending in _test.py to the list of tests. + test_files = filter(lambda f: f.endswith('_test.py'), files) + tests.extend([input_api.os_path.join(root, f) for f in test_files]) + + # A helper to augment PYTHONPATH in the unit test command environment. + def augment_pythonpath_in_environment(cmd): + env = cmd.kwargs.get('env', {}) + new_env_path = input_api.os_path.pathsep.join(pythonpath) + if 'PYTHONPATH' in env: + new_env_path += input_api.os_path.pathsep + env['PYTHONPATH'] + env['PYTHONPATH'] = new_env_path + cmd.kwargs['env'] = env + return cmd + + # Create the commands. + return map( + augment_pythonpath_in_environment, + input_api.canned_checks.GetUnitTests(input_api, output_api, tests)) + def CommonChecks(input_api, output_api): output = [] @@ -32,19 +59,17 @@ join(root, 'google_appengine', 'lib', 'webapp2-2.5.2'), join(root, 'google_appengine', 'lib', 'webob-1.2.3'), join('third_party', 'google-api-python-client'), + input_api.PresubmitLocalPath(), ] + sys.path - output.extend(input_api.canned_checks.RunPylint( - input_api, - output_api)) + + output.extend(input_api.RunTests( + input_api.canned_checks.GetPylint( + input_api, + output_api) + + GetUnitTests(input_api, output_api, sys.path))) finally: sys.path = sys_path_backup - output.extend(input_api.canned_checks.RunUnitTestsInDirectory( - input_api, - output_api, - '.', - whitelist=[r'.*_test\.py$'])) - return output
diff --git a/app.yaml b/app.yaml new file mode 100644 index 0000000..3a6d210 --- /dev/null +++ b/app.yaml
@@ -0,0 +1,31 @@ +application: reviewbot +version: 1 +runtime: python27 +api_version: 1 +threadsafe: true + +inbound_services: +- mail + +handlers: +- url: /_ah/mail/.*@.*\.appspotmail\.com$ + script: mail_dispatcher.app + login: admin +- url: /admin/app_config + script: app_config.app + login: admin + +libraries: +- name: jinja2 + version: 2.6 +- name: pycrypto + version: 2.6 +- name: webapp2 + version: 2.5.2 +- name: webob + version: 1.2.3 + +admin_console: + pages: + - name: App Configuration + url: /admin/app_config
diff --git a/handlers/__init__.py b/handlers/__init__.py new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/handlers/__init__.py
diff --git a/handlers/policy_checklist/__init__.py b/handlers/policy_checklist/__init__.py new file mode 100644 index 0000000..b3da348 --- /dev/null +++ b/handlers/policy_checklist/__init__.py
@@ -0,0 +1,5 @@ +# Copyright (c) 2013 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +from handlers.policy_checklist.handler import process
diff --git a/handlers/policy_checklist/addition_comment.txt b/handlers/policy_checklist/addition_comment.txt new file mode 100644 index 0000000..8640ac9 --- /dev/null +++ b/handlers/policy_checklist/addition_comment.txt
@@ -0,0 +1,33 @@ +When adding a new policy setting, please consider the following: + +- Ideally, your new policy should be expressing the intention of an + administrator and/or cover a well-scoped user-visible behavior of the + browser as opposed to an implementation-specific parameter. Consider + that your implementation may change in the future, and you might have + to re-implement your policy on top of the new feature implementation. +- Make sure your policy definition has proper supported_on declarations + specifying the platform and versions this policy is supported on. +- Make sure feature flags are correct. In particular: + - dynamic_refresh - whether your feature can adjust to changes in + policy at run time. You typically use PrefChangeRegistrar to do so. + - per_profile - whether your policy is browser-global or can be set + independently for each Chrome Profile. This is usually true if you + read the policy value from the Profile's PrefService and false if + you read it from the local_state PrefService. + - can_be_recommended - whether your feature supports admin-supplied + default values that the user can override. +- Make sure you put a helpful policy description: + - Describe the effect of setting the policy in a way that makes sense + to somebody who is not intimately familiar with your feature, such + as administrators and other Chromium developers. + - Do mention behavior for the cases where the policy gets ignored + (i.e. when not configured and for booleans where only one value is + effective). It's nice for completeness and admins have been asking + specifically for this piece of information in the past. +- Write a browser_test for you new policy. The ideal test would fire up + the browser with the policy set and check whether the policy affects + user-visible behavior in the intended way. See + chrome/browser/policy/policy_browsertest.cc for examples. +- If you're adding a device policy for Chrome OS, be sure to update + chrome/browser/chromeos/policy/device_policy_decoder_chromeos.{h,cc} + so the policy shows up on the chrome://policy page.
diff --git a/handlers/policy_checklist/handler.py b/handlers/policy_checklist/handler.py new file mode 100644 index 0000000..ec26e37 --- /dev/null +++ b/handlers/policy_checklist/handler.py
@@ -0,0 +1,81 @@ +# Copyright (c) 2013 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +"""Handler for chrome/app/policy/policy_templates.json + +This handler examines changes to the policy_templates.json file and posts +comments with checklists for the patch author and reviewer to go through to +avoid common pitfalls. +""" + +import os + +import jinja2 + +import model.app_config +import util +import handlers.policy_checklist.parser + + +POLICY_TEMPLATES_FILE = 'chrome/app/policy/policy_templates.json' +MAX_INLINE_COMMENTS = 10 + + +REVIEW_MESSAGE_TEMPLATE = 'review_message.txt' +ADDITION_COMMENT_TEMPLATE = 'addition_comment.txt' +MODIFICATION_COMMENT_TEMPLATE = 'modification_comment.txt' + + +JINJA_ENVIRONMENT = jinja2.Environment( + loader=jinja2.FileSystemLoader(os.path.dirname(__file__))) + + +def process(addr, message, review, rietveld): + """Handles reviews for chrome/app/policy/policy_templates.json. + + This looks at the patch to identify additions/modifications to policy + definitions and posts comments with a checklist intended for the author and + reviewer to go through in order to catch common mistakes. + """ + + if POLICY_TEMPLATES_FILE not in review.latest_patchset.files: + return + + # Only process the change if the mail is directly to us or we haven't + # processed this review yet. + client_id = model.app_config.get().client_id + if (not addr in util.get_emails(getattr(message, 'to', '')) and + client_id in [m.sender for m in review.issue_data.messages]): + return + + # Don't process reverts. + if 'revert' in review.issue_data.description.lower(): + return + + # Parse the patch, look at the chunks and generate inline comments. + chunks = handlers.policy_checklist.parser.parse( + review.latest_patchset.files[POLICY_TEMPLATES_FILE].patch.lines) + for chunk in chunks[0:MAX_INLINE_COMMENTS]: + if chunk.additions and not chunk.removals: + template = JINJA_ENVIRONMENT.get_template(ADDITION_COMMENT_TEMPLATE) + else: + template = JINJA_ENVIRONMENT.get_template(MODIFICATION_COMMENT_TEMPLATE) + + if chunk.comment_pos[1] is not None: + line, side = chunk.comment_pos[1], 'b' + elif chunk.comment_pos[0] is not None: + line, side = chunk.comment_pos[0], 'a' + else: + # No suitable position? + continue + + rietveld.add_inline_comment( + review.issue_id, review.latest_patchset.patchset, + review.latest_patchset.files[POLICY_TEMPLATES_FILE].id, + line, side, template.render(review=review, chunk=chunk)) + + # Finally, post all inline comments. + if len(chunks) > 0: + template = JINJA_ENVIRONMENT.get_template(REVIEW_MESSAGE_TEMPLATE) + rietveld.post_comment(review.issue_id, template.render(review=review), True)
diff --git a/handlers/policy_checklist/modification_comment.txt b/handlers/policy_checklist/modification_comment.txt new file mode 100644 index 0000000..dd26a11 --- /dev/null +++ b/handlers/policy_checklist/modification_comment.txt
@@ -0,0 +1,17 @@ +When modifying an existing policy setting, please consider the following: + +- Make sure the policy meta data is up-to-date, in particular + supported_on, and the feature flags. +- In general, don't change policy semantics in a way that is + incompatible (as determined by user/admin-visible behavior) with + previous semantics. In particular, consider that existing policy + deployments may affect both old and new browser versions, and both + should behave according to the admin's intentions. +- It is OK to expand semantics of policy values as long as the previous + policy description is compatible with the new behavior. +- It is OK to update feature implementations and the policy description + when Chrome changes as long as the intended effect of the policy + remains intact. +- The process for removing policies is to deprecate them first, wait a + few releases (if possible) and then remove them. Make sure you put the + deprecated flag if you deprecate a policy.
diff --git a/handlers/policy_checklist/parser.py b/handlers/policy_checklist/parser.py new file mode 100644 index 0000000..1ab5659 --- /dev/null +++ b/handlers/policy_checklist/parser.py
@@ -0,0 +1,174 @@ +# Copyright (c) 2013 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +import re + +import util + + +CONTEXT_THRESHOLD = 12 +PROPERTY_NAME_RE = re.compile(r"^\s*'(\w+)'\s*:") + + +def nmin(*args): + """Calculates the minimum of |args|, ignoring None entries.""" + values = [v for v in args if v is not None] + return None if len(values) == 0 else min(values) + + +def nmax(*args): + """Calculates the maximum of |args|, ignoring None entries.""" + values = [v for v in args if v is not None] + return None if len(values) == 0 else max(values) + + +def nadd(a, b): + """Calculates a + b, returning None if either a or b is None""" + return None if (a is None or b is None) else a + b + + +def nsub(a, b): + """Calculates a - b, returning None if either a or b is None""" + return None if (a is None or b is None) else a - b + + +def get_indentation_level(line): + """Returns the indentation level (number of leading spaces) for |line|.""" + nspaces = len(line) - len(line.lstrip(' ')) + return None if nspaces == 0 else nspaces + + +class PolicyChangeParser(object): + """Parses a policy_templates.json diff to identify logical changes. + + This takes a list of triples of the form (old_line, new_line, text) as + returned by patching.ParsePatchToLines and produces a list of dictionaries + describing the logical changes that have been made. The dictionaries contain + these keys: + * start: A pair (old_line, new_line) indicating where the change starts. + * end: A pair (old_line, new_line) indicating where the change ends. + * comment_pos: A pair (old_line, new_line), indicating a suitable place to + put an inline comment. This is typically the line where the + policy name is found in the diff. + * additions: Whether there have been line additions. + * removals: Whether there have been line removals. + """ + + def __init__(self, lines): + self.lines = lines + self.chunks_list = [] + self.reset() + + def run(self): + """Main parsing function. + + The code goes over the diff line by line, keeping track of the current line. + It keeps track of the current line numbers, and where the last changes + happened in the old and new version of the file. + + Certain events trigger start of a new logical change. These are + discontinuities in the cursor position and decreases of the indentation + level. Once a block closes, the information for that block is recorded in + the result list. + """ + self.chunks_list = [] + self.last_change = [None, None] + cursor = [None, None] + self.reset() + for (a_line, b_line, line) in self.lines: + # Skip comment lines. + if line.startswith('#'): + continue + + # See whether the current line has a JSON property. + keyword = None + match = PROPERTY_NAME_RE.match(line) + if match: + keyword = match.group(1).lower() + + + # Check whether the current block closes. + line_indent = get_indentation_level(line) + if (self.block_indent is not None and + line_indent is not None and + line_indent < self.block_indent): + self.block_closed = True + + # Update various cursors. + cursor = [nmax(a_line, cursor[0]), nmax(b_line, cursor[1])] + offset = nmin(nsub(cursor[0], self.last_change[0]), + nsub(cursor[1], self.last_change[1])) + + # Update change tracking state. + if a_line is not None and b_line is None: + self.removals = True + self.last_change[0] = a_line + self.text_changed |= any([c.isalnum() for c in line]) + elif a_line is None and b_line is not None: + self.additions = True + self.last_change[1] = b_line + self.text_changed |= any([c.isalnum() for c in line]) + + # If the indentation block closes or the last chunk is too far away, + # assume a new one starts. + if (self.block_closed or + (offset is not None and (offset > CONTEXT_THRESHOLD))): + self.flush_chunk() + + # Try to figure out block indent from properties exclusively used for + # policy definitions. + if (self.block_indent is None and + keyword in ('id', 'schema', 'future', 'features', 'supported_on', + 'example_value', 'deprecated')): + self.block_indent = line_indent + + # Put the comment on the policy name property if we see it fly by. + if keyword == 'name': + # Filter out name labels on enum items and schemas. + if self.block_indent is not None and self.block_indent != line_indent: + pass + elif a_line is not None and b_line is None: + self.comment_pos[0] = a_line + elif a_line is None and b_line is not None: + self.comment_pos[1] = b_line + + self.chunk_start = [nmin(self.last_change[0], self.chunk_start[0]), + nmin(self.last_change[1], self.chunk_start[1])] + + # Flush the last chunk. + if self.chunk_start != [None, None]: + self.flush_chunk() + + def flush_chunk(self): + if self.text_changed: + comment_pos = [nmax(self.chunk_start[0], self.comment_pos[0]), + nmax(self.chunk_start[1], self.comment_pos[1])] + self.chunks_list.append( + util.ObjectDict( + { 'start': self.chunk_start, + 'end': [nadd(self.last_change[0], 1), + nadd(self.last_change[1], 1)], + 'comment_pos': comment_pos, + 'additions': self.additions, + 'removals': self.removals })) + self.reset() + + def reset(self): + # This is called from __init__. + # pylint: disable=W0201 + self.chunk_start = [None, None] + self.last_change = [None, None] + self.comment_pos = [None, None] + self.block_indent = None + self.block_closed = False + self.additions = False + self.removals = False + self.text_changed = False + + +def parse(lines): + """Helper function to parse lines to a list of chunks directly.""" + parser = PolicyChangeParser(lines) + parser.run() + return parser.chunks_list
diff --git a/handlers/policy_checklist/parser_test.py b/handlers/policy_checklist/parser_test.py new file mode 100755 index 0000000..585f325 --- /dev/null +++ b/handlers/policy_checklist/parser_test.py
@@ -0,0 +1,466 @@ +#!/usr/bin/env python +# Copyright (c) 2013 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +import unittest + +import patching +import handlers.policy_checklist.parser + + +def parse_patch_to_chunks(text): + lines = patching.ParsePatchToLines(text.splitlines()) + return handlers.policy_checklist.parser.parse(lines) + + +class PolicyChecklistParserTest(unittest.TestCase): + def test_basic_addition(self): + self.assertEquals([ + { + 'start': [None, 4985], + 'end': [None, 5003], + 'comment_pos': [None, 4986], + 'additions': True, + 'removals': False, + }], parse_patch_to_chunks(""" +Index: chrome/app/policy/policy_templates.json +diff --git a/chrome/app/policy/policy_templates.json b/chrome/app/policy/policy_templates.json +index 7fca71bb7cae78f36f65ec166671a9596cd63a4b..52680ca8ee898a0f1a81c00a7cef669e38fcac51 100644 +--- a/chrome/app/policy/policy_templates.json ++++ b/chrome/app/policy/policy_templates.json +@@ -112,7 +112,7 @@ + # persistent IDs for all fields (but not for groups!) are needed. These are + # specified by the 'id' keys of each policy. NEVER CHANGE EXISTING IDs, + # because doing so would break the deployed wire format! +-# For your editing convenience: highest ID currently used: 218 ++# For your editing convenience: highest ID currently used: 219 + # + # Placeholders: + # The following placeholder strings are automatically substituted: +@@ -4982,6 +4982,24 @@ + + This policy is for internal use by Chrome itself.''', + }, ++ { ++ 'name': 'SupervisedUsersEnabled', ++ 'type': 'main', ++ 'schema': { 'type': 'boolean' }, ++ 'supported_on': ['chrome_os:29-'], ++ 'device_only': True, ++ 'features': { ++ 'dynamic_refresh': False, ++ }, ++ 'example_value': True, ++ 'id': 219, ++ 'caption': '''Enable supervised users.''', ++ 'desc': '''If set to true, it is possible to create and log in as a supervised user. ++ ++ If set to false or not configured, supervised users creation and login will be disabled. All existing supervised users will be hidden. ++ ++ NOTE: The default behavior for consumer and enterprise devices differs: on consumer devices supervised users are enabled by default, but on enterprice devices they aren't.''' ++ }, + ], + }, + ], +""")) + + def test_one_addition_shifted(self): + self.assertEquals([ + { + 'start': [None, 4068], + 'end': [None, 4081], + 'comment_pos': [None, 4068], + 'additions': True, + 'removals': False, + }], parse_patch_to_chunks(""" +Index: chrome/app/policy/policy_templates.json +diff --git a/chrome/app/policy/policy_templates.json b/chrome/app/policy/policy_templates.json +index 1cc92452f6375b7b718c232c42f29d19816554aa..10ad804e04cdcec44eaee5438caa82435b799671 100644 +--- a/chrome/app/policy/policy_templates.json ++++ b/chrome/app/policy/policy_templates.json +@@ -117,7 +117,7 @@ + # persistent IDs for all fields (but not for groups!) are needed. These are + # specified by the 'id' keys of each policy. NEVER CHANGE EXISTING IDs, + # because doing so would break the deployed wire format! +-# For your editing convenience: highest ID currently used: 235 ++# For your editing convenience: highest ID currently used: 236 + # + # Placeholders: + # The following placeholder strings are automatically substituted: +@@ -4065,6 +4065,20 @@ + The format of the value follows the names of timezones in the "IANA Time Zone Database" (see "http://en.wikipedia.org/wiki/List_of_tz_database_time"). In particular, most timezones can be referred to by "continent/large_city" or "ocean/large_city".''', + }, + { ++ 'name': 'SystemUse24HourClock', ++ 'type': 'main', ++ 'schema': { 'type': 'boolean' }, ++ 'supported_on': ['chrome_os:30-'], ++ 'device_only': True, ++ 'features': { ++ 'dynamic_refresh': True, ++ }, ++ 'example_value': True, ++ 'id': 236, ++ 'caption': '''Use 24 hour clock by default''', ++ 'desc': '''Specifies the clock format be used for the device. Users can override clock format for the current session. However, on logout it is set back to the specified value. If an empty string is provided, device owner preference is used.''', ++ }, ++ { + 'name': 'ShowLogoutButtonInTray', + 'type': 'main', + 'schema': { 'type': 'boolean' }, +""")) + + def test_nested_blocks(self): + self.assertEquals([ + { + 'start': [None, 5256], + 'end': [None, 5290], + 'comment_pos': [None, 5259], + 'additions': True, + 'removals': False, + }], parse_patch_to_chunks(""" +Index: chrome/app/policy/policy_templates.json +diff --git a/chrome/app/policy/policy_templates.json b/chrome/app/policy/policy_templates.json +index 86bc3ac3677d3405afdf39f17c7be373fc0fa0d1..3a32920b3d745fb50432cc15a11a45b8f88a95d8 100644 +--- a/chrome/app/policy/policy_templates.json ++++ b/chrome/app/policy/policy_templates.json +@@ -5253,6 +5253,40 @@ + }, + ], + }, ++ # TODO(joaodasilva): replace the 'dict' type with a more generic ++ # 'json' type. The actual schema type for this should be 'array'. ++ { ++ 'name': 'ManagedBookmarks', ++ 'type': 'dict', ++ 'schema': { ++ 'type': 'object', ++ 'items': { ++ 'type': 'object', ++ 'properties': { ++ 'name': { 'type': 'string' }, ++ 'url': { 'type': 'string' }, ++ }, ++ }, ++ }, ++ 'supported_on': ['android:30-'], ++ 'features': { ++ 'dynamic_refresh': True, ++ 'per_profile': True, ++ }, ++ 'future': True, ++ 'example_value': { "name": "Google", "url": "google.com" }, ++ 'id': 227, ++ 'caption': '''Managed Bookmarks''', ++ 'desc': '''Configures a list of managed bookmarks. ++ ++ The policy is a list of bookmarks, and each bookmark is a dictionary containing the bookmark "name" and target "url". ++ ++ These bookmarks are placed in a Managed bookmarks folder inside the Mobile bookmarks. These bookmarks can't be modified by the user. ++ ++ When this policy is set then the Managed bookmarks are the default folder opened when the bookmarks view is opened in Chrome. ++ ++ Managed bookmarks are not synced to the user account.''', ++ }, + ], + 'messages': { + # Messages that are not associated to any policies. +""")) + + def test_two_additions(self): + self.assertEquals([ + { + 'start': [None, 4502], + 'end': [None, 4525], + 'comment_pos': [None, 4503], + 'additions': True, + 'removals': False, + }, { + 'start': [None, 4525], + 'end': [None, 4548], + 'comment_pos': [None, 4526], + 'additions': True, + 'removals': False, + }], parse_patch_to_chunks(""" +Index: chrome/app/policy/policy_templates.json +diff --git a/chrome/app/policy/policy_templates.json b/chrome/app/policy/policy_templates.json +index 3abaf03cb7529667e01472287eb56773bdcbfc69..fbd4a5bc36fc09825eb321ad100088416610b50a 100644 +--- a/chrome/app/policy/policy_templates.json ++++ b/chrome/app/policy/policy_templates.json +@@ -112,7 +112,7 @@ + # persistent IDs for all fields (but not for groups!) are needed. These are + # specified by the 'id' keys of each policy. NEVER CHANGE EXISTING IDs, + # because doing so would break the deployed wire format! +-# For your editing convenience: highest ID currently used: 210 ++# For your editing convenience: highest ID currently used: 214 + # + # Placeholders: + # The following placeholder strings are automatically substituted: +@@ -4499,6 +4499,52 @@ + + If this policy is left unset, Accessibility options will not appear in the system tray menu, but the user can cause the Accessibility options to appear via the Settings page.''' + }, ++ { ++ 'name': 'LargeCursorEnabled', ++ 'type': 'main', ++ 'schema': { 'type': 'boolean' }, ++ 'supported_on': ['chrome_os:29-'], ++ 'features': { ++ 'can_be_recommended': True, ++ 'dynamic_refresh': True, ++ 'per_profile': True, ++ }, ++ 'example_value': True, ++ 'id': 211, ++ 'caption': '''Enable large cursor''', ++ 'desc': '''Enable the large cursor accessibility feature. ++ ++ If this policy is set to true, the large cursor will always be enabled. ++ ++ If this policy is set to false, the large cursor will always be disabled. ++ ++ If you set this policy, users cannot change or override it. ++ ++ If this policy is left unset, the large cursor is disabled initially but can be enabled by the user anytime.''' ++ }, ++ { ++ 'name': 'SpokenFeedbackEnabled', ++ 'type': 'main', ++ 'schema': { 'type': 'boolean' }, ++ 'supported_on': ['chrome_os:29-'], ++ 'features': { ++ 'can_be_recommended': True, ++ 'dynamic_refresh': True, ++ 'per_profile': True, ++ }, ++ 'example_value': True, ++ 'id': 212, ++ 'caption': '''Enable spoken feedback''', ++ 'desc': '''Enable the spoken feedback accessibility feature. ++ ++ If this policy is set to true, spoken feedback will always be enabled. ++ ++ If this policy is set to false, spoken feedback will always be disabled. ++ ++ If you set this policy, users cannot change or override it. ++ ++ If this policy is left unset, spoken feedback is disabled initially but can be enabled by the user anytime.''' ++ }, + ], + }, + { +""")) + + def test_group_additions(self): + self.assertEquals([ + { + 'start': [None, 4523], + 'end': [None, 4545], + 'comment_pos': [None, 4530], + 'additions': True, + 'removals': False, + }, { + 'start': [None, 4545], + 'end': [None, 4564], + 'comment_pos': [None, 4546], + 'additions': True, + 'removals': False, + }], parse_patch_to_chunks(""" +Index: chrome/app/policy/policy_templates.json +diff --git a/chrome/app/policy/policy_templates.json b/chrome/app/policy/policy_templates.json +index 6a1293696283e232070e61104a369e52a615cdf1..dad016adf0ae2055a6e7cb1275fb99d33c703ab2 100644 +--- a/chrome/app/policy/policy_templates.json ++++ b/chrome/app/policy/policy_templates.json +@@ -112,7 +112,7 @@ + # persistent IDs for all fields (but not for groups!) are needed. These are + # specified by the 'id' keys of each policy. NEVER CHANGE EXISTING IDs, + # because doing so would break the deployed wire format! +-# For your editing convenience: highest ID currently used: 199 ++# For your editing convenience: highest ID currently used: 201 + # + # Placeholders: + # The following placeholder strings are automatically substituted: +@@ -4520,6 +4520,49 @@ + + If not specified, will not modify the Variations seed URL.''', + }, ++ { ++ 'name': 'Attestation', ++ 'type': 'group', ++ 'caption': 'Remote Attestation', ++ 'desc': 'Configure the remote attestation with TPM mechanism.', ++ 'policies': [ ++ { ++ 'name': 'AttestationEnabledForUser', ++ 'type': 'main', ++ 'schema': { 'type': 'boolean' }, ++ 'supported_on': ['chrome_os:28-'], ++ 'features': { ++ 'dynamic_refresh': True, ++ 'per_profile': True, ++ }, ++ 'example_value': True, ++ 'id': 200, ++ 'caption': '''Enable remote attestation for the user.''', ++ 'desc': '''If true, the user can use the hardware on Chrome devices to remote attest its identity to the privacy CA via the Enterprise Platform Keys API chrome.enterprise.platformKeysPrivate.challengeUserKey(). ++ ++ If it is set to false, or if it is not set, calls to the API will fail with an error code.''', ++ }, ++ { ++ 'name': 'AttestationExtensionWhitelist', ++ 'type': 'list', ++ 'schema': { ++ 'type': 'array', ++ 'items': { 'type': 'string' }, ++ }, ++ 'supported_on': ['chrome_os:28-'], ++ 'features': { ++ 'dynamic_refresh': True, ++ 'per_profile': True, ++ }, ++ 'example_value': ['ghdilpkmfbfdnomkmaiogjhjnggaggoi'], ++ 'id': 201, ++ 'caption': '''Extensions allowed to to use the remote attestation API.''', ++ 'desc': '''This policy specifies the allowed extensions to use Enterprise Platform Keys API chrome.enterprise.platformKeysPrivate.challengeUserKey() for remote attestation. Extensions must be added to this list to use the API. ++ ++ If an extension is not in the list, or the list is not set, the call to the API will fail with an error code.''', ++ }, ++ ], ++ }, + ], + 'messages': { + # Messages that are not associated to any policies. +""")) + + def test_simple_edit(self): + self.assertEquals([ + { + 'start': [300, 300], + 'end': [301, 301], + 'comment_pos': [300, 300], + 'additions': True, + 'removals': True, + }], parse_patch_to_chunks(""" +Index: chrome/app/policy/policy_templates.json +diff --git a/chrome/app/policy/policy_templates.json b/chrome/app/policy/policy_templates.json +index 02e1801f8906343b6168f86c45ae3b2c0c9f5363..86bc3ac3677d3405afdf39f17c7be373fc0fa0d1 100644 +--- a/chrome/app/policy/policy_templates.json ++++ b/chrome/app/policy/policy_templates.json +@@ -297,7 +297,7 @@ + 'name': 'DisableSpdy', + 'type': 'main', + 'schema': { 'type': 'boolean' }, +- 'supported_on': ['chrome.*:8-', 'chrome_os:0.11-'], ++ 'supported_on': ['chrome.*:8-', 'chrome_os:0.11-', 'android:30-'], + 'features': { + 'dynamic_refresh': True, + 'per_profile': False, +""")) + + def test_complex_edit(self): + self.assertEquals([ + { + 'start': [328, 328], + 'end': [332, 335], + 'comment_pos': [328, 328], + 'additions': True, + 'removals': True, + }], parse_patch_to_chunks(""" +Index: chrome/app/policy/policy_templates.json +diff --git a/chrome/app/policy/policy_templates.json b/chrome/app/policy/policy_templates.json +index 602196317bd5bc00ed0884c2ef3e563910820686..c38156df31142a39a6b6fa215bc629f2ca0e7f94 100644 +--- a/chrome/app/policy/policy_templates.json ++++ b/chrome/app/policy/policy_templates.json +@@ -325,10 +325,13 @@ + 'dynamic_refresh': True, + 'per_profile': False, + }, +- 'example_value': ['file', 'mailto'], ++ 'deprecated': True, ++ 'example_value': ['file', 'https'], + 'id': 85, + 'caption': '''Disable URL protocol schemes''', +- 'desc': '''Disables the listed protocol schemes in <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>. ++ 'desc': '''This policy is deprecated, please use URLBlacklist instead. ++ ++ Disables the listed protocol schemes in <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>. + + URLs using a scheme from this list will not load and can not be navigated to. +""")) + + def test_enum_item_names_ignored_for_comment_pos(self): + self.assertEquals([ + { + 'start': [None, 3323], + 'end': [None, 3363], + 'comment_pos': [None, 3324], + 'additions': True, + 'removals': False, + }], parse_patch_to_chunks(""" +Index: chrome/app/policy/policy_templates.json +diff --git a/chrome/app/policy/policy_templates.json b/chrome/app/policy/policy_templates.json +index c888dcbf4fc32656c048054ca149e245989281d5..7de3edae00f1e8ff5984be8f10091ee044655d8a 100644 +--- a/chrome/app/policy/policy_templates.json ++++ b/chrome/app/policy/policy_templates.json +@@ -112,7 +112,7 @@ + # persistent IDs for all fields (but not for groups!) are needed. These are + # specified by the 'id' keys of each policy. NEVER CHANGE EXISTING IDs, + # because doing so would break the deployed wire format! +-# For your editing convenience: highest ID currently used: 165 ++# For your editing convenience: highest ID currently used: 166 + # + # Placeholders: + # The following placeholder strings are automatically substituted: +@@ -3320,6 +3320,46 @@ + + If this policy is left not set, the users will be able to change whether the built-in DNS client is used by editing chrome://flags or specifying a command-line flag.''', + }, ++ { ++ 'name': 'ShelfAutoHideBehavior', ++ 'type': 'string-enum', ++ 'schema': { ++ 'type': 'string', ++ 'enum': [ ++ 'Always', ++ 'Never' ++ ], ++ }, ++ 'items': [ ++ { ++ 'name': 'AlwaysAutoHideShelf', ++ 'value': 'Always', ++ 'caption': '''Always auto-hide the shelf''', ++ }, ++ { ++ 'name': 'NeverAutoHideShelf', ++ 'value': 'Never', ++ 'caption': '''Never auto-hide the shelf''', ++ }, ++ ], ++ 'supported_on': ['chrome_os:25-'], ++ 'features': { ++ 'dynamic_refresh': True, ++ 'can_be_recommended': True, ++ }, ++ 'example_value': 'Always', ++ 'id': 166, ++ 'caption': '''Control shelf auto-hiding''', ++ 'desc': '''Control auto-hiding of the <ph name="PRODUCT_OS_NAME">$2<ex>Google Chrome OS</ex></ph> shelf. ++ ++ If this policy is set to 'AlwaysAutoHideShelf', the shelf will always auto-hide. ++ ++ If this policy is set to 'NeverAutoHideShelf', the shelf never auto-hide. ++ ++ If you set this policy, users cannot change or override it. ++ ++ If the policy is left not set, users can choose whether the shelf should auto-hide.''', ++ }, + ], + 'messages': { + # Messages that are not associated to any policies. +""")) + + +if __name__ == '__main__': + unittest.main()
diff --git a/handlers/policy_checklist/review_message.txt b/handlers/policy_checklist/review_message.txt new file mode 100644 index 0000000..ff87126 --- /dev/null +++ b/handlers/policy_checklist/review_message.txt
@@ -0,0 +1,8 @@ +Thanks for improving Chromium enterprise features! In general, +enterprise policies should work across all platforms and need to remain +backward compatible across several Chrome versions. To help avoid common +pitfalls, policy review bot has analyzed your change and added comments +with checklists of things to watch out for. + +Please direct comments and complaints about this service to +chrome-enterprise@google.com and/or mnissler@chromium.org.
diff --git a/mail_dispatcher.py b/mail_dispatcher.py new file mode 100644 index 0000000..45c732b --- /dev/null +++ b/mail_dispatcher.py
@@ -0,0 +1,101 @@ +# Copyright (c) 2013 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +"""Main app that handles incoming mail and dispatches it to handlers.""" + +import logging +import re + +import webapp2 +import webob.exc + +from google.appengine.api import app_identity +from google.appengine.api import mail + +import third_party # pylint: disable=W0611 + +import handlers.policy_checklist +from review import Review +from rietveld import Rietveld +import util + + +# Lists the handlers served by the review bot app. The local part of email +# addresses the app receives email on is used as a key into the handler map. +# Each handler is just a function that gets called with the email address, the +# email message, a Review object and a Rietveld interface. +# +# New handlers can be added by adding the code in handlers/<handler_name>.py, +# importing the module and adding an entry to the HANDLERS map. +HANDLERS = { + 'policy_checklist': handlers.policy_checklist.process +} + + +# Regular expression that matches email addresses belonging to the review bot +# app and extracts the handler name. +REVIEW_BOT_RECIPIENT_RE = re.compile('^([^@]+)@%s.appspotmail.com$' % + app_identity.get_application_id()) + + +# This is the regular expression that rietveld uses to extract the issue number +# from the mail subject at the time of writing this. This code needs to be kept +# up-to-date with the mechanism rietveld uses so the tools don't confuse issues. +RIETVELD_ISSUE_NUMBER_RE = re.compile(r'\(issue *(?P<id>\d+)\)$') + + +class MailDispatcher(webapp2.RequestHandler): + """Dispatches mail to handlers as indicated by email addresses.""" + + def post(self): + """Handles POST requests. + + Parses the incoming mail message. Dispatches to interested handlers based on + the list of mail recipients. + """ + + # Singleton Rietveld interface for this request. + rietveld = Rietveld() + + # Parse the message and instantiate the review interface. + message = mail.InboundEmailMessage(self.request.body) + match = RIETVELD_ISSUE_NUMBER_RE.search(message.subject) + if match is None: + raise webob.exc.HTTPBadRequest('Failed to parse issue id: %s' % + message.subject) + review = Review(rietveld, match.groupdict()['id']) + + # Determine recipients and run the handlers one by one. + recipients = set(util.get_emails(getattr(message, 'to', '')) + + util.get_emails(getattr(message, 'cc', ''))) + for addr in recipients: + match = REVIEW_BOT_RECIPIENT_RE.match(addr) + if not match: + continue + + try: + handler = HANDLERS[match.group(1)] + except KeyError: + continue + + try: + handler(addr, message, review, rietveld) + except: # pylint: disable=W0702 + logging.exception('Handler %s failed!', match.group(1)) + + def handle_exception(self, exception, debug): + """Handles exceptions to print HTTP error details. + + Args: + exception: The exception. + debug: Whether we're in debug mode. + """ + if isinstance(exception, webob.exc.HTTPException): + logging.warning('Request %s failed: %d - %s', + self.request.url, exception.code, exception.detail) + + webapp2.RequestHandler.handle_exception(self, exception, debug) + + +app = webapp2.WSGIApplication([('/_ah/mail/.*', MailDispatcher)])
diff --git a/util.py b/util.py index 732bb61..85c7687 100644 --- a/util.py +++ b/util.py
@@ -2,6 +2,7 @@ # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. +import collections import email.utils @@ -17,7 +18,7 @@ return property(get_property) -class LazyDict(object): +class LazyDict(collections.Mapping): """A simple immutable and lazy dictionary. This looks up the actual key values on first access to the key and caches the @@ -33,18 +34,25 @@ self.items[name] = self.lookup(name) return self.items[name] + def __iter__(self): + return self.items.iterkeys() -class ObjectDict(object): + def __len__(self): + return len(self.items) + + def __repr__(self): + return repr(self.items) + + +class ObjectDict(collections.Mapping): """Wraps a dictionary to allow value retrieval in dot notation.""" def __init__(self, data): - self.data = data + self.__data = data def __getitem__(self, name): - val = self.data[name] - if type(val) == dict: - return ObjectDict(val) - return val + val = self.__data[name] + return ObjectDict.wrap(val) def __getattr__(self, name): try: @@ -53,7 +61,19 @@ raise AttributeError(e) def __iter__(self): - return self.data.iterkeys() + return self.__data.iterkeys() + + def __len__(self): + return len(self.__data) + + def __repr__(self): + return repr(self.__data) + + @staticmethod + def wrap(val): + if isinstance(val, dict): + return ObjectDict(val) + return val def get_emails(string):