From abc90cdcb17cd482e72a442ad6f46d603a9550bd Mon Sep 17 00:00:00 2001 From: Carlos Lopez Date: Wed, 20 May 2020 15:10:22 -0600 Subject: [PATCH 01/16] Adding serialiazer for study files --- crc/models/file.py | 8 +++++++- crc/models/study.py | 18 ++++++++++++++++++ crc/models/workflow.py | 3 ++- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/crc/models/file.py b/crc/models/file.py index 6d83e4b2..942b2a80 100644 --- a/crc/models/file.py +++ b/crc/models/file.py @@ -6,7 +6,7 @@ from marshmallow_sqlalchemy import SQLAlchemyAutoSchema from sqlalchemy import func, Index from sqlalchemy.dialects.postgresql import UUID -from crc import db +from crc import db, ma class FileType(enum.Enum): @@ -139,3 +139,9 @@ class LookupDataSchema(SQLAlchemyAutoSchema): include_relationships = False include_fk = False # Includes foreign keys + +class SimpleFileSchema(ma.Schema): + + class Meta: + model = FileModel + fields = ["name"] diff --git a/crc/models/study.py b/crc/models/study.py index 6716df8e..79186f3f 100644 --- a/crc/models/study.py +++ b/crc/models/study.py @@ -5,6 +5,7 @@ from sqlalchemy import func from crc import db, ma from crc.api.common import ApiErrorSchema +from crc.models.file import FileModel, SimpleFileSchema from crc.models.protocol_builder import ProtocolBuilderStatus, ProtocolBuilderStudy from crc.models.workflow import WorkflowSpecCategoryModel, WorkflowState, WorkflowStatus, WorkflowSpecModel, \ WorkflowModel @@ -39,6 +40,10 @@ class StudyModel(db.Model): if self.on_hold: self.protocol_builder_status = ProtocolBuilderStatus.HOLD + def files(self): + _files = FileModel.query.filter_by(workflow_id=self.workflow[0].id) + return _files + class WorkflowMetadata(object): def __init__(self, id, name, display_name, description, spec_version, category_id, state: WorkflowState, status: WorkflowStatus, @@ -154,3 +159,16 @@ class StudySchema(ma.Schema): def make_study(self, data, **kwargs): """Can load the basic study data for updates to the database, but categories are write only""" return Study(**data) + + +class StudyFilesSchema(ma.Schema): + + # files = fields.List(fields.Nested(SimpleFileSchema), dump_only=True) + files = fields.Method('_files') + + class Meta: + model = Study + additional = ["id", "title", "last_updated", "primary_investigator_id"] + + def _files(self, obj): + return [file.name for file in obj.files()] diff --git a/crc/models/workflow.py b/crc/models/workflow.py index ea072e93..e5152eac 100644 --- a/crc/models/workflow.py +++ b/crc/models/workflow.py @@ -73,10 +73,11 @@ class WorkflowModel(db.Model): bpmn_workflow_json = db.Column(db.JSON) status = db.Column(db.Enum(WorkflowStatus)) study_id = db.Column(db.Integer, db.ForeignKey('study.id')) + study = db.relationship("StudyModel", backref='workflow') workflow_spec_id = db.Column(db.String, db.ForeignKey('workflow_spec.id')) workflow_spec = db.relationship("WorkflowSpecModel") spec_version = db.Column(db.String) total_tasks = db.Column(db.Integer, default=0) completed_tasks = db.Column(db.Integer, default=0) # task_history = db.Column(db.ARRAY(db.String), default=[]) # The history stack of user completed tasks. - last_updated = db.Column(db.DateTime) \ No newline at end of file + last_updated = db.Column(db.DateTime) From 4628834106554486c1f0c447751eb46575cfb1ea Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Thu, 21 May 2020 12:11:35 -0400 Subject: [PATCH 02/16] just a few more logging details. --- crc/api/user.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/crc/api/user.py b/crc/api/user.py index 83245d19..74a11841 100644 --- a/crc/api/user.py +++ b/crc/api/user.py @@ -1,7 +1,7 @@ import json import connexion -from flask import redirect, g +from flask import redirect, g, request from crc import sso, app, db from crc.api.common import ApiError @@ -35,9 +35,12 @@ def get_current_user(): @sso.login_handler def sso_login(user_info): - app.logger.info("Login from Shibboleth happening. " + json.dump(user_info)) + redirect = request.args.get('redirect') + app.logger.info("SSO_LOGIN: Full URL: " + request.url) + app.logger.info("SSO_LOGIN: User Details: " + json.dump(user_info)) + app.logger.info("SSO_LOGIN: Will try to redirect to : " + redirect) # TODO: Get redirect URL from Shibboleth request header - _handle_login(user_info) + _handle_login(user_info, redirect) def _handle_login(user_info, redirect_url=app.config['FRONTEND_AUTH_CALLBACK']): @@ -86,8 +89,10 @@ def _handle_login(user_info, redirect_url=app.config['FRONTEND_AUTH_CALLBACK']): # Return the frontend auth callback URL, with auth token appended. auth_token = user.encode_auth_token().decode() if redirect_url is not None: + app.logger.info("SSO_LOGIN: REDIRECTING TO: " + redirect_url) return redirect('%s/%s' % (redirect_url, auth_token)) else: + app.logger.info("SSO_LOGIN: NO REDIRECT, JUST RETURNING AUTH TOKEN.") return auth_token def backdoor( From 0265db7146a923a5c7850180911ab8077b675059 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Thu, 21 May 2020 16:02:45 -0400 Subject: [PATCH 03/16] adding an /sso endpoint for testing. --- crc/api/user.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crc/api/user.py b/crc/api/user.py index 74a11841..e7688dfc 100644 --- a/crc/api/user.py +++ b/crc/api/user.py @@ -42,6 +42,10 @@ def sso_login(user_info): # TODO: Get redirect URL from Shibboleth request header _handle_login(user_info, redirect) +@app.route('/sso') +def index(): + return str(request.headers) + def _handle_login(user_info, redirect_url=app.config['FRONTEND_AUTH_CALLBACK']): """On successful login, adds user to database if the user is not already in the system, From b3ae9ee77057a10720603af82a432567a4367d6a Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Thu, 21 May 2020 16:28:34 -0400 Subject: [PATCH 04/16] changing the mapping, because 'Uid' not 'uid' --- config/default.py | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/config/default.py b/config/default.py index dd19f2ab..cb70e726 100644 --- a/config/default.py +++ b/config/default.py @@ -30,16 +30,31 @@ SWAGGER_AUTH_KEY = environ.get('SWAGGER_AUTH_KEY', default="SWAGGER") #: Default attribute map for single signon. SSO_LOGIN_URL = '/login' SSO_ATTRIBUTE_MAP = { - 'eppn': (False, 'eppn'), # dhf8r@virginia.edu - 'uid': (True, 'uid'), # dhf8r + 'Eppn': (False, 'eppn'), # dhf8r@virginia.edu + 'Uid': (True, 'uid'), # dhf8r 'givenName': (False, 'first_name'), # Daniel - 'mail': (False, 'email_address'), # dhf8r@Virginia.EDU - 'sn': (False, 'last_name'), # Funk + 'Sn': (False, 'last_name'), # Funk 'affiliation': (False, 'affiliation'), # 'staff@virginia.edu;member@virginia.edu' 'displayName': (False, 'display_name'), # Daniel Harold Funk 'title': (False, 'title') # SOFTWARE ENGINEER V } +# This what I see coming back: +# X-Remote-Cn: Daniel Harold Funk (dhf8r) +# X-Remote-Sn: Funk +# X-Remote-Givenname: Daniel +# X-Remote-Uid: dhf8r +# Eppn: dhf8r@virginia.edu +# Cn: Daniel Harold Funk (dhf8r) +# Sn: Funk +# Givenname: Daniel +# Uid: dhf8r +# X-Remote-User: dhf8r@virginia.edu +# X-Forwarded-For: 128.143.0.10 +# X-Forwarded-Host: dev.crconnect.uvadcos.io +# X-Forwarded-Server: dev.crconnect.uvadcos.io +# Connection: Keep-Alive + # %s/%i placeholders expected for uva_id and study_id in various calls. PB_BASE_URL = environ.get('PB_BASE_URL', default="http://localhost:5001/pb/") PB_USER_STUDIES_URL = environ.get('PB_USER_STUDIES_URL', default=PB_BASE_URL + "user_studies?uva_id=%s") From 4627318818cfaa4eeec10d1033d322b9095bf177 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Fri, 22 May 2020 07:55:58 -0400 Subject: [PATCH 05/16] Dropping flask_sso library in favor of reading from the headers directly. Updating login to read from ldap once it has the user_id. Adding more information to the sso endpoint. --- Pipfile | 1 - Pipfile.lock | 31 +++++++++--------------------- crc/__init__.py | 1 - crc/api/user.py | 50 +++++++++++++++++++++++++++++++++++++++++-------- 4 files changed, 51 insertions(+), 32 deletions(-) diff --git a/Pipfile b/Pipfile index 375ba042..77c70afc 100644 --- a/Pipfile +++ b/Pipfile @@ -31,7 +31,6 @@ sphinx = "*" recommonmark = "*" psycopg2-binary = "*" docxtpl = "*" -flask-sso = "*" python-dateutil = "*" pandas = "*" xlrd = "*" diff --git a/Pipfile.lock b/Pipfile.lock index 904d32bd..036d4bf9 100644 --- a/Pipfile.lock +++ b/Pipfile.lock @@ -1,11 +1,11 @@ { "_meta": { "hash": { - "sha256": "26d23456010d3e5a559386d412cef3beacd92d5a4e474f2afdb0737ea0f20f04" + "sha256": "1ca737db75750ea4351c15b4b0b26155d90bc5522705ed293a0c2773600b6a0a" }, "pipfile-spec": 6, "requires": { - "python_version": "3.7" + "python_version": "3.6.9" }, "sources": [ { @@ -96,12 +96,6 @@ ], "version": "==3.6.3.0" }, - "blinker": { - "hashes": [ - "sha256:471aee25f3992bd325afa3772f1063dbdbbca947a041b8b89466dc00d606f8b6" - ], - "version": "==1.4" - }, "celery": { "hashes": [ "sha256:108a0bf9018a871620936c33a3ee9f6336a89f8ef0a0f567a9001f4aa361415f", @@ -307,13 +301,6 @@ ], "version": "==2.4.1" }, - "flask-sso": { - "hashes": [ - "sha256:541a8a2387c6eac4325c53f8f7f863a03173b37aa558a37a430010d7fc1a3633" - ], - "index": "pypi", - "version": "==0.4.0" - }, "future": { "hashes": [ "sha256:b1bead90b70cf6ec3f0710ae53a525360fa360d306a86583adc6bf83a4db537d" @@ -711,10 +698,10 @@ }, "six": { "hashes": [ - "sha256:236bdbdce46e6e6a3d61a337c0f8b763ca1e8717c03b369e87a7ec7ce1319c0a", - "sha256:8f3cd2e254d8f793e7f3d6d9df77b92252b52637291d0f0da013c76ea2724b6c" + "sha256:30639c035cdb23534cd4aa2dd52c3bf48f06e5f4a941509c8bafd8ce11080259", + "sha256:8b74bedcbbbaca38ff6d7491d76f2b06b3592611af620f8426e82dddb04a5ced" ], - "version": "==1.14.0" + "version": "==1.15.0" }, "snowballstemmer": { "hashes": [ @@ -783,7 +770,7 @@ "spiffworkflow": { "editable": true, "git": "https://github.com/sartography/SpiffWorkflow.git", - "ref": "2c9698894f7e526a91bf3ca8c4b9fc9b6b01e807" + "ref": "cb098ee6d55b85bf7795997f4ad5f78c27d15381" }, "sqlalchemy": { "hashes": [ @@ -955,10 +942,10 @@ }, "six": { "hashes": [ - "sha256:236bdbdce46e6e6a3d61a337c0f8b763ca1e8717c03b369e87a7ec7ce1319c0a", - "sha256:8f3cd2e254d8f793e7f3d6d9df77b92252b52637291d0f0da013c76ea2724b6c" + "sha256:30639c035cdb23534cd4aa2dd52c3bf48f06e5f4a941509c8bafd8ce11080259", + "sha256:8b74bedcbbbaca38ff6d7491d76f2b06b3592611af620f8426e82dddb04a5ced" ], - "version": "==1.14.0" + "version": "==1.15.0" }, "wcwidth": { "hashes": [ diff --git a/crc/__init__.py b/crc/__init__.py index aa301108..7cdc4ead 100644 --- a/crc/__init__.py +++ b/crc/__init__.py @@ -31,7 +31,6 @@ session = db.session migrate = Migrate(app, db) ma = Marshmallow(app) -sso = SSO(app=app) from crc import models from crc import api diff --git a/crc/api/user.py b/crc/api/user.py index e7688dfc..5c9bc108 100644 --- a/crc/api/user.py +++ b/crc/api/user.py @@ -3,10 +3,10 @@ import json import connexion from flask import redirect, g, request -from crc import sso, app, db +from crc import app, db from crc.api.common import ApiError from crc.models.user import UserModel, UserModelSchema - +from crc.services.ldap_service import LdapService """ .. module:: crc.api.user @@ -32,21 +32,55 @@ def verify_token(token): def get_current_user(): return UserModelSchema().dump(g.user) +def sso_login(): + # This what I see coming back: + # X-Remote-Cn: Daniel Harold Funk (dhf8r) + # X-Remote-Sn: Funk + # X-Remote-Givenname: Daniel + # X-Remote-Uid: dhf8r + # Eppn: dhf8r@virginia.edu + # Cn: Daniel Harold Funk (dhf8r) + # Sn: Funk + # Givenname: Daniel + # Uid: dhf8r + # X-Remote-User: dhf8r@virginia.edu + # X-Forwarded-For: 128.143.0.10 + # X-Forwarded-Host: dev.crconnect.uvadcos.io + # X-Forwarded-Server: dev.crconnect.uvadcos.io + # Connection: Keep-Alive + uid = request.headers.get("Uid") + if not uid: + uid = request.headers.get("X-Remote-Uid") + + if not uid: + raise ApiError("invalid_sso_credentials", "'Uid' nor 'X-Remote-Uid' were present in the headers: %s" + % str(request.headers)) -@sso.login_handler -def sso_login(user_info): redirect = request.args.get('redirect') app.logger.info("SSO_LOGIN: Full URL: " + request.url) - app.logger.info("SSO_LOGIN: User Details: " + json.dump(user_info)) + app.logger.info("SSO_LOGIN: User Id: " + uid) app.logger.info("SSO_LOGIN: Will try to redirect to : " + redirect) + + ldap_service = LdapService() + info = ldap_service.user_info(uid) + + user = UserModel(uid=uid, email_address=info.email, display_name=info.display_name, + affiliation=info.affiliation, title=info.title) + # TODO: Get redirect URL from Shibboleth request header - _handle_login(user_info, redirect) + _handle_login(user, redirect) @app.route('/sso') -def index(): - return str(request.headers) +def sso(): + response = "" + response += "

