openstack
/
barbican
Code Issues Proposed changes

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
This commit is contained in:
Pan 2016-08-25 12:56:07 -04:00
parent 6f46088a65
commit 38ecf5b51f
4 changed files with 111 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
barbican/api/controllers/consumers.py
View File

@ -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)

95
barbican/tests/api/test_resources.py
View File

@ -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)

8
etc/barbican/policy.json
View File

@ -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",

8
functionaltests/api/v1/functional/test_acls.py
View File

@ -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},
}