From 5b2d7447e658118c458d2e250079a6080cfaf675 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Wed, 11 May 2016 11:41:19 +0200 Subject: [PATCH] Projects: limit returned projects to allowable projects. Before this, if there was any project returned by a query on /projects that the user did not have access to, a 403 would be returned. Now we just don't include that project in the result. --- pillar/application/modules/projects.py | 7 +++- pillar/application/utils/authorization.py | 28 +++++++++++-- tests/test_project_management.py | 48 +++++++++++++++++++++-- 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/pillar/application/modules/projects.py b/pillar/application/modules/projects.py index c66bdd20..b5b0f357 100644 --- a/pillar/application/modules/projects.py +++ b/pillar/application/modules/projects.py @@ -367,8 +367,11 @@ def before_returning_project_permissions(response): def before_returning_project_resource_permissions(response): - for item in response['_items']: - check_permissions('projects', item, 'GET', append_allowed_methods=True) + # Return only those projects the user has access to. + allow = [project for project in response['_items'] + if authorization.has_permissions('projects', project, + 'GET', append_allowed_methods=True)] + response['_items'] = allow def project_node_type_has_method(response): diff --git a/pillar/application/utils/authorization.py b/pillar/application/utils/authorization.py index 4490a586..3929b677 100644 --- a/pillar/application/utils/authorization.py +++ b/pillar/application/utils/authorization.py @@ -28,6 +28,28 @@ def check_permissions(collection_name, resource, method, append_allowed_methods= :type check_node_type: str """ + if not has_permissions(collection_name, resource, method, append_allowed_methods, + check_node_type): + abort(403) + + +def has_permissions(collection_name, resource, method, append_allowed_methods=False, + check_node_type=None): + """Check user permissions to access a node. We look up node permissions from + world to groups to users and match them with the computed user permissions. + + :param collection_name: name of the collection the resource comes from. + :param resource: resource from MongoDB + :type resource: dict + :param method: name of the requested HTTP method + :param append_allowed_methods: whether to return the list of allowed methods + in the resource. Only valid when method='GET'. + :param check_node_type: node type to check. Only valid when collection_name='projects'. + :type check_node_type: str + :returns: True if the user has access, False otherwise. + :rtype: bool + """ + # Check some input values. if collection_name not in CHECK_PERMISSIONS_IMPLEMENTED_FOR: raise ValueError('check_permission only implemented for %s, not for %s', @@ -44,7 +66,7 @@ def check_permissions(collection_name, resource, method, append_allowed_methods= if not computed_permissions: log.info('No permissions available to compute for %s on resource %r', method, resource.get('node_type', resource)) - abort(403) + return False # Accumulate allowed methods from the user, group and world level. allowed_methods = set() @@ -77,9 +99,9 @@ def check_permissions(collection_name, resource, method, append_allowed_methods= else: assign_to = resource assign_to['allowed_methods'] = list(set(allowed_methods)) - return + return True - abort(403) + return False def compute_aggr_permissions(collection_name, resource, check_node_type): diff --git a/tests/test_project_management.py b/tests/test_project_management.py index 0634ae66..4ba17183 100644 --- a/tests/test_project_management.py +++ b/tests/test_project_management.py @@ -26,9 +26,10 @@ class AbstractProjectTest(AbstractPillarTest): data={'project_name': project_name}) return resp - def _create_user_and_project(self, roles): - self._create_user_with_token(roles, 'token') - resp = self._create_project(u'Prøject El Niño', 'token') + def _create_user_and_project(self, roles, user_id='cafef00df00df00df00df00d', token='token', + project_name=u'Prøject El Niño'): + self._create_user_with_token(roles, token, user_id=user_id) + resp = self._create_project(project_name, token) self.assertEqual(201, resp.status_code, resp.data) project = json.loads(resp.data) @@ -112,6 +113,47 @@ class ProjectCreationTest(AbstractProjectTest): self.assertEqual([], db_proj['permissions']['world']) self.assertTrue(db_proj['is_private']) + def test_project_list(self): + """Test that we get an empty list when querying for non-existing projects, instead of 403""" + + proj_a = self._create_user_and_project(user_id=24 * 'a', + roles={u'subscriber'}, + project_name=u'Prøject A', + token='token-a') + proj_b = self._create_user_and_project(user_id=24 * 'b', + roles={u'subscriber'}, + project_name=u'Prøject B', + token='token-b') + + # Assertion: each user must have access to their own project. + resp = self.client.get('/projects/%s' % proj_a['_id'], + headers={'Authorization': self.make_header('token-a')}) + self.assertEqual(200, resp.status_code, resp.data) + resp = self.client.get('/projects/%s' % proj_b['_id'], + headers={'Authorization': self.make_header('token-b')}) + self.assertEqual(200, resp.status_code, resp.data) + + # Getting a project list should return projects you have access to. + resp = self.client.get('/projects', + headers={'Authorization': self.make_header('token-a')}) + self.assertEqual(200, resp.status_code) + proj_list = json.loads(resp.data) + self.assertEqual([u'Prøject A'], [p['name'] for p in proj_list['_items']]) + + resp = self.client.get('/projects', + headers={'Authorization': self.make_header('token-b')}) + self.assertEqual(200, resp.status_code) + proj_list = json.loads(resp.data) + self.assertEqual([u'Prøject B'], [p['name'] for p in proj_list['_items']]) + + # No access to anything for user C, should result in empty list. + self._create_user_with_token(roles={u'subscriber'}, token='token-c', user_id=12 * 'c') + resp = self.client.get('/projects', + headers={'Authorization': self.make_header('token-c')}) + self.assertEqual(200, resp.status_code) + proj_list = json.loads(resp.data) + self.assertEqual([], proj_list['_items']) + class ProjectEditTest(AbstractProjectTest): def test_editing_as_subscriber(self):