fix(activity): answer another user's item like a missing one, and skip foreign history rows (#1507)

Two small fixes in `activity_routes.py`.

**Another user's item answers like a missing one.** Dismissing a task or
request id answered 404 when it didn't exist and 403 when it belonged to
someone else, so a user could test whether an arbitrary id exists. A
non-admin now gets the same 404 for both. In dismiss-many a foreign key
is reported with the missing ones and the rest carry on, and the owner
is still written to the log. Admins are unchanged.

**A foreign row in someone's viewer state no longer gives a 500.** If a
non-admin's viewer state referenced another user's download or request
row, building history raised a `RuntimeError`. The row is now skipped
with a warning.

Not changed: history still raises when a row that viewer state points to
has been deleted. That is a different case, and I didn't want to widen
this.

Tests: the existing forbidden test now expects the 404 and the log
context, and four new ones cover a foreign download and a foreign
request answering like a missing one (including dismiss-many), an admin
still dismissing a foreign row, and history skipping foreign rows. Ruff,
basedpyright and the backend suite pass.

shelfmark/core/activity_routes.py

@@ -291,14 +291,17 @@ def _activity_ws_room(actor: _ActorContext) -> str:
     return "admins"
 
 
-def _check_item_ownership(actor: _ActorContext, row: dict[str, Any]) -> str | None:
-    """Return an error string if the actor doesn't own the item, else None."""
+def _actor_owns_item(actor: _ActorContext, row: dict[str, Any]) -> bool:
+    """Admins own every row; a user only the rows recorded under their id.
+
+    A row the actor does not own is answered exactly like a row that does not
+    exist. Answering it differently (403 against 404) would let any user find
+    out which ids exist, one guess at a time.
+    """
     if actor.is_admin:
-        return None
+        return True
     owner_user_id = normalize_positive_int(row.get("user_id"))
-    if owner_user_id != actor.db_user_id:
-        return "Forbidden"
-    return None
+    return owner_user_id == actor.db_user_id
 
 
 def _check_terminal_download(row: dict[str, Any]) -> str | None:
@@ -737,7 +740,10 @@ def register_activity_routes(
                 )
 
             existing = download_history_service.get_by_task_id(task_id)
