From ec5e63b5a9b83c9dfde7ac8fa129e50c14555f15 Mon Sep 17 00:00:00 2001 From: Nutomic Date: Mon, 10 Oct 2022 15:20:36 +0000 Subject: [PATCH] Fix check for federated mod actions (#2489) --- api_tests/src/community.spec.ts | 37 +++++++++++++ .../apub/src/activities/block/block_user.rs | 2 +- .../apub/src/activities/community/add_mod.rs | 2 +- .../src/activities/community/remove_mod.rs | 2 +- .../apub/src/activities/community/update.rs | 2 +- .../src/activities/create_or_update/post.rs | 2 +- crates/apub/src/activities/deletion/mod.rs | 4 +- crates/apub/src/activities/mod.rs | 52 +++++++------------ 8 files changed, 64 insertions(+), 39 deletions(-) diff --git a/api_tests/src/community.spec.ts b/api_tests/src/community.spec.ts index 723f3bb3f..381b546b3 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 5297cda73..72c431637 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 ada7afdf1..4dbd463b7 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 7c5d817f9..ede6a0089 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 51e2a6ea1..57a31221e 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 5ac57fb00..d9d7b8545 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 8bc67721e..87dcd2d8f 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 bd928e72c..d3400705d 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, 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(()); - } - - // the user is not a valid mod - return Err(LemmyError::from_message("Not a mod")); + 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(()); } - Ok(()) + + // 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(()); + } + + Err(LemmyError::from_message("Not a mod")) } /// For Add/Remove community moderator activities, check that the target field actually contains