Merge "Add separate policies for updating node instance_info and extra"

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:

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.
 

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

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

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)

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)

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.