Headers

" + response += str(request.headers) + response += "

Environment

" + response += str(request.environ) + return response +@app.route('/login') def _handle_login(user_info, redirect_url=app.config['FRONTEND_AUTH_CALLBACK']): """On successful login, adds user to database if the user is not already in the system, then returns the frontend auth callback URL, with auth token appended. From 1cc9a60bfe9ddef1509c9d24108d1d73d416b37d Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Fri, 22 May 2020 08:01:33 -0400 Subject: [PATCH 06/16] clearing out the remaining references to the flask_sso library. --- crc/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/crc/__init__.py b/crc/__init__.py index 7cdc4ead..9f1c4ee3 100644 --- a/crc/__init__.py +++ b/crc/__init__.py @@ -6,7 +6,6 @@ from flask_cors import CORS from flask_marshmallow import Marshmallow from flask_migrate import Migrate from flask_sqlalchemy import SQLAlchemy -from flask_sso import SSO logging.basicConfig(level=logging.INFO) From b490005af78e7c5fc31bf65e0552e1f015a1e6dd Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Fri, 22 May 2020 09:50:18 -0400 Subject: [PATCH 07/16] dropping the remaining config stuff for flask_sso. updaing the user 'sso' endpoint to provide additional information for debugging. Pulling information from ldap to stay super consistent on where we get our information. --- config/default.py | 28 --------------- crc/api/user.py | 68 +++++++++++++----------------------- crc/services/ldap_service.py | 37 +++++++++++++------- tests/test_authentication.py | 19 +++++++++- tests/test_ldap_service.py | 2 +- 5 files changed, 69 insertions(+), 85 deletions(-) diff --git a/config/default.py b/config/default.py index cb70e726..d2486f86 100644 --- a/config/default.py +++ b/config/default.py @@ -27,34 +27,6 @@ TOKEN_AUTH_SECRET_KEY = environ.get('TOKEN_AUTH_SECRET_KEY', default="Shhhh!!! T FRONTEND_AUTH_CALLBACK = environ.get('FRONTEND_AUTH_CALLBACK', default="http://localhost:4200/session") SWAGGER_AUTH_KEY = environ.get('SWAGGER_AUTH_KEY', default="SWAGGER") -#: Default attribute map for single signon. -SSO_LOGIN_URL = '/login' -SSO_ATTRIBUTE_MAP = { - 'Eppn': (False, 'eppn'), # dhf8r@virginia.edu - 'Uid': (True, 'uid'), # dhf8r - 'givenName': (False, 'first_name'), # Daniel - 'Sn': (False, 'last_name'), # Funk - 'affiliation': (False, 'affiliation'), # 'staff@virginia.edu;member@virginia.edu' - 'displayName': (False, 'display_name'), # Daniel Harold Funk - 'title': (False, 'title') # SOFTWARE ENGINEER V -} - -# This what I see coming back: -# X-Remote-Cn: Daniel Harold Funk (dhf8r) -# X-Remote-Sn: Funk -# X-Remote-Givenname: Daniel -# X-Remote-Uid: dhf8r -# Eppn: dhf8r@virginia.edu -# Cn: Daniel Harold Funk (dhf8r) -# Sn: Funk -# Givenname: Daniel -# Uid: dhf8r -# X-Remote-User: dhf8r@virginia.edu -# X-Forwarded-For: 128.143.0.10 -# X-Forwarded-Host: dev.crconnect.uvadcos.io -# X-Forwarded-Server: dev.crconnect.uvadcos.io -# Connection: Keep-Alive - # %s/%i placeholders expected for uva_id and study_id in various calls. PB_BASE_URL = environ.get('PB_BASE_URL', default="http://localhost:5001/pb/") PB_USER_STUDIES_URL = environ.get('PB_USER_STUDIES_URL', default=PB_BASE_URL + "user_studies?uva_id=%s") diff --git a/crc/api/user.py b/crc/api/user.py index 5c9bc108..6924eb27 100644 --- a/crc/api/user.py +++ b/crc/api/user.py @@ -6,7 +6,7 @@ from flask import redirect, g, request from crc import app, db from crc.api.common import ApiError from crc.models.user import UserModel, UserModelSchema -from crc.services.ldap_service import LdapService +from crc.services.ldap_service import LdapService, LdapUserInfo """ .. module:: crc.api.user @@ -32,6 +32,7 @@ def verify_token(token): def get_current_user(): return UserModelSchema().dump(g.user) +@app.route('/login') def sso_login(): # This what I see coming back: # X-Remote-Cn: Daniel Harold Funk (dhf8r) @@ -59,67 +60,48 @@ def sso_login(): redirect = request.args.get('redirect') 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 : " + redirect) + app.logger.info("SSO_LOGIN: Will try to redirect to : " + str(redirect)) ldap_service = LdapService() info = ldap_service.user_info(uid) - user = UserModel(uid=uid, email_address=info.email, display_name=info.display_name, - affiliation=info.affiliation, title=info.title) - - # TODO: Get redirect URL from Shibboleth request header - _handle_login(user, redirect) + return _handle_login(info, redirect) @app.route('/sso') def sso(): response = "" response += "

Headers

" - response += str(request.headers) + response += "
    " + for k,v in request.headers: + response += "
  • %s %s
    • \n" % (k, v) response += "

    Environment

    " - response += str(request.environ) + for k,v in request.environ: + response += "
  • %s %s
    • \n" % (k, v) return response -@app.route('/login') -def _handle_login(user_info, redirect_url=app.config['FRONTEND_AUTH_CALLBACK']): +def _handle_login(user_info: LdapUserInfo, redirect_url=app.config['FRONTEND_AUTH_CALLBACK']): """On successful login, adds user to database if the user is not already in the system, then returns the frontend auth callback URL, with auth token appended. Args: - user_info (dict of { - uid: str, - affiliation: Optional[str], - display_name: Optional[str], - email_address: Optional[str], - eppn: Optional[str], - first_name: Optional[str], - last_name: Optional[str], - title: Optional[str], - }): Dictionary of user attributes - redirect_url: Optional[str] + user_info - an ldap user_info object. + redirect_url: Optional[str] Returns: Response. 302 - Redirects to the frontend auth callback URL, with auth token appended. """ - uid = user_info['uid'] - user = db.session.query(UserModel).filter(UserModel.uid == uid).first() + user = db.session.query(UserModel).filter(UserModel.uid == user_info.uid).first() if user is None: # Add new user - user = UserModelSchema().load(user_info, session=db.session) - else: - # Update existing user data - user = UserModelSchema().load(user_info, session=db.session, instance=user, partial=True) + user = UserModel() - # Build display_name if not set - if 'display_name' not in user_info or len(user_info['display_name']) == 0: - display_name_list = [] - - for prop in ['first_name', 'last_name']: - if prop in user_info and len(user_info[prop]) > 0: - display_name_list.append(user_info[prop]) - - user.display_name = ' '.join(display_name_list) + user.uid = user_info.uid + user.display_name = user_info.display_name + user.email_address = user_info.email_address + user.affiliation = user_info.affiliation + user.title = user_info.title db.session.add(user) db.session.commit() @@ -133,6 +115,8 @@ def _handle_login(user_info, redirect_url=app.config['FRONTEND_AUTH_CALLBACK']): app.logger.info("SSO_LOGIN: NO REDIRECT, JUST RETURNING AUTH TOKEN.") return auth_token + + def backdoor( uid=None, affiliation=None, @@ -165,11 +149,9 @@ def backdoor( ApiError. If on production, returns a 404 error. """ if not 'PRODUCTION' in app.config or not app.config['PRODUCTION']: - user_info = {} - for key in UserModel.__dict__.keys(): - if key in connexion.request.args: - user_info[key] = connexion.request.args[key] - - return _handle_login(user_info, redirect_url) + ldap_info = LdapUserInfo() + ldap_info.uid = connexion.request.args["uid"] + ldap_info.email_address = connexion.request.args["email_address"] + return _handle_login(ldap_info, redirect_url) else: raise ApiError('404', 'unknown') diff --git a/crc/services/ldap_service.py b/crc/services/ldap_service.py index fd76ad78..d0bb3f7b 100644 --- a/crc/services/ldap_service.py +++ b/crc/services/ldap_service.py @@ -8,17 +8,30 @@ from crc.api.common import ApiError class LdapUserInfo(object): - def __init__(self, entry): - self.display_name = entry.displayName.value - self.given_name = ", ".join(entry.givenName) - self.email = entry.mail.value - self.telephone_number = ", ".join(entry.telephoneNumber) - self.title = ", ".join(entry.title) - self.department = ", ".join(entry.uvaDisplayDepartment) - self.affiliation = ", ".join(entry.uvaPersonIAMAffiliation) - self.sponsor_type = ", ".join(entry.uvaPersonSponsoredType) - self.uid = entry.uid.value + def __init__(self): + self.display_name = '' + self.given_name = '' + self.email_address = '' + self.telephone_number = '' + self.title = '' + self.department = '' + self.affiliation = '' + self.sponsor_type = '' + self.uid = '' + @classmethod + def from_entry(cls, entry): + instance = cls() + instance.display_name = entry.displayName.value + instance.given_name = ", ".join(entry.givenName) + instance.email_address = entry.mail.value + instance.telephone_number = ", ".join(entry.telephoneNumber) + instance.title = ", ".join(entry.title) + instance.department = ", ".join(entry.uvaDisplayDepartment) + instance.affiliation = ", ".join(entry.uvaPersonIAMAffiliation) + instance.sponsor_type = ", ".join(entry.uvaPersonSponsoredType) + instance.uid = entry.uid.value + return instance class LdapService(object): search_base = "ou=People,o=University of Virginia,c=US" @@ -50,7 +63,7 @@ class LdapService(object): if len(self.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] - return(LdapUserInfo(entry)) + return LdapUserInfo.from_entry(entry) def search_users(self, query, limit): search_string = LdapService.uid_search_string % query @@ -64,6 +77,6 @@ class LdapService(object): for entry in self.conn.entries: if count > limit: break - results.append(LdapUserInfo(entry)) + results.append(LdapUserInfo.from_entry(entry)) count += 1 return results diff --git a/tests/test_authentication.py b/tests/test_authentication.py index 5a19fb32..2767797a 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -12,7 +12,7 @@ class TestAuthentication(BaseTest): self.assertTrue(isinstance(auth_token, bytes)) self.assertEqual("dhf8r", user.decode_auth_token(auth_token).get("sub")) - def test_auth_creates_user(self): + def test_backdoor_auth_creates_user(self): new_uid = 'czn1z'; self.load_example_data() user = db.session.query(UserModel).filter(UserModel.uid == new_uid).first() @@ -37,6 +37,23 @@ class TestAuthentication(BaseTest): self.assertTrue(rv_2.status_code == 302) self.assertTrue(str.startswith(rv_2.location, redirect_url)) + def test_normal_auth_creates_user(self): + new_uid = 'lb3dp' # This user is in the test ldap system. + self.load_example_data() + user = db.session.query(UserModel).filter(UserModel.uid == new_uid).first() + self.assertIsNone(user) + redirect_url = 'http://worlds.best.website/admin' + headers = dict(Uid=new_uid) + rv = self.app.get('login', follow_redirects=False, headers=headers) + self.assert_success(rv) + user = db.session.query(UserModel).filter(UserModel.uid == new_uid).first() + self.assertIsNotNone(user) + self.assertEquals(new_uid, user.uid) + self.assertEquals("Laura Barnes", user.display_name) + self.assertEquals("lb3dp@virginia.edu", user.email_address) + self.assertEquals("E0:Associate Professor of Systems and Information Engineering", user.title) + + def test_current_user_status(self): self.load_example_data() rv = self.app.get('/v1.0/user') diff --git a/tests/test_ldap_service.py b/tests/test_ldap_service.py index 8bd80107..4be65960 100644 --- a/tests/test_ldap_service.py +++ b/tests/test_ldap_service.py @@ -21,7 +21,7 @@ class TestLdapService(BaseTest): self.assertEqual("lb3dp", user_info.uid) self.assertEqual("Laura Barnes", user_info.display_name) self.assertEqual("Laura", user_info.given_name) - self.assertEqual("lb3dp@virginia.edu", user_info.email) + self.assertEqual("lb3dp@virginia.edu", user_info.email_address) self.assertEqual("+1 (434) 924-1723", user_info.telephone_number) self.assertEqual("E0:Associate Professor of Systems and Information Engineering", user_info.title) self.assertEqual("E0:EN-Eng Sys and Environment", user_info.department) From 1ed7930aabba5f8a06cc5204ece244f03a36329d Mon Sep 17 00:00:00 2001 From: Carlos Lopez Date: Fri, 22 May 2020 09:46:03 -0600 Subject: [PATCH 08/16] Endpoint for studies with files --- crc/api.yml | 13 +++++++++++++ crc/api/study.py | 9 ++++++++- crc/services/study_service.py | 6 ++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/crc/api.yml b/crc/api.yml index 52abbde4..35c12862 100644 --- a/crc/api.yml +++ b/crc/api.yml @@ -110,6 +110,19 @@ paths: application/json: schema: $ref: "#/components/schemas/Study" + /study-files: + get: + operationId: crc.api.study.all_studies_and_files + summary: Provides a list of studies with submitted files + tags: + - Studies + responses: + '200': + description: An array of studies, with submitted files, ordered by the last modified date. + content: + application/json: + schema: + $ref: "#/components/schemas/Study" /study/{study_id}: parameters: - name: study_id diff --git a/crc/api/study.py b/crc/api/study.py index c162e15a..88a68b58 100644 --- a/crc/api/study.py +++ b/crc/api/study.py @@ -7,7 +7,7 @@ from sqlalchemy.exc import IntegrityError from crc import session from crc.api.common import ApiError, ApiErrorSchema from crc.models.protocol_builder import ProtocolBuilderStatus, ProtocolBuilderStudy -from crc.models.study import StudySchema, StudyModel, Study +from crc.models.study import StudySchema, StudyFilesSchema, StudyModel, Study from crc.services.protocol_builder import ProtocolBuilderService from crc.services.study_service import StudyService @@ -67,6 +67,13 @@ def all_studies(): return results +def all_studies_and_files(): + """Returns all studies with submitted files""" + studies = StudyService.get_studies_with_files() + results = StudyFilesSchema(many=True).dump(studies) + return results + + def post_update_study_from_protocol_builder(study_id): """Update a single study based on data received from the protocol builder.""" diff --git a/crc/services/study_service.py b/crc/services/study_service.py index 6d1f4bc5..8e1fd9c6 100644 --- a/crc/services/study_service.py +++ b/crc/services/study_service.py @@ -32,6 +32,12 @@ class StudyService(object): studies.append(StudyService.get_study(study_model.id, study_model)) return studies + @staticmethod + def get_studies_with_files(): + """Returns a list of all studies""" + db_studies = session.query(StudyModel).all() + return db_studies + @staticmethod def get_study(study_id, study_model: StudyModel = None): """Returns a study model that contains all the workflows organized by category. From 992a85e9a550f56d58917c61d1c14a614f8d9f3e Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Fri, 22 May 2020 11:56:43 -0400 Subject: [PATCH 09/16] Rough idea of what the Approvals model will look like. --- crc/models/user.py | 8 ++++++++ crc/models/workflow.py | 3 ++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/crc/models/user.py b/crc/models/user.py index 9080f42d..dc7f4e03 100644 --- a/crc/models/user.py +++ b/crc/models/user.py @@ -7,6 +7,12 @@ from crc import db, app from crc.api.common import ApiError +class Approvals(db.Model): + approval_uid = db.Column(db.String, unique=True) + workflow_id = db.Column(db.Integer, primary_key=True) + workflow_version = db.Column(db.String) + is_approved = db.Column(db.Boolean) + class UserModel(db.Model): __tablename__ = 'user' id = db.Column(db.Integer, primary_key=True) @@ -19,6 +25,8 @@ class UserModel(db.Model): last_name = db.Column(db.String, nullable=True) title = db.Column(db.String, nullable=True) + # Add Department and School + def encode_auth_token(self): """ diff --git a/crc/models/workflow.py b/crc/models/workflow.py index e5152eac..7d690a4b 100644 --- a/crc/models/workflow.py +++ b/crc/models/workflow.py @@ -79,5 +79,6 @@ class WorkflowModel(db.Model): spec_version = db.Column(db.String) total_tasks = db.Column(db.Integer, default=0) completed_tasks = db.Column(db.Integer, default=0) -# task_history = db.Column(db.ARRAY(db.String), default=[]) # The history stack of user completed tasks. last_updated = db.Column(db.DateTime) + # todo: Add a version that represents the files associated with this workflow + # version = "32" \ No newline at end of file From 503c1c8f18be27943a58572b60162276730bf625 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Fri, 22 May 2020 14:37:49 -0400 Subject: [PATCH 10/16] Allow disabling the Protocol Builder PB_ENABLED can be set to false in the configuration (either in a file called instance/config.py, or as an environment variable) Added a check in the base_test, to assure that we are always running tests with the test configuration, and bail out otherwise. Setting TESTING=true as an environment variable will get this, but so well the correct ordering of imports. Just be dead certain the first file every test file imports is base_test.py. Aaron was right, and we call the Protocol Builder in all kinds of awful places. But we don't do this now. So Carlos, you should have the ability to reuse a lot of the logic in the study_service now. I dropped the poorly named "study-update" endpoint completely. We weren't using it. POST and PUT to Study still work just fine for doing exactly that. All the tests now run and pass with the Protocol builder disabled. Tests that specifically check PB behavior turn it back on for the test, or mock it out. --- config/default.py | 1 + config/testing.py | 1 + crc/api.yml | 20 --------- crc/api/study.py | 32 ++++--------- crc/services/protocol_builder.py | 52 ++++++++++------------ crc/services/study_service.py | 33 +++++++++----- tests/base_test.py | 12 ++--- tests/test_protocol_builder.py | 6 ++- tests/test_study_api.py | 27 +++++------ tests/test_tasks_api.py | 4 ++ tests/test_workflow_spec_validation_api.py | 16 +------ 11 files changed, 81 insertions(+), 123 deletions(-) diff --git a/config/default.py b/config/default.py index d2486f86..f31f2889 100644 --- a/config/default.py +++ b/config/default.py @@ -28,6 +28,7 @@ FRONTEND_AUTH_CALLBACK = environ.get('FRONTEND_AUTH_CALLBACK', default="http://l SWAGGER_AUTH_KEY = environ.get('SWAGGER_AUTH_KEY', default="SWAGGER") # %s/%i placeholders expected for uva_id and study_id in various calls. +PB_ENABLED = environ.get('PB_ENABLED', default=True) PB_BASE_URL = environ.get('PB_BASE_URL', default="http://localhost:5001/pb/") PB_USER_STUDIES_URL = environ.get('PB_USER_STUDIES_URL', default=PB_BASE_URL + "user_studies?uva_id=%s") PB_INVESTIGATORS_URL = environ.get('PB_INVESTIGATORS_URL', default=PB_BASE_URL + "investigators?studyid=%i") diff --git a/config/testing.py b/config/testing.py index 823a0178..f481224a 100644 --- a/config/testing.py +++ b/config/testing.py @@ -6,6 +6,7 @@ DEVELOPMENT = True TESTING = True SQLALCHEMY_DATABASE_URI = "postgresql://crc_user:crc_pass@localhost:5432/crc_test" TOKEN_AUTH_SECRET_KEY = "Shhhh!!! This is secret! And better darn well not show up in prod." +PB_ENABLED = False print('### USING TESTING CONFIG: ###') print('SQLALCHEMY_DATABASE_URI = ', SQLALCHEMY_DATABASE_URI) diff --git a/crc/api.yml b/crc/api.yml index 52abbde4..56fdfbdb 100644 --- a/crc/api.yml +++ b/crc/api.yml @@ -156,26 +156,6 @@ paths: application/json: schema: $ref: "#/components/schemas/Study" - /study-update/{study_id}: - post: - operationId: crc.api.study.post_update_study_from_protocol_builder - summary: If the study is up-to-date with Protocol Builder, returns a 304 Not Modified. If out of date, return a 202 Accepted and study state changes to updating. - tags: - - Study Status - parameters: - - name: study_id - in: path - required: true - description: The id of the study that should be checked for updates. - schema: - type: integer - format: int32 - responses: - '304': - description: Study is currently up to date and does not need to be reloaded from Protocol Builder - '202': - description: Request accepted, will preform an update. Study state set to "updating" - /workflow-specification: get: operationId: crc.api.workflow.all_specifications diff --git a/crc/api/study.py b/crc/api/study.py index c162e15a..d56b20c2 100644 --- a/crc/api/study.py +++ b/crc/api/study.py @@ -1,20 +1,14 @@ -from typing import List - -from connexion import NoContent from flask import g from sqlalchemy.exc import IntegrityError from crc import session from crc.api.common import ApiError, ApiErrorSchema -from crc.models.protocol_builder import ProtocolBuilderStatus, ProtocolBuilderStudy from crc.models.study import StudySchema, StudyModel, Study -from crc.services.protocol_builder import ProtocolBuilderService from crc.services.study_service import StudyService def add_study(body): - """This should never get called, and is subject to deprication. Studies - should be added through the protocol builder only.""" + """Or any study like object. """ study: Study = StudySchema().load(body) study_model = StudyModel(**study.model_args()) session.add(study_model) @@ -59,28 +53,18 @@ def delete_study(study_id): def all_studies(): - """Returns all the studies associated with the current user. Assures we are - in sync with values read in from the protocol builder. """ - StudyService.synch_all_studies_with_protocol_builder(g.user) + """Returns all the studies associated with the current user. """ + StudyService.synch_with_protocol_builder_if_enabled(g.user) studies = StudyService.get_studies_for_user(g.user) results = StudySchema(many=True).dump(studies) return results -def post_update_study_from_protocol_builder(study_id): - """Update a single study based on data received from - the protocol builder.""" - - db_study = session.query(StudyModel).filter_by(study_id=study_id).all() - pb_studies: List[ProtocolBuilderStudy] = ProtocolBuilderService.get_studies(g.user.uid) - pb_study = next((pbs for pbs in pb_studies if pbs.STUDYID == study_id), None) - if pb_study: - db_study.update_from_protocol_builder(pb_study) - else: - db_study.inactive = True - db_study.protocol_builder_status = ProtocolBuilderStatus.ABANDONED - - return NoContent, 304 +def all_studies_and_files(): + """Returns all studies with submitted files""" + studies = StudyService.get_studies_for_user(g.user) + results = StudySchema(many=True).dump(studies) + return results diff --git a/crc/services/protocol_builder.py b/crc/services/protocol_builder.py index 118d871a..efe4bd72 100644 --- a/crc/services/protocol_builder.py +++ b/crc/services/protocol_builder.py @@ -10,6 +10,7 @@ from crc.models.protocol_builder import ProtocolBuilderStudy, ProtocolBuilderStu class ProtocolBuilderService(object): + ENABLED = app.config['PB_ENABLED'] STUDY_URL = app.config['PB_USER_STUDIES_URL'] INVESTIGATOR_URL = app.config['PB_INVESTIGATORS_URL'] REQUIRED_DOCS_URL = app.config['PB_REQUIRED_DOCS_URL'] @@ -17,6 +18,7 @@ class ProtocolBuilderService(object): @staticmethod 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)) response = requests.get(ProtocolBuilderService.STUDY_URL % user_id) @@ -30,40 +32,32 @@ class ProtocolBuilderService(object): @staticmethod def get_investigators(study_id) -> {}: - ProtocolBuilderService.check_args(study_id) - response = requests.get(ProtocolBuilderService.INVESTIGATOR_URL % study_id) - if response.ok and response.text: - pb_studies = json.loads(response.text) - return pb_studies - else: - raise ApiError("protocol_builder_error", - "Received an invalid response from the protocol builder (status %s): %s" % - (response.status_code, response.text)) + return ProtocolBuilderService.__make_request(study_id, ProtocolBuilderService.INVESTIGATOR_URL) @staticmethod def get_required_docs(study_id) -> Optional[List[ProtocolBuilderRequiredDocument]]: - ProtocolBuilderService.check_args(study_id) - response = requests.get(ProtocolBuilderService.REQUIRED_DOCS_URL % study_id) + return ProtocolBuilderService.__make_request(study_id, ProtocolBuilderService.REQUIRED_DOCS_URL) + + @staticmethod + def get_study_details(study_id) -> {}: + return ProtocolBuilderService.__make_request(study_id, ProtocolBuilderService.STUDY_DETAILS_URL) + + @staticmethod + def __enabled_or_raise(): + if not ProtocolBuilderService.ENABLED: + raise ApiError("protocol_builder_disabled", "The Protocol Builder Service is currently disabled.") + + @staticmethod + def __make_request(study_id, url): + ProtocolBuilderService.__enabled_or_raise() + if not isinstance(study_id, int): + raise ApiError("invalid_study_id", "This study id is invalid: " + str(study_id)) + response = requests.get(url % study_id) if response.ok and response.text: return json.loads(response.text) else: raise ApiError("protocol_builder_error", - "Received an invalid response from the protocol builder (status %s): %s" % - (response.status_code, response.text)) + "Received an invalid response from the protocol builder (status %s): %s when calling " + "url '%s'." % + (response.status_code, response.text, url)) - @staticmethod - def get_study_details(study_id) -> {}: - ProtocolBuilderService.check_args(study_id) - response = requests.get(ProtocolBuilderService.STUDY_DETAILS_URL % study_id) - if response.ok and response.text: - pb_study_details = json.loads(response.text) - return pb_study_details - else: - raise ApiError("protocol_builder_error", - "Received an invalid response from the protocol builder (status %s): %s" % - (response.status_code, response.text)) - - @staticmethod - def check_args(study_id): - if not isinstance(study_id, int): - raise ApiError("invalid_study_id", "This study id is invalid: " + str(study_id)) diff --git a/crc/services/study_service.py b/crc/services/study_service.py index 6d1f4bc5..75fa5f76 100644 --- a/crc/services/study_service.py +++ b/crc/services/study_service.py @@ -110,23 +110,29 @@ class StudyService(object): """Returns a list of documents related to the study, and any file information that is available..""" - # Get PB required docs - try: - pb_docs = ProtocolBuilderService.get_required_docs(study_id=study_id) - except requests.exceptions.ConnectionError as ce: - app.logger.error("Failed to connect to the Protocol Builder - %s" % str(ce)) + # Get PB required docs, if Protocol Builder Service is enabled. + if ProtocolBuilderService.ENABLED: + try: + pb_docs = ProtocolBuilderService.get_required_docs(study_id=study_id) + except requests.exceptions.ConnectionError as ce: + app.logger.error("Failed to connect to the Protocol Builder - %s" % str(ce)) + pb_docs = [] + else: pb_docs = [] - # Loop through all known document types, get the counts for those files, and use pb_docs to mark those required. + # Loop through all known document types, get the counts for those files, + # and use pb_docs to mark those as required. doc_dictionary = FileService.get_reference_data(FileService.DOCUMENT_LIST, 'code', ['id']) documents = {} for code, doc in doc_dictionary.items(): - pb_data = next((item for item in pb_docs if int(item['AUXDOCID']) == int(doc['id'])), None) - doc['required'] = False - if pb_data: - doc['required'] = True + if ProtocolBuilderService.ENABLED: + pb_data = next((item for item in pb_docs if int(item['AUXDOCID']) == int(doc['id'])), None) + doc['required'] = False + if pb_data: + doc['required'] = True + doc['study_id'] = study_id doc['code'] = code @@ -153,7 +159,6 @@ class StudyService(object): doc['status'] = workflow.status.value documents[code] = doc - return documents @@ -201,9 +206,13 @@ class StudyService(object): @staticmethod - def synch_all_studies_with_protocol_builder(user): + def synch_with_protocol_builder_if_enabled(user): """Assures that the studies we have locally for the given user are in sync with the studies available in protocol builder. """ + + if not ProtocolBuilderService.ENABLED: + return + # Get studies matching this user from Protocol Builder pb_studies: List[ProtocolBuilderStudy] = ProtocolBuilderService.get_studies(user.uid) diff --git a/tests/base_test.py b/tests/base_test.py index 290b1506..6d0acfee 100644 --- a/tests/base_test.py +++ b/tests/base_test.py @@ -1,7 +1,9 @@ # Set environment variable to testing before loading. # IMPORTANT - Environment must be loaded before app, models, etc.... -import json import os +os.environ["TESTING"] = "true" + +import json import unittest import urllib.parse import datetime @@ -10,10 +12,6 @@ from crc.models.protocol_builder import ProtocolBuilderStatus from crc.models.study import StudyModel from crc.services.file_service import FileService from crc.services.study_service import StudyService -from crc.services.workflow_processor import WorkflowProcessor - -os.environ["TESTING"] = "true" - from crc.models.file import FileModel, FileDataModel, CONTENT_TYPES from crc.models.workflow import WorkflowSpecModel, WorkflowSpecModelSchema, WorkflowModel from crc.models.user import UserModel @@ -32,6 +30,10 @@ class BaseTest(unittest.TestCase): efficiently when we have a database in place. """ + if not app.config['TESTING']: + raise ("INVALID TEST CONFIGURATION. This is almost always in import order issue." + "The first class to import in each test should be the base_test.py file.") + auths = {} test_uid = "dhf8r" diff --git a/tests/test_protocol_builder.py b/tests/test_protocol_builder.py index 20ce567e..a386a218 100644 --- a/tests/test_protocol_builder.py +++ b/tests/test_protocol_builder.py @@ -1,7 +1,7 @@ from unittest.mock import patch -from crc.services.protocol_builder import ProtocolBuilderService from tests.base_test import BaseTest +from crc.services.protocol_builder import ProtocolBuilderService class TestProtocolBuilder(BaseTest): @@ -10,6 +10,7 @@ class TestProtocolBuilder(BaseTest): @patch('crc.services.protocol_builder.requests.get') def test_get_studies(self, mock_get): + ProtocolBuilderService.ENABLED = True mock_get.return_value.ok = True mock_get.return_value.text = self.protocol_builder_response('user_studies.json') response = ProtocolBuilderService.get_studies(self.test_uid) @@ -17,6 +18,7 @@ class TestProtocolBuilder(BaseTest): @patch('crc.services.protocol_builder.requests.get') def test_get_investigators(self, mock_get): + ProtocolBuilderService.ENABLED = True mock_get.return_value.ok = True mock_get.return_value.text = self.protocol_builder_response('investigators.json') response = ProtocolBuilderService.get_investigators(self.test_study_id) @@ -28,6 +30,7 @@ class TestProtocolBuilder(BaseTest): @patch('crc.services.protocol_builder.requests.get') def test_get_required_docs(self, mock_get): + ProtocolBuilderService.ENABLED = True mock_get.return_value.ok = True mock_get.return_value.text = self.protocol_builder_response('required_docs.json') response = ProtocolBuilderService.get_required_docs(self.test_study_id) @@ -37,6 +40,7 @@ class TestProtocolBuilder(BaseTest): @patch('crc.services.protocol_builder.requests.get') def test_get_details(self, mock_get): + ProtocolBuilderService.ENABLED = True mock_get.return_value.ok = True mock_get.return_value.text = self.protocol_builder_response('study_details.json') response = ProtocolBuilderService.get_study_details(self.test_study_id) diff --git a/tests/test_study_api.py b/tests/test_study_api.py index 3d081da9..ce1cbf64 100644 --- a/tests/test_study_api.py +++ b/tests/test_study_api.py @@ -1,4 +1,5 @@ import json +from tests.base_test import BaseTest from datetime import datetime, timezone from unittest.mock import patch @@ -8,7 +9,7 @@ from crc.models.protocol_builder import ProtocolBuilderStatus, \ from crc.models.stats import TaskEventModel from crc.models.study import StudyModel, StudySchema from crc.models.workflow import WorkflowSpecModel, WorkflowModel, WorkflowSpecCategoryModel -from tests.base_test import BaseTest +from crc.services.protocol_builder import ProtocolBuilderService class TestStudyApi(BaseTest): @@ -38,24 +39,12 @@ class TestStudyApi(BaseTest): study = session.query(StudyModel).first() self.assertIsNotNone(study) - @patch('crc.services.protocol_builder.ProtocolBuilderService.get_investigators') # mock_studies - @patch('crc.services.protocol_builder.ProtocolBuilderService.get_required_docs') # mock_docs - @patch('crc.services.protocol_builder.ProtocolBuilderService.get_study_details') # mock_details - @patch('crc.services.protocol_builder.ProtocolBuilderService.get_studies') # mock_studies - def test_get_study(self, mock_studies, mock_details, mock_docs, mock_investigators): + def test_get_study(self): """Generic test, but pretty detailed, in that the study should return a categorized list of workflows This starts with out loading the example data, to show that all the bases are covered from ground 0.""" - # Mock Protocol Builder responses - studies_response = self.protocol_builder_response('user_studies.json') - mock_studies.return_value = ProtocolBuilderStudySchema(many=True).loads(studies_response) - details_response = self.protocol_builder_response('study_details.json') - mock_details.return_value = json.loads(details_response) - docs_response = self.protocol_builder_response('required_docs.json') - mock_docs.return_value = json.loads(docs_response) - investigators_response = self.protocol_builder_response('investigators.json') - mock_investigators.return_value = json.loads(investigators_response) - + """NOTE: The protocol builder is not enabled or mocked out. As the master workflow (which is empty), + and the test workflow do not need it, and it is disabled in the configuration.""" new_study = self.add_test_study() new_study = session.query(StudyModel).filter_by(id=new_study["id"]).first() # Add a category @@ -65,7 +54,7 @@ class TestStudyApi(BaseTest): # Create a workflow specification self.create_workflow("random_fact", study=new_study, category_id=new_category.id) # Assure there is a master specification, and it has the lookup files it needs. - spec = self.load_test_spec("top_level_workflow", master_spec=True) + spec = self.load_test_spec("empty_workflow", master_spec=True) self.create_reference_document() api_response = self.app.get('/v1.0/study/%i' % new_study.id, @@ -126,6 +115,9 @@ class TestStudyApi(BaseTest): @patch('crc.services.protocol_builder.ProtocolBuilderService.get_study_details') # mock_details @patch('crc.services.protocol_builder.ProtocolBuilderService.get_studies') # mock_studies def test_get_all_studies(self, mock_studies, mock_details, mock_docs, mock_investigators): + # Enable the protocol builder for these tests, as the master_workflow and other workflows + # depend on using the PB for data. + ProtocolBuilderService.ENABLED = True self.load_example_data() s = StudyModel( id=54321, # This matches one of the ids from the study_details_json data. @@ -208,6 +200,7 @@ class TestStudyApi(BaseTest): self.assertEqual(study.sponsor, json_data['sponsor']) self.assertEqual(study.ind_number, json_data['ind_number']) + def test_delete_study(self): self.load_example_data() study = session.query(StudyModel).first() diff --git a/tests/test_tasks_api.py b/tests/test_tasks_api.py index 7cf3c8a2..9c8f8c52 100644 --- a/tests/test_tasks_api.py +++ b/tests/test_tasks_api.py @@ -8,6 +8,7 @@ from crc.models.api_models import WorkflowApiSchema, MultiInstanceType, TaskSche from crc.models.file import FileModelSchema from crc.models.stats import TaskEventModel from crc.models.workflow import WorkflowStatus +from crc.services.protocol_builder import ProtocolBuilderService from crc.services.workflow_service import WorkflowService from tests.base_test import BaseTest @@ -302,6 +303,9 @@ class TestTasksApi(BaseTest): @patch('crc.services.protocol_builder.requests.get') def test_multi_instance_task(self, mock_get): + # Enable the protocol builder. + ProtocolBuilderService.ENABLED = True + # This depends on getting a list of investigators back from the protocol builder. mock_get.return_value.ok = True mock_get.return_value.text = self.protocol_builder_response('investigators.json') diff --git a/tests/test_workflow_spec_validation_api.py b/tests/test_workflow_spec_validation_api.py index 7cb7a3c7..123c42c8 100644 --- a/tests/test_workflow_spec_validation_api.py +++ b/tests/test_workflow_spec_validation_api.py @@ -20,21 +20,7 @@ class TestWorkflowSpecValidation(BaseTest): json_data = json.loads(rv.get_data(as_text=True)) return ApiErrorSchema(many=True).load(json_data) - @patch('crc.services.protocol_builder.ProtocolBuilderService.get_investigators') # mock_studies - @patch('crc.services.protocol_builder.ProtocolBuilderService.get_required_docs') # mock_docs - @patch('crc.services.protocol_builder.ProtocolBuilderService.get_study_details') # mock_details - @patch('crc.services.protocol_builder.ProtocolBuilderService.get_studies') # mock_studies - def test_successful_validation_of_test_workflows(self, mock_studies, mock_details, mock_docs, mock_investigators): - - # Mock Protocol Builder responses - studies_response = self.protocol_builder_response('user_studies.json') - mock_studies.return_value = ProtocolBuilderStudySchema(many=True).loads(studies_response) - details_response = self.protocol_builder_response('study_details.json') - mock_details.return_value = json.loads(details_response) - docs_response = self.protocol_builder_response('required_docs.json') - mock_docs.return_value = json.loads(docs_response) - investigators_response = self.protocol_builder_response('investigators.json') - mock_investigators.return_value = json.loads(investigators_response) + def test_successful_validation_of_test_workflows(self): self.assertEqual(0, len(self.validate_workflow("parallel_tasks"))) self.assertEqual(0, len(self.validate_workflow("decision_table"))) From 2e94ab1584e9d7d092a0eaf2bde35fd17bb50e0c Mon Sep 17 00:00:00 2001 From: Aaron Louie Date: Fri, 22 May 2020 14:55:55 -0400 Subject: [PATCH 11/16] Only runs upgrade if env variable says to. --- Dockerfile | 1 - docker_run.sh | 12 +++++------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/Dockerfile b/Dockerfile index fae1657f..fc86ddb0 100644 --- a/Dockerfile +++ b/Dockerfile @@ -17,7 +17,6 @@ RUN pip install pipenv && \ COPY . /app/ ENV FLASK_APP=/app/crc/__init__.py -CMD ["pipenv", "run", "flask", "db", "upgrade"] CMD ["pipenv", "run", "python", "/app/run.py"] # expose ports diff --git a/docker_run.sh b/docker_run.sh index 95792392..1ff01c11 100755 --- a/docker_run.sh +++ b/docker_run.sh @@ -3,17 +3,15 @@ # run migrations export FLASK_APP=./crc/__init__.py -for entry in ./instance/* ; do - echo "$entry" - cat $entry -done - if [ "$DOWNGRADE_DB" = "true" ]; then - echo 'Downgrading...' + echo 'Downgrading database...' pipenv run flask db downgrade fi -pipenv run flask db upgrade +if [ "$UPGRADE_DB" = "true" ]; then + echo 'Upgrading database...' + pipenv run flask db upgrade +fi if [ "$RESET_DB" = "true" ]; then echo 'Resetting database...' From 1017bb1897c5239c27070237d5d398baac22d53f Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Fri, 22 May 2020 15:30:22 -0400 Subject: [PATCH 12/16] On the tools/render_docx api, allow sending the json data in the body, rather than as a ludicrous long get parameter. Silly Dan. --- crc/api.yml | 10 +++------- crc/api/tools.py | 3 ++- tests/test_tools_api.py | 8 +++++--- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/crc/api.yml b/crc/api.yml index 52abbde4..3cb04161 100644 --- a/crc/api.yml +++ b/crc/api.yml @@ -755,13 +755,6 @@ paths: schema: type: string /render_docx: - parameters: - - name: data - in: query - required: true - description: The json data to use in populating the template - schema: - type: string put: operationId: crc.api.tools.render_docx security: [] # Disable security for this endpoint only. @@ -777,6 +770,9 @@ paths: file: type: string format: binary + data: + type: string + format: json responses: '200': description: Returns the generated document. diff --git a/crc/api/tools.py b/crc/api/tools.py index ee732b5c..6fb31b71 100644 --- a/crc/api/tools.py +++ b/crc/api/tools.py @@ -25,13 +25,14 @@ def render_markdown(data, template): raise ApiError(code="invalid", message=str(e)) -def render_docx(data): +def render_docx(): """ Provides a quick way to verify that a Jinja docx template will work properly on a given json data structure. Useful for folks that are building these templates. """ try: file = connexion.request.files['file'] + data = connexion.request.form['data'] target_stream = CompleteTemplate().make_template(file, json.loads(data)) return send_file( io.BytesIO(target_stream.read()), diff --git a/tests/test_tools_api.py b/tests/test_tools_api.py index 19f885ad..48ac65a7 100644 --- a/tests/test_tools_api.py +++ b/tests/test_tools_api.py @@ -1,8 +1,8 @@ import json import os -from crc import app from tests.base_test import BaseTest +from crc import app class TestStudyApi(BaseTest): @@ -22,11 +22,13 @@ class TestStudyApi(BaseTest): {"option": "Address", "selected": False}, {"option": "Phone", "selected": True, "stored": ["Send or Transmit outside of UVA"]}]} with open(filepath, 'rb') as f: - file_data = {'file': (f, 'my_new_file.bpmn')} - rv = self.app.put('/v1.0/render_docx?data=%s' % json.dumps(template_data), + file_data = {'file': (f, 'my_new_file.bpmn'), 'data': json.dumps(template_data)} + rv = self.app.put('/v1.0/render_docx', data=file_data, follow_redirects=True, content_type='multipart/form-data') self.assert_success(rv) + self.assertIsNotNone(rv.data) + self.assertEquals('application/octet-stream', rv.content_type) def test_list_scripts(self): rv = self.app.get('/v1.0/list_scripts') From d1606ffb1a955b0a9fa68a6e68ef83c3e49752f3 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Fri, 22 May 2020 15:31:38 -0400 Subject: [PATCH 13/16] forgot to include the new empty master workflow, which allows the tests to all pass. --- tests/data/empty_workflow/empty_workflow.bpmn | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 tests/data/empty_workflow/empty_workflow.bpmn diff --git a/tests/data/empty_workflow/empty_workflow.bpmn b/tests/data/empty_workflow/empty_workflow.bpmn new file mode 100644 index 00000000..17b8351c --- /dev/null +++ b/tests/data/empty_workflow/empty_workflow.bpmn @@ -0,0 +1,26 @@ + + + + + SequenceFlow_0lvudp8 + + + + SequenceFlow_0lvudp8 + + + + + + + + + + + + + + + + + From 148e86bb42af05a5d0e160d81d0bd26da768f9c0 Mon Sep 17 00:00:00 2001 From: Dan Funk Date: Fri, 22 May 2020 18:25:00 -0400 Subject: [PATCH 14/16] Building out the boilerplate code to make pushing forward on this a little friendlier. There is an approval api file, and approval model file and an approval test file. --- crc/api.yml | 57 ++++++++++++++++++++++++++++-- crc/api/approval.py | 10 ++++++ crc/models/approval.py | 77 +++++++++++++++++++++++++++++++++++++++++ crc/models/user.py | 6 ---- tests/base_test.py | 4 +-- tests/test_approvals.py | 14 ++++++++ 6 files changed, 157 insertions(+), 11 deletions(-) create mode 100644 crc/api/approval.py create mode 100644 crc/models/approval.py create mode 100644 tests/test_approvals.py diff --git a/crc/api.yml b/crc/api.yml index 15b570a5..a08231d0 100644 --- a/crc/api.yml +++ b/crc/api.yml @@ -795,6 +795,54 @@ paths: type: array items: $ref: "#/components/schemas/Script" + /approval: + parameters: + - name: approver_uid + in: query + required: false + description: Restrict results to a given approver uid, maybe we restrict the use of this at somepoint. + schema: + type: string + get: + operationId: crc.api.approval.get_approvals + summary: Provides a list of workflows approvals + tags: + - Approvals + responses: + '200': + description: An array of approvals + content: + application/json: + schema: + type: array + items: + $ref: "#/components/schemas/Approval" + /approval/{approval_id}: + parameters: + - name: approval_id + in: path + required: true + description: The id of the approval in question. + schema: + type: integer + format: int32 + put: + operationId: crc.api.approval.update_approval + summary: Updates an approval with the given parameters + tags: + - Approvals + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/Approval' + responses: + '200': + description: Study updated successfully. + content: + application/json: + schema: + $ref: "#/components/schemas/Approval" components: securitySchemes: jwt: @@ -1202,7 +1250,10 @@ components: readOnly: true task: $ref: "#/components/schemas/Task" - - - + Approval: + properties: + id: + type: number + format: integer + example: 5 diff --git a/crc/api/approval.py b/crc/api/approval.py new file mode 100644 index 00000000..134f9a8c --- /dev/null +++ b/crc/api/approval.py @@ -0,0 +1,10 @@ +from crc.models.approval import ApprovalModel, Approval + + +def get_approvals(approver_uid = None): + approval_model = ApprovalModel() + approval = Approval.from_model(approval_model) + return {} + +def update_approval(approval_id): + return {} \ No newline at end of file diff --git a/crc/models/approval.py b/crc/models/approval.py new file mode 100644 index 00000000..4860bf59 --- /dev/null +++ b/crc/models/approval.py @@ -0,0 +1,77 @@ +import enum + +from marshmallow import INCLUDE + +from crc import db, ma +from crc.models.study import StudyModel +from crc.models.workflow import WorkflowModel + + +class ApprovalStatus(enum.Enum): + WAITING = "WAITING" # no one has done jack. + APPROVED = "APPROVED" # approved by the reviewer + DECLINED = "DECLINED" # rejected by the reviewer + CANCELED = "CANCELED" # The document was replaced with a new version and this review is no longer needed. + + +class ApprovalModel(db.Model): + __tablename__ = 'approval' + id = db.Column(db.Integer, primary_key=True) + study_id = db.Column(db.Integer, db.ForeignKey(StudyModel.id), nullable=False) + study = db.relationship(StudyModel, backref='approval') + workflow_id = db.Column(db.Integer, db.ForeignKey(WorkflowModel.id), nullable=False) + workflow_version = db.Column(db.String) + approver_uid = db.Column(db.String) # Not linked to user model, as they may not have logged in yet. + status = db.Column(db.String) + message = db.Column(db.String) + + +class Approval(object): + + @classmethod + def from_model(cls, model: ApprovalModel): + instance = cls() + + instance.id = model.id + instance.workflow_version = model.workflow_version + instance.approver_uid = model.approver_uid + instance.status = model.status + instance.study_id = model.study_id + if model.study: + instance.title = model.study.title + + +class ApprovalSchema(ma.Schema): + class Meta: + model = Approval + fields = ["id", "workflow_version", "approver_uid", "status", + "study_id", "title"] + unknown = INCLUDE + +# Carlos: Here is the data structure I was trying to imagine. +# If I were to continue down my current traing of thought, I'd create +# another class called just "Approval" that can take an ApprovalModel from the +# database and construct a data structure like this one, that can +# be provided to the API at an /approvals endpoint with GET and PUT +# dat = { "approvals": [ +# {"id": 1, +# "study_id": 20, +# "workflow_id": 454, +# "study_title": "Dan Funk (dhf8r)", # Really it's just the name of the Principal Investigator +# "workflow_version": "21", +# "approver": { # Pulled from ldap +# "uid": "bgb22", +# "display_name": "Billy Bob (bgb22)", +# "title": "E42:He's a hoopy frood", +# "department": "E0:EN-Eng Study of Parallel Universes", +# }, +# "files": [ +# { +# "id": 124, +# "name": "ResearchRestart.docx", +# "content_type": "docx-something-whatever" +# } +# ] +# } +# ... +# ] diff --git a/crc/models/user.py b/crc/models/user.py index dc7f4e03..d9ee8f72 100644 --- a/crc/models/user.py +++ b/crc/models/user.py @@ -7,12 +7,6 @@ from crc import db, app from crc.api.common import ApiError -class Approvals(db.Model): - approval_uid = db.Column(db.String, unique=True) - workflow_id = db.Column(db.Integer, primary_key=True) - workflow_version = db.Column(db.String) - is_approved = db.Column(db.Boolean) - class UserModel(db.Model): __tablename__ = 'user' id = db.Column(db.Integer, primary_key=True) diff --git a/tests/base_test.py b/tests/base_test.py index 6d0acfee..990e72cf 100644 --- a/tests/base_test.py +++ b/tests/base_test.py @@ -31,8 +31,8 @@ class BaseTest(unittest.TestCase): """ if not app.config['TESTING']: - raise ("INVALID TEST CONFIGURATION. This is almost always in import order issue." - "The first class to import in each test should be the base_test.py file.") + raise (Exception("INVALID TEST CONFIGURATION. This is almost always in import order issue." + "The first class to import in each test should be the base_test.py file.")) auths = {} test_uid = "dhf8r" diff --git a/tests/test_approvals.py b/tests/test_approvals.py new file mode 100644 index 00000000..38000b88 --- /dev/null +++ b/tests/test_approvals.py @@ -0,0 +1,14 @@ +from tests.base_test import BaseTest + + +class TestApprovals(BaseTest): + + def test_list_approvals(self): + rv = self.app.get('/v1.0/approval', headers=self.logged_in_headers()) + self.assert_success(rv) + + def test_update_approval(self): + rv = self.app.put('/v1.0/approval/1', + headers=self.logged_in_headers(), + data={}) + self.assert_success(rv) From 20bf01a88800b6e36a64a4c12a9396e9ca4397ba Mon Sep 17 00:00:00 2001 From: Aaron Louie Date: Fri, 22 May 2020 22:04:11 -0400 Subject: [PATCH 15/16] Adds cascade to study relationship so data loader doesn't freak out. --- crc/models/approval.py | 2 +- example_data.py | 3 ++- migrations/versions/55c6cd407d89_.py | 39 ++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 migrations/versions/55c6cd407d89_.py diff --git a/crc/models/approval.py b/crc/models/approval.py index 4860bf59..7d513a7f 100644 --- a/crc/models/approval.py +++ b/crc/models/approval.py @@ -18,7 +18,7 @@ class ApprovalModel(db.Model): __tablename__ = 'approval' id = db.Column(db.Integer, primary_key=True) study_id = db.Column(db.Integer, db.ForeignKey(StudyModel.id), nullable=False) - study = db.relationship(StudyModel, backref='approval') + study = db.relationship(StudyModel, backref='approval', cascade='all,delete') workflow_id = db.Column(db.Integer, db.ForeignKey(WorkflowModel.id), nullable=False) workflow_version = db.Column(db.String) approver_uid = db.Column(db.String) # Not linked to user model, as they may not have logged in yet. diff --git a/example_data.py b/example_data.py index 22e6f95b..7f53b13c 100644 --- a/example_data.py +++ b/example_data.py @@ -14,7 +14,8 @@ class ExampleDataLoader: session.flush() # Clear out any transactions before deleting it all to avoid spurious errors. for table in reversed(db.metadata.sorted_tables): session.execute(table.delete()) - session.flush() + session.commit() + session.flush() def load_all(self): diff --git a/migrations/versions/55c6cd407d89_.py b/migrations/versions/55c6cd407d89_.py new file mode 100644 index 00000000..e5dd02ee --- /dev/null +++ b/migrations/versions/55c6cd407d89_.py @@ -0,0 +1,39 @@ +"""empty message + +Revision ID: 55c6cd407d89 +Revises: cc4bccc5e5a8 +Create Date: 2020-05-22 22:02:46.650170 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '55c6cd407d89' +down_revision = 'cc4bccc5e5a8' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('approval', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('study_id', sa.Integer(), nullable=False), + sa.Column('workflow_id', sa.Integer(), nullable=False), + sa.Column('workflow_version', sa.String(), nullable=True), + sa.Column('approver_uid', sa.String(), nullable=True), + sa.Column('status', sa.String(), nullable=True), + sa.Column('message', sa.String(), nullable=True), + sa.ForeignKeyConstraint(['study_id'], ['study.id'], ), + sa.ForeignKeyConstraint(['workflow_id'], ['workflow.id'], ), + sa.PrimaryKeyConstraint('id') + ) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_table('approval') + # ### end Alembic commands ### From 6c14248ef9fa8e0ecb86dff82d379de223f7f7b5 Mon Sep 17 00:00:00 2001 From: Aaron Louie Date: Sat, 23 May 2020 14:49:02 -0400 Subject: [PATCH 16/16] Adds 'v1.0/' to login route --- crc/api/user.py | 2 +- tests/test_authentication.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crc/api/user.py b/crc/api/user.py index 6924eb27..3e423fc6 100644 --- a/crc/api/user.py +++ b/crc/api/user.py @@ -32,7 +32,7 @@ def verify_token(token): def get_current_user(): return UserModelSchema().dump(g.user) -@app.route('/login') +@app.route('/v1.0/login') def sso_login(): # This what I see coming back: # X-Remote-Cn: Daniel Harold Funk (dhf8r) diff --git a/tests/test_authentication.py b/tests/test_authentication.py index 2767797a..7601a220 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -44,7 +44,7 @@ class TestAuthentication(BaseTest): self.assertIsNone(user) redirect_url = 'http://worlds.best.website/admin' headers = dict(Uid=new_uid) - rv = self.app.get('login', follow_redirects=False, headers=headers) + rv = self.app.get('v1.0/login', follow_redirects=False, headers=headers) self.assert_success(rv) user = db.session.query(UserModel).filter(UserModel.uid == new_uid).first() self.assertIsNotNone(user)