From 38ecf5b51fef1293e9c1d95d8110c50ae5997f28 Mon Sep 17 00:00:00 2001 From: Pan Date: Thu, 25 Aug 2016 12:56:07 -0400 Subject: [PATCH] Remove consumer check for project_id to match containers I believe this is the correct behavior, as it would match how containers handles these operations. This change facilitates the LBaaS Barbican TLS workflow (which should be the same as what other services will use in the future too). The RBAC settings for consumer POST should be set to use the same ACL rules as container GET (plus admin). The RBAC settings for consumer DELETE should be: * Any user with Delete permissions on the Container * Any user that both: has ACL Read access to the Container; is a member of the project that created the Consumer being deleted Change-Id: Ie84784573893934c2887814a200e7386314b4f18 Closes-Bug: #1519170 --- barbican/api/controllers/consumers.py | 50 +++++----- barbican/tests/api/test_resources.py | 95 +++++++++++++++---- etc/barbican/policy.json | 8 +- .../api/v1/functional/test_acls.py | 8 +- 4 files changed, 111 insertions(+), 50 deletions(-) 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 41b99365f..166c12c00 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}, }