From cc762ce608593f84f31541688b5052374b8f75f5 Mon Sep 17 00:00:00 2001 From: David Reinhart Date: Tue, 1 Sep 2026 13:12:24 -0700 Subject: [PATCH 1/5] feat(urns): forward URNs retired by publication to the published record publish_score_set overwrites the tmp: URN of an experiment set, an experiment and a score set in place, and refresh_variant_urns rebuilds every variant URN from the score set's. Nothing recorded the old value, so a link already shared to the unpublished record began returning 404 under a name the caller had no way to guess. Reloading a score set page after publishing it was enough to hit this. Record what each URN became in a new urn_redirects table, and resolve it in forward_retired_urns, an application-wide dependency: a read naming a retired URN is answered 308 to the same path under the record's current URN. One implementation covers every route that takes a URN, sub-resources included, and since substitution operates on the URN substring, a variant follows its score set without a row of its own. A dependency rather than ASGI middleware, because it needs the request's session; middleware runs outside dependency resolution, so it would open a session of its own that no dependency_overrides could redirect. Reads only: an owner is permitted to publish a published score set, so forwarding a stale POST .../publish would rename a live public record. And only onto a target confirmed public, since a Location header names its target to an anonymous caller before any route checks a permission. That check also keeps a deleted record's surviving row from answering a permanent redirect with a 404. The dependency reads the path from the ASGI scope. request.url.path truncates at the '#' in a variant URN, because Starlette rebuilds that URL by re-parsing it, which turns everything after the '#' into a fragment and drops the variant number, the sub-resource and the query string. lib/logging/context.py has the same pattern and is left for a separate change. Forwarding is one hop, which is all that can arise while nothing renames a published record. URNs retired before this are unrecoverable, since publication overwrote them and kept no history, so the table is not backfilled and links to records published earlier stay broken. --- .../c4b18d0f7a92_add_urn_redirects.py | 35 ++++ src/mavedb/lib/urn_redirects.py | 187 ++++++++++++++++++ src/mavedb/models/__init__.py | 1 + src/mavedb/models/urn_redirect.py | 36 ++++ src/mavedb/routers/score_sets.py | 8 + src/mavedb/server_main.py | 7 +- tests/lib/test_urn_redirects.py | 82 ++++++++ tests/routers/test_urn_redirects.py | 154 +++++++++++++++ 8 files changed, 508 insertions(+), 2 deletions(-) create mode 100644 alembic/versions/c4b18d0f7a92_add_urn_redirects.py create mode 100644 src/mavedb/lib/urn_redirects.py create mode 100644 src/mavedb/models/urn_redirect.py create mode 100644 tests/lib/test_urn_redirects.py create mode 100644 tests/routers/test_urn_redirects.py diff --git a/alembic/versions/c4b18d0f7a92_add_urn_redirects.py b/alembic/versions/c4b18d0f7a92_add_urn_redirects.py new file mode 100644 index 000000000..9da5e8dc7 --- /dev/null +++ b/alembic/versions/c4b18d0f7a92_add_urn_redirects.py @@ -0,0 +1,35 @@ +"""Add urn_redirects, forwarding URNs that publication has retired + +Revision ID: c4b18d0f7a92 +Revises: a7f3c2e9b104 +Create Date: 2026-09-01 00:00:00.000000 + +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "c4b18d0f7a92" +down_revision = "a7f3c2e9b104" +branch_labels = None +depends_on = None + + +def upgrade(): + # Not backfilled: publication overwrote each record's temporary URN in place and kept no history of + # it, so the URNs retired before this table existed cannot be recovered. + op.create_table( + "urn_redirects", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("old_urn", sa.String(length=64), nullable=False), + sa.Column("new_urn", sa.String(length=64), nullable=False), + sa.Column("created_at", sa.DateTime(timezone=True), server_default=sa.text("now()"), nullable=False), + sa.PrimaryKeyConstraint("id"), + ) + op.create_index("ix_urn_redirects_old_urn", "urn_redirects", ["old_urn"], unique=True) + + +def downgrade(): + op.drop_index("ix_urn_redirects_old_urn", table_name="urn_redirects") + op.drop_table("urn_redirects") diff --git a/src/mavedb/lib/urn_redirects.py b/src/mavedb/lib/urn_redirects.py new file mode 100644 index 000000000..584a0c232 --- /dev/null +++ b/src/mavedb/lib/urn_redirects.py @@ -0,0 +1,187 @@ +"""Forwarding of URNs that publication has retired. + +A dataset is created with a ``tmp:`` URN and keeps it until it is published, at which point +:func:`mavedb.routers.score_sets.publish_score_set` overwrites the URN in place with a permanent one. +Any link already shared to the unpublished record then stops resolving, which is what +https://github.com/VariantEffect/mavedb-ui/issues/617 reports: the record is still there, under a name +the caller has no way to guess. + +Publication records what each retired URN became, and every read is checked here before it reaches its +route. A request naming a retired URN is answered with ``308 Permanent Redirect`` to the same path +under the record's current URN. + +Three limits: + + - Only a public target is forwarded to. A ``Location`` header names the record it points at, and + forwarding happens before any route checks a permission, so the header would disclose that URN to + an anonymous caller. See _target_is_public. + - Forwarding is one hop. Nothing in the application renames a published record, so a chain cannot + arise; if one ever could, resolution here would need to follow it. + - URNs retired before this was added are unrecoverable. Publication overwrote them and no history of + them was kept, so links to records published earlier stay broken. +""" + +import logging +from typing import Optional +from urllib.parse import quote + +from fastapi import Depends, HTTPException +from sqlalchemy import select +from sqlalchemy.orm import Session +from starlette.requests import Request + +from mavedb.deps import get_db +from mavedb.lib.logging.context import logging_context, save_to_logging_context +from mavedb.lib.validation.urn_re import ( + MAVEDB_EXPERIMENT_SET_URN_RE, + MAVEDB_EXPERIMENT_URN_RE, + MAVEDB_SCORE_SET_URN_RE, + MAVEDB_TMP_URN_RE, +) +from mavedb.models.experiment import Experiment +from mavedb.models.experiment_set import ExperimentSet +from mavedb.models.score_set import ScoreSet +from mavedb.models.urn_redirect import UrnRedirect + +logger = logging.getLogger(__name__) + +# Methods whose requests are forwarded. See forward_retired_urns for why a write is not. +SAFE_METHODS = frozenset({"GET", "HEAD"}) + +# The record kinds publication renames, recognized by the shape of the URN it gave them. Matched with +# fullmatch, under which the three patterns are mutually exclusive. +FORWARDING_TARGET_MODELS = ( + (MAVEDB_SCORE_SET_URN_RE, ScoreSet), + (MAVEDB_EXPERIMENT_URN_RE, Experiment), + (MAVEDB_EXPERIMENT_SET_URN_RE, ExperimentSet), +) + + +def record_urn_redirect(db: Session, old_urn: Optional[str], new_urn: str) -> None: + """ + Record that a record's URN has changed, so that requests naming the old one can be forwarded. + + Staged on the session rather than committed, so that a caller which reassigns several URNs -- as + publication does, across an experiment set, an experiment and a score set -- commits the redirects + together with the renames they describe. + + :param db: An active database session. + :param old_urn: The URN being retired. A record that never had one, or a rename that is not a + change, is not worth a row and is ignored. + :param new_urn: The URN replacing it. + """ + if not old_urn or old_urn == new_urn: + return + + db.add(UrnRedirect(old_urn=old_urn, new_urn=new_urn)) # type: ignore[call-arg] + + +def _target_is_public(db: Session, urn: str) -> bool: + """ + Report whether the record a redirect points to is one that may be named to any caller. + + A ``Location`` header discloses the URN it carries, to whoever asked -- including an anonymous + caller, since forwarding happens before a route checks anything. Publication only ever records a + redirect onto a record it is making public, and nothing in the application returns a published + record to private, so a private target should not arise; a row written out of band, or by some + later feature, would be enough for one to. Withholding on anything but a confirmed public record + keeps that from becoming a disclosure. + + A target that no longer exists is likewise not public: a deleted record leaves its redirect row + behind, and forwarding to it would answer a permanent redirect with a 404. + + :param db: An active database session. + :param urn: The URN a redirect points to. + :return: True only if a record under this URN exists and is public. + """ + for urn_re, model in FORWARDING_TARGET_MODELS: + if urn_re.fullmatch(urn): + private = db.execute(select(model.private).where(model.urn == urn)).scalar_one_or_none() + return private is False + + return False + + +def forwarded_path(db: Session, path: str) -> Optional[str]: + """ + Rewrite a request path so that any retired URN in it names the record's current URN instead. + + Substitution is by substring, not by path segment, so a variant URN -- ``{score_set_urn}#{n}`` -- + is carried along by its score set's redirect. + + :param db: An active database session. + :param path: The decoded request path. + :return: The rewritten path, or None if the path should be served as it is: it names no retired URN, + or one whose target this caller must not be told about. See _target_is_public. + """ + # A live temporary URN belongs to an unpublished record and matches nothing in the table, so the + # lookup below distinguishes the two cases and no separate check for publication is needed. + candidate_urns = set(MAVEDB_TMP_URN_RE.findall(path)) + if not candidate_urns: + return None + + redirects = db.execute( + select(UrnRedirect.old_urn, UrnRedirect.new_urn).where(UrnRedirect.old_urn.in_(candidate_urns)) + ).all() + if not redirects: + return None + + forwarded = path + for old_urn, new_urn in redirects: + if not _target_is_public(db, new_urn): + return None + forwarded = forwarded.replace(old_urn, new_urn) + + return forwarded + + +def forward_retired_urns(request: Request, db: Session = Depends(get_db)) -> None: + """ + Forward a request that names a retired URN to the same resource under its current URN. + + Installed as an application-wide dependency in :mod:`mavedb.server_main`, which is what makes one + implementation cover every route that takes a URN, sub-resources included: a stale link to a score + set's scores CSV or mapped variants is forwarded on the same terms as a link to the score set. + + A dependency rather than ASGI middleware, though it sits at the same single point in the request + path, because it needs the request's database session. Middleware runs outside dependency + resolution, so it would have to open a session of its own, which no ``dependency_overrides`` could + redirect and which would therefore reach past the test database. + + ``308`` rather than ``301``: the redirect is permanent, and 308 forbids a client from rewriting the + request to a GET on the way, which is what makes the header safe to emit for any method. + + Only reads are forwarded. What the issue asks for is that shared *links* keep working, and a write + is a different proposition: the caller addressed a private draft, and the record now under that URN + is published, with different rules and a wider audience. ``POST .../publish`` is the sharp case -- + an owner is permitted to publish a published score set, so forwarding a stale one would rename a + live public record. A write to a retired URN keeps getting the 404 it gets today, which tells the + client to look the record up again. + """ + if request.scope["method"] not in SAFE_METHODS: + return + + # The ASGI scope rather than request.url: Starlette builds request.url by re-parsing the decoded + # path, so a variant URN's '#' starts a fragment there and everything after it -- the variant + # number, the sub-resource, the query -- is silently dropped. + path = request.scope["path"] + query = request.scope.get("query_string", b"").decode("ascii") + + forwarded = forwarded_path(db, path) + if forwarded is None: + return + + # The scope's path is percent-decoded, so a '#' in it has to be re-encoded or it would open a + # fragment in the header. Relative, so that a proxy's scheme and host survive. + location = quote(forwarded, safe="/:") + if query: + location = f"{location}?{query}" + + save_to_logging_context({"requested_resource": path, "forwarded_to": forwarded}) + logger.info(msg="Forwarding a request that named a retired URN.", extra=logging_context()) + + raise HTTPException( + status_code=308, + detail="This URN was replaced when the record was published; the record has moved permanently.", + headers={"Location": location}, + ) diff --git a/src/mavedb/models/__init__.py b/src/mavedb/models/__init__.py index 2f0d65b48..5be50ec13 100644 --- a/src/mavedb/models/__init__.py +++ b/src/mavedb/models/__init__.py @@ -35,6 +35,7 @@ "taxonomy", "uniprot_identifier", "uniprot_offset", + "urn_redirect", "user", "variant_annotation_status", "variant", diff --git a/src/mavedb/models/urn_redirect.py b/src/mavedb/models/urn_redirect.py new file mode 100644 index 000000000..5cd24f38a --- /dev/null +++ b/src/mavedb/models/urn_redirect.py @@ -0,0 +1,36 @@ +""" +SQLAlchemy model for URNs that publication has retired. +""" + +from datetime import datetime + +from sqlalchemy import DateTime, Integer, String, func +from sqlalchemy.orm import Mapped, mapped_column + +from mavedb.db.base import Base + + +class UrnRedirect(Base): + """ + Records that a record's URN was replaced, and by what. + + Publishing a dataset overwrites the ``tmp:`` URN it was created with, so every link already + shared to the unpublished record stops resolving. One row is written here per URN publication + retires, and requests naming a retired URN are forwarded to its replacement. + + Only experiment sets, experiments and score sets get rows. A variant's URN is + ``{score_set_urn}#{n}``, so forwarding replaces the retired URN wherever it appears in a request + path and a variant follows its score set without a row of its own. + """ + + __tablename__ = "urn_redirects" + + id: Mapped[int] = mapped_column(Integer, primary_key=True) + + old_urn: Mapped[str] = mapped_column(String(64), nullable=False, unique=True, index=True) + new_urn: Mapped[str] = mapped_column(String(64), nullable=False) + + created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), nullable=False, server_default=func.now()) + + def __repr__(self) -> str: + return f"" diff --git a/src/mavedb/routers/score_sets.py b/src/mavedb/routers/score_sets.py index aff713429..d38988695 100644 --- a/src/mavedb/routers/score_sets.py +++ b/src/mavedb/routers/score_sets.py @@ -77,6 +77,7 @@ from mavedb.lib.target_genes import find_or_create_target_gene_by_accession, find_or_create_target_gene_by_sequence from mavedb.lib.taxonomies import find_or_create_taxonomy from mavedb.lib.types.authentication import UserData +from mavedb.lib.urn_redirects import record_urn_redirect from mavedb.lib.urns import ( generate_experiment_set_urn, generate_experiment_urn, @@ -2563,7 +2564,9 @@ async def publish_score_set( published_date = date.today() if item.experiment.experiment_set.private or not item.experiment.experiment_set.published_date: + retired_experiment_set_urn = item.experiment.experiment_set.urn item.experiment.experiment_set.urn = generate_experiment_set_urn(db) + record_urn_redirect(db, retired_experiment_set_urn, item.experiment.experiment_set.urn) item.experiment.experiment_set.private = False item.experiment.experiment_set.published_date = published_date db.add(item.experiment.experiment_set) @@ -2571,18 +2574,23 @@ async def publish_score_set( save_to_logging_context({"experiment_set": item.experiment.experiment_set.urn}) if item.experiment.private or not item.experiment.published_date: + retired_experiment_urn = item.experiment.urn item.experiment.urn = generate_experiment_urn( db, item.experiment.experiment_set, experiment_is_meta_analysis=len(item.meta_analyzes_score_sets) > 0, ) + record_urn_redirect(db, retired_experiment_urn, item.experiment.urn) item.experiment.private = False item.experiment.published_date = published_date db.add(item.experiment) save_to_logging_context({"experiment": item.experiment.urn}) + retired_score_set_urn = item.urn item.urn = generate_score_set_urn(db, item.experiment) + # Variant URNs are rewritten below from the score set's, so this one redirect forwards them too. + record_urn_redirect(db, retired_score_set_urn, item.urn) item.private = False item.published_date = published_date refresh_variant_urns(db, item) diff --git a/src/mavedb/server_main.py b/src/mavedb/server_main.py index 880bfcfe1..7f2233e28 100644 --- a/src/mavedb/server_main.py +++ b/src/mavedb/server_main.py @@ -3,7 +3,7 @@ import uvicorn from eutils._internal.exceptions import EutilsRequestError # type: ignore -from fastapi import FastAPI +from fastapi import Depends, FastAPI from fastapi.encoders import jsonable_encoder from fastapi.exceptions import RequestValidationError from fastapi.middleware.cors import CORSMiddleware @@ -37,6 +37,7 @@ from mavedb.lib.middleware import CatchAllErrorMiddleware from mavedb.lib.permissions.exceptions import PermissionException from mavedb.lib.slack import send_slack_error +from mavedb.lib.urn_redirects import forward_retired_urns from mavedb.models import * # noqa: F403 from mavedb.routers import ( access_keys, @@ -75,7 +76,9 @@ # an instance of the related class has been created. configure_mappers() -app = FastAPI() +# forward_retired_urns is applied to every route, so that one implementation forwards a read of a URN +# publication has retired wherever that URN points: a record, or any of its sub-resources. +app = FastAPI(dependencies=[Depends(forward_retired_urns)]) # `add_middleware` inserts at the head of the stack, so the *first* call here is the innermost layer. # CatchAllErrorMiddleware must sit inside both CORSMiddleware and the context middleware: CORS has to # decorate the 500 it produces, and the correlation id it returns comes from the context. diff --git a/tests/lib/test_urn_redirects.py b/tests/lib/test_urn_redirects.py new file mode 100644 index 000000000..9f5efb116 --- /dev/null +++ b/tests/lib/test_urn_redirects.py @@ -0,0 +1,82 @@ +"""Tests for forwarding URNs that publication has retired.""" + +import pytest + +from mavedb.lib.urn_redirects import forwarded_path, record_urn_redirect +from mavedb.models.urn_redirect import UrnRedirect + +from tests.helpers.constants import VALID_SCORE_SET_URN + +RETIRED_URN = "tmp:00000000-0000-4000-8000-000000000001" +PUBLISHED_URN = VALID_SCORE_SET_URN + + +@pytest.mark.integration +class TestRecordUrnRedirect: + def test_records_a_rename(self, session): + record_urn_redirect(session, RETIRED_URN, PUBLISHED_URN) + session.commit() + + redirect = session.query(UrnRedirect).one() + assert redirect.old_urn == RETIRED_URN + assert redirect.new_urn == PUBLISHED_URN + + def test_ignores_a_record_that_had_no_urn(self, session): + record_urn_redirect(session, None, PUBLISHED_URN) + session.commit() + + assert session.query(UrnRedirect).count() == 0 + + def test_ignores_a_rename_that_changes_nothing(self, session): + record_urn_redirect(session, PUBLISHED_URN, PUBLISHED_URN) + session.commit() + + assert session.query(UrnRedirect).count() == 0 + + +@pytest.mark.integration +class TestForwardedPath: + @pytest.fixture + def retired(self, session, setup_lib_db_with_score_set): + """Retire a temporary URN onto a real, public score set. + + A real record is needed, not just a row in the table: forwarding withholds a target it cannot + confirm is public, so a redirect pointing at nothing forwards nowhere. + """ + setup_lib_db_with_score_set.private = False + record_urn_redirect(session, RETIRED_URN, PUBLISHED_URN) + session.commit() + + def test_withholds_a_target_that_is_private(self, session, retired, setup_lib_db_with_score_set): + setup_lib_db_with_score_set.private = True + session.commit() + + assert forwarded_path(session, f"/api/v1/score-sets/{RETIRED_URN}") is None + + def test_withholds_a_target_that_does_not_exist(self, session): + """A deleted record leaves its redirect row behind.""" + record_urn_redirect(session, RETIRED_URN, PUBLISHED_URN) + session.commit() + + assert forwarded_path(session, f"/api/v1/score-sets/{RETIRED_URN}") is None + + def test_forwards_a_retired_urn(self, session, retired): + assert forwarded_path(session, f"/api/v1/score-sets/{RETIRED_URN}") == f"/api/v1/score-sets/{PUBLISHED_URN}" + + def test_forwards_a_sub_resource_of_a_retired_urn(self, session, retired): + assert ( + forwarded_path(session, f"/api/v1/score-sets/{RETIRED_URN}/scores") + == f"/api/v1/score-sets/{PUBLISHED_URN}/scores" + ) + + def test_forwards_a_variant_of_a_retired_score_set(self, session, retired): + """A variant URN is built from its score set's, so the score set's redirect carries it.""" + assert forwarded_path(session, f"/api/v1/variants/{RETIRED_URN}#4") == f"/api/v1/variants/{PUBLISHED_URN}#4" + + def test_leaves_a_path_naming_no_temporary_urn_alone(self, session, retired): + assert forwarded_path(session, f"/api/v1/score-sets/{PUBLISHED_URN}") is None + + def test_leaves_a_live_temporary_urn_alone(self, session, retired): + """An unpublished record still answers to its temporary URN, and has no row in the table.""" + live_urn = "tmp:00000000-0000-4000-8000-000000000002" + assert forwarded_path(session, f"/api/v1/score-sets/{live_urn}") is None diff --git a/tests/routers/test_urn_redirects.py b/tests/routers/test_urn_redirects.py new file mode 100644 index 000000000..cdeeb095a --- /dev/null +++ b/tests/routers/test_urn_redirects.py @@ -0,0 +1,154 @@ +# ruff: noqa: E402 + +from urllib.parse import quote, unquote + +import pytest + +arq = pytest.importorskip("arq") +cdot = pytest.importorskip("cdot") +fastapi = pytest.importorskip("fastapi") + +from mavedb.models.experiment import Experiment as ExperimentDbModel +from mavedb.models.experiment_set import ExperimentSet as ExperimentSetDbModel +from mavedb.models.score_set import ScoreSet as ScoreSetDbModel + +from tests.helpers.util.experiment import create_experiment +from tests.helpers.util.score_set import ( + create_seq_score_set, + create_seq_score_set_with_variants, + publish_score_set, +) + +UNKNOWN_TMP_URN = "tmp:00000000-0000-4000-8000-00000000ffff" + +COLLECTIONS = {"experiment_set": "experiment-sets", "experiment": "experiments", "score_set": "score-sets"} + + +@pytest.fixture +def published(session, data_provider, client, setup_router_db, data_files): + """Publish a score set, and report what each of its records was called before and after.""" + experiment = create_experiment(client) + score_set = create_seq_score_set_with_variants( + client, session, data_provider, experiment["urn"], data_files / "scores.csv" + ) + published_score_set = publish_score_set(client, score_set["urn"]) + + return { + "experiment_set": (experiment["experimentSetUrn"], published_score_set["experiment"]["experimentSetUrn"]), + "experiment": (experiment["urn"], published_score_set["experiment"]["urn"]), + "score_set": (score_set["urn"], published_score_set["urn"]), + } + + +@pytest.mark.integration +class TestForwardingRetiredUrns: + @pytest.mark.parametrize("record", ["experiment_set", "experiment", "score_set"]) + def test_retired_urn_is_forwarded_to_the_published_record(self, client, published, record): + """Publication renames all three records, so a stale link to any of them has to resolve.""" + retired_urn, published_urn = published[record] + collection = COLLECTIONS[record] + + response = client.get(f"/api/v1/{collection}/{retired_urn}", follow_redirects=False) + + assert response.status_code == 308 + assert response.headers["location"] == f"/api/v1/{collection}/{published_urn}" + + @pytest.mark.parametrize("record", ["experiment_set", "experiment", "score_set"]) + def test_a_client_following_the_forward_reaches_the_record(self, client, published, record): + retired_urn, published_urn = published[record] + collection = COLLECTIONS[record] + + response = client.get(f"/api/v1/{collection}/{retired_urn}") + + assert response.status_code == 200 + assert response.json()["urn"] == published_urn + + def test_a_sub_resource_of_a_retired_urn_is_forwarded(self, client, published): + """The complaint in mavedb-ui#617 is about a page, which loads more than the record itself.""" + retired_urn, published_urn = published["score_set"] + + response = client.get(f"/api/v1/score-sets/{retired_urn}/scores", follow_redirects=False) + + assert response.status_code == 308 + assert response.headers["location"] == f"/api/v1/score-sets/{published_urn}/scores" + + def test_a_variant_of_a_retired_score_set_is_forwarded(self, client, published): + """A variant URN is derived from its score set's, and needs no redirect of its own.""" + retired_urn, published_urn = published["score_set"] + + variant_path = f"/api/v1/variants/{quote(f'{retired_urn}#1', safe='')}/csv-namespaces" + + response = client.get(variant_path, follow_redirects=False) + + assert response.status_code == 308 + location = response.headers["location"] + assert unquote(location) == f"/api/v1/variants/{published_urn}#1/csv-namespaces" + # The '#' has to come back encoded, or a client reads the rest of the location as a fragment. + assert "%23" in location + assert client.get(variant_path).status_code == 200 + + def test_a_query_string_survives_forwarding(self, client, published): + retired_urn, published_urn = published["score_set"] + + response = client.get(f"/api/v1/score-sets/{retired_urn}/scores?start=0&limit=1", follow_redirects=False) + + assert response.status_code == 308 + assert response.headers["location"] == f"/api/v1/score-sets/{published_urn}/scores?start=0&limit=1" + + def test_a_live_temporary_urn_is_served_not_forwarded(self, client, setup_router_db): + """An unpublished record still answers to the temporary URN it was created with.""" + experiment = create_experiment(client) + score_set = create_seq_score_set(client, experiment["urn"]) + + response = client.get(f"/api/v1/score-sets/{score_set['urn']}", follow_redirects=False) + + assert response.status_code == 200 + assert response.json()["urn"] == score_set["urn"] + + def test_an_unknown_temporary_urn_is_still_a_404(self, client, setup_router_db): + response = client.get(f"/api/v1/score-sets/{UNKNOWN_TMP_URN}", follow_redirects=False) + + assert response.status_code == 404 + + @pytest.mark.parametrize( + "record,model", + [ + ("score_set", ScoreSetDbModel), + ("experiment", ExperimentDbModel), + ("experiment_set", ExperimentSetDbModel), + ], + ) + def test_a_retired_urn_is_not_forwarded_to_a_private_record(self, session, client, published, record, model): + """A Location header names its target, to an anonymous caller, before any route checks anything. + + Publication only records a redirect onto a record it is making public, and nothing in the + application returns a published record to private, so this state is reached here the only way + it could be reached in production: out of band. + """ + retired_urn, published_urn = published[record] + session.query(model).filter(model.urn == published_urn).one().private = True + session.commit() + + response = client.get(f"/api/v1/{COLLECTIONS[record]}/{retired_urn}", follow_redirects=False) + + assert response.status_code == 404 + assert published_urn not in response.text + + def test_a_retired_urn_is_not_forwarded_to_a_deleted_record(self, session, client, published): + """A deleted record leaves its redirect row behind, pointing at a URN that resolves to nothing.""" + retired_urn, published_urn = published["score_set"] + session.delete(session.query(ScoreSetDbModel).filter(ScoreSetDbModel.urn == published_urn).one()) + session.commit() + + response = client.get(f"/api/v1/score-sets/{retired_urn}", follow_redirects=False) + + assert response.status_code == 404 + assert published_urn not in response.text + + def test_a_write_to_a_retired_urn_is_not_forwarded(self, client, published): + """Forwarding a stale publish request would rename the live public record it reached.""" + retired_urn, _ = published["score_set"] + + response = client.post(f"/api/v1/score-sets/{retired_urn}/publish", follow_redirects=False) + + assert response.status_code == 404 From c2153f3fa4b94ce5a2870e2a9de09880732f627d Mon Sep 17 00:00:00 2001 From: David Reinhart Date: Tue, 8 Sep 2026 08:41:34 -0700 Subject: [PATCH 2/5] test(urns): guard the forwarding lib tests behind the server extra The two "Pytest on Core Dependencies" jobs run poetry install --with dev without --extras server, so fastapi, starlette, arq, cdot and psycopg2 are all absent. tests/lib/test_urn_redirects.py imported mavedb.lib.urn_redirects at module scope, which reaches fastapi and starlette directly and arq, biocommons and cdot through mavedb.deps, so the module failed to import at collection rather than skipping, and one unimportable module ends the whole run: "Interrupted: 1 error during collection". Guard the import the way every sibling module already does. tests/routers/test_urn_redirects.py carried its guards from the start; this one was written without them. Verified under a meta path finder that makes the server extra unimportable: tests/ collects clean and runs 980 passed, 116 skipped, where before it stopped during collection. --- tests/lib/test_urn_redirects.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/lib/test_urn_redirects.py b/tests/lib/test_urn_redirects.py index 9f5efb116..aab0b2da5 100644 --- a/tests/lib/test_urn_redirects.py +++ b/tests/lib/test_urn_redirects.py @@ -1,7 +1,17 @@ """Tests for forwarding URNs that publication has retired.""" +# ruff: noqa: E402 + import pytest +# The module under test reaches fastapi and starlette directly, and arq, biocommons and cdot through +# mavedb.deps, all of which live in the `server` extra. Guarding the import keeps the core-dependency +# CI job skipping this module rather than failing to collect it. The DB fixtures need psycopg2. +pytest.importorskip("psycopg2") +pytest.importorskip("fastapi") +pytest.importorskip("arq") +pytest.importorskip("cdot") + from mavedb.lib.urn_redirects import forwarded_path, record_urn_redirect from mavedb.models.urn_redirect import UrnRedirect From 69e035e6bc804ae8dca0394cacbc675dd22fae2e Mon Sep 17 00:00:00 2001 From: David Reinhart Date: Tue, 15 Sep 2026 13:14:07 -0700 Subject: [PATCH 3/5] fix(score-sets): reject publishing a score set that is already published publish_score_set assigned a fresh URN unconditionally: generate_score_set_urn ran on every call, and nothing checked whether the score set was already public. _handle_publish_action permits an owner to publish their own score set without regard to its state, so an owner could POST .../publish a second time and give a live public record a new URN, retiring the one already cited, indexed and shared. refresh_variant_urns then rewrote every variant URN to match. Reject the second call with a 409 before any URN is generated. This gap predates the URN forwarding in cc762ce6 and is reachable without it, but forwarding is what makes it worth closing now: the redirect table records what each retired URN became, and a repeat publish would fill it with rows retiring permanent URNs, which forwarded_path never matches because it only resolves tmp: shapes. With publication confined to private score sets, every recorded old URN is a temporary one. --- src/mavedb/routers/score_sets.py | 7 +++++++ tests/routers/test_score_set.py | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/mavedb/routers/score_sets.py b/src/mavedb/routers/score_sets.py index d38988695..cf11e3f86 100644 --- a/src/mavedb/routers/score_sets.py +++ b/src/mavedb/routers/score_sets.py @@ -2531,6 +2531,13 @@ async def publish_score_set( assert_permission(user_data, item, Action.PUBLISH) + if not item.private: + logger.info( + msg="Failed to publish score set; The requested score set has already been published.", + extra=logging_context(), + ) + raise HTTPException(status_code=409, detail="This score set has already been published.") + if not item.experiment: logger.info( msg="Failed to publish score set; The requested score set does not belong to an experiment.", diff --git a/tests/routers/test_score_set.py b/tests/routers/test_score_set.py index 7d9290a9a..5c5a395e4 100644 --- a/tests/routers/test_score_set.py +++ b/tests/routers/test_score_set.py @@ -1748,6 +1748,28 @@ def test_publish_score_set(session, data_provider, client, setup_router_db, data assert all([variant.urn.startswith("urn:mavedb:") for variant in score_set_variants]) +def test_cannot_publish_an_already_published_score_set(session, data_provider, client, setup_router_db, data_files): + """Publishing assigns a fresh URN unconditionally, so a second publish would rename a public record.""" + 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_score_set = publish_score_set(client, score_set["urn"]) + + with patch.object(arq.ArqRedis, "enqueue_job", return_value=None) as worker_queue: + response = client.post(f"/api/v1/score-sets/{published_score_set['urn']}/publish") + worker_queue.assert_not_called() + + assert response.status_code == 409 + assert "already been published" in response.json()["detail"] + + # The URN the caller already shared still resolves to this record. + unchanged = client.get(f"/api/v1/score-sets/{published_score_set['urn']}") + assert unchanged.status_code == 200 + assert unchanged.json()["urn"] == published_score_set["urn"] + + def test_publish_score_set_discards_pipeline_when_entrypoint_enqueue_fails( session, data_provider, client, setup_router_db, data_files ): From df88be3471e009732f540ec1f19ce652b5b1d5f1 Mon Sep 17 00:00:00 2001 From: David Reinhart Date: Tue, 15 Sep 2026 14:43:38 -0700 Subject: [PATCH 4/5] refactor(urns): defer redirect target visibility to the permission rules _target_is_public decided whether a redirect's target could be named to an anonymous caller by reading the record's private column. That restated a rule the permission layer already owns, and would drift from it silently: a later change to what makes a record readable would have to be made in both places, or the Location header would disclose a URN a route would have withheld. Fetch the record and ask has_permission(None, target, Action.READ), which covers all three kinds a redirect can point at. Behaviour is unchanged for every case under test: a public target forwards, a private one is withheld, and a target that no longer exists is withheld. Dispatching on the URN pattern with the model named in each branch replaces the table of models the loop read from, which also keeps the fetched record concretely typed rather than as Base. Also cuts the design rationale from forward_retired_urns, leaving what a caller acts on. The removed paragraph on why writes are not forwarded argued from an owner being permitted to republish a published score set, which 69e035e6 no longer allows. --- src/mavedb/lib/urn_redirects.py | 45 +++++++++++---------------------- 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/src/mavedb/lib/urn_redirects.py b/src/mavedb/lib/urn_redirects.py index 584a0c232..d131d38ab 100644 --- a/src/mavedb/lib/urn_redirects.py +++ b/src/mavedb/lib/urn_redirects.py @@ -32,6 +32,8 @@ from mavedb.deps import get_db from mavedb.lib.logging.context import logging_context, save_to_logging_context +from mavedb.lib.permissions.actions import Action +from mavedb.lib.permissions.core import has_permission from mavedb.lib.validation.urn_re import ( MAVEDB_EXPERIMENT_SET_URN_RE, MAVEDB_EXPERIMENT_URN_RE, @@ -48,14 +50,6 @@ # Methods whose requests are forwarded. See forward_retired_urns for why a write is not. SAFE_METHODS = frozenset({"GET", "HEAD"}) -# The record kinds publication renames, recognized by the shape of the URN it gave them. Matched with -# fullmatch, under which the three patterns are mutually exclusive. -FORWARDING_TARGET_MODELS = ( - (MAVEDB_SCORE_SET_URN_RE, ScoreSet), - (MAVEDB_EXPERIMENT_URN_RE, Experiment), - (MAVEDB_EXPERIMENT_SET_URN_RE, ExperimentSet), -) - def record_urn_redirect(db: Session, old_urn: Optional[str], new_urn: str) -> None: """ @@ -84,22 +78,25 @@ def _target_is_public(db: Session, urn: str) -> bool: caller, since forwarding happens before a route checks anything. Publication only ever records a redirect onto a record it is making public, and nothing in the application returns a published record to private, so a private target should not arise; a row written out of band, or by some - later feature, would be enough for one to. Withholding on anything but a confirmed public record - keeps that from becoming a disclosure. + later feature, would be enough for one to. Deferring to ``Action.READ`` with no user keeps that + from becoming a disclosure, and keeps this answer in step with the rules a route would apply. A target that no longer exists is likewise not public: a deleted record leaves its redirect row behind, and forwarding to it would answer a permanent redirect with a 404. :param db: An active database session. :param urn: The URN a redirect points to. - :return: True only if a record under this URN exists and is public. + :return: True only if a record under this URN exists and an anonymous caller may read it. """ - for urn_re, model in FORWARDING_TARGET_MODELS: - if urn_re.fullmatch(urn): - private = db.execute(select(model.private).where(model.urn == urn)).scalar_one_or_none() - return private is False + target: Optional[ScoreSet | Experiment | ExperimentSet] = None + if MAVEDB_SCORE_SET_URN_RE.fullmatch(urn): + target = db.query(ScoreSet).filter(ScoreSet.urn == urn).one_or_none() + elif MAVEDB_EXPERIMENT_URN_RE.fullmatch(urn): + target = db.query(Experiment).filter(Experiment.urn == urn).one_or_none() + elif MAVEDB_EXPERIMENT_SET_URN_RE.fullmatch(urn): + target = db.query(ExperimentSet).filter(ExperimentSet.urn == urn).one_or_none() - return False + return target is not None and has_permission(None, target, Action.READ).permitted def forwarded_path(db: Session, path: str) -> Optional[str]: @@ -143,20 +140,8 @@ def forward_retired_urns(request: Request, db: Session = Depends(get_db)) -> Non implementation cover every route that takes a URN, sub-resources included: a stale link to a score set's scores CSV or mapped variants is forwarded on the same terms as a link to the score set. - A dependency rather than ASGI middleware, though it sits at the same single point in the request - path, because it needs the request's database session. Middleware runs outside dependency - resolution, so it would have to open a session of its own, which no ``dependency_overrides`` could - redirect and which would therefore reach past the test database. - - ``308`` rather than ``301``: the redirect is permanent, and 308 forbids a client from rewriting the - request to a GET on the way, which is what makes the header safe to emit for any method. - - Only reads are forwarded. What the issue asks for is that shared *links* keep working, and a write - is a different proposition: the caller addressed a private draft, and the record now under that URN - is published, with different rules and a wider audience. ``POST .../publish`` is the sharp case -- - an owner is permitted to publish a published score set, so forwarding a stale one would rename a - live public record. A write to a retired URN keeps getting the 404 it gets today, which tells the - client to look the record up again. + Only reads are forwarded. A write to a retired URN keeps getting the 404 it gets today, which + tells the client to look the record up again. """ if request.scope["method"] not in SAFE_METHODS: return From 7aa7e744b6c0432eded54b001624f60042f5eb27 Mon Sep 17 00:00:00 2001 From: David Reinhart Date: Wed, 16 Sep 2026 14:53:11 -0700 Subject: [PATCH 5/5] test(urns): cover every entity type publication renames forwarded_path was only ever exercised against a score set target, so the experiment and experiment set branches df88be34 introduced in _target_is_public had no coverage at this level. The router tests reached them end to end, which is a slower signal and does not isolate the lookup. Drive the fixture and the parametrized tests from one ENTITY_TYPES table that names each type once, carrying the URN the fixture gives it, the route its URN sits under, the attribute path reaching it from the score set the fixtures build, and the sub-resource the API serves after its URN. Adding a type is one entry: the fixture iterates the table and every parametrize takes it as its argument source. Retired URNs come from generate_temp_urn rather than a hand-kept list, since a redirect row is unique on the URN it retires. That holds for as long as new types keep arriving through score set publication, which is the only thing that renames a URN today. An entity published by its own workflow would have no attribute path from a score set, so the fixture would need another way to build it. The table comment says so where someone adding an entry will read it. Every route-bearing test now runs for all three types. The sub-resource test covers the two types that have one, since the API serves nothing after an experiment set's URN, and the variant test stays on score sets, since a variant URN is built from a score set's. --- tests/lib/test_urn_redirects.py | 118 +++++++++++++++++++++++++------- 1 file changed, 94 insertions(+), 24 deletions(-) diff --git a/tests/lib/test_urn_redirects.py b/tests/lib/test_urn_redirects.py index aab0b2da5..4c7410aae 100644 --- a/tests/lib/test_urn_redirects.py +++ b/tests/lib/test_urn_redirects.py @@ -12,14 +12,48 @@ pytest.importorskip("arq") pytest.importorskip("cdot") +from mavedb.lib.temp_urns import generate_temp_urn from mavedb.lib.urn_redirects import forwarded_path, record_urn_redirect from mavedb.models.urn_redirect import UrnRedirect -from tests.helpers.constants import VALID_SCORE_SET_URN +from tests.helpers.constants import VALID_EXPERIMENT_SET_URN, VALID_EXPERIMENT_URN, VALID_SCORE_SET_URN RETIRED_URN = "tmp:00000000-0000-4000-8000-000000000001" +LIVE_TMP_URN = "tmp:00000000-0000-4000-8000-000000000002" PUBLISHED_URN = VALID_SCORE_SET_URN +# Every entity type publication renames. Adding an entry extends the fixture and each parametrized +# test below, which are all driven by these keys. +# +# urn the URN setup_lib_db_with_score_set gives this entity +# route the collection its URN sits under in a request path +# score_set_path the attribute path reaching it from that score set, empty for the score set +# itself. Every type here is renamed by score set publication; an entity with +# its own publishing workflow would need the fixture to build it another way. +# sub_resource a path segment the API serves after its URN, or None where it serves none +ENTITY_TYPES = { + "score_set": { + "urn": VALID_SCORE_SET_URN, + "route": "score-sets", + "score_set_path": "", + "sub_resource": "scores", + }, + "experiment": { + "urn": VALID_EXPERIMENT_URN, + "route": "experiments", + "score_set_path": "experiment", + "sub_resource": "score-sets", + }, + "experiment_set": { + "urn": VALID_EXPERIMENT_SET_URN, + "route": "experiment-sets", + "score_set_path": "experiment.experiment_set", + "sub_resource": None, + }, +} + +ENTITY_TYPES_WITH_SUB_RESOURCE = [name for name, entity in ENTITY_TYPES.items() if entity["sub_resource"]] + @pytest.mark.integration class TestRecordUrnRedirect: @@ -48,45 +82,81 @@ def test_ignores_a_rename_that_changes_nothing(self, session): class TestForwardedPath: @pytest.fixture def retired(self, session, setup_lib_db_with_score_set): - """Retire a temporary URN onto a real, public score set. + """Retire a temporary URN onto a public record of every entity type, keyed by type. A real record is needed, not just a row in the table: forwarding withholds a target it cannot confirm is public, so a redirect pointing at nothing forwards nowhere. """ - setup_lib_db_with_score_set.private = False - record_urn_redirect(session, RETIRED_URN, PUBLISHED_URN) - session.commit() + retired = {} + for entity_type, entity in ENTITY_TYPES.items(): + record = setup_lib_db_with_score_set + for attribute in filter(None, entity["score_set_path"].split(".")): + record = getattr(record, attribute) - def test_withholds_a_target_that_is_private(self, session, retired, setup_lib_db_with_score_set): - setup_lib_db_with_score_set.private = True - session.commit() + record.private = False - assert forwarded_path(session, f"/api/v1/score-sets/{RETIRED_URN}") is None + # A redirect row is unique on the URN it retires, so every type needs its own. + retired_urn = generate_temp_urn() + record_urn_redirect(session, retired_urn, record.urn) + retired[entity_type] = (retired_urn, record) - def test_withholds_a_target_that_does_not_exist(self, session): - """A deleted record leaves its redirect row behind.""" - record_urn_redirect(session, RETIRED_URN, PUBLISHED_URN) session.commit() + return retired - assert forwarded_path(session, f"/api/v1/score-sets/{RETIRED_URN}") is None + @pytest.mark.parametrize("entity_type", ENTITY_TYPES) + def test_forwards_a_retired_urn(self, session, retired, entity_type): + retired_urn, record = retired[entity_type] + route = ENTITY_TYPES[entity_type]["route"] - def test_forwards_a_retired_urn(self, session, retired): - assert forwarded_path(session, f"/api/v1/score-sets/{RETIRED_URN}") == f"/api/v1/score-sets/{PUBLISHED_URN}" + assert forwarded_path(session, f"/api/v1/{route}/{retired_urn}") == f"/api/v1/{route}/{record.urn}" + + @pytest.mark.parametrize("entity_type", ENTITY_TYPES_WITH_SUB_RESOURCE) + def test_forwards_a_sub_resource_of_a_retired_urn(self, session, retired, entity_type): + retired_urn, record = retired[entity_type] + route = ENTITY_TYPES[entity_type]["route"] + sub_resource = ENTITY_TYPES[entity_type]["sub_resource"] - def test_forwards_a_sub_resource_of_a_retired_urn(self, session, retired): assert ( - forwarded_path(session, f"/api/v1/score-sets/{RETIRED_URN}/scores") - == f"/api/v1/score-sets/{PUBLISHED_URN}/scores" + forwarded_path(session, f"/api/v1/{route}/{retired_urn}/{sub_resource}") + == f"/api/v1/{route}/{record.urn}/{sub_resource}" ) + @pytest.mark.parametrize("entity_type", ENTITY_TYPES) + def test_withholds_a_target_that_is_private(self, session, retired, entity_type): + retired_urn, record = retired[entity_type] + route = ENTITY_TYPES[entity_type]["route"] + + record.private = True + session.commit() + + assert forwarded_path(session, f"/api/v1/{route}/{retired_urn}") is None + + @pytest.mark.parametrize("entity_type", ENTITY_TYPES) + def test_withholds_a_target_that_does_not_exist(self, session, entity_type): + """A deleted record leaves its redirect row behind. Without the fixture, none of these exist.""" + entity = ENTITY_TYPES[entity_type] + + record_urn_redirect(session, RETIRED_URN, entity["urn"]) + session.commit() + + assert forwarded_path(session, f"/api/v1/{entity['route']}/{RETIRED_URN}") is None + + @pytest.mark.parametrize("entity_type", ENTITY_TYPES) + def test_leaves_a_path_naming_no_temporary_urn_alone(self, session, retired, entity_type): + _, record = retired[entity_type] + route = ENTITY_TYPES[entity_type]["route"] + + assert forwarded_path(session, f"/api/v1/{route}/{record.urn}") is None + def test_forwards_a_variant_of_a_retired_score_set(self, session, retired): """A variant URN is built from its score set's, so the score set's redirect carries it.""" - assert forwarded_path(session, f"/api/v1/variants/{RETIRED_URN}#4") == f"/api/v1/variants/{PUBLISHED_URN}#4" + retired_urn, score_set = retired["score_set"] - def test_leaves_a_path_naming_no_temporary_urn_alone(self, session, retired): - assert forwarded_path(session, f"/api/v1/score-sets/{PUBLISHED_URN}") is None + assert forwarded_path(session, f"/api/v1/variants/{retired_urn}#4") == f"/api/v1/variants/{score_set.urn}#4" - def test_leaves_a_live_temporary_urn_alone(self, session, retired): + @pytest.mark.parametrize("entity_type", ENTITY_TYPES) + def test_leaves_a_live_temporary_urn_alone(self, session, retired, entity_type): """An unpublished record still answers to its temporary URN, and has no row in the table.""" - live_urn = "tmp:00000000-0000-4000-8000-000000000002" - assert forwarded_path(session, f"/api/v1/score-sets/{live_urn}") is None + route = ENTITY_TYPES[entity_type]["route"] + + assert forwarded_path(session, f"/api/v1/{route}/{LIVE_TMP_URN}") is None