From 6d177dd27678490bc1f8a102fe3b4fc73f7e7527 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Tue, 29 Aug 2017 15:49:03 -0500 Subject: [PATCH 1/7] Add site-level rules and tests --- api/api.py | 6 + api/dao/base.py | 5 +- api/dao/containerstorage.py | 6 + api/dao/containerutil.py | 43 ++++ api/dao/hierarchy.py | 5 +- api/handlers/listhandler.py | 8 +- api/jobs/handlers.py | 127 +++++++----- api/jobs/rules.py | 21 +- test/integration_tests/python/conftest.py | 2 +- test/integration_tests/python/test_rules.py | 218 +++++++++++++++++++- 10 files changed, 369 insertions(+), 72 deletions(-) diff --git a/api/api.py b/api/api.py index 36f609f98..0573e2ea3 100644 --- a/api/api.py +++ b/api/api.py @@ -171,6 +171,12 @@ def prefix(path, routes): ]), + # Site + + route('//rules', RulesHandler, m=['GET', 'POST']), + route('//rules/', RuleHandler, m=['GET', 'PUT', 'DELETE']), + + # Groups route('/groups', GroupHandler, h='get_all', m=['GET']), diff --git a/api/dao/base.py b/api/dao/base.py index 5d01b2d2a..2d153e6ff 100644 --- a/api/dao/base.py +++ b/api/dao/base.py @@ -4,7 +4,6 @@ from . import APIStorageException, APIConflictException, APINotFoundException from . import consistencychecker from . import containerutil -from . import hierarchy from .. import config from .. import util @@ -89,7 +88,7 @@ def get_children(self, _id, projection=None, uid=None): query = {containerutil.singularize(self.cont_name): _id} else: query = {containerutil.singularize(self.cont_name): bson.ObjectId(_id)} - + if uid: query['permissions'] = {'$elemMatch': {'_id': uid}} if not projection: @@ -160,7 +159,7 @@ def update_el(self, _id, payload, unset_payload=None, recursive=False, r_payload except bson.InvalidId as e: raise APIStorageException(e.message) if recursive and r_payload is not None: - hierarchy.propagate_changes(self.cont_name, _id, {}, {'$set': util.mongo_dict(r_payload)}) + containerutil.propagate_changes(self.cont_name, _id, {}, {'$set': util.mongo_dict(r_payload)}) return self.dbc.update_one({'_id': _id}, update) def delete_el(self, _id): diff --git a/api/dao/containerstorage.py b/api/dao/containerstorage.py index f0b55fb98..e0074abaa 100644 --- a/api/dao/containerstorage.py +++ b/api/dao/containerstorage.py @@ -8,6 +8,7 @@ from .. import config from ..jobs.gears import validate_gear_config, get_gear from ..jobs.jobs import Job +from ..jobs.rules import copy_site_rules_for_project from .base import ContainerStorage @@ -43,6 +44,11 @@ class ProjectStorage(ContainerStorage): def __init__(self): super(ProjectStorage,self).__init__('projects', use_object_id=True) + def create_el(self, payload): + result = super(ProjectStorage, self).create_el(payload) + copy_site_rules_for_project(result.inserted_id) + return result + def update_el(self, _id, payload, unset_payload=None, recursive=False, r_payload=None, replace_metadata=False): result = super(ProjectStorage, self).update_el(_id, payload, unset_payload=unset_payload, recursive=recursive, r_payload=r_payload, replace_metadata=replace_metadata) diff --git a/api/dao/containerutil.py b/api/dao/containerutil.py index 3002df4c3..50feaf9dc 100644 --- a/api/dao/containerutil.py +++ b/api/dao/containerutil.py @@ -1,4 +1,5 @@ import bson.objectid +import copy from . import APIPermissionException from .. import config @@ -16,6 +17,48 @@ } PLURAL_TO_SINGULAR = {p: s for s, p in SINGULAR_TO_PLURAL.iteritems()} +def propagate_changes(cont_name, _id, query, update): + """ + Propagates changes down the heirarchy tree. + + cont_name and _id refer to top level container (which will not be modified here) + """ + + + if cont_name == 'groups': + project_ids = [p['_id'] for p in config.db.projects.find({'group': _id}, [])] + session_ids = [s['_id'] for s in config.db.sessions.find({'project': {'$in': project_ids}}, [])] + + project_q = copy.deepcopy(query) + project_q['_id'] = {'$in': project_ids} + session_q = copy.deepcopy(query) + session_q['_id'] = {'$in': session_ids} + acquisition_q = copy.deepcopy(query) + acquisition_q['session'] = {'$in': session_ids} + + config.db.projects.update_many(project_q, update) + config.db.sessions.update_many(session_q, update) + config.db.acquisitions.update_many(acquisition_q, update) + + + # Apply change to projects + elif cont_name == 'projects': + session_ids = [s['_id'] for s in config.db.sessions.find({'project': _id}, [])] + + session_q = copy.deepcopy(query) + session_q['project'] = _id + acquisition_q = copy.deepcopy(query) + acquisition_q['session'] = {'$in': session_ids} + + config.db.sessions.update_many(session_q, update) + config.db.acquisitions.update_many(acquisition_q, update) + + elif cont_name == 'sessions': + query['session'] = _id + config.db.acquisitions.update_many(query, update) + else: + raise ValueError('changes can only be propagated from group, project or session level') + def add_id_to_subject(subject, pid): """ diff --git a/api/dao/hierarchy.py b/api/dao/hierarchy.py index c47282f04..65e10063e 100644 --- a/api/dao/hierarchy.py +++ b/api/dao/hierarchy.py @@ -9,6 +9,7 @@ from .. import files from .. import util from .. import config +from .base import ContainerStorage from ..auth import has_access from . import APIStorageException, APINotFoundException, APIPermissionException, containerutil @@ -155,7 +156,7 @@ def propagate_changes(cont_name, _id, query, update): config.db.sessions.update_many(session_q, update) config.db.acquisitions.update_many(acquisition_q, update) - + # Apply change to projects elif cont_name == 'projects': session_ids = [s['_id'] for s in config.db.sessions.find({'project': _id}, [])] @@ -356,7 +357,7 @@ def _find_or_create_destination_project(group_id, project_label, timestamp, user 'created': timestamp, 'modified': timestamp } - result = config.db.projects.insert_one(project) + result = ContainerStorage.factory('project').create_el(project) project['_id'] = result.inserted_id return project diff --git a/api/handlers/listhandler.py b/api/handlers/listhandler.py index 1201afd0d..5da8dd754 100644 --- a/api/handlers/listhandler.py +++ b/api/handlers/listhandler.py @@ -16,7 +16,7 @@ from ..dao import noop from ..dao import liststorage from ..dao import APIStorageException -from ..dao import hierarchy +from ..dao import containerutil from ..web.request import log_access, AccessType @@ -251,7 +251,7 @@ def _propagate_permissions(self, cont_name, _id, query=None, update=None): query = {} if cont_name == 'groups': try: - hierarchy.propagate_changes(cont_name, _id, query, update) + containerutil.propagate_changes(cont_name, _id, query, update) except APIStorageException as e: self.abort(400, e.message) elif cont_name == 'projects': @@ -260,7 +260,7 @@ def _propagate_permissions(self, cont_name, _id, query=None, update=None): update = {'$set': { 'permissions': config.db[cont_name].find_one({'_id': oid},{'permissions': 1})['permissions'] }} - hierarchy.propagate_changes(cont_name, oid, {}, update) + containerutil.propagate_changes(cont_name, oid, {}, update) except APIStorageException: self.abort(500, 'permissions not propagated from {} {} down hierarchy'.format(cont_name, _id)) @@ -339,7 +339,7 @@ def _propagate_group_tags(self, cont_name, _id, query, update): method to propagate tag changes from a group to its projects, sessions and acquisitions """ try: - hierarchy.propagate_changes(cont_name, _id, query, update) + containerutil.propagate_changes(cont_name, _id, query, update) except APIStorageException: self.abort(500, 'tag change not propagated from group {}'.format(_id)) diff --git a/api/jobs/handlers.py b/api/jobs/handlers.py index 69f4217a2..ca291ee62 100644 --- a/api/jobs/handlers.py +++ b/api/jobs/handlers.py @@ -10,14 +10,14 @@ from .. import upload from .. import util from ..auth import require_login, has_access -from ..dao import APIPermissionException -from ..dao.containerstorage import AcquisitionStorage +from ..dao import APIPermissionException, APINotFoundException +from ..dao.containerstorage import ProjectStorage, AcquisitionStorage from ..dao.containerutil import create_filereference_from_dictionary, create_containerreference_from_dictionary, create_containerreference_from_filereference, ContainerReference from ..web import base from ..validators import InputValidationException from .. import config from . import batch -from ..validators import validate_data +from ..validators import validate_data, verify_payload_exists from .gears import validate_gear_config, get_gears, get_gear, get_invocation_schema, remove_gear, upsert_gear, suggest_container from .jobs import Job, Logs @@ -136,92 +136,109 @@ class RulesHandler(base.RequestHandler): def get(self, cid): """List rules""" - project = config.db.projects.find_one({'_id': bson.ObjectId(cid)}) - - if project and (self.superuser_request or has_access(self.uid, project, 'ro')): - return config.db.project_rules.find({'project_id' : cid}) + if cid == 'site': + if self.public_request: + raise APIPermissionException('Viewing site-level rules requires login.') else: - self.abort(404, 'Project not found') + project = ProjectStorage().get_container(cid, projection={'permissions': 1}) + config.log.debug('project permissions are {}\n and the user is {}\n\n'.format(project, self.uid)) + config.log.debug('Some key intel:\n user_is_admin: {}\n user_has_access {}\n\n'.format(self.user_is_admin, has_access(self.uid, project, 'ro'))) + if not self.user_is_admin and not has_access(self.uid, project, 'ro'): + raise APIPermissionException('User does not have access to project {} rules'.format(cid)) + + return config.db.project_rules.find({'project_id' : cid}) + + @verify_payload_exists def post(self, cid): """Add a rule""" - project = config.db.projects.find_one({'_id': bson.ObjectId(cid)}) - - if project: - if self.superuser_request or has_access(self.uid, project, 'admin'): - doc = self.request.json + if cid == 'site': + if not self.user_is_admin: + raise APIPermissionException('Adding site-level rules can only be done by a site admin.') + else: + project = ProjectStorage().get_container(cid, projection={'permissions': 1}) + if not self.user_is_admin and not has_access(self.uid, project, 'admin'): + raise APIPermissionException('Adding rules to a project can only be done by a project admin.') - validate_data(doc, 'rule-add.json', 'input', 'POST', optional=True) + doc = self.request.json - doc['project_id'] = cid + validate_data(doc, 'rule-add.json', 'input', 'POST', optional=True) - result = config.db.project_rules.insert_one(doc) - return { '_id': result.inserted_id } + doc['project_id'] = cid - elif has_access(self.uid, project, 'ro'): - self.abort(403, 'Adding rules to a project can only be done by a project admin.') + result = config.db.project_rules.insert_one(doc) + return { '_id': result.inserted_id } - self.abort(404, 'Project not found') class RuleHandler(base.RequestHandler): def get(self, cid, rid): """Get rule""" - project = config.db.projects.find_one({'_id': bson.ObjectId(cid)}) - - if project and (self.superuser_request or has_access(self.uid, project, 'ro')): - result = config.db.project_rules.find_one({'project_id' : cid, '_id': bson.ObjectId(rid)}) - if result: - return result - else: - self.abort(404, 'Rule not found') + if cid == 'site': + if self.public_request: + raise APIPermissionException('Viewing site-level rules requires login.') else: - self.abort(404, 'Project not found') + project = ProjectStorage().get_container(cid, projection={'permissions': 1}) + config.log.debug('project permissions are {}\n and the user is {}\n\n'.format(project, self.uid)) + config.log.debug('Some key intel:\n user_is_admin: {}\n user_has_access {}\n\n'.format(self.user_is_admin, has_access(self.uid, project, 'ro'))) + if not self.user_is_admin and not has_access(self.uid, project, 'ro'): + raise APIPermissionException('User does not have access to project {} rules'.format(cid)) + + result = config.db.project_rules.find_one({'project_id' : cid, '_id': bson.ObjectId(rid)}) + + if not result: + raise APINotFoundException('Rule not found.') + return result + + + @verify_payload_exists def put(self, cid, rid): """Change a rule""" - project = config.db.projects.find_one({'_id': bson.ObjectId(cid)}) + if cid == 'site': + if not self.user_is_admin: + raise APIPermissionException('Modifying site-level rules can only be done by a site admin.') + else: + project = ProjectStorage().get_container(cid, projection={'permissions': 1}) + if not self.user_is_admin and not has_access(self.uid, project, 'admin'): + raise APIPermissionException('Modifying project rules can only be done by a project admin.') - if project: - if self.superuser_request or has_access(self.uid, project, 'admin'): - result = config.db.project_rules.find_one({'project_id' : cid, '_id': bson.ObjectId(rid)}) - if result is None: - self.abort(404, 'Rule not found') + result = config.db.project_rules.find_one({'project_id' : cid, '_id': bson.ObjectId(rid)}) - doc = self.request.json - validate_data(doc, 'rule-update.json', 'input', 'POST', optional=True) + if not result: + raise APINotFoundException('Rule not found.') - doc['_id'] = result['_id'] - doc['project_id'] = cid + doc = self.request.json + validate_data(doc, 'rule-update.json', 'input', 'POST', optional=True) - config.db.project_rules.update_one({'_id': bson.ObjectId(rid)}, {'$set': doc }) - return + doc['_id'] = result['_id'] + doc['project_id'] = cid - elif has_access(self.uid, project, 'ro'): - self.abort(403, 'Changing project rules can only be done by a project admin.') + config.db.project_rules.replace_one({'_id': bson.ObjectId(rid)}, doc) + + return - self.abort(404, 'Project not found') def delete(self, cid, rid): """Remove a rule""" - project = config.db.projects.find_one({'_id': bson.ObjectId(cid)}) - - if project: - if self.superuser_request or has_access(self.uid, project, 'admin'): + if cid == 'site': + if not self.user_is_admin: + raise APIPermissionException('Modifying site-level rules can only be done by a site admin.') + else: + project = ProjectStorage().get_container(cid, projection={'permissions': 1}) + if not self.user_is_admin and not has_access(self.uid, project, 'admin'): + raise APIPermissionException('Modifying project rules can only be done by a project admin.') - result = config.db.project_rules.delete_one({'project_id' : cid, '_id': bson.ObjectId(rid)}) - if result.deleted_count != 1: - self.abort(404, 'Rule not found') - return - elif has_access(self.uid, project, 'ro'): - self.abort(403, 'Changing project rules can only be done by a project admin.') + result = config.db.project_rules.delete_one({'project_id' : cid, '_id': bson.ObjectId(rid)}) + if result.deleted_count != 1: + raise APINotFoundException('Rule not found.') + return - self.abort(404, 'Project not found') class JobsHandler(base.RequestHandler): """Provide /jobs API routes.""" diff --git a/api/jobs/rules.py b/api/jobs/rules.py index d2e29f172..c543be32c 100644 --- a/api/jobs/rules.py +++ b/api/jobs/rules.py @@ -261,8 +261,25 @@ def get_rules_for_container(db, container): result = list(db.project_rules.find({'project_id': str(container['_id'])})) if not result: - print 'Container ' + str(container['_id']) + ' found NO rules' + log.debug('Container ' + str(container['_id']) + ' found NO rules') return [] else: - print 'Container ' + str(container['_id']) + ' found ' + str(len(result)) + ' rules' + log.debug('Container ' + str(container['_id']) + ' found ' + str(len(result)) + ' rules') return result + +def copy_site_rules_for_project(project_id): + """ + Copy and insert all site-level rules for project. + + Note: Assumes project exists and caller has access. + """ + + site_rules = config.db.project_rules.find({'project_id' : 'site'}) + + for doc in site_rules: + doc.pop('_id') + doc['project_id'] = str(project_id) + config.db.project_rules.insert_one(doc) + + + diff --git a/test/integration_tests/python/conftest.py b/test/integration_tests/python/conftest.py index 1421dfb76..88a9c6a63 100644 --- a/test/integration_tests/python/conftest.py +++ b/test/integration_tests/python/conftest.py @@ -201,7 +201,7 @@ def log(request): def with_user(data_builder, randstr, as_public): """Return AttrDict with new user, api-key and api-accessor""" api_key = randstr() - user = data_builder.create_user(api_key=api_key, root=True) + user = data_builder.create_user(api_key=api_key, root=False) session = copy.deepcopy(as_public) session.headers.update({'Authorization': 'scitran-user ' + api_key}) return attrdict.AttrDict(user=user, api_key=api_key, session=session) diff --git a/test/integration_tests/python/test_rules.py b/test/integration_tests/python/test_rules.py index f55e53bff..f8a528552 100644 --- a/test/integration_tests/python/test_rules.py +++ b/test/integration_tests/python/test_rules.py @@ -1,3 +1,209 @@ +def test_site_rules(randstr, data_builder, as_admin, as_user, as_public): + gear_name = randstr() + gear = data_builder.create_gear(gear={'name': gear_name, 'version': '0.0.1'}) + + gear_2_name = randstr() + gear_2 = data_builder.create_gear(gear={'name': gear_2_name, 'version': '0.0.1'}) + + rule = { + 'alg': gear_name, + 'name': 'csv-job-trigger-rule', + 'any': [], + 'all': [ + {'type': 'file.type', 'value': 'tabular data'}, + ] + } + + # GET ALL + # attempt to get site rules without login + r = as_public.get('/site/rules') + assert r.status_code == 403 + + # get empty list of site rules + r = as_admin.get('/site/rules') + assert r.ok + assert r.json() == [] + + + # POST + # attempt to add site rule without admin + r = as_user.post('/site/rules', json=rule) + assert r.status_code == 403 + + # attempt to add site rule with empty payload + r = as_admin.post('site/rules', json={}) + + # add site rule + r = as_admin.post('/site/rules', json=rule) + assert r.ok + rule_id = r.json()['_id'] + + r = as_admin.get('/site/rules') + assert r.ok + assert len(r.json()) == 1 + + # GET ALL + # attempt to get site rules without login + r = as_public.get('/site/rules') + assert r.status_code == 403 + + # test rule is returned in list + r = as_admin.get('/site/rules') + assert r.ok + assert r.json()[0]['_id'] == rule_id + + + # GET ONE + # attempt to get specific site rule without login + r = as_public.get('/site/rules/' + rule_id) + assert r.status_code == 403 + + # attempt to get non-existent site rule + r = as_admin.get('/site/rules/000000000000000000000000') + assert r.status_code == 404 + + # get specific site rule + r = as_admin.get('/site/rules/' + rule_id) + assert r.ok + assert r.json()['alg'] == gear_name + + + # PUT + update = {'alg': gear_2_name} + + # attempt to modify site rule without admin + r = as_user.put('/site/rules/' + rule_id, json=update) + assert r.status_code == 403 + + # attempt to modify non-existent site rule + r = as_admin.put('/site/rules/000000000000000000000000', json=update) + assert r.status_code == 404 + + # attempt to modify site rule with empty payload + r = as_admin.put('/site/rules/' + rule_id, json={}) + assert r.status_code == 400 + + # modify site rule + r = as_admin.put('/site/rules/' + rule_id, json=update) + assert r.ok + r = as_admin.get('/site/rules/' + rule_id) + assert r.ok + assert r.json()['alg'] == gear_2_name + + + # DELETE + # attempt to delete rule without admin + r = as_user.delete('/site/rules/' + rule_id) + assert r.status_code == 403 + + # attempt to delete non-existent site rule + r = as_admin.delete('/site/rules/000000000000000000000000') + assert r.status_code == 404 + + # delete site rule + r = as_admin.delete('/site/rules/' + rule_id) + assert r.ok + + r = as_admin.get('/site/rules/' + rule_id) + assert r.status_code == 404 + + + + +def test_site_rules_copied_to_new_projects(randstr, data_builder, file_form, as_admin, as_root): + gear_1_name = randstr() + gear_1 = data_builder.create_gear(gear={'name': gear_1_name, 'version': '0.0.1'}) + + rule_1 = { + 'alg': gear_1_name, + 'name': 'csv-job-trigger-rule', + 'any': [], + 'all': [ + {'type': 'file.type', 'value': 'tabular data'}, + ] + } + + gear_2_name = randstr() + gear_2 = data_builder.create_gear(gear={'name': gear_2_name, 'version': '0.0.1'}) + + rule_2 = { + 'alg': gear_2_name, + 'name': 'text-job-trigger-rule', + 'any': [], + 'all': [ + {'type': 'file.type', 'value': 'text'}, + ] + } + + # Add rules to site level + r = as_admin.post('/site/rules', json=rule_1) + assert r.ok + rule_id_1 = r.json()['_id'] + + r = as_admin.post('/site/rules', json=rule_2) + assert r.ok + rule_id_2 = r.json()['_id'] + + # Ensure rules exist + r = as_admin.get('/site/rules') + assert r.ok + assert len(r.json()) == 2 + + + # Create new project via POST + group = data_builder.create_group() + r = as_admin.post('/projects', json={ + 'group': group, + 'label': 'project_1' + }) + assert r.ok + project_id = r.json()['_id'] + + r = as_admin.get('/projects/'+project_id+'/rules') + assert r.ok + assert len(r.json()) == 2 + + # Create new project via upload + r = as_admin.post('/upload/label', files=file_form( + 'acquisition.csv', + meta={ + 'group': {'_id': group}, + 'project': { + 'label': 'test_project', + }, + 'session': { + 'label': 'test_session_label', + 'subject': { + 'code': 'test_subject_code' + }, + }, + 'acquisition': { + 'label': 'test_acquisition_label', + 'files': [{'name': 'acquisition.csv'}] + } + }) + ) + assert r.ok + + # Find newly created project id + projects = as_root.get('/projects').json() + for p in projects: + if p['label'] == 'test_project': + project_2 = p['_id'] + break + + assert project_2 + r = as_admin.get('/projects/'+project_2+'/rules') + assert r.ok + assert len(r.json()) == 2 + + # Cleanup site rules + r = as_admin.delete('/site/rules/' + rule_id_1) + assert r.ok + r = as_admin.delete('/site/rules/' + rule_id_2) + assert r.ok + + def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, api_db): # create versioned gear to cover code selecting latest gear gear_name = randstr() @@ -5,6 +211,8 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a gear_2 = data_builder.create_gear(gear={'name': gear_name, 'version': '0.0.2'}) project = data_builder.create_project() + bad_payload = {'test': 'rules'} + # try to get all project rules of non-existent project r = as_admin.get('/projects/000000000000000000000000/rules') assert r.status_code == 404 @@ -15,7 +223,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a # try to get project rules w/o permissions r = with_user.session.get('/projects/' + project + '/rules') - assert r.status_code == 404 + assert r.status_code == 403 # get project rules (yet empty list) r = as_admin.get('/projects/' + project + '/rules') @@ -27,7 +235,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a assert r.ok # try to add rule to non-existent project - r = as_admin.post('/projects/000000000000000000000000/rules') + r = as_admin.post('/projects/000000000000000000000000/rules', json=bad_payload) assert r.status_code == 404 # add read-only perms for user @@ -36,7 +244,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a assert r.ok # try to add rule w/ read-only project perms - r = with_user.session.post('/projects/' + project + '/rules') + r = with_user.session.post('/projects/' + project + '/rules', json=bad_payload) assert r.status_code == 403 # add invalid project rule w/ non-existent gear @@ -68,11 +276,11 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a assert r.status_code == 500 # try to update rule of non-existent project - r = as_admin.put('/projects/000000000000000000000000/rules/000000000000000000000000') + r = as_admin.put('/projects/000000000000000000000000/rules/000000000000000000000000', json=bad_payload) assert r.status_code == 404 # try to update non-existent rule - r = as_admin.put('/projects/' + project + '/rules/000000000000000000000000') + r = as_admin.put('/projects/' + project + '/rules/000000000000000000000000', json=bad_payload) assert r.status_code == 404 # try to update rule w/ read-only project perms From 46803d6271da88e876558298391d308a9b50490d Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Thu, 31 Aug 2017 15:13:31 -0500 Subject: [PATCH 2/7] Clean up projects/groups after test --- test/integration_tests/python/test_rules.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/integration_tests/python/test_rules.py b/test/integration_tests/python/test_rules.py index f8a528552..86c54c4da 100644 --- a/test/integration_tests/python/test_rules.py +++ b/test/integration_tests/python/test_rules.py @@ -203,6 +203,9 @@ def test_site_rules_copied_to_new_projects(randstr, data_builder, file_form, as_ r = as_admin.delete('/site/rules/' + rule_id_2) assert r.ok + # delete group and children recursively (created by upload) + data_builder.delete_group(group, recursive=True) + def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, api_db): # create versioned gear to cover code selecting latest gear From fe783860c0d8ecd6122b21581689436674fdef93 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Tue, 5 Sep 2017 14:40:12 -0500 Subject: [PATCH 3/7] Require alg to exist --- api/jobs/gears.py | 4 +-- api/jobs/handlers.py | 36 +++++++++++++-------- test/integration_tests/python/test_rules.py | 35 ++++++++++++-------- 3 files changed, 45 insertions(+), 30 deletions(-) diff --git a/api/jobs/gears.py b/api/jobs/gears.py index 8e9f83836..c809e355a 100644 --- a/api/jobs/gears.py +++ b/api/jobs/gears.py @@ -12,7 +12,7 @@ from .. import config from .jobs import Job -from ..dao import APIValidationException +from ..dao import APIValidationException, APINotFoundException from ..dao.base import ContainerStorage log = config.log @@ -46,7 +46,7 @@ def get_gear_by_name(name): gear_doc = list(config.db.gears.find({'gear.name': name}).sort('created', pymongo.DESCENDING)) if len(gear_doc) == 0 : - raise Exception('Unknown gear ' + name) + raise APINotFoundException('Unknown gear ' + name) return gear_doc[0] diff --git a/api/jobs/handlers.py b/api/jobs/handlers.py index ca291ee62..ca1a0ab23 100644 --- a/api/jobs/handlers.py +++ b/api/jobs/handlers.py @@ -19,7 +19,7 @@ from . import batch from ..validators import validate_data, verify_payload_exists -from .gears import validate_gear_config, get_gears, get_gear, get_invocation_schema, remove_gear, upsert_gear, suggest_container +from .gears import validate_gear_config, get_gears, get_gear, get_invocation_schema, remove_gear, upsert_gear, suggest_container, get_gear_by_name from .jobs import Job, Logs from .batch import check_state, update from .queue import Queue @@ -136,17 +136,18 @@ class RulesHandler(base.RequestHandler): def get(self, cid): """List rules""" + projection = None + if cid == 'site': if self.public_request: raise APIPermissionException('Viewing site-level rules requires login.') + projection = {'project_id': 0} else: project = ProjectStorage().get_container(cid, projection={'permissions': 1}) - config.log.debug('project permissions are {}\n and the user is {}\n\n'.format(project, self.uid)) - config.log.debug('Some key intel:\n user_is_admin: {}\n user_has_access {}\n\n'.format(self.user_is_admin, has_access(self.uid, project, 'ro'))) if not self.user_is_admin and not has_access(self.uid, project, 'ro'): raise APIPermissionException('User does not have access to project {} rules'.format(cid)) - return config.db.project_rules.find({'project_id' : cid}) + return config.db.project_rules.find({'project_id' : cid}, projection=projection) @verify_payload_exists @@ -164,6 +165,10 @@ def post(self, cid): doc = self.request.json validate_data(doc, 'rule-add.json', 'input', 'POST', optional=True) + try: + get_gear_by_name(doc['alg']) + except APINotFoundException: + self.abort(400, 'Cannot find gear for alg {}, alg not valid'.format(doc['alg'])) doc['project_id'] = cid @@ -176,17 +181,17 @@ class RuleHandler(base.RequestHandler): def get(self, cid, rid): """Get rule""" + projection = None if cid == 'site': if self.public_request: raise APIPermissionException('Viewing site-level rules requires login.') + projection = {'project_id': 0} else: project = ProjectStorage().get_container(cid, projection={'permissions': 1}) - config.log.debug('project permissions are {}\n and the user is {}\n\n'.format(project, self.uid)) - config.log.debug('Some key intel:\n user_is_admin: {}\n user_has_access {}\n\n'.format(self.user_is_admin, has_access(self.uid, project, 'ro'))) if not self.user_is_admin and not has_access(self.uid, project, 'ro'): raise APIPermissionException('User does not have access to project {} rules'.format(cid)) - result = config.db.project_rules.find_one({'project_id' : cid, '_id': bson.ObjectId(rid)}) + result = config.db.project_rules.find_one({'project_id' : cid, '_id': bson.ObjectId(rid)}, projection=projection) if not result: raise APINotFoundException('Rule not found.') @@ -206,17 +211,20 @@ def put(self, cid, rid): if not self.user_is_admin and not has_access(self.uid, project, 'admin'): raise APIPermissionException('Modifying project rules can only be done by a project admin.') - result = config.db.project_rules.find_one({'project_id' : cid, '_id': bson.ObjectId(rid)}) + doc = config.db.project_rules.find_one({'project_id' : cid, '_id': bson.ObjectId(rid)}) - if not result: + if not doc: raise APINotFoundException('Rule not found.') - doc = self.request.json - validate_data(doc, 'rule-update.json', 'input', 'POST', optional=True) - - doc['_id'] = result['_id'] - doc['project_id'] = cid + updates = self.request.json + validate_data(updates, 'rule-update.json', 'input', 'POST', optional=True) + if updates.get('alg'): + try: + get_gear_by_name(updates['alg']) + except APINotFoundException: + self.abort(400, 'Cannot find gear for alg {}, alg not valid'.format(updates['alg'])) + doc.update(updates) config.db.project_rules.replace_one({'_id': bson.ObjectId(rid)}, doc) return diff --git a/test/integration_tests/python/test_rules.py b/test/integration_tests/python/test_rules.py index 86c54c4da..66cf0a7d6 100644 --- a/test/integration_tests/python/test_rules.py +++ b/test/integration_tests/python/test_rules.py @@ -250,34 +250,36 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a r = with_user.session.post('/projects/' + project + '/rules', json=bad_payload) assert r.status_code == 403 - # add invalid project rule w/ non-existent gear - # NOTE this is a legacy rule - r = as_admin.post('/projects/' + project + '/rules', json={ + rule_json = { 'alg': 'non-existent-gear-name', 'name': 'csv-job-trigger-rule', 'any': [], 'all': [ {'type': 'file.type', 'value': 'tabular data'}, ] - }) + } + + # try to add project rule w/ non-existent gear + # NOTE this is a legacy rule + r = as_admin.post('/projects/' + project + '/rules', json=rule_json) + assert r.status_code == 400 + + # add project rule w/ proper gear alg + # NOTE this is a legacy rule + rule_json['alg'] = gear_name + r = as_admin.post('/projects/' + project + '/rules', json=rule_json) assert r.ok rule = r.json()['_id'] # get project rules (verify rule was added) r = as_admin.get('/projects/' + project + '/rules') assert r.ok - assert r.json()[0]['alg'] == 'non-existent-gear-name' + assert r.json()[0]['alg'] == gear_name # try to get single project rule using non-existent rule id r = as_admin.get('/projects/' + project + '/rules/000000000000000000000000') assert r.status_code == 404 - # try to upload file that matches invalid rule (500, Unknown gear) - # NOTE with an invalid and some valid rules an upload could conceivably return - # a 500 after already having created jobs for the valid rules - r = as_admin.post('/projects/' + project + '/files', files=file_form('test.csv')) - assert r.status_code == 500 - # try to update rule of non-existent project r = as_admin.put('/projects/000000000000000000000000/rules/000000000000000000000000', json=bad_payload) assert r.status_code == 404 @@ -290,14 +292,19 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a r = with_user.session.put('/projects/' + project + '/rules/' + rule, json={'alg': gear_name}) assert r.status_code == 403 - # update rule to use a valid gear - r = as_admin.put('/projects/' + project + '/rules/' + rule, json={'alg': gear_name}) + # try to update rule to with invalid gear alg + r = as_admin.put('/projects/' + project + '/rules/' + rule, json={'alg': 'not-a-real-gear'}) + assert r.status_code == 400 + + # update name of rule + rule_name = 'improved-csv-trigger-rule' + r = as_admin.put('/projects/' + project + '/rules/' + rule, json={'name': rule_name}) assert r.ok # verify rule was updated r = as_admin.get('/projects/' + project + '/rules/' + rule) assert r.ok - assert r.json()['alg'] == gear_name + assert r.json()['name'] == rule_name # upload file that matches rule r = as_admin.post('/projects/' + project + '/files', files=file_form('test2.csv')) From 2eceebdd1eb052b81b6563e7bd58c275c4f74592 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Tue, 5 Sep 2017 19:04:36 -0500 Subject: [PATCH 4/7] Add script to find malformed rules --- bin/check_rule_algs.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100755 bin/check_rule_algs.py diff --git a/bin/check_rule_algs.py b/bin/check_rule_algs.py new file mode 100755 index 000000000..111a81994 --- /dev/null +++ b/bin/check_rule_algs.py @@ -0,0 +1,22 @@ +# Print list of rules with invalid algs + +from api import config +from api.dao import APINotFoundException +from api.jobs.gears import get_gear_by_name + +if __name__ == '__main__': + + for rule in config.db.project_rules.find({}): + alg = rule.get('alg') + + if not alg: + print 'Rule {} has no alg.'.format(rule['_id']) + + else: + try: + get_gear_by_name(alg) + except APINotFoundException: + print 'Rule {} with alg {} does not match any gear in the system'.format(rule['_id'], alg) + + + From f3af013d2836689f7ad41dd8a773d72a1df25686 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Wed, 6 Sep 2017 12:49:50 -0500 Subject: [PATCH 5/7] Add generic integrity check script --- bin/check_rule_algs.py | 22 ----------- bin/integrity_check.py | 84 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 22 deletions(-) delete mode 100755 bin/check_rule_algs.py create mode 100755 bin/integrity_check.py diff --git a/bin/check_rule_algs.py b/bin/check_rule_algs.py deleted file mode 100755 index 111a81994..000000000 --- a/bin/check_rule_algs.py +++ /dev/null @@ -1,22 +0,0 @@ -# Print list of rules with invalid algs - -from api import config -from api.dao import APINotFoundException -from api.jobs.gears import get_gear_by_name - -if __name__ == '__main__': - - for rule in config.db.project_rules.find({}): - alg = rule.get('alg') - - if not alg: - print 'Rule {} has no alg.'.format(rule['_id']) - - else: - try: - get_gear_by_name(alg) - except APINotFoundException: - print 'Rule {} with alg {} does not match any gear in the system'.format(rule['_id'], alg) - - - diff --git a/bin/integrity_check.py b/bin/integrity_check.py new file mode 100755 index 000000000..9dd218684 --- /dev/null +++ b/bin/integrity_check.py @@ -0,0 +1,84 @@ +# Print list of rules with invalid algs +import argparse +import logging +import sys + +from api import config +from api.dao import APINotFoundException +from api.jobs.gears import get_gear_by_name + + +# Methods should return true if all integrity checks passed +INTEGRITY_CHECKS = { + "test_one": "Run test one", + "test_two": "Run test two", + "check_rule_alg": "Confirm alg keys in rules table can be resolved to gear in gear table" +} + + +def test_one(): + logging.warning('ran test 1') + return False + + +def test_two(): + logging.warning('ran test 2') + return True + + +def check_rule_alg(): + errors = False + + for rule in config.db.project_rules.find({}): + alg = rule.get('alg') + + if not alg: + errors = True + logging.warning('Rule {} has no alg.'.format(rule['_id'])) + + else: + try: + get_gear_by_name(alg) + except APINotFoundException: + errors = True + logging.warning('Rule {} with alg {} does not match any gear in the system'.format(rule['_id'], alg)) + + return not errors + + +if __name__ == '__main__': + try: + parser = argparse.ArgumentParser() + parser.add_argument("--all", help="Run all checks", action="store_true") + + for method, desc in INTEGRITY_CHECKS.iteritems(): + parser.add_argument("--"+method, help=desc, action="store_true") + + # get list of method to run: + args = parser.parse_args() + if args.all: + methods = INTEGRITY_CHECKS.keys() + else: + methods = [m for m, flag in vars(args).iteritems() if flag] + + + errors = False + for method in methods: + try: + logging.info('Running {}...'.format(method)) + passed = globals()[method]() + if not passed: + errors = True + logging.warning('{} found integrity issues.'.format(method)) + logging.info('{} complete.'.format(method)) + except: + logging.exception('Failed to run check {}'.format(method)) + + except Exception as e: + logging.exception('Main method failed...') + sys.exit(1) + + + + + From a2ac7e7f53de8f51763bd0c19b748a93c887073e Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Wed, 6 Sep 2017 12:50:58 -0500 Subject: [PATCH 6/7] Change function name --- bin/integrity_check.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/integrity_check.py b/bin/integrity_check.py index 9dd218684..d2ff40992 100755 --- a/bin/integrity_check.py +++ b/bin/integrity_check.py @@ -12,7 +12,7 @@ INTEGRITY_CHECKS = { "test_one": "Run test one", "test_two": "Run test two", - "check_rule_alg": "Confirm alg keys in rules table can be resolved to gear in gear table" + "rule_alg": "Confirm alg keys in rules table can be resolved to gear in gear table" } @@ -26,7 +26,7 @@ def test_two(): return True -def check_rule_alg(): +def rule_alg(): errors = False for rule in config.db.project_rules.find({}): From 990920be284f37e51ee01edfa28bf34c6a32779b Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Wed, 6 Sep 2017 15:22:09 -0500 Subject: [PATCH 7/7] Remove test checks --- bin/integrity_check.py | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/bin/integrity_check.py b/bin/integrity_check.py index d2ff40992..e777a25c0 100755 --- a/bin/integrity_check.py +++ b/bin/integrity_check.py @@ -10,22 +10,10 @@ # Methods should return true if all integrity checks passed INTEGRITY_CHECKS = { - "test_one": "Run test one", - "test_two": "Run test two", "rule_alg": "Confirm alg keys in rules table can be resolved to gear in gear table" } -def test_one(): - logging.warning('ran test 1') - return False - - -def test_two(): - logging.warning('ran test 2') - return True - - def rule_alg(): errors = False @@ -74,6 +62,13 @@ def rule_alg(): except: logging.exception('Failed to run check {}'.format(method)) + if errors: + logging.error('One or more checks failed') + sys.exit(1) + else: + logging.info('Checks complete.') + sys.exit(0) + except Exception as e: logging.exception('Main method failed...') sys.exit(1)