diff --git a/backend/app/services/github_backup.py b/backend/app/services/github_backup.py index 26d8632ab..1319471e8 100644 --- a/backend/app/services/github_backup.py +++ b/backend/app/services/github_backup.py @@ -869,6 +869,13 @@ class GitHubBackupService: # deleted_at is what lets a restore put them back the way they # were instead of resurrecting them as visible archives. "deleted_at": str(a.deleted_at) if a.deleted_at else None, + # Who owns the archive, for the same reason deleted_at is here: + # it is not decoration, it is what the access check runs on. + # _ensure_archive_visible (api/routes/archives.py) fails closed on + # a NULL created_by_id and the list paths filter on it, so a + # restored row without it is invisible to everyone but an admin — + # while the restore reports it restored. + "created_by_id": a.created_by_id, } archive_list.append(archive_data) diff --git a/backend/app/services/github_restore.py b/backend/app/services/github_restore.py index 66531549d..4a7f1b688 100644 --- a/backend/app/services/github_restore.py +++ b/backend/app/services/github_restore.py @@ -48,6 +48,7 @@ from backend.app.models.project import Project from backend.app.models.settings import Settings from backend.app.models.spool import Spool from backend.app.models.spool_usage_history import SpoolUsageHistory +from backend.app.models.user import User from backend.app.schemas.github_backup import RestoreCategory from backend.app.services.git_providers.factory import get_provider_backend from backend.app.services.printer_manager import printer_manager @@ -760,6 +761,19 @@ class GitHubRestoreService: valid_printers = set((await db.execute(select(Printer.id))).scalars().all()) valid_projects = set((await db.execute(select(Project.id))).scalars().all()) + # Ownership decides visibility, not just attribution: an archive with a + # NULL created_by_id is a 404 to every caller without archives:read_all + # (_ensure_archive_visible fails closed on it) and never appears in the + # ownership-scoped list queries. Hoisted like the two above. + # + # Note this is the one place a raw backup id is reused, against the + # module's own rule at the top of the file. Users have no natural key the + # backup carries today, and the id is validated rather than trusted, so a + # *stale* id clears instead of pointing somewhere wrong. What it cannot + # catch is a live id belonging to a different person on a different + # instance. Collecting username and resolving on that would close it; + # raised with the maintainer rather than decided here. + valid_users = set((await db.execute(select(User.id))).scalars().all()) # Only metadata is backed up, never the 3MF/thumbnail bytes, and # print_archives.file_path is NOT NULL — so inserted rows get an empty @@ -817,8 +831,20 @@ class GitHubRestoreService: if project_id is not None and project_id not in valid_projects: tally.note("Some archives referenced projects that no longer exist — link cleared") project_id = None + created_by_id = entry.get("created_by_id") + if created_by_id is not None and created_by_id not in valid_users: + # Coerced rather than failing the row: the archive is still worth + # having, and an admin can reassign it. Said out loud because a + # cleared owner is not silent-safe — the archive becomes visible + # only to archives:read_all until someone does. + tally.note( + "Some archives referenced users that no longer exist — owner cleared, so they are " + "visible only to users with the archives:read_all permission until an admin reassigns them" + ) + created_by_id = None fields["printer_id"] = printer_id fields["project_id"] = project_id + fields["created_by_id"] = created_by_id if existing is not None: if old_id is not None: diff --git a/backend/tests/integration/test_github_restore_api.py b/backend/tests/integration/test_github_restore_api.py index 97fc2d0c5..3032dd8de 100644 --- a/backend/tests/integration/test_github_restore_api.py +++ b/backend/tests/integration/test_github_restore_api.py @@ -4,6 +4,9 @@ from unittest.mock import AsyncMock, patch import pytest from httpx import AsyncClient +from sqlalchemy import select + +from backend.tests.integration.test_ownership_permissions import TestOwnershipPermissionsSetup @pytest.fixture(autouse=True) @@ -276,6 +279,83 @@ class TestStatusExposesRestoreState: assert response.json()["restore_running"] is False +class TestRestoredArchivesAreVisibleToTheirOwner(TestOwnershipPermissionsSetup): + """The archive-ownership blocker, proved through the route that enforces it. + + ``_ensure_archive_visible`` fails closed on a NULL ``created_by_id`` — 404 for + any caller without ``archives:read_all`` — so before the collector and the + restore carried the column across, a multi-user instance got archives the + tally called restored and their owner could not open. + """ + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_the_owning_non_admin_can_open_a_restored_archive( + self, async_client: AsyncClient, auth_setup, db_session + ): + from backend.app.models.archive import PrintArchive + from backend.app.services.github_restore import _CategoryTally, github_restore_service + + owner_id = auth_setup["operator_user"]["id"] + payload = { + "archives": [ + { + "id": 77, + "filename": "benchy.3mf", + "file_size": 2048, + "content_hash": "abc123", + "print_name": "Benchy", + "started_at": "2026-03-01 10:00:00", + "created_at": "2026-03-01 10:00:00", + "created_by_id": owner_id, + } + ] + } + await github_restore_service._restore_archives(db_session, payload, False, _CategoryTally(), {}) + await db_session.commit() + + restored = (await db_session.execute(select(PrintArchive))).scalar_one() + assert restored.id != 77, "the backup's primary key must not be reused" + + response = await async_client.get( + f"/api/v1/archives/{restored.id}", + headers={"Authorization": f"Bearer {auth_setup['operator_token']}"}, + ) + + assert response.status_code == 200, "the owner cannot see their own restored archive" + assert response.json()["print_name"] == "Benchy" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_a_different_operator_still_cannot(self, async_client: AsyncClient, auth_setup, db_session): + """Control: carrying the owner across must not widen who can read it.""" + from backend.app.models.archive import PrintArchive + from backend.app.services.github_restore import _CategoryTally, github_restore_service + + payload = { + "archives": [ + { + "id": 77, + "filename": "benchy.3mf", + "file_size": 2048, + "content_hash": "abc123", + "started_at": "2026-03-01 10:00:00", + "created_by_id": auth_setup["operator_user"]["id"], + } + ] + } + await github_restore_service._restore_archives(db_session, payload, False, _CategoryTally(), {}) + await db_session.commit() + + restored = (await db_session.execute(select(PrintArchive))).scalar_one() + response = await async_client.get( + f"/api/v1/archives/{restored.id}", + headers={"Authorization": f"Bearer {auth_setup['operator2_token']}"}, + ) + + assert response.status_code == 404 + + class TestRestoreDoesNotOpenTheMetricsEndpoint: """The companion-credential rule, proved against the endpoint it protects. diff --git a/backend/tests/unit/test_github_restore.py b/backend/tests/unit/test_github_restore.py index e00aa5d4c..a7170c7c3 100644 --- a/backend/tests/unit/test_github_restore.py +++ b/backend/tests/unit/test_github_restore.py @@ -17,6 +17,7 @@ from backend.app.models.archive import PrintArchive from backend.app.models.settings import Settings from backend.app.models.spool import Spool from backend.app.models.spool_usage_history import SpoolUsageHistory +from backend.app.models.user import User from backend.app.schemas.github_backup import GitHubRestoreRequest, RestoreCategory from backend.app.services.github_restore import ( _COMPANION_CREDENTIAL_ENV, @@ -1302,6 +1303,149 @@ class TestSoftDeletedArchiveRoundTrip: assert row.deleted_at == deleted_at, "a deleted archive must not come back visible" +class TestRestoredArchiveOwnership: + """A restored archive without an owner is invisible to the person who owns it. + + ``created_by_id`` is not attribution, it is the column the access check runs + on: ``_ensure_archive_visible`` fails closed on NULL (404 for any caller + without ``archives:read_all``) and the list paths filter + ``created_by_id == user.id``. So on a multi-user instance the tally reported + archives restored while their owner could neither list nor open them. + """ + + def _entry(self, **overrides): + entry = { + "id": 77, + "filename": "benchy.3mf", + "file_size": 2048, + "content_hash": "abc123", + "started_at": "2026-03-01 10:00:00", + "created_at": "2026-03-01 10:00:00", + } + entry.update(overrides) + return entry + + async def _user(self, db, username="alice"): + user = User(username=username, role="operator") + db.add(user) + await db.flush() + return user + + @pytest.mark.asyncio + async def test_owner_is_carried_across(self, db_session): + user = await self._user(db_session) + tally = _CategoryTally() + + await _service()._restore_archives( + db_session, {"archives": [self._entry(created_by_id=user.id)]}, False, tally, {} + ) + await db_session.commit() + + row = (await db_session.execute(select(PrintArchive))).scalar_one() + assert row.created_by_id == user.id + assert not any("owner cleared" in note for note in tally.notes) + + @pytest.mark.asyncio + async def test_an_unknown_owner_is_cleared_with_a_note_not_failed(self, db_session): + """The archive is still worth having; an admin can reassign it.""" + tally = _CategoryTally() + + await _service()._restore_archives( + db_session, {"archives": [self._entry(created_by_id=4242)]}, False, tally, {} + ) + await db_session.commit() + + row = (await db_session.execute(select(PrintArchive))).scalar_one() + assert row.created_by_id is None + assert tally.restored == 1 and tally.failed == 0 + assert any("owner cleared" in note and "archives:read_all" in note for note in tally.notes) + + @pytest.mark.asyncio + async def test_the_owner_note_is_emitted_once_for_many_rows(self, db_session): + tally = _CategoryTally() + archives = [ + self._entry(id=1, content_hash="h1", filename="a.3mf", created_by_id=4242), + self._entry(id=2, content_hash="h2", filename="b.3mf", created_by_id=4243), + ] + + await _service()._restore_archives(db_session, {"archives": archives}, False, tally, {}) + await db_session.commit() + + assert sum(1 for note in tally.notes if "owner cleared" in note) == 1 + + @pytest.mark.asyncio + async def test_a_backup_without_the_key_still_restores(self, db_session): + """Backups taken before the collector recorded it just can't know the owner.""" + tally = _CategoryTally() + + await _service()._restore_archives(db_session, {"archives": [self._entry()]}, False, tally, {}) + await db_session.commit() + + row = (await db_session.execute(select(PrintArchive))).scalar_one() + assert row.created_by_id is None + assert not any("owner cleared" in note for note in tally.notes) + + @pytest.mark.asyncio + async def test_overwrite_makes_the_local_owner_match_the_backup(self, db_session): + alice = await self._user(db_session, "alice") + bob = await self._user(db_session, "bob") + db_session.add( + PrintArchive( + filename="benchy.3mf", + file_path="/data/benchy.3mf", + file_size=2048, + content_hash="abc123", + started_at=datetime(2026, 3, 1, 10, 0, 0), + created_by_id=bob.id, + ) + ) + await db_session.commit() + + await _service()._restore_archives( + db_session, {"archives": [self._entry(created_by_id=alice.id)]}, True, _CategoryTally(), {} + ) + await db_session.commit() + + row = (await db_session.execute(select(PrintArchive))).scalar_one() + assert row.created_by_id == alice.id + + @pytest.mark.asyncio + async def test_owner_survives_collect_then_restore(self, db_session): + """Both halves, because each looks harmless alone. + + The collector never wrote the key, so there was nothing for the restore + to carry across even once it wanted to. + """ + from backend.app.services.github_backup import github_backup_service + + user = await self._user(db_session) + db_session.add( + PrintArchive( + filename="owned.3mf", + file_path="", + file_size=1024, + content_hash="hash-owned", + started_at=datetime(2026, 3, 1, 10, 0, 0), + created_by_id=user.id, + ) + ) + await db_session.commit() + + files: dict = {} + await github_backup_service._collect_archives(db_session, files) + payload = files[ARCHIVES_PATH] + assert payload["archives"][0]["created_by_id"] == user.id + + await db_session.execute(PrintArchive.__table__.delete()) + await db_session.commit() + + await _service()._restore_archives(db_session, payload, False, _CategoryTally(), {}) + await db_session.commit() + + row = (await db_session.execute(select(PrintArchive))).scalar_one() + assert row.created_by_id == user.id, "a restored archive its owner cannot see is not restored" + + class TestCategoryPathMapping: def setup_method(self): self.service = _service()