Merge "Restrict ability to change owner on provisioned or allocated node"

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):

ironic/api/controllers/v1/utils.py

@@ -1164,7 +1164,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.
@@ -1175,8 +1191,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)
@@ -1185,13 +1199,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)
+    check_node_policy(policy_name, rpc_node['owner'])
-    target_dict['node.owner'] = rpc_node['owner']
-    policy.authorize(policy_name, target_dict, cdict)
-
     return rpc_node
 
 

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',

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

@@ -3116,7 +3116,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
@@ -3129,6 +3137,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())

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

@@ -790,6 +790,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()

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.