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

Refactor inbox, simplify and split into multiple files #72

Merged
nutomic merged 8 commits from inbox-refactoring into main 2020-07-29 17:13:16 +00:00
Conversation 6 Commits 8 Files Changed 25 +1960 -1758
nutomic commented 2020-07-23 14:38:48 +00:00
Owner
Copy Link

Need to figure out how to convert activities to the proper type, same way as asonix' ap-relay does. Unfortunately that repo is down...

Need to figure out how to convert activities to the proper type, same way as asonix' ap-relay does. Unfortunately that repo is down...
nutomic commented 2020-07-28 12:48:27 +00:00
Author
Owner
Copy Link

Okay this is (mostly) working now. Tests are passing, but votes are not working because of what seems to be a bug in our code. Essentially, activitystreams now checks the domain of the actor field against the domain of the ID field (for security reasons). But this check fails when upvoting a remote post, because we use the community ID for creating the ID of the Like activity. So if a user on instance alpha upvotes a post in a community on instance beta, the upvote activity will have different domains in the actor and id fields which is not supposed to happen. The problem is here, but probably also in other places. Hope this makes sense. Its probably better to fix that in a separate PR, as it will require a general rework of activity ID generation.

Besides, I havent tested all federation functionality, so there may be more problems.

Edit: To be clear, this domain check was added in a new activitystreams version, I think we just never tested that particular functionality since then. So it has nothing to do with this PR. I think the proper fix would be to add a helper function that generates activity IDs like https://local-domain/activity/create/uuid.

