Add support for items for bool policies

Make this a required field for new main/bool policies.

Since the item field isn't always copied over from main policies,
also add a warning check that the policy descriptions describe the
various cases.

Add items to MediaRecommendationsEnabled and BlockThirdPartyCookies.

No-Presubmit: true
BUG: 1139306
Change-Id: I519202ed9eeb491b27f804f26fbe94ae8e501598
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2507457
Commit-Queue: Chris Sharp <csharp@chromium.org>
Reviewed-by: Yann Dago <ydago@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824431}

components/policy/resources/policy_templates.json

@@ -4989,11 +4989,26 @@
         'dynamic_refresh': True,
         'per_profile': True,
       },
+      'items': [
+        {
+          'value': True,
+          'caption': 'Block 3rd party cookies',
+        },
+        {
+          'value': False,
+          'caption': 'Allow 3rd party cookies',
+        },
+        {
+          'value': None,
+          'caption': 'Allow 3rd party cookes, but allow the user to change this setting',
+        },
+      ],
       'example_value': False,
+      'default': None,
       'id': 39,
       'caption': '''Block third party cookies''',
       'tags': [],
-      'desc': '''Setting the policy to True prevents webpage elements that aren't from the domain that's in the browser's address bar from setting cookies. Setting the policy to False lets those elements set cookies and prevents users from changing this setting.
+      'desc': '''Setting the policy to Enabled prevents webpage elements that aren't from the domain that's in the browser's address bar from setting cookies. Setting the policy to Disabled lets those elements set cookies and prevents users from changing this setting.
 
       Leaving it unset turns third-party cookies on, but users can change this setting.''',
     },
@@ -23197,6 +23212,16 @@ If you disable this policy, web page scrolling to specific text fragments via UR
       },
       'example_value': True,
       'default': True,
+      'items': [
+        {
+          'value': True,
+          'caption': 'Show media recommendations to the user',
+        },
+        {
+          'value': False,
+          'caption': 'Hide media recommendations from the user',
+        },
+      ],
       'id': 788,
       'caption': 'Enable Media Recommendations',
       'tags': [],

components/policy/tools/syntax_check_policy_template_json.py

@@ -522,6 +522,112 @@ class PolicyTemplateChecker(object):
           ('Default for policy %s of type %s should be one of %s, got %s') %
           (policy.get('name'), policy_type, acceptable_values, default))
 
+  def _NeedsItems(self, policy):
+    return policy.get('type') in ('main', 'int-enum', 'string-enum',
+                                  'string-enum-list')
+
+  def _CheckItems(self, policy, item_type):
+    if not self._NeedsItems(policy):
+      return
+
+    # TODO(crbug.com/1139306): Remove this check once all main policies
+    # have specified their items field.
+    policy_type = policy.get('type')
+    if policy_type == 'main' and 'items' not in policy:
+      return
+
+    items = self._CheckContains(policy, 'items', list)
+    if items is None:
+      return
+
+    if len(items) < 1:
+      self._Error('"items" must not be empty.', 'policy', policy, items)
+      return
+
+    # Ensure all items have valid captions.
+    for item in items:
+      self._CheckContains(item,
+                          'caption',
+                          str,
+                          container_name='item',
+                          identifier=policy.get('name'))
+
+    if policy_type == 'main':
+      # Main (bool) policies must contain a list of items to clearly
+      # indicate what the states mean.
+      required_values = [True, False]
+
+      # The unset item can only appear if the default is None, since
+      # there is no other way for it to be set.
+      if 'default' in policy and policy['default'] == None:
+        required_values.append(None)
+
+      # Since the item captions don't appear everywhere the description does,
+      # try and ensure the items are still described in the descriptions.
+      value_to_names = {
+          None: {'None', 'Unset', 'unset'},
+          True: {'True', 'Enabled'},
+          False: {'False', 'Disabled'},
+      }
+      for value in required_values:
+        names = value_to_names[value]
+        if not any(name in policy['desc'] for name in names):
+          self._Warning(
+              ('Policy %s doesn\'t seem to describe what happens when it is '
+               'set to %s. If possible update the description to describe this '
+               'while using at least one of %s') %
+              (policy.get('name'), value, names))
+
+      values_seen = set()
+      for item in items:
+        # Bool items shouldn't have names, since it's the same information
+        # as the value field.
+        if 'name' in item:
+          self._Error(
+              ('Policy %s has item %s with an unexpected name field, '
+               'please delete the name field.') % (policy.get('name'), item))
+
+        # Each item must have a value.
+        if 'value' not in item:
+          self._Error(
+              ('Policy %s has item %s which is missing the value field') %
+              (policy.get('name'), item))
+        else:
+          value = item['value']
+          if value in values_seen:
+            self._Error(
+                ('Policy %s has multiple items with the same value (%s), each '
+                 'value should only appear once') % (policy.get('name'), value))
+          else:
+            values_seen.add(value)
+            if value not in required_values:
+              self._Error(
+                  ('Policy %s of type main has an item with a value %s, value '
+                   'must be one of %s') %
+                  (policy.get('name'), name, required_names))
+
+      if not values_seen.issuperset(required_values):
+        self._Error(
+            ('Policy %s is missing some required values, found "%s", requires '
+             '"%s"') % (policy.get('name'), list(values_seen), required_values))
+
+    if policy_type in ('int-enum', 'string-enum', 'string-enum-list'):
+      for item in items:
+        # Each item must have a name.
+        self._CheckContains(item,
+                            'name',
+                            str,
+                            container_name='item',
+                            identifier=policy.get('name'),
+                            regexp_check=NO_WHITESPACE)
+
+        # Each item must have a value of the correct type.
+        self._CheckContains(item,
+                            'value',
+                            item_type,
+                            container_name='item',
+                            identifier=policy.get('name'))
+
   def _CheckPolicy(self, policy, is_in_group, policy_ids, deleted_policy_ids,
                    current_version):
     if not isinstance(policy, dict):
@@ -890,44 +996,12 @@ class PolicyTemplateChecker(object):
       if is_in_group:
         self.num_policies_in_groups += 1
 
-    if policy_type in ('int-enum', 'string-enum', 'string-enum-list'):
-      # Enums must contain a list of items.
-      items = self._CheckContains(policy, 'items', list)
-      if items is not None:
-        if len(items) < 1:
-          self._Error('"items" must not be empty.', 'policy', policy, items)
-        for item in items:
-          # Each item must have a name.
-          # Note: |policy.get('name')| is used instead of |policy['name']|
-          # because it returns None rather than failing when no key called
-          # 'name' exists.
-          self._CheckContains(
-              item,
-              'name',
-              str,
-              container_name='item',
-              identifier=policy.get('name'),
-              regexp_check=NO_WHITESPACE)
-
-          # Each item must have a value of the correct type.
-          self._CheckContains(
-              item,
-              'value',
-              item_type,
-              container_name='item',
-              identifier=policy.get('name'))
-
-          # Each item must have a caption.
-          self._CheckContains(
-              item,
-              'caption',
-              str,
-              container_name='item',
-              identifier=policy.get('name'))
-
-    if policy_type == 'external':
-      # Each policy referencing external data must specify a maximum data size.
-      self._CheckContains(policy, 'max_size', int)
+      self._CheckItems(policy, item_type)
+
+      if policy_type == 'external':
+        # Each policy referencing external data must specify a maximum data
+        # size.
+        self._CheckContains(policy, 'max_size', int)
 
   def _CheckPlatform(self, platforms, field_name, policy_name):
     ''' Verifies the |platforms| list. Records any error with |field_name| and
