Use policy for control the 'host' field

In before, we manually remove the 'host' field from container
if the request was not made by admin users. This patch allows
operators to define a policy to decide whether to expose
this field. This is more flexible.

In addition, the field removal logic is moved to 'format_container'
to ensure that the field was removed (by policy) in all HTTP
methods (instead of 'GET' method only).

The policy is of the form "container:get_one:<field_name>".
For example, if the field is 'host', the corresponding policy
would be "container:get_one:host". If there is no policy defined
for a field, this field is allowed. Otherwise, the visibility
of this field is decided by the defined policy. The policy
enforcement logic is extended to allow a field 'might_not_exist'.

Change-Id: I40a0bfb4c26b8b46218f36ae0da9043b0f40b10b

zun/api/controllers/v1/containers.py

@@ -31,6 +31,7 @@ from zun.common import context as zun_context
 from zun.common import exception
 from zun.common.i18n import _
 from zun.common import name_generator
+from zun.common.policies import container as policies
 from zun.common import policy
 from zun.common import utils
 import zun.conf
@@ -66,9 +67,10 @@ class ContainerCollection(collection.Collection):
     @staticmethod
     def convert_with_links(rpc_containers, limit, url=None,
                            expand=False, **kwargs):
+        context = pecan.request.context
         collection = ContainerCollection()
         collection.containers = \
-            [view.format_container(url, p) for p in rpc_containers]
+            [view.format_container(url, p, context) for p in rpc_containers]
         collection.next = collection.get_next(limit, url=url, **kwargs)
         return collection
 
@@ -210,6 +212,8 @@ class ContainersController(base.Controller):
         filters = {}
         for filter_key in container_allowed_filters:
             if filter_key in kwargs:
+                policy_action = policies.CONTAINER % ('get_one:' + filter_key)
+                context.can(policy_action, might_not_exist=True)
                 filter_value = kwargs.pop(filter_key)
                 filters[filter_key] = filter_value
         marker_obj = None
@@ -228,9 +232,6 @@ class ContainersController(base.Controller):
                                             sort_key,
                                             sort_dir,
                                             filters=filters)
