Perform policy checks only once on list responses

The policy engine is currently being called for every attribute
of every resource to be returned by a list response. This is
harming the API performance; moreover such a high number of checks
is also unnecessary.

This patch therefore slightly changes the API logic so that list
response first determine the list of attributes which should be
returned querying the policy engine and then use this list for
all resource items to be returned.

To this aim a few methods in base.py needed to be refactored.
This patch also removes the routine check_if_exists from policy.py
and the related PolicyNotFound exception.

Finally, this patch also removes unnecessary admin_or_owner rules
when applied to attributes. This kind of rule indeed has no effect
anyway because of Neutron's ownership checks. The rules were removed
because this change won't allow anymore for having attribute-level
policies whose evaluation result depends on the resource value.

Implements blueprint faster-list-responses

Change-Id: I21b8273add5d5984f512ad94af5a99cf0b0a5d93

etc/policy.json

@@ -47,7 +47,6 @@
     "create_port:port_security_enabled": "rule:admin_or_network_owner",
     "create_port:binding:host_id": "rule:admin_only",
     "create_port:binding:profile": "rule:admin_only",
-    "create_port:binding:vnic_type": "rule:admin_or_owner",
     "create_port:mac_learning_enabled": "rule:admin_or_network_owner",
     "get_port": "rule:admin_or_owner",
     "get_port:queue_id": "rule:admin_only",
@@ -55,13 +54,11 @@
     "get_port:binding:vif_details": "rule:admin_only",
     "get_port:binding:host_id": "rule:admin_only",
     "get_port:binding:profile": "rule:admin_only",
-    "get_port:binding:vnic_type": "rule:admin_or_owner",
     "update_port": "rule:admin_or_owner",
     "update_port:fixed_ips": "rule:admin_or_network_owner",
     "update_port:port_security_enabled": "rule:admin_or_network_owner",
     "update_port:binding:host_id": "rule:admin_only",
     "update_port:binding:profile": "rule:admin_only",
-    "update_port:binding:vnic_type": "rule:admin_or_owner",
     "update_port:mac_learning_enabled": "rule:admin_or_network_owner",
     "delete_port": "rule:admin_or_owner",
 
@@ -83,8 +80,6 @@
 
     "create_firewall_rule": "",
     "get_firewall_rule": "rule:admin_or_owner or rule:shared_firewalls",
-    "create_firewall_rule:shared": "rule:admin_or_owner",
-    "get_firewall_rule:shared": "rule:admin_or_owner",
     "update_firewall_rule": "rule:admin_or_owner",
     "delete_firewall_rule": "rule:admin_or_owner",
 

neutron/api/v2/base.py

@@ -126,41 +126,48 @@ class Controller(object):
                                     % self._plugin.__class__.__name__)
         return getattr(self._plugin, native_sorting_attr_name, False)
 
-    def _is_visible(self, context, attr_name, data):
+    def _exclude_attributes_by_policy(self, context, data):
-        action = "%s:%s" % (self._plugin_handlers[self.SHOW], attr_name)
+        """Identifies attributes to exclude according to authZ policies.
-        # Optimistically init authz_check to True
+
-        authz_check = True
+        Return a list of attribute names which should be stripped from the
-        try:
+        response returned to the user because the user is not authorized
-            attr = (attributes.RESOURCE_ATTRIBUTE_MAP
+        to see them.
-                    [self._collection].get(attr_name))
+        """
-            if attr and attr.get('enforce_policy'):
+        attributes_to_exclude = []
-                authz_check = policy.check_if_exists(
+        for attr_name in data.keys():
-                    context, action, data)
+            attr_data = self._attr_info.get(attr_name)
-        except KeyError:
+            if attr_data and attr_data['is_visible']:
-            # The extension was not configured for adding its resources
+                if policy.check(
-            # to the global resource attribute map. Policy check should
+                    context,
-            # not be performed
+                    '%s:%s' % (self._plugin_handlers[self.SHOW], attr_name),
-            LOG.debug(_("The resource %(resource)s was not found in the "
+                    None,
-                        "RESOURCE_ATTRIBUTE_MAP; unable to perform authZ "
+                    might_not_exist=True):
-                        "check for attribute %(attr)s"),
+                    # this attribute is visible, check next one
-                      {'resource': self._collection,
+                    continue
-                       'attr': attr_name})
+            # if the code reaches this point then either the policy check
-        except exceptions.PolicyRuleNotFound:
+            # failed or the attribute was not visible in the first place
-            # Just ignore the exception. Do not even log it, as this will add
+            attributes_to_exclude.append(attr_name)
-            # a lot of unnecessary info in the log (and take time as well to
+        return attributes_to_exclude
-            # write info to the logger)
-            pass
-        attr_val = self._attr_info.get(attr_name)
-        return attr_val and attr_val['is_visible'] and authz_check
 
     def _view(self, context, data, fields_to_strip=None):
-        # make sure fields_to_strip is iterable
+        """Build a view of an API resource.
-        if not fields_to_strip:
-            fields_to_strip = []
 
+        :param context: the neutron context
+        :param data: the object for which a view is being created
+        :param fields_to_strip: attributes to remove from the view
+
+        :returns: a view of the object which includes only attributes
+        visible according to API resource declaration and authZ policies.
+        """
+        fields_to_strip = ((fields_to_strip or []) +
+                           self._exclude_attributes_by_policy(context, data))
+        return self._filter_attributes(context, data, fields_to_strip)
+
+    def _filter_attributes(self, context, data, fields_to_strip=None):
+        if not fields_to_strip:
+            return data
         return dict(item for item in data.iteritems()
-                    if (self._is_visible(context, item[0], data) and
+                    if (item[0] not in fields_to_strip))
-                        item[0] not in fields_to_strip))
 
     def _do_field_list(self, original_fields):
         fields_to_add = None
