From e102214809add8f92a34293920379f66dea0c078 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Wed, 3 Jun 2020 15:03:22 -0400 Subject: [PATCH 01/10] minor cleanup of error codes. --- crc/api/file.py | 2 +- crc/api/study.py | 2 +- crc/api/tools.py | 8 ++++---- crc/services/file_service.py | 2 +- crc/services/lookup_service.py | 2 +- crc/services/protocol_builder.py | 2 +- crc/services/study_service.py | 4 ++-- crc/services/workflow_service.py | 6 +++--- 8 files changed, 14 insertions(+), 14 deletions(-) diff --git a/crc/api/file.py b/crc/api/file.py index a537cfe5..5cf54221 100644 --- a/crc/api/file.py +++ b/crc/api/file.py @@ -122,7 +122,7 @@ def get_file_info(file_id): def update_file_info(file_id, body): if file_id is None: - raise ApiError('unknown_file', 'Please provide a valid File ID.') + raise ApiError('no_such_file', 'Please provide a valid File ID.') file_model = session.query(FileModel).filter_by(id=file_id).first() diff --git a/crc/api/study.py b/crc/api/study.py index e9a251f8..8fdd1b4a 100644 --- a/crc/api/study.py +++ b/crc/api/study.py @@ -50,7 +50,7 @@ def update_study(study_id, body): def get_study(study_id): study = StudyService.get_study(study_id) if (study is None): - raise ApiError("Study not found", status_code=404) + raise ApiError("unknown_study", 'The study "' + study_id + '" is not recognized.', status_code=404) return StudySchema().dump(study) diff --git a/crc/api/tools.py b/crc/api/tools.py index 6fb31b71..4699be5f 100644 --- a/crc/api/tools.py +++ b/crc/api/tools.py @@ -20,9 +20,9 @@ def render_markdown(data, template): data = json.loads(data) return template.render(**data) except UndefinedError as ue: - raise ApiError(code="undefined field", message=ue.message) + raise ApiError(code="undefined_field", message=ue.message) except Exception as e: - raise ApiError(code="invalid", message=str(e)) + raise ApiError(code="invalid_render", message=str(e)) def render_docx(): @@ -42,9 +42,9 @@ def render_docx(): cache_timeout=-1 # Don't cache these files on the browser. ) except ValueError as e: - raise ApiError(code="invalid", message=str(e)) + raise ApiError(code="undefined_field", message=str(e)) except Exception as e: - raise ApiError(code="invalid", message=str(e)) + raise ApiError(code="invalid_render", message=str(e)) def list_scripts(): diff --git a/crc/services/file_service.py b/crc/services/file_service.py index 9142a7c3..ce4b7261 100644 --- a/crc/services/file_service.py +++ b/crc/services/file_service.py @@ -274,7 +274,7 @@ class FileService(object): workflow_spec_model = FileService.find_spec_model_in_db(workflow) if workflow_spec_model is None: - raise ApiError(code="workflow_model_error", + raise ApiError(code="unknown_workflow", message="Something is wrong. I can't find the workflow you are using.") file_data_model = session.query(FileDataModel) \ diff --git a/crc/services/lookup_service.py b/crc/services/lookup_service.py index 076fbe9a..71424b6b 100644 --- a/crc/services/lookup_service.py +++ b/crc/services/lookup_service.py @@ -103,7 +103,7 @@ class LookupService(object): workflow_id=workflow_model.id, name=file_name) if len(latest_files) < 1: - raise ApiError("missing_file", "Unable to locate the lookup data file '%s'" % file_name) + raise ApiError("invalid_enum", "Unable to locate the lookup data file '%s'" % file_name) else: data_model = latest_files[0] diff --git a/crc/services/protocol_builder.py b/crc/services/protocol_builder.py index 5fc5535f..8d1d7886 100644 --- a/crc/services/protocol_builder.py +++ b/crc/services/protocol_builder.py @@ -25,7 +25,7 @@ class ProtocolBuilderService(object): def get_studies(user_id) -> {}: ProtocolBuilderService.__enabled_or_raise() if not isinstance(user_id, str): - raise ApiError("invalid_user_id", "This user id is invalid: " + str(user_id)) + raise ApiError("protocol_builder_error", "This user id is invalid: " + str(user_id)) response = requests.get(ProtocolBuilderService.STUDY_URL % user_id) if response.ok and response.text: pb_studies = ProtocolBuilderStudySchema(many=True).loads(response.text) diff --git a/crc/services/study_service.py b/crc/services/study_service.py index 8d9df0db..0bc80bcf 100644 --- a/crc/services/study_service.py +++ b/crc/services/study_service.py @@ -319,9 +319,9 @@ class StudyService(object): try: StudyService._create_workflow_model(study_model, workflow_spec) except WorkflowTaskExecException as wtee: - errors.append(ApiError.from_task("workflow_execution_exception", str(wtee), wtee.task)) + errors.append(ApiError.from_task("workflow_startup_exception", str(wtee), wtee.task)) except WorkflowException as we: - errors.append(ApiError.from_task_spec("workflow_execution_exception", str(we), we.sender)) + errors.append(ApiError.from_task_spec("workflow_startup_exception", str(we), we.sender)) return errors @staticmethod diff --git a/crc/services/workflow_service.py b/crc/services/workflow_service.py index c3834f0a..5efa8cab 100644 --- a/crc/services/workflow_service.py +++ b/crc/services/workflow_service.py @@ -82,7 +82,7 @@ class WorkflowService(object): processor = WorkflowProcessor(workflow_model, validate_only=True) except WorkflowException as we: WorkflowService.delete_test_data() - raise ApiError.from_workflow_exception("workflow_execution_exception", str(we), we) + raise ApiError.from_workflow_exception("workflow_validation_exception", str(we), we) while not processor.bpmn_workflow.is_completed(): try: @@ -96,7 +96,7 @@ class WorkflowService(object): task.complete() except WorkflowException as we: WorkflowService.delete_test_data() - raise ApiError.from_workflow_exception("workflow_execution_exception", str(we), we) + raise ApiError.from_workflow_exception("workflow_validation_exception", str(we), we) WorkflowService.delete_test_data() return processor.bpmn_workflow.last_task.data @@ -162,7 +162,7 @@ class WorkflowService(object): options.append({"id": d.value, "label": d.label}) return random.choice(options) else: - raise ApiError.from_task("invalid_autocomplete", "The settings for this auto complete field " + raise ApiError.from_task("unknown_lookup_option", "The settings for this auto complete field " "are incorrect: %s " % field.id, task) elif field.type == "long": return random.randint(1, 1000) From c179c7781bbc5058e014f2851c3ab3237827e0a7 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Wed, 3 Jun 2020 16:50:47 -0400 Subject: [PATCH 02/10] Do not process or return cancelled approvals via the API. --- crc/api/approval.py | 15 ++++----------- crc/services/approval_service.py | 4 ++-- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/crc/api/approval.py b/crc/api/approval.py index 6ab887f4..7c931257 100644 --- a/crc/api/approval.py +++ b/crc/api/approval.py @@ -3,28 +3,21 @@ import pickle from base64 import b64decode from datetime import datetime -from SpiffWorkflow import Workflow -from SpiffWorkflow.serializer.dict import DictionarySerializer -from SpiffWorkflow.serializer.json import JSONSerializer from flask import g -from crc import app, db, session - -from crc.api.common import ApiError, ApiErrorSchema +from crc import db, session +from crc.api.common import ApiError from crc.models.approval import Approval, ApprovalModel, ApprovalSchema, ApprovalStatus -from crc.models.ldap import LdapSchema -from crc.models.study import Study from crc.models.workflow import WorkflowModel from crc.services.approval_service import ApprovalService from crc.services.ldap_service import LdapService -from crc.services.workflow_processor import WorkflowProcessor def get_approvals(everything=False): if everything: - approvals = ApprovalService.get_all_approvals() + approvals = ApprovalService.get_all_approvals(ignore_cancelled=True) else: - approvals = ApprovalService.get_approvals_per_user(g.user.uid) + approvals = ApprovalService.get_approvals_per_user(g.user.uid, ignore_cancelled=True) results = ApprovalSchema(many=True).dump(approvals) return results diff --git a/crc/services/approval_service.py b/crc/services/approval_service.py index bba53a61..8b13615d 100644 --- a/crc/services/approval_service.py +++ b/crc/services/approval_service.py @@ -40,14 +40,14 @@ class ApprovalService(object): return main_approval @staticmethod - def get_approvals_per_user(approver_uid): + def get_approvals_per_user(approver_uid, ignore_cancelled=False): """Returns a list of approval objects (not db models) for the given approver. """ studies = db.session.query(StudyModel).join(ApprovalModel).\ filter(ApprovalModel.approver_uid == approver_uid).all() approvals = [] for study in studies: - approval = ApprovalService.__one_approval_from_study(study, approver_uid) + approval = ApprovalService.__one_approval_from_study(study, approver_uid, ignore_cancelled) if approval: approvals.append(approval) return approvals From 217ecfc9114fae1f9ce5da1376bcf088ee965883 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Wed, 3 Jun 2020 17:34:27 -0400 Subject: [PATCH 03/10] When you can't delete a file, mark it as archived. Don't include archived files in new approval requests. --- crc/api.yml | 2 +- crc/api/approval.py | 6 ++-- crc/models/file.py | 5 ++- crc/services/approval_service.py | 21 ++++++------ crc/services/file_service.py | 20 ++++++++---- tests/test_files_api.py | 37 ++++++++++++++++++++++ tests/test_workflow_spec_validation_api.py | 4 +-- 7 files changed, 73 insertions(+), 22 deletions(-) diff --git a/crc/api.yml b/crc/api.yml index 1688874f..a3fd835b 100644 --- a/crc/api.yml +++ b/crc/api.yml @@ -468,7 +468,7 @@ paths: $ref: "#/components/schemas/File" delete: operationId: crc.api.file.delete_file - summary: Removes an existing file + summary: Removes an existing file. In the event the file can not be deleted, it is archived. tags: - Files responses: diff --git a/crc/api/approval.py b/crc/api/approval.py index 7c931257..1408fec5 100644 --- a/crc/api/approval.py +++ b/crc/api/approval.py @@ -15,9 +15,9 @@ from crc.services.ldap_service import LdapService def get_approvals(everything=False): if everything: - approvals = ApprovalService.get_all_approvals(ignore_cancelled=True) + approvals = ApprovalService.get_all_approvals(include_cancelled=True) else: - approvals = ApprovalService.get_approvals_per_user(g.user.uid, ignore_cancelled=True) + approvals = ApprovalService.get_approvals_per_user(g.user.uid, include_cancelled=False) results = ApprovalSchema(many=True).dump(approvals) return results @@ -33,7 +33,7 @@ def get_approvals_for_study(study_id=None): def get_csv(): """A huge bit of a one-off for RRT, but 3 weeks of midnight work can convince a man to do just about anything""" - approvals = ApprovalService.get_all_approvals(ignore_cancelled=True) + approvals = ApprovalService.get_all_approvals(include_cancelled=False) output = [] errors = [] ldapService = LdapService() diff --git a/crc/models/file.py b/crc/models/file.py index 9cbfb7fc..a0c94985 100644 --- a/crc/models/file.py +++ b/crc/models/file.py @@ -82,7 +82,10 @@ class FileModel(db.Model): workflow_spec_id = db.Column(db.String, db.ForeignKey('workflow_spec.id'), nullable=True) workflow_id = db.Column(db.Integer, db.ForeignKey('workflow.id'), nullable=True) irb_doc_code = db.Column(db.String, nullable=True) # Code reference to the irb_documents.xlsx reference file. - + # A request was made to delete the file, but we can't because there are + # active approvals or running workflows that depend on it. So we archive + # it instead, hide it in the interface. + archived = db.Column(db.Boolean, default=False) class File(object): @classmethod diff --git a/crc/services/approval_service.py b/crc/services/approval_service.py index 8b13615d..a71f4ba1 100644 --- a/crc/services/approval_service.py +++ b/crc/services/approval_service.py @@ -15,7 +15,7 @@ class ApprovalService(object): """Provides common tools for working with an Approval""" @staticmethod - def __one_approval_from_study(study, approver_uid = None, ignore_cancelled=False): + def __one_approval_from_study(study, approver_uid = None, include_cancelled=True): """Returns one approval, with all additional approvals as 'related_approvals', the main approval can be pinned to an approver with an optional argument. Will return null if no approvals exist on the study.""" @@ -23,7 +23,7 @@ class ApprovalService(object): related_approvals = [] query = db.session.query(ApprovalModel).\ filter(ApprovalModel.study_id == study.id) - if ignore_cancelled: + if not include_cancelled: query=query.filter(ApprovalModel.status != ApprovalStatus.CANCELED.value) approvals = query.all() @@ -40,35 +40,38 @@ class ApprovalService(object): return main_approval @staticmethod - def get_approvals_per_user(approver_uid, ignore_cancelled=False): + def get_approvals_per_user(approver_uid, include_cancelled=False): """Returns a list of approval objects (not db models) for the given approver. """ studies = db.session.query(StudyModel).join(ApprovalModel).\ filter(ApprovalModel.approver_uid == approver_uid).all() approvals = [] for study in studies: - approval = ApprovalService.__one_approval_from_study(study, approver_uid, ignore_cancelled) + approval = ApprovalService.__one_approval_from_study(study, approver_uid, include_cancelled) if approval: approvals.append(approval) return approvals @staticmethod - def get_all_approvals(ignore_cancelled=False): + def get_all_approvals(include_cancelled=True): """Returns a list of all approval objects (not db models), one record per study, with any associated approvals grouped under the first approval.""" studies = db.session.query(StudyModel).all() approvals = [] for study in studies: - approval = ApprovalService.__one_approval_from_study(study, ignore_cancelled=ignore_cancelled) + approval = ApprovalService.__one_approval_from_study(study, include_cancelled=include_cancelled) if approval: approvals.append(approval) return approvals @staticmethod - def get_approvals_for_study(study_id): + def get_approvals_for_study(study_id, include_cancelled=True): """Returns an array of Approval objects for the study, it does not compute the related approvals.""" - db_approvals = session.query(ApprovalModel).filter_by(study_id=study_id).all() + query = session.query(ApprovalModel).filter_by(study_id=study_id) + if not include_cancelled: + query = query.filter(ApprovalModel.status != ApprovalStatus.CANCELED.value) + db_approvals = query.all() return [Approval.from_model(approval_model) for approval_model in db_approvals] @@ -101,7 +104,7 @@ class ApprovalService(object): # Construct as hash of the latest files to see if things have changed since # the last approval. workflow = db.session.query(WorkflowModel).filter(WorkflowModel.id == workflow_id).first() - workflow_data_files = FileService.get_workflow_data_files(workflow_id) + workflow_data_files = FileService.get_workflow_data_files(workflow_id, include_archives=False) current_data_file_ids = list(data_file.id for data_file in workflow_data_files) if len(current_data_file_ids) == 0: diff --git a/crc/services/file_service.py b/crc/services/file_service.py index ce4b7261..cda16a88 100644 --- a/crc/services/file_service.py +++ b/crc/services/file_service.py @@ -195,7 +195,7 @@ class FileService(object): @staticmethod def get_files(workflow_spec_id=None, workflow_id=None, - name=None, is_reference=False, irb_doc_code=None): + name=None, is_reference=False, irb_doc_code=None, include_archives=True): query = session.query(FileModel).filter_by(is_reference=is_reference) if workflow_spec_id: query = query.filter_by(workflow_spec_id=workflow_spec_id) @@ -208,6 +208,10 @@ class FileService(object): if name: query = query.filter_by(name=name) + + if not include_archives: + query = query.filter(FileModel.archived == False) + query = query.order_by(FileModel.id) results = query.all() @@ -238,11 +242,11 @@ class FileService(object): return latest_data_files @staticmethod - def get_workflow_data_files(workflow_id=None): + def get_workflow_data_files(workflow_id=None, include_archives=True): """Returns all the FileDataModels related to a running workflow - So these are the latest data files that were uploaded or generated that go along with this workflow. Not related to the spec in any way""" - file_models = FileService.get_files(workflow_id=workflow_id) + file_models = FileService.get_files(workflow_id=workflow_id, include_archives=include_archives) latest_data_files = [] for file_model in file_models: latest_data_files.append(FileService.get_file_data(file_model.id)) @@ -316,6 +320,10 @@ class FileService(object): session.query(FileModel).filter_by(id=file_id).delete() session.commit() except IntegrityError as ie: - app.logger.error("Failed to delete file: %i, due to %s" % (file_id, str(ie))) - raise ApiError('file_integrity_error', "You are attempting to delete a file that is " - "required by other records in the system.") \ No newline at end of file + # We can't delete the file or file data, because it is referenced elsewhere, + # but we can at least mark it as deleted on the table. + session.rollback() + file_model = session.query(FileModel).filter_by(id=file_id).first() + file_model.archived = True + session.commit() + app.logger.error("Failed to delete file, so archiving it instead. %i, due to %s" % (file_id, str(ie))) diff --git a/tests/test_files_api.py b/tests/test_files_api.py index ecce309c..4c8c3eb6 100644 --- a/tests/test_files_api.py +++ b/tests/test_files_api.py @@ -9,6 +9,8 @@ from crc.models.workflow import WorkflowSpecModel from crc.services.file_service import FileService from crc.services.workflow_processor import WorkflowProcessor from example_data import ExampleDataLoader +from crc.services.approval_service import ApprovalService +from crc.models.approval import ApprovalModel, ApprovalStatus class TestFilesApi(BaseTest): @@ -218,6 +220,41 @@ class TestFilesApi(BaseTest): rv = self.app.get('/v1.0/file/%i' % file.id, headers=self.logged_in_headers()) self.assertEqual(404, rv.status_code) + def test_delete_file_after_approval(self): + self.create_reference_document() + workflow = self.create_workflow("empty_workflow") + FileService.add_workflow_file(workflow_id=workflow.id, + name="anything.png", content_type="text", + binary_data=b'5678', irb_doc_code="UVACompl_PRCAppr") + FileService.add_workflow_file(workflow_id=workflow.id, + name="anotother_anything.png", content_type="text", + binary_data=b'1234', irb_doc_code="Study_App_Doc") + + ApprovalService.add_approval(study_id=workflow.study_id, workflow_id=workflow.id, approver_uid="dhf8r") + + file = session.query(FileModel).\ + filter(FileModel.workflow_id == workflow.id).\ + filter(FileModel.name == "anything.png").first() + self.assertFalse(file.archived) + rv = self.app.get('/v1.0/file/%i' % file.id, headers=self.logged_in_headers()) + self.assert_success(rv) + + rv = self.app.delete('/v1.0/file/%i' % file.id, headers=self.logged_in_headers()) + self.assert_success(rv) + + session.refresh(file) + self.assertTrue(file.archived) + + ApprovalService.add_approval(study_id=workflow.study_id, workflow_id=workflow.id, approver_uid="dhf8r") + + approvals = session.query(ApprovalModel)\ + .filter(ApprovalModel.status == ApprovalStatus.PENDING.value)\ + .filter(ApprovalModel.study_id == workflow.study_id).all() + + self.assertEquals(1, len(approvals)) + self.assertEquals(1, len(approvals[0].approval_files)) + + def test_change_primary_bpmn(self): self.load_example_data() spec = session.query(WorkflowSpecModel).first() diff --git a/tests/test_workflow_spec_validation_api.py b/tests/test_workflow_spec_validation_api.py index e2f652d9..cb9b6b77 100644 --- a/tests/test_workflow_spec_validation_api.py +++ b/tests/test_workflow_spec_validation_api.py @@ -71,7 +71,7 @@ class TestWorkflowSpecValidation(BaseTest): self.load_example_data() errors = self.validate_workflow("invalid_expression") self.assertEqual(2, len(errors)) - self.assertEqual("workflow_execution_exception", errors[0]['code']) + self.assertEqual("workflow_validation_exception", errors[0]['code']) self.assertEqual("ExclusiveGateway_003amsm", errors[0]['task_id']) self.assertEqual("Has Bananas Gateway", errors[0]['task_name']) self.assertEqual("invalid_expression.bpmn", errors[0]['file_name']) @@ -92,7 +92,7 @@ class TestWorkflowSpecValidation(BaseTest): self.load_example_data() errors = self.validate_workflow("invalid_script") self.assertEqual(2, len(errors)) - self.assertEqual("workflow_execution_exception", errors[0]['code']) + self.assertEqual("workflow_validation_exception", errors[0]['code']) self.assertTrue("NoSuchScript" in errors[0]['message']) self.assertEqual("Invalid_Script_Task", errors[0]['task_id']) self.assertEqual("An Invalid Script Reference", errors[0]['task_name']) From 1324533865b4996d5e7812e785355eeb87162413 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Thu, 4 Jun 2020 09:49:42 -0400 Subject: [PATCH 04/10] Some additional cleanup - when a file is "archived" it is no longer returned for any endpoints about files, but it is directly accessible via id, in the event some request is made for it at a later date. --- crc/api.yml | 2 +- crc/api/approval.py | 2 +- crc/services/approval_service.py | 2 +- crc/services/file_service.py | 17 ++++++++------- tests/test_file_service.py | 33 ++++++++++++++++++++++++++++- tests/test_files_api.py | 36 +++++++++++++++++++++++++++++++- tests/test_tasks_api.py | 1 - 7 files changed, 80 insertions(+), 13 deletions(-) diff --git a/crc/api.yml b/crc/api.yml index a3fd835b..07e61251 100644 --- a/crc/api.yml +++ b/crc/api.yml @@ -468,7 +468,7 @@ paths: $ref: "#/components/schemas/File" delete: operationId: crc.api.file.delete_file - summary: Removes an existing file. In the event the file can not be deleted, it is archived. + summary: Removes an existing file. In the event the file can not be deleted, it is marked as "archived" in the database and is no longer returned unless specifically requested by id. tags: - Files responses: diff --git a/crc/api/approval.py b/crc/api/approval.py index 1408fec5..4f413aa4 100644 --- a/crc/api/approval.py +++ b/crc/api/approval.py @@ -31,7 +31,7 @@ def get_approvals_for_study(study_id=None): # ----- Being decent into madness ---- # def get_csv(): - """A huge bit of a one-off for RRT, but 3 weeks of midnight work can convince a + """A damn lie, it's a json file. A huge bit of a one-off for RRT, but 3 weeks of midnight work can convince a man to do just about anything""" approvals = ApprovalService.get_all_approvals(include_cancelled=False) output = [] diff --git a/crc/services/approval_service.py b/crc/services/approval_service.py index a71f4ba1..b6605f7b 100644 --- a/crc/services/approval_service.py +++ b/crc/services/approval_service.py @@ -104,7 +104,7 @@ class ApprovalService(object): # Construct as hash of the latest files to see if things have changed since # the last approval. workflow = db.session.query(WorkflowModel).filter(WorkflowModel.id == workflow_id).first() - workflow_data_files = FileService.get_workflow_data_files(workflow_id, include_archives=False) + workflow_data_files = FileService.get_workflow_data_files(workflow_id) current_data_file_ids = list(data_file.id for data_file in workflow_data_files) if len(current_data_file_ids) == 0: diff --git a/crc/services/file_service.py b/crc/services/file_service.py index cda16a88..c0046a26 100644 --- a/crc/services/file_service.py +++ b/crc/services/file_service.py @@ -96,6 +96,7 @@ class FileService(object): def get_workflow_files(workflow_id): """Returns all the file models associated with a running workflow.""" return session.query(FileModel).filter(FileModel.workflow_id == workflow_id).\ + filter(FileModel.archived == False).\ order_by(FileModel.id).all() @staticmethod @@ -139,6 +140,7 @@ class FileService(object): else: file_model.type = FileType[file_extension] file_model.content_type = content_type + file_model.archived = False # Unarchive the file if it is archived. if latest_data_model is None: version = 1 @@ -188,14 +190,15 @@ class FileService(object): def get_files_for_study(study_id, irb_doc_code=None): query = session.query(FileModel).\ join(WorkflowModel).\ - filter(WorkflowModel.study_id == study_id) + filter(WorkflowModel.study_id == study_id).\ + filter(FileModel.archived == False) if irb_doc_code: query = query.filter(FileModel.irb_doc_code == irb_doc_code) return query.all() @staticmethod def get_files(workflow_spec_id=None, workflow_id=None, - name=None, is_reference=False, irb_doc_code=None, include_archives=True): + name=None, is_reference=False, irb_doc_code=None): query = session.query(FileModel).filter_by(is_reference=is_reference) if workflow_spec_id: query = query.filter_by(workflow_spec_id=workflow_spec_id) @@ -209,8 +212,7 @@ class FileService(object): if name: query = query.filter_by(name=name) - if not include_archives: - query = query.filter(FileModel.archived == False) + query = query.filter(FileModel.archived == False) query = query.order_by(FileModel.id) @@ -242,11 +244,11 @@ class FileService(object): return latest_data_files @staticmethod - def get_workflow_data_files(workflow_id=None, include_archives=True): + def get_workflow_data_files(workflow_id=None): """Returns all the FileDataModels related to a running workflow - So these are the latest data files that were uploaded or generated that go along with this workflow. Not related to the spec in any way""" - file_models = FileService.get_files(workflow_id=workflow_id, include_archives=include_archives) + file_models = FileService.get_files(workflow_id=workflow_id) latest_data_files = [] for file_model in file_models: latest_data_files.append(FileService.get_file_data(file_model.id)) @@ -274,7 +276,8 @@ class FileService(object): @staticmethod def get_workflow_file_data(workflow, file_name): - """Given a SPIFF Workflow Model, tracks down a file with the given name in the database and returns its data""" + """This method should be deleted, find where it is used, and remove this method. + Given a SPIFF Workflow Model, tracks down a file with the given name in the database and returns its data""" workflow_spec_model = FileService.find_spec_model_in_db(workflow) if workflow_spec_model is None: diff --git a/tests/test_file_service.py b/tests/test_file_service.py index 705fef95..02a70ce8 100644 --- a/tests/test_file_service.py +++ b/tests/test_file_service.py @@ -1,8 +1,9 @@ from tests.base_test import BaseTest + +from crc import db from crc.services.file_service import FileService from crc.services.workflow_processor import WorkflowProcessor - class TestFileService(BaseTest): """Largely tested via the test_file_api, and time is tight, but adding new tests here.""" @@ -46,12 +47,42 @@ class TestFileService(BaseTest): name="anything.png", content_type="text", binary_data=b'5678') + def test_replace_archive_file_unarchives_the_file_and_updates(self): + self.load_example_data() + self.create_reference_document() + workflow = self.create_workflow('file_upload_form') + processor = WorkflowProcessor(workflow) + task = processor.next_task() + irb_code = "UVACompl_PRCAppr" # The first file referenced in pb required docs. + FileService.add_workflow_file(workflow_id=workflow.id, + irb_doc_code=irb_code, + name="anything.png", content_type="text", + binary_data=b'1234') + + # Archive the file + file_models = FileService.get_workflow_files(workflow_id=workflow.id) + self.assertEquals(1, len(file_models)) + file_model = file_models[0] + file_model.archived = True + db.session.add(file_model) + + # Assure that the file no longer comes back. + file_models = FileService.get_workflow_files(workflow_id=workflow.id) + self.assertEquals(0, len(file_models)) + + # Add the file again with different data + FileService.add_workflow_file(workflow_id=workflow.id, + irb_doc_code=irb_code, + name="anything.png", content_type="text", + binary_data=b'5678') + file_models = FileService.get_workflow_files(workflow_id=workflow.id) self.assertEquals(1, len(file_models)) file_data = FileService.get_workflow_data_files(workflow_id=workflow.id) self.assertEquals(1, len(file_data)) self.assertEquals(2, file_data[0].version) + self.assertEquals(b'5678', file_data[0].data) def test_add_file_from_form_allows_multiple_files_with_different_names(self): self.load_example_data() diff --git a/tests/test_files_api.py b/tests/test_files_api.py index 4c8c3eb6..2d14a8b5 100644 --- a/tests/test_files_api.py +++ b/tests/test_files_api.py @@ -3,7 +3,7 @@ import json from tests.base_test import BaseTest -from crc import session +from crc import session, db from crc.models.file import FileModel, FileType, FileSchema, FileModelSchema from crc.models.workflow import WorkflowSpecModel from crc.services.file_service import FileService @@ -48,6 +48,7 @@ class TestFilesApi(BaseTest): json_data = json.loads(rv.get_data(as_text=True)) self.assertEqual(2, len(json_data)) + def test_create_file(self): self.load_example_data() spec = session.query(WorkflowSpecModel).first() @@ -91,6 +92,39 @@ class TestFilesApi(BaseTest): self.assert_success(rv) + def test_archive_file_no_longer_shows_up(self): + self.load_example_data() + self.create_reference_document() + workflow = self.create_workflow('file_upload_form') + processor = WorkflowProcessor(workflow) + task = processor.next_task() + data = {'file': (io.BytesIO(b"abcdef"), 'random_fact.svg')} + correct_name = task.task_spec.form.fields[0].id + + data = {'file': (io.BytesIO(b"abcdef"), 'random_fact.svg')} + rv = self.app.post('/v1.0/file?study_id=%i&workflow_id=%s&task_id=%i&form_field_key=%s' % + (workflow.study_id, workflow.id, task.id, correct_name), data=data, follow_redirects=True, + content_type='multipart/form-data', headers=self.logged_in_headers()) + + self.assert_success(rv) + rv = self.app.get('/v1.0/file?workflow_id=%s' % workflow.id, headers=self.logged_in_headers()) + self.assert_success(rv) + self.assertEquals(1, len(json.loads(rv.get_data(as_text=True)))) + + file_model = db.session.query(FileModel).filter(FileModel.workflow_id == workflow.id).all() + self.assertEquals(1, len(file_model)) + file_model[0].archived = True + db.session.commit() + + rv = self.app.get('/v1.0/file?workflow_id=%s' % workflow.id, headers=self.logged_in_headers()) + self.assert_success(rv) + self.assertEquals(0, len(json.loads(rv.get_data(as_text=True)))) + + + + + + def test_set_reference_file(self): file_name = "irb_document_types.xls" data = {'file': (io.BytesIO(b"abcdef"), "does_not_matter.xls")} diff --git a/tests/test_tasks_api.py b/tests/test_tasks_api.py index 37849867..52c5f39d 100644 --- a/tests/test_tasks_api.py +++ b/tests/test_tasks_api.py @@ -208,7 +208,6 @@ class TestTasksApi(BaseTest): self.assert_success(rv) - def test_get_documentation_populated_in_end(self): self.load_example_data() workflow = self.create_workflow('random_fact') From bbcbfef1baf6800e3919d9de94e3a67019ad6fab Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Thu, 4 Jun 2020 10:09:36 -0400 Subject: [PATCH 05/10] Fixing the migrations so I don't break the universe. --- crc/models/file.py | 2 +- crc/services/file_service.py | 6 +++++- migrations/versions/17597692d0b0_.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) create mode 100644 migrations/versions/17597692d0b0_.py diff --git a/crc/models/file.py b/crc/models/file.py index a0c94985..15a48709 100644 --- a/crc/models/file.py +++ b/crc/models/file.py @@ -85,7 +85,7 @@ class FileModel(db.Model): # A request was made to delete the file, but we can't because there are # active approvals or running workflows that depend on it. So we archive # it instead, hide it in the interface. - archived = db.Column(db.Boolean, default=False) + archived = db.Column(db.Boolean, default=False, nullable=False) class File(object): @classmethod diff --git a/crc/services/file_service.py b/crc/services/file_service.py index c0046a26..f7d40fd7 100644 --- a/crc/services/file_service.py +++ b/crc/services/file_service.py @@ -128,7 +128,11 @@ class FileService(object): md5_checksum = UUID(hashlib.md5(binary_data).hexdigest()) if (latest_data_model is not None) and (md5_checksum == latest_data_model.md5_hash): - # This file does not need to be updated, it's the same file. + # This file does not need to be updated, it's the same file. If it is arhived, + # then de-arvhive it. + file_model.archived = False + session.add(file_model) + session.commit() return file_model # Verify the extension diff --git a/migrations/versions/17597692d0b0_.py b/migrations/versions/17597692d0b0_.py new file mode 100644 index 00000000..0b15c956 --- /dev/null +++ b/migrations/versions/17597692d0b0_.py @@ -0,0 +1,28 @@ +"""empty message + +Revision ID: 17597692d0b0 +Revises: 13424d5a6de8 +Create Date: 2020-06-03 17:33:56.454339 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '17597692d0b0' +down_revision = '13424d5a6de8' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('file', sa.Column('archived', sa.Boolean(), nullable=True, default=False)) + op.execute("UPDATE file SET archived = false") + op.alter_column('file', 'archived', nullable=False) + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column('file', 'archived') + # ### end Alembic commands ### From 68aeaf1273635ad8dec22a84c68969cb0b4bcb1c Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Thu, 4 Jun 2020 10:33:17 -0400 Subject: [PATCH 06/10] BE VERY CAREFUL where you create a new LdapService() - construction is expensive. Adding a few more details to the "csv" endpoint for RRT. --- crc/api/approval.py | 13 +++++++++++-- crc/models/approval.py | 7 +++---- crc/services/study_service.py | 4 ++-- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/crc/api/approval.py b/crc/api/approval.py index 4f413aa4..ffdd4fe0 100644 --- a/crc/api/approval.py +++ b/crc/api/approval.py @@ -49,6 +49,7 @@ def get_csv(): last_task = find_task(data['last_task']['__uuid__'], data['task_tree']) personnel = extract_value(last_task, 'personnel') training_val = extract_value(last_task, 'RequiredTraining') + pi_supervisor = extract_value(last_task, 'PISupervisor')['value'] review_complete = 'AllRequiredTraining' in training_val pi_uid = workflow.study.primary_investigator_id pi_details = ldapService.user_info(pi_uid) @@ -59,14 +60,22 @@ def get_csv(): details.append(ldapService.user_info(uid)) for person in details: - output.append({ + record = { "study_id": approval.study_id, "pi_uid": pi_details.uid, "pi": pi_details.display_name, "name": person.display_name, + "uid": person.uid, "email": person.email_address, + "supervisor": "", "review_complete": review_complete, - }) + } + # We only know the PI's supervisor. + if person.uid == pi_details.uid: + record["supervisor"] = pi_supervisor + + output.append(record) + except Exception as e: errors.append("Error pulling data for workflow #%i: %s" % (approval.workflow_id, str(e))) return {"results": output, "errors": errors } diff --git a/crc/models/approval.py b/crc/models/approval.py index d2597e5e..b72ee19a 100644 --- a/crc/models/approval.py +++ b/crc/models/approval.py @@ -48,6 +48,7 @@ class ApprovalModel(db.Model): class Approval(object): + ldap_service = LdapService() def __init__(self, **kwargs): self.__dict__.update(kwargs) @@ -71,11 +72,9 @@ class Approval(object): if model.study: instance.title = model.study.title - - ldap_service = LdapService() try: - instance.approver = ldap_service.user_info(model.approver_uid) - instance.primary_investigator = ldap_service.user_info(model.study.primary_investigator_id) + instance.approver = Approval.ldap_service.user_info(model.approver_uid) + instance.primary_investigator = Approval.ldap_service.user_info(model.study.primary_investigator_id) except ApiError as ae: app.logger.error("Ldap lookup failed for approval record %i" % model.id) diff --git a/crc/services/study_service.py b/crc/services/study_service.py index 0bc80bcf..d71c5796 100644 --- a/crc/services/study_service.py +++ b/crc/services/study_service.py @@ -26,6 +26,7 @@ from crc.models.approval import Approval class StudyService(object): """Provides common tools for working with a Study""" + ldap_service = LdapService() @staticmethod def get_studies_for_user(user): @@ -206,8 +207,7 @@ class StudyService(object): @staticmethod def get_ldap_dict_if_available(user_id): try: - ldap_service = LdapService() - return LdapSchema().dump(ldap_service.user_info(user_id)) + return LdapSchema().dump(StudyService.ldap_service.user_info(user_id)) except ApiError as ae: app.logger.info(str(ae)) return {"error": str(ae)} From 50d2acac9c66daa64c761e22de2da3c484a2c061 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Thu, 4 Jun 2020 11:57:00 -0400 Subject: [PATCH 07/10] Made a very stupid mistake with LDAP connections, pushing up quickly to production. --- crc/api/approval.py | 3 ++- crc/models/approval.py | 7 +++---- crc/services/approval_service.py | 9 ++++++--- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/crc/api/approval.py b/crc/api/approval.py index ffdd4fe0..c8cd6194 100644 --- a/crc/api/approval.py +++ b/crc/api/approval.py @@ -24,7 +24,8 @@ def get_approvals(everything=False): def get_approvals_for_study(study_id=None): db_approvals = ApprovalService.get_approvals_for_study(study_id) - approvals = [Approval.from_model(approval_model) for approval_model in db_approvals] + ldap_service = LdapService() + approvals = [Approval.from_model(approval_model, ldap_service) for approval_model in db_approvals] results = ApprovalSchema(many=True).dump(approvals) return results diff --git a/crc/models/approval.py b/crc/models/approval.py index b72ee19a..b6dab996 100644 --- a/crc/models/approval.py +++ b/crc/models/approval.py @@ -48,13 +48,12 @@ class ApprovalModel(db.Model): class Approval(object): - ldap_service = LdapService() def __init__(self, **kwargs): self.__dict__.update(kwargs) @classmethod - def from_model(cls, model: ApprovalModel): + def from_model(cls, model: ApprovalModel, ldap_service: LdapSchema): # TODO: Reduce the code by iterating over model's dict keys instance = cls() instance.id = model.id @@ -73,8 +72,8 @@ class Approval(object): if model.study: instance.title = model.study.title try: - instance.approver = Approval.ldap_service.user_info(model.approver_uid) - instance.primary_investigator = Approval.ldap_service.user_info(model.study.primary_investigator_id) + instance.approver = ldap_service.user_info(model.approver_uid) + instance.primary_investigator = ldap_service.user_info(model.study.primary_investigator_id) except ApiError as ae: app.logger.error("Ldap lookup failed for approval record %i" % model.id) diff --git a/crc/services/approval_service.py b/crc/services/approval_service.py index b6605f7b..af2f86b0 100644 --- a/crc/services/approval_service.py +++ b/crc/services/approval_service.py @@ -9,6 +9,7 @@ from crc.models.approval import ApprovalModel, ApprovalStatus, ApprovalFile, App from crc.models.study import StudyModel from crc.models.workflow import WorkflowModel from crc.services.file_service import FileService +from crc.services.ldap_service import LdapService class ApprovalService(object): @@ -27,11 +28,12 @@ class ApprovalService(object): query=query.filter(ApprovalModel.status != ApprovalStatus.CANCELED.value) approvals = query.all() + ldap_service = LdapService() for approval_model in approvals: if approval_model.approver_uid == approver_uid: - main_approval = Approval.from_model(approval_model) + main_approval = Approval.from_model(approval_model, ldap_service) else: - related_approvals.append(Approval.from_model(approval_model)) + related_approvals.append(Approval.from_model(approval_model, ldap_service)) if not main_approval and len(related_approvals) > 0: main_approval = related_approvals[0] related_approvals = related_approvals[1:] @@ -68,11 +70,12 @@ class ApprovalService(object): def get_approvals_for_study(study_id, include_cancelled=True): """Returns an array of Approval objects for the study, it does not compute the related approvals.""" + ldap_service = LdapService() query = session.query(ApprovalModel).filter_by(study_id=study_id) if not include_cancelled: query = query.filter(ApprovalModel.status != ApprovalStatus.CANCELED.value) db_approvals = query.all() - return [Approval.from_model(approval_model) for approval_model in db_approvals] + return [Approval.from_model(approval_model, ldap_service) for approval_model in db_approvals] @staticmethod From fed6e86f9288c76df0c07214f8d6ea75ef2b6e65 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Thu, 4 Jun 2020 14:59:36 -0400 Subject: [PATCH 08/10] Trying to fix LDAP issues on production. Changing LDAP to static only methods, caching the connection and calling bind before all connection requests. Also assuring we don't load the documents.xls file over and over again. --- crc/api/approval.py | 8 ++--- crc/api/user.py | 7 ++--- crc/models/approval.py | 8 ++--- crc/services/approval_service.py | 8 ++--- crc/services/file_service.py | 8 ++--- crc/services/ldap_service.py | 53 ++++++++++++++++++-------------- crc/services/lookup_service.py | 2 +- crc/services/study_service.py | 3 +- tests/test_ldap_service.py | 6 ++-- 9 files changed, 50 insertions(+), 53 deletions(-) diff --git a/crc/api/approval.py b/crc/api/approval.py index c8cd6194..9c42c82e 100644 --- a/crc/api/approval.py +++ b/crc/api/approval.py @@ -24,8 +24,7 @@ def get_approvals(everything=False): def get_approvals_for_study(study_id=None): db_approvals = ApprovalService.get_approvals_for_study(study_id) - ldap_service = LdapService() - approvals = [Approval.from_model(approval_model, ldap_service) for approval_model in db_approvals] + approvals = [Approval.from_model(approval_model) for approval_model in db_approvals] results = ApprovalSchema(many=True).dump(approvals) return results @@ -37,7 +36,6 @@ def get_csv(): approvals = ApprovalService.get_all_approvals(include_cancelled=False) output = [] errors = [] - ldapService = LdapService() for approval in approvals: try: if approval.status != ApprovalStatus.APPROVED.value: @@ -53,12 +51,12 @@ def get_csv(): pi_supervisor = extract_value(last_task, 'PISupervisor')['value'] review_complete = 'AllRequiredTraining' in training_val pi_uid = workflow.study.primary_investigator_id - pi_details = ldapService.user_info(pi_uid) + pi_details = LdapService.user_info(pi_uid) details = [] details.append(pi_details) for person in personnel: uid = person['PersonnelComputingID']['value'] - details.append(ldapService.user_info(uid)) + details.append(LdapService.user_info(uid)) for person in details: record = { diff --git a/crc/api/user.py b/crc/api/user.py index 172b7496..36e9926e 100644 --- a/crc/api/user.py +++ b/crc/api/user.py @@ -59,10 +59,7 @@ def sso_login(): app.logger.info("SSO_LOGIN: Full URL: " + request.url) app.logger.info("SSO_LOGIN: User Id: " + uid) app.logger.info("SSO_LOGIN: Will try to redirect to : " + str(redirect)) - - ldap_service = LdapService() - info = ldap_service.user_info(uid) - + info = LdapService.user_info(uid) return _handle_login(info, redirect) @app.route('/sso') @@ -151,7 +148,7 @@ def backdoor( """ if not 'PRODUCTION' in app.config or not app.config['PRODUCTION']: - ldap_info = LdapService().user_info(uid) + ldap_info = LdapService.user_info(uid) return _handle_login(ldap_info, redirect) else: raise ApiError('404', 'unknown') diff --git a/crc/models/approval.py b/crc/models/approval.py index b6dab996..ee19f4b7 100644 --- a/crc/models/approval.py +++ b/crc/models/approval.py @@ -53,7 +53,7 @@ class Approval(object): self.__dict__.update(kwargs) @classmethod - def from_model(cls, model: ApprovalModel, ldap_service: LdapSchema): + def from_model(cls, model: ApprovalModel): # TODO: Reduce the code by iterating over model's dict keys instance = cls() instance.id = model.id @@ -72,12 +72,12 @@ class Approval(object): if model.study: instance.title = model.study.title try: - instance.approver = ldap_service.user_info(model.approver_uid) - instance.primary_investigator = ldap_service.user_info(model.study.primary_investigator_id) + instance.approver = LdapService.user_info(model.approver_uid) + instance.primary_investigator = LdapService.user_info(model.study.primary_investigator_id) except ApiError as ae: app.logger.error("Ldap lookup failed for approval record %i" % model.id) - doc_dictionary = FileService.get_reference_data(FileService.DOCUMENT_LIST, 'code', ['id']) + doc_dictionary = FileService.get_doc_dictionary() instance.associated_files = [] for approval_file in model.approval_files: try: diff --git a/crc/services/approval_service.py b/crc/services/approval_service.py index af2f86b0..98c99d0b 100644 --- a/crc/services/approval_service.py +++ b/crc/services/approval_service.py @@ -28,12 +28,11 @@ class ApprovalService(object): query=query.filter(ApprovalModel.status != ApprovalStatus.CANCELED.value) approvals = query.all() - ldap_service = LdapService() for approval_model in approvals: if approval_model.approver_uid == approver_uid: - main_approval = Approval.from_model(approval_model, ldap_service) + main_approval = Approval.from_model(approval_model) else: - related_approvals.append(Approval.from_model(approval_model, ldap_service)) + related_approvals.append(Approval.from_model(approval_model)) if not main_approval and len(related_approvals) > 0: main_approval = related_approvals[0] related_approvals = related_approvals[1:] @@ -70,12 +69,11 @@ class ApprovalService(object): def get_approvals_for_study(study_id, include_cancelled=True): """Returns an array of Approval objects for the study, it does not compute the related approvals.""" - ldap_service = LdapService() query = session.query(ApprovalModel).filter_by(study_id=study_id) if not include_cancelled: query = query.filter(ApprovalModel.status != ApprovalStatus.CANCELED.value) db_approvals = query.all() - return [Approval.from_model(approval_model, ldap_service) for approval_model in db_approvals] + return [Approval.from_model(approval_model) for approval_model in db_approvals] @staticmethod diff --git a/crc/services/file_service.py b/crc/services/file_service.py index f7d40fd7..ff234a79 100644 --- a/crc/services/file_service.py +++ b/crc/services/file_service.py @@ -45,10 +45,8 @@ class FileService(object): @staticmethod def is_allowed_document(code): - data_model = FileService.get_reference_file_data(FileService.DOCUMENT_LIST) - xls = ExcelFile(data_model.data) - df = xls.parse(xls.sheet_names[0]) - return code in df['code'].values + doc_dict = FileService.get_doc_dictionary() + return code in doc_dict @staticmethod def add_workflow_file(workflow_id, irb_doc_code, name, content_type, binary_data): @@ -333,4 +331,4 @@ class FileService(object): file_model = session.query(FileModel).filter_by(id=file_id).first() file_model.archived = True session.commit() - app.logger.error("Failed to delete file, so archiving it instead. %i, due to %s" % (file_id, str(ie))) + app.logger.info("Failed to delete file, so archiving it instead. %i, due to %s" % (file_id, str(ie))) diff --git a/crc/services/ldap_service.py b/crc/services/ldap_service.py index 7d95a71b..6767f271 100644 --- a/crc/services/ldap_service.py +++ b/crc/services/ldap_service.py @@ -19,37 +19,42 @@ class LdapService(object): cn_single_search = '(&(objectclass=person)(cn=%s*))' cn_double_search = '(&(objectclass=person)(&(cn=%s*)(cn=*%s*)))' - def __init__(self): - if app.config['TESTING']: - server = Server('my_fake_server') - self.conn = Connection(server, client_strategy=MOCK_SYNC) - file_path = os.path.abspath(os.path.join(app.root_path, '..', 'tests', 'data', 'ldap_response.json')) - self.conn.strategy.entries_from_json(file_path) - self.conn.bind() - else: - server = Server(app.config['LDAP_URL'], connect_timeout=app.config['LDAP_TIMEOUT_SEC']) - self.conn = Connection(server, - auto_bind=True, - receive_timeout=app.config['LDAP_TIMEOUT_SEC'], - ) + conn = None - def __del__(self): - if self.conn: - self.conn.unbind() + @staticmethod + def __get_conn(): + if not LdapService.conn: + if app.config['TESTING']: + server = Server('my_fake_server') + conn = Connection(server, client_strategy=MOCK_SYNC) + file_path = os.path.abspath(os.path.join(app.root_path, '..', 'tests', 'data', 'ldap_response.json')) + conn.strategy.entries_from_json(file_path) + else: + server = Server(app.config['LDAP_URL'], connect_timeout=app.config['LDAP_TIMEOUT_SEC']) + conn = Connection(server, + receive_timeout=app.config['LDAP_TIMEOUT_SEC'], + ) + LdapService.conn = conn + return LdapService.conn - def user_info(self, uva_uid): + + @staticmethod + def user_info(uva_uid): user_info = db.session.query(LdapModel).filter(LdapModel.uid == uva_uid).first() if not user_info: search_string = LdapService.uid_search_string % uva_uid - self.conn.search(LdapService.search_base, search_string, attributes=LdapService.attributes) - if len(self.conn.entries) < 1: + conn = LdapService.__get_conn() + conn.bind() + conn.search(LdapService.search_base, search_string, attributes=LdapService.attributes) + if len(conn.entries) < 1: raise ApiError("missing_ldap_record", "Unable to locate a user with id %s in LDAP" % uva_uid) - entry = self.conn.entries[0] + entry = conn.entries[0] user_info = LdapModel.from_entry(entry) db.session.add(user_info) return user_info - def search_users(self, query, limit): + @staticmethod + def search_users(query, limit): if len(query.strip()) < 3: return [] elif query.endswith(' '): @@ -66,12 +71,14 @@ class LdapService(object): results = [] print(search_string) try: - self.conn.search(LdapService.search_base, search_string, attributes=LdapService.attributes) + conn = LdapService.__get_conn() + conn.bind() + conn.search(LdapService.search_base, search_string, attributes=LdapService.attributes) # Entries are returned as a generator, accessing entries # can make subsequent calls to the ldap service, so limit # those here. count = 0 - for entry in self.conn.entries: + for entry in conn.entries: if count > limit: break results.append(LdapSchema().dump(LdapModel.from_entry(entry))) diff --git a/crc/services/lookup_service.py b/crc/services/lookup_service.py index 71424b6b..b3e0bddc 100644 --- a/crc/services/lookup_service.py +++ b/crc/services/lookup_service.py @@ -189,7 +189,7 @@ class LookupService(object): @staticmethod def _run_ldap_query(query, limit): - users = LdapService().search_users(query, limit) + users = LdapService.search_users(query, limit) """Converts the user models into something akin to the LookupModel in models/file.py, so this can be returned in the same way diff --git a/crc/services/study_service.py b/crc/services/study_service.py index d71c5796..4024b5f0 100644 --- a/crc/services/study_service.py +++ b/crc/services/study_service.py @@ -26,7 +26,6 @@ from crc.models.approval import Approval class StudyService(object): """Provides common tools for working with a Study""" - ldap_service = LdapService() @staticmethod def get_studies_for_user(user): @@ -207,7 +206,7 @@ class StudyService(object): @staticmethod def get_ldap_dict_if_available(user_id): try: - return LdapSchema().dump(StudyService.ldap_service.user_info(user_id)) + return LdapSchema().dump(LdapService().user_info(user_id)) except ApiError as ae: app.logger.info(str(ae)) return {"error": str(ae)} diff --git a/tests/test_ldap_service.py b/tests/test_ldap_service.py index 9e2e8931..88c748ed 100644 --- a/tests/test_ldap_service.py +++ b/tests/test_ldap_service.py @@ -7,13 +7,13 @@ from crc.services.ldap_service import LdapService class TestLdapService(BaseTest): def setUp(self): - self.ldap_service = LdapService() + pass def tearDown(self): pass def test_get_single_user(self): - user_info = self.ldap_service.user_info("lb3dp") + user_info = LdapService.user_info("lb3dp") self.assertIsNotNone(user_info) self.assertEqual("lb3dp", user_info.uid) self.assertEqual("Laura Barnes", user_info.display_name) @@ -27,7 +27,7 @@ class TestLdapService(BaseTest): def test_find_missing_user(self): try: - user_info = self.ldap_service.user_info("nosuch") + user_info = LdapService.user_info("nosuch") self.assertFalse(True, "An API error should be raised.") except ApiError as ae: self.assertEquals("missing_ldap_record", ae.code) \ No newline at end of file From 9cfe00dfd0b6db7953a874c03580ddc5f8b32bb7 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Thu, 4 Jun 2020 15:38:45 -0400 Subject: [PATCH 09/10] Don't bind all the time. --- crc/services/ldap_service.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crc/services/ldap_service.py b/crc/services/ldap_service.py index 6767f271..3bf24105 100644 --- a/crc/services/ldap_service.py +++ b/crc/services/ldap_service.py @@ -29,9 +29,10 @@ class LdapService(object): conn = Connection(server, client_strategy=MOCK_SYNC) file_path = os.path.abspath(os.path.join(app.root_path, '..', 'tests', 'data', 'ldap_response.json')) conn.strategy.entries_from_json(file_path) + conn.bind() else: server = Server(app.config['LDAP_URL'], connect_timeout=app.config['LDAP_TIMEOUT_SEC']) - conn = Connection(server, + conn = Connection(server, auto_bind=True, receive_timeout=app.config['LDAP_TIMEOUT_SEC'], ) LdapService.conn = conn @@ -44,7 +45,6 @@ class LdapService(object): if not user_info: search_string = LdapService.uid_search_string % uva_uid conn = LdapService.__get_conn() - conn.bind() conn.search(LdapService.search_base, search_string, attributes=LdapService.attributes) if len(conn.entries) < 1: raise ApiError("missing_ldap_record", "Unable to locate a user with id %s in LDAP" % uva_uid) @@ -72,7 +72,6 @@ class LdapService(object): print(search_string) try: conn = LdapService.__get_conn() - conn.bind() conn.search(LdapService.search_base, search_string, attributes=LdapService.attributes) # Entries are returned as a generator, accessing entries # can make subsequent calls to the ldap service, so limit From b6abb0cbe267c08522abe9ff059eb59257b11316 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Thu, 4 Jun 2020 18:03:59 -0400 Subject: [PATCH 10/10] using a restartable strategy to get around login errors --- crc/services/ldap_service.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crc/services/ldap_service.py b/crc/services/ldap_service.py index 3bf24105..61097921 100644 --- a/crc/services/ldap_service.py +++ b/crc/services/ldap_service.py @@ -4,7 +4,7 @@ from attr import asdict from ldap3.core.exceptions import LDAPExceptionError from crc import app, db -from ldap3 import Connection, Server, MOCK_SYNC +from ldap3 import Connection, Server, MOCK_SYNC, RESTARTABLE from crc.api.common import ApiError from crc.models.ldap import LdapModel, LdapSchema @@ -34,7 +34,7 @@ class LdapService(object): server = Server(app.config['LDAP_URL'], connect_timeout=app.config['LDAP_TIMEOUT_SEC']) conn = Connection(server, auto_bind=True, receive_timeout=app.config['LDAP_TIMEOUT_SEC'], - ) + client_strategy=RESTARTABLE) LdapService.conn = conn return LdapService.conn