Allocation support for project scoped RBAC

Adds policy scope based RBAC handling for the allocations
endpoing which enables admins to create allocations if
they have baremetal nodes which are available to them.

Change-Id: I60e273afaf344fded9bdb8c4c8e143efc9971fc1

doc/source/admin/secure-rbac.rst

@@ -68,10 +68,12 @@ Supported Endpoints
 * /nodes/<uuid>/portgroups
 * /nodes/<uuid>/volume/connectors
 * /nodes/<uuid>/volume/targets
+* /nodes/<uuid>/allocation
 * /ports
 * /portgroups
 * /volume/connectors
 * /volume/targets
+* /allocations
 
 How Project Scoped Works
 ------------------------
@@ -147,6 +149,31 @@ change the owner.
 
 More information is available on these fields in :doc:`/configuration/policy`.
 
+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
+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``.
+
+Ability to override the owner is restricted to system scoped users by default
+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.
+
+.. 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
+   rule. This is in order to prevent endpoint misuse. Afterwards all
+   project scoped allocations will automatically populate an owner.
+   System scoped request are not subjected to this restriction,
+   and operators may change the default restriction via the
+   ``baremetal:allocation:create_restricted`` policy.
+
 Pratical differences
 --------------------
 

ironic/api/controllers/v1/allocation.py

@@ -13,6 +13,7 @@
 from http import client as http_client
 
 from ironic_lib import metrics_utils
+from oslo_config import cfg
 from oslo_utils import uuidutils
 import pecan
 from webob import exc as webob_exc
@@ -28,8 +29,10 @@ from ironic.common import exception
 from ironic.common.i18n import _
 from ironic import objects
 
-METRICS = metrics_utils.get_metrics_logger(__name__)
 
+CONF = cfg.CONF
+
+METRICS = metrics_utils.get_metrics_logger(__name__)
 
 ALLOCATION_SCHEMA = {
     'type': 'object',
@@ -133,7 +136,8 @@ class AllocationsController(pecan.rest.RestController):
     def _get_allocations_collection(self, node_ident=None, resource_class=None,
                                     state=None, owner=None, marker=None,
                                     limit=None, sort_key='id', sort_dir='asc',
-                                    resource_url=None, fields=None):
+                                    resource_url=None, fields=None,
+                                    parent_node=None):
         """Return allocations collection.
 
         :param node_ident: UUID or name of a node.
@@ -145,6 +149,9 @@ class AllocationsController(pecan.rest.RestController):
         :param fields: Optional, a list with a specified set of fields
                        of the resource to be returned.
         :param owner: project_id of owner to filter by
+        :param parent_node: The explicit parent node uuid to return if
+                            the controller is being accessed as a
+                            sub-resource. i.e. /v1/nodes/<uuid>/allocation
         """
         limit = api_utils.validate_limit(limit)
         sort_dir = api_utils.validate_sort_dir(sort_dir)
@@ -154,17 +161,48 @@ 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()
+        if cdict.get('system_scope') != 'all' and not parent_node:
+            # The user is a project scoped, and there is not an explicit
+            # parent node which will be returned.
+            if not api_utils.check_policy_true(
+                    'baremetal:allocation:list_all'):
+                # If the user cannot see everything via the policy,
+                # we need to filter the view down to only what they should
+                # 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
+
         marker_obj = None
         if marker:
             marker_obj = objects.Allocation.get_by_uuid(api.request.context,
                                                         marker)
-
         if node_ident:
             try:
-                node_uuid = api_utils.get_rpc_node(node_ident).uuid
+                # Check ability to access the associated node or requested
+                # node to filter by.
+                rpc_node = api_utils.get_rpc_node(node_ident)
+                api_utils.check_owner_policy('node', 'baremetal:node:get',
+                                             rpc_node.owner,
+                                             lessee=rpc_node.lessee,
+                                             conceal_node=False)
+                node_uuid = rpc_node.uuid
             except exception.NodeNotFound as exc:
                 exc.code = http_client.BAD_REQUEST
                 raise
+            except exception.NotAuthorized as exc:
+                if not parent_node:
+                    exc.code = http_client.BAD_REQUEST
+                raise exception.NotFound()
         else:
             node_uuid = None
 
@@ -179,13 +217,16 @@ class AllocationsController(pecan.rest.RestController):
         for key, value in possible_filters.items():
             if value is not None:
                 filters[key] = value
-
         allocations = objects.Allocation.list(api.request.context,
                                               limit=limit,
                                               marker=marker_obj,
                                               sort_key=sort_key,
                                               sort_dir=sort_dir,
                                               filters=filters)