@@ -1482,6 +1556,11 @@ class PolicyTemplateChecker(object):
                                                            None) == None:
         self._Error(('Missing default field for policy %s') % (new_policy_name))
 
+      # TODO(crbug.com/1139306): This item check should apply to all policies
+      # instead of just new ones.
+      if self._NeedsItems(policy) and new_policy.get('items', None) == None:
+        self._Error(('Missing items field for policy %') % (new_policy_name))
+
   def _LeadingWhitespace(self, line):
     match = LEADING_WHITESPACE.match(line)
     if match:

components/policy/tools/template_writers/writers/android_policy_writer.py

@@ -20,7 +20,7 @@ def _EscapeResource(resource):
   '''Escape the resource for usage in an Android resource XML file.
   This includes standard XML escaping as well as those specific to Android.
   '''
-  if type(resource) == int:
+  if resource == None or type(resource) in (int, bool):
     return str(resource)
   return xml_escape.escape(resource, {"'": "\\'", '"': '\\"', '\\': '\\\\'})
 

tools/grit/grit/gather/policy_json.py

@@ -168,9 +168,16 @@ class PolicyJson(skeleton_gatherer.SkeletonGatherer):
               (key_map[key], item['name'],
                ','.join(item['owners'] if 'owners' in item else 'unknown')))
     if item_type == 'enum_item':
-      return ('%s of the option named %s in policy %s [owner(s): %s]' %
-              (key_map[key], item['name'], parent_item['name'],
-               ','.join(parent_item['owners'] if 'owners' in parent_item else 'unknown')))
+      if 'name' in item:
+        return ('%s of the option named %s in policy %s [owner(s): %s]' %
+                (key_map[key], item['name'], parent_item['name'],
+                 ','.join(parent_item['owners'] if 'owners' in
+                          parent_item else 'unknown')))
+      else:
+        return ('%s of the option with value %s in policy %s [owner(s): %s]' %
+                (key_map[key], item['value'], parent_item['name'],
+                 ','.join(parent_item['owners'] if 'owners' in
+                          parent_item else 'unknown')))
     raise Exception('Unexpected type %s' % item_type)
 
   def _AddSchemaKeys(self, obj, depth):