activitypub: process Move activities and movedTo field on actors #309
No reviewers
amybones
Labels
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Blocks
#311 option to automatically follow new actor after move
FoundKeyGang/FoundKey
Reference: FoundKeyGang/FoundKey#309
Loading…
Reference in New Issue
No description provided.
Delete Branch "account-moving"
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?
This processes incoming
Moveactivities andmovedTofield on actors. The respective moved to actor is checked for thealsoKnownAsfield, although this is not currently saved in the database.When a
Moveactivity is received, notifications are generated for followers of the moved-from actor. Those notifications will specify which account the actor has moved to and display a follow button for that new actor.closes #88
2ab12dfebetod1b768bd3ad1b768bd3ato3b9a88f4913b9a88f491to9fd2764bbd9fd2764bbdto27ddb2496cI'm curious what's blocking merging this? With the big mastodon.lol shutdown foundkey not processing these is kinda screwing up my experience 😔
See the contribution guide on contributors with push access.
Nobody has reviewed this pull request yet and I feel unable to review my own code.
I've been getting familiar with the code and would be open to doing a code review for you if that would help get this merged. I'm obviously not a contributor, but I pinky promise I've got plenty of experience doing code reviews and that I'll do a real review and not just rubber stamp it.
Overall this looks good. There's only a couple things that stands out as a possible bugs to me.
Otherwise it's just questions about behavior. I didn't do anything like applying the PR to my local foundkey instance and then attempt to generate some account movements so I can't say definitively that this actually works on a separate deployment.
The only other overall thing, and this is just me confirming that I understand the order of things, is just to confirm that request signatures are being verified to confirm the moves are actually valid, right? I think they are handled in the inbox queue processor, but I'm not a 100% confident.
I'm requesting changes for the possible bug, but it's a soft request change I'm just looking for your view point.
@ -0,0 +11,4 @@if (activity.object == null || getApId(activity.object) !== actor.uri) return;// actor already movedif (actor.movedTo != null) return;This would prevent cycles of
a -> b -> a, right? It's uncommon but I've seen people do cycles like this in the past when, e.g. they move and then the instance they move to winds up blocking an instance they have a lot of connections with, or in the case of mastodon.lol their new instance shuts down.This would stop
a -> b, a -> cwhich also comes from previous discussion in the Misskey issue. Ifareally wanted to do that, they would have to make sure that the instance updates their data in between. For example by sending anUpdateactivity where the object is the actor themself.Got it. Thanks for clarifying the data model, I see now what's happening there :) As I said, I'm still getting familiar with the code base.
@ -0,0 +14,4 @@if (actor.movedTo != null) return;// no move targetif (activity.target == null) return;Do we expect this to be a common case? If no, it might be worth adding a warning log so that some data about how often this happens can be collected. The log can always be removed if it appears to be frequent. I bring this up since in my deployment of foundkey I've added some warning logs to a place to try and help me track down (which I'm not done doing so no PRs here) what I think are some bugs.
My expectation in general, and it's tooootally fine if this isn't how foundkey approaches these things, is to add warnings or errors for conditions that are foreseeable but it's unclear if they happen in practice. That way there's an ability to collect some data to inform what to do with the code in the future.
I don't expect this to be a common case, it's just a sanity check. If the
targetattribute of aMoveactivity that is intended to be used for account moves is not included, the activity does not make sense. Or maybe this is aMoveactivity that has different semantics in which case it does also not make sense to process it in this handler. I also don't think it would be necessary to log a malformed Activity.@ -0,0 +51,4 @@movedToId: movedTo.id,}),...followings.map(async (following) => {// TODO: autoAcceptMove?The only reason I can think of to not auto accept is if the local user has follow requests turned on, right? But even then, if they already accepted the original request, then it feels silly to make them accept again. If it's technically difficult to do here for some reason, in which case it seems fine to leave this as a todo.
What you are talking about is the new/moved actor following someone. But this comment is about the other direction.
There is already a separate issue for this, see #311. Due to concerns raised in the issue when this was discussed for Misskey, we decided that there should be a separate option for this.
@ -64,0 +76,4 @@// Can't find the move target for some reason.// Don't treat the actor as invalid, just ignore the movedTo.logger.warn(`cannot find move target "${x.movedTo}", error: ${err.toString()}`);delete x.movedTo;I understand having a warning in the log, but I'm not totally convinced that deleting the movedTo information makes sense to me?
Things that I think of: It could be that the destination isn't resolvable because it's a fedi server that possibly somehow conforms to the account moving spec, but not the lookup part, which doesn't make a ton of sense to me. Or, the destination is temporarily unreachable at the time an attempt to resolve the new account is made.
For the first case, it's unlikely that the new account is going to properly broadcast updates over activitypub, so sure. But for the second case, this means that the foundkey database is going to be inconsistent. The user is going to lose that follow silently, as well as have no indication that there's something amiss, since if they go look at the profile page for the old follow wondering where they went, it'll just show up normally, and they won't have a chance to go find the person or manually re-follow.
I agree with you for the 2nd case, it should be an atomic operation.
The correct behaviour is to throw an error there. Since there is no try/catch statement around
resolvePersonin the move handler this would cause the queue handler to retry this incoming activity at a later time.Aha. Okay, I missed that bubbling up to a retry, thanks!
@ -86,6 +87,14 @@<button class="_textButton" @click="rejectGroupInvitation()">{{ i18n.ts.reject }}</button></div></span><span v-if="notification.type === 'move'" class="text" style="opacity: 0.6;">Something that might be worth refactoring in this PR but is otherwise fine: The CSS class
textis used several times with a local override ofopacity: 0.6it might be worth factoring out a dedicated CSS class for this likedim-notification-textor even just a class that only applies the opacity. IIRC this will help a bit with rendering performance.Good idea in general, but this has nothing to do with
Moveactivities so it should be done outside of this PR.Per discussion in preview review everything looks good to me, for whatever that's worth :)
Thanks for your review!
re: the order of things - I have updated the READING.md file with a bit more details about how incoming (and outgoing) activities are being processed and where to look. Hopefully that helps.
tl;dr about your specific question: signatures are verified in the handler for the inbox queue, specifically starting from this line
Aha yeah, I was 99% sure that was verifying things, but again, still familiarizing myself with the code base.