From 16a2473b82565c5c2258f3a3b2674fca28bb8070 Mon Sep 17 00:00:00 2001
From: Tzu-Mainn Chen <tzumainn@redhat.com>
Date: Wed, 12 Feb 2020 16:00:11 +0000
Subject: [PATCH] Add separate policies for updating node instance_info and
extra
In order to provision a node using standalone Ironic, a user must
be able to update a few additional node attributes. However, we
would not want a lessee user to be able to update every node
attribute. This change allows an Ironic administrator to provide
policy-based access to updating instance_info and extra.
Change-Id: I43c22027116da1e057972dbe853403c16e965fc9
Story: #2006506
Task: #38748
---
ironic/api/controllers/v1/node.py | 20 ++-
ironic/api/controllers/v1/utils.py | 24 +++
ironic/common/policy.py | 10 ++
.../unit/api/controllers/v1/test_expose.py | 2 +
.../unit/api/controllers/v1/test_node.py | 158 ++++++++++++++++++
.../unit/api/controllers/v1/test_utils.py | 62 +++++++
...-info-extra-policies-862b2a70b941cf39.yaml | 8 +
7 files changed, 282 insertions(+), 2 deletions(-)
create mode 100644 releasenotes/notes/node-update-instance-info-extra-policies-862b2a70b941cf39.yaml
diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py
index 582ed0a6d4..905795bc8b 100644
--- a/ironic/api/controllers/v1/node.py
+++ b/ironic/api/controllers/v1/node.py
@@ -2111,8 +2111,24 @@ class NodesController(rest.RestController):
self._validate_patch(patch, reset_interfaces)
context = api.request.context
- rpc_node = api_utils.check_node_policy_and_retrieve(
- 'baremetal:node:update', node_ident, with_suffix=True)
+
+ # deal with attribute-specific policy rules
+ policy_checks = []
+ generic_update = False
+ for p in patch:
+ if p['path'].startswith('/instance_info'):
+ policy_checks.append('baremetal:node:update_instance_info')
+ elif p['path'].startswith('/extra'):
+ policy_checks.append('baremetal:node:update_extra')
+ else:
+ generic_update = True
+
+ # always do at least one check
+ if generic_update or policy_checks == []:
+ policy_checks.append('baremetal:node:update')
+
+ rpc_node = api_utils.check_multiple_node_policies_and_retrieve(
+ policy_checks, node_ident, with_suffix=True)
remove_inst_uuid_patch = [{'op': 'remove', 'path': '/instance_uuid'}]
if rpc_node.maintenance and patch == remove_inst_uuid_patch:
diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py
index 3c012ff42c..5c3349b0f5 100644
--- a/ironic/api/controllers/v1/utils.py
+++ b/ironic/api/controllers/v1/utils.py
@@ -1235,6 +1235,30 @@ def check_allocation_policy_and_retrieve(policy_name, allocation_ident):
return rpc_allocation
+def check_multiple_node_policies_and_retrieve(policy_names,
+ node_ident,
+ with_suffix=False):
+ """Check if the specified policies authorize this request on a node.
+
+ :param: policy_names: List of policy names to check.
+ :param: node_ident: the UUID or logical name of a node.
+ :param: with_suffix: whether the RPC node should include the suffix
+
+ :raises: HTTPForbidden if the policy forbids access.
+ :raises: NodeNotFound if the node is not found.
+ :return: RPC node identified by node_ident
+ """
+ rpc_node = None
+ for policy_name in policy_names:
+ if rpc_node is None:
+ rpc_node = check_node_policy_and_retrieve(policy_names[0],
+ node_ident,
+ with_suffix)
+ else:
+ check_owner_policy('node', policy_name, rpc_node['owner'])
+ return rpc_node
+
+
def check_list_policy(object_type, owner=None):
"""Check if the list policy authorizes this request on an object.
diff --git a/ironic/common/policy.py b/ironic/common/policy.py
index 16243f0f1c..828fbef739 100644
--- a/ironic/common/policy.py
+++ b/ironic/common/policy.py
@@ -104,6 +104,16 @@ node_policies = [
'rule:is_admin',
'Update Node records',
[{'path': '/nodes/{node_ident}', 'method': 'PATCH'}]),
+ policy.DocumentedRuleDefault(
+ 'baremetal:node:update_extra',
+ 'rule:baremetal:node:update',
+ 'Update Node extra field',
+ [{'path': '/nodes/{node_ident}', 'method': 'PATCH'}]),
+ policy.DocumentedRuleDefault(
+ 'baremetal:node:update_instance_info',
+ 'rule:baremetal:node:update',
+ 'Update Node instance_info field',
+ [{'path': '/nodes/{node_ident}', 'method': 'PATCH'}]),
policy.DocumentedRuleDefault(
'baremetal:node:update_owner_provisioned',
'rule:is_admin',
diff --git a/ironic/tests/unit/api/controllers/v1/test_expose.py b/ironic/tests/unit/api/controllers/v1/test_expose.py
index 0f2323d78d..c2370fc612 100644
--- a/ironic/tests/unit/api/controllers/v1/test_expose.py
+++ b/ironic/tests/unit/api/controllers/v1/test_expose.py
@@ -53,6 +53,8 @@ class TestExposedAPIMethodsCheckPolicy(test_base.TestCase):
self.assertTrue(
('api_utils.check_node_policy_and_retrieve' in src) or
('api_utils.check_list_policy' in src) or
+ ('api_utils.check_multiple_node_policies_and_retrieve' 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_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py
index db571ffa52..429fab1b85 100644
--- a/ironic/tests/unit/api/controllers/v1/test_node.py
+++ b/ironic/tests/unit/api/controllers/v1/test_node.py
@@ -3400,6 +3400,164 @@ class TestPatch(test_api_base.BaseApiTest):
self.assertEqual('application/json', response.content_type)
self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_code)
+ @mock.patch.object(api_utils, 'check_multiple_node_policies_and_retrieve')
+ def test_patch_policy_update(self, mock_cmnpar):
+ node = obj_utils.create_test_node(self.context,
+ uuid=uuidutils.generate_uuid())
+ mock_cmnpar.return_value = node
+ self.mock_update_node.return_value = node
+ headers = {api_base.Version.string: str(api_v1.max_version())}
+ response = self.patch_json('/nodes/%s' % node.uuid,
+ [{'path': '/description',
+ 'value': 'foo',
+ 'op': 'replace'}],
+ headers=headers)
+ self.assertEqual('application/json', response.content_type)
+ self.assertEqual(http_client.OK, response.status_code)
+ mock_cmnpar.assert_called_once_with(
+ ['baremetal:node:update'], node.uuid, with_suffix=True)
+
+ @mock.patch.object(api_utils, 'check_multiple_node_policies_and_retrieve')
+ def test_patch_policy_update_none(self, mock_cmnpar):
+ node = obj_utils.create_test_node(self.context,
+ uuid=uuidutils.generate_uuid())
+ mock_cmnpar.return_value = node
+ self.mock_update_node.return_value = node
+ headers = {api_base.Version.string: str(api_v1.max_version())}
+ response = self.patch_json('/nodes/%s' % node.uuid,
+ [],
+ headers=headers)
+ self.assertEqual('application/json', response.content_type)
+ self.assertEqual(http_client.OK, response.status_code)
+ mock_cmnpar.assert_called_once_with(
+ ['baremetal:node:update'], node.uuid, with_suffix=True)
+
+ @mock.patch.object(api_utils, 'check_multiple_node_policies_and_retrieve')
+ def test_patch_policy_update_extra(self, mock_cmnpar):
+ node = obj_utils.create_test_node(self.context,
+ uuid=uuidutils.generate_uuid())
+ mock_cmnpar.return_value = node
+ self.mock_update_node.return_value = node
+ headers = {api_base.Version.string: str(api_v1.max_version())}
+ response = self.patch_json('/nodes/%s' % node.uuid,
+ [{'path': '/extra/foo',
+ 'value': 'bar',
+ 'op': 'add'}],
+ headers=headers)
+ self.assertEqual('application/json', response.content_type)
+ self.assertEqual(http_client.OK, response.status_code)
+ mock_cmnpar.assert_called_once_with(
+ ['baremetal:node:update_extra'], node.uuid, with_suffix=True)
+
+ @mock.patch.object(api_utils, 'check_multiple_node_policies_and_retrieve')
+ def test_patch_policy_update_instance_info(self, mock_cmnpar):
+ node = obj_utils.create_test_node(self.context,
+ uuid=uuidutils.generate_uuid())
+ mock_cmnpar.return_value = node
+ self.mock_update_node.return_value = node
+ headers = {api_base.Version.string: str(api_v1.max_version())}
+ response = self.patch_json('/nodes/%s' % node.uuid,
+ [{'path': '/instance_info/foo',
+ 'value': 'bar',
+ 'op': 'add'}],
+ headers=headers)
+ self.assertEqual('application/json', response.content_type)
+ self.assertEqual(http_client.OK, response.status_code)
+ mock_cmnpar.assert_called_once_with(
+ ['baremetal:node:update_instance_info'],
+ node.uuid, with_suffix=True)
+
+ @mock.patch.object(api_utils, 'check_multiple_node_policies_and_retrieve')
+ def test_patch_policy_update_generic_and_extra(self, mock_cmnpar):
+ node = obj_utils.create_test_node(self.context,
+ uuid=uuidutils.generate_uuid())
+ mock_cmnpar.return_value = node
+ self.mock_update_node.return_value = node
+ headers = {api_base.Version.string: str(api_v1.max_version())}
+ response = self.patch_json('/nodes/%s' % node.uuid,
+ [{'path': '/description',
+ 'value': 'foo',
+ 'op': 'replace'},
+ {'path': '/extra/foo',
+ 'value': 'bar',
+ 'op': 'add'}],
+ headers=headers)
+ self.assertEqual('application/json', response.content_type)
+ self.assertEqual(http_client.OK, response.status_code)
+ mock_cmnpar.assert_called_once_with(
+ ['baremetal:node:update_extra', 'baremetal:node:update'],
+ node.uuid, with_suffix=True)
+
+ @mock.patch.object(api_utils, 'check_multiple_node_policies_and_retrieve')
+ def test_patch_policy_update_generic_and_instance_info(self, mock_cmnpar):
+ node = obj_utils.create_test_node(self.context,
+ uuid=uuidutils.generate_uuid())
+ mock_cmnpar.return_value = node
+ self.mock_update_node.return_value = node
+ headers = {api_base.Version.string: str(api_v1.max_version())}
+ response = self.patch_json('/nodes/%s' % node.uuid,
+ [{'path': '/description',
+ 'value': 'foo',
+ 'op': 'replace'},
+ {'path': '/instance_info/foo',
+ 'value': 'bar',
+ 'op': 'add'}],
+ headers=headers)
+ self.assertEqual('application/json', response.content_type)
+ self.assertEqual(http_client.OK, response.status_code)
+ mock_cmnpar.assert_called_once_with(
+ ['baremetal:node:update_instance_info', 'baremetal:node:update'],
+ node.uuid, with_suffix=True)
+
+ @mock.patch.object(api_utils, 'check_multiple_node_policies_and_retrieve')
+ def test_patch_policy_update_extra_and_instance_info(self, mock_cmnpar):
+ node = obj_utils.create_test_node(self.context,
+ uuid=uuidutils.generate_uuid())
+ mock_cmnpar.return_value = node
+ self.mock_update_node.return_value = node
+ headers = {api_base.Version.string: str(api_v1.max_version())}
+ response = self.patch_json('/nodes/%s' % node.uuid,
+ [{'path': '/extra/foo',
+ 'value': 'bar',
+ 'op': 'add'},
+ {'path': '/instance_info/foo',
+ 'value': 'bar',
+ 'op': 'add'}],
+ headers=headers)
+ self.assertEqual('application/json', response.content_type)
+ self.assertEqual(http_client.OK, response.status_code)
+ mock_cmnpar.assert_called_once_with(
+ ['baremetal:node:update_extra',
+ 'baremetal:node:update_instance_info'],
+ node.uuid, with_suffix=True)
+
+ @mock.patch.object(api_utils, 'check_multiple_node_policies_and_retrieve')
+ def test_patch_policy_update_generic_extra_instance_info(
+ self, mock_cmnpar):
+ node = obj_utils.create_test_node(self.context,
+ uuid=uuidutils.generate_uuid())
+ mock_cmnpar.return_value = node
+ self.mock_update_node.return_value = node
+ headers = {api_base.Version.string: str(api_v1.max_version())}
+ response = self.patch_json('/nodes/%s' % node.uuid,
+ [{'path': '/description',
+ 'value': 'foo',
+ 'op': 'replace'},
+ {'path': '/extra/foo',
+ 'value': 'bar',
+ 'op': 'add'},
+ {'path': '/instance_info/foo',
+ 'value': 'bar',
+ 'op': 'add'}],
+ headers=headers)
+ self.assertEqual('application/json', response.content_type)
+ self.assertEqual(http_client.OK, response.status_code)
+ mock_cmnpar.assert_called_once_with(
+ ['baremetal:node:update_extra',
+ 'baremetal:node:update_instance_info',
+ 'baremetal:node:update'],
+ node.uuid, with_suffix=True)
+
def _create_node_locally(node):
driver_factory.check_and_update_node_interfaces(node)
diff --git a/ironic/tests/unit/api/controllers/v1/test_utils.py b/ironic/tests/unit/api/controllers/v1/test_utils.py
index ab7c028273..077ddc3023 100644
--- a/ironic/tests/unit/api/controllers/v1/test_utils.py
+++ b/ironic/tests/unit/api/controllers/v1/test_utils.py
@@ -1016,6 +1016,68 @@ class TestCheckAllocationPolicyAndRetrieve(base.TestCase):
)
+class TestCheckMultipleNodePoliciesAndRetrieve(base.TestCase):
+ def setUp(self):
+ super(TestCheckMultipleNodePoliciesAndRetrieve, 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(utils, 'check_node_policy_and_retrieve')
+ @mock.patch.object(utils, 'check_owner_policy')
+ def test_check_multiple_node_policies_and_retrieve(
+ self, mock_cop, mock_cnpar
+ ):
+ mock_cnpar.return_value = self.node
+ mock_cop.return_value = True
+
+ rpc_node = utils.check_multiple_node_policies_and_retrieve(
+ ['fake_policy_1', 'fake_policy_2'], self.valid_node_uuid
+ )
+ mock_cnpar.assert_called_once_with('fake_policy_1',
+ self.valid_node_uuid, False)
+ mock_cop.assert_called_once_with(
+ 'node', 'fake_policy_2', '12345')
+ self.assertEqual(self.node, rpc_node)
+
+ @mock.patch.object(utils, 'check_node_policy_and_retrieve')
+ @mock.patch.object(utils, 'check_owner_policy')
+ def test_check_multiple_node_policies_and_retrieve_first_fail(
+ self, mock_cop, mock_cnpar
+ ):
+ mock_cnpar.side_effect = exception.HTTPForbidden(resource='fake')
+ mock_cop.return_value = True
+
+ self.assertRaises(
+ exception.HTTPForbidden,
+ utils.check_multiple_node_policies_and_retrieve,
+ ['fake_policy_1', 'fake_policy_2'],
+ self.valid_node_uuid
+ )
+ mock_cnpar.assert_called_once_with('fake_policy_1',
+ self.valid_node_uuid, False)
+ mock_cop.assert_not_called()
+
+ @mock.patch.object(utils, 'check_node_policy_and_retrieve')
+ @mock.patch.object(utils, 'check_owner_policy')
+ def test_check_node_policy_and_retrieve_no_node(
+ self, mock_cop, mock_cnpar
+ ):
+ mock_cnpar.return_value = self.node
+ mock_cop.side_effect = exception.HTTPForbidden(resource='fake')
+
+ self.assertRaises(
+ exception.HTTPForbidden,
+ utils.check_multiple_node_policies_and_retrieve,
+ ['fake_policy_1', 'fake_policy_2'],
+ self.valid_node_uuid
+ )
+ mock_cnpar.assert_called_once_with('fake_policy_1',
+ self.valid_node_uuid, False)
+ mock_cop.assert_called_once_with(
+ 'node', 'fake_policy_2', '12345')
+
+
class TestCheckListPolicy(base.TestCase):
@mock.patch.object(api, 'request', spec_set=["context", "version"])
@mock.patch.object(policy, 'authorize', spec=True)
diff --git a/releasenotes/notes/node-update-instance-info-extra-policies-862b2a70b941cf39.yaml b/releasenotes/notes/node-update-instance-info-extra-policies-862b2a70b941cf39.yaml
new file mode 100644
index 0000000000..ce98552398
--- /dev/null
+++ b/releasenotes/notes/node-update-instance-info-extra-policies-862b2a70b941cf39.yaml
@@ -0,0 +1,8 @@
+---
+features:
+ - |
+ Adds ``baremetal:node:update_extra`` and ``baremetal:node:instance_info``
+ policies to allow finer-grained policy control over node updates. In order
+ to use standalone Ironic to provision a node, a user must be able to
+ update these attributes, and a lessee should not be able to update all
+ node attributes.