From 2a0efffa3e5bfc1586b3a38b4fab61b3ad70686b Mon Sep 17 00:00:00 2001 From: Namekuji Date: Thu, 20 Apr 2023 19:16:02 -0400 Subject: [PATCH] prevent moving if the destination account has already moved --- .../backend/src/core/AccountMoveService.ts | 4 +- .../src/core/activitypub/ApInboxService.ts | 3 ++ .../src/server/api/endpoints/i/move.ts | 4 +- packages/backend/test/e2e/move.ts | 48 ++++++++++++------- 4 files changed, 37 insertions(+), 22 deletions(-) diff --git a/packages/backend/src/core/AccountMoveService.ts b/packages/backend/src/core/AccountMoveService.ts index 4db0f928e5..8be7b04b67 100644 --- a/packages/backend/src/core/AccountMoveService.ts +++ b/packages/backend/src/core/AccountMoveService.ts @@ -78,7 +78,7 @@ export class AccountMoveService { // add movedToUri to indicate that the user has moved const update = {} as Partial; - update.alsoKnownAs = src.alsoKnownAs?.concat([dstUri]) ?? [dstUri]; + update.alsoKnownAs = src.alsoKnownAs?.includes(dstUri) ? src.alsoKnownAs : src.alsoKnownAs?.concat([dstUri]) ?? [dstUri]; update.movedToUri = dstUri; await this.usersRepository.update(src.id, update); src = Object.assign(src, update); @@ -226,7 +226,7 @@ export class AccountMoveService { } while (newJoinings.has(id)); return id; }; - for (const joining of oldJoinings) { + for (const joining of oldJoinings) { newJoinings.set(genId(), { createdAt: new Date(), userId: dst.id, diff --git a/packages/backend/src/core/activitypub/ApInboxService.ts b/packages/backend/src/core/activitypub/ApInboxService.ts index 9df6ffad7c..2da290bfea 100644 --- a/packages/backend/src/core/activitypub/ApInboxService.ts +++ b/packages/backend/src/core/activitypub/ApInboxService.ts @@ -760,6 +760,9 @@ export class ApInboxService { } else if (!newAccount.alsoKnownAs?.includes(this.accountMoveService.getUserUri(oldAccount))) { isValidMove = false; } + if (newAccount.movedToUri) { + isValidMove = false; + } if (!isValidMove) { return 'skip: destination account invalid'; } diff --git a/packages/backend/src/server/api/endpoints/i/move.ts b/packages/backend/src/server/api/endpoints/i/move.ts index c6427a6246..256fca2cf0 100644 --- a/packages/backend/src/server/api/endpoints/i/move.ts +++ b/packages/backend/src/server/api/endpoints/i/move.ts @@ -28,7 +28,7 @@ export const meta = { errors: { destinationAccountForbids: { message: - 'Destination account doesn\'t have proper \'Known As\' alias. Did you remember to set it?', + 'Destination account doesn\'t have proper \'Known As\' alias, or has already moved.', code: 'REMOTE_ACCOUNT_FORBIDS', id: 'b5c90186-4ab0-49c8-9bba-a1f766282ba4', }, @@ -125,7 +125,7 @@ export default class extends Endpoint { } // abort if unintended - if (!allowed) throw new ApiError(meta.errors.destinationAccountForbids); + if (!allowed || moveTo.movedToUri) throw new ApiError(meta.errors.destinationAccountForbids); return await this.accountMoveService.moveFromLocal(me, moveTo); }); diff --git a/packages/backend/test/e2e/move.ts b/packages/backend/test/e2e/move.ts index ee478dea0c..f063819475 100644 --- a/packages/backend/test/e2e/move.ts +++ b/packages/backend/test/e2e/move.ts @@ -1,13 +1,12 @@ process.env.NODE_ENV = 'test'; import * as assert from 'assert'; -import { signup, startServer, initTestDb, api, sleep } from '../utils.js'; -import type { INestApplicationContext } from '@nestjs/common'; -import { loadConfig } from '@/config.js'; -import { Blocking, BlockingsRepository, Following, FollowingsRepository, Muting, MutingsRepository, User, UsersRepository } from '@/models/index.js'; -import { jobQueue } from '@/boot/common.js'; import rndstr from 'rndstr'; -import { uploadFile } from '../utils.js'; +import { loadConfig } from '@/config.js'; +import { User, UsersRepository } from '@/models/index.js'; +import { jobQueue } from '@/boot/common.js'; +import { uploadFile, signup, startServer, initTestDb, api, sleep } from '../utils.js'; +import type { INestApplicationContext } from '@nestjs/common'; describe('Account Move', () => { let app: INestApplicationContext; @@ -22,9 +21,6 @@ describe('Account Move', () => { let frank: any; let Users: UsersRepository; - let Followings: FollowingsRepository; - let Blockings: BlockingsRepository; - let Mutings: MutingsRepository; beforeAll(async () => { app = await startServer(); @@ -40,9 +36,6 @@ describe('Account Move', () => { eve = await signup({ username: 'eve' }); frank = await signup({ username: 'frank' }); Users = connection.getRepository(User); - Followings = connection.getRepository(Following); - Blockings = connection.getRepository(Blocking); - Mutings = connection.getRepository(Muting); }, 1000 * 60 * 2); afterAll(async () => { @@ -66,7 +59,7 @@ describe('Account Move', () => { test('Able to set remote user (but may fail)', async () => { const res = await api('/i/known-as', { - alsoKnownAs: `@syuilo@example.com`, + alsoKnownAs: '@syuilo@example.com', }, bob); assert.strictEqual(res.status, 400); @@ -123,7 +116,7 @@ describe('Account Move', () => { test('Unable to create an alias without the second @', async () => { const res1 = await api('/i/known-as', { - alsoKnownAs: '@alice' + alsoKnownAs: '@alice', }, bob); assert.strictEqual(res1.status, 400); @@ -131,7 +124,7 @@ describe('Account Move', () => { assert.strictEqual(res1.body.error.id, 'fcd2eef9-a9b2-4c4f-8624-038099e90aa5'); const res2 = await api('/i/known-as', { - alsoKnownAs: 'alice' + alsoKnownAs: 'alice', }, bob); assert.strictEqual(res2.status, 400); @@ -209,7 +202,7 @@ describe('Account Move', () => { test('Prohibit the root account from moving', async () => { const res = await api('/i/move', { - moveToAccount: `@bob@${url.hostname}` + moveToAccount: `@bob@${url.hostname}`, }, root); assert.strictEqual(res.status, 400); @@ -223,8 +216,8 @@ describe('Account Move', () => { }, alice); assert.strictEqual(res.status, 400); - assert.strictEqual(res.body.error.code, 'NO_SUCH_MOVE_TARGET'); - assert.strictEqual(res.body.error.id, 'b5c90186-4ab0-49c8-9bba-a1f76c202ba4'); + assert.strictEqual(res.body.error.code, 'NO_SUCH_USER'); + assert.strictEqual(res.body.error.id, 'fcd2eef9-a9b2-4c4f-8624-038099e90aa5'); }); test('Unable to move if alsoKnownAs is invalid', async () => { @@ -272,6 +265,25 @@ describe('Account Move', () => { assert.ok(lists.body[0].userIds.find((id: string) => id === alice.id)); }); + test('Unable to move if the destination account has already moved.', async () => { + await api('/i/move', { + moveToAccount: `@bob@${url.hostname}`, + }, alice); + + const newAlice = await Users.findOneByOrFail({ id: alice.id }); + assert.strictEqual(newAlice.movedToUri, `${url.origin}/users/${bob.id}`); + assert.strictEqual(newAlice.alsoKnownAs?.length, 1); + assert.strictEqual(newAlice.alsoKnownAs[0], `${url.origin}/users/${bob.id}`); + + const res = await api('/i/move', { + moveToAccount: `@alice@${url.hostname}`, + }, bob); + + assert.strictEqual(res.status, 400); + assert.strictEqual(res.body.error.code, 'REMOTE_ACCOUNT_FORBIDS'); + assert.strictEqual(res.body.error.id, 'b5c90186-4ab0-49c8-9bba-a1f766282ba4'); + }); + test('Follow and follower counts are properly adjusted', async () => { await api('/following/create', { userId: alice.id,