activitypub: process Move activities and movedTo field on actors #309

Manually merged
Johann150 merged 10 commits from account-moving into main 2023-03-23 20:48:19 +00:00
4 changed files with 84 additions and 2 deletions
Showing only changes of commit bad9eba75a - Show all commits

View file

@ -1,4 +1,4 @@
import { Entity, Column, Index, OneToOne, JoinColumn, PrimaryColumn } from 'typeorm';
import { Entity, Column, Index, OneToOne, ManyToOne, JoinColumn, PrimaryColumn } from 'typeorm';
import { id } from '../id.js';
import { DriveFile } from './drive-file.js';
@ -230,6 +230,18 @@ export class User {
})
public federateBlocks: boolean;
@Column({
...id(),
nullable: true,
})
public movedToId: User['id'] | null;
@ManyToOne(() => User, {
onDelete: 'SET NULL'
})
@JoinColumn()
public movedTo: User | null;
constructor(data: Partial<User>) {
if (data == null) return;

View file

@ -4,7 +4,7 @@ import { Resolver } from '@/remote/activitypub/resolver.js';
import { extractDbHost } from '@/misc/convert-host.js';
import { shouldBlockInstance } from '@/misc/should-block-instance.js';
import { apLogger } from '../logger.js';
import { IObject, isCreate, isDelete, isUpdate, isRead, isFollow, isAccept, isReject, isAdd, isRemove, isAnnounce, isLike, isUndo, isBlock, isCollectionOrOrderedCollection, isCollection, isFlag, getApId } from '../type.js';
import { IObject, isCreate, isDelete, isUpdate, isRead, isFollow, isAccept, isReject, isAdd, isRemove, isAnnounce, isLike, isUndo, isBlock, isCollectionOrOrderedCollection, isCollection, isFlag, isMove, getApId } from '../type.js';
import create from './create/index.js';
import performDeleteActivity from './delete/index.js';
import performUpdateActivity from './update/index.js';
@ -19,6 +19,7 @@ import add from './add/index.js';
import remove from './remove/index.js';
import block from './block/index.js';
import flag from './flag/index.js';
import { move } from './move/index.js';
export async function performActivity(actor: CacheableRemoteUser, activity: IObject, resolver: Resolver): Promise<void> {
if (isCollectionOrOrderedCollection(activity)) {
@ -73,6 +74,8 @@ async function performOneActivity(actor: CacheableRemoteUser, activity: IObject,
await block(actor, activity);
} else if (isFlag(activity)) {
await flag(actor, activity);
} else if (isMove(activity)) {
await move(actor, activity, resolver);
} else {
apLogger.warn(`unrecognized activity type: ${(activity as any).type}`);
}

View file

@ -0,0 +1,62 @@
import { IsNull } from 'typeorm';
import { CacheableRemoteUser } from '@/models/entities/user.js';
import { resolvePerson } from '@/remote/activitypub/models/person.js';
import { Followings, Users } from '@/models/index.js';
import { createNotification } from '@/services/create-notification.js';
import Resolver from '../../resolver.js';
import { IMove, isActor, getApId } from '../../type.js';
export async function move(actor: CacheableRemoteUser, activity: IMove, resolver: Resolver): Promise<void> {
// actor is not move origin
if (activity.object == null || getApId(activity.object) !== actor.uri) return;
// actor already moved
if (actor.movedTo != null) return;
Johann150 marked this conversation as resolved
Review

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 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.
Review

This would stop a -> b, a -> c which also comes from previous discussion in the Misskey issue. If a really wanted to do that, they would have to make sure that the instance updates their data in between. For example by sending an Update activity where the object is the actor themself.

This would stop `a -> b, a -> c` which also comes from previous discussion in the Misskey issue. If `a` really wanted to do that, they would have to make sure that the instance updates their data in between. For example by sending an `Update` activity where the object is the actor themself.
Review

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.

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.
// no move target
if (activity.target == null) return;
Review

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.

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.
Review

I don't expect this to be a common case, it's just a sanity check. If the target attribute of a Move activity that is intended to be used for account moves is not included, the activity does not make sense. Or maybe this is a Move activity 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.

I don't expect this to be a common case, it's just a sanity check. If the `target` attribute of a `Move` activity that is intended to be used for account moves is not included, the activity does not make sense. Or maybe this is a `Move` activity 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.
/* the database resolver can not be used here, because:
* 1. It must be ensured that the latest data is used.
* 2. The AP representation is needed, because `alsoKnownAs`
* is not stored in the database.
* This also checks whether the move target is blocked
*/
const movedToAp = await resolver.resolve(getApId(activity.target));
// move target is not an actor
if (!isActor(movedToAp)) return;
// move destination has not accepted
if (!Array.isArray(movedToAp.alsoKnownAs) || !movedToAp.alsoKnownAs.includes(actor.id)) return;
// ensure the user exists
const movedTo = await resolvePerson(getApId(activity.target), resolver, movedToAp);
// move target is already suspended
if (movedTo.isSuspended) return;
// process move for local followers
const followings = Followings.find({
select: {
followerId: true,
},
where: {
followeeId: actor.id,
followerHost: IsNull(),
},
});
await Promise.all([
Users.update(actor.id, {
movedToId: movedTo.id,
}),
...followings.map(async (following) => {
// TODO: autoAcceptMove?
Johann150 marked this conversation as resolved
Review

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.

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.
Review

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.

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.
await createNotification(following.followerId, 'move', {
notifierId: actor.id,
moveTargetId: movedTo.id,
});
}),
]);
}

View file

@ -296,6 +296,10 @@ export interface IFlag extends IActivity {
type: 'Flag';
}
export interface IMove extends IActivity {
type: 'Move';
}
export const isCreate = (object: IObject): object is ICreate => getApType(object) === 'Create';
export const isDelete = (object: IObject): object is IDelete => getApType(object) === 'Delete';
export const isUpdate = (object: IObject): object is IUpdate => getApType(object) === 'Update';
@ -310,6 +314,7 @@ export const isLike = (object: IObject): object is ILike => getApType(object) ==
export const isAnnounce = (object: IObject): object is IAnnounce => getApType(object) === 'Announce';
export const isBlock = (object: IObject): object is IBlock => getApType(object) === 'Block';
export const isFlag = (object: IObject): object is IFlag => getApType(object) === 'Flag';
export const isMove = (object: IObject): object is IMove => getApType(object) === 'Move';
export interface ILink {
href: string;