LemmyNet
/
lemmy
3
2
Fork 3
Code Pull Requests 1 Releases Wiki Activity

Dont send to blocked instances, rewrite activity_sender #109

Closed
nutomic wants to merge 0 commits from no-send-blocked into main
pull from: no-send-blocked
merge into: LemmyNet:main
Conversation 5 Commits - Files Changed - +236 -299
nutomic commented 2020-09-30 18:39:27 +00:00
Owner
Copy Link

This was supposed to be a simple one line fix, but then I noticed that the function send_activity_to_community() didnt make any sense, as it was dealing with all kinds of different activities as if they were identical (plus I had to add the block checks). So I had to rewrite all of that.

This was supposed to be a simple one line fix, but then I noticed that the function `send_activity_to_community()` didnt make any sense, as it was dealing with all kinds of different activities as if they were identical (plus I had to add the block checks). So I had to rewrite all of that.
nutomic reviewed 2020-09-30 19:20:36 +00:00
lemmy_apub/src/community.rs
@ -306,2 +276,4 @@
})
.filter_map(Result::ok)
// Don't send to blocked instances
.filter(|inbox| check_is_apub_id_valid(inbox).is_ok())
nutomic commented 2020-09-30 19:20:36 +00:00
Author
Owner
Copy Link

This is actually not needed here.

This is actually not needed here.
nutomic reviewed 2020-09-30 19:43:00 +00:00
lemmy_apub/src/activity_queue.rs
@ -41,3 +171,2 @@
for to_url in &to {
check_is_apub_id_valid(&to_url)?;
assert!(check_is_apub_id_valid(&to_url).is_ok());
nutomic commented 2020-09-30 19:43:00 +00:00
Author
Owner
Copy Link

This will be active both in debug and release builds according to documentation. Not sure if we want that or not.

This will be active both in debug and release builds according to documentation. Not sure if we want that or not.
dessalines commented 2020-10-01 22:51:19 +00:00
Owner
Copy Link

Is this right? Seems weird to have an assert in anything but a test.

Is this right? Seems weird to have an assert in anything but a test.
dessalines commented 2020-10-01 17:51:50 +00:00
Owner
Copy Link

Now that the tests are passing and that's merged, I'll work on solving these merge conflicts.

Now that the tests are passing and that's merged, I'll work on solving these merge conflicts.
dessalines reviewed 2020-10-01 22:55:22 +00:00
lemmy_apub/src/activity_queue.rs
@ -24,0 +74,4 @@
.iter()
.filter(|inbox| Some(inbox) != sender_shared_inbox.as_ref().as_ref())
.filter(|inbox| inbox != &&community_shared_inbox)
.filter(|inbox| check_is_apub_id_valid(inbox).is_ok())
dessalines commented 2020-10-01 22:55:22 +00:00
Owner
Copy Link

I spose this is the main change here.

I spose this is the main change here.
nutomic commented 2020-10-02 11:40:46 +00:00
Author
Owner
Copy Link

Yep. One thing I'm a bit unsure about is when these blocked to should be quietly dropped (like here), and when they should throw an error (like in send_to_community()).

Yep. One thing I'm a bit unsure about is when these blocked `to` should be quietly dropped (like here), and when they should throw an error (like in send_to_community()).
dessalines commented 2020-10-01 22:56:54 +00:00
Owner
Copy Link

I merged from main, and the tests passed, but I'm testing it on the federation instances now.

I merged from main, and [the tests passed](https://travis-ci.org/github/LemmyNet/lemmy/builds/732074464), but I'm testing it on the federation instances now.
dessalines commented 2020-10-01 23:48:54 +00:00
Owner
Copy Link

I deployed to ds9, enterprise, etc, and I got this error when making a new post:

lemmy_1 | thread 'main' panicked at 'Invalid config, both allowed_instances and blocked_instances are specified', lemmy_apub/src/lib.rs:123:5,

which means that the logic in lib.rs needs to be corrected to allow for both allowed and block lists being empty (IE wide open)

I deployed to ds9, enterprise, etc, and I got this error when making a new post: `lemmy_1 | thread 'main' panicked at 'Invalid config, both allowed_instances and blocked_instances are specified', lemmy_apub/src/lib.rs:123:5`, which means that the logic in `lib.rs` needs to be corrected to allow for both allowed and block lists being empty (IE wide open)
nutomic commented 2020-10-02 11:49:01 +00:00
Author
Owner
Copy Link

Ah, this here should do it (in front of the if statement in lib.rs:107).

diff --git a/lemmy_apub/src/lib.rs b/lemmy_apub/src/lib.rs
index 22eb9fbe..3b6604b6 100644
--- a/lemmy_apub/src/lib.rs
+++ b/lemmy_apub/src/lib.rs
@@ -104,7 +104,9 @@ fn check_is_apub_id_valid(apub_id: &Url) -> Result<(), LemmyError> {
let mut allowed_instances = Settings::get().get_allowed_instances();
let blocked_instances = Settings::get().get_blocked_instances();

-  if !allowed_instances.is_empty() {
+  if allowed_instances.is_empty() && blocked_instances.is_empty() {
+    Ok(())
+  } else if !allowed_instances.is_empty() {
// need to allow this explicitly because apub activities might contain objects from our local
// instance. split is needed to remove the port in our federation test setup.
allowed_instances.push(local_instance);
Ah, this here should do it (in front of the if statement in lib.rs:107). ``` diff --git a/lemmy_apub/src/lib.rs b/lemmy_apub/src/lib.rs index 22eb9fbe..3b6604b6 100644 --- a/lemmy_apub/src/lib.rs +++ b/lemmy_apub/src/lib.rs @@ -104,7 +104,9 @@ fn check_is_apub_id_valid(apub_id: &Url) -> Result<(), LemmyError> { let mut allowed_instances = Settings::get().get_allowed_instances(); let blocked_instances = Settings::get().get_blocked_instances(); - if !allowed_instances.is_empty() { + if allowed_instances.is_empty() && blocked_instances.is_empty() { + Ok(()) + } else if !allowed_instances.is_empty() { // need to allow this explicitly because apub activities might contain objects from our local // instance. split is needed to remove the port in our federation test setup. allowed_instances.push(local_instance); ```
dessalines commented 2020-10-02 13:03:20 +00:00
Owner
Copy Link

K, trying out now.

K, trying out now.
nutomic closed this pull request 2020-10-10 13:53:35 +00:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
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#109
No description provided.
Delete Branch "no-send-blocked"

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?