Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Move activity for user migration #2970

Merged
merged 13 commits into from
Nov 2, 2023
Merged

Conversation

hughrun
Copy link
Contributor

@hughrun hughrun commented Aug 29, 2023

This is both a stand-alone PR and works in partnership with #2868 - migrating user data from one server to another is Step 1, but we then need a way to tell Fediverse servers that the user has moved: that's what this PR does.

Additionally, this will cover a gap where we're currently ignoring incoming Move notifications, including when e.g. a Mastodon user migrates from one server to another. This means those users effectively become orphaned from BookWyrm because we don't follow them to the new account.

This largely follows the precedent set by Mastodon, since there is no ActivityPub standard for account migration, however there are a few differences.

Features:

  • Sends notification to local followers when a user Move action is received from any ActivityPub server;
  • Sends Move activity to followers when a user moves from one AP account to another;
  • Requires the target account to have the source account listed in alsoKnownAs/also_known_as aka an alias;
  • adds movedTo/moved_to value to the source account
  • redirects source account webfinger to the target account
  • Adds prominent notice to source user profile page
  • stops source user acting other than within preferences section
  • move can be undone at any time

Following behaviour:

Unlike Mastodon I've set this up so that followers are notified of the move but there is no action taken regarding following unless initiated by the follower (Mastodon auto-unfollows the old user and auto-follows the new user when receiving a Move activity). Notifications have a button to follow the new account, but the user remains following the old account.

This makes moves less high-stakes and provides more autonomy to the follower. It also spreads out load on servers when a popular user moves their account as follows/unfollows will happen at the speed other users see and act on notifications.

We could change this so that the button in the notification unfollows the old user as well - happy to take advice on that.

Context:

Copy link
Member

@mouse-reeve mouse-reeve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a quick pass because I haven't completely forgotten about you despite having Quite A Week Or Two right now! Let me know if anything is helpful? I'm not sure I totally answered what you were asking in some places

bookwyrm/activitypub/verbs.py Outdated Show resolved Hide resolved
bookwyrm/models/move.py Outdated Show resolved Hide resolved
bookwyrm/models/move.py Outdated Show resolved Hide resolved
bookwyrm/models/move.py Outdated Show resolved Hide resolved
bookwyrm/templates/notifications/items/move.html Outdated Show resolved Hide resolved
* update User model to allow for moved_to and also_known_as values
* allow users to add aliases (also_known_as) in UI
* allow users to move account to another one (moved_to)
* redirect webfinger to the new account after a move
* present notification to followers inviting to follow at new account

Note: unlike Mastodon we're not running any unfollow/autofollow action here: users can decide for themselves
This makes undoing moves easier.

TODO

There is still a bug with incoming Moves, at least from Mastodon.
This seems to be something to do with Update activities (rather than Move, strictly).
@hughrun hughrun self-assigned this Sep 18, 2023
@hughrun

This comment was marked as resolved.

- minor template fixes
- notification logic fixes
- don't dedupe on moved_to or also_known_as
- add migration
@hughrun hughrun removed the request for review from mouse-reeve September 25, 2023 09:25
@hughrun hughrun changed the title WIP: initial work to add 'Move' activity Add Move activity for user migration Sep 25, 2023
@hughrun hughrun marked this pull request as ready for review September 25, 2023 22:34
@hughrun hughrun removed their assignment Sep 25, 2023
@jaschaurbach
Copy link
Member

This one is great. @mouse-reeve I reviewed it and I would merge this. As this is a big one please do a review as well. Thanks!

@hughrun hughrun mentioned this pull request Oct 22, 2023
9 tasks
Copy link
Member

@mouse-reeve mouse-reeve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works really smoothly! My main question is about the additional notification model. I know that I've been the blocker on this PR, so I'm happy to consider my translation items non-blockers on merging this, and I can change them this weekend.

The bigger question (which again, I don't want to make a blocker), is how functional a moved account should be. I mentioned showing other parts of the UI in the comments, but I believe a moved account will also still receive updates to its activitystreams, and otherwise behave like a fully operational account. I suspect that's the correct behavior, because I think it is supposed to be possible to, for example, follow a moved account, and I think it would be challenging to re-build the activitystream if the user decided to un-move. However, it seems odd that a moved account can post statuses.

bookwyrm/models/move.py Outdated Show resolved Hide resolved
bookwyrm/templates/feed/layout.html Outdated Show resolved Hide resolved
bookwyrm/templates/feed/layout.html Outdated Show resolved Hide resolved
bookwyrm/templates/notifications/items/move_user.html Outdated Show resolved Hide resolved
bookwyrm/templates/notifications/items/move_user.html Outdated Show resolved Hide resolved
bookwyrm/templates/preferences/move_user.html Outdated Show resolved Hide resolved
@hughrun
Copy link
Contributor Author

hughrun commented Oct 26, 2023

However, it seems odd that a moved account can post statuses.

I definitely didn't mean to leave that ability in, oops.

Basically I agree. See comment on your review - I really just wasn't sure what to leave available and what to block, and if I'm honest it seemed a bit complicated and I took an easy route.

I agree: restoring accounts should be reasonably painless but other than that moved accounts should be essentially non-functional. I'll take a look at this and adjust the UI changes - it may make sense to just create a new template and redirect to that.

- make Move notifications less complicated
- moved users cannot do anything other than unmove or log out
- refactor translations for moved users
@hughrun
Copy link
Contributor Author

hughrun commented Oct 27, 2023

With the changes I've made post-review, it should be that the only thing a moved user can now do when they are logged in is either log out, or cancel the move. I also took the opportunity to make the UI a little nicer for such users.

Copy link
Member

@mouse-reeve mouse-reeve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't for the LIFE of me figure out how to create a PR that merges into this PR and I don't want to keep making you make stupidly tiny changes to the blocktrans tag, so I'm going to merge this into a new branch and then commit my changes and then merge THAT branch in, which seems like a ridiculous way to do it but at least it will be merged

<p>
{% id_to_username request.user.moved_to as username %}
{% blocktrans %}
<strong>You have moved your account</strong> to <a href="{{user.moved_to}}">{{ username }}</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link doesn't resolve -- annoyingly, you can't access key value pairs in a blocktrans tag, so user.moved_to is undefined. You have to define the variable in the opening tag, like {% blocktrans trimmed with moved_to_url=user.moved_to %}. trimmed is also necessary or all the whitespace will be preserved in the translation file.

@mouse-reeve mouse-reeve changed the base branch from main to move November 2, 2023 00:13
@mouse-reeve mouse-reeve merged commit 621cfa7 into bookwyrm-social:move Nov 2, 2023
10 checks passed
@hughrun hughrun deleted the move branch November 5, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants