Federated Moderation #185

Manually merged
dessalines merged 20 commits from federated-moderation into main 2021-03-24 15:49:38 +00:00
Owner

I moved the list of mods from attributedTo to a new collection, so that it can be referred to by Add/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).

I moved the list of mods from `attributedTo` to a new collection, so that it can be referred to by `Add`/`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).
nutomic added 9 commits 2021-03-05 13:50:24 +00:00
e78ba38e94
Use URL type in most outstanding struct fields (#1468)
* Use URL type in most outstanding struct fields

This fixes all known remaining cases where url fields are stored as
plain strings, with the exception of form fields where empty strings
are used as sentinels (see `diesel_option_overwrite_to_url`).

Tested for regressions in the federated docker setup attempting to
exercise all changed fields, including through apub federation.

Fixes #1385

* Add migration to fix blank-string post.url values to be null

This also then fixes #602

* Address review feedback

- Fixed some unwraps and err message formatting
- Bumped the `url` library to 2.2.1 to fix a bug with serde error
  messages
- Add unit tests for the two diesel option override functions
- Fix migration teardown by adding a no-op

* Rename lemmy_db_queries::Url to lemmy_db_queries::DbUrl

* fix compile error

* box PostOrComment variants
134fece36d
Adding a password length check to other API actions. (#1474)
* Adding a password length check to other API actions.

- Fixes #1473

* Fixing comment.
e0c61c1334
Merge pull request #1478 from LemmyNet/fix_wrong_urls
Fixing wrong user_ and community icon and banner urls.
nutomic force-pushed federated-moderation from 90926d94ff to 3ffae1f5b8 2021-03-09 17:14:29 +00:00 Compare
nutomic added 2 commits 2021-03-11 16:51:31 +00:00
nutomic added 1 commit 2021-03-11 17:12:39 +00:00
nutomic added 1 commit 2021-03-11 17:18:27 +00:00
Author
Owner

I had another try with the tests, to see what breaks where:

  • Commit 803aad3b3e breaks test "Fetch in_reply_tos". Dunno why, it seemed to work fine when I tried manually.
  • Commit 50559de6d2 breaks the tests completely. Even commenting out the check in receive_update_post() completely (so behaviour should be identical to the previous commit), it fails to run the comment.spec.ts test suite.
I had another try with the tests, to see what breaks where: - Commit 803aad3b3ea31d5985213c5124ee3cbfbe28f331 breaks test "Fetch in_reply_tos". Dunno why, it seemed to work fine when I tried manually. - Commit 50559de6d2a1656343072485c5bab3b30e23b660 breaks the tests completely. Even commenting out the check in `receive_update_post()` completely (so behaviour should be identical to the previous commit), it fails to run the `comment.spec.ts` test suite.
nutomic added 2 commits 2021-03-12 15:46:50 +00:00
Author
Owner

The last commit (de14636e10) is quite stupid, I had to make expected_domain optional for FromApub and FromApubToForm 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 be Some. Dunno if theres a better way, maybe a bool is_mod_action?

The last commit (de14636e1073f1c9ac2b9683d230ded891f912fb) is quite stupid, I had to make `expected_domain` optional for `FromApub` and `FromApubToForm` 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 be `Some`. Dunno if theres a better way, maybe a bool `is_mod_action`?
nutomic added 2 commits 2021-03-16 17:27:19 +00:00
nutomic reviewed 2021-03-16 17:30:56 +00:00
@ -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
Author
Owner

This and the one below were wrong, we never receive a Create/Comment in the user inbox, its always wrapped in an announce.

This and the one below were wrong, we never receive a Create/Comment in the user inbox, its always wrapped in an announce.
Author
Owner

Turns out I was wrong, we do receive mentions this way.

Turns out I was wrong, we do receive mentions this way.
dessalines marked this conversation as resolved
nutomic added 1 commit 2021-03-17 17:13:04 +00:00
nutomic changed title from WIP: Federated Moderation to Federated Moderation 2021-03-17 17:13:59 +00:00
nutomic force-pushed federated-moderation from 4f35546937 to 3c58c95733 2021-03-18 14:37:45 +00:00 Compare
nutomic force-pushed federated-moderation from 3c58c95733 to 4f7dca7c2b 2021-03-18 16:02:39 +00:00 Compare
Owner

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?

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?
nutomic added 29 commits 2021-03-19 16:12:38 +00:00
8ee624a542 Some changes
- Changing claim name to local_user_id to facilitate logout.
- Changing AddAdmin back to using person_id
33a326854a
Set CARGO_HOME for CI so deps arent redownloaded (#1497)
* Set CARGO_HOME for CI so deps arent redownloaded

* run find on x86

* fix path

* cleanup

* try again

* use mv
dessalines reviewed 2021-03-19 16:50:45 +00:00
@ -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);
Owner

I wonder why this didn't work in the opposite direction.

I wonder why this didn't work in the opposite direction.
Author
Owner

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.

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.
dessalines marked this conversation as resolved
@ -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 {
Owner

I'm sure this is fine, but the lack of parenthesis scares me that it might be evaluated in a strange order.

I'm sure this is fine, but the lack of parenthesis scares me that it might be evaluated in a strange order.
Author
Owner

How would the order change anything? But I can add parenthesis if it makes you happy.

How would the order change anything? But I can add parenthesis if it makes you happy.
Owner

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.

`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.
Author
Owner

