From 21efb841b052442f2bc4c8382c4e0088feeab6ae Mon Sep 17 00:00:00 2001 From: Thomas Herve Date: Wed, 8 Apr 2015 12:01:22 +0200 Subject: [PATCH] Return container not found before ACL checks This fixes a recent regression where one user would get a 403 when trying to retrieve a unknown container, instead of a 404. Change-Id: I59c6b806591ca4c943615c3b528752ef8ba1e0dd Closes-Bug: #1441555 --- barbican/api/controllers/containers.py | 36 +++++++++------------ barbican/tests/api/test_resources.py | 17 ++++++++-- barbican/tests/api/test_resources_policy.py | 3 +- 3 files changed, 31 insertions(+), 25 deletions(-) diff --git a/barbican/api/controllers/containers.py b/barbican/api/controllers/containers.py index a7d994a57..83d1f39bc 100644 --- a/barbican/api/controllers/containers.py +++ b/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): diff --git a/barbican/tests/api/test_resources.py b/barbican/tests/api/test_resources.py index dbb5aba96..684b70f30 100644 --- a/barbican/tests/api/test_resources.py +++ b/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') diff --git a/barbican/tests/api/test_resources_policy.py b/barbican/tests/api/test_resources_policy.py index 29fb0541d..a2e5f2848 100644 --- a/barbican/tests/api/test_resources_policy.py +++ b/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)