diff --git a/changelog.d/tsk-7uxooi-notifications-user-scope.md b/changelog.d/tsk-7uxooi-notifications-user-scope.md new file mode 100644 index 000000000..4198f6a4b --- /dev/null +++ b/changelog.d/tsk-7uxooi-notifications-user-scope.md @@ -0,0 +1,4 @@ +### Fixed + +- Cross-user notification read and mutation: `list`, `list_archived`, `unread_count`, `mark_read`, `archive`, and `mark_all_read` now scope to the authenticated user (`user_id IS NULL OR user_id = ?`), so a user can only see and modify their own notifications plus broadcasts. Previously these endpoints returned every user's rows and allowed cross-user mutations (CWE-862). +- Notification routes resolve the caller from `request.state.user_id` instead of a cookie-only dependency, so local-token (`taosctl notifications`) callers resolve to the primary user and keep working instead of returning 401. diff --git a/tests/test_notifications_user_scope.py b/tests/test_notifications_user_scope.py new file mode 100644 index 000000000..91f6e77e8 --- /dev/null +++ b/tests/test_notifications_user_scope.py @@ -0,0 +1,347 @@ +import pytest +import pytest_asyncio +import yaml +from httpx import ASGITransport, AsyncClient + +from tinyagentos.app import create_app +from tinyagentos.notifications import NotificationStore +from taos_test_csrf import csrf_event_hooks + + +def _row_by_title(items: list[dict], title: str) -> dict: + """Pick a notification by title, never by list position. + + Every row added in a test shares the same whole-second ``timestamp``, so the + ORDER BY on ties is whatever the index happens to yield. Selecting by title + also keeps route-scoping setups off the scoped store API, so the red these + tests produce on unfixed code is the leak, not a signature TypeError. + """ + for item in items: + if item["title"] == title: + return item + raise AssertionError(f"no notification titled {title!r}") + + +def _id_by_title(items: list[dict], title: str) -> int: + return _row_by_title(items, title)["id"] + + +def _make_config(tmp_path) -> dict: + return { + "server": {"host": "0.0.0.0", "port": 6969}, + "backends": [], + "qmd": {"url": "http://localhost:7832"}, + "agents": [], + "metrics": {"poll_interval": 30, "retention_days": 30}, + } + + +@pytest_asyncio.fixture +async def notif_store(tmp_path): + store = NotificationStore(tmp_path / "notifications.db") + await store.init() + yield store + await store.close() + + +@pytest.mark.asyncio +class TestNotificationStoreUserScope: + async def test_list_returns_own_and_broadcast(self, notif_store): + await notif_store.add("a", "a msg", user_id="u1") + await notif_store.add("b", "b msg", user_id="u2") + await notif_store.add("c", "c msg") + items = await notif_store.list(user_id="u1") + titles = {i["title"] for i in items} + assert titles == {"a", "c"} + + async def test_list_excludes_other_users(self, notif_store): + await notif_store.add("a", "a msg", user_id="u1") + await notif_store.add("b", "b msg", user_id="u2") + items = await notif_store.list(user_id="u1") + assert all(i["user_id"] != "u2" for i in items) + + async def test_list_archived_returns_own_only(self, notif_store): + await notif_store.add("a", "a msg", user_id="u1") + await notif_store.add("b", "b msg", user_id="u2") + await notif_store.add("c", "c msg") + by_title = {i["title"]: i["id"] for i in await notif_store.list()} + await notif_store.archive(by_title["a"], user_id="u1") + await notif_store.archive(by_title["b"], user_id="u2") + history = await notif_store.list_archived(user_id="u1") + titles = {h["title"] for h in history} + assert titles == {"a"} + + async def test_unread_count_counts_own_and_broadcast(self, notif_store): + await notif_store.add("a", "a msg", user_id="u1") + await notif_store.add("b", "b msg", user_id="u2") + await notif_store.add("c", "c msg") + assert await notif_store.unread_count(user_id="u1") == 2 + + async def test_none_user_id_returns_unfiltered(self, notif_store): + await notif_store.add("a", "a msg", user_id="u1") + await notif_store.add("b", "b msg", user_id="u2") + items = await notif_store.list(user_id=None) + assert len(items) == 2 + + async def test_mark_read_scoped_to_user(self, notif_store): + await notif_store.add("a", "a msg", user_id="u1") + await notif_store.add("b", "b msg", user_id="u2") + u1_items = await notif_store.list(user_id="u1") + u2_items = await notif_store.list(user_id="u2") + u1_id = u1_items[0]["id"] + u2_id = u2_items[0]["id"] + affected = await notif_store.mark_read(u2_id, user_id="u1") + assert affected == 0 + assert (await notif_store.list(user_id="u2"))[0]["read"] is False + affected = await notif_store.mark_read(u1_id, user_id="u1") + assert affected == 1 + assert (await notif_store.list(user_id="u1"))[0]["read"] is True + + async def test_archive_scoped_to_user(self, notif_store): + await notif_store.add("a", "a msg", user_id="u1") + await notif_store.add("b", "b msg", user_id="u2") + u1_items = await notif_store.list(user_id="u1") + u2_items = await notif_store.list(user_id="u2") + u1_id = u1_items[0]["id"] + u2_id = u2_items[0]["id"] + affected = await notif_store.archive(u2_id, user_id="u1") + assert affected == 0 + assert len(await notif_store.list_archived(user_id="u2")) == 0 + affected = await notif_store.archive(u1_id, user_id="u1") + assert affected == 1 + assert len(await notif_store.list_archived(user_id="u1")) == 1 + + +@pytest_asyncio.fixture +async def two_user_app(tmp_path): + """A started app with alice (primary/admin) and bob, plus a session each.""" + config = _make_config(tmp_path) + (tmp_path / "config.yaml").write_text(yaml.dump(config)) + (tmp_path / ".setup_complete").touch() + + app = create_app(data_dir=tmp_path) + + notif_store = app.state.notifications + if notif_store._db is not None: + await notif_store.close() + await notif_store.init() + + auth = app.state.auth + auth.setup_user("alice", "Alice", "", "alicepass123") + alice_rec = auth.find_user("alice") + alice_token = auth.create_session(user_id=alice_rec["id"], long_lived=True) + + bob_invite = auth.add_user_invite("bob", "alice") + auth.complete_invite("bob", bob_invite, "Bob", "", "bobpass123") + bob_rec = auth.find_user("bob") + bob_token = auth.create_session(user_id=bob_rec["id"], long_lived=True) + + app.state._startup_complete = True + + return app, alice_rec["id"], alice_token, bob_rec["id"], bob_token + + +@pytest.mark.asyncio +class TestNotificationRoutesUserScope: + async def _alice_client(self, app, alice_token): + transport = ASGITransport(app=app) + return AsyncClient( + transport=transport, + base_url="http://test", + cookies={"taos_session": alice_token}, + event_hooks=csrf_event_hooks(), + ) + + async def _bob_client(self, app, bob_token): + transport = ASGITransport(app=app) + return AsyncClient( + transport=transport, + base_url="http://test", + cookies={"taos_session": bob_token}, + event_hooks=csrf_event_hooks(), + ) + + async def test_list_excludes_other_user(self, two_user_app): + app, alice_id, alice_token, bob_id, bob_token = two_user_app + store = app.state.notifications + await store.add("alice-notif", "for alice", user_id=alice_id) + await store.add("bob-notif", "for bob", user_id=bob_id) + await store.add("broadcast", "for everyone") + async with await self._alice_client(app, alice_token) as c: + resp = await c.get("/api/notifications") + assert resp.status_code == 200 + data = resp.json() + titles = {i["title"] for i in data} + assert "alice-notif" in titles + assert "bob-notif" not in titles + assert "broadcast" in titles + + async def test_archived_excludes_other_user(self, two_user_app): + app, alice_id, alice_token, bob_id, bob_token = two_user_app + store = app.state.notifications + await store.add("alice-notif", "for alice", user_id=alice_id) + await store.add("bob-notif", "for bob", user_id=bob_id) + by_title = {i["title"]: i["id"] for i in await store.list()} + await store.archive(by_title["alice-notif"]) + await store.archive(by_title["bob-notif"]) + async with await self._alice_client(app, alice_token) as c: + resp = await c.get("/api/notifications/archived") + assert resp.status_code == 200 + data = resp.json() + titles = {i["title"] for i in data} + assert "alice-notif" in titles + assert "bob-notif" not in titles + + async def test_count_excludes_other_user(self, two_user_app): + app, alice_id, alice_token, bob_id, bob_token = two_user_app + store = app.state.notifications + await store.add("alice-notif", "for alice", user_id=alice_id) + await store.add("bob-notif", "for bob", user_id=bob_id) + async with await self._alice_client(app, alice_token) as c: + resp = await c.get("/api/notifications/count") + assert resp.status_code == 200 + assert "data-count='1'" in resp.text + assert "1" == resp.text + + async def test_mark_read_other_user_returns_404(self, two_user_app): + app, alice_id, alice_token, bob_id, bob_token = two_user_app + store = app.state.notifications + await store.add("bob-notif", "for bob", user_id=bob_id) + bob_notif_id = _id_by_title(await store.list(), "bob-notif") + async with await self._alice_client(app, alice_token) as c: + resp = await c.post(f"/api/notifications/{bob_notif_id}/read") + assert resp.status_code == 404 + assert _row_by_title(await store.list(), "bob-notif")["read"] is False + + async def test_archive_other_user_returns_404(self, two_user_app): + app, alice_id, alice_token, bob_id, bob_token = two_user_app + store = app.state.notifications + await store.add("bob-notif", "for bob", user_id=bob_id) + bob_notif_id = _id_by_title(await store.list(), "bob-notif") + async with await self._alice_client(app, alice_token) as c: + resp = await c.post(f"/api/notifications/{bob_notif_id}/archive") + assert resp.status_code == 404 + # list() filters archived = 0, so finding the row proves it stayed active. + assert _row_by_title(await store.list(), "bob-notif") + assert not [i for i in await store.list_archived() if i["title"] == "bob-notif"] + + async def test_mark_own_notification_succeeds(self, two_user_app): + app, alice_id, alice_token, bob_id, bob_token = two_user_app + store = app.state.notifications + await store.add("alice-notif", "for alice", user_id=alice_id) + alice_notif_id = _id_by_title(await store.list(), "alice-notif") + async with await self._alice_client(app, alice_token) as c: + resp = await c.post(f"/api/notifications/{alice_notif_id}/read") + assert resp.status_code == 200 + assert _row_by_title(await store.list(), "alice-notif")["read"] is True + + +@pytest.mark.asyncio +class TestNotificationRoutesLocalToken: + """The local token (``Authorization: Bearer ``, no cookie) must work. + + AuthMiddleware accepts the local token and maps it to the primary user by + setting ``request.state.user_id``; it never sets a session cookie. Every + ``taosctl notifications`` subcommand authenticates exactly that way, so a + cookie-only route dependency (``Depends(get_current_user)``) turns all of + them into 401s while the browser keeps working. + """ + + def _token_client(self, app): + """A local-token caller: Bearer header, and deliberately NO cookie.""" + return AsyncClient( + transport=ASGITransport(app=app), + base_url="http://test", + headers={"Authorization": f"Bearer {app.state.auth.get_local_token()}"}, + event_hooks=csrf_event_hooks(), + ) + + async def test_list_with_local_token_is_not_401(self, two_user_app): + app, alice_id, _alice_token, bob_id, _bob_token = two_user_app + store = app.state.notifications + await store.add("alice-notif", "for alice", user_id=alice_id) + await store.add("bob-notif", "for bob", user_id=bob_id) + await store.add("broadcast", "for everyone") + async with self._token_client(app) as c: + resp = await c.get("/api/notifications") + assert resp.status_code == 200, resp.text + titles = {i["title"] for i in resp.json()} + # The token resolves to the PRIMARY user, not to "everyone". + assert titles == {"alice-notif", "broadcast"} + + async def test_count_with_local_token_is_not_401(self, two_user_app): + app, alice_id, _alice_token, bob_id, _bob_token = two_user_app + store = app.state.notifications + await store.add("alice-notif", "for alice", user_id=alice_id) + await store.add("bob-notif", "for bob", user_id=bob_id) + async with self._token_client(app) as c: + resp = await c.get("/api/notifications/count") + assert resp.status_code == 200, resp.text + assert "data-count='1'" in resp.text + + async def test_archived_with_local_token_is_not_401(self, two_user_app): + app, alice_id, _alice_token, bob_id, _bob_token = two_user_app + store = app.state.notifications + await store.add("alice-notif", "for alice", user_id=alice_id) + await store.add("bob-notif", "for bob", user_id=bob_id) + rows = await store.list() + await store.archive(_id_by_title(rows, "alice-notif")) + await store.archive(_id_by_title(rows, "bob-notif")) + async with self._token_client(app) as c: + resp = await c.get("/api/notifications/archived") + assert resp.status_code == 200, resp.text + titles = {i["title"] for i in resp.json()} + assert titles == {"alice-notif"} + + async def test_mark_read_own_with_local_token_is_not_401(self, two_user_app): + app, alice_id, _alice_token, _bob_id, _bob_token = two_user_app + store = app.state.notifications + await store.add("alice-notif", "for alice", user_id=alice_id) + own_id = _id_by_title(await store.list(), "alice-notif") + async with self._token_client(app) as c: + resp = await c.post(f"/api/notifications/{own_id}/read") + assert resp.status_code == 200, resp.text + assert _row_by_title(await store.list(), "alice-notif")["read"] is True + + async def test_mark_read_other_user_with_local_token_returns_404(self, two_user_app): + app, _alice_id, _alice_token, bob_id, _bob_token = two_user_app + store = app.state.notifications + await store.add("bob-notif", "for bob", user_id=bob_id) + bob_notif_id = _id_by_title(await store.list(), "bob-notif") + async with self._token_client(app) as c: + resp = await c.post(f"/api/notifications/{bob_notif_id}/read") + assert resp.status_code == 404, resp.text + assert _row_by_title(await store.list(), "bob-notif")["read"] is False + + async def test_archive_own_with_local_token_is_not_401(self, two_user_app): + app, alice_id, _alice_token, _bob_id, _bob_token = two_user_app + store = app.state.notifications + await store.add("alice-notif", "for alice", user_id=alice_id) + own_id = _id_by_title(await store.list(), "alice-notif") + async with self._token_client(app) as c: + resp = await c.post(f"/api/notifications/{own_id}/archive") + assert resp.status_code == 200, resp.text + assert len(await store.list_archived(user_id=alice_id)) == 1 + + async def test_read_all_with_local_token_is_not_401(self, two_user_app): + app, alice_id, _alice_token, bob_id, _bob_token = two_user_app + store = app.state.notifications + await store.add("alice-notif", "for alice", user_id=alice_id) + await store.add("bob-notif", "for bob", user_id=bob_id) + async with self._token_client(app) as c: + resp = await c.post("/api/notifications/read-all") + assert resp.status_code == 200, resp.text + assert resp.json()["marked"] == 1 + rows = await store.list() + assert _row_by_title(rows, "alice-notif")["read"] is True + assert _row_by_title(rows, "bob-notif")["read"] is False + + async def test_mark_all_read_with_local_token_is_not_401(self, two_user_app): + app, alice_id, _alice_token, bob_id, _bob_token = two_user_app + store = app.state.notifications + await store.add("alice-notif", "for alice", user_id=alice_id) + await store.add("bob-notif", "for bob", user_id=bob_id) + async with self._token_client(app) as c: + resp = await c.post("/api/notifications/mark-all-read") + assert resp.status_code == 200, resp.text + assert resp.json()["marked"] == 1 diff --git a/tinyagentos/notifications.py b/tinyagentos/notifications.py index 278a9f4b0..123e9a3a3 100644 --- a/tinyagentos/notifications.py +++ b/tinyagentos/notifications.py @@ -211,47 +211,86 @@ async def add( except Exception: logger.warning("NotificationStore: could not schedule web-push", exc_info=True) - async def list(self, limit: int = 20, unread_only: bool = False) -> list[dict]: + async def list( + self, + limit: int = 20, + unread_only: bool = False, + user_id: str | None = None, + ) -> list[dict]: # Active feed: archived (dismissed) notifications are excluded. conds = ["archived = 0"] + if user_id is not None: + conds.append("(user_id IS NULL OR user_id = ?)") if unread_only: conds.append("read = 0") + params: tuple = (user_id, limit) if user_id is not None else (limit,) sql = ( "SELECT id, timestamp, level, title, message, read, source, data, user_id FROM notifications" f" WHERE {' AND '.join(conds)} ORDER BY timestamp DESC LIMIT ?" ) - async with self._db.execute(sql, (limit,)) as cursor: + async with self._db.execute(sql, params) as cursor: rows = await cursor.fetchall() return [_serialize_row(r) for r in rows] - async def list_archived(self, limit: int = 50) -> list[dict]: + async def list_archived( + self, + limit: int = 50, + user_id: str | None = None, + ) -> list[dict]: # History view: the dismissed notifications, newest first. Nothing is # deleted, so this is the durable record (#62 / append-only #103). + conds = ["archived = 1"] + if user_id is not None: + conds.append("(user_id IS NULL OR user_id = ?)") + params: tuple = (user_id, limit) if user_id is not None else (limit,) async with self._db.execute( "SELECT id, timestamp, level, title, message, read, source, data, user_id FROM notifications" - " WHERE archived = 1 ORDER BY timestamp DESC LIMIT ?", - (limit,), + f" WHERE {' AND '.join(conds)} ORDER BY timestamp DESC LIMIT ?", + params, ) as cursor: rows = await cursor.fetchall() return [_serialize_row(r) for r in rows] - async def unread_count(self) -> int: + async def unread_count(self, user_id: str | None = None) -> int: + conds = ["read = 0", "archived = 0"] + if user_id is not None: + conds.append("(user_id IS NULL OR user_id = ?)") + params: tuple = (user_id,) if user_id is not None else () async with self._db.execute( - "SELECT COUNT(*) FROM notifications WHERE read = 0 AND archived = 0" + f"SELECT COUNT(*) FROM notifications WHERE {' AND '.join(conds)}", + params, ) as cursor: row = await cursor.fetchone() return row[0] if row else 0 - async def mark_read(self, notif_id: int) -> None: - await self._db.execute("UPDATE notifications SET read = 1 WHERE id = ?", (notif_id,)) + async def mark_read(self, notif_id: int, user_id: str | None = None) -> int: + if user_id is not None: + cursor = await self._db.execute( + "UPDATE notifications SET read = 1 WHERE id = ? AND (user_id IS NULL OR user_id = ?)", + (notif_id, user_id), + ) + else: + # Internal/system caller: unfiltered update. + cursor = await self._db.execute( + "UPDATE notifications SET read = 1 WHERE id = ?", (notif_id,) + ) await self._db.commit() + return cursor.rowcount - async def archive(self, notif_id: int) -> None: + async def archive(self, notif_id: int, user_id: str | None = None) -> int: # Dismiss = archive. The row stays; the History view still shows it. - await self._db.execute( - "UPDATE notifications SET archived = 1 WHERE id = ?", (notif_id,) - ) + if user_id is not None: + cursor = await self._db.execute( + "UPDATE notifications SET archived = 1 WHERE id = ? AND (user_id IS NULL OR user_id = ?)", + (notif_id, user_id), + ) + else: + # Internal/system caller: unfiltered update. + cursor = await self._db.execute( + "UPDATE notifications SET archived = 1 WHERE id = ?", (notif_id,) + ) await self._db.commit() + return cursor.rowcount async def archive_by_source_ref(self, source: str, request_id) -> int: """Archive active notifications whose JSON `data.request_id` matches. @@ -262,6 +301,7 @@ async def archive_by_source_ref(self, source: str, request_id) -> int: (#62: nothing is deleted). Idempotent: rows already archived are skipped; returns the number newly archived. """ + # Intentionally global: resolves by source + request_id, not by user. async with self._db.execute( "SELECT id, data FROM notifications WHERE source = ? AND archived = 0", (source,), @@ -279,6 +319,8 @@ async def archive_by_source_ref(self, source: str, request_id) -> int: if str(payload.get("request_id")) == target: ids.append(nid) if ids: + # Intentionally global: source-ref resolution applies to the row, + # not to a specific user. placeholders = ",".join("?" * len(ids)) await self._db.execute( f"UPDATE notifications SET archived = 1, read = 1 WHERE id IN ({placeholders})", @@ -287,8 +329,15 @@ async def archive_by_source_ref(self, source: str, request_id) -> int: await self._db.commit() return len(ids) - async def mark_all_read(self) -> int: - cursor = await self._db.execute("UPDATE notifications SET read = 1 WHERE read = 0") + async def mark_all_read(self, user_id: str | None = None) -> int: + if user_id is not None: + cursor = await self._db.execute( + "UPDATE notifications SET read = 1 WHERE read = 0 AND (user_id IS NULL OR user_id = ?)", + (user_id,), + ) + else: + # Internal/system caller: unfiltered update. + cursor = await self._db.execute("UPDATE notifications SET read = 1 WHERE read = 0") await self._db.commit() return cursor.rowcount @@ -296,6 +345,7 @@ async def cleanup(self, max_age_days: int = 30) -> int: # Age out only old UNdismissed notifications. Archived rows are the # durable history a user explicitly dismissed (#62 / append-only #103), # so they are never GC'd here. + # Intentionally global: retention/prune is a system-wide operation. cutoff = int(time.time()) - (max_age_days * 86400) cursor = await self._db.execute( "DELETE FROM notifications WHERE timestamp < ? AND archived = 0", (cutoff,) diff --git a/tinyagentos/routes/notifications.py b/tinyagentos/routes/notifications.py index 35738d74b..2aba491f9 100644 --- a/tinyagentos/routes/notifications.py +++ b/tinyagentos/routes/notifications.py @@ -16,6 +16,22 @@ router = APIRouter() +def _notif_user_id(request: Request) -> str: + """Resolve the calling user the way the rest of the app does. + + AuthMiddleware sets ``request.state.user_id`` for BOTH the session cookie + and the local token (``Authorization: Bearer ``, which it maps to + the primary user), so browser sessions, ``taosctl`` and host scripts all + resolve here. A cookie-only dependency such as ``get_current_user`` would + 401 every local-token caller. Same idiom as ``routes/event_stream.py`` and + ``routes/desktop_control.py``. + """ + uid = getattr(request.state, "user_id", None) + if not uid: + raise HTTPException(status_code=401, detail="Authentication required") + return str(uid) + + def _format_ts(ts: int) -> str: """Format a unix timestamp as a relative or short date string.""" delta = int(time.time()) - ts @@ -30,8 +46,9 @@ def _format_ts(ts: int) -> str: @router.get("/api/notifications") async def list_notifications(request: Request, unread_only: bool = False): + user_id = _notif_user_id(request) store = request.app.state.notifications - items = await store.list(unread_only=unread_only) + items = await store.list(unread_only=unread_only, user_id=user_id) # Return HTML for HTMX requests, JSON otherwise if request.headers.get("hx-request"): if not items: @@ -86,44 +103,54 @@ async def create_notification(request: Request, body: CreateNotificationRequest) @router.get("/api/notifications/count", response_class=HTMLResponse) async def notification_count(request: Request): + user_id = _notif_user_id(request) store = request.app.state.notifications - count = await store.unread_count() + count = await store.unread_count(user_id=user_id) return f"{count if count else ''}" @router.get("/api/notifications/archived") async def list_archived_notifications(request: Request): """History view: dismissed notifications, newest first (nothing deleted).""" + user_id = _notif_user_id(request) store = request.app.state.notifications - return await store.list_archived() + return await store.list_archived(user_id=user_id) @router.post("/api/notifications/{notif_id}/read") async def mark_read(request: Request, notif_id: int): + user_id = _notif_user_id(request) store = request.app.state.notifications - await store.mark_read(notif_id) + affected = await store.mark_read(notif_id, user_id=user_id) + if affected == 0: + raise HTTPException(status_code=404, detail="notification not found") return {"ok": True} @router.post("/api/notifications/{notif_id}/archive") async def archive_notification(request: Request, notif_id: int): """Dismiss a notification by archiving it; it stays in the History view.""" + user_id = _notif_user_id(request) store = request.app.state.notifications - await store.archive(notif_id) + affected = await store.archive(notif_id, user_id=user_id) + if affected == 0: + raise HTTPException(status_code=404, detail="notification not found") return {"ok": True} @router.post("/api/notifications/read-all") async def mark_all_read(request: Request): + user_id = _notif_user_id(request) store = request.app.state.notifications - await store.mark_all_read() - return {"ok": True} + count = await store.mark_all_read(user_id=user_id) + return {"ok": True, "marked": count} @router.post("/api/notifications/mark-all-read") async def mark_all_read_counted(request: Request): + user_id = _notif_user_id(request) store = request.app.state.notifications - count = await store.mark_all_read() + count = await store.mark_all_read(user_id=user_id) return {"marked": count}