diff --git a/pillar/application/modules/projects.py b/pillar/application/modules/projects.py index d7ab9ab7..f267cc5c 100644 --- a/pillar/application/modules/projects.py +++ b/pillar/application/modules/projects.py @@ -11,11 +11,8 @@ from application.utils import remove_private_keys, authorization, jsonify from application.utils.gcs import GoogleCloudStorageBucket from application.utils.authorization import user_has_role, check_permissions from manage_extra.node_types.asset import node_type_asset -from manage_extra.node_types.blog import node_type_blog from manage_extra.node_types.comment import node_type_comment from manage_extra.node_types.group import node_type_group -from manage_extra.node_types.page import node_type_page -from manage_extra.node_types.post import node_type_post log = logging.getLogger(__name__) blueprint = Blueprint('projects', __name__) @@ -36,7 +33,23 @@ def before_inserting_projects(items): item.pop('url', None) +def override_is_private_field(project, original): + """Override the 'is_private' property from the world permissions. + + :param project: the project, which will be updated + """ + + world_perms = project['permissions'].get('world', []) + project['is_private'] = 'GET' not in world_perms + + +def before_inserting_override_is_private_field(projects): + for project in projects: + override_is_private_field(project, None) + + def before_edit_check_permissions(document, original): + # Allow admin users to do whatever they want. # TODO: possibly move this into the check_permissions function. if user_has_role(u'admin'): @@ -108,7 +121,7 @@ def after_inserting_project(project, db_user): return abort_with_error(status) admin_group_id = result['_id'] - log.info('Created admin group %s for project %s', admin_group_id, project_id) + log.debug('Created admin group %s for project %s', admin_group_id, project_id) # Assign the current user to the group db_user.setdefault('groups', []).append(admin_group_id) @@ -121,8 +134,10 @@ def after_inserting_project(project, db_user): log.debug('Made user %s member of group %s', user_id, admin_group_id) # Assign the group to the project with admin rights + is_admin = authorization.is_admin(db_user) + world_permissions = ['GET'] if is_admin else [] permissions = { - 'world': ['GET'], + 'world': world_permissions, 'users': [], 'groups': [ {'group': admin_group_id, @@ -143,7 +158,7 @@ def after_inserting_project(project, db_user): with_permissions(node_type_comment)] # Allow admin users to use whatever url they want. - if not user_has_role(u'admin') or not project.get('url'): + if not is_admin or not project.get('url'): project['url'] = "p-{!s}".format(project_id) # Initialize storage page (defaults to GCS) @@ -192,10 +207,10 @@ def _create_new_project(project_name, user_id, overrides): return abort_with_error(status) project.update(result) - # Now re-fetch the etag, as both the initial document and the returned - # result do not contain the same etag as the database. - document = current_app.data.driver.db['projects'].find_one(project['_id'], - projection={'_etag': 1}) + # Now re-fetch the project, as both the initial document and the returned + # result do not contain the same etag as the database. This also updates + # other fields set by hooks. + document = current_app.data.driver.db['projects'].find_one(project['_id']) project.update(document) log.info('Created project %s for user %s', project['_id'], user_id) @@ -289,11 +304,14 @@ def abort_with_error(status): def setup_app(app, url_prefix): + app.on_replace_projects += override_is_private_field app.on_replace_projects += before_edit_check_permissions app.on_replace_projects += protect_sensitive_fields + app.on_update_projects += override_is_private_field app.on_update_projects += before_edit_check_permissions app.on_update_projects += protect_sensitive_fields app.on_delete_item_projects += before_delete_project + app.on_insert_projects += before_inserting_override_is_private_field app.on_insert_projects += before_inserting_projects app.on_inserted_projects += after_inserting_projects app.register_blueprint(blueprint, url_prefix=url_prefix) diff --git a/pillar/application/utils/__init__.py b/pillar/application/utils/__init__.py index 2bd67608..85917c66 100644 --- a/pillar/application/utils/__init__.py +++ b/pillar/application/utils/__init__.py @@ -40,10 +40,15 @@ class PillarJSONEncoder(json.JSONEncoder): return json.JSONEncoder.default(self, obj) +def dumps(mongo_doc, **kwargs): + """json.dumps() for MongoDB documents.""" + return json.dumps(mongo_doc, cls=PillarJSONEncoder, **kwargs) + + def jsonify(mongo_doc, status=200, headers=None): """JSonifies a Mongo document into a Flask response object.""" - return current_app.response_class(json.dumps(mongo_doc, cls=PillarJSONEncoder), + return current_app.response_class(dumps(mongo_doc), mimetype='application/json', status=status, headers=headers) diff --git a/pillar/application/utils/authorization.py b/pillar/application/utils/authorization.py index d03601f4..8e77b6e8 100644 --- a/pillar/application/utils/authorization.py +++ b/pillar/application/utils/authorization.py @@ -112,13 +112,19 @@ def require_login(require_roles=set()): return decorator -def user_has_role(role): +def user_has_role(role, user=None): """Returns True iff the user is logged in and has the given role.""" - current_user = g.get('current_user') - if current_user is None: + if user is None: + user = g.get('current_user') + + if user is None: return False - return role in current_user['roles'] + return role in user['roles'] +def is_admin(user): + """Returns True iff the given user has the admin role.""" + + return user_has_role(u'admin', user) diff --git a/tests/test_project_management.py b/tests/test_project_management.py index 4548f59f..bc5dd87b 100644 --- a/tests/test_project_management.py +++ b/tests/test_project_management.py @@ -26,6 +26,15 @@ 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') + + self.assertEqual(201, resp.status_code, resp.data) + project = json.loads(resp.data) + + return project + class ProjectCreationTest(AbstractProjectTest): def test_project_creation_wrong_role(self): @@ -44,7 +53,8 @@ class ProjectCreationTest(AbstractProjectTest): resp = self._create_project(u'Prøject El Niño', 'token') self.assertEqual(201, resp.status_code) - # The response of a POST is not the entire document, just some _xxx fields. + # The response of a POST is the entire project, but we'll test a GET on + # the returned Location nevertheless. project_info = json.loads(resp.data.decode('utf-8')) project_id = project_info['_id'] @@ -52,8 +62,9 @@ class ProjectCreationTest(AbstractProjectTest): self.assertEqual('http://localhost/projects/%s' % project_id, resp.headers['Location']) - # Actually get the project. - resp = self.client.get(resp.headers['Location']) + # GET the project from the URL in the Location header to see if that works too. + auth_header = {'Authorization': self.make_header('token')} + resp = self.client.get(resp.headers['Location'], headers=auth_header) project = json.loads(resp.data.decode('utf-8')) project_id = project['_id'] @@ -64,7 +75,7 @@ class ProjectCreationTest(AbstractProjectTest): self.assertEqual(1, len(project['permissions']['groups'])) # Check the etag - resp = self.client.get('/projects/%s' % project_id) + resp = self.client.get('/projects/%s' % project_id, headers=auth_header) from_db = json.loads(resp.data) self.assertEqual(from_db['_etag'], project['_etag']) @@ -81,6 +92,18 @@ class ProjectCreationTest(AbstractProjectTest): self.assertEqual(str(project_id), group['name']) self.assertIn(group_id, db_user['groups']) + def test_project_creation_access_admin(self): + """Admin-created projects should be public""" + + proj = self._create_user_and_project(roles={u'admin'}) + self.assertEqual(['GET'], proj['permissions']['world']) + + def test_project_creation_access_subscriber(self): + """Subscriber-created projects should be private""" + + proj = self._create_user_and_project(roles={u'subscriber'}) + self.assertEqual([], proj['permissions']['world']) + class ProjectEditTest(AbstractProjectTest): def test_editing_as_subscriber(self): @@ -92,7 +115,8 @@ class ProjectEditTest(AbstractProjectTest): project_info = self._create_user_and_project([u'subscriber']) project_url = '/projects/%(_id)s' % project_info - resp = self.client.get(project_url) + resp = self.client.get(project_url, + headers={'Authorization': self.make_header('token')}) project = json.loads(resp.data.decode('utf-8')) # Create another user we can try and assign the project to. @@ -127,7 +151,8 @@ class ProjectEditTest(AbstractProjectTest): # Re-fetch from database to see which fields actually made it there. # equal to put_project -> changed in DB # equal to project -> not changed in DB - resp = self.client.get(project_url) + resp = self.client.get(project_url, + headers={'Authorization': self.make_header('token')}) db_proj = json.loads(resp.data) self.assertEqual(project['url'], db_proj['url']) self.assertEqual(put_project['description'], db_proj['description']) @@ -238,12 +263,13 @@ class ProjectEditTest(AbstractProjectTest): self.assertEqual(403, resp.status_code, resp.data) def test_delete_by_admin(self): - # Create test project. - project_info = self._create_user_and_project([u'subscriber']) + # Create public test project. + project_info = self._create_user_and_project([u'admin']) project_id = project_info['_id'] project_url = '/projects/%s' % project_id - # Create test user. + # Create admin user that doesn't own the project, to check that + # non-owner admins can delete projects too. self._create_user_with_token(['admin'], 'admin-token', user_id='cafef00dbeef') # Admin user should be able to DELETE. @@ -262,6 +288,7 @@ class ProjectEditTest(AbstractProjectTest): self.assertTrue(db_proj['_deleted']) # Querying for deleted projects should include it. + # TODO: limit this to admin users only. # Also see http://python-eve.org/features.html#soft-delete projection = json.dumps({'name': 1, 'permissions': 1}) where = json.dumps({'_deleted': True}) # MUST be True, 1 does not work. @@ -294,19 +321,118 @@ class ProjectEditTest(AbstractProjectTest): 'If-Match': project_info['_etag']}) self.assertEqual(204, resp.status_code, resp.data) - def _create_user_and_project(self, roles): - self._create_user_with_token(roles, 'token') + +class ProjectNodeAccess(AbstractProjectTest): + def setUp(self, **kwargs): + super(ProjectNodeAccess, self).setUp(**kwargs) + + from application.utils import PillarJSONEncoder + + # Project is created by regular subscriber, so should be private. + self.user_id = self._create_user_with_token([u'subscriber'], 'token') resp = self._create_project(u'Prøject El Niño', 'token') + self.assertEqual(201, resp.status_code) + self.assertEqual('application/json', resp.mimetype) + self.project = json.loads(resp.data) + self.project_id = ObjectId(self.project['_id']) - self.assertEqual(201, resp.status_code, resp.data) - project = json.loads(resp.data) + self._create_user_with_token([u'subscriber'], 'other-token', + user_id='deadbeefdeadbeefcafef00d') - return project + self.test_node = { + 'description': '', + 'node_type': 'asset', + 'user': self.user_id, + 'properties': { + 'status': 'published', + 'content_type': 'image', + }, + 'name': 'Micak is a cool cat', + 'project': self.project_id, + } + + # Add a node to the project + resp = self.client.post('/nodes', + headers={'Authorization': self.make_header('token'), + 'Content-Type': 'application/json'}, + data=json.dumps(self.test_node, cls=PillarJSONEncoder), + ) + self.assertEqual(201, resp.status_code, (resp.status_code, resp.data)) + self.node_info = json.loads(resp.data) + self.node_id = self.node_info['_id'] + self.node_url = '/nodes/%s' % self.node_id + + def test_node_access(self): + """Getting nodes should adhere to project access rules.""" + + # Getting the node as the project owner should work. + resp = self.client.get(self.node_url, + headers={'Authorization': self.make_header('token')}) + self.assertEqual(200, resp.status_code, (resp.status_code, resp.data)) + + # Getting the node as an outsider should not work. + resp = self.client.get(self.node_url, + headers={'Authorization': self.make_header('other-token')}) + self.assertEqual(403, resp.status_code, (resp.status_code, resp.data)) + + def test_node_resource_access(self): + # The owner of the project should get the node. + resp = self.client.get('/nodes', + headers={'Authorization': self.make_header('token')}) + self.assertEqual(200, resp.status_code, (resp.status_code, resp.data)) + listed_nodes = json.loads(resp.data)['_items'] + self.assertEquals(self.node_id, listed_nodes[0]['_id']) + + # Listing all nodes should not include nodes from private projects. + resp = self.client.get('/nodes', + headers={'Authorization': self.make_header('other-token')}) + self.assertEqual(403, resp.status_code, (resp.status_code, resp.data)) + + def test_is_private_updated_by_world_permissions(self): + """For backward compatibility, is_private should reflect absence of world-GET""" + + from application.utils import remove_private_keys, dumps + + project_url = '/projects/%s' % self.project_id + put_project = remove_private_keys(self.project) + + # Create admin user. + self._create_user_with_token(['admin'], 'admin-token', user_id='cafef00dbeef') + + # Make the project public + put_project['permissions']['world'] = ['GET'] # make public + put_project['is_private'] = True # This should be overridden. + + resp = self.client.put(project_url, + data=dumps(put_project), + headers={'Authorization': self.make_header('admin-token'), + 'Content-Type': 'application/json', + 'If-Match': self.project['_etag']}) + self.assertEqual(200, resp.status_code, resp.data) + + with self.app.test_request_context(): + projects = self.app.data.driver.db['projects'] + db_proj = projects.find_one(self.project_id) + self.assertEqual(['GET'], db_proj['permissions']['world']) + self.assertFalse(db_proj['is_private']) + + # Make the project private + put_project['permissions']['world'] = [] + + resp = self.client.put(project_url, + data=dumps(put_project), + headers={'Authorization': self.make_header('admin-token'), + 'Content-Type': 'application/json', + 'If-Match': db_proj['_etag']}) + self.assertEqual(200, resp.status_code, resp.data) + + with self.app.test_request_context(): + projects = self.app.data.driver.db['projects'] + db_proj = projects.find_one(self.project_id) + self.assertEqual([], db_proj['permissions']['world']) + self.assertTrue(db_proj['is_private']) def test_add_remove_user(self): - # Create test project - project_info = self._create_user_and_project([u'subscriber']) - project_id = project_info['_id'] project_add_user_url = '/p/users' # Create another user we can try to share the project with @@ -314,9 +440,9 @@ class ProjectEditTest(AbstractProjectTest): self._create_user_with_token(['subscriber'], 'other-token', user_id=other_user_id) - # Make request payload + # Use our API to add user to group payload = { - 'project_id': project_id, + 'project_id': self.project_id, 'user_id': other_user_id, 'action': 'add'} @@ -325,7 +451,7 @@ class ProjectEditTest(AbstractProjectTest): content_type='application/json', headers={ 'Authorization': self.make_header('token'), - 'If-Match': project_info['_etag']}) + 'If-Match': self.project['_etag']}) self.assertEqual(200, resp.status_code, resp.data) with self.app.test_request_context(): @@ -341,7 +467,7 @@ class ProjectEditTest(AbstractProjectTest): admin_group = groups.find_one({'_id': admin_group_id}) # Check if admin group name matches - self.assertEqual(admin_group['name'], str(project_info['_id'])) + self.assertEqual(admin_group['name'], str(self.project['_id'])) # Update payload to remove the user we just added payload['action'] = 'remove' @@ -351,6 +477,6 @@ class ProjectEditTest(AbstractProjectTest): content_type='application/json', headers={ 'Authorization': self.make_header('token'), - 'If-Match': project_info['_etag']}) + 'If-Match': self.project['_etag']}) self.assertEqual(200, resp.status_code, resp.data)