search

misskey-dev / misskey

🌎 An interplanetary microblogging platform 🚀
https://misskey-hub.net/
GNU Affero General Public License v3.0
9.7k stars 1.29k forks source link

admin/drive/cleanup could delete imported custom emoji #8222

Open Johann150 opened 2 years ago

Johann150 commented 2 years ago

💡 Summary

/api/admin/drive/cleanup deletes all drive files where userId = NULL: https://github.com/misskey-dev/misskey/blob/149edaecab3d160a1f480160caee055e2aff28bf/packages/backend/src/server/api/endpoints/admin/drive/cleanup.ts#L15-L17

This could accidentally delete imported emoji since they are created with userId set to NULL: https://github.com/misskey-dev/misskey/blob/149edaecab3d160a1f480160caee055e2aff28bf/packages/backend/src/server/api/endpoints/admin/emoji/copy.ts#L57

🔧❓ Possible Solutions

  1. Check that files are not used as emojis in /api/admin/drive/cleanup. This would probably be complicated, but could be done with the TypeORM equivalent of 
    ... LEFT JOIN emoji ON drive_file.url = emoji."originalUrl" WHERE emoji.id IS NULL
  2. Make @instance.actor the owner of imported emojis.
  3. Remove this API endpoint because drive files for deleted users are already deleted when the account is deleted: https://github.com/misskey-dev/misskey/blob/c69b72e1999578cba15e34677ebd366482cba734/packages/backend/src/queue/processors/db/delete-account.ts#L48

Additional Info

related to #7304

syuilo commented 2 years ago

There is also the option of not using Drive to manage file data used internally by the system, such as emoji (e.g., create a more general file table and have DriveFile refer to it).

Johann150 commented 2 years ago

That sounds much more complicated than any of the suggestions I made. Unless you have a different use for this I would absolutely not recommend it. And even then I think option 2 would be more sensible for what you say.

Johann150 commented 2 years ago

I think option 1 is too complicated.

If option 2 is implemented, we should also add a NOT NULL constraint to drive_file.user. Maybe the default for the column could be set after @instance.actor is created, but that sounds like a bad idea since it is not part of the usual database management with migrations, so it could easily get broken. Even without the default value of the column, it sounds like a good idea because I wouldn't be sure if other parts of Misskey do not also have different interpretations of the drive_file.user value.

However it would also implicate that @instance.actor is also subject to the drive usage limit, which could be a good and a bad thing. It would stop too many emoji being added to an instance (there are known performance issues with the emoji picker if many emojis are present). On the other hand it might also be an annoyance for instance administrators & moderators. Syuilo's suggestion would also avoid this issue, but continuing to use NULL could also avoid it and is already implemented and simpler.

It would also be possible to combine options 2 and 3.

However, in full view with the drive quota problem in mind I think option 3 is the best.

Johann150 commented 2 years ago

If option 3 is implemented, I'm not sure if this constraint should be changed as well:

TABLE "drive_file" CONSTRAINT "FK_860fa6f6c7df5bb887249fba22e" FOREIGN KEY ("userId") REFERENCES "user"(id) ON DELETE SET NULL

Setting to null on delete does not make sense because it would move the file to the drive of the system. Cascading the delete is also not a good idea because the file on disk has to be deleted outside of the database as well.

Maybe it should be changed to ON DELETE RESTRICT to make sure all drive files are deleted before a user can be deleted. As commented above this procedure is already followed by Misskey anway, it would just correctly mirror it in the database structure.