From 80179357a7ff054ab9eeff39c4cd2f53ad8767e2 Mon Sep 17 00:00:00 2001 From: fabi Date: Thu, 2 Jul 2026 22:19:43 +0200 Subject: [PATCH] =?UTF-8?q?fix(review-2):=20high=20=E2=80=94=20read-only?= =?UTF-8?q?=20ban=20model,=20failed-compression=20cleanup,=20live=20SSE=20?= =?UTF-8?q?eviction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H1: corrected a round-1 over-reach. Bans are read-only per USER_JOURNEYS §10: the base AuthUser extractor no longer rejects banned users (they keep read access); Require{Host,Admin} and write handlers enforce the ban via auth.is_banned → 403 (incl. self-unban). Demoted hosts still lose powers immediately (live DB role) and are bounced off the dashboard. Also fixed the login/recover redirect regression on unauthenticated 401. H2: failed compression now self-heals instead of leaving a permanent broken card — refund the quota, delete the orphaned original, soft-delete the row; the frontend toasts the uploader and evicts the card on upload-error. H3: self-delete, ban-with-hide (user-hidden), and comment-deletes now broadcast SSE; feed, diashow, and lightbox evict the affected content live instead of waiting for a reload. sse-eviction.spec.ts covers it. Riders: feed/+page.svelte also carries the medium truncated-delta pill; host.rs also carries the low atomic release_gallery claim; admin/+page.svelte also drops the dead compression_concurrency control (medium). Co-Authored-By: Claude Opus 4.8 --- backend/src/auth/middleware.rs | 19 ++++++--- backend/src/handlers/host.rs | 46 ++++++++++++++++----- backend/src/handlers/social.rs | 4 ++ backend/src/handlers/upload.rs | 8 ++++ backend/src/services/compression.rs | 19 ++++++++- e2e/specs/04-host/moderation.spec.ts | 23 +++++++++++ e2e/specs/04-host/sse-eviction.spec.ts | 51 ++++++++++++++++++++++++ frontend/src/lib/api.ts | 3 ++ frontend/src/lib/diashow/queue.ts | 17 ++++++++ frontend/src/routes/admin/+page.svelte | 5 ++- frontend/src/routes/diashow/+page.svelte | 11 +++++ frontend/src/routes/feed/+page.svelte | 40 ++++++++++++++++++- frontend/src/routes/host/+page.svelte | 19 ++++++++- 13 files changed, 245 insertions(+), 20 deletions(-) create mode 100644 e2e/specs/04-host/sse-eviction.spec.ts diff --git a/backend/src/auth/middleware.rs b/backend/src/auth/middleware.rs index 17de8f8..d7051ae 100644 --- a/backend/src/auth/middleware.rs +++ b/backend/src/auth/middleware.rs @@ -13,6 +13,10 @@ pub struct AuthUser { pub user_id: Uuid, pub event_id: Uuid, pub role: UserRole, + /// Live ban flag. Banned users keep *read* access (per USER_JOURNEYS §10), so + /// the base extractor does NOT reject them — write handlers and the + /// Require{Host,Admin} extractors enforce the ban instead. + pub is_banned: bool, pub token_hash: String, } @@ -45,15 +49,13 @@ impl FromRequestParts for AuthUser { // Trust *live* DB state, not the JWT: a role/ban stored in the token would // survive a demote/ban for the full session lifetime (up to 30d). Re-read - // the user row so a demoted host loses host powers and a banned user is - // locked out immediately on their next request. + // the user row so a demoted host loses host powers immediately. We do NOT + // reject banned users here — they retain read access by design; writes and + // host/admin actions enforce the ban downstream. let user = crate::models::user::User::find_by_id(&state.pool, claims.sub) .await .map_err(|e| AppError::Internal(e.into()))? .ok_or_else(|| AppError::Unauthorized("Benutzer nicht gefunden.".into()))?; - if user.is_banned { - return Err(AppError::Forbidden("Dein Zugang wurde gesperrt.".into())); - } // Update last_seen_at in the background (fire-and-forget). Failures are // non-fatal but worth surfacing — silent swallowing hides DB connection @@ -70,6 +72,7 @@ impl FromRequestParts for AuthUser { user_id: user.id, event_id: user.event_id, role: user.role, + is_banned: user.is_banned, token_hash, }) } @@ -86,6 +89,9 @@ impl FromRequestParts for RequireHost { state: &AppState, ) -> Result { let auth = AuthUser::from_request_parts(parts, state).await?; + if auth.is_banned { + return Err(AppError::Forbidden("Du bist gesperrt.".into())); + } match auth.role { UserRole::Host | UserRole::Admin => Ok(Self(auth)), _ => Err(AppError::Forbidden("Nur für Hosts und Admins.".into())), @@ -104,6 +110,9 @@ impl FromRequestParts for RequireAdmin { state: &AppState, ) -> Result { let auth = AuthUser::from_request_parts(parts, state).await?; + if auth.is_banned { + return Err(AppError::Forbidden("Du bist gesperrt.".into())); + } match auth.role { UserRole::Admin => Ok(Self(auth)), _ => Err(AppError::Forbidden("Nur für Admins.".into())), diff --git a/backend/src/handlers/host.rs b/backend/src/handlers/host.rs index 7994c9a..5fc4d1a 100644 --- a/backend/src/handlers/host.rs +++ b/backend/src/handlers/host.rs @@ -121,6 +121,15 @@ pub async fn ban_user( .execute(&state.pool) .await?; + // If we hid their uploads, evict them live from every feed + the diashow so a + // banned guest's content disappears without each viewer having to reload. + if body.hide_uploads { + let _ = state.sse_tx.send(SseEvent::new( + "user-hidden", + serde_json::json!({ "user_id": user_id }).to_string(), + )); + } + tracing::info!( actor_user_id = %auth.user_id, target_user_id = %user_id, @@ -329,6 +338,10 @@ pub async fn host_delete_comment( if !deleted { return Err(AppError::NotFound("Kommentar nicht gefunden.".into())); } + let _ = state.sse_tx.send(SseEvent::new( + "comment-deleted", + serde_json::json!({ "comment_id": comment_id }).to_string(), + )); tracing::info!( actor_user_id = %auth.user_id, event_id = %auth.event_id, @@ -380,19 +393,33 @@ pub async fn release_gallery( State(state): State, RequireHost(_auth): RequireHost, ) -> Result { + // Atomic claim: the conditional UPDATE is the sole gate, so two concurrent + // release calls can't both pass a check-then-set and double-enqueue exports. + // rows_affected == 0 means someone already released. + let result = sqlx::query( + "UPDATE event SET export_released_at = NOW() + WHERE slug = $1 AND export_released_at IS NULL", + ) + .bind(&state.config.event_slug) + .execute(&state.pool) + .await?; + if result.rows_affected() == 0 { + // Distinguish "no such event" from "already released" for a clean error. + let exists = Event::find_by_slug(&state.pool, &state.config.event_slug) + .await? + .is_some(); + return Err(if exists { + AppError::BadRequest("Galerie wurde bereits freigegeben.".into()) + } else { + AppError::NotFound("Event nicht gefunden.".into()) + }); + } + + // We won the claim — load the event for its id/name to enqueue export jobs. let event = Event::find_by_slug(&state.pool, &state.config.event_slug) .await? .ok_or_else(|| AppError::NotFound("Event nicht gefunden.".into()))?; - if event.export_released_at.is_some() { - return Err(AppError::BadRequest("Galerie wurde bereits freigegeben.".into())); - } - - sqlx::query("UPDATE event SET export_released_at = NOW() WHERE slug = $1") - .bind(&state.config.event_slug) - .execute(&state.pool) - .await?; - // Enqueue export jobs for export_type in ["zip", "html"] { sqlx::query( @@ -411,6 +438,7 @@ pub async fn release_gallery( event.name, state.pool.clone(), state.config.media_path.clone(), + state.config.export_path.clone(), state.sse_tx.clone(), ); diff --git a/backend/src/handlers/social.rs b/backend/src/handlers/social.rs index 168f6e0..6044c8b 100644 --- a/backend/src/handlers/social.rs +++ b/backend/src/handlers/social.rs @@ -213,5 +213,9 @@ pub async fn delete_comment( if !deleted { return Err(AppError::NotFound("Kommentar nicht gefunden.".into())); } + let _ = state.sse_tx.send(crate::state::SseEvent::new( + "comment-deleted", + serde_json::json!({ "comment_id": comment_id, "upload_id": comment.upload_id }).to_string(), + )); Ok(StatusCode::NO_CONTENT) } diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index f2043f6..7d73f3d 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -313,6 +313,14 @@ pub async fn delete_upload( Upload::soft_delete_in_event(&state.pool, upload_id, auth.event_id).await?; + // Evict the card live on every other feed + the projector diashow — otherwise + // a self-deleted post lingers until each viewer manually reloads. Same event + // the host-delete path already emits and the frontend already handles. + let _ = state.sse_tx.send(crate::state::SseEvent::new( + "upload-deleted", + serde_json::json!({ "upload_id": upload_id }).to_string(), + )); + Ok(StatusCode::NO_CONTENT) } diff --git a/backend/src/services/compression.rs b/backend/src/services/compression.rs index 504f278..44813c0 100644 --- a/backend/src/services/compression.rs +++ b/backend/src/services/compression.rs @@ -42,11 +42,28 @@ impl CompressionWorker { } Err(e) => { tracing::error!("compression failed for upload {upload_id}: {e:#}"); + // Auto-cleanup: a failed transcode would otherwise leave a + // permanently broken feed card, silently charge the uploader's + // quota, and orphan the original on disk. Refund + soft-delete + // (one tx, so v_feed excludes it), remove the orphan file, then + // tell the uploader (upload-error toast) and evict the card + // everywhere (upload-deleted, already handled by the feed). + let _ = Upload::set_compression_status(&worker.pool, upload_id, "failed").await; + if let Err(del) = Upload::soft_delete(&worker.pool, upload_id).await { + tracing::warn!(error = ?del, %upload_id, "failed to soft-delete after compression failure"); + } + let orphan = worker.media_path.join(&original_path); + if let Err(rm) = tokio::fs::remove_file(&orphan).await { + tracing::warn!(error = ?rm, path = %orphan.display(), "failed to remove orphaned original"); + } let _ = worker.sse_tx.send(SseEvent { event_type: "upload-error".to_string(), data: serde_json::json!({ "upload_id": upload_id, "error": e.to_string() }).to_string(), }); - let _ = Upload::set_compression_status(&worker.pool, upload_id, "failed").await; + let _ = worker.sse_tx.send(SseEvent { + event_type: "upload-deleted".to_string(), + data: serde_json::json!({ "upload_id": upload_id }).to_string(), + }); } } }); diff --git a/e2e/specs/04-host/moderation.spec.ts b/e2e/specs/04-host/moderation.spec.ts index 13e0f74..bcb4c17 100644 --- a/e2e/specs/04-host/moderation.spec.ts +++ b/e2e/specs/04-host/moderation.spec.ts @@ -97,4 +97,27 @@ test.describe('Host — live role/ban revocation (H1)', () => { api.unbanUser(host.jwt, host.userId, { expectedStatus: [204] }) ).rejects.toThrow(/→ 403/); }); + + // H1 / USER_JOURNEYS §10: a banned user keeps *read* access but every write is + // rejected with 403. The ban is enforced on write handlers + Require{Host,Admin}, + // NOT in the base extractor (which would wrongly block reads too). + test('a banned user keeps read access but is blocked from writes', async ({ api, host, guest }) => { + const base = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101'; + const target = await guest('BannedRW'); + await api.banUser(host.jwt, target.userId, false); + + // Reads still succeed. + const read = await fetch(base + '/api/v1/me/context', { + headers: { Authorization: `Bearer ${target.jwt}` }, + }); + expect(read.status).toBe(200); + + // Writes are rejected. + const write = await fetch(base + '/api/v1/upload', { + method: 'POST', + headers: { Authorization: `Bearer ${target.jwt}` }, + body: new FormData(), + }); + expect(write.status).toBe(403); + }); }); diff --git a/e2e/specs/04-host/sse-eviction.spec.ts b/e2e/specs/04-host/sse-eviction.spec.ts new file mode 100644 index 0000000..5fef7dc --- /dev/null +++ b/e2e/specs/04-host/sse-eviction.spec.ts @@ -0,0 +1,51 @@ +/** + * Regression for the review's H3: self-deleting an upload and banning a user with + * hide_uploads mutated visibility server-side but broadcast nothing, so the + * content lingered on every other viewer's feed (and the projector diashow) until + * a manual reload. Both now emit SSE so clients evict live. + */ +import { test, expect } from '../../fixtures/test'; +import { seedUpload } from '../../helpers/seed'; +import { SseListener } from '../../helpers/sse-listener'; + +const BASE = process.env.E2E_FRONTEND_URL ?? 'http://localhost:3101'; + +test.describe('Host — live SSE eviction (H3)', () => { + test('self-deleting an upload broadcasts upload-deleted', async ({ guest }) => { + const g = await guest('SelfDeleter'); + const uploadId = await seedUpload(g.jwt, { caption: 'delete me' }); + + const sse = new SseListener(); + await sse.start(g.jwt); + + const res = await fetch(`${BASE}/api/v1/upload/${uploadId}`, { + method: 'DELETE', + headers: { Authorization: `Bearer ${g.jwt}` }, + }); + expect(res.status).toBe(204); + + await sse.waitForEvent( + 'upload-deleted', + (e) => e.data.upload_id === uploadId + ); + }); + + test('banning a user with hide_uploads broadcasts user-hidden', async ({ + api, + host, + guest, + }) => { + const target = await guest('HideTarget'); + await seedUpload(target.jwt, { caption: 'should vanish' }); + + const sse = new SseListener(); + await sse.start(host.jwt); + + await api.banUser(host.jwt, target.userId, true); + + await sse.waitForEvent( + 'user-hidden', + (e) => e.data.user_id === target.userId + ); + }); +}); diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index ee78d9c..76604ee 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -68,6 +68,9 @@ async function request( } if (!res.ok) { + // An expired/invalid token (401) clears the dead session. Banned users are + // NOT logged out — they keep read access by design (USER_JOURNEYS §10) and + // simply get a 403 "gesperrt" toast on writes. if (res.status === 401) { clearAuth(); } diff --git a/frontend/src/lib/diashow/queue.ts b/frontend/src/lib/diashow/queue.ts index 433110f..0b7db9a 100644 --- a/frontend/src/lib/diashow/queue.ts +++ b/frontend/src/lib/diashow/queue.ts @@ -73,6 +73,23 @@ export class SlideQueue { return { wasCurrent: currentId === id }; } + /** + * Remove every slide belonging to a user (banned with hide_uploads). Returns + * true if the current head was one of them so the caller advances immediately. + */ + removeByUser(userId: string, currentId: string | null): { wasCurrent: boolean } { + let wasCurrent = false; + for (const [id, slide] of this.allKnown) { + if (slide.user_id === userId) { + if (id === currentId) wasCurrent = true; + this.allKnown.delete(id); + } + } + this.liveQueue = this.liveQueue.filter((s) => s.user_id !== userId); + this.shuffleQueue = this.shuffleQueue.filter((s) => s.user_id !== userId); + return { wasCurrent }; + } + /** Look up a slide by id — for the diashow page to render the current slide. */ get(id: string): FeedUpload | undefined { return this.allKnown.get(id); diff --git a/frontend/src/routes/admin/+page.svelte b/frontend/src/routes/admin/+page.svelte index eaa6b0d..78cf12b 100644 --- a/frontend/src/routes/admin/+page.svelte +++ b/frontend/src/routes/admin/+page.svelte @@ -57,8 +57,9 @@ title: 'Limits & Größen', fields: [ { key: 'max_image_size_mb', label: 'Max. Bildgröße (MB)', kind: 'number' }, - { key: 'max_video_size_mb', label: 'Max. Videogröße (MB)', kind: 'number' }, - { key: 'compression_concurrency', label: 'Kompressions-Worker', kind: 'number' } + { key: 'max_video_size_mb', label: 'Max. Videogröße (MB)', kind: 'number' } + // compression_concurrency is set via COMPRESSION_WORKER_CONCURRENCY at + // boot, not live — omitted so it isn't a dead no-op control. ] }, { diff --git a/frontend/src/routes/diashow/+page.svelte b/frontend/src/routes/diashow/+page.svelte index 87e25cd..37b1e67 100644 --- a/frontend/src/routes/diashow/+page.svelte +++ b/frontend/src/routes/diashow/+page.svelte @@ -99,6 +99,16 @@ } } + function handleUserHidden(data: string) { + try { + const payload = JSON.parse(data) as { user_id: string }; + const result = queue.removeByUser(payload.user_id, current?.id ?? null); + if (result.wasCurrent) advance(); + } catch { + // ignore + } + } + function revealOverlay() { showOverlay = true; if (overlayHideTimer) clearTimeout(overlayHideTimer); @@ -145,6 +155,7 @@ void acquireWakeLock(); unsubs.push(onSseEvent('upload-processed', handleUploadProcessed)); unsubs.push(onSseEvent('upload-deleted', handleUploadDeleted)); + unsubs.push(onSseEvent('user-hidden', handleUserHidden)); void loadInitial(); }); diff --git a/frontend/src/routes/feed/+page.svelte b/frontend/src/routes/feed/+page.svelte index f3ffdc3..d8039a4 100644 --- a/frontend/src/routes/feed/+page.svelte +++ b/frontend/src/routes/feed/+page.svelte @@ -27,6 +27,9 @@ let refreshing = $state(false); let pullProgress = $state(0); // 0–1+ during the drag, 0 when idle let selectedUpload = $state(null); + // Set when a truncated feed-delta means we missed too much to merge — shows a + // tap-to-refresh pill instead of yanking the user's scroll to page 1. + let feedStale = $state(false); let sentinel: HTMLDivElement; let feedObserver: IntersectionObserver | null = null; let inPlaceRefreshTimer: ReturnType | null = null; @@ -196,6 +199,28 @@ if (selectedUpload?.id === payload.upload_id) selectedUpload = null; } catch { /* ignore */ } }), + // A background transcode failed: the backend already cleaned up (refunded + // quota, removed the row) and an upload-deleted evicts the card. Only the + // uploader gets a toast — registered before upload-deleted so the card is + // still present to check ownership. + onSseEvent('upload-error', (data) => { + try { + const { upload_id } = JSON.parse(data) as { upload_id: string }; + const mine = uploads.find((u) => u.id === upload_id && u.user_id === myUserId); + if (mine) { + toast('Ein Upload konnte nicht verarbeitet werden.', 'error'); + void refreshQuota(); + } + } catch { /* ignore */ } + }), + // A banned user's uploads were hidden — drop all their cards live. + onSseEvent('user-hidden', (data) => { + try { + const { user_id } = JSON.parse(data) as { user_id: string }; + uploads = uploads.filter((u) => u.user_id !== user_id); + if (selectedUpload && selectedUpload.user_id === user_id) selectedUpload = null; + } catch { /* ignore */ } + }), // Patch the single affected card in place from the SSE payload instead of // refetching page 1 — a busy event fires these constantly and a full reload // would yank every scrolled-down user back to the top on each reaction. @@ -210,7 +235,9 @@ // Missed more than the backend delta cap while backgrounded — the // delta is only the newest slice, so merging would leave a silent gap // of older-but-still-new uploads. Resync from page 1 instead. - void loadFeed(true); + // Rather than involuntarily yank the user to page 1, surface a + // tap-to-refresh pill so they keep scroll until they resync. + feedStale = true; return; } if (delta.uploads.length) { @@ -440,6 +467,17 @@ {/if} + {#if feedStale} +
+ +
+ {/if}
diff --git a/frontend/src/routes/host/+page.svelte b/frontend/src/routes/host/+page.svelte index a68fd0e..16faa6e 100644 --- a/frontend/src/routes/host/+page.svelte +++ b/frontend/src/routes/host/+page.svelte @@ -2,6 +2,7 @@ import { goto } from '$app/navigation'; import { getToken, getRole } from '$lib/auth'; import { api } from '$lib/api'; + import type { MeContextDto } from '$lib/types'; import { onMount } from 'svelte'; import { toast, toastError } from '$lib/toast-store'; import ConfirmSheet from '$lib/components/ConfirmSheet.svelte'; @@ -86,8 +87,22 @@ onMount(async () => { const token = getToken(); - const role = getRole(); - if (!token || (role !== 'host' && role !== 'admin')) { + if (!token) { + goto('/join'); + return; + } + // Trust the *live* role, not the JWT claim: a mid-session demote leaves a + // stale 'host' in the token, but the backend now 403s every host call. Fetch + // the current role and bounce a demoted host to the feed instead of leaving + // them on a dashboard that errors on load. + try { + const ctx = await api.get('/me/context'); + if (ctx.role !== 'host' && ctx.role !== 'admin') { + goto('/feed'); + return; + } + } catch { + // Expired/invalid session (api.ts cleared it) — send them to re-auth. goto('/join'); return; }