diff --git a/ironic/api/controllers/v1/allocation.py b/ironic/api/controllers/v1/allocation.py index 871181ea1f..06e3212c53 100644 --- a/ironic/api/controllers/v1/allocation.py +++ b/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() - policy.authorize('baremetal:allocation:get', cdict, cdict) + owner = api_utils.check_list_policy('allocation', owner) 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() - policy.authorize('baremetal:allocation:get', cdict, cdict) - + rpc_allocation = api_utils.check_allocation_policy_and_retrieve( + '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() - policy.authorize('baremetal:allocation:update', cdict, cdict) + rpc_allocation = api_utils.check_allocation_policy_and_retrieve( + '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() - policy.authorize('baremetal:allocation:delete', cdict, cdict) - - rpc_allocation = api_utils.get_rpc_allocation_with_suffix( - allocation_ident) + rpc_allocation = api_utils.check_allocation_policy_and_retrieve( + 'baremetal:allocation:delete', allocation_ident) if rpc_allocation.node_id: node_uuid = objects.Node.get_by_id(api.request.context, rpc_allocation.node_id).uuid diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index bcbd23873d..582ed0a6d4 100644 --- a/ironic/api/controllers/v1/node.py +++ b/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: diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 7a77378e70..3c012ff42c 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/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): - """Check if the specified policy authorizes this request on a node. +def check_owner_policy(object_type, policy_name, owner): + """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): - """Check if the specified policy authorizes this request on a node. +def check_allocation_policy_and_retrieve(policy_name, allocation_ident): + """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 diff --git a/ironic/common/policy.py b/ironic/common/policy.py index 51bd3b3dcb..16243f0f1c 100644 --- a/ironic/common/policy.py +++ b/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', diff --git a/ironic/tests/unit/api/controllers/v1/test_allocation.py b/ironic/tests/unit/api/controllers/v1/test_allocation.py index c99ec1ec6d..22910051ad 100644 --- a/ironic/tests/unit/api/controllers/v1/test_allocation.py +++ b/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): diff --git a/ironic/tests/unit/api/controllers/v1/test_expose.py b/ironic/tests/unit/api/controllers/v1/test_expose.py index b68a2b2bfe..0f2323d78d 100644 --- a/ironic/tests/unit/api/controllers/v1/test_expose.py +++ b/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 diff --git a/ironic/tests/unit/api/controllers/v1/test_utils.py b/ironic/tests/unit/api/controllers/v1/test_utils.py index 53ee1207f3..ab7c028273 100644 --- a/ironic/tests/unit/api/controllers/v1/test_utils.py +++ b/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( - 'fake_policy', self.node['owner'] + utils.check_owner_policy( + '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' ) diff --git a/ironic/tests/unit/common/test_policy.py b/ironic/tests/unit/common/test_policy.py index 448d629fe0..4277524b79 100644 --- a/ironic/tests/unit/common/test_policy.py +++ b/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'} diff --git a/releasenotes/notes/allocation-added-owner-policy-c650074e68d03289.yaml b/releasenotes/notes/allocation-added-owner-policy-c650074e68d03289.yaml new file mode 100644 index 0000000000..2dc75d7e1e --- /dev/null +++ b/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.