From 3c0f24c3ed92ea87039475df500396753179dc61 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 7 Jun 2022 22:05:56 +0200 Subject: [PATCH 1/7] Add lots of debugging ... Signed-off-by: Joas Schilling --- lib/Federation/CloudFederationProviderTalk.php | 12 +++++++++++- tests/php/Federation/FederationTest.php | 6 ++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/Federation/CloudFederationProviderTalk.php b/lib/Federation/CloudFederationProviderTalk.php index 9ec518de2c9..9b8ce5f20d5 100644 --- a/lib/Federation/CloudFederationProviderTalk.php +++ b/lib/Federation/CloudFederationProviderTalk.php @@ -49,6 +49,7 @@ use OCP\IUserManager; use OCP\Notification\IManager as INotificationManager; use OCP\Share\Exceptions\ShareNotFound; +use Psr\Log\LoggerInterface; class CloudFederationProviderTalk implements ICloudFederationProvider { private IUserManager $userManager; @@ -68,6 +69,7 @@ class CloudFederationProviderTalk implements ICloudFederationProvider { private AttendeeMapper $attendeeMapper; private Manager $manager; + private LoggerInterface $logger; public function __construct( IUserManager $userManager, @@ -78,7 +80,8 @@ public function __construct( IURLGenerator $urlGenerator, ParticipantService $participantService, AttendeeMapper $attendeeMapper, - Manager $manager + Manager $manager, + LoggerInterface $logger ) { $this->userManager = $userManager; $this->addressHandler = $addressHandler; @@ -89,6 +92,7 @@ public function __construct( $this->participantService = $participantService; $this->attendeeMapper = $attendeeMapper; $this->manager = $manager; + $this->logger = $logger; } /** @@ -105,15 +109,18 @@ public function getShareType(): string { */ public function shareReceived(ICloudFederationShare $share): string { if (!$this->config->isFederationEnabled()) { + $this->logger->debug('Received a federation invite but federation is disabled'); throw new ProviderCouldNotAddShareException('Server does not support talk federation', '', Http::STATUS_SERVICE_UNAVAILABLE); } if (!in_array($share->getShareType(), $this->getSupportedShareTypes(), true)) { + $this->logger->debug('Received a federation invite for invalid share type'); throw new ProviderCouldNotAddShareException('Support for sharing with non-users not implemented yet', '', Http::STATUS_NOT_IMPLEMENTED); // TODO: Implement group shares } $roomType = $share->getProtocol()['roomType']; if (!is_numeric($roomType) || !in_array((int) $roomType, $this->validSharedRoomTypes(), true)) { + $this->logger->debug('Received a federation invite for invalid room type'); throw new ProviderCouldNotAddShareException('roomType is not a valid number', '', Http::STATUS_BAD_REQUEST); } @@ -139,6 +146,7 @@ public function shareReceived(ICloudFederationShare $share): string { if ($remote && $shareSecret && $shareWith && $roomToken && $remoteId && is_string($roomName) && $roomName && $owner) { $shareWith = $this->userManager->get($shareWith); if ($shareWith === null) { + $this->logger->debug('Received a federation invite for user that could not be found'); throw new ProviderCouldNotAddShareException('User does not exist', '', Http::STATUS_BAD_REQUEST); } @@ -147,6 +155,8 @@ public function shareReceived(ICloudFederationShare $share): string { $this->notifyAboutNewShare($shareWith, $shareId, $sharedByFederatedId, $sharedBy, $roomName, $roomToken, $remote); return $shareId; } + + $this->logger->debug('Received a federation invite with missing request data'); throw new ProviderCouldNotAddShareException('required request data not found', '', Http::STATUS_BAD_REQUEST); } diff --git a/tests/php/Federation/FederationTest.php b/tests/php/Federation/FederationTest.php index 8bb7bd13dde..709937c4775 100644 --- a/tests/php/Federation/FederationTest.php +++ b/tests/php/Federation/FederationTest.php @@ -84,11 +84,12 @@ public function setUp(): void { $this->userManager = $this->createMock(IUserManager::class); $this->attendeeMapper = $this->createMock(AttendeeMapper::class); $this->config = $this->createMock(Config::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->notifications = new Notifications( $this->cloudFederationFactory, $this->addressHandler, - $this->createMock(LoggerInterface::class), + $this->logger, $this->cloudFederationProviderManager, $this->createMock(IJobList::class), $this->userManager, @@ -106,7 +107,8 @@ public function setUp(): void { $this->createMock(IURLGenerator::class), $this->createMock(ParticipantService::class), $this->attendeeMapper, - $this->createMock(Manager::class) + $this->createMock(Manager::class), + $this->logger ); } From 958300955da04c0fd8feee30200dcc8cbdd0b9c9 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 7 Jun 2022 22:06:18 +0200 Subject: [PATCH 2/7] ... just to find out it's the URL generation Signed-off-by: Joas Schilling --- lib/Federation/CloudFederationProviderTalk.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Federation/CloudFederationProviderTalk.php b/lib/Federation/CloudFederationProviderTalk.php index 9b8ce5f20d5..b46c0ea372d 100644 --- a/lib/Federation/CloudFederationProviderTalk.php +++ b/lib/Federation/CloudFederationProviderTalk.php @@ -297,12 +297,12 @@ private function notifyAboutNewShare(IUser $shareWith, string $shareId, string $ $declineAction = $notification->createAction(); $declineAction->setLabel('decline') - ->setLink($this->urlGenerator->linkToOCSRouteAbsolute('spreed.Federation.rejectShare', ['id' => $shareId]), 'DELETE'); + ->setLink($this->urlGenerator->linkToOCSRouteAbsolute('spreed.Federation.rejectShare', ['apiVersion' => 'v1', 'id' => $shareId]), 'DELETE'); $notification->addAction($declineAction); $acceptAction = $notification->createAction(); $acceptAction->setLabel('accept') - ->setLink($this->urlGenerator->linkToOCSRouteAbsolute('spreed.Federation.acceptShare', ['id' => $shareId]), 'POST'); + ->setLink($this->urlGenerator->linkToOCSRouteAbsolute('spreed.Federation.acceptShare', ['apiVersion' => 'v1', 'id' => $shareId]), 'POST'); $notification->addAction($acceptAction); $this->notificationManager->notify($notification); From c4f4c25569c93125c7fee5beb161b403e086a154 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 7 Jun 2022 22:07:07 +0200 Subject: [PATCH 3/7] Try accepting the invite Signed-off-by: Joas Schilling --- .../features/bootstrap/FeatureContext.php | 66 +++++++++++++++++-- .../features/federation/invite.feature | 6 ++ 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index 6e741187f35..b1f1c48021d 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -52,6 +52,8 @@ class FeatureContext implements Context, SnippetAcceptingContext { protected static $textToMessageId; /** @var array[] */ protected static $messageIdToText; + /** @var int[] */ + protected static $remoteToInviteId; protected static $permissionsMap = [ @@ -360,6 +362,31 @@ public function userHasInvites(string $user, string $apiVersion, TableNode $form } $this->assertInvites($invites, $formData); + + foreach ($invites as $data) { + self::$remoteToInviteId[$this->translateRemoteServer($data['remote_server']) . '#' . $data['remote_token']] = $data['id']; + } + } + + /** + * @Then /^user "([^"]*)" (accepts|declines) invite to room "([^"]*)" of server "([^"]*)" \((v1)\)$/ + * + * @param string $user + * @param string $roomName + * @param string $server + * @param string $apiVersion + * @param TableNode|null $formData + */ + public function userAcceptsDeclinesRemoteInvite(string $user, string $acceptsDeclines, string $roomName, string $server, string $apiVersion, TableNode $formData = null): void { + $inviteId = self::$remoteToInviteId[$server . '#' . self::$identifierToToken[$roomName]]; + + $verb = $acceptsDeclines === 'accepts' ? 'POST' : 'DELETE'; + + $this->setCurrentUser($user); + if ($server === 'LOCAL') { + $this->sendRemoteRequest($verb, '/apps/spreed/api/' . $apiVersion . '/federation/invitation/' . $inviteId); + } + $this->assertStatusCode($this->response, 200); } /** @@ -380,19 +407,23 @@ private function assertInvites($invites, TableNode $formData) { $data['remote_token'] = self::$tokenToIdentifier[$invite['remote_token']] ?? 'unknown-token'; } if (isset($expectedInvite['remote_server'])) { - if ($invite['remote_server'] === 'localhost:8080') { - $data['remote_server'] = 'LOCAL'; - } elseif ($invite['remote_server'] === 'localhost:8180') { - $data['remote_server'] = 'REMOTE'; - } else { - $data['remote_server'] = 'unknown-server'; - } + $data['remote_server'] = $this->translateRemoteServer($invite['remote_server']); } return $data; }, $invites, $formData->getHash())); } + protected function translateRemoteServer(string $server): string { + if ($server === 'localhost:8080') { + return 'LOCAL'; + } + if ($server === 'localhost:8180') { + return 'REMOTE'; + } + return 'unknown-server'; + } + /** * @Then /^user "([^"]*)" (is|is not) participant of room "([^"]*)" \((v4)\)$/ * @@ -2414,6 +2445,27 @@ private function extractRequestTokenFromResponse(ResponseInterface $response): s */ public function sendRequest($verb, $url, $body = null, array $headers = []) { $fullUrl = $this->baseUrl . 'ocs/v2.php' . $url; + $this->sendRequestFullUrl($verb, $fullUrl, $body, $headers); + } + + /** + * @param string $verb + * @param string $url + * @param TableNode|array|null $body + * @param array $headers + */ + public function sendRemoteRequest($verb, $url, $body = null, array $headers = []) { + $fullUrl = $this->baseRemoteUrl . 'ocs/v2.php' . $url; + $this->sendRequestFullUrl($verb, $fullUrl, $body, $headers); + } + + /** + * @param string $verb + * @param string $fullUrl + * @param TableNode|array|null $body + * @param array $headers + */ + public function sendRequestFullUrl($verb, $fullUrl, $body = null, array $headers = []) { $client = new Client(); $options = ['cookies' => $this->getUserCookieJar($this->currentUser)]; if ($this->currentUser === 'admin') { diff --git a/tests/integration/features/federation/invite.feature b/tests/integration/features/federation/invite.feature index 26f93e6bbcc..17ae25a9e71 100644 --- a/tests/integration/features/federation/invite.feature +++ b/tests/integration/features/federation/invite.feature @@ -28,3 +28,9 @@ Feature: federation/invite And user "participant2" has the following invitations (v1) | remote_server | remote_token | | LOCAL | room | +# Then user "participant2" has the following notifications +# | app | object_type | object_id | subject | +# | spreed | room | room | Should have a remote invite | + And user "participant2" accepts invite to room "room" of server "LOCAL" (v1) + And user "participant2" has the following invitations (v1) + | remote_server | remote_token | From e1f5aa90f7d2faf771a407af42ad985dfa8074bd Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 7 Jun 2022 22:07:21 +0200 Subject: [PATCH 4/7] Start server on both ports Signed-off-by: Joas Schilling --- tests/integration/run.sh | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/tests/integration/run.sh b/tests/integration/run.sh index ce3ebd25e0f..832032ba3e2 100755 --- a/tests/integration/run.sh +++ b/tests/integration/run.sh @@ -17,16 +17,25 @@ echo '#' echo '# Starting PHP webserver' echo '#' php -S localhost:8080 -t ${ROOT_DIR} & -PHPPID=$! +PHPPID1=$! echo 'Running on process ID:' -echo $PHPPID +echo $PHPPID1 # also kill php process in case of ctrl+c -trap 'kill -TERM $PHPPID; wait $PHPPID' TERM +trap 'kill -TERM $PHPPID1; wait $PHPPID1' TERM # The federated server is started and stopped by the tests themselves PORT_FED=8180 export PORT_FED + +php -S localhost:${PORT_FED} -t ${ROOT_DIR} & +PHPPID2=$! +echo 'Running on process ID:' +echo $PHPPID2 + +# also kill php process in case of ctrl+c +trap 'kill -TERM $PHPPID2; wait $PHPPID2' TERM + NEXTCLOUD_ROOT_DIR=${ROOT_DIR} export NEXTCLOUD_ROOT_DIR export TEST_SERVER_URL="http://localhost:8080/" @@ -74,11 +83,13 @@ echo '' echo '#' echo '# Stopping PHP webserver and disabling spreedcheats' echo '#' -kill $PHPPID +kill $PHPPID1 +kill $PHPPID2 ${ROOT_DIR}/occ app:disable spreedcheats rm -rf ../../../spreedcheats -wait $PHPPID +wait $PHPPID1 +wait $PHPPID2 exit $RESULT From 486b89806f4185b01e61009ee486df90cbd5fa22 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 8 Jun 2022 10:08:53 +0200 Subject: [PATCH 5/7] Fix notification handling Signed-off-by: Joas Schilling --- lib/Notification/Notifier.php | 18 +++++++++++------- .../features/bootstrap/FeatureContext.php | 9 +++++++-- .../features/federation/invite.feature | 12 ++++++------ tests/php/Notification/NotifierTest.php | 17 ++++++----------- 4 files changed, 30 insertions(+), 26 deletions(-) diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index b069aead624..68af3211324 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -213,6 +213,10 @@ public function prepare(INotification $notification, string $languageCode): INot return $this->parseHostedSignalingServer($notification, $l); } + if ($notification->getObjectType() === 'remote_talk_share') { + return $this->parseRemoteInvitationMessage($notification, $l); + } + try { $room = $this->getRoom($notification->getObjectId(), $userId); } catch (RoomNotFoundException $e) { @@ -252,10 +256,6 @@ public function prepare(INotification $notification, string $languageCode): INot return $this->parseChatMessage($notification, $room, $participant, $l); } - if ($subject === 'remote_talk_share') { - return $this->parseRemoteInvitationMessage($notification, $l); - } - $this->notificationManager->markProcessed($notification); throw new \InvalidArgumentException('Unknown subject'); } @@ -300,11 +300,15 @@ protected function parseRemoteInvitationMessage(INotification $notification, IL1 $placeholders = $replacements = []; foreach ($rosParameters as $placeholder => $parameter) { $placeholders[] = '{' . $placeholder .'}'; - $replacements[] = $parameter['name']; + if ($parameter['type'] === 'user') { + $replacements[] = '@' . $parameter['name']; + } else { + $replacements[] = $parameter['name']; + } } - $notification->setParsedMessage(str_replace($placeholders, $replacements, $message)); - $notification->setRichMessage($message, $rosParameters); + $notification->setParsedSubject(str_replace($placeholders, $replacements, $message)); + $notification->setRichSubject($message, $rosParameters); return $notification; } diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index b1f1c48021d..cd0fc0c0d00 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -54,6 +54,8 @@ class FeatureContext implements Context, SnippetAcceptingContext { protected static $messageIdToText; /** @var int[] */ protected static $remoteToInviteId; + /** @var string[] */ + protected static $inviteIdToRemote; protected static $permissionsMap = [ @@ -364,7 +366,8 @@ public function userHasInvites(string $user, string $apiVersion, TableNode $form $this->assertInvites($invites, $formData); foreach ($invites as $data) { - self::$remoteToInviteId[$this->translateRemoteServer($data['remote_server']) . '#' . $data['remote_token']] = $data['id']; + self::$remoteToInviteId[$this->translateRemoteServer($data['remote_server']) . '::' . self::$tokenToIdentifier[$data['remote_token']]] = $data['id']; + self::$inviteIdToRemote[$data['id']] = $this->translateRemoteServer($data['remote_server']) . '::' . self::$tokenToIdentifier[$data['remote_token']]; } } @@ -378,7 +381,7 @@ public function userHasInvites(string $user, string $apiVersion, TableNode $form * @param TableNode|null $formData */ public function userAcceptsDeclinesRemoteInvite(string $user, string $acceptsDeclines, string $roomName, string $server, string $apiVersion, TableNode $formData = null): void { - $inviteId = self::$remoteToInviteId[$server . '#' . self::$identifierToToken[$roomName]]; + $inviteId = self::$remoteToInviteId[$server . '::' . $roomName]; $verb = $acceptsDeclines === 'accepts' ? 'POST' : 'DELETE'; @@ -2037,6 +2040,8 @@ private function assertNotifications($notifications, TableNode $formData) { if (strpos($notification['object_id'], '/') !== false) { [$roomToken, $message] = explode('/', $notification['object_id']); $data['object_id'] = self::$tokenToIdentifier[$roomToken] . '/' . self::$messageIdToText[$message] ?? 'UNKNOWN_MESSAGE'; + } elseif (strpos($expectedNotification['object_id'], 'INVITE_ID') !== false) { + $data['object_id'] = 'INVITE_ID(' . self::$inviteIdToRemote[$notification['object_id']] . ')'; } else { [$roomToken,] = explode('/', $notification['object_id']); $data['object_id'] = self::$tokenToIdentifier[$roomToken]; diff --git a/tests/integration/features/federation/invite.feature b/tests/integration/features/federation/invite.feature index 17ae25a9e71..460d28daca8 100644 --- a/tests/integration/features/federation/invite.feature +++ b/tests/integration/features/federation/invite.feature @@ -28,9 +28,9 @@ Feature: federation/invite And user "participant2" has the following invitations (v1) | remote_server | remote_token | | LOCAL | room | -# Then user "participant2" has the following notifications -# | app | object_type | object_id | subject | -# | spreed | room | room | Should have a remote invite | - And user "participant2" accepts invite to room "room" of server "LOCAL" (v1) - And user "participant2" has the following invitations (v1) - | remote_server | remote_token | + Then user "participant2" has the following notifications + | app | object_type | object_id | subject | + | spreed | remote_talk_share | INVITE_ID(LOCAL::room) | @participant1-displayname shared room room on localhost:8080 with you | +# And user "participant2" accepts invite to room "room" of server "LOCAL" (v1) +# And user "participant2" has the following invitations (v1) +# | remote_server | remote_token | diff --git a/tests/php/Notification/NotifierTest.php b/tests/php/Notification/NotifierTest.php index 7148f6e6f79..2192b3de988 100644 --- a/tests/php/Notification/NotifierTest.php +++ b/tests/php/Notification/NotifierTest.php @@ -212,8 +212,7 @@ public function testPrepareOne2One($uid, $displayName, $parsedSubject) { $n->expects($this->once()) ->method('getSubjectParameters') ->willReturn([$uid]); - $n->expects($this->exactly(2)) - ->method('getObjectType') + $n->method('getObjectType') ->willReturn('room'); $n->method('getObjectId') ->willReturn('roomToken'); @@ -329,8 +328,7 @@ public function getNotificationMock(string $parsedSubject, string $uid, string $ $n->expects($this->once()) ->method('getSubjectParameters') ->willReturn([$uid]); - $n->expects($this->exactly(2)) - ->method('getObjectType') + $n->method('getObjectType') ->willReturn('room'); $n->method('getObjectId') ->willReturn('roomToken'); @@ -461,8 +459,7 @@ public function testPrepareGroup($type, $uid, $displayName, $name, $parsedSubjec $n->expects($this->once()) ->method('getSubjectParameters') ->willReturn([$uid]); - $n->expects($this->exactly(2)) - ->method('getObjectType') + $n->method('getObjectType') ->willReturn('room'); $n->method('getObjectId') ->willReturn('roomToken'); @@ -1022,8 +1019,7 @@ public function testPrepareChatMessage(string $subject, int $roomType, array $su $notification->expects($this->once()) ->method('getSubjectParameters') ->willReturn($subjectParameters); - $notification->expects($this->exactly(2)) - ->method('getObjectType') + $notification->method('getObjectType') ->willReturn('chat'); $notification->method('getObjectId') ->willReturn('roomToken'); @@ -1144,11 +1140,10 @@ public function testPrepareThrows($message, $app, $isDisabledForUser, $validRoom $n->expects($this->never()) ->method('getObjectType'); } elseif ($objectType === null && $app === 'spreed') { - $n->expects($this->once()) - ->method('getObjectType') + $n->method('getObjectType') ->willReturn(''); } else { - $n->expects($this->exactly(2)) + $n->expects($this->any()) ->method('getObjectType') ->willReturn($objectType); } From 7ec84d056daa0a6550729550193a89acebe8a966 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 8 Jun 2022 10:45:20 +0200 Subject: [PATCH 6/7] Add integration test for accept and decline Signed-off-by: Joas Schilling --- .../features/bootstrap/FeatureContext.php | 1 + .../features/federation/invite.feature | 40 +++++++++++++++---- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index cd0fc0c0d00..00c52eb40c5 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -418,6 +418,7 @@ private function assertInvites($invites, TableNode $formData) { } protected function translateRemoteServer(string $server): string { + $server = str_replace('http://', '', $server); if ($server === 'localhost:8080') { return 'LOCAL'; } diff --git a/tests/integration/features/federation/invite.feature b/tests/integration/features/federation/invite.feature index 460d28daca8..317e8306cf3 100644 --- a/tests/integration/features/federation/invite.feature +++ b/tests/integration/features/federation/invite.feature @@ -3,7 +3,7 @@ Feature: federation/invite Given user "participant1" exists Given user "participant2" exists - Scenario: federation is disabled + Scenario: Federation is disabled Given the following "spreed" app config is set | federation_enabled | no | Given user "participant1" creates room "room" (v4) @@ -14,7 +14,7 @@ Feature: federation/invite | actorType | actorId | participantType | | users | participant1 | 1 | - Scenario: federation is enabled + Scenario: Accepting an invite Given the following "spreed" app config is set | federation_enabled | yes | Given user "participant1" creates room "room" (v4) @@ -29,8 +29,34 @@ Feature: federation/invite | remote_server | remote_token | | LOCAL | room | Then user "participant2" has the following notifications - | app | object_type | object_id | subject | - | spreed | remote_talk_share | INVITE_ID(LOCAL::room) | @participant1-displayname shared room room on localhost:8080 with you | -# And user "participant2" accepts invite to room "room" of server "LOCAL" (v1) -# And user "participant2" has the following invitations (v1) -# | remote_server | remote_token | + | app | object_type | object_id | subject | + | spreed | remote_talk_share | INVITE_ID(LOCAL::room) | @participant1-displayname shared room room on http://localhost:8080 with you | + And user "participant2" accepts invite to room "room" of server "LOCAL" (v1) + And user "participant2" has the following invitations (v1) + When user "participant1" sees the following attendees in room "room" with 200 (v4) + | actorType | actorId | participantType | + | users | participant1 | 1 | + | federated_users | participant2 | 3 | + + Scenario: Declining an invite + Given the following "spreed" app config is set + | federation_enabled | yes | + Given user "participant1" creates room "room" (v4) + | roomType | 3 | + | roomName | room | + And user "participant1" adds remote "participant2" to room "room" with 200 (v4) + When user "participant1" sees the following attendees in room "room" with 200 (v4) + | actorType | actorId | participantType | + | users | participant1 | 1 | + | federated_users | participant2 | 3 | + And user "participant2" has the following invitations (v1) + | remote_server | remote_token | + | LOCAL | room | + Then user "participant2" has the following notifications + | app | object_type | object_id | subject | + | spreed | remote_talk_share | INVITE_ID(LOCAL::room) | @participant1-displayname shared room room on http://localhost:8080 with you | + And user "participant2" declines invite to room "room" of server "LOCAL" (v1) + And user "participant2" has the following invitations (v1) + When user "participant1" sees the following attendees in room "room" with 200 (v4) + | actorType | actorId | participantType | + | users | participant1 | 1 | From be00d4e42c3df1778a1ab71bce78ec75f3ba5cda Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 8 Jun 2022 11:49:29 +0200 Subject: [PATCH 7/7] Add system messages for inviting, accepting and declining Signed-off-by: Joas Schilling --- lib/Chat/Parser/SystemMessage.php | 38 +++++++++++++++++++ lib/Chat/SystemMessage/Listener.php | 7 ++++ .../CloudFederationProviderTalk.php | 25 +++++++++++- .../features/bootstrap/FeatureContext.php | 2 +- .../features/federation/invite.feature | 18 +++++++++ tests/php/Chat/Parser/SystemMessageTest.php | 6 +++ tests/php/Federation/FederationTest.php | 4 ++ 7 files changed, 98 insertions(+), 2 deletions(-) diff --git a/lib/Chat/Parser/SystemMessage.php b/lib/Chat/Parser/SystemMessage.php index 68a794406f4..fa10556de64 100644 --- a/lib/Chat/Parser/SystemMessage.php +++ b/lib/Chat/Parser/SystemMessage.php @@ -34,6 +34,7 @@ use OCA\Talk\Room; use OCA\Talk\Share\RoomShareProvider; use OCP\Comments\IComment; +use OCP\Federation\ICloudIdManager; use OCP\Files\InvalidPathException; use OCP\Files\IRootFolder; use OCP\Files\Node; @@ -57,6 +58,7 @@ class SystemMessage { protected RoomShareProvider $shareProvider; protected PhotoCache $photoCache; protected IRootFolder $rootFolder; + protected ICloudIdManager $cloudIdManager; protected IURLGenerator $url; protected ?IL10N $l = null; @@ -80,6 +82,7 @@ public function __construct(IUserManager $userManager, RoomShareProvider $shareProvider, PhotoCache $photoCache, IRootFolder $rootFolder, + ICloudIdManager $cloudIdManager, IURLGenerator $url) { $this->userManager = $userManager; $this->groupManager = $groupManager; @@ -88,6 +91,7 @@ public function __construct(IUserManager $userManager, $this->shareProvider = $shareProvider; $this->photoCache = $photoCache; $this->rootFolder = $rootFolder; + $this->cloudIdManager = $cloudIdManager; $this->url = $url; } @@ -285,6 +289,26 @@ public function parseMessage(Message $chatMessage): void { $parsedMessage = $this->l->t('An administrator removed {user}'); } } + } elseif ($message === 'federated_user_added') { + $parsedParameters['federated_user'] = $this->getRemoteUser($parameters['federated_user']); + $parsedMessage = $this->l->t('{actor} invited {user}'); + if ($currentUserIsActor) { + $parsedMessage = $this->l->t('You invited {user}'); + } elseif ($cliIsActor) { + $parsedMessage = $this->l->t('An administrator invited {user}'); + } elseif ($parsedParameters['federated_user']['id'] === $parsedParameters['actor']['id']) { + $parsedMessage = $this->l->t('{federated_user} accepted the invitation'); + } + } elseif ($message === 'federated_user_removed') { + $parsedParameters['federated_user'] = $this->getRemoteUser($parameters['federated_user']); + $parsedMessage = $this->l->t('{actor} removed {federated_user}'); + if ($currentUserIsActor) { + $parsedMessage = $this->l->t('You removed {federated_user}'); + } elseif ($cliIsActor) { + $parsedMessage = $this->l->t('An administrator removed {federated_user}'); + } elseif ($parsedParameters['federated_user']['id'] === $parsedParameters['actor']['id']) { + $parsedMessage = $this->l->t('{federated_user} declined the invitation'); + } } elseif ($message === 'group_added') { $parsedParameters['group'] = $this->getGroup($parameters['group']); $parsedMessage = $this->l->t('{actor} added group {group}'); @@ -588,6 +612,9 @@ protected function getActor(Room $room, string $actorType, string $actorId): arr if ($actorType === Attendee::ACTOR_GUESTS) { return $this->getGuest($room, $actorId); } + if ($actorType === Attendee::ACTOR_FEDERATED_USERS) { + return $this->getRemoteUser($actorId); + } return $this->getUser($actorId); } @@ -616,6 +643,17 @@ protected function getUser(string $uid): array { ]; } + protected function getRemoteUser(string $federationId): array { + $cloudId = $this->cloudIdManager->resolveCloudId($federationId); + + return [ + 'type' => 'user', + 'id' => $cloudId->getUser(), + 'name' => $cloudId->getDisplayId(), + 'server' => $cloudId->getRemote(), + ]; + } + protected function getDisplayName(string $uid): string { $user = $this->userManager->get($uid); if ($user instanceof IUser) { diff --git a/lib/Chat/SystemMessage/Listener.php b/lib/Chat/SystemMessage/Listener.php index e0ceb60acec..b58b7d1751e 100644 --- a/lib/Chat/SystemMessage/Listener.php +++ b/lib/Chat/SystemMessage/Listener.php @@ -370,6 +370,8 @@ protected function attendeesAddedEvent(AttendeesAddedEvent $event): void { $this->sendSystemMessage($event->getRoom(), 'group_added', ['group' => $attendee->getActorId()]); } elseif ($attendee->getActorType() === Attendee::ACTOR_CIRCLES) { $this->sendSystemMessage($event->getRoom(), 'circle_added', ['circle' => $attendee->getActorId()]); + } elseif ($attendee->getActorType() === Attendee::ACTOR_FEDERATED_USERS) { + $this->sendSystemMessage($event->getRoom(), 'federated_user_added', ['federated_user' => $attendee->getActorId()]); } } } @@ -380,6 +382,8 @@ protected function attendeesRemovedEvent(AttendeesRemovedEvent $event): void { $this->sendSystemMessage($event->getRoom(), 'group_removed', ['group' => $attendee->getActorId()]); } elseif ($attendee->getActorType() === Attendee::ACTOR_CIRCLES) { $this->sendSystemMessage($event->getRoom(), 'circle_removed', ['circle' => $attendee->getActorId()]); + } elseif ($attendee->getActorType() === Attendee::ACTOR_FEDERATED_USERS) { + $this->sendSystemMessage($event->getRoom(), 'federated_user_removed', ['federated_user' => $attendee->getActorId()]); } } } @@ -399,6 +403,9 @@ protected function sendSystemMessage(Room $room, string $message, array $paramet } elseif ($this->session->exists('talk-overwrite-actor')) { $actorType = Attendee::ACTOR_USERS; $actorId = $this->session->get('talk-overwrite-actor'); + } elseif ($this->session->exists('talk-overwrite-actor-type')) { + $actorType = $this->session->get('talk-overwrite-actor-type'); + $actorId = $this->session->get('talk-overwrite-actor-id'); } else { $actorType = Attendee::ACTOR_GUESTS; $sessionId = $this->talkSession->getSessionForRoom($room->getToken()); diff --git a/lib/Federation/CloudFederationProviderTalk.php b/lib/Federation/CloudFederationProviderTalk.php index b46c0ea372d..9c1c0dd38dd 100644 --- a/lib/Federation/CloudFederationProviderTalk.php +++ b/lib/Federation/CloudFederationProviderTalk.php @@ -29,6 +29,7 @@ use OCA\FederatedFileSharing\AddressHandler; use OCA\Talk\AppInfo\Application; use OCA\Talk\Config; +use OCA\Talk\Events\AttendeesAddedEvent; use OCA\Talk\Manager; use OCA\Talk\Model\Attendee; use OCA\Talk\Model\AttendeeMapper; @@ -37,6 +38,7 @@ use OCA\Talk\Service\ParticipantService; use OCP\AppFramework\Http; use OCP\DB\Exception as DBException; +use OCP\EventDispatcher\IEventDispatcher; use OCP\Federation\Exceptions\ActionNotSupportedException; use OCP\Federation\Exceptions\AuthenticationFailedException; use OCP\Federation\Exceptions\BadRequestException; @@ -44,6 +46,7 @@ use OCP\Federation\ICloudFederationProvider; use OCP\Federation\ICloudFederationShare; use OCP\HintException; +use OCP\ISession; use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; @@ -69,6 +72,8 @@ class CloudFederationProviderTalk implements ICloudFederationProvider { private AttendeeMapper $attendeeMapper; private Manager $manager; + private ISession $session; + private IEventDispatcher $dispatcher; private LoggerInterface $logger; public function __construct( @@ -81,6 +86,8 @@ public function __construct( ParticipantService $participantService, AttendeeMapper $attendeeMapper, Manager $manager, + ISession $session, + IEventDispatcher $dispatcher, LoggerInterface $logger ) { $this->userManager = $userManager; @@ -92,6 +99,8 @@ public function __construct( $this->participantService = $participantService; $this->attendeeMapper = $attendeeMapper; $this->manager = $manager; + $this->session = $session; + $this->dispatcher = $dispatcher; $this->logger = $logger; } @@ -193,7 +202,15 @@ public function notificationReceived($notificationType, $providerId, array $noti private function shareAccepted(int $id, array $notification): array { $attendee = $this->getAttendeeAndValidate($id, $notification['sharedSecret']); - // TODO: Add activity for share accepted + $this->session->set('talk-overwrite-actor-type', $attendee->getActorType()); + $this->session->set('talk-overwrite-actor-id', $attendee->getActorId()); + + $room = $this->manager->getRoomById($attendee->getRoomId()); + $event = new AttendeesAddedEvent($room, [$attendee]); + $this->dispatcher->dispatchTyped($event); + + $this->session->remove('talk-overwrite-actor-type'); + $this->session->remove('talk-overwrite-actor-id'); return []; } @@ -206,9 +223,15 @@ private function shareAccepted(int $id, array $notification): array { private function shareDeclined(int $id, array $notification): array { $attendee = $this->getAttendeeAndValidate($id, $notification['sharedSecret']); + $this->session->set('talk-overwrite-actor-type', $attendee->getActorType()); + $this->session->set('talk-overwrite-actor-id', $attendee->getActorId()); + $room = $this->manager->getRoomById($attendee->getRoomId()); $participant = new Participant($room, $attendee, null); $this->participantService->removeAttendee($room, $participant, Room::PARTICIPANT_LEFT); + + $this->session->remove('talk-overwrite-actor-type'); + $this->session->remove('talk-overwrite-actor-id'); return []; } diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index 00c52eb40c5..ca7e6787450 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -1818,7 +1818,7 @@ public function userSeesTheFollowingSystemMessagesInRoom($user, $identifier, $st $data = [ 'room' => self::$tokenToIdentifier[$message['token']], 'actorType' => (string) $message['actorType'], - 'actorId' => ($message['actorType'] === 'guests') ? self::$sessionIdToUser[$message['actorId']]: (string) $message['actorId'], + 'actorId' => ($message['actorType'] === 'guests') ? self::$sessionIdToUser[$message['actorId']] : (string) $message['actorId'], 'systemMessage' => (string) $message['systemMessage'], ]; diff --git a/tests/integration/features/federation/invite.feature b/tests/integration/features/federation/invite.feature index 317e8306cf3..1ea48f73993 100644 --- a/tests/integration/features/federation/invite.feature +++ b/tests/integration/features/federation/invite.feature @@ -25,6 +25,10 @@ Feature: federation/invite | actorType | actorId | participantType | | users | participant1 | 1 | | federated_users | participant2 | 3 | + Then user "participant1" sees the following system messages in room "room" with 200 + | room | actorType | actorId | systemMessage | message | messageParameters | + | room | users | participant1 | federated_user_added | You invited {user} | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"},"federated_user":{"type":"user","id":"participant2","name":"participant2@localhost:8180","server":"http:\/\/localhost:8180"}} | + | room | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} | And user "participant2" has the following invitations (v1) | remote_server | remote_token | | LOCAL | room | @@ -37,6 +41,11 @@ Feature: federation/invite | actorType | actorId | participantType | | users | participant1 | 1 | | federated_users | participant2 | 3 | + Then user "participant1" sees the following system messages in room "room" with 200 + | room | actorType | actorId | systemMessage | message | messageParameters | + | room | federated_users | participant2@http://localhost:8180 | federated_user_added | {federated_user} accepted the invitation | {"actor":{"type":"user","id":"participant2","name":"participant2@localhost:8180","server":"http:\/\/localhost:8180"},"federated_user":{"type":"user","id":"participant2","name":"participant2@localhost:8180","server":"http:\/\/localhost:8180"}} | + | room | users | participant1 | federated_user_added | You invited {user} | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"},"federated_user":{"type":"user","id":"participant2","name":"participant2@localhost:8180","server":"http:\/\/localhost:8180"}} | + | room | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} | Scenario: Declining an invite Given the following "spreed" app config is set @@ -49,6 +58,10 @@ Feature: federation/invite | actorType | actorId | participantType | | users | participant1 | 1 | | federated_users | participant2 | 3 | + Then user "participant1" sees the following system messages in room "room" with 200 + | room | actorType | actorId | systemMessage | message | messageParameters | + | room | users | participant1 | federated_user_added | You invited {user} | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"},"federated_user":{"type":"user","id":"participant2","name":"participant2@localhost:8180","server":"http:\/\/localhost:8180"}} | + | room | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} | And user "participant2" has the following invitations (v1) | remote_server | remote_token | | LOCAL | room | @@ -60,3 +73,8 @@ Feature: federation/invite When user "participant1" sees the following attendees in room "room" with 200 (v4) | actorType | actorId | participantType | | users | participant1 | 1 | + Then user "participant1" sees the following system messages in room "room" with 200 + | room | actorType | actorId | systemMessage | message | messageParameters | + | room | federated_users | participant2@http://localhost:8180 | federated_user_removed | {federated_user} declined the invitation | {"actor":{"type":"user","id":"participant2","name":"participant2@localhost:8180","server":"http:\/\/localhost:8180"},"federated_user":{"type":"user","id":"participant2","name":"participant2@localhost:8180","server":"http:\/\/localhost:8180"}} | + | room | users | participant1 | federated_user_added | You invited {user} | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"},"federated_user":{"type":"user","id":"participant2","name":"participant2@localhost:8180","server":"http:\/\/localhost:8180"}} | + | room | users | participant1 | conversation_created | You created the conversation | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} | diff --git a/tests/php/Chat/Parser/SystemMessageTest.php b/tests/php/Chat/Parser/SystemMessageTest.php index 1fcf409e056..ad9daae63da 100644 --- a/tests/php/Chat/Parser/SystemMessageTest.php +++ b/tests/php/Chat/Parser/SystemMessageTest.php @@ -33,6 +33,7 @@ use OCA\Talk\Room; use OCA\Talk\Share\RoomShareProvider; use OCP\Comments\IComment; +use OCP\Federation\ICloudIdManager; use OCP\Files\Folder; use OCP\Files\InvalidPathException; use OCP\Files\IRootFolder; @@ -71,6 +72,8 @@ class SystemMessageTest extends TestCase { protected $rootFolder; /** @var IURLGenerator|MockObject */ protected $url; + /** @var ICloudIdManager|MockObject */ + protected $cloudIdManager; /** @var IL10N|MockObject */ protected $l; @@ -85,6 +88,7 @@ public function setUp(): void { $this->photoCache = $this->createMock(PhotoCache::class); $this->rootFolder = $this->createMock(IRootFolder::class); $this->url = $this->createMock(IURLGenerator::class); + $this->cloudIdManager = $this->createMock(ICloudIdManager::class); $this->l = $this->createMock(IL10N::class); $this->l->expects($this->any()) ->method('t') @@ -114,6 +118,7 @@ protected function getParser(array $methods = []): SystemMessage { $this->shareProvider, $this->photoCache, $this->rootFolder, + $this->cloudIdManager, $this->url, ]) ->onlyMethods($methods) @@ -129,6 +134,7 @@ protected function getParser(array $methods = []): SystemMessage { $this->shareProvider, $this->photoCache, $this->rootFolder, + $this->cloudIdManager, $this->url ); } diff --git a/tests/php/Federation/FederationTest.php b/tests/php/Federation/FederationTest.php index 709937c4775..0b6b6d14700 100644 --- a/tests/php/Federation/FederationTest.php +++ b/tests/php/Federation/FederationTest.php @@ -34,10 +34,12 @@ use OCA\Talk\Room; use OCA\Talk\Service\ParticipantService; use OCP\BackgroundJob\IJobList; +use OCP\EventDispatcher\IEventDispatcher; use OCP\Federation\ICloudFederationFactory; use OCP\Federation\ICloudFederationNotification; use OCP\Federation\ICloudFederationProviderManager; use OCP\Federation\ICloudFederationShare; +use OCP\ISession; use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; @@ -108,6 +110,8 @@ public function setUp(): void { $this->createMock(ParticipantService::class), $this->attendeeMapper, $this->createMock(Manager::class), + $this->createMock(ISession::class), + $this->createMock(IEventDispatcher::class), $this->logger ); }