Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Receive & store remote site bans #4571

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions crates/api_common/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,7 @@ use lemmy_db_schema::{
utils::DbPool,
};
use lemmy_db_views::{comment_view::CommentQuery, structs::LocalUserView};
use lemmy_db_views_actor::structs::{
CommunityModeratorView,
CommunityPersonBanView,
CommunityView,
};
use lemmy_db_views_actor::structs::{CommunityModeratorView, CommunityPersonBanView, CommunityView, SitePersonBanView};
use lemmy_utils::{
email::{send_email, translations::Lang},
error::{LemmyError, LemmyErrorExt, LemmyErrorType, LemmyResult},
Expand All @@ -51,6 +47,7 @@ use std::{collections::HashSet, time::Duration};
use tracing::warn;
use url::{ParseError, Url};
use urlencoding::encode;
use lemmy_db_views::structs::SiteView;

pub static AUTH_COOKIE_NAME: &str = "jwt";
#[cfg(debug_assertions)]
Expand Down Expand Up @@ -204,11 +201,26 @@ async fn check_community_ban(
community_id: CommunityId,
pool: &mut DbPool<'_>,
) -> LemmyResult<()> {
// check if user was banned from site or community
let is_banned = CommunityPersonBanView::get(pool, person.id, community_id).await?;
if is_banned {
// check if user was banned from community
let is_banned_from_community = CommunityPersonBanView::get(pool, person.id, community_id).await?;
if is_banned_from_community {
Err(LemmyErrorType::BannedFromCommunity)?
}


// for remote communities, check if the user was banned on the host site
let community_view = CommunityView::read(pool, community_id, None, false).await?;
if (!community_view.community.local) {

let instance_id = community_view.community.instance_id;
if let Some(site) = Site::read_from_instance_id(pool, instance_id).await? {
let is_banned_from_site_of_community = SitePersonBanView::get(pool, person.id, site.id).await?;
if is_banned_from_site_of_community {
Err(LemmyErrorType::BannedFromCommunity)?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on adding a separate error here, like BannedFromSiteHostingCommunity or something?

}
}
}

Ok(())
}

Expand Down
33 changes: 20 additions & 13 deletions crates/apub/src/activities/block/block_user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ use lemmy_db_schema::{
CommunityPersonBanForm,
},
moderator::{ModBan, ModBanForm, ModBanFromCommunity, ModBanFromCommunityForm},
person::{Person, PersonUpdateForm},
site::{SitePersonBan, SitePersonBanForm},
},
traits::{Bannable, Crud, Followable},
};
Expand Down Expand Up @@ -155,19 +155,26 @@ impl ActivityHandler for BlockUser {
let blocked_person = self.object.dereference(context).await?;
let target = self.target.dereference(context).await?;
match target {
SiteOrCommunity::Site(_site) => {
let blocked_person = Person::update(
&mut context.pool(),
blocked_person.id,
&PersonUpdateForm {
banned: Some(true),
ban_expires: Some(expires),
..Default::default()
},
)
.await?;
SiteOrCommunity::Site(site) => {
let site_user_ban_form = SitePersonBanForm {
site_id: site.id,
person_id: blocked_person.id,
expires: Some(expires),
};
SitePersonBan::ban(&mut context.pool(), &site_user_ban_form).await?;
if self.remove_data.unwrap_or(false) {
remove_user_data(blocked_person.id, context).await?;
let user_banned_on_home_instance = verify_domains_match(&site.id(), self.actor.inner())?
&& verify_domains_match(&site.id(), self.object.inner())?;
if user_banned_on_home_instance {
remove_user_data(blocked_person.id, context).await?;
} else {
// Currently, remote site bans federate data removal through corresponding community
// bans, because remote site bans were not even stored unless they came from the user's
// home instance. We can continue to use community bans to federate data removal for
// backwards compatibility, initially but at some point, when most instances have been
// upgraded to have the logic of storing remote site bans, we can start doing data
// removal here & remove the logic of federating community bans on remote site bans.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldnt bother with this, the mentioned changes in #4464 havent even been released yet and are quite hacky. So theres a chance that it will introduce problems of its own. Better to revert #4464 here and go for the proper solution directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this, I'd love to remove my weird logic for banning them from local communities.

}
}

// write mod log
Expand Down
20 changes: 8 additions & 12 deletions crates/apub/src/activities/block/undo_block_user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use lemmy_db_schema::{
activity::ActivitySendTargets,
community::{CommunityPersonBan, CommunityPersonBanForm},
moderator::{ModBan, ModBanForm, ModBanFromCommunity, ModBanFromCommunityForm},
person::{Person, PersonUpdateForm},
site::{SitePersonBan, SitePersonBanForm},
},
traits::{Bannable, Crud},
};
Expand Down Expand Up @@ -102,17 +102,13 @@ impl ActivityHandler for UndoBlockUser {
let mod_person = self.actor.dereference(context).await?;
let blocked_person = self.object.object.dereference(context).await?;
match self.object.target.dereference(context).await? {
SiteOrCommunity::Site(_site) => {
let blocked_person = Person::update(
&mut context.pool(),
blocked_person.id,
&PersonUpdateForm {
banned: Some(false),
ban_expires: Some(expires),
..Default::default()
},
)
.await?;
SiteOrCommunity::Site(site) => {
let site_user_ban_form = SitePersonBanForm {
site_id: site.id,
person_id: blocked_person.id,
expires: None,
};
SitePersonBan::unban(&mut context.pool(), &site_user_ban_form).await?;

// write mod log
let form = ModBanForm {
Expand Down
36 changes: 34 additions & 2 deletions crates/db_schema/src/impls/site.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ use crate::{
schema::site::dsl::{actor_id, id, instance_id, site},
source::{
actor_language::SiteLanguage,
site::{Site, SiteInsertForm, SiteUpdateForm},
site::{Site, SiteInsertForm, SitePersonBan, SitePersonBanForm, SiteUpdateForm},
},
traits::Crud,
traits::{Bannable, Crud},
utils::{get_conn, DbPool},
};
use diesel::{dsl::insert_into, result::Error, ExpressionMethods, OptionalExtension, QueryDsl};
Expand Down Expand Up @@ -100,3 +100,35 @@ impl Site {
url
}
}

#[async_trait]
impl Bannable for SitePersonBan {
type Form = SitePersonBanForm;
async fn ban(
pool: &mut DbPool<'_>,
site_person_ban_form: &SitePersonBanForm,
) -> Result<Self, Error> {
use crate::schema::site_person_ban::dsl::{person_id, site_id, site_person_ban};
let conn = &mut get_conn(pool).await?;
insert_into(site_person_ban)
.values(site_person_ban_form)
.on_conflict((site_id, person_id))
.do_update()
.set(site_person_ban_form)
.get_result::<Self>(conn)
.await
}

async fn unban(
pool: &mut DbPool<'_>,
site_person_ban_form: &SitePersonBanForm,
) -> Result<usize, Error> {
use crate::schema::site_person_ban::dsl::site_person_ban;
let conn = &mut get_conn(pool).await?;
diesel::delete(
site_person_ban.find((site_person_ban_form.person_id, site_person_ban_form.site_id)),
)
.execute(conn)
.await
}
}
12 changes: 12 additions & 0 deletions crates/db_schema/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,15 @@ diesel::table! {
}
}

diesel::table! {
site_person_ban (person_id, site_id) {
site_id -> Int4,
person_id -> Int4,
published -> Timestamptz,
expires -> Nullable<Timestamptz>,
}
}

diesel::table! {
tagline (id) {
id -> Int4,
Expand Down Expand Up @@ -1028,6 +1037,8 @@ diesel::joinable!(site -> instance (instance_id));
diesel::joinable!(site_aggregates -> site (site_id));
diesel::joinable!(site_language -> language (language_id));
diesel::joinable!(site_language -> site (site_id));
diesel::joinable!(site_person_ban -> person (person_id));
diesel::joinable!(site_person_ban -> site (site_id));
diesel::joinable!(tagline -> local_site (local_site_id));

diesel::allow_tables_to_appear_in_same_query!(
Expand Down Expand Up @@ -1102,5 +1113,6 @@ diesel::allow_tables_to_appear_in_same_query!(
site,
site_aggregates,
site_language,
site_person_ban,
tagline,
);
27 changes: 26 additions & 1 deletion crates/db_schema/src/source/site.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::newtypes::{DbUrl, InstanceId, SiteId};
use crate::newtypes::{CommunityId, DbUrl, InstanceId, PersonId, SiteId};
#[cfg(feature = "full")]
use crate::schema::site;
use chrono::{DateTime, Utc};
Expand Down Expand Up @@ -84,3 +84,28 @@ pub struct SiteUpdateForm {
pub public_key: Option<String>,
pub content_warning: Option<Option<String>>,
}

#[derive(PartialEq, Eq, Debug)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add #[skip_serializing_none]

#[cfg_attr(
feature = "full",
derive(Identifiable, Queryable, Selectable, Associations)
)]
#[cfg_attr(feature = "full", diesel(belongs_to(crate::source::site::Site)))]
#[cfg_attr(feature = "full", diesel(table_name = site_person_ban))]
#[cfg_attr(feature = "full", diesel(primary_key(person_id, site_id)))]
#[cfg_attr(feature = "full", diesel(check_for_backend(diesel::pg::Pg)))]
pub struct SitePersonBan {
pub site_id: SiteId,
pub person_id: PersonId,
pub published: DateTime<Utc>,
pub expires: Option<DateTime<Utc>>,
}

#[derive(Clone)]
#[cfg_attr(feature = "full", derive(Insertable, AsChangeset))]
#[cfg_attr(feature = "full", diesel(table_name = site_person_ban))]
pub struct SitePersonBanForm {
pub site_id: SiteId,
pub person_id: PersonId,
pub expires: Option<Option<DateTime<Utc>>>,
}
2 changes: 2 additions & 0 deletions crates/db_views_actor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,6 @@ pub mod person_block_view;
pub mod person_mention_view;
#[cfg(feature = "full")]
pub mod person_view;
#[cfg(feature = "full")]
pub mod site_person_ban_view;
pub mod structs;
25 changes: 25 additions & 0 deletions crates/db_views_actor/src/site_person_ban_view.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
use crate::structs::SitePersonBanView;
use diesel::{dsl::exists, result::Error, select, ExpressionMethods, QueryDsl};
use diesel_async::RunQueryDsl;
use lemmy_db_schema::{
newtypes::{PersonId, SiteId},
schema::site_person_ban,
utils::{get_conn, DbPool},
};

impl SitePersonBanView {
pub async fn get(
pool: &mut DbPool<'_>,
from_person_id: PersonId,
from_site_id: SiteId,
) -> Result<bool, Error> {
let conn = &mut get_conn(pool).await?;
select(exists(
site_person_ban::table
.filter(site_person_ban::site_id.eq(from_site_id))
.filter(site_person_ban::person_id.eq(from_person_id)),
))
.get_result::<bool>(conn)
.await
}
}
9 changes: 9 additions & 0 deletions crates/db_views_actor/src/structs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,3 +149,12 @@ pub struct PersonView {
pub counts: PersonAggregates,
pub is_admin: bool,
}

#[derive(Debug, Serialize, Deserialize, Clone)]
#[cfg_attr(feature = "full", derive(Queryable))]
#[cfg_attr(feature = "full", diesel(check_for_backend(diesel::pg::Pg)))]
/// A site person ban.
pub struct SitePersonBanView {
pub site: Site,
pub person: Person,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The view might not be necessary, since we're not returning this view specifically, but only fields joined from site_person_ban

Same for CommunityPersonBanView, but that's up to you whether to remove it or not.

1 change: 1 addition & 0 deletions migrations/2024-03-26-131042_site_person_ban/down.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DROP TABLE site_person_ban;
13 changes: 13 additions & 0 deletions migrations/2024-03-26-131042_site_person_ban/up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
CREATE TABLE site_person_ban
(
site_id INTEGER NOT NULL
REFERENCES site
ON UPDATE CASCADE ON DELETE CASCADE,
person_id INTEGER NOT NULL
REFERENCES person
ON UPDATE CASCADE ON DELETE CASCADE,
published TIMESTAMP WITH TIME ZONE DEFAULT NOW() NOT NULL,
expires TIMESTAMP WITH TIME ZONE,
PRIMARY KEY (person_id, site_id)
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.


9 changes: 8 additions & 1 deletion src/scheduled_tasks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use lemmy_db_schema::{
post,
received_activity,
sent_activity,
site_person_ban,
},
source::{
instance::{Instance, InstanceForm},
Expand Down Expand Up @@ -441,8 +442,14 @@ async fn update_banned_when_expired(pool: &mut DbPool<'_>) {
)
.execute(&mut conn)
.await
.inspect_err(|e| error!("Failed to remove community_ban expired rows: {e}"))
.inspect_err(|e| error!("Failed to remove community_person_ban expired rows: {e}"))
.ok();

diesel::delete(site_person_ban::table.filter(site_person_ban::expires.lt(now().nullable())))
.execute(&mut conn)
.await
.inspect_err(|e| error!("Failed to remove site_person_ban expired rows: {e}"))
.ok();
}
Err(e) => {
error!("Failed to get connection from pool: {e}");
Expand Down