From 9ae17a6ccb54ad3b098f2bdaabf8d09b64c364cd Mon Sep 17 00:00:00 2001 From: D3SOX Date: Sun, 6 Sep 2026 08:13:21 +0200 Subject: [PATCH] fix: scope playlist and subscription group changes to their owners --- src/database.rs | 3 + src/database/ownership_tests.rs | 127 ++++++++++++++++++++++++++++ src/database/playlist.rs | 15 +--- src/database/subscription_groups.rs | 11 +-- src/handlers/subscriptions.rs | 1 + 5 files changed, 136 insertions(+), 21 deletions(-) create mode 100644 src/database/ownership_tests.rs diff --git a/src/database.rs b/src/database.rs index 4c3302f..5f13fb7 100644 --- a/src/database.rs +++ b/src/database.rs @@ -14,3 +14,6 @@ pub mod video; pub mod watch_history; type DbError = diesel::result::Error; + +#[cfg(all(test, feature = "sqlite"))] +mod ownership_tests; diff --git a/src/database/ownership_tests.rs b/src/database/ownership_tests.rs new file mode 100644 index 0000000..2751ac9 --- /dev/null +++ b/src/database/ownership_tests.rs @@ -0,0 +1,127 @@ +use diesel::connection::SimpleConnection; +use diesel_async::AsyncConnection; +use diesel_migrations::MigrationHarness; + +use super::{playlist, subscription_groups}; +use crate::models::SubscriptionGroup; +use crate::{DbConnection, MIGRATIONS}; + +async fn connection() -> DbConnection { + let mut conn = DbConnection::establish(":memory:").await.unwrap(); + conn.spawn_blocking(|conn| { + conn.run_pending_migrations(MIGRATIONS).unwrap(); + conn.batch_execute( + "PRAGMA foreign_keys = ON; + INSERT INTO account (id, name_hash, password_hash) + VALUES ('owner', 'owner-hash', 'password'), ('other', 'other-hash', 'password'); + INSERT INTO channel (id, name, verified) VALUES ('channel', 'Channel', FALSE); + INSERT INTO video (id, title, upload_date, thumbnail_url, duration, uploader_id) + VALUES ('video', 'Video', 0, 'https://i.ytimg.com/vi/video/default.jpg', 60, 'channel'); + INSERT INTO playlist (id, account_id, title, description) + VALUES ('favorites', 'owner', 'Favorites', ''), ('favorites', 'other', 'Favorites', ''); + INSERT INTO playlist_video_member (account_id, playlist_id, video_id) + VALUES ('owner', 'favorites', 'video'), ('other', 'favorites', 'video'); + INSERT INTO subscription_group (id, account_id, title) + VALUES ('group', 'owner', 'Original'); + INSERT INTO subscription_group_member (subscription_group_id, channel_id) + VALUES ('group', 'channel');", + )?; + Ok(()) + }) + .await + .unwrap(); + conn +} + +#[actix_rt::test] +async fn deleting_a_playlist_preserves_other_accounts_with_the_same_id() { + let mut conn = connection().await; + playlist::delete_playlist_by_id(&mut conn, "favorites", "owner") + .await + .unwrap(); + + assert!( + playlist::get_playlist_by_id(&mut conn, "favorites", "owner") + .await + .unwrap() + .is_none() + ); + assert_eq!( + playlist::get_playlist_video_count(&mut conn, "favorites", "owner") + .await + .unwrap(), + 0 + ); + let (_, videos) = playlist::get_playlist_by_id_with_videos(&mut conn, "favorites", "other") + .await + .unwrap() + .expect("the other account's playlist must remain"); + assert_eq!(videos.len(), 1); +} + +#[actix_rt::test] +async fn updating_a_group_requires_ownership_and_preserves_members() { + let mut conn = connection().await; + let result = subscription_groups::update_existing_subscription_group( + &mut conn, + SubscriptionGroup { + id: "group".into(), + account_id: "other".into(), + title: "Stolen".into(), + }, + ) + .await; + assert!(matches!(result, Err(diesel::result::Error::NotFound))); + + let group = subscription_groups::update_existing_subscription_group( + &mut conn, + SubscriptionGroup { + id: "group".into(), + account_id: "owner".into(), + title: "Renamed".into(), + }, + ) + .await + .unwrap(); + assert_eq!(group.title, "Renamed"); + assert_eq!(group.account_id, "owner"); + let groups = subscription_groups::get_subscription_groups_by_account_id(&mut conn, "owner") + .await + .unwrap(); + assert_eq!(groups[0].1.len(), 1); + assert!( + subscription_groups::get_subscription_groups_by_account_id(&mut conn, "other") + .await + .unwrap() + .is_empty() + ); +} + +#[actix_rt::test] +async fn deleting_a_group_only_removes_its_owners_members() { + let mut conn = connection().await; + subscription_groups::delete_subscription_group_by_id(&mut conn, "group", "other") + .await + .unwrap(); + let groups = subscription_groups::get_subscription_groups_by_account_id(&mut conn, "owner") + .await + .unwrap(); + assert_eq!(groups.len(), 1); + assert_eq!(groups[0].1.len(), 1); + + subscription_groups::delete_subscription_group_by_id(&mut conn, "group", "owner") + .await + .unwrap(); + assert!( + subscription_groups::get_subscription_groups_by_account_id(&mut conn, "owner") + .await + .unwrap() + .is_empty() + ); + assert!( + subscription_groups::get_subscription_group_channels_by_id(&mut conn, "group") + .await + .unwrap() + .is_empty() + ); +} diff --git a/src/database/playlist.rs b/src/database/playlist.rs index 5d1fcf2..b846686 100644 --- a/src/database/playlist.rs +++ b/src/database/playlist.rs @@ -60,19 +60,8 @@ pub async fn delete_playlist_by_id( playlist_id_: &str, account_id_: &str, ) -> Result<(), DbError> { - // delete linked videos first to ensure database integrity - // TODO: use ON DELETE CASCADE - diesel::delete( - playlist_video_member.filter( - playlist_id - .eq(playlist_id_.to_string()) - .and(playlist_video_member_account_id.eq(account_id_)), - ), - ) - .execute(conn) - .await?; - - diesel::delete(playlist.filter(id.eq(playlist_id_.to_string()))) + // The composite foreign key cascades only this account's video memberships. + diesel::delete(playlist.filter(id.eq(playlist_id_).and(playlist_account_id.eq(account_id_)))) .execute(conn) .await?; diff --git a/src/database/subscription_groups.rs b/src/database/subscription_groups.rs index e0e5aea..992874b 100644 --- a/src/database/subscription_groups.rs +++ b/src/database/subscription_groups.rs @@ -87,7 +87,8 @@ pub async fn update_existing_subscription_group( ) -> Result { diesel::update(subscription_group) .filter(id.eq(subscription_group_.id.clone())) - .set(subscription_group_) + .filter(account_id.eq(&subscription_group_.account_id)) + .set(title.eq(&subscription_group_.title)) .returning(SubscriptionGroup::as_returning()) .get_result(conn) .await @@ -98,13 +99,7 @@ pub async fn delete_subscription_group_by_id( subscription_group_id_: &str, account_id_: &str, ) -> Result<(), DbError> { - // delete all linked channels first to ensure database integrity - // TODO: use ON DELETE CASCADE - diesel::delete(subscription_group_member) - .filter(subscription_group_id.eq(subscription_group_id_)) - .execute(conn) - .await?; - + // Memberships cascade only after an owned group has been deleted. diesel::delete(subscription_group) .filter( id.eq(subscription_group_id_) diff --git a/src/handlers/subscriptions.rs b/src/handlers/subscriptions.rs index 83c2f07..b6be105 100644 --- a/src/handlers/subscriptions.rs +++ b/src/handlers/subscriptions.rs @@ -295,6 +295,7 @@ async fn update_subscription_group( match update_existing_subscription_group(&mut conn, subscription_group).await { Ok(group) => Ok(HttpResponse::Ok().json(group)), + Err(diesel::result::Error::NotFound) => Err(HandlerError::SubscriptionGroupNotFound), Err(err) => Err(HandlerError::InternalDatabaseErrorWithContext( err.to_string(), )),