diff --git a/server/README.md b/server/README.md index 4cec5ca..51d0e02 100644 --- a/server/README.md +++ b/server/README.md @@ -121,9 +121,11 @@ Pages: `/` (routing hub), `/setup`, `/login`, `/claim`, `/settings`, this response may grow. - `GET /frame/photo-info` -- location/date overlay text for the manage menu (city + abbreviated US/CAN region or country, `MM/DD/YY`). -- `GET /frame/share/{asset_id}` -- creates a 30-minute public Immich - share link and 302s to it; scoped to the photo currently showing or - queued on *this* frame only. +- `GET /frame/share/{manage_token}` -- creates a 30-minute public Immich + share link covering every photo widget's currently-showing photo on + *this* frame and 302s to it. Authenticated by the frame's own + `manage_token` (see the manage QR below), not device credentials -- a + phone scanning the QR has no way to supply `?id=`/`?token=`. - `GET /frame/face-labels` -- up to 4 named faces with 800x480 positions, flattened (`name_0`/`x_0`/`y_0`, ...) for the device's flat-scalar parser. @@ -200,12 +202,12 @@ Pages: `/` (routing hub), `/setup`, `/login`, `/claim`, `/settings`, in the web UI (or using "Show next") only rearranges what's already in that lookahead; it doesn't add or remove photos from the album. - Auth in one breath: browsers use sessions (+CSRF), devices use - per-frame tokens (`?id=` + `?token=`), the manage QR uses its own - limited token, and `MANAGEMENT_TOKEN` survives only as the migration - credential for pre-multi-frame firmware. `/frame/share` stays scoped - to photos this frame is actually showing or has queued, not any - Immich asset ID someone might guess -- a second layer a leaked device - token alone wouldn't bypass. + per-frame tokens (`?id=` + `?token=`), the manage QR and the + scan-to-download QR both use the frame's own `manage_token` (device + tokens don't work for either -- neither is ever called by firmware, + both are opened by a phone that has no way to supply `?id=`/`?token=`), + and `MANAGEMENT_TOKEN` survives only as the migration credential for + pre-multi-frame firmware. - Calendar frame mode (`app/calendar_feed.py`) expands recurring events (RRULE/EXDATE/DST) via [`recurring-ical-events`](https://pypi.org/project/recurring-ical-events/), which is LGPL-3.0-or-later -- the only non-permissively-licensed diff --git a/server/app/immich_client.py b/server/app/immich_client.py index dc35e24..270fbff 100644 --- a/server/app/immich_client.py +++ b/server/app/immich_client.py @@ -80,10 +80,10 @@ class ImmichClient: resp.raise_for_status() return resp.json() - def create_share_link(self, asset_id: str, expires_in_s: int) -> str: - """Creates a public, view-only Immich share link for a single - asset, expiring expires_in_s seconds from now, and returns its - public URL. Used by the manage-button overlay's share QR -- + def create_share_link(self, asset_ids: list[str], expires_in_s: int) -> str: + """Creates a public, view-only Immich share link covering one or + more assets, expiring expires_in_s seconds from now, and returns + its public URL. Used by the manage-button overlay's share QR -- created lazily (only when someone actually scans it), not when the button's pressed, so the expiry clock starts when it's actually used.""" @@ -93,7 +93,7 @@ class ImmichClient: headers=self._headers, json={ "type": "INDIVIDUAL", - "assetIds": [asset_id], + "assetIds": list(asset_ids), "expiresAt": expires_at, "allowUpload": False, "allowDownload": True, diff --git a/server/app/routers/api_widgets.py b/server/app/routers/api_widgets.py index f3c860b..0e134e2 100644 --- a/server/app/routers/api_widgets.py +++ b/server/app/routers/api_widgets.py @@ -493,8 +493,8 @@ def api_widget_thumbnail( """Scoped to what this widget is actually showing/queuing -- a user merely linked to view this frame shouldn't be able to pull thumbnails for arbitrary asset ids in the owner's Immich library, only this - widget's own curated album. Same rule device.frame_share and - manage.manage_thumbnail already enforce.""" + widget's own curated album. Same rule manage.manage_thumbnail + already enforces.""" frame, widget = frame_widget _require_widget_type(widget, "photos") pcfg = db.get(PhotoWidgetConfig, widget.id) diff --git a/server/app/routers/common.py b/server/app/routers/common.py index 54ad68a..32a6e77 100644 --- a/server/app/routers/common.py +++ b/server/app/routers/common.py @@ -449,7 +449,15 @@ def build_manage_content(db: Session, frame: Frame, request) -> dict: exif = asset.get("exifInfo") or {} content["location_lines"] = _format_location(exif) content["taken_at"] = _format_taken_at(exif) - content["share_url"] = f"{base}/frame/share/{primary_cfg.current_asset_id}" + + # Unlike location/date-taken (a single fixed corner, so tied to the + # one "primary" widget above), the share link covers every photo + # widget's current photo (see manage.manage_share) -- so it only + # needs *some* photo widget to have a current photo, not specifically + # the primary one, and doesn't depend on the EXIF fetch above + # succeeding. + if any(db.get(PhotoWidgetConfig, w.id).current_asset_id for w in photo_widgets): + content["share_url"] = f"{base}/frame/share/{frame.manage_token}" panel_w, panel_h = logical_render_size(frame.orientation) face_labels: list[dict] = [] diff --git a/server/app/routers/device.py b/server/app/routers/device.py index 66d3246..dc3ebfe 100644 --- a/server/app/routers/device.py +++ b/server/app/routers/device.py @@ -15,9 +15,8 @@ from __future__ import annotations import logging import time -import httpx from fastapi import APIRouter, Depends, HTTPException, Request -from fastapi.responses import FileResponse, RedirectResponse, Response +from fastapi.responses import FileResponse, Response from pydantic import BaseModel from sqlalchemy import delete, func, select from sqlalchemy.orm import Session @@ -27,7 +26,7 @@ from ..auth import get_server_settings, require_device from ..db import frame_locked, get_db from ..firmware import firmware_path from ..image_pipeline import logical_render_size, render_panel, render_placeholder -from ..models import BatteryLog, Frame, FrameButtonAction, PhotoWidgetConfig, Widget +from ..models import BatteryLog, Frame, FrameButtonAction, Widget from ..widgets import WIDGET_TYPES from .common import ( BATTERY_HISTORY_MAX, @@ -35,9 +34,6 @@ from .common import ( RECHARGE_JUMP_PCT, RECHARGE_LOOKBACK, build_manage_content, - immich_client_for, - immich_creds, - photo_widgets_for_frame, ) logger = logging.getLogger(__name__) @@ -356,34 +352,3 @@ def frame_firmware(frame: Frame = Depends(require_device)): if not path.exists(): raise HTTPException(404, "No firmware uploaded") return FileResponse(path, media_type="application/octet-stream") - - -@router.get("/frame/share/{asset_id}") -def frame_share(asset_id: str, frame: Frame = Depends(require_device), db: Session = Depends(get_db)): - """Creates a 30-minute public Immich share link for asset_id and - redirects to it -- what the manage overlay's bottom-left QR code - points to. The link is created lazily, when this actually gets hit - (i.e. when someone scans it), not when the manage button was - pressed, so the 30-minute window starts when it's actually used. - Also scoped to the photo currently showing or queued on one of THIS - frame's own photo widgets -- not any arbitrary Immich asset id -- as - a second layer even a leaked token wouldn't bypass.""" - url, key = immich_creds(frame) - if not url or not key: - raise HTTPException(400, "Immich URL/API key not configured yet") - - photo_widgets = photo_widgets_for_frame(db, frame) - showing_or_queued = any( - asset_id == cfg.current_asset_id or asset_id in cfg.queue - for cfg in (db.get(PhotoWidgetConfig, w.id) for w in photo_widgets) - ) - if not showing_or_queued: - raise HTTPException(404, "That photo isn't currently showing or queued on this frame") - - client = immich_client_for(frame) - try: - share_url = client.create_share_link(asset_id, expires_in_s=1800) - except httpx.HTTPError as e: - raise HTTPException(502, f"Could not create share link: {e}") from e - - return RedirectResponse(share_url) diff --git a/server/app/routers/manage.py b/server/app/routers/manage.py index 820fdf2..8aebb9e 100644 --- a/server/app/routers/manage.py +++ b/server/app/routers/manage.py @@ -11,7 +11,7 @@ import logging import httpx from fastapi import APIRouter, Depends, HTTPException, Request -from fastapi.responses import HTMLResponse, Response +from fastapi.responses import HTMLResponse, RedirectResponse, Response from fastapi.templating import Jinja2Templates from pydantic import BaseModel from sqlalchemy import select @@ -19,8 +19,14 @@ from sqlalchemy.orm import Session from .. import photo_queue, quiet_hours from ..db import get_db, widget_locked -from ..models import Frame -from .common import immich_client_for, list_assets, photo_widget_config_or_404 +from ..models import Frame, PhotoWidgetConfig +from .common import ( + immich_client_for, + immich_creds, + list_assets, + photo_widget_config_or_404, + photo_widgets_for_frame, +) logger = logging.getLogger(__name__) @@ -120,3 +126,37 @@ def manage_thumbnail(asset_id: str, frame: Frame = Depends(require_manage), db: except httpx.HTTPError as e: raise HTTPException(502, f"Could not download thumbnail from Immich: {e}") from e return Response(content=content, media_type=content_type) + + +@router.get("/frame/share/{manage_token}") +def manage_share(frame: Frame = Depends(require_manage), db: Session = Depends(get_db)): + """Creates a 30-minute public Immich share link covering every photo + widget's currently-displayed asset on this frame, and redirects to it + -- what the manage overlay's bottom-left QR code points to. Lazily + created (only when someone actually scans it, not when the manage + button was pressed), so the 30-minute window starts at actual use. + Keyed on this frame's own manage_token, like the rest of this router, + rather than device credentials -- a phone scanning a QR code has no + way to supply the device's ?id=/?token=, which is why this used to + silently fall back to whichever frame happened to still carry the + legacy migration token instead of the frame that was actually + scanned.""" + photo_widgets = photo_widgets_for_frame(db, frame) + asset_ids: list[str] = [] + for widget in photo_widgets: + cfg = db.get(PhotoWidgetConfig, widget.id) + if cfg.current_asset_id and cfg.current_asset_id not in asset_ids: + asset_ids.append(cfg.current_asset_id) + if not asset_ids: + raise HTTPException(404, "No photos currently showing on this frame") + + url, key = immich_creds(frame) + if not url or not key: + raise HTTPException(400, "Immich URL/API key not configured yet") + + client = immich_client_for(frame) + try: + share_url = client.create_share_link(asset_ids, expires_in_s=1800) + except httpx.HTTPError as e: + raise HTTPException(502, f"Could not create share link: {e}") from e + return RedirectResponse(share_url) diff --git a/server/tests/test_device_widget_dispatch.py b/server/tests/test_device_widget_dispatch.py index 5998598..dfd685f 100644 --- a/server/tests/test_device_widget_dispatch.py +++ b/server/tests/test_device_widget_dispatch.py @@ -1,7 +1,7 @@ """End-to-end HTTP tests for the widget-system cutover in -routers/device.py -- /frame/image, /frame/advance, /frame/back, and -/frame/share against real widget rows (via the migration-backfilled -frame #1, or a purpose-built second frame), a real TestClient, real +routers/device.py -- /frame/image, /frame/advance, and /frame/back +against real widget rows (via the migration-backfilled frame #1, or a +purpose-built second frame), a real TestClient, real render_panel/compose_into. Only Immich itself is mocked (monkeypatched at the app.widgets.photos module boundary, same pattern as test_widgets_photos.py) -- everything else in the pipeline is real. @@ -23,7 +23,6 @@ from app.models import ( Frame, FrameButtonAction, PhotoWidgetConfig, - User, Widget, ) @@ -145,33 +144,3 @@ def test_two_widget_frame_composites_both_and_next_targets_calendar(client, db_s photo_cfg = db_session.get(PhotoWidgetConfig, photo_widget.id) assert cal_cfg.browse_offset == 1 assert photo_cfg.current_asset_id == "" # untouched -- NEXT was never bound to it - - -def test_frame_share_checks_widget_scoped_state(client, db_session, monkeypatch): - client.post("/setup", data={"username": "alice", "password": "hunter22"}) - frame = db_session.get(Frame, 1) - frame.owner_user_id = db_session.query(User).filter_by(username="alice").one().id - widget = db_session.query(Widget).filter_by(frame_id=frame.id, widget_type="photos").one() - cfg = db_session.get(PhotoWidgetConfig, widget.id) - cfg.album_id = "album-1" - cfg.current_asset_id = "asset-1" - cfg.queue = ["asset-2"] - frame.immich_url = "http://immich.example.com" - frame.immich_api_key = "key" - db_session.commit() - - # Not showing/queued -- rejected before ever touching Immich - resp = client.get("/frame/share/asset-not-on-this-frame") - assert resp.status_code == 404 - - # Currently showing -- allowed through to the (mocked) Immich call - monkeypatch.setattr( - "app.routers.device.immich_client_for", - lambda frame: type("C", (), {"create_share_link": lambda self, asset_id, expires_in_s: "https://immich.example.com/share/abc"})(), - ) - resp = client.get("/frame/share/asset-1", follow_redirects=False) - assert resp.status_code in (302, 303, 307) - - # Queued (not current) -- also allowed - resp = client.get("/frame/share/asset-2", follow_redirects=False) - assert resp.status_code in (302, 303, 307) diff --git a/server/tests/test_manage_widget_dispatch.py b/server/tests/test_manage_widget_dispatch.py index 77a1b97..d70ae7b 100644 --- a/server/tests/test_manage_widget_dispatch.py +++ b/server/tests/test_manage_widget_dispatch.py @@ -119,3 +119,106 @@ def test_manage_thumbnail_scoped_to_showing_or_queued(client, db_session, monkey def test_unknown_manage_token_404s(client, db_session): resp = client.get("/api/m/not-a-real-token/queue") assert resp.status_code == 404 + + +def test_manage_share_requires_current_photo(client, db_session): + client.post("/setup", data={"username": "alice", "password": "hunter22"}) + frame, _ = _configure_photo_widget(db_session) + resp = client.get(f"/frame/share/{frame.manage_token}", follow_redirects=False) + assert resp.status_code == 404 + + +def test_manage_share_creates_link_and_redirects(client, db_session, monkeypatch): + client.post("/setup", data={"username": "alice", "password": "hunter22"}) + frame, widget = _configure_photo_widget(db_session) + cfg = db_session.get(PhotoWidgetConfig, widget.id) + cfg.current_asset_id = "asset-1" + db_session.commit() + + calls = [] + monkeypatch.setattr( + "app.routers.manage.immich_client_for", + lambda frame: type("C", (), { + "create_share_link": lambda self, asset_ids, expires_in_s: ( + calls.append(asset_ids) or "https://immich.example.com/share/abc" + ), + })(), + ) + resp = client.get(f"/frame/share/{frame.manage_token}", follow_redirects=False) + assert resp.status_code in (302, 303, 307) + assert calls == [["asset-1"]] + + +def test_manage_share_covers_every_photo_widget(client, db_session, monkeypatch): + from app.models import Widget + + client.post("/setup", data={"username": "alice", "password": "hunter22"}) + frame, widget = _configure_photo_widget(db_session) + cfg = db_session.get(PhotoWidgetConfig, widget.id) + cfg.current_asset_id = "asset-1" + + second_widget = Widget(frame_id=frame.id, widget_type="photos", x=4, y=0, w=4, h=5, + sort_order=1, created_at=0) + db_session.add(second_widget) + db_session.flush() + db_session.add(PhotoWidgetConfig(widget_id=second_widget.id, album_id="album-2", current_asset_id="asset-9")) + db_session.commit() + + calls = [] + monkeypatch.setattr( + "app.routers.manage.immich_client_for", + lambda frame: type("C", (), { + "create_share_link": lambda self, asset_ids, expires_in_s: ( + calls.append(asset_ids) or "https://immich.example.com/share/abc" + ), + })(), + ) + resp = client.get(f"/frame/share/{frame.manage_token}", follow_redirects=False) + assert resp.status_code in (302, 303, 307) + assert calls == [["asset-1", "asset-9"]] + + +def test_manage_share_scoped_to_its_own_frame(client, db_session, monkeypatch): + """The regression test for the original bug: each frame's own share + QR must only ever surface that frame's own current photo, never + another frame's -- previously it silently resolved via whichever + frame happened to still carry the legacy migration token, regardless + of which frame's QR was actually scanned.""" + from app.models import Widget + + client.post("/setup", data={"username": "alice", "password": "hunter22"}) + frame_a, widget_a = _configure_photo_widget(db_session) + cfg_a = db_session.get(PhotoWidgetConfig, widget_a.id) + cfg_a.current_asset_id = "asset-a" + + frame_b = Frame( + name="Frame B", device_id="112233445566", device_token="devtok-b", + manage_token="mtok-b", orientation="landscape", created_at=0, + immich_url="http://immich.example.com", immich_api_key="key", + ) + db_session.add(frame_b) + db_session.flush() + widget_b = Widget(frame_id=frame_b.id, widget_type="photos", x=0, y=0, w=4, h=5, + sort_order=0, created_at=0) + db_session.add(widget_b) + db_session.flush() + db_session.add(PhotoWidgetConfig(widget_id=widget_b.id, album_id="album-b", current_asset_id="asset-b")) + db_session.commit() + + calls = [] + monkeypatch.setattr( + "app.routers.manage.immich_client_for", + lambda frame: type("C", (), { + "create_share_link": lambda self, asset_ids, expires_in_s: ( + calls.append(asset_ids) or "https://immich.example.com/share/abc" + ), + })(), + ) + client.get(f"/frame/share/{frame_a.manage_token}", follow_redirects=False) + client.get(f"/frame/share/{frame_b.manage_token}", follow_redirects=False) + assert calls == [["asset-a"], ["asset-b"]] + + +def test_unknown_manage_share_token_404s(client, db_session): + resp = client.get("/frame/share/not-a-real-token") + assert resp.status_code == 404