fix(permissions): require score set visibility to read a score calibration

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:

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
 

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()

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,

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")
 

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
 
 

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,

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(

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."""
 

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)

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):

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)
     )