From f22ab44888e4b9fd549a5da8e907f4b8fa01faed Mon Sep 17 00:00:00 2001 From: Tzu-Mainn Chen Date: Thu, 5 Dec 2019 17:21:45 +0000 Subject: [PATCH] Restrict ability to change owner on provisioned or allocated node Prevents a user from changing the owner of a provisioned node unless they pass the new policy rule 'baremetal:node:update_owner_provisioned'. In addition, always prevents a user from changing the owner of an allocated node, if the allocation specifies an owner. Story: 2006997 Task: 37766 Change-Id: I4e8559bd215f70fb895ed0d41b2154c648e03597 --- ironic/api/controllers/v1/node.py | 30 +++++++ ironic/api/controllers/v1/utils.py | 26 ++++-- ironic/common/policy.py | 5 ++ .../unit/api/controllers/v1/test_node.py | 80 ++++++++++++++++++- .../unit/api/controllers/v1/test_utils.py | 38 +++++++++ ...-owner-provision-fix-ee2348b5922f7648.yaml | 9 +++ 6 files changed, 180 insertions(+), 8 deletions(-) create mode 100644 releasenotes/notes/node-owner-provision-fix-ee2348b5922f7648.yaml diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 3c168a6d10..c1e2abe69d 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -2116,6 +2116,36 @@ class NodesController(rest.RestController): 'state': ir_states.INSPECTING} raise wsme.exc.ClientSideError(msg, status_code=http_client.CONFLICT) + elif api_utils.get_patch_values(patch, '/owner'): + + # check if updating a provisioned node's owner is allowed + if rpc_node.provision_state == ir_states.ACTIVE: + try: + api_utils.check_node_policy( + 'baremetal:node:update_owner_provisioned', + rpc_node['owner']) + except exception.HTTPForbidden: + msg = _('Cannot update owner of node "%(node)s" while it ' + 'is in state "%(state)s".') % { + 'node': rpc_node.uuid, + 'state': ir_states.ACTIVE} + raise wsme.exc.ClientSideError( + msg, status_code=http_client.CONFLICT) + + # check if node has an associated allocation with an owner + if rpc_node.allocation_id: + try: + allocation = objects.Allocation.get_by_id( + context, + rpc_node.allocation_id) + if allocation.owner is not None: + msg = _('Cannot update owner of node "%(node)s" while ' + 'it is allocated to an allocation with an ' + ' owner.') % {'node': rpc_node.uuid} + raise wsme.exc.ClientSideError( + msg, status_code=http_client.CONFLICT) + except exception.AllocationNotFound: + pass names = api_utils.get_patch_values(patch, '/name') if len(names): diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index cb8156db8d..4e191e7e3f 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -1165,7 +1165,23 @@ def check_policy(policy_name): policy.authorize(policy_name, cdict, cdict) -def check_node_policy_and_retrieve(policy_name, node_ident, with_suffix=False): +def check_node_policy(policy_name, node_owner): + """Check if the specified policy authorizes this request on a node. + + :param: policy_name: Name of the policy to check. + :param: node_owner: the node 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 + policy.authorize(policy_name, target_dict, cdict) + + +def check_node_policy_and_retrieve(policy_name, node_ident, + with_suffix=False): """Check if the specified policy authorizes this request on a node. :param: policy_name: Name of the policy to check. @@ -1176,8 +1192,6 @@ def check_node_policy_and_retrieve(policy_name, node_ident, with_suffix=False): :raises: NodeNotFound if the node is not found. :return: RPC node identified by node_ident """ - cdict = api.request.context.to_policy_values() - try: if with_suffix: rpc_node = get_rpc_node_with_suffix(node_ident) @@ -1186,13 +1200,11 @@ def check_node_policy_and_retrieve(policy_name, node_ident, with_suffix=False): except exception.NodeNotFound: # don't expose non-existence of node unless requester # has generic access to policy + cdict = api.request.context.to_policy_values() policy.authorize(policy_name, cdict, cdict) raise - target_dict = dict(cdict) - target_dict['node.owner'] = rpc_node['owner'] - policy.authorize(policy_name, target_dict, cdict) - + check_node_policy(policy_name, rpc_node['owner']) return rpc_node diff --git a/ironic/common/policy.py b/ironic/common/policy.py index 2290cb9175..9e019ccc02 100644 --- a/ironic/common/policy.py +++ b/ironic/common/policy.py @@ -101,6 +101,11 @@ node_policies = [ 'rule:is_admin', 'Update Node records', [{'path': '/nodes/{node_ident}', 'method': 'PATCH'}]), + policy.DocumentedRuleDefault( + 'baremetal:node:update_owner_provisioned', + 'rule:is_admin', + 'Update Node owner even when Node is provisioned', + [{'path': '/nodes/{node_ident}', 'method': 'PATCH'}]), policy.DocumentedRuleDefault( 'baremetal:node:delete', 'rule:is_admin', diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index 4415ca5088..c032a29d34 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -3117,7 +3117,15 @@ class TestPatch(test_api_base.BaseApiTest): self.assertEqual('application/json', response.content_type) self.assertEqual(http_client.OK, response.status_code) - def test_update_owner(self): + @mock.patch.object(policy, 'authorize', spec=True) + def test_update_owner(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + # test should not check this policy rule + if rule == 'baremetal:node:update_owner_provisioned': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + node = obj_utils.create_test_node(self.context, uuid=uuidutils.generate_uuid()) self.mock_update_node.return_value = node @@ -3130,6 +3138,76 @@ class TestPatch(test_api_base.BaseApiTest): self.assertEqual('application/json', response.content_type) self.assertEqual(http_client.OK, response.status_code) + def test_update_owner_provisioned(self): + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid(), + provision_state='active') + self.mock_update_node.return_value = node + headers = {api_base.Version.string: '1.50'} + response = self.patch_json('/nodes/%s' % node.uuid, + [{'path': '/owner', + 'value': 'meow', + 'op': 'replace'}], + headers=headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + + @mock.patch.object(policy, 'authorize', spec=True) + def test_update_owner_provisioned_forbidden(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node:update_owner_provisioned': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid(), + provision_state='active') + self.mock_update_node.return_value = node + headers = {api_base.Version.string: '1.50'} + response = self.patch_json('/nodes/%s' % node.uuid, + [{'path': '/owner', + 'value': 'meow', + 'op': 'replace'}], + headers=headers, + expect_errors=True) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.CONFLICT, response.status_code) + self.assertTrue(response.json['error_message']) + + def test_update_owner_allocation(self): + allocation = obj_utils.create_test_allocation(self.context) + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid(), + allocation_id=allocation.id) + self.mock_update_node.return_value = node + headers = {api_base.Version.string: '1.50'} + response = self.patch_json('/nodes/%s' % node.uuid, + [{'path': '/owner', + 'value': 'meow', + 'op': 'replace'}], + headers=headers) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.OK, response.status_code) + + def test_update_owner_allocation_owned(self): + allocation = obj_utils.create_test_allocation(self.context, + owner='12345') + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid(), + allocation_id=allocation.id) + self.mock_update_node.return_value = node + headers = {api_base.Version.string: '1.50'} + response = self.patch_json('/nodes/%s' % node.uuid, + [{'path': '/owner', + 'value': 'meow', + 'op': 'replace'}], + headers=headers, + expect_errors=True) + self.assertEqual('application/json', response.content_type) + self.assertEqual(http_client.CONFLICT, response.status_code) + self.assertTrue(response.json['error_message']) + def test_update_protected_old_api(self): node = obj_utils.create_test_node(self.context, uuid=uuidutils.generate_uuid()) diff --git a/ironic/tests/unit/api/controllers/v1/test_utils.py b/ironic/tests/unit/api/controllers/v1/test_utils.py index dd45bba7ef..b1029d0aed 100644 --- a/ironic/tests/unit/api/controllers/v1/test_utils.py +++ b/ironic/tests/unit/api/controllers/v1/test_utils.py @@ -789,6 +789,44 @@ class TestPortgroupIdent(base.TestCase): self.invalid_name) +class TestCheckNodePolicy(base.TestCase): + def setUp(self): + super(TestCheckNodePolicy, 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( + 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'] + ) + 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( + self, 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') + + self.assertRaises( + exception.HTTPForbidden, + utils.check_node_policy, + 'fake-policy', + self.node['owner'] + ) + + class TestCheckNodePolicyAndRetrieve(base.TestCase): def setUp(self): super(TestCheckNodePolicyAndRetrieve, self).setUp() diff --git a/releasenotes/notes/node-owner-provision-fix-ee2348b5922f7648.yaml b/releasenotes/notes/node-owner-provision-fix-ee2348b5922f7648.yaml new file mode 100644 index 0000000000..c1c107aff7 --- /dev/null +++ b/releasenotes/notes/node-owner-provision-fix-ee2348b5922f7648.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + Fixes an issue where a provisioned or allocated node could have its owner + changed. For backwards compatibility, we preserve the ability to do so + for a provisioned node through the use of the + ``baremetal:node:update_owner_provisioned`` policy rule. We always prevent + the update if the node is associated with an allocation that specifies an + owner.