Federate community reports #5496
Conversation
…e or community, and add SiteOrCommunity::local_community
dullbananas
requested review from
Nutomic,
dessalines,
phiresky and
SleeplessOne1917
as code owners
crates/apub/src/fetcher/report.rs
Outdated
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
You can directly use Either via get dependency in Cargo.toml:
activitypub_federation = {
git = "https://github.com/LemmyNet/activitypub-federation-rust.git",
branch = "object-either",
default-features = false,
features = [
"actix-web",
] }
| @@ -107,13 +107,57 @@ impl Object for SiteOrCommunity { | |||
| } | |||
| } | |||
|
|
|||
| impl Actor for SiteOrCommunity { | |||
There was a problem hiding this comment.
I also implemented Either for Actor.
There was a problem hiding this comment.
Does that mean this should come after LemmyNet/activitypub-federation-rust#139, or that its just something to keep in mind.
There was a problem hiding this comment.
I published those changes as 0.6.4 now.
https://github.com/LemmyNet/activitypub-federation-rust/releases/tag/0.6.4
| @@ -107,13 +107,57 @@ impl Object for SiteOrCommunity { | |||
| } | |||
| } | |||
|
|
|||
| impl Actor for SiteOrCommunity { | |||
There was a problem hiding this comment.
Does that mean this should come after LemmyNet/activitypub-federation-rust#139, or that its just something to keep in mind.
| ) -> LemmyResult<()> { | ||
| // admin action comes from the correct instance, so it was presumably done | ||
| // by an instance admin. | ||
| // TODO: federate instance admin status and check it here |
There was a problem hiding this comment.
So this would need to be skipped until we federate admins?
There was a problem hiding this comment.
We currently assume that any user who is on the same instance as a user or community can perform admin actions on them. This is also in the existing code. It works fine because the remote Lemmy instance performs its own permission checks, and admin actions only federate for users/communities from the same instance.
|
Just needs above comments addressed, and conflicts, then we can merge. |
This reverts commit 8208c8a.
| let community = self.to[0].dereference(context).await?; | ||
| Ok(community) | ||
| } | ||
| } |
There was a problem hiding this comment.
Its easier to keep this impl and return an error in case of community, then you dont need match in the activity_lists.rs function.
| object_id: Url, | ||
| actor: Person, | ||
| report_creator: Person, | ||
| site: Site, |
There was a problem hiding this comment.
Should be called CreateCommunityReport and ResolveCommunityReport to be clear.
There was a problem hiding this comment.
On L97, why not just split out all the different report types like we do for everything else here?
IE CreateCommentReport, CreatePrivateMessageReport, etc
There was a problem hiding this comment.
Its unnecessary because they are federated with the same activity, so it would only lead to duplicate code which does the exact same thing.
Also the name is probably fine as is because we might also want to federate private message reports later. And CreateCommunityReport can sound like reporting something in a community.
There was a problem hiding this comment.
I removed these variants and made the other two report variants use Either
| &self, | ||
| context: &Data<LemmyContext>, | ||
| ) -> LemmyResult<Either<ApubSite, ApubCommunity>> { | ||
| self.object.receiver(context).await | ||
| } |
There was a problem hiding this comment.
This method is unnecessary, you can directly write activity.object.receiver()
|
Please write an api test as well to ensure that it works, and keeps working in the future. |
| ActivityChannel::submit_activity( | ||
| SendActivityData::SendResolveReportToSite { | ||
| object_id: community_report_view.community.ap_id.inner().clone(), | ||
| actor: local_user_view.person, | ||
| report_creator: community_report_view.creator.clone(), | ||
| site, | ||
| }, | ||
| &context, | ||
| )?; |
There was a problem hiding this comment.
I don't think it's a good idea to allow admins of another instance to resolve a local community report.
While reports of posts and comments are currently mostly considered to be mod responsibility, communities are always admin responsibility.
Communities can very much be violating the reporter's instance rules, such as a remote porn community on an instance not allowing porn, but the community instance might be fine with that.
IMO the only case when a local community report of a remote community should get resolved through federation is when the community gets removed by an admin of the community instance, in all other cases the report should stay around for local admin review.
It could be allowed for an admin of the reporter's instance to federate resolving the report to the community instance, but this should likely only happen when the community does not get removed on the reporter's instance, when the admin determines the report not to be actionable.
There was a problem hiding this comment.
This should be opened as a separate issue
There was a problem hiding this comment.
it might make sense to extract the entire functionality of federating resolving of community reports if you consider that too much for this PR, but imo this shouldn't be merged with full federation of resolving those reports in the first place and then ripping it out again after merging this.
There was a problem hiding this comment.
What exact actions could a local admin take in response to a remote admin resolving a community report?
There was a problem hiding this comment.
best example is an instance that doesn't allow nsfw content, but it federates with instances that allow this.
a user on the non-nsfw instance reports the community for being nsfw, which the community instance admin resolves as not actionable, as it's allowed there. the admin of the reporter's instance will still want to see the report to remove the community locally.
|
@dullbananas deployed a lemmy-js-client for you: |
Closes #4897