Merge "Return container not found before ACL checks"

barbican/api/controllers/containers.py

@@ -37,25 +37,21 @@ def container_not_found():
 class ContainerController(controllers.ACLMixin):
     """Handles Container entity retrieval and deletion requests."""
 
-    def __init__(self, container_id):
-        self.container_id = container_id
+    def __init__(self, container):
+        self.container = container
+        self.container_id = container.id
         self.consumer_repo = repo.get_container_consumer_repository()
         self.container_repo = repo.get_container_repository()
         self.validator = validators.ContainerValidator()
-        self.consumers = consumers.ContainerConsumersController(container_id)
+        self.consumers = consumers.ContainerConsumersController(
+            self.container_id)
         self.acls = acls.ContainerACLsController(self.container_id)
-        self.container = None
 
     def get_acl_tuple(self, req, **kwargs):
-        self.container = self.container_repo.get_container_by_id(
-            entity_id=self.container_id, suppress_exception=True)
-        if self.container:
-            d = self.get_acl_dict_for_user(req, self.container.container_acls)
-            d['project_id'] = self.container.project.external_id
-            d['creator_id'] = self.container.creator_id
-            return 'container', d
-        else:
-            return None, None
+        d = self.get_acl_dict_for_user(req, self.container.container_acls)
+        d['project_id'] = self.container.project.external_id
+        d['creator_id'] = self.container.creator_id
+        return 'container', d
 
     @pecan.expose(generic=True)
     def index(self, **kwargs):
@@ -65,13 +61,6 @@ class ContainerController(controllers.ACLMixin):
     @controllers.handle_exceptions(u._('Container retrieval'))
     @controllers.enforce_rbac('container:get')
     def on_get(self, external_project_id):
-        # self.container is not present in case of unauthenticated pipeline
-        if not self.container:
-            self.container = self.container_repo.get_container_by_id(
-                entity_id=self.container_id, suppress_exception=True)
-            if not self.container:
-                container_not_found()
-
         dict_fields = self.container.to_dict_fields()
 
         for secret_ref in dict_fields['secret_refs']:
@@ -118,7 +107,12 @@ class ContainersController(controllers.ACLMixin):
 
     @pecan.expose()
     def _lookup(self, container_id, *remainder):
-        return ContainerController(container_id), remainder
+        container = self.container_repo.get_container_by_id(
+            entity_id=container_id, suppress_exception=True)
+        if not container:
+            container_not_found()
+
+        return ContainerController(container), remainder
 
     @pecan.expose(generic=True)
     def index(self, **kwargs):

barbican/tests/api/test_resources.py

@@ -784,11 +784,15 @@ class WhenCreatingConsumersUsingConsumersResource(FunctionalTest):
         self.setup_project_repository_mock(self.project_repo)
 
         # Set up mocked container
-        self.container = create_container(id_ref='id1')
+        self.container = create_container(
+            id_ref='id1',
+            project_id=self.project_internal_id,
+            external_project_id=self.external_project_id)
 
         # 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 secret repo
@@ -877,7 +881,10 @@ class WhenGettingOrDeletingConsumersUsingConsumerResource(FunctionalTest):
         self.setup_project_repository_mock(self.project_repo)
 
         # Set up mocked container
-        self.container = create_container(id_ref='id1')
+        self.container = create_container(
+            id_ref='id1',
+            project_id=self.project_internal_id,
+            external_project_id=self.external_project_id)
 
         # Set up mocked consumers
         self.consumer = create_consumer(self.container.id, id_ref='id2')
@@ -891,6 +898,7 @@ 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)
 
         # Set up mocked container consumer repo
@@ -1078,7 +1086,10 @@ class WhenPerformingUnallowedOperationsOnConsumers(FunctionalTest):
         self.setup_project_repository_mock(self.project_repo)
 
         # Set up mocked container
-        self.container = create_container(id_ref='id1')
+        self.container = create_container(
+            id_ref='id1',
+            project_id=self.project_internal_id,
+            external_project_id=self.external_project_id)
 
         # Set up mocked container consumers
         self.consumer = create_consumer(self.container.id, id_ref='id2')

barbican/tests/api/test_resources_policy.py

@@ -644,6 +644,7 @@ class WhenTestingContainerResource(BaseTestCase):
             creator_only=False, user_ids=[self.user_id, 'anyRandomId'])
         self.acl_list = [acl_read]
         container = mock.MagicMock()
+        container.id = self.container_id
         container.container_acls.__iter__.return_value = self.acl_list
         container.project.external_id = self.external_project_id
         container.creator_id = self.creator_user_id
@@ -652,7 +653,7 @@ class WhenTestingContainerResource(BaseTestCase):
 
         self.setup_container_repository_mock(self.container_repo)
 
-        self.resource = ContainerResource(self.container_id)
+        self.resource = ContainerResource(container)
 
     def test_rules_should_be_loaded(self):
         self.assertIsNotNone(self.policy_enforcer.rules)