+        for allocation in allocations:
+            api_utils.check_owner_policy('allocation',
+                                         'baremetal:allocation:get',
+                                         allocation.owner)
         return list_convert_with_links(allocations, limit,
                                        url=resource_url,
                                        fields=fields,
@@ -267,18 +308,33 @@ class AllocationsController(pecan.rest.RestController):
     def _authorize_create_allocation(self, allocation):
 
         try:
+            # PRE-RBAC this rule was logically restricted, it is more-unlocked
+            # post RBAC, but we need to ensure it is not abused.
             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')):
+                # Even if permitted, we need to go ahead and check if this is
+                # restricted for now until scoped interaction is the default
+                # interaction.
+                api_utils.check_policy('baremetal:allocation:create_pre_rbac')
+                # TODO(TheJulia): This can be removed later once we
+                # move entirely to scope based checking. This requires
+                # that if the scope enforcement is not enabled, that
+                # any user can't create an allocation until the deployment
+                # is in a new operating mode *where* owner will be added
+                # automatically if not a privilged user.
         except exception.HTTPForbidden:
             cdict = api.request.context.to_policy_values()
-            owner = cdict.get('project_id')
+            project = cdict.get('project_id')
-            if not owner or (allocation.get('owner')
+            if (project and allocation.get('owner')
-                             and owner != 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')
             api_utils.check_policy('baremetal:allocation:create_restricted')
             self._check_allowed_allocation_fields(allocation)
-            allocation['owner'] = owner
+            allocation['owner'] = project
-
         return allocation
 
     @METRICS.timer('AllocationsController.post')
@@ -291,6 +347,7 @@ class AllocationsController(pecan.rest.RestController):
         :param allocation: an allocation within the request body.
         """
         context = api.request.context
+        cdict = context.to_policy_values()
         allocation = self._authorize_create_allocation(allocation)
 
         if (allocation.get('name')
@@ -299,11 +356,47 @@ class AllocationsController(pecan.rest.RestController):
                     "'%(name)s'") % {'name': allocation['name']}
             raise exception.Invalid(msg)
 
+        # TODO(TheJulia): We need to likely look at refactoring post
+        # processing for allocations as pep8 says it is a complexity of 19,
+        # although it is not actually that horrible since it is phased out
+        # just modifying/assembling the allocation. Given that, it seems
+        # 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'):
+            # 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.
+            project_id = cdict.get('project_id')
+            req_alloc_owner = allocation.get('owner')
+            if req_alloc_owner:
+                if not api_utils.check_policy_true(
+                        'baremetal:allocation:create_restricted'):
+                    if req_alloc_owner != project_id:
+                        msg = _("Cannot create allocation with an owner "
+                                "Project ID value %(req_owner)s not matching "
+                                "the requestor Project ID %(project)s. "
+                                "Policy baremetal:allocation:create_restricted"
+                                " is required for this capability."
+                                ) % {'req_owner': req_alloc_owner,
+                                     'project': project_id}
+                        raise exception.NotAuthorized(msg)
+                # NOTE(TheJulia): IF not restricted, i.e. else above,
+                # their supplied allocation owner is okay, they are allowed
+                # to provide an override by policy.
+            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():
                 try:
                     node = api_utils.get_rpc_node(allocation['node'])
+                    api_utils.check_owner_policy(
+                        'node', 'baremetal:node:get',
+                        node.owner, node.lessee,
+                        conceal_node=allocation['node'])
                 except exception.NodeNotFound as exc:
                     exc.code = http_client.BAD_REQUEST
                     raise
@@ -323,8 +416,17 @@ class AllocationsController(pecan.rest.RestController):
         if allocation.get('candidate_nodes'):
             # Convert nodes from names to UUIDs and check their validity
             try:
+                owner = None
+                if not api_utils.check_policy_true(
+                        'baremetal:allocation:create_restricted'):
+                    owner = cdict.get('project_id')
+                # Filter the candidate search by the requestor project ID
+                # if any. The result is processes authenticating with system
+                # scope will not be impacted, where as project scoped requests
+                # will need additional authorization.
                 converted = api.request.dbapi.check_node_list(
-                    allocation['candidate_nodes'])
+                    allocation['candidate_nodes'],
+                    project=owner)
             except exception.NodeNotFound as exc:
                 exc.code = http_client.BAD_REQUEST
                 raise
@@ -458,10 +560,12 @@ class NodeAllocationController(pecan.rest.RestController):
     @method.expose()
     @args.validate(fields=args.string_list)
     def get_all(self, fields=None):
-        api_utils.check_policy('baremetal:allocation:get')
+        parent_node = self.parent_node_ident
+        result = self.inner._get_allocations_collection(
+            parent_node,
+            fields=fields,
+            parent_node=parent_node)
 
-        result = self.inner._get_allocations_collection(self.parent_node_ident,
-                                                        fields=fields)
         try:
             return result['allocations'][0]
         except IndexError:
@@ -473,15 +577,26 @@ class NodeAllocationController(pecan.rest.RestController):
     @method.expose(status_code=http_client.NO_CONTENT)
     def delete(self):
         context = api.request.context
-        api_utils.check_policy('baremetal:allocation:delete')
 
         rpc_node = api_utils.get_rpc_node_with_suffix(self.parent_node_ident)
+        # Check the policy, and 404 if not authorized.
+        api_utils.check_owner_policy('node', 'baremetal:node:get',
+                                     rpc_node.owner, lessee=rpc_node.lessee,
+                                     conceal_node=self.parent_node_ident)
+
+        # A project ID is associated, thus we should filter
+        # our search using it.
+        filters = {'node_uuid': rpc_node.uuid}
         allocations = objects.Allocation.list(
             api.request.context,
-            filters={'node_uuid': rpc_node.uuid})
+            filters=filters)
 
         try:
             rpc_allocation = allocations[0]
+            allocation_owner = allocations[0]['owner']
+            api_utils.check_owner_policy('allocation',
+                                         'baremetal:allocation:delete',
+                                         allocation_owner)
         except IndexError:
             raise exception.AllocationNotFound(
                 _("Allocation for node %s was not found") %

ironic/api/controllers/v1/utils.py

@@ -1507,9 +1507,23 @@ def check_policy(policy_name):
     # effectively a noop from an authorization perspective because the values
     # we're comparing are coming from the same place.
     cdict = api.request.context.to_policy_values()
+
     policy.authorize(policy_name, cdict, api.request.context)
 
 
+def check_policy_true(policy_name):
+    """Check if the specified policy is authorised for this request.
+
+    :policy_name: Name of the policy to check.
+    :returns: True if policy is matched, otherwise false.
+    """
+    # NOTE(lbragstad): Mapping context attributes into a target dictionary is
+    # effectively a noop from an authorization perspective because the values
+    # we're comparing are coming from the same place.
+    cdict = api.request.context.to_policy_values()
+    return policy.check_policy(policy_name, cdict, api.request.context)
+
+
 def check_owner_policy(object_type, policy_name, owner, lessee=None,
                        conceal_node=False):
     """Check if the policy authorizes this request on an object.
@@ -1592,13 +1606,13 @@ def check_allocation_policy_and_retrieve(policy_name, allocation_ident):
     try:
         rpc_allocation = get_rpc_allocation_with_suffix(
             allocation_ident)
-    except exception.AllocationNotFound:
+        # If the user is not allowed to view the allocation, then
-        # don't expose non-existence unless requester
+        # we need to check that and respond with a 404.
-        # has generic access to policy
+        check_owner_policy('allocation', 'baremetal:allocation:get',
-        cdict = api.request.context.to_policy_values()
+                           rpc_allocation['owner'])
-        policy.authorize(policy_name, cdict, api.request.context)
+    except exception.NotAuthorized:
-        raise
+        raise exception.AllocationNotFound(allocation=allocation_ident)
-
+    # The primary policy check for allocation.
     check_owner_policy('allocation', policy_name, rpc_allocation['owner'])
     return rpc_allocation
 

ironic/common/policy.py

@@ -92,6 +92,10 @@ PROJECT_OWNER_MEMBER = ('role:member and project_id:%(node.owner)s')
 PROJECT_OWNER_READER = ('role:reader and project_id:%(node.owner)s')
 PROJECT_LESSEE_ADMIN = ('role:admin and project_id:%(node.lessee)s')
 
+ALLOCATION_OWNER_ADMIN = ('role:admin and project_id:%(allocation.owner)s')
+ALLOCATION_OWNER_MEMBER = ('role:member and project_id:%(allocation.owner)s')
+ALLOCATION_OWNER_READER = ('role:reader and project_id:%(allocation.owner)s')
+
 SYSTEM_OR_OWNER_MEMBER_AND_LESSEE_ADMIN = (
     '(' + SYSTEM_MEMBER + ') or (' + PROJECT_OWNER_MEMBER + ') or (' + PROJECT_LESSEE_ADMIN + ')'  # noqa
 )
@@ -117,6 +121,25 @@ 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 + ')'
+)
+
+ALLOCATION_READER = (
+    '(' + SYSTEM_READER + ') or (' + ALLOCATION_OWNER_READER + ')'
+)
+
+ALLOCATION_CREATOR = (
+    '(' + SYSTEM_MEMBER + ') or (role:admin)'
+)
+
 # Special purpose aliases for things like "ability to access the API
 # as a reader, or permission checking that does not require node
 # owner relationship checking
@@ -1497,7 +1520,7 @@ deprecated_allocation_list = policy.DeprecatedRule(
 )
 deprecated_allocation_list_all = policy.DeprecatedRule(
     name='baremetal:allocation:list_all',
-    check_str='rule:baremetal:allocation:get'
+    check_str='rule:baremetal:allocation:get and is_admin_project:True'
 )
 deprecated_allocation_create = policy.DeprecatedRule(
     name='baremetal:allocation:create',
@@ -1523,8 +1546,8 @@ roles.
 allocation_policies = [
     policy.DocumentedRuleDefault(
         name='baremetal:allocation:get',
-        check_str=SYSTEM_READER,
+        check_str=ALLOCATION_READER,
-        scope_types=['system'],
+        scope_types=['system', 'project'],
         description='Retrieve Allocation records',
         operations=[
             {'path': '/allocations/{allocation_id}', 'method': 'GET'},
@@ -1536,8 +1559,8 @@ allocation_policies = [
     ),
     policy.DocumentedRuleDefault(
         name='baremetal:allocation:list',
-        check_str=SYSTEM_READER,
+        check_str=API_READER,
-        scope_types=['system'],
+        scope_types=['system', 'project'],
         description='Retrieve multiple Allocation records, filtered by owner',
         operations=[{'path': '/allocations', 'method': 'GET'}],
         deprecated_rule=deprecated_allocation_list,
@@ -1547,7 +1570,7 @@ allocation_policies = [
     policy.DocumentedRuleDefault(
         name='baremetal:allocation:list_all',
         check_str=SYSTEM_READER,
-        scope_types=['system'],
+        scope_types=['system', 'project'],
         description='Retrieve multiple Allocation records',
         operations=[{'path': '/allocations', 'method': 'GET'}],
         deprecated_rule=deprecated_allocation_list_all,
@@ -1556,8 +1579,8 @@ allocation_policies = [
     ),
     policy.DocumentedRuleDefault(
         name='baremetal:allocation:create',
-        check_str=SYSTEM_MEMBER,
+        check_str=ALLOCATION_CREATOR,
-        scope_types=['system'],
+        scope_types=['system', 'project'],
         description='Create Allocation records',
         operations=[{'path': '/allocations', 'method': 'POST'}],
         deprecated_rule=deprecated_allocation_create,
@@ -1567,9 +1590,9 @@ allocation_policies = [
     policy.DocumentedRuleDefault(
         name='baremetal:allocation:create_restricted',
         check_str=SYSTEM_MEMBER,
-        scope_types=['system'],
+        scope_types=['system', 'project'],
         description=(
-            'Create Allocation records that are restricted to an owner'
+            'Create Allocation records with a specific owner.'
         ),
         operations=[{'path': '/allocations', 'method': 'POST'}],
         deprecated_rule=deprecated_allocation_create_restricted,
@@ -1578,8 +1601,8 @@ allocation_policies = [
     ),
     policy.DocumentedRuleDefault(
         name='baremetal:allocation:delete',
-        check_str=SYSTEM_MEMBER,
+        check_str=ALLOCATION_ADMIN,
-        scope_types=['system'],
+        scope_types=['system', 'project'],
         description='Delete Allocation records',
         operations=[
             {'path': '/allocations/{allocation_id}', 'method': 'DELETE'},
@@ -1590,8 +1613,8 @@ allocation_policies = [
     ),
     policy.DocumentedRuleDefault(
         name='baremetal:allocation:update',
-        check_str=SYSTEM_MEMBER,
+        check_str=ALLOCATION_MEMBER,
-        scope_types=['system'],
+        scope_types=['system', 'project'],
         description='Change name and extra fields of an allocation',
         operations=[
             {'path': '/allocations/{allocation_id}', 'method': 'PATCH'},
@@ -1600,6 +1623,22 @@ allocation_policies = [
         deprecated_reason=deprecated_allocation_reason,
         deprecated_since=versionutils.deprecated.WALLABY
     ),
+    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'),
+        scope_types=['project'],
+        description=('Logical restrictor to prevent legacy allocation rule '
+                     'missuse - Requires blank allocations to originate from '
+                     'the legacy baremetal_admin.'),
+        operations=[
+            {'path': '/allocations/{allocation_id}', 'method': 'PATCH'},
+        ],
+        deprecated_reason=deprecated_allocation_reason,
+        deprecated_for_removal=True,
+        deprecated_since=versionutils.deprecated.WALLABY
+    ),
+
 ]
 
 

ironic/db/sqlalchemy/api.py

@@ -439,7 +439,7 @@ class Connection(api.Connection):
         return _paginate_query(models.Node, limit, marker,
                                sort_key, sort_dir, query)
 
-    def check_node_list(self, idents):
+    def check_node_list(self, idents, project=None):
         mapping = {}
         if idents:
             idents = set(idents)
@@ -459,6 +459,10 @@ class Connection(api.Connection):
             sql.or_(models.Node.uuid.in_(uuids),
                     models.Node.name.in_(names))
         )
+        if project:
+            query = query.filter((models.Node.owner == project)
+                                 | (models.Node.lessee == project))
+
         for row in query:
             if row[0] in idents:
                 mapping[row[0]] = row[0]

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

@@ -15,10 +15,12 @@ Tests for the API /allocations/ methods.
 
 import datetime
 from http import client as http_client
+import json
 from unittest import mock
 from urllib import parse as urlparse
 
 import fixtures
+from keystonemiddleware import auth_token
 from oslo_config import cfg
 from oslo_utils import timeutils
 from oslo_utils import uuidutils
@@ -391,6 +393,15 @@ 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
+        # 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,
+        # so may not be necessary, but filtered views are checked in
+        # the RBAC testing.
+        headers = self.headers
+        headers['X-Roles'] = "member,reader"
+        headers['OpenStack-System-Scope'] = "all"
         data = self.get_json("/allocations?owner=12345",
                              headers=self.headers)
         self.assertEqual(3, len(data['allocations']))
@@ -994,6 +1005,57 @@ class TestPost(test_api_base.BaseApiTest):
         self.assertEqual('application/json', response.content_type)
         self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_int)
 
+    @mock.patch.object(auth_token.AuthProtocol, 'process_request',
+                       autospec=True)
+    def test_create_allocation_owner_not_my_projet_id(self, mock_auth_req):
+        # This is only enforced, test wise with the new oslo policy rbac
+        # model and enforcement. Likely can be cleaned up past the Xena cycle.
+        cfg.CONF.set_override('enforce_scope', True, group='oslo_policy')
+        cfg.CONF.set_override('enforce_new_defaults', True,
+                              group='oslo_policy')
+        # Tests normally run in noauth, but we need policy
+        # enforcement to run completely here to ensure the logic is followed.
+        cfg.CONF.set_override('auth_strategy', 'keystone')
+        self.headers['X-Project-ID'] = '0987'
+        self.headers['X-Roles'] = 'admin,member,reader'
+        owner = '12345'
+        adict = apiutils.allocation_post_data(owner=owner)
+        response = self.post_json('/allocations', adict, expect_errors=True,
+                                  headers=self.headers)
+        self.assertEqual(http_client.FORBIDDEN, response.status_int)
+        expected_faultstring = ('Cannot create allocation with an owner '
+                                'Project ID value 12345 not matching the '
+                                'requestor Project ID 0987. Policy '
+                                'baremetal:allocation:create_restricted '
+                                'is required for this capability.')
+        error_body = json.loads(response.json['error_message'])
+        self.assertEqual(expected_faultstring,
+                         error_body.get('faultstring'))
+
+    def test_create_allocation_owner_auto_filled(self):
+        cfg.CONF.set_override('enforce_new_defaults', True,
+                              group='oslo_policy')
+        self.headers['X-Project-ID'] = '123456'
+        adict = apiutils.allocation_post_data()
+        self.post_json('/allocations', adict, headers=self.headers)
+        result = self.get_json('/allocations/%s' % adict['uuid'],
+                               headers=self.headers)
+        self.assertEqual('123456', result['owner'])
+
+    def test_create_allocation_is_restricted_until_scope_enforcement(self):
+        cfg.CONF.set_override('enforce_new_defaults', False,
+                              group='oslo_policy')
+        cfg.CONF.set_override('auth_strategy', 'keystone')
+        # We're setting ourselves to be a random ID and member
+        # which is allowed to create an allocation.
+        self.headers['X-Project-ID'] = '1135'
+        self.headers['X-Roles'] = 'admin, member, reader'
+        self.headers['X-Is-Admin-Project'] = 'False'
+        adict = apiutils.allocation_post_data()
+        response = self.post_json('/allocations', adict, expect_errors=True,
+                                  headers=self.headers)
+        self.assertEqual(http_client.FORBIDDEN, response.status_int)
+
     def test_backfill(self):
         node = obj_utils.create_test_node(self.context)
         adict = apiutils.allocation_post_data(node=node.uuid)
@@ -1092,7 +1154,6 @@ class TestPost(test_api_base.BaseApiTest):
                 raise exception.HTTPForbidden(resource='fake')
             return True
         mock_authorize.side_effect = mock_authorize_function
-
         owner = '12345'
         adict = apiutils.allocation_post_data()
         del adict['owner']
@@ -1126,7 +1187,6 @@ class TestPost(test_api_base.BaseApiTest):
                 raise exception.HTTPForbidden(resource='fake')
             return True
         mock_authorize.side_effect = mock_authorize_function
-
         owner = '12345'
         adict = apiutils.allocation_post_data(owner=owner)
         adict['owner'] = owner

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

@@ -1253,14 +1253,18 @@ class TestCheckAllocationPolicyAndRetrieve(base.TestCase):
             'fake_policy', self.valid_allocation_uuid
         )
         mock_graws.assert_called_once_with(self.valid_allocation_uuid)
-        mock_authorize.assert_called_once_with(
+
-            'fake_policy', expected_target, fake_context)
+        expected = [mock.call('baremetal:allocation:get',
+                              expected_target, fake_context),
+                    mock.call('fake_policy', expected_target,
+                              fake_context)]
+        mock_authorize.assert_has_calls(expected)
         self.assertEqual(self.allocation, rpc_allocation)
 
     @mock.patch.object(api, 'request', spec_set=["context"])
     @mock.patch.object(policy, 'authorize', spec=True)
     @mock.patch.object(utils, 'get_rpc_allocation_with_suffix', autospec=True)
-    def test_check_alloc_policy_and_retrieve_no_alloc_policy_forbidden(
+    def test_check_alloc_policy_and_retrieve_no_alloc_policy_not_found(
             self, mock_graws, mock_authorize, mock_pr
     ):
         mock_pr.context.to_policy_values.return_value = {}
@@ -1269,7 +1273,7 @@ class TestCheckAllocationPolicyAndRetrieve(base.TestCase):
             allocation=self.valid_allocation_uuid)
 
         self.assertRaises(
-            exception.HTTPForbidden,
+            exception.AllocationNotFound,
             utils.check_allocation_policy_and_retrieve,
             'fake-policy',
             self.valid_allocation_uuid
@@ -1295,7 +1299,7 @@ class TestCheckAllocationPolicyAndRetrieve(base.TestCase):
     @mock.patch.object(api, 'request', spec_set=["context", "version"])
     @mock.patch.object(policy, 'authorize', spec=True)
     @mock.patch.object(utils, 'get_rpc_allocation_with_suffix', autospec=True)
-    def test_check_allocation_policy_and_retrieve_policy_forbidden(
+    def test_check_allocation_policy_and_retrieve_policy_not_found(
             self, mock_graws, mock_authorize, mock_pr
     ):
         mock_pr.version.minor = 50
@@ -1304,7 +1308,7 @@ class TestCheckAllocationPolicyAndRetrieve(base.TestCase):
         mock_graws.return_value = self.allocation
 
         self.assertRaises(
-            exception.HTTPForbidden,
+            exception.AllocationNotFound,
             utils.check_allocation_policy_and_retrieve,
             'fake-policy',
             self.valid_allocation_uuid

ironic/tests/unit/api/test_acl.py

@@ -398,9 +398,15 @@ class TestRBACProjectScoped(TestACLBase):
             uuid='65ea0296-219b-4635-b0c8-a6e055da878d',
             node_id=owned_node['id'],
             connector_id='iqn.2012-06.org.openstack.magic')
+        fake_owner_allocation = db_utils.create_test_allocation(
+            node_id=owned_node['id'],
+            owner=owner_project_id,
+            resource_class="CUSTOM_TEST")
 
         # Leased nodes
+        fake_allocation_id = 61
         leased_node = db_utils.create_test_node(
+            allocation_id=fake_allocation_id,
             uuid=lessee_node_ident,
             owner=owner_project_id,
             lessee=lessee_project_id,
@@ -416,6 +422,11 @@ class TestRBACProjectScoped(TestACLBase):
             node_id=leased_node['id'])
         fake_trait = 'CUSTOM_MEOW'
         fake_vif_port_id = "0e21d58f-5de2-4956-85ff-33935ea1ca01"
+        fake_leased_allocation = db_utils.create_test_allocation(
+            id=fake_allocation_id,
+            node_id=leased_node['id'],
+            owner=lessee_project_id,
+            resource_class="CUSTOM_LEASED")
 
         # Random objects that shouldn't be project visible
         other_port = db_utils.create_test_port(
@@ -447,7 +458,9 @@ class TestRBACProjectScoped(TestACLBase):
             'other_port_ident': other_port['uuid'],
             'owner_portgroup_ident': owner_pgroup['uuid'],
             'other_portgroup_ident': other_pgroup['uuid'],
-            'driver_name': 'fake-driverz'})
+            'driver_name': 'fake-driverz',
+            'owner_allocation': fake_owner_allocation['uuid'],
+            'lessee_allocation': fake_leased_allocation['uuid']})
 
     @ddt.file_data('test_rbac_project_scoped.yaml')
     @ddt.unpack

ironic/tests/unit/api/test_rbac_legacy.yaml

@@ -1938,7 +1938,7 @@ allocations_allocation_id_get_member:
   path: '/v1/allocations/{allocation_ident}'
   method: get
   headers: *member_headers
-  assert_status: 403
+  assert_status: 404
   deprecated: true
 
 allocations_allocation_id_get_observer:
@@ -1964,7 +1964,7 @@ allocations_allocation_id_patch_member:
   method: patch
   headers: *member_headers
   body: *allocation_patch
-  assert_status: 403
+  assert_status: 404
   deprecated: true
 
 allocations_allocation_id_patch_observer:
@@ -1986,7 +1986,7 @@ allocations_allocation_id_delete_member:
   path: '/v1/allocations/{allocation_ident}'
   method: delete
   headers: *member_headers
-  assert_status: 403
+  assert_status: 404
   deprecated: true
 
 allocations_allocation_id_delete_observer:
@@ -2008,7 +2008,7 @@ nodes_allocation_get_member:
   path: '/v1/nodes/{allocated_node_ident}/allocation'
   method: get
   headers: *member_headers
-  assert_status: 403
+  assert_status: 404
   deprecated: true
 
 nodes_allocation_get_observer:
@@ -2029,7 +2029,7 @@ nodes_allocation_delete_member:
   path: '/v1/nodes/{allocated_node_ident}/allocation'
   method: delete
   headers: *member_headers
-  assert_status: 403
+  assert_status: 404
   deprecated: true
 
 nodes_allocation_delete_observer:

ironic/tests/unit/api/test_rbac_project_scoped.yaml

@@ -2272,60 +2272,153 @@ third_party_admin_cannot_get_conductors:
 
 # This is a system scoped endpoint, everything should fail in this section.
 
-owner_reader_cannot_get_allocations:
+owner_reader_can_get_allocations:
   path: '/v1/allocations'
   method: get
-  headers: *lessee_admin_headers
+  headers: *lessee_reader_headers
-  assert_status: 403
+  assert_status: 200
-  skip_reason: policy not implemented
+  assert_list_length:
+    allocations: 1
 
-lessee_reader_cannot_get_allocations:
+lessee_reader_can_get_allocations:
   path: '/v1/allocations'
   method: get
-  headers: *lessee_admin_headers
+  headers: *lessee_reader_headers
-  assert_status: 403
+  assert_status: 200
-  skip_reason: policy not implemented
+  assert_list_length:
+    allocations: 1
 
-third_party_admin_cannot_get_allocations:
+owner_reader_can_get_their_allocation:
+  path: '/v1/allocations/{owner_allocation}'
+  method: get
+  headers: *owner_reader_headers
+  assert_status: 200
+  assert_dict_contains:
+    resource_class: CUSTOM_TEST
+
+lessee_reader_can_get_their_allocation:
+  path: '/v1/allocations/{lessee_allocation}'
+  method: get
+  headers: *lessee_reader_headers
+  assert_status: 200
+  assert_dict_contains:
+    resource_class: CUSTOM_LEASED
+
+owner_admin_can_delete_their_allocation:
+  path: '/v1/allocations/{owner_allocation}'
+  method: delete
+  headers: *owner_admin_headers
+  assert_status: 503
+
+lessee_admin_can_delete_their_allocation:
+  path: '/v1/allocations/{lessee_allocation}'
+  method: delete
+  headers: *lessee_admin_headers
+  assert_status: 503
+
+owner_member_cannot_delete_their_allocation:
+  path: '/v1/allocations/{owner_allocation}'
+  method: delete
+  headers: *owner_member_headers
+  assert_status: 403
+
+lessee_member_cannot_delete_their_allocation:
+  path: '/v1/allocations/{lessee_allocation}'
+  method: delete
+  headers: *lessee_member_headers
+  assert_status: 403
+
+owner_member_can_patch_allocation:
+  path: '/v1/allocations/{owner_allocation}'
+  method: patch
+  headers: *owner_member_headers
+  body: &allocation_patch
+    - op: replace
+      path: /extra
+      value: {'test': 'testing'}
+  assert_status: 200
+
+lessee_member_can_patch_allocation:
+  path: '/v1/allocations/{lessee_allocation}'
+  method: patch
+  headers: *lessee_member_headers
+  body: *allocation_patch
+  assert_status: 200
+
+third_party_admin_can_get_allocations:
   path: '/v1/allocations'
   method: get
   headers: *third_party_admin_headers
-  assert_status: 403
+  assert_status: 200
-  skip_reason: policy not implemented
+  assert_list_length:
+    allocations: 0
 
-third_party_admin_cannot_create_allocation:
+third_party_admin_can_create_allocation:
+  # This is distinctly different than most other behavior,
+  # should be applied to filter this, however this is also handled
+  # in the conductor, the only case where a user *should* be able
+  # to pass a UUID directly in though is a special case which
+  # should not be possible unless the user is the owner of the
+  # owner or lessee of the node.
   path: '/v1/allocations'
   method: post
   headers: *third_party_admin_headers
   body: &allocation_body
     resource_class: CUSTOM_TEST
-  assert_status: 403
+  assert_status: 503
-  skip_reason: policy not implemented
+
+third_party_admin_cannot_create_allocation_with_owner_node:
+  path: '/v1/allocations'
+  method: post
+  headers: *third_party_admin_headers
+  body:
+    resource_class: CUSTOM_TEST
+    node: 1ab63b9e-66d7-4cd7-8618-dddd0f9f7881
+  assert_status: 400
+
+third_party_admin_cannot_create_allocation_with_candidates_not_owned:
+  path: '/v1/allocations'
+  method: post
+  headers: *third_party_admin_headers
+  body:
+    resource_class: CUSTOM_TEST
+    candidate_nodes:
+      - 1ab63b9e-66d7-4cd7-8618-dddd0f9f7881
+      - 38d5abed-c585-4fce-a57e-a2ffc2a2ec6f
+  assert_status: 400
+
+owner_admin_can_create_allocation_with_their_uuid:
+  # NOTE(TheJulia): Owner/Lessee are equivelent in
+  # this context, so testing only one is fine.
+  path: '/v1/allocations'
+  method: post
+  headers: *owner_admin_headers
+  body:
+    resource_class: CUSTOM_TEST
+    node: 1ab63b9e-66d7-4cd7-8618-dddd0f9f7881
+  assert_status: 503
 
 third_party_admin_cannot_read_an_allocation:
-  path: '/v1/allocations/{allocation_ident}'
+  path: '/v1/allocations/{lessee_allocation}'
   method: get
   headers: *third_party_admin_headers
-  assert_status: 403
+  assert_status: 404
-  skip_reason: policy not implemented
 
 third_party_admin_cannot_patch_an_allocation:
-  path: '/v1/allocations/{allocation_ident}'
+  path: '/v1/allocations/{owner_allocation}'
   method: patch
   headers: *third_party_admin_headers
   body:
     - op: replace
       path: /extra
       value: {'test': 'testing'}
-  assert_status: 403
+  assert_status: 404
-  skip_reason: policy not implemented
 
 third_party_admin_cannot_delete_an_allocation:
-  path: '/v1/allocations/{allocation_ident}'
+  path: '/v1/allocations/{owner_allocation}'
   method: delete
   headers: *third_party_admin_headers
-  assert_status: 403
+  assert_status: 404
-  skip_reason: policy not implemented
 
 # Allocations ( Node level) - https://docs.openstack.org/api-ref/baremetal/#node-allocation-allocations-nodes
 
@@ -2334,42 +2427,36 @@ owner_reader_can_read_node_allocation:
   method: get
   headers: *owner_reader_headers
   assert_status: 200
-  skip_reason: policy not implemented
 
 lessee_reader_can_read_node_allocation:
   path: '/v1/nodes/{lessee_node_ident}/allocation'
   method: get
   headers: *lessee_reader_headers
   assert_status: 200
-  skip_reason: policy not implemented
 
 third_party_admin_cannot_read_node_allocation:
   path: '/v1/nodes/{owner_node_ident}/allocation'
   method: get
   headers: *third_party_admin_headers
   assert_status: 404
-  skip_reason: policy not implemented
 
 owner_admin_can_delete_allocation:
   path: '/v1/nodes/{owner_node_ident}/allocation'
   method: delete
   headers: *owner_admin_headers
   assert_status: 503
-  skip_reason: policy not implemented
 
-lessee_admin_cannot_delete_allocation:
+lessee_admin_not_delete_allocation:
   path: '/v1/nodes/{allocated_node_ident}/allocation'
   method: delete
   headers: *lessee_admin_headers
-  assert_status: 403
+  assert_status: 503
-  skip_reason: policy not implemented
 
 third_party_admin_cannot_delete_allocation:
   path: '/v1/nodes/{allocated_node_ident}/allocation'
   method: delete
   headers: *third_party_admin_headers
-  assert_status: 403
+  assert_status: 404
-  skip_reason: policy not implemented
 
 # Deploy Templates - https://docs.openstack.org/api-ref/baremetal/#deploy-templates-deploy-templates
 

releasenotes/notes/allocations-restricted-rbac-create-2847943150656432.yaml

@@ -0,0 +1,13 @@
+---
+security:
+  - |
+    Ability to create an allocation has been restricted by a new policy rule
+    ``baremetal::allocation::create_pre_rbac`` which prevents creation of
+    allocations by any project administrator when operating with the new
+    Role Based Access Control model. The use and enforcement of this
+    rule is disabled when ``[oslo_policy]enforce_new_defaults`` is set which
+    also makes the population of a ``owner`` field for allocations to
+    become automatically populated. Most deployments should not encounter any
+    issues with this security change, and the policy rule will be removed
+    when support for the legacy ``baremetal_admin`` custom role has been
+    removed.

tox.ini

@@ -127,7 +127,7 @@ filename = *.py,app.wsgi
 exclude =  .venv,.git,.tox,dist,doc,*lib/python*,*egg,build
 import-order-style = pep8
 application-import-names = ironic
-max-complexity=18
+max-complexity=19
 # [H106] Don't put vim configuration in source files.
 # [H203] Use assertIs(Not)None to check for None.
 # [H204] Use assert(Not)Equal to check for equality.