Merge "Remove consumer check for project_id to match containers"

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._get_container(self.container_id)
-            container = self.container_repo.get(self.container_id,
-                                                external_project_id)
-        except exception.NotFound:
-            controllers.containers.container_not_found()
 
         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,
+        return self._return_container_data(self.container_id)
-                                           external_project_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(
+        ret_data = self._return_container_data(self.container_id)
-            self.container_id,
-            external_project_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):
+    def _get_container(self, container_id):
-        try:
+        container = self.container_repo.get_container_by_id(
-            container = self.container_repo.get(container_id,
+            container_id, suppress_exception=True)
-                                                external_project_id)
+        if not container:
-            dict_fields = container.to_dict_fields()
-        except Exception:
             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)

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):
+    def test_should_404_when_container_ref_doesnt_exist(self):
-        self.container_repo.get.side_effect = excep.NotFound()
+        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):
+    def test_should_404_when_container_ref_doesnt_exist(self):
-        self.container_repo.get.side_effect = excep.NotFound()
+        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)

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",
+    "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:all_users",
+    "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",
+    "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",
+    "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",

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_admin_b': {'user': admin_b, 'expected_return': 200},
-    'with_observer_b': {'user': observer_b, 'expected_return': 404},
+    '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},
 }