From da3e3b6d8a6684d58eecef91c675a696005f8330 Mon Sep 17 00:00:00 2001
From: Scott <97430840+scme0@users.noreply.github.com>
Date: Thu, 22 Jun 2023 05:44:12 +0930
Subject: [PATCH] Ensure site can only either be a private instance or
 federated when creating or editing site (#3237)

* Add site visibility validation

* Fix formatting

* linter changes

* Update error message to match existing check

* Remove existing check

---------

Co-authored-by: Scott Merchant <97430840+scottmerchant@users.noreply.github.com>
---
 crates/api_crud/src/site/create.rs   | 15 ++++++++++----
 crates/api_crud/src/site/update.rs   | 25 ++++++++++------------
 crates/utils/src/utils/validation.rs | 31 ++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/crates/api_crud/src/site/create.rs b/crates/api_crud/src/site/create.rs
index af8540669..2a51309a4 100644
--- a/crates/api_crud/src/site/create.rs
+++ b/crates/api_crud/src/site/create.rs
@@ -29,7 +29,7 @@ use lemmy_utils::{
   error::LemmyError,
   utils::{
     slurs::{check_slurs, check_slurs_opt},
-    validation::is_valid_body_field,
+    validation::{check_site_visibility_valid, is_valid_body_field},
   },
 };
 use url::Url;
@@ -50,6 +50,16 @@ impl PerformCrud for CreateSite {
 
     let local_user_view = local_user_view_from_jwt(&data.auth, context).await?;
 
+    // Make sure user is an admin
+    is_admin(&local_user_view)?;
+
+    check_site_visibility_valid(
+      local_site.private_instance,
+      local_site.federation_enabled,
+      &data.private_instance,
+      &data.federation_enabled,
+    )?;
+
     let sidebar = diesel_option_overwrite(&data.sidebar);
     let description = diesel_option_overwrite(&data.description);
     let icon = diesel_option_overwrite_to_url(&data.icon)?;
@@ -59,9 +69,6 @@ impl PerformCrud for CreateSite {
     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 {
       site_description_length_check(desc)?;
     }
diff --git a/crates/api_crud/src/site/update.rs b/crates/api_crud/src/site/update.rs
index ac1544ffc..fadde0a0b 100644
--- a/crates/api_crud/src/site/update.rs
+++ b/crates/api_crud/src/site/update.rs
@@ -30,7 +30,10 @@ 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},
+  utils::{
+    slurs::check_slurs_opt,
+    validation::{check_site_visibility_valid, is_valid_body_field},
+  },
 };
 
 #[async_trait::async_trait(?Send)]
@@ -48,6 +51,13 @@ impl PerformCrud for EditSite {
     // Make sure user is an admin
     is_admin(&local_user_view)?;
 
+    check_site_visibility_valid(
+      local_site.private_instance,
+      local_site.federation_enabled,
+      &data.private_instance,
+      &data.federation_enabled,
+    )?;
+
     let slur_regex = local_site_to_slur_regex(&local_site);
 
     check_slurs_opt(&data.name, &slur_regex)?;
@@ -76,19 +86,6 @@ impl PerformCrud for EditSite {
       }
     }
 
-    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",
-      ));
-    }
-
     if let Some(discussion_languages) = data.discussion_languages.clone() {
       SiteLanguage::update(context.pool(), discussion_languages.clone(), &site).await?;
     }
diff --git a/crates/utils/src/utils/validation.rs b/crates/utils/src/utils/validation.rs
index c4feb467b..41103332c 100644
--- a/crates/utils/src/utils/validation.rs
+++ b/crates/utils/src/utils/validation.rs
@@ -149,10 +149,29 @@ pub fn build_totp_2fa(site_name: &str, username: &str, secret: &str) -> Result<T
   .map_err(|e| LemmyError::from_error_message(e, "Couldnt generate TOTP"))
 }
 
+pub fn check_site_visibility_valid(
+  current_private_instance: bool,
+  current_federation_enabled: bool,
+  new_private_instance: &Option<bool>,
+  new_federation_enabled: &Option<bool>,
+) -> LemmyResult<()> {
+  let private_instance = new_private_instance.unwrap_or(current_private_instance);
+  let federation_enabled = new_federation_enabled.unwrap_or(current_federation_enabled);
+
+  if private_instance && federation_enabled {
+    return Err(LemmyError::from_message(
+      "cant_enable_private_instance_and_federation_together",
+    ));
+  }
+
+  Ok(())
+}
+
 #[cfg(test)]
 mod tests {
   use super::build_totp_2fa;
   use crate::utils::validation::{
+    check_site_visibility_valid,
     clean_url_params,
     generate_totp_2fa_secret,
     is_valid_actor_name,
@@ -226,4 +245,16 @@ mod tests {
     let totp = build_totp_2fa("lemmy", "my_name", &generated_secret);
     assert!(totp.is_ok());
   }
+
+  #[test]
+  fn test_check_site_visibility_valid() {
+    assert!(check_site_visibility_valid(true, true, &None, &None).is_err());
+    assert!(check_site_visibility_valid(true, false, &None, &Some(true)).is_err());
+    assert!(check_site_visibility_valid(false, true, &Some(true), &None).is_err());
+    assert!(check_site_visibility_valid(false, false, &Some(true), &Some(true)).is_err());
+    assert!(check_site_visibility_valid(true, false, &None, &None).is_ok());
+    assert!(check_site_visibility_valid(false, true, &None, &None).is_ok());
+    assert!(check_site_visibility_valid(false, false, &Some(true), &None).is_ok());
+    assert!(check_site_visibility_valid(false, false, &None, &Some(true)).is_ok());
+  }
 }