I dont think Rust would do that, but couldnt find any documentation on the behaviour. So I'm adding the parenthesis just in case.

I dont think Rust would do that, but couldnt find any documentation on the behaviour. So I'm adding the parenthesis just in case.
dessalines marked this conversation as resolved
@ -144,2 +110,4 @@
Ok(())
}
pub(crate) async fn fetch_community_mods(
Owner

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.

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.
Author
Owner

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.

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.
Owner

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:

  1. Get initial data
  2. When we don't receive atomic updates (the last_refresh_at is only necessary bc community and user edits aren't pushed). New community moderators are pushed, so I don't know that this is necessary.
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: 1. Get initial data 2. When we don't receive atomic updates (the last_refresh_at is only necessary bc community and user edits aren't pushed). New community moderators are pushed, so I don't know that this is necessary.
Author
Owner

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.

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.
dessalines marked this conversation as resolved
@ -377,0 +552,4 @@
T: ActorAndObjectRef + BaseExt<Kind>,
{
match announce {
None => verify_actor_is_community_mod(mod_action, community, context).await?,
Owner

So if its not an announce, then you know its local. Hopefully that check is done somewhere up the chain.

So if its not an announce, then you know its local. Hopefully that check is done somewhere up the chain.
Author
Owner

The Option<Announce> is passed into all methods which use this check.

The `Option<Announce>` is passed into all methods which use this check.
dessalines marked this conversation as resolved
@ -125,0 +118,4 @@
)
.await?;
let new_moderators = fetch_community_mods(context, group, request_counter).await?;
Owner

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.

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.
Author
Owner

There is also send_remove_mod(), and code to receive that activity in receive_for_community.rs.

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
Owner

This needs to be fixed

This needs to be fixed
Author
Owner

Oops, fixed.

Oops, fixed.
dessalines marked this conversation as resolved
nutomic added 1 commit 2021-03-19 17:09:26 +00:00
nutomic added 1 commit 2021-03-22 12:52:36 +00:00
nutomic changed title from Federated Moderation to WIP: Federated Moderation 2021-03-22 13:43:16 +00:00
nutomic added 1 commit 2021-03-22 14:08:57 +00:00
nutomic changed title from WIP: Federated Moderation to Federated Moderation 2021-03-22 14:09:04 +00:00
Author
Owner

I 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 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.
Owner
 FAIL  src/comment.spec.ts (24.963 s)
  ✓ Create a comment (559 ms)
  ✓ Create a comment in a non-existent post (10 ms)
  ✓ Update a comment (755 ms)
  ✓ Delete a comment (822 ms)
  ✓ Remove a comment from admin and community on the same instance (802 ms)
  ✓ Remove a comment from admin and community on different instance (4813 ms)
  ✓ Unlike a comment (608 ms)
  ✓ Federated comment like (546 ms)
  ✓ Reply to a comment (828 ms)
  ✓ Mention beta (853 ms)
  ✓ Comment Search (441 ms)
  ✓ A and G subscribe to B (center) A posts, G mentions B, it gets announced to A (2044 ms)
  ✕ Fetch in_reply_tos: A is unsubbed from B, B makes a post, and some embedded comments, A subs to B, B updates the lowest level comment,
A fetches both the post and all the inreplyto comments for that post. (2481 ms)

  ● Fetch in_reply_tos: A is unsubbed from B, B makes a post, and some embedded comments, A subs to B, B updates the lowest level comment,
A fetches both the post and all the inreplyto comments for that post.

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.

``` FAIL src/comment.spec.ts (24.963 s) ✓ Create a comment (559 ms) ✓ Create a comment in a non-existent post (10 ms) ✓ Update a comment (755 ms) ✓ Delete a comment (822 ms) ✓ Remove a comment from admin and community on the same instance (802 ms) ✓ Remove a comment from admin and community on different instance (4813 ms) ✓ Unlike a comment (608 ms) ✓ Federated comment like (546 ms) ✓ Reply to a comment (828 ms) ✓ Mention beta (853 ms) ✓ Comment Search (441 ms) ✓ A and G subscribe to B (center) A posts, G mentions B, it gets announced to A (2044 ms) ✕ Fetch in_reply_tos: A is unsubbed from B, B makes a post, and some embedded comments, A subs to B, B updates the lowest level comment, A fetches both the post and all the inreplyto comments for that post. (2481 ms) ● Fetch in_reply_tos: A is unsubbed from B, B makes a post, and some embedded comments, A subs to B, B updates the lowest level comment, A fetches both the post and all the inreplyto comments for that post. ``` 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.
dessalines reviewed 2021-03-22 18:25:25 +00:00
@ -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()])
Owner

