Merge "Follow-up to RBAC allocation changes"

doc/source/admin/secure-rbac.rst

@@ -177,7 +177,7 @@ Allocations
 The ``allocations`` endpoint of the API is somewhat different than other
 other endpoints as it allows for the allocation of physical machines to
 an admin. In this context, there is not already an ``owner`` or ``project_id``
-to leverage to control access for the creation process, any project admin
+to leverage to control access for the creation process, any project member
 does have the inherent prilege of requesting an allocation. That being said,
 their allocation request will require physical nodes to be owned or leased
 to the ``project_id`` through the ``node`` fields ``owner`` or ``lessee``.
@@ -187,6 +187,13 @@ and any new allocation being requested with a specific owner, if made in
 ``project`` scope, will have the ``project_id`` recorded as the owner of
 the allocation.
 
+Ultimately, an operational behavior difference exists between the ``owner``
+and ``lessee`` rights in terms of allocations exists. With the standard
+access rights, ``lessee`` users are able to create allocations if they
+own nodes which are not allocated or deployed, but they cannot reprovision
+nodes when using only a ``member`` role. This limitation is not the case
+for project-scoped users with the ``admin`` role.
+
 .. WARNING:: The allocation endpoint's use is restricted to project scoped
    interactions until ``[oslo_policy]enforce_new_defaults`` has been set
    to ``True`` using the ``baremetal:allocation:create_pre_rbac`` policy

ironic/api/controllers/v1/allocation.py

@@ -161,7 +161,6 @@ class AllocationsController(pecan.rest.RestController):
                 _("The sort_key value %(key)s is an invalid field for "
                   "sorting") % {'key': sort_key})
 
-        node_uuid = None
         # If the user is not allowed to see everything, we need to filter
         # based upon access rights.
         cdict = api.request.context.to_policy_values()
@@ -175,9 +174,6 @@ class AllocationsController(pecan.rest.RestController):
                 # be able to see in the database.
                 owner = cdict.get('project_id')
         else:
-            # The controller is being accessed as a sub-resource.
-            # Cool, but that means there can only be one result.
-            node_uuid = parent_node
             # Override if any node_ident was submitted in since this
             # is a subresource query.
             node_ident = parent_node
@@ -313,7 +309,7 @@ class AllocationsController(pecan.rest.RestController):
             api_utils.check_policy('baremetal:allocation:create')
             self._check_allowed_allocation_fields(allocation)
             if (not CONF.oslo_policy.enforce_new_defaults
-                and not allocation.get('owner')):
+                    and not allocation.get('owner')):
                 # Even if permitted, we need to go ahead and check if this is
                 # restricted for now until scoped interaction is the default
                 # interaction.
@@ -327,8 +323,7 @@ class AllocationsController(pecan.rest.RestController):
         except exception.HTTPForbidden:
             cdict = api.request.context.to_policy_values()
             project = cdict.get('project_id')
-            if (project and allocation.get('owner')
+            if project and project != allocation.get('owner'):
-                and project != allocation.get('owner')):
                 raise
             if project and not CONF.oslo_policy.enforce_new_defaults:
                 api_utils.check_policy('baremetal:allocation:create_pre_rbac')
@@ -363,7 +358,7 @@ class AllocationsController(pecan.rest.RestController):
         # not great to try for a full method rewrite at the same time as
         # RBAC work, so the complexity limit is being raised. :(
         if (CONF.oslo_policy.enforce_new_defaults
-            and cdict.get('system_scope') != 'all'):
+                and cdict.get('system_scope') != 'all'):
             # if not a system scope originated request, we need to check/apply
             # an owner - But we can only do this with when new defaults are
             # enabled.
@@ -387,7 +382,6 @@ class AllocationsController(pecan.rest.RestController):
             else:
                 # An allocation owner was not supplied, we need to save one.
                 allocation['owner'] = project_id
-
         node = None
         if allocation.get('node'):
             if api_utils.allow_allocation_backfill():

ironic/common/policy.py

