Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(bot): Allow reactions by bots #10138

Merged
merged 1 commit into from
Aug 8, 2023
Merged

feat(bot): Allow reactions by bots #10138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions appinfo/routes/routesBotController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@
'token' => '[a-z0-9]{4,30}',
];

$requirementsWithMessageId = [
'apiVersion' => 'v1',
'token' => '[a-z0-9]{4,30}',
'messageId' => '[0-9]+',
];

$requirementsWithBotId = [
'apiVersion' => 'v1',
'token' => '[a-z0-9]{4,30}',
Expand All @@ -38,6 +44,10 @@
'ocs' => [
/** @see \OCA\Talk\Controller\BotController::sendMessage() */
['name' => 'Bot#sendMessage', 'url' => '/api/{apiVersion}/bot/{token}/message', 'verb' => 'POST', 'requirements' => $requirements],
/** @see \OCA\Talk\Controller\BotController::react() */
['name' => 'Bot#react', 'url' => '/api/{apiVersion}/bot/{token}/reaction/{messageId}', 'verb' => 'POST', 'requirements' => $requirementsWithMessageId],
/** @see \OCA\Talk\Controller\BotController::deleteReaction() */
['name' => 'Bot#deleteReaction', 'url' => '/api/{apiVersion}/bot/{token}/reaction/{messageId}', 'verb' => 'DELETE', 'requirements' => $requirementsWithMessageId],
/** @see \OCA\Talk\Controller\BotController::listBots() */
['name' => 'Bot#listBots', 'url' => '/api/{apiVersion}/bot/{token}', 'verb' => 'GET', 'requirements' => $requirements],
/** @see \OCA\Talk\Controller\BotController::enableBot() */
Expand Down
30 changes: 16 additions & 14 deletions lib/Chat/ReactionManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ public function __construct(
* Add reaction
*
* @param Room $chat
* @param Participant $participant
* @param string $actorType
* @param string $actorId
* @param integer $messageId
* @param string $reaction
* @return IComment
Expand All @@ -60,23 +61,23 @@ public function __construct(
* @throws ReactionNotSupportedException
* @throws ReactionOutOfContextException
*/
public function addReactionMessage(Room $chat, Participant $participant, int $messageId, string $reaction): IComment {
public function addReactionMessage(Room $chat, string $actorType, string $actorId, int $messageId, string $reaction): IComment {
$parentMessage = $this->getCommentToReact($chat, (string) $messageId);
try {
// Check if the user already reacted with the same reaction
$this->commentsManager->getReactionComment(
(int) $parentMessage->getId(),
$participant->getAttendee()->getActorType(),
$participant->getAttendee()->getActorId(),
$actorType,
$actorId,
$reaction
);
throw new ReactionAlreadyExistsException();
} catch (NotFoundException $e) {
}

$comment = $this->commentsManager->create(
$participant->getAttendee()->getActorType(),
$participant->getAttendee()->getActorId(),
$actorType,
$actorId,
'chat',
(string) $chat->getId()
);
Expand All @@ -93,28 +94,29 @@ public function addReactionMessage(Room $chat, Participant $participant, int $me
* Delete reaction
*
* @param Room $chat
* @param Participant $participant
* @param string $actorType
* @param string $actorId
* @param integer $messageId
* @param string $reaction
* @return IComment
* @throws NotFoundException
* @throws ReactionNotSupportedException
* @throws ReactionOutOfContextException
*/
public function deleteReactionMessage(Room $chat, Participant $participant, int $messageId, string $reaction): IComment {
public function deleteReactionMessage(Room $chat, string $actorType, string $actorId, int $messageId, string $reaction): IComment {
// Just to verify that messageId is part of the room and throw error if not.
$this->getCommentToReact($chat, (string) $messageId);

$comment = $this->commentsManager->getReactionComment(
$messageId,
$participant->getAttendee()->getActorType(),
$participant->getAttendee()->getActorId(),
$actorType,
$actorId,
$reaction
);
$comment->setMessage(
json_encode([
'deleted_by_type' => $participant->getAttendee()->getActorType(),
'deleted_by_id' => $participant->getAttendee()->getActorId(),
'deleted_by_type' => $actorType,
'deleted_by_id' => $actorId,
'deleted_on' => $this->timeFactory->getDateTime()->getTimestamp(),
])
);
Expand All @@ -123,8 +125,8 @@ public function deleteReactionMessage(Room $chat, Participant $participant, int

$this->chatManager->addSystemMessage(
$chat,
$participant->getAttendee()->getActorType(),
$participant->getAttendee()->getActorId(),
$actorType,
$actorId,
json_encode(['message' => 'reaction_revoked', 'parameters' => ['message' => (int) $comment->getId()]]),
$this->timeFactory->getDateTime(),
false,
Expand Down
139 changes: 114 additions & 25 deletions lib/Controller/BotController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@
namespace OCA\Talk\Controller;

use OCA\Talk\Chat\ChatManager;
use OCA\Talk\Chat\ReactionManager;
use OCA\Talk\Exceptions\ReactionAlreadyExistsException;
use OCA\Talk\Exceptions\ReactionNotSupportedException;
use OCA\Talk\Exceptions\ReactionOutOfContextException;
use OCA\Talk\Exceptions\UnauthorizedException;
use OCA\Talk\Manager;
use OCA\Talk\Middleware\Attribute\RequireLoggedInModeratorParticipant;
Expand Down Expand Up @@ -63,42 +67,31 @@ public function __construct(
protected BotServerMapper $botServerMapper,
protected BotService $botService,
protected Manager $manager,
protected ReactionManager $reactionManager,
protected LoggerInterface $logger,
) {
parent::__construct($appName, $request);
}

/**
* Sends a new chat message to the given room.
*
* The author and timestamp are automatically set to the current user/guest
* and time.
*
* @param string $token conversation token
* @param string $message the message to send
* @param string $referenceId for the message to be able to later identify it again
* @param int $replyTo Parent id which this message is a reply to
* @param bool $silent If sent silent the chat message will not create any notifications
* @return DataResponse the status code is "201 Created" if successful, and
* "404 Not found" if the room or session for a guest user was not
* found".
* @param string $token
* @param string $message
* @return Bot
* @throws \InvalidArgumentException When the request could not be linked with a bot
*/
#[BruteForceProtection(action: 'bot')]
#[PublicPage]
public function sendMessage(string $token, string $message, string $referenceId = '', int $replyTo = 0, bool $silent = false): DataResponse {
protected function getBotFromHeaders(string $token, string $message): Bot {
$random = $this->request->getHeader('X-Nextcloud-Talk-Bot-Random');
if (empty($random) || strlen($random) < 32) {
$this->logger->error('Invalid Random received from bot response');
return new DataResponse([], Http::STATUS_BAD_REQUEST);
throw new \InvalidArgumentException('Invalid Random received from bot response', Http::STATUS_BAD_REQUEST);
}
$checksum = $this->request->getHeader('X-Nextcloud-Talk-Bot-Signature');
if (empty($checksum)) {
$this->logger->error('Invalid Signature received from bot response');
return new DataResponse([], Http::STATUS_BAD_REQUEST);
throw new \InvalidArgumentException('Invalid Signature received from bot response', Http::STATUS_BAD_REQUEST);
}

$bots = $this->botService->getBotsForToken($token);
$bot = null;
foreach ($bots as $botAttempt) {
try {
$this->checksumVerificationService->validateRequest(
Expand All @@ -107,16 +100,40 @@ public function sendMessage(string $token, string $message, string $referenceId
$botAttempt->getBotServer()->getSecret(),
$message
);
$bot = $botAttempt;
break;
return $botAttempt;
} catch (UnauthorizedException) {
}
}

if (!$bot instanceof Bot) {
$this->logger->debug('No valid Bot entry found');
$response = new DataResponse([], Http::STATUS_UNAUTHORIZED);
$response->throttle(['action' => 'bot']);
$this->logger->debug('No valid Bot entry found');
nickvergessen marked this conversation as resolved.
Show resolved Hide resolved
throw new \InvalidArgumentException('No valid Bot entry found', Http::STATUS_UNAUTHORIZED);
}

/**
* Sends a new chat message to the given room.
*
* The author and timestamp are automatically set to the current user/guest
* and time.
*
* @param string $token conversation token
* @param string $message the message to send
* @param string $referenceId for the message to be able to later identify it again
* @param int $replyTo Parent id which this message is a reply to
* @param bool $silent If sent silent the chat message will not create any notifications
* @return DataResponse the status code is "201 Created" if successful, and
* "404 Not found" if the room or session for a guest user was not
* found".
*/
#[BruteForceProtection(action: 'bot')]
#[PublicPage]
public function sendMessage(string $token, string $message, string $referenceId = '', int $replyTo = 0, bool $silent = false): DataResponse {
try {
$bot = $this->getBotFromHeaders($token, $message);
} catch (\InvalidArgumentException $e) {
$response = new DataResponse([], $e->getCode());
if ($e->getCode() === Http::STATUS_UNAUTHORIZED) {
$response->throttle(['action' => 'bot']);
}
return $response;
Comment on lines +133 to 137
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am looking for a way to avoid repetition here and keep the same logic in one place. Maybe a wrapper like this:

private function handleInvalidArgumentException(\InvalidArgumentException $e) {
	$response = new DataResponse([], $e->getCode());
	if ($e->getCode() === Http::STATUS_UNAUTHORIZED) {
		$response->throttle(['action' => 'bot']);
	}
	return $response;
}

Block from l. 130 to l. 138 is used several times. I wonder we could extract it in a method, but then we will have something like:

$bot = $this->loadBot($token, $message);
if ($bot instanceof DataResponse) {
	return $bot;
}

And I don't like it ^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I don't like mixed return types, that's why I went for the throwing instead.

}

Expand Down Expand Up @@ -149,6 +166,78 @@ public function sendMessage(string $token, string $message, string $referenceId
return new DataResponse([], Http::STATUS_CREATED);
}

#[BruteForceProtection(action: 'bot')]
#[PublicPage]
public function react(string $token, int $messageId, string $reaction): DataResponse {
try {
$bot = $this->getBotFromHeaders($token, $reaction);
} catch (\InvalidArgumentException $e) {
$response = new DataResponse([], $e->getCode());
if ($e->getCode() === Http::STATUS_UNAUTHORIZED) {
$response->throttle(['action' => 'bot']);
}
return $response;
}

$room = $this->manager->getRoomByToken($token);

$actorType = Attendee::ACTOR_BOTS;
$actorId = Attendee::ACTOR_BOT_PREFIX . $bot->getBotServer()->getUrlHash();

try {
$this->reactionManager->addReactionMessage(
$room,
$actorType,
$actorId,
$messageId,
$reaction
);
} catch (NotFoundException) {
return new DataResponse([], Http::STATUS_NOT_FOUND);
} catch (ReactionAlreadyExistsException) {
return new DataResponse([], Http::STATUS_OK);
} catch (ReactionNotSupportedException | ReactionOutOfContextException | \Exception) {
return new DataResponse([], Http::STATUS_BAD_REQUEST);
}

return new DataResponse([], Http::STATUS_CREATED);
}

#[BruteForceProtection(action: 'bot')]
#[PublicPage]
public function deleteReaction(string $token, int $messageId, string $reaction): DataResponse {
try {
$bot = $this->getBotFromHeaders($token, $reaction);
} catch (\InvalidArgumentException $e) {
$response = new DataResponse([], $e->getCode());
if ($e->getCode() === Http::STATUS_UNAUTHORIZED) {
$response->throttle(['action' => 'bot']);
}
return $response;
}

$room = $this->manager->getRoomByToken($token);

$actorType = Attendee::ACTOR_BOTS;
$actorId = Attendee::ACTOR_BOT_PREFIX . $bot->getBotServer()->getUrlHash();

try {
$this->reactionManager->deleteReactionMessage(
$room,
$actorType,
$actorId,
$messageId,
$reaction
);
} catch (ReactionNotSupportedException | ReactionOutOfContextException | NotFoundException) {
return new DataResponse([], Http::STATUS_NOT_FOUND);
} catch (\Exception) {
nickvergessen marked this conversation as resolved.
Show resolved Hide resolved
return new DataResponse([], Http::STATUS_BAD_REQUEST);
}

return new DataResponse([], Http::STATUS_OK);
}

#[NoAdminRequired]
#[RequireLoggedInModeratorParticipant]
public function listBots(): DataResponse {
Expand Down
6 changes: 4 additions & 2 deletions lib/Controller/ReactionController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ public function react(int $messageId, string $reaction): DataResponse {
try {
$this->reactionManager->addReactionMessage(
$this->getRoom(),
$this->getParticipant(),
$this->getParticipant()->getAttendee()->getActorType(),
$this->getParticipant()->getAttendee()->getActorId(),
$messageId,
$reaction
);
Expand All @@ -83,7 +84,8 @@ public function delete(int $messageId, string $reaction): DataResponse {
try {
$this->reactionManager->deleteReactionMessage(
$this->getRoom(),
$this->getParticipant(),
$this->getParticipant()->getAttendee()->getActorType(),
$this->getParticipant()->getAttendee()->getActorId(),
$messageId,
$reaction
);
Expand Down
9 changes: 9 additions & 0 deletions tests/integration/features/bootstrap/FeatureContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3191,6 +3191,7 @@ public function userReactWithOnMessageToRoomWith(string $user, string $action, s
* @Given /^user "([^"]*)" retrieve reactions "([^"]*)" of message "([^"]*)" in room "([^"]*)" with (\d+)(?: \((v1)\))?$/
*/
public function userRetrieveReactionsOfMessageInRoomWith(string $user, string $reaction, string $message, string $identifier, int $statusCode, string $apiVersion = 'v1', ?TableNode $formData = null): void {
$message = str_replace('\n', "\n", $message);
$token = self::$identifierToToken[$identifier];
$messageId = self::$textToMessageId[$message];
$this->setCurrentUser($user);
Expand All @@ -3208,6 +3209,14 @@ private function assertReactionList(?TableNode $formData): void {
foreach ($formData->getHash() as $row) {
$reaction = $row['reaction'];
unset($row['reaction']);

if ($row['actorType'] === 'bots') {
$result = preg_match('/BOT\(([^)]+)\)/', $row['actorId'], $matches);
if ($result && isset(self::$botNameToHash[$matches[1]])) {
$row['actorId'] = 'bot-' . self::$botNameToHash[$matches[1]];
}
}

$expected[$reaction][] = $row;
}

Expand Down
8 changes: 8 additions & 0 deletions tests/integration/features/chat/bots.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ Feature: chat/bots
| room | users | participant1 | participant1-displayname | - Task 2\n-Task 3 | [] |
| room | users | participant1 | participant1-displayname | * Task 1 | [] |
| room | users | participant1 | participant1-displayname | - Before call | [] |
Then user "participant1" retrieve reactions "👍" of message "- Before call" in room "room" with 200
| actorType | actorId | actorDisplayName | reaction |
Then user "participant1" retrieve reactions "👍" of message "* Task 1" in room "room" with 200
| actorType | actorId | actorDisplayName | reaction |
| bots | BOT(Call summary) | Call summary (Bot) | 👍 |
Then user "participant1" retrieve reactions "👍" of message "- Task 2\n-Task 3" in room "room" with 200
| actorType | actorId | actorDisplayName | reaction |
| bots | BOT(Call summary) | Call summary (Bot) | 👍 |

# Different states bot
# Already enabled
Expand Down
Loading