From 5059d4d7e1fc98c5a86ebe73997e21b8c9bda0f7 Mon Sep 17 00:00:00 2001 From: Kagami Sascha Rosylight Date: Sun, 9 Jul 2023 01:59:44 +0200 Subject: [PATCH] refactor(backend): skip fetching notes when the data is same-origin (#11200) * refactor(backend): skip fetching notes when the data is same-origin * Update CHANGELOG.md * sentFrom --- CHANGELOG.md | 1 + .../core/activitypub/models/ApNoteService.ts | 10 +- .../activitypub/models/ApPersonService.ts | 11 +- packages/backend/test/misc/mock-resolver.ts | 19 ++- packages/backend/test/unit/activitypub.ts | 113 ++++++++++++++++-- 5 files changed, 131 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35162f6b0..c28d0b9bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ - nsfwjs のモデルロードを排他することで、重複ロードによってメモリ使用量が増加しないように - 連合の配送ジョブのパフォーマンスを向上（ロック機構の見直し、Redisキャッシュの活用） - 全体的なDBクエリのパフォーマンスを向上 +- featuredノートのsignedGet回数を減らしました ## 13.13.2 diff --git a/packages/backend/src/core/activitypub/models/ApNoteService.ts b/packages/backend/src/core/activitypub/models/ApNoteService.ts index 35865a819..d3359ef90 100644 --- a/packages/backend/src/core/activitypub/models/ApNoteService.ts +++ b/packages/backend/src/core/activitypub/models/ApNoteService.ts @@ -177,7 +177,7 @@ export class ApNoteService { // リプライ const reply: Note | null = note.inReplyTo - ? await this.resolveNote(note.inReplyTo, resolver) + ? await this.resolveNote(note.inReplyTo, { resolver }) .then(x => { if (x == null) { this.logger.warn('Specified inReplyTo, but not found'); @@ -293,9 +293,8 @@ export class ApNoteService { * リモートサーバーからフェッチしてMisskeyに登録しそれを返します。 */ @bindThis - public async resolveNote(value: string | IObject, resolver?: Resolver): Promise { - const uri = typeof value === 'string' ? value : value.id; - if (uri == null) throw new Error('missing uri'); + public async resolveNote(value: string | IObject, options: { sentFrom?: URL, resolver?: Resolver } = {}): Promise { + const uri = getApId(value); // ブロックしていたら中断 const meta = await this.metaService.fetch(); @@ -318,7 +317,8 @@ export class ApNoteService { // リモートサーバーからフェッチしてきて登録 // ここでuriの代わりに添付されてきたNote Objectが指定されていると、サーバーフェッチを経ずにノートが生成されるが // 添付されてきたNote Objectは偽装されている可能性があるため、常にuriを指定してサーバーフェッチを行う。 - return await this.createNote(uri, resolver, true); + const createFrom = options.sentFrom?.origin === new URL(uri).origin ? value : uri; + return await this.createNote(createFrom, options.resolver, true); } finally { unlock(); } diff --git a/packages/backend/src/core/activitypub/models/ApPersonService.ts b/packages/backend/src/core/activitypub/models/ApPersonService.ts index e8b65b3d4..e89ee4632 100644 --- a/packages/backend/src/core/activitypub/models/ApPersonService.ts +++ b/packages/backend/src/core/activitypub/models/ApPersonService.ts @@ -260,7 +260,7 @@ export class ApPersonService implements OnModuleInit { // Create user let user: RemoteUser | null = null; try { - // Start transaction + // Start transaction await this.db.transaction(async transactionalEntityManager => { user = await transactionalEntityManager.save(new User({ id: this.idService.genId(), @@ -306,9 +306,9 @@ export class ApPersonService implements OnModuleInit { } }); } catch (e) { - // duplicate key error + // duplicate key error if (isDuplicateKeyValueError(e)) { - // /users/@a => /users/:id のように入力がaliasなときにエラーになることがあるのを対応 + // /users/@a => /users/:id のように入力がaliasなときにエラーになることがあるのを対応 const u = await this.usersRepository.findOneBy({ uri: person.id }); if (u == null) throw new Error('already registered'); @@ -604,7 +604,10 @@ export class ApPersonService implements OnModuleInit { const featuredNotes = await Promise.all(items .filter(item => getApType(item) === 'Note') // TODO: Noteでなくてもいいかも .slice(0, 5) - .map(item => limit(() => this.apNoteService.resolveNote(item, _resolver)))); + .map(item => limit(() => this.apNoteService.resolveNote(item, { + resolver: _resolver, + sentFrom: new URL(user.uri), + })))); await this.db.transaction(async transactionalEntityManager => { await transactionalEntityManager.delete(UserNotePining, { userId: user.id }); diff --git a/packages/backend/test/misc/mock-resolver.ts b/packages/backend/test/misc/mock-resolver.ts index a7bcd859a..9dbe77a7c 100644 --- a/packages/backend/test/misc/mock-resolver.ts +++ b/packages/backend/test/misc/mock-resolver.ts @@ -18,7 +18,8 @@ type MockResponse = { }; export class MockResolver extends Resolver { - private _rs = new Map(); + #responseMap = new Map(); + #remoteGetTrials: string[] = []; constructor(loggerService: LoggerService) { super( @@ -38,18 +39,28 @@ export class MockResolver extends Resolver { ); } - public async _register(uri: string, content: string | Record, type = 'application/activity+json') { - this._rs.set(uri, { + public register(uri: string, content: string | Record, type = 'application/activity+json'): void { + this.#responseMap.set(uri, { type, content: typeof content === 'string' ? content : JSON.stringify(content), }); } + public clear(): void { + this.#responseMap.clear(); + this.#remoteGetTrials.length = 0; + } + + public remoteGetTrials(): string[] { + return this.#remoteGetTrials; + } + @bindThis public async resolve(value: string | IObject): Promise { if (typeof value !== 'string') return value; - const r = this._rs.get(value); + this.#remoteGetTrials.push(value); + const r = this.#responseMap.get(value); if (!r) { throw new Error('Not registed for mock'); diff --git a/packages/backend/test/unit/activitypub.ts b/packages/backend/test/unit/activitypub.ts index 7cd740a2f..02b900da9 100644 --- a/packages/backend/test/unit/activitypub.ts +++ b/packages/backend/test/unit/activitypub.ts @@ -11,16 +11,19 @@ import { GlobalModule } from '@/GlobalModule.js'; import { CoreModule } from '@/core/CoreModule.js'; import { FederatedInstanceService } from '@/core/FederatedInstanceService.js'; import { LoggerService } from '@/core/LoggerService.js'; -import type { IActor } from '@/core/activitypub/type.js'; +import type { IActor, ICollection, IPost } from '@/core/activitypub/type.js'; import { Note } from '@/models/index.js'; import { secureRndstr } from '@/misc/secure-rndstr.js'; import { MockResolver } from '../misc/mock-resolver.js'; const host = 'https://host1.test'; -function createRandomActor(): IActor & { id: string } { +type NonTransientIActor = IActor & { id: string }; +type NonTransientIPost = IPost & { id: string }; + +function createRandomActor({ actorHost = host } = {}): NonTransientIActor { const preferredUsername = secureRndstr(8); - const actorId = `${host}/users/${preferredUsername.toLowerCase()}`; + const actorId = `${actorHost}/users/${preferredUsername.toLowerCase()}`; return { '@context': 'https://www.w3.org/ns/activitystreams', @@ -32,13 +35,41 @@ function createRandomActor(): IActor & { id: string } { }; } +function createRandomNote(actor: NonTransientIActor): NonTransientIPost { + const id = secureRndstr(8); + const noteId = `${new URL(actor.id).origin}/notes/${id}`; + + return { + id: noteId, + type: 'Note', + attributedTo: actor.id, + content: 'test test foo', + }; +} + +function createRandomNotes(actor: NonTransientIActor, length: number): NonTransientIPost[] { + return new Array(length).fill(null).map(() => createRandomNote(actor)); +} + +function createRandomFeaturedCollection(actor: NonTransientIActor, length: number): ICollection { + const items = createRandomNotes(actor, length); + + return { + '@context': 'https://www.w3.org/ns/activitystreams', + type: 'Collection', + id: actor.outbox as string, + totalItems: items.length, + items, + }; +} + describe('ActivityPub', () => { let noteService: ApNoteService; let personService: ApPersonService; let rendererService: ApRendererService; let resolver: MockResolver; - beforeEach(async () => { + beforeAll(async () => { const app = await Test.createTestingModule({ imports: [GlobalModule, CoreModule], }).compile(); @@ -53,7 +84,11 @@ describe('ActivityPub', () => { // Prevent ApPersonService from fetching instance, as it causes Jest import-after-test error const federatedInstanceService = app.get(FederatedInstanceService); - jest.spyOn(federatedInstanceService, 'fetch').mockImplementation(() => new Promise(() => {})); + jest.spyOn(federatedInstanceService, 'fetch').mockImplementation(() => new Promise(() => { })); + }); + + beforeEach(() => { + resolver.clear(); }); describe('Parse minimum object', () => { @@ -69,7 +104,7 @@ describe('ActivityPub', () => { }; test('Minimum Actor', async () => { - resolver._register(actor.id, actor); + resolver.register(actor.id, actor); const user = await personService.createPerson(actor.id, resolver); @@ -79,8 +114,8 @@ describe('ActivityPub', () => { }); test('Minimum Note', async () => { - resolver._register(actor.id, actor); - resolver._register(post.id, post); + resolver.register(actor.id, actor); + resolver.register(post.id, post); const note = await noteService.createNote(post.id, resolver, true); @@ -97,7 +132,7 @@ describe('ActivityPub', () => { name: secureRndstr(129), }; - resolver._register(actor.id, actor); + resolver.register(actor.id, actor); const user = await personService.createPerson(actor.id, resolver); @@ -110,7 +145,7 @@ describe('ActivityPub', () => { name: '', }; - resolver._register(actor.id, actor); + resolver.register(actor.id, actor); const user = await personService.createPerson(actor.id, resolver); @@ -126,4 +161,62 @@ describe('ActivityPub', () => { } as Note); }); }); + + describe('Featured', () => { + test('Fetch featured notes from IActor', async () => { + const actor = createRandomActor(); + actor.featured = `${actor.id}/collections/featured`; + + const featured = createRandomFeaturedCollection(actor, 5); + + resolver.register(actor.id, actor); + resolver.register(actor.featured, featured); + + await personService.createPerson(actor.id, resolver); + + // All notes in `featured` are same-origin, no need to fetch notes again + assert.deepStrictEqual(resolver.remoteGetTrials(), [actor.id, actor.featured]); + + // Created notes without resolving anything + for (const item of featured.items as IPost[]) { + const note = await noteService.fetchNote(item); + assert.ok(note); + assert.strictEqual(note.text, 'test test foo'); + assert.strictEqual(note.uri, item.id); + } + }); + + test('Fetch featured notes from IActor pointing to another remote server', async () => { + const actor1 = createRandomActor(); + actor1.featured = `${actor1.id}/collections/featured`; + const actor2 = createRandomActor({ actorHost: 'https://host2.test' }); + + const actor2Note = createRandomNote(actor2); + const featured = createRandomFeaturedCollection(actor1, 0); + (featured.items as IPost[]).push({ + ...actor2Note, + content: 'test test bar', // fraud! + }); + + resolver.register(actor1.id, actor1); + resolver.register(actor1.featured, featured); + resolver.register(actor2.id, actor2); + resolver.register(actor2Note.id, actor2Note); + + await personService.createPerson(actor1.id, resolver); + + // actor2Note is from a different server and needs to be fetched again + assert.deepStrictEqual( + resolver.remoteGetTrials(), + [actor1.id, actor1.featured, actor2Note.id, actor2.id], + ); + + const note = await noteService.fetchNote(actor2Note.id); + assert.ok(note); + + // Reflects the original content instead of the fraud + assert.strictEqual(note.text, 'test test foo'); + assert.strictEqual(note.uri, actor2Note.id); + }); + }); });