@@ -124,10 +124,6 @@ SYSTEM_MEMBER_OR_OWNER_LESSEE_ADMIN = (
 # The system has rights to manage the allocations for users, in a sense
 # a delegated management since they are not creating a real object or asset,
 # but allocations of assets.
-ALLOCATION_ADMIN = (
-    '(' + SYSTEM_MEMBER + ') or (' + ALLOCATION_OWNER_ADMIN + ')'
-)
-
 ALLOCATION_MEMBER = (
     '(' + SYSTEM_MEMBER + ') or (' + ALLOCATION_OWNER_MEMBER + ')'
 )
@@ -137,7 +133,7 @@ ALLOCATION_READER = (
 )
 
 ALLOCATION_CREATOR = (
-    '(' + SYSTEM_MEMBER + ') or (role:admin)'
+    '(' + SYSTEM_MEMBER + ') or (role:member)'
 )
 
 # Special purpose aliases for things like "ability to access the API
@@ -1533,7 +1529,7 @@ deprecated_allocation_list_all = policy.DeprecatedRule(
 )
 deprecated_allocation_create = policy.DeprecatedRule(
     name='baremetal:allocation:create',
-    check_str='rule:is_admin'
+    check_str='rule:is_admin and is_admin_project:True'
 )
 deprecated_allocation_create_restricted = policy.DeprecatedRule(
     name='baremetal:allocation:create_restricted',
@@ -1610,7 +1606,7 @@ allocation_policies = [
     ),
     policy.DocumentedRuleDefault(
         name='baremetal:allocation:delete',
-        check_str=ALLOCATION_ADMIN,
+        check_str=ALLOCATION_MEMBER,
         scope_types=['system', 'project'],
         description='Delete Allocation records',
         operations=[
@@ -1634,8 +1630,12 @@ allocation_policies = [
     ),
     policy.DocumentedRuleDefault(
         name='baremetal:allocation:create_pre_rbac',
-        check_str=('rule:is_member and role:baremetal_admin or '
+        # NOTE(TheJulia): First part of the check string is for classical
-                   'is_admin_project:True and role:admin'),
+        # administrative rights with someone using a baremetal project.
+        # The latter is more for projects and services using admin project
+        # rights. Specific checking because of the expanded rights of
+        # this functionality.
+        check_str=('(rule:is_member and role:baremetal_admin) or (is_admin_project:True and role:admin)'),  # noqa 
         scope_types=['project'],
         description=('Logical restrictor to prevent legacy allocation rule '
                      'missuse - Requires blank allocations to originate from '

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

@@ -393,7 +393,7 @@ class TestListAllocations(test_api_base.BaseApiTest):
                 owner=owner,
                 uuid=uuidutils.generate_uuid(),
                 name='allocation%s' % i)
-        # NOTE(TheJulia): Force the cast of the action to a system scoped
+        # NOTE(TheJulia): Force the cast of the action to a system
         # scoped request. System scoped is allowed to view everything,
         # where as project scoped requests are actually filtered with the
         # secure-rbac work. This was done in troubleshooting the code,
@@ -1127,17 +1127,14 @@ class TestPost(test_api_base.BaseApiTest):
         self.assertEqual('application/json', response.content_type)
         self.assertTrue(response.json['error_message'])
 
-    @mock.patch.object(policy, 'authorize', autospec=True)
+    def test_create_restricted_allocation_normal(self):
-    def test_create_restricted_allocation(self, mock_authorize):
+        cfg.CONF.set_override('enforce_new_defaults', True,
-        def mock_authorize_function(rule, target, creds):
+                              group='oslo_policy')
-            if rule == 'baremetal:allocation:create':
-                raise exception.HTTPForbidden(resource='fake')
-            return True
-        mock_authorize.side_effect = mock_authorize_function
-
         owner = '12345'
         adict = apiutils.allocation_post_data()
-        headers = {api_base.Version.string: '1.60', 'X-Project-Id': owner}
+        headers = {api_base.Version.string: '1.60',
+                   'X-Roles': 'member,reader',
+                   'X-Project-Id': owner}
         response = self.post_json('/allocations', adict, headers=headers)
         self.assertEqual('application/json', response.content_type)
         self.assertEqual(http_client.CREATED, response.status_int)
@@ -1147,13 +1144,7 @@ class TestPost(test_api_base.BaseApiTest):
         self.assertEqual(adict['uuid'], result['uuid'])
         self.assertEqual(owner, result['owner'])
 
-    @mock.patch.object(policy, 'authorize', autospec=True)
+    def test_create_restricted_allocation_older_version(self):
-    def test_create_restricted_allocation_older_version(self, mock_authorize):
-        def mock_authorize_function(rule, target, creds):
-            if rule == 'baremetal:allocation:create':
-                raise exception.HTTPForbidden(resource='fake')
-            return True
-        mock_authorize.side_effect = mock_authorize_function
         owner = '12345'
         adict = apiutils.allocation_post_data()
         del adict['owner']

ironic/tests/unit/api/test_rbac_legacy.yaml

@@ -1897,6 +1897,7 @@ allocations_post_member:
   body: *allocation_body
   assert_status: 403
   deprecated: true
+  skip_reason: This endpoint's behavior supports allocation creation as a member with the new Role Based Access Control changes. Thus this test cannot both ensure prior and post-change behavior as it is actually valid moving forward.
 
 allocations_post_observer:
   path: '/v1/allocations'

ironic/tests/unit/api/test_rbac_project_scoped.yaml

@@ -2316,17 +2316,19 @@ lessee_admin_can_delete_their_allocation:
   headers: *lessee_admin_headers
   assert_status: 503
 
-owner_member_cannot_delete_their_allocation:
+owner_member_can_delete_their_allocation:
   path: '/v1/allocations/{owner_allocation}'
   method: delete
   headers: *owner_member_headers
-  assert_status: 403
+  assert_status: 503
 
-lessee_member_cannot_delete_their_allocation:
+# Lessee in this case owns the allocation,
+# Confusing right?!
+lessee_member_can_delete_their_allocation:
   path: '/v1/allocations/{lessee_allocation}'
   method: delete
   headers: *lessee_member_headers
-  assert_status: 403
+  assert_status: 503
 
 owner_member_can_patch_allocation:
   path: '/v1/allocations/{owner_allocation}'