diff --git a/doc/source/admin/secure-rbac.rst b/doc/source/admin/secure-rbac.rst index e735bfad6d..65fef8d656 100644 --- a/doc/source/admin/secure-rbac.rst +++ b/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 diff --git a/ironic/api/controllers/v1/allocation.py b/ironic/api/controllers/v1/allocation.py index 845a5dec2b..73aca3a51e 100644 --- a/ironic/api/controllers/v1/allocation.py +++ b/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') - and project != allocation.get('owner')): + if project 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(): diff --git a/ironic/common/policy.py b/ironic/common/policy.py index 364de02633..1f5d9050c8 100644 --- a/ironic/common/policy.py +++ b/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 ' - 'is_admin_project:True and role:admin'), + # NOTE(TheJulia): First part of the check string is for classical + # 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 ' diff --git a/ironic/tests/unit/api/controllers/v1/test_allocation.py b/ironic/tests/unit/api/controllers/v1/test_allocation.py index e6cca71ddc..39a12784e8 100644 --- a/ironic/tests/unit/api/controllers/v1/test_allocation.py +++ b/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(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 - + def test_create_restricted_allocation_normal(self): + cfg.CONF.set_override('enforce_new_defaults', True, + group='oslo_policy') 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, 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 + def test_create_restricted_allocation_older_version(self): owner = '12345' adict = apiutils.allocation_post_data() del adict['owner'] diff --git a/ironic/tests/unit/api/test_rbac_legacy.yaml b/ironic/tests/unit/api/test_rbac_legacy.yaml index 5b77a98490..2f981e68fc 100644 --- a/ironic/tests/unit/api/test_rbac_legacy.yaml +++ b/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' diff --git a/ironic/tests/unit/api/test_rbac_project_scoped.yaml b/ironic/tests/unit/api/test_rbac_project_scoped.yaml index 76a2463894..6ec74d2ddc 100644 --- a/ironic/tests/unit/api/test_rbac_project_scoped.yaml +++ b/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}'