-        if not context.is_admin:
-            for container in containers:
-                del container.host
         return ContainerCollection.convert_with_links(containers, limit,
                                                       url=resource_url,
                                                       expand=expand,
@@ -257,9 +258,8 @@ class ContainersController(base.Controller):
         except exception.ContainerHostNotUp:
             raise exception.ServerNotUsable
 
-        if not context.is_admin:
+        return view.format_container(pecan.request.host_url, container,
-            del container.host
+                                     context)
-        return view.format_container(pecan.request.host_url, container)
 
     def _generate_name_for_container(self):
         """Generate a random name like: zeta-22-container."""
@@ -366,7 +366,8 @@ class ContainersController(base.Controller):
         pecan.response.location = link.build_url('containers',
                                                  new_container.uuid)
         pecan.response.status = 202
-        return view.format_container(pecan.request.host_url, new_container)
+        return view.format_container(pecan.request.host_url, new_container,
+                                     context)
 
     def _set_default_resource_limit(self, container_dict):
         # NOTE(kiennt): Default disk size will be set later.
@@ -567,7 +568,8 @@ class ContainersController(base.Controller):
         context = pecan.request.context
         compute_api = pecan.request.compute_api
         container = compute_api.container_update(context, container, patch)
-        return view.format_container(pecan.request.host_url, container)
+        return view.format_container(pecan.request.host_url, container,
+                                     context)
 
     @base.Controller.api_version("1.1", "1.13")
     @pecan.expose('json')
@@ -587,7 +589,8 @@ class ContainersController(base.Controller):
         container.name = name
         context = pecan.request.context
         container.save(context)
-        return view.format_container(pecan.request.host_url, container)
+        return view.format_container(pecan.request.host_url, container,
+                                     context)
 
     @pecan.expose('json')
     @exception.wrap_pecan_controller_exception

zun/api/controllers/v1/views/containers_view.py

@@ -14,6 +14,7 @@
 import itertools
 
 from zun.api.controllers import link
+from zun.common.policies import container as policies
 
 _basic_keys = (
     'uuid',
@@ -49,10 +50,14 @@ _basic_keys = (
 )
 
 
-def format_container(url, container):
+def format_container(url, container, context):
     def transform(key, value):
         if key not in _basic_keys:
             return
+        # strip the key if it is not allowed by policy
+        policy_action = policies.CONTAINER % ('get_one:%s' % key)
+        if not context.can(policy_action, fatal=False, might_not_exist=True):
+            return
         if key == 'uuid':
             yield ('uuid', value)
             yield ('links', [link.make_link(

zun/common/context.py

@@ -122,7 +122,7 @@ class RequestContext(context.RequestContext):
 
         return context
 
-    def can(self, action, target=None, fatal=True):
+    def can(self, action, target=None, fatal=True, might_not_exist=False):
         """Verifies that the given action is valid on the target in this context.
 
         :param action: string representing the action to be checked.
@@ -133,6 +133,9 @@ class RequestContext(context.RequestContext):
             {'project_id': self.project_id, 'user_id': self.user_id}
         :param fatal: if False, will return False when an
             exception.NotAuthorized occurs.
+        :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.
 
         :raises zun.common.exception.NotAuthorized: if verification fails and
             fatal is True.
@@ -145,7 +148,8 @@ class RequestContext(context.RequestContext):
                       'user_id': self.user_id}
 
         try:
-            return policy.authorize(self, action, target)
+            return policy.authorize(self, action, target,
+                                    might_not_exist=might_not_exist)
         except exception.NotAuthorized:
             if fatal:
                 raise

zun/common/policies/container.py

@@ -72,6 +72,29 @@ rules = [
             }
         ]
     ),
+    policy.DocumentedRuleDefault(
+        name=CONTAINER % 'get_one:host',
+        check_str=base.RULE_ADMIN_API,
+        description='Retrieve the host field of containers.',
+        operations=[
+            {
+                'path': '/v1/containers/{container_ident}',
+                'method': 'GET'
+            },
+            {
+                'path': '/v1/containers',
+                'method': 'GET'
+            },
+            {
+                'path': '/v1/containers',
+                'method': 'POST'
+            },
+            {
+                'path': '/v1/containers/{container_ident}',
+                'method': 'PATCH'
+            }
+        ]
+    ),
     policy.DocumentedRuleDefault(
         name=CONTAINER % 'get_one_all_projects',
         check_str=base.RULE_ADMIN_API,

zun/common/policy.py

@@ -102,7 +102,8 @@ def enforce(context, rule=None, target=None,
                             do_raise=do_raise, exc=exc, *args, **kwargs)
 
 
-def authorize(context, action, target, do_raise=True, exc=None):
+def authorize(context, action, target, do_raise=True, exc=None,
+              might_not_exist=False):
     """Verifies that the action is valid on the target in this context.
 
        :param context: zun context
@@ -115,10 +116,13 @@ def authorize(context, action, target, do_raise=True, exc=None):
        :param do_raise: if True (the default), raises PolicyNotAuthorized;
            if False, returns False
        :param exc: Class of the exception to raise if the check fails.
-                   Any remaining arguments passed to :meth:`authorize` (both
+            Any remaining arguments passed to :meth:`authorize` (both
-                   positional and keyword arguments) will be passed to
+            positional and keyword arguments) will be passed to
-                   the exception class. If not specified,
+            the exception class. If not specified,
-                   :class:`PolicyNotAuthorized` will be used.
+            :class:`PolicyNotAuthorized` will be used.
+       :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.
 
        :raises zun.common.exception.PolicyNotAuthorized: if verification fails
            and do_raise is True. Or if 'exc' is specified it will raise an
@@ -131,6 +135,8 @@ def authorize(context, action, target, do_raise=True, exc=None):
     credentials = context.to_policy_values()
     if not exc:
         exc = exception.PolicyNotAuthorized
+    if might_not_exist and not (_ENFORCER.rules and action in _ENFORCER.rules):
+        return True
     try:
         result = _ENFORCER.enforce(action, target, credentials,
                                    do_raise=do_raise, exc=exc, action=action)

zun/tests/unit/api/controllers/v1/test_containers.py

@@ -166,6 +166,29 @@ class TestContainerController(api_base.FunctionalTest):
                              content_type='application/json')
 
         self.assertEqual(202, response.status_int)
+        self.assertNotIn('host', response.json)
+        self.assertTrue(mock_container_create.called)
+        self.assertTrue(mock_container_create.call_args[1]['run'] is False)
+        mock_neutron_get_network.assert_called_once()
+
+    @patch('zun.common.context.RequestContext.can')
+    @patch('zun.network.neutron.NeutronAPI.get_available_network')
+    @patch('zun.compute.api.API.container_create')
+    @patch('zun.compute.api.API.image_search')
+    def test_create_container_by_admin(
+            self, mock_search, mock_container_create, mock_neutron_get_network,
+            mock_can):
+        mock_container_create.side_effect = lambda x, y, **z: y
+        mock_can.return_value = True
+        params = ('{"name": "MyDocker", "image": "ubuntu",'
+                  '"command": "env", "memory": "512",'
+                  '"environment": {"key1": "val1", "key2": "val2"}}')
+        response = self.post('/v1/containers/',
+                             params=params,
+                             content_type='application/json')
+
+        self.assertEqual(202, response.status_int)
+        self.assertIn('host', response.json)
         self.assertTrue(mock_container_create.called)
         self.assertTrue(mock_container_create.call_args[1]['run'] is False)
         mock_neutron_get_network.assert_called_once()
@@ -269,6 +292,7 @@ class TestContainerController(api_base.FunctionalTest):
         self.assertEqual(20, c.get('disk'))
         self.assertEqual({"Name": "no", "MaximumRetryCount": "0"},
                          c.get('restart_policy'))
+        self.assertNotIn('host', c)
         requested_networks = \
             mock_container_create.call_args[1]['requested_networks']
         self.assertEqual(1, len(requested_networks))
@@ -328,6 +352,7 @@ class TestContainerController(api_base.FunctionalTest):
         self.assertIsNone(c.get('runtime'))
         self.assertIsNone(c.get('hostname'))
         self.assertEqual({}, c.get('restart_policy'))
+        self.assertNotIn('host', c)
         mock_neutron_get_network.assert_called_once()
         requested_networks = \
             mock_container_create.call_args[1]['requested_networks']
@@ -365,6 +390,7 @@ class TestContainerController(api_base.FunctionalTest):
         # [1] https://bugs.launchpad.net/zun/+bug/1746401
         # self.assertEqual(10, c.get('disk'))
         self.assertEqual({}, c.get('environment'))
+        self.assertNotIn('host', c)
         mock_neutron_get_network.assert_called_once()
         extra_spec = \
             mock_container_create.call_args[1]['extra_spec']
@@ -400,6 +426,7 @@ class TestContainerController(api_base.FunctionalTest):
         self.assertEqual('512', c.get('memory'))
         self.assertEqual({"Name": "no", "MaximumRetryCount": "0"},
                          c.get('restart_policy'))
+        self.assertNotIn('host', c)
         mock_neutron_get_network.assert_called_once()
         requested_networks = \
             mock_container_create.call_args[1]['requested_networks']
@@ -436,6 +463,7 @@ class TestContainerController(api_base.FunctionalTest):
         self.assertEqual('512', c.get('memory'))
         self.assertEqual({"Name": "no", "MaximumRetryCount": "0"},
                          c.get('restart_policy'))
+        self.assertNotIn('host', c)
         mock_neutron_get_network.assert_called_once()
         requested_networks = \
             mock_container_create.call_args[1]['requested_networks']
@@ -472,6 +500,7 @@ class TestContainerController(api_base.FunctionalTest):
         self.assertEqual('512', c.get('memory'))
         self.assertEqual({"Name": "unless-stopped", "MaximumRetryCount": "0"},
                          c.get('restart_policy'))
+        self.assertNotIn('host', c)
         mock_neutron_get_network.assert_called_once()
         requested_networks = \
             mock_container_create.call_args[1]['requested_networks']
@@ -516,6 +545,7 @@ class TestContainerController(api_base.FunctionalTest):
         self.assertEqual('512', c.get('memory'))
         self.assertEqual({"key1": "val1", "key2": "val2"},
                          c.get('environment'))
+        self.assertNotIn('host', c)
         requested_networks = \
             mock_container_create.call_args[1]['requested_networks']
         self.assertEqual(1, len(requested_networks))
@@ -638,6 +668,7 @@ class TestContainerController(api_base.FunctionalTest):
         self.assertEqual('env', c.get('command'))
         self.assertEqual('Creating', c.get('status'))
         self.assertEqual('512', c.get('memory'))
+        self.assertIn('host', c)
         requested_networks = \
             mock_container_create.call_args[1]['requested_networks']
         self.assertEqual(1, len(requested_networks))
@@ -701,6 +732,29 @@ class TestContainerController(api_base.FunctionalTest):
         self.assertEqual(1, len(actual_containers))
         self.assertEqual(test_container['uuid'],
                          actual_containers[0].get('uuid'))
+        self.assertNotIn('host', actual_containers[0])
+
+    @patch('zun.common.context.RequestContext.can')
+    @patch('zun.objects.Container.list')
+    def test_get_all_containers_by_admin(self, mock_container_list, mock_can):
+        mock_can.return_value = True
+        test_container = utils.get_test_container()
+        containers = [objects.Container(self.context, **test_container)]
+        mock_container_list.return_value = containers
+
+        response = self.get('/v1/containers/')
+
+        mock_container_list.assert_called_once_with(mock.ANY,
+                                                    1000, None, 'id', 'asc',
+                                                    filters={})
+        context = mock_container_list.call_args[0][0]
+        self.assertIs(False, context.all_projects)
+        self.assertEqual(200, response.status_int)
+        actual_containers = response.json['containers']
+        self.assertEqual(1, len(actual_containers))
+        self.assertEqual(test_container['uuid'],
+                         actual_containers[0].get('uuid'))
+        self.assertIn('host', actual_containers[0])
 
     @patch('zun.common.policy.enforce')
     @patch('zun.objects.Container.list')
@@ -776,6 +830,22 @@ class TestContainerController(api_base.FunctionalTest):
         self.assertEqual(test_container['uuid'],
                          actual_containers[0].get('uuid'))
 
+    @patch('zun.objects.Container.list')
+    def test_get_all_containers_with_filter_disallow(
+            self, mock_container_list):
+        test_container = utils.get_test_container()
+        containers = [objects.Container(self.context, **test_container)]
+        mock_container_list.return_value = containers
+
+        response = self.get('/v1/containers/?host=fake-name',
+                            expect_errors=True)
+        self.assertEqual(403, response.status_int)
+        self.assertEqual('application/json', response.content_type)
+        rule = 'container:get_one:host'
+        self.assertEqual(
+            "Policy doesn't allow %s to be performed." % rule,
+            response.json['errors'][0]['detail'])
+
     @patch('zun.objects.Container.list')
     def test_get_all_containers_with_unknown_parameter(
             self, mock_container_list):
@@ -807,12 +877,10 @@ class TestContainerController(api_base.FunctionalTest):
         self.assertEqual(test_container['uuid'],
                          actual_containers[0].get('uuid'))
 
-    @patch('zun.common.policy.enforce')
     @patch('zun.compute.api.API.container_show')
     @patch('zun.objects.Container.get_by_uuid')
     def test_get_one_by_uuid(self, mock_container_get_by_uuid,
-                             mock_container_show, mock_policy):
+                             mock_container_show):
-        mock_policy.return_value = True
         test_container = utils.get_test_container()
         test_container_obj = objects.Container(self.context, **test_container)
         mock_container_get_by_uuid.return_value = test_container_obj
@@ -828,6 +896,30 @@ class TestContainerController(api_base.FunctionalTest):
         self.assertEqual(200, response.status_int)
         self.assertEqual(test_container['uuid'],
                          response.json['uuid'])
+        self.assertNotIn('host', response.json)
+
+    @patch('zun.common.context.RequestContext.can')
+    @patch('zun.compute.api.API.container_show')
+    @patch('zun.objects.Container.get_by_uuid')
+    def test_get_one_by_admin(self, mock_container_get_by_uuid,
+                              mock_container_show, mock_can):
+        mock_can.return_value = True
+        test_container = utils.get_test_container()
+        test_container_obj = objects.Container(self.context, **test_container)
+        mock_container_get_by_uuid.return_value = test_container_obj
+        mock_container_show.return_value = test_container_obj
+
+        response = self.get('/v1/containers/%s/' % test_container['uuid'])
+
+        mock_container_get_by_uuid.assert_called_once_with(
+            mock.ANY,
+            test_container['uuid'])
+        context = mock_container_get_by_uuid.call_args[0][0]
+        self.assertIs(False, context.all_projects)
+        self.assertEqual(200, response.status_int)
+        self.assertEqual(test_container['uuid'],
+                         response.json['uuid'])
+        self.assertIn('host', response.json)
 
     @patch('zun.common.policy.enforce')
     @patch('zun.compute.api.API.container_show')
@@ -874,6 +966,33 @@ class TestContainerController(api_base.FunctionalTest):
 
         self.assertEqual(200, response.status_int)
         self.assertTrue(mock_update.called)
+        self.assertNotIn('host', response.json)
+
+    @patch('zun.common.context.RequestContext.can')
+    @patch('zun.objects.ComputeNode.get_by_name')
+    @patch('zun.compute.api.API.container_update')
+    @patch('zun.objects.Container.get_by_uuid')
+    def test_patch_by_admin(self, mock_container_get_by_uuid, mock_update,
+                            mock_computenode, mock_can):
+        mock_can.return_value = True
+        test_container = utils.get_test_container()
+        test_container_obj = objects.Container(self.context, **test_container)
+        mock_container_get_by_uuid.return_value = test_container_obj
+        mock_update.return_value = test_container_obj
+        test_host = utils.get_test_compute_node()
+        numa = objects.numa.NUMATopology._from_dict(test_host['numa_topology'])
+        test_host['numa_topology'] = numa
+        test_host_obj = objects.ComputeNode(self.context, **test_host)
+        mock_computenode.return_value = test_host_obj
+        params = {'cpu': 1}
+        container_uuid = test_container.get('uuid')
+        response = self.patch_json(
+            '/containers/%s/' % container_uuid,
+            params=params)
+
+        self.assertEqual(200, response.status_int)
+        self.assertTrue(mock_update.called)
+        self.assertIn('host', response.json)
 
     def _action_test(self, test_container_obj, action, ident_field,
                      mock_container_action, status_code, query_param=''):