bookwyrm
bookwyrm copied to clipboard
Add signatures to requests to mastodon to support authorized fetch mode
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.
@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 that would be great! thank you
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?
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?
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?
I might have a fix for this. Just testing some things, more info soon.
@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?
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
Closed in favor of #2613