From 56ab67f80402752052d0a222c52b4dc3df44cffb Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Thu, 12 Dec 2024 15:59:07 +0100 Subject: [PATCH 01/51] Split image endpoints into API v3 and v4 --- crates/routes/src/images.rs | 64 ++--- src/api_routes_v3.rs | 490 ++++++++++++++++++------------------ src/api_routes_v4.rs | 21 +- 3 files changed, 286 insertions(+), 289 deletions(-) diff --git a/crates/routes/src/images.rs b/crates/routes/src/images.rs index 50897b95dd..dc981f6177 100644 --- a/crates/routes/src/images.rs +++ b/crates/routes/src/images.rs @@ -18,27 +18,13 @@ use lemmy_db_schema::source::{ local_site::LocalSite, }; use lemmy_db_views::structs::LocalUserView; -use lemmy_utils::{error::LemmyResult, rate_limit::RateLimitCell, REQWEST_TIMEOUT}; +use lemmy_utils::{error::LemmyResult, REQWEST_TIMEOUT}; use reqwest::Body; -use reqwest_middleware::{ClientWithMiddleware, RequestBuilder}; +use reqwest_middleware::RequestBuilder; use serde::Deserialize; use std::time::Duration; use url::Url; -pub fn config(cfg: &mut ServiceConfig, client: ClientWithMiddleware, rate_limit: &RateLimitCell) { - cfg - .app_data(Data::new(client)) - .service( - resource("/pictrs/image") - .wrap(rate_limit.image()) - .route(post().to(upload)), - ) - // This has optional query params: /image/{filename}?format=jpg&thumbnail=256 - .service(resource("/pictrs/image/{filename}").route(get().to(full_res))) - .service(resource("/pictrs/image/delete/{token}/{filename}").route(get().to(delete))) - .service(resource("/pictrs/healthz").route(get().to(healthz))); -} - trait ProcessUrl { /// If thumbnail or format is given, this uses the pictrs process endpoint. /// Otherwise, it uses the normal pictrs url (IE image/original). @@ -46,7 +32,7 @@ trait ProcessUrl { } #[derive(Deserialize, Clone)] -struct PictrsGetParams { +pub struct PictrsGetParams { format: Option, thumbnail: Option, } @@ -99,15 +85,12 @@ impl ProcessUrl for ImageProxyParams { } } } -fn adapt_request( - request: &HttpRequest, - client: &ClientWithMiddleware, - url: String, -) -> RequestBuilder { +fn adapt_request(request: &HttpRequest, context: &LemmyContext, url: String) -> RequestBuilder { // remove accept-encoding header so that pictrs doesn't compress the response const INVALID_HEADERS: &[HeaderName] = &[ACCEPT_ENCODING, HOST]; - let client_request = client + let client_request = context + .client() .request(convert_method(request.method()), url) .timeout(REQWEST_TIMEOUT); @@ -124,19 +107,17 @@ fn adapt_request( }) } -async fn upload( +pub async fn upload_image( req: HttpRequest, body: Payload, // require login local_user_view: LocalUserView, - client: Data, context: Data, ) -> LemmyResult { - // TODO: check rate limit here let pictrs_config = context.settings().pictrs_config()?; let image_url = format!("{}image", pictrs_config.url); - let mut client_req = adapt_request(&req, &client, image_url); + let mut client_req = adapt_request(&req, &context, image_url); if let Some(addr) = req.head().peer_addr { client_req = client_req.header("X-Forwarded-For", addr.to_string()) @@ -169,11 +150,10 @@ async fn upload( Ok(HttpResponse::build(convert_status(status)).json(images)) } -async fn full_res( +pub async fn get_full_res_image( filename: Path, Query(params): Query, req: HttpRequest, - client: Data, context: Data, local_user_view: Option, ) -> LemmyResult { @@ -189,15 +169,11 @@ async fn full_res( let processed_url = params.process_url(name, &pictrs_config.url); - image(processed_url, req, &client).await + image(processed_url, req, &context).await } -async fn image( - url: String, - req: HttpRequest, - client: &ClientWithMiddleware, -) -> LemmyResult { - let mut client_req = adapt_request(&req, client, url); +async fn image(url: String, req: HttpRequest, context: &LemmyContext) -> LemmyResult { + let mut client_req = adapt_request(&req, context, url); if let Some(addr) = req.head().peer_addr { client_req = client_req.header("X-Forwarded-For", addr.to_string()); @@ -222,10 +198,9 @@ async fn image( Ok(client_res.body(BodyStream::new(res.bytes_stream()))) } -async fn delete( +pub async fn delete_image( components: Path<(String, String)>, req: HttpRequest, - client: Data, context: Data, // require login _local_user_view: LocalUserView, @@ -235,7 +210,7 @@ async fn delete( let pictrs_config = context.settings().pictrs_config()?; let url = format!("{}image/delete/{}/{}", pictrs_config.url, &token, &file); - let mut client_req = adapt_request(&req, &client, url); + let mut client_req = adapt_request(&req, &context, url); if let Some(addr) = req.head().peer_addr { client_req = client_req.header("X-Forwarded-For", addr.to_string()); @@ -248,15 +223,11 @@ async fn delete( Ok(HttpResponse::build(convert_status(res.status())).body(BodyStream::new(res.bytes_stream()))) } -async fn healthz( - req: HttpRequest, - client: Data, - context: Data, -) -> LemmyResult { +pub async fn pictrs_healthz(req: HttpRequest, context: Data) -> LemmyResult { let pictrs_config = context.settings().pictrs_config()?; let url = format!("{}healthz", pictrs_config.url); - let mut client_req = adapt_request(&req, &client, url); + let mut client_req = adapt_request(&req, &context, url); if let Some(addr) = req.head().peer_addr { client_req = client_req.header("X-Forwarded-For", addr.to_string()); @@ -270,7 +241,6 @@ async fn healthz( pub async fn image_proxy( Query(params): Query, req: HttpRequest, - client: Data, context: Data, ) -> LemmyResult, HttpResponse>> { let url = Url::parse(¶ms.url)?; @@ -291,7 +261,7 @@ pub async fn image_proxy( Ok(Either::Left(Redirect::to(url.to_string()).respond_to(&req))) } else { // Proxy the image data through Lemmy - Ok(Either::Right(image(processed_url, req, &client).await?)) + Ok(Either::Right(image(processed_url, req, &context).await?)) } } diff --git a/src/api_routes_v3.rs b/src/api_routes_v3.rs index eefaf5b871..56dde6db95 100644 --- a/src/api_routes_v3.rs +++ b/src/api_routes_v3.rs @@ -134,252 +134,262 @@ use lemmy_apub::api::{ search::search, user_settings_backup::{export_settings, import_settings}, }; -use lemmy_routes::images::image_proxy; +use lemmy_routes::images::{delete_image, get_full_res_image, image_proxy, pictrs_healthz, upload_image}; use lemmy_utils::rate_limit::RateLimitCell; // Deprecated, use api v4 instead. // When removing api v3, we also need to rewrite all links in database with // `/api/v3/image_proxy` to use `/api/v4/image_proxy` instead. pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { - cfg.service( - scope("/api/v3") - .route("/image_proxy", get().to(image_proxy)) - // Site - .service( - scope("/site") - .wrap(rate_limit.message()) - .route("", get().to(get_site_v3)) - // Admin Actions - .route("", post().to(create_site)) - .route("", put().to(update_site)) - .route("/block", post().to(user_block_instance)), - ) - .service( - resource("/modlog") - .wrap(rate_limit.message()) - .route(get().to(get_mod_log)), - ) - .service( - resource("/search") - .wrap(rate_limit.search()) - .route(get().to(search)), - ) - .service( - resource("/resolve_object") - .wrap(rate_limit.message()) - .route(get().to(resolve_object)), - ) - // Community - .service( - resource("/community") - .guard(guard::Post()) - .wrap(rate_limit.register()) - .route(post().to(create_community)), - ) - .service( - scope("/community") - .wrap(rate_limit.message()) - .route("", get().to(get_community)) - .route("", put().to(update_community)) - .route("/hide", put().to(hide_community)) - .route("/list", get().to(list_communities)) - .route("/follow", post().to(follow_community)) - .route("/block", post().to(user_block_community)) - .route("/delete", post().to(delete_community)) - // Mod Actions - .route("/remove", post().to(remove_community)) - .route("/transfer", post().to(transfer_community)) - .route("/ban_user", post().to(ban_from_community)) - .route("/mod", post().to(add_mod_to_community)), - ) - .service( - scope("/federated_instances") - .wrap(rate_limit.message()) - .route("", get().to(get_federated_instances)), - ) - // Post - .service( - // Handle POST to /post separately to add the post() rate limitter - resource("/post") - .guard(guard::Post()) - .wrap(rate_limit.post()) - .route(post().to(create_post)), - ) - .service( - scope("/post") - .wrap(rate_limit.message()) - .route("", get().to(get_post)) - .route("", put().to(update_post)) - .route("/delete", post().to(delete_post)) - .route("/remove", post().to(remove_post)) - .route("/mark_as_read", post().to(mark_post_as_read)) - .route("/hide", post().to(hide_post)) - .route("/lock", post().to(lock_post)) - .route("/feature", post().to(feature_post)) - .route("/list", get().to(list_posts)) - .route("/like", post().to(like_post)) - .route("/like/list", get().to(list_post_likes)) - .route("/save", put().to(save_post)) - .route("/report", post().to(create_post_report)) - .route("/report/resolve", put().to(resolve_post_report)) - .route("/report/list", get().to(list_post_reports)) - .route("/site_metadata", get().to(get_link_metadata)), - ) - // Comment - .service( - // Handle POST to /comment separately to add the comment() rate limitter - resource("/comment") - .guard(guard::Post()) - .wrap(rate_limit.comment()) - .route(post().to(create_comment)), - ) - .service( - scope("/comment") - .wrap(rate_limit.message()) - .route("", get().to(get_comment)) - .route("", put().to(update_comment)) - .route("/delete", post().to(delete_comment)) - .route("/remove", post().to(remove_comment)) - .route("/mark_as_read", post().to(mark_reply_as_read)) - .route("/distinguish", post().to(distinguish_comment)) - .route("/like", post().to(like_comment)) - .route("/like/list", get().to(list_comment_likes)) - .route("/save", put().to(save_comment)) - .route("/list", get().to(list_comments)) - .route("/report", post().to(create_comment_report)) - .route("/report/resolve", put().to(resolve_comment_report)) - .route("/report/list", get().to(list_comment_reports)), - ) - // Private Message - .service( - scope("/private_message") - .wrap(rate_limit.message()) - .route("/list", get().to(get_private_message)) - .route("", post().to(create_private_message)) - .route("", put().to(update_private_message)) - .route("/delete", post().to(delete_private_message)) - .route("/mark_as_read", post().to(mark_pm_as_read)) - .route("/report", post().to(create_pm_report)) - .route("/report/resolve", put().to(resolve_pm_report)) - .route("/report/list", get().to(list_pm_reports)), - ) - // User - .service( - // Account action, I don't like that it's in /user maybe /accounts - // Handle /user/register separately to add the register() rate limiter - resource("/user/register") - .guard(guard::Post()) - .wrap(rate_limit.register()) - .route(post().to(register)), - ) - // User - .service( - // Handle /user/login separately to add the register() rate limiter - // TODO: pretty annoying way to apply rate limits for register and login, we should - // group them under a common path so that rate limit is only applied once (eg under - // /account). - resource("/user/login") - .guard(guard::Post()) - .wrap(rate_limit.register()) - .route(post().to(login)), - ) - .service( - resource("/user/password_reset") - .wrap(rate_limit.register()) - .route(post().to(reset_password)), - ) - .service( - // Handle captcha separately - resource("/user/get_captcha") - .wrap(rate_limit.post()) - .route(get().to(get_captcha)), - ) - .service( - resource("/user/export_settings") - .wrap(rate_limit.import_user_settings()) - .route(get().to(export_settings)), - ) - .service( - resource("/user/import_settings") - .wrap(rate_limit.import_user_settings()) - .route(post().to(import_settings)), - ) - // TODO, all the current account related actions under /user need to get moved here eventually - .service( - scope("/account") - .wrap(rate_limit.message()) - .route("/list_media", get().to(list_media)), - ) - // User actions - .service( - scope("/user") - .wrap(rate_limit.message()) - .route("", get().to(read_person)) - .route("/mention", get().to(list_mentions)) - .route( - "/mention/mark_as_read", - post().to(mark_person_mention_as_read), - ) - .route("/replies", get().to(list_replies)) - // Admin action. I don't like that it's in /user - .route("/ban", post().to(ban_from_site)) - .route("/banned", get().to(list_banned_users)) - .route("/block", post().to(user_block_person)) - // TODO Account actions. I don't like that they're in /user maybe /accounts - .route("/logout", post().to(logout)) - .route("/delete_account", post().to(delete_account)) - .route("/password_change", post().to(change_password_after_reset)) - // TODO mark_all_as_read feels off being in this section as well - .route("/mark_all_as_read", post().to(mark_all_notifications_read)) - .route("/save_user_settings", put().to(save_user_settings)) - .route("/change_password", put().to(change_password)) - .route("/report_count", get().to(report_count)) - .route("/unread_count", get().to(unread_count)) - .route("/verify_email", post().to(verify_email)) - .route("/leave_admin", post().to(leave_admin)) - .route("/totp/generate", post().to(generate_totp_secret)) - .route("/totp/update", post().to(update_totp)) - .route("/list_logins", get().to(list_logins)) - .route("/validate_auth", get().to(validate_auth)), - ) - // Admin Actions - .service( - scope("/admin") - .wrap(rate_limit.message()) - .route("/add", post().to(add_admin)) - .route( - "/registration_application/count", - get().to(get_unread_registration_application_count), - ) - .route( - "/registration_application/list", - get().to(list_registration_applications), - ) - .route( - "/registration_application/approve", - put().to(approve_registration_application), - ) - .route( - "/registration_application", - get().to(get_registration_application), - ) - .route("/list_all_media", get().to(list_all_media)) - .service( - scope("/purge") - .route("/person", post().to(purge_person)) - .route("/community", post().to(purge_community)) - .route("/post", post().to(purge_post)) - .route("/comment", post().to(purge_comment)), - ), - ) - .service( - scope("/custom_emoji") - .wrap(rate_limit.message()) - .route("", post().to(create_custom_emoji)) - .route("", put().to(update_custom_emoji)) - .route("/delete", post().to(delete_custom_emoji)), - ), - ); + cfg + .service( + resource("/pictrs/image") + .wrap(rate_limit.image()) + .route(post().to(upload_image)), + ) + .service(resource("/pictrs/image/{filename}").route(get().to(get_full_res_image))) + .service(resource("/pictrs/image/delete/{token}/{filename}").route(get().to(delete_image))) + .service(resource("/pictrs/healthz").route(get().to(pictrs_healthz))) + .service( + scope("/api/v3") + .route("/image_proxy", get().to(image_proxy)) + // Site + .service( + scope("/site") + .wrap(rate_limit.message()) + .route("", get().to(get_site_v3)) + // Admin Actions + .route("", post().to(create_site)) + .route("", put().to(update_site)) + .route("/block", post().to(user_block_instance)), + ) + .service( + resource("/modlog") + .wrap(rate_limit.message()) + .route(get().to(get_mod_log)), + ) + .service( + resource("/search") + .wrap(rate_limit.search()) + .route(get().to(search)), + ) + .service( + resource("/resolve_object") + .wrap(rate_limit.message()) + .route(get().to(resolve_object)), + ) + // Community + .service( + resource("/community") + .guard(guard::Post()) + .wrap(rate_limit.register()) + .route(post().to(create_community)), + ) + .service( + scope("/community") + .wrap(rate_limit.message()) + .route("", get().to(get_community)) + .route("", put().to(update_community)) + .route("/hide", put().to(hide_community)) + .route("/list", get().to(list_communities)) + .route("/follow", post().to(follow_community)) + .route("/block", post().to(user_block_community)) + .route("/delete", post().to(delete_community)) + // Mod Actions + .route("/remove", post().to(remove_community)) + .route("/transfer", post().to(transfer_community)) + .route("/ban_user", post().to(ban_from_community)) + .route("/mod", post().to(add_mod_to_community)), + ) + .service( + scope("/federated_instances") + .wrap(rate_limit.message()) + .route("", get().to(get_federated_instances)), + ) + // Post + .service( + // Handle POST to /post separately to add the post() rate limitter + resource("/post") + .guard(guard::Post()) + .wrap(rate_limit.post()) + .route(post().to(create_post)), + ) + .service( + scope("/post") + .wrap(rate_limit.message()) + .route("", get().to(get_post)) + .route("", put().to(update_post)) + .route("/delete", post().to(delete_post)) + .route("/remove", post().to(remove_post)) + .route("/mark_as_read", post().to(mark_post_as_read)) + .route("/hide", post().to(hide_post)) + .route("/lock", post().to(lock_post)) + .route("/feature", post().to(feature_post)) + .route("/list", get().to(list_posts)) + .route("/like", post().to(like_post)) + .route("/like/list", get().to(list_post_likes)) + .route("/save", put().to(save_post)) + .route("/report", post().to(create_post_report)) + .route("/report/resolve", put().to(resolve_post_report)) + .route("/report/list", get().to(list_post_reports)) + .route("/site_metadata", get().to(get_link_metadata)), + ) + // Comment + .service( + // Handle POST to /comment separately to add the comment() rate limitter + resource("/comment") + .guard(guard::Post()) + .wrap(rate_limit.comment()) + .route(post().to(create_comment)), + ) + .service( + scope("/comment") + .wrap(rate_limit.message()) + .route("", get().to(get_comment)) + .route("", put().to(update_comment)) + .route("/delete", post().to(delete_comment)) + .route("/remove", post().to(remove_comment)) + .route("/mark_as_read", post().to(mark_reply_as_read)) + .route("/distinguish", post().to(distinguish_comment)) + .route("/like", post().to(like_comment)) + .route("/like/list", get().to(list_comment_likes)) + .route("/save", put().to(save_comment)) + .route("/list", get().to(list_comments)) + .route("/report", post().to(create_comment_report)) + .route("/report/resolve", put().to(resolve_comment_report)) + .route("/report/list", get().to(list_comment_reports)), + ) + // Private Message + .service( + scope("/private_message") + .wrap(rate_limit.message()) + .route("/list", get().to(get_private_message)) + .route("", post().to(create_private_message)) + .route("", put().to(update_private_message)) + .route("/delete", post().to(delete_private_message)) + .route("/mark_as_read", post().to(mark_pm_as_read)) + .route("/report", post().to(create_pm_report)) + .route("/report/resolve", put().to(resolve_pm_report)) + .route("/report/list", get().to(list_pm_reports)), + ) + // User + .service( + // Account action, I don't like that it's in /user maybe /accounts + // Handle /user/register separately to add the register() rate limiter + resource("/user/register") + .guard(guard::Post()) + .wrap(rate_limit.register()) + .route(post().to(register)), + ) + // User + .service( + // Handle /user/login separately to add the register() rate limiter + // TODO: pretty annoying way to apply rate limits for register and login, we should + // group them under a common path so that rate limit is only applied once (eg under + // /account). + resource("/user/login") + .guard(guard::Post()) + .wrap(rate_limit.register()) + .route(post().to(login)), + ) + .service( + resource("/user/password_reset") + .wrap(rate_limit.register()) + .route(post().to(reset_password)), + ) + .service( + // Handle captcha separately + resource("/user/get_captcha") + .wrap(rate_limit.post()) + .route(get().to(get_captcha)), + ) + .service( + resource("/user/export_settings") + .wrap(rate_limit.import_user_settings()) + .route(get().to(export_settings)), + ) + .service( + resource("/user/import_settings") + .wrap(rate_limit.import_user_settings()) + .route(post().to(import_settings)), + ) + // TODO, all the current account related actions under /user need to get moved here + // eventually + .service( + scope("/account") + .wrap(rate_limit.message()) + .route("/list_media", get().to(list_media)), + ) + // User actions + .service( + scope("/user") + .wrap(rate_limit.message()) + .route("", get().to(read_person)) + .route("/mention", get().to(list_mentions)) + .route( + "/mention/mark_as_read", + post().to(mark_person_mention_as_read), + ) + .route("/replies", get().to(list_replies)) + // Admin action. I don't like that it's in /user + .route("/ban", post().to(ban_from_site)) + .route("/banned", get().to(list_banned_users)) + .route("/block", post().to(user_block_person)) + // TODO Account actions. I don't like that they're in /user maybe /accounts + .route("/logout", post().to(logout)) + .route("/delete_account", post().to(delete_account)) + .route("/password_change", post().to(change_password_after_reset)) + // TODO mark_all_as_read feels off being in this section as well + .route("/mark_all_as_read", post().to(mark_all_notifications_read)) + .route("/save_user_settings", put().to(save_user_settings)) + .route("/change_password", put().to(change_password)) + .route("/report_count", get().to(report_count)) + .route("/unread_count", get().to(unread_count)) + .route("/verify_email", post().to(verify_email)) + .route("/leave_admin", post().to(leave_admin)) + .route("/totp/generate", post().to(generate_totp_secret)) + .route("/totp/update", post().to(update_totp)) + .route("/list_logins", get().to(list_logins)) + .route("/validate_auth", get().to(validate_auth)), + ) + // Admin Actions + .service( + scope("/admin") + .wrap(rate_limit.message()) + .route("/add", post().to(add_admin)) + .route( + "/registration_application/count", + get().to(get_unread_registration_application_count), + ) + .route( + "/registration_application/list", + get().to(list_registration_applications), + ) + .route( + "/registration_application/approve", + put().to(approve_registration_application), + ) + .route( + "/registration_application", + get().to(get_registration_application), + ) + .route("/list_all_media", get().to(list_all_media)) + .service( + scope("/purge") + .route("/person", post().to(purge_person)) + .route("/community", post().to(purge_community)) + .route("/post", post().to(purge_post)) + .route("/comment", post().to(purge_comment)), + ), + ) + .service( + scope("/custom_emoji") + .wrap(rate_limit.message()) + .route("", post().to(create_custom_emoji)) + .route("", put().to(update_custom_emoji)) + .route("/delete", post().to(delete_custom_emoji)), + ), + ); cfg.service( scope("/sitemap.xml") .wrap(rate_limit.message()) diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index a9f71c9da9..ee88274c94 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -159,7 +159,13 @@ use lemmy_apub::api::{ search::search, user_settings_backup::{export_settings, import_settings}, }; -use lemmy_routes::images::image_proxy; +use lemmy_routes::images::{ + delete_image, + get_full_res_image, + image_proxy, + pictrs_healthz, + upload_image, +}; use lemmy_utils::rate_limit::RateLimitCell; pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { @@ -387,6 +393,17 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .wrap(rate_limit.register()) .route("/authenticate", post().to(authenticate_with_oauth)), ) - .route("/sitemap.xml", get().to(get_sitemap)), + .route("/sitemap.xml", get().to(get_sitemap)) + .service( + scope("/image") + .service( + resource("") + .wrap(rate_limit.image()) + .route(post().to(upload_image)), + ) + .route("/{filename}", get().to(get_full_res_image)) + .route("{token}/{filename}", delete().to(delete_image)) + .route("/healthz", get().to(pictrs_healthz)), + ), ); } From 05843fb7b5d6d1808b2473274eb84bc503d47641 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Thu, 12 Dec 2024 16:06:51 +0100 Subject: [PATCH 02/51] Move into subfolders --- .../routes/src/{images.rs => images/mod.rs} | 68 ++----------------- crates/routes/src/images/utils.rs | 63 +++++++++++++++++ .../utils/src/utils/markdown/image_links.rs | 12 ++-- src/api_routes_v3.rs | 2 +- src/api_routes_v4.rs | 2 +- 5 files changed, 78 insertions(+), 69 deletions(-) rename crates/routes/src/{images.rs => images/mod.rs} (82%) create mode 100644 crates/routes/src/images/utils.rs diff --git a/crates/routes/src/images.rs b/crates/routes/src/images/mod.rs similarity index 82% rename from crates/routes/src/images.rs rename to crates/routes/src/images/mod.rs index dc981f6177..0f98b330e3 100644 --- a/crates/routes/src/images.rs +++ b/crates/routes/src/images/mod.rs @@ -2,7 +2,6 @@ use actix_web::{ body::{BodyStream, BoxBody}, http::{ header::{HeaderName, ACCEPT_ENCODING, HOST}, - Method, StatusCode, }, web::*, @@ -10,8 +9,6 @@ use actix_web::{ HttpResponse, Responder, }; -use futures::stream::{Stream, StreamExt}; -use http::HeaderValue; use lemmy_api_common::{context::LemmyContext, request::PictrsResponse}; use lemmy_db_schema::source::{ images::{LocalImage, LocalImageForm, RemoteImage}, @@ -24,6 +21,9 @@ use reqwest_middleware::RequestBuilder; use serde::Deserialize; use std::time::Duration; use url::Url; +use utils::{convert_header, convert_method, convert_status, make_send}; + +mod utils; trait ProcessUrl { /// If thumbnail or format is given, this uses the pictrs process endpoint. @@ -223,7 +223,10 @@ pub async fn delete_image( Ok(HttpResponse::build(convert_status(res.status())).body(BodyStream::new(res.bytes_stream()))) } -pub async fn pictrs_healthz(req: HttpRequest, context: Data) -> LemmyResult { +pub async fn pictrs_healthz( + req: HttpRequest, + context: Data, +) -> LemmyResult { let pictrs_config = context.settings().pictrs_config()?; let url = format!("{}healthz", pictrs_config.url); @@ -264,60 +267,3 @@ pub async fn image_proxy( Ok(Either::Right(image(processed_url, req, &context).await?)) } } - -fn make_send(mut stream: S) -> impl Stream + Send + Unpin + 'static -where - S: Stream + Unpin + 'static, - S::Item: Send, -{ - // NOTE: the 8 here is arbitrary - let (tx, rx) = tokio::sync::mpsc::channel(8); - - // NOTE: spawning stream into a new task can potentially hit this bug: - // - https://github.com/actix/actix-web/issues/1679 - // - // Since 4.0.0-beta.2 this issue is incredibly less frequent. I have not personally reproduced it. - // That said, it is still technically possible to encounter. - actix_web::rt::spawn(async move { - while let Some(res) = stream.next().await { - if tx.send(res).await.is_err() { - break; - } - } - }); - - SendStream { rx } -} - -struct SendStream { - rx: tokio::sync::mpsc::Receiver, -} - -impl Stream for SendStream -where - T: Send, -{ - type Item = T; - - fn poll_next( - mut self: std::pin::Pin<&mut Self>, - cx: &mut std::task::Context<'_>, - ) -> std::task::Poll> { - std::pin::Pin::new(&mut self.rx).poll_recv(cx) - } -} - -// TODO: remove these conversions after actix-web upgrades to http 1.0 -#[allow(clippy::expect_used)] -fn convert_status(status: http::StatusCode) -> StatusCode { - StatusCode::from_u16(status.as_u16()).expect("status can be converted") -} - -#[allow(clippy::expect_used)] -fn convert_method(method: &Method) -> http::Method { - http::Method::from_bytes(method.as_str().as_bytes()).expect("method can be converted") -} - -fn convert_header<'a>(name: &'a http::HeaderName, value: &'a HeaderValue) -> (&'a str, &'a [u8]) { - (name.as_str(), value.as_bytes()) -} diff --git a/crates/routes/src/images/utils.rs b/crates/routes/src/images/utils.rs new file mode 100644 index 0000000000..c951b4b014 --- /dev/null +++ b/crates/routes/src/images/utils.rs @@ -0,0 +1,63 @@ +use actix_web::http::{Method, StatusCode}; +use futures::stream::{Stream, StreamExt}; +use http::HeaderValue; + +pub(super) fn make_send(mut stream: S) -> impl Stream + Send + Unpin + 'static +where + S: Stream + Unpin + 'static, + S::Item: Send, +{ + // NOTE: the 8 here is arbitrary + let (tx, rx) = tokio::sync::mpsc::channel(8); + + // NOTE: spawning stream into a new task can potentially hit this bug: + // - https://github.com/actix/actix-web/issues/1679 + // + // Since 4.0.0-beta.2 this issue is incredibly less frequent. I have not personally reproduced it. + // That said, it is still technically possible to encounter. + actix_web::rt::spawn(async move { + while let Some(res) = stream.next().await { + if tx.send(res).await.is_err() { + break; + } + } + }); + + SendStream { rx } +} + +pub(super) struct SendStream { + rx: tokio::sync::mpsc::Receiver, +} + +impl Stream for SendStream +where + T: Send, +{ + type Item = T; + + fn poll_next( + mut self: std::pin::Pin<&mut Self>, + cx: &mut std::task::Context<'_>, + ) -> std::task::Poll> { + std::pin::Pin::new(&mut self.rx).poll_recv(cx) + } +} + +// TODO: remove these conversions after actix-web upgrades to http 1.0 +#[allow(clippy::expect_used)] +pub(super) fn convert_status(status: http::StatusCode) -> StatusCode { + StatusCode::from_u16(status.as_u16()).expect("status can be converted") +} + +#[allow(clippy::expect_used)] +pub(super) fn convert_method(method: &Method) -> http::Method { + http::Method::from_bytes(method.as_str().as_bytes()).expect("method can be converted") +} + +pub(super) fn convert_header<'a>( + name: &'a http::HeaderName, + value: &'a HeaderValue, +) -> (&'a str, &'a [u8]) { + (name.as_str(), value.as_bytes()) +} diff --git a/crates/utils/src/utils/markdown/image_links.rs b/crates/utils/src/utils/markdown/image_links.rs index 0990b1bc70..2185bdcad2 100644 --- a/crates/utils/src/utils/markdown/image_links.rs +++ b/crates/utils/src/utils/markdown/image_links.rs @@ -18,7 +18,7 @@ pub fn markdown_rewrite_image_links(mut src: String) -> (String, Vec) { // If link points to remote domain, replace with proxied link if parsed.domain() != Some(&SETTINGS.hostname) { let mut proxied = format!( - "{}/api/v4/image_proxy?url={}", + "{}/api/v4/image/proxy?url={}", SETTINGS.get_protocol_and_hostname(), encode(url), ); @@ -115,7 +115,7 @@ mod tests { ( "remote image proxied", "![link](http://example.com/image.jpg)", - "![link](https://lemmy-alpha/api/v4/image_proxy?url=http%3A%2F%2Fexample.com%2Fimage.jpg)", + "![link](https://lemmy-alpha/api/v4/image/proxy?url=http%3A%2F%2Fexample.com%2Fimage.jpg)", ), ( "local image unproxied", @@ -125,7 +125,7 @@ mod tests { ( "multiple image links", "![link](http://example.com/image1.jpg) ![link](http://example.com/image2.jpg)", - "![link](https://lemmy-alpha/api/v4/image_proxy?url=http%3A%2F%2Fexample.com%2Fimage1.jpg) ![link](https://lemmy-alpha/api/v4/image_proxy?url=http%3A%2F%2Fexample.com%2Fimage2.jpg)", + "![link](https://lemmy-alpha/api/v4/image/proxy?url=http%3A%2F%2Fexample.com%2Fimage1.jpg) ![link](https://lemmy-alpha/api/v4/image/proxy?url=http%3A%2F%2Fexample.com%2Fimage2.jpg)", ), ( "empty link handled", @@ -135,7 +135,7 @@ mod tests { ( "empty label handled", "![](http://example.com/image.jpg)", - "![](https://lemmy-alpha/api/v4/image_proxy?url=http%3A%2F%2Fexample.com%2Fimage.jpg)" + "![](https://lemmy-alpha/api/v4/image/proxy?url=http%3A%2F%2Fexample.com%2Fimage.jpg)" ), ( "invalid image link removed", @@ -145,12 +145,12 @@ mod tests { ( "label with nested markdown handled", "![a *b* c](http://example.com/image.jpg)", - "![a *b* c](https://lemmy-alpha/api/v4/image_proxy?url=http%3A%2F%2Fexample.com%2Fimage.jpg)" + "![a *b* c](https://lemmy-alpha/api/v4/image/proxy?url=http%3A%2F%2Fexample.com%2Fimage.jpg)" ), ( "custom emoji support", r#"![party-blob](https://www.hexbear.net/pictrs/image/83405746-0620-4728-9358-5f51b040ffee.gif "emoji party-blob")"#, - r#"![party-blob](https://lemmy-alpha/api/v4/image_proxy?url=https%3A%2F%2Fwww.hexbear.net%2Fpictrs%2Fimage%2F83405746-0620-4728-9358-5f51b040ffee.gif "emoji party-blob")"# + r#"![party-blob](https://lemmy-alpha/api/v4/image/proxy?url=https%3A%2F%2Fwww.hexbear.net%2Fpictrs%2Fimage%2F83405746-0620-4728-9358-5f51b040ffee.gif "emoji party-blob")"# ) ]; diff --git a/src/api_routes_v3.rs b/src/api_routes_v3.rs index 56dde6db95..452271dd1f 100644 --- a/src/api_routes_v3.rs +++ b/src/api_routes_v3.rs @@ -139,7 +139,7 @@ use lemmy_utils::rate_limit::RateLimitCell; // Deprecated, use api v4 instead. // When removing api v3, we also need to rewrite all links in database with -// `/api/v3/image_proxy` to use `/api/v4/image_proxy` instead. +// `/api/v3/image_proxy` to use `/api/v4/image/proxy` instead. pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { cfg .service( diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index ee88274c94..48f4031d4b 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -172,7 +172,6 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { cfg.service( scope("/api/v4") .wrap(rate_limit.message()) - .route("/image_proxy", get().to(image_proxy)) // Site .service( scope("/site") @@ -401,6 +400,7 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .wrap(rate_limit.image()) .route(post().to(upload_image)), ) + .route("/proxy", get().to(image_proxy)) .route("/{filename}", get().to(get_full_res_image)) .route("{token}/{filename}", delete().to(delete_image)) .route("/healthz", get().to(pictrs_healthz)), From cfa866a53427ac15d1f70fe10a8855f7321fd34f Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 13 Dec 2024 11:53:43 +0100 Subject: [PATCH 03/51] Upload avatar endpoint and other changes --- Cargo.lock | 1 + api_tests/run-federation-test.sh | 2 +- crates/api/src/local_user/save_settings.rs | 5 - crates/api_common/src/person.rs | 3 - crates/api_common/src/request.rs | 9 +- crates/routes/Cargo.toml | 1 + crates/routes/src/images/mod.rs | 219 ++++++--------------- crates/routes/src/images/person.rs | 36 ++++ crates/routes/src/images/utils.rs | 183 ++++++++++++++++- crates/utils/src/error.rs | 2 +- src/api_routes_v3.rs | 8 +- src/api_routes_v4.rs | 12 +- src/lib.rs | 12 +- 13 files changed, 301 insertions(+), 192 deletions(-) create mode 100644 crates/routes/src/images/person.rs diff --git a/Cargo.lock b/Cargo.lock index eebb1ce1a7..0b19fe6f6a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2773,6 +2773,7 @@ dependencies = [ "lemmy_utils", "reqwest 0.12.8", "reqwest-middleware", + "reqwest-tracing", "rss", "serde", "tokio", diff --git a/api_tests/run-federation-test.sh b/api_tests/run-federation-test.sh index 969a95b3e7..f9eab50395 100755 --- a/api_tests/run-federation-test.sh +++ b/api_tests/run-federation-test.sh @@ -11,7 +11,7 @@ killall -s1 lemmy_server || true popd pnpm i -pnpm api-test || true +pnpm api-test-image || true killall -s1 lemmy_server || true killall -s1 pict-rs || true diff --git a/crates/api/src/local_user/save_settings.rs b/crates/api/src/local_user/save_settings.rs index 992fea1635..bcf1b02b6e 100644 --- a/crates/api/src/local_user/save_settings.rs +++ b/crates/api/src/local_user/save_settings.rs @@ -46,10 +46,6 @@ pub async fn save_user_settings( .as_deref(), ); - let avatar = diesel_url_update(data.avatar.as_deref())?; - replace_image(&avatar, &local_user_view.person.avatar, &context).await?; - let avatar = proxy_image_link_opt_api(avatar, &context).await?; - let banner = diesel_url_update(data.banner.as_deref())?; replace_image(&banner, &local_user_view.person.banner, &context).await?; let banner = proxy_image_link_opt_api(banner, &context).await?; @@ -108,7 +104,6 @@ pub async fn save_user_settings( bio, matrix_user_id, bot_account: data.bot_account, - avatar, banner, ..Default::default() }; diff --git a/crates/api_common/src/person.rs b/crates/api_common/src/person.rs index b95cf5e774..9c8a1d087b 100644 --- a/crates/api_common/src/person.rs +++ b/crates/api_common/src/person.rs @@ -114,9 +114,6 @@ pub struct SaveUserSettings { /// The language of the lemmy interface #[cfg_attr(feature = "full", ts(optional))] pub interface_language: Option, - /// A URL for your avatar. - #[cfg_attr(feature = "full", ts(optional))] - pub avatar: Option, /// A URL for your banner. #[cfg_attr(feature = "full", ts(optional))] pub banner: Option, diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index c6f86b8067..5b4d85a5e8 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -250,7 +250,8 @@ fn extract_opengraph_data(html_bytes: &[u8], url: &Url) -> LemmyResult>, + #[serde(default)] + pub files: Vec, pub msg: String, } @@ -388,9 +389,8 @@ async fn generate_pictrs_thumbnail(image_url: &Url, context: &LemmyContext) -> L .json::() .await?; - let files = res.files.unwrap_or_default(); - - let image = files + let image = res + .files .first() .ok_or(LemmyErrorType::PictrsResponseError(res.msg))?; @@ -467,6 +467,7 @@ async fn is_image_content_type(client: &ClientWithMiddleware, url: &Url) -> Lemm } /// When adding a new avatar, banner or similar image, delete the old one. +/// TODO: remove this function pub async fn replace_image( new_image: &Option>, old_image: &Option, diff --git a/crates/routes/Cargo.toml b/crates/routes/Cargo.toml index 91c3ed6830..3bd85d0e5f 100644 --- a/crates/routes/Cargo.toml +++ b/crates/routes/Cargo.toml @@ -33,4 +33,5 @@ url = { workspace = true } tracing = { workspace = true } tokio = { workspace = true } http.workspace = true +reqwest-tracing = { workspace = true } rss = "2.0.10" diff --git a/crates/routes/src/images/mod.rs b/crates/routes/src/images/mod.rs index 0f98b330e3..c5f6a9c2c5 100644 --- a/crates/routes/src/images/mod.rs +++ b/crates/routes/src/images/mod.rs @@ -1,112 +1,33 @@ use actix_web::{ body::{BodyStream, BoxBody}, - http::{ - header::{HeaderName, ACCEPT_ENCODING, HOST}, - StatusCode, - }, + http::StatusCode, web::*, HttpRequest, HttpResponse, Responder, }; -use lemmy_api_common::{context::LemmyContext, request::PictrsResponse}; +use lemmy_api_common::{context::LemmyContext, SuccessResponse}; use lemmy_db_schema::source::{ - images::{LocalImage, LocalImageForm, RemoteImage}, + images::{LocalImage, RemoteImage}, local_site::LocalSite, }; use lemmy_db_views::structs::LocalUserView; -use lemmy_utils::{error::LemmyResult, REQWEST_TIMEOUT}; -use reqwest::Body; -use reqwest_middleware::RequestBuilder; +use lemmy_utils::error::LemmyResult; use serde::Deserialize; -use std::time::Duration; use url::Url; -use utils::{convert_header, convert_method, convert_status, make_send}; +use utils::{ + adapt_request, + convert_header, + do_upload_image, + PictrsGetParams, + ProcessUrl, + UploadType, + PICTRS_CLIENT, +}; +pub mod person; mod utils; -trait ProcessUrl { - /// If thumbnail or format is given, this uses the pictrs process endpoint. - /// Otherwise, it uses the normal pictrs url (IE image/original). - fn process_url(&self, image_url: &str, pictrs_url: &Url) -> String; -} - -#[derive(Deserialize, Clone)] -pub struct PictrsGetParams { - format: Option, - thumbnail: Option, -} - -impl ProcessUrl for PictrsGetParams { - fn process_url(&self, src: &str, pictrs_url: &Url) -> String { - if self.format.is_none() && self.thumbnail.is_none() { - format!("{}image/original/{}", pictrs_url, src) - } else { - // Take file type from name, or jpg if nothing is given - let format = self - .clone() - .format - .unwrap_or_else(|| src.split('.').last().unwrap_or("jpg").to_string()); - - let mut url = format!("{}image/process.{}?src={}", pictrs_url, format, src); - - if let Some(size) = self.thumbnail { - url = format!("{url}&thumbnail={size}",); - } - url - } - } -} - -#[derive(Deserialize, Clone)] -pub struct ImageProxyParams { - url: String, - format: Option, - thumbnail: Option, -} - -impl ProcessUrl for ImageProxyParams { - fn process_url(&self, proxy_url: &str, pictrs_url: &Url) -> String { - if self.format.is_none() && self.thumbnail.is_none() { - format!("{}image/original?proxy={}", pictrs_url, proxy_url) - } else { - // Take file type from name, or jpg if nothing is given - let format = self - .clone() - .format - .unwrap_or_else(|| proxy_url.split('.').last().unwrap_or("jpg").to_string()); - - let mut url = format!("{}image/process.{}?proxy={}", pictrs_url, format, proxy_url); - - if let Some(size) = self.thumbnail { - url = format!("{url}&thumbnail={size}",); - } - url - } - } -} -fn adapt_request(request: &HttpRequest, context: &LemmyContext, url: String) -> RequestBuilder { - // remove accept-encoding header so that pictrs doesn't compress the response - const INVALID_HEADERS: &[HeaderName] = &[ACCEPT_ENCODING, HOST]; - - let client_request = context - .client() - .request(convert_method(request.method()), url) - .timeout(REQWEST_TIMEOUT); - - request - .headers() - .iter() - .fold(client_request, |client_req, (key, value)| { - if INVALID_HEADERS.contains(key) { - client_req - } else { - // TODO: remove as_str and as_bytes conversions after actix-web upgrades to http 1.0 - client_req.header(key.as_str(), value.as_bytes()) - } - }) -} - pub async fn upload_image( req: HttpRequest, body: Payload, @@ -114,40 +35,9 @@ pub async fn upload_image( local_user_view: LocalUserView, context: Data, ) -> LemmyResult { - let pictrs_config = context.settings().pictrs_config()?; - let image_url = format!("{}image", pictrs_config.url); - - let mut client_req = adapt_request(&req, &context, image_url); - - if let Some(addr) = req.head().peer_addr { - client_req = client_req.header("X-Forwarded-For", addr.to_string()) - }; - let res = client_req - .timeout(Duration::from_secs(pictrs_config.upload_timeout)) - .body(Body::wrap_stream(make_send(body))) - .send() - .await?; - - let status = res.status(); - let images = res.json::().await?; - if let Some(images) = &images.files { - for image in images { - let form = LocalImageForm { - local_user_id: Some(local_user_view.local_user.id), - pictrs_alias: image.file.to_string(), - pictrs_delete_token: image.delete_token.to_string(), - }; - - let protocol_and_hostname = context.settings().get_protocol_and_hostname(); - let thumbnail_url = image.thumbnail_url(&protocol_and_hostname)?; + let image = do_upload_image(req, body, UploadType::Other, &local_user_view, &context).await?; - // Also store the details for the image - let details_form = image.details.build_image_details_form(&thumbnail_url); - LocalImage::create(&mut context.pool(), &form, &details_form).await?; - } - } - - Ok(HttpResponse::build(convert_status(status)).json(images)) + Ok(HttpResponse::Ok().json(image)) } pub async fn get_full_res_image( @@ -169,11 +59,11 @@ pub async fn get_full_res_image( let processed_url = params.process_url(name, &pictrs_config.url); - image(processed_url, req, &context).await + image(processed_url, req).await } -async fn image(url: String, req: HttpRequest, context: &LemmyContext) -> LemmyResult { - let mut client_req = adapt_request(&req, context, url); +async fn image(url: String, req: HttpRequest) -> LemmyResult { + let mut client_req = adapt_request(&req, url); if let Some(addr) = req.head().peer_addr { client_req = client_req.header("X-Forwarded-For", addr.to_string()); @@ -198,47 +88,66 @@ async fn image(url: String, req: HttpRequest, context: &LemmyContext) -> LemmyRe Ok(client_res.body(BodyStream::new(res.bytes_stream()))) } +#[derive(Deserialize, Clone)] +pub struct DeleteImageParams { + file: String, + token: String, +} + pub async fn delete_image( - components: Path<(String, String)>, - req: HttpRequest, + data: Json, context: Data, // require login _local_user_view: LocalUserView, -) -> LemmyResult { - let (token, file) = components.into_inner(); - +) -> LemmyResult { let pictrs_config = context.settings().pictrs_config()?; - let url = format!("{}image/delete/{}/{}", pictrs_config.url, &token, &file); - - let mut client_req = adapt_request(&req, &context, url); - - if let Some(addr) = req.head().peer_addr { - client_req = client_req.header("X-Forwarded-For", addr.to_string()); - } + let url = format!( + "{}image/delete/{}/{}", + pictrs_config.url, &data.token, &data.file + ); - let res = client_req.send().await?; + PICTRS_CLIENT.delete(url).send().await?.error_for_status()?; - LocalImage::delete_by_alias(&mut context.pool(), &file).await?; + LocalImage::delete_by_alias(&mut context.pool(), &data.file).await?; - Ok(HttpResponse::build(convert_status(res.status())).body(BodyStream::new(res.bytes_stream()))) + Ok(SuccessResponse::default()) } -pub async fn pictrs_healthz( - req: HttpRequest, - context: Data, -) -> LemmyResult { +pub async fn pictrs_healthz(context: Data) -> LemmyResult { let pictrs_config = context.settings().pictrs_config()?; let url = format!("{}healthz", pictrs_config.url); - let mut client_req = adapt_request(&req, &context, url); + PICTRS_CLIENT.get(url).send().await?.error_for_status()?; - if let Some(addr) = req.head().peer_addr { - client_req = client_req.header("X-Forwarded-For", addr.to_string()); - } + Ok(SuccessResponse::default()) +} - let res = client_req.send().await?; +#[derive(Deserialize, Clone)] +pub struct ImageProxyParams { + url: String, + format: Option, + thumbnail: Option, +} - Ok(HttpResponse::build(convert_status(res.status())).body(BodyStream::new(res.bytes_stream()))) +impl ProcessUrl for ImageProxyParams { + fn process_url(&self, proxy_url: &str, pictrs_url: &Url) -> String { + if self.format.is_none() && self.thumbnail.is_none() { + format!("{}image/original?proxy={}", pictrs_url, proxy_url) + } else { + // Take file type from name, or jpg if nothing is given + let format = self + .clone() + .format + .unwrap_or_else(|| proxy_url.split('.').last().unwrap_or("jpg").to_string()); + + let mut url = format!("{}image/process.{}?proxy={}", pictrs_url, format, proxy_url); + + if let Some(size) = self.thumbnail { + url = format!("{url}&thumbnail={size}",); + } + url + } + } } pub async fn image_proxy( @@ -264,6 +173,6 @@ pub async fn image_proxy( Ok(Either::Left(Redirect::to(url.to_string()).respond_to(&req))) } else { // Proxy the image data through Lemmy - Ok(Either::Right(image(processed_url, req, &context).await?)) + Ok(Either::Right(image(processed_url, req).await?)) } } diff --git a/crates/routes/src/images/person.rs b/crates/routes/src/images/person.rs new file mode 100644 index 0000000000..edac1e41b2 --- /dev/null +++ b/crates/routes/src/images/person.rs @@ -0,0 +1,36 @@ +use super::utils::{delete_old_image, do_upload_image, UploadType}; +use actix_web::{self, web::*, HttpRequest}; +use lemmy_api_common::{context::LemmyContext, SuccessResponse}; +use lemmy_db_schema::{ + source::person::{Person, PersonUpdateForm}, + traits::Crud, +}; +use lemmy_db_views::structs::LocalUserView; +use lemmy_utils::error::LemmyResult; +use url::Url; + +pub async fn upload_avatar( + req: HttpRequest, + body: Payload, + local_user_view: LocalUserView, + context: Data, +) -> LemmyResult> { + let image = do_upload_image(req, body, UploadType::Avatar, &local_user_view, &context).await?; + + delete_old_image(&local_user_view.person.avatar, &context).await?; + + let avatar = format!( + "{}/api/v4/image/{}", + context.settings().get_protocol_and_hostname(), + image.file + ); + let avatar = Some(Some(Url::parse(&avatar)?.into())); + let person_form = PersonUpdateForm { + avatar, + ..Default::default() + }; + + Person::update(&mut context.pool(), local_user_view.person.id, &person_form).await?; + + Ok(Json(SuccessResponse::default())) +} diff --git a/crates/routes/src/images/utils.rs b/crates/routes/src/images/utils.rs index c951b4b014..28d144a99b 100644 --- a/crates/routes/src/images/utils.rs +++ b/crates/routes/src/images/utils.rs @@ -1,6 +1,97 @@ -use actix_web::http::{Method, StatusCode}; +use actix_web::{ + http::{ + header::{HeaderName, ACCEPT_ENCODING, HOST}, + Method, + StatusCode, + }, + web::{Data, Payload}, + HttpRequest, +}; use futures::stream::{Stream, StreamExt}; use http::HeaderValue; +use lemmy_api_common::{ + context::LemmyContext, + request::{client_builder, delete_image_from_pictrs, PictrsFile, PictrsResponse}, + LemmyErrorType, +}; +use lemmy_db_schema::{ + newtypes::DbUrl, + source::images::{LocalImage, LocalImageForm}, +}; +use lemmy_db_views::structs::LocalUserView; +use lemmy_utils::{error::LemmyResult, settings::SETTINGS, REQWEST_TIMEOUT}; +use reqwest::Body; +use reqwest_middleware::{ClientBuilder, ClientWithMiddleware, RequestBuilder}; +use reqwest_tracing::TracingMiddleware; +use serde::Deserialize; +use std::{sync::LazyLock, time::Duration}; +use url::Url; + +// Pictrs cannot use proxy +pub(super) static PICTRS_CLIENT: LazyLock = LazyLock::new(|| { + ClientBuilder::new( + client_builder(&SETTINGS) + .no_proxy() + .build() + .expect("build pictrs client"), + ) + .with(TracingMiddleware::default()) + .build() +}); + +#[derive(Deserialize, Clone)] +pub struct PictrsGetParams { + format: Option, + thumbnail: Option, +} + +pub(super) trait ProcessUrl { + /// If thumbnail or format is given, this uses the pictrs process endpoint. + /// Otherwise, it uses the normal pictrs url (IE image/original). + fn process_url(&self, image_url: &str, pictrs_url: &Url) -> String; +} + +impl ProcessUrl for PictrsGetParams { + fn process_url(&self, src: &str, pictrs_url: &Url) -> String { + if self.format.is_none() && self.thumbnail.is_none() { + format!("{}image/original/{}", pictrs_url, src) + } else { + // Take file type from name, or jpg if nothing is given + let format = self + .clone() + .format + .unwrap_or_else(|| src.split('.').last().unwrap_or("jpg").to_string()); + + let mut url = format!("{}image/process.{}?src={}", pictrs_url, format, src); + + if let Some(size) = self.thumbnail { + url = format!("{url}&thumbnail={size}",); + } + url + } + } +} + +pub(super) fn adapt_request(request: &HttpRequest, url: String) -> RequestBuilder { + // remove accept-encoding header so that pictrs doesn't compress the response + const INVALID_HEADERS: &[HeaderName] = &[ACCEPT_ENCODING, HOST]; + + let client_request = PICTRS_CLIENT + .request(convert_method(request.method()), url) + .timeout(REQWEST_TIMEOUT); + + request + .headers() + .iter() + .fold(client_request, |client_req, (key, value)| { + if INVALID_HEADERS.contains(key) { + client_req + } else { + // TODO: remove as_str and as_bytes conversions after actix-web upgrades to http 1.0 + client_req.header(key.as_str(), value.as_bytes()) + } + }) +} pub(super) fn make_send(mut stream: S) -> impl Stream + Send + Unpin + 'static where @@ -45,11 +136,6 @@ where } // TODO: remove these conversions after actix-web upgrades to http 1.0 -#[allow(clippy::expect_used)] -pub(super) fn convert_status(status: http::StatusCode) -> StatusCode { - StatusCode::from_u16(status.as_u16()).expect("status can be converted") -} - #[allow(clippy::expect_used)] pub(super) fn convert_method(method: &Method) -> http::Method { http::Method::from_bytes(method.as_str().as_bytes()).expect("method can be converted") @@ -61,3 +147,88 @@ pub(super) fn convert_header<'a>( ) -> (&'a str, &'a [u8]) { (name.as_str(), value.as_bytes()) } + +pub(super) enum UploadType { + Avatar, + Other, +} + +pub(super) async fn do_upload_image( + req: HttpRequest, + body: Payload, + upload_type: UploadType, + local_user_view: &LocalUserView, + context: &Data, +) -> LemmyResult { + let pictrs_config = context.settings().pictrs_config()?; + let image_url = format!("{}image", pictrs_config.url); + + let mut client_req = adapt_request(&req, image_url); + + client_req = match upload_type { + UploadType::Avatar => { + let max_size = context + .settings() + .pictrs_config()? + .max_thumbnail_size + .to_string(); + client_req.query(&[ + ("max_width", max_size.as_ref()), + ("max_height", max_size.as_ref()), + ("allow_animation", "false"), + ("allow_video", "false"), + ]) + } + _ => client_req, + }; + if let Some(addr) = req.head().peer_addr { + client_req = client_req.header("X-Forwarded-For", addr.to_string()) + }; + let res = client_req + .timeout(Duration::from_secs(pictrs_config.upload_timeout)) + .body(Body::wrap_stream(make_send(body))) + .send() + .await? + .error_for_status()?; + + let mut images = res.json::().await?; + for image in &images.files { + // Pictrs allows uploading multiple images in a single request. Lemmy doesnt need this, + // but still a user may upload multiple and so we need to store all links in db for + // to allow deletion via web ui. + let form = LocalImageForm { + local_user_id: Some(local_user_view.local_user.id), + pictrs_alias: image.file.to_string(), + pictrs_delete_token: image.delete_token.to_string(), + }; + + let protocol_and_hostname = context.settings().get_protocol_and_hostname(); + let thumbnail_url = image.thumbnail_url(&protocol_and_hostname)?; + + // Also store the details for the image + let details_form = image.details.build_image_details_form(&thumbnail_url); + LocalImage::create(&mut context.pool(), &form, &details_form).await?; + } + let image = images + .files + .pop() + .ok_or(LemmyErrorType::InvalidImageUpload)?; + + Ok(image) +} + +/// When adding a new avatar, banner or similar image, delete the old one. +pub(super) async fn delete_old_image( + old_image: &Option, + context: &Data, +) -> LemmyResult<()> { + if let Some(old_image) = old_image { + let image = LocalImage::delete_by_url(&mut context.pool(), old_image) + .await + .ok(); + if let Some(image) = image { + delete_image_from_pictrs(&image.pictrs_alias, &image.pictrs_delete_token, context).await?; + } + } + Ok(()) +} diff --git a/crates/utils/src/error.rs b/crates/utils/src/error.rs index f45bc271f6..d2605608e2 100644 --- a/crates/utils/src/error.rs +++ b/crates/utils/src/error.rs @@ -23,7 +23,6 @@ pub enum LemmyErrorType { CouldntUpdateComment, CouldntUpdatePrivateMessage, CannotLeaveAdmin, - // TODO: also remove the translations of unused errors PictrsResponseError(String), PictrsPurgeResponseError(String), ImageUrlMissingPathSegments, @@ -31,6 +30,7 @@ pub enum LemmyErrorType { PictrsApiKeyNotProvided, NoContentTypeHeader, NotAnImageType, + InvalidImageUpload, NotAModOrAdmin, NotTopMod, NotLoggedIn, diff --git a/src/api_routes_v3.rs b/src/api_routes_v3.rs index 452271dd1f..56e3721d7c 100644 --- a/src/api_routes_v3.rs +++ b/src/api_routes_v3.rs @@ -134,7 +134,13 @@ use lemmy_apub::api::{ search::search, user_settings_backup::{export_settings, import_settings}, }; -use lemmy_routes::images::{delete_image, get_full_res_image, image_proxy, pictrs_healthz, upload_image}; +use lemmy_routes::images::{ + delete_image, + get_full_res_image, + image_proxy, + pictrs_healthz, + upload_image, +}; use lemmy_utils::rate_limit::RateLimitCell; // Deprecated, use api v4 instead. diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index 48f4031d4b..e67c6d7d4f 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -160,11 +160,7 @@ use lemmy_apub::api::{ user_settings_backup::{export_settings, import_settings}, }; use lemmy_routes::images::{ - delete_image, - get_full_res_image, - image_proxy, - pictrs_healthz, - upload_image, + delete_image, get_full_res_image, image_proxy, person::upload_avatar, pictrs_healthz, upload_image }; use lemmy_utils::rate_limit::RateLimitCell; @@ -293,7 +289,8 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route("/change_password", put().to(change_password)) .route("/totp/generate", post().to(generate_totp_secret)) .route("/totp/update", post().to(update_totp)) - .route("/verify_email", post().to(verify_email)), + .route("/verify_email", post().to(verify_email)) + .route("/avatar", post().to(upload_avatar)), ) .route("/account/settings/save", put().to(save_user_settings)) .service( @@ -401,7 +398,8 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route(post().to(upload_image)), ) .route("/proxy", get().to(image_proxy)) - .route("/{filename}", get().to(get_full_res_image)) + .route("/image/{filename}", get().to(get_full_res_image)) + // TODO: params are a bit strange like this .route("{token}/{filename}", delete().to(delete_image)) .route("/healthz", get().to(pictrs_healthz)), ), diff --git a/src/lib.rs b/src/lib.rs index 5586b61592..c287487755 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -285,17 +285,12 @@ fn create_http_server( .build() .map_err(|e| LemmyErrorType::Unknown(format!("Should always be buildable: {e}")))?; - let context: LemmyContext = federation_config.deref().clone(); - let rate_limit_cell = federation_config.rate_limit_cell().clone(); - - // Pictrs cannot use proxy - let pictrs_client = ClientBuilder::new(client_builder(&SETTINGS).no_proxy().build()?) - .with(TracingMiddleware::default()) - .build(); - // Create Http server let bind = (settings.bind, settings.port); let server = HttpServer::new(move || { + let context: LemmyContext = federation_config.deref().clone(); + let rate_limit_cell = federation_config.rate_limit_cell().clone(); + let cors_config = cors_config(&settings); let app = App::new() .wrap(middleware::Logger::new( @@ -328,7 +323,6 @@ fn create_http_server( } }) .configure(feeds::config) - .configure(|cfg| images::config(cfg, pictrs_client.clone(), &rate_limit_cell)) .configure(nodeinfo::config) }) .disable_signals() From d252be2113d465b09ab10622a860f4d9ab622b04 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 13 Dec 2024 15:04:07 +0100 Subject: [PATCH 04/51] Various other changes fixes #1772 fixes #4001 --- api_tests/package.json | 2 +- api_tests/pnpm-lock.yaml | 10 +- api_tests/run-federation-test.sh | 2 +- api_tests/src/image.spec.ts | 84 +++++++-------- config/defaults.hjson | 2 +- crates/api_common/src/image.rs | 42 ++++++++ crates/api_common/src/lib.rs | 1 + crates/api_common/src/request.rs | 12 ++- crates/api_common/src/utils.rs | 4 +- crates/routes/src/images/mod.rs | 142 +++++++++---------------- crates/routes/src/images/utils.rs | 78 +++++++------- crates/utils/src/settings/structs.rs | 2 +- crates/utils/src/utils/markdown/mod.rs | 10 +- src/api_routes_v3.rs | 12 +-- src/api_routes_v4.rs | 20 ++-- src/lib.rs | 2 +- 16 files changed, 208 insertions(+), 217 deletions(-) create mode 100644 crates/api_common/src/image.rs diff --git a/api_tests/package.json b/api_tests/package.json index 7ea21d0ba0..5ace77ecfe 100644 --- a/api_tests/package.json +++ b/api_tests/package.json @@ -28,7 +28,7 @@ "eslint": "^9.14.0", "eslint-plugin-prettier": "^5.1.3", "jest": "^29.5.0", - "lemmy-js-client": "0.20.0-api-v4.16", + "lemmy-js-client": "0.20.0-image-api-rework.3", "prettier": "^3.2.5", "ts-jest": "^29.1.0", "typescript": "^5.5.4", diff --git a/api_tests/pnpm-lock.yaml b/api_tests/pnpm-lock.yaml index 496606e6c2..615a0ceca2 100644 --- a/api_tests/pnpm-lock.yaml +++ b/api_tests/pnpm-lock.yaml @@ -30,8 +30,8 @@ importers: specifier: ^29.5.0 version: 29.7.0(@types/node@22.9.0) lemmy-js-client: - specifier: 0.20.0-api-v4.16 - version: 0.20.0-api-v4.16 + specifier: 0.20.0-image-api-rework.3 + version: 0.20.0-image-api-rework.3 prettier: specifier: ^3.2.5 version: 3.3.3 @@ -1167,8 +1167,8 @@ packages: resolution: {integrity: sha512-eTIzlVOSUR+JxdDFepEYcBMtZ9Qqdef+rnzWdRZuMbOywu5tO2w2N7rqjoANZ5k9vywhL6Br1VRjUIgTQx4E8w==} engines: {node: '>=6'} - lemmy-js-client@0.20.0-api-v4.16: - resolution: {integrity: sha512-9Wn7b8YT2KnEA286+RV1B3mLmecAynvAERoC0ZZiccfSgkEvd3rG9A5X9ejiPqp+JzDZJeisO57+Ut4QHr5oTw==} + lemmy-js-client@0.20.0-image-api-rework.3: + resolution: {integrity: sha512-SB20z+WD2S821q05OxzI2Lkwq1BWBNWM6Xd1l1bqKL310CRSAG4lln26+j8bjWxMgl/fYTqre8KG6l1YDiV3+Q==} leven@3.1.0: resolution: {integrity: sha512-qsda+H8jTaUaN/x5vzW2rzc+8Rw4TAQ/4KjB46IwK5VH+IlVeeeje/EoZRpiXvIqjFgK84QffqPztGI3VBLG1A==} @@ -3077,7 +3077,7 @@ snapshots: kleur@3.0.3: {} - lemmy-js-client@0.20.0-api-v4.16: {} + lemmy-js-client@0.20.0-image-api-rework.3: {} leven@3.1.0: {} diff --git a/api_tests/run-federation-test.sh b/api_tests/run-federation-test.sh index f9eab50395..969a95b3e7 100755 --- a/api_tests/run-federation-test.sh +++ b/api_tests/run-federation-test.sh @@ -11,7 +11,7 @@ killall -s1 lemmy_server || true popd pnpm i -pnpm api-test-image || true +pnpm api-test || true killall -s1 lemmy_server || true killall -s1 pict-rs || true diff --git a/api_tests/src/image.spec.ts b/api_tests/src/image.spec.ts index a3478081ad..fa22f271d1 100644 --- a/api_tests/src/image.spec.ts +++ b/api_tests/src/image.spec.ts @@ -2,9 +2,9 @@ jest.setTimeout(120000); import { UploadImage, - DeleteImage, PurgePerson, PurgePost, + DeleteImageParams, } from "lemmy-js-client"; import { alpha, @@ -54,13 +54,12 @@ test("Upload image and delete it", async () => { image: Buffer.from("test"), }; const upload = await alphaImage.uploadImage(upload_form); - expect(upload.files![0].file).toBeDefined(); - expect(upload.files![0].delete_token).toBeDefined(); - expect(upload.url).toBeDefined(); - expect(upload.delete_url).toBeDefined(); + expect(upload.image_url).toBeDefined(); + expect(upload.filename).toBeDefined(); + expect(upload.delete_token).toBeDefined(); // ensure that image download is working. theres probably a better way to do this - const response = await fetch(upload.url ?? ""); + const response = await fetch(upload.image_url ?? ""); const content = await response.text(); expect(content.length).toBeGreaterThan(0); @@ -77,26 +76,21 @@ test("Upload image and delete it", async () => { const previousThumbnails = 1; expect(listAllMediaRes.images.length).toBe(previousThumbnails); - // The deleteUrl is a combination of the endpoint, delete token, and alias - let firstImage = listMediaRes.images[0]; - let deleteUrl = `${alphaUrl}/pictrs/image/delete/${firstImage.local_image.pictrs_delete_token}/${firstImage.local_image.pictrs_alias}`; - expect(deleteUrl).toBe(upload.delete_url); - // Make sure the uploader is correct - expect(firstImage.person.actor_id).toBe( + expect(listMediaRes.images[0].person.actor_id).toBe( `http://lemmy-alpha:8541/u/lemmy_alpha`, ); // delete image - const delete_form: DeleteImage = { - token: upload.files![0].delete_token, - filename: upload.files![0].file, + const delete_form: DeleteImageParams = { + token: upload.delete_token, + filename: upload.filename, }; const delete_ = await alphaImage.deleteImage(delete_form); - expect(delete_).toBe(true); + expect(delete_.success).toBe(true); // ensure that image is deleted - const response2 = await fetch(upload.url ?? ""); + const response2 = await fetch(upload.image_url ?? ""); const content2 = await response2.text(); expect(content2).toBe(""); @@ -119,13 +113,12 @@ test("Purge user, uploaded image removed", async () => { image: Buffer.from("test"), }; const upload = await user.uploadImage(upload_form); - expect(upload.files![0].file).toBeDefined(); - expect(upload.files![0].delete_token).toBeDefined(); - expect(upload.url).toBeDefined(); - expect(upload.delete_url).toBeDefined(); + expect(upload.filename).toBeDefined(); + expect(upload.delete_token).toBeDefined(); + expect(upload.image_url).toBeDefined(); // ensure that image download is working. theres probably a better way to do this - const response = await fetch(upload.url ?? ""); + const response = await fetch(upload.image_url ?? ""); const content = await response.text(); expect(content.length).toBeGreaterThan(0); @@ -138,7 +131,7 @@ test("Purge user, uploaded image removed", async () => { expect(delete_.success).toBe(true); // ensure that image is deleted - const response2 = await fetch(upload.url ?? ""); + const response2 = await fetch(upload.image_url ?? ""); const content2 = await response2.text(); expect(content2).toBe(""); }); @@ -151,13 +144,12 @@ test("Purge post, linked image removed", async () => { image: Buffer.from("test"), }; const upload = await user.uploadImage(upload_form); - expect(upload.files![0].file).toBeDefined(); - expect(upload.files![0].delete_token).toBeDefined(); - expect(upload.url).toBeDefined(); - expect(upload.delete_url).toBeDefined(); + expect(upload.filename).toBeDefined(); + expect(upload.delete_token).toBeDefined(); + expect(upload.image_url).toBeDefined(); // ensure that image download is working. theres probably a better way to do this - const response = await fetch(upload.url ?? ""); + const response = await fetch(upload.image_url ?? ""); const content = await response.text(); expect(content.length).toBeGreaterThan(0); @@ -165,9 +157,9 @@ test("Purge post, linked image removed", async () => { let post = await createPost( user, community.community!.community.id, - upload.url, + upload.image_url, ); - expect(post.post_view.post.url).toBe(upload.url); + expect(post.post_view.post.url).toBe(upload.image_url); expect(post.post_view.image_details).toBeDefined(); // purge post @@ -178,7 +170,7 @@ test("Purge post, linked image removed", async () => { expect(delete_.success).toBe(true); // ensure that image is deleted - const response2 = await fetch(upload.url ?? ""); + const response2 = await fetch(upload.image_url ?? ""); const content2 = await response2.text(); expect(content2).toBe(""); }); @@ -200,11 +192,11 @@ test("Images in remote image post are proxied if setting enabled", async () => { // remote image gets proxied after upload expect( post.thumbnail_url?.startsWith( - "http://lemmy-gamma:8561/api/v4/image_proxy?url", + "http://lemmy-gamma:8561/api/v4/image/proxy?url", ), ).toBeTruthy(); expect( - post.body?.startsWith("![](http://lemmy-gamma:8561/api/v4/image_proxy?url"), + post.body?.startsWith("![](http://lemmy-gamma:8561/api/v4/image/proxy?url"), ).toBeTruthy(); // Make sure that it ends with jpg, to be sure its an image @@ -223,12 +215,12 @@ test("Images in remote image post are proxied if setting enabled", async () => { expect( epsilonPost.thumbnail_url?.startsWith( - "http://lemmy-epsilon:8581/api/v4/image_proxy?url", + "http://lemmy-epsilon:8581/api/v4/image/proxy?url", ), ).toBeTruthy(); expect( epsilonPost.body?.startsWith( - "![](http://lemmy-epsilon:8581/api/v4/image_proxy?url", + "![](http://lemmy-epsilon:8581/api/v4/image/proxy?url", ), ).toBeTruthy(); @@ -250,7 +242,7 @@ test("Thumbnail of remote image link is proxied if setting enabled", async () => // remote image gets proxied after upload expect( post.thumbnail_url?.startsWith( - "http://lemmy-gamma:8561/api/v4/image_proxy?url", + "http://lemmy-gamma:8561/api/v4/image/proxy?url", ), ).toBeTruthy(); @@ -268,7 +260,7 @@ test("Thumbnail of remote image link is proxied if setting enabled", async () => expect( epsilonPost.thumbnail_url?.startsWith( - "http://lemmy-epsilon:8581/api/v4/image_proxy?url", + "http://lemmy-epsilon:8581/api/v4/image/proxy?url", ), ).toBeTruthy(); @@ -292,14 +284,14 @@ test("No image proxying if setting is disabled", async () => { let post = await createPost( alpha, community.community_view.community.id, - upload.url, + upload.image_url, `![](${sampleImage})`, ); expect(post.post_view.post).toBeDefined(); // remote image doesn't get proxied after upload expect( - post.post_view.post.url?.startsWith("http://127.0.0.1:8551/pictrs/image/"), + post.post_view.post.url?.startsWith("http://lemmy-beta:8551/api/v4/image/"), ).toBeTruthy(); expect(post.post_view.post.body).toBe(`![](${sampleImage})`); @@ -312,7 +304,7 @@ test("No image proxying if setting is disabled", async () => { // remote image doesn't get proxied after federation expect( - betaPost.post.url?.startsWith("http://127.0.0.1:8551/pictrs/image/"), + betaPost.post.url?.startsWith("http://lemmy-beta:8551/api/v4/image/"), ).toBeTruthy(); expect(betaPost.post.body).toBe(`![](${sampleImage})`); // Make sure the alt text got federated @@ -334,7 +326,7 @@ test("Make regular post, and give it a custom thumbnail", async () => { alphaImage, community.community_view.community.id, wikipediaUrl, - upload1.url!, + upload1.image_url!, ); // Wait for the metadata to get fetched, since this is backgrounded now @@ -344,7 +336,7 @@ test("Make regular post, and give it a custom thumbnail", async () => { ); expect(post.post_view.post.url).toBe(wikipediaUrl); // Make sure it uses custom thumbnail - expect(post.post_view.post.thumbnail_url).toBe(upload1.url); + expect(post.post_view.post.thumbnail_url).toBe(upload1.image_url); }); test("Create an image post, and make sure a custom thumbnail doesn't overwrite it", async () => { @@ -363,14 +355,14 @@ test("Create an image post, and make sure a custom thumbnail doesn't overwrite i let post = await createPostWithThumbnail( alphaImage, community.community_view.community.id, - upload1.url!, - upload2.url!, + upload1.image_url!, + upload2.image_url!, ); post = await waitUntil( () => getPost(alphaImage, post.post_view.post.id), p => p.post_view.post.thumbnail_url != undefined, ); - expect(post.post_view.post.url).toBe(upload1.url); + expect(post.post_view.post.url).toBe(upload1.image_url); // Make sure the custom thumbnail is ignored - expect(post.post_view.post.thumbnail_url == upload2.url).toBe(false); + expect(post.post_view.post.thumbnail_url == upload2.image_url).toBe(false); }); diff --git a/config/defaults.hjson b/config/defaults.hjson index 9e24407cd0..14749e542e 100644 --- a/config/defaults.hjson +++ b/config/defaults.hjson @@ -44,7 +44,7 @@ # or # If enabled, all images from remote domains are rewritten to pass through - # `/api/v4/image_proxy`, including embedded images in markdown. Images are stored temporarily + # `/api/v4/image/proxy`, including embedded images in markdown. Images are stored temporarily # in pict-rs for caching. This improves privacy as users don't expose their IP to untrusted # servers, and decreases load on other servers. However it increases bandwidth use for the # local server. diff --git a/crates/api_common/src/image.rs b/crates/api_common/src/image.rs new file mode 100644 index 0000000000..df033f2858 --- /dev/null +++ b/crates/api_common/src/image.rs @@ -0,0 +1,42 @@ +use serde::{Deserialize, Serialize}; +use serde_with::skip_serializing_none; +#[cfg(feature = "full")] +use ts_rs::TS; +use url::Url; + +#[skip_serializing_none] +#[derive(Debug, Serialize, Deserialize, Clone, Default, PartialEq, Eq, Hash)] +#[cfg_attr(feature = "full", derive(TS))] +#[cfg_attr(feature = "full", ts(export))] +pub struct ImageGetParams { + pub file_type: Option, + pub max_size: Option, +} + +#[derive(Debug, Serialize, Deserialize, Clone, Default, PartialEq, Eq, Hash)] +#[cfg_attr(feature = "full", derive(TS))] +#[cfg_attr(feature = "full", ts(export))] +pub struct DeleteImageParams { + pub filename: String, + pub token: String, +} + +#[skip_serializing_none] +#[derive(Debug, Serialize, Deserialize, Clone, Default, PartialEq, Eq, Hash)] +#[cfg_attr(feature = "full", derive(TS))] +#[cfg_attr(feature = "full", ts(export))] +pub struct ImageProxyParams { + pub url: String, + pub file_type: Option, + pub max_size: Option, +} + +#[skip_serializing_none] +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, Hash)] +#[cfg_attr(feature = "full", derive(TS))] +#[cfg_attr(feature = "full", ts(export))] +pub struct UploadImageResponse { + pub image_url: Url, + pub filename: String, + pub delete_token: String, +} diff --git a/crates/api_common/src/lib.rs b/crates/api_common/src/lib.rs index 6e09d904d6..e02df464ad 100644 --- a/crates/api_common/src/lib.rs +++ b/crates/api_common/src/lib.rs @@ -7,6 +7,7 @@ pub mod community; #[cfg(feature = "full")] pub mod context; pub mod custom_emoji; +pub mod image; pub mod oauth_provider; pub mod person; pub mod post; diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index 5b4d85a5e8..e337cbdec9 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -263,9 +263,15 @@ pub struct PictrsFile { } impl PictrsFile { - pub fn thumbnail_url(&self, protocol_and_hostname: &str) -> Result { + pub fn image_url(&self, protocol_and_hostname: &str) -> Result { Url::parse(&format!( - "{protocol_and_hostname}/pictrs/image/{}", + "{protocol_and_hostname}/api/v4/image/{}", + self.file + )) + } + pub fn delete_url(&self, protocol_and_hostname: &str) -> Result { + Url::parse(&format!( + "{protocol_and_hostname}/api/v4/image/{}", self.file )) } @@ -402,7 +408,7 @@ async fn generate_pictrs_thumbnail(image_url: &Url, context: &LemmyContext) -> L pictrs_delete_token: image.delete_token.clone(), }; let protocol_and_hostname = context.settings().get_protocol_and_hostname(); - let thumbnail_url = image.thumbnail_url(&protocol_and_hostname)?; + let thumbnail_url = image.image_url(&protocol_and_hostname)?; // Also store the details for the image let details_form = image.details.build_image_details_form(&thumbnail_url); diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 21154c823c..6daeec4cd1 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -1177,7 +1177,7 @@ fn build_proxied_image_url( protocol_and_hostname: &str, ) -> Result { Url::parse(&format!( - "{}/api/v4/image_proxy?url={}", + "{}/api/v4/image/proxy?url={}", protocol_and_hostname, encode(link.as_str()) )) @@ -1256,7 +1256,7 @@ mod tests { ) .await?; assert_eq!( - "https://lemmy-alpha/api/v4/image_proxy?url=http%3A%2F%2Flemmy-beta%2Fimage.png", + "https://lemmy-alpha/api/v4/image/proxy?url=http%3A%2F%2Flemmy-beta%2Fimage.png", proxied.as_str() ); diff --git a/crates/routes/src/images/mod.rs b/crates/routes/src/images/mod.rs index c5f6a9c2c5..632aa30f20 100644 --- a/crates/routes/src/images/mod.rs +++ b/crates/routes/src/images/mod.rs @@ -1,29 +1,17 @@ -use actix_web::{ - body::{BodyStream, BoxBody}, - http::StatusCode, - web::*, - HttpRequest, - HttpResponse, - Responder, +use actix_web::{body::BoxBody, web::*, HttpRequest, HttpResponse, Responder}; +use lemmy_api_common::{ + context::LemmyContext, + image::{DeleteImageParams, ImageGetParams, ImageProxyParams, UploadImageResponse}, + SuccessResponse, }; -use lemmy_api_common::{context::LemmyContext, SuccessResponse}; use lemmy_db_schema::source::{ images::{LocalImage, RemoteImage}, local_site::LocalSite, }; use lemmy_db_views::structs::LocalUserView; use lemmy_utils::error::LemmyResult; -use serde::Deserialize; use url::Url; -use utils::{ - adapt_request, - convert_header, - do_upload_image, - PictrsGetParams, - ProcessUrl, - UploadType, - PICTRS_CLIENT, -}; +use utils::{do_get_image, do_upload_image, file_type, UploadType, PICTRS_CLIENT}; pub mod person; mod utils; @@ -31,18 +19,22 @@ mod utils; pub async fn upload_image( req: HttpRequest, body: Payload, - // require login local_user_view: LocalUserView, context: Data, -) -> LemmyResult { +) -> LemmyResult> { let image = do_upload_image(req, body, UploadType::Other, &local_user_view, &context).await?; - Ok(HttpResponse::Ok().json(image)) + let image_url = image.image_url(&context.settings().get_protocol_and_hostname())?; + Ok(Json(UploadImageResponse { + image_url, + filename: image.file, + delete_token: image.delete_token, + })) } -pub async fn get_full_res_image( +pub async fn get_image( filename: Path, - Query(params): Query, + Query(params): Query, req: HttpRequest, context: Data, local_user_view: Option, @@ -55,43 +47,20 @@ pub async fn get_full_res_image( let name = &filename.into_inner(); // If there are no query params, the URL is original - let pictrs_config = context.settings().pictrs_config()?; - - let processed_url = params.process_url(name, &pictrs_config.url); - - image(processed_url, req).await -} - -async fn image(url: String, req: HttpRequest) -> LemmyResult { - let mut client_req = adapt_request(&req, url); - - if let Some(addr) = req.head().peer_addr { - client_req = client_req.header("X-Forwarded-For", addr.to_string()); - } - - if let Some(addr) = req.head().peer_addr { - client_req = client_req.header("X-Forwarded-For", addr.to_string()); - } - - let res = client_req.send().await?; - - if res.status() == http::StatusCode::NOT_FOUND { - return Ok(HttpResponse::NotFound().finish()); - } - - let mut client_res = HttpResponse::build(StatusCode::from_u16(res.status().as_u16())?); - - for (name, value) in res.headers().iter().filter(|(h, _)| *h != "connection") { - client_res.insert_header(convert_header(name, value)); - } + let pictrs_url = context.settings().pictrs_config()?.url; + let processed_url = if params.file_type.is_none() && params.max_size.is_none() { + format!("{}image/original/{}", pictrs_url, name) + } else { + let file_type = file_type(params.file_type, name); + let mut url = format!("{}image/process.{}?src={}", pictrs_url, file_type, name); - Ok(client_res.body(BodyStream::new(res.bytes_stream()))) -} + if let Some(size) = params.max_size { + url = format!("{url}&thumbnail={size}",); + } + url + }; -#[derive(Deserialize, Clone)] -pub struct DeleteImageParams { - file: String, - token: String, + do_get_image(processed_url, req).await } pub async fn delete_image( @@ -99,55 +68,27 @@ pub async fn delete_image( context: Data, // require login _local_user_view: LocalUserView, -) -> LemmyResult { +) -> LemmyResult> { let pictrs_config = context.settings().pictrs_config()?; let url = format!( "{}image/delete/{}/{}", - pictrs_config.url, &data.token, &data.file + pictrs_config.url, &data.token, &data.filename ); PICTRS_CLIENT.delete(url).send().await?.error_for_status()?; - LocalImage::delete_by_alias(&mut context.pool(), &data.file).await?; + LocalImage::delete_by_alias(&mut context.pool(), &data.filename).await?; - Ok(SuccessResponse::default()) + Ok(Json(SuccessResponse::default())) } -pub async fn pictrs_healthz(context: Data) -> LemmyResult { +pub async fn pictrs_health(context: Data) -> LemmyResult> { let pictrs_config = context.settings().pictrs_config()?; let url = format!("{}healthz", pictrs_config.url); PICTRS_CLIENT.get(url).send().await?.error_for_status()?; - Ok(SuccessResponse::default()) -} - -#[derive(Deserialize, Clone)] -pub struct ImageProxyParams { - url: String, - format: Option, - thumbnail: Option, -} - -impl ProcessUrl for ImageProxyParams { - fn process_url(&self, proxy_url: &str, pictrs_url: &Url) -> String { - if self.format.is_none() && self.thumbnail.is_none() { - format!("{}image/original?proxy={}", pictrs_url, proxy_url) - } else { - // Take file type from name, or jpg if nothing is given - let format = self - .clone() - .format - .unwrap_or_else(|| proxy_url.split('.').last().unwrap_or("jpg").to_string()); - - let mut url = format!("{}image/process.{}?proxy={}", pictrs_url, format, proxy_url); - - if let Some(size) = self.thumbnail { - url = format!("{url}&thumbnail={size}",); - } - url - } - } + Ok(Json(SuccessResponse::default())) } pub async fn image_proxy( @@ -162,7 +103,20 @@ pub async fn image_proxy( RemoteImage::validate(&mut context.pool(), url.clone().into()).await?; let pictrs_config = context.settings().pictrs_config()?; - let processed_url = params.process_url(¶ms.url, &pictrs_config.url); + let processed_url = if params.file_type.is_none() && params.max_size.is_none() { + format!("{}image/original?proxy={}", pictrs_config.url, params.url) + } else { + let file_type = file_type(params.file_type, url.as_str()); + let mut url = format!( + "{}image/process.{}?proxy={}", + pictrs_config.url, file_type, url + ); + + if let Some(size) = params.max_size { + url = format!("{url}&thumbnail={size}",); + } + url + }; let bypass_proxy = pictrs_config .proxy_bypass_domains @@ -173,6 +127,6 @@ pub async fn image_proxy( Ok(Either::Left(Redirect::to(url.to_string()).respond_to(&req))) } else { // Proxy the image data through Lemmy - Ok(Either::Right(image(processed_url, req).await?)) + Ok(Either::Right(do_get_image(processed_url, req).await?)) } } diff --git a/crates/routes/src/images/utils.rs b/crates/routes/src/images/utils.rs index 28d144a99b..94a807f787 100644 --- a/crates/routes/src/images/utils.rs +++ b/crates/routes/src/images/utils.rs @@ -1,4 +1,5 @@ use actix_web::{ + body::BodyStream, http::{ header::{HeaderName, ACCEPT_ENCODING, HOST}, Method, @@ -6,6 +7,7 @@ use actix_web::{ }, web::{Data, Payload}, HttpRequest, + HttpResponse, }; use futures::stream::{Stream, StreamExt}; use http::HeaderValue; @@ -23,7 +25,6 @@ use lemmy_utils::{error::LemmyResult, settings::SETTINGS, REQWEST_TIMEOUT}; use reqwest::Body; use reqwest_middleware::{ClientBuilder, ClientWithMiddleware, RequestBuilder}; use reqwest_tracing::TracingMiddleware; -use serde::Deserialize; use std::{sync::LazyLock, time::Duration}; use url::Url; @@ -39,40 +40,7 @@ pub(super) static PICTRS_CLIENT: LazyLock = LazyLock::new( .build() }); -#[derive(Deserialize, Clone)] -pub struct PictrsGetParams { - format: Option, - thumbnail: Option, -} - -pub(super) trait ProcessUrl { - /// If thumbnail or format is given, this uses the pictrs process endpoint. - /// Otherwise, it uses the normal pictrs url (IE image/original). - fn process_url(&self, image_url: &str, pictrs_url: &Url) -> String; -} - -impl ProcessUrl for PictrsGetParams { - fn process_url(&self, src: &str, pictrs_url: &Url) -> String { - if self.format.is_none() && self.thumbnail.is_none() { - format!("{}image/original/{}", pictrs_url, src) - } else { - // Take file type from name, or jpg if nothing is given - let format = self - .clone() - .format - .unwrap_or_else(|| src.split('.').last().unwrap_or("jpg").to_string()); - - let mut url = format!("{}image/process.{}?src={}", pictrs_url, format, src); - - if let Some(size) = self.thumbnail { - url = format!("{url}&thumbnail={size}",); - } - url - } - } -} - -pub(super) fn adapt_request(request: &HttpRequest, url: String) -> RequestBuilder { +fn adapt_request(request: &HttpRequest, url: String) -> RequestBuilder { // remove accept-encoding header so that pictrs doesn't compress the response const INVALID_HEADERS: &[HeaderName] = &[ACCEPT_ENCODING, HOST]; @@ -141,10 +109,7 @@ pub(super) fn convert_method(method: &Method) -> http::Method { http::Method::from_bytes(method.as_str().as_bytes()).expect("method can be converted") } -pub(super) fn convert_header<'a>( - name: &'a http::HeaderName, - value: &'a HeaderValue, -) -> (&'a str, &'a [u8]) { +fn convert_header<'a>(name: &'a http::HeaderName, value: &'a HeaderValue) -> (&'a str, &'a [u8]) { (name.as_str(), value.as_bytes()) } @@ -203,7 +168,7 @@ pub(super) async fn do_upload_image( }; let protocol_and_hostname = context.settings().get_protocol_and_hostname(); - let thumbnail_url = image.thumbnail_url(&protocol_and_hostname)?; + let thumbnail_url = image.image_url(&protocol_and_hostname)?; // Also store the details for the image let details_form = image.details.build_image_details_form(&thumbnail_url); @@ -217,6 +182,32 @@ pub(super) async fn do_upload_image( Ok(image) } +pub(super) async fn do_get_image(url: String, req: HttpRequest) -> LemmyResult { + let mut client_req = adapt_request(&req, url); + + if let Some(addr) = req.head().peer_addr { + client_req = client_req.header("X-Forwarded-For", addr.to_string()); + } + + if let Some(addr) = req.head().peer_addr { + client_req = client_req.header("X-Forwarded-For", addr.to_string()); + } + + let res = client_req.send().await?; + + if res.status() == http::StatusCode::NOT_FOUND { + return Ok(HttpResponse::NotFound().finish()); + } + + let mut client_res = HttpResponse::build(StatusCode::from_u16(res.status().as_u16())?); + + for (name, value) in res.headers().iter().filter(|(h, _)| *h != "connection") { + client_res.insert_header(convert_header(name, value)); + } + + Ok(client_res.body(BodyStream::new(res.bytes_stream()))) +} + /// When adding a new avatar, banner or similar image, delete the old one. pub(super) async fn delete_old_image( old_image: &Option, @@ -232,3 +223,10 @@ pub(super) async fn delete_old_image( } Ok(()) } + +/// Take file type from param, name, or use jpg if nothing is given +pub(super) fn file_type(file_type: Option, name: &str) -> String { + file_type + .clone() + .unwrap_or_else(|| name.split('.').last().unwrap_or("jpg").to_string()) +} diff --git a/crates/utils/src/settings/structs.rs b/crates/utils/src/settings/structs.rs index 1aef9f79ba..9e561713c5 100644 --- a/crates/utils/src/settings/structs.rs +++ b/crates/utils/src/settings/structs.rs @@ -120,7 +120,7 @@ pub enum PictrsImageMode { #[default] StoreLinkPreviews, /// If enabled, all images from remote domains are rewritten to pass through - /// `/api/v4/image_proxy`, including embedded images in markdown. Images are stored temporarily + /// `/api/v4/image/proxy`, including embedded images in markdown. Images are stored temporarily /// in pict-rs for caching. This improves privacy as users don't expose their IP to untrusted /// servers, and decreases load on other servers. However it increases bandwidth use for the /// local server. diff --git a/crates/utils/src/utils/markdown/mod.rs b/crates/utils/src/utils/markdown/mod.rs index ba509596ef..94dd322641 100644 --- a/crates/utils/src/utils/markdown/mod.rs +++ b/crates/utils/src/utils/markdown/mod.rs @@ -141,7 +141,7 @@ mod tests { ( "remote image proxied", "![link](http://example.com/image.jpg)", - "![link](https://lemmy-alpha/api/v4/image_proxy?url=http%3A%2F%2Fexample.com%2Fimage.jpg)", + "![link](https://lemmy-alpha/api/v4/image/proxy?url=http%3A%2F%2Fexample.com%2Fimage.jpg)", ), ( "local image unproxied", @@ -151,7 +151,7 @@ mod tests { ( "multiple image links", "![link](http://example.com/image1.jpg) ![link](http://example.com/image2.jpg)", - "![link](https://lemmy-alpha/api/v4/image_proxy?url=http%3A%2F%2Fexample.com%2Fimage1.jpg) ![link](https://lemmy-alpha/api/v4/image_proxy?url=http%3A%2F%2Fexample.com%2Fimage2.jpg)", + "![link](https://lemmy-alpha/api/v4/image/proxy?url=http%3A%2F%2Fexample.com%2Fimage1.jpg) ![link](https://lemmy-alpha/api/v4/image/proxy?url=http%3A%2F%2Fexample.com%2Fimage2.jpg)", ), ( "empty link handled", @@ -161,7 +161,7 @@ mod tests { ( "empty label handled", "![](http://example.com/image.jpg)", - "![](https://lemmy-alpha/api/v4/image_proxy?url=http%3A%2F%2Fexample.com%2Fimage.jpg)" + "![](https://lemmy-alpha/api/v4/image/proxy?url=http%3A%2F%2Fexample.com%2Fimage.jpg)" ), ( "invalid image link removed", @@ -171,12 +171,12 @@ mod tests { ( "label with nested markdown handled", "![a *b* c](http://example.com/image.jpg)", - "![a *b* c](https://lemmy-alpha/api/v4/image_proxy?url=http%3A%2F%2Fexample.com%2Fimage.jpg)" + "![a *b* c](https://lemmy-alpha/api/v4/image/proxy?url=http%3A%2F%2Fexample.com%2Fimage.jpg)" ), ( "custom emoji support", r#"![party-blob](https://www.hexbear.net/pictrs/image/83405746-0620-4728-9358-5f51b040ffee.gif "emoji party-blob")"#, - r#"![party-blob](https://lemmy-alpha/api/v4/image_proxy?url=https%3A%2F%2Fwww.hexbear.net%2Fpictrs%2Fimage%2F83405746-0620-4728-9358-5f51b040ffee.gif "emoji party-blob")"# + r#"![party-blob](https://lemmy-alpha/api/v4/image/proxy?url=https%3A%2F%2Fwww.hexbear.net%2Fpictrs%2Fimage%2F83405746-0620-4728-9358-5f51b040ffee.gif "emoji party-blob")"# ) ]; diff --git a/src/api_routes_v3.rs b/src/api_routes_v3.rs index 56e3721d7c..e7cb828838 100644 --- a/src/api_routes_v3.rs +++ b/src/api_routes_v3.rs @@ -134,13 +134,7 @@ use lemmy_apub::api::{ search::search, user_settings_backup::{export_settings, import_settings}, }; -use lemmy_routes::images::{ - delete_image, - get_full_res_image, - image_proxy, - pictrs_healthz, - upload_image, -}; +use lemmy_routes::images::{delete_image, get_image, image_proxy, pictrs_health, upload_image}; use lemmy_utils::rate_limit::RateLimitCell; // Deprecated, use api v4 instead. @@ -153,9 +147,9 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .wrap(rate_limit.image()) .route(post().to(upload_image)), ) - .service(resource("/pictrs/image/{filename}").route(get().to(get_full_res_image))) + .service(resource("/pictrs/image/{filename}").route(get().to(get_image))) .service(resource("/pictrs/image/delete/{token}/{filename}").route(get().to(delete_image))) - .service(resource("/pictrs/healthz").route(get().to(pictrs_healthz))) + .service(resource("/pictrs/healthz").route(get().to(pictrs_health))) .service( scope("/api/v3") .route("/image_proxy", get().to(image_proxy)) diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index e67c6d7d4f..99ae86fbc6 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -160,7 +160,12 @@ use lemmy_apub::api::{ user_settings_backup::{export_settings, import_settings}, }; use lemmy_routes::images::{ - delete_image, get_full_res_image, image_proxy, person::upload_avatar, pictrs_healthz, upload_image + delete_image, + get_image, + image_proxy, + person::upload_avatar, + pictrs_health, + upload_image, }; use lemmy_utils::rate_limit::RateLimitCell; @@ -289,8 +294,7 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route("/change_password", put().to(change_password)) .route("/totp/generate", post().to(generate_totp_secret)) .route("/totp/update", post().to(update_totp)) - .route("/verify_email", post().to(verify_email)) - .route("/avatar", post().to(upload_avatar)), + .route("/verify_email", post().to(verify_email)), ) .route("/account/settings/save", put().to(save_user_settings)) .service( @@ -318,6 +322,7 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route("/unread_count", get().to(unread_count)) .route("/list_logins", get().to(list_logins)) .route("/validate_auth", get().to(validate_auth)) + .route("/avatar", post().to(upload_avatar)) .service( scope("/block") .route("/person", post().to(user_block_person)) @@ -395,13 +400,12 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .service( resource("") .wrap(rate_limit.image()) - .route(post().to(upload_image)), + .route(post().to(upload_image)) + .route(delete().to(delete_image)), ) .route("/proxy", get().to(image_proxy)) - .route("/image/{filename}", get().to(get_full_res_image)) - // TODO: params are a bit strange like this - .route("{token}/{filename}", delete().to(delete_image)) - .route("/healthz", get().to(pictrs_healthz)), + .route("/{filename}", get().to(get_image)) + .route("/health", get().to(pictrs_health)), ), ); } diff --git a/src/lib.rs b/src/lib.rs index c287487755..ae060ec75e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -36,7 +36,7 @@ use lemmy_apub::{ }; use lemmy_db_schema::{source::secret::Secret, utils::build_db_pool}; use lemmy_federate::{Opts, SendManager}; -use lemmy_routes::{feeds, images, nodeinfo, webfinger}; +use lemmy_routes::{feeds, nodeinfo, webfinger}; use lemmy_utils::{ error::{LemmyErrorType, LemmyResult}, rate_limit::RateLimitCell, From e8157789260a7ae65db83429c40c9442a720e551 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 13 Dec 2024 15:13:27 +0100 Subject: [PATCH 05/51] clippy --- crates/routes/src/images/utils.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/routes/src/images/utils.rs b/crates/routes/src/images/utils.rs index 94a807f787..dcc4188272 100644 --- a/crates/routes/src/images/utils.rs +++ b/crates/routes/src/images/utils.rs @@ -26,9 +26,9 @@ use reqwest::Body; use reqwest_middleware::{ClientBuilder, ClientWithMiddleware, RequestBuilder}; use reqwest_tracing::TracingMiddleware; use std::{sync::LazyLock, time::Duration}; -use url::Url; // Pictrs cannot use proxy +#[allow(clippy::expect_used)] pub(super) static PICTRS_CLIENT: LazyLock = LazyLock::new(|| { ClientBuilder::new( client_builder(&SETTINGS) From 60ba7af2a15248a3bc595d186ca4d7a5568ecd80 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 13 Dec 2024 15:23:05 +0100 Subject: [PATCH 06/51] config options --- config/defaults.hjson | 4 ++++ crates/routes/src/images/utils.rs | 11 ++++++----- crates/utils/src/settings/structs.rs | 12 ++++++++++++ 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/config/defaults.hjson b/config/defaults.hjson index 14749e542e..c1cb8d06b4 100644 --- a/config/defaults.hjson +++ b/config/defaults.hjson @@ -64,6 +64,10 @@ upload_timeout: 30 # Resize post thumbnails to this maximum width/height. max_thumbnail_size: 512 + # Maximum size for user avatar, community icon and site icon. + max_avatar_size: 512 + # Maximum size for user, community and site banner. + max_banner_size: 512 } # Email sending configuration. All options except login/password are mandatory email: { diff --git a/crates/routes/src/images/utils.rs b/crates/routes/src/images/utils.rs index dcc4188272..e4b2432014 100644 --- a/crates/routes/src/images/utils.rs +++ b/crates/routes/src/images/utils.rs @@ -61,7 +61,7 @@ fn adapt_request(request: &HttpRequest, url: String) -> RequestBuilder { }) } -pub(super) fn make_send(mut stream: S) -> impl Stream + Send + Unpin + 'static +fn make_send(mut stream: S) -> impl Stream + Send + Unpin + 'static where S: Stream + Unpin + 'static, S::Item: Send, @@ -85,7 +85,7 @@ where SendStream { rx } } -pub(super) struct SendStream { +struct SendStream { rx: tokio::sync::mpsc::Receiver, } @@ -135,15 +135,16 @@ pub(super) async fn do_upload_image( let max_size = context .settings() .pictrs_config()? - .max_thumbnail_size + .max_avatar_size .to_string(); client_req.query(&[ - ("max_width", max_size.as_ref()), - ("max_height", max_size.as_ref()), + ("resize", max_size.as_ref()), ("allow_animation", "false"), ("allow_video", "false"), ]) } + // TODO: same as above but using `max_banner_size` + // UploadType::Banner => {} _ => client_req, }; if let Some(addr) = req.head().peer_addr { diff --git a/crates/utils/src/settings/structs.rs b/crates/utils/src/settings/structs.rs index 9e561713c5..3048843936 100644 --- a/crates/utils/src/settings/structs.rs +++ b/crates/utils/src/settings/structs.rs @@ -104,6 +104,18 @@ pub struct PictrsConfig { /// Resize post thumbnails to this maximum width/height. #[default(512)] pub max_thumbnail_size: u32, + + /// Maximum size for user avatar, community icon and site icon. + #[default(512)] + pub max_avatar_size: u32, + + /// Maximum size for user, community and site banner. + /// + /// TODO: Unfortunately pictrs can only resize images to fit in a*a square, no rectangle. + /// Otherwise we have to use crop, or use max_width/max_height which throws error + /// if image is larger. + #[default(512)] + pub max_banner_size: u32, } #[derive(Debug, Deserialize, Serialize, Clone, SmartDefault, Document, PartialEq)] From 7c771d2113581449c0166ac5608ea6be56bafd40 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 13 Dec 2024 15:28:16 +0100 Subject: [PATCH 07/51] fix ts bindings --- config/defaults.hjson | 4 ++++ crates/api_common/src/image.rs | 4 ++++ crates/utils/src/settings/structs.rs | 4 ++-- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/config/defaults.hjson b/config/defaults.hjson index c1cb8d06b4..aaf2a94d84 100644 --- a/config/defaults.hjson +++ b/config/defaults.hjson @@ -67,6 +67,10 @@ # Maximum size for user avatar, community icon and site icon. max_avatar_size: 512 # Maximum size for user, community and site banner. + # + # TODO: Unfortunately pictrs can only resize images to fit in a*a square, no rectangle. + # Otherwise we have to use crop, or use max_width/max_height which throws error + # if image is larger. max_banner_size: 512 } # Email sending configuration. All options except login/password are mandatory diff --git a/crates/api_common/src/image.rs b/crates/api_common/src/image.rs index df033f2858..c28832921a 100644 --- a/crates/api_common/src/image.rs +++ b/crates/api_common/src/image.rs @@ -9,7 +9,9 @@ use url::Url; #[cfg_attr(feature = "full", derive(TS))] #[cfg_attr(feature = "full", ts(export))] pub struct ImageGetParams { + #[cfg_attr(feature = "full", ts(optional))] pub file_type: Option, + #[cfg_attr(feature = "full", ts(optional))] pub max_size: Option, } @@ -27,7 +29,9 @@ pub struct DeleteImageParams { #[cfg_attr(feature = "full", ts(export))] pub struct ImageProxyParams { pub url: String, + #[cfg_attr(feature = "full", ts(optional))] pub file_type: Option, + #[cfg_attr(feature = "full", ts(optional))] pub max_size: Option, } diff --git a/crates/utils/src/settings/structs.rs b/crates/utils/src/settings/structs.rs index 3048843936..28710e8ca9 100644 --- a/crates/utils/src/settings/structs.rs +++ b/crates/utils/src/settings/structs.rs @@ -110,8 +110,8 @@ pub struct PictrsConfig { pub max_avatar_size: u32, /// Maximum size for user, community and site banner. - /// - /// TODO: Unfortunately pictrs can only resize images to fit in a*a square, no rectangle. + /// + /// TODO: Unfortunately pictrs can only resize images to fit in a*a square, no rectangle. /// Otherwise we have to use crop, or use max_width/max_height which throws error /// if image is larger. #[default(512)] From 8ae4b405d0648465b378d64471073c8e35e74fcd Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Tue, 17 Dec 2024 11:29:43 +0100 Subject: [PATCH 08/51] fix api tests --- api_tests/package.json | 2 +- api_tests/pnpm-lock.yaml | 10 +++++----- api_tests/src/community.spec.ts | 3 +-- api_tests/src/follow.spec.ts | 1 - api_tests/src/image.spec.ts | 1 - api_tests/src/post.spec.ts | 1 - api_tests/src/shared.ts | 11 +++++------ api_tests/src/user.spec.ts | 25 +++---------------------- 8 files changed, 15 insertions(+), 39 deletions(-) diff --git a/api_tests/package.json b/api_tests/package.json index 5ace77ecfe..f4c6dd2850 100644 --- a/api_tests/package.json +++ b/api_tests/package.json @@ -28,7 +28,7 @@ "eslint": "^9.14.0", "eslint-plugin-prettier": "^5.1.3", "jest": "^29.5.0", - "lemmy-js-client": "0.20.0-image-api-rework.3", + "lemmy-js-client": "0.20.0-image-api-rework.5", "prettier": "^3.2.5", "ts-jest": "^29.1.0", "typescript": "^5.5.4", diff --git a/api_tests/pnpm-lock.yaml b/api_tests/pnpm-lock.yaml index 615a0ceca2..509b5bb5e3 100644 --- a/api_tests/pnpm-lock.yaml +++ b/api_tests/pnpm-lock.yaml @@ -30,8 +30,8 @@ importers: specifier: ^29.5.0 version: 29.7.0(@types/node@22.9.0) lemmy-js-client: - specifier: 0.20.0-image-api-rework.3 - version: 0.20.0-image-api-rework.3 + specifier: 0.20.0-image-api-rework.5 + version: 0.20.0-image-api-rework.5 prettier: specifier: ^3.2.5 version: 3.3.3 @@ -1167,8 +1167,8 @@ packages: resolution: {integrity: sha512-eTIzlVOSUR+JxdDFepEYcBMtZ9Qqdef+rnzWdRZuMbOywu5tO2w2N7rqjoANZ5k9vywhL6Br1VRjUIgTQx4E8w==} engines: {node: '>=6'} - lemmy-js-client@0.20.0-image-api-rework.3: - resolution: {integrity: sha512-SB20z+WD2S821q05OxzI2Lkwq1BWBNWM6Xd1l1bqKL310CRSAG4lln26+j8bjWxMgl/fYTqre8KG6l1YDiV3+Q==} + lemmy-js-client@0.20.0-image-api-rework.5: + resolution: {integrity: sha512-fMbgLefWy7B1QtX6c7Y/CMAbV70tKpT5eDKsdnCtKhV1eB73pATKNPaFtrMuAYu9h13M6LDlry6mX7CeWuJb6Q==} leven@3.1.0: resolution: {integrity: sha512-qsda+H8jTaUaN/x5vzW2rzc+8Rw4TAQ/4KjB46IwK5VH+IlVeeeje/EoZRpiXvIqjFgK84QffqPztGI3VBLG1A==} @@ -3077,7 +3077,7 @@ snapshots: kleur@3.0.3: {} - lemmy-js-client@0.20.0-image-api-rework.3: {} + lemmy-js-client@0.20.0-image-api-rework.5: {} leven@3.1.0: {} diff --git a/api_tests/src/community.spec.ts b/api_tests/src/community.spec.ts index 2bb0920881..a0b9d41d25 100644 --- a/api_tests/src/community.spec.ts +++ b/api_tests/src/community.spec.ts @@ -16,7 +16,6 @@ import { followCommunity, banPersonFromCommunity, resolvePerson, - getSite, createPost, getPost, resolvePost, @@ -36,7 +35,7 @@ import { userBlockInstance, } from "./shared"; import { AdminAllowInstanceParams } from "lemmy-js-client/dist/types/AdminAllowInstanceParams"; -import { EditCommunity, EditSite, GetPosts } from "lemmy-js-client"; +import { EditCommunity, GetPosts } from "lemmy-js-client"; beforeAll(setupLogins); afterAll(unfollows); diff --git a/api_tests/src/follow.spec.ts b/api_tests/src/follow.spec.ts index 936ce26065..c447e14cd7 100644 --- a/api_tests/src/follow.spec.ts +++ b/api_tests/src/follow.spec.ts @@ -5,7 +5,6 @@ import { setupLogins, resolveBetaCommunity, followCommunity, - getSite, waitUntil, beta, betaUrl, diff --git a/api_tests/src/image.spec.ts b/api_tests/src/image.spec.ts index fa22f271d1..d25ab7f4c3 100644 --- a/api_tests/src/image.spec.ts +++ b/api_tests/src/image.spec.ts @@ -18,7 +18,6 @@ import { epsilon, followCommunity, gamma, - getSite, imageFetchLimit, registerUser, resolveBetaCommunity, diff --git a/api_tests/src/post.spec.ts b/api_tests/src/post.spec.ts index 4158bbdc7b..c7193c573b 100644 --- a/api_tests/src/post.spec.ts +++ b/api_tests/src/post.spec.ts @@ -30,7 +30,6 @@ import { listPostReports, randomString, registerUser, - getSite, unfollows, resolveCommunity, waitUntil, diff --git a/api_tests/src/shared.ts b/api_tests/src/shared.ts index 1ed13d9cff..79614cb704 100644 --- a/api_tests/src/shared.ts +++ b/api_tests/src/shared.ts @@ -1,12 +1,11 @@ import { - AdminBlockInstanceParams, ApproveCommunityPendingFollower, BlockCommunity, BlockCommunityResponse, CommunityId, CommunityVisibility, CreatePrivateMessageReport, - DeleteImage, + DeleteImageParams, EditCommunity, GetCommunityPendingFollowsCountResponse, GetReplies, @@ -210,7 +209,9 @@ async function allowInstance(api: LemmyHttp, instance: string) { // Ignore errors from duplicate allows (because setup gets called for each test file) try { await api.adminAllowInstance(params); - } catch {} + } catch { + //console.log("Failed to allow instance"); + } } export async function createPost( @@ -715,7 +716,6 @@ export async function saveUserSettingsBio( export async function saveUserSettingsFederated( api: LemmyHttp, ): Promise { - let avatar = sampleImage; let banner = sampleImage; let bio = "a changed bio"; let form: SaveUserSettings = { @@ -724,7 +724,6 @@ export async function saveUserSettingsFederated( default_post_sort_type: "Hot", default_listing_type: "All", interface_language: "", - avatar, banner, display_name: "user321", show_avatars: false, @@ -947,7 +946,7 @@ export async function deleteAllImages(api: LemmyHttp) { Promise.all( imagesRes.images .map(image => { - const form: DeleteImage = { + const form: DeleteImageParams = { token: image.local_image.pictrs_delete_token, filename: image.local_image.pictrs_alias, }; diff --git a/api_tests/src/user.spec.ts b/api_tests/src/user.spec.ts index f7f80aecb2..7a25e61403 100644 --- a/api_tests/src/user.spec.ts +++ b/api_tests/src/user.spec.ts @@ -21,7 +21,6 @@ import { fetchFunction, alphaImage, unfollows, - saveUserSettingsBio, getMyUser, getPersonDetails, } from "./shared"; @@ -182,42 +181,24 @@ test("Set a new avatar, old avatar is deleted", async () => { const upload_form1: UploadImage = { image: Buffer.from("test1"), }; - const upload1 = await alphaImage.uploadImage(upload_form1); - expect(upload1.url).toBeDefined(); - - let form1 = { - avatar: upload1.url, - }; - await saveUserSettings(alpha, form1); + await alpha.userUploadAvatar(upload_form1); const listMediaRes1 = await alphaImage.listMedia(); expect(listMediaRes1.images.length).toBe(1); const upload_form2: UploadImage = { image: Buffer.from("test2"), }; - const upload2 = await alphaImage.uploadImage(upload_form2); - expect(upload2.url).toBeDefined(); - - let form2 = { - avatar: upload2.url, - }; - await saveUserSettings(alpha, form2); + await alpha.userUploadAvatar(upload_form2); // make sure only the new avatar is kept const listMediaRes2 = await alphaImage.listMedia(); expect(listMediaRes2.images.length).toBe(1); // Upload that same form2 avatar, make sure it isn't replaced / deleted - await saveUserSettings(alpha, form2); + await alpha.userUploadAvatar(upload_form2); // make sure only the new avatar is kept const listMediaRes3 = await alphaImage.listMedia(); expect(listMediaRes3.images.length).toBe(1); - // Now try to save a user settings, with the icon missing, - // and make sure it doesn't clear the data, or delete the image - await saveUserSettingsBio(alpha); - let my_user = await getMyUser(alpha); - expect(my_user.local_user_view.person.avatar).toBe(upload2.url); - // make sure only the new avatar is kept const listMediaRes4 = await alphaImage.listMedia(); expect(listMediaRes4.images.length).toBe(1); From b0d4bdb8ff0d9e4bb9e7c3f4132a264259e7b470 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Tue, 17 Dec 2024 16:10:55 +0100 Subject: [PATCH 09/51] Add option to disable image upload (fixes #1118) --- crates/api_common/src/request.rs | 10 +++++----- crates/api_common/src/site.rs | 3 +++ crates/api_common/src/utils.rs | 4 ++-- crates/api_crud/src/site/read.rs | 1 + crates/routes/src/images/mod.rs | 13 +++++++++---- crates/routes/src/images/utils.rs | 4 ++-- crates/utils/src/error.rs | 1 + crates/utils/src/settings/mod.rs | 2 +- crates/utils/src/settings/structs.rs | 5 +++++ 9 files changed, 29 insertions(+), 14 deletions(-) diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index e337cbdec9..a03e6598f3 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -320,7 +320,7 @@ pub async fn purge_image_from_pictrs(image_url: &Url, context: &LemmyContext) -> .next_back() .ok_or(LemmyErrorType::ImageUrlMissingLastPathSegment)?; - let pictrs_config = context.settings().pictrs_config()?; + let pictrs_config = context.settings().pictrs()?; let purge_url = format!("{}internal/purge?alias={}", pictrs_config.url, alias); let pictrs_api_key = pictrs_config @@ -348,7 +348,7 @@ pub async fn delete_image_from_pictrs( delete_token: &str, context: &LemmyContext, ) -> LemmyResult<()> { - let pictrs_config = context.settings().pictrs_config()?; + let pictrs_config = context.settings().pictrs()?; let url = format!( "{}image/delete/{}/{}", pictrs_config.url, &delete_token, &alias @@ -366,7 +366,7 @@ pub async fn delete_image_from_pictrs( /// Retrieves the image with local pict-rs and generates a thumbnail. Returns the thumbnail url. #[tracing::instrument(skip_all)] async fn generate_pictrs_thumbnail(image_url: &Url, context: &LemmyContext) -> LemmyResult { - let pictrs_config = context.settings().pictrs_config()?; + let pictrs_config = context.settings().pictrs()?; match pictrs_config.image_mode() { PictrsImageMode::None => return Ok(image_url.clone()), @@ -382,7 +382,7 @@ async fn generate_pictrs_thumbnail(image_url: &Url, context: &LemmyContext) -> L "{}image/download?url={}&resize={}", pictrs_config.url, encode(image_url.as_str()), - context.settings().pictrs_config()?.max_thumbnail_size + context.settings().pictrs()?.max_thumbnail_size ); let res = context @@ -425,7 +425,7 @@ pub async fn fetch_pictrs_proxied_image_details( image_url: &Url, context: &LemmyContext, ) -> LemmyResult { - let pictrs_url = context.settings().pictrs_config()?.url; + let pictrs_url = context.settings().pictrs()?.url; let encoded_image_url = encode(image_url.as_str()); // Pictrs needs you to fetch the proxied image before you can fetch the details diff --git a/crates/api_common/src/site.rs b/crates/api_common/src/site.rs index 7f1000b14e..0b1fb2200e 100644 --- a/crates/api_common/src/site.rs +++ b/crates/api_common/src/site.rs @@ -455,6 +455,9 @@ pub struct GetSiteResponse { #[cfg_attr(feature = "full", ts(optional))] pub admin_oauth_providers: Option>, pub blocked_urls: Vec, + // If true then uploads for post images or markdown images are disabled. Only avatars, icons and + // banners can be set. + pub image_upload_disabled: bool, } #[skip_serializing_none] diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 6daeec4cd1..3daa341070 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -1060,7 +1060,7 @@ pub async fn process_markdown( markdown_check_for_blocked_urls(&text, url_blocklist)?; - if context.settings().pictrs_config()?.image_mode() == PictrsImageMode::ProxyAllImages { + if context.settings().pictrs()?.image_mode() == PictrsImageMode::ProxyAllImages { let (text, links) = markdown_rewrite_image_links(text); RemoteImage::create(&mut context.pool(), links.clone()).await?; @@ -1130,7 +1130,7 @@ async fn proxy_image_link_internal( pub async fn proxy_image_link(link: Url, context: &LemmyContext) -> LemmyResult { proxy_image_link_internal( link, - context.settings().pictrs_config()?.image_mode(), + context.settings().pictrs()?.image_mode(), context, ) .await diff --git a/crates/api_crud/src/site/read.rs b/crates/api_crud/src/site/read.rs index 220fe1bd53..b618c0fa2a 100644 --- a/crates/api_crud/src/site/read.rs +++ b/crates/api_crud/src/site/read.rs @@ -69,5 +69,6 @@ async fn read_site(context: &LemmyContext) -> LemmyResult { tagline, oauth_providers: Some(oauth_providers), admin_oauth_providers: Some(admin_oauth_providers), + image_upload_disabled: context.settings().pictrs()?.disable_image_upload, }) } diff --git a/crates/routes/src/images/mod.rs b/crates/routes/src/images/mod.rs index 632aa30f20..8e5a547345 100644 --- a/crates/routes/src/images/mod.rs +++ b/crates/routes/src/images/mod.rs @@ -2,6 +2,7 @@ use actix_web::{body::BoxBody, web::*, HttpRequest, HttpResponse, Responder}; use lemmy_api_common::{ context::LemmyContext, image::{DeleteImageParams, ImageGetParams, ImageProxyParams, UploadImageResponse}, + LemmyErrorType, SuccessResponse, }; use lemmy_db_schema::source::{ @@ -22,6 +23,10 @@ pub async fn upload_image( local_user_view: LocalUserView, context: Data, ) -> LemmyResult> { + if context.settings().pictrs()?.disable_image_upload { + return Err(LemmyErrorType::ImageUploadDisabled.into()); + } + let image = do_upload_image(req, body, UploadType::Other, &local_user_view, &context).await?; let image_url = image.image_url(&context.settings().get_protocol_and_hostname())?; @@ -47,7 +52,7 @@ pub async fn get_image( let name = &filename.into_inner(); // If there are no query params, the URL is original - let pictrs_url = context.settings().pictrs_config()?.url; + let pictrs_url = context.settings().pictrs()?.url; let processed_url = if params.file_type.is_none() && params.max_size.is_none() { format!("{}image/original/{}", pictrs_url, name) } else { @@ -69,7 +74,7 @@ pub async fn delete_image( // require login _local_user_view: LocalUserView, ) -> LemmyResult> { - let pictrs_config = context.settings().pictrs_config()?; + let pictrs_config = context.settings().pictrs()?; let url = format!( "{}image/delete/{}/{}", pictrs_config.url, &data.token, &data.filename @@ -83,7 +88,7 @@ pub async fn delete_image( } pub async fn pictrs_health(context: Data) -> LemmyResult> { - let pictrs_config = context.settings().pictrs_config()?; + let pictrs_config = context.settings().pictrs()?; let url = format!("{}healthz", pictrs_config.url); PICTRS_CLIENT.get(url).send().await?.error_for_status()?; @@ -102,7 +107,7 @@ pub async fn image_proxy( // for arbitrary purposes. RemoteImage::validate(&mut context.pool(), url.clone().into()).await?; - let pictrs_config = context.settings().pictrs_config()?; + let pictrs_config = context.settings().pictrs()?; let processed_url = if params.file_type.is_none() && params.max_size.is_none() { format!("{}image/original?proxy={}", pictrs_config.url, params.url) } else { diff --git a/crates/routes/src/images/utils.rs b/crates/routes/src/images/utils.rs index e4b2432014..50e0b8fd40 100644 --- a/crates/routes/src/images/utils.rs +++ b/crates/routes/src/images/utils.rs @@ -125,7 +125,7 @@ pub(super) async fn do_upload_image( local_user_view: &LocalUserView, context: &Data, ) -> LemmyResult { - let pictrs_config = context.settings().pictrs_config()?; + let pictrs_config = context.settings().pictrs()?; let image_url = format!("{}image", pictrs_config.url); let mut client_req = adapt_request(&req, image_url); @@ -134,7 +134,7 @@ pub(super) async fn do_upload_image( UploadType::Avatar => { let max_size = context .settings() - .pictrs_config()? + .pictrs()? .max_avatar_size .to_string(); client_req.query(&[ diff --git a/crates/utils/src/error.rs b/crates/utils/src/error.rs index d2605608e2..d2a40b4ec5 100644 --- a/crates/utils/src/error.rs +++ b/crates/utils/src/error.rs @@ -31,6 +31,7 @@ pub enum LemmyErrorType { NoContentTypeHeader, NotAnImageType, InvalidImageUpload, + ImageUploadDisabled, NotAModOrAdmin, NotTopMod, NotLoggedIn, diff --git a/crates/utils/src/settings/mod.rs b/crates/utils/src/settings/mod.rs index 72d986d2da..6ef535ab95 100644 --- a/crates/utils/src/settings/mod.rs +++ b/crates/utils/src/settings/mod.rs @@ -97,7 +97,7 @@ impl Settings { WEBFINGER_REGEX.clone() } - pub fn pictrs_config(&self) -> LemmyResult { + pub fn pictrs(&self) -> LemmyResult { self .pictrs .clone() diff --git a/crates/utils/src/settings/structs.rs b/crates/utils/src/settings/structs.rs index 28710e8ca9..041980d2e5 100644 --- a/crates/utils/src/settings/structs.rs +++ b/crates/utils/src/settings/structs.rs @@ -116,6 +116,11 @@ pub struct PictrsConfig { /// if image is larger. #[default(512)] pub max_banner_size: u32, + + /// Prevent users from uploading images for posts or embedding in markdown. Avatars, icons and + /// banners can still be uploaded. + #[default(false)] + pub disable_image_upload: bool, } #[derive(Debug, Deserialize, Serialize, Clone, SmartDefault, Document, PartialEq)] From 55b8aced9d7c8b1245bc6f835d82639b0566fc6f Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Tue, 17 Dec 2024 16:23:43 +0100 Subject: [PATCH 10/51] split files into upload, download --- config/defaults.hjson | 3 + crates/api/src/site/leave_admin.rs | 1 + crates/api_common/src/utils.rs | 7 +- crates/api_crud/src/site/read.rs | 2 +- crates/routes/src/images/download.rs | 113 +++++++++++++++++++++++ crates/routes/src/images/mod.rs | 111 ++-------------------- crates/routes/src/images/person.rs | 36 -------- crates/routes/src/images/upload.rs | 132 +++++++++++++++++++++++++++ crates/routes/src/images/utils.rs | 122 ++----------------------- crates/utils/src/settings/structs.rs | 4 +- src/api_routes_v3.rs | 7 +- src/api_routes_v4.rs | 6 +- 12 files changed, 277 insertions(+), 267 deletions(-) create mode 100644 crates/routes/src/images/download.rs delete mode 100644 crates/routes/src/images/person.rs create mode 100644 crates/routes/src/images/upload.rs diff --git a/config/defaults.hjson b/config/defaults.hjson index aaf2a94d84..d6c24cce81 100644 --- a/config/defaults.hjson +++ b/config/defaults.hjson @@ -72,6 +72,9 @@ # Otherwise we have to use crop, or use max_width/max_height which throws error # if image is larger. max_banner_size: 512 + # Prevent users from uploading images for posts or embedding in markdown. Avatars, icons and + # banners can still be uploaded. + image_upload_disabled: false } # Email sending configuration. All options except login/password are mandatory email: { diff --git a/crates/api/src/site/leave_admin.rs b/crates/api/src/site/leave_admin.rs index fde258dd2e..042009d243 100644 --- a/crates/api/src/site/leave_admin.rs +++ b/crates/api/src/site/leave_admin.rs @@ -76,5 +76,6 @@ pub async fn leave_admin( blocked_urls, tagline, my_user: None, + image_upload_disabled: context.settings().pictrs()?.image_upload_disabled, })) } diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 3daa341070..27b7ad5311 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -1128,12 +1128,7 @@ async fn proxy_image_link_internal( /// Rewrite a link to go through `/api/v4/image_proxy` endpoint. This is only for remote urls and /// if image_proxy setting is enabled. pub async fn proxy_image_link(link: Url, context: &LemmyContext) -> LemmyResult { - proxy_image_link_internal( - link, - context.settings().pictrs()?.image_mode(), - context, - ) - .await + proxy_image_link_internal(link, context.settings().pictrs()?.image_mode(), context).await } pub async fn proxy_image_link_opt_api( diff --git a/crates/api_crud/src/site/read.rs b/crates/api_crud/src/site/read.rs index b618c0fa2a..64d2237a0f 100644 --- a/crates/api_crud/src/site/read.rs +++ b/crates/api_crud/src/site/read.rs @@ -69,6 +69,6 @@ async fn read_site(context: &LemmyContext) -> LemmyResult { tagline, oauth_providers: Some(oauth_providers), admin_oauth_providers: Some(admin_oauth_providers), - image_upload_disabled: context.settings().pictrs()?.disable_image_upload, + image_upload_disabled: context.settings().pictrs()?.image_upload_disabled, }) } diff --git a/crates/routes/src/images/download.rs b/crates/routes/src/images/download.rs new file mode 100644 index 0000000000..b54b72f1ca --- /dev/null +++ b/crates/routes/src/images/download.rs @@ -0,0 +1,113 @@ +use super::utils::{adapt_request, convert_header, file_type}; +use actix_web::{ + body::{BodyStream, BoxBody}, + http::StatusCode, + web::{Data, *}, + HttpRequest, + HttpResponse, + Responder, +}; +use lemmy_api_common::{ + context::LemmyContext, + image::{ImageGetParams, ImageProxyParams}, +}; +use lemmy_db_schema::source::{images::RemoteImage, local_site::LocalSite}; +use lemmy_db_views::structs::LocalUserView; +use lemmy_utils::error::LemmyResult; +use url::Url; + +pub async fn get_image( + filename: Path, + Query(params): Query, + req: HttpRequest, + context: Data, + local_user_view: Option, +) -> LemmyResult { + // block access to images if instance is private and unauthorized, public + let local_site = LocalSite::read(&mut context.pool()).await?; + if local_site.private_instance && local_user_view.is_none() { + return Ok(HttpResponse::Unauthorized().finish()); + } + let name = &filename.into_inner(); + + // If there are no query params, the URL is original + let pictrs_url = context.settings().pictrs()?.url; + let processed_url = if params.file_type.is_none() && params.max_size.is_none() { + format!("{}image/original/{}", pictrs_url, name) + } else { + let file_type = file_type(params.file_type, name); + let mut url = format!("{}image/process.{}?src={}", pictrs_url, file_type, name); + + if let Some(size) = params.max_size { + url = format!("{url}&thumbnail={size}",); + } + url + }; + + do_get_image(processed_url, req).await +} +pub async fn image_proxy( + Query(params): Query, + req: HttpRequest, + context: Data, +) -> LemmyResult, HttpResponse>> { + let url = Url::parse(¶ms.url)?; + + // Check that url corresponds to a federated image so that this can't be abused as a proxy + // for arbitrary purposes. + RemoteImage::validate(&mut context.pool(), url.clone().into()).await?; + + let pictrs_config = context.settings().pictrs()?; + let processed_url = if params.file_type.is_none() && params.max_size.is_none() { + format!("{}image/original?proxy={}", pictrs_config.url, params.url) + } else { + let file_type = file_type(params.file_type, url.as_str()); + let mut url = format!( + "{}image/process.{}?proxy={}", + pictrs_config.url, file_type, url + ); + + if let Some(size) = params.max_size { + url = format!("{url}&thumbnail={size}",); + } + url + }; + + let bypass_proxy = pictrs_config + .proxy_bypass_domains + .iter() + .any(|s| url.domain().is_some_and(|d| d == s)); + if bypass_proxy { + // Bypass proxy and redirect user to original image + Ok(Either::Left(Redirect::to(url.to_string()).respond_to(&req))) + } else { + // Proxy the image data through Lemmy + Ok(Either::Right(do_get_image(processed_url, req).await?)) + } +} + +pub(super) async fn do_get_image(url: String, req: HttpRequest) -> LemmyResult { + let mut client_req = adapt_request(&req, url); + + if let Some(addr) = req.head().peer_addr { + client_req = client_req.header("X-Forwarded-For", addr.to_string()); + } + + if let Some(addr) = req.head().peer_addr { + client_req = client_req.header("X-Forwarded-For", addr.to_string()); + } + + let res = client_req.send().await?; + + if res.status() == http::StatusCode::NOT_FOUND { + return Ok(HttpResponse::NotFound().finish()); + } + + let mut client_res = HttpResponse::build(StatusCode::from_u16(res.status().as_u16())?); + + for (name, value) in res.headers().iter().filter(|(h, _)| *h != "connection") { + client_res.insert_header(convert_header(name, value)); + } + + Ok(client_res.body(BodyStream::new(res.bytes_stream()))) +} diff --git a/crates/routes/src/images/mod.rs b/crates/routes/src/images/mod.rs index 8e5a547345..52d3d89df3 100644 --- a/crates/routes/src/images/mod.rs +++ b/crates/routes/src/images/mod.rs @@ -1,73 +1,14 @@ -use actix_web::{body::BoxBody, web::*, HttpRequest, HttpResponse, Responder}; -use lemmy_api_common::{ - context::LemmyContext, - image::{DeleteImageParams, ImageGetParams, ImageProxyParams, UploadImageResponse}, - LemmyErrorType, - SuccessResponse, -}; -use lemmy_db_schema::source::{ - images::{LocalImage, RemoteImage}, - local_site::LocalSite, -}; +use actix_web::web::*; +use lemmy_api_common::{context::LemmyContext, image::DeleteImageParams, SuccessResponse}; +use lemmy_db_schema::source::images::LocalImage; use lemmy_db_views::structs::LocalUserView; use lemmy_utils::error::LemmyResult; -use url::Url; -use utils::{do_get_image, do_upload_image, file_type, UploadType, PICTRS_CLIENT}; +use utils::PICTRS_CLIENT; -pub mod person; +pub mod download; +pub mod upload; mod utils; -pub async fn upload_image( - req: HttpRequest, - body: Payload, - local_user_view: LocalUserView, - context: Data, -) -> LemmyResult> { - if context.settings().pictrs()?.disable_image_upload { - return Err(LemmyErrorType::ImageUploadDisabled.into()); - } - - let image = do_upload_image(req, body, UploadType::Other, &local_user_view, &context).await?; - - let image_url = image.image_url(&context.settings().get_protocol_and_hostname())?; - Ok(Json(UploadImageResponse { - image_url, - filename: image.file, - delete_token: image.delete_token, - })) -} - -pub async fn get_image( - filename: Path, - Query(params): Query, - req: HttpRequest, - context: Data, - local_user_view: Option, -) -> LemmyResult { - // block access to images if instance is private and unauthorized, public - let local_site = LocalSite::read(&mut context.pool()).await?; - if local_site.private_instance && local_user_view.is_none() { - return Ok(HttpResponse::Unauthorized().finish()); - } - let name = &filename.into_inner(); - - // If there are no query params, the URL is original - let pictrs_url = context.settings().pictrs()?.url; - let processed_url = if params.file_type.is_none() && params.max_size.is_none() { - format!("{}image/original/{}", pictrs_url, name) - } else { - let file_type = file_type(params.file_type, name); - let mut url = format!("{}image/process.{}?src={}", pictrs_url, file_type, name); - - if let Some(size) = params.max_size { - url = format!("{url}&thumbnail={size}",); - } - url - }; - - do_get_image(processed_url, req).await -} - pub async fn delete_image( data: Json, context: Data, @@ -95,43 +36,3 @@ pub async fn pictrs_health(context: Data) -> LemmyResult, - req: HttpRequest, - context: Data, -) -> LemmyResult, HttpResponse>> { - let url = Url::parse(¶ms.url)?; - - // Check that url corresponds to a federated image so that this can't be abused as a proxy - // for arbitrary purposes. - RemoteImage::validate(&mut context.pool(), url.clone().into()).await?; - - let pictrs_config = context.settings().pictrs()?; - let processed_url = if params.file_type.is_none() && params.max_size.is_none() { - format!("{}image/original?proxy={}", pictrs_config.url, params.url) - } else { - let file_type = file_type(params.file_type, url.as_str()); - let mut url = format!( - "{}image/process.{}?proxy={}", - pictrs_config.url, file_type, url - ); - - if let Some(size) = params.max_size { - url = format!("{url}&thumbnail={size}",); - } - url - }; - - let bypass_proxy = pictrs_config - .proxy_bypass_domains - .iter() - .any(|s| url.domain().is_some_and(|d| d == s)); - if bypass_proxy { - // Bypass proxy and redirect user to original image - Ok(Either::Left(Redirect::to(url.to_string()).respond_to(&req))) - } else { - // Proxy the image data through Lemmy - Ok(Either::Right(do_get_image(processed_url, req).await?)) - } -} diff --git a/crates/routes/src/images/person.rs b/crates/routes/src/images/person.rs deleted file mode 100644 index edac1e41b2..0000000000 --- a/crates/routes/src/images/person.rs +++ /dev/null @@ -1,36 +0,0 @@ -use super::utils::{delete_old_image, do_upload_image, UploadType}; -use actix_web::{self, web::*, HttpRequest}; -use lemmy_api_common::{context::LemmyContext, SuccessResponse}; -use lemmy_db_schema::{ - source::person::{Person, PersonUpdateForm}, - traits::Crud, -}; -use lemmy_db_views::structs::LocalUserView; -use lemmy_utils::error::LemmyResult; -use url::Url; - -pub async fn upload_avatar( - req: HttpRequest, - body: Payload, - local_user_view: LocalUserView, - context: Data, -) -> LemmyResult> { - let image = do_upload_image(req, body, UploadType::Avatar, &local_user_view, &context).await?; - - delete_old_image(&local_user_view.person.avatar, &context).await?; - - let avatar = format!( - "{}/api/v4/image/{}", - context.settings().get_protocol_and_hostname(), - image.file - ); - let avatar = Some(Some(Url::parse(&avatar)?.into())); - let person_form = PersonUpdateForm { - avatar, - ..Default::default() - }; - - Person::update(&mut context.pool(), local_user_view.person.id, &person_form).await?; - - Ok(Json(SuccessResponse::default())) -} diff --git a/crates/routes/src/images/upload.rs b/crates/routes/src/images/upload.rs new file mode 100644 index 0000000000..c009aae263 --- /dev/null +++ b/crates/routes/src/images/upload.rs @@ -0,0 +1,132 @@ +use super::utils::{adapt_request, delete_old_image, make_send}; +use actix_web::{self, web::*, HttpRequest}; +use lemmy_api_common::{ + context::LemmyContext, + image::UploadImageResponse, + request::{PictrsFile, PictrsResponse}, + LemmyErrorType, + SuccessResponse, +}; +use lemmy_db_schema::{ + source::{ + images::{LocalImage, LocalImageForm}, + person::{Person, PersonUpdateForm}, + }, + traits::Crud, +}; +use lemmy_db_views::structs::LocalUserView; +use lemmy_utils::error::LemmyResult; +use reqwest::Body; +use std::time::Duration; +use url::Url; + +pub async fn upload_image( + req: HttpRequest, + body: Payload, + local_user_view: LocalUserView, + context: Data, +) -> LemmyResult> { + if context.settings().pictrs()?.image_upload_disabled { + return Err(LemmyErrorType::ImageUploadDisabled.into()); + } + + let image = do_upload_image(req, body, UploadType::Other, &local_user_view, &context).await?; + + let image_url = image.image_url(&context.settings().get_protocol_and_hostname())?; + Ok(Json(UploadImageResponse { + image_url, + filename: image.file, + delete_token: image.delete_token, + })) +} + +pub async fn upload_avatar( + req: HttpRequest, + body: Payload, + local_user_view: LocalUserView, + context: Data, +) -> LemmyResult> { + let image = do_upload_image(req, body, UploadType::Avatar, &local_user_view, &context).await?; + + delete_old_image(&local_user_view.person.avatar, &context).await?; + + let avatar = format!( + "{}/api/v4/image/{}", + context.settings().get_protocol_and_hostname(), + image.file + ); + let avatar = Some(Some(Url::parse(&avatar)?.into())); + let person_form = PersonUpdateForm { + avatar, + ..Default::default() + }; + + Person::update(&mut context.pool(), local_user_view.person.id, &person_form).await?; + + Ok(Json(SuccessResponse::default())) +} +pub enum UploadType { + Avatar, + Other, +} + +pub async fn do_upload_image( + req: HttpRequest, + body: Payload, + upload_type: UploadType, + local_user_view: &LocalUserView, + context: &Data, +) -> LemmyResult { + let pictrs_config = context.settings().pictrs()?; + let image_url = format!("{}image", pictrs_config.url); + + let mut client_req = adapt_request(&req, image_url); + + client_req = match upload_type { + UploadType::Avatar => { + let max_size = context.settings().pictrs()?.max_avatar_size.to_string(); + client_req.query(&[ + ("resize", max_size.as_ref()), + ("allow_animation", "false"), + ("allow_video", "false"), + ]) + } + // TODO: same as above but using `max_banner_size` + // UploadType::Banner => {} + _ => client_req, + }; + if let Some(addr) = req.head().peer_addr { + client_req = client_req.header("X-Forwarded-For", addr.to_string()) + }; + let res = client_req + .timeout(Duration::from_secs(pictrs_config.upload_timeout)) + .body(Body::wrap_stream(make_send(body))) + .send() + .await? + .error_for_status()?; + + let mut images = res.json::().await?; + for image in &images.files { + // Pictrs allows uploading multiple images in a single request. Lemmy doesnt need this, + // but still a user may upload multiple and so we need to store all links in db for + // to allow deletion via web ui. + let form = LocalImageForm { + local_user_id: Some(local_user_view.local_user.id), + pictrs_alias: image.file.to_string(), + pictrs_delete_token: image.delete_token.to_string(), + }; + + let protocol_and_hostname = context.settings().get_protocol_and_hostname(); + let thumbnail_url = image.image_url(&protocol_and_hostname)?; + + // Also store the details for the image + let details_form = image.details.build_image_details_form(&thumbnail_url); + LocalImage::create(&mut context.pool(), &form, &details_form).await?; + } + let image = images + .files + .pop() + .ok_or(LemmyErrorType::InvalidImageUpload)?; + + Ok(image) +} diff --git a/crates/routes/src/images/utils.rs b/crates/routes/src/images/utils.rs index 50e0b8fd40..040734a8bc 100644 --- a/crates/routes/src/images/utils.rs +++ b/crates/routes/src/images/utils.rs @@ -1,31 +1,22 @@ use actix_web::{ - body::BodyStream, http::{ header::{HeaderName, ACCEPT_ENCODING, HOST}, Method, - StatusCode, }, - web::{Data, Payload}, + web::Data, HttpRequest, - HttpResponse, }; use futures::stream::{Stream, StreamExt}; use http::HeaderValue; use lemmy_api_common::{ context::LemmyContext, - request::{client_builder, delete_image_from_pictrs, PictrsFile, PictrsResponse}, - LemmyErrorType, + request::{client_builder, delete_image_from_pictrs}, }; -use lemmy_db_schema::{ - newtypes::DbUrl, - source::images::{LocalImage, LocalImageForm}, -}; -use lemmy_db_views::structs::LocalUserView; +use lemmy_db_schema::{newtypes::DbUrl, source::images::LocalImage}; use lemmy_utils::{error::LemmyResult, settings::SETTINGS, REQWEST_TIMEOUT}; -use reqwest::Body; use reqwest_middleware::{ClientBuilder, ClientWithMiddleware, RequestBuilder}; use reqwest_tracing::TracingMiddleware; -use std::{sync::LazyLock, time::Duration}; +use std::sync::LazyLock; // Pictrs cannot use proxy #[allow(clippy::expect_used)] @@ -40,7 +31,7 @@ pub(super) static PICTRS_CLIENT: LazyLock = LazyLock::new( .build() }); -fn adapt_request(request: &HttpRequest, url: String) -> RequestBuilder { +pub(super) fn adapt_request(request: &HttpRequest, url: String) -> RequestBuilder { // remove accept-encoding header so that pictrs doesn't compress the response const INVALID_HEADERS: &[HeaderName] = &[ACCEPT_ENCODING, HOST]; @@ -61,7 +52,7 @@ fn adapt_request(request: &HttpRequest, url: String) -> RequestBuilder { }) } -fn make_send(mut stream: S) -> impl Stream + Send + Unpin + 'static +pub(super) fn make_send(mut stream: S) -> impl Stream + Send + Unpin + 'static where S: Stream + Unpin + 'static, S::Item: Send, @@ -109,106 +100,13 @@ pub(super) fn convert_method(method: &Method) -> http::Method { http::Method::from_bytes(method.as_str().as_bytes()).expect("method can be converted") } -fn convert_header<'a>(name: &'a http::HeaderName, value: &'a HeaderValue) -> (&'a str, &'a [u8]) { +pub(super) fn convert_header<'a>( + name: &'a http::HeaderName, + value: &'a HeaderValue, +) -> (&'a str, &'a [u8]) { (name.as_str(), value.as_bytes()) } -pub(super) enum UploadType { - Avatar, - Other, -} - -pub(super) async fn do_upload_image( - req: HttpRequest, - body: Payload, - upload_type: UploadType, - local_user_view: &LocalUserView, - context: &Data, -) -> LemmyResult { - let pictrs_config = context.settings().pictrs()?; - let image_url = format!("{}image", pictrs_config.url); - - let mut client_req = adapt_request(&req, image_url); - - client_req = match upload_type { - UploadType::Avatar => { - let max_size = context - .settings() - .pictrs()? - .max_avatar_size - .to_string(); - client_req.query(&[ - ("resize", max_size.as_ref()), - ("allow_animation", "false"), - ("allow_video", "false"), - ]) - } - // TODO: same as above but using `max_banner_size` - // UploadType::Banner => {} - _ => client_req, - }; - if let Some(addr) = req.head().peer_addr { - client_req = client_req.header("X-Forwarded-For", addr.to_string()) - }; - let res = client_req - .timeout(Duration::from_secs(pictrs_config.upload_timeout)) - .body(Body::wrap_stream(make_send(body))) - .send() - .await? - .error_for_status()?; - - let mut images = res.json::().await?; - for image in &images.files { - // Pictrs allows uploading multiple images in a single request. Lemmy doesnt need this, - // but still a user may upload multiple and so we need to store all links in db for - // to allow deletion via web ui. - let form = LocalImageForm { - local_user_id: Some(local_user_view.local_user.id), - pictrs_alias: image.file.to_string(), - pictrs_delete_token: image.delete_token.to_string(), - }; - - let protocol_and_hostname = context.settings().get_protocol_and_hostname(); - let thumbnail_url = image.image_url(&protocol_and_hostname)?; - - // Also store the details for the image - let details_form = image.details.build_image_details_form(&thumbnail_url); - LocalImage::create(&mut context.pool(), &form, &details_form).await?; - } - let image = images - .files - .pop() - .ok_or(LemmyErrorType::InvalidImageUpload)?; - - Ok(image) -} - -pub(super) async fn do_get_image(url: String, req: HttpRequest) -> LemmyResult { - let mut client_req = adapt_request(&req, url); - - if let Some(addr) = req.head().peer_addr { - client_req = client_req.header("X-Forwarded-For", addr.to_string()); - } - - if let Some(addr) = req.head().peer_addr { - client_req = client_req.header("X-Forwarded-For", addr.to_string()); - } - - let res = client_req.send().await?; - - if res.status() == http::StatusCode::NOT_FOUND { - return Ok(HttpResponse::NotFound().finish()); - } - - let mut client_res = HttpResponse::build(StatusCode::from_u16(res.status().as_u16())?); - - for (name, value) in res.headers().iter().filter(|(h, _)| *h != "connection") { - client_res.insert_header(convert_header(name, value)); - } - - Ok(client_res.body(BodyStream::new(res.bytes_stream()))) -} - /// When adding a new avatar, banner or similar image, delete the old one. pub(super) async fn delete_old_image( old_image: &Option, diff --git a/crates/utils/src/settings/structs.rs b/crates/utils/src/settings/structs.rs index 041980d2e5..b9c38f2036 100644 --- a/crates/utils/src/settings/structs.rs +++ b/crates/utils/src/settings/structs.rs @@ -116,11 +116,11 @@ pub struct PictrsConfig { /// if image is larger. #[default(512)] pub max_banner_size: u32, - + /// Prevent users from uploading images for posts or embedding in markdown. Avatars, icons and /// banners can still be uploaded. #[default(false)] - pub disable_image_upload: bool, + pub image_upload_disabled: bool, } #[derive(Debug, Deserialize, Serialize, Clone, SmartDefault, Document, PartialEq)] diff --git a/src/api_routes_v3.rs b/src/api_routes_v3.rs index e7cb828838..59b7de610f 100644 --- a/src/api_routes_v3.rs +++ b/src/api_routes_v3.rs @@ -134,7 +134,12 @@ use lemmy_apub::api::{ search::search, user_settings_backup::{export_settings, import_settings}, }; -use lemmy_routes::images::{delete_image, get_image, image_proxy, pictrs_health, upload_image}; +use lemmy_routes::images::{ + delete_image, + download::{get_image, image_proxy}, + pictrs_health, + upload::upload_image, +}; use lemmy_utils::rate_limit::RateLimitCell; // Deprecated, use api v4 instead. diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index 99ae86fbc6..4bfe06a6a6 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -161,11 +161,9 @@ use lemmy_apub::api::{ }; use lemmy_routes::images::{ delete_image, - get_image, - image_proxy, - person::upload_avatar, + download::{get_image, image_proxy}, pictrs_health, - upload_image, + upload::{upload_avatar, upload_image}, }; use lemmy_utils::rate_limit::RateLimitCell; From 460ccc8e5a37159b54f736b2c330815ea2658301 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 18 Dec 2024 11:11:31 +0100 Subject: [PATCH 11/51] move sitemap to top level, not in api --- src/api_routes_v3.rs | 6 ------ src/api_routes_v4.rs | 2 -- src/lib.rs | 8 +++++++- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/api_routes_v3.rs b/src/api_routes_v3.rs index 59b7de610f..738299cdac 100644 --- a/src/api_routes_v3.rs +++ b/src/api_routes_v3.rs @@ -87,7 +87,6 @@ use lemmy_api::{ unread_count::get_unread_registration_application_count, }, }, - sitemap::get_sitemap, }; use lemmy_api_crud::{ comment::{ @@ -395,9 +394,4 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route("/delete", post().to(delete_custom_emoji)), ), ); - cfg.service( - scope("/sitemap.xml") - .wrap(rate_limit.message()) - .route("", get().to(get_sitemap)), - ); } diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index 4bfe06a6a6..f8dabd0622 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -96,7 +96,6 @@ use lemmy_api::{ unread_count::get_unread_registration_application_count, }, }, - sitemap::get_sitemap, }; use lemmy_api_crud::{ comment::{ @@ -392,7 +391,6 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .wrap(rate_limit.register()) .route("/authenticate", post().to(authenticate_with_oauth)), ) - .route("/sitemap.xml", get().to(get_sitemap)) .service( scope("/image") .service( diff --git a/src/lib.rs b/src/lib.rs index ae060ec75e..03dc783007 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,13 +11,14 @@ use actix_cors::Cors; use actix_web::{ dev::{ServerHandle, ServiceResponse}, middleware::{self, Condition, ErrorHandlerResponse, ErrorHandlers}, - web::Data, + web::{get, scope, Data}, App, HttpResponse, HttpServer, }; use actix_web_prom::PrometheusMetricsBuilder; use clap::Parser; +use lemmy_api::sitemap::get_sitemap; use lemmy_api_common::{ context::LemmyContext, lemmy_db_views::structs::SiteView, @@ -324,6 +325,11 @@ fn create_http_server( }) .configure(feeds::config) .configure(nodeinfo::config) + .service( + scope("/sitemap.xml") + .wrap(rate_limit_cell.message()) + .route("", get().to(get_sitemap)), + ) }) .disable_signals() .bind(bind)? From 5f0619534e2edefe87e598206e940488d19cd285 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 18 Dec 2024 11:24:30 +0100 Subject: [PATCH 12/51] simplify code --- api_tests/run-federation-test.sh | 2 +- crates/routes/src/images/upload.rs | 51 ++++++++++++---------------- crates/utils/src/settings/structs.rs | 1 + src/api_routes_v4.rs | 4 +-- 4 files changed, 26 insertions(+), 32 deletions(-) diff --git a/api_tests/run-federation-test.sh b/api_tests/run-federation-test.sh index 969a95b3e7..f9eab50395 100755 --- a/api_tests/run-federation-test.sh +++ b/api_tests/run-federation-test.sh @@ -11,7 +11,7 @@ killall -s1 lemmy_server || true popd pnpm i -pnpm api-test || true +pnpm api-test-image || true killall -s1 lemmy_server || true killall -s1 pict-rs || true diff --git a/crates/routes/src/images/upload.rs b/crates/routes/src/images/upload.rs index c009aae263..735a18d154 100644 --- a/crates/routes/src/images/upload.rs +++ b/crates/routes/src/images/upload.rs @@ -3,7 +3,7 @@ use actix_web::{self, web::*, HttpRequest}; use lemmy_api_common::{ context::LemmyContext, image::UploadImageResponse, - request::{PictrsFile, PictrsResponse}, + request::PictrsResponse, LemmyErrorType, SuccessResponse, }; @@ -18,7 +18,12 @@ use lemmy_db_views::structs::LocalUserView; use lemmy_utils::error::LemmyResult; use reqwest::Body; use std::time::Duration; -use url::Url; +use UploadType::*; + +pub enum UploadType { + Avatar, + Other, +} pub async fn upload_image( req: HttpRequest, @@ -30,45 +35,28 @@ pub async fn upload_image( return Err(LemmyErrorType::ImageUploadDisabled.into()); } - let image = do_upload_image(req, body, UploadType::Other, &local_user_view, &context).await?; - - let image_url = image.image_url(&context.settings().get_protocol_and_hostname())?; - Ok(Json(UploadImageResponse { - image_url, - filename: image.file, - delete_token: image.delete_token, - })) + Ok(Json( + do_upload_image(req, body, Other, &local_user_view, &context).await?, + )) } -pub async fn upload_avatar( +pub async fn upload_user_avatar( req: HttpRequest, body: Payload, local_user_view: LocalUserView, context: Data, ) -> LemmyResult> { - let image = do_upload_image(req, body, UploadType::Avatar, &local_user_view, &context).await?; - + let image = do_upload_image(req, body, Avatar, &local_user_view, &context).await?; delete_old_image(&local_user_view.person.avatar, &context).await?; - let avatar = format!( - "{}/api/v4/image/{}", - context.settings().get_protocol_and_hostname(), - image.file - ); - let avatar = Some(Some(Url::parse(&avatar)?.into())); let person_form = PersonUpdateForm { - avatar, + avatar: Some(Some(image.image_url.into())), ..Default::default() }; - Person::update(&mut context.pool(), local_user_view.person.id, &person_form).await?; Ok(Json(SuccessResponse::default())) } -pub enum UploadType { - Avatar, - Other, -} pub async fn do_upload_image( req: HttpRequest, @@ -76,14 +64,14 @@ pub async fn do_upload_image( upload_type: UploadType, local_user_view: &LocalUserView, context: &Data, -) -> LemmyResult { +) -> LemmyResult { let pictrs_config = context.settings().pictrs()?; let image_url = format!("{}image", pictrs_config.url); let mut client_req = adapt_request(&req, image_url); client_req = match upload_type { - UploadType::Avatar => { + Avatar => { let max_size = context.settings().pictrs()?.max_avatar_size.to_string(); client_req.query(&[ ("resize", max_size.as_ref()), @@ -92,7 +80,7 @@ pub async fn do_upload_image( ]) } // TODO: same as above but using `max_banner_size` - // UploadType::Banner => {} + // Banner => {} _ => client_req, }; if let Some(addr) = req.head().peer_addr { @@ -128,5 +116,10 @@ pub async fn do_upload_image( .pop() .ok_or(LemmyErrorType::InvalidImageUpload)?; - Ok(image) + let url = image.image_url(&context.settings().get_protocol_and_hostname())?; + Ok(UploadImageResponse { + image_url: url, + filename: image.file, + delete_token: image.delete_token, + }) } diff --git a/crates/utils/src/settings/structs.rs b/crates/utils/src/settings/structs.rs index b9c38f2036..153c4567e1 100644 --- a/crates/utils/src/settings/structs.rs +++ b/crates/utils/src/settings/structs.rs @@ -114,6 +114,7 @@ pub struct PictrsConfig { /// TODO: Unfortunately pictrs can only resize images to fit in a*a square, no rectangle. /// Otherwise we have to use crop, or use max_width/max_height which throws error /// if image is larger. + /// TODO: whats a sensible default here? #[default(512)] pub max_banner_size: u32, diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index f8dabd0622..90f751b558 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -162,7 +162,7 @@ use lemmy_routes::images::{ delete_image, download::{get_image, image_proxy}, pictrs_health, - upload::{upload_avatar, upload_image}, + upload::{upload_image, upload_user_avatar}, }; use lemmy_utils::rate_limit::RateLimitCell; @@ -319,7 +319,7 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route("/unread_count", get().to(unread_count)) .route("/list_logins", get().to(list_logins)) .route("/validate_auth", get().to(validate_auth)) - .route("/avatar", post().to(upload_avatar)) + .route("/avatar", post().to(upload_user_avatar)) .service( scope("/block") .route("/person", post().to(user_block_person)) From 5f49b2aaec2af04dcc5acf8ada63ce5ee571401d Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 18 Dec 2024 11:26:59 +0100 Subject: [PATCH 13/51] add upload user banner --- crates/api/src/local_user/save_settings.rs | 9 +-------- crates/api_common/src/person.rs | 3 --- crates/routes/src/images/upload.rs | 19 +++++++++++++++++++ src/api_routes_v4.rs | 3 ++- 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/crates/api/src/local_user/save_settings.rs b/crates/api/src/local_user/save_settings.rs index bcf1b02b6e..930f6bf483 100644 --- a/crates/api/src/local_user/save_settings.rs +++ b/crates/api/src/local_user/save_settings.rs @@ -3,12 +3,10 @@ use actix_web::web::Json; use lemmy_api_common::{ context::LemmyContext, person::SaveUserSettings, - request::replace_image, utils::{ get_url_blocklist, local_site_to_slur_regex, process_markdown_opt, - proxy_image_link_opt_api, send_verification_email, }, SuccessResponse, @@ -21,7 +19,7 @@ use lemmy_db_schema::{ person::{Person, PersonUpdateForm}, }, traits::Crud, - utils::{diesel_string_update, diesel_url_update}, + utils::diesel_string_update, }; use lemmy_db_views::structs::{LocalUserView, SiteView}; use lemmy_utils::{ @@ -46,10 +44,6 @@ pub async fn save_user_settings( .as_deref(), ); - let banner = diesel_url_update(data.banner.as_deref())?; - replace_image(&banner, &local_user_view.person.banner, &context).await?; - let banner = proxy_image_link_opt_api(banner, &context).await?; - let display_name = diesel_string_update(data.display_name.as_deref()); let matrix_user_id = diesel_string_update(data.matrix_user_id.as_deref()); let email_deref = data.email.as_deref().map(str::to_lowercase); @@ -104,7 +98,6 @@ pub async fn save_user_settings( bio, matrix_user_id, bot_account: data.bot_account, - banner, ..Default::default() }; diff --git a/crates/api_common/src/person.rs b/crates/api_common/src/person.rs index 9c8a1d087b..5e79023776 100644 --- a/crates/api_common/src/person.rs +++ b/crates/api_common/src/person.rs @@ -114,9 +114,6 @@ pub struct SaveUserSettings { /// The language of the lemmy interface #[cfg_attr(feature = "full", ts(optional))] pub interface_language: Option, - /// A URL for your banner. - #[cfg_attr(feature = "full", ts(optional))] - pub banner: Option, /// Your display name, which can contain strange characters, and does not need to be unique. #[cfg_attr(feature = "full", ts(optional))] pub display_name: Option, diff --git a/crates/routes/src/images/upload.rs b/crates/routes/src/images/upload.rs index 735a18d154..6b24940b95 100644 --- a/crates/routes/src/images/upload.rs +++ b/crates/routes/src/images/upload.rs @@ -22,6 +22,7 @@ use UploadType::*; pub enum UploadType { Avatar, + Banner, Other, } @@ -58,6 +59,24 @@ pub async fn upload_user_avatar( Ok(Json(SuccessResponse::default())) } +pub async fn upload_user_banner( + req: HttpRequest, + body: Payload, + local_user_view: LocalUserView, + context: Data, +) -> LemmyResult> { + let image = do_upload_image(req, body, Banner, &local_user_view, &context).await?; + delete_old_image(&local_user_view.person.banner, &context).await?; + + let person_form = PersonUpdateForm { + banner: Some(Some(image.image_url.into())), + ..Default::default() + }; + Person::update(&mut context.pool(), local_user_view.person.id, &person_form).await?; + + Ok(Json(SuccessResponse::default())) +} + pub async fn do_upload_image( req: HttpRequest, body: Payload, diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index 90f751b558..f725b66ca5 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -162,7 +162,7 @@ use lemmy_routes::images::{ delete_image, download::{get_image, image_proxy}, pictrs_health, - upload::{upload_image, upload_user_avatar}, + upload::{upload_image, upload_user_avatar, upload_user_banner}, }; use lemmy_utils::rate_limit::RateLimitCell; @@ -320,6 +320,7 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route("/list_logins", get().to(list_logins)) .route("/validate_auth", get().to(validate_auth)) .route("/avatar", post().to(upload_user_avatar)) + .route("/banner", post().to(upload_user_banner)) .service( scope("/block") .route("/person", post().to(user_block_person)) From a6f7e76bec2da12b52515b085ea3667d3de3b8bf Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 18 Dec 2024 11:39:19 +0100 Subject: [PATCH 14/51] community icon/banner --- crates/api_common/src/image.rs | 8 +++ crates/api_crud/src/community/create.rs | 10 ---- crates/routes/src/images/upload.rs | 74 +++++++++++++++++++++---- src/api_routes_v4.rs | 10 +++- 4 files changed, 80 insertions(+), 22 deletions(-) diff --git a/crates/api_common/src/image.rs b/crates/api_common/src/image.rs index c28832921a..e93b367f60 100644 --- a/crates/api_common/src/image.rs +++ b/crates/api_common/src/image.rs @@ -1,3 +1,4 @@ +use lemmy_db_schema::newtypes::CommunityId; use serde::{Deserialize, Serialize}; use serde_with::skip_serializing_none; #[cfg(feature = "full")] @@ -44,3 +45,10 @@ pub struct UploadImageResponse { pub filename: String, pub delete_token: String, } + +/// Parameter for setting community icon or banner. Can't use POST data here as it already contains +/// the image data. +#[derive(Debug, Serialize, Deserialize, Clone, Default, PartialEq, Eq, Hash)] +pub struct CommunityIdQuery { + pub id: CommunityId, +} diff --git a/crates/api_crud/src/community/create.rs b/crates/api_crud/src/community/create.rs index c81157950d..0437396cc7 100644 --- a/crates/api_crud/src/community/create.rs +++ b/crates/api_crud/src/community/create.rs @@ -13,7 +13,6 @@ use lemmy_api_common::{ is_admin, local_site_to_slur_regex, process_markdown_opt, - proxy_image_link_api, EndpointType, }, }; @@ -31,7 +30,6 @@ use lemmy_db_schema::{ }, }, traits::{ApubActor, Crud, Followable, Joinable}, - utils::diesel_url_create, }; use lemmy_db_views::structs::{LocalUserView, SiteView}; use lemmy_utils::{ @@ -76,12 +74,6 @@ pub async fn create_community( check_slurs(desc, &slur_regex)?; } - let icon = diesel_url_create(data.icon.as_deref())?; - let icon = proxy_image_link_api(icon, &context).await?; - - let banner = diesel_url_create(data.banner.as_deref())?; - let banner = proxy_image_link_api(banner, &context).await?; - is_valid_actor_name(&data.name, local_site.actor_name_max_length as usize)?; if let Some(desc) = &data.description { @@ -108,8 +100,6 @@ pub async fn create_community( let community_form = CommunityInsertForm { sidebar, description, - icon, - banner, nsfw: data.nsfw, actor_id: Some(community_actor_id.clone()), private_key: Some(keypair.private_key), diff --git a/crates/routes/src/images/upload.rs b/crates/routes/src/images/upload.rs index 6b24940b95..8774216411 100644 --- a/crates/routes/src/images/upload.rs +++ b/crates/routes/src/images/upload.rs @@ -2,13 +2,15 @@ use super::utils::{adapt_request, delete_old_image, make_send}; use actix_web::{self, web::*, HttpRequest}; use lemmy_api_common::{ context::LemmyContext, - image::UploadImageResponse, + image::{CommunityIdQuery, UploadImageResponse}, request::PictrsResponse, + utils::is_mod_or_admin, LemmyErrorType, SuccessResponse, }; use lemmy_db_schema::{ source::{ + community::{Community, CommunityUpdateForm}, images::{LocalImage, LocalImageForm}, person::{Person, PersonUpdateForm}, }, @@ -50,11 +52,11 @@ pub async fn upload_user_avatar( let image = do_upload_image(req, body, Avatar, &local_user_view, &context).await?; delete_old_image(&local_user_view.person.avatar, &context).await?; - let person_form = PersonUpdateForm { + let form = PersonUpdateForm { avatar: Some(Some(image.image_url.into())), ..Default::default() }; - Person::update(&mut context.pool(), local_user_view.person.id, &person_form).await?; + Person::update(&mut context.pool(), local_user_view.person.id, &form).await?; Ok(Json(SuccessResponse::default())) } @@ -68,11 +70,55 @@ pub async fn upload_user_banner( let image = do_upload_image(req, body, Banner, &local_user_view, &context).await?; delete_old_image(&local_user_view.person.banner, &context).await?; - let person_form = PersonUpdateForm { + let form = PersonUpdateForm { banner: Some(Some(image.image_url.into())), ..Default::default() }; - Person::update(&mut context.pool(), local_user_view.person.id, &person_form).await?; + Person::update(&mut context.pool(), local_user_view.person.id, &form).await?; + + Ok(Json(SuccessResponse::default())) +} + +pub async fn upload_community_icon( + req: HttpRequest, + query: Query, + body: Payload, + local_user_view: LocalUserView, + context: Data, +) -> LemmyResult> { + let community: Community = Community::read(&mut context.pool(), query.id).await?; + is_mod_or_admin(&mut context.pool(), &local_user_view.person, community.id).await?; + + let image = do_upload_image(req, body, Avatar, &local_user_view, &context).await?; + delete_old_image(&community.icon, &context).await?; + + let form = CommunityUpdateForm { + icon: Some(Some(image.image_url.into())), + ..Default::default() + }; + Community::update(&mut context.pool(), community.id, &form).await?; + + Ok(Json(SuccessResponse::default())) +} + +pub async fn upload_community_banner( + req: HttpRequest, + query: Query, + body: Payload, + local_user_view: LocalUserView, + context: Data, +) -> LemmyResult> { + let community: Community = Community::read(&mut context.pool(), query.id).await?; + is_mod_or_admin(&mut context.pool(), &local_user_view.person, community.id).await?; + + let image = do_upload_image(req, body, Banner, &local_user_view, &context).await?; + delete_old_image(&community.icon, &context).await?; + + let form = CommunityUpdateForm { + banner: Some(Some(image.image_url.into())), + ..Default::default() + }; + Community::update(&mut context.pool(), community.id, &form).await?; Ok(Json(SuccessResponse::default())) } @@ -84,29 +130,35 @@ pub async fn do_upload_image( local_user_view: &LocalUserView, context: &Data, ) -> LemmyResult { - let pictrs_config = context.settings().pictrs()?; - let image_url = format!("{}image", pictrs_config.url); + let pictrs = context.settings().pictrs()?; + let image_url = format!("{}image", pictrs.url); let mut client_req = adapt_request(&req, image_url); client_req = match upload_type { Avatar => { - let max_size = context.settings().pictrs()?.max_avatar_size.to_string(); + let max_size = pictrs.max_avatar_size.to_string(); + client_req.query(&[ + ("resize", max_size.as_ref()), + ("allow_animation", "false"), + ("allow_video", "false"), + ]) + } + Banner => { + let max_size = pictrs.max_banner_size.to_string(); client_req.query(&[ ("resize", max_size.as_ref()), ("allow_animation", "false"), ("allow_video", "false"), ]) } - // TODO: same as above but using `max_banner_size` - // Banner => {} _ => client_req, }; if let Some(addr) = req.head().peer_addr { client_req = client_req.header("X-Forwarded-For", addr.to_string()) }; let res = client_req - .timeout(Duration::from_secs(pictrs_config.upload_timeout)) + .timeout(Duration::from_secs(pictrs.upload_timeout)) .body(Body::wrap_stream(make_send(body))) .send() .await? diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index f725b66ca5..c7f0dd3548 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -162,7 +162,13 @@ use lemmy_routes::images::{ delete_image, download::{get_image, image_proxy}, pictrs_health, - upload::{upload_image, upload_user_avatar, upload_user_banner}, + upload::{ + upload_community_banner, + upload_community_icon, + upload_image, + upload_user_avatar, + upload_user_banner, + }, }; use lemmy_utils::rate_limit::RateLimitCell; @@ -205,6 +211,8 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route("/transfer", post().to(transfer_community)) .route("/ban_user", post().to(ban_from_community)) .route("/mod", post().to(add_mod_to_community)) + .route("/icon", post().to(upload_community_icon)) + .route("/banner", post().to(upload_community_banner)) .service( scope("/pending_follows") .route("/count", get().to(get_pending_follows_count)) From ada8f1ba8e7948e97877762ac8dc5452739a9e93 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 18 Dec 2024 11:52:17 +0100 Subject: [PATCH 15/51] site icon/banner --- api_tests/run-federation-test.sh | 2 +- crates/api_common/src/community.rs | 6 ---- crates/api_common/src/request.rs | 35 +++--------------- crates/api_common/src/site.rs | 10 ------ crates/api_common/src/utils.rs | 25 ------------- crates/api_crud/src/community/update.rs | 14 +------- crates/api_crud/src/site/create.rs | 11 +----- crates/api_crud/src/site/update.rs | 14 +------- crates/routes/src/images/upload.rs | 47 +++++++++++++++++++++++-- src/api_routes_v4.rs | 6 +++- 10 files changed, 58 insertions(+), 112 deletions(-) diff --git a/api_tests/run-federation-test.sh b/api_tests/run-federation-test.sh index f9eab50395..969a95b3e7 100755 --- a/api_tests/run-federation-test.sh +++ b/api_tests/run-federation-test.sh @@ -11,7 +11,7 @@ killall -s1 lemmy_server || true popd pnpm i -pnpm api-test-image || true +pnpm api-test || true killall -s1 lemmy_server || true killall -s1 pict-rs || true diff --git a/crates/api_common/src/community.rs b/crates/api_common/src/community.rs index 898767b345..9105c6f843 100644 --- a/crates/api_common/src/community.rs +++ b/crates/api_common/src/community.rs @@ -177,12 +177,6 @@ pub struct EditCommunity { /// A shorter, one line description of your community. #[cfg_attr(feature = "full", ts(optional))] pub description: Option, - /// An icon URL. - #[cfg_attr(feature = "full", ts(optional))] - pub icon: Option, - /// A banner URL. - #[cfg_attr(feature = "full", ts(optional))] - pub banner: Option, /// Whether its an NSFW community. #[cfg_attr(feature = "full", ts(optional))] pub nsfw: Option, diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index a03e6598f3..1691b93de6 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -9,13 +9,10 @@ use activitypub_federation::config::Data; use chrono::{DateTime, Utc}; use encoding_rs::{Encoding, UTF_8}; use futures::StreamExt; -use lemmy_db_schema::{ - newtypes::DbUrl, - source::{ - images::{ImageDetailsForm, LocalImage, LocalImageForm}, - post::{Post, PostUpdateForm}, - site::Site, - }, +use lemmy_db_schema::source::{ + images::{ImageDetailsForm, LocalImage, LocalImageForm}, + post::{Post, PostUpdateForm}, + site::Site, }; use lemmy_utils::{ error::{LemmyError, LemmyErrorExt, LemmyErrorType, LemmyResult}, @@ -472,30 +469,6 @@ async fn is_image_content_type(client: &ClientWithMiddleware, url: &Url) -> Lemm } } -/// When adding a new avatar, banner or similar image, delete the old one. -/// TODO: remove this function -pub async fn replace_image( - new_image: &Option>, - old_image: &Option, - context: &Data, -) -> LemmyResult<()> { - if let (Some(Some(new_image)), Some(old_image)) = (new_image, old_image) { - // Note: Oftentimes front ends will include the current image in the form. - // In this case, deleting `old_image` would also be deletion of `new_image`, - // so the deletion must be skipped for the image to be kept. - if new_image != old_image { - // Ignore errors because image may be stored externally. - let image = LocalImage::delete_by_url(&mut context.pool(), old_image) - .await - .ok(); - if let Some(image) = image { - delete_image_from_pictrs(&image.pictrs_alias, &image.pictrs_delete_token, context).await?; - } - } - } - Ok(()) -} - #[cfg(test)] mod tests { diff --git a/crates/api_common/src/site.rs b/crates/api_common/src/site.rs index 0b1fb2200e..7ccf06ef5f 100644 --- a/crates/api_common/src/site.rs +++ b/crates/api_common/src/site.rs @@ -201,10 +201,6 @@ pub struct CreateSite { #[cfg_attr(feature = "full", ts(optional))] pub description: Option, #[cfg_attr(feature = "full", ts(optional))] - pub icon: Option, - #[cfg_attr(feature = "full", ts(optional))] - pub banner: Option, - #[cfg_attr(feature = "full", ts(optional))] pub enable_nsfw: Option, #[cfg_attr(feature = "full", ts(optional))] pub community_creation_admin_only: Option, @@ -298,12 +294,6 @@ pub struct EditSite { /// A shorter, one line description of your site. #[cfg_attr(feature = "full", ts(optional))] pub description: Option, - /// A url for your site's icon. - #[cfg_attr(feature = "full", ts(optional))] - pub icon: Option, - /// A url for your site's banner. - #[cfg_attr(feature = "full", ts(optional))] - pub banner: Option, /// Whether to enable NSFW. #[cfg_attr(feature = "full", ts(optional))] pub enable_nsfw: Option, diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 27b7ad5311..8b73fb6e82 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -1131,31 +1131,6 @@ pub async fn proxy_image_link(link: Url, context: &LemmyContext) -> LemmyResult< proxy_image_link_internal(link, context.settings().pictrs()?.image_mode(), context).await } -pub async fn proxy_image_link_opt_api( - link: Option>, - context: &LemmyContext, -) -> LemmyResult>> { - if let Some(Some(link)) = link { - proxy_image_link(link.into(), context) - .await - .map(Some) - .map(Some) - } else { - Ok(link) - } -} - -pub async fn proxy_image_link_api( - link: Option, - context: &LemmyContext, -) -> LemmyResult> { - if let Some(link) = link { - proxy_image_link(link.into(), context).await.map(Some) - } else { - Ok(link) - } -} - pub async fn proxy_image_link_opt_apub( link: Option, context: &LemmyContext, diff --git a/crates/api_crud/src/community/update.rs b/crates/api_crud/src/community/update.rs index d9c062c531..944f5bade5 100644 --- a/crates/api_crud/src/community/update.rs +++ b/crates/api_crud/src/community/update.rs @@ -6,14 +6,12 @@ use lemmy_api_common::{ build_response::build_community_response, community::{CommunityResponse, EditCommunity}, context::LemmyContext, - request::replace_image, send_activity::{ActivityChannel, SendActivityData}, utils::{ check_community_mod_action, get_url_blocklist, local_site_to_slur_regex, process_markdown_opt, - proxy_image_link_opt_api, }, }; use lemmy_db_schema::{ @@ -23,7 +21,7 @@ use lemmy_db_schema::{ local_site::LocalSite, }, traits::Crud, - utils::{diesel_string_update, diesel_url_update}, + utils::diesel_string_update, }; use lemmy_db_views::structs::LocalUserView; use lemmy_utils::{ @@ -58,14 +56,6 @@ pub async fn update_community( let old_community = Community::read(&mut context.pool(), data.community_id).await?; - let icon = diesel_url_update(data.icon.as_deref())?; - replace_image(&icon, &old_community.icon, &context).await?; - let icon = proxy_image_link_opt_api(icon, &context).await?; - - let banner = diesel_url_update(data.banner.as_deref())?; - replace_image(&banner, &old_community.banner, &context).await?; - let banner = proxy_image_link_opt_api(banner, &context).await?; - // Verify its a mod (only mods can edit it) check_community_mod_action( &local_user_view.person, @@ -91,8 +81,6 @@ pub async fn update_community( title: data.title.clone(), sidebar, description, - icon, - banner, nsfw: data.nsfw, posting_restricted_to_mods: data.posting_restricted_to_mods, visibility: data.visibility, diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index c8140cc280..34965742d9 100644 --- a/crates/api_crud/src/site/create.rs +++ b/crates/api_crud/src/site/create.rs @@ -13,7 +13,6 @@ use lemmy_api_common::{ local_site_rate_limit_to_rate_limit_config, local_site_to_slur_regex, process_markdown_opt, - proxy_image_link_api, }, }; use lemmy_db_schema::{ @@ -24,7 +23,7 @@ use lemmy_db_schema::{ site::{Site, SiteUpdateForm}, }, traits::Crud, - utils::{diesel_string_update, diesel_url_create}, + utils::diesel_string_update, }; use lemmy_db_views::structs::{LocalUserView, SiteView}; use lemmy_utils::{ @@ -63,18 +62,10 @@ pub async fn create_site( let url_blocklist = get_url_blocklist(&context).await?; let sidebar = process_markdown_opt(&data.sidebar, &slur_regex, &url_blocklist, &context).await?; - let icon = diesel_url_create(data.icon.as_deref())?; - let icon = proxy_image_link_api(icon, &context).await?; - - let banner = diesel_url_create(data.banner.as_deref())?; - let banner = proxy_image_link_api(banner, &context).await?; - let site_form = SiteUpdateForm { name: Some(data.name.clone()), sidebar: diesel_string_update(sidebar.as_deref()), description: diesel_string_update(data.description.as_deref()), - icon: Some(icon), - banner: Some(banner), actor_id: Some(actor_id), last_refreshed_at: Some(Utc::now()), inbox_url, diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index d2585ea43b..40b51208ea 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -5,7 +5,6 @@ use actix_web::web::Json; use chrono::Utc; use lemmy_api_common::{ context::LemmyContext, - request::replace_image, site::{EditSite, SiteResponse}, utils::{ get_url_blocklist, @@ -13,7 +12,6 @@ use lemmy_api_common::{ local_site_rate_limit_to_rate_limit_config, local_site_to_slur_regex, process_markdown_opt, - proxy_image_link_opt_api, }, }; use lemmy_db_schema::{ @@ -26,7 +24,7 @@ use lemmy_db_schema::{ site::{Site, SiteUpdateForm}, }, traits::Crud, - utils::{diesel_string_update, diesel_url_update}, + utils::diesel_string_update, RegistrationMode, }; use lemmy_db_views::structs::{LocalUserView, SiteView}; @@ -72,20 +70,10 @@ pub async fn update_site( .as_deref(), ); - let icon = diesel_url_update(data.icon.as_deref())?; - replace_image(&icon, &site.icon, &context).await?; - let icon = proxy_image_link_opt_api(icon, &context).await?; - - let banner = diesel_url_update(data.banner.as_deref())?; - replace_image(&banner, &site.banner, &context).await?; - let banner = proxy_image_link_opt_api(banner, &context).await?; - let site_form = SiteUpdateForm { name: data.name.clone(), sidebar, description: diesel_string_update(data.description.as_deref()), - icon, - banner, content_warning: diesel_string_update(data.content_warning.as_deref()), updated: Some(Some(Utc::now())), ..Default::default() diff --git a/crates/routes/src/images/upload.rs b/crates/routes/src/images/upload.rs index 8774216411..9fd20b3ef0 100644 --- a/crates/routes/src/images/upload.rs +++ b/crates/routes/src/images/upload.rs @@ -4,7 +4,7 @@ use lemmy_api_common::{ context::LemmyContext, image::{CommunityIdQuery, UploadImageResponse}, request::PictrsResponse, - utils::is_mod_or_admin, + utils::{is_admin, is_mod_or_admin}, LemmyErrorType, SuccessResponse, }; @@ -13,6 +13,7 @@ use lemmy_db_schema::{ community::{Community, CommunityUpdateForm}, images::{LocalImage, LocalImageForm}, person::{Person, PersonUpdateForm}, + site::{Site, SiteUpdateForm}, }, traits::Crud, }; @@ -112,7 +113,7 @@ pub async fn upload_community_banner( is_mod_or_admin(&mut context.pool(), &local_user_view.person, community.id).await?; let image = do_upload_image(req, body, Banner, &local_user_view, &context).await?; - delete_old_image(&community.icon, &context).await?; + delete_old_image(&community.banner, &context).await?; let form = CommunityUpdateForm { banner: Some(Some(image.image_url.into())), @@ -123,6 +124,48 @@ pub async fn upload_community_banner( Ok(Json(SuccessResponse::default())) } +pub async fn upload_site_icon( + req: HttpRequest, + body: Payload, + local_user_view: LocalUserView, + context: Data, +) -> LemmyResult> { + is_admin(&local_user_view)?; + let site = Site::read_local(&mut context.pool()).await?; + + let image = do_upload_image(req, body, Avatar, &local_user_view, &context).await?; + delete_old_image(&site.icon, &context).await?; + + let form = SiteUpdateForm { + icon: Some(Some(image.image_url.into())), + ..Default::default() + }; + Site::update(&mut context.pool(), site.id, &form).await?; + + Ok(Json(SuccessResponse::default())) +} + +pub async fn upload_site_banner( + req: HttpRequest, + body: Payload, + local_user_view: LocalUserView, + context: Data, +) -> LemmyResult> { + is_admin(&local_user_view)?; + let site = Site::read_local(&mut context.pool()).await?; + + let image = do_upload_image(req, body, Banner, &local_user_view, &context).await?; + delete_old_image(&site.banner, &context).await?; + + let form = SiteUpdateForm { + banner: Some(Some(image.image_url.into())), + ..Default::default() + }; + Site::update(&mut context.pool(), site.id, &form).await?; + + Ok(Json(SuccessResponse::default())) +} + pub async fn do_upload_image( req: HttpRequest, body: Payload, diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index c7f0dd3548..86b35312aa 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -166,6 +166,8 @@ use lemmy_routes::images::{ upload_community_banner, upload_community_icon, upload_image, + upload_site_banner, + upload_site_icon, upload_user_avatar, upload_user_banner, }, @@ -181,7 +183,9 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { scope("/site") .route("", get().to(get_site_v4)) .route("", post().to(create_site)) - .route("", put().to(update_site)), + .route("", put().to(update_site)) + .route("/icon", post().to(upload_site_icon)) + .route("/banner", post().to(upload_site_banner)), ) .route("/modlog", get().to(get_mod_log)) .service( From 4951aa0fd171554321940997942c95b510bb1887 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 18 Dec 2024 11:55:37 +0100 Subject: [PATCH 16/51] update js client --- api_tests/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api_tests/package.json b/api_tests/package.json index f4c6dd2850..4939d8c82f 100644 --- a/api_tests/package.json +++ b/api_tests/package.json @@ -28,7 +28,7 @@ "eslint": "^9.14.0", "eslint-plugin-prettier": "^5.1.3", "jest": "^29.5.0", - "lemmy-js-client": "0.20.0-image-api-rework.5", + "lemmy-js-client": "0.20.0-image-api-rework.6", "prettier": "^3.2.5", "ts-jest": "^29.1.0", "typescript": "^5.5.4", From 961c7f1fcbe03b27797f0e8ee6608bb8cea6fa4e Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 8 Jan 2025 10:27:12 +0100 Subject: [PATCH 17/51] wip --- config/defaults.hjson | 1 + crates/routes/src/images/mod.rs | 9 +++++++++ src/api_routes_v4.rs | 1 + 3 files changed, 11 insertions(+) diff --git a/config/defaults.hjson b/config/defaults.hjson index d6c24cce81..c13b709075 100644 --- a/config/defaults.hjson +++ b/config/defaults.hjson @@ -71,6 +71,7 @@ # TODO: Unfortunately pictrs can only resize images to fit in a*a square, no rectangle. # Otherwise we have to use crop, or use max_width/max_height which throws error # if image is larger. + # TODO: whats a sensible default here? max_banner_size: 512 # Prevent users from uploading images for posts or embedding in markdown. Avatars, icons and # banners can still be uploaded. diff --git a/crates/routes/src/images/mod.rs b/crates/routes/src/images/mod.rs index 52d3d89df3..1d3260e448 100644 --- a/crates/routes/src/images/mod.rs +++ b/crates/routes/src/images/mod.rs @@ -9,6 +9,15 @@ pub mod download; pub mod upload; mod utils; +pub async fn delete_community_icon( + data: Json, + context: Data, + local_user_view: LocalUserView, +) -> LemmyResult> { + todo!() +} + +// TODO: get rid of delete tokens and allow deletion by admin or uploader pub async fn delete_image( data: Json, context: Data, diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index 253167983f..3572f8ebff 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -207,6 +207,7 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route("/ban_user", post().to(ban_from_community)) .route("/mod", post().to(add_mod_to_community)) .route("/icon", post().to(upload_community_icon)) + .route("/icon", delete().to(delete_community_icon)) .route("/banner", post().to(upload_community_banner)) .service( scope("/pending_follows") From 705703f5396c0d34bceba61fdfc2eec5668856cb Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Thu, 9 Jan 2025 15:23:06 +0100 Subject: [PATCH 18/51] add delete endpoints --- api_tests/package.json | 2 +- api_tests/pnpm-lock.yaml | 10 +- crates/routes/src/images/delete.rs | 142 +++++++++++++++++++++++++++++ crates/routes/src/images/mod.rs | 33 +------ src/api_routes_v3.rs | 2 +- src/api_routes_v4.rs | 17 +++- 6 files changed, 166 insertions(+), 40 deletions(-) create mode 100644 crates/routes/src/images/delete.rs diff --git a/api_tests/package.json b/api_tests/package.json index b75ddbd520..39deef22f9 100644 --- a/api_tests/package.json +++ b/api_tests/package.json @@ -28,7 +28,7 @@ "eslint": "^9.14.0", "eslint-plugin-prettier": "^5.1.3", "jest": "^29.5.0", - "lemmy-js-client": "0.20.0-image-api-rework.7", + "lemmy-js-client": "0.20.0-image-api-rework.8", "prettier": "^3.2.5", "ts-jest": "^29.1.0", "typescript": "^5.5.4", diff --git a/api_tests/pnpm-lock.yaml b/api_tests/pnpm-lock.yaml index 01bc71cb64..5aaa59d2cb 100644 --- a/api_tests/pnpm-lock.yaml +++ b/api_tests/pnpm-lock.yaml @@ -30,8 +30,8 @@ importers: specifier: ^29.5.0 version: 29.7.0(@types/node@22.9.0) lemmy-js-client: - specifier: 0.20.0-image-api-rework.7 - version: 0.20.0-image-api-rework.7 + specifier: 0.20.0-image-api-rework.8 + version: 0.20.0-image-api-rework.8 prettier: specifier: ^3.2.5 version: 3.3.3 @@ -1167,8 +1167,8 @@ packages: resolution: {integrity: sha512-eTIzlVOSUR+JxdDFepEYcBMtZ9Qqdef+rnzWdRZuMbOywu5tO2w2N7rqjoANZ5k9vywhL6Br1VRjUIgTQx4E8w==} engines: {node: '>=6'} - lemmy-js-client@0.20.0-image-api-rework.7: - resolution: {integrity: sha512-zZVOu8l9ctwEof71/DDkg/pwwBercLJIJja7KT1WxYS2pG6Hng9bD0+VDesbdvacCltFDbbEvk8pgJs3ES4OIQ==} + lemmy-js-client@0.20.0-image-api-rework.8: + resolution: {integrity: sha512-Ns/ayfCSm2lHbdAU1tGIZSx6kJ2ZeS7UiXlPuH0IzHQSi8Yuyzj3srDCyHpE6Td3pmXbQlt9N1ziPE4KeRJ3CA==} leven@3.1.0: resolution: {integrity: sha512-qsda+H8jTaUaN/x5vzW2rzc+8Rw4TAQ/4KjB46IwK5VH+IlVeeeje/EoZRpiXvIqjFgK84QffqPztGI3VBLG1A==} @@ -3077,7 +3077,7 @@ snapshots: kleur@3.0.3: {} - lemmy-js-client@0.20.0-image-api-rework.7: {} + lemmy-js-client@0.20.0-image-api-rework.8: {} leven@3.1.0: {} diff --git a/crates/routes/src/images/delete.rs b/crates/routes/src/images/delete.rs new file mode 100644 index 0000000000..82f48b736d --- /dev/null +++ b/crates/routes/src/images/delete.rs @@ -0,0 +1,142 @@ +use super::utils::{delete_old_image, PICTRS_CLIENT}; +use actix_web::web::*; +use lemmy_api_common::{ + context::LemmyContext, + image::{CommunityIdQuery, DeleteImageParams}, + utils::{is_admin, is_mod_or_admin}, + SuccessResponse, +}; +use lemmy_db_schema::{ + source::{ + community::{Community, CommunityUpdateForm}, + images::LocalImage, + person::{Person, PersonUpdateForm}, + site::{Site, SiteUpdateForm}, + }, + traits::Crud, +}; +use lemmy_db_views::structs::LocalUserView; +use lemmy_utils::error::LemmyResult; + +pub async fn delete_site_icon( + context: Data, + local_user_view: LocalUserView, +) -> LemmyResult> { + let site = Site::read_local(&mut context.pool()).await?; + is_admin(&local_user_view)?; + + delete_old_image(&site.icon, &context).await?; + + let form = SiteUpdateForm { + icon: Some(None), + ..Default::default() + }; + Site::update(&mut context.pool(), site.id, &form).await?; + + Ok(Json(SuccessResponse::default())) +} +pub async fn delete_site_banner( + context: Data, + local_user_view: LocalUserView, +) -> LemmyResult> { + let site = Site::read_local(&mut context.pool()).await?; + is_admin(&local_user_view)?; + + delete_old_image(&site.banner, &context).await?; + + let form = SiteUpdateForm { + banner: Some(None), + ..Default::default() + }; + Site::update(&mut context.pool(), site.id, &form).await?; + + Ok(Json(SuccessResponse::default())) +} + +pub async fn delete_community_icon( + data: Json, + context: Data, + local_user_view: LocalUserView, +) -> LemmyResult> { + let community = Community::read(&mut context.pool(), data.id).await?; + is_mod_or_admin(&mut context.pool(), &local_user_view.person, community.id).await?; + + delete_old_image(&community.icon, &context).await?; + + let form = CommunityUpdateForm { + icon: Some(None), + ..Default::default() + }; + Community::update(&mut context.pool(), community.id, &form).await?; + + Ok(Json(SuccessResponse::default())) +} + +pub async fn delete_community_banner( + data: Json, + context: Data, + local_user_view: LocalUserView, +) -> LemmyResult> { + let community = Community::read(&mut context.pool(), data.id).await?; + is_mod_or_admin(&mut context.pool(), &local_user_view.person, community.id).await?; + + delete_old_image(&community.icon, &context).await?; + + let form = CommunityUpdateForm { + icon: Some(None), + ..Default::default() + }; + Community::update(&mut context.pool(), community.id, &form).await?; + + Ok(Json(SuccessResponse::default())) +} + +pub async fn delete_user_avatar( + context: Data, + local_user_view: LocalUserView, +) -> LemmyResult> { + delete_old_image(&local_user_view.person.avatar, &context).await?; + + let form = PersonUpdateForm { + avatar: Some(None), + ..Default::default() + }; + Person::update(&mut context.pool(), local_user_view.person.id, &form).await?; + + Ok(Json(SuccessResponse::default())) +} + +pub async fn delete_user_banner( + context: Data, + local_user_view: LocalUserView, +) -> LemmyResult> { + delete_old_image(&local_user_view.person.banner, &context).await?; + + let form = PersonUpdateForm { + banner: Some(None), + ..Default::default() + }; + Person::update(&mut context.pool(), local_user_view.person.id, &form).await?; + + Ok(Json(SuccessResponse::default())) +} + +// TODO: get rid of delete tokens and allow deletion by admin or uploader +pub async fn delete_image( + data: Json, + context: Data, + // require login + _local_user_view: LocalUserView, +) -> LemmyResult> { + let pictrs_config = context.settings().pictrs()?; + let url = format!( + "{}image/delete/{}/{}", + pictrs_config.url, &data.token, &data.filename + ); + + PICTRS_CLIENT.delete(url).send().await?.error_for_status()?; + + LocalImage::delete_by_alias(&mut context.pool(), &data.filename).await?; + + Ok(Json(SuccessResponse::default())) +} diff --git a/crates/routes/src/images/mod.rs b/crates/routes/src/images/mod.rs index 1d3260e448..f4297e17e5 100644 --- a/crates/routes/src/images/mod.rs +++ b/crates/routes/src/images/mod.rs @@ -1,42 +1,13 @@ use actix_web::web::*; -use lemmy_api_common::{context::LemmyContext, image::DeleteImageParams, SuccessResponse}; -use lemmy_db_schema::source::images::LocalImage; -use lemmy_db_views::structs::LocalUserView; +use lemmy_api_common::{context::LemmyContext, SuccessResponse}; use lemmy_utils::error::LemmyResult; use utils::PICTRS_CLIENT; +pub mod delete; pub mod download; pub mod upload; mod utils; -pub async fn delete_community_icon( - data: Json, - context: Data, - local_user_view: LocalUserView, -) -> LemmyResult> { - todo!() -} - -// TODO: get rid of delete tokens and allow deletion by admin or uploader -pub async fn delete_image( - data: Json, - context: Data, - // require login - _local_user_view: LocalUserView, -) -> LemmyResult> { - let pictrs_config = context.settings().pictrs()?; - let url = format!( - "{}image/delete/{}/{}", - pictrs_config.url, &data.token, &data.filename - ); - - PICTRS_CLIENT.delete(url).send().await?.error_for_status()?; - - LocalImage::delete_by_alias(&mut context.pool(), &data.filename).await?; - - Ok(Json(SuccessResponse::default())) -} - pub async fn pictrs_health(context: Data) -> LemmyResult> { let pictrs_config = context.settings().pictrs()?; let url = format!("{}healthz", pictrs_config.url); diff --git a/src/api_routes_v3.rs b/src/api_routes_v3.rs index 36d7ad69a5..817e3f6a16 100644 --- a/src/api_routes_v3.rs +++ b/src/api_routes_v3.rs @@ -124,7 +124,7 @@ use lemmy_apub::api::{ user_settings_backup::{export_settings, import_settings}, }; use lemmy_routes::images::{ - delete_image, + delete::delete_image, download::{get_image, image_proxy}, pictrs_health, upload::upload_image, diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index 3572f8ebff..8cbe11c19d 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -150,7 +150,15 @@ use lemmy_apub::api::{ user_settings_backup::{export_settings, import_settings}, }; use lemmy_routes::images::{ - delete_image, + delete::{ + delete_community_banner, + delete_community_icon, + delete_image, + delete_site_banner, + delete_site_icon, + delete_user_avatar, + delete_user_banner, + }, download::{get_image, image_proxy}, pictrs_health, upload::{ @@ -176,7 +184,9 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route("", post().to(create_site)) .route("", put().to(update_site)) .route("/icon", post().to(upload_site_icon)) - .route("/banner", post().to(upload_site_banner)), + .route("/icon", delete().to(delete_site_icon)) + .route("/banner", post().to(upload_site_banner)) + .route("/banner", delete().to(delete_site_banner)), ) .route("/modlog", get().to(get_mod_log)) .service( @@ -209,6 +219,7 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route("/icon", post().to(upload_community_icon)) .route("/icon", delete().to(delete_community_icon)) .route("/banner", post().to(upload_community_banner)) + .route("/banner", delete().to(delete_community_banner)) .service( scope("/pending_follows") .route("/count", get().to(get_pending_follows_count)) @@ -327,7 +338,9 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route("/list_logins", get().to(list_logins)) .route("/validate_auth", get().to(validate_auth)) .route("/avatar", post().to(upload_user_avatar)) + .route("/avatar", delete().to(delete_user_avatar)) .route("/banner", post().to(upload_user_banner)) + .route("/banner", delete().to(delete_user_banner)) .service( scope("/block") .route("/person", post().to(user_block_person)) From 216fca2fe52295f557ff4d9c92558f5e191702e6 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Thu, 9 Jan 2025 15:31:26 +0100 Subject: [PATCH 19/51] change comment --- src/api_routes_v3.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/api_routes_v3.rs b/src/api_routes_v3.rs index 817e3f6a16..574646309a 100644 --- a/src/api_routes_v3.rs +++ b/src/api_routes_v3.rs @@ -132,8 +132,7 @@ use lemmy_routes::images::{ use lemmy_utils::rate_limit::RateLimitCell; // Deprecated, use api v4 instead. -// When removing api v3, we also need to rewrite all links in database with -// `/api/v3/image_proxy` to use `/api/v4/image/proxy` instead. +// When removing api v3, make sure to keep `/api/v3/image_proxy` as it is still used in old posts. pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { cfg .service( From 4d51f5311e831927e268cd872d115f3ebf8602ee Mon Sep 17 00:00:00 2001 From: Nutomic Date: Thu, 9 Jan 2025 15:35:43 +0100 Subject: [PATCH 20/51] optimization Co-authored-by: dullbananas --- crates/routes/src/images/download.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/routes/src/images/download.rs b/crates/routes/src/images/download.rs index b54b72f1ca..2b85813900 100644 --- a/crates/routes/src/images/download.rs +++ b/crates/routes/src/images/download.rs @@ -24,9 +24,11 @@ pub async fn get_image( local_user_view: Option, ) -> LemmyResult { // block access to images if instance is private and unauthorized, public - let local_site = LocalSite::read(&mut context.pool()).await?; - if local_site.private_instance && local_user_view.is_none() { - return Ok(HttpResponse::Unauthorized().finish()); + if local_user_view.is_none() { + let local_site = LocalSite::read(&mut context.pool()).await?; + if local_site.private_instance { + return Ok(HttpResponse::Unauthorized().finish()); + } } let name = &filename.into_inner(); From 96cf01751bf4296b8f495a308cf83cafce6b7d49 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Thu, 9 Jan 2025 15:40:24 +0100 Subject: [PATCH 21/51] move fn --- crates/routes/src/images/download.rs | 12 ++++++++++-- crates/routes/src/images/utils.rs | 7 ------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/crates/routes/src/images/download.rs b/crates/routes/src/images/download.rs index 2b85813900..803dbe19e9 100644 --- a/crates/routes/src/images/download.rs +++ b/crates/routes/src/images/download.rs @@ -1,4 +1,4 @@ -use super::utils::{adapt_request, convert_header, file_type}; +use super::utils::{adapt_request, convert_header}; use actix_web::{ body::{BodyStream, BoxBody}, http::StatusCode, @@ -23,7 +23,7 @@ pub async fn get_image( context: Data, local_user_view: Option, ) -> LemmyResult { - // block access to images if instance is private and unauthorized, public + // block access to images if instance is private if local_user_view.is_none() { let local_site = LocalSite::read(&mut context.pool()).await?; if local_site.private_instance { @@ -48,6 +48,7 @@ pub async fn get_image( do_get_image(processed_url, req).await } + pub async fn image_proxy( Query(params): Query, req: HttpRequest, @@ -113,3 +114,10 @@ pub(super) async fn do_get_image(url: String, req: HttpRequest) -> LemmyResult, name: &str) -> String { + file_type + .clone() + .unwrap_or_else(|| name.split('.').last().unwrap_or("jpg").to_string()) +} diff --git a/crates/routes/src/images/utils.rs b/crates/routes/src/images/utils.rs index 040734a8bc..18e115a8d1 100644 --- a/crates/routes/src/images/utils.rs +++ b/crates/routes/src/images/utils.rs @@ -122,10 +122,3 @@ pub(super) async fn delete_old_image( } Ok(()) } - -/// Take file type from param, name, or use jpg if nothing is given -pub(super) fn file_type(file_type: Option, name: &str) -> String { - file_type - .clone() - .unwrap_or_else(|| name.split('.').last().unwrap_or("jpg").to_string()) -} From f04da5eb539a36cd901be4bf3bd43b0d017ef3e4 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Thu, 9 Jan 2025 16:41:44 +0100 Subject: [PATCH 22/51] 1024px banner --- config/defaults.hjson | 10 +++------- crates/utils/src/settings/structs.rs | 10 +++------- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/config/defaults.hjson b/config/defaults.hjson index c13b709075..56565b213e 100644 --- a/config/defaults.hjson +++ b/config/defaults.hjson @@ -66,13 +66,9 @@ max_thumbnail_size: 512 # Maximum size for user avatar, community icon and site icon. max_avatar_size: 512 - # Maximum size for user, community and site banner. - # - # TODO: Unfortunately pictrs can only resize images to fit in a*a square, no rectangle. - # Otherwise we have to use crop, or use max_width/max_height which throws error - # if image is larger. - # TODO: whats a sensible default here? - max_banner_size: 512 + # Maximum size for user, community and site banner. Larger images are downscaled to fit + # into a square of this size. + max_banner_size: 1024 # Prevent users from uploading images for posts or embedding in markdown. Avatars, icons and # banners can still be uploaded. image_upload_disabled: false diff --git a/crates/utils/src/settings/structs.rs b/crates/utils/src/settings/structs.rs index 153c4567e1..1b0bc60f0c 100644 --- a/crates/utils/src/settings/structs.rs +++ b/crates/utils/src/settings/structs.rs @@ -109,13 +109,9 @@ pub struct PictrsConfig { #[default(512)] pub max_avatar_size: u32, - /// Maximum size for user, community and site banner. - /// - /// TODO: Unfortunately pictrs can only resize images to fit in a*a square, no rectangle. - /// Otherwise we have to use crop, or use max_width/max_height which throws error - /// if image is larger. - /// TODO: whats a sensible default here? - #[default(512)] + /// Maximum size for user, community and site banner. Larger images are downscaled to fit + /// into a square of this size. + #[default(1024)] pub max_banner_size: u32, /// Prevent users from uploading images for posts or embedding in markdown. Avatars, icons and From 7cbbb9aa334c7f94b14413da6634fd4b1bbbd400 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 09:51:48 +0100 Subject: [PATCH 23/51] dont use static client --- crates/routes/src/images/delete.rs | 6 ++++-- crates/routes/src/images/download.rs | 19 ++++++++++++----- crates/routes/src/images/mod.rs | 9 +++++--- crates/routes/src/images/upload.rs | 25 +++++++++++++++------- crates/routes/src/images/utils.rs | 32 ++++++++-------------------- src/lib.rs | 6 ++++++ 6 files changed, 56 insertions(+), 41 deletions(-) diff --git a/crates/routes/src/images/delete.rs b/crates/routes/src/images/delete.rs index 82f48b736d..5499027a83 100644 --- a/crates/routes/src/images/delete.rs +++ b/crates/routes/src/images/delete.rs @@ -1,4 +1,4 @@ -use super::utils::{delete_old_image, PICTRS_CLIENT}; +use super::utils::delete_old_image; use actix_web::web::*; use lemmy_api_common::{ context::LemmyContext, @@ -17,6 +17,7 @@ use lemmy_db_schema::{ }; use lemmy_db_views::structs::LocalUserView; use lemmy_utils::error::LemmyResult; +use reqwest_middleware::ClientWithMiddleware; pub async fn delete_site_icon( context: Data, @@ -125,6 +126,7 @@ pub async fn delete_user_banner( pub async fn delete_image( data: Json, context: Data, + client: Data, // require login _local_user_view: LocalUserView, ) -> LemmyResult> { @@ -134,7 +136,7 @@ pub async fn delete_image( pictrs_config.url, &data.token, &data.filename ); - PICTRS_CLIENT.delete(url).send().await?.error_for_status()?; + client.delete(url).send().await?.error_for_status()?; LocalImage::delete_by_alias(&mut context.pool(), &data.filename).await?; diff --git a/crates/routes/src/images/download.rs b/crates/routes/src/images/download.rs index 803dbe19e9..0c6f97eb51 100644 --- a/crates/routes/src/images/download.rs +++ b/crates/routes/src/images/download.rs @@ -14,14 +14,16 @@ use lemmy_api_common::{ use lemmy_db_schema::source::{images::RemoteImage, local_site::LocalSite}; use lemmy_db_views::structs::LocalUserView; use lemmy_utils::error::LemmyResult; +use reqwest_middleware::ClientWithMiddleware; use url::Url; pub async fn get_image( filename: Path, Query(params): Query, req: HttpRequest, - context: Data, local_user_view: Option, + context: Data, + client: Data, ) -> LemmyResult { // block access to images if instance is private if local_user_view.is_none() { @@ -46,12 +48,13 @@ pub async fn get_image( url }; - do_get_image(processed_url, req).await + do_get_image(processed_url, req, client).await } pub async fn image_proxy( Query(params): Query, req: HttpRequest, + client: Data, context: Data, ) -> LemmyResult, HttpResponse>> { let url = Url::parse(¶ms.url)?; @@ -85,12 +88,18 @@ pub async fn image_proxy( Ok(Either::Left(Redirect::to(url.to_string()).respond_to(&req))) } else { // Proxy the image data through Lemmy - Ok(Either::Right(do_get_image(processed_url, req).await?)) + Ok(Either::Right( + do_get_image(processed_url, req, client).await?, + )) } } -pub(super) async fn do_get_image(url: String, req: HttpRequest) -> LemmyResult { - let mut client_req = adapt_request(&req, url); +pub(super) async fn do_get_image( + url: String, + req: HttpRequest, + client: Data, +) -> LemmyResult { + let mut client_req = adapt_request(&req, url, client); if let Some(addr) = req.head().peer_addr { client_req = client_req.header("X-Forwarded-For", addr.to_string()); diff --git a/crates/routes/src/images/mod.rs b/crates/routes/src/images/mod.rs index f4297e17e5..57dd290831 100644 --- a/crates/routes/src/images/mod.rs +++ b/crates/routes/src/images/mod.rs @@ -1,18 +1,21 @@ use actix_web::web::*; use lemmy_api_common::{context::LemmyContext, SuccessResponse}; use lemmy_utils::error::LemmyResult; -use utils::PICTRS_CLIENT; +use reqwest_middleware::ClientWithMiddleware; pub mod delete; pub mod download; pub mod upload; mod utils; -pub async fn pictrs_health(context: Data) -> LemmyResult> { +pub async fn pictrs_health( + client: Data, + context: Data, +) -> LemmyResult> { let pictrs_config = context.settings().pictrs()?; let url = format!("{}healthz", pictrs_config.url); - PICTRS_CLIENT.get(url).send().await?.error_for_status()?; + client.get(url).send().await?.error_for_status()?; Ok(Json(SuccessResponse::default())) } diff --git a/crates/routes/src/images/upload.rs b/crates/routes/src/images/upload.rs index 9fd20b3ef0..8b7008d0d4 100644 --- a/crates/routes/src/images/upload.rs +++ b/crates/routes/src/images/upload.rs @@ -20,6 +20,7 @@ use lemmy_db_schema::{ use lemmy_db_views::structs::LocalUserView; use lemmy_utils::error::LemmyResult; use reqwest::Body; +use reqwest_middleware::ClientWithMiddleware; use std::time::Duration; use UploadType::*; @@ -33,6 +34,7 @@ pub async fn upload_image( req: HttpRequest, body: Payload, local_user_view: LocalUserView, + client: Data, context: Data, ) -> LemmyResult> { if context.settings().pictrs()?.image_upload_disabled { @@ -40,7 +42,7 @@ pub async fn upload_image( } Ok(Json( - do_upload_image(req, body, Other, &local_user_view, &context).await?, + do_upload_image(req, body, Other, &local_user_view, client, &context).await?, )) } @@ -48,9 +50,10 @@ pub async fn upload_user_avatar( req: HttpRequest, body: Payload, local_user_view: LocalUserView, + client: Data, context: Data, ) -> LemmyResult> { - let image = do_upload_image(req, body, Avatar, &local_user_view, &context).await?; + let image = do_upload_image(req, body, Avatar, &local_user_view, client, &context).await?; delete_old_image(&local_user_view.person.avatar, &context).await?; let form = PersonUpdateForm { @@ -66,9 +69,10 @@ pub async fn upload_user_banner( req: HttpRequest, body: Payload, local_user_view: LocalUserView, + client: Data, context: Data, ) -> LemmyResult> { - let image = do_upload_image(req, body, Banner, &local_user_view, &context).await?; + let image = do_upload_image(req, body, Banner, &local_user_view, client, &context).await?; delete_old_image(&local_user_view.person.banner, &context).await?; let form = PersonUpdateForm { @@ -85,12 +89,13 @@ pub async fn upload_community_icon( query: Query, body: Payload, local_user_view: LocalUserView, + client: Data, context: Data, ) -> LemmyResult> { let community: Community = Community::read(&mut context.pool(), query.id).await?; is_mod_or_admin(&mut context.pool(), &local_user_view.person, community.id).await?; - let image = do_upload_image(req, body, Avatar, &local_user_view, &context).await?; + let image = do_upload_image(req, body, Avatar, &local_user_view, client, &context).await?; delete_old_image(&community.icon, &context).await?; let form = CommunityUpdateForm { @@ -107,12 +112,13 @@ pub async fn upload_community_banner( query: Query, body: Payload, local_user_view: LocalUserView, + client: Data, context: Data, ) -> LemmyResult> { let community: Community = Community::read(&mut context.pool(), query.id).await?; is_mod_or_admin(&mut context.pool(), &local_user_view.person, community.id).await?; - let image = do_upload_image(req, body, Banner, &local_user_view, &context).await?; + let image = do_upload_image(req, body, Banner, &local_user_view, client, &context).await?; delete_old_image(&community.banner, &context).await?; let form = CommunityUpdateForm { @@ -128,12 +134,13 @@ pub async fn upload_site_icon( req: HttpRequest, body: Payload, local_user_view: LocalUserView, + client: Data, context: Data, ) -> LemmyResult> { is_admin(&local_user_view)?; let site = Site::read_local(&mut context.pool()).await?; - let image = do_upload_image(req, body, Avatar, &local_user_view, &context).await?; + let image = do_upload_image(req, body, Avatar, &local_user_view, client, &context).await?; delete_old_image(&site.icon, &context).await?; let form = SiteUpdateForm { @@ -149,12 +156,13 @@ pub async fn upload_site_banner( req: HttpRequest, body: Payload, local_user_view: LocalUserView, + client: Data, context: Data, ) -> LemmyResult> { is_admin(&local_user_view)?; let site = Site::read_local(&mut context.pool()).await?; - let image = do_upload_image(req, body, Banner, &local_user_view, &context).await?; + let image = do_upload_image(req, body, Banner, &local_user_view, client, &context).await?; delete_old_image(&site.banner, &context).await?; let form = SiteUpdateForm { @@ -171,12 +179,13 @@ pub async fn do_upload_image( body: Payload, upload_type: UploadType, local_user_view: &LocalUserView, + client: Data, context: &Data, ) -> LemmyResult { let pictrs = context.settings().pictrs()?; let image_url = format!("{}image", pictrs.url); - let mut client_req = adapt_request(&req, image_url); + let mut client_req = adapt_request(&req, image_url, client); client_req = match upload_type { Avatar => { diff --git a/crates/routes/src/images/utils.rs b/crates/routes/src/images/utils.rs index 18e115a8d1..064ee0311e 100644 --- a/crates/routes/src/images/utils.rs +++ b/crates/routes/src/images/utils.rs @@ -8,34 +8,20 @@ use actix_web::{ }; use futures::stream::{Stream, StreamExt}; use http::HeaderValue; -use lemmy_api_common::{ - context::LemmyContext, - request::{client_builder, delete_image_from_pictrs}, -}; +use lemmy_api_common::{context::LemmyContext, request::delete_image_from_pictrs}; use lemmy_db_schema::{newtypes::DbUrl, source::images::LocalImage}; -use lemmy_utils::{error::LemmyResult, settings::SETTINGS, REQWEST_TIMEOUT}; -use reqwest_middleware::{ClientBuilder, ClientWithMiddleware, RequestBuilder}; -use reqwest_tracing::TracingMiddleware; -use std::sync::LazyLock; - -// Pictrs cannot use proxy -#[allow(clippy::expect_used)] -pub(super) static PICTRS_CLIENT: LazyLock = LazyLock::new(|| { - ClientBuilder::new( - client_builder(&SETTINGS) - .no_proxy() - .build() - .expect("build pictrs client"), - ) - .with(TracingMiddleware::default()) - .build() -}); +use lemmy_utils::{error::LemmyResult, REQWEST_TIMEOUT}; +use reqwest_middleware::{ClientWithMiddleware, RequestBuilder}; -pub(super) fn adapt_request(request: &HttpRequest, url: String) -> RequestBuilder { +pub(super) fn adapt_request( + request: &HttpRequest, + url: String, + client: Data, +) -> RequestBuilder { // remove accept-encoding header so that pictrs doesn't compress the response const INVALID_HEADERS: &[HeaderName] = &[ACCEPT_ENCODING, HOST]; - let client_request = PICTRS_CLIENT + let client_request = client .request(convert_method(request.method()), url) .timeout(REQWEST_TIMEOUT); diff --git a/src/lib.rs b/src/lib.rs index 075dba0906..aa5db99a71 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -330,6 +330,11 @@ fn create_http_server( .build() .map_err(|e| LemmyErrorType::Unknown(format!("Should always be buildable: {e}")))?; + // Pictrs cannot use proxy + let pictrs_client = ClientBuilder::new(client_builder(&SETTINGS).no_proxy().build()?) + .with(TracingMiddleware::default()) + .build(); + // Create Http server let bind = (settings.bind, settings.port); let server = HttpServer::new(move || { @@ -350,6 +355,7 @@ fn create_http_server( .wrap(ErrorHandlers::new().default_handler(jsonify_plain_text_errors)) .app_data(Data::new(context.clone())) .app_data(Data::new(rate_limit_cell.clone())) + .app_data(Data::new(pictrs_client)) .wrap(FederationMiddleware::new(federation_config.clone())) .wrap(SessionMiddleware::new(context.clone())) .wrap(Condition::new( From 83c3304e9d5e23bbd9d282aed0c5f0c3306d8221 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 10:11:13 +0100 Subject: [PATCH 24/51] fix api tests --- api_tests/src/user.spec.ts | 17 ++++++++++++++--- src/lib.rs | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/api_tests/src/user.spec.ts b/api_tests/src/user.spec.ts index da9237598a..5516772629 100644 --- a/api_tests/src/user.spec.ts +++ b/api_tests/src/user.spec.ts @@ -191,20 +191,23 @@ test("Set a new avatar, old avatar is deleted", async () => { const upload_form1: UploadImage = { image: Buffer.from("test1"), }; - await alpha.userUploadAvatar(upload_form1); + await alpha.uploadUserAvatar(upload_form1); const listMediaRes1 = await alphaImage.listMedia(); expect(listMediaRes1.images.length).toBe(1); + let my_user1 = await alpha.getMyUser(); + expect(my_user1.local_user_view.person.avatar).toBeDefined(); + const upload_form2: UploadImage = { image: Buffer.from("test2"), }; - await alpha.userUploadAvatar(upload_form2); + await alpha.uploadUserAvatar(upload_form2); // make sure only the new avatar is kept const listMediaRes2 = await alphaImage.listMedia(); expect(listMediaRes2.images.length).toBe(1); // Upload that same form2 avatar, make sure it isn't replaced / deleted - await alpha.userUploadAvatar(upload_form2); + await alpha.uploadUserAvatar(upload_form2); // make sure only the new avatar is kept const listMediaRes3 = await alphaImage.listMedia(); expect(listMediaRes3.images.length).toBe(1); @@ -212,4 +215,12 @@ test("Set a new avatar, old avatar is deleted", async () => { // make sure only the new avatar is kept const listMediaRes4 = await alphaImage.listMedia(); expect(listMediaRes4.images.length).toBe(1); + + // delete the avatar + await alpha.deleteUserAvatar(); + // make sure only the new avatar is kept + const listMediaRes5 = await alphaImage.listMedia(); + expect(listMediaRes5.images.length).toBe(0); + let my_user2 = await alpha.getMyUser(); + expect(my_user2.local_user_view.person.avatar).toBeUndefined(); }); diff --git a/src/lib.rs b/src/lib.rs index aa5db99a71..065a035136 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -355,7 +355,7 @@ fn create_http_server( .wrap(ErrorHandlers::new().default_handler(jsonify_plain_text_errors)) .app_data(Data::new(context.clone())) .app_data(Data::new(rate_limit_cell.clone())) - .app_data(Data::new(pictrs_client)) + .app_data(Data::new(pictrs_client.clone())) .wrap(FederationMiddleware::new(federation_config.clone())) .wrap(SessionMiddleware::new(context.clone())) .wrap(Condition::new( From f040f9146cd14de281ad89a5b1d2badcbeae97b0 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 10:13:42 +0100 Subject: [PATCH 25/51] shear --- Cargo.lock | 1 - crates/routes/Cargo.toml | 1 - 2 files changed, 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6caff2429f..7afc8ec0d2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2789,7 +2789,6 @@ dependencies = [ "lemmy_utils", "reqwest 0.12.8", "reqwest-middleware", - "reqwest-tracing", "rss", "serde", "tokio", diff --git a/crates/routes/Cargo.toml b/crates/routes/Cargo.toml index 3bd85d0e5f..91c3ed6830 100644 --- a/crates/routes/Cargo.toml +++ b/crates/routes/Cargo.toml @@ -33,5 +33,4 @@ url = { workspace = true } tracing = { workspace = true } tokio = { workspace = true } http.workspace = true -reqwest-tracing = { workspace = true } rss = "2.0.10" From 6123659711e8cffa31a07d52d5fe4d1ab7ae520f Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 11:08:24 +0100 Subject: [PATCH 26/51] proxy pictrs in request.rs (fixes #5270) --- crates/api_common/src/context.rs | 16 +++++++++++++++- crates/api_common/src/request.rs | 13 ++++++------- crates/routes/src/images/delete.rs | 9 ++++++--- crates/routes/src/images/download.rs | 11 ++++------- crates/routes/src/images/mod.rs | 13 +++++++------ crates/routes/src/images/upload.rs | 25 ++++++++----------------- crates/routes/src/images/utils.rs | 7 ++++--- src/lib.rs | 10 ++++------ 8 files changed, 54 insertions(+), 50 deletions(-) diff --git a/crates/api_common/src/context.rs b/crates/api_common/src/context.rs index b578914d1f..1c423d156a 100644 --- a/crates/api_common/src/context.rs +++ b/crates/api_common/src/context.rs @@ -15,6 +15,9 @@ use std::sync::Arc; pub struct LemmyContext { pool: ActualDbPool, client: Arc, + /// Pictrs requests must bypass proxy. Unfortunately no_proxy can only be set on ClientBuilder + /// and not on RequestBuilder, so we need a separate client here. + pictrs_client: Arc, secret: Arc, rate_limit_cell: RateLimitCell, } @@ -23,12 +26,14 @@ impl LemmyContext { pub fn create( pool: ActualDbPool, client: ClientWithMiddleware, + pictrs_client: ClientWithMiddleware, secret: Secret, rate_limit_cell: RateLimitCell, ) -> LemmyContext { LemmyContext { pool, client: Arc::new(client), + pictrs_client: Arc::new(pictrs_client), secret: Arc::new(secret), rate_limit_cell, } @@ -42,6 +47,9 @@ impl LemmyContext { pub fn client(&self) -> &ClientWithMiddleware { &self.client } + pub fn pictrs_client(&self) -> &ClientWithMiddleware { + &self.pictrs_client + } pub fn settings(&self) -> &'static Settings { &SETTINGS } @@ -70,7 +78,13 @@ impl LemmyContext { let rate_limit_cell = RateLimitCell::with_test_config(); - let context = LemmyContext::create(pool, client, secret, rate_limit_cell.clone()); + let context = LemmyContext::create( + pool, + client.clone(), + client, + secret, + rate_limit_cell.clone(), + ); FederationConfig::builder() .domain(context.settings().hostname.clone()) diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index 756d9517f3..5428c4c4b4 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -319,7 +319,7 @@ struct PictrsPurgeResponse { /// - It might not be an image /// - Pictrs might not be set up pub async fn purge_image_from_pictrs(image_url: &Url, context: &LemmyContext) -> LemmyResult<()> { - is_image_content_type(context.client(), image_url).await?; + is_image_content_type(context.pictrs_client(), image_url).await?; let alias = image_url .path_segments() @@ -334,7 +334,7 @@ pub async fn purge_image_from_pictrs(image_url: &Url, context: &LemmyContext) -> .api_key .ok_or(LemmyErrorType::PictrsApiKeyNotProvided)?; let response = context - .client() + .pictrs_client() .post(&purge_url) .timeout(REQWEST_TIMEOUT) .header("x-api-token", pictrs_api_key) @@ -361,7 +361,7 @@ pub async fn delete_image_from_pictrs( pictrs_config.url, &delete_token, &alias ); context - .client() + .pictrs_client() .delete(&url) .timeout(REQWEST_TIMEOUT) .send() @@ -384,7 +384,6 @@ async fn generate_pictrs_thumbnail(image_url: &Url, context: &LemmyContext) -> L }; // fetch remote non-pictrs images for persistent thumbnail link - // TODO: should limit size once supported by pictrs let fetch_url = format!( "{}image/download?url={}&resize={}", pictrs_config.url, @@ -393,7 +392,7 @@ async fn generate_pictrs_thumbnail(image_url: &Url, context: &LemmyContext) -> L ); let res = context - .client() + .pictrs_client() .get(&fetch_url) .timeout(REQWEST_TIMEOUT) .send() @@ -439,7 +438,7 @@ pub async fn fetch_pictrs_proxied_image_details( let proxy_url = format!("{pictrs_url}image/original?proxy={encoded_image_url}"); context - .client() + .pictrs_client() .get(&proxy_url) .timeout(REQWEST_TIMEOUT) .send() @@ -450,7 +449,7 @@ pub async fn fetch_pictrs_proxied_image_details( let details_url = format!("{pictrs_url}image/details/original?proxy={encoded_image_url}"); let res = context - .client() + .pictrs_client() .get(&details_url) .timeout(REQWEST_TIMEOUT) .send() diff --git a/crates/routes/src/images/delete.rs b/crates/routes/src/images/delete.rs index 5499027a83..b28c87c6c2 100644 --- a/crates/routes/src/images/delete.rs +++ b/crates/routes/src/images/delete.rs @@ -17,7 +17,6 @@ use lemmy_db_schema::{ }; use lemmy_db_views::structs::LocalUserView; use lemmy_utils::error::LemmyResult; -use reqwest_middleware::ClientWithMiddleware; pub async fn delete_site_icon( context: Data, @@ -126,7 +125,6 @@ pub async fn delete_user_banner( pub async fn delete_image( data: Json, context: Data, - client: Data, // require login _local_user_view: LocalUserView, ) -> LemmyResult> { @@ -136,7 +134,12 @@ pub async fn delete_image( pictrs_config.url, &data.token, &data.filename ); - client.delete(url).send().await?.error_for_status()?; + context + .pictrs_client() + .delete(url) + .send() + .await? + .error_for_status()?; LocalImage::delete_by_alias(&mut context.pool(), &data.filename).await?; diff --git a/crates/routes/src/images/download.rs b/crates/routes/src/images/download.rs index 0c6f97eb51..76f09a8d12 100644 --- a/crates/routes/src/images/download.rs +++ b/crates/routes/src/images/download.rs @@ -14,7 +14,6 @@ use lemmy_api_common::{ use lemmy_db_schema::source::{images::RemoteImage, local_site::LocalSite}; use lemmy_db_views::structs::LocalUserView; use lemmy_utils::error::LemmyResult; -use reqwest_middleware::ClientWithMiddleware; use url::Url; pub async fn get_image( @@ -23,7 +22,6 @@ pub async fn get_image( req: HttpRequest, local_user_view: Option, context: Data, - client: Data, ) -> LemmyResult { // block access to images if instance is private if local_user_view.is_none() { @@ -48,13 +46,12 @@ pub async fn get_image( url }; - do_get_image(processed_url, req, client).await + do_get_image(processed_url, req, &context).await } pub async fn image_proxy( Query(params): Query, req: HttpRequest, - client: Data, context: Data, ) -> LemmyResult, HttpResponse>> { let url = Url::parse(¶ms.url)?; @@ -89,7 +86,7 @@ pub async fn image_proxy( } else { // Proxy the image data through Lemmy Ok(Either::Right( - do_get_image(processed_url, req, client).await?, + do_get_image(processed_url, req, &context).await?, )) } } @@ -97,9 +94,9 @@ pub async fn image_proxy( pub(super) async fn do_get_image( url: String, req: HttpRequest, - client: Data, + context: &LemmyContext, ) -> LemmyResult { - let mut client_req = adapt_request(&req, url, client); + let mut client_req = adapt_request(&req, url, context); if let Some(addr) = req.head().peer_addr { client_req = client_req.header("X-Forwarded-For", addr.to_string()); diff --git a/crates/routes/src/images/mod.rs b/crates/routes/src/images/mod.rs index 57dd290831..aefe428312 100644 --- a/crates/routes/src/images/mod.rs +++ b/crates/routes/src/images/mod.rs @@ -1,21 +1,22 @@ use actix_web::web::*; use lemmy_api_common::{context::LemmyContext, SuccessResponse}; use lemmy_utils::error::LemmyResult; -use reqwest_middleware::ClientWithMiddleware; pub mod delete; pub mod download; pub mod upload; mod utils; -pub async fn pictrs_health( - client: Data, - context: Data, -) -> LemmyResult> { +pub async fn pictrs_health(context: Data) -> LemmyResult> { let pictrs_config = context.settings().pictrs()?; let url = format!("{}healthz", pictrs_config.url); - client.get(url).send().await?.error_for_status()?; + context + .pictrs_client() + .get(url) + .send() + .await? + .error_for_status()?; Ok(Json(SuccessResponse::default())) } diff --git a/crates/routes/src/images/upload.rs b/crates/routes/src/images/upload.rs index 8b7008d0d4..8f77b5a7a8 100644 --- a/crates/routes/src/images/upload.rs +++ b/crates/routes/src/images/upload.rs @@ -20,7 +20,6 @@ use lemmy_db_schema::{ use lemmy_db_views::structs::LocalUserView; use lemmy_utils::error::LemmyResult; use reqwest::Body; -use reqwest_middleware::ClientWithMiddleware; use std::time::Duration; use UploadType::*; @@ -34,7 +33,6 @@ pub async fn upload_image( req: HttpRequest, body: Payload, local_user_view: LocalUserView, - client: Data, context: Data, ) -> LemmyResult> { if context.settings().pictrs()?.image_upload_disabled { @@ -42,7 +40,7 @@ pub async fn upload_image( } Ok(Json( - do_upload_image(req, body, Other, &local_user_view, client, &context).await?, + do_upload_image(req, body, Other, &local_user_view, &context).await?, )) } @@ -50,10 +48,9 @@ pub async fn upload_user_avatar( req: HttpRequest, body: Payload, local_user_view: LocalUserView, - client: Data, context: Data, ) -> LemmyResult> { - let image = do_upload_image(req, body, Avatar, &local_user_view, client, &context).await?; + let image = do_upload_image(req, body, Avatar, &local_user_view, &context).await?; delete_old_image(&local_user_view.person.avatar, &context).await?; let form = PersonUpdateForm { @@ -69,10 +66,9 @@ pub async fn upload_user_banner( req: HttpRequest, body: Payload, local_user_view: LocalUserView, - client: Data, context: Data, ) -> LemmyResult> { - let image = do_upload_image(req, body, Banner, &local_user_view, client, &context).await?; + let image = do_upload_image(req, body, Banner, &local_user_view, &context).await?; delete_old_image(&local_user_view.person.banner, &context).await?; let form = PersonUpdateForm { @@ -89,13 +85,12 @@ pub async fn upload_community_icon( query: Query, body: Payload, local_user_view: LocalUserView, - client: Data, context: Data, ) -> LemmyResult> { let community: Community = Community::read(&mut context.pool(), query.id).await?; is_mod_or_admin(&mut context.pool(), &local_user_view.person, community.id).await?; - let image = do_upload_image(req, body, Avatar, &local_user_view, client, &context).await?; + let image = do_upload_image(req, body, Avatar, &local_user_view, &context).await?; delete_old_image(&community.icon, &context).await?; let form = CommunityUpdateForm { @@ -112,13 +107,12 @@ pub async fn upload_community_banner( query: Query, body: Payload, local_user_view: LocalUserView, - client: Data, context: Data, ) -> LemmyResult> { let community: Community = Community::read(&mut context.pool(), query.id).await?; is_mod_or_admin(&mut context.pool(), &local_user_view.person, community.id).await?; - let image = do_upload_image(req, body, Banner, &local_user_view, client, &context).await?; + let image = do_upload_image(req, body, Banner, &local_user_view, &context).await?; delete_old_image(&community.banner, &context).await?; let form = CommunityUpdateForm { @@ -134,13 +128,12 @@ pub async fn upload_site_icon( req: HttpRequest, body: Payload, local_user_view: LocalUserView, - client: Data, context: Data, ) -> LemmyResult> { is_admin(&local_user_view)?; let site = Site::read_local(&mut context.pool()).await?; - let image = do_upload_image(req, body, Avatar, &local_user_view, client, &context).await?; + let image = do_upload_image(req, body, Avatar, &local_user_view, &context).await?; delete_old_image(&site.icon, &context).await?; let form = SiteUpdateForm { @@ -156,13 +149,12 @@ pub async fn upload_site_banner( req: HttpRequest, body: Payload, local_user_view: LocalUserView, - client: Data, context: Data, ) -> LemmyResult> { is_admin(&local_user_view)?; let site = Site::read_local(&mut context.pool()).await?; - let image = do_upload_image(req, body, Banner, &local_user_view, client, &context).await?; + let image = do_upload_image(req, body, Banner, &local_user_view, &context).await?; delete_old_image(&site.banner, &context).await?; let form = SiteUpdateForm { @@ -179,13 +171,12 @@ pub async fn do_upload_image( body: Payload, upload_type: UploadType, local_user_view: &LocalUserView, - client: Data, context: &Data, ) -> LemmyResult { let pictrs = context.settings().pictrs()?; let image_url = format!("{}image", pictrs.url); - let mut client_req = adapt_request(&req, image_url, client); + let mut client_req = adapt_request(&req, image_url, &context); client_req = match upload_type { Avatar => { diff --git a/crates/routes/src/images/utils.rs b/crates/routes/src/images/utils.rs index 064ee0311e..80108360ca 100644 --- a/crates/routes/src/images/utils.rs +++ b/crates/routes/src/images/utils.rs @@ -11,17 +11,18 @@ use http::HeaderValue; use lemmy_api_common::{context::LemmyContext, request::delete_image_from_pictrs}; use lemmy_db_schema::{newtypes::DbUrl, source::images::LocalImage}; use lemmy_utils::{error::LemmyResult, REQWEST_TIMEOUT}; -use reqwest_middleware::{ClientWithMiddleware, RequestBuilder}; +use reqwest_middleware::RequestBuilder; pub(super) fn adapt_request( request: &HttpRequest, url: String, - client: Data, + context: &LemmyContext, ) -> RequestBuilder { // remove accept-encoding header so that pictrs doesn't compress the response const INVALID_HEADERS: &[HeaderName] = &[ACCEPT_ENCODING, HOST]; - let client_request = client + let client_request = context + .pictrs_client() .request(convert_method(request.method()), url) .timeout(REQWEST_TIMEOUT); diff --git a/src/lib.rs b/src/lib.rs index 065a035136..bd84d02640 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -195,9 +195,13 @@ pub async fn start_lemmy_server(args: CmdArgs) -> LemmyResult<()> { let client = ClientBuilder::new(client_builder(&SETTINGS).build()?) .with(TracingMiddleware::default()) .build(); + let pictrs_client = ClientBuilder::new(client_builder(&SETTINGS).no_proxy().build()?) + .with(TracingMiddleware::default()) + .build(); let context = LemmyContext::create( pool.clone(), client.clone(), + pictrs_client, secret.clone(), rate_limit_cell.clone(), ); @@ -330,11 +334,6 @@ fn create_http_server( .build() .map_err(|e| LemmyErrorType::Unknown(format!("Should always be buildable: {e}")))?; - // Pictrs cannot use proxy - let pictrs_client = ClientBuilder::new(client_builder(&SETTINGS).no_proxy().build()?) - .with(TracingMiddleware::default()) - .build(); - // Create Http server let bind = (settings.bind, settings.port); let server = HttpServer::new(move || { @@ -355,7 +354,6 @@ fn create_http_server( .wrap(ErrorHandlers::new().default_handler(jsonify_plain_text_errors)) .app_data(Data::new(context.clone())) .app_data(Data::new(rate_limit_cell.clone())) - .app_data(Data::new(pictrs_client.clone())) .wrap(FederationMiddleware::new(federation_config.clone())) .wrap(SessionMiddleware::new(context.clone())) .wrap(Condition::new( From ec74669c5119ee799b99139fd23d8328577ea7c1 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 11:24:10 +0100 Subject: [PATCH 27/51] clippy --- crates/api_common/src/claims.rs | 17 +++-------------- crates/routes/src/images/upload.rs | 2 +- 2 files changed, 4 insertions(+), 15 deletions(-) diff --git a/crates/api_common/src/claims.rs b/crates/api_common/src/claims.rs index 759673f4bd..5063ea8fe1 100644 --- a/crates/api_common/src/claims.rs +++ b/crates/api_common/src/claims.rs @@ -78,29 +78,18 @@ mod tests { instance::Instance, local_user::{LocalUser, LocalUserInsertForm}, person::{Person, PersonInsertForm}, - secret::Secret, }, traits::Crud, - utils::build_db_pool_for_tests, }; - use lemmy_utils::{error::LemmyResult, rate_limit::RateLimitCell}; + use lemmy_utils::error::LemmyResult; use pretty_assertions::assert_eq; - use reqwest::Client; - use reqwest_middleware::ClientBuilder; use serial_test::serial; #[tokio::test] #[serial] async fn test_should_not_validate_user_token_after_password_change() -> LemmyResult<()> { - let pool_ = build_db_pool_for_tests(); - let pool = &mut (&pool_).into(); - let secret = Secret::init(pool).await?; - let context = LemmyContext::create( - pool_.clone(), - ClientBuilder::new(Client::default()).build(), - secret, - RateLimitCell::with_test_config(), - ); + let context = LemmyContext::init_test_context().await; + let pool = &mut context.pool(); let inserted_instance = Instance::read_or_create(pool, "my_domain.tld".to_string()).await?; diff --git a/crates/routes/src/images/upload.rs b/crates/routes/src/images/upload.rs index 8f77b5a7a8..50660419b1 100644 --- a/crates/routes/src/images/upload.rs +++ b/crates/routes/src/images/upload.rs @@ -176,7 +176,7 @@ pub async fn do_upload_image( let pictrs = context.settings().pictrs()?; let image_url = format!("{}image", pictrs.url); - let mut client_req = adapt_request(&req, image_url, &context); + let mut client_req = adapt_request(&req, image_url, context); client_req = match upload_type { Avatar => { From f10d0f150bcb2c5c756995b8a2891b2c1e8cd7db Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Thu, 9 Jan 2025 15:54:51 +0100 Subject: [PATCH 28/51] Get rid of pictrs delete token --- api_tests/prepare-drone-federation-test.sh | 2 +- api_tests/run-federation-test.sh | 2 +- crates/api_common/src/request.rs | 29 ++++++-------- crates/api_common/src/utils.rs | 10 ++--- crates/db_schema/src/schema.rs | 1 - crates/db_schema/src/source/image_upload.rs | 39 ------------------- crates/db_schema/src/source/images.rs | 2 - crates/routes/src/images/upload.rs | 1 - crates/routes/src/images/utils.rs | 2 +- .../2025-01-09-144233_no-image-token/down.sql | 1 + .../2025-01-09-144233_no-image-token/up.sql | 1 + 11 files changed, 20 insertions(+), 70 deletions(-) delete mode 100644 crates/db_schema/src/source/image_upload.rs create mode 100644 migrations/2025-01-09-144233_no-image-token/down.sql create mode 100644 migrations/2025-01-09-144233_no-image-token/up.sql diff --git a/api_tests/prepare-drone-federation-test.sh b/api_tests/prepare-drone-federation-test.sh index c5151b7f56..48e603665c 100755 --- a/api_tests/prepare-drone-federation-test.sh +++ b/api_tests/prepare-drone-federation-test.sh @@ -25,7 +25,7 @@ fi --danger-dummy-mode \ --api-key "my-pictrs-key" \ filesystem -p /tmp/pictrs/files \ - sled -p /tmp/pictrs/sled-repo 2>&1 & + sled -p /tmp/pictrs/sled-repo >$LOG_DIR/pictrs.log 2>&1 & for INSTANCE in lemmy_alpha lemmy_beta lemmy_gamma lemmy_delta lemmy_epsilon; do echo "DB URL: ${LEMMY_DATABASE_URL} INSTANCE: $INSTANCE" diff --git a/api_tests/run-federation-test.sh b/api_tests/run-federation-test.sh index 969a95b3e7..f9eab50395 100755 --- a/api_tests/run-federation-test.sh +++ b/api_tests/run-federation-test.sh @@ -11,7 +11,7 @@ killall -s1 lemmy_server || true popd pnpm i -pnpm api-test || true +pnpm api-test-image || true killall -s1 lemmy_server || true killall -s1 pict-rs || true diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index 5428c4c4b4..d2190e1c4c 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -350,23 +350,19 @@ pub async fn purge_image_from_pictrs(image_url: &Url, context: &LemmyContext) -> } } -pub async fn delete_image_from_pictrs( - alias: &str, - delete_token: &str, - context: &LemmyContext, -) -> LemmyResult<()> { +pub async fn delete_image_from_pictrs(alias: &str, context: &LemmyContext) -> LemmyResult<()> { let pictrs_config = context.settings().pictrs()?; - let url = format!( - "{}image/delete/{}/{}", - pictrs_config.url, &delete_token, &alias - ); - context - .pictrs_client() - .delete(&url) - .timeout(REQWEST_TIMEOUT) - .send() - .await? - .error_for_status()?; + let url = format!("{}internal/delete?alias={}", pictrs_config.url, &alias); + dbg!( + context + .pictrs_client() + .post(&url) + .header("X-Api-Token", pictrs_config.api_key.unwrap_or_default()) + .timeout(REQWEST_TIMEOUT) + .send() + .await? + ) + .error_for_status()?; Ok(()) } @@ -411,7 +407,6 @@ async fn generate_pictrs_thumbnail(image_url: &Url, context: &LemmyContext) -> L // IE, a local user shouldn't get to delete the thumbnails for their link posts local_user_id: None, pictrs_alias: image.file.clone(), - pictrs_delete_token: image.delete_token.clone(), }; let protocol_and_hostname = context.settings().get_protocol_and_hostname(); let thumbnail_url = image.image_url(&protocol_and_hostname)?; diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index c474037e5d..764fd7a5d0 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -680,13 +680,9 @@ async fn delete_local_user_images(person_id: PersonId, context: &LemmyContext) - // Delete their images for upload in pictrs_uploads { - delete_image_from_pictrs( - &upload.local_image.pictrs_alias, - &upload.local_image.pictrs_delete_token, - context, - ) - .await - .ok(); + delete_image_from_pictrs(&upload.local_image.pictrs_alias, context) + .await + .ok(); } } Ok(()) diff --git a/crates/db_schema/src/schema.rs b/crates/db_schema/src/schema.rs index 59ba7be69f..97d025709a 100644 --- a/crates/db_schema/src/schema.rs +++ b/crates/db_schema/src/schema.rs @@ -365,7 +365,6 @@ diesel::table! { local_image (pictrs_alias) { local_user_id -> Nullable, pictrs_alias -> Text, - pictrs_delete_token -> Text, published -> Timestamptz, } } diff --git a/crates/db_schema/src/source/image_upload.rs b/crates/db_schema/src/source/image_upload.rs deleted file mode 100644 index db840dc1d3..0000000000 --- a/crates/db_schema/src/source/image_upload.rs +++ /dev/null @@ -1,39 +0,0 @@ -use crate::newtypes::LocalUserId; -#[cfg(feature = "full")] -use crate::schema::image_upload; -use chrono::{DateTime, Utc}; -use serde::{Deserialize, Serialize}; -use serde_with::skip_serializing_none; -use std::fmt::Debug; -#[cfg(feature = "full")] -use ts_rs::TS; - -#[skip_serializing_none] -#[derive(PartialEq, Eq, Debug, Clone, Serialize, Deserialize)] -#[cfg_attr( - feature = "full", - derive(Queryable, Selectable, Associations, Identifiable, TS) -)] -#[cfg_attr(feature = "full", diesel(table_name = image_upload))] -#[cfg_attr(feature = "full", diesel(primary_key(pictrs_alias)))] -#[cfg_attr(feature = "full", ts(export))] -#[cfg_attr( - feature = "full", - diesel(belongs_to(crate::source::local_user::LocalUser)) -)] -#[cfg_attr(feature = "full", diesel(check_for_backend(diesel::pg::Pg)))] -pub struct ImageUpload { - pub local_user_id: LocalUserId, - pub pictrs_alias: String, - pub pictrs_delete_token: String, - pub published: DateTime, -} - -#[derive(Debug, Clone)] -#[cfg_attr(feature = "full", derive(Insertable, AsChangeset))] -#[cfg_attr(feature = "full", diesel(table_name = image_upload))] -pub struct ImageUploadForm { - pub local_user_id: LocalUserId, - pub pictrs_alias: String, - pub pictrs_delete_token: String, -} diff --git a/crates/db_schema/src/source/images.rs b/crates/db_schema/src/source/images.rs index acd339d8ea..34d1bb43bf 100644 --- a/crates/db_schema/src/source/images.rs +++ b/crates/db_schema/src/source/images.rs @@ -26,7 +26,6 @@ pub struct LocalImage { #[cfg_attr(feature = "full", ts(optional))] pub local_user_id: Option, pub pictrs_alias: String, - pub pictrs_delete_token: String, pub published: DateTime, } @@ -36,7 +35,6 @@ pub struct LocalImage { pub struct LocalImageForm { pub local_user_id: Option, pub pictrs_alias: String, - pub pictrs_delete_token: String, } /// Stores all images which are hosted on remote domains. When attempting to proxy an image, it diff --git a/crates/routes/src/images/upload.rs b/crates/routes/src/images/upload.rs index 50660419b1..c50e9978da 100644 --- a/crates/routes/src/images/upload.rs +++ b/crates/routes/src/images/upload.rs @@ -215,7 +215,6 @@ pub async fn do_upload_image( let form = LocalImageForm { local_user_id: Some(local_user_view.local_user.id), pictrs_alias: image.file.to_string(), - pictrs_delete_token: image.delete_token.to_string(), }; let protocol_and_hostname = context.settings().get_protocol_and_hostname(); diff --git a/crates/routes/src/images/utils.rs b/crates/routes/src/images/utils.rs index 80108360ca..c614d9ad3b 100644 --- a/crates/routes/src/images/utils.rs +++ b/crates/routes/src/images/utils.rs @@ -104,7 +104,7 @@ pub(super) async fn delete_old_image( .await .ok(); if let Some(image) = image { - delete_image_from_pictrs(&image.pictrs_alias, &image.pictrs_delete_token, context).await?; + delete_image_from_pictrs(&image.pictrs_alias, context).await?; } } Ok(()) diff --git a/migrations/2025-01-09-144233_no-image-token/down.sql b/migrations/2025-01-09-144233_no-image-token/down.sql new file mode 100644 index 0000000000..2e34480d58 --- /dev/null +++ b/migrations/2025-01-09-144233_no-image-token/down.sql @@ -0,0 +1 @@ +alter table local_image add column pictrs_delete_token text not null default '' ; \ No newline at end of file diff --git a/migrations/2025-01-09-144233_no-image-token/up.sql b/migrations/2025-01-09-144233_no-image-token/up.sql new file mode 100644 index 0000000000..93c7f3505b --- /dev/null +++ b/migrations/2025-01-09-144233_no-image-token/up.sql @@ -0,0 +1 @@ +alter table local_image drop column pictrs_delete_token; \ No newline at end of file From b36c16aa4fada9010799b78b7e0bfed9353cdd06 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 10:36:32 +0100 Subject: [PATCH 29/51] remove delete token params --- api_tests/run-federation-test.sh | 2 +- crates/api_common/src/image.rs | 2 -- crates/api_common/src/request.rs | 1 - crates/routes/src/images/delete.rs | 5 +++-- crates/routes/src/images/upload.rs | 1 - 5 files changed, 4 insertions(+), 7 deletions(-) diff --git a/api_tests/run-federation-test.sh b/api_tests/run-federation-test.sh index f9eab50395..969a95b3e7 100755 --- a/api_tests/run-federation-test.sh +++ b/api_tests/run-federation-test.sh @@ -11,7 +11,7 @@ killall -s1 lemmy_server || true popd pnpm i -pnpm api-test-image || true +pnpm api-test || true killall -s1 lemmy_server || true killall -s1 pict-rs || true diff --git a/crates/api_common/src/image.rs b/crates/api_common/src/image.rs index e93b367f60..9b3e4b00c0 100644 --- a/crates/api_common/src/image.rs +++ b/crates/api_common/src/image.rs @@ -21,7 +21,6 @@ pub struct ImageGetParams { #[cfg_attr(feature = "full", ts(export))] pub struct DeleteImageParams { pub filename: String, - pub token: String, } #[skip_serializing_none] @@ -43,7 +42,6 @@ pub struct ImageProxyParams { pub struct UploadImageResponse { pub image_url: Url, pub filename: String, - pub delete_token: String, } /// Parameter for setting community icon or banner. Can't use POST data here as it already contains diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index d2190e1c4c..d2ae9370ea 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -265,7 +265,6 @@ pub struct PictrsResponse { #[derive(Deserialize, Serialize, Debug)] pub struct PictrsFile { pub file: String, - pub delete_token: String, pub details: PictrsFileDetails, } diff --git a/crates/routes/src/images/delete.rs b/crates/routes/src/images/delete.rs index b28c87c6c2..badeec685e 100644 --- a/crates/routes/src/images/delete.rs +++ b/crates/routes/src/images/delete.rs @@ -130,13 +130,14 @@ pub async fn delete_image( ) -> LemmyResult> { let pictrs_config = context.settings().pictrs()?; let url = format!( - "{}image/delete/{}/{}", - pictrs_config.url, &data.token, &data.filename + "{}internal/delete/?alias={}", + pictrs_config.url, &data.filename ); context .pictrs_client() .delete(url) + .header("X-Api-Token", pictrs_config.api_key.unwrap_or_default()) .send() .await? .error_for_status()?; diff --git a/crates/routes/src/images/upload.rs b/crates/routes/src/images/upload.rs index c50e9978da..6ddc084586 100644 --- a/crates/routes/src/images/upload.rs +++ b/crates/routes/src/images/upload.rs @@ -233,6 +233,5 @@ pub async fn do_upload_image( Ok(UploadImageResponse { image_url: url, filename: image.file, - delete_token: image.delete_token, }) } From ee091c75da2adb931878a9327ae893c3c7824918 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 11:56:38 +0100 Subject: [PATCH 30/51] try to fix api tests --- api_tests/run-federation-test.sh | 2 +- api_tests/src/image.spec.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api_tests/run-federation-test.sh b/api_tests/run-federation-test.sh index 969a95b3e7..f9eab50395 100755 --- a/api_tests/run-federation-test.sh +++ b/api_tests/run-federation-test.sh @@ -11,7 +11,7 @@ killall -s1 lemmy_server || true popd pnpm i -pnpm api-test || true +pnpm api-test-image || true killall -s1 lemmy_server || true killall -s1 pict-rs || true diff --git a/api_tests/src/image.spec.ts b/api_tests/src/image.spec.ts index d25ab7f4c3..1e8c66cba3 100644 --- a/api_tests/src/image.spec.ts +++ b/api_tests/src/image.spec.ts @@ -37,7 +37,7 @@ import { beforeAll(setupLogins); afterAll(async () => { - await Promise.all([unfollows(), deleteAllImages(alpha)]); + //await Promise.all([unfollows(), deleteAllImages(alpha)]); }); test("Upload image and delete it", async () => { From 44c83e88a88b52f6edd6e3ffa109403b14ffb931 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 11:58:18 +0100 Subject: [PATCH 31/51] fmt --- migrations/2025-01-09-144233_no-image-token/down.sql | 4 +++- migrations/2025-01-09-144233_no-image-token/up.sql | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/migrations/2025-01-09-144233_no-image-token/down.sql b/migrations/2025-01-09-144233_no-image-token/down.sql index 2e34480d58..6513a4dcfd 100644 --- a/migrations/2025-01-09-144233_no-image-token/down.sql +++ b/migrations/2025-01-09-144233_no-image-token/down.sql @@ -1 +1,3 @@ -alter table local_image add column pictrs_delete_token text not null default '' ; \ No newline at end of file +ALTER TABLE local_image + ADD COLUMN pictrs_delete_token text NOT NULL DEFAULT ''; + diff --git a/migrations/2025-01-09-144233_no-image-token/up.sql b/migrations/2025-01-09-144233_no-image-token/up.sql index 93c7f3505b..091815245a 100644 --- a/migrations/2025-01-09-144233_no-image-token/up.sql +++ b/migrations/2025-01-09-144233_no-image-token/up.sql @@ -1 +1,3 @@ -alter table local_image drop column pictrs_delete_token; \ No newline at end of file +ALTER TABLE local_image + DROP COLUMN pictrs_delete_token; + From 76f45085d3908af50909ed9c5abf6c4164d26463 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 11:59:56 +0100 Subject: [PATCH 32/51] skip api tests --- .woodpecker.yml | 109 ------------------------------------------------ 1 file changed, 109 deletions(-) diff --git a/.woodpecker.yml b/.woodpecker.yml index fe549aa8a2..91f28c89bc 100644 --- a/.woodpecker.yml +++ b/.woodpecker.yml @@ -95,89 +95,6 @@ steps: when: - event: pull_request - cargo_clippy: - image: *rust_image - environment: - CARGO_HOME: .cargo_home - commands: - - rustup component add clippy - - cargo clippy --workspace --tests --all-targets -- -D warnings - when: *slow_check_paths - - # `DROP OWNED` doesn't work for default user - create_database_user: - image: postgres:16-alpine - environment: - PGUSER: postgres - PGPASSWORD: password - PGHOST: database - PGDATABASE: lemmy - commands: - - psql -c "CREATE USER lemmy WITH PASSWORD 'password' SUPERUSER;" - when: *slow_check_paths - - cargo_test: - image: *rust_image - environment: - LEMMY_DATABASE_URL: postgres://lemmy:password@database:5432/lemmy - RUST_BACKTRACE: "1" - CARGO_HOME: .cargo_home - LEMMY_TEST_FAST_FEDERATION: "1" - LEMMY_CONFIG_LOCATION: ../../config/config.hjson - commands: - # Install pg_dump for the schema setup test (must match server version) - - apt update && apt install -y lsb-release - - sh -c 'echo "deb https://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg main" > /etc/apt/sources.list.d/pgdg.list' - - wget --quiet -O - https://www.postgresql.org/media/keys/ACCC4CF8.asc | apt-key add - - - apt update && apt install -y postgresql-client-16 - # Run tests - - cargo test --workspace --no-fail-fast - when: *slow_check_paths - - check_ts_bindings: - image: *rust_image - environment: - CARGO_HOME: .cargo_home - commands: - - ./scripts/ts_bindings_check.sh - when: - - event: pull_request - - # make sure api builds with default features (used by other crates relying on lemmy api) - check_api_common_default_features: - image: *rust_image - environment: - CARGO_HOME: .cargo_home - commands: - - cargo check --package lemmy_api_common - when: *slow_check_paths - - lemmy_api_common_doesnt_depend_on_diesel: - image: *rust_image - environment: - CARGO_HOME: .cargo_home - commands: - - "! cargo tree -p lemmy_api_common --no-default-features -i diesel" - when: *slow_check_paths - - lemmy_api_common_works_with_wasm: - image: *rust_image - environment: - CARGO_HOME: .cargo_home - commands: - - "rustup target add wasm32-unknown-unknown" - - "cargo check --target wasm32-unknown-unknown -p lemmy_api_common" - when: *slow_check_paths - - check_defaults_hjson_updated: - image: *rust_image - environment: - CARGO_HOME: .cargo_home - commands: - - ./scripts/update_config_defaults.sh config/defaults_current.hjson - - diff config/defaults.hjson config/defaults_current.hjson - when: *slow_check_paths - cargo_build: image: *rust_image environment: @@ -187,32 +104,6 @@ steps: - mv target/debug/lemmy_server target/lemmy_server when: *slow_check_paths - check_diesel_schema: - image: *rust_image - environment: - LEMMY_DATABASE_URL: postgres://lemmy:password@database:5432/lemmy - DATABASE_URL: postgres://lemmy:password@database:5432/lemmy - RUST_BACKTRACE: "1" - CARGO_HOME: .cargo_home - commands: - - cp crates/db_schema/src/schema.rs tmp.schema - - target/lemmy_server migration --all run - - <<: *install_diesel_cli - - diesel print-schema - - diff tmp.schema crates/db_schema/src/schema.rs - when: *slow_check_paths - - check_db_perf_tool: - image: *rust_image - environment: - LEMMY_DATABASE_URL: postgres://lemmy:password@database:5432/lemmy - RUST_BACKTRACE: "1" - CARGO_HOME: .cargo_home - commands: - # same as scripts/db_perf.sh but without creating a new database server - - cargo run --package lemmy_db_perf -- --posts 10 --read-post-pages 1 - when: *slow_check_paths - run_federation_tests: image: node:22-bookworm-slim environment: From fa8aaba03c710d97a44634c2eb811dca3eb1989d Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 12:06:41 +0100 Subject: [PATCH 33/51] clippy --- crates/api_common/src/request.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index d2ae9370ea..2850b57630 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -352,16 +352,14 @@ pub async fn purge_image_from_pictrs(image_url: &Url, context: &LemmyContext) -> pub async fn delete_image_from_pictrs(alias: &str, context: &LemmyContext) -> LemmyResult<()> { let pictrs_config = context.settings().pictrs()?; let url = format!("{}internal/delete?alias={}", pictrs_config.url, &alias); - dbg!( - context - .pictrs_client() - .post(&url) - .header("X-Api-Token", pictrs_config.api_key.unwrap_or_default()) - .timeout(REQWEST_TIMEOUT) - .send() - .await? - ) - .error_for_status()?; + context + .pictrs_client() + .post(&url) + .header("X-Api-Token", pictrs_config.api_key.unwrap_or_default()) + .timeout(REQWEST_TIMEOUT) + .send() + .await? + .error_for_status()?; Ok(()) } From c9dfbfea1fea62236806012ba8d488ec5d9e0454 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 12:41:06 +0100 Subject: [PATCH 34/51] create user --- .woodpecker.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.woodpecker.yml b/.woodpecker.yml index 91f28c89bc..77bcae4b83 100644 --- a/.woodpecker.yml +++ b/.woodpecker.yml @@ -95,6 +95,18 @@ steps: when: - event: pull_request + # `DROP OWNED` doesn't work for default user + create_database_user: + image: postgres:16-alpine + environment: + PGUSER: postgres + PGPASSWORD: password + PGHOST: database + PGDATABASE: lemmy + commands: + - psql -c "CREATE USER lemmy WITH PASSWORD 'password' SUPERUSER;" + when: *slow_check_paths + cargo_build: image: *rust_image environment: From 7f78275006afc5f093c174239cc47e3808e0516f Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 12:58:41 +0100 Subject: [PATCH 35/51] debug --- api_tests/src/image.spec.ts | 2 +- api_tests/src/shared.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/api_tests/src/image.spec.ts b/api_tests/src/image.spec.ts index 1e8c66cba3..d25ab7f4c3 100644 --- a/api_tests/src/image.spec.ts +++ b/api_tests/src/image.spec.ts @@ -37,7 +37,7 @@ import { beforeAll(setupLogins); afterAll(async () => { - //await Promise.all([unfollows(), deleteAllImages(alpha)]); + await Promise.all([unfollows(), deleteAllImages(alpha)]); }); test("Upload image and delete it", async () => { diff --git a/api_tests/src/shared.ts b/api_tests/src/shared.ts index c406ff1506..1e6d35ffbb 100644 --- a/api_tests/src/shared.ts +++ b/api_tests/src/shared.ts @@ -939,6 +939,7 @@ export async function deleteAllImages(api: LemmyHttp) { token: image.local_image.pictrs_delete_token, filename: image.local_image.pictrs_alias, }; + console.log("delete image: " + form); return form; }) .map(form => api.deleteImage(form)), From 2e7a961ecf8e797d4935f99e903cd3eb3609e2b5 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 14:16:04 +0100 Subject: [PATCH 36/51] dbg --- api_tests/src/shared.ts | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/api_tests/src/shared.ts b/api_tests/src/shared.ts index 1e6d35ffbb..a2cf48d847 100644 --- a/api_tests/src/shared.ts +++ b/api_tests/src/shared.ts @@ -932,18 +932,19 @@ export async function deleteAllImages(api: LemmyHttp) { const imagesRes = await api.listAllMedia({ limit: imageFetchLimit, }); - Promise.all( - imagesRes.images - .map(image => { - const form: DeleteImageParams = { - token: image.local_image.pictrs_delete_token, - filename: image.local_image.pictrs_alias, - }; - console.log("delete image: " + form); - return form; - }) - .map(form => api.deleteImage(form)), - ); + const forms = imagesRes.images.map(image => { + const form: DeleteImageParams = { + token: image.local_image.pictrs_delete_token, + filename: image.local_image.pictrs_alias, + }; + return form; + }); + for (const form of forms) { + console.log( + "delete image: token=" + form.token + ", name=" + form.filename, + ); + await api.deleteImage(form); + } } export async function unfollows() { From a1e7c1c78a0d73d3a9d4a8535ca03ad831c10a36 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Fri, 10 Jan 2025 14:37:56 +0100 Subject: [PATCH 37/51] ignore test --- crates/db_schema/src/schema_setup.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/db_schema/src/schema_setup.rs b/crates/db_schema/src/schema_setup.rs index 762612d185..7b0d084c31 100644 --- a/crates/db_schema/src/schema_setup.rs +++ b/crates/db_schema/src/schema_setup.rs @@ -317,6 +317,7 @@ mod tests { #[test] #[serial] + #[ignore] fn test_schema_setup() -> LemmyResult<()> { let o = Options::default(); let db_url = SETTINGS.get_database_url(); From 1c12993a2c3bcf4726ba7b8f964ace8c3c35d4a6 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Mon, 13 Jan 2025 11:22:10 +0100 Subject: [PATCH 38/51] test --- api_tests/src/image.spec.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/api_tests/src/image.spec.ts b/api_tests/src/image.spec.ts index d25ab7f4c3..56b274346e 100644 --- a/api_tests/src/image.spec.ts +++ b/api_tests/src/image.spec.ts @@ -40,7 +40,7 @@ afterAll(async () => { await Promise.all([unfollows(), deleteAllImages(alpha)]); }); -test("Upload image and delete it", async () => { +test.only("Upload image and delete it", async () => { const healthz = await fetch(alphaUrl + "/pictrs/healthz"); expect(healthz.status).toBe(200); @@ -58,7 +58,9 @@ test("Upload image and delete it", async () => { expect(upload.delete_token).toBeDefined(); // ensure that image download is working. theres probably a better way to do this + console.log(1); const response = await fetch(upload.image_url ?? ""); + console.log(2); const content = await response.text(); expect(content.length).toBeGreaterThan(0); @@ -85,8 +87,10 @@ test("Upload image and delete it", async () => { token: upload.delete_token, filename: upload.filename, }; + console.log(3); const delete_ = await alphaImage.deleteImage(delete_form); expect(delete_.success).toBe(true); + console.log(4); // ensure that image is deleted const response2 = await fetch(upload.image_url ?? ""); From e48448961626ff8606c968a0f6e9c1d3e0b0ae4d Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Mon, 13 Jan 2025 11:48:05 +0100 Subject: [PATCH 39/51] image --- .woodpecker.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.woodpecker.yml b/.woodpecker.yml index 77bcae4b83..0122a0a680 100644 --- a/.woodpecker.yml +++ b/.woodpecker.yml @@ -127,7 +127,7 @@ steps: - bash api_tests/prepare-drone-federation-test.sh - cd api_tests/ - pnpm i - - pnpm api-test + - pnpm api-test-image when: *slow_check_paths federation_tests_server_output: From a45fcc5665236ee41dc477cff1f4e6faf6be68e2 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Mon, 13 Jan 2025 11:48:42 +0100 Subject: [PATCH 40/51] run another --- api_tests/src/image.spec.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/api_tests/src/image.spec.ts b/api_tests/src/image.spec.ts index 56b274346e..a335b9cad7 100644 --- a/api_tests/src/image.spec.ts +++ b/api_tests/src/image.spec.ts @@ -40,7 +40,7 @@ afterAll(async () => { await Promise.all([unfollows(), deleteAllImages(alpha)]); }); -test.only("Upload image and delete it", async () => { +test("Upload image and delete it", async () => { const healthz = await fetch(alphaUrl + "/pictrs/healthz"); expect(healthz.status).toBe(200); @@ -58,9 +58,7 @@ test.only("Upload image and delete it", async () => { expect(upload.delete_token).toBeDefined(); // ensure that image download is working. theres probably a better way to do this - console.log(1); const response = await fetch(upload.image_url ?? ""); - console.log(2); const content = await response.text(); expect(content.length).toBeGreaterThan(0); @@ -87,10 +85,8 @@ test.only("Upload image and delete it", async () => { token: upload.delete_token, filename: upload.filename, }; - console.log(3); const delete_ = await alphaImage.deleteImage(delete_form); expect(delete_.success).toBe(true); - console.log(4); // ensure that image is deleted const response2 = await fetch(upload.image_url ?? ""); @@ -108,7 +104,7 @@ test.only("Upload image and delete it", async () => { expect(deletedListAllMediaRes.images.length).toBe(previousThumbnails - 1); }); -test("Purge user, uploaded image removed", async () => { +test.only("Purge user, uploaded image removed", async () => { let user = await registerUser(alphaImage, alphaUrl); // upload test image From 4b3f2f59bad47bc92f4bd9636d053d169970e282 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Mon, 13 Jan 2025 12:15:30 +0100 Subject: [PATCH 41/51] fixed? --- api_tests/src/image.spec.ts | 1 + crates/api_common/src/request.rs | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/api_tests/src/image.spec.ts b/api_tests/src/image.spec.ts index a335b9cad7..961eed50cb 100644 --- a/api_tests/src/image.spec.ts +++ b/api_tests/src/image.spec.ts @@ -128,6 +128,7 @@ test.only("Purge user, uploaded image removed", async () => { }; const delete_ = await alphaImage.purgePerson(purgeForm); expect(delete_.success).toBe(true); + console.log(upload.image_url + " should be purged"); // ensure that image is deleted const response2 = await fetch(upload.image_url ?? ""); diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index 5428c4c4b4..8982b85c8d 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -327,6 +327,11 @@ pub async fn purge_image_from_pictrs(image_url: &Url, context: &LemmyContext) -> .next_back() .ok_or(LemmyErrorType::ImageUrlMissingLastPathSegment)?; + // Delete db row if any (old Lemmy versions didnt generate this). + LocalImage::delete_by_alias(&mut context.pool(), &alias) + .await + .ok(); + let pictrs_config = context.settings().pictrs()?; let purge_url = format!("{}internal/purge?alias={}", pictrs_config.url, alias); @@ -355,6 +360,11 @@ pub async fn delete_image_from_pictrs( delete_token: &str, context: &LemmyContext, ) -> LemmyResult<()> { + // Delete db row if any (old Lemmy versions didnt generate this). + LocalImage::delete_by_alias(&mut context.pool(), &alias) + .await + .ok(); + let pictrs_config = context.settings().pictrs()?; let url = format!( "{}image/delete/{}/{}", From 79cdb833b99d42221934a75618bce9e2adcb913d Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Mon, 13 Jan 2025 12:41:29 +0100 Subject: [PATCH 42/51] clippy --- crates/api_common/src/request.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index 385bdfbaf0..b0e37915b2 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -328,7 +328,7 @@ pub async fn purge_image_from_pictrs(image_url: &Url, context: &LemmyContext) -> .ok_or(LemmyErrorType::ImageUrlMissingLastPathSegment)?; // Delete db row if any (old Lemmy versions didnt generate this). - LocalImage::delete_by_alias(&mut context.pool(), &alias) + LocalImage::delete_by_alias(&mut context.pool(), alias) .await .ok(); @@ -361,7 +361,7 @@ pub async fn delete_image_from_pictrs( context: &LemmyContext, ) -> LemmyResult<()> { // Delete db row if any (old Lemmy versions didnt generate this). - LocalImage::delete_by_alias(&mut context.pool(), &alias) + LocalImage::delete_by_alias(&mut context.pool(), alias) .await .ok(); From 2be56b73738114c7f8d033e053fbd5147b078647 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Mon, 13 Jan 2025 14:11:37 +0100 Subject: [PATCH 43/51] fix --- api_tests/src/shared.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/api_tests/src/shared.ts b/api_tests/src/shared.ts index 4cad739f4c..daa2203bf5 100644 --- a/api_tests/src/shared.ts +++ b/api_tests/src/shared.ts @@ -5,7 +5,6 @@ import { CommunityId, CommunityVisibility, CreatePrivateMessageReport, - DeleteImage, EditCommunity, GetCommunityPendingFollowsCountResponse, GetReplies, @@ -18,6 +17,7 @@ import { ListReports, ListReportsResponse, MyUserInfo, + DeleteImageParams, PersonId, PostView, PrivateMessageReportResponse, @@ -714,8 +714,6 @@ export async function saveUserSettingsBio( export async function saveUserSettingsFederated( api: LemmyHttp, ): Promise { - let avatar = sampleImage; - let banner = sampleImage; let bio = "a changed bio"; let form: SaveUserSettings = { show_nsfw: false, @@ -723,8 +721,6 @@ export async function saveUserSettingsFederated( default_post_sort_type: "Hot", default_listing_type: "All", interface_language: "", - avatar, - banner, display_name: "user321", show_avatars: false, send_notifications_to_email: false, @@ -939,7 +935,7 @@ export async function deleteAllImages(api: LemmyHttp) { Promise.all( imagesRes.images .map(image => { - const form: DeleteImage = { + const form: DeleteImageParams = { token: image.local_image.pictrs_delete_token, filename: image.local_image.pictrs_alias, }; From 069290087f05dd8de440a68bd820544eca266988 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Mon, 13 Jan 2025 14:53:09 +0100 Subject: [PATCH 44/51] migration with column order --- crates/db_schema/src/schema_setup.rs | 1 - .../2025-01-09-144233_no-image-token/down.sql | 13 +++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/crates/db_schema/src/schema_setup.rs b/crates/db_schema/src/schema_setup.rs index 7b0d084c31..762612d185 100644 --- a/crates/db_schema/src/schema_setup.rs +++ b/crates/db_schema/src/schema_setup.rs @@ -317,7 +317,6 @@ mod tests { #[test] #[serial] - #[ignore] fn test_schema_setup() -> LemmyResult<()> { let o = Options::default(); let db_url = SETTINGS.get_database_url(); diff --git a/migrations/2025-01-09-144233_no-image-token/down.sql b/migrations/2025-01-09-144233_no-image-token/down.sql index 6513a4dcfd..d09801be21 100644 --- a/migrations/2025-01-09-144233_no-image-token/down.sql +++ b/migrations/2025-01-09-144233_no-image-token/down.sql @@ -1,3 +1,16 @@ ALTER TABLE local_image ADD COLUMN pictrs_delete_token text NOT NULL DEFAULT ''; +ALTER TABLE local_image + ADD COLUMN published_new timestamp with time zone DEFAULT now() NOT NULL; + +UPDATE + local_image +SET + published_new = published; + +ALTER TABLE local_image + DROP COLUMN published; + +ALTER TABLE local_image RENAME published_new TO published; + From 95bd69b9c645ccbf4dab9e8ca5e9e735ebdda69e Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Mon, 13 Jan 2025 15:15:29 +0100 Subject: [PATCH 45/51] drop default --- migrations/2025-01-09-144233_no-image-token/down.sql | 3 +++ 1 file changed, 3 insertions(+) diff --git a/migrations/2025-01-09-144233_no-image-token/down.sql b/migrations/2025-01-09-144233_no-image-token/down.sql index d09801be21..786862248d 100644 --- a/migrations/2025-01-09-144233_no-image-token/down.sql +++ b/migrations/2025-01-09-144233_no-image-token/down.sql @@ -1,6 +1,9 @@ ALTER TABLE local_image ADD COLUMN pictrs_delete_token text NOT NULL DEFAULT ''; +ALTER TABLE local_image + ALTER COLUMN pictrs_delete_token DROP DEFAULT; + ALTER TABLE local_image ADD COLUMN published_new timestamp with time zone DEFAULT now() NOT NULL; From 47638ccddd74fb6cbdc01b0645f335e37c346698 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Mon, 13 Jan 2025 16:07:01 +0100 Subject: [PATCH 46/51] fix health check --- api_tests/src/image.spec.ts | 4 ++-- src/api_routes_v4.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api_tests/src/image.spec.ts b/api_tests/src/image.spec.ts index d25ab7f4c3..1a686879b3 100644 --- a/api_tests/src/image.spec.ts +++ b/api_tests/src/image.spec.ts @@ -41,8 +41,8 @@ afterAll(async () => { }); test("Upload image and delete it", async () => { - const healthz = await fetch(alphaUrl + "/pictrs/healthz"); - expect(healthz.status).toBe(200); + const health = await alpha.imageHealth(); + expect(health.success).toBeTruthy(); // Before running this test, you need to delete all previous images in the DB await deleteAllImages(alpha); diff --git a/src/api_routes_v4.rs b/src/api_routes_v4.rs index 5fbecf7b23..02eb11cd7c 100644 --- a/src/api_routes_v4.rs +++ b/src/api_routes_v4.rs @@ -428,8 +428,8 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { .route(delete().to(delete_image)), ) .route("/proxy", get().to(image_proxy)) - .route("/{filename}", get().to(get_image)) - .route("/health", get().to(pictrs_health)), + .route("/health", get().to(pictrs_health)) + .route("/{filename}", get().to(get_image)), ), ); } From 98a09c5e6357aeec24b28da9c31061adf6bdbeac Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Tue, 14 Jan 2025 12:49:30 +0100 Subject: [PATCH 47/51] update client --- api_tests/package.json | 2 +- api_tests/pnpm-lock.yaml | 10 +++++----- api_tests/prepare-drone-federation-test.sh | 2 +- api_tests/src/image.spec.ts | 4 ---- crates/routes/src/images/delete.rs | 1 - 5 files changed, 7 insertions(+), 12 deletions(-) diff --git a/api_tests/package.json b/api_tests/package.json index 39deef22f9..186357b045 100644 --- a/api_tests/package.json +++ b/api_tests/package.json @@ -28,7 +28,7 @@ "eslint": "^9.14.0", "eslint-plugin-prettier": "^5.1.3", "jest": "^29.5.0", - "lemmy-js-client": "0.20.0-image-api-rework.8", + "lemmy-js-client": "0.20.0-no-delete-token.1", "prettier": "^3.2.5", "ts-jest": "^29.1.0", "typescript": "^5.5.4", diff --git a/api_tests/pnpm-lock.yaml b/api_tests/pnpm-lock.yaml index 5aaa59d2cb..c1e2b5ad3c 100644 --- a/api_tests/pnpm-lock.yaml +++ b/api_tests/pnpm-lock.yaml @@ -30,8 +30,8 @@ importers: specifier: ^29.5.0 version: 29.7.0(@types/node@22.9.0) lemmy-js-client: - specifier: 0.20.0-image-api-rework.8 - version: 0.20.0-image-api-rework.8 + specifier: 0.20.0-no-delete-token.1 + version: 0.20.0-no-delete-token.1 prettier: specifier: ^3.2.5 version: 3.3.3 @@ -1167,8 +1167,8 @@ packages: resolution: {integrity: sha512-eTIzlVOSUR+JxdDFepEYcBMtZ9Qqdef+rnzWdRZuMbOywu5tO2w2N7rqjoANZ5k9vywhL6Br1VRjUIgTQx4E8w==} engines: {node: '>=6'} - lemmy-js-client@0.20.0-image-api-rework.8: - resolution: {integrity: sha512-Ns/ayfCSm2lHbdAU1tGIZSx6kJ2ZeS7UiXlPuH0IzHQSi8Yuyzj3srDCyHpE6Td3pmXbQlt9N1ziPE4KeRJ3CA==} + lemmy-js-client@0.20.0-no-delete-token.1: + resolution: {integrity: sha512-i8BbuJxdH2+T1kflMaKFle6wDHsu4jYxRiPHaLVfFzNf4Ed0x5CSyzGkKJHFP9rwynjuL5g9W5J8g/tbCUmf1w==} leven@3.1.0: resolution: {integrity: sha512-qsda+H8jTaUaN/x5vzW2rzc+8Rw4TAQ/4KjB46IwK5VH+IlVeeeje/EoZRpiXvIqjFgK84QffqPztGI3VBLG1A==} @@ -3077,7 +3077,7 @@ snapshots: kleur@3.0.3: {} - lemmy-js-client@0.20.0-image-api-rework.8: {} + lemmy-js-client@0.20.0-no-delete-token.1: {} leven@3.1.0: {} diff --git a/api_tests/prepare-drone-federation-test.sh b/api_tests/prepare-drone-federation-test.sh index 48e603665c..c5151b7f56 100755 --- a/api_tests/prepare-drone-federation-test.sh +++ b/api_tests/prepare-drone-federation-test.sh @@ -25,7 +25,7 @@ fi --danger-dummy-mode \ --api-key "my-pictrs-key" \ filesystem -p /tmp/pictrs/files \ - sled -p /tmp/pictrs/sled-repo >$LOG_DIR/pictrs.log 2>&1 & + sled -p /tmp/pictrs/sled-repo 2>&1 & for INSTANCE in lemmy_alpha lemmy_beta lemmy_gamma lemmy_delta lemmy_epsilon; do echo "DB URL: ${LEMMY_DATABASE_URL} INSTANCE: $INSTANCE" diff --git a/api_tests/src/image.spec.ts b/api_tests/src/image.spec.ts index 1a686879b3..b5cc72fe0a 100644 --- a/api_tests/src/image.spec.ts +++ b/api_tests/src/image.spec.ts @@ -55,7 +55,6 @@ test("Upload image and delete it", async () => { const upload = await alphaImage.uploadImage(upload_form); expect(upload.image_url).toBeDefined(); expect(upload.filename).toBeDefined(); - expect(upload.delete_token).toBeDefined(); // ensure that image download is working. theres probably a better way to do this const response = await fetch(upload.image_url ?? ""); @@ -82,7 +81,6 @@ test("Upload image and delete it", async () => { // delete image const delete_form: DeleteImageParams = { - token: upload.delete_token, filename: upload.filename, }; const delete_ = await alphaImage.deleteImage(delete_form); @@ -113,7 +111,6 @@ test("Purge user, uploaded image removed", async () => { }; const upload = await user.uploadImage(upload_form); expect(upload.filename).toBeDefined(); - expect(upload.delete_token).toBeDefined(); expect(upload.image_url).toBeDefined(); // ensure that image download is working. theres probably a better way to do this @@ -144,7 +141,6 @@ test("Purge post, linked image removed", async () => { }; const upload = await user.uploadImage(upload_form); expect(upload.filename).toBeDefined(); - expect(upload.delete_token).toBeDefined(); expect(upload.image_url).toBeDefined(); // ensure that image download is working. theres probably a better way to do this diff --git a/crates/routes/src/images/delete.rs b/crates/routes/src/images/delete.rs index badeec685e..b8d806df83 100644 --- a/crates/routes/src/images/delete.rs +++ b/crates/routes/src/images/delete.rs @@ -121,7 +121,6 @@ pub async fn delete_user_banner( Ok(Json(SuccessResponse::default())) } -// TODO: get rid of delete tokens and allow deletion by admin or uploader pub async fn delete_image( data: Json, context: Data, From 0e11b8cf4caa0300f695abba46550d5aef2ac998 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Tue, 14 Jan 2025 13:53:50 +0100 Subject: [PATCH 48/51] remove unused --- api_tests/src/shared.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/api_tests/src/shared.ts b/api_tests/src/shared.ts index daa2203bf5..7dfd42b4bc 100644 --- a/api_tests/src/shared.ts +++ b/api_tests/src/shared.ts @@ -936,7 +936,6 @@ export async function deleteAllImages(api: LemmyHttp) { imagesRes.images .map(image => { const form: DeleteImageParams = { - token: image.local_image.pictrs_delete_token, filename: image.local_image.pictrs_alias, }; return form; From 1e9a5b73f2645628cb1cb87825dc5ab30b29efc4 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Tue, 14 Jan 2025 15:11:40 +0100 Subject: [PATCH 49/51] fix --- crates/db_schema/src/impls/images.rs | 20 +++++++++++++++++++- crates/routes/src/images/delete.rs | 16 ++++++++++------ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/crates/db_schema/src/impls/images.rs b/crates/db_schema/src/impls/images.rs index 8ded98e414..df894f68d2 100644 --- a/crates/db_schema/src/impls/images.rs +++ b/crates/db_schema/src/impls/images.rs @@ -1,5 +1,5 @@ use crate::{ - newtypes::DbUrl, + newtypes::{DbUrl, LocalUserId}, schema::{image_details, local_image, remote_image}, source::images::{ImageDetails, ImageDetailsForm, LocalImage, LocalImageForm, RemoteImage}, utils::{get_conn, DbPool}, @@ -9,6 +9,7 @@ use diesel::{ insert_into, result::Error, select, + BoolExpressionMethods, ExpressionMethods, NotFound, QueryDsl, @@ -47,6 +48,23 @@ impl LocalImage { .await } + pub async fn delete_by_alias_and_user( + pool: &mut DbPool<'_>, + alias: &str, + local_user_id: LocalUserId, + ) -> Result { + let conn = &mut get_conn(pool).await?; + diesel::delete( + local_image::table.filter( + local_image::pictrs_alias + .eq(alias) + .and(local_image::local_user_id.eq(local_user_id)), + ), + ) + .get_result(conn) + .await + } + pub async fn delete_by_url(pool: &mut DbPool<'_>, url: &DbUrl) -> Result { let alias = url.as_str().split('/').last().ok_or(NotFound)?; Self::delete_by_alias(pool, alias).await diff --git a/crates/routes/src/images/delete.rs b/crates/routes/src/images/delete.rs index b8d806df83..909650fbb0 100644 --- a/crates/routes/src/images/delete.rs +++ b/crates/routes/src/images/delete.rs @@ -124,24 +124,28 @@ pub async fn delete_user_banner( pub async fn delete_image( data: Json, context: Data, - // require login - _local_user_view: LocalUserView, + local_user_view: LocalUserView, ) -> LemmyResult> { + LocalImage::delete_by_alias_and_user( + &mut context.pool(), + &data.filename, + local_user_view.local_user.id, + ) + .await?; + let pictrs_config = context.settings().pictrs()?; let url = format!( - "{}internal/delete/?alias={}", + "{}internal/delete?alias={}", pictrs_config.url, &data.filename ); context .pictrs_client() - .delete(url) + .post(url) .header("X-Api-Token", pictrs_config.api_key.unwrap_or_default()) .send() .await? .error_for_status()?; - LocalImage::delete_by_alias(&mut context.pool(), &data.filename).await?; - Ok(Json(SuccessResponse::default())) } From 6cfd825e3351c48a3cc69de8518b779ce3e8c23f Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 15 Jan 2025 10:16:31 +0100 Subject: [PATCH 50/51] reuse delete_image_from_pictrs --- crates/routes/src/images/delete.rs | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/crates/routes/src/images/delete.rs b/crates/routes/src/images/delete.rs index 909650fbb0..0321473174 100644 --- a/crates/routes/src/images/delete.rs +++ b/crates/routes/src/images/delete.rs @@ -3,6 +3,7 @@ use actix_web::web::*; use lemmy_api_common::{ context::LemmyContext, image::{CommunityIdQuery, DeleteImageParams}, + request::delete_image_from_pictrs, utils::{is_admin, is_mod_or_admin}, SuccessResponse, }; @@ -133,19 +134,7 @@ pub async fn delete_image( ) .await?; - let pictrs_config = context.settings().pictrs()?; - let url = format!( - "{}internal/delete?alias={}", - pictrs_config.url, &data.filename - ); - - context - .pictrs_client() - .post(url) - .header("X-Api-Token", pictrs_config.api_key.unwrap_or_default()) - .send() - .await? - .error_for_status()?; + delete_image_from_pictrs(&data.filename, &context).await?; Ok(Json(SuccessResponse::default())) } From 9d592d6c8c9db758619adecc9987eaebea35c7d5 Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 15 Jan 2025 10:35:06 +0100 Subject: [PATCH 51/51] update lib --- api_tests/package.json | 2 +- api_tests/pnpm-lock.yaml | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/api_tests/package.json b/api_tests/package.json index aa401ed9b7..7e1fd8127e 100644 --- a/api_tests/package.json +++ b/api_tests/package.json @@ -28,7 +28,7 @@ "eslint": "^9.18.0", "eslint-plugin-prettier": "^5.1.3", "jest": "^29.5.0", - "lemmy-js-client": "0.20.0-no-delete-token.1", + "lemmy-js-client": "0.20.0-no-delete-token.2", "prettier": "^3.4.2", "ts-jest": "^29.1.0", "typescript": "^5.7.3", diff --git a/api_tests/pnpm-lock.yaml b/api_tests/pnpm-lock.yaml index f742acf1bc..1a68b27772 100644 --- a/api_tests/pnpm-lock.yaml +++ b/api_tests/pnpm-lock.yaml @@ -30,8 +30,8 @@ importers: specifier: ^29.5.0 version: 29.7.0(@types/node@22.10.6) lemmy-js-client: - specifier: 0.20.0-no-delete-token.1 - version: 0.20.0-no-delete-token.1 + specifier: 0.20.0-no-delete-token.2 + version: 0.20.0-no-delete-token.2 prettier: specifier: ^3.4.2 version: 3.4.2 @@ -1157,8 +1157,8 @@ packages: resolution: {integrity: sha512-eTIzlVOSUR+JxdDFepEYcBMtZ9Qqdef+rnzWdRZuMbOywu5tO2w2N7rqjoANZ5k9vywhL6Br1VRjUIgTQx4E8w==} engines: {node: '>=6'} - lemmy-js-client@0.20.0-no-delete-token.1: - resolution: {integrity: sha512-i8BbuJxdH2+T1kflMaKFle6wDHsu4jYxRiPHaLVfFzNf4Ed0x5CSyzGkKJHFP9rwynjuL5g9W5J8g/tbCUmf1w==} + lemmy-js-client@0.20.0-no-delete-token.2: + resolution: {integrity: sha512-3ra3DpD8XR6RRwCeUDLI/ztFgVuF1IoUoft+xKVDALyupwRWUsA3JcHXRIcFd1a2Qt+pHJtWbc5Iwvybakxwdg==} leven@3.1.0: resolution: {integrity: sha512-qsda+H8jTaUaN/x5vzW2rzc+8Rw4TAQ/4KjB46IwK5VH+IlVeeeje/EoZRpiXvIqjFgK84QffqPztGI3VBLG1A==} @@ -3060,7 +3060,7 @@ snapshots: kleur@3.0.3: {} - lemmy-js-client@0.20.0-no-delete-token.1: {} + lemmy-js-client@0.20.0-no-delete-token.2: {} leven@3.1.0: {}