From: Felix Ableitner Date: Mon, 1 Jun 2020 14:17:20 +0000 (+0200) Subject: address review comments X-Git-Url: http://these/git/%7B%60https:/%7B%60css/themes/%22https:/nerdica.net/%7BdocsUrl%7D?a=commitdiff_plain;h=1aa30d855e111174f1e3931fdf0964a889cb9b7e;p=lemmy.git address review comments --- diff --git a/server/src/apub/activities.rs b/server/src/apub/activities.rs index 3816093c..a747ed21 100644 --- a/server/src/apub/activities.rs +++ b/server/src/apub/activities.rs @@ -1,5 +1,9 @@ -use crate::apub::{extensions::signatures::sign, is_apub_id_valid, ActorType}; -use activitystreams::{context, object::properties::ObjectProperties, public}; +use crate::{ + apub::{extensions::signatures::sign, is_apub_id_valid, ActorType}, + db::{activity::insert_activity, community::Community, user::User_}, +}; +use activitystreams::{context, object::properties::ObjectProperties, public, Activity, Base}; +use diesel::PgConnection; use failure::{Error, _core::fmt::Debug}; use isahc::prelude::*; use log::debug; @@ -22,6 +26,27 @@ pub fn populate_object_props( Ok(()) } +pub fn send_activity_to_community( + creator: &User_, + conn: &PgConnection, + community: &Community, + to: Vec, + activity: A, +) -> Result<(), Error> +where + A: Activity + Base + Serialize + Debug, +{ + insert_activity(&conn, creator.id, &activity, true)?; + + // if this is a local community, we need to do an announce from the community instead + if community.local { + Community::do_announce(activity, &community, creator, conn)?; + } else { + send_activity(&activity, creator, to)?; + } + Ok(()) +} + /// Send an activity to a list of recipients, using the correct headers etc. pub fn send_activity(activity: &A, actor: &dyn ActorType, to: Vec) -> Result<(), Error> where diff --git a/server/src/apub/comment.rs b/server/src/apub/comment.rs index d199eddb..7c77a1c7 100644 --- a/server/src/apub/comment.rs +++ b/server/src/apub/comment.rs @@ -1,6 +1,6 @@ use crate::{ apub::{ - activities::{populate_object_props, send_activity}, + activities::{populate_object_props, send_activity_to_community}, create_apub_response, create_apub_tombstone_response, create_tombstone, @@ -14,7 +14,6 @@ use crate::{ }, convert_datetime, db::{ - activity::insert_activity, comment::{Comment, CommentForm}, community::Community, post::Post, @@ -30,15 +29,13 @@ use activitystreams::{ context, link::Mention, object::{kind::NoteType, properties::ObjectProperties, Note, Tombstone}, - Activity, - Base, }; use actix_web::{body::Body, web::Path, HttpResponse, Result}; use diesel::PgConnection; -use failure::{Error, _core::fmt::Debug}; +use failure::Error; use itertools::Itertools; use log::debug; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; #[derive(Deserialize)] pub struct CommentQuery { @@ -178,7 +175,7 @@ impl ApubObjectType for Comment { .set_actor_xsd_any_uri(creator.actor_id.to_owned())? .set_object_base_box(note)?; - Comment::send_comment_activity(&creator, &conn, &community, maa.inboxes,create)?; + send_activity_to_community(&creator, &conn, &community, maa.inboxes, create)?; Ok(()) } @@ -203,7 +200,7 @@ impl ApubObjectType for Comment { .set_actor_xsd_any_uri(creator.actor_id.to_owned())? .set_object_base_box(note)?; - Comment::send_comment_activity(&creator, &conn, &community, maa.inboxes, update)?; + send_activity_to_community(&creator, &conn, &community, maa.inboxes, update)?; Ok(()) } @@ -225,7 +222,13 @@ impl ApubObjectType for Comment { .set_actor_xsd_any_uri(creator.actor_id.to_owned())? .set_object_base_box(note)?; - Comment::send_comment_activity(&creator, &conn, &community,vec!(community.get_shared_inbox_url()), delete)?; + send_activity_to_community( + &creator, + &conn, + &community, + vec![community.get_shared_inbox_url()], + delete, + )?; Ok(()) } @@ -265,7 +268,13 @@ impl ApubObjectType for Comment { .set_actor_xsd_any_uri(creator.actor_id.to_owned())? .set_object_base_box(delete)?; - Comment::send_comment_activity(&creator, &conn, &community, vec!(community.get_shared_inbox_url()),undo)?; + send_activity_to_community( + &creator, + &conn, + &community, + vec![community.get_shared_inbox_url()], + undo, + )?; Ok(()) } @@ -287,7 +296,13 @@ impl ApubObjectType for Comment { .set_actor_xsd_any_uri(mod_.actor_id.to_owned())? .set_object_base_box(note)?; - Comment::send_comment_activity(&mod_, &conn, &community, vec!(community.get_shared_inbox_url()),remove)?; + send_activity_to_community( + &mod_, + &conn, + &community, + vec![community.get_shared_inbox_url()], + remove, + )?; Ok(()) } @@ -326,7 +341,13 @@ impl ApubObjectType for Comment { .set_actor_xsd_any_uri(mod_.actor_id.to_owned())? .set_object_base_box(remove)?; - Comment::send_comment_activity(&mod_, &conn, &community, vec!(community.get_shared_inbox_url()),undo)?; + send_activity_to_community( + &mod_, + &conn, + &community, + vec![community.get_shared_inbox_url()], + undo, + )?; Ok(()) } } @@ -349,7 +370,13 @@ impl ApubLikeableType for Comment { .set_actor_xsd_any_uri(creator.actor_id.to_owned())? .set_object_base_box(note)?; - Comment::send_comment_activity(&creator, &conn, &community, vec!(community.get_shared_inbox_url()),like)?; + send_activity_to_community( + &creator, + &conn, + &community, + vec![community.get_shared_inbox_url()], + like, + )?; Ok(()) } @@ -370,7 +397,13 @@ impl ApubLikeableType for Comment { .set_actor_xsd_any_uri(creator.actor_id.to_owned())? .set_object_base_box(note)?; - Comment::send_comment_activity(&creator, &conn, &community, vec!(community.get_shared_inbox_url()),dislike)?; + send_activity_to_community( + &creator, + &conn, + &community, + vec![community.get_shared_inbox_url()], + dislike, + )?; Ok(()) } @@ -407,7 +440,13 @@ impl ApubLikeableType for Comment { .set_actor_xsd_any_uri(creator.actor_id.to_owned())? .set_object_base_box(like)?; - Comment::send_comment_activity(&creator, &conn, &community, vec!(community.get_shared_inbox_url()), undo)?; + send_activity_to_community( + &creator, + &conn, + &community, + vec![community.get_shared_inbox_url()], + undo, + )?; Ok(()) } } @@ -455,7 +494,7 @@ fn collect_non_local_mentions_and_addresses( } } - let mut inboxes = vec!(community.get_shared_inbox_url()); + let mut inboxes = vec![community.get_shared_inbox_url()]; inboxes.extend(mention_inboxes); inboxes = inboxes.into_iter().unique().collect(); @@ -465,26 +504,3 @@ fn collect_non_local_mentions_and_addresses( tags, }) } - -impl Comment { - fn send_comment_activity( - creator: &User_, - conn: &PgConnection, - community: &Community, - to: Vec, - activity: A, - ) -> Result<(), Error> - where - A: Activity + Base + Serialize + Debug, - { - insert_activity(&conn, creator.id, &activity, true)?; - - // if this is a local community, we need to do an announce from the community instead - if community.local { - Community::do_announce(activity, &community, &creator.actor_id, conn, true)?; - } else { - send_activity(&activity, creator, to)?; - } - Ok(()) - } -} diff --git a/server/src/apub/community.rs b/server/src/apub/community.rs index 9cd65334..a81e21cf 100644 --- a/server/src/apub/community.rs +++ b/server/src/apub/community.rs @@ -385,15 +385,12 @@ impl Community { pub fn do_announce( activity: A, community: &Community, - sender: &str, + sender: &dyn ActorType, conn: &PgConnection, - is_local_activity: bool, ) -> Result where A: Activity + Base + Serialize + Debug, { - insert_activity(&conn, -1, &activity, is_local_activity)?; - let mut announce = Announce::default(); populate_object_props( &mut announce.object_props, @@ -405,14 +402,13 @@ impl Community { .set_actor_xsd_any_uri(community.actor_id.to_owned())? .set_object_base_box(BaseBox::from_concrete(activity)?)?; - insert_activity(&conn, -1, &announce, true)?; + insert_activity(&conn, community.creator_id, &announce, true)?; // dont send to the instance where the activity originally came from, because that would result // in a database error (same data inserted twice) let mut to = community.get_follower_inboxes(&conn)?; - let sending_user = get_or_fetch_and_upsert_remote_user(&sender, conn)?; // this seems to be the "easiest" stable alternative for remove_item() - to.retain(|x| *x != sending_user.get_shared_inbox_url()); + to.retain(|x| *x != sender.get_shared_inbox_url()); send_activity(&announce, community, to)?; diff --git a/server/src/apub/post.rs b/server/src/apub/post.rs index 2b8455d5..f53af309 100644 --- a/server/src/apub/post.rs +++ b/server/src/apub/post.rs @@ -1,6 +1,6 @@ use crate::{ apub::{ - activities::{populate_object_props, send_activity}, + activities::{populate_object_props, send_activity_to_community}, create_apub_response, create_apub_tombstone_response, create_tombstone, @@ -16,7 +16,6 @@ use crate::{ }, convert_datetime, db::{ - activity::insert_activity, community::Community, post::{Post, PostForm}, user::User_, @@ -29,15 +28,13 @@ use activitystreams::{ activity::{Create, Delete, Dislike, Like, Remove, Undo, Update}, context, object::{kind::PageType, properties::ObjectProperties, AnyImage, Image, Page, Tombstone}, - Activity, - Base, BaseBox, }; use activitystreams_ext::Ext1; use actix_web::{body::Body, web::Path, HttpResponse, Result}; use diesel::PgConnection; -use failure::{Error, _core::fmt::Debug}; -use serde::{Deserialize, Serialize}; +use failure::Error; +use serde::Deserialize; #[derive(Deserialize)] pub struct PostQuery { @@ -241,7 +238,13 @@ impl ApubObjectType for Post { .set_actor_xsd_any_uri(creator.actor_id.to_owned())? .set_object_base_box(BaseBox::from_concrete(page)?)?; - Post::send_post_activity(creator, conn, &community, create)?; + send_activity_to_community( + creator, + conn, + &community, + vec![community.get_shared_inbox_url()], + create, + )?; Ok(()) } @@ -262,7 +265,13 @@ impl ApubObjectType for Post { .set_actor_xsd_any_uri(creator.actor_id.to_owned())? .set_object_base_box(BaseBox::from_concrete(page)?)?; - Post::send_post_activity(creator, conn, &community, update)?; + send_activity_to_community( + creator, + conn, + &community, + vec![community.get_shared_inbox_url()], + update, + )?; Ok(()) } @@ -285,7 +294,13 @@ impl ApubObjectType for Post { let community = Community::read(conn, self.community_id)?; - Post::send_post_activity(creator, conn, &community, delete)?; + send_activity_to_community( + creator, + conn, + &community, + vec![community.get_shared_inbox_url()], + delete, + )?; Ok(()) } @@ -323,7 +338,13 @@ impl ApubObjectType for Post { .set_object_base_box(delete)?; let community = Community::read(conn, self.community_id)?; - Post::send_post_activity(creator, conn, &community, undo)?; + send_activity_to_community( + creator, + conn, + &community, + vec![community.get_shared_inbox_url()], + undo, + )?; Ok(()) } @@ -346,7 +367,13 @@ impl ApubObjectType for Post { let community = Community::read(conn, self.community_id)?; - Post::send_post_activity(mod_, conn, &community, remove)?; + send_activity_to_community( + mod_, + conn, + &community, + vec![community.get_shared_inbox_url()], + remove, + )?; Ok(()) } fn send_undo_remove(&self, mod_: &User_, conn: &PgConnection) -> Result<(), Error> { @@ -382,7 +409,13 @@ impl ApubObjectType for Post { .set_object_base_box(remove)?; let community = Community::read(conn, self.community_id)?; - Post::send_post_activity(mod_, conn, &community, undo)?; + send_activity_to_community( + mod_, + conn, + &community, + vec![community.get_shared_inbox_url()], + undo, + )?; Ok(()) } } @@ -404,7 +437,13 @@ impl ApubLikeableType for Post { .set_actor_xsd_any_uri(creator.actor_id.to_owned())? .set_object_base_box(BaseBox::from_concrete(page)?)?; - Post::send_post_activity(&creator, &conn, &community, like)?; + send_activity_to_community( + &creator, + &conn, + &community, + vec![community.get_shared_inbox_url()], + like, + )?; Ok(()) } @@ -424,7 +463,13 @@ impl ApubLikeableType for Post { .set_actor_xsd_any_uri(creator.actor_id.to_owned())? .set_object_base_box(BaseBox::from_concrete(page)?)?; - Post::send_post_activity(&creator, &conn, &community, dislike)?; + send_activity_to_community( + &creator, + &conn, + &community, + vec![community.get_shared_inbox_url()], + dislike, + )?; Ok(()) } @@ -460,29 +505,13 @@ impl ApubLikeableType for Post { .set_actor_xsd_any_uri(creator.actor_id.to_owned())? .set_object_base_box(like)?; - Post::send_post_activity(&creator, &conn, &community, undo)?; - Ok(()) - } -} - -impl Post { - fn send_post_activity( - creator: &User_, - conn: &PgConnection, - community: &Community, - activity: A, - ) -> Result<(), Error> - where - A: Activity + Base + Serialize + Debug, - { - insert_activity(&conn, creator.id, &activity, true)?; - - // if this is a local community, we need to do an announce from the community instead - if community.local { - Community::do_announce(activity, &community, &creator.actor_id, conn, true)?; - } else { - send_activity(&activity, creator, vec![community.get_shared_inbox_url()])?; - } + send_activity_to_community( + &creator, + &conn, + &community, + vec![community.get_shared_inbox_url()], + undo, + )?; Ok(()) } } diff --git a/server/src/apub/shared_inbox.rs b/server/src/apub/shared_inbox.rs index b4a214a0..81dd14c3 100644 --- a/server/src/apub/shared_inbox.rs +++ b/server/src/apub/shared_inbox.rs @@ -122,6 +122,7 @@ pub async fn shared_inbox( // TODO: this is hacky, we should probably send the community id directly somehow let to = cc.replace("/followers", ""); + // TODO: this is ugly match get_or_fetch_and_upsert_remote_user(&sender.to_string(), &conn) { Ok(u) => verify(&request, &u), Err(_) => { @@ -134,15 +135,15 @@ pub async fn shared_inbox( (SharedAcceptedObjects::Create(c), Some("Page")) => { receive_create_post(&c, &conn, chat_server)?; announce_activity_if_valid::(*c, &to, sender, conn) - }, + } (SharedAcceptedObjects::Update(u), Some("Page")) => { receive_update_post(&u, &conn, chat_server)?; announce_activity_if_valid::(*u, &to, sender, conn) - }, + } (SharedAcceptedObjects::Like(l), Some("Page")) => { receive_like_post(&l, &conn, chat_server)?; announce_activity_if_valid::(*l, &to, sender, conn) - }, + } (SharedAcceptedObjects::Dislike(d), Some("Page")) => { receive_dislike_post(&d, &conn, chat_server)?; announce_activity_if_valid::(*d, &to, sender, conn) @@ -150,11 +151,11 @@ pub async fn shared_inbox( (SharedAcceptedObjects::Delete(d), Some("Page")) => { receive_delete_post(&d, &conn, chat_server)?; announce_activity_if_valid::(*d, &to, sender, conn) - }, + } (SharedAcceptedObjects::Remove(r), Some("Page")) => { receive_remove_post(&r, &conn, chat_server)?; announce_activity_if_valid::(*r, &to, sender, conn) - }, + } (SharedAcceptedObjects::Create(c), Some("Note")) => { receive_create_comment(&c, &conn, chat_server)?; announce_activity_if_valid::(*c, &to, sender, conn) @@ -166,7 +167,7 @@ pub async fn shared_inbox( (SharedAcceptedObjects::Like(l), Some("Note")) => { receive_like_comment(&l, &conn, chat_server)?; announce_activity_if_valid::(*l, &to, sender, conn) - }, + } (SharedAcceptedObjects::Dislike(d), Some("Note")) => { receive_dislike_comment(&d, &conn, chat_server)?; announce_activity_if_valid::(*d, &to, sender, conn) @@ -190,22 +191,21 @@ pub async fn shared_inbox( (SharedAcceptedObjects::Undo(u), Some("Delete")) => { receive_undo_delete(&u, &conn, chat_server)?; announce_activity_if_valid::(*u, &to, sender, conn) - }, + } (SharedAcceptedObjects::Undo(u), Some("Remove")) => { receive_undo_remove(&u, &conn, chat_server)?; announce_activity_if_valid::(*u, &to, sender, conn) - }, + } (SharedAcceptedObjects::Undo(u), Some("Like")) => { receive_undo_like(&u, &conn, chat_server)?; announce_activity_if_valid::(*u, &to, sender, conn) - }, - (SharedAcceptedObjects::Announce(a), _) => { - receive_announce(a, &conn, chat_server) - }, + } + (SharedAcceptedObjects::Announce(a), _) => receive_announce(a, &conn, chat_server), (a, _) => receive_unhandled_activity(a), } } +// TODO: should pass in sender as ActorType, but thats a bit tricky in shared_inbox() fn announce_activity_if_valid( activity: A, community_uri: &str, @@ -215,12 +215,14 @@ fn announce_activity_if_valid( where A: Activity + Base + Serialize + Debug, { - // TODO: first check that it is addressed to a local community let community = Community::read_from_actor_id(conn, &community_uri)?; - if !community.local { - // ignore this object + if community.local { + let sending_user = get_or_fetch_and_upsert_remote_user(&sender.to_string(), &conn)?; + insert_activity(&conn, sending_user.id, &activity, false)?; + Community::do_announce(activity, &community, &sending_user, conn) + } else { + Ok(HttpResponse::NotFound().finish()) } - Community::do_announce(activity, &community, sender, conn, false) } fn receive_announce( diff --git a/server/src/db/activity.rs b/server/src/db/activity.rs index 12137887..714c8202 100644 --- a/server/src/db/activity.rs +++ b/server/src/db/activity.rs @@ -71,8 +71,7 @@ where updated: None, }; debug!("inserting activity for user {}, data {:?}", user_id, data); - // TODO: this is broken - //Activity::create(&conn, &activity_form)?; + Activity::create(&conn, &activity_form)?; Ok(()) }