-            if existing is None:
+            if existing is None or not _actor_owns_item(actor, existing):
+                download_log_context: dict[str, Any] = (
+                    _download_row_log_context(existing) if existing is not None else {}
+                )
                 return _activity_error_response(
                     "dismiss",
                     status_code=404,
@@ -746,6 +752,7 @@ def register_activity_routes(
                     viewer_scope=actor.viewer_scope,
                     item_type="download",
                     item_key=f"download:{task_id}",
+                    **download_log_context,
                 )
 
             live_queue_index = _build_queue_index(queue_status(user_id=actor.owner_scope))
@@ -753,18 +760,6 @@ def register_activity_routes(
                 existing,
                 has_live_queue_entry=task_id in live_queue_index,
             )
-            ownership_error = _check_item_ownership(actor, existing)
-            if ownership_error is not None:
-                return _activity_error_response(
-                    "dismiss",
-                    status_code=403,
-                    error=ownership_error,
-                    auth_mode=resolve_auth_mode(),
-                    viewer_scope=actor.viewer_scope,
-                    item_type="download",
-                    item_key=f"download:{task_id}",
-                    **_download_row_log_context(effective_existing),
-                )
             terminal_error = _check_terminal_download(effective_existing)
             if terminal_error is not None:
                 return _activity_error_response(
@@ -802,7 +797,12 @@ def register_activity_routes(
                 )
 
             request_row = user_db.get_request(request_id)
-            if request_row is None:
+            if request_row is None or not _actor_owns_item(actor, request_row):
+                request_log_context: dict[str, Any] = (
+                    _request_row_log_context(request_row)
+                    if request_row is not None
+                    else {"request_id": request_id}
+                )
                 return _activity_error_response(
                     "dismiss",
                     status_code=404,
@@ -811,21 +811,9 @@ def register_activity_routes(
                     viewer_scope=actor.viewer_scope,
                     item_type="request",
                     item_key=f"request:{request_id}",
-                    request_id=request_id,
+                    **request_log_context,
                 )
 
-            ownership_error = _check_item_ownership(actor, request_row)
-            if ownership_error is not None:
-                return _activity_error_response(
-                    "dismiss",
-                    status_code=403,
-                    error=ownership_error,
-                    auth_mode=resolve_auth_mode(),
-                    viewer_scope=actor.viewer_scope,
-                    item_type="request",
-                    item_key=f"request:{request_id}",
-                    **_request_row_log_context(request_row),
-                )
             terminal_error = _check_terminal_request(request_row)
             if terminal_error is not None:
                 return _activity_error_response(
@@ -952,25 +940,27 @@ def register_activity_routes(
                 if existing is None:
                     missing_item_keys.append(f"download:{viewer_task_id}")
                     continue
+                if not _actor_owns_item(actor, existing):
+                    # Reported with the missing ids below; only the log tells them apart.
+                    _log_activity_rejection(
+                        "dismiss_many",
+                        status_code=404,
+                        reason="Download not found",
+                        auth_mode=resolve_auth_mode(),
+                        viewer_scope=actor.viewer_scope,
+                        item_type="download",
+                        item_key=f"download:{task_id}",
+                        item_count=len(items),
+                        **_download_row_log_context(existing),
+                    )
+                    missing_item_keys.append(f"download:{viewer_task_id}")
+                    continue
                 if live_queue_index is None:
                     live_queue_index = _build_queue_index(queue_status(user_id=actor.owner_scope))
                 effective_existing = _effective_download_row_for_activity(
                     existing,
                     has_live_queue_entry=task_id in live_queue_index,
                 )
-                ownership_error = _check_item_ownership(actor, existing)
-                if ownership_error is not None:
-                    return _activity_error_response(
-                        "dismiss_many",
-                        status_code=403,
-                        error=ownership_error,
-                        auth_mode=resolve_auth_mode(),
-                        viewer_scope=actor.viewer_scope,
-                        item_type="download",
-                        item_key=f"download:{task_id}",
-                        item_count=len(items),
-                        **_download_row_log_context(effective_existing),
-                    )
                 terminal_error = _check_terminal_download(effective_existing)
                 if terminal_error is not None:
                     return _activity_error_response(
@@ -1004,12 +994,11 @@ def register_activity_routes(
                 if request_row is None:
                     missing_item_keys.append(f"request:{request_id}")
                     continue
-                ownership_error = _check_item_ownership(actor, request_row)
-                if ownership_error is not None:
-                    return _activity_error_response(
+                if not _actor_owns_item(actor, request_row):
+                    _log_activity_rejection(
                         "dismiss_many",
-                        status_code=403,
-                        error=ownership_error,
+                        status_code=404,
+                        reason="Request not found",
                         auth_mode=resolve_auth_mode(),
                         viewer_scope=actor.viewer_scope,
                         item_type="request",
@@ -1017,6 +1006,8 @@ def register_activity_routes(
                         item_count=len(items),
                         **_request_row_log_context(request_row),
                     )
+                    missing_item_keys.append(f"request:{request_id}")
+                    continue
                 terminal_error = _check_terminal_request(request_row)
                 if terminal_error is not None:
                     return _activity_error_response(
@@ -1135,11 +1126,14 @@ def register_activity_routes(
                     msg = f"Download history row not found for {item_key}"
                     raise RuntimeError(msg)
 
-                if not actor.is_admin:
-                    owner_user_id = normalize_positive_int(download_row.get("user_id"))
-                    if owner_user_id != actor.db_user_id:
-                        msg = f"Viewer state out of scope for {item_key}"
-                        raise RuntimeError(msg)
+                if not _actor_owns_item(actor, download_row):
+                    # Stale or foreign viewer state must not take the whole page down.
+                    logger.warning(
+                        "Activity history skipped %s: out of scope for viewer %s",
+                        item_key,
+                        actor.viewer_scope,
+                    )
+                    continue
 
                 effective_download_row = _effective_download_row_for_activity(
                     download_row,
@@ -1164,11 +1158,13 @@ def register_activity_routes(
                     msg = f"Request row not found for {item_key}"
                     raise RuntimeError(msg)
 
-                if not actor.is_admin:
-                    owner_user_id = normalize_positive_int(request_row.get("user_id"))
-                    if owner_user_id != actor.db_user_id:
-                        msg = f"Viewer state out of scope for {item_key}"
-                        raise RuntimeError(msg)
+                if not _actor_owns_item(actor, request_row):
+                    logger.warning(
+                        "Activity history skipped %s: out of scope for viewer %s",
+                        item_key,
+                        actor.viewer_scope,
+                    )
+                    continue
 
                 populate_request_usernames([request_row], user_db)
                 entry = _request_history_entry(

tests/core/test_activity_routes_api.py

@@ -1030,18 +1030,161 @@ class TestActivityRoutes:
                     },
                 )
 
-        assert response.status_code == 403
-        assert response.json["error"] == "Forbidden"
-        mock_warning.assert_called_once()
-        log_message = mock_warning.call_args.args[0]
-        assert "Activity dismiss_many rejected" in log_message
-        assert "status=403" in log_message
-        assert "reason=Forbidden" in log_message
-        assert "auth_mode=builtin" in log_message
-        assert f"viewer_scope=user:{intruder['id']}" in log_message
-        assert f"owner_user_id={owner['id']}" in log_message
-        assert "final_status=complete" in log_message
-        assert "request_id=321" in log_message
+        # Another user's row is answered exactly like a missing one, so the id cannot
+        # be probed. The log still carries the owner context for the operator.
+        assert response.status_code == 404
+        assert response.json == {
+            "error": "One or more activity items were not found",
+            "missing_item_keys": ["download:forbidden-download-task"],
+        }
+        log_messages = [call.args[0] for call in mock_warning.call_args_list]
+        assert any(
+            "Activity dismiss_many rejected" in message
+            and "status=404" in message
+            and "auth_mode=builtin" in message
+            and f"viewer_scope=user:{intruder['id']}" in message
+            and f"owner_user_id={owner['id']}" in message
+            and "final_status=complete" in message
+            and "request_id=321" in message
+            for message in log_messages
+        )
+
+    def test_dismiss_answers_another_users_download_like_a_missing_one(self, main_module, client):
+        owner = _create_user(main_module, prefix="owner")
+        intruder = _create_user(main_module, prefix="intruder")
+        _set_session(
+            client, user_id=intruder["username"], db_user_id=intruder["id"], is_admin=False
+        )
+        _record_terminal_download(
+            main_module,
+            task_id="foreign-dismiss-task",
+            user_id=owner["id"],
+            username=owner["username"],
+        )
+
+        with patch.object(main_module, "get_auth_mode", return_value="builtin"):
+            foreign = client.post(
+                "/api/activity/dismiss",
+                json={"item_type": "download", "item_key": "download:foreign-dismiss-task"},
+            )
+            missing = client.post(
+                "/api/activity/dismiss",
+                json={"item_type": "download", "item_key": "download:no-such-task"},
+            )
+
+        assert foreign.status_code == missing.status_code == 404
+        assert foreign.json == missing.json == {"error": "Download not found"}
+        assert "download:foreign-dismiss-task" not in _hidden_item_keys(
+            main_module, viewer_scope=f"user:{intruder['id']}"
+        )
+
+    def test_dismiss_answers_another_users_request_like_a_missing_one(self, main_module, client):
+        owner = _create_user(main_module, prefix="owner")
+        intruder = _create_user(main_module, prefix="intruder")
+        _set_session(
+            client, user_id=intruder["username"], db_user_id=intruder["id"], is_admin=False
+        )
+        request_row = main_module.user_db.create_request(
+            user_id=owner["id"],
+            content_type="ebook",
+            request_level="book",
+            policy_mode="request_book",
+            book_data={"title": "Owner's Request", "provider": "openlibrary", "provider_id": "x"},
+            status="fulfilled",
+            delivery_state="none",
+        )
+
+        with patch.object(main_module, "get_auth_mode", return_value="builtin"):
+            foreign = client.post(
+                "/api/activity/dismiss",
+                json={"item_type": "request", "item_key": f"request:{request_row['id']}"},
+            )
+            missing = client.post(
+                "/api/activity/dismiss",
+                json={"item_type": "request", "item_key": "request:987654321"},
+            )
+            many = client.post(
+                "/api/activity/dismiss-many",
+                json={
+                    "items": [
+                        {"item_type": "request", "item_key": f"request:{request_row['id']}"},
+                        {"item_type": "request", "item_key": "request:987654321"},
+                    ]
+                },
+            )
+
+        assert foreign.status_code == missing.status_code == 404
+        assert foreign.json == missing.json == {"error": "Request not found"}
+        assert many.status_code == 404
+        assert many.json["missing_item_keys"] == [
+            f"request:{request_row['id']}",
+            "request:987654321",
+        ]
+
+    def test_admin_still_dismisses_another_users_download(self, main_module, client):
+        owner = _create_user(main_module, prefix="owner")
+        admin = _create_user(main_module, prefix="admin", role="admin")
+        _set_session(client, user_id=admin["username"], db_user_id=admin["id"], is_admin=True)
+        _record_terminal_download(
+            main_module,
+            task_id="admin-foreign-dismiss-task",
+            user_id=owner["id"],
+            username=owner["username"],
+        )
+
+        with patch.object(main_module, "get_auth_mode", return_value="builtin"):
+            response = client.post(
+                "/api/activity/dismiss",
+                json={"item_type": "download", "item_key": "download:admin-foreign-dismiss-task"},
+            )
+
+        assert response.status_code == 200
+        assert "download:admin-foreign-dismiss-task" in _hidden_item_keys(
+            main_module, viewer_scope="admin:shared"
+        )
+
+    def test_history_skips_rows_the_viewer_does_not_own(self, main_module, client):
+        owner = _create_user(main_module, prefix="owner")
+        viewer = _create_user(main_module, prefix="viewer")
+        _set_session(client, user_id=viewer["username"], db_user_id=viewer["id"], is_admin=False)
+        _record_terminal_download(
+            main_module,
+            task_id="history-own-task",
+            user_id=viewer["id"],
+            username=viewer["username"],
+        )
+        _record_terminal_download(
+            main_module,
+            task_id="history-foreign-task",
+            user_id=owner["id"],
+            username=owner["username"],
+        )
+        foreign_request = main_module.user_db.create_request(
+            user_id=owner["id"],
+            content_type="ebook",
+            request_level="book",
+            policy_mode="request_book",
+            book_data={"title": "Owner's Request", "provider": "openlibrary", "provider_id": "y"},
+            status="fulfilled",
+            delivery_state="none",
+        )
+        # Viewer state that references rows this viewer does not own.
+        for item_type, item_key in (
+            ("download", "download:history-foreign-task"),
+            ("request", f"request:{foreign_request['id']}"),
+            ("download", "download:history-own-task"),
+        ):
+            main_module.activity_view_state_service.dismiss(
+                viewer_scope=f"user:{viewer['id']}",
+                item_type=item_type,
+                item_key=item_key,
+            )
+
+        with patch.object(main_module, "get_auth_mode", return_value="builtin"):
+            response = client.get("/api/activity/history?limit=10&offset=0")
+
+        assert response.status_code == 200
+        assert [row["item_key"] for row in response.json] == ["download:history-own-task"]
 
     def test_snapshot_backfills_undismissed_terminal_download_from_download_history(
         self, main_module, client