Fix verify_mod_action check for remote admin actions (#2190)

* Fix verify_mod_action check for remote admin actions

* fix federation test

diff --git a/api_tests/src/post.spec.ts b/api_tests/src/post.spec.ts
index f545754f752e20a398999c30c59440d47fc2dd22..b2f12298c309b7ae960e79bc8b2b23e85ec791fc 100644 (file)
--- a/api_tests/src/post.spec.ts
+++ b/api_tests/src/post.spec.ts
@@ -225,9 +225,10 @@ test('Delete a post', async () => {
 });
 
 test('Remove a post from admin and community on different instance', async () => {
-  let postRes = await createPost(alpha, betaCommunity.community.id);
+  let postRes = await createPost(gamma, betaCommunity.community.id);
 
-  let removedPost = await removePost(alpha, true, postRes.post_view.post);
+  let alphaPost = (await resolvePost(alpha, postRes.post_view.post)).post;
+  let removedPost = await removePost(alpha, true, alphaPost.post);
   expect(removedPost.post_view.post.removed).toBe(true);
   expect(removedPost.post_view.post.name).toBe(postRes.post_view.post.name);
 
@@ -236,7 +237,7 @@ test('Remove a post from admin and community on different instance', async () =>
   expect(betaPost.post.removed).toBe(false);
 
   // Undelete
-  let undeletedPost = await removePost(alpha, false, postRes.post_view.post);
+  let undeletedPost = await removePost(alpha, false, alphaPost.post);
   expect(undeletedPost.post_view.post.removed).toBe(false);
 
   // Make sure lemmy beta sees post is undeleted

diff --git a/crates/apub/src/activities/block/block_user.rs b/crates/apub/src/activities/block/block_user.rs
index 01eb00969877249407a9f830889027f5d5f09c24..28104dba19131a59b6ac88f939d43f52b27d1a3f 100644 (file)
--- a/crates/apub/src/activities/block/block_user.rs
+++ b/crates/apub/src/activities/block/block_user.rs
@@ -133,7 +133,14 @@ impl ActivityHandler for BlockUser {
       }
       SiteOrCommunity::Community(community) => {
         verify_person_in_community(&self.actor, &community, context, request_counter).await?;
-        verify_mod_action(&self.actor, &community, context, request_counter).await?;
+        verify_mod_action(
+          &self.actor,
+          self.object.inner(),
+          &community,
+          context,
+          request_counter,
+        )
+        .await?;
       }
     }
     Ok(())

diff --git a/crates/apub/src/activities/community/add_mod.rs b/crates/apub/src/activities/community/add_mod.rs
index 4fba5ccc063b8c085e00343e6c322f58a1d1501d..10baa781c532e741c69d99db655f7d0248cbbbe6 100644 (file)
--- a/crates/apub/src/activities/community/add_mod.rs
+++ b/crates/apub/src/activities/community/add_mod.rs
@@ -74,7 +74,14 @@ impl ActivityHandler for AddMod {
     verify_activity(&self.id, self.actor.inner(), &context.settings())?;
     let community = self.get_community(context, request_counter).await?;
     verify_person_in_community(&self.actor, &community, context, request_counter).await?;
-    verify_mod_action(&self.actor, &community, context, request_counter).await?;
+    verify_mod_action(
+      &self.actor,
+      self.object.inner(),
+      &community,
+      context,
+      request_counter,
+    )
+    .await?;
     verify_add_remove_moderator_target(&self.target, &community)?;
     Ok(())
   }

diff --git a/crates/apub/src/activities/community/remove_mod.rs b/crates/apub/src/activities/community/remove_mod.rs
index 448aa23d21e6f0ed6d4e5a2060a478d8286153d8..e404df04d53b0816a48399f2e4d874506d3dd6ce 100644 (file)
--- a/crates/apub/src/activities/community/remove_mod.rs
+++ b/crates/apub/src/activities/community/remove_mod.rs
@@ -74,7 +74,14 @@ impl ActivityHandler for RemoveMod {
     verify_activity(&self.id, self.actor.inner(), &context.settings())?;
     let community = self.get_community(context, request_counter).await?;
     verify_person_in_community(&self.actor, &community, context, request_counter).await?;
-    verify_mod_action(&self.actor, &community, context, request_counter).await?;
+    verify_mod_action(
+      &self.actor,
+      self.object.inner(),
+      &community,
+      context,
+      request_counter,
+    )
+    .await?;
     verify_add_remove_moderator_target(&self.target, &community)?;
     Ok(())
   }

diff --git a/crates/apub/src/activities/community/update.rs b/crates/apub/src/activities/community/update.rs
index cddc2e25d34fef0e45a1d09fbdce6fcd8163c2fb..f260363bb8470e840e6557e912d8dcb7103303b0 100644 (file)
--- a/crates/apub/src/activities/community/update.rs
+++ b/crates/apub/src/activities/community/update.rs
@@ -65,7 +65,14 @@ impl ActivityHandler for UpdateCommunity {
     verify_activity(&self.id, self.actor.inner(), &context.settings())?;
     let community = self.get_community(context, request_counter).await?;
     verify_person_in_community(&self.actor, &community, context, request_counter).await?;
-    verify_mod_action(&self.actor, &community, context, request_counter).await?;
+    verify_mod_action(
+      &self.actor,
+      self.object.id.inner(),
+      &community,
+      context,
+      request_counter,
+    )
+    .await?;
     ApubCommunity::verify(
       &self.object,
       &community.actor_id.clone().into(),

diff --git a/crates/apub/src/activities/create_or_update/post.rs b/crates/apub/src/activities/create_or_update/post.rs
index 9fd12b37e317bd0bbca14f677890a8fe4184dfb1..907e23907890499715748a9fa4f26fc8079919a0 100644 (file)
--- a/crates/apub/src/activities/create_or_update/post.rs
+++ b/crates/apub/src/activities/create_or_update/post.rs
@@ -103,7 +103,14 @@ impl ActivityHandler for CreateOrUpdatePost {
       CreateOrUpdateType::Update => {
         let is_mod_action = self.object.is_mod_action(context).await?;
         if is_mod_action {
-          verify_mod_action(&self.actor, &community, context, request_counter).await?;
+          verify_mod_action(
+            &self.actor,
+            self.object.id.inner(),
+            &community,
+            context,
+            request_counter,
+          )
+          .await?;
         } else {
           verify_domains_match(self.actor.inner(), self.object.id.inner())?;
           verify_urls_match(self.actor.inner(), self.object.attributed_to.inner())?;

diff --git a/crates/apub/src/activities/deletion/mod.rs b/crates/apub/src/activities/deletion/mod.rs
index cc8319e80b656e6609ce5d078655a8651cb92b4a..f0c3a541fb99d9f1c186def27391ee1d22b1a072 100644 (file)
--- a/crates/apub/src/activities/deletion/mod.rs
+++ b/crates/apub/src/activities/deletion/mod.rs
@@ -162,7 +162,14 @@ pub(in crate::activities) async fn verify_delete_activity(
         verify_person_in_community(&activity.actor, &community, context, request_counter).await?;
       }
       // community deletion is always a mod (or admin) action
-      verify_mod_action(&activity.actor, &community, context, request_counter).await?;
+      verify_mod_action(
+        &activity.actor,
+        activity.object.id(),
+        &community,
+        context,
+        request_counter,
+      )
+      .await?;
     }
     DeletableObjects::Post(p) => {
       verify_is_public(&activity.to, &[])?;
@@ -207,7 +214,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, community, context, request_counter).await?;
+    verify_mod_action(actor, object_id, community, 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 6334584a41715248781959bbe28ac4feb3a484a4..77d4883f00a764dc5e8aab9697da45ad1a5f9038 100644 (file)
--- a/crates/apub/src/activities/mod.rs
+++ b/crates/apub/src/activities/mod.rs
@@ -86,15 +86,20 @@ fn verify_activity(id: &Url, actor: &Url, settings: &Settings) -> Result<(), Lem
 /// 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.
+///
+/// * `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
+/// * `community` - The community inside which moderation is happening
 #[tracing::instrument(skip_all)]
 pub(crate) async fn verify_mod_action(
-  actor_id: &ObjectId<ApubPerson>,
+  mod_id: &ObjectId<ApubPerson>,
+  object_id: &Url,
   community: &ApubCommunity,
   context: &LemmyContext,
   request_counter: &mut i32,
 ) -> Result<(), LemmyError> {
   if community.local {
-    let actor = actor_id
+    let actor = mod_id
       .dereference(context, context.client(), request_counter)
       .await?;
 
@@ -102,13 +107,25 @@ pub(crate) async fn verify_mod_action(
     //       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)
     })
     .await?;
-    if !is_mod_or_admin {
-      return Err(LemmyError::from_message("Not a mod"));
+
+    // 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(());
+    }
+
+    // the user is not a valid mod
+    return Err(LemmyError::from_message("Not a mod"));
   }
   Ok(())
 }