bookwyrm icon indicating copy to clipboard operation
bookwyrm copied to clipboard

Add signatures to requests to mastodon to support authorized fetch mode

Open renatolond opened this issue 3 years ago • 5 comments

Hi! Before anything, thanks for the work in this, it's really appreciated ;)

I installed an instance to test bookwyrm and I noticed I couldn't follow my bookwyrm user from my test mastodon instance. My test mastodon instance has Authorized Fetch turned on, and so what was happening was that when mastodon made a request towards bookwyrm to follow my user, bookwyrm would try to look up the actor and fail because mastodon would reject the api call.

This PR tries to add the required code to sign the request, however, python is not my main language and I think some refactoring could be done before this is a real PR :)

Mostly: It seems we already some of this work over at bookwyrm/broadcast.py, in the sign_and_send method, I'm wondering if this could be moved over to the activitypub base.

Another, bigger, problem is that I'm not sure there's always an actor when doing this request. Mastodon solves this by having a "instance-wide actor", but I'm not sure if this is needed in bookwyrm too, or if for now changing the calls to resolve_remote_id would be enough.

renatolond avatar Dec 29 '20 20:12 renatolond

@mouse-reeve Would you be okay if I advanced on this using an instance-wide actor? It would be something like Mastodon or Pleroma's @[email protected], and it would be used for these queries where we need to sign requests but no user required the data (to discover a profile in another instance that sent an activitypub, for instance).

renatolond avatar Jan 05 '21 19:01 renatolond

@renatolond that would be great! thank you

mouse-reeve avatar Jan 05 '21 19:01 mouse-reeve

Hey, sorry for taking so long to get back to this! I've been looking at this and I'm having some trouble to sort out how to do this. This was my idea: Just like in Mastodon, I was trying to get the instance-wide actor and if it did not exist, I was trying to create it with a fixed id number. However, this seems to present a couple of issues:

  • The save method for the user checks if the id exists or not to decide if the user was saved or not. Since I pass an id, the method assumes this user already exists and skips other checks trying to save before creating remote_id, outbox and etc. Then the actor is None and the serialization of the model fails. (This is the current state of the branch in the "WIP" commit) I then tried changing the created condition to follow what I saw as another way to check for persisted state in django (https://stackoverflow.com/a/35647389) but that seems to make the save to get into a loop (tries to save, tries to publish to activity pub, which seems to call save again, etc)
Traceback (most recent call last):
  File "/home/renatocerqueira/src/bookwyrm/bookwyrm/tests/activitypub/test_base_activity.py", line 40, in setUp
    self.user.save(broadcast=False, update_fields=["remote_id"])
  File "/home/renatocerqueira/src/bookwyrm/.virtualenv/lib/python3.10/site-packages/model_utils/tracker.py", line 239, in save
    ret = original_save(instance, *args, **kwargs)
  File "/home/renatocerqueira/src/bookwyrm/bookwyrm/models/user.py", line 343, in save
    self.save(broadcast=False, update_fields=["key_pair"])
  File "/home/renatocerqueira/src/bookwyrm/.virtualenv/lib/python3.10/site-packages/model_utils/tracker.py", line 239, in save
    ret = original_save(instance, *args, **kwargs)
  File "/home/renatocerqueira/src/bookwyrm/bookwyrm/models/user.py", line 343, in save
    self.save(broadcast=False, update_fields=["key_pair"])
  File "/home/renatocerqueira/src/bookwyrm/.virtualenv/lib/python3.10/site-packages/model_utils/tracker.py", line 239, in save
    ret = original_save(instance, *args, **kwargs)
  File "/home/renatocerqueira/src/bookwyrm/bookwyrm/models/user.py", line 343, in save
    self.save(broadcast=False, update_fields=["key_pair"])
  File "/home/renatocerqueira/src/bookwyrm/.virtualenv/lib/python3.10/site-packages/model_utils/tracker.py", line 239, in save
    ret = original_save(instance, *args, **kwargs)
    ...

So I'm not sure what would be the best way to guarantee this. Should I already save with a broadcast false in this case, maybe?

renatolond avatar Jan 05 '22 14:01 renatolond

Hm, do you need to create a user, or just a key pair? It looks like the sender is just providing a remote_id and key_pair? But I may be missing the bigger picture. Alternatively, can you save the user object and then set the id afterwards?

mouse-reeve avatar Jan 06 '22 01:01 mouse-reeve

Hmm, I think it needs to be a user because it needs to also answer to queries from other AP instances if they call back. I probably could, do you think it would work then?

renatolond avatar Jan 11 '22 11:01 renatolond

I might have a fix for this. Just testing some things, more info soon.

hughrun avatar Jan 18 '23 04:01 hughrun

@mouse-reeve Ok I think I may have gotten this working. It will definitely need more eyes on my patch to check for anything dodgy, and there's possibly still at least one test not passing, I think. Is it better (or even possible) to push changes to this existing PR, or to create a new one incorporating all @renatolond's commits plus my additions?

hughrun avatar Jan 19 '23 08:01 hughrun

I've never figured out how to push to someone else's PR (which doesn't mean it's not possible...), so I would just go with whatever type of PR you're able to open

mouse-reeve avatar Jan 19 '23 13:01 mouse-reeve

Closed in favor of #2613

renatolond avatar Jan 20 '23 09:01 renatolond