Merge "Expose allocation owner to additional policy checks"

ironic/api/controllers/v1/allocation.py

@@ -334,8 +334,7 @@ class AllocationsController(pecan.rest.RestController):
                        of the resource to be returned.
         :param owner: Filter by owner.
         """
-        cdict = api.request.context.to_policy_values()
+        owner = api_utils.check_list_policy('allocation', owner)
-        policy.authorize('baremetal:allocation:get', cdict, cdict)
 
         self._check_allowed_allocation_fields(fields)
         if owner is not None and not api_utils.allow_allocation_owner():
@@ -355,13 +354,10 @@ class AllocationsController(pecan.rest.RestController):
         :param fields: Optional, a list with a specified set of fields
                        of the resource to be returned.
         """
-        cdict = api.request.context.to_policy_values()
+        rpc_allocation = api_utils.check_allocation_policy_and_retrieve(
-        policy.authorize('baremetal:allocation:get', cdict, cdict)
+            'baremetal:allocation:get', allocation_ident)
-
         self._check_allowed_allocation_fields(fields)
 
-        rpc_allocation = api_utils.get_rpc_allocation_with_suffix(
-            allocation_ident)
         return Allocation.convert_with_links(rpc_allocation, fields=fields)
 
     @METRICS.timer('AllocationsController.post')
@@ -485,9 +481,10 @@ class AllocationsController(pecan.rest.RestController):
         if not api_utils.allow_allocation_update():
             raise webob_exc.HTTPMethodNotAllowed(_(
                 "The API version does not allow updating allocations"))
+
         context = api.request.context
-        cdict = context.to_policy_values()
+        rpc_allocation = api_utils.check_allocation_policy_and_retrieve(
-        policy.authorize('baremetal:allocation:update', cdict, cdict)
+            'baremetal:allocation:update', allocation_ident)
         self._validate_patch(patch)
         names = api_utils.get_patch_values(patch, '/name')
         for name in names:
@@ -495,8 +492,6 @@ class AllocationsController(pecan.rest.RestController):
                 msg = _("Cannot update allocation with invalid name "
                         "'%(name)s'") % {'name': name}
                 raise exception.Invalid(msg)
-        rpc_allocation = api_utils.get_rpc_allocation_with_suffix(
-            allocation_ident)
         allocation_dict = rpc_allocation.as_dict()
         allocation = Allocation(**api_utils.apply_jsonpatch(allocation_dict,
                                                             patch))
@@ -528,11 +523,8 @@ class AllocationsController(pecan.rest.RestController):
         :param allocation_ident: UUID or logical name of an allocation.
         """
         context = api.request.context
-        cdict = context.to_policy_values()
+        rpc_allocation = api_utils.check_allocation_policy_and_retrieve(
-        policy.authorize('baremetal:allocation:delete', cdict, cdict)
+            'baremetal:allocation:delete', allocation_ident)
-
-        rpc_allocation = api_utils.get_rpc_allocation_with_suffix(
-            allocation_ident)
         if rpc_allocation.node_id:
             node_uuid = objects.Node.get_by_id(api.request.context,
                                                rpc_allocation.node_id).uuid

ironic/api/controllers/v1/node.py

@@ -1836,7 +1836,7 @@ class NodesController(rest.RestController):
                                      with description field contains matching
                                      value.
         """
-        owner = api_utils.check_node_list_policy(owner)
+        owner = api_utils.check_list_policy('node', owner)
 
         api_utils.check_allow_specify_fields(fields)
         api_utils.check_allowed_fields(fields)
