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 6f4814788c..89ae38a5a3 100644
--- a/ironic/api/controllers/v1/node.py
+++ b/ironic/api/controllers/v1/node.py
@@ -1833,7 +1833,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)
@@ -1910,7 +1910,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)
@@ -2136,7 +2136,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 28b6c9174e..df037892e4 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 68e8a7f475..f25b9cb03c 100644
--- a/ironic/tests/unit/api/controllers/v1/test_utils.py
+++ b/ironic/tests/unit/api/controllers/v1/test_utils.py
@@ -791,30 +791,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
@@ -823,7 +823,8 @@ class TestCheckNodePolicy(base.TestCase):
self.assertRaises(
exception.HTTPForbidden,
- utils.check_node_policy,
+ utils.check_owner_policy,
+ 'node',
'fake-policy',
self.node['owner']
)
@@ -930,10 +931,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 = {
@@ -941,12 +1021,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 = {
@@ -954,12 +1034,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):
@@ -972,12 +1052,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):
@@ -990,12 +1071,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):
@@ -1008,12 +1090,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):
@@ -1028,7 +1110,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.