Add ext_parent policy check

Add common parent owner check for the resources which introduced by
service plugin.

Then port forwarding resource will share the same tenant_id with
floatingip. That means only the fip owner can create/update/get/delete
the associated port forwarding resource.

Partially-Implements: blueprint port-forwarding
Partial-Bug: #1491317
Change-Id: I450c674e55ca15e1d9a6a6224138f3305427da68

doc/source/contributor/internals/policy.rst

@@ -180,7 +180,13 @@ overrides the generic check performed by oslo_policy in this case.
 It uses for those cases where neutron needs to check whether the project
 submitting a request for a new resource owns the parent resource of the one
 being created. Current usages of ``OwnerCheck`` include, for instance,
-creating and updating a subnet.
+creating and updating a subnet. This class supports the extension parent
+resources owner check which the parent resource introduced by
+service plugins. Such as router and floatingip owner check for ``router``
+service plugin. Developers can register the extension resource name and service
+plugin name which were registered in neutron-lib into
+``EXT_PARENT_RESOURCE_MAPPING`` which is located in
+``neutron.common.constants``.
 
 The check, performed in the ``__call__`` method, works as follows:
 
@@ -192,7 +198,9 @@ The check, performed in the ``__call__`` method, works as follows:
   * if the previous check failed, extract a parent resource type and a
     parent field name from the target field. For instance
     ``networks:tenant_id`` identifies the ``tenant_id`` attribute of the
-    ``network`` resource;
+    ``network`` resource. For extension parent resource case,
+    ``ext_parent:tenant_id`` identifies the ``tenant_id`` attribute of the
+    registered extension resource in ``EXT_PARENT_RESOURCE_MAPPING``;
   * if no parent resource or target field could be identified raise a
     ``PolicyCheckError`` exception;
   * Retrieve a 'parent foreign key' from the ``_RESOURCE_FOREIGN_KEYS`` data

etc/policy.json

@@ -13,6 +13,7 @@
     "shared_address_scopes": "field:address_scopes:shared=True",
     "external": "field:networks:router:external=True",
     "default": "rule:admin_or_owner",
+    "admin_or_ext_parent_owner": "rule:context_is_admin or tenant_id:%(ext_parent:tenant_id)s",
 
     "create_subnet": "rule:admin_or_network_owner",
     "create_subnet:segment_id": "rule:admin_only",
@@ -250,9 +251,9 @@
     "update_log": "rule:admin_only",
     "delete_log": "rule:admin_only",
 
-    "create_floatingip_port_forwarding": "rule:regular_user",
-    "get_floatingip_port_forwarding": "rule:regular_user",
-    "get_floatingip_port_forwardings": "rule:regular_user",
-    "update_floatingip_port_forwarding": "rule:regular_user",
-    "delete_floatingip_port_forwarding": "rule:regular_user"
+    "create_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner",
+    "get_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner",
+    "get_floatingip_port_forwardings": "rule:admin_or_ext_parent_owner",
+    "update_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner",
+    "delete_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner"
 }

neutron/api/v2/base.py

@@ -310,12 +310,15 @@ class Controller(object):
             # FIXME(salvatore-orlando): obj_getter might return references to
             # other resources. Must check authZ on them too.
             # Omit items from list that should not be visible
-            obj_list = [obj for obj in obj_list
-                        if policy.check(request.context,
-                                        self._plugin_handlers[self.SHOW],
-                                        obj,
-                                        plugin=self._plugin,
-                                        pluralized=self._collection)]
+            tmp_list = []
+            for obj in obj_list:
+                self._set_parent_id_into_ext_resources_request(
+                    request, obj, parent_id, is_get=True)
+                if policy.check(
+                        request.context, self._plugin_handlers[self.SHOW],
+                        obj, plugin=self._plugin, pluralized=self._collection):
+                    tmp_list.append(obj)
+            obj_list = tmp_list
         # 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
@@ -346,6 +349,8 @@ class Controller(object):
             kwargs[self._parent_id_name] = parent_id
         obj_getter = getattr(self._plugin, action)
         obj = obj_getter(request.context, id, **kwargs)
+        self._set_parent_id_into_ext_resources_request(
+            request, obj, parent_id, is_get=True)
         # Check authz
         # FIXME(salvatore-orlando): obj_getter might return references to
         # other resources. Must check authZ on them too.
@@ -457,6 +462,11 @@ class Controller(object):
         for item in items:
             self._validate_network_tenant_ownership(request,
                                                     item[self._resource])
