From 9a8247dfa90ee25536f20d8e358713b40f92c7f3 Mon Sep 17 00:00:00 2001 From: Vitor Mattos Date: Mon, 2 Jan 2023 14:37:17 -0300 Subject: [PATCH 1/7] Hide expired messages when do not run expire job Signed-off-by: Vitor Mattos --- lib/Controller/ChatController.php | 5 +++++ .../features/bootstrap/FeatureContext.php | 14 -------------- .../features/chat/message-expiration.feature | 4 +--- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/lib/Controller/ChatController.php b/lib/Controller/ChatController.php index 9d5713637dc..d241a505a2c 100644 --- a/lib/Controller/ChatController.php +++ b/lib/Controller/ChatController.php @@ -472,6 +472,11 @@ public function receiveMessages(int $lookIntoFuture, $message = $this->messageParser->createMessage($this->room, $this->participant, $comment, $this->l); $this->messageParser->parseMessage($message); + $expireDate = $message->getComment()->getExpireDate(); + if ($expireDate instanceof \DateTime && $expireDate < $this->timeFactory->getDateTime()) { + $message->setVisibility(false); + } + if (!$message->getVisibility()) { $commentIdToIndex[$id] = null; continue; diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index ea071d472ed..54a129c73ff 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -2758,20 +2758,6 @@ public function waitForXSeconds($seconds): void { sleep($seconds); } - /** - * @When apply message expiration job manually - */ - public function applyMessageExpirationJob(): void { - $currentUser = $this->currentUser; - $this->setCurrentUser('admin'); - $this->sendRequest('GET', '/apps/spreedcheats/get_message_expiration_job'); - $response = $this->getDataFromResponse($this->response); - Assert::assertIsArray($response, 'Job not found'); - Assert::assertArrayHasKey('id', $response, 'Job not found'); - $this->runOcc(['background-job:execute', $response['id'], '--force-execute']); - $this->setCurrentUser($currentUser); - } - /* * Requests */ diff --git a/tests/integration/features/chat/message-expiration.feature b/tests/integration/features/chat/message-expiration.feature index 3d64053748c..c211faa746a 100644 --- a/tests/integration/features/chat/message-expiration.feature +++ b/tests/integration/features/chat/message-expiration.feature @@ -15,11 +15,10 @@ Feature: chat/message-expiration And user "participant3" set the message expiration to 3 of room "room" with 404 (v4) And user "participant1" set the message expiration to 3 of room "room" with 200 (v4) And user "participant1" sends message "Message 2" to room "room" with 201 - Then user "participant1" is participant of the following rooms (v4) + And user "participant1" is participant of the following rooms (v4) | id | type | messageExpiration | | room | 3 | 3 | And wait for 3 seconds - And apply message expiration job manually Then user "participant1" sees the following messages in room "room" with 200 | room | actorType | actorId | actorDisplayName | message | messageParameters | parentMessage | | room | users | participant1 | participant1-displayname | Message 1 | [] | | @@ -34,7 +33,6 @@ Feature: chat/message-expiration | room | actorType | actorId | actorDisplayName | message | messageParameters | | room2 | users | participant1 | participant1-displayname | {file} | "IGNORE" | And wait for 3 seconds - And apply message expiration job manually Then user "participant1" sees the following messages in room "room2" with 200 | room | actorType | actorId | actorDisplayName | message | messageParameters | parentMessage | And user "participant1" gets last share From f6225ba299d056ba5477e3634c467208f58cf961 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 4 Jan 2023 14:50:40 +0100 Subject: [PATCH 2/7] Only call getDateTime() once Signed-off-by: Joas Schilling --- lib/Controller/ChatController.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Controller/ChatController.php b/lib/Controller/ChatController.php index d241a505a2c..d7e165284bf 100644 --- a/lib/Controller/ChatController.php +++ b/lib/Controller/ChatController.php @@ -466,6 +466,7 @@ public function receiveMessages(int $lookIntoFuture, $this->preloadShares($comments); $i = 0; + $now = $this->timeFactory->getDateTime('now', new \DateTimeZone('UTC')); $messages = $commentIdToIndex = $parentIds = []; foreach ($comments as $comment) { $id = (int) $comment->getId(); @@ -473,7 +474,7 @@ public function receiveMessages(int $lookIntoFuture, $this->messageParser->parseMessage($message); $expireDate = $message->getComment()->getExpireDate(); - if ($expireDate instanceof \DateTime && $expireDate < $this->timeFactory->getDateTime()) { + if ($expireDate instanceof \DateTime && $expireDate < $now) { $message->setVisibility(false); } From f6ec8571d7fef547cf9b00afb67fd3a915cbf264 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 4 Jan 2023 14:50:53 +0100 Subject: [PATCH 3/7] Remove unused method Signed-off-by: Joas Schilling --- .../spreedcheats/appinfo/routes.php | 1 - .../lib/Controller/ApiController.php | 20 ------------------- 2 files changed, 21 deletions(-) diff --git a/tests/integration/spreedcheats/appinfo/routes.php b/tests/integration/spreedcheats/appinfo/routes.php index 57798148f60..7798d929fe4 100644 --- a/tests/integration/spreedcheats/appinfo/routes.php +++ b/tests/integration/spreedcheats/appinfo/routes.php @@ -26,6 +26,5 @@ return [ 'ocs' => [ ['name' => 'Api#resetSpreed', 'url' => '/', 'verb' => 'DELETE'], - ['name' => 'Api#getMessageExpirationJob', 'url' => '/get_message_expiration_job', 'verb' => 'GET'], ], ]; diff --git a/tests/integration/spreedcheats/lib/Controller/ApiController.php b/tests/integration/spreedcheats/lib/Controller/ApiController.php index 34f7b0d7f97..a53234beef1 100644 --- a/tests/integration/spreedcheats/lib/Controller/ApiController.php +++ b/tests/integration/spreedcheats/lib/Controller/ApiController.php @@ -103,24 +103,4 @@ public function resetSpreed(): DataResponse { return new DataResponse(); } - - /** - * @NoCSRFRequired - * - * @return DataResponse - */ - public function getMessageExpirationJob(): DataResponse { - $query = $this->db->getQueryBuilder(); - $query->select('id') - ->from('jobs') - ->where( - $query->expr()->eq('class', $query->createNamedParameter(ExpireChatMessages::class)) - ); - $result = $query->executeQuery(); - $job = $result->fetchOne(); - if ($job) { - return new DataResponse(['id' => (int) $job]); - } - return new DataResponse([], Http::STATUS_NOT_FOUND); - } } From 3dded0a10e5993dbd2c8128152c525aff7bcd421 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 4 Jan 2023 15:06:47 +0100 Subject: [PATCH 4/7] Also check the expiration on all other renderings Signed-off-by: Joas Schilling --- lib/Controller/ChatController.php | 17 +++++++++++++++-- lib/Controller/RoomController.php | 6 ++++++ lib/Dashboard/TalkWidget.php | 7 ++++++- lib/Notification/Notifier.php | 10 ++++++++++ lib/Search/MessageSearch.php | 11 ++++++++++- .../lib/Controller/ApiController.php | 2 -- tests/php/Notification/NotifierTest.php | 5 +++++ 7 files changed, 52 insertions(+), 6 deletions(-) diff --git a/lib/Controller/ChatController.php b/lib/Controller/ChatController.php index d7e165284bf..3ab4a541672 100644 --- a/lib/Controller/ChatController.php +++ b/lib/Controller/ChatController.php @@ -466,7 +466,7 @@ public function receiveMessages(int $lookIntoFuture, $this->preloadShares($comments); $i = 0; - $now = $this->timeFactory->getDateTime('now', new \DateTimeZone('UTC')); + $now = $this->timeFactory->getDateTime(); $messages = $commentIdToIndex = $parentIds = []; foreach ($comments as $comment) { $id = (int) $comment->getId(); @@ -475,7 +475,8 @@ public function receiveMessages(int $lookIntoFuture, $expireDate = $message->getComment()->getExpireDate(); if ($expireDate instanceof \DateTime && $expireDate < $now) { - $message->setVisibility(false); + $commentIdToIndex[$id] = null; + continue; } if (!$message->getVisibility()) { @@ -528,6 +529,12 @@ public function receiveMessages(int $lookIntoFuture, continue; } + $expireDate = $message->getComment()->getExpireDate(); + if ($expireDate instanceof \DateTime && $expireDate < $now) { + $commentIdToIndex[$id] = null; + continue; + } + $loadedParents[$parentId] = [ 'id' => (int) $parentId, 'deleted' => true, @@ -855,6 +862,12 @@ protected function getMessagesForRoom(Room $room, array $messageIds): array { $message = $this->messageParser->createMessage($room, $this->participant, $comment, $this->l); $this->messageParser->parseMessage($message); + $now = $this->timeFactory->getDateTime(); + $expireDate = $message->getComment()->getExpireDate(); + if ($expireDate instanceof \DateTime && $expireDate < $now) { + continue; + } + if (!$message->getVisibility()) { continue; } diff --git a/lib/Controller/RoomController.php b/lib/Controller/RoomController.php index 8c7c5197efb..0bd35b5862f 100644 --- a/lib/Controller/RoomController.php +++ b/lib/Controller/RoomController.php @@ -624,6 +624,12 @@ protected function formatLastMessage(Room $room, Participant $participant, IComm return []; } + $now = $this->timeFactory->getDateTime(); + $expireDate = $message->getComment()->getExpireDate(); + if ($expireDate instanceof \DateTime && $expireDate < $now) { + return []; + } + return $message->toArray($this->getResponseFormat()); } diff --git a/lib/Dashboard/TalkWidget.php b/lib/Dashboard/TalkWidget.php index 634d0bf9a3c..baee61f6f6b 100644 --- a/lib/Dashboard/TalkWidget.php +++ b/lib/Dashboard/TalkWidget.php @@ -70,6 +70,7 @@ public function __construct( $this->l10n = $l10n; $this->manager = $manager; $this->messageParser = $messageParser; + $this->timeFactory = $timeFactory; } /** @@ -170,7 +171,11 @@ protected function prepareRoom(Room $room, string $userId): WidgetItem { if ($lastMessage instanceof IComment) { $message = $this->messageParser->createMessage($room, $participant, $room->getLastMessage(), $this->l10n); $this->messageParser->parseMessage($message); - if ($message->getVisibility()) { + + $now = $this->timeFactory->getDateTime(); + $expireDate = $message->getComment()->getExpireDate(); + if ((!$expireDate instanceof \DateTime || $expireDate >= $now) + && $message->getVisibility()) { $placeholders = $replacements = []; foreach ($message->getMessageParameters() as $placeholder => $parameter) { diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index a369ae016e9..3439d810103 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -38,6 +38,7 @@ use OCA\Talk\Room; use OCA\Talk\Service\ParticipantService; use OCA\Talk\Webinary; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\Comments\ICommentsManager; use OCP\Comments\NotFoundException; use OCP\HintException; @@ -68,6 +69,7 @@ class Notifier implements INotifier { protected INotificationManager $notificationManager; protected ICommentsManager $commentManager; protected MessageParser $messageParser; + protected ITimeFactory $timeFactory; protected Definitions $definitions; protected AddressHandler $addressHandler; @@ -87,6 +89,7 @@ public function __construct(IFactory $lFactory, INotificationManager $notificationManager, CommentsManager $commentManager, MessageParser $messageParser, + ITimeFactory $timeFactory, Definitions $definitions, AddressHandler $addressHandler) { $this->lFactory = $lFactory; @@ -100,6 +103,7 @@ public function __construct(IFactory $lFactory, $this->notificationManager = $notificationManager; $this->commentManager = $commentManager; $this->messageParser = $messageParser; + $this->timeFactory = $timeFactory; $this->definitions = $definitions; $this->addressHandler = $addressHandler; } @@ -385,6 +389,12 @@ protected function parseChatMessage(INotification $notification, Room $room, Par throw new AlreadyProcessedException(); } + $now = $this->timeFactory->getDateTime(); + $expireDate = $message->getComment()->getExpireDate(); + if ($expireDate instanceof \DateTime && $expireDate < $now) { + throw new AlreadyProcessedException(); + } + if ($message->getMessageType() === ChatManager::VERB_MESSAGE_DELETED) { throw new AlreadyProcessedException(); } diff --git a/lib/Search/MessageSearch.php b/lib/Search/MessageSearch.php index 6ac0dfc3fc1..50daf766186 100644 --- a/lib/Search/MessageSearch.php +++ b/lib/Search/MessageSearch.php @@ -32,6 +32,7 @@ use OCA\Talk\Model\Attendee; use OCA\Talk\Room; use OCA\Talk\Webinary; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\Comments\IComment; use OCP\IL10N; use OCP\IURLGenerator; @@ -45,6 +46,7 @@ class MessageSearch implements IProvider { protected RoomManager $roomManager; protected ChatManager $chatManager; protected MessageParser $messageParser; + protected ITimeFactory $timeFactory; protected IURLGenerator $url; protected IL10N $l; @@ -52,12 +54,14 @@ public function __construct( RoomManager $roomManager, ChatManager $chatManager, MessageParser $messageParser, + ITimeFactory $timeFactory, IURLGenerator $url, IL10N $l ) { $this->roomManager = $roomManager; $this->chatManager = $chatManager; $this->messageParser = $messageParser; + $this->timeFactory = $timeFactory; $this->url = $url; $this->l = $l; } @@ -190,8 +194,13 @@ protected function commentToSearchResultEntry(Room $room, IUser $user, IComment $messageStr = '…' . mb_substr($messageStr, $matchPosition - 10); } + $now = $this->timeFactory->getDateTime(); + $expireDate = $message->getComment()->getExpireDate(); + if ($expireDate instanceof \DateTime && $expireDate < $now) { + throw new UnauthorizedException('Expired'); + } + if (!$message->getVisibility()) { - $commentIdToIndex[$id] = null; throw new UnauthorizedException('Not visible'); } diff --git a/tests/integration/spreedcheats/lib/Controller/ApiController.php b/tests/integration/spreedcheats/lib/Controller/ApiController.php index a53234beef1..4faf6f7898d 100644 --- a/tests/integration/spreedcheats/lib/Controller/ApiController.php +++ b/tests/integration/spreedcheats/lib/Controller/ApiController.php @@ -25,8 +25,6 @@ namespace OCA\SpreedCheats\Controller; -use OCA\Talk\BackgroundJob\ExpireChatMessages; -use OCP\AppFramework\Http; use OCP\AppFramework\OCSController; use OCP\AppFramework\Http\DataResponse; use OCP\IDBConnection; diff --git a/tests/php/Notification/NotifierTest.php b/tests/php/Notification/NotifierTest.php index fb1f5455b15..95483d963fa 100644 --- a/tests/php/Notification/NotifierTest.php +++ b/tests/php/Notification/NotifierTest.php @@ -35,6 +35,7 @@ use OCA\Talk\Participant; use OCA\Talk\Room; use OCA\Talk\Service\ParticipantService; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\Comments\IComment; use OCP\IL10N; use OCP\IURLGenerator; @@ -72,6 +73,8 @@ class NotifierTest extends TestCase { protected $commentsManager; /** @var MessageParser|MockObject */ protected $messageParser; + /** @var ITimeFactory|MockObject */ + protected $timeFactory; /** @var Definitions|MockObject */ protected $definitions; protected ?Notifier $notifier = null; @@ -92,6 +95,7 @@ public function setUp(): void { $this->notificationManager = $this->createMock(INotificationManager::class); $this->commentsManager = $this->createMock(CommentsManager::class); $this->messageParser = $this->createMock(MessageParser::class); + $this->timeFactory = $this->createMock(ITimeFactory::class); $this->definitions = $this->createMock(Definitions::class); $this->addressHandler = $this->createMock(AddressHandler::class); @@ -107,6 +111,7 @@ public function setUp(): void { $this->notificationManager, $this->commentsManager, $this->messageParser, + $this->timeFactory, $this->definitions, $this->addressHandler ); From 589f01f42a75b2b1119ca9eb0d35bdb5e64f66a9 Mon Sep 17 00:00:00 2001 From: Vitor Mattos Date: Mon, 2 Jan 2023 14:37:17 -0300 Subject: [PATCH 5/7] Hide expired messages when do not run expire job Signed-off-by: Vitor Mattos --- lib/Controller/ChatController.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/Controller/ChatController.php b/lib/Controller/ChatController.php index 3ab4a541672..42af921f7ea 100644 --- a/lib/Controller/ChatController.php +++ b/lib/Controller/ChatController.php @@ -474,9 +474,8 @@ public function receiveMessages(int $lookIntoFuture, $this->messageParser->parseMessage($message); $expireDate = $message->getComment()->getExpireDate(); - if ($expireDate instanceof \DateTime && $expireDate < $now) { - $commentIdToIndex[$id] = null; - continue; + if ($expireDate instanceof \DateTime && $expireDate < $this->timeFactory->getDateTime()) { + $message->setVisibility(false); } if (!$message->getVisibility()) { From 26b873883fca3bd59ddb84c48035da90b2f8bdfd Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 4 Jan 2023 14:50:40 +0100 Subject: [PATCH 6/7] Only call getDateTime() once Signed-off-by: Joas Schilling --- lib/Controller/ChatController.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Controller/ChatController.php b/lib/Controller/ChatController.php index 42af921f7ea..dfbb58fbeaa 100644 --- a/lib/Controller/ChatController.php +++ b/lib/Controller/ChatController.php @@ -466,7 +466,7 @@ public function receiveMessages(int $lookIntoFuture, $this->preloadShares($comments); $i = 0; - $now = $this->timeFactory->getDateTime(); + $now = $this->timeFactory->getDateTime('now', new \DateTimeZone('UTC')); $messages = $commentIdToIndex = $parentIds = []; foreach ($comments as $comment) { $id = (int) $comment->getId(); @@ -474,7 +474,7 @@ public function receiveMessages(int $lookIntoFuture, $this->messageParser->parseMessage($message); $expireDate = $message->getComment()->getExpireDate(); - if ($expireDate instanceof \DateTime && $expireDate < $this->timeFactory->getDateTime()) { + if ($expireDate instanceof \DateTime && $expireDate < $now) { $message->setVisibility(false); } From 0a5a9a1aae9d368827d9a4fe78f86286be0c1f74 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 4 Jan 2023 15:06:47 +0100 Subject: [PATCH 7/7] Also check the expiration on all other renderings Signed-off-by: Joas Schilling Signed-off-by: Vitor Mattos --- lib/Controller/ChatController.php | 5 +++-- lib/Dashboard/TalkWidget.php | 5 ++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/Controller/ChatController.php b/lib/Controller/ChatController.php index dfbb58fbeaa..3ab4a541672 100644 --- a/lib/Controller/ChatController.php +++ b/lib/Controller/ChatController.php @@ -466,7 +466,7 @@ public function receiveMessages(int $lookIntoFuture, $this->preloadShares($comments); $i = 0; - $now = $this->timeFactory->getDateTime('now', new \DateTimeZone('UTC')); + $now = $this->timeFactory->getDateTime(); $messages = $commentIdToIndex = $parentIds = []; foreach ($comments as $comment) { $id = (int) $comment->getId(); @@ -475,7 +475,8 @@ public function receiveMessages(int $lookIntoFuture, $expireDate = $message->getComment()->getExpireDate(); if ($expireDate instanceof \DateTime && $expireDate < $now) { - $message->setVisibility(false); + $commentIdToIndex[$id] = null; + continue; } if (!$message->getVisibility()) { diff --git a/lib/Dashboard/TalkWidget.php b/lib/Dashboard/TalkWidget.php index baee61f6f6b..169569e6dff 100644 --- a/lib/Dashboard/TalkWidget.php +++ b/lib/Dashboard/TalkWidget.php @@ -31,6 +31,7 @@ use OCA\Talk\Manager; use OCA\Talk\Participant; use OCA\Talk\Room; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\Comments\IComment; use OCP\Dashboard\IAPIWidget; use OCP\Dashboard\IButtonWidget; @@ -50,6 +51,7 @@ class TalkWidget implements IAPIWidget, IIconWidget, IButtonWidget, IOptionWidge private IL10N $l10n; private Manager $manager; private MessageParser $messageParser; + protected ITimeFactory $timeFactory; public function __construct( IUserSession $userSession, @@ -57,7 +59,8 @@ public function __construct( IURLGenerator $url, IL10N $l10n, Manager $manager, - MessageParser $messageParser + MessageParser $messageParser, + ITimeFactory $timeFactory ) { $user = $userSession->getUser(); if ($user instanceof IUser && $talkConfig->isDisabledForUser($user)) {