fix(federation): Handle expected client errors well

Signed-off-by: Joas Schilling <coding@schilljs.com>

lib/Controller/AvatarController.php

@@ -29,7 +29,6 @@ namespace OCA\Talk\Controller;
 
 use InvalidArgumentException;
 use OCA\Talk\Exceptions\CannotReachRemoteException;
-use OCA\Talk\Exceptions\RemoteClientException;
 use OCA\Talk\Middleware\Attribute\FederationSupported;
 use OCA\Talk\Middleware\Attribute\RequireModeratorParticipant;
 use OCA\Talk\Middleware\Attribute\RequireParticipantOrLoggedInAndListedConversation;
@@ -144,7 +143,7 @@ class AvatarController extends AEnvironmentAwareController {
 			$proxy = \OCP\Server::get(\OCA\Talk\Federation\Proxy\TalkV1\Controller\AvatarController::class);
 			try {
 				return $proxy->getAvatar($this->room, $this->participant, $darkTheme);
-			} catch (RemoteClientException|CannotReachRemoteException) {
+			} catch (CannotReachRemoteException) {
 				// Falling back to a local "globe" avatar for indicating the federation
 			}
 		}

lib/Federation/Proxy/TalkV1/Controller/AvatarController.php

@@ -27,7 +27,6 @@ declare(strict_types=1);
 namespace OCA\Talk\Federation\Proxy\TalkV1\Controller;
 
 use OCA\Talk\Exceptions\CannotReachRemoteException;
-use OCA\Talk\Exceptions\RemoteClientException;
 use OCA\Talk\Federation\Proxy\TalkV1\ProxyRequest;
 use OCA\Talk\Participant;
 use OCA\Talk\ResponseDefinitions;
