diff --git a/barbican/api/controllers/consumers.py b/barbican/api/controllers/consumers.py index 887529a15..c8032fecb 100644 --- a/barbican/api/controllers/consumers.py +++ b/barbican/api/controllers/consumers.py @@ -33,6 +33,12 @@ def _consumer_not_found(): 'another castle.')) +def _consumer_ownership_mismatch(): + """Throw exception indicating the user does not own this consumer.""" + pecan.abort(403, u._('Not Allowed. Sorry, only the creator of a consumer ' + 'can delete it.')) + + class ContainerConsumerController(controllers.ACLMixin): """Handles Consumer entity retrieval and deletion requests.""" @@ -51,7 +57,6 @@ class ContainerConsumerController(controllers.ACLMixin): def on_get(self, external_project_id): consumer = self.consumer_repo.get( entity_id=self.consumer_id, - external_project_id=external_project_id, suppress_exception=True) if not consumer: _consumer_not_found() @@ -92,11 +97,6 @@ class ContainerConsumersController(controllers.ACLMixin): LOG.debug(u._('Start consumers on_get ' 'for container-ID %s:'), self.container_id) - try: - self.container_repo.get(self.container_id, external_project_id) - except exception.NotFound: - controllers.containers.container_not_found() - result = self.consumer_repo.get_by_container_id( self.container_id, offset_arg=kw.get('offset', 0), @@ -136,18 +136,13 @@ class ContainerConsumersController(controllers.ACLMixin): data = api.load_body(pecan.request, validator=self.validator) LOG.debug('Start on_post...%s', data) - try: - container = self.container_repo.get(self.container_id, - external_project_id) - except exception.NotFound: - controllers.containers.container_not_found() + container = self._get_container(self.container_id) self.quota_enforcer.enforce(project) new_consumer = models.ContainerConsumerMetadatum(self.container_id, project.id, data) - new_consumer.project_id = project.id self.consumer_repo.create_or_update_from(new_consumer, container) url = hrefs.convert_consumer_to_href(new_consumer.container_id) @@ -156,8 +151,7 @@ class ContainerConsumersController(controllers.ACLMixin): LOG.info(u._LI('Created a consumer for project: %s'), external_project_id) - return self._return_container_data(self.container_id, - external_project_id) + return self._return_container_data(self.container_id) @index.when(method='DELETE', template='json') @controllers.handle_exceptions(u._('ContainerConsumer deletion')) @@ -176,6 +170,13 @@ class ContainerConsumersController(controllers.ACLMixin): _consumer_not_found() LOG.debug("Found consumer: %s", consumer) + container = self._get_container(self.container_id) + owner_of_consumer = consumer.project_id == external_project_id + owner_of_container = container.project.external_id \ + == external_project_id + if not owner_of_consumer and not owner_of_container: + _consumer_ownership_mismatch() + try: self.consumer_repo.delete_entity_by_id(consumer.id, external_project_id) @@ -183,21 +184,22 @@ class ContainerConsumersController(controllers.ACLMixin): LOG.exception(u._LE('Problem deleting consumer')) _consumer_not_found() - ret_data = self._return_container_data( - self.container_id, - external_project_id - ) + ret_data = self._return_container_data(self.container_id) LOG.info(u._LI('Deleted a consumer for project: %s'), external_project_id) return ret_data - def _return_container_data(self, container_id, external_project_id): - try: - container = self.container_repo.get(container_id, - external_project_id) - dict_fields = container.to_dict_fields() - except Exception: + def _get_container(self, container_id): + container = self.container_repo.get_container_by_id( + container_id, suppress_exception=True) + if not container: controllers.containers.container_not_found() + return container + + def _return_container_data(self, container_id): + container = self._get_container(container_id) + + dict_fields = container.to_dict_fields() for secret_ref in dict_fields['secret_refs']: hrefs.convert_to_hrefs(secret_ref) diff --git a/barbican/tests/api/test_resources.py b/barbican/tests/api/test_resources.py index 7c6a12024..10e65a60e 100644 --- a/barbican/tests/api/test_resources.py +++ b/barbican/tests/api/test_resources.py @@ -786,7 +786,6 @@ class WhenCreatingConsumersUsingConsumersResource(FunctionalTest): # Set up mocked container repo self.container_repo = mock.MagicMock() - self.container_repo.get.return_value = self.container self.container_repo.get_container_by_id.return_value = self.container self.setup_container_repository_mock(self.container_repo) @@ -824,22 +823,12 @@ class WhenCreatingConsumersUsingConsumersResource(FunctionalTest): ) self.assertEqual(415, resp.status_int) - def test_should_404_consumer_bad_container_id(self): - self.container_repo.get.side_effect = excep.NotFound() + def test_should_404_when_container_ref_doesnt_exist(self): + self.container_repo.get_container_by_id.return_value = None resp = self.app.post_json( '/containers/{0}/consumers/'.format('bad_id'), self.consumer_ref, expect_errors=True ) - self.container_repo.get.side_effect = None - self.assertEqual(404, resp.status_int) - - def test_should_raise_exception_when_container_ref_doesnt_exist(self): - self.container_repo.get.return_value = None - resp = self.app.post_json( - '/containers/{0}/consumers/'.format(self.container.id), - self.consumer_ref, - expect_errors=True - ) self.assertEqual(404, resp.status_int) @@ -896,7 +885,6 @@ class WhenGettingOrDeletingConsumersUsingConsumerResource(FunctionalTest): # Set up mocked container repo self.container_repo = mock.MagicMock() - self.container_repo.get.return_value = self.container self.container_repo.get_container_by_id.return_value = self.container self.setup_container_repository_mock(self.container_repo) @@ -928,12 +916,11 @@ class WhenGettingOrDeletingConsumersUsingConsumerResource(FunctionalTest): self.assertEqual(self.consumer.name, resp.json['consumers'][0]['name']) self.assertEqual(self.consumer.URL, resp.json['consumers'][0]['URL']) - def test_should_404_with_bad_container_id(self): - self.container_repo.get.side_effect = excep.NotFound() + def test_should_404_when_container_ref_doesnt_exist(self): + self.container_repo.get_container_by_id.return_value = None resp = self.app.get('/containers/{0}/consumers/'.format( 'bad_id' ), expect_errors=True) - self.container_repo.get.side_effect = None self.assertEqual(404, resp.status_int) def test_should_get_consumer_by_id(self): @@ -1105,7 +1092,7 @@ class WhenPerformingUnallowedOperationsOnConsumers(FunctionalTest): # Set up container repo self.container_repo = mock.MagicMock() - self.container_repo.get.return_value = self.container + self.container_repo.get_container_by_id.return_value = self.container self.setup_container_repository_mock(self.container_repo) # Set up container consumer repo @@ -1156,3 +1143,75 @@ class WhenPerformingUnallowedOperationsOnConsumers(FunctionalTest): expect_errors=True ) self.assertEqual(405, resp.status_int) + + +class WhenOwnershipMismatch(FunctionalTest): + + def setUp(self): + super( + WhenOwnershipMismatch, self + ).setUp() + self.app = webtest.TestApp(app.build_wsgi_app(self.root)) + self.app.extra_environ = get_barbican_env(self.external_project_id) + + @property + def root(self): + self._init() + + class RootController(object): + containers = controllers.containers.ContainersController() + return RootController() + + def _init(self): + self.external_project_id = 'keystoneid1234' + self.project_internal_id = 'projectid1234' + + # Set up mocked project + self.project = models.Project() + self.project.id = self.project_internal_id + self.project.external_id = self.external_project_id + + # Set up mocked project repo + self.project_repo = mock.MagicMock() + self.project_repo.get.return_value = self.project + self.setup_project_repository_mock(self.project_repo) + + # Set up mocked container + self.container = create_container( + id_ref='id1', + project_id=self.project_internal_id, + external_project_id='differentProjectId') + + # Set up mocked consumers + self.consumer = create_consumer(self.container.id, + self.project_internal_id, + id_ref='id2') + self.consumer2 = create_consumer(self.container.id, + self.project_internal_id, + id_ref='id3') + + self.consumer_ref = { + 'name': self.consumer.name, + 'URL': self.consumer.URL + } + + # Set up mocked container repo + self.container_repo = mock.MagicMock() + self.container_repo.get.return_value = self.container + self.container_repo.get_container_by_id.return_value = self.container + self.setup_container_repository_mock(self.container_repo) + + # Set up mocked container consumer repo + self.consumer_repo = mock.MagicMock() + self.consumer_repo.get_by_values.return_value = self.consumer + self.consumer_repo.delete_entity_by_id.return_value = None + self.setup_container_consumer_repository_mock(self.consumer_repo) + + # Set up mocked secret repo + self.setup_secret_repository_mock() + + def test_consumer_check_ownership_mismatch(self): + resp = self.app.delete_json( + '/containers/{0}/consumers/'.format(self.container.id), + self.consumer_ref, expect_errors=True) + self.assertEqual(403, resp.status_int) diff --git a/etc/barbican/policy.json b/etc/barbican/policy.json index 06e0b06c9..87e87cb5a 100644 --- a/etc/barbican/policy.json +++ b/etc/barbican/policy.json @@ -38,10 +38,10 @@ "order:get": "rule:all_users", "order:put": "rule:admin_or_creator", "order:delete": "rule:admin", - "consumer:get": "rule:all_users", - "consumers:get": "rule:all_users", - "consumers:post": "rule:admin", - "consumers:delete": "rule:admin", + "consumer:get": "rule:admin or rule:observer or rule:creator or rule:audit or rule:container_non_private_read or rule:container_project_creator or rule:container_project_admin or rule:container_acl_read", + "consumers:get": "rule:admin or rule:observer or rule:creator or rule:audit or rule:container_non_private_read or rule:container_project_creator or rule:container_project_admin or rule:container_acl_read", + "consumers:post": "rule:admin or rule:container_non_private_read or rule:container_project_creator or rule:container_project_admin or rule:container_acl_read", + "consumers:delete": "rule:admin or rule:container_non_private_read or rule:container_project_creator or rule:container_project_admin or rule:container_acl_read", "containers:post": "rule:admin_or_creator", "containers:get": "rule:all_but_audit", "container:get": "rule:container_non_private_read or rule:container_project_creator or rule:container_project_admin or rule:container_acl_read", diff --git a/functionaltests/api/v1/functional/test_acls.py b/functionaltests/api/v1/functional/test_acls.py index fd8e3a048..6882e4063 100644 --- a/functionaltests/api/v1/functional/test_acls.py +++ b/functionaltests/api/v1/functional/test_acls.py @@ -130,8 +130,8 @@ test_data_read_container_consumer_acl_only = { 'with_creator_a': {'user': creator_a, 'expected_return': 200}, 'with_observer_a': {'user': observer_a, 'expected_return': 200}, 'with_auditor_a': {'user': auditor_a, 'expected_return': 200}, - 'with_admin_b': {'user': admin_b, 'expected_return': 404}, - 'with_observer_b': {'user': observer_b, 'expected_return': 404}, + 'with_admin_b': {'user': admin_b, 'expected_return': 200}, + 'with_observer_b': {'user': observer_b, 'expected_return': 200}, } test_data_delete_container_consumer_acl_only = { @@ -139,7 +139,7 @@ test_data_delete_container_consumer_acl_only = { 'with_creator_a': {'user': creator_a, 'expected_return': 403}, 'with_observer_a': {'user': observer_a, 'expected_return': 403}, 'with_auditor_a': {'user': auditor_a, 'expected_return': 403}, - 'with_admin_b': {'user': admin_b, 'expected_return': 404}, + 'with_admin_b': {'user': admin_b, 'expected_return': 403}, 'with_observer_b': {'user': observer_b, 'expected_return': 403}, } @@ -148,7 +148,7 @@ test_data_create_container_consumer_acl_only = { 'with_creator_a': {'user': creator_a, 'expected_return': 403}, 'with_observer_a': {'user': observer_a, 'expected_return': 403}, 'with_auditor_a': {'user': auditor_a, 'expected_return': 403}, - 'with_admin_b': {'user': admin_b, 'expected_return': 404}, + 'with_admin_b': {'user': admin_b, 'expected_return': 200}, 'with_observer_b': {'user': observer_b, 'expected_return': 403}, }