diff --git a/alembic/versions/c4b18d0f7a92_add_urn_redirects.py b/alembic/versions/c4b18d0f7a92_add_urn_redirects.py new file mode 100644 index 00000000..9da5e8dc --- /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 00000000..584a0c23 --- /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 2f0d65b4..5be50ec1 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 00000000..5cd24f38 --- /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 aff71342..d3898869 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 880bfcfe..7f2233e2 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 00000000..9f5efb11 --- /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 00000000..cdeeb095 --- /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