From 40f7f2fb5eab85c6bbdc395ae3c55985e8fdb19c Mon Sep 17 00:00:00 2001 From: nils Date: Fri, 17 Jul 2026 14:52:23 +0200 Subject: [PATCH] Multi-tenant: albums owned per photographer - albums.owner_id (migration 0003, backfilled to the original user) - owned::{album,photo,share} are the only admin data-access paths; another tenant's resources are indistinguishable from nonexistent (404) - every admin handler threaded through ownership; each ALLOWED_EMAILS entry is now its own isolated workspace - tenant-isolation integration test matrix (tests/tenancy.rs, env-gated on TEST_DATABASE_URL) driving the real router --- Cargo.lock | 1 + Cargo.toml | 1 + README.md | 16 ++- migrations/0003_multi_tenant.sql | 10 ++ src/lib.rs | 1 + src/models.rs | 2 + src/owned.rs | 46 ++++++ src/routes/albums.rs | 41 ++++-- src/routes/photos.rs | 17 ++- src/routes/shares.rs | 18 +-- src/routes/zip.rs | 5 +- tests/tenancy.rs | 235 +++++++++++++++++++++++++++++++ 12 files changed, 357 insertions(+), 36 deletions(-) create mode 100644 migrations/0003_multi_tenant.sql create mode 100644 src/owned.rs create mode 100644 tests/tenancy.rs diff --git a/Cargo.lock b/Cargo.lock index 3ac1d15..226e48b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2574,6 +2574,7 @@ dependencies = [ "time", "tokio", "tokio-util", + "tower", "tower-http", "tracing", "tracing-subscriber", diff --git a/Cargo.toml b/Cargo.toml index 34bc187..1ac8f74 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -45,6 +45,7 @@ uuid = { version = "1", features = ["v4", "serde"] } [dev-dependencies] cookie = { version = "0.18", features = ["signed"] } +tower = { version = "0.5", features = ["util"] } [profile.release] lto = "thin" diff --git a/README.md b/README.md index 4742380..b4c6cdb 100644 --- a/README.md +++ b/README.md @@ -30,10 +30,13 @@ collect ratings and tags, and let clients download originals. `photos//preview.jpg`, `photos//thumb.jpg`. The bucket stays fully private; all image traffic is streamed through the API with auth checks (no bucket CORS or public access needed). -- **Auth**: photographer signs in via any OIDC provider (authorization-code +- **Auth**: photographers sign in via any OIDC provider (authorization-code flow + userinfo); only emails in `ALLOWED_EMAILS` may sign in, and sessions are re-checked against the allowlist on every request, so removing an email - revokes access immediately. Clients use unguessable share tokens, optionally + revokes access immediately. **Multi-tenant**: each allowed email is its own + workspace — albums, photos, and share links are owned per photographer and + invisible to the others (enforced via ownership-scoped data access and + covered by the tenant-isolation test matrix). Clients use unguessable share tokens, optionally gated by an argon2-hashed password (10 wrong guesses lock the link for 15 minutes). - **Frontend**: React + Vite SPA — justified gallery, lightbox with rating @@ -55,6 +58,13 @@ cargo run --bin worker # job worker (separate terminal, same env) cd frontend && npm install && npm run dev # UI on :5173, proxies /api ``` +Tests (the tenant-isolation matrix needs a disposable database): + +```sh +createdb photos_test # or: docker compose exec postgres createdb -U photos photos_test +TEST_DATABASE_URL=postgres://photos:photos@localhost:5432/photos_test cargo test +``` + Register the OIDC client with redirect URI `/api/auth/callback` (locally: `http://localhost:5173/api/auth/callback`). Any standard OIDC provider works (Authentik, Keycloak, Zitadel, Dex, ...); the app uses @@ -158,8 +168,6 @@ Notes: - Full RAW develop fallback for files whose embedded preview is tiny (exceedingly rare on modern cameras; `darktable-cli` in the worker image would cover it). -- Multiple photographer accounts with separate libraries (any allowed email - sees everything). - No S3 orphan sweeper: a crash in the narrow window between an upload's S3 put and its DB commit can leave an unreferenced original in the bucket (never data loss — just unclaimed storage). diff --git a/migrations/0003_multi_tenant.sql b/migrations/0003_multi_tenant.sql new file mode 100644 index 0000000..6b0ff01 --- /dev/null +++ b/migrations/0003_multi_tenant.sql @@ -0,0 +1,10 @@ +-- Albums gain an owner: the tenancy root. Photos, shares, ratings and tags +-- all hang off albums, so this single column scopes everything. +alter table albums add column owner_id uuid references users(id); + +-- Existing albums belong to the instance's original photographer. +update albums set owner_id = (select id from users order by created_at limit 1); + +alter table albums alter column owner_id set not null; + +create index albums_owner_idx on albums (owner_id); diff --git a/src/lib.rs b/src/lib.rs index cf9abd6..4fb8526 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -4,6 +4,7 @@ pub mod error; pub mod imaging; pub mod jobs; pub mod models; +pub mod owned; pub mod routes; pub mod s3; pub mod state; diff --git a/src/models.rs b/src/models.rs index c7c1102..4172664 100644 --- a/src/models.rs +++ b/src/models.rs @@ -96,6 +96,8 @@ impl JobStatus { #[derive(Debug, Clone, sqlx::FromRow, Serialize)] pub struct Album { pub id: Uuid, + #[serde(skip_serializing)] + pub owner_id: Uuid, pub name: String, pub description: String, pub created_at: DateTime, diff --git a/src/owned.rs b/src/owned.rs new file mode 100644 index 0000000..b760a70 --- /dev/null +++ b/src/owned.rs @@ -0,0 +1,46 @@ +//! Tenant-scoped data access. These are the ONLY functions admin handlers may +//! use to fetch albums, photos, or shares — every one joins ownership, so a +//! handler cannot accidentally reach across tenants. Another user's resource +//! is indistinguishable from a nonexistent one (404). + +use uuid::Uuid; + +use crate::error::ApiError; +use crate::models::{Album, Photo, Share}; +use crate::state::AppState; + +pub async fn album(state: &AppState, album_id: Uuid, owner: Uuid) -> Result { + let album: Option = + sqlx::query_as("select * from albums where id = $1 and owner_id = $2") + .bind(album_id) + .bind(owner) + .fetch_optional(&state.db) + .await?; + album.ok_or_else(ApiError::not_found) +} + +pub async fn photo(state: &AppState, photo_id: Uuid, owner: Uuid) -> Result { + let photo: Option = sqlx::query_as( + "select p.* from photos p + join albums a on a.id = p.album_id + where p.id = $1 and a.owner_id = $2", + ) + .bind(photo_id) + .bind(owner) + .fetch_optional(&state.db) + .await?; + photo.ok_or_else(ApiError::not_found) +} + +pub async fn share(state: &AppState, share_id: Uuid, owner: Uuid) -> Result { + let share: Option = sqlx::query_as( + "select s.* from shares s + join albums a on a.id = s.album_id + where s.id = $1 and a.owner_id = $2", + ) + .bind(share_id) + .bind(owner) + .fetch_optional(&state.db) + .await?; + share.ok_or_else(ApiError::not_found) +} diff --git a/src/routes/albums.rs b/src/routes/albums.rs index 36062d1..6f28c91 100644 --- a/src/routes/albums.rs +++ b/src/routes/albums.rs @@ -6,8 +6,10 @@ use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use uuid::Uuid; +use crate::auth::AuthUser; use crate::error::{ApiError, ApiResult}; use crate::models::{Album, JobKind, Photo, PhotoStatus}; +use crate::owned; use crate::state::AppState; #[derive(Serialize, sqlx::FromRow)] @@ -23,6 +25,7 @@ pub struct AlbumListItem { pub async fn list( State(state): State, + user: AuthUser, ) -> ApiResult>> { let albums: Vec = sqlx::query_as( "select a.id, a.name, a.description, a.created_at, @@ -35,9 +38,11 @@ pub async fn list( order by coalesce(p.taken_at, p.created_at), p.filename limit 1 ) c on true + where a.owner_id = $2 order by a.created_at desc", ) .bind(PhotoStatus::Ready.as_str()) + .bind(user.id) .fetch_all(&state.db) .await?; Ok(Json(albums)) @@ -52,18 +57,21 @@ pub struct CreateAlbum { pub async fn create( State(state): State, + user: AuthUser, Json(body): Json, ) -> ApiResult> { let name = body.name.trim(); if name.is_empty() { return Err(ApiError::bad_request("album name is required")); } - let album: Album = - sqlx::query_as("insert into albums (name, description) values ($1, $2) returning *") - .bind(name) - .bind(body.description.trim()) - .fetch_one(&state.db) - .await?; + let album: Album = sqlx::query_as( + "insert into albums (owner_id, name, description) values ($1, $2, $3) returning *", + ) + .bind(user.id) + .bind(name) + .bind(body.description.trim()) + .fetch_one(&state.db) + .await?; Ok(Json(album)) } @@ -94,12 +102,10 @@ pub struct AlbumDetail { pub async fn get_one( State(state): State, + user: AuthUser, Path(album_id): Path, ) -> ApiResult> { - let album: Album = sqlx::query_as("select * from albums where id = $1") - .bind(album_id) - .fetch_one(&state.db) - .await?; + let album = owned::album(&state, album_id, user.id).await?; let photos: Vec = sqlx::query_as( "select * from photos where album_id = $1 order by coalesce(taken_at, created_at), filename", @@ -159,6 +165,7 @@ pub struct UpdateAlbum { pub async fn update( State(state): State, + user: AuthUser, Path(album_id): Path, Json(body): Json, ) -> ApiResult> { @@ -170,12 +177,13 @@ pub async fn update( let album: Album = sqlx::query_as( "update albums set name = coalesce($2, name), description = coalesce($3, description) - where id = $1 + where id = $1 and owner_id = $4 returning *", ) .bind(album_id) .bind(body.name.as_deref().map(str::trim)) .bind(body.description.as_deref().map(str::trim)) + .bind(user.id) .fetch_one(&state.db) .await?; Ok(Json(album)) @@ -183,16 +191,19 @@ pub async fn update( pub async fn delete( State(state): State, + user: AuthUser, Path(album_id): Path, ) -> ApiResult> { let mut tx = state.db.begin().await?; // Lock the album row: concurrent uploads block on their FK check against // it, then fail once it's gone and clean up their own S3 objects — so no // photo can slip in between the cleanup enqueue and the cascade delete. - let locked: Option<(Uuid,)> = sqlx::query_as("select id from albums where id = $1 for update") - .bind(album_id) - .fetch_optional(&mut *tx) - .await?; + let locked: Option<(Uuid,)> = + sqlx::query_as("select id from albums where id = $1 and owner_id = $2 for update") + .bind(album_id) + .bind(user.id) + .fetch_optional(&mut *tx) + .await?; if locked.is_none() { return Err(ApiError::not_found()); } diff --git a/src/routes/photos.rs b/src/routes/photos.rs index 50d6e2e..d6f5524 100644 --- a/src/routes/photos.rs +++ b/src/routes/photos.rs @@ -9,9 +9,11 @@ use sha2::Digest; use tokio::io::AsyncWriteExt; use uuid::Uuid; +use crate::auth::AuthUser; use crate::error::{ApiError, ApiResult}; use crate::jobs; use crate::models::{JobKind, Photo, PhotoStatus}; +use crate::owned; use crate::s3; use crate::state::AppState; @@ -41,18 +43,13 @@ fn sanitize_filename(raw: &str) -> Result { pub async fn upload( State(state): State, + user: AuthUser, Path(album_id): Path, Query(query): Query, headers: HeaderMap, body: Body, ) -> ApiResult> { - let album_exists: Option<(Uuid,)> = sqlx::query_as("select id from albums where id = $1") - .bind(album_id) - .fetch_optional(&state.db) - .await?; - if album_exists.is_none() { - return Err(ApiError::not_found()); - } + owned::album(&state, album_id, user.id).await?; let filename = sanitize_filename(&query.filename)?; let content_type = headers @@ -178,8 +175,10 @@ pub async fn upload( /// content already exists in the album. pub async fn by_hash( State(state): State, + user: AuthUser, Path((album_id, sha256)): Path<(Uuid, String)>, ) -> ApiResult> { + owned::album(&state, album_id, user.id).await?; let photo: Option = sqlx::query_as("select * from photos where album_id = $1 and sha256 = $2") .bind(album_id) @@ -191,8 +190,10 @@ pub async fn by_hash( pub async fn delete( State(state): State, + user: AuthUser, Path(photo_id): Path, ) -> ApiResult> { + owned::photo(&state, photo_id, user.id).await?; let mut tx = state.db.begin().await?; let deleted = sqlx::query("delete from photos where id = $1") .bind(photo_id) @@ -213,8 +214,10 @@ pub async fn delete( pub async fn reprocess( State(state): State, + user: AuthUser, Path(photo_id): Path, ) -> ApiResult> { + owned::photo(&state, photo_id, user.id).await?; let mut tx = state.db.begin().await?; let updated = sqlx::query("update photos set status = $2, error = null where id = $1") .bind(photo_id) diff --git a/src/routes/shares.rs b/src/routes/shares.rs index 1e35ffc..1a12967 100644 --- a/src/routes/shares.rs +++ b/src/routes/shares.rs @@ -7,8 +7,9 @@ use chrono::{DateTime, Utc}; use serde::Deserialize; use uuid::Uuid; -use crate::auth::random_token; +use crate::auth::{random_token, AuthUser}; use crate::error::{ApiError, ApiResult}; +use crate::owned; use crate::state::AppState; #[derive(sqlx::FromRow)] @@ -48,8 +49,10 @@ const SHARE_COLUMNS: &str = "s.id, s.token, s.label, s.password_hash, s.allow_do pub async fn list( State(state): State, + user: AuthUser, Path(album_id): Path, ) -> ApiResult>> { + owned::album(&state, album_id, user.id).await?; let rows: Vec = sqlx::query_as(&format!( "select {SHARE_COLUMNS} from shares s where s.album_id = $1 order by s.created_at desc" )) @@ -75,16 +78,11 @@ fn default_true() -> bool { pub async fn create( State(state): State, + user: AuthUser, Path(album_id): Path, Json(body): Json, ) -> ApiResult> { - let album_exists: Option<(Uuid,)> = sqlx::query_as("select id from albums where id = $1") - .bind(album_id) - .fetch_optional(&state.db) - .await?; - if album_exists.is_none() { - return Err(ApiError::not_found()); - } + owned::album(&state, album_id, user.id).await?; let password_hash = match body.password.as_deref().map(str::trim) { Some(pw) if !pw.is_empty() => { @@ -131,8 +129,10 @@ pub async fn create( /// their way into the 15-minute lock). pub async fn reset_lock( State(state): State, + user: AuthUser, Path(share_id): Path, ) -> ApiResult> { + owned::share(&state, share_id, user.id).await?; let updated = sqlx::query("update shares set failed_attempts = 0, locked_until = null where id = $1") .bind(share_id) @@ -146,8 +146,10 @@ pub async fn reset_lock( pub async fn delete( State(state): State, + user: AuthUser, Path(share_id): Path, ) -> ApiResult> { + owned::share(&state, share_id, user.id).await?; let deleted = sqlx::query("delete from shares where id = $1") .bind(share_id) .execute(&state.db) diff --git a/src/routes/zip.rs b/src/routes/zip.rs index 9088404..8e654f0 100644 --- a/src/routes/zip.rs +++ b/src/routes/zip.rs @@ -81,12 +81,13 @@ async fn album_name(state: &AppState, album_id: Uuid) -> Result, + user: crate::auth::AuthUser, Path(album_id): Path, Form(request): Form, ) -> ApiResult { - let name = album_name(&state, album_id).await?; + let album = crate::owned::album(&state, album_id, user.id).await?; let photos = ready_photos(&state, album_id, &parse_ids(&request.ids)?).await?; - stream_zip(state, photos, &name) + stream_zip(state, photos, &album.name) } pub async fn share_zip( diff --git a/tests/tenancy.rs b/tests/tenancy.rs new file mode 100644 index 0000000..3b6a953 --- /dev/null +++ b/tests/tenancy.rs @@ -0,0 +1,235 @@ +//! Tenant-isolation matrix: every admin endpoint must treat another user's +//! resources as nonexistent (404), and unauthenticated requests as 401. +//! +//! Needs a disposable Postgres database: +//! TEST_DATABASE_URL=postgres://photos:photos@localhost:5432/photos_test cargo test +//! Skips silently when TEST_DATABASE_URL is unset. S3 is never contacted — +//! the matrix stops at the ownership checks by construction. + +use axum::body::Body; +use axum::http::{header, Request, StatusCode}; +use cookie::{Cookie, CookieJar, Key}; +use sha2::{Digest, Sha512}; +use tower::ServiceExt; +use uuid::Uuid; + +use photos::config::Config; +use photos::state::AppState; + +const SECRET: &str = "test-secret-test-secret-test-secret-1234"; + +fn test_config(database_url: String) -> Config { + Config { + database_url, + bind_addr: "127.0.0.1:0".into(), + public_url: "http://localhost:8080".into(), + session_secret: SECRET.into(), + s3_bucket: "photos".into(), + // Unroutable on purpose: no test may reach S3. + s3_endpoint: Some("http://127.0.0.1:9".into()), + s3_region: "us-east-1".into(), + s3_access_key: "test".into(), + s3_secret_key: "test".into(), + s3_force_path_style: true, + oidc_issuer: "https://auth.invalid".into(), + oidc_client_id: "x".into(), + oidc_client_secret: "x".into(), + allowed_emails: vec!["a@test".into(), "b@test".into()], + static_dir: "frontend/dist".into(), + worker_concurrency: 1, + dev_autologin_email: None, + } +} + +fn session_for(user_id: Uuid, email: &str) -> String { + let key = Key::from(&Sha512::digest(SECRET.as_bytes())); + let exp = chrono::Utc::now().timestamp() + 3600; + let mut jar = CookieJar::new(); + jar.signed_mut(&key).add(Cookie::new( + "photos_session", + format!("{user_id}|{exp}|{email}"), + )); + format!("photos_session={}", jar.get("photos_session").unwrap().value()) +} + +async fn request( + router: &axum::Router, + method: &str, + path: &str, + cookie: Option<&str>, + json: Option, +) -> (StatusCode, serde_json::Value) { + let mut builder = Request::builder().method(method).uri(path); + if let Some(cookie) = cookie { + builder = builder.header(header::COOKIE, cookie); + } + let body = match json { + Some(value) => { + builder = builder.header(header::CONTENT_TYPE, "application/json"); + Body::from(value.to_string()) + } + // Zip endpoints take a form; everything else ignores the body. + None if path.ends_with("/zip") => { + builder = builder.header(header::CONTENT_TYPE, "application/x-www-form-urlencoded"); + Body::from("ids=") + } + None => Body::empty(), + }; + let response = router + .clone() + .oneshot(builder.body(body).unwrap()) + .await + .unwrap(); + let status = response.status(); + let bytes = axum::body::to_bytes(response.into_body(), 1 << 20) + .await + .unwrap(); + let value = serde_json::from_slice(&bytes).unwrap_or(serde_json::Value::Null); + (status, value) +} + +async fn seed_user(state: &AppState, email: &str) -> Uuid { + let (id,): (Uuid,) = sqlx::query_as( + "insert into users (oidc_subject, email) values ($1, $2) returning id", + ) + .bind(format!("test:{email}")) + .bind(email) + .fetch_one(&state.db) + .await + .unwrap(); + id +} + +#[tokio::test] +async fn tenant_isolation_matrix() { + let Ok(database_url) = std::env::var("TEST_DATABASE_URL") else { + eprintln!("TEST_DATABASE_URL not set — skipping tenancy matrix"); + return; + }; + let state = AppState::new(test_config(database_url)).await.unwrap(); + sqlx::query("truncate users, albums, photos, shares, ratings, tags, jobs cascade") + .execute(&state.db) + .await + .unwrap(); + + let alice = seed_user(&state, "a@test").await; + let bob = seed_user(&state, "b@test").await; + let cookie_a = session_for(alice, "a@test"); + let cookie_b = session_for(bob, "b@test"); + let router = photos::routes::router(&state).with_state(state.clone()); + + // Alice creates an album through the real API. + let (status, album) = request( + &router, + "POST", + "/api/albums", + Some(&cookie_a), + Some(serde_json::json!({ "name": "Alice's Wedding" })), + ) + .await; + assert_eq!(status, StatusCode::OK); + let album_id = album["id"].as_str().unwrap().to_string(); + + // Seed a photo (kept non-ready so no code path reaches S3) and a share. + let photo_id = Uuid::new_v4(); + sqlx::query( + "insert into photos (id, album_id, filename, content_type, size_bytes, sha256) + values ($1, $2::uuid, 'a.jpg', 'image/jpeg', 3, 'hash-a')", + ) + .bind(photo_id) + .bind(&album_id) + .execute(&state.db) + .await + .unwrap(); + let share_id = Uuid::new_v4(); + sqlx::query("insert into shares (id, album_id, token) values ($1, $2::uuid, 'tenanttesttoken123456789')") + .bind(share_id) + .bind(&album_id) + .execute(&state.db) + .await + .unwrap(); + + // ---- Bob vs Alice's resources: everything must be a 404 (or absent). ---- + let bob_hits: &[(&str, String, Option)] = &[ + ("GET", format!("/api/albums/{album_id}"), None), + ( + "PATCH", + format!("/api/albums/{album_id}"), + Some(serde_json::json!({ "name": "stolen" })), + ), + ("DELETE", format!("/api/albums/{album_id}"), None), + ("POST", format!("/api/albums/{album_id}/photos?filename=x.jpg"), None), + ("GET", format!("/api/albums/{album_id}/shares"), None), + ( + "POST", + format!("/api/albums/{album_id}/shares"), + Some(serde_json::json!({ "label": "x" })), + ), + ("POST", format!("/api/albums/{album_id}/zip"), None), + ("GET", format!("/api/albums/{album_id}/photos/by-hash/hash-a"), None), + ("DELETE", format!("/api/photos/{photo_id}"), None), + ("POST", format!("/api/photos/{photo_id}/reprocess"), None), + ("DELETE", format!("/api/shares/{share_id}"), None), + ("POST", format!("/api/shares/{share_id}/reset-lock"), None), + ]; + for (method, path, body) in bob_hits { + let (status, _) = request(&router, method, path, Some(&cookie_b), body.clone()).await; + assert_eq!( + status, + StatusCode::NOT_FOUND, + "cross-tenant {method} {path} must 404" + ); + } + let (status, list) = request(&router, "GET", "/api/albums", Some(&cookie_b), None).await; + assert_eq!(status, StatusCode::OK); + assert_eq!(list.as_array().unwrap().len(), 0, "bob must see no albums"); + + // ---- Alice keeps full access to her own resources. ---- + let (status, list) = request(&router, "GET", "/api/albums", Some(&cookie_a), None).await; + assert_eq!(status, StatusCode::OK); + assert_eq!(list.as_array().unwrap().len(), 1); + let (status, _) = request( + &router, + "GET", + &format!("/api/albums/{album_id}"), + Some(&cookie_a), + None, + ) + .await; + assert_eq!(status, StatusCode::OK); + let (status, _) = request( + &router, + "GET", + &format!("/api/albums/{album_id}/photos/by-hash/hash-a"), + Some(&cookie_a), + None, + ) + .await; + assert_eq!(status, StatusCode::OK); + // Ownership check runs before the body is read: empty upload = 400, not 404. + let (status, _) = request( + &router, + "POST", + &format!("/api/albums/{album_id}/photos?filename=x.jpg"), + Some(&cookie_a), + None, + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST); + // No ready photos yet: zip is a 400 for the owner, never an S3 call. + let (status, _) = request( + &router, + "POST", + &format!("/api/albums/{album_id}/zip"), + Some(&cookie_a), + None, + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST); + + // ---- No session at all: 401 on the admin surface. ---- + for path in ["/api/albums", "/api/me"] { + let (status, _) = request(&router, "GET", path, None, None).await; + assert_eq!(status, StatusCode::UNAUTHORIZED, "{path} without session"); + } +}