Federated Moderation #185
Loading…
Reference in New Issue
No description provided.
Delete Branch "federated-moderation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I moved the list of mods from
attributedTo
to a new collection, so that it can be referred to byAdd
/Remove
activities. For that it probably has to go to/c/xyz/mods
instead of being included directly in the community object.It works totally fine in my testing, though I noticed that the order of mods doesnt change in the frontend if I transfer the community to a different user (I'm definitely updating
community.creator_id
correctly).90926d94ff
to3ffae1f5b8
I had another try with the tests, to see what breaks where:
803aad3b3e
breaks test "Fetch in_reply_tos". Dunno why, it seemed to work fine when I tried manually.50559de6d2
breaks the tests completely. Even commenting out the check inreceive_update_post()
completely (so behaviour should be identical to the previous commit), it fails to run thecomment.spec.ts
test suite.The last commit (
de14636e10
) is quite stupid, I had to makeexpected_domain
optional forFromApub
andFromApubToForm
because that cant be used when a remote mod changes the sticky/locked flags for a post. But for comment, user, community that parameter always has to beSome
. Dunno if theres a better way, maybe a boolis_mod_action
?@ -317,3 +337,1 @@
} else {
receive_create_private_message(&context, create, expected_domain, request_counter).await
}
receive_create_private_message(&context, create, expected_domain, request_counter).await
This and the one below were wrong, we never receive a Create/Comment in the user inbox, its always wrapped in an announce.
Turns out I was wrong, we do receive mentions this way.
WIP: Federated Moderationto Federated Moderation4f35546937
to3c58c95733
3c58c95733
to4f7dca7c2b
I spose I'll wait until after the merge conflict to go over. You said you didn't want to include this in 0.10.0 correct?
@ -175,1 +177,3 @@
let lockedPostRes = await lockPost(alpha, true, postRes.post_view.post);
let searchBeta = await searchPost(beta, postRes.post_view.post);
let betaPost1 = searchBeta.posts[0];
let lockedPostRes = await lockPost(beta, true, betaPost1.post);
I wonder why this didn't work in the opposite direction.
Because it was wrong. The remote user who is not a mod was locking their own post, which is not allowed as far as I understand. Until now the apub code allowed that, but I added a check for it.
@ -53,0 +76,4 @@
let stickied = page.ext_one.stickied.context(location_info!())?;
let locked = !page.ext_one.comments_enabled.context(location_info!())?;
let mut mod_action_allowed = false;
if stickied != old_post.stickied || locked != old_post.locked {
I'm sure this is fine, but the lack of parenthesis scares me that it might be evaluated in a strange order.
How would the order change anything? But I can add parenthesis if it makes you happy.
if (((stickied != old_post.stickied) || locked) != old_post.locked)
is different from
if ((stickied != old_post.stickied) || (locked != old_post.locked))
Without parenthesis order of operations might do the first.
I dont think Rust would do that, but couldnt find any documentation on the behaviour. So I'm adding the parenthesis just in case.
@ -144,2 +110,4 @@
Ok(())
}
pub(crate) async fn fetch_community_mods(
I see this is fetched every time
from_apub
is run on community. This doesn't seem right, it should be a smart upsert like the others.Any idea how? I dont see how an upsert works here, because we cant simply overwrite a single database row, but need to insert or delete rows.
Check out
get_or_fetch_upsert_community
, it looks like its already doing the initial moderators fetching.It should be keyed just like the
should_refetch_actor(c.last_refreshed_at)
, after the initial community fetch. In fact I don't even know that that's necessary, since the only reason for fetching is to:Ah that makes sense, I'm moving the update logic from objects/community.rs to fetcher/community.rs. But we still need to run this every time, because its possible that our instance doesnt follow the community, and doesnt receive any activities (we could possibly add a check for this later).
And it is still necessary to fetch the community creator in FromApubToForm, until that field is removed.
@ -377,0 +552,4 @@
T: ActorAndObjectRef + BaseExt<Kind>,
{
match announce {
None => verify_actor_is_community_mod(mod_action, community, context).await?,
So if its not an announce, then you know its local. Hopefully that check is done somewhere up the chain.
The
Option<Announce>
is passed into all methods which use this check.@ -125,0 +118,4 @@
)
.await?;
let new_moderators = fetch_community_mods(context, group, request_counter).await?;
Hopefully this is an upsert / initial fetch, seems quite a waste to be fetching this every time.
It also seemed like the send_add_mod means that newly added mods get pushed.
There is also send_remove_mod(), and code to receive that activity in receive_for_community.rs.
@ -29,7 +29,11 @@ services:
- ./volumes/pictrs_alpha:/mnt
lemmy-alpha-ui:
<<<<<<< HEAD
This needs to be fixed
Oops, fixed.
Federated Moderationto WIP: Federated ModerationWIP: Federated Moderationto Federated ModerationI made these changes compatible with Lemmy v0.9.9, so we can avoid a breaking change. Based on limited manual testing, communities, posts and comments federate both ways without problems now.
I did some extra logging around that, and found that its not fetching any comments like it should. There's something broken in the fetching code.
@ -69,2 +73,3 @@
.set_published(convert_datetime(self.published))
.set_to(public())
// NOTE: included community id for compatibility with lemmy v0.9.9
.set_many_tos(vec![community.actor_id.into_inner(), public()])
I tried removing these, and setting to public, it didn't fix the test.
This cant be it, the test failure happened before I made this change.
@ -108,0 +114,4 @@
context,
expected_domain,
request_counter,
mod_action_allowed,
I'm confused by this. One is
is_mod_action
and the other ismod_action_allowed
. What does converting a comment from apub -> DB have to do with mod actions? IE the core types are separate from actions on them.That is indeed confusing. The check in
Post::FromApub
is so that only mods can change the locked/sticky flag for a post, but not the post author. It would definitely be good if we could verify this in a simpler way.Tracing down the error is extremely hard to follow, but it appears to be a receiving comment or post problem.
To clarify the issue:
Error in response: LemmyError { inner: NotFound }
It would be really nice if diesel could log the full query when that happens.
@ -242,0 +249,4 @@
let community = blocking(context.pool(), move |conn| {
Community::read_from_followers_url(conn, &cid.into())
})
.await?;
This is probably not necessary, because the method is only called from
Post::fromApub
, and the post object has the community ID into
. In fact we could change this method to takePage
as param directly, instead of making it generic.Added a commit to fix this method, including both variants for getting the community, and debug comments which show where the problem is.
@ -242,0 +242,4 @@
let community = blocking(context.pool(), move |conn| {
Community::read_from_apub_id(conn, &cid2.into())
})
.await?;
This is where the problem is. It should be
get_or_fetch_and_upsert_community()
like it was before, but that results in a stack overflow. Reading directly from the database fixes that, but its obviously not the same. I tried to add#![recursion_limit = "512"]
in every crate, but it didnt help.Tests are fine now, but history is a mess. Squashing it all into one commit isnt a good idea because there are too many changes. So before merging, let me know and I will manually squash some of the commits together where it makes sense.
b44bbea428
toc7524d924b