fix(federation): Handle federation errors so we have better logs

Signed-off-by: Joas Schilling <coding@schilljs.com>
nextcloud · Feb 29, 2024 · 2f70ca1 · 2f70ca1
1 parent fcffe85
commit 2f70ca1
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 31 deletions.
diff --git a/lib/BackgroundJob/RetryJob.php b/lib/BackgroundJob/RetryJob.php
@@ -76,7 +76,7 @@ public function execute(IJobList $jobList, ?ILogger $logger = null): void {
 
 	protected function run($argument): void {
 		$remote = $argument['remote'];
-		$data = json_decode($argument['data'], true);
+		$data = json_decode($argument['data'], true, flags: JSON_THROW_ON_ERROR);
 		$try = (int)$argument['try'] + 1;
 
 		$this->backendNotifier->sendUpdateDataToRemote($remote, $data, $try);

diff --git a/lib/Federation/BackendNotifier.php b/lib/Federation/BackendNotifier.php
@@ -33,6 +33,7 @@
 use OCA\Talk\Model\Attendee;
 use OCA\Talk\Room;
 use OCP\App\IAppManager;
+use OCP\AppFramework\Http;
 use OCP\AppFramework\Services\IAppConfig;
 use OCP\BackgroundJob\IJobList;
 use OCP\DB\Exception;
@@ -43,6 +44,7 @@
 use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\IUserManager;
+use OCP\OCM\Exceptions\OCMProviderException;
 use OCP\Server;
 use Psr\Log\LoggerInterface;
 use SensitiveParameter;
@@ -146,13 +148,22 @@ public function sendRemoteShare(
 		$protocol['name'] = FederationManager::TALK_PROTOCOL_NAME;
 		$share->setProtocol($protocol);
 
-		$response = $this->federationProviderManager->sendShare($share);
-		if (is_array($response)) {
-			return true;
-		}
-		$this->logger->info("Failed sharing $roomToken with $shareWith");
+		try {
+			$response = $this->federationProviderManager->sendCloudShare($share);
+			if ($response->getStatusCode() === Http::STATUS_CREATED) {
+				return true;
+			}
+
+			$this->logger->warning("Failed sharing $roomToken with $shareWith, received status code {code}\n{body}", [
+				'code' => $response->getStatusCode(),
+				'body' => (string) $response->getBody(),
+			]);
 
-		return false;
+			return false;
+		} catch (OCMProviderException $e) {
+			$this->logger->error("Failed sharing $roomToken with $shareWith, received OCMProviderException", ['exception' => $e]);
+			return false;
+		}
 	}
 
 	/**
@@ -178,13 +189,25 @@ public function sendShareAccepted(
 				'remoteServerUrl' => $this->getServerRemoteUrl(),
 				'sharedSecret' => $accessToken,
 				'message' => 'Recipient accepted the share',
+			]
+		);
+
+		try {
+			$response = $this->federationProviderManager->sendCloudNotification($remote, $notification);
+			if ($response->getStatusCode() === Http::STATUS_CREATED) {
+				return true;
+			}
+
+			$this->logger->warning("Failed to send share accepted notification for share from $remote, received status code {code}\n{body}", [
+				'code' => $response->getStatusCode(),
+				'body' => (string) $response->getBody(),
 			]);
-		$response = $this->federationProviderManager->sendNotification($remote, $notification);
-		if (!is_array($response)) {
-			$this->logger->info("Failed to send share accepted notification for share from $remote");
+
+			return false;
+		} catch (OCMProviderException $e) {
+			$this->logger->error("Failed to send share accepted notification for share from $remote, received OCMProviderException", ['exception' => $e]);
 			return false;
 		}
-		return true;
 	}
 
 	/**
@@ -210,12 +233,23 @@ public function sendShareDeclined(
 				'message' => 'Recipient declined the share',
 			]
 		);
-		$response = $this->federationProviderManager->sendNotification($remote, $notification);
-		if (!is_array($response)) {
-			$this->logger->info("Failed to send share declined notification for share from $remote");
+
+		try {
+			$response = $this->federationProviderManager->sendCloudNotification($remote, $notification);
+			if ($response->getStatusCode() === Http::STATUS_CREATED) {
+				return true;
+			}
+
+			$this->logger->warning("Failed to send share declined notification for share from $remote, received status code {code}\n{body}", [
+				'code' => $response->getStatusCode(),
+				'body' => (string) $response->getBody(),
+			]);
+
+			return false;
+		} catch (OCMProviderException $e) {
+			$this->logger->error("Failed to send share declined notification for share from $remote, received OCMProviderException", ['exception' => $e]);
 			return false;
 		}
-		return true;
 	}
 
 	public function sendRemoteUnShare(
@@ -328,18 +362,28 @@ public function sendUpdateDataToRemote(string $remote, array $data, int $try): v
 	}
 
 	protected function sendUpdateToRemote(string $remote, ICloudFederationNotification $notification, int $try = 0): void {
-		\OC::$server->getLogger()->error('sendUpdateToRemote');
-		\OC::$server->getLogger()->error(json_encode($notification->getMessage()));
-		$response = $this->federationProviderManager->sendNotification($remote, $notification);
-		if (!is_array($response)) {
-			$this->jobList->add(RetryJob::class,
-				[
-					'remote' => $remote,
-					'data' => json_encode($notification->getMessage()),
-					'try' => $try,
-				]
-			);
+		try {
+			$response = $this->federationProviderManager->sendCloudNotification($remote, $notification);
+			if ($response->getStatusCode() === Http::STATUS_CREATED) {
+				return;
+			}
+
+			$this->logger->warning("Failed to send notification for share from $remote, received status code {code}\n{body}", [
+				'code' => $response->getStatusCode(),
+				'body' => (string) $response->getBody(),
+			]);
+		} catch (OCMProviderException $e) {
+			$this->logger->error("Failed to send notification for share from $remote, received OCMProviderException", ['exception' => $e]);
 		}
+
+		$this->jobList->add(
+			RetryJob::class,
+			[
+				'remote' => $remote,
+				'data' => json_encode($notification->getMessage(), JSON_THROW_ON_ERROR),
+				'try' => $try,
+			]
+		);
 	}
 
 	protected function prepareRemoteUrl(string $remote): string {

diff --git a/tests/php/Federation/FederationTest.php b/tests/php/Federation/FederationTest.php
@@ -38,6 +38,7 @@
 use OCA\Talk\Service\ParticipantService;
 use OCA\Talk\Service\RoomService;
 use OCP\App\IAppManager;
+use OCP\AppFramework\Http;
 use OCP\AppFramework\Services\IAppConfig;
 use OCP\BackgroundJob\IJobList;
 use OCP\EventDispatcher\IEventDispatcher;
@@ -46,7 +47,9 @@
 use OCP\Federation\ICloudFederationProviderManager;
 use OCP\Federation\ICloudFederationShare;
 use OCP\Federation\ICloudIdManager;
+use OCP\Http\Client\IResponse;
 use OCP\ICacheFactory;
+use OCP\IRequest;
 use OCP\ISession;
 use OCP\IURLGenerator;
 use OCP\IUser;
@@ -227,7 +230,7 @@ public function testSendRemoteShareWithOwner() {
 			->willReturn($cloudShare);
 
 		$this->cloudFederationProviderManager->expects($this->once())
-			->method('sendShare')
+			->method('sendCloudShare')
 			->with($cloudShare);
 
 		$this->addressHandler->expects($this->once())
@@ -396,10 +399,13 @@ public function testSendAcceptNotification() {
 			->with()
 			->willReturn($notification);
 
+		$response = $this->createMock(IResponse::class);
+		$response->method('getStatusCode')
+			->willReturn(Http::STATUS_CREATED);
 		$this->cloudFederationProviderManager->expects($this->once())
-			->method('sendNotification')
+			->method('sendCloudNotification')
 			->with($remote, $notification)
-			->willReturn([]);
+			->willReturn($response);
 
 		$this->addressHandler->method('urlContainProtocol')
 			->with($remote)
@@ -438,10 +444,13 @@ public function testSendRejectNotification() {
 			->with()
 			->willReturn($notification);
 
+		$response = $this->createMock(IResponse::class);
+		$response->method('getStatusCode')
+			->willReturn(Http::STATUS_CREATED);
 		$this->cloudFederationProviderManager->expects($this->once())
-			->method('sendNotification')
+			->method('sendCloudNotification')
 			->with($remote, $notification)
-			->willReturn([]);
+			->willReturn($response);
 
 		$this->addressHandler->method('urlContainProtocol')
 			->with($remote)