From a51ffe60bdc10a6374f9d08088267725210220cf Mon Sep 17 00:00:00 2001 From: Nina Blanson Date: Fri, 16 Jun 2023 08:25:57 -0500 Subject: [PATCH 01/11] Fixes #2900 - Checks slur regex to see if it is too permissive along with small validation organization --- crates/api_common/src/utils.rs | 11 +- crates/api_common/src/utils/site_utils.rs | 123 ++++++++++++++++++++++ crates/api_crud/src/site/create.rs | 37 ++++--- crates/api_crud/src/site/update.rs | 21 ++-- 4 files changed, 156 insertions(+), 36 deletions(-) create mode 100644 crates/api_common/src/utils/site_utils.rs diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 1912221e14..5fac4d4b13 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -1,3 +1,5 @@ +pub mod site_utils; + use crate::{ context::LemmyContext, request::purge_image_from_pictrs, @@ -312,15 +314,6 @@ pub fn password_length_check(pass: &str) -> Result<(), LemmyError> { } } -/// Checks the site description length -pub fn site_description_length_check(description: &str) -> Result<(), LemmyError> { - if description.len() > 150 { - Err(LemmyError::from_message("site_description_length_overflow")) - } else { - Ok(()) - } -} - /// Checks for a honeypot. If this field is filled, fail the rest of the function pub fn honeypot_check(honeypot: &Option) -> Result<(), LemmyError> { if honeypot.is_some() && honeypot != &Some(String::new()) { diff --git a/crates/api_common/src/utils/site_utils.rs b/crates/api_common/src/utils/site_utils.rs new file mode 100644 index 0000000000..0c373ff9f9 --- /dev/null +++ b/crates/api_common/src/utils/site_utils.rs @@ -0,0 +1,123 @@ +/// Helper functions for manipulating parts of a site. +use lemmy_utils::error::LemmyError; +use regex::{Regex, RegexBuilder}; + +const SITE_NAME_MAX_LENGTH: usize = 20; +const SITE_DESCRIPTION_MAX_LENGTH: usize = 150; + +/// Attempts to build the regex and checks it for common errors before inserting into the DB. +pub fn build_and_check_regex(regex_str: &Option<&str>) -> Result, LemmyError> { + regex_str.map_or_else( + || Ok(None::), + |slurs| { + RegexBuilder::new(slurs) + .case_insensitive(true) + .build() + .map_err(|e| LemmyError::from_error_message(e, "invalid_regex")) + .and_then(|regex| { + // NOTE: It is difficult to know, in the universe of user-crafted regex, which ones + // may match against any string text. To keep it simple, we'll match the regex + // against an innocuous string - a single number - which should help catch a regex + // that accidentally matches against all strings. + if regex.is_match("1") { + return Err(LemmyError::from_message("permissive_slur")); + } + + Ok(Some(regex)) + }) + }, + ) +} + +/// Checks the site name length, the limit as defined in the DB. +pub fn site_name_length_check(name: &str) -> Result<(), LemmyError> { + length_check( + name, + SITE_NAME_MAX_LENGTH, + String::from("site_name_length_overflow"), + ) +} + +/// Checks the site description length, the limit as defined in the DB. +pub fn site_description_length_check(description: &str) -> Result<(), LemmyError> { + length_check( + description, + SITE_DESCRIPTION_MAX_LENGTH, + String::from("site_description_length_overflow"), + ) +} + +fn length_check(item: &str, max_length: usize, msg: String) -> Result<(), LemmyError> { + if item.len() > max_length { + Err(LemmyError::from_message(&msg)) + } else { + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use crate::utils::site_utils::{ + build_and_check_regex, + site_description_length_check, + site_name_length_check, + }; + + #[test] + fn test_valid_slur_regex() { + let valid_regexes = [&None, &Some("(foo|bar)")]; + + valid_regexes.iter().for_each(|regex| { + let result = build_and_check_regex(regex); + + assert!(result.is_ok()); + }); + } + + #[test] + fn test_too_permissive_slur_regex() { + let match_everything_regexes = [ + &Some("["), // Invalid regex + &Some("(foo|bar|)"), // Matches all values + &Some(".*"), // Matches all values + ]; + + match_everything_regexes.iter().for_each(|regex| { + let result = build_and_check_regex(regex); + + assert!(result.is_err(), "Testing regex: {:?}", regex); + }); + } + + #[test] + fn test_test_valid_site_name() { + let result = site_name_length_check("awesome.comm"); + + assert!(result.is_ok()) + } + + #[test] + fn test_test_invalid_site_name() { + let result = site_name_length_check("too long community name"); + + assert!(result.err().is_some_and(|error| error + .message + .is_some_and(|msg| msg == "site_name_length_overflow"))); + } + + #[test] + fn test_test_valid_site_description() { + let result = site_description_length_check("cool cats"); + + assert!(result.is_ok()) + } + + #[test] + fn test_test_invalid_site_description() { + let result = site_description_length_check(&(0..10001).map(|_| 'A').collect::()); + + assert!(result.err().is_some_and(|error| error + .message + .is_some_and(|msg| msg == "site_description_length_overflow"))); + } +} diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index af85406698..a2d53c8f0a 100644 --- a/crates/api_crud/src/site/create.rs +++ b/crates/api_crud/src/site/create.rs @@ -8,9 +8,8 @@ use lemmy_api_common::{ generate_site_inbox_url, is_admin, local_site_rate_limit_to_rate_limit_config, - local_site_to_slur_regex, local_user_view_from_jwt, - site_description_length_check, + site_utils::{build_and_check_regex, site_description_length_check, site_name_length_check}, }, }; use lemmy_db_schema::{ @@ -41,31 +40,30 @@ impl PerformCrud for CreateSite { #[tracing::instrument(skip(context))] async fn perform(&self, context: &Data) -> Result { let data: &CreateSite = self; - + let local_user_view = local_user_view_from_jwt(&data.auth, context).await?; let local_site = LocalSite::read(context.pool()).await?; + // BEGIN VALIDATION + // Make sure user is an admin; other types of users should not update site data... + is_admin(&local_user_view)?; + + // Make sure the site hasn't already been set up... if local_site.site_setup { return Err(LemmyError::from_message("site_already_exists")); }; - let local_user_view = local_user_view_from_jwt(&data.auth, context).await?; - - let sidebar = diesel_option_overwrite(&data.sidebar); - let description = diesel_option_overwrite(&data.description); - let icon = diesel_option_overwrite_to_url(&data.icon)?; - let banner = diesel_option_overwrite_to_url(&data.banner)?; + // Check that the slur regex compiles, and returns the regex if valid... + let slur_regex = build_and_check_regex(&local_site.slur_filter_regex.as_deref())?; - let slur_regex = local_site_to_slur_regex(&local_site); + site_name_length_check(&data.name)?; check_slurs(&data.name, &slur_regex)?; - check_slurs_opt(&data.description, &slur_regex)?; - - // Make sure user is an admin - is_admin(&local_user_view)?; - if let Some(Some(desc)) = &description { + if let Some(desc) = &data.description { site_description_length_check(desc)?; + check_slurs_opt(&data.description, &slur_regex)?; } + // Ensure that the sidebar has fewer than the max num characters... is_valid_body_field(&data.sidebar)?; let application_question = diesel_option_overwrite(&data.application_question); @@ -75,16 +73,17 @@ impl PerformCrud for CreateSite { .registration_mode .unwrap_or(local_site.registration_mode), )?; + // END VALIDATION let actor_id: DbUrl = Url::parse(&context.settings().get_protocol_and_hostname())?.into(); let inbox_url = Some(generate_site_inbox_url(&actor_id)?); let keypair = generate_actor_keypair()?; let site_form = SiteUpdateForm::builder() .name(Some(data.name.clone())) - .sidebar(sidebar) - .description(description) - .icon(icon) - .banner(banner) + .sidebar(diesel_option_overwrite(&data.sidebar)) + .description(diesel_option_overwrite(&data.description)) + .icon(diesel_option_overwrite_to_url(&data.icon)?) + .banner(diesel_option_overwrite_to_url(&data.banner)?) .actor_id(Some(actor_id)) .last_refreshed_at(Some(naive_now())) .inbox_url(inbox_url) diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index ac1544ffca..272a14104d 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -6,9 +6,8 @@ use lemmy_api_common::{ utils::{ is_admin, local_site_rate_limit_to_rate_limit_config, - local_site_to_slur_regex, local_user_view_from_jwt, - site_description_length_check, + site_utils::{build_and_check_regex, site_description_length_check, site_name_length_check}, }, }; use lemmy_db_schema::{ @@ -45,18 +44,24 @@ impl PerformCrud for EditSite { let local_site = site_view.local_site; let site = site_view.site; - // Make sure user is an admin + // BEGIN VALIDATION + // Make sure user is an admin; other types of users should not update site data... is_admin(&local_user_view)?; - let slur_regex = local_site_to_slur_regex(&local_site); + // Check that the slur regex compiles, and return the regex if valid... + let slur_regex = build_and_check_regex(&local_site.slur_filter_regex.as_deref())?; - check_slurs_opt(&data.name, &slur_regex)?; - check_slurs_opt(&data.description, &slur_regex)?; + if let Some(name) = &data.name { + site_name_length_check(name)?; + check_slurs_opt(&data.name, &slur_regex)?; + } if let Some(desc) = &data.description { site_description_length_check(desc)?; + check_slurs_opt(&data.description, &slur_regex)?; } + // Ensure that the sidebar has fewer than the max num characters... is_valid_body_field(&data.sidebar)?; let application_question = diesel_option_overwrite(&data.application_question); @@ -88,14 +93,14 @@ impl PerformCrud for EditSite { "cant_enable_private_instance_and_federation_together", )); } + // END VALIDATION if let Some(discussion_languages) = data.discussion_languages.clone() { SiteLanguage::update(context.pool(), discussion_languages.clone(), &site).await?; } - let name = data.name.clone(); let site_form = SiteUpdateForm::builder() - .name(name) + .name(data.name.clone()) .sidebar(diesel_option_overwrite(&data.sidebar)) .description(diesel_option_overwrite(&data.description)) .icon(diesel_option_overwrite_to_url(&data.icon)?) From ca6c519172564847bf423997eb95660330be36c3 Mon Sep 17 00:00:00 2001 From: Nina Blanson Date: Fri, 16 Jun 2023 08:52:32 -0500 Subject: [PATCH 02/11] Clean up variable names, add handler for valid empty string usecase --- crates/api_common/src/utils/site_utils.rs | 24 ++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/crates/api_common/src/utils/site_utils.rs b/crates/api_common/src/utils/site_utils.rs index 0c373ff9f9..ef55210233 100644 --- a/crates/api_common/src/utils/site_utils.rs +++ b/crates/api_common/src/utils/site_utils.rs @@ -5,12 +5,18 @@ use regex::{Regex, RegexBuilder}; const SITE_NAME_MAX_LENGTH: usize = 20; const SITE_DESCRIPTION_MAX_LENGTH: usize = 150; -/// Attempts to build the regex and checks it for common errors before inserting into the DB. -pub fn build_and_check_regex(regex_str: &Option<&str>) -> Result, LemmyError> { - regex_str.map_or_else( +/// Attempts to build a regex and check it for common errors before inserting into the DB. +pub fn build_and_check_regex(regex_str_opt: &Option<&str>) -> Result, LemmyError> { + regex_str_opt.map_or_else( || Ok(None::), - |slurs| { - RegexBuilder::new(slurs) + |regex_str| { + if regex_str.is_empty() { + // If the proposed regex is empty, return as having no regex at all; this is the same + // behavior that happens downstream before the write to the database. + return Ok(None::); + } + + RegexBuilder::new(regex_str) .case_insensitive(true) .build() .map_err(|e| LemmyError::from_error_message(e, "invalid_regex")) @@ -20,7 +26,7 @@ pub fn build_and_check_regex(regex_str: &Option<&str>) -> Result, // against an innocuous string - a single number - which should help catch a regex // that accidentally matches against all strings. if regex.is_match("1") { - return Err(LemmyError::from_message("permissive_slur")); + return Err(LemmyError::from_message("permissive_regex")); } Ok(Some(regex)) @@ -65,12 +71,12 @@ mod tests { #[test] fn test_valid_slur_regex() { - let valid_regexes = [&None, &Some("(foo|bar)")]; + let valid_regexes = [&None, &Some(""), &Some("(foo|bar)")]; valid_regexes.iter().for_each(|regex| { let result = build_and_check_regex(regex); - assert!(result.is_ok()); + assert!(result.is_ok(), "Testing regex: {:?}", regex); }); } @@ -114,7 +120,7 @@ mod tests { #[test] fn test_test_invalid_site_description() { - let result = site_description_length_check(&(0..10001).map(|_| 'A').collect::()); + let result = site_description_length_check(&(0..151).map(|_| 'A').collect::()); assert!(result.err().is_some_and(|error| error .message From 68360806b157df36764477aaebd48841090b215e Mon Sep 17 00:00:00 2001 From: Nina Blanson Date: Fri, 16 Jun 2023 16:44:49 -0500 Subject: [PATCH 03/11] Update tests --- crates/api_common/src/utils/site_utils.rs | 24 +++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/crates/api_common/src/utils/site_utils.rs b/crates/api_common/src/utils/site_utils.rs index ef55210233..413d12212b 100644 --- a/crates/api_common/src/utils/site_utils.rs +++ b/crates/api_common/src/utils/site_utils.rs @@ -83,16 +83,24 @@ mod tests { #[test] fn test_too_permissive_slur_regex() { let match_everything_regexes = [ - &Some("["), // Invalid regex - &Some("(foo|bar|)"), // Matches all values - &Some(".*"), // Matches all values + (&Some("["), "invalid_regex"), + (&Some("(foo|bar|)"), "permissive_regex"), + (&Some(".*"), "permissive_regex"), ]; - match_everything_regexes.iter().for_each(|regex| { - let result = build_and_check_regex(regex); - - assert!(result.is_err(), "Testing regex: {:?}", regex); - }); + match_everything_regexes + .iter() + .for_each(|&(regex_str, expected_err)| { + let result = build_and_check_regex(regex_str); + + assert!( + result + .err() + .is_some_and(|error| error.message.is_some_and(|msg| msg == expected_err)), + "Testing regex: {:?}", + regex_str + ); + }); } #[test] From c01e9446193911c6ebb878fe5bca21e2722e712b Mon Sep 17 00:00:00 2001 From: Nina Blanson Date: Sat, 17 Jun 2023 12:15:38 -0500 Subject: [PATCH 04/11] Create validation function and add tests --- crates/api_common/src/utils.rs | 2 - crates/api_common/src/utils/site_utils.rs | 137 ----------- crates/api_crud/src/site/create.rs | 178 +++++++++++++-- crates/api_crud/src/site/update.rs | 262 ++++++++++++++++++---- crates/utils/src/utils/validation.rs | 171 +++++++++++++- 5 files changed, 536 insertions(+), 214 deletions(-) delete mode 100644 crates/api_common/src/utils/site_utils.rs diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 5200f2c255..298648fd37 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -1,5 +1,3 @@ -pub mod site_utils; - use crate::{ context::LemmyContext, request::purge_image_from_pictrs, diff --git a/crates/api_common/src/utils/site_utils.rs b/crates/api_common/src/utils/site_utils.rs deleted file mode 100644 index 413d12212b..0000000000 --- a/crates/api_common/src/utils/site_utils.rs +++ /dev/null @@ -1,137 +0,0 @@ -/// Helper functions for manipulating parts of a site. -use lemmy_utils::error::LemmyError; -use regex::{Regex, RegexBuilder}; - -const SITE_NAME_MAX_LENGTH: usize = 20; -const SITE_DESCRIPTION_MAX_LENGTH: usize = 150; - -/// Attempts to build a regex and check it for common errors before inserting into the DB. -pub fn build_and_check_regex(regex_str_opt: &Option<&str>) -> Result, LemmyError> { - regex_str_opt.map_or_else( - || Ok(None::), - |regex_str| { - if regex_str.is_empty() { - // If the proposed regex is empty, return as having no regex at all; this is the same - // behavior that happens downstream before the write to the database. - return Ok(None::); - } - - RegexBuilder::new(regex_str) - .case_insensitive(true) - .build() - .map_err(|e| LemmyError::from_error_message(e, "invalid_regex")) - .and_then(|regex| { - // NOTE: It is difficult to know, in the universe of user-crafted regex, which ones - // may match against any string text. To keep it simple, we'll match the regex - // against an innocuous string - a single number - which should help catch a regex - // that accidentally matches against all strings. - if regex.is_match("1") { - return Err(LemmyError::from_message("permissive_regex")); - } - - Ok(Some(regex)) - }) - }, - ) -} - -/// Checks the site name length, the limit as defined in the DB. -pub fn site_name_length_check(name: &str) -> Result<(), LemmyError> { - length_check( - name, - SITE_NAME_MAX_LENGTH, - String::from("site_name_length_overflow"), - ) -} - -/// Checks the site description length, the limit as defined in the DB. -pub fn site_description_length_check(description: &str) -> Result<(), LemmyError> { - length_check( - description, - SITE_DESCRIPTION_MAX_LENGTH, - String::from("site_description_length_overflow"), - ) -} - -fn length_check(item: &str, max_length: usize, msg: String) -> Result<(), LemmyError> { - if item.len() > max_length { - Err(LemmyError::from_message(&msg)) - } else { - Ok(()) - } -} - -#[cfg(test)] -mod tests { - use crate::utils::site_utils::{ - build_and_check_regex, - site_description_length_check, - site_name_length_check, - }; - - #[test] - fn test_valid_slur_regex() { - let valid_regexes = [&None, &Some(""), &Some("(foo|bar)")]; - - valid_regexes.iter().for_each(|regex| { - let result = build_and_check_regex(regex); - - assert!(result.is_ok(), "Testing regex: {:?}", regex); - }); - } - - #[test] - fn test_too_permissive_slur_regex() { - let match_everything_regexes = [ - (&Some("["), "invalid_regex"), - (&Some("(foo|bar|)"), "permissive_regex"), - (&Some(".*"), "permissive_regex"), - ]; - - match_everything_regexes - .iter() - .for_each(|&(regex_str, expected_err)| { - let result = build_and_check_regex(regex_str); - - assert!( - result - .err() - .is_some_and(|error| error.message.is_some_and(|msg| msg == expected_err)), - "Testing regex: {:?}", - regex_str - ); - }); - } - - #[test] - fn test_test_valid_site_name() { - let result = site_name_length_check("awesome.comm"); - - assert!(result.is_ok()) - } - - #[test] - fn test_test_invalid_site_name() { - let result = site_name_length_check("too long community name"); - - assert!(result.err().is_some_and(|error| error - .message - .is_some_and(|msg| msg == "site_name_length_overflow"))); - } - - #[test] - fn test_test_valid_site_description() { - let result = site_description_length_check("cool cats"); - - assert!(result.is_ok()) - } - - #[test] - fn test_test_invalid_site_description() { - let result = site_description_length_check(&(0..151).map(|_| 'A').collect::()); - - assert!(result.err().is_some_and(|error| error - .message - .is_some_and(|msg| msg == "site_description_length_overflow"))); - } -} diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index a2d53c8f0a..ab6b57ddb4 100644 --- a/crates/api_crud/src/site/create.rs +++ b/crates/api_crud/src/site/create.rs @@ -9,7 +9,6 @@ use lemmy_api_common::{ is_admin, local_site_rate_limit_to_rate_limit_config, local_user_view_from_jwt, - site_utils::{build_and_check_regex, site_description_length_check, site_name_length_check}, }, }; use lemmy_db_schema::{ @@ -25,10 +24,15 @@ use lemmy_db_schema::{ }; use lemmy_db_views::structs::SiteView; use lemmy_utils::{ - error::LemmyError, + error::{LemmyError, LemmyResult}, utils::{ slurs::{check_slurs, check_slurs_opt}, - validation::is_valid_body_field, + validation::{ + build_and_check_regex, + is_valid_body_field, + site_description_length_check, + site_name_length_check, + }, }, }; use url::Url; @@ -43,28 +47,10 @@ impl PerformCrud for CreateSite { let local_user_view = local_user_view_from_jwt(&data.auth, context).await?; let local_site = LocalSite::read(context.pool()).await?; - // BEGIN VALIDATION - // Make sure user is an admin; other types of users should not update site data... + // Make sure user is an admin; other types of users should not create site data... is_admin(&local_user_view)?; - // Make sure the site hasn't already been set up... - if local_site.site_setup { - return Err(LemmyError::from_message("site_already_exists")); - }; - - // Check that the slur regex compiles, and returns the regex if valid... - let slur_regex = build_and_check_regex(&local_site.slur_filter_regex.as_deref())?; - - site_name_length_check(&data.name)?; - check_slurs(&data.name, &slur_regex)?; - - if let Some(desc) = &data.description { - site_description_length_check(desc)?; - check_slurs_opt(&data.description, &slur_regex)?; - } - - // Ensure that the sidebar has fewer than the max num characters... - is_valid_body_field(&data.sidebar)?; + validate_create_payload(local_site.site_setup, local_site.slur_filter_regex, data)?; let application_question = diesel_option_overwrite(&data.application_question); check_application_question( @@ -73,7 +59,6 @@ impl PerformCrud for CreateSite { .registration_mode .unwrap_or(local_site.registration_mode), )?; - // END VALIDATION let actor_id: DbUrl = Url::parse(&context.settings().get_protocol_and_hostname())?.into(); let inbox_url = Some(generate_site_inbox_url(&actor_id)?); @@ -156,3 +141,148 @@ impl PerformCrud for CreateSite { }) } } + +fn validate_create_payload( + is_site_setup: bool, + site_regex: Option, + create_site: &CreateSite, +) -> LemmyResult<()> { + // Make sure the site hasn't already been set up... + if is_site_setup { + return Err(LemmyError::from_message("site_already_exists")); + }; + + // Check that the slur regex compiles, and returns the regex if valid... + let slur_regex = build_and_check_regex(&site_regex.as_deref())?; + + // Validate the site name... + site_name_length_check(&create_site.name)?; + check_slurs(&create_site.name, &slur_regex)?; + + // If provided, validate the site description... + if let Some(desc) = &create_site.description { + site_description_length_check(desc)?; + check_slurs_opt(&create_site.description, &slur_regex)?; + } + + // Ensure that the sidebar has fewer than the max num characters... + is_valid_body_field(&create_site.sidebar) +} + +#[cfg(test)] +mod tests { + use crate::site::create::validate_create_payload; + use lemmy_api_common::site::CreateSite; + + #[test] + fn test_validate_create() { + fn create_payload( + site_name: String, + site_description: Option, + site_sidebar: Option, + ) -> CreateSite { + CreateSite { + name: site_name, + sidebar: site_sidebar, + description: site_description, + icon: None, + banner: None, + enable_downvotes: None, + enable_nsfw: None, + community_creation_admin_only: None, + require_email_verification: None, + application_question: None, + private_instance: None, + default_theme: None, + default_post_listing_type: None, + legal_information: None, + application_email_admins: None, + hide_modlog_mod_names: None, + discussion_languages: None, + slur_filter_regex: None, + actor_name_max_length: None, + rate_limit_message: None, + rate_limit_message_per_second: None, + rate_limit_post: None, + rate_limit_post_per_second: None, + rate_limit_register: None, + rate_limit_register_per_second: None, + rate_limit_image: None, + rate_limit_image_per_second: None, + rate_limit_comment: None, + rate_limit_comment_per_second: None, + rate_limit_search: None, + rate_limit_search_per_second: None, + federation_enabled: None, + federation_debug: None, + federation_worker_count: None, + captcha_enabled: None, + captcha_difficulty: None, + allowed_instances: None, + blocked_instances: None, + taglines: None, + registration_mode: None, + auth: Default::default(), + } + } + + let invalid_payloads = [( + true, + &None::, + &create_payload(String::from("site_name"), None::, None::), + "site_already_exists", + )]; + let valid_payloads = [ + ( + false, + &None::, + &create_payload(String::from("site_name"), None::, None::), + ), + ( + false, + &None::, + &create_payload( + String::from("site_name"), + Some(String::new()), + Some(String::new()), + ), + ), + ]; + + invalid_payloads + .iter() + .enumerate() + .for_each(|(idx, &(is_site_setup, site_regex, create_site, expected_err))| { + match validate_create_payload(is_site_setup, site_regex.clone(), create_site) { + Ok(_) => { + panic!( + "Got Ok, but validation should have failed with error: {} for invalid_payloads.nth({})", + expected_err, + idx + ) + } + Err(error) => { + assert!( + error.message.eq(&Some(String::from(expected_err))), + "Got Err {:?}, but should have failed with message: {} for invalid_payloads.nth({})", + error.message, + expected_err, + idx + ) + } + } + }); + + valid_payloads.iter().enumerate().for_each( + |(idx, &(is_site_setup, site_regex, create_site))| { + let result = validate_create_payload(is_site_setup, site_regex.clone(), create_site); + + assert!( + result.is_ok(), + "Got Err, but should have got Ok for valid_payloads.nth({})", + idx + ); + }, + ) + } +} diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index 272a14104d..25e7cd30b3 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -3,12 +3,7 @@ use actix_web::web::Data; use lemmy_api_common::{ context::LemmyContext, site::{EditSite, SiteResponse}, - utils::{ - is_admin, - local_site_rate_limit_to_rate_limit_config, - local_user_view_from_jwt, - site_utils::{build_and_check_regex, site_description_length_check, site_name_length_check}, - }, + utils::{is_admin, local_site_rate_limit_to_rate_limit_config, local_user_view_from_jwt}, }; use lemmy_db_schema::{ source::{ @@ -28,8 +23,16 @@ use lemmy_db_schema::{ }; use lemmy_db_views::structs::SiteView; use lemmy_utils::{ - error::LemmyError, - utils::{slurs::check_slurs_opt, validation::is_valid_body_field}, + error::{LemmyError, LemmyResult}, + utils::{ + slurs::check_slurs_opt, + validation::{ + build_and_check_regex, + is_valid_body_field, + site_description_length_check, + site_name_length_check, + }, + }, }; #[async_trait::async_trait(?Send)] @@ -44,25 +47,15 @@ impl PerformCrud for EditSite { let local_site = site_view.local_site; let site = site_view.site; - // BEGIN VALIDATION // Make sure user is an admin; other types of users should not update site data... is_admin(&local_user_view)?; - // Check that the slur regex compiles, and return the regex if valid... - let slur_regex = build_and_check_regex(&local_site.slur_filter_regex.as_deref())?; - - if let Some(name) = &data.name { - site_name_length_check(name)?; - check_slurs_opt(&data.name, &slur_regex)?; - } - - if let Some(desc) = &data.description { - site_description_length_check(desc)?; - check_slurs_opt(&data.description, &slur_regex)?; - } - - // Ensure that the sidebar has fewer than the max num characters... - is_valid_body_field(&data.sidebar)?; + validate_update_payload( + local_site.slur_filter_regex, + local_site.federation_enabled, + local_site.private_instance, + data, + )?; let application_question = diesel_option_overwrite(&data.application_question); check_application_question( @@ -72,29 +65,6 @@ impl PerformCrud for EditSite { .unwrap_or(local_site.registration_mode), )?; - if let Some(listing_type) = &data.default_post_listing_type { - // only allow all or local as default listing types - if listing_type != &ListingType::All && listing_type != &ListingType::Local { - return Err(LemmyError::from_message( - "invalid_default_post_listing_type", - )); - } - } - - let enabled_private_instance_with_federation = data.private_instance == Some(true) - && data - .federation_enabled - .unwrap_or(local_site.federation_enabled); - let enabled_federation_with_private_instance = data.federation_enabled == Some(true) - && data.private_instance.unwrap_or(local_site.private_instance); - - if enabled_private_instance_with_federation || enabled_federation_with_private_instance { - return Err(LemmyError::from_message( - "cant_enable_private_instance_and_federation_together", - )); - } - // END VALIDATION - if let Some(discussion_languages) = data.discussion_languages.clone() { SiteLanguage::update(context.pool(), discussion_languages.clone(), &site).await?; } @@ -213,3 +183,201 @@ impl PerformCrud for EditSite { Ok(res) } } + +fn validate_update_payload( + site_regex: Option, + federation_enabled: bool, + private_instance: bool, + edit_site: &EditSite, +) -> LemmyResult<()> { + // Check that the slur regex compiles, and return the regex if valid... + let slur_regex = build_and_check_regex(&site_regex.as_deref())?; + + if let Some(name) = &edit_site.name { + // The name doesn't need to be updated, but if provided it cannot be blanked out... + site_name_length_check(name)?; + check_slurs_opt(&edit_site.name, &slur_regex)?; + } + + if let Some(desc) = &edit_site.description { + site_description_length_check(desc)?; + check_slurs_opt(&edit_site.description, &slur_regex)?; + } + + if let Some(listing_type) = &edit_site.default_post_listing_type { + // Only allow all or local as default listing types... + if listing_type != &ListingType::All && listing_type != &ListingType::Local { + return Err(LemmyError::from_message( + "invalid_default_post_listing_type", + )); + } + } + + let enabled_private_instance_with_federation = edit_site.private_instance == Some(true) + && edit_site.federation_enabled.unwrap_or(federation_enabled); + let enabled_federation_with_private_instance = edit_site.federation_enabled == Some(true) + && edit_site.private_instance.unwrap_or(private_instance); + + if enabled_private_instance_with_federation || enabled_federation_with_private_instance { + return Err(LemmyError::from_message( + "cant_enable_private_instance_and_federation_together", + )); + } + + // Ensure that the sidebar has fewer than the max num characters... + is_valid_body_field(&edit_site.sidebar) +} + +#[cfg(test)] +mod tests { + use crate::site::update::validate_update_payload; + use lemmy_api_common::site::EditSite; + use lemmy_db_schema::ListingType; + + #[test] + fn test_validate_create_invalid_payload() { + fn create_payload( + site_name: Option, + site_description: Option, + site_sidebar: Option, + site_listing_type: Option, + is_private: Option, + is_federation: Option, + ) -> EditSite { + EditSite { + name: site_name, + sidebar: site_sidebar, + description: site_description, + icon: None, + banner: None, + enable_downvotes: None, + enable_nsfw: None, + community_creation_admin_only: None, + require_email_verification: None, + application_question: None, + private_instance: is_private, + default_theme: None, + default_post_listing_type: site_listing_type, + legal_information: None, + application_email_admins: None, + hide_modlog_mod_names: None, + discussion_languages: None, + slur_filter_regex: None, + actor_name_max_length: None, + rate_limit_message: None, + rate_limit_message_per_second: None, + rate_limit_post: None, + rate_limit_post_per_second: None, + rate_limit_register: None, + rate_limit_register_per_second: None, + rate_limit_image: None, + rate_limit_image_per_second: None, + rate_limit_comment: None, + rate_limit_comment_per_second: None, + rate_limit_search: None, + rate_limit_search_per_second: None, + federation_enabled: is_federation, + federation_debug: None, + federation_worker_count: None, + captcha_enabled: None, + captcha_difficulty: None, + allowed_instances: None, + blocked_instances: None, + taglines: None, + registration_mode: None, + reports_email_admins: None, + auth: Default::default(), + } + } + + let invalid_payloads = [ + ( + &None, + &create_payload( + Some(String::from("site_name")), + None::, + None::, + Some(ListingType::Subscribed), + Some(true), + Some(false), + ), + "invalid_default_post_listing_type", + ), + ( + &None, + &create_payload( + Some(String::from("site_name")), + None::, + None::, + None::, + Some(true), + Some(true), + ), + "cant_enable_private_instance_and_federation_together", + ), + ]; + + let valid_payloads = [ + ( + &None::, + &create_payload( + None::, + None::, + None::, + None::, + Some(true), + Some(false), + ), + ), + ( + &Some(String::new()), + &create_payload( + Some(String::from("site_name")), + Some(String::new()), + Some(String::new()), + Some(ListingType::All), + Some(false), + Some(true), + ), + ), + ]; + + invalid_payloads.iter().enumerate().for_each( + |(idx, &(site_regex, edit_site, expected_err))| match validate_update_payload( + site_regex.clone(), + false, + true, + edit_site, + ) { + Ok(_) => { + panic!( + "Got Ok, but validation should have failed with error: {} for invalid_payloads.nth({})", + expected_err, idx + ) + } + Err(error) => { + assert!( + error.message.eq(&Some(String::from(expected_err))), + "Got Err {:?}, but should have failed with message: {} for invalid_payloads.nth({})", + error.message, + expected_err, + idx + ) + } + }, + ); + + valid_payloads + .iter() + .enumerate() + .for_each(|(idx, &(site_regex, edit_site))| { + let result = validate_update_payload(site_regex.clone(), true, false, edit_site); + + assert!( + result.is_ok(), + "Got Err, but should have got Ok for valid_payloads.nth({})", + idx + ); + }) + } +} diff --git a/crates/utils/src/utils/validation.rs b/crates/utils/src/utils/validation.rs index c4feb467bb..bebd4a173c 100644 --- a/crates/utils/src/utils/validation.rs +++ b/crates/utils/src/utils/validation.rs @@ -1,7 +1,7 @@ use crate::error::{LemmyError, LemmyResult}; use itertools::Itertools; use once_cell::sync::Lazy; -use regex::Regex; +use regex::{Regex, RegexBuilder}; use totp_rs::{Secret, TOTP}; use url::Url; @@ -17,8 +17,12 @@ static CLEAN_URL_PARAMS_REGEX: Lazy = Lazy::new(|| { Regex::new(r"^utm_source|utm_medium|utm_campaign|utm_term|utm_content|gclid|gclsrc|dclid|fbclid$") .expect("compile regex") }); + const BODY_MAX_LENGTH: usize = 10000; const BIO_MAX_LENGTH: usize = 300; +const SITE_NAME_MAX_LENGTH: usize = 20; +const SITE_NAME_MIN_LENGTH: usize = 1; +const SITE_DESCRIPTION_MAX_LENGTH: usize = 150; fn has_newline(name: &str) -> bool { name.contains('\n') @@ -82,14 +86,83 @@ pub fn is_valid_body_field(body: &Option) -> LemmyResult<()> { } pub fn is_valid_bio_field(bio: &str) -> LemmyResult<()> { - let check = bio.chars().count() <= BIO_MAX_LENGTH; - if !check { - Err(LemmyError::from_message("bio_length_overflow")) + max_length_check(bio, BIO_MAX_LENGTH, String::from("bio_length_overflow")) +} + +/// Checks the site name length, the limit as defined in the DB. +pub fn site_name_length_check(name: &str) -> LemmyResult<()> { + min_max_length_check( + name, + SITE_NAME_MIN_LENGTH, + SITE_NAME_MAX_LENGTH, + String::from("site_name_required"), + String::from("site_name_length_overflow"), + ) +} + +/// Checks the site description length, the limit as defined in the DB. +pub fn site_description_length_check(description: &str) -> LemmyResult<()> { + max_length_check( + description, + SITE_DESCRIPTION_MAX_LENGTH, + String::from("site_description_length_overflow"), + ) +} + +fn max_length_check(item: &str, max_length: usize, msg: String) -> LemmyResult<()> { + if item.len() > max_length { + Err(LemmyError::from_message(&msg)) + } else { + Ok(()) + } +} + +fn min_max_length_check( + item: &str, + min_length: usize, + max_length: usize, + min_msg: String, + max_msg: String, +) -> LemmyResult<()> { + if item.len() > max_length { + Err(LemmyError::from_message(&max_msg)) + } else if item.len() < min_length { + Err(LemmyError::from_message(&min_msg)) } else { Ok(()) } } +/// Attempts to build a regex and check it for common errors before inserting into the DB. +pub fn build_and_check_regex(regex_str_opt: &Option<&str>) -> LemmyResult> { + regex_str_opt.map_or_else( + || Ok(None::), + |regex_str| { + if regex_str.is_empty() { + // If the proposed regex is empty, return as having no regex at all; this is the same + // behavior that happens downstream before the write to the database. + return Ok(None::); + } + + RegexBuilder::new(regex_str) + .case_insensitive(true) + .build() + .map_err(|e| LemmyError::from_error_message(e, "invalid_regex")) + .and_then(|regex| { + // NOTE: It is difficult to know, in the universe of user-crafted regex, which ones + // may match against any string text. To keep it simple, we'll match the regex + // against an innocuous string - a single number - which should help catch a regex + // that accidentally matches against all strings. + if regex.is_match("1") { + return Err(LemmyError::from_message("permissive_regex")); + } + + Ok(Some(regex)) + }) + }, + ) +} + pub fn clean_url_params(url: &Url) -> Url { let mut url_out = url.clone(); if url.query().is_some() { @@ -153,12 +226,15 @@ pub fn build_totp_2fa(site_name: &str, username: &str, secret: &str) -> Result()); + + assert!(result.is_err()); + assert!(result + .unwrap_err() + .message + .eq(&Some(String::from("site_description_length_overflow")))); + } + + #[test] + fn test_valid_slur_regex() { + let valid_regexes = [&None, &Some(""), &Some("(foo|bar)")]; + + valid_regexes.iter().for_each(|regex| { + let result = build_and_check_regex(regex); + + assert!(result.is_ok(), "Testing regex: {:?}", regex); + }); + } + + #[test] + fn test_too_permissive_slur_regex() { + let match_everything_regexes = [ + (&Some("["), "invalid_regex"), + (&Some("(foo|bar|)"), "permissive_regex"), + (&Some(".*"), "permissive_regex"), + ]; + + match_everything_regexes + .iter() + .for_each(|&(regex_str, expected_err)| { + let result = build_and_check_regex(regex_str); + + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .message + .eq(&Some(String::from(expected_err))), + "Testing regex {:?}, expected error {}", + regex_str, + expected_err + ); + }); + } } From 3569b0f8e0aebdc2db62ed545f7ae6fb5f1afcc3 Mon Sep 17 00:00:00 2001 From: Nina Blanson Date: Sat, 17 Jun 2023 13:10:06 -0500 Subject: [PATCH 05/11] Test clean up --- crates/api_crud/src/site/create.rs | 4 +- crates/api_crud/src/site/update.rs | 4 +- crates/utils/src/utils/validation.rs | 79 ++++++++++++++++++++-------- 3 files changed, 60 insertions(+), 27 deletions(-) diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index ab6b57ddb4..02e0d64db4 100644 --- a/crates/api_crud/src/site/create.rs +++ b/crates/api_crud/src/site/create.rs @@ -275,10 +275,8 @@ mod tests { valid_payloads.iter().enumerate().for_each( |(idx, &(is_site_setup, site_regex, create_site))| { - let result = validate_create_payload(is_site_setup, site_regex.clone(), create_site); - assert!( - result.is_ok(), + validate_create_payload(is_site_setup, site_regex.clone(), create_site).is_ok(), "Got Err, but should have got Ok for valid_payloads.nth({})", idx ); diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index 25e7cd30b3..a6a9354552 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -371,10 +371,8 @@ mod tests { .iter() .enumerate() .for_each(|(idx, &(site_regex, edit_site))| { - let result = validate_update_payload(site_regex.clone(), true, false, edit_site); - assert!( - result.is_ok(), + validate_update_payload(site_regex.clone(), true, false, edit_site).is_ok(), "Got Err, but should have got Ok for valid_payloads.nth({})", idx ); diff --git a/crates/utils/src/utils/validation.rs b/crates/utils/src/utils/validation.rs index bebd4a173c..92b5ecba91 100644 --- a/crates/utils/src/utils/validation.rs +++ b/crates/utils/src/utils/validation.rs @@ -230,11 +230,15 @@ mod tests { clean_url_params, generate_totp_2fa_secret, is_valid_actor_name, + is_valid_bio_field, is_valid_display_name, is_valid_matrix_id, is_valid_post_title, site_description_length_check, site_name_length_check, + BIO_MAX_LENGTH, + SITE_DESCRIPTION_MAX_LENGTH, + SITE_NAME_MAX_LENGTH, }; use url::Url; @@ -304,19 +308,30 @@ mod tests { } #[test] - fn test_test_valid_site_name() { - let result = site_name_length_check("awesome.comm"); - - assert!(result.is_ok()) - } - - #[test] - fn test_test_invalid_site_name() { + fn test_valid_site_name() { + let valid_names = [ + (0..SITE_NAME_MAX_LENGTH).map(|_| 'A').collect::(), + String::from("A"), + ]; let invalid_names = [ - ("too long community name", "site_name_length_overflow"), - ("", "site_name_required"), + ( + &(0..SITE_NAME_MAX_LENGTH + 1) + .map(|_| 'A') + .collect::(), + "site_name_length_overflow", + ), + (&String::new(), "site_name_required"), ]; + valid_names.iter().for_each(|valid_name| { + assert!( + site_name_length_check(valid_name).is_ok(), + "Expected {} of length {} to be Ok.", + valid_name, + valid_name.len() + ) + }); + invalid_names .iter() .for_each(|&(invalid_name, expected_err)| { @@ -336,21 +351,43 @@ mod tests { } #[test] - fn test_test_valid_site_description() { - let result = site_description_length_check("cool cats"); + fn test_valid_bio() { + assert!(is_valid_bio_field(&(0..BIO_MAX_LENGTH).map(|_| 'A').collect::()).is_ok()); + + let invalid_result = + is_valid_bio_field(&(0..BIO_MAX_LENGTH + 1).map(|_| 'A').collect::()); - assert!(result.is_ok()) + assert!( + invalid_result.is_err() + && invalid_result + .unwrap_err() + .message + .eq(&Some(String::from("bio_length_overflow"))) + ); } #[test] - fn test_test_invalid_site_description() { - let result = site_description_length_check(&(0..151).map(|_| 'A').collect::()); - - assert!(result.is_err()); - assert!(result - .unwrap_err() - .message - .eq(&Some(String::from("site_description_length_overflow")))); + fn test_valid_site_description() { + assert!(site_description_length_check( + &(0..SITE_DESCRIPTION_MAX_LENGTH) + .map(|_| 'A') + .collect::() + ) + .is_ok()); + + let invalid_result = site_description_length_check( + &(0..SITE_DESCRIPTION_MAX_LENGTH + 1) + .map(|_| 'A') + .collect::(), + ); + + assert!( + invalid_result.is_err() + && invalid_result + .unwrap_err() + .message + .eq(&Some(String::from("site_description_length_overflow"))) + ); } #[test] From 3526feb0899cbf29e09bbaaf8e71a1f79ae15216 Mon Sep 17 00:00:00 2001 From: Nina Blanson Date: Sat, 17 Jun 2023 15:49:01 -0500 Subject: [PATCH 06/11] Use payload value vs local site value to prevent stunlocking --- crates/api_crud/src/site/create.rs | 67 +++++++++++------------- crates/api_crud/src/site/update.rs | 82 +++++++++++++----------------- crates/apub/src/api/read_person.rs | 1 + 3 files changed, 66 insertions(+), 84 deletions(-) diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index 02e0d64db4..36fd2226d5 100644 --- a/crates/api_crud/src/site/create.rs +++ b/crates/api_crud/src/site/create.rs @@ -50,7 +50,7 @@ impl PerformCrud for CreateSite { // Make sure user is an admin; other types of users should not create site data... is_admin(&local_user_view)?; - validate_create_payload(local_site.site_setup, local_site.slur_filter_regex, data)?; + validate_create_payload(local_site.site_setup, data)?; let application_question = diesel_option_overwrite(&data.application_question); check_application_question( @@ -142,18 +142,14 @@ impl PerformCrud for CreateSite { } } -fn validate_create_payload( - is_site_setup: bool, - site_regex: Option, - create_site: &CreateSite, -) -> LemmyResult<()> { +fn validate_create_payload(is_site_setup: bool, create_site: &CreateSite) -> LemmyResult<()> { // Make sure the site hasn't already been set up... if is_site_setup { return Err(LemmyError::from_message("site_already_exists")); }; // Check that the slur regex compiles, and returns the regex if valid... - let slur_regex = build_and_check_regex(&site_regex.as_deref())?; + let slur_regex = build_and_check_regex(&create_site.slur_filter_regex.as_deref())?; // Validate the site name... site_name_length_check(&create_site.name)?; @@ -228,19 +224,16 @@ mod tests { let invalid_payloads = [( true, - &None::, &create_payload(String::from("site_name"), None::, None::), "site_already_exists", )]; let valid_payloads = [ ( false, - &None::, &create_payload(String::from("site_name"), None::, None::), ), ( false, - &None::, &create_payload( String::from("site_name"), Some(String::new()), @@ -249,38 +242,38 @@ mod tests { ), ]; - invalid_payloads - .iter() - .enumerate() - .for_each(|(idx, &(is_site_setup, site_regex, create_site, expected_err))| { - match validate_create_payload(is_site_setup, site_regex.clone(), create_site) { - Ok(_) => { - panic!( - "Got Ok, but validation should have failed with error: {} for invalid_payloads.nth({})", - expected_err, - idx - ) - } - Err(error) => { - assert!( - error.message.eq(&Some(String::from(expected_err))), - "Got Err {:?}, but should have failed with message: {} for invalid_payloads.nth({})", - error.message, - expected_err, - idx - ) - } + invalid_payloads.iter().enumerate().for_each( + |(idx, &(is_site_setup, create_site, expected_err))| match validate_create_payload( + is_site_setup, + create_site, + ) { + Ok(_) => { + panic!( + "Got Ok, but validation should have failed with error: {} for invalid_payloads.nth({})", + expected_err, idx + ) } - }); + Err(error) => { + assert!( + error.message.eq(&Some(String::from(expected_err))), + "Got Err {:?}, but should have failed with message: {} for invalid_payloads.nth({})", + error.message, + expected_err, + idx + ) + } + }, + ); - valid_payloads.iter().enumerate().for_each( - |(idx, &(is_site_setup, site_regex, create_site))| { + valid_payloads + .iter() + .enumerate() + .for_each(|(idx, &(is_site_setup, create_site))| { assert!( - validate_create_payload(is_site_setup, site_regex.clone(), create_site).is_ok(), + validate_create_payload(is_site_setup, create_site).is_ok(), "Got Err, but should have got Ok for valid_payloads.nth({})", idx ); - }, - ) + }) } } diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index a6a9354552..170f5b54a9 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -51,7 +51,6 @@ impl PerformCrud for EditSite { is_admin(&local_user_view)?; validate_update_payload( - local_site.slur_filter_regex, local_site.federation_enabled, local_site.private_instance, data, @@ -185,13 +184,12 @@ impl PerformCrud for EditSite { } fn validate_update_payload( - site_regex: Option, federation_enabled: bool, private_instance: bool, edit_site: &EditSite, ) -> LemmyResult<()> { // Check that the slur regex compiles, and return the regex if valid... - let slur_regex = build_and_check_regex(&site_regex.as_deref())?; + let slur_regex = build_and_check_regex(&edit_site.slur_filter_regex.as_deref())?; if let Some(name) = &edit_site.name { // The name doesn't need to be updated, but if provided it cannot be blanked out... @@ -292,7 +290,6 @@ mod tests { let invalid_payloads = [ ( - &None, &create_payload( Some(String::from("site_name")), None::, @@ -304,7 +301,6 @@ mod tests { "invalid_default_post_listing_type", ), ( - &None, &create_payload( Some(String::from("site_name")), None::, @@ -318,61 +314,53 @@ mod tests { ]; let valid_payloads = [ - ( - &None::, - &create_payload( - None::, - None::, - None::, - None::, - Some(true), - Some(false), - ), + create_payload( + None::, + None::, + None::, + None::, + Some(true), + Some(false), ), - ( - &Some(String::new()), - &create_payload( - Some(String::from("site_name")), - Some(String::new()), - Some(String::new()), - Some(ListingType::All), - Some(false), - Some(true), - ), + create_payload( + Some(String::from("site_name")), + Some(String::new()), + Some(String::new()), + Some(ListingType::All), + Some(false), + Some(true), ), ]; - invalid_payloads.iter().enumerate().for_each( - |(idx, &(site_regex, edit_site, expected_err))| match validate_update_payload( - site_regex.clone(), - false, - true, - edit_site, - ) { - Ok(_) => { - panic!( + invalid_payloads + .iter() + .enumerate() + .for_each(|(idx, &(edit_site, expected_err))| { + match validate_update_payload(false, true, edit_site) { + Ok(_) => { + panic!( "Got Ok, but validation should have failed with error: {} for invalid_payloads.nth({})", expected_err, idx ) + } + Err(error) => { + assert!( + error.message.eq(&Some(String::from(expected_err))), + "Got Err {:?}, but should have failed with message: {} for invalid_payloads.nth({})", + error.message, + expected_err, + idx + ) + } } - Err(error) => { - assert!( - error.message.eq(&Some(String::from(expected_err))), - "Got Err {:?}, but should have failed with message: {} for invalid_payloads.nth({})", - error.message, - expected_err, - idx - ) - } - }, - ); + }); valid_payloads .iter() .enumerate() - .for_each(|(idx, &(site_regex, edit_site))| { + .for_each(|(idx, edit_site)| { assert!( - validate_update_payload(site_regex.clone(), true, false, edit_site).is_ok(), + validate_update_payload(true, false, edit_site).is_ok(), "Got Err, but should have got Ok for valid_payloads.nth({})", idx ); diff --git a/crates/apub/src/api/read_person.rs b/crates/apub/src/api/read_person.rs index 1e4797fa4f..5c97e8bbc7 100644 --- a/crates/apub/src/api/read_person.rs +++ b/crates/apub/src/api/read_person.rs @@ -23,6 +23,7 @@ impl PerformApub for GetPersonDetails { context: &Data, ) -> Result { let data: &GetPersonDetails = self; + println!("{:?}", data); // Check to make sure a person name or an id is given if data.username.is_none() && data.person_id.is_none() { From aba61cf7386368474f2d89d26c15a727fd83b83e Mon Sep 17 00:00:00 2001 From: Nina Blanson Date: Sat, 17 Jun 2023 16:14:16 -0500 Subject: [PATCH 07/11] Remove println added while testing --- crates/apub/src/api/read_person.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/apub/src/api/read_person.rs b/crates/apub/src/api/read_person.rs index 5c97e8bbc7..1e4797fa4f 100644 --- a/crates/apub/src/api/read_person.rs +++ b/crates/apub/src/api/read_person.rs @@ -23,7 +23,6 @@ impl PerformApub for GetPersonDetails { context: &Data, ) -> Result { let data: &GetPersonDetails = self; - println!("{:?}", data); // Check to make sure a person name or an id is given if data.username.is_none() && data.person_id.is_none() { From 753d50c2269a9aec8ebbc5ce033652a012f9ee79 Mon Sep 17 00:00:00 2001 From: Nina Blanson Date: Sat, 17 Jun 2023 17:06:06 -0500 Subject: [PATCH 08/11] Fall back to local site regex if not provided from request --- crates/api_crud/src/site/create.rs | 120 ++++++++++++++++++++-------- crates/api_crud/src/site/update.rs | 123 +++++++++++++++++++++-------- 2 files changed, 180 insertions(+), 63 deletions(-) diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index 36fd2226d5..c4625a8890 100644 --- a/crates/api_crud/src/site/create.rs +++ b/crates/api_crud/src/site/create.rs @@ -50,7 +50,7 @@ impl PerformCrud for CreateSite { // Make sure user is an admin; other types of users should not create site data... is_admin(&local_user_view)?; - validate_create_payload(local_site.site_setup, data)?; + validate_create_payload(local_site.site_setup, local_site.slur_filter_regex, data)?; let application_question = diesel_option_overwrite(&data.application_question); check_application_question( @@ -142,14 +142,24 @@ impl PerformCrud for CreateSite { } } -fn validate_create_payload(is_site_setup: bool, create_site: &CreateSite) -> LemmyResult<()> { +fn validate_create_payload( + is_site_setup: bool, + local_site_slur_filter_regex: Option, + create_site: &CreateSite, +) -> LemmyResult<()> { // Make sure the site hasn't already been set up... if is_site_setup { return Err(LemmyError::from_message("site_already_exists")); }; // Check that the slur regex compiles, and returns the regex if valid... - let slur_regex = build_and_check_regex(&create_site.slur_filter_regex.as_deref())?; + // Prioritize using new slur regex from the request; if not provided, use the existing regex. + let slur_regex = build_and_check_regex( + &create_site + .slur_filter_regex + .as_deref() + .or(local_site_slur_filter_regex.as_deref()), + )?; // Validate the site name... site_name_length_check(&create_site.name)?; @@ -176,6 +186,7 @@ mod tests { site_name: String, site_description: Option, site_sidebar: Option, + site_slur_filter_regex: Option, ) -> CreateSite { CreateSite { name: site_name, @@ -195,7 +206,7 @@ mod tests { application_email_admins: None, hide_modlog_mod_names: None, discussion_languages: None, - slur_filter_regex: None, + slur_filter_regex: site_slur_filter_regex, actor_name_max_length: None, rate_limit_message: None, rate_limit_message_per_second: None, @@ -222,58 +233,105 @@ mod tests { } } - let invalid_payloads = [( - true, - &create_payload(String::from("site_name"), None::, None::), - "site_already_exists", - )]; + let invalid_payloads = [ + ( + false, + &Some(String::from("(foo|bar)")), + &create_payload( + String::from("foo site_name"), + None::, + None::, + None::, + ), + "slurs", + ), + ( + false, + &Some(String::from("(foo|bar)")), + &create_payload( + String::from("zeta site_name"), + None::, + None::, + Some(String::from("(zeta|alpha)")), + ), + "slurs", + ), + ( + true, + &None::, + &create_payload( + String::from("site_name"), + None::, + None::, + None::, + ), + "site_already_exists", + ), + ]; let valid_payloads = [ ( false, - &create_payload(String::from("site_name"), None::, None::), + &None::, + &create_payload( + String::from("site_name"), + None::, + None::, + None::, + ), ), ( false, + &None::, &create_payload( String::from("site_name"), Some(String::new()), Some(String::new()), + None::, + ), + ), + ( + false, + &Some(String::from("(foo|bar)")), + &create_payload( + String::from("foo site_name"), + Some(String::new()), + Some(String::new()), + Some(String::new()), ), ), ]; invalid_payloads.iter().enumerate().for_each( - |(idx, &(is_site_setup, create_site, expected_err))| match validate_create_payload( - is_site_setup, - create_site, - ) { - Ok(_) => { - panic!( + |(idx, &(is_site_setup, local_slur_filter_regex, create_site, expected_err))| { + match validate_create_payload(is_site_setup, local_slur_filter_regex.clone(), create_site) { + Ok(_) => { + panic!( "Got Ok, but validation should have failed with error: {} for invalid_payloads.nth({})", expected_err, idx ) - } - Err(error) => { - assert!( - error.message.eq(&Some(String::from(expected_err))), - "Got Err {:?}, but should have failed with message: {} for invalid_payloads.nth({})", - error.message, - expected_err, - idx - ) + } + Err(error) => { + assert!( + error.message.eq(&Some(String::from(expected_err))), + "Got Err {:?}, but should have failed with message: {} for invalid_payloads.nth({})", + error.message, + expected_err, + idx + ) + } } }, ); - valid_payloads - .iter() - .enumerate() - .for_each(|(idx, &(is_site_setup, create_site))| { + valid_payloads.iter().enumerate().for_each( + |(idx, &(is_site_setup, local_slur_filter_regex, create_site))| { assert!( - validate_create_payload(is_site_setup, create_site).is_ok(), + validate_create_payload(is_site_setup, local_slur_filter_regex.clone(), create_site) + .is_ok(), "Got Err, but should have got Ok for valid_payloads.nth({})", idx ); - }) + }, + ) } } diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index 170f5b54a9..46fa525338 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -51,6 +51,7 @@ impl PerformCrud for EditSite { is_admin(&local_user_view)?; validate_update_payload( + local_site.slur_filter_regex, local_site.federation_enabled, local_site.private_instance, data, @@ -184,12 +185,19 @@ impl PerformCrud for EditSite { } fn validate_update_payload( + local_site_slur_filter_regex: Option, federation_enabled: bool, private_instance: bool, edit_site: &EditSite, ) -> LemmyResult<()> { // Check that the slur regex compiles, and return the regex if valid... - let slur_regex = build_and_check_regex(&edit_site.slur_filter_regex.as_deref())?; + // Prioritize using new slur regex from the request; if not provided, use the existing regex. + let slur_regex = build_and_check_regex( + &edit_site + .slur_filter_regex + .as_deref() + .or(local_site_slur_filter_regex.as_deref()), + )?; if let Some(name) = &edit_site.name { // The name doesn't need to be updated, but if provided it cannot be blanked out... @@ -239,8 +247,9 @@ mod tests { site_description: Option, site_sidebar: Option, site_listing_type: Option, - is_private: Option, - is_federation: Option, + site_slur_filter_regex: Option, + site_is_private: Option, + site_is_federated: Option, ) -> EditSite { EditSite { name: site_name, @@ -253,14 +262,14 @@ mod tests { community_creation_admin_only: None, require_email_verification: None, application_question: None, - private_instance: is_private, + private_instance: site_is_private, default_theme: None, default_post_listing_type: site_listing_type, legal_information: None, application_email_admins: None, hide_modlog_mod_names: None, discussion_languages: None, - slur_filter_regex: None, + slur_filter_regex: site_slur_filter_regex, actor_name_max_length: None, rate_limit_message: None, rate_limit_message_per_second: None, @@ -274,7 +283,7 @@ mod tests { rate_limit_comment_per_second: None, rate_limit_search: None, rate_limit_search_per_second: None, - federation_enabled: is_federation, + federation_enabled: site_is_federated, federation_debug: None, federation_worker_count: None, captcha_enabled: None, @@ -290,22 +299,52 @@ mod tests { let invalid_payloads = [ ( + &Some(String::from("(foo|bar)")), + &create_payload( + Some(String::from("foo site_name")), + None::, + None::, + None::, + None::, + Some(true), + Some(false), + ), + "slurs", + ), + ( + &Some(String::from("(foo|bar)")), + &create_payload( + Some(String::from("zeta site_name")), + None::, + None::, + None::, + Some(String::from("(zeta|alpha)")), + Some(true), + Some(false), + ), + "slurs", + ), + ( + &None::, &create_payload( Some(String::from("site_name")), None::, None::, Some(ListingType::Subscribed), + None::, Some(true), Some(false), ), "invalid_default_post_listing_type", ), ( + &None::, &create_payload( Some(String::from("site_name")), None::, None::, None::, + None::, Some(true), Some(true), ), @@ -314,29 +353,48 @@ mod tests { ]; let valid_payloads = [ - create_payload( - None::, - None::, - None::, - None::, - Some(true), - Some(false), + ( + &None::, + &create_payload( + None::, + None::, + None::, + None::, + None::, + Some(true), + Some(false), + ), + ), + ( + &None::, + &create_payload( + Some(String::from("site_name")), + Some(String::new()), + Some(String::new()), + Some(ListingType::All), + None::, + Some(false), + Some(true), + ), ), - create_payload( - Some(String::from("site_name")), - Some(String::new()), - Some(String::new()), - Some(ListingType::All), - Some(false), - Some(true), + ( + &Some(String::from("(foo|bar)")), + &create_payload( + Some(String::from("foo site_name")), + Some(String::new()), + Some(String::new()), + Some(ListingType::All), + Some(String::new()), + Some(false), + Some(true), + ), ), ]; - invalid_payloads - .iter() - .enumerate() - .for_each(|(idx, &(edit_site, expected_err))| { - match validate_update_payload(false, true, edit_site) { + invalid_payloads.iter().enumerate().for_each( + |(idx, &(local_site_slur_filter_regex, edit_site, expected_err))| { + match validate_update_payload(local_site_slur_filter_regex.clone(), false, true, edit_site) + { Ok(_) => { panic!( "Got Ok, but validation should have failed with error: {} for invalid_payloads.nth({})", @@ -353,17 +411,18 @@ mod tests { ) } } - }); + }, + ); - valid_payloads - .iter() - .enumerate() - .for_each(|(idx, edit_site)| { + valid_payloads.iter().enumerate().for_each( + |(idx, &(local_site_slur_filter_regex, edit_site))| { assert!( - validate_update_payload(true, false, edit_site).is_ok(), + validate_update_payload(local_site_slur_filter_regex.clone(), true, false, edit_site) + .is_ok(), "Got Err, but should have got Ok for valid_payloads.nth({})", idx ); - }) + }, + ) } } From 33eff7e8e1c5b4251a293d55c47c15c7b27fc829 Mon Sep 17 00:00:00 2001 From: Nina Blanson Date: Sun, 18 Jun 2023 04:25:48 -0500 Subject: [PATCH 09/11] Attempt clean up of flaky comment_view tests --- crates/api_crud/src/site/update.rs | 2 +- crates/db_views/src/comment_view.rs | 97 +++++++++++++++++------------ 2 files changed, 58 insertions(+), 41 deletions(-) diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index 46fa525338..dc7400aeca 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -241,7 +241,7 @@ mod tests { use lemmy_db_schema::ListingType; #[test] - fn test_validate_create_invalid_payload() { + fn test_validate_update_payload() { fn create_payload( site_name: Option, site_description: Option, diff --git a/crates/db_views/src/comment_view.rs b/crates/db_views/src/comment_view.rs index 6ac629b5a0..e16d99076a 100644 --- a/crates/db_views/src/comment_view.rs +++ b/crates/db_views/src/comment_view.rs @@ -429,10 +429,30 @@ mod tests { } async fn init_data(pool: &DbPool) -> Data { + // Languages + let english_id = Language::read_id_from_code(pool, Some("en")).await.unwrap(); + let finnish_id = Language::read_id_from_code(pool, Some("fi")).await.unwrap(); + let polish_id = Language::read_id_from_code(pool, Some("pl")) + .await + .unwrap() + .unwrap(); + + // Create instance let inserted_instance = Instance::read_or_create(pool, "my_domain.tld".to_string()) .await .unwrap(); + // Create community + let new_community = CommunityInsertForm::builder() + .name("test community 5".to_string()) + .title("nada".to_owned()) + .public_key("pubkey".to_string()) + .instance_id(inserted_instance.id) + .build(); + + let inserted_community = Community::create(pool, &new_community).await.unwrap(); + + // Create person Timmy let new_person = PersonInsertForm::builder() .name("timmy".into()) .public_key("pubkey".to_string()) @@ -445,6 +465,7 @@ mod tests { .build(); let inserted_local_user = LocalUser::create(pool, &local_user_form).await.unwrap(); + // Create person Sara let new_person_2 = PersonInsertForm::builder() .name("sara".into()) .public_key("pubkey".to_string()) @@ -452,107 +473,92 @@ mod tests { .build(); let inserted_person_2 = Person::create(pool, &new_person_2).await.unwrap(); - let new_community = CommunityInsertForm::builder() - .name("test community 5".to_string()) - .title("nada".to_owned()) - .public_key("pubkey".to_string()) - .instance_id(inserted_instance.id) - .build(); - - let inserted_community = Community::create(pool, &new_community).await.unwrap(); + // Create a comment tree with this hierarchy between Timmy and Sara + // 0 + // \ \ + // 1 2 + // \ + // 3 4 + // \ + // 5 + // Timmy creates a post and adds comment 0 let new_post = PostInsertForm::builder() .name("A test post 2".into()) .creator_id(inserted_person.id) .community_id(inserted_community.id) .build(); - let inserted_post = Post::create(pool, &new_post).await.unwrap(); - let english_id = Language::read_id_from_code(pool, Some("en")).await.unwrap(); - // Create a comment tree with this hierarchy - // 0 - // \ \ - // 1 2 - // \ - // 3 4 - // \ - // 5 let comment_form_0 = CommentInsertForm::builder() .content("Comment 0".into()) .creator_id(inserted_person.id) .post_id(inserted_post.id) .language_id(english_id) .build(); - let inserted_comment_0 = Comment::create(pool, &comment_form_0, None).await.unwrap(); + // Sara adds comment 1 to the post let comment_form_1 = CommentInsertForm::builder() .content("Comment 1, A test blocked comment".into()) .creator_id(inserted_person_2.id) .post_id(inserted_post.id) .language_id(english_id) .build(); - let inserted_comment_1 = Comment::create(pool, &comment_form_1, Some(&inserted_comment_0.path)) .await .unwrap(); - let finnish_id = Language::read_id_from_code(pool, Some("fi")).await.unwrap(); + // Timmy adds comment 2 to the post let comment_form_2 = CommentInsertForm::builder() .content("Comment 2".into()) .creator_id(inserted_person.id) .post_id(inserted_post.id) .language_id(finnish_id) .build(); - let inserted_comment_2 = Comment::create(pool, &comment_form_2, Some(&inserted_comment_0.path)) .await .unwrap(); + // Timmy replies to comment 1, adding comment 3 to the post. let comment_form_3 = CommentInsertForm::builder() .content("Comment 3".into()) .creator_id(inserted_person.id) .post_id(inserted_post.id) .language_id(english_id) .build(); - let _inserted_comment_3 = Comment::create(pool, &comment_form_3, Some(&inserted_comment_1.path)) .await .unwrap(); - let polish_id = Language::read_id_from_code(pool, Some("pl")) - .await - .unwrap() - .unwrap(); + // Timmy replies again to comment 1, adding comment 4 to the post. let comment_form_4 = CommentInsertForm::builder() .content("Comment 4".into()) .creator_id(inserted_person.id) .post_id(inserted_post.id) .language_id(Some(polish_id)) .build(); - let inserted_comment_4 = Comment::create(pool, &comment_form_4, Some(&inserted_comment_1.path)) .await .unwrap(); + // Timmy replies to his own comment, 4, adding comment 5 to the post. let comment_form_5 = CommentInsertForm::builder() .content("Comment 5".into()) .creator_id(inserted_person.id) .post_id(inserted_post.id) .build(); - let _inserted_comment_5 = Comment::create(pool, &comment_form_5, Some(&inserted_comment_4.path)) .await .unwrap(); + // Timmy blocks Sara let timmy_blocks_sara_form = PersonBlockForm { person_id: inserted_person.id, target_id: inserted_person_2.id, }; - let inserted_block = PersonBlock::block(pool, &timmy_blocks_sara_form) .await .unwrap(); @@ -565,13 +571,13 @@ mod tests { }; assert_eq!(expected_block, inserted_block); + // Timmy adds a like to comment 0. let comment_like_form = CommentLikeForm { comment_id: inserted_comment_0.id, post_id: inserted_post.id, person_id: inserted_person.id, score: 1, }; - let _inserted_comment_like = CommentLike::like(pool, &comment_like_form).await.unwrap(); Data { @@ -600,7 +606,7 @@ mod tests { let read_comment_views_no_person = CommentQuery::builder() .pool(pool) - .sort(Some(CommentSortType::Hot)) + .sort(Some(CommentSortType::Old)) .post_id(Some(data.inserted_post.id)) .build() .list() @@ -608,13 +614,14 @@ mod tests { .unwrap(); assert_eq!( - expected_comment_view_no_person, - read_comment_views_no_person[0] + expected_comment_view_no_person, read_comment_views_no_person[0], + "Expected comment 0, got comment: {}. All comments: {:?}", + read_comment_views_no_person[0].comment.content, read_comment_views_no_person, ); let read_comment_views_with_person = CommentQuery::builder() .pool(pool) - .sort(Some(CommentSortType::Hot)) + .sort(Some(CommentSortType::Old)) .post_id(Some(data.inserted_post.id)) .local_user(Some(&data.inserted_local_user)) .build() @@ -623,12 +630,17 @@ mod tests { .unwrap(); assert_eq!( - expected_comment_view_with_person, - read_comment_views_with_person[0] + expected_comment_view_with_person, read_comment_views_with_person[0], + "Expected comment 0, got comment: {}. All comments: {:?}", + read_comment_views_no_person[0].comment.content, read_comment_views_no_person, ); - // Make sure its 1, not showing the blocked comment - assert_eq!(5, read_comment_views_with_person.len()); + assert_eq!( + 5, + read_comment_views_with_person.len(), + "Expected no blocked content, but received incorrect num comments {:?}", + read_comment_views_with_person + ); let read_comment_from_blocked_person = CommentView::read( pool, @@ -735,7 +747,12 @@ mod tests { .list() .await .unwrap(); - assert_eq!(5, all_languages.len()); + assert_eq!( + 5, + all_languages.len(), + "Expected 5 languages, received {:?}", + all_languages + ); // change user lang to finnish, should only show one post in finnish and one undetermined let finnish_id = Language::read_id_from_code(pool, Some("fi")) From cf1f0e4378ce424d16d8724a815a928858059313 Mon Sep 17 00:00:00 2001 From: Nina Blanson Date: Mon, 19 Jun 2023 18:08:33 -0500 Subject: [PATCH 10/11] Pull in latest submodule --- crates/utils/translations | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/utils/translations b/crates/utils/translations index f45ddff206..7fc71d0860 160000 --- a/crates/utils/translations +++ b/crates/utils/translations @@ -1 +1 @@ -Subproject commit f45ddff206adb52ab0ac7555bf14978edac5d2f2 +Subproject commit 7fc71d0860bbe5c6d620ec27112350ffe5b9229c From 597d8f15438f8828ac5f73f305c9969938c3332e Mon Sep 17 00:00:00 2001 From: Nina Blanson Date: Tue, 27 Jun 2023 01:08:27 -0500 Subject: [PATCH 11/11] Move application, post check into functions, add more tests and improve test readability --- crates/api_crud/src/site/create.rs | 483 +++++++++++++++++++++-------- crates/api_crud/src/site/mod.rs | 88 +++++- crates/api_crud/src/site/update.rs | 455 ++++++++++++++++++--------- 3 files changed, 751 insertions(+), 275 deletions(-) diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs index 3e03485e8d..838d5bc409 100644 --- a/crates/api_crud/src/site/create.rs +++ b/crates/api_crud/src/site/create.rs @@ -1,4 +1,7 @@ -use crate::{site::check_application_question, PerformCrud}; +use crate::{ + site::{application_question_check, site_default_post_listing_type_check}, + PerformCrud, +}; use activitypub_federation::http_signatures::generate_actor_keypair; use actix_web::web::Data; use lemmy_api_common::{ @@ -51,22 +54,7 @@ impl PerformCrud for CreateSite { // Make sure user is an admin; other types of users should not create site data... is_admin(&local_user_view)?; - validate_create_payload(local_site.site_setup, local_site.slur_filter_regex, data)?; - - check_site_visibility_valid( - local_site.private_instance, - local_site.federation_enabled, - &data.private_instance, - &data.federation_enabled, - )?; - - let application_question = diesel_option_overwrite(&data.application_question); - check_application_question( - &application_question, - data - .registration_mode - .unwrap_or(local_site.registration_mode), - )?; + validate_create_payload(&local_site, data)?; let actor_id: DbUrl = Url::parse(&context.settings().get_protocol_and_hostname())?.into(); let inbox_url = Some(generate_site_inbox_url(&actor_id)?); @@ -96,7 +84,7 @@ impl PerformCrud for CreateSite { .enable_nsfw(data.enable_nsfw) .community_creation_admin_only(data.community_creation_admin_only) .require_email_verification(data.require_email_verification) - .application_question(application_question) + .application_question(diesel_option_overwrite(&data.application_question)) .private_instance(data.private_instance) .default_theme(data.default_theme.clone()) .default_post_listing_type(data.default_post_listing_type) @@ -149,13 +137,9 @@ impl PerformCrud for CreateSite { } } -fn validate_create_payload( - is_site_setup: bool, - local_site_slur_filter_regex: Option, - create_site: &CreateSite, -) -> LemmyResult<()> { +fn validate_create_payload(local_site: &LocalSite, create_site: &CreateSite) -> LemmyResult<()> { // Make sure the site hasn't already been set up... - if is_site_setup { + if local_site.site_setup { return Err(LemmyError::from_message("site_already_exists")); }; @@ -165,179 +149,436 @@ fn validate_create_payload( &create_site .slur_filter_regex .as_deref() - .or(local_site_slur_filter_regex.as_deref()), + .or(local_site.slur_filter_regex.as_deref()), )?; - // Validate the site name... site_name_length_check(&create_site.name)?; check_slurs(&create_site.name, &slur_regex)?; - // If provided, validate the site description... if let Some(desc) = &create_site.description { site_description_length_check(desc)?; check_slurs_opt(&create_site.description, &slur_regex)?; } + site_default_post_listing_type_check(&create_site.default_post_listing_type)?; + + check_site_visibility_valid( + local_site.private_instance, + local_site.federation_enabled, + &create_site.private_instance, + &create_site.federation_enabled, + )?; + // Ensure that the sidebar has fewer than the max num characters... - is_valid_body_field(&create_site.sidebar, false) + is_valid_body_field(&create_site.sidebar, false)?; + + application_question_check( + &local_site.application_question, + &create_site.application_question, + create_site + .registration_mode + .unwrap_or(local_site.registration_mode), + ) } #[cfg(test)] mod tests { use crate::site::create::validate_create_payload; use lemmy_api_common::site::CreateSite; + use lemmy_db_schema::{source::local_site::LocalSite, ListingType, RegistrationMode}; #[test] - fn test_validate_create() { - fn create_payload( - site_name: String, - site_description: Option, - site_sidebar: Option, - site_slur_filter_regex: Option, - ) -> CreateSite { - CreateSite { - name: site_name, - sidebar: site_sidebar, - description: site_description, - icon: None, - banner: None, - enable_downvotes: None, - enable_nsfw: None, - community_creation_admin_only: None, - require_email_verification: None, - application_question: None, - private_instance: None, - default_theme: None, - default_post_listing_type: None, - legal_information: None, - application_email_admins: None, - hide_modlog_mod_names: None, - discussion_languages: None, - slur_filter_regex: site_slur_filter_regex, - actor_name_max_length: None, - rate_limit_message: None, - rate_limit_message_per_second: None, - rate_limit_post: None, - rate_limit_post_per_second: None, - rate_limit_register: None, - rate_limit_register_per_second: None, - rate_limit_image: None, - rate_limit_image_per_second: None, - rate_limit_comment: None, - rate_limit_comment_per_second: None, - rate_limit_search: None, - rate_limit_search_per_second: None, - federation_enabled: None, - federation_debug: None, - captcha_enabled: None, - captcha_difficulty: None, - allowed_instances: None, - blocked_instances: None, - taglines: None, - registration_mode: None, - auth: Default::default(), - } - } - + fn test_validate_invalid_create_payload() { let invalid_payloads = [ ( - false, - &Some(String::from("(foo|bar)")), - &create_payload( - String::from("foo site_name"), + "CreateSite attempted on set up LocalSite", + "site_already_exists", + &generate_local_site( + true, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), None::, None::, + None::, None::, + None::, + None::, + None::, + None::, ), + ), + ( + "CreateSite name matches LocalSite slur filter", "slurs", + &generate_local_site( + false, + Some(String::from("(foo|bar)")), + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("foo site_name"), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + None::, + ), ), ( - false, - &Some(String::from("(foo|bar)")), - &create_payload( + "CreateSite name matches new slur filter", + "slurs", + &generate_local_site( + false, + Some(String::from("(foo|bar)")), + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( String::from("zeta site_name"), None::, None::, + None::, Some(String::from("(zeta|alpha)")), + None::, + None::, + None::, + None::, ), - "slurs", ), ( - true, - &None::, - &create_payload( + "CreateSite listing type is Subscribed, which is invalid", + "invalid_default_post_listing_type", + &generate_local_site( + false, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( String::from("site_name"), None::, None::, + Some(ListingType::Subscribed), None::, + None::, + None::, + None::, + None::, ), - "site_already_exists", ), - ]; - let valid_payloads = [ ( - false, - &None::, - &create_payload( + "CreateSite is both private and federated", + "cant_enable_private_instance_and_federation_together", + &generate_local_site( + false, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( String::from("site_name"), None::, None::, + None::, + None::, + Some(true), + Some(true), None::, + None::, ), ), ( - false, - &None::, - &create_payload( + "LocalSite is private, but CreateSite also makes it federated", + "cant_enable_private_instance_and_federation_together", + &generate_local_site( + false, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( String::from("site_name"), - Some(String::new()), - Some(String::new()), None::, + None::, + None::, + None::, + None::, + Some(true), + None::, + None::, ), ), ( - false, - &Some(String::from("(foo|bar)")), - &create_payload( - String::from("foo site_name"), - Some(String::new()), - Some(String::new()), - Some(String::new()), + "CreateSite requires application, but neither it nor LocalSite has an application question", + "application_question_required", + &generate_local_site( + false, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + Some(RegistrationMode::RequireApplication), ), ), ]; invalid_payloads.iter().enumerate().for_each( - |(idx, &(is_site_setup, local_slur_filter_regex, create_site, expected_err))| { - match validate_create_payload(is_site_setup, local_slur_filter_regex.clone(), create_site) { + |( + idx, + &(reason, expected_err, local_site, create_site), + )| { + match validate_create_payload( + local_site, + create_site, + ) { Ok(_) => { panic!( - "Got Ok, but validation should have failed with error: {} for invalid_payloads.nth({})", - expected_err, idx - ) + "Got Ok, but validation should have failed with error: {} for reason: {}. invalid_payloads.nth({})", + expected_err, reason, idx + ) } Err(error) => { assert!( error.message.eq(&Some(String::from(expected_err))), - "Got Err {:?}, but should have failed with message: {} for invalid_payloads.nth({})", + "Got Err {:?}, but should have failed with message: {} for reason: {}. invalid_payloads.nth({})", error.message, expected_err, + reason, idx ) } } }, ); + } + + #[test] + fn test_validate_valid_create_payload() { + let valid_payloads = [ + ( + "No changes between LocalSite and CreateSite", + &generate_local_site( + false, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + None::, + ), + ), + ( + "CreateSite allows clearing and changing values", + &generate_local_site( + false, + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), + Some(String::new()), + Some(String::new()), + Some(ListingType::All), + Some(String::new()), + Some(false), + Some(true), + Some(String::new()), + Some(RegistrationMode::Open), + ), + ), + ( + "CreateSite clears existing slur filter regex", + &generate_local_site( + false, + Some(String::from("(foo|bar)")), + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_create_site( + String::from("foo site_name"), + None::, + None::, + None::, + Some(String::new()), + None::, + None::, + None::, + None::, + ), + ), + ( + "LocalSite has application question and CreateSite now requires applications,", + &generate_local_site( + false, + None::, + true, + false, + Some(String::from("question")), + RegistrationMode::Open, + ), + &generate_create_site( + String::from("site_name"), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + Some(RegistrationMode::RequireApplication), + ), + ), + ]; - valid_payloads.iter().enumerate().for_each( - |(idx, &(is_site_setup, local_slur_filter_regex, create_site))| { + valid_payloads + .iter() + .enumerate() + .for_each(|(idx, &(reason, local_site, edit_site))| { assert!( - validate_create_payload(is_site_setup, local_slur_filter_regex.clone(), create_site) - .is_ok(), - "Got Err, but should have got Ok for valid_payloads.nth({})", + validate_create_payload(local_site, edit_site).is_ok(), + "Got Err, but should have got Ok for reason: {}. valid_payloads.nth({})", + reason, idx ); - }, - ) + }) + } + + fn generate_local_site( + site_setup: bool, + site_slur_filter_regex: Option, + site_is_private: bool, + site_is_federated: bool, + site_application_question: Option, + site_registration_mode: RegistrationMode, + ) -> LocalSite { + LocalSite { + id: Default::default(), + site_id: Default::default(), + site_setup, + enable_downvotes: false, + enable_nsfw: false, + community_creation_admin_only: false, + require_email_verification: false, + application_question: site_application_question, + private_instance: site_is_private, + default_theme: String::new(), + default_post_listing_type: ListingType::All, + legal_information: None, + hide_modlog_mod_names: false, + application_email_admins: false, + slur_filter_regex: site_slur_filter_regex, + actor_name_max_length: 0, + federation_enabled: site_is_federated, + captcha_enabled: false, + captcha_difficulty: String::new(), + published: Default::default(), + updated: None, + registration_mode: site_registration_mode, + reports_email_admins: false, + } + } + + // Allow the test helper function to have too many arguments. + // It's either this or generate the entire struct each time for testing. + #[allow(clippy::too_many_arguments)] + fn generate_create_site( + site_name: String, + site_description: Option, + site_sidebar: Option, + site_listing_type: Option, + site_slur_filter_regex: Option, + site_is_private: Option, + site_is_federated: Option, + site_application_question: Option, + site_registration_mode: Option, + ) -> CreateSite { + CreateSite { + name: site_name, + sidebar: site_sidebar, + description: site_description, + icon: None, + banner: None, + enable_downvotes: None, + enable_nsfw: None, + community_creation_admin_only: None, + require_email_verification: None, + application_question: site_application_question, + private_instance: site_is_private, + default_theme: None, + default_post_listing_type: site_listing_type, + legal_information: None, + application_email_admins: None, + hide_modlog_mod_names: None, + discussion_languages: None, + slur_filter_regex: site_slur_filter_regex, + actor_name_max_length: None, + rate_limit_message: None, + rate_limit_message_per_second: None, + rate_limit_post: None, + rate_limit_post_per_second: None, + rate_limit_register: None, + rate_limit_register_per_second: None, + rate_limit_image: None, + rate_limit_image_per_second: None, + rate_limit_comment: None, + rate_limit_comment_per_second: None, + rate_limit_search: None, + rate_limit_search_per_second: None, + federation_enabled: site_is_federated, + federation_debug: None, + captcha_enabled: None, + captcha_difficulty: None, + allowed_instances: None, + blocked_instances: None, + taglines: None, + registration_mode: site_registration_mode, + auth: Default::default(), + } } } diff --git a/crates/api_crud/src/site/mod.rs b/crates/api_crud/src/site/mod.rs index d0c09b935d..a98f2057c6 100644 --- a/crates/api_crud/src/site/mod.rs +++ b/crates/api_crud/src/site/mod.rs @@ -1,19 +1,95 @@ -use lemmy_db_schema::RegistrationMode; -use lemmy_utils::error::LemmyError; +use lemmy_db_schema::{ListingType, RegistrationMode}; +use lemmy_utils::error::{LemmyError, LemmyResult}; mod create; mod read; mod update; -pub fn check_application_question( - application_question: &Option>, +/// Checks whether the default post listing type is valid for a site. +pub fn site_default_post_listing_type_check( + default_post_listing_type: &Option, +) -> LemmyResult<()> { + if let Some(listing_type) = default_post_listing_type { + // Only allow all or local as default listing types... + if listing_type != &ListingType::All && listing_type != &ListingType::Local { + Err(LemmyError::from_message( + "invalid_default_post_listing_type", + )) + } else { + Ok(()) + } + } else { + Ok(()) + } +} + +/// Checks whether the application question and registration mode align. +pub fn application_question_check( + current_application_question: &Option, + new_application_question: &Option, registration_mode: RegistrationMode, -) -> Result<(), LemmyError> { +) -> LemmyResult<()> { + let has_no_question: bool = + current_application_question.is_none() && new_application_question.is_none(); + let is_nullifying_question: bool = new_application_question == &Some(String::new()); + if registration_mode == RegistrationMode::RequireApplication - && application_question.as_ref().unwrap_or(&None).is_none() + && (has_no_question || is_nullifying_question) { Err(LemmyError::from_message("application_question_required")) } else { Ok(()) } } + +#[cfg(test)] +mod tests { + use crate::site::{application_question_check, site_default_post_listing_type_check}; + use lemmy_db_schema::{ListingType, RegistrationMode}; + + #[test] + fn test_site_default_post_listing_type_check() { + assert!(site_default_post_listing_type_check(&None::).is_ok()); + assert!(site_default_post_listing_type_check(&Some(ListingType::All)).is_ok()); + assert!(site_default_post_listing_type_check(&Some(ListingType::Local)).is_ok()); + assert!(site_default_post_listing_type_check(&Some(ListingType::Subscribed)).is_err()); + } + + #[test] + fn test_application_question_check() { + assert!( + application_question_check(&Some(String::from("q")), &Some(String::new()), RegistrationMode::RequireApplication).is_err(), + "Expected application to be invalid because an application is required, current question: {:?}, new question: {:?}", + "q", + String::new(), + ); + assert!( + application_question_check(&None, &None, RegistrationMode::RequireApplication).is_err(), + "Expected application to be invalid because an application is required, current question: {:?}, new question: {:?}", + None::, + None:: + ); + + assert!( + application_question_check(&None, &None, RegistrationMode::Open).is_ok(), + "Expected application to be valid because no application required, current question: {:?}, new question: {:?}, mode: {:?}", + None::, + None::, + RegistrationMode::Open + ); + assert!( + application_question_check(&None, &Some(String::from("q")), RegistrationMode::RequireApplication).is_ok(), + "Expected application to be valid because new application provided, current question: {:?}, new question: {:?}, mode: {:?}", + None::, + Some(String::from("q")), + RegistrationMode::RequireApplication + ); + assert!( + application_question_check(&Some(String::from("q")), &None, RegistrationMode::RequireApplication).is_ok(), + "Expected application to be valid because application existed, current question: {:?}, new question: {:?}, mode: {:?}", + Some(String::from("q")), + None::, + RegistrationMode::RequireApplication + ); + } +} diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs index 5ff6a133d2..c9f97e8dfa 100644 --- a/crates/api_crud/src/site/update.rs +++ b/crates/api_crud/src/site/update.rs @@ -1,4 +1,7 @@ -use crate::{site::check_application_question, PerformCrud}; +use crate::{ + site::{application_question_check, site_default_post_listing_type_check}, + PerformCrud, +}; use actix_web::web::Data; use lemmy_api_common::{ context::LemmyContext, @@ -18,7 +21,6 @@ use lemmy_db_schema::{ }, traits::Crud, utils::{diesel_option_overwrite, diesel_option_overwrite_to_url, naive_now}, - ListingType, RegistrationMode, }; use lemmy_db_views::structs::SiteView; @@ -51,20 +53,7 @@ impl PerformCrud for EditSite { // Make sure user is an admin; other types of users should not update site data... is_admin(&local_user_view)?; - validate_update_payload( - local_site.slur_filter_regex, - local_site.federation_enabled, - local_site.private_instance, - data, - )?; - - let application_question = diesel_option_overwrite(&data.application_question); - check_application_question( - &application_question, - data - .registration_mode - .unwrap_or(local_site.registration_mode), - )?; + validate_update_payload(&local_site, data)?; if let Some(discussion_languages) = data.discussion_languages.clone() { SiteLanguage::update(context.pool(), discussion_languages.clone(), &site).await?; @@ -91,7 +80,7 @@ impl PerformCrud for EditSite { .enable_nsfw(data.enable_nsfw) .community_creation_admin_only(data.community_creation_admin_only) .require_email_verification(data.require_email_verification) - .application_question(application_question) + .application_question(diesel_option_overwrite(&data.application_question)) .private_instance(data.private_instance) .default_theme(data.default_theme.clone()) .default_post_listing_type(data.default_post_listing_type) @@ -184,19 +173,14 @@ impl PerformCrud for EditSite { } } -fn validate_update_payload( - local_site_slur_filter_regex: Option, - federation_enabled: bool, - private_instance: bool, - edit_site: &EditSite, -) -> LemmyResult<()> { +fn validate_update_payload(local_site: &LocalSite, edit_site: &EditSite) -> LemmyResult<()> { // Check that the slur regex compiles, and return the regex if valid... // Prioritize using new slur regex from the request; if not provided, use the existing regex. let slur_regex = build_and_check_regex( &edit_site .slur_filter_regex .as_deref() - .or(local_site_slur_filter_regex.as_deref()), + .or(local_site.slur_filter_regex.as_deref()), )?; if let Some(name) = &edit_site.name { @@ -210,131 +194,113 @@ fn validate_update_payload( check_slurs_opt(&edit_site.description, &slur_regex)?; } - if let Some(listing_type) = &edit_site.default_post_listing_type { - // Only allow all or local as default listing types... - if listing_type != &ListingType::All && listing_type != &ListingType::Local { - return Err(LemmyError::from_message( - "invalid_default_post_listing_type", - )); - } - } + site_default_post_listing_type_check(&edit_site.default_post_listing_type)?; check_site_visibility_valid( - private_instance, - federation_enabled, + local_site.private_instance, + local_site.federation_enabled, &edit_site.private_instance, &edit_site.federation_enabled, )?; // Ensure that the sidebar has fewer than the max num characters... - is_valid_body_field(&edit_site.sidebar, false) + is_valid_body_field(&edit_site.sidebar, false)?; + + application_question_check( + &local_site.application_question, + &edit_site.application_question, + edit_site + .registration_mode + .unwrap_or(local_site.registration_mode), + ) } #[cfg(test)] mod tests { use crate::site::update::validate_update_payload; use lemmy_api_common::site::EditSite; - use lemmy_db_schema::ListingType; + use lemmy_db_schema::{source::local_site::LocalSite, ListingType, RegistrationMode}; #[test] - fn test_validate_update_payload() { - fn create_payload( - site_name: Option, - site_description: Option, - site_sidebar: Option, - site_listing_type: Option, - site_slur_filter_regex: Option, - site_is_private: Option, - site_is_federated: Option, - ) -> EditSite { - EditSite { - name: site_name, - sidebar: site_sidebar, - description: site_description, - icon: None, - banner: None, - enable_downvotes: None, - enable_nsfw: None, - community_creation_admin_only: None, - require_email_verification: None, - application_question: None, - private_instance: site_is_private, - default_theme: None, - default_post_listing_type: site_listing_type, - legal_information: None, - application_email_admins: None, - hide_modlog_mod_names: None, - discussion_languages: None, - slur_filter_regex: site_slur_filter_regex, - actor_name_max_length: None, - rate_limit_message: None, - rate_limit_message_per_second: None, - rate_limit_post: None, - rate_limit_post_per_second: None, - rate_limit_register: None, - rate_limit_register_per_second: None, - rate_limit_image: None, - rate_limit_image_per_second: None, - rate_limit_comment: None, - rate_limit_comment_per_second: None, - rate_limit_search: None, - rate_limit_search_per_second: None, - federation_enabled: site_is_federated, - federation_debug: None, - captcha_enabled: None, - captcha_difficulty: None, - allowed_instances: None, - blocked_instances: None, - taglines: None, - registration_mode: None, - reports_email_admins: None, - auth: Default::default(), - } - } - + fn test_validate_invalid_update_payload() { let invalid_payloads = [ ( - &Some(String::from("(foo|bar)")), - &create_payload( + "EditSite name matches LocalSite slur filter", + "slurs", + &generate_local_site( + Some(String::from("(foo|bar)")), + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( Some(String::from("foo site_name")), None::, None::, None::, None::, - Some(true), - Some(false), + None::, + None::, + None::, + None::, ), - "slurs", ), ( - &Some(String::from("(foo|bar)")), - &create_payload( + "EditSite name matches new slur filter", + "slurs", + &generate_local_site( + Some(String::from("(foo|bar)")), + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( Some(String::from("zeta site_name")), None::, None::, None::, Some(String::from("(zeta|alpha)")), - Some(true), - Some(false), + None::, + None::, + None::, + None::, ), - "slurs", ), ( - &None::, - &create_payload( + "EditSite listing type is Subscribed, which is invalid", + "invalid_default_post_listing_type", + &generate_local_site( + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( Some(String::from("site_name")), None::, None::, Some(ListingType::Subscribed), None::, - Some(true), - Some(false), + None::, + None::, + None::, + None::, ), - "invalid_default_post_listing_type", ), ( - &None::, - &create_payload( + "EditSite is both private and federated", + "cant_enable_private_instance_and_federation_together", + &generate_local_site( + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( Some(String::from("site_name")), None::, None::, @@ -342,82 +308,275 @@ mod tests { None::, Some(true), Some(true), + None::, + None::, ), - "cant_enable_private_instance_and_federation_together", ), - ]; - - let valid_payloads = [ ( - &None::, - &create_payload( + "LocalSite is private, but EditSite also makes it federated", + "cant_enable_private_instance_and_federation_together", + &generate_local_site( None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("site_name")), None::, None::, None::, None::, + None::, Some(true), - Some(false), + None::, + None::, ), ), ( - &None::, - &create_payload( - Some(String::from("site_name")), - Some(String::new()), - Some(String::new()), - Some(ListingType::All), + "EditSite requires application, but neither it nor LocalSite has an application question", + "application_question_required", + &generate_local_site( None::, - Some(false), - Some(true), + true, + false, + None::, + RegistrationMode::Open, ), - ), - ( - &Some(String::from("(foo|bar)")), - &create_payload( - Some(String::from("foo site_name")), - Some(String::new()), - Some(String::new()), - Some(ListingType::All), - Some(String::new()), - Some(false), - Some(true), + &generate_edit_site( + Some(String::from("site_name")), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + Some(RegistrationMode::RequireApplication), ), ), ]; invalid_payloads.iter().enumerate().for_each( - |(idx, &(local_site_slur_filter_regex, edit_site, expected_err))| { - match validate_update_payload(local_site_slur_filter_regex.clone(), false, true, edit_site) - { + |( + idx, + &(reason, expected_err, local_site, edit_site), + )| { + match validate_update_payload(local_site, edit_site) { Ok(_) => { panic!( - "Got Ok, but validation should have failed with error: {} for invalid_payloads.nth({})", - expected_err, idx - ) + "Got Ok, but validation should have failed with error: {} for reason: {}. invalid_payloads.nth({})", + expected_err, reason, idx + ) } Err(error) => { assert!( error.message.eq(&Some(String::from(expected_err))), - "Got Err {:?}, but should have failed with message: {} for invalid_payloads.nth({})", + "Got Err {:?}, but should have failed with message: {} for reason: {}. invalid_payloads.nth({})", error.message, expected_err, + reason, idx ) } } }, ); + } - valid_payloads.iter().enumerate().for_each( - |(idx, &(local_site_slur_filter_regex, edit_site))| { + #[test] + fn test_validate_valid_update_payload() { + let valid_payloads = [ + ( + "No changes between LocalSite and EditSite", + &generate_local_site( + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + None::, + None::, + None::, + None::, + None::, + None::, + None::, + None::, + None::, + ), + ), + ( + "EditSite allows clearing and changing values", + &generate_local_site( + None::, + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("site_name")), + Some(String::new()), + Some(String::new()), + Some(ListingType::All), + Some(String::new()), + Some(false), + Some(true), + Some(String::new()), + Some(RegistrationMode::Open), + ), + ), + ( + "EditSite name passes slur filter regex", + &generate_local_site( + Some(String::from("(foo|bar)")), + true, + false, + None::, + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("foo site_name")), + None::, + None::, + None::, + Some(String::new()), + None::, + None::, + None::, + None::, + ), + ), + ( + "LocalSite has application question and EditSite now requires applications,", + &generate_local_site( + None::, + true, + false, + Some(String::from("question")), + RegistrationMode::Open, + ), + &generate_edit_site( + Some(String::from("site_name")), + None::, + None::, + None::, + None::, + None::, + None::, + None::, + Some(RegistrationMode::RequireApplication), + ), + ), + ]; + + valid_payloads + .iter() + .enumerate() + .for_each(|(idx, &(reason, local_site, edit_site))| { assert!( - validate_update_payload(local_site_slur_filter_regex.clone(), true, false, edit_site) - .is_ok(), - "Got Err, but should have got Ok for valid_payloads.nth({})", + validate_update_payload(local_site, edit_site).is_ok(), + "Got Err, but should have got Ok for reason: {}. valid_payloads.nth({})", + reason, idx ); - }, - ) + }) + } + + fn generate_local_site( + site_slur_filter_regex: Option, + site_is_private: bool, + site_is_federated: bool, + site_application_question: Option, + site_registration_mode: RegistrationMode, + ) -> LocalSite { + LocalSite { + id: Default::default(), + site_id: Default::default(), + site_setup: true, + enable_downvotes: false, + enable_nsfw: false, + community_creation_admin_only: false, + require_email_verification: false, + application_question: site_application_question, + private_instance: site_is_private, + default_theme: String::new(), + default_post_listing_type: ListingType::All, + legal_information: None, + hide_modlog_mod_names: false, + application_email_admins: false, + slur_filter_regex: site_slur_filter_regex, + actor_name_max_length: 0, + federation_enabled: site_is_federated, + captcha_enabled: false, + captcha_difficulty: String::new(), + published: Default::default(), + updated: None, + registration_mode: site_registration_mode, + reports_email_admins: false, + } + } + + // Allow the test helper function to have too many arguments. + // It's either this or generate the entire struct each time for testing. + #[allow(clippy::too_many_arguments)] + fn generate_edit_site( + site_name: Option, + site_description: Option, + site_sidebar: Option, + site_listing_type: Option, + site_slur_filter_regex: Option, + site_is_private: Option, + site_is_federated: Option, + site_application_question: Option, + site_registration_mode: Option, + ) -> EditSite { + EditSite { + name: site_name, + sidebar: site_sidebar, + description: site_description, + icon: None, + banner: None, + enable_downvotes: None, + enable_nsfw: None, + community_creation_admin_only: None, + require_email_verification: None, + application_question: site_application_question, + private_instance: site_is_private, + default_theme: None, + default_post_listing_type: site_listing_type, + legal_information: None, + application_email_admins: None, + hide_modlog_mod_names: None, + discussion_languages: None, + slur_filter_regex: site_slur_filter_regex, + actor_name_max_length: None, + rate_limit_message: None, + rate_limit_message_per_second: None, + rate_limit_post: None, + rate_limit_post_per_second: None, + rate_limit_register: None, + rate_limit_register_per_second: None, + rate_limit_image: None, + rate_limit_image_per_second: None, + rate_limit_comment: None, + rate_limit_comment_per_second: None, + rate_limit_search: None, + rate_limit_search_per_second: None, + federation_enabled: site_is_federated, + federation_debug: None, + captcha_enabled: None, + captcha_difficulty: None, + allowed_instances: None, + blocked_instances: None, + taglines: None, + registration_mode: site_registration_mode, + reports_email_admins: None, + auth: Default::default(), + } } }