diff --git a/src/mavedb/lib/annotation/annotate.py b/src/mavedb/lib/annotation/annotate.py index e5e289a3..83b13b71 100644 --- a/src/mavedb/lib/annotation/annotate.py +++ b/src/mavedb/lib/annotation/annotate.py @@ -8,14 +8,14 @@ See: https://va-spec.ga4gh.org/en/latest/va-standard-profiles/community-profiles/acmg-2015-profiles.html#variant-pathogenicity-statement-acmg-2015 """ -from typing import Optional, Union +from typing import Optional, Sequence, TypeVar, Union from ga4gh.va_spec.acmg_2015 import VariantPathogenicityStatement from ga4gh.va_spec.base.core import ExperimentalVariantFunctionalImpactStudyResult, Statement from mavedb.lib.annotation.classification import functional_classification_of_variant -from mavedb.lib.annotation.exceptions import MappingDataDoesntExistException from mavedb.lib.annotation.evidence_line import acmg_evidence_line, functional_evidence_line +from mavedb.lib.annotation.exceptions import MappingDataDoesntExistException from mavedb.lib.annotation.proposition import ( mapped_variant_to_experimental_variant_clinical_impact_proposition, mapped_variant_to_experimental_variant_functional_impact_proposition, @@ -26,39 +26,62 @@ ) from mavedb.lib.annotation.study_result import mapped_variant_to_experimental_variant_impact_study_result from mavedb.lib.annotation.util import ( + calibration_scope_extension, + calibrations_available_for_annotation, can_annotate_variant_for_functional_statement, can_annotate_variant_for_pathogenicity_evidence, - score_calibration_may_be_used_for_annotation, select_strongest_functional_calibration, select_strongest_pathogenicity_calibration, ) +from mavedb.lib.permissions.principal import Principal from mavedb.models.mapped_variant import MappedVariant +from mavedb.models.score_calibration import ScoreCalibration + +Annotation = TypeVar( + "Annotation", ExperimentalVariantFunctionalImpactStudyResult, Statement, VariantPathogenicityStatement +) + + +def _disclosing_calibration_scope(annotation: Annotation, calibrations: Sequence[ScoreCalibration]) -> Annotation: + """Record on the annotation which principal it was built for. + + Applied at the top-level entry points only. Nested study results and statements built as components of + an evidence line inherit the scope of the object that contains them. + """ + # model_copy rather than assigning to `.extensions`: mypy resolves the field's element type to a + # `ga4gh.va_spec.base.core.Extension` that does not exist at runtime (the ga4gh namespace packages + # confuse its import resolution), so a direct assignment is a false positive. + return annotation.model_copy( + update={"extensions": [*(annotation.extensions or []), calibration_scope_extension(calibrations)]} + ) def variant_study_result(mapped_variant: MappedVariant) -> ExperimentalVariantFunctionalImpactStudyResult: - return mapped_variant_to_experimental_variant_impact_study_result(mapped_variant) + # A study result reports the measured score and carries no calibration-derived evidence, so its scope + # is public regardless of viewer. Disclosed anyway, so that a missing scope never has to be read as + # "public" or "generated before disclosure existed". + return _disclosing_calibration_scope(mapped_variant_to_experimental_variant_impact_study_result(mapped_variant), []) def variant_functional_impact_statement( - mapped_variant: MappedVariant, allow_research_use_only_calibrations: bool = False + mapped_variant: MappedVariant, + allow_research_use_only_calibrations: bool = False, + principal: Optional[Principal] = None, ) -> Optional[Statement]: if not can_annotate_variant_for_functional_statement( - mapped_variant, allow_research_use_only_calibrations=allow_research_use_only_calibrations + mapped_variant, allow_research_use_only_calibrations=allow_research_use_only_calibrations, principal=principal ): return None study_result = mapped_variant_to_experimental_variant_impact_study_result(mapped_variant) functional_proposition = mapped_variant_to_experimental_variant_functional_impact_proposition(mapped_variant) - # Collect eligible calibrations - eligible_calibrations = [] - for score_calibration in mapped_variant.variant.score_set.score_calibrations: - if score_calibration_may_be_used_for_annotation( - score_calibration, - annotation_type="functional", - allow_research_use_only_calibrations=allow_research_use_only_calibrations, - ): - eligible_calibrations.append(score_calibration) + eligible_calibrations = calibrations_available_for_annotation( + mapped_variant, + "functional", + allow_research_use_only_calibrations=allow_research_use_only_calibrations, + principal=principal, + ) # Select the calibration with the strongest evidence strongest_calibration, strongest_range = select_strongest_functional_calibration( @@ -77,16 +100,21 @@ def variant_functional_impact_statement( for score_calibration in eligible_calibrations: functional_evidence.append(functional_evidence_line(mapped_variant, score_calibration, [study_result])) - return mapped_variant_to_functional_statement( - mapped_variant, functional_proposition, functional_evidence, strongest_calibration, classification + return _disclosing_calibration_scope( + mapped_variant_to_functional_statement( + mapped_variant, functional_proposition, functional_evidence, strongest_calibration, classification + ), + eligible_calibrations, ) def variant_pathogenicity_statement( - mapped_variant: MappedVariant, allow_research_use_only_calibrations: bool = False + mapped_variant: MappedVariant, + allow_research_use_only_calibrations: bool = False, + principal: Optional[Principal] = None, ) -> Optional[VariantPathogenicityStatement]: if not can_annotate_variant_for_pathogenicity_evidence( - mapped_variant, allow_research_use_only_calibrations=allow_research_use_only_calibrations + mapped_variant, allow_research_use_only_calibrations=allow_research_use_only_calibrations, principal=principal ): return None @@ -94,15 +122,12 @@ def variant_pathogenicity_statement( functional_proposition = mapped_variant_to_experimental_variant_functional_impact_proposition(mapped_variant) clinical_proposition = mapped_variant_to_experimental_variant_clinical_impact_proposition(mapped_variant) - # Collect eligible calibrations - eligible_calibrations = [] - for score_calibration in mapped_variant.variant.score_set.score_calibrations: - if score_calibration_may_be_used_for_annotation( - score_calibration, - annotation_type="pathogenicity", - allow_research_use_only_calibrations=allow_research_use_only_calibrations, - ): - eligible_calibrations.append(score_calibration) + eligible_calibrations = calibrations_available_for_annotation( + mapped_variant, + "pathogenicity", + allow_research_use_only_calibrations=allow_research_use_only_calibrations, + principal=principal, + ) # Select the calibration with the strongest evidence strongest_calibration, strongest_range = select_strongest_pathogenicity_calibration( @@ -130,25 +155,33 @@ def variant_pathogenicity_statement( acmg_evidence_line(mapped_variant, score_calibration, clinical_proposition, [functional_statement]) ) - return mapped_variant_to_pathogenicity_statement( - mapped_variant, clinical_proposition, clinical_evidence, strongest_calibration, strongest_range + return _disclosing_calibration_scope( + mapped_variant_to_pathogenicity_statement( + mapped_variant, clinical_proposition, clinical_evidence, strongest_calibration, strongest_range + ), + eligible_calibrations, ) def variant_highest_level_annotation( mapped_variant: MappedVariant, + principal: Optional[Principal] = None, ) -> Optional[Union[ExperimentalVariantFunctionalImpactStudyResult, Statement, VariantPathogenicityStatement]]: """ Build the single highest-materialized VA-Spec layer for a mapped variant. Layer ladder (highest to lowest): pathogenicity statement -> functional impact statement -> study result. Returns None when the variant has no post-mapped allele and therefore cannot be annotated. + + The viewer decides which layer is reachable as well as what the layer contains: a variant whose only + calibration is invisible to this principal degrades to a study result rather than yielding a statement + with nothing in it. """ try: - if can_annotate_variant_for_pathogenicity_evidence(mapped_variant): - return variant_pathogenicity_statement(mapped_variant) - if can_annotate_variant_for_functional_statement(mapped_variant): - return variant_functional_impact_statement(mapped_variant) + if can_annotate_variant_for_pathogenicity_evidence(mapped_variant, principal=principal): + return variant_pathogenicity_statement(mapped_variant, principal=principal) + if can_annotate_variant_for_functional_statement(mapped_variant, principal=principal): + return variant_functional_impact_statement(mapped_variant, principal=principal) return variant_study_result(mapped_variant) except MappingDataDoesntExistException: return None diff --git a/src/mavedb/lib/annotation/util.py b/src/mavedb/lib/annotation/util.py index b8c21515..23ae95b6 100644 --- a/src/mavedb/lib/annotation/util.py +++ b/src/mavedb/lib/annotation/util.py @@ -1,4 +1,4 @@ -from typing import Literal, Optional +from typing import Iterable, Literal, Optional from ga4gh.core.models import Extension from ga4gh.va_spec.base.enums import StrengthOfEvidenceProvided as VaSpecStrengthOfEvidenceProvided @@ -21,12 +21,17 @@ ) from mavedb.lib.annotation.exceptions import MappingDataDoesntExistException from mavedb.lib.mapping import extract_ids_from_post_mapped_metadata +from mavedb.lib.permissions.principal import Principal +from mavedb.lib.permissions.score_calibration import ScoreCalibrationViewer from mavedb.lib.types.annotation import SequenceFeature from mavedb.lib.variants import target_for_variant from mavedb.models.mapped_variant import MappedVariant from mavedb.models.score_calibration import ScoreCalibration from mavedb.models.score_calibration_functional_classification import ScoreCalibrationFunctionalClassification +CALIBRATION_SCOPE_EXTENSION_NAME = "mavedb_calibration_scope" +"""Extension naming the principal an annotation was built for. See ``calibration_scope_extension``.""" + def allele_from_mapped_variant_dictionary_result(allelic_mapping_results: dict) -> Allele: """ @@ -230,40 +235,84 @@ def score_calibration_may_be_used_for_annotation( return True -def _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation( +def calibrations_available_for_annotation( mapped_variant: MappedVariant, annotation_type: Literal["pathogenicity", "functional"], allow_research_use_only_calibrations: bool = False, -) -> bool: + principal: Optional[Principal] = None, +) -> list[ScoreCalibration]: """ - Check if a mapped variant's score set contains any of the required calibrations for annotation. + Select the calibrations on a mapped variant's score set that may build the requested annotation. + + Two independent questions decide this, and they are asked by different collaborators. + - Eligibility: Does this calibration carry the classifications the annotation type needs, and is + its research-use-only standing permitted here. This is ``score_calibration_may_be_used_for_annotation``. + - Visibility: May this principal read it at all. This is the viewer's, because a calibration's READ rule + is stricter than its score set's. Args: - mapped_variant (MappedVariant): The mapped variant object containing the variant with score set data. - annotation_type (Literal["pathogenicity", "functional"]): The type of annotation to check for. - Must be either "pathogenicity" or "functional". - allow_research_use_only_calibrations (bool, optional): Whether to consider calibrations marked as - research use only as valid for annotation. Defaults to False. + mapped_variant (MappedVariant): The mapped variant whose score set's calibrations are considered. + annotation_type (Literal["pathogenicity", "functional"]): The type of annotation to be built. + allow_research_use_only_calibrations (bool, optional): Whether calibrations marked research use + only are eligible. Defaults to False. + principal (Optional[Principal], optional): The caller being served. Defaults to None, an anonymous + caller, so a function that omits it gets public calibrations only. Returns: - bool: True if the variant's score set contains at least one valid calibration with the required - classifications for the specified annotation type. False otherwise. + list[ScoreCalibration]: The eligible, visible calibrations, in score set order. """ - if mapped_variant.variant.score_set.score_calibrations is None: - return False + viewer = (principal if principal is not None else Principal()).viewer_for(ScoreCalibrationViewer) - return any( - score_calibration_may_be_used_for_annotation( + return [ + score_calibration + for score_calibration in viewer.visible(mapped_variant.variant.score_set.score_calibrations) + if score_calibration_may_be_used_for_annotation( score_calibration, annotation_type, allow_research_use_only_calibrations=allow_research_use_only_calibrations, ) - for score_calibration in mapped_variant.variant.score_set.score_calibrations + ] + + +def calibration_scope_extension(calibrations: Iterable[ScoreCalibration]) -> Extension: + """ + Describe the principal an annotation was built for, given the calibrations behind it. + + VA-Spec statements carry no stable identifier, so two callers can receive materially different + statements from the same URL. Naming the scope on the object itself is what keeps that honest: a + consumer holding a record can tell whether it is the one anyone would get, or one widened by the + requester's own access. + + Args: + calibrations (Iterable[ScoreCalibration]): The calibrations contributing evidence to the annotation. + + Returns: + Extension: A ``mavedb_calibration_scope`` extension, ``restricted`` when any contributing + calibration is private and ``public`` otherwise. + """ + if any(calibration.private for calibration in calibrations): + return Extension( + name=CALIBRATION_SCOPE_EXTENSION_NAME, + value="restricted", + description=( + "Built from at least one private score calibration, visible to the requesting viewer. " + "Another viewer requesting this variant may receive fewer evidence lines, or none." + ), + ) + + return Extension( + name=CALIBRATION_SCOPE_EXTENSION_NAME, + value="public", + description=( + "Built only from public score calibrations. Any viewer requesting this variant receives the same evidence." + ), ) def can_annotate_variant_for_pathogenicity_evidence( - mapped_variant: MappedVariant, allow_research_use_only_calibrations=False + mapped_variant: MappedVariant, + allow_research_use_only_calibrations=False, + principal: Optional[Principal] = None, ) -> bool: """ Determine if a mapped variant can be annotated for pathogenicity evidence. @@ -275,6 +324,11 @@ def can_annotate_variant_for_pathogenicity_evidence( Args: mapped_variant (MappedVariant): The mapped variant object to evaluate for pathogenicity evidence annotation eligibility. + allow_research_use_only_calibrations (bool, optional): Whether calibrations marked research use + only are eligible. Defaults to False. + principal (Optional[Principal], optional): The caller being served. Defaults to None, an anonymous + caller. Must match the principal the annotation itself will be built for, or this answers a + different question than the one the caller is about to act on. Returns: bool: True if the variant can be annotated for pathogenicity evidence, @@ -290,16 +344,21 @@ def can_annotate_variant_for_pathogenicity_evidence( """ if not _can_annotate_variant_base_assumptions(mapped_variant): return False - if not _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation( - mapped_variant, "pathogenicity", allow_research_use_only_calibrations=allow_research_use_only_calibrations - ): - return False - return True + return bool( + calibrations_available_for_annotation( + mapped_variant, + "pathogenicity", + allow_research_use_only_calibrations=allow_research_use_only_calibrations, + principal=principal, + ) + ) def can_annotate_variant_for_functional_statement( - mapped_variant: MappedVariant, allow_research_use_only_calibrations=False + mapped_variant: MappedVariant, + allow_research_use_only_calibrations=False, + principal: Optional[Principal] = None, ) -> bool: """ Determine if a mapped variant can be annotated for functional statements. @@ -311,6 +370,11 @@ def can_annotate_variant_for_functional_statement( Args: mapped_variant (MappedVariant): The variant object to check for annotation eligibility, containing mapping information and score data. + allow_research_use_only_calibrations (bool, optional): Whether calibrations marked research use + only are eligible. Defaults to False. + principal (Optional[Principal], optional): The caller being served. Defaults to None, an anonymous + caller. Must match the principal the annotation itself will be built for, or this answers a + different question than the one the caller is about to act on. Returns: bool: True if the variant can be annotated for functional statements, @@ -323,12 +387,15 @@ def can_annotate_variant_for_functional_statement( """ if not _can_annotate_variant_base_assumptions(mapped_variant): return False - if not _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation( - mapped_variant, "functional", allow_research_use_only_calibrations=allow_research_use_only_calibrations - ): - return False - return True + return bool( + calibrations_available_for_annotation( + mapped_variant, + "functional", + allow_research_use_only_calibrations=allow_research_use_only_calibrations, + principal=principal, + ) + ) def sequence_feature_for_mapped_variant(mapped_variant: MappedVariant) -> SequenceFeature: diff --git a/src/mavedb/lib/authorization.py b/src/mavedb/lib/authorization.py index 94f011c9..252febb4 100644 --- a/src/mavedb/lib/authorization.py +++ b/src/mavedb/lib/authorization.py @@ -5,6 +5,7 @@ from mavedb.lib.authentication import get_current_user from mavedb.lib.logging.context import logging_context, save_to_logging_context +from mavedb.lib.permissions.principal import Principal from mavedb.lib.types.authentication import UserData from mavedb.models.enums.user_role import UserRole @@ -26,6 +27,17 @@ async def require_current_user( return user_data +async def get_principal( + user_data: Optional[UserData] = Depends(get_current_user), +) -> Principal: + """The principal for this request, for handlers that fan out to permission-checked sibling entities. + + Resolved through ``Depends`` rather than constructed in each handler because FastAPI caches a + dependency's result for the life of one request. + """ + return Principal(user_data) + + async def require_current_user_with_email( user_data: UserData = Depends(require_current_user), ) -> UserData: diff --git a/src/mavedb/lib/permissions/principal.py b/src/mavedb/lib/permissions/principal.py new file mode 100644 index 00000000..5926b96a --- /dev/null +++ b/src/mavedb/lib/permissions/principal.py @@ -0,0 +1,47 @@ +"""The identity a request acts on behalf of, and the viewers derived from it. + +A ``Principal`` is the single thing worth threading through a fan-out read. It carries the caller rather +than any one entity's viewer, so a function that later needs to filter a second entity type does not grow a +second parameter — it asks the principal for another viewer. + +See ``permissions.viewer`` for what a viewer does, and each entity's permission module for its concrete +viewer. +""" + +from dataclasses import dataclass, field +from typing import Any, Optional, TypeVar + +from mavedb.lib.permissions.viewer import Viewer +from mavedb.lib.types.authentication import UserData + +ViewerT = TypeVar("ViewerT", bound=Viewer[Any]) + + +@dataclass(frozen=True) +class Principal: + """The caller a read is being served. + + Defaults to anonymous, so a caller that constructs one with no arguments serves what any member of the + public could already see rather than everything in the database. + + Viewers are built on first use and kept so that repeated access to the same viewer type does not incur + additional construction overhead or permission checks. + + Request-scoped. Never use a ``Principal`` as a default argument value — Python evaluates defaults once + at import, so the instance, and every cache inside it, would be shared by all requests for the life of + the process. An entity published mid-process would keep its stale verdict, and two callers could be + answered from one another's cache. Take ``Optional[Principal] = None`` and build one when it is missing. + ``test_principal.py`` enforces this by inspection. + """ + + user_data: Optional[UserData] = None + + _viewers: dict[type, Viewer[Any]] = field(default_factory=dict, compare=False, repr=False) + + def viewer_for(self, viewer_class: type[ViewerT]) -> ViewerT: + """The viewer of the given type for this caller, built once and reused.""" + if viewer_class not in self._viewers: + self._viewers[viewer_class] = viewer_class(self.user_data) + + # The dict is heterogeneous by design; the key recovers the value's type. + return self._viewers[viewer_class] # type: ignore[return-value] diff --git a/src/mavedb/lib/permissions/score_calibration.py b/src/mavedb/lib/permissions/score_calibration.py index 1aa71158..86b404f5 100644 --- a/src/mavedb/lib/permissions/score_calibration.py +++ b/src/mavedb/lib/permissions/score_calibration.py @@ -1,9 +1,11 @@ +from dataclasses import dataclass from typing import Optional from mavedb.lib.logging.context import save_to_logging_context from mavedb.lib.permissions.actions import Action from mavedb.lib.permissions.models import PermissionResponse from mavedb.lib.permissions.utils import deny_action_for_entity, roles_permitted +from mavedb.lib.permissions.viewer import Viewer from mavedb.lib.types.authentication import UserData from mavedb.models.enums.user_role import UserRole from mavedb.models.score_calibration import ScoreCalibration @@ -78,6 +80,20 @@ def has_permission(user_data: Optional[UserData], entity: ScoreCalibration, acti ) +@dataclass(frozen=True) +class ScoreCalibrationViewer(Viewer[ScoreCalibration]): + """The audience a calibration-bearing export is being built for. + + Needed wherever a read fans out to calibrations, because a calibration's READ rule is stricter than its + score set's: publishing a score set does not publish its calibrations, and reading one does not entitle + a caller to read its private calibrations. + """ + + @staticmethod + def _has_permission(user_data: Optional[UserData], entity: ScoreCalibration, action: Action) -> PermissionResponse: + return has_permission(user_data, entity, action) + + def _handle_read_action( user_data: Optional[UserData], entity: ScoreCalibration, diff --git a/src/mavedb/lib/permissions/viewer.py b/src/mavedb/lib/permissions/viewer.py new file mode 100644 index 00000000..d9f6eea4 --- /dev/null +++ b/src/mavedb/lib/permissions/viewer.py @@ -0,0 +1,86 @@ +"""What one entity type's rules permit a given caller to read. + +Router-boundary ``assert_permission`` covers the entity a request names. A path that fans out from there +to sibling entities — a score set to its calibrations, a variant to every score set measuring the same +allele — leaves that boundary behind, and nothing in a function signature says so. A viewer is what lets +"may this caller see this?" be asked at the point of fan-out, rather than assumed to have been asked +upstream. + +This module holds only the entity-agnostic behaviour. Each entity's concrete viewer lives beside that +entity's permission rules — see ``ScoreCalibrationViewer`` in ``permissions.score_calibration``. Callers +do not usually construct a viewer directly; they thread a ``Principal`` and ask it for one (see +``permissions.principal``). + +Known limitation: a viewer filters entities that have already been loaded, which is correct but leaves two +gaps. It cannot constrain values *derived* from entities. A count, or a "has any calibration" boolean, +bypasses it entirely. In addition, filtering by reassigning an ORM collection is undone by any later eager load of +that relationship. The durable fix for the second is a composable SQL predicate (a reusable WHERE clause +rather than a sealed loader, so queries keep their joins), which is worth building once a second entity +needs it. +""" + +from abc import ABC, abstractmethod +from dataclasses import dataclass, field +from typing import Generic, Iterable, Optional, TypeVar + +from mavedb.lib.permissions.actions import Action +from mavedb.lib.permissions.models import PermissionResponse +from mavedb.lib.types.authentication import UserData + +EntityT = TypeVar("EntityT") + + +@dataclass(frozen=True) +class Viewer(ABC, Generic[EntityT]): + """One entity type's read rules, bound to a caller. + + Defaults to anonymous, so a viewer constructed with no arguments admits only what any member of the + public could already see rather than everything in the database. + """ + + user_data: Optional[UserData] = None + + _readable: dict[int, bool] = field(default_factory=dict, compare=False, repr=False) + """Memoized READ answers, keyed by entity id. + + A fan-out re-asks the same handful of entities repeatedly. The answer cannot change within a request, + so it is asked once. Instances are therefore request-scoped: do not share one across requests, or an + entity published mid-process would keep its stale answer. + """ + + @staticmethod + @abstractmethod + def _has_permission(user_data: Optional[UserData], entity: EntityT, action: Action) -> PermissionResponse: + """The permission rules for this entity type. Bound to the entity's own permission module.""" + + def _is_indeterminate(self, entity: EntityT) -> bool: + """Whether the entity cannot state its own visibility, and so must be withheld. + + Permission handlers raise on an unset ``private`` flag, and a raising permission check inside a + streaming generator surfaces to the user as a truncated download rather than a denial. Withholding + is the safe reading of "I don't know". + """ + return getattr(entity, "private", False) is None + + def may_read(self, entity: EntityT) -> bool: + """Whether this viewer is permitted to read an entity.""" + if self._is_indeterminate(entity): + return False + + entity_id = getattr(entity, "id", None) + + # Every unsaved entity shares a null id, so caching one verdict would apply it to all of them. + if entity_id is None: + return self._has_permission(self.user_data, entity, Action.READ).permitted + + if entity_id not in self._readable: + self._readable[entity_id] = self._has_permission(self.user_data, entity, Action.READ).permitted + + return self._readable[entity_id] + + def visible(self, entities: Optional[Iterable[EntityT]]) -> list[EntityT]: + """Drop the entities this viewer may not read.""" + if not entities: + return [] + + return [entity for entity in entities if self.may_read(entity)] diff --git a/src/mavedb/routers/experiment_sets.py b/src/mavedb/routers/experiment_sets.py index 6bc5214c..3f72b8ad 100644 --- a/src/mavedb/routers/experiment_sets.py +++ b/src/mavedb/routers/experiment_sets.py @@ -59,16 +59,21 @@ def fetch_experiment_set( # error otherwise. logger.debug(msg="The requested resources does not exist.", extra=logging_context()) raise HTTPException(status_code=404, detail=f"experiment set with URN {urn} not found") - else: - item.experiments.sort(key=attrgetter("urn")) assert_permission(user_data, item, Action.READ) - # Filter experiment sub-resources to only those experiments readable by the requesting user. - item.experiments[:] = [exp for exp in item.experiments if has_permission(user_data, exp, Action.READ).permitted] - enriched_experiments = [enrich_experiment_with_num_score_sets(exp, user_data) for exp in item.experiments] - enriched_item = experiment_set.ExperimentSet.model_validate(item).copy( - update={"experiments": enriched_experiments, "num_experiments": len(enriched_experiments)} + # Narrow to the experiments this caller may read, without touching item.experiments. + # ExperimentSet.experiments is mapped with cascade="all, delete-orphan": removing members from the ORM + # collection marks them as orphans, and the next flush deletes those experiments along with their score + # sets and variants. Only autoflush=False and the absence of a commit on this path made that survivable. + readable_experiments = sorted( + (experiment for experiment in item.experiments if has_permission(user_data, experiment, Action.READ).permitted), + key=attrgetter("urn"), ) + enriched_experiments = [ + enrich_experiment_with_num_score_sets(experiment, user_data) for experiment in readable_experiments + ] - return enriched_item + return experiment_set.ExperimentSet.model_validate(item).copy( + update={"experiments": enriched_experiments, "num_experiments": len(enriched_experiments)} + ) diff --git a/src/mavedb/routers/experiments.py b/src/mavedb/routers/experiments.py index 66ce079e..debaebcb 100644 --- a/src/mavedb/routers/experiments.py +++ b/src/mavedb/routers/experiments.py @@ -10,7 +10,7 @@ from mavedb import deps from mavedb.lib.authentication import get_current_user -from mavedb.lib.authorization import require_current_user, require_current_user_with_email +from mavedb.lib.authorization import get_principal, require_current_user, require_current_user_with_email from mavedb.lib.contributors import find_or_create_contributor from mavedb.lib.exceptions import NonexistentOrcidUserError from mavedb.lib.experiments import enrich_experiment_with_num_score_sets @@ -24,6 +24,8 @@ from mavedb.lib.logging import LoggedRoute from mavedb.lib.logging.context import logging_context, save_to_logging_context from mavedb.lib.permissions import Action, assert_permission, has_permission +from mavedb.lib.permissions.principal import Principal +from mavedb.lib.permissions.score_calibration import ScoreCalibrationViewer from mavedb.lib.score_sets import find_superseded_score_set_tail from mavedb.lib.types.authentication import UserData from mavedb.lib.validation.exceptions import ValidationError @@ -175,6 +177,7 @@ def get_experiment_score_sets( urn: str, db: Session = Depends(deps.get_db), user_data: Optional[UserData] = Depends(get_current_user), + principal: Principal = Depends(get_principal), ) -> Any: """ Get all score sets belonging to an experiment. @@ -215,10 +218,26 @@ def get_experiment_score_sets( filtered_score_sets.sort(key=attrgetter("urn")) save_to_logging_context({"associated_resources": [item.urn for item in score_set_result]}) + # A calibration's READ rule is stricter than its score set's, so a published score set can carry + # calibrations this caller may not see. Filtered on the serialized view rather than by reassigning + # ScoreSet.score_calibrations, whose delete-orphan cascade would mark the withheld rows for deletion. + viewer = principal.viewer_for(ScoreCalibrationViewer) + enriched_score_sets = [] for fs in filtered_score_sets: enriched_experiment = enrich_experiment_with_num_score_sets(fs.experiment, user_data) - response_item = score_set.ScoreSet.model_validate(fs).copy(update={"experiment": enriched_experiment}) + visible_calibration_ids = {calibration.id for calibration in viewer.visible(fs.score_calibrations)} + validated_item = score_set.ScoreSet.model_validate(fs) + response_item = validated_item.copy( + update={ + "experiment": enriched_experiment, + "score_calibrations": [ + calibration + for calibration in (validated_item.score_calibrations or []) + if calibration.id in visible_calibration_ids + ], + } + ) enriched_score_sets.append(response_item) return enriched_score_sets diff --git a/src/mavedb/routers/mapped_variant.py b/src/mavedb/routers/mapped_variant.py index 7b97b304..f6bfb7f9 100644 --- a/src/mavedb/routers/mapped_variant.py +++ b/src/mavedb/routers/mapped_variant.py @@ -17,8 +17,9 @@ variant_study_result, ) from mavedb.lib.annotation.exceptions import MappingDataDoesntExistException -from mavedb.lib.authorization import get_current_user +from mavedb.lib.authorization import get_current_user, get_principal from mavedb.lib.logging import LoggedRoute +from mavedb.lib.permissions.principal import Principal from mavedb.lib.logging.context import ( logging_context, save_to_logging_context, @@ -137,7 +138,11 @@ async def show_mapped_variant_study_result( summary="Construct a VA-Spec Statement from a mapped variant", ) async def show_mapped_variant_functional_impact_statement( - *, urn: str, db: Session = Depends(deps.get_db), user: Optional[UserData] = Depends(get_current_user) + *, + urn: str, + db: Session = Depends(deps.get_db), + user: Optional[UserData] = Depends(get_current_user), + principal: Principal = Depends(get_principal), ) -> Statement: """ Construct a single VA-Spec Statement from a mapped variant by URN. @@ -147,7 +152,7 @@ async def show_mapped_variant_functional_impact_statement( mapped_variant = await fetch_mapped_variant_by_variant_urn(db, user, urn) try: - functional_impact = variant_functional_impact_statement(mapped_variant) + functional_impact = variant_functional_impact_statement(mapped_variant, principal=principal) except MappingDataDoesntExistException as e: logger.info( msg="Could not construct a functional impact statement for this mapped variant; No mapping data exists for this score set.", @@ -179,7 +184,11 @@ async def show_mapped_variant_functional_impact_statement( summary="Construct a VA-Spec EvidenceLine from a mapped variant", ) async def show_mapped_variant_acmg_evidence_line( - *, urn: str, db: Session = Depends(deps.get_db), user: Optional[UserData] = Depends(get_current_user) + *, + urn: str, + db: Session = Depends(deps.get_db), + user: Optional[UserData] = Depends(get_current_user), + principal: Principal = Depends(get_principal), ) -> VariantPathogenicityStatement: """ Construct a list of VA-Spec EvidenceLine(s) from a mapped variant by URN. @@ -189,7 +198,7 @@ async def show_mapped_variant_acmg_evidence_line( mapped_variant = await fetch_mapped_variant_by_variant_urn(db, user, urn) try: - pathogenicity_statement = variant_pathogenicity_statement(mapped_variant) + pathogenicity_statement = variant_pathogenicity_statement(mapped_variant, principal=principal) except MappingDataDoesntExistException as e: logger.info( msg="Could not construct a pathogenicity statement for this mapped variant; No mapping data exists for this score set.", diff --git a/src/mavedb/routers/score_sets.py b/src/mavedb/routers/score_sets.py index 371862d1..4d1a30ad 100644 --- a/src/mavedb/routers/score_sets.py +++ b/src/mavedb/routers/score_sets.py @@ -3,6 +3,7 @@ import logging import time from datetime import date, datetime +from functools import partial from typing import Any, List, Optional, Sequence, TypedDict, Union import numpy as np @@ -17,7 +18,7 @@ from ga4gh.va_spec.base.core import ExperimentalVariantFunctionalImpactStudyResult, Statement from pydantic import ValidationError from sqlalchemy import or_, select -from sqlalchemy.exc import MultipleResultsFound, IntegrityError +from sqlalchemy.exc import IntegrityError, MultipleResultsFound from sqlalchemy.orm import Session, contains_eager from mavedb import deps @@ -30,6 +31,7 @@ from mavedb.lib.annotation.exceptions import MappingDataDoesntExistException from mavedb.lib.authorization import ( get_current_user, + get_principal, require_current_user, require_current_user_with_email, ) @@ -48,6 +50,8 @@ save_to_logging_context, ) from mavedb.lib.permissions import Action, assert_permission, has_permission +from mavedb.lib.permissions.principal import Principal +from mavedb.lib.permissions.score_calibration import ScoreCalibrationViewer from mavedb.lib.score_calibrations import create_score_calibration from mavedb.lib.score_sets import ( CLINVAR_NS_PATTERN, @@ -579,7 +583,7 @@ async def fetch_score_set_by_urn( try: query = db.query(ScoreSet).filter(ScoreSet.urn == urn) if owner_or_contributor is not None: - query.filter( + query = query.filter( or_( ScoreSet.private.is_(False), ScoreSet.created_by_id == owner_or_contributor.user.id, @@ -587,7 +591,7 @@ async def fetch_score_set_by_urn( ) ) if only_published: - query.filter(ScoreSet.private.is_(False)) + query = query.filter(ScoreSet.private.is_(False)) item = query.one_or_none() except MultipleResultsFound: logger.info( @@ -601,12 +605,62 @@ async def fetch_score_set_by_urn( assert_permission(user, item, Action.READ) - if item.superseding_score_set and not has_permission(user, item.superseding_score_set, Action.READ).permitted: - item.superseding_score_set = None + # Narrowing what the score set carries belongs to _score_set_response, so that this function's other + # callers -- supersession lookup, publication -- receive the score set as it actually is. + return item - item.score_calibrations = [sc for sc in item.score_calibrations if has_permission(user, sc, Action.READ).permitted] - return item +def _score_set_response(item: ScoreSet, principal: Principal) -> score_set.ScoreSet: + """ + Serialize a score set for a response, withholding the sub-resources this caller may not read. + + Every route in this module that returns a ``ScoreSet`` view model builds it here. The two sub-resources + a score set carries have READ rules stricter than its own, and each was leaked from a different route + before this was centralized: + + - Calibrations. Publishing a score set does not publish its calibrations, and owning a score set does + not entitle its owner to a community calibration someone else attached to it. + - The superseding score set, which is usually still private while the score set it replaces is public. + + The search routes are the deliberate exception: they answer with ``ShortScoreSet``, which carries neither + sub-resource, so there is nothing for this function to narrow. Any route that widens its response model + to ``ScoreSet`` must come through here. + + Local to this module by design. A shared response constructor was considered and deferred: the same ORM + graph is also serialized as CSV, VA-Spec NDJSON and ScoreSetPublicDump, none of which such a constructor + would cover, so the durable guarantee belongs at the session rather than the response layer. + + Narrowing is applied to the validated view. ``ScoreSet.score_calibrations`` is mapped with + ``cascade="all, delete-orphan"``, and assigning ``superseding_score_set = None`` nulls the other score + set's ``replaces_id``; narrowing the ORM objects instead stages both as writes. + + Args: + item (ScoreSet): The score set to serialize. Asserting READ on the score set itself belongs to the + caller. + principal (Principal): The caller being served. + + Returns: + score_set.ScoreSet: The score set view model, carrying only what this caller may read. + """ + visible_calibration_ids = { + calibration.id for calibration in principal.viewer_for(ScoreCalibrationViewer).visible(item.score_calibrations) + } + superseding_is_visible = item.superseding_score_set is not None and ( + has_permission(principal.user_data, item.superseding_score_set, Action.READ).permitted + ) + + validated_item = score_set.ScoreSet.model_validate(item) + return validated_item.model_copy( + update={ + "experiment": enrich_experiment_with_num_score_sets(item.experiment, principal.user_data), + "score_calibrations": [ + calibration + for calibration in (validated_item.score_calibrations or []) + if calibration.id in visible_calibration_ids + ], + "superseding_score_set": validated_item.superseding_score_set if superseding_is_visible else None, + } + ) router = APIRouter( @@ -781,6 +835,7 @@ def list_recently_published_score_sets( ), db: Session = Depends(deps.get_db), user_data: Optional[UserData] = Depends(get_current_user), + principal: Principal = Depends(get_principal), ) -> Any: """ Return the most recently published score sets, ordered by publication date descending. @@ -795,19 +850,9 @@ def list_recently_published_score_sets( .all() ) - result = [] - for item in items: - if not has_permission(user_data, item, Action.READ).permitted: - continue - if ( - item.superseding_score_set - and not has_permission(user_data, item.superseding_score_set, Action.READ).permitted - ): - item.superseding_score_set = None - enriched_experiment = enrich_experiment_with_num_score_sets(item.experiment, user_data) - result.append(score_set.ScoreSet.model_validate(item).copy(update={"experiment": enriched_experiment})) - - return result + return [ + _score_set_response(item, principal) for item in items if has_permission(user_data, item, Action.READ).permitted + ] @router.get( @@ -823,6 +868,7 @@ async def show_score_sets( urns: str = Query(..., description="Comma-separated list of score set URNs"), db: Session = Depends(deps.get_db), user_data: UserData = Depends(get_current_user), + principal: Principal = Depends(get_principal), ) -> Any: """ Fetch score sets identified by a list of URNs. @@ -835,9 +881,7 @@ async def show_score_sets( response_items: list[score_set.ScoreSet] = [] for urn in urn_list: item = await fetch_score_set_by_urn(db, urn, user_data, None, False) - enriched_experiment = enrich_experiment_with_num_score_sets(item.experiment, user_data) - response_item = score_set.ScoreSet.model_validate(item).copy(update={"experiment": enriched_experiment}) - response_items.append(response_item) + response_items.append(_score_set_response(item, principal)) return response_items @@ -855,14 +899,14 @@ async def show_score_set( urn: str, db: Session = Depends(deps.get_db), user_data: UserData = Depends(get_current_user), + principal: Principal = Depends(get_principal), ) -> Any: """ Fetch a single score set by URN. """ save_to_logging_context({"requested_resource": urn}) item = await fetch_score_set_by_urn(db, urn, user_data, None, False) - enriched_experiment = enrich_experiment_with_num_score_sets(item.experiment, user_data) - return score_set.ScoreSet.model_validate(item).copy(update={"experiment": enriched_experiment}) + return _score_set_response(item, principal) @router.get( @@ -1230,6 +1274,7 @@ def get_score_set_annotated_variants( urn: str, db: Session = Depends(deps.get_db), user_data: Optional[UserData] = Depends(get_current_user), + principal: Principal = Depends(get_principal), ) -> Any: """ Retrieve annotated variants with pathogenicity statements for a given score set. @@ -1299,7 +1344,7 @@ def get_score_set_annotated_variants( ) return StreamingResponse( - _stream_generated_annotations(mapped_variants, variant_pathogenicity_statement), + _stream_generated_annotations(mapped_variants, partial(variant_pathogenicity_statement, principal=principal)), media_type="application/x-ndjson", headers={ "X-Total-Count": str(len(mapped_variants)), @@ -1329,6 +1374,7 @@ def get_score_set_annotated_variants_functional_statement( urn: str, db: Session = Depends(deps.get_db), user_data: Optional[UserData] = Depends(get_current_user), + principal: Principal = Depends(get_principal), ): """ Retrieve functional impact statements for annotated variants in a score set. @@ -1391,7 +1437,9 @@ def get_score_set_annotated_variants_functional_statement( ) return StreamingResponse( - _stream_generated_annotations(mapped_variants, variant_functional_impact_statement), + _stream_generated_annotations( + mapped_variants, partial(variant_functional_impact_statement, principal=principal) + ), media_type="application/x-ndjson", headers={ "X-Total-Count": str(len(mapped_variants)), @@ -1510,6 +1558,7 @@ async def create_score_set( item_create: score_set.ScoreSetCreate, db: Session = Depends(deps.get_db), user_data: UserData = Depends(require_current_user_with_email), + principal: Principal = Depends(get_principal), ) -> Any: """ Create a score set. @@ -1843,8 +1892,7 @@ async def create_score_set( save_to_logging_context({"created_resource": item.urn}) - enriched_experiment = enrich_experiment_with_num_score_sets(item.experiment, user_data) - return score_set.ScoreSet.model_validate(item).copy(update={"experiment": enriched_experiment}) + return _score_set_response(item, principal) @router.post( @@ -1900,6 +1948,7 @@ async def upload_score_set_variant_data( db: Session = Depends(deps.get_db), user_data: UserData = Depends(require_current_user_with_email), worker: ArqRedis = Depends(deps.get_worker), + principal: Principal = Depends(get_principal), ) -> Any: """ Upload scores and variant count files for a score set, and initiate processing these files to @@ -1976,8 +2025,7 @@ async def upload_score_set_variant_data( db.commit() db.refresh(item) - enriched_experiment = enrich_experiment_with_num_score_sets(item.experiment, user_data) - return score_set.ScoreSet.model_validate(item).copy(update={"experiment": enriched_experiment}) + return _score_set_response(item, principal) @router.patch( @@ -2032,6 +2080,7 @@ async def update_score_set_with_variants( db: Session = Depends(deps.get_db), user_data: UserData = Depends(require_current_user_with_email), worker: ArqRedis = Depends(deps.get_worker), + principal: Principal = Depends(get_principal), ) -> Any: """ Update a score set and variants. @@ -2168,8 +2217,7 @@ async def update_score_set_with_variants( db.commit() db.refresh(updatedItem) - enriched_experiment = enrich_experiment_with_num_score_sets(updatedItem.experiment, user_data) - return score_set.ScoreSet.model_validate(updatedItem).copy(update={"experiment": enriched_experiment}) + return _score_set_response(updatedItem, principal) @router.put( @@ -2186,6 +2234,7 @@ async def update_score_set( db: Session = Depends(deps.get_db), user_data: UserData = Depends(require_current_user_with_email), worker: ArqRedis = Depends(deps.get_worker), + principal: Principal = Depends(get_principal), ) -> Any: """ Update a score set. @@ -2245,8 +2294,7 @@ async def update_score_set( db.commit() db.refresh(updatedItem) - enriched_experiment = enrich_experiment_with_num_score_sets(updatedItem.experiment, user_data) - return score_set.ScoreSet.model_validate(updatedItem).copy(update={"experiment": enriched_experiment}) + return _score_set_response(updatedItem, principal) @router.delete( @@ -2300,6 +2348,7 @@ async def publish_score_set( db: Session = Depends(deps.get_db), user_data: UserData = Depends(require_current_user), worker: ArqRedis = Depends(deps.get_worker), + principal: Principal = Depends(get_principal), ) -> Any: """ Publish a score set. @@ -2391,8 +2440,7 @@ async def publish_score_set( ) send_slack_error(err=exc) - enriched_experiment = enrich_experiment_with_num_score_sets(item.experiment, user_data) - return score_set.ScoreSet.model_validate(item).copy(update={"experiment": enriched_experiment}) + return _score_set_response(item, principal) @router.get( diff --git a/src/mavedb/scripts/export_public_data.py b/src/mavedb/scripts/export_public_data.py index 4ced338a..3ce31a2b 100644 --- a/src/mavedb/scripts/export_public_data.py +++ b/src/mavedb/scripts/export_public_data.py @@ -18,7 +18,7 @@ import os from datetime import datetime, timezone from itertools import chain -from typing import Callable, Iterable, TypeVar +from typing import Callable, Iterable, Optional, TypeVar from zipfile import ZipFile from fastapi.encoders import jsonable_encoder @@ -26,6 +26,8 @@ from sqlalchemy.orm import Session, joinedload, lazyload from mavedb.lib.annotation.annotate import variant_highest_level_annotation +from mavedb.lib.permissions.principal import Principal +from mavedb.lib.permissions.score_calibration import ScoreCalibrationViewer from mavedb.lib.score_sets import get_current_mapped_variants_for_annotation, get_score_set_variants_as_csv from mavedb.models.experiment import Experiment from mavedb.models.experiment_set import ExperimentSet @@ -43,41 +45,55 @@ T = TypeVar("T") -def filter_experiment_sets(experiment_sets: Iterable[ExperimentSet]) -> Iterable[ExperimentSet]: - """ - Filter a list of experiment sets. Exclude any experiments with no score sets, then exclude experiment sets with no - experiments. - - Filtering is done on the basis of the current contents of Experiment.score_set, which will have been loaded using a - query that excludes unpublished score sets and those licensed other than under CC0. - """ - return filter(filter_experiment_set, experiment_sets) +def flatmap(f: Callable[[S], Iterable[T]], items: Iterable[S]) -> Iterable[T]: + return chain.from_iterable(map(f, items)) -def filter_experiment_set(experiment_set: ExperimentSet): +def public_experiment_set( + experiment_set_view: ExperimentSetPublicDump, visible_calibration_ids: set[int] +) -> Optional[ExperimentSetPublicDump]: """ - Filter an experiment set. Exclude any experiments it contains that do not contain score sets, and return a value - indicating whether any experiments remain. + Narrow a validated experiment set to what belongs in the public dump. - Filtering is done on the basis of the current contents of Experiment.score_set, which will have been loaded using a - query that excludes unpublished score sets and those licensed other than under CC0. - """ - experiment_set.experiments = list(filter_experiments(experiment_set.experiments)) - return len(experiment_set.experiments) > 0 + Drops calibrations an anonymous caller may not read, then experiments left with no score sets, and + returns None for an experiment set left with no experiments. The score sets themselves need no filter: + the loading query already restricts them to published, CC0-licensed ones. + Narrowing the validated view rather than the ORM graph is deliberate. ``ExperimentSet.experiments`` and + ``ScoreSet.score_calibrations`` are both mapped with ``cascade="all, delete-orphan"``, so removing a + member from either ORM collection marks the removed row as an orphan and the next flush deletes it. + This script can flush: ``with_database_session`` commits when invoked with ``--commit``. -def filter_experiments(experiments: Iterable[Experiment]) -> Iterable[Experiment]: - """ - Filter a list of experiments, excluding any whose score_sets collection is empty. + Args: + experiment_set_view (ExperimentSetPublicDump): The validated experiment set to narrow. + visible_calibration_ids (set[int]): Ids of the calibrations an anonymous caller may read. - Filtering is done on the basis of the current contents of score_sets, which will have been loaded using a query that - excludes unpublished score sets and those licensed other than under CC0. + Returns: + Optional[ExperimentSetPublicDump]: The narrowed experiment set, or None if nothing public remains. """ - return filter(lambda e: len(e.score_sets) > 0, experiments) + experiments = [] + for experiment_view in experiment_set_view.experiments: + if not experiment_view.score_sets: + continue + + score_sets = [ + score_set_view.model_copy( + update={ + "score_calibrations": [ + calibration + for calibration in (score_set_view.score_calibrations or []) + if calibration.id in visible_calibration_ids + ] + } + ) + for score_set_view in experiment_view.score_sets + ] + experiments.append(experiment_view.model_copy(update={"score_sets": score_sets})) + if not experiments: + return None -def flatmap(f: Callable[[S], Iterable[T]], items: Iterable[S]) -> Iterable[T]: - return chain.from_iterable(map(f, items)) + return experiment_set_view.model_copy(update={"experiments": experiments}) @script_environment.command() @@ -99,26 +115,51 @@ def export_public_data(db: Session): .order_by(ExperimentSet.urn) ) - # Filter the stream of experiment sets to exclude experiments and experiment sets with no public, CC0-licensed score - # sets. - experiment_sets = list(filter_experiment_sets(experiment_sets_query.all())) - logger.info(f"Found {len(experiment_sets)} published experiment sets with CC0-licensed score sets.") + experiment_sets = experiment_sets_query.all() + + # The dump is built for an anonymous principal. Publishing a score set does not publish its + # calibrations: a calibration keeps its own `private` flag and a stricter READ rule, so every artifact + # below is scoped to what this viewer may read. + public_principal = Principal() + public_viewer = public_principal.viewer_for(ScoreCalibrationViewer) + all_calibrations = [ + calibration + for score_set_orm in flatmap(lambda es: flatmap(lambda e: e.score_sets, es.experiments), experiment_sets) + for calibration in (score_set_orm.score_calibrations or []) + ] + + # TODO(#372): Nullable ids. + visible_calibration_ids: set[int] = {calibration.id for calibration in public_viewer.visible(all_calibrations)} # type: ignore + if len(all_calibrations) > len(visible_calibration_ids): + logger.info( + f"Withholding {len(all_calibrations) - len(visible_calibration_ids)} non-public score " + "calibration(s) from the dump." + ) # TODO To support very large data sets, we may want to use custom code for JSON-encoding an iterator. # Issue: https://github.com/VariantEffect/mavedb-api/issues/192 # See, for instance, https://stackoverflow.com/questions/12670395/json-encoding-very-long-iterators. - experiment_set_views = list(map(lambda es: ExperimentSetPublicDump.model_validate(es), experiment_sets)) + experiment_set_views = [ + narrowed + for narrowed in ( + public_experiment_set(ExperimentSetPublicDump.model_validate(es), visible_calibration_ids) + for es in experiment_sets + ) + if narrowed is not None + ] + logger.info(f"Found {len(experiment_set_views)} published experiment sets with CC0-licensed score sets.") - # Get a list of IDS of all the score sets included. - score_set_ids = list( - flatmap(lambda es: flatmap(lambda e: map(lambda ss: ss.id, e.score_sets), es.experiments), experiment_sets) + score_set_urns = list( + flatmap( + lambda es: flatmap(lambda e: map(lambda ss: ss.urn, e.score_sets), es.experiments), experiment_set_views + ) ) timestamp_format = "%Y%m%d%H%M%S" zip_file_name = f"mavedb-dump.{datetime.now().strftime(timestamp_format)}.zip" - logger.info(f"Writing {zip_file_name} with {len(score_set_ids)} score sets.") + logger.info(f"Writing {zip_file_name} with {len(score_set_urns)} score sets.") json_data = { "title": "MaveDB public data", "asOf": datetime.now(timezone.utc).isoformat(), @@ -135,12 +176,12 @@ def export_public_data(db: Session): zipfile.write(os.path.join(resources_dir, "README.md"), "README.md") # Write score and count files for each score set. - num_score_sets = len(score_set_ids) - for i, score_set_id in enumerate(score_set_ids): - score_set = db.scalars(select(ScoreSet).where(ScoreSet.id == score_set_id)).one_or_none() - if score_set is not None and score_set.urn is not None: - logger.info(f"[{i + 1}/{num_score_sets}] Exporting score set {score_set.urn}") - csv_filename_base = score_set.urn.replace(":", "-") + num_score_sets = len(score_set_urns) + for i, score_set_urn in enumerate(score_set_urns): + score_set = db.scalars(select(ScoreSet).where(ScoreSet.urn == score_set_urn)).one_or_none() + if score_set is not None: + logger.info(f"[{i + 1}/{num_score_sets}] Exporting score set {score_set_urn}") + csv_filename_base = score_set_urn.replace(":", "-") csv_str = get_score_set_variants_as_csv(db, score_set, ["scores"], namespaced=True) zipfile.writestr(f"csv/{csv_filename_base}.scores.csv", csv_str) @@ -151,7 +192,7 @@ def export_public_data(db: Session): has_annotations = ( db.scalars( select(ScoreSet) - .where(ScoreSet.id == score_set_id) + .where(ScoreSet.id == score_set.id) .join(Variant) .join(MappedVariant) .where(MappedVariant.current.is_(True)) @@ -190,7 +231,7 @@ def export_public_data(db: Session): select(MappedVariant) .join(Variant, Variant.id == MappedVariant.variant_id) .options(joinedload(MappedVariant.variant)) - .where(Variant.score_set_id == score_set_id) + .where(Variant.score_set_id == score_set.id) .where(MappedVariant.current.is_(True)) ).all() mapped_variant_views = [ @@ -211,7 +252,7 @@ def export_public_data(db: Session): va_lines = [] num_annotations = 0 for mv in annotated_variants: - annotation = variant_highest_level_annotation(mv) + annotation = variant_highest_level_annotation(mv, principal=public_principal) if annotation is not None: num_annotations += 1 record = { diff --git a/tests/helpers/constants.py b/tests/helpers/constants.py index bf78d38f..d582312b 100644 --- a/tests/helpers/constants.py +++ b/tests/helpers/constants.py @@ -55,6 +55,8 @@ VALID_MD5_DIGEST = "01234abcde%" VALID_VMC_DIGEST = "GS_ASNKvN4=%" +PRIVATE_CALIBRATION_OWNER_ID = 42 + TEST_SEQREPO_INITIAL_STATE = [ {f"refseq:{VALID_ACCESSION}": {"seq_id": "seq1", "seq": "AAAA", "namespace": "refseq", "alias": VALID_ACCESSION}}, {f"MD5:{VALID_MD5_DIGEST}": {"seq_id": "seq2", "seq": "CCCC", "namespace": "MD5", "alias": VALID_MD5_DIGEST}}, diff --git a/tests/lib/annotation/conftest.py b/tests/lib/annotation/conftest.py index 851f6fcf..29a056c6 100644 --- a/tests/lib/annotation/conftest.py +++ b/tests/lib/annotation/conftest.py @@ -5,14 +5,36 @@ including mock objects with proper calibrations and configurations. """ +from unittest.mock import Mock + import pytest +from tests.helpers.constants import PRIVATE_CALIBRATION_OWNER_ID from tests.helpers.mocks.factories import ( create_mock_mapped_variant, create_mock_mapped_variant_with_functional_calibration_score_set, create_mock_mapped_variant_with_pathogenicity_calibration_score_set, ) +# Permission related helpers coupled to logging context. +try: + from .conftest_optional import * # noqa: F403 +except ImportError: + pass + + +def make_private(mapped_variant, *, owner_id: int = PRIVATE_CALIBRATION_OWNER_ID): + """Mark every calibration on a mapped variant's score set private, owned by ``owner_id``. + + The real permission check reads ``created_by_id`` and the owning score set's contributor list, neither + of which the annotation mocks populate. + """ + for calibration in mapped_variant.variant.score_set.score_calibrations: + calibration.private = True + calibration.created_by_id = owner_id + calibration.score_set = Mock(contributors=[], created_by_id=owner_id, modified_by_id=owner_id) + return mapped_variant + @pytest.fixture def mock_mapped_variant(): diff --git a/tests/lib/annotation/conftest_optional.py b/tests/lib/annotation/conftest_optional.py new file mode 100644 index 00000000..9b536556 --- /dev/null +++ b/tests/lib/annotation/conftest_optional.py @@ -0,0 +1,13 @@ +from unittest.mock import Mock + +from mavedb.lib.permissions.principal import Principal +from mavedb.models.enums.user_role import UserRole +from tests.helpers.constants import PRIVATE_CALIBRATION_OWNER_ID + + +def admin_principal() -> Principal: + return Principal(Mock(user=Mock(id=1, username="admin"), active_roles=[UserRole.admin])) + + +def owner_principal(owner_id: int = PRIVATE_CALIBRATION_OWNER_ID) -> Principal: + return Principal(Mock(user=Mock(id=owner_id, username="owner"), active_roles=[])) diff --git a/tests/lib/annotation/test_annotate.py b/tests/lib/annotation/test_annotate.py index b05c2c18..0d0b091f 100644 --- a/tests/lib/annotation/test_annotate.py +++ b/tests/lib/annotation/test_annotate.py @@ -12,6 +12,7 @@ import pytest pytest.importorskip("psycopg2") +pytest.importorskip("fastapi") from mavedb.lib.annotation.annotate import ( variant_functional_impact_statement, @@ -19,6 +20,19 @@ variant_pathogenicity_statement, variant_study_result, ) +from mavedb.lib.annotation.util import CALIBRATION_SCOPE_EXTENSION_NAME +from tests.lib.annotation.conftest import admin_principal, make_private, owner_principal + + +def scope_of(annotation) -> str: + """The disclosed principal of an annotation, which every emitted object must carry.""" + scopes = [ + extension.value + for extension in (annotation.extensions or []) + if extension.name == CALIBRATION_SCOPE_EXTENSION_NAME + ] + assert len(scopes) == 1, f"expected exactly one calibration scope extension, found {scopes}" + return scopes[0] @pytest.mark.unit @@ -32,6 +46,11 @@ def test_variant_study_result_creates_valid_result(self, mock_mapped_variant): assert result is not None assert result.type == "ExperimentalVariantFunctionalImpactStudyResult" + def test_a_study_result_discloses_a_calibration_scope(self, mock_mapped_variant): + # Emitted unconditionally so that a record with no scope is never ambiguous between "public" and + # "produced before disclosure existed". + assert scope_of(variant_study_result(mock_mapped_variant)) == "public" + @pytest.mark.unit class TestVariantFunctionalImpactStatement: @@ -115,6 +134,39 @@ def test_variant_not_in_any_range_returns_indeterminate( # Classification should be INDETERMINATE assert result.classification.primaryCoding.code.root == "indeterminate" + def test_no_statement_is_built_from_a_private_calibration( + self, mock_mapped_variant_with_functional_calibration_score_set + ): + """A private calibration's thresholds and baseline scores must not reach an anonymous caller.""" + mapped_variant = make_private(mock_mapped_variant_with_functional_calibration_score_set) + + assert variant_functional_impact_statement(mapped_variant) is None + + def test_an_entitled_caller_receives_a_statement_from_a_private_calibration( + self, mock_mapped_variant_with_functional_calibration_score_set + ): + """Viewer-scoped emission: an export shows each principal what that principal may see.""" + mapped_variant = make_private(mock_mapped_variant_with_functional_calibration_score_set) + + assert variant_functional_impact_statement(mapped_variant, principal=admin_principal()) is not None + + def test_a_public_statement_discloses_a_public_calibration_scope( + self, mock_mapped_variant_with_functional_calibration_score_set + ): + # VA-Spec statements carry no stable id, so a viewer-scoped statement must say that it is one. + statement = variant_functional_impact_statement(mock_mapped_variant_with_functional_calibration_score_set) + + assert scope_of(statement) == "public" + + def test_a_statement_widened_by_entitlement_discloses_a_restricted_scope( + self, mock_mapped_variant_with_functional_calibration_score_set + ): + mapped_variant = make_private(mock_mapped_variant_with_functional_calibration_score_set) + + statement = variant_functional_impact_statement(mapped_variant, principal=admin_principal()) + + assert scope_of(statement) == "restricted" + @pytest.mark.unit class TestVariantPathogenicityStatement: @@ -296,6 +348,30 @@ def test_pathogenicity_evidence_line_has_evidence_items_are_statement_instances( ), "hasEvidenceItems contained a raw dict instead of a model instance" assert evidence_item.type == "Statement" + def test_no_statement_is_built_from_a_private_calibration( + self, mock_mapped_variant_with_pathogenicity_calibration_score_set + ): + """A private calibration's ACMG criteria must not reach an anonymous caller.""" + mapped_variant = make_private(mock_mapped_variant_with_pathogenicity_calibration_score_set) + + assert variant_pathogenicity_statement(mapped_variant) is None + + def test_the_owner_receives_a_statement_from_their_private_calibration( + self, mock_mapped_variant_with_pathogenicity_calibration_score_set + ): + mapped_variant = make_private(mock_mapped_variant_with_pathogenicity_calibration_score_set) + + assert variant_pathogenicity_statement(mapped_variant, principal=owner_principal()) is not None + + def test_a_statement_widened_by_entitlement_discloses_a_restricted_scope( + self, mock_mapped_variant_with_pathogenicity_calibration_score_set + ): + mapped_variant = make_private(mock_mapped_variant_with_pathogenicity_calibration_score_set) + + statement = variant_pathogenicity_statement(mapped_variant, principal=admin_principal()) + + assert scope_of(statement) == "restricted" + @pytest.mark.unit class TestVariantHighestLevelAnnotation: @@ -330,3 +406,25 @@ def test_none_when_unmapped(self, mock_mapped_variant): result = variant_highest_level_annotation(mock_mapped_variant) assert result is None + + def test_degrades_to_a_study_result_when_the_calibration_is_private( + self, mock_mapped_variant_with_pathogenicity_calibration_score_set + ): + # A study result reports the measured score, which publishing the score set did make public. The + # variant is still described; only the calibration-derived interpretation is withheld. + mapped_variant = make_private(mock_mapped_variant_with_pathogenicity_calibration_score_set) + + result = variant_highest_level_annotation(mapped_variant) + + assert result is not None + assert result.type == "ExperimentalVariantFunctionalImpactStudyResult" + + def test_reaches_the_statement_layer_for_an_entitled_caller( + self, mock_mapped_variant_with_pathogenicity_calibration_score_set + ): + mapped_variant = make_private(mock_mapped_variant_with_pathogenicity_calibration_score_set) + + result = variant_highest_level_annotation(mapped_variant, principal=admin_principal()) + + assert result is not None + assert result.type != "ExperimentalVariantFunctionalImpactStudyResult" diff --git a/tests/lib/annotation/test_util.py b/tests/lib/annotation/test_util.py index 515ae628..d44cbca3 100644 --- a/tests/lib/annotation/test_util.py +++ b/tests/lib/annotation/test_util.py @@ -15,11 +15,12 @@ import pytest pytest.importorskip("psycopg2") +pytest.importorskip("fastapi") from mavedb.lib.annotation.exceptions import MappingDataDoesntExistException from mavedb.lib.annotation.util import ( _can_annotate_variant_base_assumptions, - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation, + calibrations_available_for_annotation, can_annotate_variant_for_functional_statement, can_annotate_variant_for_pathogenicity_evidence, score_calibration_may_be_used_for_annotation, @@ -29,12 +30,23 @@ variation_from_mapped_variant, vrs_object_from_mapped_variant, ) +from mavedb.lib.permissions.principal import Principal from tests.helpers.constants import ( TEST_SEQUENCE_LOCATION_ACCESSION, TEST_VALID_POST_MAPPED_VRS_ALLELE, TEST_VALID_POST_MAPPED_VRS_ALLELE_LENGTH_EXPRESSION, TEST_VALID_POST_MAPPED_VRS_ALLELE_RLE, ) +from tests.lib.annotation.conftest import admin_principal, make_private + + +def _has_calibrations_for_annotation(*args, **kwargs) -> bool: + """Whether any calibration survived both the eligibility and visibility checks. + + ``calibrations_available_for_annotation`` returns the surviving calibrations; the cases below predate + that and assert only on whether the list was empty. + """ + return bool(calibrations_available_for_annotation(*args, **kwargs)) @pytest.mark.unit @@ -192,25 +204,19 @@ def test_returns_true_for_pathogenicity_with_any_acmg_classification( @pytest.mark.unit class TestVariantScoreCalibrationsHaveRequiredCalibrationsAndRangesForAnnotation: """ - Unit tests for the _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation function. + Unit tests for calibration availability, via the _has_calibrations_for_annotation adapter below. This function is used by both functional and pathogenicity annotation checks, so we test it separately here to avoid duplication in the tests for those checks. """ @pytest.mark.parametrize("kind", ["functional", "pathogenicity"], ids=["functional", "pathogenicity"]) def test_score_range_check_returns_false_when_calibrations_are_none(self, mock_mapped_variant, kind): mock_mapped_variant.variant.score_set.score_calibrations = None - assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation(mock_mapped_variant, kind) - is False - ) + assert _has_calibrations_for_annotation(mock_mapped_variant, kind) is False @pytest.mark.parametrize("kind", ["functional", "pathogenicity"], ids=["functional", "pathogenicity"]) def test_score_range_check_returns_false_when_no_calibrations_present(self, mock_mapped_variant, kind): mock_mapped_variant.variant.score_set.score_calibrations = [] - assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation(mock_mapped_variant, kind) - is False - ) + assert _has_calibrations_for_annotation(mock_mapped_variant, kind) is False @pytest.mark.parametrize("annotation_type", ["functional", "pathogenicity"], ids=["functional", "pathogenicity"]) def test_score_range_check_returns_false_when_all_calibrations_are_research_use_only_and_not_allowed( @@ -224,9 +230,7 @@ def test_score_range_check_returns_false_when_all_calibrations_are_research_use_ calibration.research_use_only = True assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation( - mock_mapped_variant_with_functional_calibration_score_set, annotation_type - ) + _has_calibrations_for_annotation(mock_mapped_variant_with_functional_calibration_score_set, annotation_type) is False ) @@ -249,9 +253,7 @@ def test_score_range_check_returns_true_when_research_use_only_calibrations_are_ calibration.research_use_only = True assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation( - mock_mapped_variant, kind, allow_research_use_only_calibrations=True - ) + _has_calibrations_for_annotation(mock_mapped_variant, kind, allow_research_use_only_calibrations=True) is True ) @@ -271,10 +273,7 @@ def test_score_range_check_returns_false_when_calibrations_present_with_empty_ra for calibration in mock_mapped_variant.variant.score_set.score_calibrations: calibration.functional_classifications = None - assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation(mock_mapped_variant, kind) - is False - ) + assert _has_calibrations_for_annotation(mock_mapped_variant, kind) is False def test_pathogenicity_range_check_returns_false_when_no_acmg_calibration( self, @@ -290,7 +289,7 @@ def test_pathogenicity_range_check_returns_false_when_no_acmg_calibration( calibration.functional_classifications = acmg_classification_removed assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation( + _has_calibrations_for_annotation( mock_mapped_variant_with_pathogenicity_calibration_score_set, "pathogenicity" ) is False @@ -309,7 +308,7 @@ def test_pathogenicity_range_check_returns_true_when_some_acmg_calibration( calibration.functional_classifications = acmg_classification_removed assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation( + _has_calibrations_for_annotation( mock_mapped_variant_with_pathogenicity_calibration_score_set, "pathogenicity" ) is True @@ -328,10 +327,7 @@ def test_score_range_check_returns_true_when_calibration_kind_exists_with_ranges ): mock_mapped_variant = request.getfixturevalue(variant_fixture) - assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation(mock_mapped_variant, kind) - is True - ) + assert _has_calibrations_for_annotation(mock_mapped_variant, kind) is True def test_score_range_check_returns_true_when_mixed_research_use_calibrations_exist_functional( self, mock_mapped_variant_with_functional_calibration_score_set @@ -351,9 +347,7 @@ def test_score_range_check_returns_true_when_mixed_research_use_calibrations_exi # Should return True because at least one non-research-only calibration has valid classifications assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation( - mock_mapped_variant_with_functional_calibration_score_set, "functional" - ) + _has_calibrations_for_annotation(mock_mapped_variant_with_functional_calibration_score_set, "functional") is True ) @@ -375,7 +369,7 @@ def test_score_range_check_returns_true_when_mixed_research_use_calibrations_exi # Should return True because at least one non-research-only calibration has valid classifications assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation( + _has_calibrations_for_annotation( mock_mapped_variant_with_pathogenicity_calibration_score_set, "pathogenicity" ) is True @@ -399,9 +393,7 @@ def test_score_range_check_handles_mixed_functional_classifications( # Should return True because at least one calibration has valid functional classifications assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation( - mock_mapped_variant_with_functional_calibration_score_set, "functional" - ) + _has_calibrations_for_annotation(mock_mapped_variant_with_functional_calibration_score_set, "functional") is True ) @@ -423,9 +415,7 @@ def test_pathogenicity_annotation_with_functional_classifications_but_no_acmg( # Should return False because no ACMG classifications exist assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation( - mock_mapped_variant_with_functional_calibration_score_set, "pathogenicity" - ) + _has_calibrations_for_annotation(mock_mapped_variant_with_functional_calibration_score_set, "pathogenicity") is False ) @@ -441,10 +431,52 @@ def test_functional_annotation_with_empty_functional_classifications_list( calibration.functional_classifications = [] assert ( - _variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation( + _has_calibrations_for_annotation(mock_mapped_variant_with_functional_calibration_score_set, "functional") + is False + ) + + +@pytest.mark.unit +class TestCalibrationAvailabilityIsScopedToTheCaller: + """A calibration's READ rule is stricter than its score set's, so publishing a score set does not + publish its calibrations. These cases exist because this function once read + ``score_set.score_calibrations`` directly and handed private calibrations to anyone. + """ + + def test_a_private_calibration_is_not_available_for_annotation( + self, mock_mapped_variant_with_functional_calibration_score_set + ): + mapped_variant = make_private(mock_mapped_variant_with_functional_calibration_score_set) + + assert calibrations_available_for_annotation(mapped_variant, "functional") == [] + + def test_omitting_the_principal_withholds_rather_than_widens( + self, mock_mapped_variant_with_functional_calibration_score_set + ): + # The whole design rests on an omitted principal meaning "the public". Passing an explicitly + # anonymous principal and passing none at all must agree. + mapped_variant = make_private(mock_mapped_variant_with_functional_calibration_score_set) + + assert calibrations_available_for_annotation( + mapped_variant, "functional" + ) == calibrations_available_for_annotation(mapped_variant, "functional", principal=Principal()) + + def test_an_entitled_caller_still_receives_a_private_calibration( + self, mock_mapped_variant_with_functional_calibration_score_set + ): + mapped_variant = make_private(mock_mapped_variant_with_functional_calibration_score_set) + + assert calibrations_available_for_annotation(mapped_variant, "functional", principal=admin_principal()) != [] + + def test_a_public_calibration_is_still_available_to_anyone( + self, mock_mapped_variant_with_functional_calibration_score_set + ): + # The counterweight: withholding private calibrations must not withhold what publishing released. + assert ( + calibrations_available_for_annotation( mock_mapped_variant_with_functional_calibration_score_set, "functional" ) - is False + != [] ) @@ -458,8 +490,8 @@ def test_pathogenicity_range_check_returns_false_when_base_assumptions_fail(self def test_pathogenicity_range_check_returns_false_when_pathogenicity_ranges_check_fails(self, mock_mapped_variant): with patch( - "mavedb.lib.annotation.util._variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation", - return_value=False, + "mavedb.lib.annotation.util.calibrations_available_for_annotation", + return_value=[], ): result = can_annotate_variant_for_pathogenicity_evidence(mock_mapped_variant) @@ -492,8 +524,8 @@ def test_functional_range_check_returns_false_when_functional_classifications_ch self, mock_mapped_variant ): with patch( - "mavedb.lib.annotation.util._variant_score_calibrations_have_required_calibrations_and_ranges_for_annotation", - return_value=False, + "mavedb.lib.annotation.util.calibrations_available_for_annotation", + return_value=[], ): result = can_annotate_variant_for_functional_statement(mock_mapped_variant) diff --git a/tests/lib/permissions/test_principal.py b/tests/lib/permissions/test_principal.py new file mode 100644 index 00000000..4eba914f --- /dev/null +++ b/tests/lib/permissions/test_principal.py @@ -0,0 +1,77 @@ +# ruff: noqa: E402 + +"""Tests for the Principal. + +A principal is what gets threaded through a fan-out read, so these cases cover both halves of its job: +handing out the right viewer for a caller, and never becoming shared state between callers. +""" + +import pytest + +pytest.importorskip("fastapi", reason="Skipping permissions tests; FastAPI is required but not installed.") + +import importlib +import inspect +import pkgutil + +import mavedb +from mavedb.lib.permissions.principal import Principal +from mavedb.lib.permissions.score_calibration import ScoreCalibrationViewer +from mavedb.lib.permissions.viewer import Viewer +from tests.lib.permissions.conftest import EntityTestHelper + + +class TestPrincipal: + def test_a_viewer_is_built_once_and_reused(self) -> None: + # This is what makes threading one Principal cheaper than constructing a viewer per record: the + # viewer's own READ memoization only pays off if the viewer itself survives. + principal = Principal() + + assert principal.viewer_for(ScoreCalibrationViewer) is principal.viewer_for(ScoreCalibrationViewer) + + def test_a_viewer_inherits_the_principals_caller(self) -> None: + admin = EntityTestHelper.create_user_data("admin") + calibration = EntityTestHelper.create_score_calibration(entity_state="private") + + assert Principal(admin).viewer_for(ScoreCalibrationViewer).may_read(calibration) is True + assert Principal().viewer_for(ScoreCalibrationViewer).may_read(calibration) is False + + def test_distinct_principals_do_not_share_viewers(self) -> None: + # Two principals in flight at once must not be able to answer for each other. + anonymous, admin = Principal(), Principal(EntityTestHelper.create_user_data("admin")) + + assert anonymous.viewer_for(ScoreCalibrationViewer) is not admin.viewer_for(ScoreCalibrationViewer) + + def test_an_anonymous_principal_is_the_default(self) -> None: + assert Principal().user_data is None + + +class TestNoSharedPrincipalOrViewerDefaults: + """No function may default a parameter to an Principal or Viewer instance. + + Python evaluates default arguments once at import, so such an instance — and its permission caches — + would be shared by every request for the life of the process. A calibration published mid-process would + keep its stale verdict, and two callers could be answered from one another's cache. The correct shape is + ``Optional[Principal] = None``, building one when it is missing. + """ + + def test_no_module_defaults_a_parameter_to_a_live_principal_or_viewer(self) -> None: + offenders = [] + + for module_info in pkgutil.walk_packages(mavedb.__path__, prefix="mavedb."): + try: + module = importlib.import_module(module_info.name) + except Exception: # optional extras (arq, cdot) are not installed in every environment + continue + + for name, function in inspect.getmembers(module, inspect.isfunction): + if inspect.getmodule(function) is not module: + continue + for parameter in inspect.signature(function).parameters.values(): + if isinstance(parameter.default, (Principal, Viewer)): + offenders.append(f"{module_info.name}.{name}({parameter.name}=...)") + + assert offenders == [], ( + "These defaults would be shared across every request for the life of the process; " + f"take Optional[...] = None instead: {offenders}" + ) diff --git a/tests/lib/permissions/test_score_calibration.py b/tests/lib/permissions/test_score_calibration.py index 0c94b8e2..a9ea8370 100644 --- a/tests/lib/permissions/test_score_calibration.py +++ b/tests/lib/permissions/test_score_calibration.py @@ -11,6 +11,7 @@ from mavedb.lib.permissions.actions import Action from mavedb.lib.permissions.score_calibration import ( + ScoreCalibrationViewer, _handle_change_rank_action, _handle_delete_action, _handle_publish_action, @@ -98,6 +99,33 @@ def test_requires_private_attribute(self, entity_helper: EntityTestHelper) -> No assert "private" in str(exc_info.value) +class TestScoreCalibrationViewer: + """Test that the viewer wires ScoreCalibration's rules into the generic Viewer. + + The rules themselves are covered by the action-handler suites below; the caching and fail-closed + behaviour the viewer inherits is covered by test_viewer.py. + """ + + def test_read_is_delegated_to_score_calibration_permissions(self, entity_helper: EntityTestHelper) -> None: + score_calibration = entity_helper.create_score_calibration("private") + + with mock.patch( + "mavedb.lib.permissions.score_calibration.has_permission", wraps=has_permission + ) as mock_has_permission: + ScoreCalibrationViewer().may_read(score_calibration) + + mock_has_permission.assert_called_once_with(None, score_calibration, Action.READ) + + def test_a_viewer_with_no_caller_withholds_a_private_calibration(self, entity_helper: EntityTestHelper) -> None: + # Export paths construct viewers with no arguments, so that default must mean "the public". + assert ScoreCalibrationViewer().may_read(entity_helper.create_score_calibration("private")) is False + + def test_a_viewer_with_no_caller_still_receives_a_published_calibration( + self, entity_helper: EntityTestHelper + ) -> None: + assert ScoreCalibrationViewer().may_read(entity_helper.create_score_calibration("published")) is True + + class TestScoreCalibrationReadActionHandler: """Test the _handle_read_action helper function directly.""" diff --git a/tests/lib/permissions/test_viewer.py b/tests/lib/permissions/test_viewer.py new file mode 100644 index 00000000..e22ba5d4 --- /dev/null +++ b/tests/lib/permissions/test_viewer.py @@ -0,0 +1,125 @@ +# ruff: noqa: E402 + +"""Tests for the generic Viewer contract. + +Every concrete viewer inherits its caching, its fail-closed behaviour and its default audience from the base +class, so those are exercised here once against a stand-in entity rather than once per entity type. A +concrete viewer's own rules belong with that entity's permission tests. +""" + +import pytest + +pytest.importorskip("fastapi", reason="Skipping permissions tests; FastAPI is required but not installed.") + +from dataclasses import dataclass +from types import SimpleNamespace +from typing import Any, Optional +from unittest.mock import Mock + +from mavedb.lib.permissions.actions import Action +from mavedb.lib.permissions.models import PermissionResponse +from mavedb.lib.permissions.viewer import Viewer +from mavedb.lib.types.authentication import UserData + +_PERMISSION_CALLS: list[tuple[Optional[UserData], Any, Action]] = [] +"""Every call the fake viewer's rules received, so the base class's caching can be asserted on directly.""" + + +def _entity(entity_id: Optional[int] = 1, *, permitted: bool = True, private: Optional[bool] = False, owner=None): + """A stand-in entity. + + ``private`` and ``permitted`` are separate because the base class reads the former (an unset flag means + the entity cannot state its own visibility) while the latter is the verdict a subclass's rules return. + """ + return SimpleNamespace(id=entity_id, permitted=permitted, private=private, owner=owner) + + +@dataclass(frozen=True) +class _FakeViewer(Viewer[SimpleNamespace]): + """A viewer whose rules are whatever the entity was built to say.""" + + @staticmethod + def _has_permission(user_data: Optional[UserData], entity: SimpleNamespace, action: Action) -> PermissionResponse: + _PERMISSION_CALLS.append((user_data, entity, action)) + return PermissionResponse(entity.permitted or (user_data is not None and user_data is entity.owner)) + + +@pytest.fixture(autouse=True) +def _reset_permission_calls(): + _PERMISSION_CALLS.clear() + + +class TestViewerDefaults: + def test_a_viewer_is_anonymous_unless_given_a_caller(self) -> None: + assert _FakeViewer().user_data is None + + def test_the_caller_is_threaded_through_to_the_rules(self) -> None: + user_data = Mock() + + _FakeViewer(user_data).may_read(_entity()) + + assert [(call_user_data, action) for call_user_data, _, action in _PERMISSION_CALLS] == [ + (user_data, Action.READ) + ] + + +class TestViewerFailsClosed: + def test_an_unset_private_flag_is_withheld(self) -> None: + # has_permission raises on an unset `private`, and a raising permission check inside a streaming + # generator surfaces as a truncated download rather than a denial. Fail closed instead. + assert _FakeViewer().may_read(_entity(private=None)) is False + + def test_the_rules_are_not_consulted_for_an_indeterminate_entity(self) -> None: + _FakeViewer().may_read(_entity(private=None)) + + assert _PERMISSION_CALLS == [] + + def test_an_entity_with_no_private_flag_is_not_indeterminate(self) -> None: + # Not every entity type carries a `private` column; its absence must not read as "unknown". + assert _FakeViewer().may_read(SimpleNamespace(id=1, permitted=True, owner=None)) is True + + +class TestViewerMemoization: + def test_the_same_entity_is_asked_about_only_once(self) -> None: + # A fan-out re-asks the same handful of entities once per record. Without memoization that is one + # permission check, and one logging-context write, per record. + entity = _entity() + viewer = _FakeViewer() + + for _ in range(5): + viewer.may_read(entity) + + assert len(_PERMISSION_CALLS) == 1 + + def test_memoization_does_not_conflate_distinct_entities(self) -> None: + viewer = _FakeViewer() + + assert viewer.may_read(_entity(1, permitted=True)) is True + assert viewer.may_read(_entity(2, permitted=False)) is False + + def test_an_unsaved_entity_is_not_memoized_under_a_null_id(self) -> None: + # Two distinct unsaved entities both have id None; caching either answer would leak one's verdict + # onto the other. + viewer = _FakeViewer() + + assert viewer.may_read(_entity(None, permitted=True)) is True + assert viewer.may_read(_entity(None, permitted=False)) is False + + def test_one_viewers_answer_does_not_leak_to_another(self) -> None: + # The memo is a per-instance field. A shared one would answer each caller out of the last one's cache. + owner = Mock() + entity = _entity(permitted=False, owner=owner) + + assert _FakeViewer(owner).may_read(entity) is True + assert _FakeViewer().may_read(entity) is False + + +class TestViewerVisible: + def test_visible_drops_the_entities_the_viewer_may_not_read(self) -> None: + readable, unreadable = _entity(1, permitted=True), _entity(2, permitted=False) + + assert _FakeViewer().visible([readable, unreadable]) == [readable] + + @pytest.mark.parametrize("entities", [None, []], ids=["none", "empty"]) + def test_visible_tolerates_having_nothing_to_filter(self, entities) -> None: + assert _FakeViewer().visible(entities) == [] diff --git a/tests/routers/test_experiments.py b/tests/routers/test_experiments.py index 2b6be3b5..60d55214 100644 --- a/tests/routers/test_experiments.py +++ b/tests/routers/test_experiments.py @@ -24,6 +24,7 @@ from tests.helpers.constants import ( EXTRA_USER, TEST_BIORXIV_IDENTIFIER, + TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED, TEST_CROSSREF_IDENTIFIER, TEST_EXPERIMENT_WITH_KEYWORD, TEST_EXPERIMENT_WITH_KEYWORD_HAS_DUPLICATE_OTHERS_RESPONSE, @@ -40,8 +41,13 @@ TEST_USER2, ) from tests.helpers.dependency_overrider import DependencyOverrider +from tests.helpers.util.common import deepcamelize from tests.helpers.util.contributor import add_contributor from tests.helpers.util.experiment import create_experiment +from tests.helpers.util.score_calibration import ( + create_test_score_calibration_in_score_set_via_client, + publish_test_score_calibration_via_client, +) from tests.helpers.util.score_set import create_seq_score_set, create_seq_score_set_with_variants, publish_score_set from tests.helpers.util.user import change_ownership from tests.helpers.util.variant import mock_worker_variant_insertion @@ -1796,6 +1802,86 @@ def test_non_owner_searches_published_superseding_score_sets_for_experiments( assert response.json()[0]["urn"] == published_superseding_score_set["urn"] +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + [ + {"dbName": "PubMed", "identifier": f"{TEST_PUBMED_IDENTIFIER}"}, + {"dbName": "bioRxiv", "identifier": f"{TEST_BIORXIV_IDENTIFIER}"}, + ] + ], + indirect=["mock_publication_fetch"], +) +def test_experiment_score_sets_withhold_private_calibrations_from_anonymous_users( + session, data_provider, client, setup_router_db, data_files, anonymous_app_overrides, mock_publication_fetch +): + """A published score set can carry an unpublished calibration. + + This endpoint checks READ on the experiment and on each score set, but a calibration's READ rule is + stricter than its score set's, so it needs its own filter. Without it the listing served every private + calibration's thresholds to anyone. + """ + experiment = create_experiment(client) + score_set = create_seq_score_set(client, experiment["urn"]) + score_set = mock_worker_variant_insertion(client, session, data_provider, score_set, data_files / "scores.csv") + create_test_score_calibration_in_score_set_via_client( + client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + ) + + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + published = publish_score_set(client, score_set["urn"]) + + experiment_urn = published["experiment"]["urn"] + + # The owner sees their own private calibration. + owner_response = client.get(f"/api/v1/experiments/{experiment_urn}/score-sets") + assert owner_response.status_code == 200 + owner_entry = next(ss for ss in owner_response.json() if ss["urn"] == published["urn"]) + assert len(owner_entry.get("scoreCalibrations") or []) == 1 + + with DependencyOverrider(anonymous_app_overrides): + anonymous_response = client.get(f"/api/v1/experiments/{experiment_urn}/score-sets") + + assert anonymous_response.status_code == 200 + anonymous_entry = next(ss for ss in anonymous_response.json() if ss["urn"] == published["urn"]) + assert (anonymous_entry.get("scoreCalibrations") or []) == [] + + +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + [ + {"dbName": "PubMed", "identifier": f"{TEST_PUBMED_IDENTIFIER}"}, + {"dbName": "bioRxiv", "identifier": f"{TEST_BIORXIV_IDENTIFIER}"}, + ] + ], + indirect=["mock_publication_fetch"], +) +def test_experiment_score_sets_serve_published_calibrations_to_anonymous_users( + session, data_provider, client, setup_router_db, data_files, anonymous_app_overrides, mock_publication_fetch +): + """The filter withholds only what a calibration's own READ rule withholds.""" + experiment = create_experiment(client) + score_set = create_seq_score_set(client, experiment["urn"]) + score_set = mock_worker_variant_insertion(client, session, data_provider, score_set, data_files / "scores.csv") + calibration = create_test_score_calibration_in_score_set_via_client( + client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + ) + publish_test_score_calibration_via_client(client, calibration["urn"]) + + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + published = publish_score_set(client, score_set["urn"]) + + experiment_urn = published["experiment"]["urn"] + + with DependencyOverrider(anonymous_app_overrides): + anonymous_response = client.get(f"/api/v1/experiments/{experiment_urn}/score-sets") + + assert anonymous_response.status_code == 200 + anonymous_entry = next(ss for ss in anonymous_response.json() if ss["urn"] == published["urn"]) + assert [c["urn"] for c in (anonymous_entry.get("scoreCalibrations") or [])] == [calibration["urn"]] + + def test_search_score_sets_for_contributor_experiments(session, client, setup_router_db, data_files, data_provider): experiment = create_experiment(client) score_set = create_seq_score_set(client, experiment["urn"]) diff --git a/tests/routers/test_score_set.py b/tests/routers/test_score_set.py index 4a896c3c..b7ecb514 100644 --- a/tests/routers/test_score_set.py +++ b/tests/routers/test_score_set.py @@ -24,6 +24,7 @@ from mavedb.models.experiment import Experiment as ExperimentDbModel from mavedb.models.job_run import JobRun from mavedb.models.pipeline import Pipeline +from mavedb.models.score_calibration import ScoreCalibration as ScoreCalibrationDbModel from mavedb.models.mapped_variant import MappedVariant as MappedVariantDbModel from mavedb.models.score_set import ScoreSet as ScoreSetDbModel from mavedb.models.variant import Variant as VariantDbModel @@ -1786,6 +1787,50 @@ def test_recently_published_returns_published_score_sets(session, data_provider, assert published_2["urn"] in returned_urns +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + [ + {"dbName": "PubMed", "identifier": f"{TEST_PUBMED_IDENTIFIER}"}, + {"dbName": "bioRxiv", "identifier": f"{TEST_BIORXIV_IDENTIFIER}"}, + ] + ], + indirect=["mock_publication_fetch"], +) +def test_recently_published_withholds_private_calibrations_from_anonymous_users( + session, data_provider, client, setup_router_db, data_files, anonymous_app_overrides, mock_publication_fetch +): + """A published score set can carry an unpublished calibration. + + This endpoint checks READ on the score set and on its superseding score set, but a calibration's READ + rule is stricter than its score set's, so it needs its own filter. Without it the listing served every + private calibration's thresholds to anyone. + """ + experiment = create_experiment(client) + score_set = create_seq_score_set_with_mapped_variants( + client, session, data_provider, experiment["urn"], data_files / "scores.csv" + ) + create_test_score_calibration_in_score_set_via_client( + client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + ) + + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + published = publish_score_set(client, score_set["urn"]) + + # The owner sees their own private calibration. + owner_response = client.get("/api/v1/score-sets/recently-published") + assert owner_response.status_code == 200 + owner_entry = next(ss for ss in owner_response.json() if ss["urn"] == published["urn"]) + assert len(owner_entry.get("scoreCalibrations") or []) == 1 + + with DependencyOverrider(anonymous_app_overrides): + anonymous_response = client.get("/api/v1/score-sets/recently-published") + + assert anonymous_response.status_code == 200 + anonymous_entry = next(ss for ss in anonymous_response.json() if ss["urn"] == published["urn"]) + assert (anonymous_entry.get("scoreCalibrations") or []) == [] + + def test_recently_published_does_not_return_unpublished_score_sets(client, setup_router_db): experiment = create_experiment(client) create_seq_score_set(client, experiment["urn"]) @@ -2890,9 +2935,7 @@ def test_search_score_sets_not_affected_by_experiment_metadata( assert response.json()["numScoreSets"] == num_score_sets -def test_cannot_create_multiple_superseding_versions( - session, data_provider, client, setup_router_db, data_files -): +def test_cannot_create_multiple_superseding_versions(session, data_provider, client, setup_router_db, data_files): """Attempting to create multiple superseding versions should fail.""" experiment = create_experiment(client, {"title": "Original Experiment"}) score_set = create_seq_score_set(client, experiment["urn"], update={"title": "Original Score Set"}) @@ -2918,7 +2961,9 @@ def test_cannot_create_multiple_superseding_versions( response = client.post("/api/v1/score-sets/", json=score_set_post_payload) assert response.status_code == 409 - assert (f"This score set has been superseded by score set: {first_superseding['urn']}.") in response.json()["detail"] + assert (f"This score set has been superseded by score set: {first_superseding['urn']}.") in response.json()[ + "detail" + ] def test_search_score_sets_not_affected_by_an_unpublishing_superseding_versions( @@ -4559,3 +4604,67 @@ def test_cannot_fetch_gnomad_variants_for_score_set_when_none_exist( f"No gnomad variants matching the provided filters associated with score set URN {score_set['urn']} were found" in response_data["detail"] ) + + +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + [ + {"dbName": "PubMed", "identifier": f"{TEST_PUBMED_IDENTIFIER}"}, + {"dbName": "bioRxiv", "identifier": f"{TEST_BIORXIV_IDENTIFIER}"}, + ] + ], + indirect=["mock_publication_fetch"], +) +def test_publish_withholds_a_community_private_calibration_from_the_score_set_owner( + session, data_provider, client, setup_router_db, data_files, mock_publication_fetch +): + """Owning a score set does not entitle its owner to every calibration attached to it. + + A community calibration -- one contributed by someone who is not a contributor to the score set -- is + readable only by its own creator while private. The owner-facing mutation endpoints returned the score + set wholesale, so publishing handed the owner a calibration they cannot fetch directly. + """ + experiment = create_experiment(client) + score_set = create_seq_score_set(client, experiment["urn"]) + score_set = mock_worker_variant_insertion(client, session, data_provider, score_set, data_files / "scores.csv") + calibration = create_test_score_calibration_in_score_set_via_client( + client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + ) + + calibration_item = session.query(ScoreCalibrationDbModel).filter_by(urn=calibration["urn"]).one() + calibration_item.investigator_provided = False + session.commit() + change_ownership(session, calibration["urn"], ScoreCalibrationDbModel) + + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + published = publish_score_set(client, score_set["urn"]) + + assert (published.get("scoreCalibrations") or []) == [] + + +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + [ + {"dbName": "PubMed", "identifier": f"{TEST_PUBMED_IDENTIFIER}"}, + {"dbName": "bioRxiv", "identifier": f"{TEST_BIORXIV_IDENTIFIER}"}, + ] + ], + indirect=["mock_publication_fetch"], +) +def test_publish_returns_the_owners_own_private_calibration( + session, data_provider, client, setup_router_db, data_files, mock_publication_fetch +): + """The filter withholds only what a calibration's own READ rule withholds.""" + experiment = create_experiment(client) + score_set = create_seq_score_set(client, experiment["urn"]) + score_set = mock_worker_variant_insertion(client, session, data_provider, score_set, data_files / "scores.csv") + calibration = create_test_score_calibration_in_score_set_via_client( + client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + ) + + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + published = publish_score_set(client, score_set["urn"]) + + assert [c["urn"] for c in (published.get("scoreCalibrations") or [])] == [calibration["urn"]]