@@ -50,7 +49,6 @@ class AvatarController {
 	 * @see \OCA\Talk\Controller\AvatarController::getAvatar()
 	 *
 	 * @return FileDisplayResponse<Http::STATUS_OK, array{Content-Type: string}>
-	 * @throws RemoteClientException
 	 * @throws CannotReachRemoteException
 	 *
 	 * 200: Room avatar returned
@@ -64,6 +62,7 @@ class AvatarController {
 
 		if ($proxy->getStatusCode() !== Http::STATUS_OK) {
 			$this->proxy->logUnexpectedStatusCode(__METHOD__, $proxy->getStatusCode());
+			throw new CannotReachRemoteException('Avatar request had unexpected status code');
 		}
 
 		$content = $proxy->getBody();

lib/Federation/Proxy/TalkV1/Controller/ChatController.php

@@ -27,7 +27,6 @@ declare(strict_types=1);
 namespace OCA\Talk\Federation\Proxy\TalkV1\Controller;
 
 use OCA\Talk\Exceptions\CannotReachRemoteException;
-use OCA\Talk\Exceptions\RemoteClientException;
 use OCA\Talk\Federation\Proxy\TalkV1\ProxyRequest;
 use OCA\Talk\Federation\Proxy\TalkV1\UserConverter;
 use OCA\Talk\Participant;
@@ -52,7 +51,6 @@ class ChatController {
 	 *
 	 * @return DataResponse<Http::STATUS_CREATED, ?TalkChatMessageWithParent, array{X-Chat-Last-Common-Read?: numeric-string}>|DataResponse<Http::STATUS_BAD_REQUEST|Http::STATUS_NOT_FOUND|Http::STATUS_REQUEST_ENTITY_TOO_LARGE|Http::STATUS_TOO_MANY_REQUESTS, array<empty>, array{}>
 	 * @throws CannotReachRemoteException
-	 * @throws RemoteClientException
 	 *
 	 * 201: Message sent successfully
 	 * 400: Sending message is not possible
@@ -74,11 +72,9 @@ class ChatController {
 			],
 		);
 
-		/** @var Http::STATUS_CREATED|Http::STATUS_BAD_REQUEST|Http::STATUS_NOT_FOUND|Http::STATUS_REQUEST_ENTITY_TOO_LARGE|Http::STATUS_TOO_MANY_REQUESTS $statusCode */
 		$statusCode = $proxy->getStatusCode();
-
 		if ($statusCode !== Http::STATUS_CREATED) {
-			if (in_array($statusCode, [
+			if (!in_array($statusCode, [
 				Http::STATUS_BAD_REQUEST,
 				Http::STATUS_NOT_FOUND,
 				Http::STATUS_REQUEST_ENTITY_TOO_LARGE,
@@ -112,7 +108,6 @@ class ChatController {
 	/**
 	 * @return DataResponse<Http::STATUS_OK|Http::STATUS_NOT_MODIFIED, TalkChatMessageWithParent[], array{X-Chat-Last-Common-Read?: numeric-string, X-Chat-Last-Given?: numeric-string}>
 	 * @throws CannotReachRemoteException
-	 * @throws RemoteClientException
 	 *
 	 *  200: Messages returned
 	 *  304: No messages
@@ -158,7 +153,6 @@ class ChatController {
 			return new DataResponse([], Http::STATUS_NOT_MODIFIED);
 		}
 
-
 		$headers = [];
 		if ($proxy->getHeader('X-Chat-Last-Common-Read')) {
 			$headers['X-Chat-Last-Common-Read'] = (string) (int) $proxy->getHeader('X-Chat-Last-Common-Read');
@@ -179,7 +173,6 @@ class ChatController {
 	/**
 	 * @return DataResponse<Http::STATUS_OK|Http::STATUS_NOT_MODIFIED, TalkChatMessageWithParent[], array{X-Chat-Last-Common-Read?: numeric-string, X-Chat-Last-Given?: numeric-string}>
 	 * @throws CannotReachRemoteException
-	 * @throws RemoteClientException
 	 *
 	 * 200: Message context returned
 	 * 304: No messages
@@ -200,7 +193,6 @@ class ChatController {
 			return new DataResponse([], Http::STATUS_NOT_MODIFIED);
 		}
 
-
 		$headers = [];
 		if ($proxy->getHeader('X-Chat-Last-Common-Read')) {
 			$headers['X-Chat-Last-Common-Read'] = (string) (int) $proxy->getHeader('X-Chat-Last-Common-Read');
@@ -221,7 +213,6 @@ class ChatController {
 	/**
 	 * @return DataResponse<Http::STATUS_OK|Http::STATUS_ACCEPTED, TalkChatMessageWithParent, array{X-Chat-Last-Common-Read?: numeric-string}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>|DataResponse<Http::STATUS_FORBIDDEN|Http::STATUS_NOT_FOUND|Http::STATUS_METHOD_NOT_ALLOWED, array<empty>, array{}>
 	 * @throws CannotReachRemoteException
-	 * @throws RemoteClientException
 	 *
 	 * 200: Message edited successfully
 	 * 202: Message edited successfully, but a bot or Matterbridge is configured, so the information can be replicated to other services
@@ -281,7 +272,6 @@ class ChatController {
 	/**
 	 * @return DataResponse<Http::STATUS_OK|Http::STATUS_ACCEPTED, TalkChatMessageWithParent, array{X-Chat-Last-Common-Read?: numeric-string}>|DataResponse<Http::STATUS_BAD_REQUEST|Http::STATUS_FORBIDDEN|Http::STATUS_NOT_FOUND|Http::STATUS_METHOD_NOT_ALLOWED, array<empty>, array{}>
 	 * @throws CannotReachRemoteException
-	 * @throws RemoteClientException
 	 *
 	 * 200: Message deleted successfully
 	 * 202: Message deleted successfully, but a bot or Matterbridge is configured, so the information can be replicated elsewhere
@@ -340,7 +330,6 @@ class ChatController {
 	 *
 	 * @return DataResponse<Http::STATUS_OK, TalkChatMentionSuggestion[], array{}>
 	 * @throws CannotReachRemoteException
-	 * @throws RemoteClientException
 	 *
 	 * 200: List of mention suggestions returned
 	 */

lib/Federation/Proxy/TalkV1/Controller/RoomController.php

@@ -26,7 +26,6 @@ declare(strict_types=1);
 namespace OCA\Talk\Federation\Proxy\TalkV1\Controller;
 
 use OCA\Talk\Exceptions\CannotReachRemoteException;
-use OCA\Talk\Exceptions\RemoteClientException;
 use OCA\Talk\Federation\Proxy\TalkV1\ProxyRequest;
 use OCA\Talk\Federation\Proxy\TalkV1\UserConverter;
 use OCA\Talk\Participant;
@@ -53,7 +52,6 @@ class RoomController {
 	 * @param bool $includeStatus Include the user statuses
 	 * @return DataResponse<Http::STATUS_OK, TalkParticipant[], array{X-Nextcloud-Has-User-Statuses?: bool}>
 	 * @throws CannotReachRemoteException
-	 * @throws RemoteClientException
 	 *
 	 * 200: Participants returned
 	 * 403: Missing permissions for getting participants
@@ -87,7 +85,6 @@ class RoomController {
 	 *
 	 * @return DataResponse<Http::STATUS_OK, TalkCapabilities|array<empty>, array{X-Nextcloud-Talk-Hash: string}>
 	 * @throws CannotReachRemoteException
-	 * @throws RemoteClientException
 	 *
 	 * 200: Get capabilities successfully
 	 */

lib/Federation/Proxy/TalkV1/ProxyRequest.php

@@ -28,6 +28,7 @@ namespace OCA\Talk\Federation\Proxy\TalkV1;
 
 use GuzzleHttp\Exception\ClientException;
 use GuzzleHttp\Exception\ServerException;
+use OC\Http\Client\Response;
 use OCA\Talk\Exceptions\CannotReachRemoteException;
 use OCA\Talk\Exceptions\RemoteClientException;
 use OCP\AppFramework\Http;
@@ -77,9 +78,50 @@ class ProxyRequest {
 		];
 	}
 
+	/**
+	 * @param 'get'|'post'|'put'|'delete' $verb
+	 * @throws CannotReachRemoteException
+	 */
+	protected function request(
+		string $verb,
+		string $cloudId,
+		#[SensitiveParameter]
+		string $accessToken,
+		string $url,
+		array $parameters,
+	): IResponse {
+		$requestOptions = $this->generateDefaultRequestOptions($cloudId, $accessToken);
+		if (!empty($parameters)) {
+			$requestOptions['json'] = $parameters;
+		}
+
+		try {
+			return $this->clientService->newClient()->{$verb}($url, $requestOptions);
+		} catch (ClientException $e) {
+			$status = $e->getResponse()->getStatusCode();
+
+			try {
+				$body = $e->getResponse()->getBody()->getContents();
+				$data = json_decode($body, true, flags: JSON_THROW_ON_ERROR);
+				if (!is_array($data)) {
+					throw new \RuntimeException('JSON response is not an array');
+				}
+			} catch (\Throwable $e) {
+				throw new CannotReachRemoteException('Error parsing JSON response', $e->getCode(), $e);
+			}
+
+			$clientException = new RemoteClientException($e->getMessage(), $status, $e, $data);
+			$this->logger->debug('Client error from remote', ['exception' => $clientException]);
+			return new Response($e->getResponse(), false);
+		} catch (ServerException|\Throwable $e) {
+			$serverException = new CannotReachRemoteException($e->getMessage(), $e->getCode(), $e);
+			$this->logger->error('Could not reach remote', ['exception' => $serverException]);
+			throw $serverException;
+		}
+	}
+
 	/**
 	 * @throws CannotReachRemoteException
-	 * @throws RemoteClientException
 	 */
 	public function get(
 		string $cloudId,
@@ -88,39 +130,17 @@ class ProxyRequest {
 		string $url,
 		array $parameters = [],
 	): IResponse {
-		$requestOptions = $this->generateDefaultRequestOptions($cloudId, $accessToken);
-		if (!empty($parameters)) {
-			$requestOptions['json'] = $parameters;
-		}
-
-		try {
-			return $this->clientService->newClient()->get($url, $requestOptions);
-		} catch (ClientException $e) {
-			$status = $e->getResponse()->getStatusCode();
-
-			try {
-				$body = $e->getResponse()->getBody()->getContents();
-				$data = json_decode($body, true, flags: JSON_THROW_ON_ERROR);
-				if (!is_array($data)) {
-					throw new \RuntimeException('JSON response is not an array');
-				}
-			} catch (\Throwable $e) {
-				throw new CannotReachRemoteException('Error parsing JSON response', $e->getCode(), $e);
-			}
-
-			$clientException = new RemoteClientException($e->getMessage(), $status, $e, $data);
-			$this->logger->error('Client error from remote', ['exception' => $clientException]);
-			throw $clientException;
-		} catch (ServerException|\Throwable $e) {
-			$serverException = new CannotReachRemoteException($e->getMessage(), $e->getCode(), $e);
-			$this->logger->error('Could not reach remote', ['exception' => $serverException]);
-			throw $serverException;
-		}
+		return $this->request(
+			'get',
+			$cloudId,
+			$accessToken,
+			$url,
+			$parameters,
+		);
 	}
 
 	/**
 	 * @throws CannotReachRemoteException
-	 * @throws RemoteClientException
 	 */
 	public function put(
 		string $cloudId,
@@ -129,39 +149,17 @@ class ProxyRequest {
 		string $url,
 		array $parameters = [],
 	): IResponse {
-		$requestOptions = $this->generateDefaultRequestOptions($cloudId, $accessToken);
-		if (!empty($parameters)) {
-			$requestOptions['json'] = $parameters;
-		}
-
-		try {
-			return $this->clientService->newClient()->put($url, $requestOptions);
-		} catch (ClientException $e) {
-			$status = $e->getResponse()->getStatusCode();
-
-			try {
-				$body = $e->getResponse()->getBody()->getContents();
-				$data = json_decode($body, true, flags: JSON_THROW_ON_ERROR);
-				if (!is_array($data)) {
-					throw new \RuntimeException('JSON response is not an array');
-				}
-			} catch (\Throwable $e) {
-				throw new CannotReachRemoteException('Error parsing JSON response', $e->getCode(), $e);
-			}
-
-			$clientException = new RemoteClientException($e->getMessage(), $status, $e, $data);
-			$this->logger->error('Client error from remote', ['exception' => $clientException]);
-			throw $clientException;
-		} catch (ServerException|\Throwable $e) {
-			$serverException = new CannotReachRemoteException($e->getMessage(), $e->getCode(), $e);
-			$this->logger->error('Could not reach remote', ['exception' => $serverException]);
-			throw $serverException;
-		}
+		return $this->request(
+			'put',
+			$cloudId,
+			$accessToken,
+			$url,
+			$parameters,
+		);
 	}
 
 	/**
 	 * @throws CannotReachRemoteException
-	 * @throws RemoteClientException
 	 */
 	public function delete(
 		string $cloudId,
@@ -170,39 +168,17 @@ class ProxyRequest {
 		string $url,
 		array $parameters = [],
 	): IResponse {
-		$requestOptions = $this->generateDefaultRequestOptions($cloudId, $accessToken);
-		if (!empty($parameters)) {
-			$requestOptions['json'] = $parameters;
-		}
-
-		try {
-			return $this->clientService->newClient()->delete($url, $requestOptions);
-		} catch (ClientException $e) {
-			$status = $e->getResponse()->getStatusCode();
-
-			try {
-				$body = $e->getResponse()->getBody()->getContents();
-				$data = json_decode($body, true, flags: JSON_THROW_ON_ERROR);
-				if (!is_array($data)) {
-					throw new \RuntimeException('JSON response is not an array');
-				}
-			} catch (\Throwable $e) {
-				throw new CannotReachRemoteException('Error parsing JSON response', $e->getCode(), $e);
-			}
-
-			$clientException = new RemoteClientException($e->getMessage(), $status, $e, $data);
-			$this->logger->error('Client error from remote', ['exception' => $clientException]);
-			throw $clientException;
-		} catch (ServerException|\Throwable $e) {
-			$serverException = new CannotReachRemoteException($e->getMessage(), $e->getCode(), $e);
-			$this->logger->error('Could not reach remote', ['exception' => $serverException]);
-			throw $serverException;
-		}
+		return $this->request(
+			'delete',
+			$cloudId,
+			$accessToken,
+			$url,
+			$parameters,
+		);
 	}
 
 	/**
 	 * @throws CannotReachRemoteException
-	 * @throws RemoteClientException
 	 */
 	public function post(
 		string $cloudId,
@@ -211,34 +187,13 @@ class ProxyRequest {
 		string $url,
 		array $parameters = [],
 	): IResponse {
-		$requestOptions = $this->generateDefaultRequestOptions($cloudId, $accessToken);
-		if (!empty($parameters)) {
-			$requestOptions['json'] = $parameters;
-		}
-
-		try {
-			return $this->clientService->newClient()->post($url, $requestOptions);
-		} catch (ClientException $e) {
-			$status = $e->getResponse()->getStatusCode();
-
-			try {
-				$body = $e->getResponse()->getBody()->getContents();
-				$data = json_decode($body, true, flags: JSON_THROW_ON_ERROR);
-				if (!is_array($data)) {
-					throw new \RuntimeException('JSON response is not an array');
-				}
-			} catch (\Throwable $e) {
-				throw new CannotReachRemoteException('Error parsing JSON response', $e->getCode(), $e);
-			}
-
-			$clientException = new RemoteClientException($e->getMessage(), $status, $e, $data);
-			$this->logger->error('Client error from remote', ['exception' => $clientException]);
-			throw $clientException;
-		} catch (ServerException|\Throwable $e) {
-			$serverException = new CannotReachRemoteException($e->getMessage(), $e->getCode(), $e);
-			$this->logger->error('Could not reach remote', ['exception' => $serverException]);
-			throw $serverException;
-		}
+		return $this->request(
+			'post',
+			$cloudId,
+			$accessToken,
+			$url,
+			$parameters,
+		);
 	}
 
 	/**

tests/integration/features/bootstrap/FeatureContext.php

@@ -1946,6 +1946,11 @@ class FeatureContext implements Context, SnippetAcceptingContext {
 	public function userSendsMessageToRoom(string $user, string $sendingMode, string $message, string $identifier, string $statusCode, string $apiVersion = 'v1') {
 		$message = substr($message, 1, -1);
 		$message = str_replace('\n', "\n", $message);
+
+		if ($message === '413 Payload Too Large') {
+			$message .= "\n" . str_repeat('1', 32000);
+		}
+
 		if ($sendingMode === 'silent sends') {
 			$body = new TableNode([['message', $message], ['silent', true]]);
 		} else {

tests/integration/features/federation/chat.feature

@@ -125,3 +125,28 @@ Feature: federation/chat
       | room | actorType       | actorId                  | actorDisplayName         | message                   | messageParameters | parentMessage             |
       | room | users           | participant2             | participant2-displayname | Message deleted by author | {"actor":{"type":"user","id":"participant2","name":"participant2@localhost:8180","server":"http:\/\/localhost:8180"}}                | Message deleted by author |
       | room | federated_users | participant1@{$BASE_URL} | participant1-displayname | Message deleted by author | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}}                |                           |
+
+  Scenario: Error handling of chatting (posting a too long message)
+    Given the following "spreed" app config is set
+      | federation_enabled | yes |
+    Given user "participant1" creates room "room" (v4)
+      | roomType | 2 |
+      | roomName | room |
+    And user "participant1" adds federated_user "participant2" to room "room" with 200 (v4)
+    And user "participant1" adds federated_user "participant3" to room "room" with 200 (v4)
+    And user "participant2" has the following invitations (v1)
+      | remoteServerUrl | remoteToken | state | inviterCloudId                     | inviterDisplayName       |
+      | LOCAL           | room        | 0     | participant1@http://localhost:8080 | participant1-displayname |
+    And user "participant2" accepts invite to room "room" of server "LOCAL" with 200 (v1)
+      | id   | name | type | remoteServer | remoteToken |
+      | room | room | 2    | LOCAL        | room        |
+    And user "participant3" has the following invitations (v1)
+      | remoteServerUrl | remoteToken | state | inviterCloudId                     | inviterDisplayName       |
+      | LOCAL           | room        | 0     | participant1@http://localhost:8080 | participant1-displayname |
+    And user "participant3" accepts invite to room "room" of server "LOCAL" with 200 (v1)
+      | id   | name | type | remoteServer | remoteToken |
+      | room | room | 2    | LOCAL        | room        |
+    Then user "participant2" is participant of the following rooms (v4)
+      | id   | type |
+      | room | 2    |
+    And user "participant2" sends message "413 Payload Too Large" to room "LOCAL::room" with 413