@@ -245,9 +252,19 @@ class Controller(object):
                                         self._plugin_handlers[self.SHOW],
                                         obj,
                                         plugin=self._plugin)]
+        # Use the first element in the list for discriminating which attributes
+        # should be filtered out because of authZ policies
+        # fields_to_add contains a list of attributes added for request policy
+        # checks but that were not required by the user. They should be
+        # therefore stripped
+        fields_to_strip = fields_to_add or []
+        if obj_list:
+            fields_to_strip += self._exclude_attributes_by_policy(
+                request.context, obj_list[0])
         collection = {self._collection:
-                      [self._view(request.context, obj,
+                      [self._filter_attributes(
-                                  fields_to_strip=fields_to_add)
+                          request.context, obj,
+                          fields_to_strip=fields_to_strip)
                        for obj in obj_list]}
         pagination_links = pagination_helper.get_links(obj_list)
         if pagination_links:
@@ -318,9 +335,12 @@ class Controller(object):
                 kwargs = {self._resource: item}
                 if parent_id:
                     kwargs[self._parent_id_name] = parent_id
-                objs.append(self._view(request.context,
+                fields_to_strip = self._exclude_attributes_by_policy(
-                                       obj_creator(request.context,
+                    request.context, item)
-                                                   **kwargs)))
+                objs.append(self._filter_attributes(
+                    request.context,
+                    obj_creator(request.context, **kwargs),
+                    fields_to_strip=fields_to_strip))
             return objs
         # Note(salvatore-orlando): broad catch as in theory a plugin
         # could raise any kind of exception
@@ -409,8 +429,13 @@ class Controller(object):
             # plugin does atomic bulk create operations
             obj_creator = getattr(self._plugin, "%s_bulk" % action)
             objs = obj_creator(request.context, body, **kwargs)
-            return notify({self._collection: [self._view(request.context, obj)
+            # Use first element of list to discriminate attributes which
-                                              for obj in objs]})
+            # should be removed because of authZ policies
+            fields_to_strip = self._exclude_attributes_by_policy(
+                request.context, objs[0])
+            return notify({self._collection: [self._filter_attributes(
+                request.context, obj, fields_to_strip=fields_to_strip)
+                for obj in objs]})
         else:
             obj_creator = getattr(self._plugin, action)
             if self._collection in body:
@@ -424,8 +449,8 @@ class Controller(object):
 
                 self._nova_notifier.send_network_change(
                     action, {}, {self._resource: obj})
-                return notify({self._resource: self._view(request.context,
+                return notify({self._resource: self._view(
-                                                          obj)})
+                    request.context, obj)})
 
     def delete(self, request, id, **kwargs):
         """Deletes the specified entity."""

neutron/common/exceptions.py

@@ -96,10 +96,6 @@ class PolicyFileNotFound(NotFound):
     message = _("Policy configuration policy.json could not be found")
 
 
-class PolicyRuleNotFound(NotFound):
-    message = _("Requested rule:%(rule)s cannot be found")
-
-
 class PolicyInitError(NeutronException):
     message = _("Failed to init policy %(policy)s because %(reason)s")
 

neutron/policy.py

@@ -324,7 +324,7 @@ def _prepare_check(context, action, target):
     return match_rule, target, credentials
 
 
-def check(context, action, target, plugin=None):
+def check(context, action, target, plugin=None, might_not_exist=False):
     """Verifies that the action is valid on the target in this context.
 
     :param context: neutron context
@@ -335,25 +335,14 @@ def check(context, action, target, plugin=None):
         location of the object e.g. ``{'project_id': context.project_id}``
     :param plugin: currently unused and deprecated.
         Kept for backward compatibility.
+    :param might_not_exist: If True the policy check is skipped (and the
+        function returns True) if the specified policy does not exist.
+        Defaults to false.
 
     :return: Returns True if access is permitted else False.
     """
-    return policy.check(*(_prepare_check(context, action, target)))
+    if might_not_exist and not (policy._rules and action in policy._rules):
-
+        return True
-
-def check_if_exists(context, action, target):
-    """Verify if the action can be authorized, and raise if it is unknown.
-
-    Check whether the action can be performed on the target within this
-    context, and raise a PolicyRuleNotFound exception if the action is
-    not defined in the policy engine.
-    """
-    # TODO(salvatore-orlando): Consider modifying oslo policy engine in
-    # order to allow to raise distinct exception when check fails and
-    # when policy is missing
-    # Raise if there's no match for requested action in the policy engine
-    if not policy._rules or action not in policy._rules:
-        raise exceptions.PolicyRuleNotFound(rule=action)
     return policy.check(*(_prepare_check(context, action, target)))
 
 

neutron/tests/unit/test_policy.py

@@ -108,11 +108,13 @@ class PolicyTestCase(base.BaseTestCase):
         result = policy.check(self.context, action, self.target)
         self.assertEqual(result, False)
 
-    def test_check_if_exists_non_existent_action_raises(self):
+    def test_check_non_existent_action(self):
         action = "example:idonotexist"
-        self.assertRaises(exceptions.PolicyRuleNotFound,
+        result_1 = policy.check(self.context, action, self.target)
-                          policy.check_if_exists,
+        self.assertFalse(result_1)
-                          self.context, action, self.target)
+        result_2 = policy.check(self.context, action, self.target,
+                                might_not_exist=True)
+        self.assertTrue(result_2)
 
     def test_enforce_good_action(self):
         action = "example:allowed"