I tried removing these, and setting to public, it didn't fix the test.

I tried removing these, and setting to public, it didn't fix the test.
Author
Owner

This cant be it, the test failure happened before I made this change.

This cant be it, the test failure happened before I made this change.
dessalines marked this conversation as resolved
dessalines reviewed 2021-03-22 18:30:38 +00:00
@ -108,0 +114,4 @@
context,
expected_domain,
request_counter,
mod_action_allowed,
Owner

I'm confused by this. One is is_mod_action and the other is mod_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.

I'm confused by this. One is `is_mod_action` and the other is `mod_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.
Author
Owner

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.

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.
dessalines marked this conversation as resolved
Owner

Tracing down the error is extremely hard to follow, but it appears to be a receiving comment or post problem.

To clarify the issue:

  • A not following B
  • B makes post, and 2 comments.
  • A follows B
  • B updates comment.
  • A should receive the post and 2 comments. It receives the post, but no comments.
Tracing down the error is extremely hard to follow, but it appears to be a receiving comment or post problem. To clarify the issue: - A not following B - B makes post, and 2 comments. - A follows B - B updates comment. - A **should** receive the post and 2 comments. It receives the post, but no comments.
Author
Owner

Error in response: LemmyError { inner: NotFound }

It would be really nice if diesel could log the full query when that happens.

`Error in response: LemmyError { inner: NotFound }` It would be really nice if diesel could log the full query when that happens.
nutomic reviewed 2021-03-23 17:21:39 +00:00
@ -242,0 +249,4 @@
let community = blocking(context.pool(), move |conn| {
Community::read_from_followers_url(conn, &cid.into())
})
.await?;
Author
Owner

This is probably not necessary, because the method is only called from Post::fromApub, and the post object has the community ID in to. In fact we could change this method to take Page as param directly, instead of making it generic.

This is probably not necessary, because the method is only called from `Post::fromApub`, and the post object has the community ID in `to`. In fact we could change this method to take `Page` as param directly, instead of making it generic.
Author
Owner

Added a commit to fix this method, including both variants for getting the community, and debug comments which show where the problem is.

Added a commit to fix this method, including both variants for getting the community, and debug comments which show where the problem is.
dessalines marked this conversation as resolved
nutomic reviewed 2021-03-23 17:23:07 +00:00
@ -242,0 +242,4 @@
let community = blocking(context.pool(), move |conn| {
Community::read_from_apub_id(conn, &cid2.into())
})
.await?;
Author
Owner

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.

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.
dessalines marked this conversation as resolved
Author
Owner

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.

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.
nutomic force-pushed federated-moderation from b44bbea428 to c7524d924b 2021-03-24 15:35:33 +00:00 Compare
dessalines manually merged commit 8a10a9079f into main 2021-03-24 15:49:38 +00:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: LemmyNet/lemmy#185
No description provided.