From c115c70a75932d526d14f56e192c9af36bef6349 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Wed, 23 Sep 2026 14:23:01 -0700 Subject: [PATCH] fix(permissions): require score set visibility to read a score calibration A calibration could be published while its score set was still private, and the calibration READ rule permitted any non-private calibration without consulting the score set. The variant routes under /score-calibrations/{urn} check calibration READ only, so an unauthenticated caller holding the URN of such a calibration received the private score set's variants, including every score and count column. Any contributor could create and publish one, releasing data before the score set's owner did. Close both halves. Publishing a calibration now fails with 400 while its score set is private, the check that had been left commented out since calibrations were introduced. Calibration READ now also requires READ on the score set, delegated to the score set's own rule, so calibrations already published this way stop leaking on deploy, and writers that bypass the publish route (an admin move, the calibration loader scripts) cannot reopen it. Denials are 404, matching the variant routes. This also withholds a private calibration from its creator once they can no longer read the score set. /score-calibrations/me returned every calibration a user had created with no permission check, so a contributor removed from a private score set still received its calibrations' ranges and tmp URN. It now filters on READ. Existing rows are left as they are; clearing private and primary on public calibrations of private score sets is a separate data fix. Most of the test churn is fixtures that modelled the leaked state: calibrations published on unpublished score sets, mock score sets with no private attribute, and dump score sets carrying a published date with private left true. --- .../lib/permissions/score_calibration.py | 14 +- src/mavedb/lib/score_calibrations.py | 6 +- src/mavedb/routers/score_calibrations.py | 32 ++- tests/helpers/mocks/factories.py | 4 + tests/helpers/util/score_calibration.py | 12 + tests/lib/annotation/conftest.py | 4 +- tests/lib/csv/test_variant.py | 8 + tests/lib/permissions/conftest.py | 11 +- .../lib/permissions/test_score_calibration.py | 40 +++ tests/lib/test_score_calibrations.py | 28 +++ tests/routers/test_experiments.py | 4 +- tests/routers/test_mapped_variants.py | 10 + tests/routers/test_score_calibrations.py | 237 +++++++++++++++++- tests/routers/test_score_set.py | 16 +- tests/scripts/conftest.py | 3 +- 15 files changed, 400 insertions(+), 29 deletions(-) diff --git a/src/mavedb/lib/permissions/score_calibration.py b/src/mavedb/lib/permissions/score_calibration.py index 86b404f5c..d00d01791 100644 --- a/src/mavedb/lib/permissions/score_calibration.py +++ b/src/mavedb/lib/permissions/score_calibration.py @@ -4,6 +4,7 @@ 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.score_set import has_permission as score_set_has_permission 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 @@ -105,9 +106,12 @@ def _handle_read_action( """ Handle READ action permission check for ScoreCalibration entities. - ScoreCalibrations are generally readable by anyone who can access the - associated ScoreSet, as they provide important contextual information - about the score data. + TODO(#876) - Separates the readability concern by adding the notion of a separate + calibration method. For now, A ScoreCalibration is never readable by a user who + cannot read its ScoreSet, because the calibration variant routes return the score + set's variants and scores. Among users who can read the ScoreSet, published + calibrations are readable by all and private calibrations only by their owner, + contributors (for investigator-provided calibrations), and admins. Args: user_data: The user's authentication data. @@ -120,6 +124,10 @@ def _handle_read_action( Returns: PermissionResponse: Permission result with appropriate HTTP status. """ + # Deny as not found so the calibrations of an unreadable score set are not acknowledged. + if not score_set_has_permission(user_data, entity.score_set, Action.READ).permitted: + return deny_action_for_entity(entity, True, user_data, False, "score calibration") + ## Allow read access under the following conditions: # Any user may read a ScoreCalibration if it is not private. if not private: diff --git a/src/mavedb/lib/score_calibrations.py b/src/mavedb/lib/score_calibrations.py index 11e1b2e88..1796b3a8d 100644 --- a/src/mavedb/lib/score_calibrations.py +++ b/src/mavedb/lib/score_calibrations.py @@ -506,7 +506,8 @@ def publish_score_calibration(db: Session, calibration: ScoreCalibration, user: Raises ------ ValueError - If the calibration is already published (i.e., `private` is False). + If the calibration is already published (i.e., `private` is False), or if its + score set is still private. Notes ----- @@ -516,6 +517,9 @@ def publish_score_calibration(db: Session, calibration: ScoreCalibration, user: if not calibration.private: raise ValueError("Calibration is already published.") + if calibration.score_set.private: + raise ValueError("Cannot publish a calibration whose score set is private.") + calibration.private = False calibration.modified_by = user diff --git a/src/mavedb/routers/score_calibrations.py b/src/mavedb/routers/score_calibrations.py index e8b31478c..1df20b5ee 100644 --- a/src/mavedb/routers/score_calibrations.py +++ b/src/mavedb/routers/score_calibrations.py @@ -67,14 +67,23 @@ def list_my_calibrations( db: Session = Depends(deps.get_db), user_data: UserData = Depends(require_current_user), ) -> list[ScoreCalibration]: - """List all score calibrations created by the current user.""" - return ( + """ + List the score calibrations created by the current user that the user may still read. + + Calibrations on score sets the user can no longer read, for example after being removed as a + contributor, are omitted. + """ + calibrations = ( db.query(ScoreCalibration) .filter(ScoreCalibration.created_by_id == user_data.user.id) .options(selectinload(ScoreCalibration.score_set).selectinload(ScoreSet.contributors)) .all() ) + return [ + calibration for calibration in calibrations if has_permission(user_data, calibration, Action.READ).permitted + ] + @router.get( "/{urn}", @@ -672,6 +681,8 @@ def publish_score_calibration_route( ) -> ScoreCalibration: """ Publish a score calibration, making it publicly visible. + + The calibration's score set must already be published. """ save_to_logging_context({"requested_resource": urn, "resource_property": "private"}) @@ -691,15 +702,14 @@ def publish_score_calibration_route( logger.debug("The requested score calibration is already public", extra=logging_context()) return item - # XXX: desired? - # if item.score_set.private: - # logger.debug( - # "Score calibrations associated with private score sets cannot be published", extra=logging_context() - # ) - # raise HTTPException( - # status_code=400, - # detail="Score calibrations associated with private score sets cannot be published. First publish the score set, then calibrations.", - # ) + if item.score_set.private: + logger.debug( + "Score calibrations associated with private score sets cannot be published", extra=logging_context() + ) + raise HTTPException( + status_code=400, + detail="Score calibrations associated with private score sets cannot be published. First publish the score set, then calibrations.", + ) item = publish_score_calibration(db, item, user_data.user) db.commit() diff --git a/tests/helpers/mocks/factories.py b/tests/helpers/mocks/factories.py index 6e83d089c..7c7f7aac7 100644 --- a/tests/helpers/mocks/factories.py +++ b/tests/helpers/mocks/factories.py @@ -153,6 +153,10 @@ def create_mock_score_set( mock_target_gene = create_mock_target_gene() mock_experiment = create_mock_experiment() + # Read by the score set permission check, which calibration visibility depends on. + kwargs.setdefault("private", False) + kwargs.setdefault("contributors", []) + return create_sealed_mock( urn=urn, title=title, diff --git a/tests/helpers/util/score_calibration.py b/tests/helpers/util/score_calibration.py index a535096c2..ab3d0b9c0 100644 --- a/tests/helpers/util/score_calibration.py +++ b/tests/helpers/util/score_calibration.py @@ -48,6 +48,18 @@ def create_test_score_calibration_in_score_set_via_client( return calibration +def force_publish_test_score_calibration(db: "Session", calibration_urn: str) -> None: + """Mark a calibration public without the publish route's checks. + + Reproduces calibrations that were published while their score set was still private, which the publish + route now refuses. + """ + calibration = db.query(ScoreCalibration).filter(ScoreCalibration.urn == calibration_urn).one() + calibration.private = False + db.add(calibration) + db.commit() + + def publish_test_score_calibration_via_client(client: "TestClient", calibration_urn: str): response = client.post(f"/api/v1/score-calibrations/{calibration_urn}/publish") diff --git a/tests/lib/annotation/conftest.py b/tests/lib/annotation/conftest.py index 29a056c67..fbdbf052d 100644 --- a/tests/lib/annotation/conftest.py +++ b/tests/lib/annotation/conftest.py @@ -27,12 +27,12 @@ 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. + of which the annotation mocks populate. The score set stays public so only the calibration is private. """ 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) + calibration.score_set = Mock(private=False, contributors=[], created_by_id=owner_id, modified_by_id=owner_id) return mapped_variant diff --git a/tests/lib/csv/test_variant.py b/tests/lib/csv/test_variant.py index 6b475d6fd..37029fd18 100644 --- a/tests/lib/csv/test_variant.py +++ b/tests/lib/csv/test_variant.py @@ -54,6 +54,10 @@ def _add_pathogenicity_calibration(db, score_set, variants_in_abnormal_range, ur Only *variants_in_abnormal_range* are associated with the abnormal range, which is what ``functional_classification_of_variant`` consults to classify a variant. """ + # A public calibration is only readable when its score set is too. + score_set.private = False + db.add(score_set) + calibration = ScoreCalibration( score_set_id=score_set.id, urn=urn, @@ -111,6 +115,10 @@ def _add_rangeless_calibration(db, score_set, urn, title): It can support neither a functional nor a pathogenicity annotation, so every column of its namespace would be NA. """ + # A public calibration is only readable when its score set is too. + score_set.private = False + db.add(score_set) + calibration = ScoreCalibration( score_set_id=score_set.id, urn=urn, diff --git a/tests/lib/permissions/conftest.py b/tests/lib/permissions/conftest.py index 302159f5e..933372a4a 100644 --- a/tests/lib/permissions/conftest.py +++ b/tests/lib/permissions/conftest.py @@ -169,15 +169,22 @@ def create_user(user_id: int = 5): ) @staticmethod - def create_score_calibration(entity_state: str = "private", investigator_provided: bool = False): + def create_score_calibration( + entity_state: str = "private", + investigator_provided: bool = False, + score_set_state: Optional[str] = None, + score_set_owner_id: int = 2, + ): """Create a ScoreCalibration mock for testing. Args: entity_state: "private" or "published" (affects score_set and private property) investigator_provided: True if investigator-provided, False if community-provided + score_set_state: "private" or "published" for the score set; defaults to entity_state + score_set_owner_id: ID of the score set's owner; defaults to the calibration's owner """ private = entity_state == "private" - score_set = EntityTestHelper.create_score_set(entity_state) + score_set = EntityTestHelper.create_score_set(score_set_state or entity_state, owner_id=score_set_owner_id) # ScoreCalibrations have their own private property plus associated ScoreSet return Mock( diff --git a/tests/lib/permissions/test_score_calibration.py b/tests/lib/permissions/test_score_calibration.py index a9ea8370d..9689ef42c 100644 --- a/tests/lib/permissions/test_score_calibration.py +++ b/tests/lib/permissions/test_score_calibration.py @@ -205,6 +205,46 @@ def test_handle_read_action(self, test_case: PermissionTest, entity_helper: Enti assert result.http_code == test_case.expected_code +class TestScoreCalibrationReadRequiresScoreSetRead: + """A calibration is never more visible than its score set, since its variant routes return the score set's scores.""" + + @pytest.mark.parametrize( + "user_type, should_be_permitted", + [ + ("admin", True), + ("owner", True), + ("contributor", True), + ("mapper", True), + ("other_user", False), + ("anonymous", False), + ], + ) + def test_published_calibration_on_private_score_set( + self, entity_helper: EntityTestHelper, user_type: str, should_be_permitted: bool + ) -> None: + score_calibration = entity_helper.create_score_calibration("published", score_set_state="private") + + result = has_permission(entity_helper.create_user_data(user_type), score_calibration, Action.READ) + + assert result.permitted == should_be_permitted + if not should_be_permitted: + assert result.http_code == 404 + + def test_owner_who_cannot_read_the_score_set_cannot_read_their_calibration( + self, entity_helper: EntityTestHelper + ) -> None: + # The calibration's creator is neither the score set's owner nor one of its contributors, e.g. after + # being removed from the contributor list. + score_calibration = entity_helper.create_score_calibration( + "private", investigator_provided=True, score_set_owner_id=7 + ) + + result = has_permission(entity_helper.create_user_data("owner"), score_calibration, Action.READ) + + assert not result.permitted + assert result.http_code == 404 + + class TestScoreCalibrationUpdateActionHandler: """Test the _handle_update_action helper function directly.""" diff --git a/tests/lib/test_score_calibrations.py b/tests/lib/test_score_calibrations.py index 9a5ba43c5..3b080e90d 100644 --- a/tests/lib/test_score_calibrations.py +++ b/tests/lib/test_score_calibrations.py @@ -1193,12 +1193,39 @@ async def test_publish_score_calibration_marks_calibration_public( existing_calibration = await create_test_range_based_score_calibration_in_score_set( session, setup_lib_db_with_score_set.urn, test_user ) + existing_calibration.score_set.private = False assert existing_calibration.private is True published_calibration = publish_score_calibration(session, existing_calibration, test_user) assert published_calibration.private is False +@pytest.mark.asyncio +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + [ + {"dbName": "PubMed", "identifier": TEST_PUBMED_IDENTIFIER}, + {"dbName": "bioRxiv", "identifier": TEST_BIORXIV_IDENTIFIER}, + ], + ], + indirect=["mock_publication_fetch"], +) +async def test_cannot_publish_calibration_when_score_set_is_private( + setup_lib_db_with_score_set, session, mock_publication_fetch +): + test_user = session.execute(select(User)).scalars().first() + + existing_calibration = await create_test_range_based_score_calibration_in_score_set( + session, setup_lib_db_with_score_set.urn, test_user + ) + assert existing_calibration.score_set.private is True + + with pytest.raises(ValueError, match="Cannot publish a calibration whose score set is private."): + publish_score_calibration(session, existing_calibration, test_user) + assert existing_calibration.private is True + + @pytest.mark.asyncio @pytest.mark.parametrize( "mock_publication_fetch", @@ -1218,6 +1245,7 @@ async def test_publish_score_calibration_user_is_set_as_modifier( existing_calibration = await create_test_range_based_score_calibration_in_score_set( session, setup_lib_db_with_score_set.urn, test_user ) + existing_calibration.score_set.private = False publish_user = session.execute(select(User).where(User.id != test_user.id)).scalars().first() published_calibration = publish_score_calibration(session, existing_calibration, publish_user) diff --git a/tests/routers/test_experiments.py b/tests/routers/test_experiments.py index acd949392..e4f56785e 100644 --- a/tests/routers/test_experiments.py +++ b/tests/routers/test_experiments.py @@ -1914,11 +1914,11 @@ def test_experiment_score_sets_serve_published_calibrations_to_anonymous_users( 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"]) + publish_test_score_calibration_via_client(client, calibration["urn"]) + experiment_urn = published["experiment"]["urn"] with DependencyOverrider(anonymous_app_overrides): diff --git a/tests/routers/test_mapped_variants.py b/tests/routers/test_mapped_variants.py index 51f45452f..6372faf78 100644 --- a/tests/routers/test_mapped_variants.py +++ b/tests/routers/test_mapped_variants.py @@ -1,6 +1,7 @@ # ruff: noqa: E402 import json +from unittest.mock import patch import pytest @@ -32,6 +33,7 @@ from tests.helpers.util.score_set import ( create_seq_score_set_with_mapped_variants, create_seq_score_set_with_variants, + publish_score_set, ) @@ -213,6 +215,8 @@ def test_show_mapped_variant_functional_impact_statement( experiment["urn"], data_files / "scores.csv", ) + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -294,6 +298,8 @@ def test_cannot_show_mapped_variant_functional_impact_statement_when_no_mapping_ experiment["urn"], data_files / "scores.csv", ) + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -360,6 +366,8 @@ def test_show_mapped_variant_clinical_evidence_line( experiment["urn"], data_files / "scores.csv", ) + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -441,6 +449,8 @@ def test_cannot_show_mapped_variant_clinical_evidence_line_when_no_mapping_data_ experiment["urn"], data_files / "scores.csv", ) + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) diff --git a/tests/routers/test_score_calibrations.py b/tests/routers/test_score_calibrations.py index fe1aeba7c..16b4b4c31 100644 --- a/tests/routers/test_score_calibrations.py +++ b/tests/routers/test_score_calibrations.py @@ -32,6 +32,7 @@ from tests.helpers.util.score_calibration import ( create_publish_and_promote_score_calibration, create_test_score_calibration_in_score_set_via_client, + force_publish_test_score_calibration, publish_test_score_calibration_via_client, ) from tests.helpers.util.score_set import create_seq_score_set_with_mapped_variants, publish_score_set @@ -297,6 +298,8 @@ def test_anonymous_user_can_get_score_calibration_when_public( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -332,6 +335,8 @@ def test_other_user_can_get_score_calibration_when_public( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -367,6 +372,8 @@ def test_creating_user_can_get_score_calibration_when_public( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -401,6 +408,8 @@ def test_contributing_user_can_get_score_calibration_when_public( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -445,6 +454,8 @@ def test_admin_user_can_get_score_calibration_when_public( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -787,11 +798,11 @@ def test_anonymous_user_can_get_score_calibrations_for_score_set_when_public( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) - publish_test_score_calibration_via_client(client, calibration["urn"]) - with patch.object(ArqRedis, "enqueue_job", return_value=None): score_set = publish_score_set(client, score_set["urn"]) + publish_test_score_calibration_via_client(client, calibration["urn"]) + with DependencyOverrider(anonymous_app_overrides): response = client.get(f"/api/v1/score-calibrations/score-set/{score_set['urn']}") @@ -832,11 +843,11 @@ def test_other_user_can_get_score_calibrations_for_score_set_when_public( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) - publish_test_score_calibration_via_client(client, calibration["urn"]) - with patch.object(ArqRedis, "enqueue_job", return_value=None): score_set = publish_score_set(client, score_set["urn"]) + publish_test_score_calibration_via_client(client, calibration["urn"]) + with DependencyOverrider(extra_user_app_overrides): response = client.get(f"/api/v1/score-calibrations/score-set/{score_set['urn']}") @@ -877,7 +888,7 @@ def test_anonymous_user_cannot_get_score_calibrations_for_score_set_when_calibra client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) - publish_test_score_calibration_via_client(client, calibration["urn"]) + force_publish_test_score_calibration(session, calibration["urn"]) with DependencyOverrider(anonymous_app_overrides): response = client.get(f"/api/v1/score-calibrations/score-set/{score_set['urn']}") @@ -917,7 +928,7 @@ def test_other_user_cannot_get_score_calibrations_for_score_set_when_calibration client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) - publish_test_score_calibration_via_client(client, calibration["urn"]) + force_publish_test_score_calibration(session, calibration["urn"]) with DependencyOverrider(extra_user_app_overrides): response = client.get(f"/api/v1/score-calibrations/score-set/{score_set['urn']}") @@ -948,6 +959,8 @@ def test_creating_user_can_get_score_calibrations_for_score_set_when_public( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -988,6 +1001,8 @@ def test_contributing_user_can_get_score_calibrations_for_score_set_when_public( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -1038,6 +1053,8 @@ def test_admin_user_can_get_score_calibrations_for_score_set_when_public( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -1148,6 +1165,8 @@ def test_get_primary_score_calibration_for_score_set_when_exists( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -1183,6 +1202,8 @@ def test_get_primary_score_calibration_for_score_set_when_multiple_exist( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) @@ -2212,6 +2233,8 @@ def test_cannot_update_published_score_calibration_as_score_set_owner( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -2400,6 +2423,8 @@ def test_can_update_published_score_calibration_as_admin_user( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -2826,6 +2851,8 @@ def test_cannot_delete_published_score_calibration_as_owner( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -2985,6 +3012,8 @@ def test_can_delete_published_score_calibration_as_admin_user( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3021,6 +3050,8 @@ def test_cannot_delete_primary_score_calibration( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3069,6 +3100,8 @@ def test_cannot_promote_score_calibration_as_anonymous_user( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3103,6 +3136,8 @@ def test_cannot_promote_score_calibration_when_score_calibration_not_owned_by_us experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3139,6 +3174,8 @@ def test_can_promote_score_calibration_as_score_set_owner( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3173,6 +3210,8 @@ def test_can_promote_score_calibration_as_score_set_contributor( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3218,6 +3257,8 @@ def test_can_promote_score_calibration_as_admin_user( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3254,6 +3295,8 @@ def test_can_promote_existing_primary_to_primary( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) primary_calibration = create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3288,6 +3331,8 @@ def test_cannot_promote_research_use_only_to_primary( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], @@ -3357,6 +3402,8 @@ def test_cannot_promote_to_primary_if_primary_exists( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3393,6 +3440,8 @@ def test_can_promote_to_primary_if_primary_exists_when_demote_existing_is_true( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) primary_calibration = create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3441,6 +3490,8 @@ def test_score_set_owner_can_promote_to_primary_with_demote_existing_flag_on_com experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) with DependencyOverrider(admin_app_overrides): primary_calibration = create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) @@ -3501,6 +3552,8 @@ def test_cannot_demote_score_calibration_as_anonymous_user( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3536,6 +3589,8 @@ def test_cannot_demote_score_calibration_when_score_calibration_not_owned_by_use experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3571,6 +3626,8 @@ def test_can_demote_score_calibration_as_score_set_contributor( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3617,6 +3674,8 @@ def test_can_demote_score_calibration_as_score_set_owner( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3653,6 +3712,8 @@ def test_can_demote_score_calibration_as_admin_user( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3690,6 +3751,8 @@ def test_can_demote_non_primary_score_calibration( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3826,6 +3889,8 @@ def test_can_publish_score_calibration_as_score_set_owner( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3862,6 +3927,8 @@ def test_can_publish_score_calibration_as_admin_user( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3899,6 +3966,8 @@ def test_can_publish_already_published_calibration( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -3920,6 +3989,45 @@ def test_can_publish_already_published_calibration( assert published_calibration_2["private"] is False +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + [ + {"dbName": "PubMed", "identifier": TEST_PUBMED_IDENTIFIER}, + {"dbName": "bioRxiv", "identifier": TEST_BIORXIV_IDENTIFIER}, + ] + ], + indirect=["mock_publication_fetch"], +) +def test_cannot_publish_score_calibration_when_score_set_is_private( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + experiment = create_experiment(client) + score_set = create_seq_score_set_with_mapped_variants( + client, + session, + data_provider, + experiment["urn"], + 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) + ) + + response = client.post( + f"/api/v1/score-calibrations/{calibration['urn']}/publish", + ) + + assert response.status_code == 400 + error = response.json() + assert "Score calibrations associated with private score sets cannot be published" in error["detail"] + + persisted = session.execute( + select(CalibrationDbModel).where(CalibrationDbModel.urn == calibration["urn"]) + ).scalar_one() + assert persisted.private is True + + ########################################################### # GET /score-calibrations/{urn}/functional-classifications/{id}/variants ########################################################### @@ -4206,6 +4314,8 @@ def test_anonymous_user_can_get_variants_for_public_calibration( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, @@ -4480,6 +4590,8 @@ def test_anonymous_user_can_get_all_variants_for_public_calibration( experiment["urn"], data_files / "scores.csv", ) + with patch.object(ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) calibration = create_test_score_calibration_in_score_set_via_client( client, @@ -4503,6 +4615,71 @@ def test_anonymous_user_can_get_all_variants_for_public_calibration( assert len(fc_variants["variants"]) == calibration["functionalClassifications"][i]["variantCount"] +def _create_published_calibration_on_private_score_set(client, session, data_provider, data_files) -> dict: + """Create a calibration marked public on a score set that is still private.""" + experiment = create_experiment(client) + score_set = create_seq_score_set_with_mapped_variants( + client, + session, + data_provider, + experiment["urn"], + 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) + ) + + force_publish_test_score_calibration(session, calibration["urn"]) + + return calibration + + +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + [ + {"dbName": "PubMed", "identifier": TEST_PUBMED_IDENTIFIER}, + {"dbName": "bioRxiv", "identifier": TEST_BIORXIV_IDENTIFIER}, + ] + ], + indirect=["mock_publication_fetch"], +) +@pytest.mark.parametrize("route", ["", "/variants", "/functional-classifications/{fc_id}/variants"]) +def test_anonymous_user_cannot_read_published_calibration_on_private_score_set( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files, anonymous_app_overrides, route +): + calibration = _create_published_calibration_on_private_score_set(client, session, data_provider, data_files) + fc_id = calibration["functionalClassifications"][0]["id"] + + with DependencyOverrider(anonymous_app_overrides): + response = client.get(f"/api/v1/score-calibrations/{calibration['urn']}{route.format(fc_id=fc_id)}") + + assert response.status_code == 404 + error = response.json() + assert f"score calibration with URN '{calibration['urn']}' not found" in error["detail"] + + +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + [ + {"dbName": "PubMed", "identifier": TEST_PUBMED_IDENTIFIER}, + {"dbName": "bioRxiv", "identifier": TEST_BIORXIV_IDENTIFIER}, + ] + ], + indirect=["mock_publication_fetch"], +) +def test_score_set_owner_can_read_published_calibration_on_private_score_set( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): + calibration = _create_published_calibration_on_private_score_set(client, session, data_provider, data_files) + + response = client.get(f"/api/v1/score-calibrations/{calibration['urn']}/variants") + + assert response.status_code == 200 + assert len(response.json()) == len(calibration["functionalClassifications"]) + + ########################################################### # Independent calibration creation and publication source validation ########################################################### @@ -4939,6 +5116,54 @@ def test_authenticated_user_sees_own_calibrations( assert calibrations[0]["scoreSetUrn"] == score_set["urn"] +@pytest.mark.parametrize( + "mock_publication_fetch", + [ + [ + {"dbName": "PubMed", "identifier": TEST_PUBMED_IDENTIFIER}, + {"dbName": "bioRxiv", "identifier": TEST_BIORXIV_IDENTIFIER}, + ] + ], + indirect=["mock_publication_fetch"], +) +def test_user_does_not_see_own_calibrations_on_score_sets_they_can_no_longer_read( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files, extra_user_app_overrides +): + experiment = create_experiment(client) + score_set = create_seq_score_set_with_mapped_variants( + client, + session, + data_provider, + experiment["urn"], + data_files / "scores.csv", + ) + add_contributor( + session, + score_set["urn"], + ScoreSetDbModel, + EXTRA_USER["username"], + EXTRA_USER["first_name"], + EXTRA_USER["last_name"], + ) + with DependencyOverrider(extra_user_app_overrides): + create_test_score_calibration_in_score_set_via_client( + client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) + ) + + score_set_item = session.execute( + select(ScoreSetDbModel).where(ScoreSetDbModel.urn == score_set["urn"]) + ).scalar_one() + score_set_item.contributors = [] + session.add(score_set_item) + session.commit() + + with DependencyOverrider(extra_user_app_overrides): + response = client.get("/api/v1/score-calibrations/me") + + assert response.status_code == 200 + assert response.json() == [] + + @pytest.mark.parametrize( "mock_publication_fetch", [ diff --git a/tests/routers/test_score_set.py b/tests/routers/test_score_set.py index 7d9290a9a..ab15a4de3 100644 --- a/tests/routers/test_score_set.py +++ b/tests/routers/test_score_set.py @@ -1142,9 +1142,15 @@ def test_extra_user_can_only_view_published_score_calibrations_in_score_set( ], indirect=["mock_publication_fetch"], ) -def test_creating_user_can_view_all_score_calibrations_in_score_set(client, setup_router_db, mock_publication_fetch): +def test_creating_user_can_view_all_score_calibrations_in_score_set( + client, setup_router_db, mock_publication_fetch, session, data_provider, data_files +): 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") + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) + private_calibration = create_test_score_calibration_in_score_set_via_client( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -4416,6 +4422,8 @@ def test_get_annotated_pathogenicity_evidence_lines_for_score_set( experiment["urn"], data_files / "scores.csv", ) + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -4505,6 +4513,8 @@ def test_get_annotated_pathogenicity_evidence_lines_for_score_set_when_some_vari experiment["urn"], data_files / "scores.csv", ) + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -4547,6 +4557,8 @@ def test_get_annotated_functional_impact_statement_for_score_set( experiment["urn"], data_files / "scores.csv", ) + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) @@ -4638,6 +4650,8 @@ def test_get_annotated_functional_impact_statement_for_score_set_when_some_varia experiment["urn"], data_files / "scores.csv", ) + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + score_set = publish_score_set(client, score_set["urn"]) create_publish_and_promote_score_calibration( client, score_set["urn"], deepcamelize(TEST_BRNICH_SCORE_CALIBRATION_RANGE_BASED) ) diff --git a/tests/scripts/conftest.py b/tests/scripts/conftest.py index af77e8011..eb81dee7c 100644 --- a/tests/scripts/conftest.py +++ b/tests/scripts/conftest.py @@ -287,7 +287,7 @@ def make_dump_score_set(session, sample_user, dump_experiment, dump_licenses, du is how a variant the mapper could not place is stored, and which the README documents as yielding a null `annotation`. Note that the shared `TEST_MINIMAL_MAPPED_VARIANT` uses an empty dict here, a shape production never stores and the annotation layer cannot parse. - published: sets published_date, which the dump's selection query requires. + published: sets published_date, which the dump's selection query requires, and clears private. cc0: whether the score set carries the CC0 license the dump requires. """ counter = {"n": 0} @@ -318,6 +318,7 @@ def _make( modified_by=sample_user, licence_id=CC0_LICENSE_ID if cc0 else OTHER_LICENSE_ID, published_date=date(2024, 1, 1) if published else None, + private=not published, dataset_columns={"score_columns": score_columns, "count_columns": list(count_columns)}, target_genes=[ TargetGene(