From: Nutomic <me@nutomic.com>
Date: Mon, 10 Oct 2022 15:20:36 +0000 (+0000)
Subject: Fix check for federated mod actions (#2489)
X-Git-Url: http://these/git/%7B%60https:/matrix.to?a=commitdiff_plain;h=ec5e63b5a9b83c9dfde7ac8fa129e50c14555f15;p=lemmy.git

Fix check for federated mod actions (#2489)
---

diff --git a/api_tests/src/community.spec.ts b/api_tests/src/community.spec.ts
index 723f3bb3..381b546b 100644
--- a/api_tests/src/community.spec.ts
+++ b/api_tests/src/community.spec.ts
@@ -4,6 +4,7 @@ import { CommunityView } from 'lemmy-js-client';
 import {
   alpha,
   beta,
+  gamma,
   setupLogins,
   resolveCommunity,
   createCommunity,
@@ -11,6 +12,12 @@ import {
   removeCommunity,
   getCommunity,
   followCommunity,
+  banPersonFromCommunity,
+  resolvePerson,
+  getSite,
+  createPost,
+  getPost,
+  resolvePost,
 } from './shared';
 
 beforeAll(async () => {
@@ -162,3 +169,33 @@ test('Search for beta community', async () => {
   let alphaCommunity = (await resolveCommunity(alpha, searchShort)).community.unwrap();
   assertCommunityFederation(alphaCommunity, communityRes.community_view);
 });
+
+test('Admin actions in remote community are not federated to origin', async () => {
+  // create a community on alpha
+  let communityRes = (await createCommunity(alpha)).community_view;
+  expect(communityRes.community.name).toBeDefined();
+
+  // gamma follows community and posts in it
+  let gammaCommunity = (await resolveCommunity(gamma, communityRes.community.actor_id)).community.unwrap();
+  let gammaFollow = (await followCommunity(gamma, true, gammaCommunity.community.id));
+  expect(gammaFollow.community_view.subscribed).toBe("Subscribed");
+  let gammaPost = (await createPost(gamma, gammaCommunity.community.id)).post_view;
+  expect(gammaPost.post.id).toBeDefined();
+  expect(gammaPost.creator_banned_from_community).toBe(false);
+
+  // admin of beta decides to ban gamma from community
+  let betaCommunity = (await resolveCommunity(beta, communityRes.community.actor_id)).community.unwrap();
+  let bannedUserInfo1 = (await getSite(gamma)).my_user.unwrap().local_user_view.person;
+  let bannedUserInfo2 = (await resolvePerson(beta, bannedUserInfo1.actor_id)).person.unwrap();
+  let banRes = (await banPersonFromCommunity(beta, bannedUserInfo2.person.id, betaCommunity.community.id, true, true));
+  console.log(banRes);
+  expect(banRes.banned).toBe(true);
+
+  // ban doesnt federate to community's origin instance alpha
+  let alphaPost = (await resolvePost(alpha, gammaPost.post)).post.unwrap();
+  expect(alphaPost.creator_banned_from_community).toBe(false);
+
+  // and neither to gamma
+  let gammaPost2 = (await getPost(gamma, gammaPost.post.id));
+  expect(gammaPost2.post_view.creator_banned_from_community).toBe(false);
+});
diff --git a/crates/apub/src/activities/block/block_user.rs b/crates/apub/src/activities/block/block_user.rs
index 5297cda7..72c43163 100644
--- a/crates/apub/src/activities/block/block_user.rs
+++ b/crates/apub/src/activities/block/block_user.rs
@@ -145,7 +145,7 @@ impl ActivityHandler for BlockUser {
         verify_mod_action(
           &self.actor,
           self.object.inner(),
-          &community,
+          community.id,
           context,
           request_counter,
         )
diff --git a/crates/apub/src/activities/community/add_mod.rs b/crates/apub/src/activities/community/add_mod.rs
index ada7afdf..4dbd463b 100644
--- a/crates/apub/src/activities/community/add_mod.rs
+++ b/crates/apub/src/activities/community/add_mod.rs
@@ -90,7 +90,7 @@ impl ActivityHandler for AddMod {
     verify_mod_action(
       &self.actor,
       self.object.inner(),
-      &community,
+      community.id,
       context,
       request_counter,
     )
diff --git a/crates/apub/src/activities/community/remove_mod.rs b/crates/apub/src/activities/community/remove_mod.rs
index 7c5d817f..ede6a008 100644
--- a/crates/apub/src/activities/community/remove_mod.rs
+++ b/crates/apub/src/activities/community/remove_mod.rs
@@ -90,7 +90,7 @@ impl ActivityHandler for RemoveMod {
     verify_mod_action(
       &self.actor,
       self.object.inner(),
-      &community,
+      community.id,
       context,
       request_counter,
     )
diff --git a/crates/apub/src/activities/community/update.rs b/crates/apub/src/activities/community/update.rs
index 51e2a6ea..57a31221 100644
--- a/crates/apub/src/activities/community/update.rs
+++ b/crates/apub/src/activities/community/update.rs
@@ -78,7 +78,7 @@ impl ActivityHandler for UpdateCommunity {
     verify_mod_action(
       &self.actor,
       self.object.id.inner(),
-      &community,
+      community.id,
       context,
       request_counter,
     )
diff --git a/crates/apub/src/activities/create_or_update/post.rs b/crates/apub/src/activities/create_or_update/post.rs
index 5ac57fb0..d9d7b854 100644
--- a/crates/apub/src/activities/create_or_update/post.rs
+++ b/crates/apub/src/activities/create_or_update/post.rs
@@ -120,7 +120,7 @@ impl ActivityHandler for CreateOrUpdatePost {
           verify_mod_action(
             &self.actor,
             self.object.id.inner(),
-            &community,
+            community.id,
             context,
             request_counter,
           )
diff --git a/crates/apub/src/activities/deletion/mod.rs b/crates/apub/src/activities/deletion/mod.rs
index 8bc67721..87dcd2d8 100644
--- a/crates/apub/src/activities/deletion/mod.rs
+++ b/crates/apub/src/activities/deletion/mod.rs
@@ -159,7 +159,7 @@ pub(in crate::activities) async fn verify_delete_activity(
       verify_mod_action(
         &activity.actor,
         activity.object.id(),
-        &community,
+        community.id,
         context,
         request_counter,
       )
@@ -208,7 +208,7 @@ async fn verify_delete_post_or_comment(
 ) -> Result<(), LemmyError> {
   verify_person_in_community(actor, community, context, request_counter).await?;
   if is_mod_action {
-    verify_mod_action(actor, object_id, community, context, request_counter).await?;
+    verify_mod_action(actor, object_id, community.id, context, request_counter).await?;
   } else {
     // domain of post ap_id and post.creator ap_id are identical, so we just check the former
     verify_domains_match(actor.inner(), object_id)?;
diff --git a/crates/apub/src/activities/mod.rs b/crates/apub/src/activities/mod.rs
index bd928e72..d3400705 100644
--- a/crates/apub/src/activities/mod.rs
+++ b/crates/apub/src/activities/mod.rs
@@ -14,7 +14,7 @@ use activitypub_federation::{
 use activitystreams_kinds::public;
 use anyhow::anyhow;
 use lemmy_api_common::utils::blocking;
-use lemmy_db_schema::source::community::Community;
+use lemmy_db_schema::{newtypes::CommunityId, source::community::Community};
 use lemmy_db_views_actor::structs::{CommunityPersonBanView, CommunityView};
 use lemmy_utils::error::LemmyError;
 use lemmy_websocket::LemmyContext;
@@ -75,9 +75,7 @@ pub(crate) async fn verify_person_in_community(
   Ok(())
 }
 
-/// Verify that the actor is a community mod. This check is only run if the community is local,
-/// because in case of remote communities, admins can also perform mod actions. As admin status
-/// is not federated, we cant verify their actions remotely.
+/// Verify that mod action in community was performed by a moderator.
 ///
 /// * `mod_id` - Activitypub ID of the mod or admin who performed the action
 /// * `object_id` - Activitypub ID of the actor or object that is being moderated
@@ -86,40 +84,30 @@ pub(crate) async fn verify_person_in_community(
 pub(crate) async fn verify_mod_action(
   mod_id: &ObjectId<ApubPerson>,
   object_id: &Url,
-  community: &ApubCommunity,
+  community_id: CommunityId,
   context: &LemmyContext,
   request_counter: &mut i32,
 ) -> Result<(), LemmyError> {
-  if community.local {
-    let actor = mod_id
-      .dereference(context, local_instance(context), request_counter)
-      .await?;
-
-    // Note: this will also return true for admins in addition to mods, but as we dont know about
-    //       remote admins, it doesnt make any difference.
-    let community_id = community.id;
-    let actor_id = actor.id;
-
-    let is_mod_or_admin = blocking(context.pool(), move |conn| {
-      CommunityView::is_mod_or_admin(conn, actor_id, community_id)
-    })
+  let mod_ = mod_id
+    .dereference(context, local_instance(context), request_counter)
     .await?;
 
-    // mod action was done either by a community mod or a local admin, so its allowed
-    if is_mod_or_admin {
-      return Ok(());
-    }
-
-    // mod action comes from the same instance as the moderated object, so it was presumably done
-    // by an instance admin and is legitimate (admin status is not federated).
-    if mod_id.inner().domain() == object_id.domain() {
-      return Ok(());
-    }
+  let is_mod_or_admin = blocking(context.pool(), move |conn| {
+    CommunityView::is_mod_or_admin(conn, mod_.id, community_id)
+  })
+  .await?;
+  if is_mod_or_admin {
+    return Ok(());
+  }
 
-    // the user is not a valid mod
-    return Err(LemmyError::from_message("Not a mod"));
+  // mod action comes from the same instance as the moderated object, so it was presumably done
+  // by an instance admin.
+  // TODO: federate instance admin status and check it here
+  if mod_id.inner().domain() == object_id.domain() {
+    return Ok(());
   }
-  Ok(())
+
+  Err(LemmyError::from_message("Not a mod"))
 }
 
 /// For Add/Remove community moderator activities, check that the target field actually contains