Okay this is (mostly) working now. Tests are passing, but votes are not working because of what seems to be a bug in our code. Essentially, `activitystreams` now checks the domain of the actor field against the domain of the ID field (for security reasons). But this check fails when upvoting a remote post, because we use the community ID for creating the ID of the Like activity. So if a user on instance alpha upvotes a post in a community on instance beta, the upvote activity will have different domains in the actor and id fields which is not supposed to happen. [The problem is here](https://yerbamate.dev/LemmyNet/lemmy/src/branch/main/server/src/apub/post.rs#L487), but probably also in other places. Hope this makes sense. Its probably better to fix that in a separate PR, as it will require a general rework of activity ID generation. Besides, I havent tested all federation functionality, so there may be more problems. Edit: To be clear, this domain check was added in a new activitystreams version, I think we just never tested that particular functionality since then. So it has nothing to do with this PR. I think the proper fix would be to add a helper function that generates activity IDs like `https://local-domain/activity/create/uuid`.
dessalines commented 2020-07-28 15:26:13 +00:00
Owner
Copy Link

Hrm.... I spose its no problem to use the actor domain for all the id generations.

I'll test and merge this PR in a min, and also add a unit test for federated like, to make sure that it does fail.

Hrm.... I spose its no problem to use the actor domain for all the id generations. I'll test and merge this PR in a min, and also add a unit test for federated like, to make sure that it does fail.
nutomic changed title from WIP: Refactor inbox, simplify and split into multiple files to Refactor inbox, simplify and split into multiple files 2020-07-28 17:22:12 +00:00
dessalines commented 2020-07-28 22:59:37 +00:00
Owner
Copy Link

Sry this took me a bit, I adding the unit test now.

Sry this took me a bit, I adding the unit test now.
dessalines commented 2020-07-28 23:29:44 +00:00
Owner
Copy Link

I'm getting a lot of these errors btw:

lemmy-gamma_1     | [2020-07-28T23:22:34Z DEBUG lemmy_server::apub::fetcher] Receive error, Json deserialize error: missing field `ca
tegory` at line 1 column 135
lemmy-gamma_1     | [2020-07-28T23:22:34Z ERROR actix_http::response] Internal Server Error: LemmyError { inner: RecvError("Json dese
rialize error: missing field `category` at line 1 column 135")

Seems to be trying to send or fetch something to itself too:

lemmy-alpha_1     | [2020-07-28T23:22:32Z DEBUG lemmy_server::apub] lemmy-alpha not in ["lemmy-beta", "lemmy-gamma"]
lemmy-alpha_1     | [2020-07-28T23:22:32Z ERROR actix_http::response] Internal Server Error: LemmyError { inner: ErrorMessage { msg:
"Activitypub uri invalid or blocked: http://lemmy-alpha:8540/c/main/followers" }
I'm getting a lot of these errors btw: ``` lemmy-gamma_1 | [2020-07-28T23:22:34Z DEBUG lemmy_server::apub::fetcher] Receive error, Json deserialize error: missing field `ca tegory` at line 1 column 135 lemmy-gamma_1 | [2020-07-28T23:22:34Z ERROR actix_http::response] Internal Server Error: LemmyError { inner: RecvError("Json dese rialize error: missing field `category` at line 1 column 135") ``` Seems to be trying to send or fetch something to itself too: ``` lemmy-alpha_1 | [2020-07-28T23:22:32Z DEBUG lemmy_server::apub] lemmy-alpha not in ["lemmy-beta", "lemmy-gamma"] lemmy-alpha_1 | [2020-07-28T23:22:32Z ERROR actix_http::response] Internal Server Error: LemmyError { inner: ErrorMessage { msg: "Activitypub uri invalid or blocked: http://lemmy-alpha:8540/c/main/followers" } ```
dessalines commented 2020-07-28 23:39:13 +00:00
Owner
Copy Link

Okay the unit test is done, run

git merge inbox-refactoring-dessalines

Okay the unit test is done, run `git merge inbox-refactoring-dessalines`
nutomic commented 2020-07-29 12:03:12 +00:00
Author
Owner
Copy Link

Okay I fixed a bug in the community fetching (the one in your second log, have to remove the /followers at the end so its the proper community ID and can be found in the DB). I also renamed the fetch_remote_xxx methods because we actually use them for local objects as well.

Also I disabled the ID check in FromApub by using id_insecure(), until we figure out how to do that properly.

Not sure about the missing field error, can you tell me how to reproduce that?

Okay I fixed a bug in the community fetching (the one in your second log, have to remove the `/followers` at the end so its the proper community ID and can be found in the DB). I also renamed the fetch_remote_xxx methods because we actually use them for local objects as well. Also I disabled the ID check in FromApub by using `id_insecure()`, until we figure out how to do that properly. Not sure about the missing field error, can you tell me how to reproduce that?
dessalines reviewed 2020-07-29 16:24:11 +00:00
server/src/apub/comment.rs
@ -115,3 +121,1 @@
self.updated,
NoteType::Note.to_string(),
)
create_tombstone(self.deleted, &self.ap_id, self.updated, NoteType::Note)
dessalines commented 2020-07-29 16:24:11 +00:00
Owner
Copy Link

This is nice that we don't have to use the string types anymore.

This is nice that we don't have to use the string types anymore.
dessalines reviewed 2020-07-29 16:30:32 +00:00
server/src/apub/comment.rs
@ -179,3 +182,3 @@
updated: note.updated().map(|u| u.to_owned().naive_local()),
deleted: None,
ap_id: note.id(actor_id.domain().unwrap())?.unwrap().to_string(),
ap_id: note.id_unchecked().unwrap().to_string(),
dessalines commented 2020-07-29 16:30:32 +00:00
Owner
Copy Link

Did you wanna try to fix our id generation in this ticket? Or just leave a TODO for later.

Maybe seeing the full object would help me think about this.

Did you wanna try to fix our id generation in this ticket? Or just leave a TODO for later. Maybe seeing the full object would help me think about this.
dessalines reviewed 2020-07-29 16:32:28 +00:00
server/src/apub/community.rs
@ -372,3 +361,1 @@
.id(actor_id.domain().unwrap())?
.unwrap()
.to_string(),
actor_id: group.inner.id_unchecked().unwrap().to_string(),
dessalines commented 2020-07-29 16:32:27 +00:00
Owner
Copy Link

Might be good to add a TODO here also.

Might be good to add a TODO here also.
dessalines reviewed 2020-07-29 16:33:47 +00:00
server/src/apub/inbox/activities/create.rs
@ -0,0 +42,4 @@
dbg!(create.object().as_single_kind_str());
match create.object().as_single_kind_str() {
Some("Page") => receive_create_post(create, client, pool, chat_server).await,
Some("Note") => receive_create_comment(create, client, pool, chat_server).await,
dessalines commented 2020-07-29 16:33:47 +00:00
Owner
Copy Link

This looks good, much better organized.

This looks good, much better organized.
dessalines reviewed 2020-07-29 16:42:00 +00:00
server/src/apub/activities.rs
@ -69,2 +71,4 @@
Ok(())
}
pub(in crate::apub) fn generate_activity_id<T>(kind: T) -> Result<Url, ParseError>
dessalines commented 2020-07-29 16:42:00 +00:00
Owner
Copy Link

I still need to see the full objects to make sense of this, but if I'm getting this right, in the example of liking a federated comment:

{
  ap_id: /domain_A/activities/note/like
  object: {
    ap_id: /domain_B/note/1
  }
}

To me I don't see why the domain of the activity needs to match the domain of the object. I'm gonna bring asonix into this.

I still need to see the full objects to make sense of this, but if I'm getting this right, in the example of liking a federated comment: ``` { ap_id: /domain_A/activities/note/like object: { ap_id: /domain_B/note/1 } } ``` To me I don't see why the domain of the activity needs to match the domain of the object. I'm gonna bring asonix into this.
nutomic merged commit f5211aab2a into main 2020-07-29 17:13:16 +00:00
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#72
No description provided.
Delete Branch "inbox-refactoring"

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?