Simplified permissions for projects.

Instead of the additional 'is_private' field, we now just use
the permission system and set/remove world GET permissions.

'is_private' is still kept for backward compatibility and possibly
easy querying for public projects, and is always set based on
world GET permissions.

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
+    # Now re-fetch the project, as both the initial document and the returned
-    # result do not contain the same etag as the database.
+    # result do not contain the same etag as the database. This also updates
-    document = current_app.data.driver.db['projects'].find_one(project['_id'],
+    # other fields set by hooks.
-                                                               projection={'_etag': 1})
+    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)

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)

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 user is None:
-    if current_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)

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.
+        # GET the project from the URL in the Location header to see if that works too.
-        resp = self.client.get(resp.headers['Location'])
+        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.
+        # Create public test project.
-        project_info = self._create_user_and_project([u'subscriber'])
+        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)
+        self._create_user_with_token([u'subscriber'], 'other-token',
-        project = json.loads(resp.data)
+                                     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)