From 65cac217139085d5d94f58d16e4e91a6d7234b85 Mon Sep 17 00:00:00 2001 From: Nutomic Date: Mon, 4 Apr 2022 14:46:49 +0000 Subject: [PATCH] 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 | 7 +++--- .../apub/src/activities/block/block_user.rs | 9 ++++++- .../apub/src/activities/community/add_mod.rs | 9 ++++++- .../src/activities/community/remove_mod.rs | 9 ++++++- .../apub/src/activities/community/update.rs | 9 ++++++- .../src/activities/create_or_update/post.rs | 9 ++++++- crates/apub/src/activities/deletion/mod.rs | 11 ++++++-- crates/apub/src/activities/mod.rs | 25 ++++++++++++++++--- 8 files changed, 74 insertions(+), 14 deletions(-) diff --git a/api_tests/src/post.spec.ts b/api_tests/src/post.spec.ts index f545754f..b2f12298 100644 --- 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 01eb0096..28104dba 100644 --- 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 4fba5ccc..10baa781 100644 --- 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 448aa23d..e404df04 100644 --- 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 cddc2e25..f260363b 100644 --- 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 9fd12b37..907e2390 100644 --- 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 cc8319e8..f0c3a541 100644 --- 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 6334584a..77d4883f 100644 --- 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, + mod_id: &ObjectId, + 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(()) } -- 2.44.1