diff --git a/pyproject.toml b/pyproject.toml index f8d0de49..44135b9e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "poetry.core.masonry.api" [tool.poetry] name = "mavedb" -version = "2026.2.7.1" +version = "2026.2.7.2" description = "API for MaveDB, the database of Multiplexed Assays of Variant Effect." license = "AGPL-3.0-only" readme = "README.md" diff --git a/src/mavedb/__init__.py b/src/mavedb/__init__.py index a24dc316..9e6e7566 100644 --- a/src/mavedb/__init__.py +++ b/src/mavedb/__init__.py @@ -6,7 +6,7 @@ logger = module_logging.getLogger(__name__) __project__ = "mavedb-api" -__version__ = "2026.2.7.1" +__version__ = "2026.2.7.2" logger.info(f"MaveDB {__version__}") diff --git a/src/mavedb/routers/experiments.py b/src/mavedb/routers/experiments.py index debaebcb..b78ca27d 100644 --- a/src/mavedb/routers/experiments.py +++ b/src/mavedb/routers/experiments.py @@ -112,6 +112,16 @@ def search_experiments(search: ExperimentsSearch, db: Session = Depends(deps.get """ Search experiments. """ + # This endpoint is unauthenticated, so it serves published experiments only. `build_search_experiments_query_filter` + # narrows by owner or contributor, not by visibility, and receives None here; without this the search would return + # every unpublished experiment in the database. Private experiments are reached through /me/experiments/search. + if search.published is False: + raise HTTPException( + status_code=422, + detail="Cannot search for private experiments except in the context of the current user's data.", + ) + search.published = True + items = _search_experiments(db, None, search) return [enrich_experiment_with_num_score_sets(exp, None) for exp in items] diff --git a/src/mavedb/routers/score_sets.py b/src/mavedb/routers/score_sets.py index 843db6f1..aff71342 100644 --- a/src/mavedb/routers/score_sets.py +++ b/src/mavedb/routers/score_sets.py @@ -582,17 +582,18 @@ async def fetch_score_set_by_urn( :param user: The user who has requested the score set. If the user does not have read permission, the score set will not be returned. If None, the score set is returned only if publicly visible. :param owner_or_contributor: If not None, require that the result be a score set of which this user is owner or - contributor. + contributor. This is an ownership requirement, not a visibility one: it does not admit score sets that are + merely public. Combining it with only_published therefore yields published score sets owned by this user. :param only_published: If true, only return the score set if it is published. - :return: The score set, or None if the URL was not found or refers to a private score set not owned by the specified - user. + :return: The score set. + :raises HTTPException: 404 if no score set matches the URN and the supplied filters, or 500 if more than one + does. Read permission is asserted on the result and raises through assert_permission. """ try: query = db.query(ScoreSet).filter(ScoreSet.urn == urn) if owner_or_contributor is not None: query = query.filter( or_( - ScoreSet.private.is_(False), ScoreSet.created_by_id == owner_or_contributor.user.id, ScoreSet.contributors.any(Contributor.orcid_id == owner_or_contributor.user.username), ) @@ -734,15 +735,17 @@ def search_score_sets( ) score_sets, num_score_sets = _search_score_sets(db, None, search).values() + + # Unconditional, because this enrichment is also what filters the nested experiment's score set URNs by + # permission. Serializing the ORM experiment directly instead reaches SavedExperiment's score_set_urns + # validator, which lists every score set on the experiment, disclosing the URNs of private ones. enriched_score_sets = [] - if search.include_experiment_score_set_urns_and_count: - for ss in score_sets: - enriched_experiment = enrich_experiment_with_num_score_sets(ss.experiment, user_data) - response_item = score_set.ScoreSet.model_validate(ss).copy(update={"experiment": enriched_experiment}) - enriched_score_sets.append(response_item) - score_sets = enriched_score_sets + for ss in score_sets: + enriched_experiment = enrich_experiment_with_num_score_sets(ss.experiment, user_data) + response_item = score_set.ScoreSet.model_validate(ss).copy(update={"experiment": enriched_experiment}) + enriched_score_sets.append(response_item) - return {"score_sets": score_sets, "num_score_sets": num_score_sets} + return {"score_sets": enriched_score_sets, "num_score_sets": num_score_sets} @router.post("/score-sets/search/filter-options", status_code=200, response_model=ScoreSetsSearchFilterOptionsResponse) @@ -1763,6 +1766,8 @@ async def create_score_set( save_to_logging_context({"requested_superseded_score_set": item_create.superseded_score_set_urn}) if item_create.superseded_score_set_urn is not None: + # Passing user_data as owner_or_contributor is what authorizes the supersession: the fetch returns + # only published score sets this user owns or contributes to. There is no Action for supersession yet. superseded_score_set = await fetch_score_set_by_urn( db, item_create.superseded_score_set_urn, user_data, user_data, True ) diff --git a/src/mavedb/routers/target_genes.py b/src/mavedb/routers/target_genes.py index a304ee89..67513578 100644 --- a/src/mavedb/routers/target_genes.py +++ b/src/mavedb/routers/target_genes.py @@ -1,7 +1,7 @@ from typing import Any, List, Optional from fastapi import APIRouter, Depends, HTTPException -from sqlalchemy.orm import Session, selectinload +from sqlalchemy.orm import Query, Session, selectinload from mavedb import deps from mavedb.lib.authentication import get_current_user @@ -83,16 +83,27 @@ def list_target_genes( return sorted(validated_items, key=lambda i: i.name) +def _published_target_genes(db: Session) -> Query[TargetGene]: + """ + Query the target genes belonging to published score sets. + + The two routes below aggregate over the whole table and are unauthenticated, so they have no entity to + assert a permission on. The parent score set's published state is what keeps the target names and + categories of unpublished work out of the response. + """ + return db.query(TargetGene).join(ScoreSet).filter(ScoreSet.published_date.is_not(None)) + + @router.get("/target-genes/names", status_code=200, response_model=List[str], summary="List target gene names") def list_target_gene_names( *, db: Session = Depends(deps.get_db), ) -> Any: """ - List distinct target gene names, in alphabetical order. + List distinct target gene names from published score sets, in alphabetical order. """ - items = db.query(TargetGene).all() + items = _published_target_genes(db).all() names = map(lambda item: item.name, items) return sorted(list(set(names))) @@ -105,10 +116,10 @@ def list_target_gene_categories( db: Session = Depends(deps.get_db), ) -> Any: """ - List distinct target genes categories, in alphabetical order. + List distinct target gene categories from published score sets, in alphabetical order. """ - items = db.query(TargetGene).all() + items = _published_target_genes(db).all() categories = map(lambda item: item.category, items) return sorted(list(set(categories))) diff --git a/src/mavedb/view_models/search.py b/src/mavedb/view_models/search.py index 778b3adb..22959599 100644 --- a/src/mavedb/view_models/search.py +++ b/src/mavedb/view_models/search.py @@ -32,7 +32,6 @@ class ScoreSetsSearch(BaseModel): publication_identifiers: Optional[list[str]] = None keywords: Optional[list[ControlledKeywordSearch]] = None text: Optional[str] = None - include_experiment_score_set_urns_and_count: Optional[bool] = True offset: Optional[int] = None limit: Optional[int] = None diff --git a/tests/routers/test_experiments.py b/tests/routers/test_experiments.py index 60d55214..acd94939 100644 --- a/tests/routers/test_experiments.py +++ b/tests/routers/test_experiments.py @@ -1574,14 +1574,61 @@ def test_users_get_one_score_set_from_own_experiment_with_a_superseding_score_se assert pub_score_set["urn"] not in response_data["scoreSetUrns"] -def test_search_experiments(session, client, setup_router_db): - experiment = create_experiment(client) +def _publish_experiment(session, data_provider, client, data_files, update=None): + """Publish an experiment, and return it. + + Publishing a score set is the only path that publishes its experiment, so an experiment cannot be + published without one. + """ + experiment = create_experiment(client, update) + 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): + published_score_set = publish_score_set(client, score_set["urn"]) + + return published_score_set["experiment"] + + +def test_search_experiments(session, data_provider, client, setup_router_db, data_files): + experiment = _publish_experiment(session, data_provider, client, data_files) search_payload = {"text": experiment["shortDescription"]} response = client.post("/api/v1/experiments/search", json=search_payload) assert response.status_code == 200 assert response.json()[0]["title"] == experiment["title"] +def test_search_experiments_excludes_unpublished(session, data_provider, client, setup_router_db, data_files): + """The public search endpoint serves published experiments only. + + Both experiments match the search text, so this fails whether the visibility filter is too permissive + or too restrictive. Asserting only that an unpublished experiment is absent would also pass if the + search returned nothing at all. + """ + published = _publish_experiment( + session, data_provider, client, data_files, update={"title": "Published Experiment"} + ) + unpublished = create_experiment(client, update={"title": "Unpublished Experiment"}) + + search_payload = {"text": TEST_MINIMAL_EXPERIMENT["shortDescription"]} + response = client.post("/api/v1/experiments/search", json=search_payload) + + assert response.status_code == 200 + returned_urns = [item["urn"] for item in response.json()] + assert published["urn"] in returned_urns + assert unpublished["urn"] not in returned_urns + + +def test_search_experiments_rejects_explicit_unpublished_search(session, client, setup_router_db): + """Unpublished experiments are reached through /me/experiments/search, never this endpoint.""" + response = client.post("/api/v1/experiments/search", json={"published": False}) + assert response.status_code == 422 + assert ( + response.json()["detail"] + == "Cannot search for private experiments except in the context of the current user's data." + ) + + def test_search_my_experiments(session, client, setup_router_db): experiment = create_experiment(client) search_payload = {"text": experiment["shortDescription"]} @@ -1934,8 +1981,8 @@ def test_search_score_sets_for_my_experiments(session, client, setup_router_db, ) -def test_search_their_experiments(session, client, setup_router_db): - experiment = create_experiment(client) +def test_search_their_experiments(session, data_provider, client, setup_router_db, data_files): + experiment = _publish_experiment(session, data_provider, client, data_files) change_ownership(session, experiment["urn"], ExperimentDbModel) change_ownership(session, experiment["experimentSetUrn"], ExperimentSetDbModel) search_payload = {"text": experiment["shortDescription"]} @@ -1945,6 +1992,22 @@ def test_search_their_experiments(session, client, setup_router_db): assert response.json()[0]["createdBy"]["firstName"] == EXTRA_USER["first_name"] +def test_cannot_search_their_unpublished_experiments(session, client, setup_router_db): + """Another user's unpublished experiment is not disclosed by the public search endpoint. + + Regression test: build_search_experiments_query_filter narrows by owner or contributor and receives + None from this endpoint, so before the visibility filter was added this returned the experiment along + with its owner's name and ORCID iD. + """ + experiment = create_experiment(client) + change_ownership(session, experiment["urn"], ExperimentDbModel) + change_ownership(session, experiment["experimentSetUrn"], ExperimentSetDbModel) + search_payload = {"text": experiment["shortDescription"]} + response = client.post("/api/v1/experiments/search", json=search_payload) + assert response.status_code == 200 + assert experiment["urn"] not in [item["urn"] for item in response.json()] + + def test_search_not_my_experiments(session, client, setup_router_db): experiment = create_experiment(client) change_ownership(session, experiment["urn"], ExperimentDbModel) @@ -1955,13 +2018,27 @@ def test_search_not_my_experiments(session, client, setup_router_db): assert len(response.json()) == 0 -def test_anonymous_search_experiments(session, client, anonymous_app_overrides, setup_router_db): - experiment = create_experiment(client) - search_payload = {"text": experiment["shortDescription"]} +def test_anonymous_search_experiments( + session, data_provider, client, anonymous_app_overrides, setup_router_db, data_files +): + """An anonymous caller sees published experiments, and only those. + + Both experiments match the search text, so this fails whether the visibility filter is too permissive + or too restrictive. + """ + published = _publish_experiment( + session, data_provider, client, data_files, update={"title": "Published Experiment"} + ) + unpublished = create_experiment(client, update={"title": "Unpublished Experiment"}) + + search_payload = {"text": TEST_MINIMAL_EXPERIMENT["shortDescription"]} with DependencyOverrider(anonymous_app_overrides): response = client.post("/api/v1/experiments/search", json=search_payload) + assert response.status_code == 200 - assert response.json()[0]["title"] == experiment["title"] + returned_urns = [item["urn"] for item in response.json()] + assert published["urn"] in returned_urns + assert unpublished["urn"] not in returned_urns def test_anonymous_cannot_search_my_experiments(session, client, anonymous_app_overrides, setup_router_db): diff --git a/tests/routers/test_score_set.py b/tests/routers/test_score_set.py index 26a45bb3..7d9290a9 100644 --- a/tests/routers/test_score_set.py +++ b/tests/routers/test_score_set.py @@ -362,6 +362,100 @@ def test_cannot_create_score_set_with_invalid_target_gene_category(client, mock_ assert all(field in response_data["detail"][0]["msg"] for field in TargetCategory._member_names_) +######################################################################################################################## +# Score set supersession +######################################################################################################################## + + +def _publish_score_set_owned_by_extra_user(session, data_provider, client, data_files): + """Create and publish a score set, then reassign it to the extra user.""" + 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): + published = publish_score_set(client, score_set["urn"]) + + change_ownership(session, published["urn"], ScoreSetDbModel) + return published + + +def test_owner_can_supersede_own_published_score_set(session, data_provider, client, setup_router_db, 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): + published = publish_score_set(client, score_set["urn"]) + + score_set_post_payload = deepcopy(TEST_MINIMAL_SEQ_SCORESET) + score_set_post_payload["experimentUrn"] = published["experiment"]["urn"] + score_set_post_payload["supersededScoreSetUrn"] = published["urn"] + + response = client.post("/api/v1/score-sets/", json=score_set_post_payload) + assert response.status_code == 200 + assert response.json()["supersededScoreSet"]["urn"] == published["urn"] + + +def test_cannot_supersede_other_users_published_score_set(session, data_provider, client, setup_router_db, data_files): + """A published score set may only be superseded by its owner or a contributor. + + Regression test: fetch_score_set_by_urn's owner_or_contributor filter previously admitted any + non-private score set, which only_published already guaranteed, so this call was unauthorized. + """ + published = _publish_score_set_owned_by_extra_user(session, data_provider, client, data_files) + + score_set_post_payload = deepcopy(TEST_MINIMAL_SEQ_SCORESET) + score_set_post_payload["experimentUrn"] = published["experiment"]["urn"] + score_set_post_payload["supersededScoreSetUrn"] = published["urn"] + + response = client.post("/api/v1/score-sets/", json=score_set_post_payload) + assert response.status_code == 404 + assert published["urn"] in response.json()["detail"] + + +def test_cannot_lock_owner_out_of_superseding_their_own_score_set( + session, data_provider, client, setup_router_db, data_files +): + """Supersession is one-shot, so an unauthorized claim would permanently block the real owner.""" + published = _publish_score_set_owned_by_extra_user(session, data_provider, client, data_files) + + score_set_post_payload = deepcopy(TEST_MINIMAL_SEQ_SCORESET) + score_set_post_payload["experimentUrn"] = published["experiment"]["urn"] + score_set_post_payload["supersededScoreSetUrn"] = published["urn"] + assert client.post("/api/v1/score-sets/", json=score_set_post_payload).status_code == 404 + + # The owner's own supersession must still be available afterwards. + response = client.get(f"/api/v1/score-sets/{published['urn']}") + assert response.status_code == 200 + assert response.json().get("supersedingScoreSet") is None + + +def test_contributor_can_supersede_score_set( + session, data_provider, client, setup_router_db, data_files, extra_user_app_overrides +): + """A contributor to a published score set may record its successor, as well as its owner.""" + experiment = create_experiment(client, {"contributors": [{"orcidId": EXTRA_USER["username"]}]}) + score_set = create_seq_score_set( + client, experiment["urn"], update={"contributors": [{"orcidId": EXTRA_USER["username"]}]} + ) + 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): + published = publish_score_set(client, score_set["urn"]) + + score_set_post_payload = deepcopy(TEST_MINIMAL_SEQ_SCORESET) + score_set_post_payload["experimentUrn"] = published["experiment"]["urn"] + score_set_post_payload["supersededScoreSetUrn"] = published["urn"] + + # The extra user contributes to the score set but does not own it. + with DependencyOverrider(extra_user_app_overrides): + response = client.post("/api/v1/score-sets/", json=score_set_post_payload) + + assert response.status_code == 200 + assert response.json()["supersededScoreSet"]["urn"] == published["urn"] + + ######################################################################################################################## # Score set updating ######################################################################################################################## @@ -2519,6 +2613,48 @@ def test_search_public_score_sets_no_match(session, data_provider, client, setup assert len(response.json()["scoreSets"]) == 0 +def test_search_public_score_sets_does_not_disclose_private_sibling_urns( + session, data_provider, client, anonymous_app_overrides, setup_router_db, data_files +): + """A search result's experiment lists only the score set URNs the caller may read. + + Regression test: the enrichment that filters those URNs used to be skipped when the request set + includeExperimentScoreSetUrnsAndCount to false, and SavedExperiment's validator then listed every score + set on the experiment. The field is gone, and an unknown field is ignored rather than rejected, so the + old request shape must now be filtered too. + """ + experiment = create_experiment(client, {"title": "Experiment 1"}) + published = create_seq_score_set(client, experiment["urn"], update={"title": "Test Fnord Score Set"}) + published = mock_worker_variant_insertion(client, session, data_provider, published, data_files / "scores.csv") + + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + published = publish_score_set(client, published["urn"]) + + # Unpublished, and inside the now-public experiment. This is the URN that leaked. + private = create_seq_score_set( + client, published["experiment"]["urn"], update={"title": "Unpublished Fnord Score Set"} + ) + + # The owner may read both, so the enrichment is permission-scoped rather than a blanket strip. + response = client.post("/api/v1/score-sets/search", json={"text": "fnord"}) + assert response.status_code == 200 + assert set(response.json()["scoreSets"][0]["experiment"]["scoreSetUrns"]) == {published["urn"], private["urn"]} + + for search_payload in ( + {"text": "fnord"}, + {"text": "fnord", "includeExperimentScoreSetUrnsAndCount": False}, + ): + with DependencyOverrider(anonymous_app_overrides): + response = client.post("/api/v1/score-sets/search", json=search_payload) + + assert response.status_code == 200 + assert len(response.json()["scoreSets"]) == 1 + + score_set_urns = response.json()["scoreSets"][0]["experiment"]["scoreSetUrns"] + assert published["urn"] in score_set_urns + assert private["urn"] not in score_set_urns + + def test_search_public_score_sets_match(session, data_provider, client, setup_router_db, data_files): experiment = create_experiment(client, {"title": "Experiment 1"}) score_set = create_seq_score_set(client, experiment["urn"], update={"title": "Test Fnord Score Set"}) diff --git a/tests/routers/test_target_gene.py b/tests/routers/test_target_gene.py index 5ca1b4a2..63887cfb 100644 --- a/tests/routers/test_target_gene.py +++ b/tests/routers/test_target_gene.py @@ -1,5 +1,6 @@ # ruff: noqa: E402 import pytest +from copy import deepcopy from unittest.mock import patch arq = pytest.importorskip("arq") @@ -8,7 +9,8 @@ from mavedb.models.score_set import ScoreSet as ScoreSetDbModel -from tests.helpers.constants import TEST_USER +from tests.helpers.constants import TEST_MINIMAL_SEQ_SCORESET, TEST_USER +from tests.helpers.dependency_overrider import DependencyOverrider from tests.helpers.util.contributor import add_contributor from tests.helpers.util.experiment import create_experiment from tests.helpers.util.user import change_ownership @@ -165,3 +167,62 @@ def test_fetch_public_target_gene_by_id(session, data_provider, client, setup_ro response = client.get("/api/v1/target-genes/1") assert response.status_code == 200 assert response.json()["scoreSetUrn"] == published_score_set["urn"] + + +def _score_set_with_target(client, experiment_urn, name, category): + """Create a score set whose single target gene carries the given name and category.""" + target_genes = deepcopy(TEST_MINIMAL_SEQ_SCORESET["targetGenes"]) + target_genes[0]["name"] = name + target_genes[0]["category"] = category + return create_seq_score_set(client, experiment_urn, update={"targetGenes": target_genes}) + + +def _published_and_unpublished_targets(session, data_provider, client, data_files): + """Put one published and one unpublished score set, with distinct target genes, in one experiment. + + The unpublished score set is created after the publish, against the experiment's published URN, so that + it sits inside a public experiment. That is the arrangement in which its target gene leaks. + """ + experiment = create_experiment(client, {"title": "Experiment 1"}) + published = _score_set_with_target(client, experiment["urn"], "PUBLISHEDGENE", "protein_coding") + published = mock_worker_variant_insertion(client, session, data_provider, published, data_files / "scores.csv") + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None): + published = publish_score_set(client, published["urn"]) + + _score_set_with_target(client, published["experiment"]["urn"], "UNPUBLISHEDGENE", "regulatory") + + +def test_list_target_gene_names_excludes_unpublished(session, data_provider, client, setup_router_db, data_files): + """Target gene names from unpublished score sets are not disclosed by this unauthenticated route. + + Asserted against one published and one unpublished score set so that the test fails whether the filter + is missing or too aggressive. + """ + _published_and_unpublished_targets(session, data_provider, client, data_files) + + response = client.get("/api/v1/target-genes/names") + assert response.status_code == 200 + assert "PUBLISHEDGENE" in response.json() + assert "UNPUBLISHEDGENE" not in response.json() + + +def test_list_target_gene_categories_excludes_unpublished(session, data_provider, client, setup_router_db, data_files): + _published_and_unpublished_targets(session, data_provider, client, data_files) + + response = client.get("/api/v1/target-genes/categories") + assert response.status_code == 200 + assert "protein_coding" in response.json() + assert "regulatory" not in response.json() + + +def test_anonymous_list_target_gene_names_excludes_unpublished( + session, data_provider, client, anonymous_app_overrides, setup_router_db, data_files +): + _published_and_unpublished_targets(session, data_provider, client, data_files) + + with DependencyOverrider(anonymous_app_overrides): + response = client.get("/api/v1/target-genes/names") + + assert response.status_code == 200 + assert "PUBLISHEDGENE" in response.json() + assert "UNPUBLISHEDGENE" not in response.json()