From eb40aeb89b139d843cac1ecf9370a6b808b3aeea Mon Sep 17 00:00:00 2001
From: Dessalines <dessalines@users.noreply.github.com>
Date: Fri, 21 Apr 2023 17:41:03 -0400
Subject: [PATCH] Remove last Option<Vec.. from API. Fixes #2820 (#2822)

* Remove last Option<Vec.. from API. Fixes #2820

* Add empty allowed_instances check.

* Adding comment for allowed_instances.
---
 crates/api_common/src/site.rs  |  4 +-
 crates/api_common/src/utils.rs |  4 --
 crates/apub/src/lib.rs         | 71 +++++++++++++++++-----------------
 crates/apub/src/objects/mod.rs |  4 --
 4 files changed, 38 insertions(+), 45 deletions(-)

diff --git a/crates/api_common/src/site.rs b/crates/api_common/src/site.rs
index 8e5595141..7bce3d1c7 100644
--- a/crates/api_common/src/site.rs
+++ b/crates/api_common/src/site.rs
@@ -250,8 +250,8 @@ pub struct LeaveAdmin {
 #[derive(Debug, Serialize, Deserialize, Clone)]
 pub struct FederatedInstances {
   pub linked: Vec<Instance>,
-  pub allowed: Option<Vec<Instance>>,
-  pub blocked: Option<Vec<Instance>>,
+  pub allowed: Vec<Instance>,
+  pub blocked: Vec<Instance>,
 }
 
 #[derive(Debug, Serialize, Deserialize, Clone)]
diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs
index ee3ee8449..75f878fc4 100644
--- a/crates/api_common/src/utils.rs
+++ b/crates/api_common/src/utils.rs
@@ -318,10 +318,6 @@ pub async fn build_federated_instances(
     let allowed = Instance::allowlist(pool).await?;
     let blocked = Instance::blocklist(pool).await?;
 
-    // These can return empty vectors, so convert them to options
-    let allowed = (!allowed.is_empty()).then_some(allowed);
-    let blocked = (!blocked.is_empty()).then_some(blocked);
-
     Ok(Some(FederatedInstances {
       linked,
       allowed,
diff --git a/crates/apub/src/lib.rs b/crates/apub/src/lib.rs
index 43e26961c..a5bc41d1f 100644
--- a/crates/apub/src/lib.rs
+++ b/crates/apub/src/lib.rs
@@ -69,16 +69,22 @@ fn check_apub_id_valid(apub_id: &Url, local_site_data: &LocalSiteData) -> Result
     return Err("Federation disabled");
   }
 
-  if let Some(blocked) = local_site_data.blocked_instances.as_ref() {
-    if blocked.iter().any(|i| domain.eq(&i.domain)) {
-      return Err("Domain is blocked");
-    }
+  if local_site_data
+    .blocked_instances
+    .iter()
+    .any(|i| domain.eq(&i.domain))
+  {
+    return Err("Domain is blocked");
   }
 
-  if let Some(allowed) = local_site_data.allowed_instances.as_ref() {
-    if !allowed.iter().any(|i| domain.eq(&i.domain)) {
-      return Err("Domain is not in allowlist");
-    }
+  // Only check this if there are instances in the allowlist
+  if !local_site_data.allowed_instances.is_empty()
+    && !local_site_data
+      .allowed_instances
+      .iter()
+      .any(|i| domain.eq(&i.domain))
+  {
+    return Err("Domain is not in allowlist");
   }
 
   Ok(())
@@ -87,8 +93,8 @@ fn check_apub_id_valid(apub_id: &Url, local_site_data: &LocalSiteData) -> Result
 #[derive(Clone)]
 pub(crate) struct LocalSiteData {
   local_site: Option<LocalSite>,
-  allowed_instances: Option<Vec<Instance>>,
-  blocked_instances: Option<Vec<Instance>>,
+  allowed_instances: Vec<Instance>,
+  blocked_instances: Vec<Instance>,
 }
 
 pub(crate) async fn fetch_local_site_data(
@@ -96,12 +102,8 @@ pub(crate) async fn fetch_local_site_data(
 ) -> Result<LocalSiteData, diesel::result::Error> {
   // LocalSite may be missing
   let local_site = LocalSite::read(pool).await.ok();
-  let allowed = Instance::allowlist(pool).await?;
-  let blocked = Instance::blocklist(pool).await?;
-
-  // These can return empty vectors, so convert them to options
-  let allowed_instances = (!allowed.is_empty()).then_some(allowed);
-  let blocked_instances = (!blocked.is_empty()).then_some(blocked);
+  let allowed_instances = Instance::allowlist(pool).await?;
+  let blocked_instances = Instance::blocklist(pool).await?;
 
   Ok(LocalSiteData {
     local_site,
@@ -126,26 +128,25 @@ pub(crate) fn check_apub_id_valid_with_strictness(
   }
   check_apub_id_valid(apub_id, local_site_data).map_err(LemmyError::from_message)?;
 
-  if let Some(allowed) = local_site_data.allowed_instances.as_ref() {
-    // Only check allowlist if this is a community
-    if is_strict {
-      // need to allow this explicitly because apub receive might contain objects from our local
-      // instance.
-      let mut allowed_and_local = allowed
-        .iter()
-        .map(|i| i.domain.clone())
-        .collect::<Vec<String>>();
-      let local_instance = settings
-        .get_hostname_without_port()
-        .expect("local hostname is valid");
-      allowed_and_local.push(local_instance);
+  // Only check allowlist if this is a community, and there are instances in the allowlist
+  if is_strict && !local_site_data.allowed_instances.is_empty() {
+    // need to allow this explicitly because apub receive might contain objects from our local
+    // instance.
+    let mut allowed_and_local = local_site_data
+      .allowed_instances
+      .iter()
+      .map(|i| i.domain.clone())
+      .collect::<Vec<String>>();
+    let local_instance = settings
+      .get_hostname_without_port()
+      .expect("local hostname is valid");
+    allowed_and_local.push(local_instance);
 
-      let domain = apub_id.domain().expect("apud id has domain").to_string();
-      if !allowed_and_local.contains(&domain) {
-        return Err(LemmyError::from_message(
-          "Federation forbidden by strict allowlist",
-        ));
-      }
+    let domain = apub_id.domain().expect("apud id has domain").to_string();
+    if !allowed_and_local.contains(&domain) {
+      return Err(LemmyError::from_message(
+        "Federation forbidden by strict allowlist",
+      ));
     }
   }
   Ok(())
diff --git a/crates/apub/src/objects/mod.rs b/crates/apub/src/objects/mod.rs
index ff4182dd2..1dbc7c63a 100644
--- a/crates/apub/src/objects/mod.rs
+++ b/crates/apub/src/objects/mod.rs
@@ -64,7 +64,6 @@ pub(crate) mod tests {
   };
   use lemmy_db_schema::{source::secret::Secret, utils::build_db_pool_for_tests};
   use lemmy_utils::{
-    error::LemmyError,
     rate_limit::{RateLimitCell, RateLimitConfig},
     settings::SETTINGS,
   };
@@ -89,9 +88,6 @@ pub(crate) mod tests {
 
   // TODO: would be nice if we didnt have to use a full context for tests.
   pub(crate) async fn init_context() -> Data<LemmyContext> {
-    async fn x() -> Result<String, LemmyError> {
-      Ok(String::new())
-    }
     // call this to run migrations
     let pool = build_db_pool_for_tests().await;