@@ -1913,7 +1913,7 @@ class NodesController(rest.RestController):
                                      with description field contains matching
                                      value.
         """
-        owner = api_utils.check_node_list_policy(owner)
+        owner = api_utils.check_list_policy('node', owner)
 
         api_utils.check_for_invalid_state_and_allow_filter(provision_state)
         api_utils.check_allow_specify_driver(driver)
@@ -2139,7 +2139,8 @@ class NodesController(rest.RestController):
             # check if updating a provisioned node's owner is allowed
             if rpc_node.provision_state == ir_states.ACTIVE:
                 try:
-                    api_utils.check_node_policy(
+                    api_utils.check_owner_policy(
+                        'node',
                         'baremetal:node:update_owner_provisioned',
                         rpc_node['owner'])
                 except exception.HTTPForbidden:

ironic/api/controllers/v1/utils.py

@@ -1167,18 +1167,19 @@ def check_policy(policy_name):
     policy.authorize(policy_name, cdict, cdict)
 
 
-def check_node_policy(policy_name, node_owner):
+def check_owner_policy(object_type, policy_name, owner):
-    """Check if the specified policy authorizes this request on a node.
+    """Check if the policy authorizes this request on an object.
 
+    :param: object_type: type of object being checked
     :param: policy_name: Name of the policy to check.
-    :param: node_owner: the node owner
+    :param: owner: the owner
 
     :raises: HTTPForbidden if the policy forbids access.
     """
     cdict = api.request.context.to_policy_values()
 
     target_dict = dict(cdict)
-    target_dict['node.owner'] = node_owner
+    target_dict[object_type + '.owner'] = owner
     policy.authorize(policy_name, target_dict, cdict)
 
 
