Fix policy checks added with runbooks

In the runbooks change, I43555ef72cb882adcada2ed875fda40eed0dd034,
new policies were added for a user sending a list of service steps
or clean steps to the API.

This was done with the generic check_policy helper, however the helper
does not understand how to populate the ``node`` mapping data to enable
RBAC rule value matching. Doing so requires a special node policy
checker method.

As such, the policy checker was changed, and additional tests were added.

One final note, strucutrally the new policies were being checked *after*
we stated to do state verification of the request. RBAC checks should be
performed upfront... which also eases the burden of testing the RBAC
model. Accordingly, the policy checks were moved together
in the provision state logic.

Closes-Bug: 2086823
Change-Id: I18c56cb4becf9e6181689ddc0f1c7433327a3aa6

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.