From 7b82e9ec73015deff7063ec8e6cb6f690cb55d70 Mon Sep 17 00:00:00 2001 From: Daniel Nilsson Date: Mon, 4 Mar 2024 16:55:50 +0100 Subject: [PATCH] Allow document id variant API connect, and improve user cred check when accessing variants (#4455) * Allow direct navigation to unique document_id specified variants * changelog * properly check permissions on variant * update changelog * fix changelog * changelog * Merge with safety fix. * leftover typos * if unset, use institute and case of variant * add test * lint... * lint * not that kind of object * Update CHANGELOG.md Co-authored-by: Chiara Rasi * Refactor a common variant institute and case check * lint.. * TypeHints * would have been nice if that was linted. * one more missing variant test * fix that new route --------- Co-authored-by: Chiara Rasi Co-authored-by: tereseboderus --- CHANGELOG.md | 2 + scout/server/blueprints/api/views.py | 57 +++++-- scout/server/blueprints/cases/controllers.py | 2 +- .../server/blueprints/variant/controllers.py | 145 +++++++++++------- .../variant/verification_controllers.py | 2 +- scout/server/blueprints/variant/views.py | 15 +- scout/server/utils.py | 31 +++- tests/server/blueprints/api/test_api_views.py | 7 +- .../variant/test_variant_controllers.py | 12 +- .../blueprints/variant/test_variant_views.py | 20 +++ 10 files changed, 210 insertions(+), 83 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45949aad8a..8e69c5a575 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,9 +9,11 @@ About changelog [here](https://keepachangelog.com/en/1.0.0/) - Added tags for Sniffles and CNVpytor, two LRS SV callers ### Changed - In the diagnoses page genes associated with a disease are displayed using hgnc symbol instead of hgnc id +- Refactor view route to allow navigation directly to unique variant document id, improve permissions check ### Fixed - Refactored code in cases blueprints and variant_events adapter (set diseases for partial causative variants) to use "disease" instead of "omim" to encompass also ORPHA terms - Refactored code in `scout/parse/omim.py` and `scout/parse/disease_terms.py` to use "disease" instead of "phenotype" to differentiate from HPO terms +- Be more careful about checking access to variant on API access ## [4.78] ### Added diff --git a/scout/server/blueprints/api/views.py b/scout/server/blueprints/api/views.py index bd19c29a28..92999bba49 100644 --- a/scout/server/blueprints/api/views.py +++ b/scout/server/blueprints/api/views.py @@ -1,9 +1,11 @@ +from typing import Optional, Tuple + from bson import json_util from flask import Blueprint, Response, abort, url_for from flask_login import current_user from scout.server.extensions import store -from scout.server.utils import institute_and_case +from scout.server.utils import institute_and_case, variant_institute_and_case api_bp = Blueprint("api", __name__, url_prefix="/api/v1") @@ -17,19 +19,53 @@ def case(institute_id, case_name): return Response(json_util.dumps(case_obj), mimetype="application/json") +def _lookup_variant( + variant_id: str, + institute_id: Optional[str] = None, + case_name: Optional[str] = None, +) -> Optional[Tuple[dict, dict, dict]]: + """Lookup variant using variant document id. Return institute, case, and variant obj dicts. + + The institute and case lookup adds a bit of security, checking if user is admin or + has access to institute and case, but since the variant is looked up using document_id, + we also run an additional security check that the given case_id matches the variant case_id. + + The check without institute and case is strict, as the user cannot choose what institute to + match against the ones the user has access to. + """ + + variant_obj = store.variant(variant_id) + + if not variant_obj: + return abort(404) + + institute_obj, case_obj = variant_institute_and_case( + store, variant_obj, institute_id, case_name + ) + + return (institute_obj, case_obj, variant_obj) + + @api_bp.route("///") -def variant(institute_id, case_name, variant_id): +@api_bp.route("/variant/") +def variant( + variant_id: str, institute_id: Optional[str] = None, case_name: Optional[str] = None +) -> Optional[Response]: """Display a specific SNV variant.""" - variant_obj = store.variant(variant_id) + + (_, _, variant_obj) = _lookup_variant(variant_id, institute_id, case_name) + return Response(json_util.dumps(variant_obj), mimetype="application/json") @api_bp.route("////pin") -def pin_variant(institute_id, case_name, variant_id): +@api_bp.route("//pin") +def pin_variant( + variant_id: str, institute_id: Optional[str] = None, case_name: Optional[str] = None +): """Pin an existing variant""" - institute_obj, case_obj = institute_and_case(store, institute_id, case_name) - variant_obj = store.variant(variant_id) + (institute_obj, case_obj, variant_obj) = _lookup_variant(variant_id, institute_id, case_name) user_obj = store.user(current_user.email) link = url_for( @@ -44,10 +80,13 @@ def pin_variant(institute_id, case_name, variant_id): @api_bp.route("////unpin") -def unpin_variant(institute_id, case_name, variant_id): +@api_bp.route("//unpin") +def unpin_variant( + variant_id: str, institute_id: Optional[str] = None, case_name: Optional[str] = None +): """Un-pin an existing, pinned variant""" - institute_obj, case_obj = institute_and_case(store, institute_id, case_name) - variant_obj = store.variant(variant_id) + + (institute_obj, case_obj, variant_obj) = _lookup_variant(variant_id, institute_id, case_name) user_obj = store.user(current_user.email) link = url_for( diff --git a/scout/server/blueprints/cases/controllers.py b/scout/server/blueprints/cases/controllers.py index bfeb8b9c54..43f7d4fa25 100644 --- a/scout/server/blueprints/cases/controllers.py +++ b/scout/server/blueprints/cases/controllers.py @@ -566,9 +566,9 @@ def case_report_variants(store: MongoAdapter, case_obj: dict, institute_obj: dic def _get_decorated_var(var_obj: dict) -> dict: return variant_decorator( store=store, + variant_id=None, institute_id=institute_obj["_id"], case_name=case_obj["display_name"], - variant_id=None, variant_obj=var_obj, add_other=False, get_overlapping=False, diff --git a/scout/server/blueprints/variant/controllers.py b/scout/server/blueprints/variant/controllers.py index 7efa288a74..1b978056b3 100644 --- a/scout/server/blueprints/variant/controllers.py +++ b/scout/server/blueprints/variant/controllers.py @@ -1,9 +1,9 @@ import logging import os -from typing import Dict, List +from typing import Dict, List, Optional import requests -from flask import Markup, current_app, flash, url_for +from flask import Markup, abort, current_app, flash, url_for from flask_login import current_user from scout.adapter import MongoAdapter @@ -33,8 +33,8 @@ case_has_alignments, case_has_mt_alignments, case_has_rna_tracks, - institute_and_case, user_institutes, + variant_institute_and_case, ) from .utils import ( @@ -54,7 +54,7 @@ LOG = logging.getLogger(__name__) -def tx_overview(variant_obj): +def tx_overview(variant_obj: dict): """Prepares the content of the transcript overview to be shown on variant and general report pages. Basically show transcripts that contain RefSeq or are canonical. @@ -128,7 +128,7 @@ def tx_overview(variant_obj): ) -def get_igv_tracks(build="37"): +def get_igv_tracks(build: str = "37") -> set: """Return all available IGV tracks for the given genome build, as a set Args: @@ -149,17 +149,17 @@ def get_igv_tracks(build="37"): def variant( - store, - institute_id, - case_name, - variant_id=None, - variant_obj=None, - add_other=True, - get_overlapping=True, - variant_type=None, - case_obj=None, - institute_obj=None, -): + store: MongoAdapter, + variant_id: Optional[str], + institute_id: str = None, + case_name: str = None, + variant_obj: dict = None, + add_other: bool = True, + get_overlapping: bool = True, + variant_type: str = None, + case_obj: dict = None, + institute_obj: dict = None, +) -> Optional[dict]: """Pre-process a single variant for the detailed variant view. Adds information from case and institute that is not present on the variant @@ -194,15 +194,19 @@ def variant( } """ - if not (institute_obj and case_obj): - institute_obj, case_obj = institute_and_case(store, institute_id, case_name) + # If the variant is already collected we skip this part if not variant_obj: # NOTE this will query with variant_id == document_id, not the variant_id. variant_obj = store.variant(variant_id) - if variant_obj is None or variant_obj.get("case_id") != case_obj["_id"]: - return None + if not variant_obj: + return abort(404) + + if not (institute_obj and case_obj): + (institute_obj, case_obj) = variant_institute_and_case( + store, variant_obj, institute_id, case_name + ) variant_type = variant_type or variant_obj.get("variant_type", "clinical") @@ -375,7 +379,7 @@ def variant( } -def variant_rank_scores(store, case_obj, variant_obj): +def variant_rank_scores(store: MongoAdapter, case_obj: dict, variant_obj: dict) -> list: """Retrive rank score values and ranges for the variant Args: @@ -393,9 +397,6 @@ def variant_rank_scores(store, case_obj, variant_obj): ): # Retrieve rank score results saved in variant document rank_score_results = variant_obj.get("rank_score_results") - rm_link_prefix = None - rm_file_extension = None - if variant_obj.get("category") == "sv": rank_model_version = case_obj.get("sv_rank_model_version") rm_link_prefix = current_app.config.get("SV_RANK_MODEL_LINK_PREFIX") @@ -514,14 +515,11 @@ def observations(store: MongoAdapter, loqusdb: LoqusDB, variant_obj: dict) -> Di def str_variant_reviewer( - store, - case_obj, - variant_id, -): + store: MongoAdapter, + case_obj: dict, + variant_id: str, +) -> dict: """Controller populating data and calling REViewer Service to fetch svg. - Args: - case_obj(dict) - variant_obj(dict) Returns: data(dict): {"individuals": list(dict())}} individual dicts being dict with keys: @@ -577,7 +575,7 @@ def str_variant_reviewer( } -def variant_acmg(store, institute_id, case_name, variant_id): +def variant_acmg(store: MongoAdapter, institute_id: str, case_name: str, variant_id: str): """Collect data relevant for rendering ACMG classification form. Args: @@ -589,8 +587,15 @@ def variant_acmg(store, institute_id, case_name, variant_id): Returns: data(dict): Things for the template """ - institute_obj, case_obj = institute_and_case(store, institute_id, case_name) variant_obj = store.variant(variant_id) + + if not variant_obj: + return abort(404) + + institute_obj, case_obj = variant_institute_and_case( + store, variant_obj, institute_id, case_name + ) + return dict( institute=institute_obj, case=case_obj, @@ -600,7 +605,9 @@ def variant_acmg(store, institute_id, case_name, variant_id): ) -def check_reset_variant_classification(store, evaluation_obj, link): +def check_reset_variant_classification( + store: MongoAdapter, evaluation_obj: dict, link: str +) -> bool: """Check if this was the last ACMG evaluation left on the variant. If there is still a classification we want to remove the classification. @@ -613,32 +620,47 @@ def check_reset_variant_classification(store, evaluation_obj, link): """ - if len(list(store.get_evaluations_case_specific(evaluation_obj["variant_specific"]))) == 0: - variant_obj = store.variant(document_id=evaluation_obj["variant_specific"]) - acmg_classification = variant_obj.get("acmg_classification") - if isinstance(acmg_classification, int): - institute_obj, case_obj = institute_and_case( - store, - evaluation_obj["institute"]["_id"], - evaluation_obj["case"]["display_name"], - ) - user_obj = store.user(current_user.email) - - new_acmg = None - store.submit_evaluation( - variant_obj=variant_obj, - user_obj=user_obj, - institute_obj=institute_obj, - case_obj=case_obj, - link=link, - classification=new_acmg, - ) - return True + if list(store.get_evaluations_case_specific(evaluation_obj["variant_specific"])): + return False - return False + variant_obj = store.variant(document_id=evaluation_obj["variant_specific"]) + if not variant_obj: + return abort(404) -def variant_acmg_post(store, institute_id, case_name, variant_id, user_email, criteria): + acmg_classification = variant_obj.get("acmg_classification") + + if not isinstance(acmg_classification, int): + return False + + institute_obj, case_obj = variant_institute_and_case( + store, + variant_obj, + evaluation_obj["institute"]["_id"], + evaluation_obj["case"]["display_name"], + ) + user_obj = store.user(current_user.email) + + new_acmg = None + store.submit_evaluation( + variant_obj=variant_obj, + user_obj=user_obj, + institute_obj=institute_obj, + case_obj=case_obj, + link=link, + classification=new_acmg, + ) + return True + + +def variant_acmg_post( + store: MongoAdapter, + institute_id: str, + case_name: str, + variant_id: str, + user_email: str, + criteria: list, +) -> dict: """Calculate an ACMG classification based on a list of criteria. Args: @@ -653,8 +675,15 @@ def variant_acmg_post(store, institute_id, case_name, variant_id, user_email, cr data(dict): Things for the template """ - institute_obj, case_obj = institute_and_case(store, institute_id, case_name) variant_obj = store.variant(variant_id) + + if not variant_obj: + return abort(404) + + institute_obj, case_obj = variant_institute_and_case( + store, variant_obj, institute_id, case_name + ) + user_obj = store.user(user_email) variant_link = url_for( "variant.variant", diff --git a/scout/server/blueprints/variant/verification_controllers.py b/scout/server/blueprints/variant/verification_controllers.py index 618a8266c1..d6cfdce883 100644 --- a/scout/server/blueprints/variant/verification_controllers.py +++ b/scout/server/blueprints/variant/verification_controllers.py @@ -51,9 +51,9 @@ def variant_verification( data = variant_controller( store=store, + variant_id=variant_id, institute_id=institute_id, case_name=case_name, - variant_id=variant_id, add_other=False, get_overlapping=False, ) diff --git a/scout/server/blueprints/variant/views.py b/scout/server/blueprints/variant/views.py index f097ef0960..a58f27d1fe 100644 --- a/scout/server/blueprints/variant/views.py +++ b/scout/server/blueprints/variant/views.py @@ -41,14 +41,18 @@ def update_tracks_settings(): return redirect(request.referrer) +@variant_bp.route("/document_id/") @variant_bp.route("///") @templated("variant/variant.html") -def variant(institute_id, case_name, variant_id): +def variant(variant_id, institute_id=None, case_name=None): """Display a specific SNV variant.""" LOG.debug("Variants view requesting data for variant %s", variant_id) data = variant_controller( - store=store, institute_id=institute_id, case_name=case_name, variant_id=variant_id + store=store, + variant_id=variant_id, + institute_id=institute_id, + case_name=case_name, ) if data is None: flash("An error occurred while retrieving variant object", "danger") @@ -70,7 +74,7 @@ def cancer_variant(institute_id, case_name, variant_id): LOG.debug("Variants view requesting data for variant %s", variant_id) data = variant_controller( - store=store, institute_id=institute_id, case_name=case_name, variant_id=variant_id + store=store, variant_id=variant_id, institute_id=institute_id, case_name=case_name ) if data is None: flash("An error occurred while retrieving variant object", "danger") @@ -94,7 +98,10 @@ def cancer_variant(institute_id, case_name, variant_id): def sv_variant(institute_id, case_name, variant_id): """Display a specific structural variant.""" data = variant_controller( - store=store, institute_id=institute_id, case_name=case_name, variant_id=variant_id + store=store, + variant_id=variant_id, + institute_id=institute_id, + case_name=case_name, ) if data is None: diff --git a/scout/server/utils.py b/scout/server/utils.py index 918b077bae..54051d6258 100644 --- a/scout/server/utils.py +++ b/scout/server/utils.py @@ -6,7 +6,7 @@ import zipfile from functools import wraps from io import BytesIO -from typing import Dict +from typing import Dict, Optional, Tuple import pdfkit from bson.objectid import ObjectId @@ -101,6 +101,35 @@ def public_endpoint(function): return function +def variant_institute_and_case( + store, variant_obj: dict, institute_id: Optional[str], case_name: Optional[str] +) -> Tuple[dict, dict]: + """Fetch insitiute and case objects. + + Ensure that case_id on the variant matches that of any case given. + Ensure user has access to variant institute, or to the case through institute case sharing. + """ + + if not case_name: + variant_case_obj = store.case( + case_id=variant_obj["case_id"], projection={"display_name": 1} + ) + case_name = variant_case_obj["display_name"] + + if not institute_id: + institute_id = variant_obj["institute"] + + (institute_obj, case_obj) = institute_and_case(store, institute_id, case_name) + + if case_obj is None: + return abort(404) + + if variant_obj.get("case_id") != case_obj.get("_id"): + return abort(403) + + return (institute_obj, case_obj) + + def institute_and_case(store, institute_id, case_name=None): """Fetch insitiute and case objects.""" diff --git a/tests/server/blueprints/api/test_api_views.py b/tests/server/blueprints/api/test_api_views.py index 2bc0307b45..e2e7c0adf1 100644 --- a/tests/server/blueprints/api/test_api_views.py +++ b/tests/server/blueprints/api/test_api_views.py @@ -37,9 +37,9 @@ def test_variant(app, case_obj, variant_obj): resp = client.get( url_for( "api.variant", + variant_id=variant_id, institute_id=case_obj["owner"], case_name=case_name, - variant_id=variant_id, ) ) # THEN it should return a valid response @@ -57,13 +57,14 @@ def test_pin(app, case_obj, variant_obj): with app.test_client() as client: # GIVEN that the user could be logged in client.get(url_for("auto_login")) + # WHEN the API is invoked with the right params resp = client.get( url_for( "api.pin_variant", + variant_id=variant_id, institute_id=case_obj["owner"], case_name=case_name, - variant_id=variant_id, link="pinned it", ) ) @@ -99,9 +100,9 @@ def test_unpin(app, institute_obj, case_obj, variant_obj): resp = client.get( url_for( "api.unpin_variant", + variant_id=variant_id, institute_id=case_obj["owner"], case_name=case_name, - variant_id=variant_id, link="unpinned it", ) ) diff --git a/tests/server/blueprints/variant/test_variant_controllers.py b/tests/server/blueprints/variant/test_variant_controllers.py index 675f2e44ff..76445d6fbd 100644 --- a/tests/server/blueprints/variant/test_variant_controllers.py +++ b/tests/server/blueprints/variant/test_variant_controllers.py @@ -424,8 +424,8 @@ def test_variant_controller_with_compounds(app, institute_obj, case_obj): ## WHEN fetching the variant from the controller data = variant( store, - institute_id, - case_name, + institute_id=institute_id, + case_name=case_name, variant_id=var_id, add_other=True, get_overlapping=True, @@ -461,9 +461,9 @@ def test_variant_controller_with_clnsig(app, institute_obj, case_obj): ## WHEN fetching the variant from the controller data = variant( store, - institute_id, - case_name, variant_id=var_id, + institute_id=institute_id, + case_name=case_name, add_other=True, get_overlapping=True, variant_type=category, @@ -489,9 +489,9 @@ def test_variant_controller(app, institute_obj, case_obj, variant_obj): ## WHEN fetching the variant from the controller data = variant( store, - institute_id, - case_name, variant_id=var_id, + institute_id=institute_id, + case_name=case_name, add_other=True, get_overlapping=True, variant_type=category, diff --git a/tests/server/blueprints/variant/test_variant_views.py b/tests/server/blueprints/variant/test_variant_views.py index 4c4f626cc7..9be856085d 100644 --- a/tests/server/blueprints/variant/test_variant_views.py +++ b/tests/server/blueprints/variant/test_variant_views.py @@ -98,6 +98,26 @@ def test_variant(app, institute_obj, case_obj, variant_obj): assert resp.status_code == 200 +def test_variant_by_document_id(app, variant_obj): + # GIVEN an initialized app + # GIVEN a valid user + + with app.test_client() as client: + # GIVEN that the user could be logged in + resp = client.get(url_for("auto_login")) + assert resp.status_code == 200 + + # WHEN sending a request (GET) to the variant page with only a variant ID + resp = client.get( + url_for( + "variant.variant", + variant_id=variant_obj["_id"], + ) + ) + # THEN it should return a page + assert resp.status_code == 200 + + def test_cancer_variant(app, cancer_case_obj, cancer_variant_obj): # GIVEN a cancer case object with a variant saved in database assert store.case_collection.insert_one(cancer_case_obj)