@@ -1206,27 +1207,52 @@ def check_node_policy_and_retrieve(policy_name, node_ident,
         policy.authorize(policy_name, cdict, cdict)
         raise
 
-    check_node_policy(policy_name, rpc_node['owner'])
+    check_owner_policy('node', policy_name, rpc_node['owner'])
     return rpc_node
 
 
-def check_node_list_policy(owner=None):
+def check_allocation_policy_and_retrieve(policy_name, allocation_ident):
-    """Check if the specified policy authorizes this request on a node.
+    """Check if the specified policy authorizes request on allocation.
 
+    :param: policy_name: Name of the policy to check.
+    :param: allocation_ident: the UUID or logical name of a node.
+
+    :raises: HTTPForbidden if the policy forbids access.
+    :raises: AllocationNotFound if the node is not found.
+    :return: RPC node identified by node_ident
+    """
+    try:
+        rpc_allocation = get_rpc_allocation_with_suffix(
+            allocation_ident)
+    except exception.AllocationNotFound:
+        # don't expose non-existence unless requester
+        # has generic access to policy
+        cdict = api.request.context.to_policy_values()
+        policy.authorize(policy_name, cdict, cdict)
+        raise
+
+    check_owner_policy('allocation', policy_name, rpc_allocation['owner'])
+    return rpc_allocation
+
+
+def check_list_policy(object_type, owner=None):
+    """Check if the list policy authorizes this request on an object.
+
+    :param: object_type: type of object being checked
     :param: owner: owner filter for list query, if any
 
     :raises: HTTPForbidden if the policy forbids access.
-    :raises: NodeNotFound if the node is not found.
     :return: owner that should be used for list query, if needed
     """
     cdict = api.request.context.to_policy_values()
     try:
-        policy.authorize('baremetal:node:list_all', cdict, cdict)
+        policy.authorize('baremetal:%s:list_all' % object_type,
+                         cdict, cdict)
     except exception.HTTPForbidden:
         project_owner = cdict.get('project_id')
         if (not project_owner or (owner and owner != project_owner)):
             raise
-        policy.authorize('baremetal:node:list', cdict, cdict)
+        policy.authorize('baremetal:%s:list' % object_type, cdict, cdict)
         return project_owner
     return owner
 

ironic/common/policy.py

@@ -66,6 +66,9 @@ default_policies = [
     policy.RuleDefault('is_node_owner',
                        'project_id:%(node.owner)s',
                        description='Owner of node'),
+    policy.RuleDefault('is_allocation_owner',
+                       'project_id:%(allocation.owner)s',
+                       description='Owner of allocation'),
 ]
 
 # NOTE(deva): to follow policy-in-code spec, we define defaults for
@@ -437,9 +440,18 @@ allocation_policies = [
         'baremetal:allocation:get',
         'rule:is_admin or rule:is_observer',
         'Retrieve Allocation records',
-        [{'path': '/allocations', 'method': 'GET'},
+        [{'path': '/allocations/{allocation_id}', 'method': 'GET'},
-         {'path': '/allocations/{allocation_id}', 'method': 'GET'},
          {'path': '/nodes/{node_ident}/allocation', 'method': 'GET'}]),
+    policy.DocumentedRuleDefault(
+        'baremetal:allocation:list',
+        'rule:baremetal:allocation:get',
+        'Retrieve multiple Allocation records, filtered by owner',
+        [{'path': '/allocations', 'method': 'GET'}]),
+    policy.DocumentedRuleDefault(
+        'baremetal:allocation:list_all',
+        'rule:baremetal:allocation:get',
+        'Retrieve multiple Allocation records',
+        [{'path': '/allocations', 'method': 'GET'}]),
     policy.DocumentedRuleDefault(
         'baremetal:allocation:create',
         'rule:is_admin',

ironic/tests/unit/api/controllers/v1/test_allocation.py

@@ -268,6 +268,78 @@ class TestListAllocations(test_api_base.BaseApiTest):
                                  expect_errors=True)
         self.assertEqual(http_client.NOT_FOUND, response.status_int)
 
+    @mock.patch.object(policy, 'authorize', spec=True)
+    def test_allocation_get_all_forbidden(self, mock_authorize):
+        def mock_authorize_function(rule, target, creds):
+            raise exception.HTTPForbidden(resource='fake')
+        mock_authorize.side_effect = mock_authorize_function
+
+        response = self.get_json('/allocations', expect_errors=True,
+                                 headers={
+                                     api_base.Version.string: '1.60',
+                                     'X-Project-Id': '12345'
+                                 })
+        self.assertEqual(http_client.FORBIDDEN, response.status_int)
+
+    @mock.patch.object(policy, 'authorize', spec=True)
+    def test_allocation_get_all_forbidden_no_project(self, mock_authorize):
+        def mock_authorize_function(rule, target, creds):
+            if rule == 'baremetal:allocation:list_all':
+                raise exception.HTTPForbidden(resource='fake')
+            return True
+        mock_authorize.side_effect = mock_authorize_function
+
+        response = self.get_json('/allocations', expect_errors=True,
+                                 headers={
+                                     api_base.Version.string: '1.59',
+                                 })
+        self.assertEqual(http_client.FORBIDDEN, response.status_int)
+
+    @mock.patch.object(policy, 'authorize', spec=True)
+    def test_allocation_get_all_forbid_owner_proj_mismatch(
+            self, mock_authorize):
+        def mock_authorize_function(rule, target, creds):
+            if rule == 'baremetal:allocation:list_all':
+                raise exception.HTTPForbidden(resource='fake')
+            return True
+        mock_authorize.side_effect = mock_authorize_function
+
+        response = self.get_json('/allocations?owner=54321',
+                                 expect_errors=True,
+                                 headers={
+                                     api_base.Version.string: '1.60',
+                                     'X-Project-Id': '12345'
+                                 })
+        self.assertEqual(http_client.FORBIDDEN, response.status_int)
+
+    @mock.patch.object(policy, 'authorize', spec=True)
+    def test_allocation_get_all_non_admin(self, mock_authorize):
+        def mock_authorize_function(rule, target, creds):
+            if rule == 'baremetal:allocation:list_all':
+                raise exception.HTTPForbidden(resource='fake')
+            return True
+        mock_authorize.side_effect = mock_authorize_function
+
+        allocations = []
+        for id in range(5):
+            allocation = obj_utils.create_test_allocation(
+                self.context,
+                uuid=uuidutils.generate_uuid(),
+                owner='12345')
+            allocations.append(allocation.uuid)
+        for id in range(2):
+            allocation = obj_utils.create_test_allocation(
+                self.context,
+                uuid=uuidutils.generate_uuid())
+
+        data = self.get_json('/allocations', headers={
+            api_base.Version.string: '1.60',
+            'X-Project-Id': '12345'})
+        self.assertEqual(len(allocations), len(data['allocations']))
+
+        uuids = [n['uuid'] for n in data['allocations']]
+        self.assertEqual(sorted(allocations), sorted(uuids))
+
     def test_sort_key(self):
         allocations = []
         for id_ in range(3):

ironic/tests/unit/api/controllers/v1/test_expose.py

@@ -52,7 +52,7 @@ class TestExposedAPIMethodsCheckPolicy(test_base.TestCase):
             src = inspect.getsource(func)
             self.assertTrue(
                 ('api_utils.check_node_policy_and_retrieve' in src) or
-                ('api_utils.check_node_list_policy' in src) or
+                ('api_utils.check_list_policy' in src) or
                 ('self._get_node_and_topic' in src) or
                 ('api_utils.check_port_policy_and_retrieve' in src) or
                 ('api_utils.check_port_list_policy' in src) or

ironic/tests/unit/api/controllers/v1/test_utils.py

@@ -797,30 +797,30 @@ class TestPortgroupIdent(base.TestCase):
                           self.invalid_name)
 
 
-class TestCheckNodePolicy(base.TestCase):
+class TestCheckOwnerPolicy(base.TestCase):
     def setUp(self):
-        super(TestCheckNodePolicy, self).setUp()
+        super(TestCheckOwnerPolicy, self).setUp()
         self.valid_node_uuid = uuidutils.generate_uuid()
         self.node = test_api_utils.post_get_test_node()
         self.node['owner'] = '12345'
 
     @mock.patch.object(api, 'request', spec_set=["context", "version"])
     @mock.patch.object(policy, 'authorize', spec=True)
-    def test_check_node_policy(
+    def test_check_owner_policy(
             self, mock_authorize, mock_pr
     ):
         mock_pr.version.minor = 50
         mock_pr.context.to_policy_values.return_value = {}
 
-        utils.check_node_policy(
+        utils.check_owner_policy(
-            'fake_policy', self.node['owner']
+            'node', 'fake_policy', self.node['owner']
         )
         mock_authorize.assert_called_once_with(
             'fake_policy', {'node.owner': '12345'}, {})
 
     @mock.patch.object(api, 'request', spec_set=["context", "version"])
     @mock.patch.object(policy, 'authorize', spec=True)
-    def test_check_node_policy_forbidden(
+    def test_check_owner_policy_forbidden(
             self, mock_authorize, mock_pr
     ):
         mock_pr.version.minor = 50
@@ -829,7 +829,8 @@ class TestCheckNodePolicy(base.TestCase):
 
         self.assertRaises(
             exception.HTTPForbidden,
-            utils.check_node_policy,
+            utils.check_owner_policy,
+            'node',
             'fake-policy',
             self.node['owner']
         )
@@ -936,10 +937,89 @@ class TestCheckNodePolicyAndRetrieve(base.TestCase):
         )
 
 
-class TestCheckNodeListPolicy(base.TestCase):
+class TestCheckAllocationPolicyAndRetrieve(base.TestCase):
+    def setUp(self):
+        super(TestCheckAllocationPolicyAndRetrieve, self).setUp()
+        self.valid_allocation_uuid = uuidutils.generate_uuid()
+        self.allocation = test_api_utils.allocation_post_data()
+        self.allocation['owner'] = '12345'
+
     @mock.patch.object(api, 'request', spec_set=["context", "version"])
     @mock.patch.object(policy, 'authorize', spec=True)
-    def test_check_node_list_policy(
+    @mock.patch.object(utils, 'get_rpc_allocation_with_suffix')
+    def test_check_node_policy_and_retrieve(
+            self, mock_graws, mock_authorize, mock_pr
+    ):
+        mock_pr.version.minor = 60
+        mock_pr.context.to_policy_values.return_value = {}
+        mock_graws.return_value = self.allocation
+
+        rpc_allocation = utils.check_allocation_policy_and_retrieve(
+            'fake_policy', self.valid_allocation_uuid
+        )
+        mock_graws.assert_called_once_with(self.valid_allocation_uuid)
+        mock_authorize.assert_called_once_with(
+            'fake_policy', {'allocation.owner': '12345'}, {})
+        self.assertEqual(self.allocation, rpc_allocation)
+
+    @mock.patch.object(api, 'request', spec_set=["context"])
+    @mock.patch.object(policy, 'authorize', spec=True)
+    @mock.patch.object(utils, 'get_rpc_allocation_with_suffix')
+    def test_check_alloc_policy_and_retrieve_no_alloc_policy_forbidden(
+            self, mock_graws, mock_authorize, mock_pr
+    ):
+        mock_pr.context.to_policy_values.return_value = {}
+        mock_authorize.side_effect = exception.HTTPForbidden(resource='fake')
+        mock_graws.side_effect = exception.AllocationNotFound(
+            allocation=self.valid_allocation_uuid)
+
+        self.assertRaises(
+            exception.HTTPForbidden,
+            utils.check_allocation_policy_and_retrieve,
+            'fake-policy',
+            self.valid_allocation_uuid
+        )
+
+    @mock.patch.object(api, 'request', spec_set=["context"])
+    @mock.patch.object(policy, 'authorize', spec=True)
+    @mock.patch.object(utils, 'get_rpc_allocation_with_suffix')
+    def test_check_allocation_policy_and_retrieve_no_allocation(
+            self, mock_graws, mock_authorize, mock_pr
+    ):
+        mock_pr.context.to_policy_values.return_value = {}
+        mock_graws.side_effect = exception.AllocationNotFound(
+            allocation=self.valid_allocation_uuid)
+
+        self.assertRaises(
+            exception.AllocationNotFound,
+            utils.check_allocation_policy_and_retrieve,
+            'fake-policy',
+            self.valid_allocation_uuid
+        )
+
+    @mock.patch.object(api, 'request', spec_set=["context", "version"])
+    @mock.patch.object(policy, 'authorize', spec=True)
+    @mock.patch.object(utils, 'get_rpc_allocation_with_suffix')
+    def test_check_allocation_policy_and_retrieve_policy_forbidden(
+            self, mock_graws, mock_authorize, mock_pr
+    ):
+        mock_pr.version.minor = 50
+        mock_pr.context.to_policy_values.return_value = {}
+        mock_authorize.side_effect = exception.HTTPForbidden(resource='fake')
+        mock_graws.return_value = self.allocation
+
+        self.assertRaises(
+            exception.HTTPForbidden,
+            utils.check_allocation_policy_and_retrieve,
+            'fake-policy',
+            self.valid_allocation_uuid
+        )
+
+
+class TestCheckListPolicy(base.TestCase):
+    @mock.patch.object(api, 'request', spec_set=["context", "version"])
+    @mock.patch.object(policy, 'authorize', spec=True)
+    def test_check_list_policy(
             self, mock_authorize, mock_pr
     ):
         mock_pr.context.to_policy_values.return_value = {
@@ -947,12 +1027,12 @@ class TestCheckNodeListPolicy(base.TestCase):
         }
         mock_pr.version.minor = 50
 
-        owner = utils.check_node_list_policy()
+        owner = utils.check_list_policy('node')
         self.assertIsNone(owner)
 
     @mock.patch.object(api, 'request', spec_set=["context", "version"])
     @mock.patch.object(policy, 'authorize', spec=True)
-    def test_check_node_list_policy_with_owner(
+    def test_check_list_policy_with_owner(
             self, mock_authorize, mock_pr
     ):
         mock_pr.context.to_policy_values.return_value = {
@@ -960,12 +1040,12 @@ class TestCheckNodeListPolicy(base.TestCase):
         }
         mock_pr.version.minor = 50
 
-        owner = utils.check_node_list_policy('12345')
+        owner = utils.check_list_policy('node', '12345')
         self.assertEqual(owner, '12345')
 
     @mock.patch.object(api, 'request', spec_set=["context", "version"])
     @mock.patch.object(policy, 'authorize', spec=True)
-    def test_check_node_list_policy_forbidden(
+    def test_check_list_policy_forbidden(
             self, mock_authorize, mock_pr
     ):
         def mock_authorize_function(rule, target, creds):
@@ -978,12 +1058,13 @@ class TestCheckNodeListPolicy(base.TestCase):
 
         self.assertRaises(
             exception.HTTPForbidden,
-            utils.check_node_list_policy,
+            utils.check_list_policy,
+            'node'
         )
 
     @mock.patch.object(api, 'request', spec_set=["context", "version"])
     @mock.patch.object(policy, 'authorize', spec=True)
-    def test_check_node_list_policy_forbidden_no_project(
+    def test_check_list_policy_forbidden_no_project(
             self, mock_authorize, mock_pr
     ):
         def mock_authorize_function(rule, target, creds):
@@ -996,12 +1077,13 @@ class TestCheckNodeListPolicy(base.TestCase):
 
         self.assertRaises(
             exception.HTTPForbidden,
-            utils.check_node_list_policy,
+            utils.check_list_policy,
+            'node'
         )
 
     @mock.patch.object(api, 'request', spec_set=["context", "version"])
     @mock.patch.object(policy, 'authorize', spec=True)
-    def test_check_node_list_policy_non_admin(
+    def test_check_list_policy_non_admin(
             self, mock_authorize, mock_pr
     ):
         def mock_authorize_function(rule, target, creds):
@@ -1014,12 +1096,12 @@ class TestCheckNodeListPolicy(base.TestCase):
         }
         mock_pr.version.minor = 50
 
-        owner = utils.check_node_list_policy()
+        owner = utils.check_list_policy('node')
         self.assertEqual(owner, '12345')
 
     @mock.patch.object(api, 'request', spec_set=["context", "version"])
     @mock.patch.object(policy, 'authorize', spec=True)
-    def test_check_node_list_policy_non_admin_owner_proj_mismatch(
+    def test_check_list_policy_non_admin_owner_proj_mismatch(
             self, mock_authorize, mock_pr
     ):
         def mock_authorize_function(rule, target, creds):
@@ -1034,7 +1116,8 @@ class TestCheckNodeListPolicy(base.TestCase):
 
         self.assertRaises(
             exception.HTTPForbidden,
-            utils.check_node_list_policy,
+            utils.check_list_policy,
+            'node',
             '54321'
         )
 

ironic/tests/unit/common/test_policy.py

@@ -69,6 +69,19 @@ class PolicyInCodeTestCase(base.TestCase):
         self.assertTrue(policy.check('is_node_owner', target, c1))
         self.assertFalse(policy.check('is_node_owner', target, c2))
 
+    def test_is_allocation_owner(self):
+        c1 = {'project_id': '1234',
+              'project_name': 'demo',
+              'project_domain_id': 'default'}
+        c2 = {'project_id': '5678',
+              'project_name': 'demo',
+              'project_domain_id': 'default'}
+        target = dict.copy(c1)
+        target['allocation.owner'] = '1234'
+
+        self.assertTrue(policy.check('is_allocation_owner', target, c1))
+        self.assertFalse(policy.check('is_allocation_owner', target, c2))
+
     def test_node_get(self):
         creds = {'roles': ['baremetal_observer'], 'project_name': 'demo',
                  'project_domain_id': 'default'}

releasenotes/notes/allocation-added-owner-policy-c650074e68d03289.yaml

@@ -0,0 +1,7 @@
+---
+features:
+  - |
+    Adds ``is_allocation_owner`` policy rule, which can be applied to allocation
+    get/update/delete rules. Also adds ``baremetal:allocation:list`` and
+    ``baremetal:allocation:list_all`` rules for listing owned allocations and all
+    allocations. Default rules are unaffected, so default behavior is unchanged.