From fa7d240896b18efc2b589968764d43e279b8a8f8 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 2 Apr 2025 08:39:44 +0200 Subject: [PATCH] fix(performance): Parse file shares inaccurately to prevent setupFS In some cases we only use the "plain text" version of a chat message. We can render the conversation list subline without knowing the exact path of a file. All other information is still available on the cache entry. The path is only needed in some special cases when afterwards the viewer is proposed to the user. Signed-off-by: Joas Schilling --- lib/Chat/MessageParser.php | 9 +++-- lib/Chat/Parser/SystemMessage.php | 36 +++++++++++++------ lib/Chat/SystemMessage/Listener.php | 2 +- .../Reference/TalkReferenceProvider.php | 2 +- lib/Controller/ChatController.php | 2 +- lib/Dashboard/TalkWidget.php | 2 +- lib/Events/MessageParseEvent.php | 5 +++ lib/Notification/Notifier.php | 2 +- lib/Service/BotService.php | 4 +-- lib/Service/RoomFormatter.php | 2 +- tests/php/Chat/Parser/SystemMessageTest.php | 16 ++++----- tests/php/Controller/ChatControllerTest.php | 4 +-- 12 files changed, 56 insertions(+), 30 deletions(-) diff --git a/lib/Chat/MessageParser.php b/lib/Chat/MessageParser.php index 3aa74f255f..739d1ba7b1 100644 --- a/lib/Chat/MessageParser.php +++ b/lib/Chat/MessageParser.php @@ -66,7 +66,12 @@ class MessageParser { return $message; } - public function parseMessage(Message $message): void { + /** + * @param bool $allowInaccurate File share messages will not have fully correct data for the file object + * E.g. path is only the file name and preview generation is estimated by + * mimetype only. This is done to prevent a filesystem setup. + */ + public function parseMessage(Message $message, bool $allowInaccurate = false): void { $message->setMessage($message->getComment()->getMessage(), []); $verb = $message->getComment()->getVerb(); @@ -77,7 +82,7 @@ class MessageParser { $this->setMessageActor($message); $this->setLastEditInfo($message); - $event = new MessageParseEvent($message->getRoom(), $message); + $event = new MessageParseEvent($message->getRoom(), $message, $allowInaccurate); $this->dispatcher->dispatchTyped($event); } diff --git a/lib/Chat/Parser/SystemMessage.php b/lib/Chat/Parser/SystemMessage.php index 9b6684b49c..5d683dadb2 100644 --- a/lib/Chat/Parser/SystemMessage.php +++ b/lib/Chat/Parser/SystemMessage.php @@ -28,6 +28,7 @@ use OCP\Comments\ICommentsManager; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\Federation\ICloudIdManager; +use OCP\Files\FileInfo; use OCP\Files\InvalidPathException; use OCP\Files\IRootFolder; use OCP\Files\Node; @@ -92,7 +93,7 @@ class SystemMessage implements IEventListener { if ($event->getMessage()->getMessageType() === ChatManager::VERB_SYSTEM) { try { - $this->parseMessage($event->getMessage()); + $this->parseMessage($event->getMessage(), $event->allowInaccurate()); // Disabled so we can parse mentions in captions: $event->stopPropagation(); } catch (\OutOfBoundsException $e) { // Unknown message, ignore @@ -111,7 +112,7 @@ class SystemMessage implements IEventListener { * @param Message $chatMessage * @throws \OutOfBoundsException */ - protected function parseMessage(Message $chatMessage): void { + protected function parseMessage(Message $chatMessage, $allowInaccurate): void { $this->l = $chatMessage->getL10n(); $comment = $chatMessage->getComment(); $room = $chatMessage->getRoom(); @@ -515,7 +516,7 @@ class SystemMessage implements IEventListener { } } elseif ($message === 'file_shared') { try { - $parsedParameters['file'] = $this->getFileFromShare($room, $participant, $parameters['share']); + $parsedParameters['file'] = $this->getFileFromShare($room, $participant, $parameters['share'], $allowInaccurate); $parsedMessage = '{file}'; $metaData = $parameters['metaData'] ?? []; if (isset($metaData['messageType'])) { @@ -759,18 +760,24 @@ class SystemMessage implements IEventListener { } /** - * @param ?Participant $participant - * @param string $shareId - * @return array * @throws InvalidPathException * @throws NotFoundException * @throws ShareNotFound */ - protected function getFileFromShare(Room $room, ?Participant $participant, string $shareId): array { + protected function getFileFromShare(Room $room, ?Participant $participant, string $shareId, bool $allowInaccurate): array { $share = $this->shareProvider->getShareById((int)$shareId); if ($participant && $participant->getAttendee()->getActorType() === Attendee::ACTOR_USERS) { - if ($share->getShareOwner() !== $participant->getAttendee()->getActorId()) { + if ($allowInaccurate) { + $node = $share->getNodeCacheEntry(); + if ($node === null) { + throw new ShareNotFound(); + } + + $name = $node->getName(); + $size = $node->getSize(); + $path = $name; + } elseif ($share->getShareOwner() !== $participant->getAttendee()->getActorId()) { $userFolder = $this->rootFolder->getUserFolder($participant->getAttendee()->getActorId()); if (!$userFolder instanceof Node) { throw new ShareNotFound(); @@ -814,7 +821,12 @@ class SystemMessage implements IEventListener { } elseif ($participant && $room->getType() !== Room::TYPE_PUBLIC && $participant->getAttendee()->getActorType() === Attendee::ACTOR_FEDERATED_USERS) { throw new ShareNotFound(); } else { - $node = $share->getNode(); + if ($allowInaccurate) { + $node = $share->getNodeCacheEntry(); + } else { + $node = $share->getNode(); + } + $name = $node->getName(); $size = $node->getSize(); $path = $name; @@ -825,7 +837,11 @@ class SystemMessage implements IEventListener { } $fileId = $node->getId(); - $isPreviewAvailable = $this->previewManager->isAvailable($node); + if ($node instanceof FileInfo) { + $isPreviewAvailable = $this->previewManager->isAvailable($node); + } else { + $isPreviewAvailable = $size > 0 && $this->previewManager->isMimeSupported($node->getMimeType()); + } $data = [ 'type' => 'file', diff --git a/lib/Chat/SystemMessage/Listener.php b/lib/Chat/SystemMessage/Listener.php index 70911e227c..eb7d7d4e8d 100644 --- a/lib/Chat/SystemMessage/Listener.php +++ b/lib/Chat/SystemMessage/Listener.php @@ -469,7 +469,7 @@ class Listener implements IEventListener { try { $parentComment = $this->chatManager->getParentComment($room, (string)$replyTo); $parentMessage = $this->messageParser->createMessage($room, $participant, $parentComment, $this->l); - $this->messageParser->parseMessage($parentMessage); + $this->messageParser->parseMessage($parentMessage, true); if ($parentMessage->isReplyable()) { $parent = $parentComment; } diff --git a/lib/Collaboration/Reference/TalkReferenceProvider.php b/lib/Collaboration/Reference/TalkReferenceProvider.php index e80b321f6a..c74bca4983 100644 --- a/lib/Collaboration/Reference/TalkReferenceProvider.php +++ b/lib/Collaboration/Reference/TalkReferenceProvider.php @@ -173,7 +173,7 @@ class TalkReferenceProvider extends ADiscoverableReferenceProvider implements IS throw new RoomNotFoundException(); } $message = $this->messageParser->createMessage($room, $participant, $comment, $this->l); - $this->messageParser->parseMessage($message); + $this->messageParser->parseMessage($message, true); } else { try { $proxy = $this->proxyCacheMessageMapper->findById($room, (int)$messageId); diff --git a/lib/Controller/ChatController.php b/lib/Controller/ChatController.php index b9b9e4936d..5b7f337b6f 100644 --- a/lib/Controller/ChatController.php +++ b/lib/Controller/ChatController.php @@ -553,7 +553,7 @@ class ChatController extends AEnvironmentAwareOCSController { foreach ($comments as $comment) { $nextOffset = (int)$comment->getId(); $message = $this->messageParser->createMessage($this->room, $this->participant, $comment, $this->l); - $this->messageParser->parseMessage($message); + $this->messageParser->parseMessage($message, true); if (!$message->getVisibility()) { continue; diff --git a/lib/Dashboard/TalkWidget.php b/lib/Dashboard/TalkWidget.php index 5b5aea5855..7dfbefa63a 100644 --- a/lib/Dashboard/TalkWidget.php +++ b/lib/Dashboard/TalkWidget.php @@ -281,7 +281,7 @@ class TalkWidget implements IAPIWidget, IIconWidget, IButtonWidget, IOptionWidge } } elseif ($room->getLastMessageId() && $room->getLastMessage() && !$room->isFederatedConversation()) { $message = $this->messageParser->createMessage($room, $participant, $room->getLastMessage(), $this->l10n); - $this->messageParser->parseMessage($message); + $this->messageParser->parseMessage($message, true); $subtitle = $this->getSubtitleFromMessage($message); } diff --git a/lib/Events/MessageParseEvent.php b/lib/Events/MessageParseEvent.php index f2e06ca5c2..e8cef55a2c 100644 --- a/lib/Events/MessageParseEvent.php +++ b/lib/Events/MessageParseEvent.php @@ -15,6 +15,7 @@ class MessageParseEvent extends ARoomEvent { public function __construct( Room $room, protected Message $message, + protected bool $allowInaccurate, ) { parent::__construct($room); } @@ -22,4 +23,8 @@ class MessageParseEvent extends ARoomEvent { public function getMessage(): Message { return $this->message; } + + public function allowInaccurate(): bool { + return $this->allowInaccurate; + } } diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index a9e43c24d1..9d3136422d 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -525,7 +525,7 @@ class Notifier implements INotifier { } $message = $this->messageParser->createMessage($room, $participant, $comment, $l); - $this->messageParser->parseMessage($message); + $this->messageParser->parseMessage($message, true); if (!$message->getVisibility()) { throw new AlreadyProcessedException(); diff --git a/lib/Service/BotService.php b/lib/Service/BotService.php index 4517e4f035..5d320ffa5b 100644 --- a/lib/Service/BotService.php +++ b/lib/Service/BotService.php @@ -106,7 +106,7 @@ class BotService { $parent, $this->l10nFactory->get('spreed', 'en', 'en') ); - $messageParser->parseMessage($parentMessage); + $messageParser->parseMessage($parentMessage, true); $parentMessageData = [ 'message' => $parentMessage->getMessage(), 'parameters' => $parentMessage->getMessageParameters(), @@ -125,7 +125,7 @@ class BotService { $event->getComment(), $this->l10nFactory->get('spreed', 'en', 'en') ); - $messageParser->parseMessage($message); + $messageParser->parseMessage($message, true); $messageData = [ 'message' => $message->getMessage(), 'parameters' => $message->getMessageParameters(), diff --git a/lib/Service/RoomFormatter.php b/lib/Service/RoomFormatter.php index f215bd45bf..48faea4672 100644 --- a/lib/Service/RoomFormatter.php +++ b/lib/Service/RoomFormatter.php @@ -435,7 +435,7 @@ class RoomFormatter { IComment $lastMessage, ): ?array { $message = $this->messageParser->createMessage($room, $participant, $lastMessage, $this->l10n); - $this->messageParser->parseMessage($message); + $this->messageParser->parseMessage($message, true); if (!$message->getVisibility()) { return null; diff --git a/tests/php/Chat/Parser/SystemMessageTest.php b/tests/php/Chat/Parser/SystemMessageTest.php index 9c3c823e05..9e26ded7f3 100644 --- a/tests/php/Chat/Parser/SystemMessageTest.php +++ b/tests/php/Chat/Parser/SystemMessageTest.php @@ -632,7 +632,7 @@ class SystemMessageTest extends TestCase { 'parameters' => $parameters, ]), [], $message); - self::invokePrivate($parser, 'parseMessage', [$chatMessage]); + self::invokePrivate($parser, 'parseMessage', [$chatMessage, false]); $this->assertSame($expectedMessage, $chatMessage->getMessage()); $this->assertSame($expectedParameters, $chatMessage->getMessageParameters()); @@ -672,7 +672,7 @@ class SystemMessageTest extends TestCase { $chatMessage->setMessage($return, []); $this->expectException(\OutOfBoundsException::class); - self::invokePrivate($parser, 'parseMessage', [$chatMessage]); + self::invokePrivate($parser, 'parseMessage', [$chatMessage, false]); } public function testGetFileFromShareForGuest(): void { @@ -743,7 +743,7 @@ class SystemMessageTest extends TestCase { 'preview-available' => 'yes', 'width' => '1234', 'height' => '4567', - ], self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23'])); + ], self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23', false])); } public function testGetFileFromShareForGuestWithBlurhash(): void { @@ -820,7 +820,7 @@ class SystemMessageTest extends TestCase { 'width' => '1234', 'height' => '4567', 'blurhash' => 'LEHV9uae2yk8pyo0adR*.7kCMdnj', - ], self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23'])); + ], self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23', false])); } public function testGetFileFromShareForOwner(): void { @@ -897,7 +897,7 @@ class SystemMessageTest extends TestCase { 'permissions' => '27', 'mimetype' => 'httpd/unix-directory', 'preview-available' => 'no', - ], self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23'])); + ], self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23', false])); } public function testGetFileFromShareForRecipient(): void { @@ -982,7 +982,7 @@ class SystemMessageTest extends TestCase { 'permissions' => '27', 'mimetype' => 'application/octet-stream', 'preview-available' => 'no', - ], self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23'])); + ], self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23', false])); } public function testGetFileFromShareForRecipientThrows(): void { @@ -1026,7 +1026,7 @@ class SystemMessageTest extends TestCase { $parser = $this->getParser(); $this->expectException(NotFoundException::class); - self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23']); + self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23', false]); } public function testGetFileFromShareThrows(): void { @@ -1046,7 +1046,7 @@ class SystemMessageTest extends TestCase { ->willReturn($attendee); $parser = $this->getParser(); $this->expectException(ShareNotFound::class); - self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23']); + self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23', false]); } public static function dataGetActor(): array { diff --git a/tests/php/Controller/ChatControllerTest.php b/tests/php/Controller/ChatControllerTest.php index 1e0897770e..a10db705ca 100644 --- a/tests/php/Controller/ChatControllerTest.php +++ b/tests/php/Controller/ChatControllerTest.php @@ -421,8 +421,8 @@ class ChatControllerTest extends TestCase { $i = 0; $expectedCalls = [ - [$parentMessage], - [$chatMessage], + [$parentMessage, false], + [$chatMessage, false], ]; $this->messageParser->expects($this->exactly(2)) ->method('parseMessage')