+            # For ext resources policy check, we support two types, such as
+            # parent_id is in request body, another type is parent_id is in
+            # request url, which we can get from kwargs.
+            self._set_parent_id_into_ext_resources_request(
+                request, item[self._resource], parent_id)
             policy.enforce(request.context,
                            action,
                            item[self._resource],
@@ -632,6 +642,10 @@ class Controller(object):
         # Ensure policy engine is initialized
         policy.init()
         parent_id = kwargs.get(self._parent_id_name)
+        # If the parent_id exist, we should get orig_obj with
+        # self._parent_id_name field.
+        if parent_id and self._parent_id_name not in field_list:
+            field_list.append(self._parent_id_name)
         orig_obj = self._item(request, id, field_list=field_list,
                               parent_id=parent_id)
         orig_object_copy = copy.copy(orig_obj)
@@ -640,6 +654,10 @@ class Controller(object):
         # which attributes are set explicitly so that it can distinguish them
         # from the ones that are set to their default values.
         orig_obj[n_const.ATTRIBUTES_TO_UPDATE] = body[self._resource].keys()
+        # Then get the ext_parent_id, format to ext_parent_parent_resource_id
+        if self._parent_id_name in orig_obj:
+            self._set_parent_id_into_ext_resources_request(
+                request, orig_obj, parent_id)
         try:
             policy.enforce(request.context,
                            action,
@@ -759,6 +777,37 @@ class Controller(object):
             msg = _('The resource could not be found.')
             raise webob.exc.HTTPNotFound(msg)
 
+    def _set_parent_id_into_ext_resources_request(
+            self, request, resource_item, parent_id, is_get=False):
+        if not parent_id:
+            return
+
+        # This will pass most create/update/delete cases
+        if not is_get and (request.context.is_admin or
+                           request.context.is_advsvc or
+                           self.parent['member_name'] not in
+                           n_const.EXT_PARENT_RESOURCE_MAPPING or
+                           resource_item.get(self._parent_id_name)):
+            return
+
+        # Then we arrive here, that means the request or get obj contains
+        # ext_parent. If this func is called by list/get, and it contains
+        # _parent_id_name. We need to re-add the ex_parent prefix to policy.
+        if is_get:
+            if (not request.context.is_admin or
+                    not request.context.is_advsvc and
+                    self.parent['member_name'] in
+                    n_const.EXT_PARENT_RESOURCE_MAPPING):
+                resource_item.setdefault(
+                    "%s_%s" % (n_const.EXT_PARENT_PREFIX,
+                               self._parent_id_name),
+                    parent_id)
+        # If this func is called by create/update/delete, we just add.
+        else:
+            resource_item.setdefault(
+                "%s_%s" % (n_const.EXT_PARENT_PREFIX, self._parent_id_name),
+                parent_id)
+
 
 def create_resource(collection, resource, plugin, params, allow_bulk=False,
                     member_actions=None, parent=None, allow_pagination=False,

neutron/common/constants.py

@@ -13,8 +13,9 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+from neutron_lib.api.definitions import l3
 from neutron_lib import constants as lib_constants
-
+from neutron_lib.plugins import constants as plugin_consts
 
 ROUTER_PORT_OWNERS = lib_constants.ROUTER_INTERFACE_OWNERS_SNAT + \
     (lib_constants.DEVICE_OWNER_ROUTER_GW,)
@@ -230,3 +231,16 @@ IEC_BASE = 1024
 
 # Port bindings handling
 NO_ACTIVE_BINDING = 'no_active_binding'
+
+# Registered extension parent resource check mapping
+# If we want to register some service plugin resources into policy and check
+# the owner when operating their subresources. We can write here to use
+# existing policy engine for parent resource owner check.
+# Each entry here should be PARENT_RESOURCE_NAME: SERVICE_PLUGIN_NAME,
+# PARENT_RESOURCE_NAME is usually from api definition.
+# SERVICE_PLUGIN_NAME is the service plugin which introduced the resource and
+# registered the service plugin name in neutron-lib.
+EXT_PARENT_RESOURCE_MAPPING = {
+    l3.FLOATINGIP: plugin_consts.L3
+}
+EXT_PARENT_PREFIX = 'ext_parent'

neutron/policy.py

@@ -224,7 +224,11 @@ class OwnerCheck(policy.Check):
         # resource is handled by the core plugin. It might be worth
         # having a way to map resources to plugins so to make this
         # check more general
-        f = getattr(directory.get_plugin(), 'get_%s' % resource_type)
+        plugin = directory.get_plugin()
+        if resource_type in const.EXT_PARENT_RESOURCE_MAPPING:
+            plugin = directory.get_plugin(
+                const.EXT_PARENT_RESOURCE_MAPPING[resource_type])
+        f = getattr(plugin, 'get_%s' % resource_type)
         # f *must* exist, if not found it is better to let neutron
         # explode. Check will be performed with admin context
         try:
@@ -272,6 +276,13 @@ class OwnerCheck(policy.Check):
                     reason=err_reason)
             parent_foreign_key = _RESOURCE_FOREIGN_KEYS.get(
                 "%ss" % parent_res, None)
+            if parent_res == const.EXT_PARENT_PREFIX:
+                for resource in const.EXT_PARENT_RESOURCE_MAPPING:
+                    key = "%s_%s_id" % (const.EXT_PARENT_PREFIX, resource)
+                    if key in target:
+                        parent_foreign_key = key
+                        parent_res = resource
+                        break
             if not parent_foreign_key:
                 err_reason = (_("Unable to verify match:%(match)s as the "
                                 "parent resource: %(res)s was not found") %

neutron/tests/etc/policy.json

@@ -13,6 +13,7 @@
     "shared_address_scopes": "field:address_scopes:shared=True",
     "external": "field:networks:router:external=True",
     "default": "rule:admin_or_owner",
+    "admin_or_ext_parent_owner": "rule:context_is_admin or tenant_id:%(ext_parent:tenant_id)s",
 
     "create_subnet": "rule:admin_or_network_owner",
     "create_subnet:segment_id": "rule:admin_only",
@@ -250,9 +251,9 @@
     "update_log": "rule:admin_only",
     "delete_log": "rule:admin_only",
 
-    "create_floatingip_port_forwarding": "rule:regular_user",
-    "get_floatingip_port_forwarding": "rule:regular_user",
-    "get_floatingip_port_forwardings": "rule:regular_user",
-    "update_floatingip_port_forwarding": "rule:regular_user",
-    "delete_floatingip_port_forwarding": "rule:regular_user"
+    "create_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner",
+    "get_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner",
+    "get_floatingip_port_forwardings": "rule:admin_or_ext_parent_owner",
+    "update_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner",
+    "delete_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner"
 }

neutron/tests/unit/test_policy.py

@@ -599,3 +599,26 @@ class NeutronPolicyTestCase(base.BaseTestCase):
         result = policy._is_attribute_explicitly_set(
             attr, resource, target, action)
         self.assertFalse(result)
+
+    @mock.patch("neutron.common.constants.EXT_PARENT_RESOURCE_MAPPING",
+                {'parentresource': 'registered_plugin_name'})
+    @mock.patch("neutron_lib.plugins.directory.get_plugin")
+    def test_enforce_tenant_id_check_parent_resource_owner(
+            self, mock_get_plugin):
+
+        def fakegetparent(*args, **kwargs):
+            return {'tenant_id': 'fake'}
+        mock_plugin = mock.Mock()
+        mock_plugin.get_parentresource = fakegetparent
+        mock_get_plugin.return_value = mock_plugin
+
+        self._set_rules(
+            admin_or_ext_parent_owner="rule:context_is_admin or "
+                                      "tenant_id:%(ext_parent:tenant_id)s",
+            create_parentresource_subresource="rule:admin_or_ext_parent_owner")
+        self.fakepolicyinit()
+        action = 'create_parentresource_subresource'
+        target = {'ext_parent_parentresource_id': 'whatever', 'foo': 'bar'}
+        result = policy.enforce(self.context, action, target)
+        mock_get_plugin.assert_called_with('registered_plugin_name')
+        self.assertTrue(result)

releasenotes/notes/extend-policy-for-extension-resource-owner-check-4a19b84889660506.yaml

@@ -0,0 +1,9 @@
+---
+features:
+  - |
+    Introduces extension parent resources owner check in
+    ``neutron.policy.OwnerCheck``. It can be used by registering an extension
+    parent resource and service plugin which introduced the corresponding
+    parent resource into ``EXT_PARENT_RESOURCE_MAPPING`` located in
+    ``neutron.common.constants``. And introduces a new policy role
+    ``admin_or_ext_parent_owner`` into ``policy.json`` for this function.