Fix scan-to-download auth and share every photo widget's current photo
The share QR's URL carried no auth params at all, so it silently fell back through require_device's legacy-token resolution to whichever frame happened to still be flagged legacy -- working only by accident for a single frame, sharing the wrong frame's photos for any other, and going fully dead once that frame's legacy flag was cleared. Move the endpoint to manage.py, keyed on the frame's own manage_token (same pattern /m/<manage_token> already uses) instead of device auth. Since the server now resolves assets itself instead of trusting a caller-supplied asset_id, it naturally generalizes to gather every photo widget's current photo into one Immich share link, not just one "primary" widget's.
This commit is contained in:
+11
-9
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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] = []
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user