From ea7451e32caa7863d6e511a39a508e54c60113a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Douglas=20Mendiz=C3=A1bal?= Date: Fri, 12 Nov 2021 11:54:26 -0600 Subject: [PATCH] Fix policy for Orders This patch adds checks to make sure that the project_id of the token matches the project_id that owns the Order. Currently, having a role on any project will allow the request to be processed, which results in a 404 - Not Found instead of 401 - Forbidden. Change-Id: Ie0e6f6edae40e47d45afbe92fd509032cb091b1a (cherry picked from commit 5d81a3c4531f1179bcac69c4b10542da04822f8b) (cherry picked from commit 382b5086a2723da8f8d0a07d263ee47a9c6df5e7) --- barbican/api/controllers/orders.py | 6 ++++++ barbican/common/policies/orders.py | 8 ++++++-- barbican/tests/api/test_resources_policy.py | 19 +++++++++++++++---- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/barbican/api/controllers/orders.py b/barbican/api/controllers/orders.py index 5ed9ee7ad..635407ae5 100644 --- a/barbican/api/controllers/orders.py +++ b/barbican/api/controllers/orders.py @@ -66,6 +66,12 @@ class OrderController(controllers.ACLMixin): self.queue = queue_resource or async_client.TaskClient() self.type_order_validator = validators.TypeOrderValidator() + def get_acl_tuple(self, req, **kwargs): + acl = dict() + acl['project_id'] = self.order.project.external_id + acl['creator_id'] = self.order.creator_id + return 'order', acl + @pecan.expose(generic=True) def index(self, **kwargs): pecan.abort(405) # HTTP 405 Method Not Allowed as default diff --git a/barbican/common/policies/orders.py b/barbican/common/policies/orders.py index f0580f00a..04e8ee3b9 100644 --- a/barbican/common/policies/orders.py +++ b/barbican/common/policies/orders.py @@ -12,7 +12,9 @@ from oslo_policy import policy + _MEMBER = "role:member" +_PROJECT_MEMBER = f"{_MEMBER} and project_id:%(target.order.project_id)s" rules = [ policy.DocumentedRuleDefault( @@ -53,7 +55,8 @@ rules = [ ), policy.DocumentedRuleDefault( name='order:get', - check_str=f'rule:all_users or {_MEMBER}', + check_str='rule:all_users and project_id:%(target.order.project_id)s ' + f'or {_PROJECT_MEMBER}', scope_types=['project'], description='Retrieves an orders metadata.', operations=[ @@ -65,7 +68,8 @@ rules = [ ), policy.DocumentedRuleDefault( name='order:delete', - check_str=f'rule:admin or {_MEMBER}', + check_str='rule:admin and project_id:%(target.order.project_id)s or ' + f'{_PROJECT_MEMBER}', scope_types=['project'], description='Deletes an order.', operations=[ diff --git a/barbican/tests/api/test_resources_policy.py b/barbican/tests/api/test_resources_policy.py index 3b00b6be8..bf6ab224a 100644 --- a/barbican/tests/api/test_resources_policy.py +++ b/barbican/tests/api/test_resources_policy.py @@ -983,7 +983,8 @@ class WhenTestingOrdersResource(BaseTestCase): def test_should_pass_get_orders(self): self._assert_pass_rbac(['admin', 'observer', 'creator'], - self._invoke_on_get) + self._invoke_on_get, + project_id=self.external_project_id) def test_should_raise_get_orders(self): self._assert_fail_rbac([None, 'audit', 'bogus'], @@ -1004,8 +1005,16 @@ class WhenTestingOrderResource(BaseTestCase): self.external_project_id = '12345project' self.order_id = '12345order' + order = mock.MagicMock() + order.id = self.order_id + order.project.external_id = self.external_project_id + order.creator_id = 'CRE-A-TOR-UUID' + # Force an error on GET and DELETE calls that pass RBAC, # as we are not testing such flows in this test module. + mock_to_dict = mock.MagicMock() + mock_to_dict.side_effect = self._generate_get_error() + order.to_dict_fields = mock_to_dict self.order_repo = mock.MagicMock() fail_method = mock.MagicMock(return_value=None, side_effect=self._generate_get_error()) @@ -1014,21 +1023,23 @@ class WhenTestingOrderResource(BaseTestCase): self.setup_order_repository_mock(self.order_repo) - self.resource = OrderResource(self.order_id) + self.resource = OrderResource(order) def test_rules_should_be_loaded(self): self.assertIsNotNone(self.policy_enforcer.rules) def test_should_pass_get_order(self): self._assert_pass_rbac(['admin', 'observer', 'creator', 'audit'], - self._invoke_on_get) + self._invoke_on_get, + project_id=self.external_project_id) def test_should_raise_get_order(self): self._assert_fail_rbac([None, 'bogus'], self._invoke_on_get) def test_should_pass_delete_order(self): - self._assert_pass_rbac(['admin'], self._invoke_on_delete) + self._assert_pass_rbac(['admin'], self._invoke_on_delete, + project_id=self.external_project_id) def test_should_raise_delete_order(self): self._assert_fail_rbac([None, 'audit', 'observer', 'creator', 'bogus'],