From b7dc3cca22ea26babe0d202c042fab0f0582b946 Mon Sep 17 00:00:00 2001 From: Johann150 Date: Sun, 3 Dec 2023 14:18:34 +0100 Subject: [PATCH] activitypub: send all cascade deletes The `deleteNotes` function would not correctly handle cases where cascade deleted notes were from a different user than the initially deleted note. Changelog: Fixed --- .../remote/activitypub/kernel/delete/note.ts | 2 +- .../activitypub/kernel/undo/announce.ts | 2 +- .../server/api/endpoints/notes/unrenote.ts | 2 +- packages/backend/src/services/note/delete.ts | 51 +++++++++++++------ 4 files changed, 38 insertions(+), 19 deletions(-) diff --git a/packages/backend/src/remote/activitypub/kernel/delete/note.ts b/packages/backend/src/remote/activitypub/kernel/delete/note.ts index d855f7f92..6dc39e6b8 100644 --- a/packages/backend/src/remote/activitypub/kernel/delete/note.ts +++ b/packages/backend/src/remote/activitypub/kernel/delete/note.ts @@ -29,7 +29,7 @@ export default async function(actor: IRemoteUser, uri: string): Promise return 'skip: cant delete other actors note'; } - await deleteNotes([note], actor); + await deleteNotes([note]); return 'ok: note deleted'; } } finally { diff --git a/packages/backend/src/remote/activitypub/kernel/undo/announce.ts b/packages/backend/src/remote/activitypub/kernel/undo/announce.ts index 05e0edbb6..2aa112fad 100644 --- a/packages/backend/src/remote/activitypub/kernel/undo/announce.ts +++ b/packages/backend/src/remote/activitypub/kernel/undo/announce.ts @@ -13,6 +13,6 @@ export const undoAnnounce = async (actor: IRemoteUser, activity: IAnnounce): Pro if (!note) return 'skip: no such Announce'; - await deleteNotes([note], actor); + await deleteNotes([note]); return 'ok: deleted'; }; diff --git a/packages/backend/src/server/api/endpoints/notes/unrenote.ts b/packages/backend/src/server/api/endpoints/notes/unrenote.ts index 909835f5f..f7eec1af5 100644 --- a/packages/backend/src/server/api/endpoints/notes/unrenote.ts +++ b/packages/backend/src/server/api/endpoints/notes/unrenote.ts @@ -50,5 +50,5 @@ export default define(meta, paramDef, async (ps, user) => { if (renotes.length === 0) return; - await deleteNotes(renotes, user); + await deleteNotes(renotes); }); diff --git a/packages/backend/src/services/note/delete.ts b/packages/backend/src/services/note/delete.ts index f22ce80db..b0733d2f8 100644 --- a/packages/backend/src/services/note/delete.ts +++ b/packages/backend/src/services/note/delete.ts @@ -15,25 +15,43 @@ import { DeliverManager } from '@/remote/activitypub/deliver-manager.js'; import { countSameRenotes } from '@/misc/count-same-renotes.js'; import { registerOrFetchInstanceDoc } from '../register-or-fetch-instance-doc.js'; import { deliverMultipleToRelays } from '../relay.js'; +import { groupByX } from '@/prelude/array.js'; /** * Delete several notes of the same user. * @param notes Array of notes to be deleted. * @param user Author of the notes. Will be fetched if not provided. */ -export async function deleteNotes(notes: Note[], user?: User): Promise { - if (notes.length === 0) return; - - const fetchedUser = user ?? await Users.findOneByOrFail({ id: notes[0].userId }); - - const cascadingNotes = await Promise.all( +export async function deleteNotes(notes: Note[], cascading?: Note[]): Promise { + const cascadingNotes = cascading ?? await Promise.all( notes.map(note => findCascadingNotes(note)) ).then(res => res.flat()); + const allNotes = notes.concat(cascadingNotes); + if (allNotes.length === 0) return; + + // check if notes of different users are affected, they need to be handled separately + const userId = allNotes[0].userId; + if (allNotes.some(note => note.userId !== userId)) { + const notesGrouped = groupByX(notes, x => x.userId); + const cascadingGrouped = groupByX(cascadingNotes, x => x.userId); + + const users = [...new Set([...Object.keys(notesGrouped), ...Object.keys(cascadingGrouped)])]; + + await Promise.all( + users.map(userId => + deleteNotes(notesGrouped[userId] ?? [], cascadingGrouped[userId] ?? []) + ) + ); + return; + } + + // all of the notes left over are by a single user + const fetchedUser = await Users.findOneByOrFail({ id: userId }); + // perform side effects for notes and cascaded notes await Promise.all( - notes.concat(cascadingNotes) - .map(note => deletionSideEffects(note, fetchedUser)) + allNotes.map(note => deletionSideEffects(note, fetchedUser)) ); // Compute delivery content for later. @@ -41,7 +59,7 @@ export async function deleteNotes(notes: Note[], user?: User): Promise { // the database since we may need some information from parent // notes that cause this one to be cascade-deleted. let content = await Promise.all( - notes.concat(cascadingNotes) + allNotes // only deliver for local notes that are not local-only .filter(note => note.userHost == null && !note.localOnly) .map(async note => { @@ -65,8 +83,7 @@ export async function deleteNotes(notes: Note[], user?: User): Promise { manager.addEveryone(); // Check mentioned users, since not all may have a shared inbox. await Promise.all( - notes.concat(cascadingNotes) - .map(note => getMentionedRemoteUsers(note)) + allNotes.map(note => getMentionedRemoteUsers(note)) ) .then(remoteUsers => { remoteUsers.flat() @@ -77,15 +94,17 @@ export async function deleteNotes(notes: Note[], user?: User): Promise { // It is important that this is done before delivering the activities. // Otherwise there might be a race condition where we tell someone // the note exists and they can successfully fetch it. - await Notes.delete({ - id: In(notes.map(x => x.id)), - userId: fetchedUser.id, - }); + if (notes.length > 0) { + await Notes.delete({ + id: In(notes.map(x => x.id)), + userId: fetchedUser.id, + }); + } // deliver the previously computed content await Promise.all([ manager.execute(), - deliverMultipleToRelays(user, content), + deliverMultipleToRelays(fetchedUser, content), ]); }