Fix verify_mod_action check for remote admin actions (#2190)

* Fix verify_mod_action check for remote admin actions

* fix federation test

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

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(())

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(())
   }

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(())
   }

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(),

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())?;

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)?;

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(())
 }