From 589992632dce68789b2b4d406268279ae38f9374 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 26 Jul 2017 11:22:37 -0500 Subject: [PATCH 01/17] loggin projects --- api/web/request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/web/request.py b/api/web/request.py index 7421eef65..f5dff9f3c 100644 --- a/api/web/request.py +++ b/api/web/request.py @@ -61,7 +61,7 @@ def log_user_access_from_request(self, *args, **kwargs): cont_id = kwargs.get(cont_id_kwarg) # Only log view_container events when the container is a session - if access_type is AccessType.view_container and cont_name not in ['sessions', 'session']: + if access_type is AccessType.view_container and cont_name not in ['sessions', 'session', 'projects', 'project']: return result self.log_user_access(access_type, cont_name, cont_id) From 1f0ca26c43d9ac72feb5badfdcd42683c9bf4f5c Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 26 Jul 2017 13:12:09 -0500 Subject: [PATCH 02/17] new no-phi-ro access level --- api/auth/__init__.py | 4 ++++ api/auth/containerauth.py | 17 +++++++++++++---- api/auth/groupauth.py | 2 +- api/auth/listauth.py | 6 +++--- api/web/request.py | 2 +- raml/schemas/definitions/permission.json | 2 +- tests/integration_tests/python/conftest.py | 5 +++-- .../integration_tests/python/test_access_log.py | 4 ++-- tests/integration_tests/python/test_groups.py | 1 + 9 files changed, 29 insertions(+), 14 deletions(-) diff --git a/api/auth/__init__.py b/api/auth/__init__.py index d2e185b95..3d0ab02ba 100644 --- a/api/auth/__init__.py +++ b/api/auth/__init__.py @@ -2,6 +2,10 @@ from ..web.errors import APIPermissionException PERMISSIONS = [ + { + 'rid': 'no-phi-ro', + 'name': 'Read-Only (No PHI)' + }, { 'rid': 'ro', 'name': 'Read-Only', diff --git a/api/auth/containerauth.py b/api/auth/containerauth.py index b6265bc2b..fabd6f817 100644 --- a/api/auth/containerauth.py +++ b/api/auth/containerauth.py @@ -3,6 +3,7 @@ """ from . import _get_access, INTEGER_PERMISSIONS +from ..dao import noop def default_container(handler, container=None, target_parent_container=None): @@ -18,7 +19,7 @@ def f(method, _id=None, payload=None, unset_payload=None, recursive=False, r_pay if method == 'GET' and container.get('public', False): has_access = True elif method == 'GET': - has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['ro'] + has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['no-phi-ro'] elif method == 'POST': required_perm = 'rw' if target_parent_container.get('cont_name') == 'group': @@ -47,7 +48,15 @@ def f(method, _id=None, payload=None, unset_payload=None, recursive=False, r_pay has_access = False if has_access: - return exec_op(method, _id=_id, payload=payload, unset_payload=unset_payload, recursive=recursive, r_payload=r_payload, replace_metadata=replace_metadata, projection=projection) + result = exec_op(method, _id=_id, payload=payload, unset_payload=unset_payload, recursive=recursive, r_payload=r_payload, replace_metadata=replace_metadata, projection=projection) + + if method == 'GET' and exec_op is not noop: + handler.phi = _get_access(handler.uid, container) > INTEGER_PERMISSIONS['no-phi-ro'] + if not handler.phi: + if handler.is_true('phi'): + handler.abort(403, "User not authorized to view PHI fields.") + result = phi_scrub(result) + return result else: error_msg = 'user not authorized to perform a {} operation on the container.'.format(method) if additional_error_msg: @@ -67,7 +76,7 @@ def f(method, _id=None, payload = None): if method == 'GET' and container.get('public', False): has_access = True elif method == 'GET': - has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['ro'] + has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['no-phi-ro'] elif method == 'DELETE': has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['admin'] elif method == 'POST': @@ -92,7 +101,7 @@ def f(method, _id=None, payload=None): if method == 'GET' and parent_container.get('public', False): has_access = True elif method == 'GET': - has_access = access >= INTEGER_PERMISSIONS['ro'] + has_access = access >= INTEGER_PERMISSIONS['no-phi-ro'] elif method in ['POST', 'PUT', 'DELETE']: has_access = access >= INTEGER_PERMISSIONS['rw'] else: diff --git a/api/auth/groupauth.py b/api/auth/groupauth.py index fd98052d5..a32aee2be 100644 --- a/api/auth/groupauth.py +++ b/api/auth/groupauth.py @@ -17,7 +17,7 @@ def f(method, _id=None, query=None, payload=None, projection=None): handler.abort(403, 'not allowed to perform operation') elif _get_access(handler.uid, group) >= INTEGER_PERMISSIONS['admin']: pass - elif method == 'GET' and _get_access(handler.uid, group) >= INTEGER_PERMISSIONS['ro']: + elif method == 'GET' and _get_access(handler.uid, group) >= INTEGER_PERMISSIONS['no-phi-ro']: pass else: handler.abort(403, 'not allowed to perform operation') diff --git a/api/auth/listauth.py b/api/auth/listauth.py index 9253bb5ed..a3daf00f6 100644 --- a/api/auth/listauth.py +++ b/api/auth/listauth.py @@ -23,7 +23,7 @@ def f(method, _id, query_params=None, payload=None, exclude_params=None): if method == 'GET' and container.get('public', False): min_access = -1 elif method == 'GET': - min_access = INTEGER_PERMISSIONS['ro'] + min_access = INTEGER_PERMISSIONS['no-phi-ro'] elif method in ['POST', 'PUT', 'DELETE']: min_access = INTEGER_PERMISSIONS['rw'] else: @@ -59,7 +59,7 @@ def group_tags_sublist(handler, container): access = _get_access(handler.uid, container) def g(exec_op): def f(method, _id, query_params = None, payload = None, exclude_params=None): - if method == 'GET' and access >= INTEGER_PERMISSIONS['ro']: + if method == 'GET' and access >= INTEGER_PERMISSIONS['no-phi-ro']: return exec_op(method, _id, query_params, payload, exclude_params) elif access >= INTEGER_PERMISSIONS['rw']: return exec_op(method, _id, query_params, payload, exclude_params) @@ -95,7 +95,7 @@ def f(method, _id, query_params = None, payload = None, exclude_params=None): pass elif method == 'POST' and access >= INTEGER_PERMISSIONS['rw'] and payload['user'] == handler.uid: pass - elif method == 'GET' and (access >= INTEGER_PERMISSIONS['ro'] or container.get('public')): + elif method == 'GET' and (access >= INTEGER_PERMISSIONS['no-phi-ro'] or container.get('public')): pass elif method in ['GET', 'DELETE', 'PUT'] and container['notes'][0]['user'] == handler.uid: pass diff --git a/api/web/request.py b/api/web/request.py index f5dff9f3c..299b81410 100644 --- a/api/web/request.py +++ b/api/web/request.py @@ -61,7 +61,7 @@ def log_user_access_from_request(self, *args, **kwargs): cont_id = kwargs.get(cont_id_kwarg) # Only log view_container events when the container is a session - if access_type is AccessType.view_container and cont_name not in ['sessions', 'session', 'projects', 'project']: + if access_type is AccessType.view_container and cont_name not in ['sessions', 'session', 'project', 'projects']: return result self.log_user_access(access_type, cont_name, cont_id) diff --git a/raml/schemas/definitions/permission.json b/raml/schemas/definitions/permission.json index 490aafa51..a338d493f 100644 --- a/raml/schemas/definitions/permission.json +++ b/raml/schemas/definitions/permission.json @@ -3,7 +3,7 @@ "type": "object", "definitions": { "_id": { "type": "string" }, - "access": { "enum": ["ro", "rw", "admin"] }, + "access": { "enum": ["no-phi-ro", "ro", "rw", "admin"] }, "permission":{ "type":"object", "properties":{ diff --git a/tests/integration_tests/python/conftest.py b/tests/integration_tests/python/conftest.py index 7cd2593d5..e84f98ea5 100644 --- a/tests/integration_tests/python/conftest.py +++ b/tests/integration_tests/python/conftest.py @@ -219,6 +219,7 @@ class DataBuilder(object): 'session': 'project', 'acquisition': 'session', } + parent_to_child = {parent: child for child, parent in child_to_parent.items()} def __init__(self, session, randstr=lambda: binascii.hexlify(os.urandom(10))): @@ -254,8 +255,8 @@ def create(self, resource, **kwargs): payload['gear']['name'] = self.randstr() # add missing label fields using randstr - # such fields are: [project.label, session.label, acquisition.label] - if resource in self.child_to_parent and 'label' not in payload: + # such fields are: [project.label, session.label, acquisition.label, group.label] + if resource in ['project', 'group', 'groups', 'session', 'acquisition'] and 'label' not in payload: payload['label'] = self.randstr() # add missing parent container when creating child container diff --git a/tests/integration_tests/python/test_access_log.py b/tests/integration_tests/python/test_access_log.py index 083f5f791..29d66841a 100644 --- a/tests/integration_tests/python/test_access_log.py +++ b/tests/integration_tests/python/test_access_log.py @@ -265,8 +265,8 @@ def test_access_log_fails(data_builder, as_admin, log_db): r = as_admin.delete('/projects/' + project + '/files/' + file_name) assert r.status_code == 500 + log_db.command('collMod', 'access_log', validator={}, validationLevel='strict') + r = as_admin.get('/projects/' + project) assert r.ok assert r.json()['files'] - - log_db.command('collMod', 'access_log', validator={}, validationLevel='strict') diff --git a/tests/integration_tests/python/test_groups.py b/tests/integration_tests/python/test_groups.py index 9ebcddf1f..94cfe620e 100644 --- a/tests/integration_tests/python/test_groups.py +++ b/tests/integration_tests/python/test_groups.py @@ -10,6 +10,7 @@ def test_groups(as_user, as_admin, data_builder): # Able to find new group r = as_admin.get('/groups/' + group) assert r.ok + assert r.json()['label'] != None first_modified = r.json()['modified'] # Test to make sure that list of roles nor name exists in a newly created group From b86460f0d7dafe913655279d80ed57657e9765c9 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 26 Jul 2017 16:40:43 -0500 Subject: [PATCH 03/17] phi flag and logging implemented on containers --- api/auth/containerauth.py | 48 +++++++++++++++- api/handlers/collectionshandler.py | 2 +- api/handlers/containerhandler.py | 8 +++ api/handlers/refererhandler.py | 3 + api/web/request.py | 2 +- .../python/test_containers.py | 56 +++++++++++++++++++ 6 files changed, 114 insertions(+), 5 deletions(-) diff --git a/api/auth/containerauth.py b/api/auth/containerauth.py index fabd6f817..0093e0d56 100644 --- a/api/auth/containerauth.py +++ b/api/auth/containerauth.py @@ -1,10 +1,13 @@ """ Purpose of this module is to define all the permissions checker decorators for the ContainerHandler classes. """ - +from copy import deepcopy from . import _get_access, INTEGER_PERMISSIONS from ..dao import noop +PHI_FIELDS = {'info': '***', 'subject': {'firstname': "***", 'lastname': "***", 'sex': "***", + 'age': "***", 'race': "***", 'ethnicity': "***", 'info': "***"}, 'tags': "***"} +SCRUB = "***" def default_container(handler, container=None, target_parent_container=None): """ @@ -87,7 +90,15 @@ def f(method, _id=None, payload = None): has_access = False if has_access: - return exec_op(method, _id=_id, payload=payload) + result = exec_op(method, _id=_id, payload=payload) + + if method == 'GET' and exec_op is not noop: + handler.phi = _get_access(handler.uid, container) > INTEGER_PERMISSIONS['no-phi-ro'] + if not handler.phi: + if handler.is_true('phi'): + handler.abort(403, "User not authorized to view PHI fields.") + result = phi_scrub(result) + return result else: handler.abort(403, 'user not authorized to perform a {} operation on the container'.format(method)) return f @@ -108,7 +119,14 @@ def f(method, _id=None, payload=None): has_access = False if has_access: - return exec_op(method, _id=_id, payload=payload) + result = exec_op(method, _id=_id, payload=payload) + + if method == 'GET' and exec_op is not noop: + if not handler.phi_get_access(handler.uid, parent_container) > INTEGER_PERMISSIONS['no-phi-ro']: + if handler.is_true('phi'): + handler.abort(403, "User not authorized to view PHI fields.") + result = phi_scrub(result) + return result else: handler.abort(403, 'user not authorized to perform a {} operation on parent container'.format(method)) return f @@ -136,6 +154,13 @@ def f(method, query=None, user=None, public=False, projection=None): query['permissions'] = {'$elemMatch': {'_id': handler.uid}} if handler.is_true('public'): query['$or'] = [{'public': True}, {'permissions': query.pop('permissions')}] + if handler.is_true('phi'): + temp_query = deepcopy(query) + temp_query['permissions'] = {'$elemMatch': {'_id': handler.uid, 'access': 'no-phi-ro'}} + not_allowed = exec_op(method, query=temp_query, user=user, public=public, projection=projection) + if not_allowed: + handler.abort(403, "User does not have PHI access to one or more elements") + return exec_op(method, query=query, user=user, public=public, projection=projection) return f return g @@ -147,3 +172,20 @@ def f(method, query=None, user=None, public=False, projection=None): query['public'] = True return exec_op(method, query=query, user=user, public=public, projection=projection) return f + +def phi_scrub(result): + for field in PHI_FIELDS: + if result.get(field) and isinstance(PHI_FIELDS[field], dict): + for deeper_field in PHI_FIELDS[field]: + if result.get(field): + result[field][deeper_field] = SCRUB + elif result.get(field): + result[field] = SCRUB + result = file_info_scrub(result) + return result + +def file_info_scrub(result): + for file_index, file_ in enumerate(result['files']): + if file_.get('info'): + result['files'][file_index]['info'] = SCRUB + return result diff --git a/api/handlers/collectionshandler.py b/api/handlers/collectionshandler.py index 39b07a08f..6d93c991c 100644 --- a/api/handlers/collectionshandler.py +++ b/api/handlers/collectionshandler.py @@ -31,7 +31,7 @@ def __init__(self, request=None, response=None): self.storage = self.container_handler_configurations['collections']['storage'] def get(self, **kwargs): - return super(CollectionsHandler, self).get('collections', **kwargs) + return super(CollectionsHandler, self).get(cont_name='collections', **kwargs) def post(self): mongo_validator, payload_validator = self._get_validators() diff --git a/api/handlers/containerhandler.py b/api/handlers/containerhandler.py index 22392b247..b9deccaea 100644 --- a/api/handlers/containerhandler.py +++ b/api/handlers/containerhandler.py @@ -89,6 +89,7 @@ def __init__(self, request=None, response=None): super(ContainerHandler, self).__init__(request, response) self.storage = None self.config = None + self.phi = False @log_access(AccessType.view_container) def get(self, cont_name, **kwargs): @@ -116,6 +117,9 @@ def get(self, cont_name, **kwargs): inflate_job_info = cont_name == 'sessions' result['analyses'] = AnalysisStorage().get_analyses(cont_name, _id, inflate_job_info) + if not self.phi: + for analysis_ in result['analyses']: + containerauth.file_info_scrub(analysis_) return self.handle_origin(result) def handle_origin(self, result): @@ -318,6 +322,8 @@ def get_all(self, cont_name, par_cont_name=None, par_id=None): if self.is_true('permissions'): if not projection: projection = None + if self.is_true('phi'): + projection = None # select which permission filter will be applied to the list of results. if self.superuser_request: @@ -361,6 +367,8 @@ def get_all(self, cont_name, par_cont_name=None, par_id=None): result = containerutil.get_stats(result, cont_name) result = self.handle_origin(result) modified_results.append(result) + if self.is_true('phi'): + self.log_user_access(AccessType.view_container, cont_name, result.get('_id')) if self.is_true('join_avatars'): modified_results = self.join_user_info(modified_results) diff --git a/api/handlers/refererhandler.py b/api/handlers/refererhandler.py index 628300935..6ac8c3e04 100644 --- a/api/handlers/refererhandler.py +++ b/api/handlers/refererhandler.py @@ -63,6 +63,9 @@ class AnalysesHandler(RefererHandler): payload_schema_file = 'analysis.json' update_payload_schema_file = 'analysis-update.json' + def __init__(self, request=None, response=None): + super(AnalysesHandler, self).__init__(request, response) + self.phi = False def post(self, cont_name, cid): """ diff --git a/api/web/request.py b/api/web/request.py index 299b81410..16b1a3270 100644 --- a/api/web/request.py +++ b/api/web/request.py @@ -61,7 +61,7 @@ def log_user_access_from_request(self, *args, **kwargs): cont_id = kwargs.get(cont_id_kwarg) # Only log view_container events when the container is a session - if access_type is AccessType.view_container and cont_name not in ['sessions', 'session', 'project', 'projects']: + if access_type is AccessType.view_container and not self.phi: return result self.log_user_access(access_type, cont_name, cont_id) diff --git a/tests/integration_tests/python/test_containers.py b/tests/integration_tests/python/test_containers.py index f0fc8ff4a..46d8ce289 100644 --- a/tests/integration_tests/python/test_containers.py +++ b/tests/integration_tests/python/test_containers.py @@ -262,6 +262,62 @@ def test_get_all_for_user(as_admin, as_public): r = as_admin.get('/users/' + user_id + '/sessions') assert r.ok +def test_phi_access(as_admin, data_builder, log_db): + group = data_builder.create_group() + project = data_builder.create_project() + session = data_builder.create_session() + r = as_admin.put('/sessions/' + session, json={"subject":{"firstname":"FirstName"}}, params={'replace_metadata':True}) + assert r.ok + + # Test phi access for list returns with phi access level + pre_log = log_db.access_log.count({}) + r = as_admin.get('/sessions') + assert r.ok + for session_ in r.json(): + assert session_.get('subject').get('firstname') == None + assert pre_log == log_db.access_log.count({}) + r = as_admin.get('/sessions', params={'phi':True}) + assert r.ok + for session_ in r.json(): + assert session_.get('subject').get('firstname') == "FirstName" + assert pre_log == log_db.access_log.count({}) - len(r.json()) + + # Test phi access for individual elements with phi access level + pre_log = log_db.access_log.count({}) + r = as_admin.get('/sessions/' + session) + assert r.ok + assert r.json().get('subject').get('firstname') == 'FirstName' + assert pre_log == log_db.access_log.count({}) - 1 + + r = as_admin.get('/sessions/' + session, params={'phi':True}) + assert r.ok + assert r.json().get('subject').get('firstname') == 'FirstName' + assert pre_log == log_db.access_log.count({}) - 2 + + # Set access level to no-phi-ro (Read-Only No PHI) + r = as_admin.put('/projects/' + project + '/permissions/admin@user.com', json={'access': 'no-phi-ro'}) + assert r.ok + + # Test phi access for list returns without phi access level + pre_log = log_db.access_log.count({}) + r = as_admin.get('/sessions') + assert r.ok + for session_ in r.json(): + assert session_.get('subject').get('firstname') == None + assert pre_log == log_db.access_log.count({}) + r = as_admin.get('/sessions', params={'phi':True}) + assert r.status_code == 403 + + # Test phi access for individual elements without phi access level + pre_log = log_db.access_log.count({}) + r = as_admin.get('/sessions/' + session) + assert r.ok + assert r.json().get('subject').get('firstname') == '***' + assert pre_log == log_db.access_log.count({}) + + r = as_admin.get('/sessions/' + session, params={'phi':True}) + assert r.status_code == 403 + def test_get_container(data_builder, default_payload, file_form, as_drone, as_admin, as_public, api_db): project = data_builder.create_project() From ceae24ef17e03f27a5a96c8c5d505048ac8ad0f7 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 26 Jul 2017 17:01:00 -0500 Subject: [PATCH 04/17] analyses follow phi rules --- api/auth/containerauth.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/auth/containerauth.py b/api/auth/containerauth.py index 0093e0d56..927853148 100644 --- a/api/auth/containerauth.py +++ b/api/auth/containerauth.py @@ -122,7 +122,8 @@ def f(method, _id=None, payload=None): result = exec_op(method, _id=_id, payload=payload) if method == 'GET' and exec_op is not noop: - if not handler.phi_get_access(handler.uid, parent_container) > INTEGER_PERMISSIONS['no-phi-ro']: + handler.phi = _get_access(handler.uid, parent_container) > INTEGER_PERMISSIONS['no-phi-ro'] + if not handler.phi: if handler.is_true('phi'): handler.abort(403, "User not authorized to view PHI fields.") result = phi_scrub(result) From 8a54e9275133035c164262263dd81dc1f1088d51 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 27 Sep 2017 13:58:42 -0500 Subject: [PATCH 05/17] Renamed permission; blacklist PHI instead of scrub --- api/auth/__init__.py | 2 +- api/auth/containerauth.py | 80 ++++++++----------- api/auth/groupauth.py | 2 +- api/auth/listauth.py | 6 +- api/dao/containerstorage.py | 4 +- api/handlers/containerhandler.py | 5 +- raml/schemas/definitions/permission.json | 2 +- .../python/test_containers.py | 11 ++- 8 files changed, 49 insertions(+), 63 deletions(-) diff --git a/api/auth/__init__.py b/api/auth/__init__.py index 3d0ab02ba..db3787e97 100644 --- a/api/auth/__init__.py +++ b/api/auth/__init__.py @@ -3,7 +3,7 @@ PERMISSIONS = [ { - 'rid': 'no-phi-ro', + 'rid': 'ro-no-phi', 'name': 'Read-Only (No PHI)' }, { diff --git a/api/auth/containerauth.py b/api/auth/containerauth.py index 927853148..8636edf1a 100644 --- a/api/auth/containerauth.py +++ b/api/auth/containerauth.py @@ -5,9 +5,8 @@ from . import _get_access, INTEGER_PERMISSIONS from ..dao import noop -PHI_FIELDS = {'info': '***', 'subject': {'firstname': "***", 'lastname': "***", 'sex': "***", - 'age': "***", 'race': "***", 'ethnicity': "***", 'info': "***"}, 'tags': "***"} -SCRUB = "***" +PHI_FIELDS = {'info': 0, 'subject.firstname':0, 'subject.lastname': 0, 'subject.sex': 0, + 'subject.age': 0, 'subject.race': 0, 'subject.ethnicity': 0, 'subject.info': 0, 'tags': 0, 'files.info':0} def default_container(handler, container=None, target_parent_container=None): """ @@ -22,7 +21,7 @@ def f(method, _id=None, payload=None, unset_payload=None, recursive=False, r_pay if method == 'GET' and container.get('public', False): has_access = True elif method == 'GET': - has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['no-phi-ro'] + has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['ro-no-phi'] elif method == 'POST': required_perm = 'rw' if target_parent_container.get('cont_name') == 'group': @@ -50,15 +49,16 @@ def f(method, _id=None, payload=None, unset_payload=None, recursive=False, r_pay else: has_access = False + if method == 'GET' and exec_op is not noop: + handler.phi = _get_access(handler.uid, container) > INTEGER_PERMISSIONS['ro-no-phi'] + if not handler.phi: + if handler.is_true('phi'): + handler.abort(403, "User not authorized to view PHI fields.") + else: + projection = PHI_FIELDS + if has_access: result = exec_op(method, _id=_id, payload=payload, unset_payload=unset_payload, recursive=recursive, r_payload=r_payload, replace_metadata=replace_metadata, projection=projection) - - if method == 'GET' and exec_op is not noop: - handler.phi = _get_access(handler.uid, container) > INTEGER_PERMISSIONS['no-phi-ro'] - if not handler.phi: - if handler.is_true('phi'): - handler.abort(403, "User not authorized to view PHI fields.") - result = phi_scrub(result) return result else: error_msg = 'user not authorized to perform a {} operation on the container.'.format(method) @@ -76,10 +76,11 @@ def collection_permissions(handler, container=None, _=None): """ def g(exec_op): def f(method, _id=None, payload = None): + projection = None if method == 'GET' and container.get('public', False): has_access = True elif method == 'GET': - has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['no-phi-ro'] + has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['ro-no-phi'] elif method == 'DELETE': has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['admin'] elif method == 'POST': @@ -89,15 +90,15 @@ def f(method, _id=None, payload = None): else: has_access = False + if method == 'GET' and exec_op is not noop: + handler.phi = _get_access(handler.uid, container) > INTEGER_PERMISSIONS['ro-no-phi'] + if not handler.phi: + if handler.is_true('phi'): + handler.abort(403, "User not authorized to view PHI fields.") + projection = PHI_FIELDS + if has_access: - result = exec_op(method, _id=_id, payload=payload) - - if method == 'GET' and exec_op is not noop: - handler.phi = _get_access(handler.uid, container) > INTEGER_PERMISSIONS['no-phi-ro'] - if not handler.phi: - if handler.is_true('phi'): - handler.abort(403, "User not authorized to view PHI fields.") - result = phi_scrub(result) + result = exec_op(method, _id=_id, payload=payload, projection=projection) return result else: handler.abort(403, 'user not authorized to perform a {} operation on the container'.format(method)) @@ -108,25 +109,26 @@ def f(method, _id=None, payload = None): def default_referer(handler, parent_container=None): def g(exec_op): def f(method, _id=None, payload=None): + projection = None access = _get_access(handler.uid, parent_container) if method == 'GET' and parent_container.get('public', False): has_access = True elif method == 'GET': - has_access = access >= INTEGER_PERMISSIONS['no-phi-ro'] + has_access = access >= INTEGER_PERMISSIONS['ro-no-phi'] elif method in ['POST', 'PUT', 'DELETE']: has_access = access >= INTEGER_PERMISSIONS['rw'] else: - has_access = False + has_access = False + + if method == 'GET' and exec_op is not noop: + handler.phi = _get_access(handler.uid, parent_container) > INTEGER_PERMISSIONS['ro-no-phi'] + if not handler.phi: + if handler.is_true('phi'): + handler.abort(403, "User not authorized to view PHI fields.") + projection = PHI_FIELDS if has_access: - result = exec_op(method, _id=_id, payload=payload) - - if method == 'GET' and exec_op is not noop: - handler.phi = _get_access(handler.uid, parent_container) > INTEGER_PERMISSIONS['no-phi-ro'] - if not handler.phi: - if handler.is_true('phi'): - handler.abort(403, "User not authorized to view PHI fields.") - result = phi_scrub(result) + result = exec_op(method, _id=_id, payload=payload, projection=projection) return result else: handler.abort(403, 'user not authorized to perform a {} operation on parent container'.format(method)) @@ -157,7 +159,7 @@ def f(method, query=None, user=None, public=False, projection=None): query['$or'] = [{'public': True}, {'permissions': query.pop('permissions')}] if handler.is_true('phi'): temp_query = deepcopy(query) - temp_query['permissions'] = {'$elemMatch': {'_id': handler.uid, 'access': 'no-phi-ro'}} + temp_query['permissions'] = {'$elemMatch': {'_id': handler.uid, 'access': 'ro-no-phi'}} not_allowed = exec_op(method, query=temp_query, user=user, public=public, projection=projection) if not_allowed: handler.abort(403, "User does not have PHI access to one or more elements") @@ -174,19 +176,3 @@ def f(method, query=None, user=None, public=False, projection=None): return exec_op(method, query=query, user=user, public=public, projection=projection) return f -def phi_scrub(result): - for field in PHI_FIELDS: - if result.get(field) and isinstance(PHI_FIELDS[field], dict): - for deeper_field in PHI_FIELDS[field]: - if result.get(field): - result[field][deeper_field] = SCRUB - elif result.get(field): - result[field] = SCRUB - result = file_info_scrub(result) - return result - -def file_info_scrub(result): - for file_index, file_ in enumerate(result['files']): - if file_.get('info'): - result['files'][file_index]['info'] = SCRUB - return result diff --git a/api/auth/groupauth.py b/api/auth/groupauth.py index a32aee2be..fd6ad6f26 100644 --- a/api/auth/groupauth.py +++ b/api/auth/groupauth.py @@ -17,7 +17,7 @@ def f(method, _id=None, query=None, payload=None, projection=None): handler.abort(403, 'not allowed to perform operation') elif _get_access(handler.uid, group) >= INTEGER_PERMISSIONS['admin']: pass - elif method == 'GET' and _get_access(handler.uid, group) >= INTEGER_PERMISSIONS['no-phi-ro']: + elif method == 'GET' and _get_access(handler.uid, group) >= INTEGER_PERMISSIONS['ro-no-phi']: pass else: handler.abort(403, 'not allowed to perform operation') diff --git a/api/auth/listauth.py b/api/auth/listauth.py index a3daf00f6..3dd0de2f9 100644 --- a/api/auth/listauth.py +++ b/api/auth/listauth.py @@ -23,7 +23,7 @@ def f(method, _id, query_params=None, payload=None, exclude_params=None): if method == 'GET' and container.get('public', False): min_access = -1 elif method == 'GET': - min_access = INTEGER_PERMISSIONS['no-phi-ro'] + min_access = INTEGER_PERMISSIONS['ro-no-phi'] elif method in ['POST', 'PUT', 'DELETE']: min_access = INTEGER_PERMISSIONS['rw'] else: @@ -59,7 +59,7 @@ def group_tags_sublist(handler, container): access = _get_access(handler.uid, container) def g(exec_op): def f(method, _id, query_params = None, payload = None, exclude_params=None): - if method == 'GET' and access >= INTEGER_PERMISSIONS['no-phi-ro']: + if method == 'GET' and access >= INTEGER_PERMISSIONS['ro-no-phi']: return exec_op(method, _id, query_params, payload, exclude_params) elif access >= INTEGER_PERMISSIONS['rw']: return exec_op(method, _id, query_params, payload, exclude_params) @@ -95,7 +95,7 @@ def f(method, _id, query_params = None, payload = None, exclude_params=None): pass elif method == 'POST' and access >= INTEGER_PERMISSIONS['rw'] and payload['user'] == handler.uid: pass - elif method == 'GET' and (access >= INTEGER_PERMISSIONS['no-phi-ro'] or container.get('public')): + elif method == 'GET' and (access >= INTEGER_PERMISSIONS['ro-no-phi'] or container.get('public')): pass elif method in ['GET', 'DELETE', 'PUT'] and container['notes'][0]['user'] == handler.uid: pass diff --git a/api/dao/containerstorage.py b/api/dao/containerstorage.py index 3c4961b8d..7566fb32a 100644 --- a/api/dao/containerstorage.py +++ b/api/dao/containerstorage.py @@ -315,10 +315,10 @@ def get_parent(self, parent_type, parent_id): return parent_storage.get_container(parent_id) - def get_analyses(self, parent_type, parent_id, inflate_job_info=False): + def get_analyses(self, parent_type, parent_id, inflate_job_info=False, projection=None): parent_type = containerutil.singularize(parent_type) parent_id = bson.ObjectId(parent_id) - analyses = self.get_all_el({'parent.type': parent_type, 'parent.id': parent_id}, None, None) + analyses = self.get_all_el({'parent.type': parent_type, 'parent.id': parent_id}, None, projection) if inflate_job_info: for analysis in analyses: self.inflate_job_info(analysis) diff --git a/api/handlers/containerhandler.py b/api/handlers/containerhandler.py index b9deccaea..7d2554650 100644 --- a/api/handlers/containerhandler.py +++ b/api/handlers/containerhandler.py @@ -116,10 +116,7 @@ def get(self, cont_name, **kwargs): fileinfo['path'] = util.path_from_hash(fileinfo['hash']) inflate_job_info = cont_name == 'sessions' - result['analyses'] = AnalysisStorage().get_analyses(cont_name, _id, inflate_job_info) - if not self.phi: - for analysis_ in result['analyses']: - containerauth.file_info_scrub(analysis_) + result['analyses'] = AnalysisStorage().get_analyses(cont_name, _id, inflate_job_info, containerauth.PHI_FIELDS) return self.handle_origin(result) def handle_origin(self, result): diff --git a/raml/schemas/definitions/permission.json b/raml/schemas/definitions/permission.json index a338d493f..8bcd81841 100644 --- a/raml/schemas/definitions/permission.json +++ b/raml/schemas/definitions/permission.json @@ -3,7 +3,7 @@ "type": "object", "definitions": { "_id": { "type": "string" }, - "access": { "enum": ["no-phi-ro", "ro", "rw", "admin"] }, + "access": { "enum": ["ro-no-phi", "ro", "rw", "admin"] }, "permission":{ "type":"object", "properties":{ diff --git a/tests/integration_tests/python/test_containers.py b/tests/integration_tests/python/test_containers.py index 46d8ce289..b316ae6d8 100644 --- a/tests/integration_tests/python/test_containers.py +++ b/tests/integration_tests/python/test_containers.py @@ -266,7 +266,7 @@ def test_phi_access(as_admin, data_builder, log_db): group = data_builder.create_group() project = data_builder.create_project() session = data_builder.create_session() - r = as_admin.put('/sessions/' + session, json={"subject":{"firstname":"FirstName"}}, params={'replace_metadata':True}) + r = as_admin.put('/sessions/' + session, json={"subject":{"firstname":"FirstName", "code":"Subject_Code"}}, params={'replace_metadata':True}) assert r.ok # Test phi access for list returns with phi access level @@ -292,10 +292,11 @@ def test_phi_access(as_admin, data_builder, log_db): r = as_admin.get('/sessions/' + session, params={'phi':True}) assert r.ok assert r.json().get('subject').get('firstname') == 'FirstName' + assert r.json().get('subject').get('code') == 'Subject_Code' assert pre_log == log_db.access_log.count({}) - 2 - # Set access level to no-phi-ro (Read-Only No PHI) - r = as_admin.put('/projects/' + project + '/permissions/admin@user.com', json={'access': 'no-phi-ro'}) + # Set access level to ro-no-phi (Read-Only No PHI) + r = as_admin.put('/projects/' + project + '/permissions/admin@user.com', json={'access': 'ro-no-phi'}) assert r.ok # Test phi access for list returns without phi access level @@ -304,6 +305,7 @@ def test_phi_access(as_admin, data_builder, log_db): assert r.ok for session_ in r.json(): assert session_.get('subject').get('firstname') == None + assert session_.get('subject').get('code') == 'Subject_Code' assert pre_log == log_db.access_log.count({}) r = as_admin.get('/sessions', params={'phi':True}) assert r.status_code == 403 @@ -312,7 +314,8 @@ def test_phi_access(as_admin, data_builder, log_db): pre_log = log_db.access_log.count({}) r = as_admin.get('/sessions/' + session) assert r.ok - assert r.json().get('subject').get('firstname') == '***' + assert r.json().get('subject').get('firstname') == None + assert r.json().get('subject').get('code') == 'Subject_Code' assert pre_log == log_db.access_log.count({}) r = as_admin.get('/sessions/' + session, params={'phi':True}) From ea8cbc2fdae8fdf4c630f8522f04ef819eecea9a Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Mon, 2 Oct 2017 11:42:53 -0500 Subject: [PATCH 06/17] reorganized phi logic into handlers --- api/auth/containerauth.py | 59 +++++++------------ api/dao/basecontainerstorage.py | 3 +- api/handlers/collectionshandler.py | 8 ++- api/handlers/containerhandler.py | 36 +++++++---- api/handlers/refererhandler.py | 12 +++- .../python/test_containers.py | 20 ++++++- 6 files changed, 82 insertions(+), 56 deletions(-) diff --git a/api/auth/containerauth.py b/api/auth/containerauth.py index 8636edf1a..3186c87a8 100644 --- a/api/auth/containerauth.py +++ b/api/auth/containerauth.py @@ -3,7 +3,6 @@ """ from copy import deepcopy from . import _get_access, INTEGER_PERMISSIONS -from ..dao import noop PHI_FIELDS = {'info': 0, 'subject.firstname':0, 'subject.lastname': 0, 'subject.sex': 0, 'subject.age': 0, 'subject.race': 0, 'subject.ethnicity': 0, 'subject.info': 0, 'tags': 0, 'files.info':0} @@ -15,8 +14,7 @@ def default_container(handler, container=None, target_parent_container=None): on the container before actually executing this method. """ def g(exec_op): - def f(method, _id=None, payload=None, unset_payload=None, recursive=False, r_payload=None, replace_metadata=False): - projection = None + def f(method, _id=None, payload=None, unset_payload=None, recursive=False, r_payload=None, projection=None, replace_metadata=False, phi=False): additional_error_msg = None if method == 'GET' and container.get('public', False): has_access = True @@ -49,17 +47,12 @@ def f(method, _id=None, payload=None, unset_payload=None, recursive=False, r_pay else: has_access = False - if method == 'GET' and exec_op is not noop: - handler.phi = _get_access(handler.uid, container) > INTEGER_PERMISSIONS['ro-no-phi'] - if not handler.phi: - if handler.is_true('phi'): - handler.abort(403, "User not authorized to view PHI fields.") - else: - projection = PHI_FIELDS + if method == 'GET' and phi: + if not _get_access(handler.uid, container) > INTEGER_PERMISSIONS['ro-no-phi']: + handler.abort(403, "User not authorized to view PHI fields on the container.") if has_access: - result = exec_op(method, _id=_id, payload=payload, unset_payload=unset_payload, recursive=recursive, r_payload=r_payload, replace_metadata=replace_metadata, projection=projection) - return result + return exec_op(method, _id=_id, payload=payload, unset_payload=unset_payload, recursive=recursive, r_payload=r_payload, replace_metadata=replace_metadata, projection=projection) else: error_msg = 'user not authorized to perform a {} operation on the container.'.format(method) if additional_error_msg: @@ -75,8 +68,7 @@ def collection_permissions(handler, container=None, _=None): Permissions are checked on the collection itself or not at all if the collection is new. """ def g(exec_op): - def f(method, _id=None, payload = None): - projection = None + def f(method, _id=None, payload = None, projection=None, phi=False): if method == 'GET' and container.get('public', False): has_access = True elif method == 'GET': @@ -90,16 +82,12 @@ def f(method, _id=None, payload = None): else: has_access = False - if method == 'GET' and exec_op is not noop: - handler.phi = _get_access(handler.uid, container) > INTEGER_PERMISSIONS['ro-no-phi'] - if not handler.phi: - if handler.is_true('phi'): - handler.abort(403, "User not authorized to view PHI fields.") - projection = PHI_FIELDS + if method == 'GET' and phi: + if not _get_access(handler.uid, container) > INTEGER_PERMISSIONS['ro-no-phi']: + handler.abort(403, "User not authorized to view PHI fields.") if has_access: - result = exec_op(method, _id=_id, payload=payload, projection=projection) - return result + return exec_op(method, _id=_id, payload=payload, projection=projection) else: handler.abort(403, 'user not authorized to perform a {} operation on the container'.format(method)) return f @@ -108,8 +96,7 @@ def f(method, _id=None, payload = None): def default_referer(handler, parent_container=None): def g(exec_op): - def f(method, _id=None, payload=None): - projection = None + def f(method, _id=None, payload=None, projection = None, phi=False): access = _get_access(handler.uid, parent_container) if method == 'GET' and parent_container.get('public', False): has_access = True @@ -120,16 +107,12 @@ def f(method, _id=None, payload=None): else: has_access = False - if method == 'GET' and exec_op is not noop: - handler.phi = _get_access(handler.uid, parent_container) > INTEGER_PERMISSIONS['ro-no-phi'] - if not handler.phi: - if handler.is_true('phi'): - handler.abort(403, "User not authorized to view PHI fields.") - projection = PHI_FIELDS + if method == 'GET' and phi: + if not _get_access(handler.uid, parent_container) > INTEGER_PERMISSIONS['ro-no-phi']: + handler.abort(403, "User not authorized to view PHI fields.") if has_access: - result = exec_op(method, _id=_id, payload=payload, projection=projection) - return result + return exec_op(method, _id=_id, payload=payload, projection=projection) else: handler.abort(403, 'user not authorized to perform a {} operation on parent container'.format(method)) return f @@ -141,9 +124,11 @@ def public_request(handler, container=None): For public requests we allow only GET operations on containers marked as public. """ def g(exec_op): - def f(method, _id=None, payload = None): + def f(method, _id=None, payload = None, phi=False, projection=None): + if phi: + handler.abort(403, "Must be logged in to view PHI fields.") if method == 'GET' and container.get('public', False): - return exec_op(method, _id, payload) + return exec_op(method, _id, payload, projection) else: handler.abort(403, 'not authorized to perform a {} operation on this container'.format(method)) return f @@ -151,13 +136,13 @@ def f(method, _id=None, payload = None): def list_permission_checker(handler): def g(exec_op): - def f(method, query=None, user=None, public=False, projection=None): + def f(method, query=None, user=None, public=False, projection=None, phi=False): if user and (user['_id'] != handler.uid): handler.abort(403, 'User ' + handler.uid + ' may not see the Projects of User ' + user['_id']) query['permissions'] = {'$elemMatch': {'_id': handler.uid}} if handler.is_true('public'): query['$or'] = [{'public': True}, {'permissions': query.pop('permissions')}] - if handler.is_true('phi'): + if phi: temp_query = deepcopy(query) temp_query['permissions'] = {'$elemMatch': {'_id': handler.uid, 'access': 'ro-no-phi'}} not_allowed = exec_op(method, query=temp_query, user=user, public=public, projection=projection) @@ -170,7 +155,7 @@ def f(method, query=None, user=None, public=False, projection=None): def list_public_request(exec_op): - def f(method, query=None, user=None, public=False, projection=None): + def f(method, query=None, user=None, public=False, projection=None, phi=False): # pylint: disable=unused-argument if public: query['public'] = True return exec_op(method, query=query, user=user, public=public, projection=projection) diff --git a/api/dao/basecontainerstorage.py b/api/dao/basecontainerstorage.py index 3f6a610b1..0785fbcb7 100644 --- a/api/dao/basecontainerstorage.py +++ b/api/dao/basecontainerstorage.py @@ -129,11 +129,10 @@ def _to_mongo(self, payload): def exec_op(self, action, _id=None, payload=None, query=None, user=None, public=False, projection=None, recursive=False, r_payload=None, # pylint: disable=unused-argument - replace_metadata=False, unset_payload=None): + replace_metadata=False, unset_payload=None, phi=False): # pylint: disable=unused-argument """ Generic method to exec a CRUD operation from a REST verb. """ - check = consistencychecker.get_container_storage_checker(action, self.cont_name) data_op = payload or {'_id': _id} check(data_op) diff --git a/api/handlers/collectionshandler.py b/api/handlers/collectionshandler.py index 6d93c991c..5ae052781 100644 --- a/api/handlers/collectionshandler.py +++ b/api/handlers/collectionshandler.py @@ -109,7 +109,13 @@ def get_all(self): else: permchecker = containerauth.list_permission_checker(self) query = {} - results = permchecker(self.storage.exec_op)('GET', query=query, public=self.public_request, projection=projection) + if self.is_true('phi'): + projection = None + phi = True + else: + phi = False + projection.update(self.PHI_FIELDS) + results = permchecker(self.storage.exec_op)('GET', query=query, public=self.public_request, projection=projection, phi=phi) if not self.superuser_request and not self.is_true('join_avatars'): self._filter_all_permissions(results, self.uid) if self.is_true('join_avatars'): diff --git a/api/handlers/containerhandler.py b/api/handlers/containerhandler.py index 7d2554650..8a315d33c 100644 --- a/api/handlers/containerhandler.py +++ b/api/handlers/containerhandler.py @@ -5,7 +5,7 @@ from .. import config from .. import util from .. import validators -from ..auth import containerauth, always_ok +from ..auth import containerauth, always_ok, _get_access, INTEGER_PERMISSIONS from ..dao import containerstorage, containerutil, noop from ..dao.containerstorage import AnalysisStorage from ..jobs.gears import get_gear @@ -44,6 +44,10 @@ class ContainerHandler(base.RequestHandler): } default_list_projection = ['files', 'notes', 'timestamp', 'timezone', 'public'] + # Hard-coded PHI fields, will be changed to user set PHI fields + PHI_FIELDS = {'info': 0, 'subject.firstname':0, 'subject.lastname': 0, 'subject.sex': 0, + 'subject.age': 0, 'subject.race': 0, 'subject.ethnicity': 0, 'subject.info': 0, 'tags': 0, 'files.info':0} + # This configurations are used by the ContainerHandler class to load the storage, # the permissions checker and the json schema validators used to handle a request. # @@ -85,6 +89,7 @@ class ContainerHandler(base.RequestHandler): } } + def __init__(self, request=None, response=None): super(ContainerHandler, self).__init__(request, response) self.storage = None @@ -94,14 +99,18 @@ def __init__(self, request=None, response=None): @log_access(AccessType.view_container) def get(self, cont_name, **kwargs): _id = kwargs.get('cid') + projection = self.PHI_FIELDS self.config = self.container_handler_configurations[cont_name] self.storage = self.config['storage'] container = self._get_container(_id) + if _get_access(self.uid, container) > INTEGER_PERMISSIONS['ro-no-phi'] or self.superuser_request: + self.phi = True + projection = None permchecker = self._get_permchecker(container) try: # This line exec the actual get checking permissions using the decorator permchecker - result = permchecker(self.storage.exec_op)('GET', _id) + result = permchecker(self.storage.exec_op)('GET', _id, projection=projection, phi=self.phi) except APIStorageException as e: self.abort(400, e.message) if result is None: @@ -116,7 +125,7 @@ def get(self, cont_name, **kwargs): fileinfo['path'] = util.path_from_hash(fileinfo['hash']) inflate_job_info = cont_name == 'sessions' - result['analyses'] = AnalysisStorage().get_analyses(cont_name, _id, inflate_job_info, containerauth.PHI_FIELDS) + result['analyses'] = AnalysisStorage().get_analyses(cont_name, _id, inflate_job_info, self.PHI_FIELDS) return self.handle_origin(result) def handle_origin(self, result): @@ -312,16 +321,20 @@ def match_ids(x): def get_all(self, cont_name, par_cont_name=None, par_id=None): self.config = self.container_handler_configurations[cont_name] self.storage = self.config['storage'] - - projection = self.config['list_projection'].copy() - if self.is_true('info'): - projection.pop('info') + projection = None + # if self.is_true('info'): Seems redundant with new phi functionality + # projection.pop('info') if self.is_true('permissions'): if not projection: projection = None if self.is_true('phi'): - projection = None - + phi = True + else: + phi = False + if projection == None: + projection = self.PHI_FIELDS + else: + projection.update(self.PHI_FIELDS) # select which permission filter will be applied to the list of results. if self.superuser_request: permchecker = always_ok @@ -343,8 +356,9 @@ def get_all(self, cont_name, par_cont_name=None, par_id=None): query = {} if not self.is_true('archived'): query['archived'] = {'$ne': True} + # this request executes the actual reqeust filtering containers based on the user permissions - results = permchecker(self.storage.exec_op)('GET', query=query, public=self.public_request, projection=projection) + results = permchecker(self.storage.exec_op)('GET', query=query, public=self.public_request, projection=projection, phi=phi) if results is None: self.abort(404, 'No elements found in container {}'.format(self.storage.cont_name)) # return only permissions of the current user unless superuser or getting avatars @@ -364,7 +378,7 @@ def get_all(self, cont_name, par_cont_name=None, par_id=None): result = containerutil.get_stats(result, cont_name) result = self.handle_origin(result) modified_results.append(result) - if self.is_true('phi'): + if phi: self.log_user_access(AccessType.view_container, cont_name, result.get('_id')) if self.is_true('join_avatars'): diff --git a/api/handlers/refererhandler.py b/api/handlers/refererhandler.py index 6ac8c3e04..b07e8573a 100644 --- a/api/handlers/refererhandler.py +++ b/api/handlers/refererhandler.py @@ -16,7 +16,7 @@ from .. import upload from .. import util from .. import validators -from ..auth import containerauth, always_ok +from ..auth import containerauth, always_ok, _get_access, INTEGER_PERMISSIONS from ..dao import containerstorage, noop from ..dao.basecontainerstorage import ContainerStorage from ..dao.containerutil import singularize @@ -63,6 +63,9 @@ class AnalysesHandler(RefererHandler): payload_schema_file = 'analysis.json' update_payload_schema_file = 'analysis-update.json' + # Hard-coded PHI fields, will be changed to user set PHI fields + PHI_FIELDS = {'info': 0, 'tags': 0, 'files.info':0} + def __init__(self, request=None, response=None): super(AnalysesHandler, self).__init__(request, response) self.phi = False @@ -133,8 +136,13 @@ def get(self, **kwargs): _id = kwargs.get('_id') analysis = self.storage.get_container(_id) parent = self.storage.get_parent(analysis['parent']['type'], analysis['parent']['id']) + projection = self.PHI_FIELDS + if _get_access(self.uid, parent) > INTEGER_PERMISSIONS['ro-no-phi'] or self.superuser_request: + self.phi = True + projection = None permchecker = self.get_permchecker(parent) - permchecker(noop)('GET') + permchecker(noop)('GET',phi=self.phi) + analysis = self.storage.get_container(_id, projection=projection) if self.is_true('inflate_job'): self.storage.inflate_job_info(analysis) diff --git a/tests/integration_tests/python/test_containers.py b/tests/integration_tests/python/test_containers.py index b316ae6d8..788cf7470 100644 --- a/tests/integration_tests/python/test_containers.py +++ b/tests/integration_tests/python/test_containers.py @@ -262,7 +262,7 @@ def test_get_all_for_user(as_admin, as_public): r = as_admin.get('/users/' + user_id + '/sessions') assert r.ok -def test_phi_access(as_admin, data_builder, log_db): +def test_phi_access(as_user, as_admin, data_builder, log_db): group = data_builder.create_group() project = data_builder.create_project() session = data_builder.create_session() @@ -298,6 +298,8 @@ def test_phi_access(as_admin, data_builder, log_db): # Set access level to ro-no-phi (Read-Only No PHI) r = as_admin.put('/projects/' + project + '/permissions/admin@user.com', json={'access': 'ro-no-phi'}) assert r.ok + r = as_admin.post('/projects/' + project + '/permissions', json={'access': 'ro-no-phi', '_id': 'user@user.com'}) + assert r.ok # Test phi access for list returns without phi access level pre_log = log_db.access_log.count({}) @@ -310,7 +312,7 @@ def test_phi_access(as_admin, data_builder, log_db): r = as_admin.get('/sessions', params={'phi':True}) assert r.status_code == 403 - # Test phi access for individual elements without phi access level + # Test phi access for individual elements without phi access level but with super_user pre_log = log_db.access_log.count({}) r = as_admin.get('/sessions/' + session) assert r.ok @@ -319,7 +321,19 @@ def test_phi_access(as_admin, data_builder, log_db): assert pre_log == log_db.access_log.count({}) r = as_admin.get('/sessions/' + session, params={'phi':True}) - assert r.status_code == 403 + assert r.status_code == 200 + + # Test phi access for individual elements without phi access level and w/o super_user + as_admin + pre_log = log_db.access_log.count({}) + r = as_user.get('/sessions/' + session) + assert r.ok + assert r.json().get('subject').get('firstname') == None + assert r.json().get('subject').get('code') == 'Subject_Code' + assert pre_log == log_db.access_log.count({}) + + r = as_user.get('/sessions/' + session, params={'phi':True}) + assert r.status_code == 200 def test_get_container(data_builder, default_payload, file_form, as_drone, as_admin, as_public, api_db): From cb8e2daa07c9eeb2683cbdbbc2fa0f88b471943d Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 4 Oct 2017 16:59:22 -0500 Subject: [PATCH 07/17] tests for collections added --- api/auth/containerauth.py | 4 +- api/handlers/collectionshandler.py | 7 ++- raml/resources/collections.raml | 3 + .../integration_tests/abao/abao_test_hooks.js | 7 +++ .../python/test_collection.py | 60 +++++++++++++++++++ 5 files changed, 76 insertions(+), 5 deletions(-) diff --git a/api/auth/containerauth.py b/api/auth/containerauth.py index 3186c87a8..2f31d83ad 100644 --- a/api/auth/containerauth.py +++ b/api/auth/containerauth.py @@ -4,8 +4,6 @@ from copy import deepcopy from . import _get_access, INTEGER_PERMISSIONS -PHI_FIELDS = {'info': 0, 'subject.firstname':0, 'subject.lastname': 0, 'subject.sex': 0, - 'subject.age': 0, 'subject.race': 0, 'subject.ethnicity': 0, 'subject.info': 0, 'tags': 0, 'files.info':0} def default_container(handler, container=None, target_parent_container=None): """ @@ -128,7 +126,7 @@ def f(method, _id=None, payload = None, phi=False, projection=None): if phi: handler.abort(403, "Must be logged in to view PHI fields.") if method == 'GET' and container.get('public', False): - return exec_op(method, _id, payload, projection) + return exec_op(method, _id, payload=payload, projection=projection) else: handler.abort(403, 'not authorized to perform a {} operation on this container'.format(method)) return f diff --git a/api/handlers/collectionshandler.py b/api/handlers/collectionshandler.py index 5ae052781..98af9fe1e 100644 --- a/api/handlers/collectionshandler.py +++ b/api/handlers/collectionshandler.py @@ -6,6 +6,7 @@ from ..dao import containerstorage, containerutil, noop from ..web.errors import APIStorageException from ..validators import verify_payload_exists +from ..web.request import AccessType from .containerhandler import ContainerHandler @@ -101,7 +102,7 @@ def delete(self, **kwargs): config.db.acquisitions.update_many({'collections': bson.ObjectId(_id)}, {'$pull': {'collections': bson.ObjectId(_id)}}) def get_all(self): - projection = self.container_handler_configurations['collections']['list_projection'] + projection = None if self.superuser_request: permchecker = always_ok elif self.public_request: @@ -114,7 +115,7 @@ def get_all(self): phi = True else: phi = False - projection.update(self.PHI_FIELDS) + projection = {'info': 0, 'tags': 0, 'files.info':0} results = permchecker(self.storage.exec_op)('GET', query=query, public=self.public_request, projection=projection, phi=phi) if not self.superuser_request and not self.is_true('join_avatars'): self._filter_all_permissions(results, self.uid) @@ -123,6 +124,8 @@ def get_all(self): for result in results: if self.is_true('stats'): result = containerutil.get_stats(result, 'collections') + if phi: + self.log_user_access(AccessType.view_container, cont_name='collections', cont_id=result.get('_id')) return results def curators(self): diff --git a/raml/resources/collections.raml b/raml/resources/collections.raml index 53fc43235..a156d6aa3 100644 --- a/raml/resources/collections.raml +++ b/raml/resources/collections.raml @@ -1,4 +1,7 @@ get: + queryParameters: + phi: + description: To return phi description: List all collections responses: 200: diff --git a/tests/integration_tests/abao/abao_test_hooks.js b/tests/integration_tests/abao/abao_test_hooks.js index 270c43aae..c12cbe5a8 100644 --- a/tests/integration_tests/abao/abao_test_hooks.js +++ b/tests/integration_tests/abao/abao_test_hooks.js @@ -341,11 +341,18 @@ hooks.before("GET /groups/{GroupId}/projects -> 200", function(test, done) { }); +hooks.before("GET /collections -> 200", function(test, done) { + test.request.query = { + "phi": true + }; + done(); +}); // set initial test_collection_1 hooks.after("GET /collections -> 200", function(test, done) { test_collection_1 = test.response.body[0]; collection_id = test.response.body[0]._id; delete_collection_id = test.response.body[1]._id; + assert.equal(test_collection_1.label, "test-collection-1"); done(); }); diff --git a/tests/integration_tests/python/test_collection.py b/tests/integration_tests/python/test_collection.py index c88d5fb37..5e3fa1edc 100644 --- a/tests/integration_tests/python/test_collection.py +++ b/tests/integration_tests/python/test_collection.py @@ -113,3 +113,63 @@ def test_collections(data_builder, as_admin, as_user): # test if collection is listed at acquisition r = as_admin.get('/acquisitions/' + acquisition) assert collection not in r.json()['collections'] + +def test_collections_phi(data_builder, as_admin, as_user, log_db, file_form): + session = data_builder.create_session() + acquisition = data_builder.create_acquisition() + + # create collection + r = as_admin.post('/collections', json={ + 'label': 'SciTran/Testing', + 'public': True + }) + assert r.ok + collection = r.json()['_id'] + + file_name = 'test_file.txt' + + assert as_admin.post('/collections/' + collection + '/files', files=file_form(file_name)).ok + + # Attempt full replace of info + file_info = { + 'a': 'b', + 'test': 123, + 'map': { + 'a': 'b' + }, + 'list': [1,2,3] + } + + + r = as_admin.post('/collections/' + collection + '/files/' + file_name + '/info', json={ + 'replace': file_info + }) + assert r.ok + + + # Test phi access for list returns with phi access level + pre_log = log_db.access_log.count({}) + r = as_admin.get('/collections', params={"phi":False}) + assert r.ok + for collection_ in r.json(): + assert collection_.get('files',[{}])[0].get('info') == None + assert pre_log == log_db.access_log.count({}) + r = as_admin.get('/collections', params={'phi':True}) + assert r.ok + for collection_ in r.json(): + print collection_ + assert collection_.get('files',[{}])[0].get('info').get('a') == "b" + assert pre_log == log_db.access_log.count({}) - len(r.json()) + + # Test phi access for individual elements with phi access level + pre_log = log_db.access_log.count({}) + r = as_admin.get('/collections/' + collection) + assert r.ok + assert r.json().get('files',[{}])[0].get('info').get('a') == "b" + assert pre_log == log_db.access_log.count({}) - 1 + pre_log = log_db.access_log.count({}) + + r = as_admin.get('/collections/' + collection, params={'phi':True}) + assert r.ok + assert r.json().get('files',[{}])[0].get('info').get('a') == "b" + assert pre_log == log_db.access_log.count({}) - 1 From 9ef9f4da90337676e4d6c61721d90f5634c84ab3 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Mon, 9 Oct 2017 11:28:30 -0500 Subject: [PATCH 08/17] removed ro-no-phi and added no-phi boolean to permission --- api/auth/__init__.py | 11 +++++++---- api/auth/containerauth.py | 16 ++++++++-------- api/auth/groupauth.py | 2 +- api/auth/listauth.py | 6 +++--- api/handlers/containerhandler.py | 4 ++-- api/handlers/refererhandler.py | 4 ++-- raml/schemas/definitions/permission.json | 5 +++-- .../integration_tests/python/test_containers.py | 6 +++--- 8 files changed, 29 insertions(+), 25 deletions(-) diff --git a/api/auth/__init__.py b/api/auth/__init__.py index db3787e97..624ce811e 100644 --- a/api/auth/__init__.py +++ b/api/auth/__init__.py @@ -2,10 +2,6 @@ from ..web.errors import APIPermissionException PERMISSIONS = [ - { - 'rid': 'ro-no-phi', - 'name': 'Read-Only (No PHI)' - }, { 'rid': 'ro', 'name': 'Read-Only', @@ -32,6 +28,13 @@ def _get_access(uid, container): def has_access(uid, container, perm): return _get_access(uid, container) >= INTEGER_PERMISSIONS[perm] +# Returns true if user has phi access +def check_phi(uid, container): + permissions_list = container.get('permissions', []) + for perm in permissions_list: + if perm['_id'] == uid and perm.get('no-phi'): + return False + return has_access(uid, container, 'ro') def always_ok(exec_op): """ diff --git a/api/auth/containerauth.py b/api/auth/containerauth.py index 2f31d83ad..4bf579480 100644 --- a/api/auth/containerauth.py +++ b/api/auth/containerauth.py @@ -2,7 +2,7 @@ Purpose of this module is to define all the permissions checker decorators for the ContainerHandler classes. """ from copy import deepcopy -from . import _get_access, INTEGER_PERMISSIONS +from . import _get_access, check_phi, INTEGER_PERMISSIONS def default_container(handler, container=None, target_parent_container=None): @@ -17,7 +17,7 @@ def f(method, _id=None, payload=None, unset_payload=None, recursive=False, r_pay if method == 'GET' and container.get('public', False): has_access = True elif method == 'GET': - has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['ro-no-phi'] + has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['ro'] elif method == 'POST': required_perm = 'rw' if target_parent_container.get('cont_name') == 'group': @@ -46,7 +46,7 @@ def f(method, _id=None, payload=None, unset_payload=None, recursive=False, r_pay has_access = False if method == 'GET' and phi: - if not _get_access(handler.uid, container) > INTEGER_PERMISSIONS['ro-no-phi']: + if not check_phi(handler.uid, container): handler.abort(403, "User not authorized to view PHI fields on the container.") if has_access: @@ -70,7 +70,7 @@ def f(method, _id=None, payload = None, projection=None, phi=False): if method == 'GET' and container.get('public', False): has_access = True elif method == 'GET': - has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['ro-no-phi'] + has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['ro'] elif method == 'DELETE': has_access = _get_access(handler.uid, container) >= INTEGER_PERMISSIONS['admin'] elif method == 'POST': @@ -81,7 +81,7 @@ def f(method, _id=None, payload = None, projection=None, phi=False): has_access = False if method == 'GET' and phi: - if not _get_access(handler.uid, container) > INTEGER_PERMISSIONS['ro-no-phi']: + if not check_phi(handler.uid, container): handler.abort(403, "User not authorized to view PHI fields.") if has_access: @@ -99,14 +99,14 @@ def f(method, _id=None, payload=None, projection = None, phi=False): if method == 'GET' and parent_container.get('public', False): has_access = True elif method == 'GET': - has_access = access >= INTEGER_PERMISSIONS['ro-no-phi'] + has_access = access >= INTEGER_PERMISSIONS['ro'] elif method in ['POST', 'PUT', 'DELETE']: has_access = access >= INTEGER_PERMISSIONS['rw'] else: has_access = False if method == 'GET' and phi: - if not _get_access(handler.uid, parent_container) > INTEGER_PERMISSIONS['ro-no-phi']: + if not check_phi(handler.uid, parent_container): handler.abort(403, "User not authorized to view PHI fields.") if has_access: @@ -142,7 +142,7 @@ def f(method, query=None, user=None, public=False, projection=None, phi=False): query['$or'] = [{'public': True}, {'permissions': query.pop('permissions')}] if phi: temp_query = deepcopy(query) - temp_query['permissions'] = {'$elemMatch': {'_id': handler.uid, 'access': 'ro-no-phi'}} + temp_query['permissions'] = {'$elemMatch': {'_id': handler.uid, 'no-phi': True}} not_allowed = exec_op(method, query=temp_query, user=user, public=public, projection=projection) if not_allowed: handler.abort(403, "User does not have PHI access to one or more elements") diff --git a/api/auth/groupauth.py b/api/auth/groupauth.py index fd6ad6f26..fd98052d5 100644 --- a/api/auth/groupauth.py +++ b/api/auth/groupauth.py @@ -17,7 +17,7 @@ def f(method, _id=None, query=None, payload=None, projection=None): handler.abort(403, 'not allowed to perform operation') elif _get_access(handler.uid, group) >= INTEGER_PERMISSIONS['admin']: pass - elif method == 'GET' and _get_access(handler.uid, group) >= INTEGER_PERMISSIONS['ro-no-phi']: + elif method == 'GET' and _get_access(handler.uid, group) >= INTEGER_PERMISSIONS['ro']: pass else: handler.abort(403, 'not allowed to perform operation') diff --git a/api/auth/listauth.py b/api/auth/listauth.py index 3dd0de2f9..9253bb5ed 100644 --- a/api/auth/listauth.py +++ b/api/auth/listauth.py @@ -23,7 +23,7 @@ def f(method, _id, query_params=None, payload=None, exclude_params=None): if method == 'GET' and container.get('public', False): min_access = -1 elif method == 'GET': - min_access = INTEGER_PERMISSIONS['ro-no-phi'] + min_access = INTEGER_PERMISSIONS['ro'] elif method in ['POST', 'PUT', 'DELETE']: min_access = INTEGER_PERMISSIONS['rw'] else: @@ -59,7 +59,7 @@ def group_tags_sublist(handler, container): access = _get_access(handler.uid, container) def g(exec_op): def f(method, _id, query_params = None, payload = None, exclude_params=None): - if method == 'GET' and access >= INTEGER_PERMISSIONS['ro-no-phi']: + if method == 'GET' and access >= INTEGER_PERMISSIONS['ro']: return exec_op(method, _id, query_params, payload, exclude_params) elif access >= INTEGER_PERMISSIONS['rw']: return exec_op(method, _id, query_params, payload, exclude_params) @@ -95,7 +95,7 @@ def f(method, _id, query_params = None, payload = None, exclude_params=None): pass elif method == 'POST' and access >= INTEGER_PERMISSIONS['rw'] and payload['user'] == handler.uid: pass - elif method == 'GET' and (access >= INTEGER_PERMISSIONS['ro-no-phi'] or container.get('public')): + elif method == 'GET' and (access >= INTEGER_PERMISSIONS['ro'] or container.get('public')): pass elif method in ['GET', 'DELETE', 'PUT'] and container['notes'][0]['user'] == handler.uid: pass diff --git a/api/handlers/containerhandler.py b/api/handlers/containerhandler.py index 8a315d33c..c519baba9 100644 --- a/api/handlers/containerhandler.py +++ b/api/handlers/containerhandler.py @@ -5,7 +5,7 @@ from .. import config from .. import util from .. import validators -from ..auth import containerauth, always_ok, _get_access, INTEGER_PERMISSIONS +from ..auth import containerauth, always_ok, check_phi from ..dao import containerstorage, containerutil, noop from ..dao.containerstorage import AnalysisStorage from ..jobs.gears import get_gear @@ -103,7 +103,7 @@ def get(self, cont_name, **kwargs): self.config = self.container_handler_configurations[cont_name] self.storage = self.config['storage'] container = self._get_container(_id) - if _get_access(self.uid, container) > INTEGER_PERMISSIONS['ro-no-phi'] or self.superuser_request: + if check_phi(self.uid, container) or self.superuser_request: self.phi = True projection = None diff --git a/api/handlers/refererhandler.py b/api/handlers/refererhandler.py index b07e8573a..c4dcf2267 100644 --- a/api/handlers/refererhandler.py +++ b/api/handlers/refererhandler.py @@ -16,7 +16,7 @@ from .. import upload from .. import util from .. import validators -from ..auth import containerauth, always_ok, _get_access, INTEGER_PERMISSIONS +from ..auth import containerauth, always_ok, check_phi from ..dao import containerstorage, noop from ..dao.basecontainerstorage import ContainerStorage from ..dao.containerutil import singularize @@ -137,7 +137,7 @@ def get(self, **kwargs): analysis = self.storage.get_container(_id) parent = self.storage.get_parent(analysis['parent']['type'], analysis['parent']['id']) projection = self.PHI_FIELDS - if _get_access(self.uid, parent) > INTEGER_PERMISSIONS['ro-no-phi'] or self.superuser_request: + if check_phi(self.uid, parent)or self.superuser_request: self.phi = True projection = None permchecker = self.get_permchecker(parent) diff --git a/raml/schemas/definitions/permission.json b/raml/schemas/definitions/permission.json index 8bcd81841..51340e36b 100644 --- a/raml/schemas/definitions/permission.json +++ b/raml/schemas/definitions/permission.json @@ -3,12 +3,13 @@ "type": "object", "definitions": { "_id": { "type": "string" }, - "access": { "enum": ["ro-no-phi", "ro", "rw", "admin"] }, + "access": { "enum": ["ro", "rw", "admin"] }, "permission":{ "type":"object", "properties":{ "_id":{"$ref":"#/definitions/_id"}, - "access":{"$ref":"#/definitions/access"} + "access":{"$ref":"#/definitions/access"}, + "no-phi":{"type": "boolean"} }, "additionalProperties": false }, diff --git a/tests/integration_tests/python/test_containers.py b/tests/integration_tests/python/test_containers.py index 788cf7470..c06c98f7c 100644 --- a/tests/integration_tests/python/test_containers.py +++ b/tests/integration_tests/python/test_containers.py @@ -295,10 +295,10 @@ def test_phi_access(as_user, as_admin, data_builder, log_db): assert r.json().get('subject').get('code') == 'Subject_Code' assert pre_log == log_db.access_log.count({}) - 2 - # Set access level to ro-no-phi (Read-Only No PHI) - r = as_admin.put('/projects/' + project + '/permissions/admin@user.com', json={'access': 'ro-no-phi'}) + # Set no-phi flag to true + r = as_admin.put('/projects/' + project + '/permissions/admin@user.com', json={'no-phi': True}) assert r.ok - r = as_admin.post('/projects/' + project + '/permissions', json={'access': 'ro-no-phi', '_id': 'user@user.com'}) + r = as_admin.post('/projects/' + project + '/permissions', json={'access': 'ro', 'no-phi': True, '_id': 'user@user.com'}) assert r.ok # Test phi access for list returns without phi access level From e125f9ef56ff62c2092e933ff6c8da07dfae69da Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 11 Oct 2017 14:20:16 -0500 Subject: [PATCH 09/17] Removed commented code; phi for get_all_for_user --- api/handlers/collectionshandler.py | 4 ++-- api/handlers/containerhandler.py | 24 ++++++++++++------------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/api/handlers/collectionshandler.py b/api/handlers/collectionshandler.py index 98af9fe1e..8d31bc1d4 100644 --- a/api/handlers/collectionshandler.py +++ b/api/handlers/collectionshandler.py @@ -160,7 +160,7 @@ def get_sessions(self, cid): if not self.superuser_request: query['permissions._id'] = self.uid - projection = self.container_handler_configurations['sessions']['list_projection'] + projection = self.PHI_FIELDS sessions = list(containerstorage.SessionStorage().get_all_el(query, None, projection)) @@ -193,7 +193,7 @@ def get_acquisitions(self, cid): if not self.superuser_request: query['permissions._id'] = self.uid - projection = self.container_handler_configurations['acquisitions']['list_projection'] + projection = self.PHI_FIELDS acquisitions = list(containerstorage.AcquisitionStorage().get_all_el(query, None, projection)) diff --git a/api/handlers/containerhandler.py b/api/handlers/containerhandler.py index c519baba9..78ba202e1 100644 --- a/api/handlers/containerhandler.py +++ b/api/handlers/containerhandler.py @@ -42,7 +42,6 @@ class ContainerHandler(base.RequestHandler): 'sessions': True, 'acquisitions': True } - default_list_projection = ['files', 'notes', 'timestamp', 'timezone', 'public'] # Hard-coded PHI fields, will be changed to user set PHI fields PHI_FIELDS = {'info': 0, 'subject.firstname':0, 'subject.lastname': 0, 'subject.sex': 0, @@ -61,7 +60,6 @@ class ContainerHandler(base.RequestHandler): 'parent_storage': containerstorage.GroupStorage(), 'storage_schema_file': 'project.json', 'payload_schema_file': 'project.json', - 'list_projection': {'info': 0}, 'propagated_properties': ['archived', 'public'], 'children_cont': 'sessions' }, @@ -72,10 +70,6 @@ class ContainerHandler(base.RequestHandler): 'storage_schema_file': 'session.json', 'payload_schema_file': 'session.json', # Remove subject first/last from list view to better log access to this information - 'list_projection': {'info': 0, 'analyses': 0, 'subject.firstname': 0, - 'subject.lastname': 0, 'subject.sex': 0, 'subject.age': 0, - 'subject.race': 0, 'subject.ethnicity': 0, 'subject.info': 0, - 'files.info': 0, 'tags': 0}, 'propagated_properties': ['archived'], 'children_cont': 'acquisitions' }, @@ -84,8 +78,7 @@ class ContainerHandler(base.RequestHandler): 'permchecker': containerauth.default_container, 'parent_storage': containerstorage.SessionStorage(), 'storage_schema_file': 'acquisition.json', - 'payload_schema_file': 'acquisition.json', - 'list_projection': {'info': 0, 'collections': 0, 'files.info': 0, 'tags': 0} + 'payload_schema_file': 'acquisition.json' } } @@ -322,8 +315,6 @@ def get_all(self, cont_name, par_cont_name=None, par_id=None): self.config = self.container_handler_configurations[cont_name] self.storage = self.config['storage'] projection = None - # if self.is_true('info'): Seems redundant with new phi functionality - # projection.pop('info') if self.is_true('permissions'): if not projection: projection = None @@ -407,7 +398,13 @@ def _add_results_counts(self, results, cont_name): def get_all_for_user(self, cont_name, uid): self.config = self.container_handler_configurations[cont_name] self.storage = self.config['storage'] - projection = self.config['list_projection'] + projection = None + phi = False + if self.is_true('phi'): + phi = True + else: + projection = self.PHI_FIELDS + # select which permission filter will be applied to the list of results. if self.superuser_request or self.user_is_admin: permchecker = always_ok @@ -420,11 +417,14 @@ def get_all_for_user(self, cont_name, uid): '_id': uid } try: - results = permchecker(self.storage.exec_op)('GET', query=query, user=user, projection=projection) + results = permchecker(self.storage.exec_op)('GET', query=query, user=user, projection=projection, phi=phi) except APIStorageException as e: self.abort(400, e.message) if results is None: self.abort(404, 'Element not found in container {} {}'.format(self.storage.cont_name, uid)) + if phi: + for result in results: + self.log_user_access(AccessType.view_container, cont_name, result.get('_id')) self._filter_all_permissions(results, uid) return results From d9d019ebac317b014edd866d84d5e375d094c9f0 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 11 Oct 2017 17:06:37 -0500 Subject: [PATCH 10/17] no-phi -> phi-access; get_ses/acq on collections use phi --- api/auth/__init__.py | 6 ++-- api/auth/containerauth.py | 2 +- api/handlers/collectionshandler.py | 34 ++++++++++++++++--- api/handlers/containerhandler.py | 2 +- api/handlers/grouphandler.py | 2 +- api/handlers/listhandler.py | 4 +-- bin/database.py | 16 +++++++++ raml/schemas/definitions/permission.json | 4 +-- raml/schemas/input/permission.json | 2 +- .../integration_tests/abao/abao_test_hooks.js | 19 +++++++---- .../python/test_containers.py | 10 +++--- tests/integration_tests/python/test_groups.py | 4 +-- tests/integration_tests/python/test_jobs.py | 3 +- .../python/test_permissions.py | 8 +++-- .../python/test_propagation.py | 8 ++--- tests/integration_tests/python/test_rules.py | 2 +- 16 files changed, 89 insertions(+), 37 deletions(-) diff --git a/api/auth/__init__.py b/api/auth/__init__.py index 624ce811e..ba9a0d651 100644 --- a/api/auth/__init__.py +++ b/api/auth/__init__.py @@ -32,9 +32,9 @@ def has_access(uid, container, perm): def check_phi(uid, container): permissions_list = container.get('permissions', []) for perm in permissions_list: - if perm['_id'] == uid and perm.get('no-phi'): - return False - return has_access(uid, container, 'ro') + if perm['_id'] == uid and perm.get('phi-access'): + return has_access(uid, container, 'ro') + return False def always_ok(exec_op): """ diff --git a/api/auth/containerauth.py b/api/auth/containerauth.py index 4bf579480..9255418ff 100644 --- a/api/auth/containerauth.py +++ b/api/auth/containerauth.py @@ -142,7 +142,7 @@ def f(method, query=None, user=None, public=False, projection=None, phi=False): query['$or'] = [{'public': True}, {'permissions': query.pop('permissions')}] if phi: temp_query = deepcopy(query) - temp_query['permissions'] = {'$elemMatch': {'_id': handler.uid, 'no-phi': True}} + temp_query['permissions'] = {'$elemMatch': {'_id': handler.uid, 'phi-access': False}} not_allowed = exec_op(method, query=temp_query, user=user, public=public, projection=projection) if not_allowed: handler.abort(403, "User does not have PHI access to one or more elements") diff --git a/api/handlers/collectionshandler.py b/api/handlers/collectionshandler.py index 8d31bc1d4..98939e52b 100644 --- a/api/handlers/collectionshandler.py +++ b/api/handlers/collectionshandler.py @@ -41,7 +41,8 @@ def post(self): payload_validator(payload, 'POST') payload['permissions'] = [{ '_id': self.uid, - 'access': 'admin' + 'access': 'admin', + 'phi-access': True }] payload['curator'] = self.uid payload['created'] = payload['modified'] = datetime.datetime.utcnow() @@ -160,10 +161,21 @@ def get_sessions(self, cid): if not self.superuser_request: query['permissions._id'] = self.uid - projection = self.PHI_FIELDS + if self.superuser_request: + permchecker = always_ok + elif self.public_request: + permchecker = containerauth.list_public_request + else: + permchecker = containerauth.list_permission_checker(self) - sessions = list(containerstorage.SessionStorage().get_all_el(query, None, projection)) + projection = self.PHI_FIELDS + if self.is_true('phi'): + projection = None + phi = True + else: + phi = False + sessions = permchecker(self.storage.exec_op)('GET', query=query, public=self.public_request, projection=projection, phi=phi) self._filter_all_permissions(sessions, self.uid) if self.is_true('measurements'): self._add_session_measurements(sessions) @@ -192,11 +204,23 @@ def get_acquisitions(self, cid): if not self.superuser_request: query['permissions._id'] = self.uid + if self.superuser_request: + permchecker = always_ok + elif self.public_request: + permchecker = containerauth.list_public_request + else: + permchecker = containerauth.list_permission_checker(self) projection = self.PHI_FIELDS + if self.is_true('phi'): + projection = None + phi = True + else: + phi = False - acquisitions = list(containerstorage.AcquisitionStorage().get_all_el(query, None, projection)) - + log.debug(query) + log.debug(projection) + acquisitions = permchecker(self.storage.exec_op)('GET', query=query, public=self.public_request, projection=projection, phi=phi) self._filter_all_permissions(acquisitions, self.uid) for acquisition in acquisitions: diff --git a/api/handlers/containerhandler.py b/api/handlers/containerhandler.py index 78ba202e1..093957276 100644 --- a/api/handlers/containerhandler.py +++ b/api/handlers/containerhandler.py @@ -451,7 +451,7 @@ def post(self, cont_name): if self.is_true('inherit') and cont_name == 'projects': payload['permissions'] = parent_container.get('permissions') elif cont_name =='projects': - payload['permissions'] = [{'_id': self.uid, 'access': 'admin'}] if self.uid else [] + payload['permissions'] = [{'_id': self.uid, 'access': 'admin', 'phi-access':True}] if self.uid else [] else: payload['permissions'] = parent_container.get('permissions', []) # Created and modified timestamps are added here to the payload diff --git a/api/handlers/grouphandler.py b/api/handlers/grouphandler.py index a006b1209..52d57b000 100644 --- a/api/handlers/grouphandler.py +++ b/api/handlers/grouphandler.py @@ -83,7 +83,7 @@ def post(self): payload_validator = validators.from_schema_path(payload_schema_uri) payload_validator(payload, 'POST') payload['created'] = payload['modified'] = datetime.datetime.utcnow() - payload['permissions'] = [{'_id': self.uid, 'access': 'admin'}] if self.uid else [] + payload['permissions'] = [{'_id': self.uid, 'access': 'admin', 'phi-access':True}] if self.uid else [] result = mongo_validator(permchecker(self.storage.exec_op))('POST', payload=payload) if result.acknowledged: if result.upserted_id: diff --git a/api/handlers/listhandler.py b/api/handlers/listhandler.py index c6cea1c40..3bda975f8 100644 --- a/api/handlers/listhandler.py +++ b/api/handlers/listhandler.py @@ -215,7 +215,7 @@ def post(self, cont_name, list_name, **kwargs): payload = self.request.json_body if cont_name == 'groups' and self.request.params.get('propagate') =='true': - self._propagate_permissions(cont_name, _id, query={'permissions._id' : payload['_id']}, update={'$set': {'permissions.$.access': payload['access']}}) + self._propagate_permissions(cont_name, _id, query={'permissions._id' : payload['_id']}, update={'$set': {'permissions.$.access': payload['access'], 'permissions.$.phi-access': payload['phi-access']}}) self._propagate_permissions(cont_name, _id, query={'permissions._id': {'$ne': payload['_id']}}, update={'$addToSet': {'permissions': payload}}) elif cont_name != 'groups': self._propagate_permissions(cont_name, _id) @@ -228,7 +228,7 @@ def put(self, cont_name, list_name, **kwargs): payload = self.request.json_body payload['_id'] = kwargs.get('_id') if cont_name == 'groups' and self.request.params.get('propagate') =='true': - self._propagate_permissions(cont_name, _id, query={'permissions._id' : payload['_id']}, update={'$set': {'permissions.$.access': payload['access']}}) + self._propagate_permissions(cont_name, _id, query={'permissions._id' : payload['_id']}, update={'$set': {'permissions.$.access': payload['access'], 'permissions.$.phi-access': payload['phi-access']}}) self._propagate_permissions(cont_name, _id, query={'permissions._id': {'$ne': payload['_id']}}, update={'$addToSet': {'permissions': payload}}) elif cont_name != 'groups': self._propagate_permissions(cont_name, _id) diff --git a/bin/database.py b/bin/database.py index 57672b556..13e980a54 100755 --- a/bin/database.py +++ b/bin/database.py @@ -1219,6 +1219,22 @@ def upgrade_to_37(): cursor = config.db[coll].find({'permissions.site': {'$exists': True}}) process_cursor(cursor, upgrade_to_32_closure, context = coll) +def upgrade_to_38_closure(coll_item, coll): + permissions = coll_item.get('permissions', []) + for permission_ in permissions: + permission_['phi-access'] = True + result = config.db[coll].update_one({'_id': coll_item['_id']}, {'$set': {'permissions' : permissions}}) + if result.modified_count == 0: + return "Failed to add phi access to permissions in {} container {}".format(coll_item.get("_id"), coll) + return True + +def upgrade_to_38(): + """ + permissions now have a mandatory phi-access field + """ + for coll in ['acquisitions', 'groups', 'projects', 'sessions', 'analyses']: + cursor = config.db[coll].find({'permissions.site': {'$exists': True}}) + process_cursor(cursor, upgrade_to_38_closure, context = coll) def upgrade_to_38_closure(user): diff --git a/raml/schemas/definitions/permission.json b/raml/schemas/definitions/permission.json index 51340e36b..51b7e85c0 100644 --- a/raml/schemas/definitions/permission.json +++ b/raml/schemas/definitions/permission.json @@ -9,13 +9,13 @@ "properties":{ "_id":{"$ref":"#/definitions/_id"}, "access":{"$ref":"#/definitions/access"}, - "no-phi":{"type": "boolean"} + "phi-access":{"type": "boolean"} }, "additionalProperties": false }, "permission-output-default-required":{ "allOf":[{"$ref":"#/definitions/permission"}], - "required":["_id", "access"] + "required":["_id", "access", "phi-access"] } } } diff --git a/raml/schemas/input/permission.json b/raml/schemas/input/permission.json index 85172151f..2ccfe500f 100644 --- a/raml/schemas/input/permission.json +++ b/raml/schemas/input/permission.json @@ -3,5 +3,5 @@ "type": "object", "allOf":[{"$ref":"../definitions/permission.json#/definitions/permission"}], "key_fields": ["_id"], - "required": ["_id", "access"] + "required": ["_id", "access", "phi-access"] } diff --git a/tests/integration_tests/abao/abao_test_hooks.js b/tests/integration_tests/abao/abao_test_hooks.js index c12cbe5a8..1ad38fad5 100644 --- a/tests/integration_tests/abao/abao_test_hooks.js +++ b/tests/integration_tests/abao/abao_test_hooks.js @@ -218,7 +218,8 @@ hooks.before("POST /groups/{GroupId}/permissions -> 200", function(test, done) { }; test.request.body = { _id: "test@user.com", - access: "ro" + access: "ro", + "phi-access": true } done(); }); @@ -246,7 +247,8 @@ hooks.before("PUT /groups/{GroupId}/permissions/{UserId} -> 200", function(test, }; test.request.body = { _id: "test@user.com", - access: "admin" + access: "admin", + "phi-access": true }; done(); }); @@ -259,6 +261,7 @@ hooks.before("PUT /groups/{GroupId}/permissions/{UserId} -> 400", function(test, test.request.body = { _id: "test@user.com", access: "rw", + "phi-access": true, not_a_real_property: "foo" }; done(); @@ -466,7 +469,8 @@ hooks.before("POST /collections/{CollectionId}/permissions -> 200", function(tes }; test.request.body = { "_id":"test@user.com", - "access":"ro" + "access":"ro", + "phi-access": true }; done(); }); @@ -496,7 +500,8 @@ hooks.before("PUT /collections/{CollectionId}/permissions/{UserId} -> 200", func }; test.request.body = { "access":"rw", - "_id":"test@user.com" + "_id":"test@user.com", + "phi-access": true }; done(); }); @@ -1266,7 +1271,8 @@ hooks.before("POST /projects/{ProjectId}/permissions -> 200", function(test, don }; test.request.body = { "_id":"test@user.com", - "access":"ro" + "access":"ro", + "phi-access": true }; done(); }); @@ -1296,7 +1302,8 @@ hooks.before("PUT /projects/{ProjectId}/permissions/{UserId} -> 200", function(t }; test.request.body = { "access":"rw", - "_id":"test@user.com" + "_id":"test@user.com", + "phi-access": true }; done(); }); diff --git a/tests/integration_tests/python/test_containers.py b/tests/integration_tests/python/test_containers.py index c06c98f7c..ad06aaebd 100644 --- a/tests/integration_tests/python/test_containers.py +++ b/tests/integration_tests/python/test_containers.py @@ -296,9 +296,9 @@ def test_phi_access(as_user, as_admin, data_builder, log_db): assert pre_log == log_db.access_log.count({}) - 2 # Set no-phi flag to true - r = as_admin.put('/projects/' + project + '/permissions/admin@user.com', json={'no-phi': True}) + r = as_admin.put('/projects/' + project + '/permissions/admin@user.com', json={'phi-access': False}) assert r.ok - r = as_admin.post('/projects/' + project + '/permissions', json={'access': 'ro', 'no-phi': True, '_id': 'user@user.com'}) + r = as_admin.post('/projects/' + project + '/permissions', json={'access': 'ro', 'phi-access': False, '_id': 'user@user.com'}) assert r.ok # Test phi access for list returns without phi access level @@ -476,7 +476,8 @@ def test_post_container(data_builder, as_admin, as_user): r = as_admin.post('/groups/' + group + '/permissions', json={ '_id': uid, - 'access': 'rw' + 'access': 'rw', + 'phi-access': True }) assert r.ok @@ -494,7 +495,8 @@ def test_post_container(data_builder, as_admin, as_user): r = as_admin.post('/projects/' + project + '/permissions', json={ '_id': uid, - 'access': 'rw' + 'access': 'rw', + 'phi-access': True }) assert r.ok diff --git a/tests/integration_tests/python/test_groups.py b/tests/integration_tests/python/test_groups.py index 94cfe620e..8ba485782 100644 --- a/tests/integration_tests/python/test_groups.py +++ b/tests/integration_tests/python/test_groups.py @@ -67,7 +67,7 @@ def test_groups(as_user, as_admin, data_builder): assert d5 > d4 # Add a permission to the group - user = {'access': 'rw', '_id': 'newUser@fakeuser.com'} + user = {'access': 'rw', '_id': 'newUser@fakeuser.com', 'phi-access': True} r = as_admin.post('/groups/' + group + '/permissions', json=user) assert r.ok @@ -107,7 +107,7 @@ def test_groups(as_user, as_admin, data_builder): assert d8 > d7 group2 = data_builder.create_group() - r = as_admin.post('/groups/' + group2 + '/permissions', json={'access':'admin','_id':'user@user.com'}) + r = as_admin.post('/groups/' + group2 + '/permissions', json={'access':'admin','_id':'user@user.com', 'phi-access': True}) assert r.ok r = as_user.get('/groups') assert r.ok diff --git a/tests/integration_tests/python/test_jobs.py b/tests/integration_tests/python/test_jobs.py index 16a0a04d3..20e934ba4 100644 --- a/tests/integration_tests/python/test_jobs.py +++ b/tests/integration_tests/python/test_jobs.py @@ -183,7 +183,8 @@ def test_jobs(data_builder, default_payload, as_public, as_user, as_admin, as_ro r = as_admin.post('/projects/' + project + '/permissions', json={ '_id': uid, - 'access': 'ro' + 'access': 'ro', + 'phi-access': True }) assert r.ok diff --git a/tests/integration_tests/python/test_permissions.py b/tests/integration_tests/python/test_permissions.py index 3eab504be..f3743ff57 100644 --- a/tests/integration_tests/python/test_permissions.py +++ b/tests/integration_tests/python/test_permissions.py @@ -14,7 +14,8 @@ def test_permissions(data_builder, as_admin): # Add permissions for user 1 r = as_admin.post(permissions_path, json={ '_id': user_1, - 'access': 'ro' + 'access': 'ro', + 'phi-access': True }) assert r.ok @@ -32,7 +33,8 @@ def test_permissions(data_builder, as_admin): # Add user 2 to have ro access r = as_admin.post(permissions_path, json={ '_id': user_2, - 'access': 'ro' + 'access': 'ro', + 'phi-access': True }) assert r.ok @@ -66,7 +68,7 @@ def test_group_permissions(data_builder, as_admin, as_public): assert r.status_code == 404 # Create permission for user - r = as_admin.post(permissions_path, json={'_id': user, 'access': 'rw'}) + r = as_admin.post(permissions_path, json={'_id': user, 'access': 'rw', 'phi-access': True}) assert r.ok # Verify new user permission diff --git a/tests/integration_tests/python/test_propagation.py b/tests/integration_tests/python/test_propagation.py index 81c8cb580..994f6b595 100644 --- a/tests/integration_tests/python/test_propagation.py +++ b/tests/integration_tests/python/test_propagation.py @@ -127,7 +127,7 @@ def get_user_in_perms(perms, uid): user_id = 'propagation@user.com' # Add user to project permissions - payload = {'_id': user_id, 'access': 'admin'} + payload = {'_id': user_id, 'access': 'admin', 'phi-access': True} r = as_admin.post('/projects/' + project + '/permissions', json=payload) assert r.ok @@ -147,7 +147,7 @@ def get_user_in_perms(perms, uid): assert r.ok and user # Modify user permissions - payload = {'access': 'rw', '_id': user_id} + payload = {'access': 'rw', '_id': user_id, 'phi-access': True} r = as_admin.put('/projects/' + project + '/permissions/' + user_id, json=payload) assert r.ok @@ -208,7 +208,7 @@ def get_user_in_perms(perms, uid): user_id = 'propagation@user.com' # Add user to group permissions - payload = {'_id': user_id, 'access': 'admin'} + payload = {'_id': user_id, 'access': 'admin', 'phi-access': True} r = as_admin.post('/groups/' + group + '/permissions', json=payload, params={'propagate': 'true'}) assert r.ok @@ -240,7 +240,7 @@ def get_user_in_perms(perms, uid): assert r.ok and user # Modify user permissions - payload = {'access': 'rw', '_id': user_id} + payload = {'access': 'rw', '_id': user_id, 'phi-access':True} r = as_admin.put('/groups/' + group + '/permissions/' + user_id, json=payload, params={'propagate': 'true'}) assert r.ok diff --git a/tests/integration_tests/python/test_rules.py b/tests/integration_tests/python/test_rules.py index a69c7900b..78a6a8f1b 100644 --- a/tests/integration_tests/python/test_rules.py +++ b/tests/integration_tests/python/test_rules.py @@ -265,7 +265,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a # add read-only perms for user r = as_admin.post('/projects/' + project + '/permissions', json={ - '_id': with_user.user, 'access': 'ro'}) + '_id': with_user.user, 'access': 'ro', 'phi-access': True}) assert r.ok # try to add rule w/ read-only project perms From 2dcc6e9e0da0f1b14a016dfacc2d9a239b97a65b Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 25 Oct 2017 12:12:20 -0500 Subject: [PATCH 11/17] Tested root access to phi fields --- tests/integration_tests/python/test_containers.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/integration_tests/python/test_containers.py b/tests/integration_tests/python/test_containers.py index ad06aaebd..4f98a8af7 100644 --- a/tests/integration_tests/python/test_containers.py +++ b/tests/integration_tests/python/test_containers.py @@ -262,7 +262,7 @@ def test_get_all_for_user(as_admin, as_public): r = as_admin.get('/users/' + user_id + '/sessions') assert r.ok -def test_phi_access(as_user, as_admin, data_builder, log_db): +def test_phi_access(as_user, as_admin, as_root, data_builder, log_db): group = data_builder.create_group() project = data_builder.create_project() session = data_builder.create_session() @@ -314,11 +314,11 @@ def test_phi_access(as_user, as_admin, data_builder, log_db): # Test phi access for individual elements without phi access level but with super_user pre_log = log_db.access_log.count({}) - r = as_admin.get('/sessions/' + session) + r = as_root.get('/sessions/' + session) assert r.ok - assert r.json().get('subject').get('firstname') == None + assert r.json().get('subject').get('firstname') == "FirstName" assert r.json().get('subject').get('code') == 'Subject_Code' - assert pre_log == log_db.access_log.count({}) + assert pre_log == log_db.access_log.count({}) - 1 r = as_admin.get('/sessions/' + session, params={'phi':True}) assert r.status_code == 200 From 3d1438bc59616249b257792e87cc78539b2954df Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 25 Oct 2017 15:04:30 -0500 Subject: [PATCH 12/17] self.PHI for collections removing files.info projection --- api/dao/basecontainerstorage.py | 4 ++- api/handlers/collectionshandler.py | 4 +-- api/handlers/containerhandler.py | 11 +++++-- api/handlers/refererhandler.py | 2 +- .../python/test_collection.py | 29 +++++++++++++++++-- 5 files changed, 41 insertions(+), 9 deletions(-) diff --git a/api/dao/basecontainerstorage.py b/api/dao/basecontainerstorage.py index 0785fbcb7..23297cf5c 100644 --- a/api/dao/basecontainerstorage.py +++ b/api/dao/basecontainerstorage.py @@ -215,7 +215,7 @@ def get_all_el(self, query, user, projection, fill_defaults=False): # if projection includes files.info, add new key `info_exists` if projection and 'files.info' in projection: replace_info_with_bool = True - projection.pop('files.info') + file_info_projection = projection.pop('files.info') else: replace_info_with_bool = False @@ -227,6 +227,8 @@ def get_all_el(self, query, user, projection, fill_defaults=False): if replace_info_with_bool: for f in cont.get('files', []): f['info_exists'] = bool(f.pop('info', False)) + if replace_info_with_bool: + projection['files.info'] = file_info_projection return results def modify_info(self, _id, payload, modify_subject=False): diff --git a/api/handlers/collectionshandler.py b/api/handlers/collectionshandler.py index 98939e52b..f27f1e027 100644 --- a/api/handlers/collectionshandler.py +++ b/api/handlers/collectionshandler.py @@ -168,7 +168,7 @@ def get_sessions(self, cid): else: permchecker = containerauth.list_permission_checker(self) - projection = self.PHI_FIELDS + projection = self.PHI_FIELDS.copy() if self.is_true('phi'): projection = None phi = True @@ -211,7 +211,7 @@ def get_acquisitions(self, cid): else: permchecker = containerauth.list_permission_checker(self) - projection = self.PHI_FIELDS + projection = self.PHI_FIELDS.copy() if self.is_true('phi'): projection = None phi = True diff --git a/api/handlers/containerhandler.py b/api/handlers/containerhandler.py index 093957276..44b452821 100644 --- a/api/handlers/containerhandler.py +++ b/api/handlers/containerhandler.py @@ -92,11 +92,16 @@ def __init__(self, request=None, response=None): @log_access(AccessType.view_container) def get(self, cont_name, **kwargs): _id = kwargs.get('cid') - projection = self.PHI_FIELDS + projection = self.PHI_FIELDS.copy() + log.debug(self.PHI_FIELDS) + log.debug(projection) self.config = self.container_handler_configurations[cont_name] self.storage = self.config['storage'] container = self._get_container(_id) + log.debug(container) if check_phi(self.uid, container) or self.superuser_request: + log.debug("PHI") + log.debug(self.superuser_request) self.phi = True projection = None @@ -323,7 +328,7 @@ def get_all(self, cont_name, par_cont_name=None, par_id=None): else: phi = False if projection == None: - projection = self.PHI_FIELDS + projection = self.PHI_FIELDS.copy() else: projection.update(self.PHI_FIELDS) # select which permission filter will be applied to the list of results. @@ -403,7 +408,7 @@ def get_all_for_user(self, cont_name, uid): if self.is_true('phi'): phi = True else: - projection = self.PHI_FIELDS + projection = self.PHI_FIELDS.copy() # select which permission filter will be applied to the list of results. if self.superuser_request or self.user_is_admin: diff --git a/api/handlers/refererhandler.py b/api/handlers/refererhandler.py index c4dcf2267..c747ffd96 100644 --- a/api/handlers/refererhandler.py +++ b/api/handlers/refererhandler.py @@ -136,7 +136,7 @@ def get(self, **kwargs): _id = kwargs.get('_id') analysis = self.storage.get_container(_id) parent = self.storage.get_parent(analysis['parent']['type'], analysis['parent']['id']) - projection = self.PHI_FIELDS + projection = self.PHI_FIELDS.copy() if check_phi(self.uid, parent)or self.superuser_request: self.phi = True projection = None diff --git a/tests/integration_tests/python/test_collection.py b/tests/integration_tests/python/test_collection.py index 5e3fa1edc..2ae98118d 100644 --- a/tests/integration_tests/python/test_collection.py +++ b/tests/integration_tests/python/test_collection.py @@ -114,7 +114,7 @@ def test_collections(data_builder, as_admin, as_user): r = as_admin.get('/acquisitions/' + acquisition) assert collection not in r.json()['collections'] -def test_collections_phi(data_builder, as_admin, as_user, log_db, file_form): +def test_collections_phi(data_builder, as_admin, as_user, as_root, log_db, file_form): session = data_builder.create_session() acquisition = data_builder.create_acquisition() @@ -167,9 +167,34 @@ def test_collections_phi(data_builder, as_admin, as_user, log_db, file_form): assert r.ok assert r.json().get('files',[{}])[0].get('info').get('a') == "b" assert pre_log == log_db.access_log.count({}) - 1 + + # Test phi access without phi access + r = as_admin.put("/collections/" + collection + "/permissions/admin@user.com", json={"phi-access":False}) + assert r.ok + r = as_admin.get("/collections/" + collection + "/permissions/admin@user.com") + assert r.ok + assert r.json().get("phi-access") == False + + pre_log = log_db.access_log.count({}) + r = as_admin.get('/collections', params={"phi":False}) + assert r.ok + for collection_ in r.json(): + assert collection_.get('files',[{}])[0].get('info') == None + assert pre_log == log_db.access_log.count({}) + r = as_admin.get('/collections', params={'phi':True}) + assert r.status_code == 403 + + # Test phi access for individual elements without phi access level + pre_log = log_db.access_log.count({}) + r = as_admin.get('/collections/' + collection) + assert r.ok + assert r.json().get('files',[{}])[0].get('info') == None + assert pre_log == log_db.access_log.count({}) pre_log = log_db.access_log.count({}) - r = as_admin.get('/collections/' + collection, params={'phi':True}) + # Test phi access for individual elements without phi access level but with root + r = as_root.get('/collections/' + collection) assert r.ok assert r.json().get('files',[{}])[0].get('info').get('a') == "b" assert pre_log == log_db.access_log.count({}) - 1 + From dae084cbf7fbe4b8c4c44fe74d57548a73e94658 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Fri, 27 Oct 2017 14:06:11 -0500 Subject: [PATCH 13/17] self.PHI is not modified --- api/dao/basecontainerstorage.py | 6 ++-- api/handlers/containerhandler.py | 2 +- raml/examples/output/acquisition-list.json | 6 ++-- raml/examples/output/acquisition.json | 3 +- raml/examples/output/collection-list.json | 3 +- raml/examples/output/collection.json | 3 +- raml/examples/output/groups-list.json | 9 ++++-- raml/examples/output/permission.json | 3 +- raml/examples/output/project-list.json | 19 ++++++++++-- raml/examples/output/project.json | 3 +- raml/examples/output/session-list.json | 9 ++++-- raml/examples/output/session.json | 3 +- raml/schemas/definitions/permission.json | 5 ++-- .../python/test_collection.py | 30 +++---------------- .../python/test_containers.py | 3 ++ 15 files changed, 58 insertions(+), 49 deletions(-) diff --git a/api/dao/basecontainerstorage.py b/api/dao/basecontainerstorage.py index 23297cf5c..5d0691b54 100644 --- a/api/dao/basecontainerstorage.py +++ b/api/dao/basecontainerstorage.py @@ -215,7 +215,7 @@ def get_all_el(self, query, user, projection, fill_defaults=False): # if projection includes files.info, add new key `info_exists` if projection and 'files.info' in projection: replace_info_with_bool = True - file_info_projection = projection.pop('files.info') + projection.pop('files.info') else: replace_info_with_bool = False @@ -227,8 +227,8 @@ def get_all_el(self, query, user, projection, fill_defaults=False): if replace_info_with_bool: for f in cont.get('files', []): f['info_exists'] = bool(f.pop('info', False)) - if replace_info_with_bool: - projection['files.info'] = file_info_projection + # if replace_info_with_bool: + # projection['files.info'] = file_info_projection return results def modify_info(self, _id, payload, modify_subject=False): diff --git a/api/handlers/containerhandler.py b/api/handlers/containerhandler.py index 44b452821..4825dcb09 100644 --- a/api/handlers/containerhandler.py +++ b/api/handlers/containerhandler.py @@ -123,7 +123,7 @@ def get(self, cont_name, **kwargs): fileinfo['path'] = util.path_from_hash(fileinfo['hash']) inflate_job_info = cont_name == 'sessions' - result['analyses'] = AnalysisStorage().get_analyses(cont_name, _id, inflate_job_info, self.PHI_FIELDS) + result['analyses'] = AnalysisStorage().get_analyses(cont_name, _id, inflate_job_info, self.PHI_FIELDS.copy()) return self.handle_origin(result) def handle_origin(self, result): diff --git a/raml/examples/output/acquisition-list.json b/raml/examples/output/acquisition-list.json index 1b29d562c..17006a169 100644 --- a/raml/examples/output/acquisition-list.json +++ b/raml/examples/output/acquisition-list.json @@ -73,7 +73,8 @@ "permissions": [ { "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-access": true } ] }, @@ -112,7 +113,8 @@ "permissions": [ { "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-access": true } ] } diff --git a/raml/examples/output/acquisition.json b/raml/examples/output/acquisition.json index fef6ba738..f7b61f7fb 100644 --- a/raml/examples/output/acquisition.json +++ b/raml/examples/output/acquisition.json @@ -33,7 +33,8 @@ "permissions": [ { "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-access": true } ] } diff --git a/raml/examples/output/collection-list.json b/raml/examples/output/collection-list.json index e766d50c0..5d22f5211 100644 --- a/raml/examples/output/collection-list.json +++ b/raml/examples/output/collection-list.json @@ -6,6 +6,7 @@ "_id": "57c5c0bd9e512c606dd3df09", "permissions": [{ "access": "admin", - "_id": "user@test.com" + "_id": "user@test.com", + "phi-access": true }] }] diff --git a/raml/examples/output/collection.json b/raml/examples/output/collection.json index 445be806b..6a6c999e0 100644 --- a/raml/examples/output/collection.json +++ b/raml/examples/output/collection.json @@ -7,6 +7,7 @@ "_id": "57c5c0bd9e512c606dd3df09", "permissions": [{ "access": "admin", - "_id": "user@test.com" + "_id": "user@test.com", + "phi-access": true }] } diff --git a/raml/examples/output/groups-list.json b/raml/examples/output/groups-list.json index 5f89d59f5..8901c3471 100644 --- a/raml/examples/output/groups-list.json +++ b/raml/examples/output/groups-list.json @@ -4,15 +4,18 @@ "permissions": [ { "access": "admin", - "_id": "group_admin@fakeuser.com" + "_id": "group_admin@fakeuser.com", + "phi-access": true }, { "access": "rw", - "_id": "group_member_read-write@fakeuser.com" + "_id": "group_member_read-write@fakeuser.com", + "phi-access": true }, { "access": "ro", - "_id": "group_member_read-only@fakeuser.com" + "_id": "group_member_read-only@fakeuser.com", + "phi-access": true } ], "created": "2016-08-19T11:41:15.360000+00:00", diff --git a/raml/examples/output/permission.json b/raml/examples/output/permission.json index ae383f550..4b227be0b 100644 --- a/raml/examples/output/permission.json +++ b/raml/examples/output/permission.json @@ -1,4 +1,5 @@ { "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-acces": true } diff --git a/raml/examples/output/project-list.json b/raml/examples/output/project-list.json index a597be608..aa3f4663a 100644 --- a/raml/examples/output/project-list.json +++ b/raml/examples/output/project-list.json @@ -7,7 +7,12 @@ "public": false, "permissions": [{ "access": "admin", +<<<<<<< HEAD "_id": "coltonlw@flywheel.io" +======= + "_id": "coltonlw@flywheel.io", + "phi-access": true +>>>>>>> 1662a602... self.PHI is not modified }] }, { "group": "scitran", @@ -18,7 +23,12 @@ "public": false, "permissions": [{ "access": "admin", +<<<<<<< HEAD "_id": "coltonlw@flywheel.io" +======= + "_id": "coltonlw@flywheel.io", + "phi-access": true +>>>>>>> 1662a602... self.PHI is not modified }] }, { "group": "scitran", @@ -29,7 +39,8 @@ "public": false, "permissions": [{ "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-access": true }] }, { "group": "test-group", @@ -41,7 +52,8 @@ "public": false, "permissions": [{ "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-access": true }] }, { "group": "scitran", @@ -52,6 +64,7 @@ "_id": "57ed6312466d8e01c91ee427", "permissions": [{ "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-access": true }] }] diff --git a/raml/examples/output/project.json b/raml/examples/output/project.json index ea610b4d1..af8d5fb5e 100644 --- a/raml/examples/output/project.json +++ b/raml/examples/output/project.json @@ -8,6 +8,7 @@ "public": false, "permissions": [{ "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-access": true }] } diff --git a/raml/examples/output/session-list.json b/raml/examples/output/session-list.json index 9f6cbc64f..de4e7842b 100644 --- a/raml/examples/output/session-list.json +++ b/raml/examples/output/session-list.json @@ -12,7 +12,8 @@ "public": false, "permissions": [{ "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-access": true }] }, { "group": "scitran", @@ -28,7 +29,8 @@ "public": false, "permissions": [{ "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-access": true }] }, { "group": "scitran", @@ -44,6 +46,7 @@ "public": false, "permissions": [{ "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-access": true }] }] diff --git a/raml/examples/output/session.json b/raml/examples/output/session.json index 74dc0a6f5..0d2304663 100644 --- a/raml/examples/output/session.json +++ b/raml/examples/output/session.json @@ -12,6 +12,7 @@ "public": false, "permissions": [{ "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-access": true }] } diff --git a/raml/schemas/definitions/permission.json b/raml/schemas/definitions/permission.json index 51b7e85c0..c195b31e8 100644 --- a/raml/schemas/definitions/permission.json +++ b/raml/schemas/definitions/permission.json @@ -4,18 +4,19 @@ "definitions": { "_id": { "type": "string" }, "access": { "enum": ["ro", "rw", "admin"] }, + "phi-access": {"type": "boolean"}, "permission":{ "type":"object", "properties":{ "_id":{"$ref":"#/definitions/_id"}, "access":{"$ref":"#/definitions/access"}, - "phi-access":{"type": "boolean"} + "phi-access":{"$ref":"#/definitions/phi-access"} }, "additionalProperties": false }, "permission-output-default-required":{ "allOf":[{"$ref":"#/definitions/permission"}], - "required":["_id", "access", "phi-access"] + "required":["_id", "access"] } } } diff --git a/tests/integration_tests/python/test_collection.py b/tests/integration_tests/python/test_collection.py index 2ae98118d..ff018ae13 100644 --- a/tests/integration_tests/python/test_collection.py +++ b/tests/integration_tests/python/test_collection.py @@ -114,7 +114,7 @@ def test_collections(data_builder, as_admin, as_user): r = as_admin.get('/acquisitions/' + acquisition) assert collection not in r.json()['collections'] -def test_collections_phi(data_builder, as_admin, as_user, as_root, log_db, file_form): +def test_collections_phi(data_builder, as_admin, as_user, log_db, file_form): session = data_builder.create_session() acquisition = data_builder.create_acquisition() @@ -167,34 +167,12 @@ def test_collections_phi(data_builder, as_admin, as_user, as_root, log_db, file_ assert r.ok assert r.json().get('files',[{}])[0].get('info').get('a') == "b" assert pre_log == log_db.access_log.count({}) - 1 - - # Test phi access without phi access - r = as_admin.put("/collections/" + collection + "/permissions/admin@user.com", json={"phi-access":False}) - assert r.ok - r = as_admin.get("/collections/" + collection + "/permissions/admin@user.com") - assert r.ok - assert r.json().get("phi-access") == False - - pre_log = log_db.access_log.count({}) - r = as_admin.get('/collections', params={"phi":False}) - assert r.ok - for collection_ in r.json(): - assert collection_.get('files',[{}])[0].get('info') == None - assert pre_log == log_db.access_log.count({}) - r = as_admin.get('/collections', params={'phi':True}) - assert r.status_code == 403 - - # Test phi access for individual elements without phi access level - pre_log = log_db.access_log.count({}) - r = as_admin.get('/collections/' + collection) - assert r.ok - assert r.json().get('files',[{}])[0].get('info') == None - assert pre_log == log_db.access_log.count({}) pre_log = log_db.access_log.count({}) - # Test phi access for individual elements without phi access level but with root - r = as_root.get('/collections/' + collection) + r = as_admin.get('/collections/' + collection, params={'phi':True}) assert r.ok assert r.json().get('files',[{}])[0].get('info').get('a') == "b" assert pre_log == log_db.access_log.count({}) - 1 + r = as_admin.delete('/collections/'+ collection) + assert r.ok diff --git a/tests/integration_tests/python/test_containers.py b/tests/integration_tests/python/test_containers.py index 4f98a8af7..429f0e464 100644 --- a/tests/integration_tests/python/test_containers.py +++ b/tests/integration_tests/python/test_containers.py @@ -335,6 +335,9 @@ def test_phi_access(as_user, as_admin, as_root, data_builder, log_db): r = as_user.get('/sessions/' + session, params={'phi':True}) assert r.status_code == 200 + r = as_admin.delete('/sessions/' + session) + assert r.ok + def test_get_container(data_builder, default_payload, file_form, as_drone, as_admin, as_public, api_db): project = data_builder.create_project() From 82e1dc7b58618ace1dff895c26bb84f22ea93624 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 1 Nov 2017 10:27:47 -0500 Subject: [PATCH 14/17] allow additional properties on output schemas --- api/auth/containerauth.py | 4 +++ api/dao/basecontainerstorage.py | 2 -- raml/schemas/definitions/acquisition.json | 2 +- raml/schemas/definitions/analysis.json | 2 +- raml/schemas/definitions/collection.json | 2 +- raml/schemas/definitions/file.json | 2 +- raml/schemas/definitions/project.json | 2 +- raml/schemas/definitions/session.json | 2 +- .../python/test_collection.py | 35 ++++++++++++++++++- 9 files changed, 44 insertions(+), 9 deletions(-) diff --git a/api/auth/containerauth.py b/api/auth/containerauth.py index 9255418ff..05a8e6c0c 100644 --- a/api/auth/containerauth.py +++ b/api/auth/containerauth.py @@ -3,6 +3,8 @@ """ from copy import deepcopy from . import _get_access, check_phi, INTEGER_PERMISSIONS +from .. import config +log = config.log def default_container(handler, container=None, target_parent_container=None): @@ -143,6 +145,8 @@ def f(method, query=None, user=None, public=False, projection=None, phi=False): if phi: temp_query = deepcopy(query) temp_query['permissions'] = {'$elemMatch': {'_id': handler.uid, 'phi-access': False}} + log.debug(temp_query) + log.debug(query) not_allowed = exec_op(method, query=temp_query, user=user, public=public, projection=projection) if not_allowed: handler.abort(403, "User does not have PHI access to one or more elements") diff --git a/api/dao/basecontainerstorage.py b/api/dao/basecontainerstorage.py index 5d0691b54..0785fbcb7 100644 --- a/api/dao/basecontainerstorage.py +++ b/api/dao/basecontainerstorage.py @@ -227,8 +227,6 @@ def get_all_el(self, query, user, projection, fill_defaults=False): if replace_info_with_bool: for f in cont.get('files', []): f['info_exists'] = bool(f.pop('info', False)) - # if replace_info_with_bool: - # projection['files.info'] = file_info_projection return results def modify_info(self, _id, payload, modify_subject=False): diff --git a/raml/schemas/definitions/acquisition.json b/raml/schemas/definitions/acquisition.json index c04a6679a..7df51b79b 100644 --- a/raml/schemas/definitions/acquisition.json +++ b/raml/schemas/definitions/acquisition.json @@ -51,7 +51,7 @@ } } }, - "additionalProperties":false + "additionalProperties":true } } } diff --git a/raml/schemas/definitions/analysis.json b/raml/schemas/definitions/analysis.json index 1a8da4353..b6c568d46 100644 --- a/raml/schemas/definitions/analysis.json +++ b/raml/schemas/definitions/analysis.json @@ -67,7 +67,7 @@ "input":{"type":"boolean"}, "output":{"type":"boolean"} }, - "additionalProperties":false + "additionalProperties":true } }, "job":{ diff --git a/raml/schemas/definitions/collection.json b/raml/schemas/definitions/collection.json index 5bb0b652c..54b874c34 100644 --- a/raml/schemas/definitions/collection.json +++ b/raml/schemas/definitions/collection.json @@ -55,7 +55,7 @@ } } }, - "additionalProperties":false + "additionalProperties":true } } } diff --git a/raml/schemas/definitions/file.json b/raml/schemas/definitions/file.json index 49cde25e7..20832055e 100644 --- a/raml/schemas/definitions/file.json +++ b/raml/schemas/definitions/file.json @@ -83,7 +83,7 @@ "size":{"$ref":"#/definitions/size"}, "info_exists": {"type": "boolean"} }, - "additionalProperties": false, + "additionalProperties": true, "required":["modified", "size"] } } diff --git a/raml/schemas/definitions/project.json b/raml/schemas/definitions/project.json index 0c3c21a1a..d30f22e1c 100644 --- a/raml/schemas/definitions/project.json +++ b/raml/schemas/definitions/project.json @@ -46,7 +46,7 @@ } } }, - "additionalProperties": false + "additionalProperties": true } } } diff --git a/raml/schemas/definitions/session.json b/raml/schemas/definitions/session.json index 827272908..88c83fc55 100644 --- a/raml/schemas/definitions/session.json +++ b/raml/schemas/definitions/session.json @@ -65,7 +65,7 @@ } } }, - "additionalProperties": false + "additionalProperties": true } } } diff --git a/tests/integration_tests/python/test_collection.py b/tests/integration_tests/python/test_collection.py index ff018ae13..29896882b 100644 --- a/tests/integration_tests/python/test_collection.py +++ b/tests/integration_tests/python/test_collection.py @@ -114,7 +114,7 @@ def test_collections(data_builder, as_admin, as_user): r = as_admin.get('/acquisitions/' + acquisition) assert collection not in r.json()['collections'] -def test_collections_phi(data_builder, as_admin, as_user, log_db, file_form): +def test_collections_phi(data_builder, as_admin, as_user, log_db, as_root,file_form): session = data_builder.create_session() acquisition = data_builder.create_acquisition() @@ -174,5 +174,38 @@ def test_collections_phi(data_builder, as_admin, as_user, log_db, file_form): assert r.json().get('files',[{}])[0].get('info').get('a') == "b" assert pre_log == log_db.access_log.count({}) - 1 + + # Test phi access without phi access + r = as_admin.put("/collections/" + collection + "/permissions/admin@user.com", json={"phi-access":False}) + assert r.ok + r = as_admin.get("/collections/" + collection + "/permissions/admin@user.com") + assert r.ok + assert r.json().get("phi-access") == False + + pre_log = log_db.access_log.count({}) + r = as_admin.get('/collections', params={"phi":False}) + assert r.ok + for collection_ in r.json(): + assert collection_.get('files',[{}])[0].get('info') == None + assert pre_log == log_db.access_log.count({}) + r = as_admin.get('/collections', params={'phi':True}) + assert r.status_code == 403 + + # Test phi access for individual elements without phi access level + pre_log = log_db.access_log.count({}) + r = as_admin.get('/collections/' + collection) + assert r.ok + assert r.json().get('files',[{}])[0].get('info') == None + assert pre_log == log_db.access_log.count({}) + pre_log = log_db.access_log.count({}) + + + # Test phi access for individual elements without phi access level but with root + r = as_root.get('/collections/' + collection) + assert r.ok + assert r.json().get('files',[{}])[0].get('info').get('a') == "b" + assert pre_log == log_db.access_log.count({}) - 1 + + r = as_admin.delete('/collections/'+ collection) assert r.ok From c6d73475f3f5374872d12624b8d0e93b6e999935 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 1 Nov 2017 15:14:18 -0500 Subject: [PATCH 15/17] Propagating permissions of only one field of permission works --- api/handlers/listhandler.py | 11 +++++++++-- tests/integration_tests/python/test_users.py | 3 ++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/api/handlers/listhandler.py b/api/handlers/listhandler.py index 3bda975f8..984d2034f 100644 --- a/api/handlers/listhandler.py +++ b/api/handlers/listhandler.py @@ -228,8 +228,15 @@ def put(self, cont_name, list_name, **kwargs): payload = self.request.json_body payload['_id'] = kwargs.get('_id') if cont_name == 'groups' and self.request.params.get('propagate') =='true': - self._propagate_permissions(cont_name, _id, query={'permissions._id' : payload['_id']}, update={'$set': {'permissions.$.access': payload['access'], 'permissions.$.phi-access': payload['phi-access']}}) - self._propagate_permissions(cont_name, _id, query={'permissions._id': {'$ne': payload['_id']}}, update={'$addToSet': {'permissions': payload}}) + group = self.get("groups","permissions",**kwargs) + update = {'$set': {'permissions.$.access': payload.get('access'), 'permissions.$.phi-access': payload.get('phi-access')}} + if not update['$set']['permissions.$.access']: + update['$set'].pop('permissions.$.access') + elif not update['$set']['permissions.$.phi-access']: + update['$set'].pop('permissions.$.phi-access') + self._propagate_permissions(cont_name, _id, query={'permissions._id' : payload['_id']}, update=update) + + self._propagate_permissions(cont_name, _id, query={'permissions._id': {'$ne': payload['_id']}}, update={'$addToSet': {'permissions': group}}) elif cont_name != 'groups': self._propagate_permissions(cont_name, _id) return result diff --git a/tests/integration_tests/python/test_users.py b/tests/integration_tests/python/test_users.py index a646d4adf..ee427b1c9 100644 --- a/tests/integration_tests/python/test_users.py +++ b/tests/integration_tests/python/test_users.py @@ -95,7 +95,8 @@ def test_users(data_builder, as_root, as_admin, as_user, as_public): project = data_builder.create_project() r = as_admin.post('/projects/' + project + '/permissions', json={ '_id': new_user_id_admin, - 'access': 'ro' + 'access': 'ro', + 'phi-access': True }) assert r.ok From b4432f63ad8bd328a785498465a58abc027f8021 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Mon, 13 Nov 2017 10:24:20 -0600 Subject: [PATCH 16/17] added parent to analysis schema --- raml/schemas/definitions/acquisition.json | 2 +- raml/schemas/definitions/analysis.json | 10 +++++++++- raml/schemas/definitions/collection.json | 2 +- raml/schemas/definitions/file.json | 2 +- raml/schemas/definitions/project.json | 2 +- raml/schemas/definitions/session.json | 2 +- 6 files changed, 14 insertions(+), 6 deletions(-) diff --git a/raml/schemas/definitions/acquisition.json b/raml/schemas/definitions/acquisition.json index 7df51b79b..c04a6679a 100644 --- a/raml/schemas/definitions/acquisition.json +++ b/raml/schemas/definitions/acquisition.json @@ -51,7 +51,7 @@ } } }, - "additionalProperties":true + "additionalProperties":false } } } diff --git a/raml/schemas/definitions/analysis.json b/raml/schemas/definitions/analysis.json index b6c568d46..963b022e2 100644 --- a/raml/schemas/definitions/analysis.json +++ b/raml/schemas/definitions/analysis.json @@ -59,6 +59,7 @@ "measurements": {"$ref":"../definitions/file.json#/definitions/measurements"}, "tags": {"$ref":"../definitions/file.json#/definitions/tags"}, "info": {"$ref":"../definitions/file.json#/definitions/info"}, + "info_exists":{"type":"boolean"}, "origin":{"$ref":"../definitions/file.json#/definitions/origin"}, "hash":{"$ref":"../definitions/file.json#/definitions/hash"}, "created":{"$ref":"../definitions/created-modified.json#/definitions/created"}, @@ -67,7 +68,7 @@ "input":{"type":"boolean"}, "output":{"type":"boolean"} }, - "additionalProperties":true + "additionalProperties":false } }, "job":{ @@ -78,6 +79,13 @@ }, "notes":{"$ref":"#/definitions/notes"}, "description":{"$ref":"#/definitions/description"}, + "parent":{ + "type": "object", + "properties": { + "type": "string", + "id": {"$ref":"../definitions/objectid.json#"} + } + }, "label":{"$ref":"#/definitions/label"}, "user":{"$ref":"#/definitions/user"}, "created":{"$ref":"../definitions/created-modified.json#/definitions/created"}, diff --git a/raml/schemas/definitions/collection.json b/raml/schemas/definitions/collection.json index 54b874c34..5bb0b652c 100644 --- a/raml/schemas/definitions/collection.json +++ b/raml/schemas/definitions/collection.json @@ -55,7 +55,7 @@ } } }, - "additionalProperties":true + "additionalProperties":false } } } diff --git a/raml/schemas/definitions/file.json b/raml/schemas/definitions/file.json index 20832055e..49cde25e7 100644 --- a/raml/schemas/definitions/file.json +++ b/raml/schemas/definitions/file.json @@ -83,7 +83,7 @@ "size":{"$ref":"#/definitions/size"}, "info_exists": {"type": "boolean"} }, - "additionalProperties": true, + "additionalProperties": false, "required":["modified", "size"] } } diff --git a/raml/schemas/definitions/project.json b/raml/schemas/definitions/project.json index d30f22e1c..0c3c21a1a 100644 --- a/raml/schemas/definitions/project.json +++ b/raml/schemas/definitions/project.json @@ -46,7 +46,7 @@ } } }, - "additionalProperties": true + "additionalProperties": false } } } diff --git a/raml/schemas/definitions/session.json b/raml/schemas/definitions/session.json index 88c83fc55..827272908 100644 --- a/raml/schemas/definitions/session.json +++ b/raml/schemas/definitions/session.json @@ -65,7 +65,7 @@ } } }, - "additionalProperties": true + "additionalProperties": false } } } From 406ac2d0283901cd18a1779b4319e3e687b8eca5 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Mon, 4 Dec 2017 15:32:18 -0600 Subject: [PATCH 17/17] collection get_sessions uses SessionStorage --- api/handlers/collectionshandler.py | 10 ++++------ raml/examples/input/permission-update.json | 5 +++-- raml/examples/input/permission.json | 3 ++- raml/examples/output/acquisition-list.json | 3 ++- raml/examples/output/permission.json | 2 +- raml/examples/output/project-list.json | 8 -------- tests/integration_tests/python/test_collection.py | 9 ++++----- 7 files changed, 16 insertions(+), 24 deletions(-) diff --git a/api/handlers/collectionshandler.py b/api/handlers/collectionshandler.py index f27f1e027..7bafdba59 100644 --- a/api/handlers/collectionshandler.py +++ b/api/handlers/collectionshandler.py @@ -118,6 +118,7 @@ def get_all(self): phi = False projection = {'info': 0, 'tags': 0, 'files.info':0} results = permchecker(self.storage.exec_op)('GET', query=query, public=self.public_request, projection=projection, phi=phi) + log.debug(results) if not self.superuser_request and not self.is_true('join_avatars'): self._filter_all_permissions(results, self.uid) if self.is_true('join_avatars'): @@ -158,8 +159,6 @@ def get_sessions(self, cid): if not self.is_true('archived'): query['archived'] = {'$ne': True} - if not self.superuser_request: - query['permissions._id'] = self.uid if self.superuser_request: permchecker = always_ok @@ -175,7 +174,8 @@ def get_sessions(self, cid): else: phi = False - sessions = permchecker(self.storage.exec_op)('GET', query=query, public=self.public_request, projection=projection, phi=phi) + sessions = permchecker(containerstorage.SessionStorage().exec_op)('GET', query=query, public=self.public_request, projection=projection, phi=phi) + log.debug(sessions) self._filter_all_permissions(sessions, self.uid) if self.is_true('measurements'): self._add_session_measurements(sessions) @@ -218,9 +218,7 @@ def get_acquisitions(self, cid): else: phi = False - log.debug(query) - log.debug(projection) - acquisitions = permchecker(self.storage.exec_op)('GET', query=query, public=self.public_request, projection=projection, phi=phi) + acquisitions = permchecker(containerstorage.AcquisitionStorage().exec_op)('GET', query=query, public=self.public_request, projection=projection, phi=phi) self._filter_all_permissions(acquisitions, self.uid) for acquisition in acquisitions: diff --git a/raml/examples/input/permission-update.json b/raml/examples/input/permission-update.json index 9c8a26f20..70b1c7b63 100644 --- a/raml/examples/input/permission-update.json +++ b/raml/examples/input/permission-update.json @@ -1,4 +1,5 @@ { - "access": "ro", - "_id": "coltonlw@flywheel.io" + "access": "ro", + "_id": "coltonlw@flywheel.io", + "phi-access": true } diff --git a/raml/examples/input/permission.json b/raml/examples/input/permission.json index 7a6ecbf7a..2bbe48159 100644 --- a/raml/examples/input/permission.json +++ b/raml/examples/input/permission.json @@ -1,4 +1,5 @@ { "_id":"coltonlw@flywheel.io", - "access":"admin" + "access":"admin", + "phi-access": true } diff --git a/raml/examples/output/acquisition-list.json b/raml/examples/output/acquisition-list.json index 17006a169..248b0ab53 100644 --- a/raml/examples/output/acquisition-list.json +++ b/raml/examples/output/acquisition-list.json @@ -34,7 +34,8 @@ "permissions": [ { "access": "admin", - "_id": "coltonlw@flywheel.io" + "_id": "coltonlw@flywheel.io", + "phi-access": true } ] }, diff --git a/raml/examples/output/permission.json b/raml/examples/output/permission.json index 4b227be0b..b353de43e 100644 --- a/raml/examples/output/permission.json +++ b/raml/examples/output/permission.json @@ -1,5 +1,5 @@ { "access": "admin", "_id": "coltonlw@flywheel.io", - "phi-acces": true + "phi-access": true } diff --git a/raml/examples/output/project-list.json b/raml/examples/output/project-list.json index aa3f4663a..925aa28ed 100644 --- a/raml/examples/output/project-list.json +++ b/raml/examples/output/project-list.json @@ -7,12 +7,8 @@ "public": false, "permissions": [{ "access": "admin", -<<<<<<< HEAD - "_id": "coltonlw@flywheel.io" -======= "_id": "coltonlw@flywheel.io", "phi-access": true ->>>>>>> 1662a602... self.PHI is not modified }] }, { "group": "scitran", @@ -23,12 +19,8 @@ "public": false, "permissions": [{ "access": "admin", -<<<<<<< HEAD - "_id": "coltonlw@flywheel.io" -======= "_id": "coltonlw@flywheel.io", "phi-access": true ->>>>>>> 1662a602... self.PHI is not modified }] }, { "group": "scitran", diff --git a/tests/integration_tests/python/test_collection.py b/tests/integration_tests/python/test_collection.py index 29896882b..15c90636d 100644 --- a/tests/integration_tests/python/test_collection.py +++ b/tests/integration_tests/python/test_collection.py @@ -72,7 +72,7 @@ def test_collections(data_builder, as_admin, as_user): assert r.ok uid = r.json()['_id'] - r = as_admin.post('/collections/' + collection + '/permissions', json={'_id': uid, 'access': 'ro'}) + r = as_admin.post('/collections/' + collection + '/permissions', json={'_id': uid, 'access': 'ro', 'phi-access':True}) assert r.ok # test user cannot see sessions or acquisitions @@ -85,7 +85,7 @@ def test_collections(data_builder, as_admin, as_user): assert r.json() == [] # add user to project - r = as_admin.post('/projects/' + project2 + '/permissions', json={'_id': uid, 'access': 'ro'}) + r = as_admin.post('/projects/' + project2 + '/permissions', json={'_id': uid, 'access': 'ro', 'phi-access':True}) assert r.ok # test user can now see some of sessions and acquisitions @@ -157,7 +157,6 @@ def test_collections_phi(data_builder, as_admin, as_user, log_db, as_root,file_f r = as_admin.get('/collections', params={'phi':True}) assert r.ok for collection_ in r.json(): - print collection_ assert collection_.get('files',[{}])[0].get('info').get('a') == "b" assert pre_log == log_db.access_log.count({}) - len(r.json()) @@ -175,7 +174,7 @@ def test_collections_phi(data_builder, as_admin, as_user, log_db, as_root,file_f assert pre_log == log_db.access_log.count({}) - 1 - # Test phi access without phi access + # Test phi access without phi access r = as_admin.put("/collections/" + collection + "/permissions/admin@user.com", json={"phi-access":False}) assert r.ok r = as_admin.get("/collections/" + collection + "/permissions/admin@user.com") @@ -198,7 +197,7 @@ def test_collections_phi(data_builder, as_admin, as_user, log_db, as_root,file_f assert r.json().get('files',[{}])[0].get('info') == None assert pre_log == log_db.access_log.count({}) pre_log = log_db.access_log.count({}) - + # Test phi access for individual elements without phi access level but with root r = as_root.get('/collections/' + collection)