Merge "Fix policy checks added with runbooks"

ironic/api/controllers/v1/node.py

@@ -1206,11 +1206,32 @@ class NodeStatesController(rest.RestController):
 
         api_utils.check_allow_management_verbs(target)
 
+        # Secondary RBAC policy checking...
+        # done first because RBAC policies must be asserted in order
+        # of visibility to can I make a change upfront, before diving
+        # into if the change is valid to make.
+        if not runbook:
+            if clean_steps:
+                api_utils.check_owner_policy(
+                    'node',
+                    'baremetal:node:set_provision_state:clean_steps',
+                    rpc_node['owner'], rpc_node['lessee'],
+                    conceal_node=False)
+            if service_steps:
+                api_utils.check_owner_policy(
+                    'node',
+                    'baremetal:node:set_provision_state:service_steps',
+                    rpc_node['owner'], rpc_node['lessee'],
+                    conceal_node=False)
+
         if (target in (ir_states.ACTIVE, ir_states.REBUILD)
                 and rpc_node.maintenance):
             raise exception.NodeInMaintenance(op=_('provisioning'),
                                               node=rpc_node.uuid)
 
+        # This code does upfront state sanity checking, in that we're past
+        # basic RBAC policy checking, and we're shifting gears to content
+        # validation.
         m = ir_states.machine.copy()
         m.initialize(rpc_node.provision_state)
         if not m.is_actionable_event(ir_states.VERBS.get(target, target)):
@@ -1236,13 +1257,6 @@ class NodeStatesController(rest.RestController):
             clean_steps, service_steps, disable_ramdisk = self._handle_runbook(
                 rpc_node, target, runbook, clean_steps, service_steps
             )
-        else:
-            if clean_steps:
-                api_utils.check_policy(
-                    'baremetal:node:set_provision_state:clean_steps')
-            if service_steps:
-                api_utils.check_policy(
-                    'baremetal:node:set_provision_state:service_steps')
 
         if clean_steps and target != ir_states.VERBS['clean']:
             msg = (_('"clean_steps" is only valid when setting target '

ironic/tests/unit/api/test_rbac_project_scoped.yaml

@@ -1369,6 +1369,80 @@ service_cannot_change_provision_state:
   body: *provision_body
   assert_status: 404
 
+# The following tests fail on lock checking, which is fine,
+# as it demonstrates we're past API RBAC policy checking...
+# And these checks center around secondary policies, so the
+# what we send is more important.
+owner_member_can_set_provision_state_clean:
+  path: '/v1/nodes/{owner_node_ident}/states/provision'
+  method: put
+  headers: *owner_member_headers
+  body: &provision_body_clean_steps
+    target: clean
+    clean_steps:
+      - interface: deploy
+        step: update_firmware
+        args:
+          foo: bar
+        priority: 99
+  assert_status: 409
+
+owner_reader_cannot_set_provision_state_clean:
+  path: '/v1/nodes/{owner_node_ident}/states/provision'
+  method: put
+  headers: *owner_reader_headers
+  body: *provision_body_clean_steps
+  assert_status: 403
+
+lessee_admin_can_set_provision_state_clean:
+  path: '/v1/nodes/{lessee_node_ident}/states/provision'
+  method: put
+  headers: *lessee_admin_headers
+  body: *provision_body_clean_steps
+  assert_status: 409
+
+lessee_member_cannot_set_provision_state_clean:
+  path: '/v1/nodes/{lessee_node_ident}/states/provision'
+  method: put
+  headers: *lessee_member_headers
+  body: *provision_body_clean_steps
+  assert_status: 403
+
+owner_member_can_set_provision_state_service:
+  path: '/v1/nodes/{owner_node_ident}/states/provision'
+  method: put
+  headers: *owner_member_headers
+  body: &provision_body_service_steps
+    target: service
+    clean_steps:
+      - interface: deploy
+        step: update_firmware
+        args:
+          foo: bar
+        priority: 99
+  assert_status: 409
+
+owner_reader_cannot_set_provision_state_service:
+  path: '/v1/nodes/{owner_node_ident}/states/provision'
+  method: put
+  headers: *owner_reader_headers
+  body: *provision_body_service_steps
+  assert_status: 403
+
+lessee_admin_can_set_provision_state_service:
+  path: '/v1/nodes/{lessee_node_ident}/states/provision'
+  method: put
+  headers: *lessee_admin_headers
+  body: *provision_body_service_steps
+  assert_status: 409
+
+lessee_member_cannot_set_provision_state_service:
+  path: '/v1/nodes/{lessee_node_ident}/states/provision'
+  method: put
+  headers: *lessee_member_headers
+  body: *provision_body_service_steps
+  assert_status: 403
+
 # Raid configuration
 
 owner_admin_can_set_raid_config:

releasenotes/notes/fix-set-provision-state-subpolicy-13ae3ef7497d20c1.yaml

@@ -0,0 +1,12 @@
+---
+fixes:
+  - |
+    Fixes newly added policy rules,
+    ``baremetal:node:set_provision_state:clean_steps`` and
+    ``baremetal:node:set_provision_state:service_steps``which impacted
+    ``project scoped`` users utilizing the ``2024.2`` release of Ironic
+    where they were attempting to invoke ``service`` or ``clean``
+    provision state commands.
+    This was due to a misunderstanding of the correct policy checker to
+    invoke, and additional testing has been added around these functions
+    to ensure they work as expected moving forward.