Merge pull request #759 from nextcloud/backport/758/stable20v20.0.0

[stable20] Also check responses of 400 errors and so delete unknown devices

2 files changed, 96 insertions, 32 deletions

diff --git a/lib/Push.php b/lib/Push.php
index 00864ae..18efc57 100644
--- a/lib/Push.php
+++ b/lib/Push.php
@@ -21,6 +21,8 @@
 
 namespace OCA\Notifications;
 
+use GuzzleHttp\Exception\ClientException;
+use GuzzleHttp\Exception\ServerException;
 use OC\Authentication\Exceptions\InvalidTokenException;
 use OC\Authentication\Token\IProvider;
 use OC\Security\IdentityProof\Key;
@@ -284,44 +286,68 @@ class Push {
 						'notifications' => $notifications,
 					],
 				]);
-			} catch (\Exception $e) {
-				$this->log->logException($e, [
+			} catch (ClientException $e) {
+				// Server responded with 4xx (400 Bad Request mostlikely)
+				$response = $e->getResponse();
+			} catch (ServerException $e) {
+				// Server responded with 5xx
+				$response = $e->getResponse();
+				$body = $response->getBody();
+				$error = \is_string($body) ? $body : ('no reason given (' . $response->getStatusCode() . ')');
+
+				$this->log->debug('Could not send notification to push server [{url}]: {error}', [
+					'error' => $error,
+					'url' => $proxyServer,
 					'app' => 'notifications',
-					'level' => $e->getCode() === Http::STATUS_BAD_REQUEST ? ILogger::INFO : ILogger::WARN,
 				]);
-				continue;
-			}
 
-			$status = $response->getStatusCode();
-			if ($status === Http::STATUS_SERVICE_UNAVAILABLE && $this->config->getSystemValue('debug', false)) {
-				$body = $response->getBody();
-				$this->log->debug('Could not send notification to push server [{url}]: {error}',[
-					'error' => \is_string($body) ? $body : 'no reason given',
-					'url' => $proxyServer,
+				$this->printInfo('Could not send notification to push server [' . $proxyServer . ']: ' . $error);
+				continue;
+			} catch (\Exception $e) {
+				$this->log->logException($e, [
 					'app' => 'notifications',
+					'level' => ILogger::ERROR,
 				]);
+
+				$error = $e->getMessage() ?: 'no reason given';
+				$this->printInfo('Could not send notification to push server [' . get_class($e) . ']: ' . $error);
 				continue;
 			}
 
+			$status = $response->getStatusCode();
 			$body = $response->getBody();
 			$bodyData = json_decode($body, true);
-			if ($status !== Http::STATUS_OK) {
+
+			if (is_array($bodyData) && isset($bodyData['unknown'], $bodyData['failed'])) {
+				if (is_array($bodyData['unknown'])) {
+					// Proxy returns null when the array is empty
+					foreach ($bodyData['unknown'] as $unknownDevice) {
+						$this->printInfo('Deleting device because it is unknown by the push server: ' . $unknownDevice);
+						$this->deletePushTokenByDeviceIdentifier($unknownDevice);
+					}
+				}
+
+				if ($bodyData['failed'] !== 0) {
+					$this->printInfo('Push notification sent, but ' . $bodyData['failed'] . ' failed');
+				} else {
+					$this->printInfo('Push notification sent successfully');
+				}
+			} elseif ($status !== Http::STATUS_OK) {
 				$error = \is_string($body) && $bodyData === null ? $body : 'no reason given';
 				$this->printInfo('Could not send notification to push server [' . $proxyServer . ']: ' . $error);
-				$this->log->error('Could not send notification to push server [{url}]: {error}', [
+				$this->log->warning('Could not send notification to push server [{url}]: {error}', [
 					'error' => $error,
 					'url' => $proxyServer,
 					'app' => 'notifications',
 				]);
 			} else {
-				$this->printInfo('Push notification sent');
-			}
-
-			if (is_array($bodyData) && !empty($bodyData['unknown']) && is_array($bodyData['unknown'])) {
-				foreach ($bodyData['unknown'] as $unknownDevice) {
-					$this->printInfo('Deleting device because it is unknown by the push server: ' . $unknownDevice);
-					$this->deletePushTokenByDeviceIdentifier($unknownDevice);
-				}
+				$error = \is_string($body) && $bodyData === null ? $body : 'no reason given';
+				$this->printInfo('Push notification sent but response was not parsable, using an outdated push proxy? [' . $proxyServer . ']: ' . $error);
+				$this->log->info('Push notification sent but response was not parsable, using an outdated push proxy? [{url}]: {error}', [
+					'error' => $error,
+					'url' => $proxyServer,
+					'app' => 'notifications',
+				]);
 			}
 		}
 	}
diff --git a/tests/Unit/PushTest.php b/tests/Unit/PushTest.php
index 88d1efd..63497ba 100644
--- a/tests/Unit/PushTest.php
+++ b/tests/Unit/PushTest.php
@@ -22,6 +22,8 @@
 
 namespace OCA\Notifications\Tests\Unit;
 
+use GuzzleHttp\Exception\ClientException;
+use GuzzleHttp\Exception\ServerException;
 use OC\Authentication\Exceptions\InvalidTokenException;
 use OC\Authentication\Token\IProvider;
 use OC\Security\IdentityProof\Key;
@@ -403,7 +405,7 @@ class PushTest extends TestCase {
 	 * @param bool $isDebug
 	 */
 	public function testPushToDeviceSending($isDebug) {
-		$push = $this->getPush(['getDevicesForUser', 'encryptAndSign', 'deletePushToken', 'validateToken']);
+		$push = $this->getPush(['getDevicesForUser', 'encryptAndSign', 'deletePushToken', 'validateToken', 'deletePushTokenByDeviceIdentifier']);
 
 		/** @var INotification|MockObject $notification */
 		$notification = $this->createMock(INotification::class);
@@ -447,6 +449,11 @@ class PushTest extends TestCase {
 					'token' => 64,
 					'apptype' => 'other',
 				],
+				[
+					'proxyserver' => 'badrequest-with-devices',
+					'token' => 128,
+					'apptype' => 'other',
+				],
 			]);
 
 		$this->config
@@ -472,11 +479,11 @@ class PushTest extends TestCase {
 			->with($user)
 			->willReturn($key);
 
-		$push->expects($this->exactly(5))
+		$push->expects($this->exactly(6))
 			->method('validateToken')
 			->willReturn(true);
 
-		$push->expects($this->exactly(5))
+		$push->expects($this->exactly(6))
 			->method('encryptAndSign')
 			->willReturn(['Payload']);
 
@@ -509,7 +516,7 @@ class PushTest extends TestCase {
 			->method('logException')
 			->with($e, [
 				'app' => 'notifications',
-				'level' => ILogger::WARN,
+				'level' => ILogger::ERROR,
 			]);
 
 		/** @var IResponse|MockObject $response1 */
@@ -520,6 +527,9 @@ class PushTest extends TestCase {
 		$response1->expects($this->once())
 			->method('getBody')
 			->willReturn(null);
+		$e = $this->createMock(ClientException::class);
+		$e->method('getResponse')
+			->willReturn($response1);
 		$client->expects($this->at(1))
 			->method('post')
 			->with('badrequest/notifications', [
@@ -527,10 +537,10 @@ class PushTest extends TestCase {
 					'notifications' => ['["Payload"]'],
 				],
 			])
-			->willReturn($response1);
+			->willThrowException($e);
 
 		$this->logger->expects($this->at(1))
-			->method('error')
+			->method('warning')
 			->with('Could not send notification to push server [{url}]: {error}', [
 				'error' => 'no reason given',
 				'url' => 'badrequest',
@@ -540,11 +550,11 @@ class PushTest extends TestCase {
 		/** @var IResponse|MockObject $response1 */
 		$response2 = $this->createMock(IResponse::class);
 		$response2->expects($this->once())
-			->method('getStatusCode')
-			->willReturn(Http::STATUS_SERVICE_UNAVAILABLE);
-		$response2->expects($this->once())
 			->method('getBody')
 			->willReturn('Maintenance');
+		$e = $this->createMock(ServerException::class);
+		$e->method('getResponse')
+			->willReturn($response2);
 		$client->expects($this->at(2))
 			->method('post')
 			->with('unavailable/notifications', [
@@ -552,9 +562,9 @@ class PushTest extends TestCase {
 					'notifications' => ['["Payload"]'],
 				],
 			])
-			->willReturn($response2);
+			->willThrowException($e);
 
-		$this->logger->expects($isDebug ? $this->at(2) : $this->never())
+		$this->logger->expects($this->at(2))
 			->method('debug')
 			->with('Could not send notification to push server [{url}]: {error}', [
 				'error' => 'Maintenance',
@@ -576,6 +586,34 @@ class PushTest extends TestCase {
 			])
 			->willReturn($response3);
 
+		/** @var IResponse|MockObject $response1 */
+		$response4 = $this->createMock(IResponse::class);
+		$response4->expects($this->once())
+			->method('getStatusCode')
+			->willReturn(Http::STATUS_BAD_REQUEST);
+		$response4->expects($this->once())
+			->method('getBody')
+			->willReturn(json_encode([
+				'failed' => 1,
+				'unknown' => [
+					'123456'
+				]
+			]));
+		$e = $this->createMock(ClientException::class);
+		$e->method('getResponse')
+			->willReturn($response4);
+		$client->expects($this->at(4))
+			->method('post')
+			->with('badrequest-with-devices/notifications', [
+				'body' => [
+					'notifications' => ['["Payload"]'],
+				],
+			])
+			->willThrowException($e);
+
+		$push->method('deletePushTokenByDeviceIdentifier')
+			->with('123456');
+
 		$push->pushToDevice(207787, $notification);
 	}