Merge pull request #761 from nextcloud/backport/758/stable18v18.0.10RC2v18.0.10RC1v18.0.10

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

2 files changed, 42 insertions, 25 deletions

diff --git a/lib/Push.php b/lib/Push.php
index b2114c8..e389d95 100644
--- a/lib/Push.php
+++ b/lib/Push.php
@@ -22,6 +22,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;
@@ -189,40 +191,56 @@ class Push {
 						'notifications' => $notifications,
 					],
 				]);
+			} 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',
+				]);
+
+				continue;
 			} catch (\Exception $e) {
 				$this->log->logException($e, [
 					'app' => 'notifications',
-					'level' => $e->getCode() === Http::STATUS_BAD_REQUEST ? ILogger::INFO : ILogger::WARN,
+					'level' => ILogger::ERROR,
 				]);
 				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',
+			$body = $response->getBody();
+			$bodyData = json_decode($body, true);
+
+			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->deletePushTokenByDeviceIdentifier($unknownDevice);
+					}
+				}
+			} elseif ($status !== Http::STATUS_OK) {
+				$error = \is_string($body) && $bodyData === null ? $body : 'no reason given';
+				$this->log->warning('Could not send notification to push server [{url}]: {error}', [
+					'error' => $error,
 					'url' => $proxyServer,
 					'app' => 'notifications',
 				]);
-				continue;
-			}
-
-			$body = $response->getBody();
-			$bodyData = json_decode($body, true);
-			if ($status !== Http::STATUS_OK) {
-				$this->log->error('Could not send notification to push server [{url}]: {error}',[
-					'error' => \is_string($body) && $bodyData === null ? $body : 'no reason given',
+			} else {
+				$error = \is_string($body) && $bodyData === null ? $body : 'no reason given';
+				$this->log->info('Push notification sent but response was not parsable, using an outdated push proxy? [{url}]: {error}', [
+					'error' => $error,
 					'url' => $proxyServer,
 					'app' => 'notifications',
 				]);
 			}
-
-			if (is_array($bodyData) && !empty($bodyData['unknown']) && is_array($bodyData['unknown'])) {
-				foreach ($bodyData['unknown'] as $unknownDevice) {
-					$this->deletePushTokenByDeviceIdentifier($unknownDevice);
-				}
-			}
 		}
 	}
 
diff --git a/tests/Unit/PushTest.php b/tests/Unit/PushTest.php
index 190cfb0..14148e9 100644
--- a/tests/Unit/PushTest.php
+++ b/tests/Unit/PushTest.php
@@ -427,10 +427,9 @@ class PushTest extends TestCase {
 				],
 			]);
 
-		$this->config->expects($this->exactly(2))
+		$this->config->expects($this->exactly(1))
 			->method('getSystemValue')
 			->willReturnMap([
-				['debug', false, $isDebug],
 				['force_language', false, false],
 			]);
 
@@ -490,7 +489,7 @@ class PushTest extends TestCase {
 			->method('logException')
 			->with($e, [
 				'app' => 'notifications',
-				'level' => ILogger::WARN,
+				'level' => ILogger::ERROR,
 			]);
 
 		/** @var IResponse|\PHPUnit_Framework_MockObject_MockObject $response1 */
@@ -511,7 +510,7 @@ class PushTest extends TestCase {
 			->willReturn($response1);
 
 		$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',
@@ -535,8 +534,8 @@ class PushTest extends TestCase {
 				])
 			->willReturn($response2);
 
-		$this->logger->expects($isDebug ? $this->at(2) : $this->never())
-			->method('debug')
+		$this->logger->expects($this->at(2))
+			->method('warning')
 			->with('Could not send notification to push server [{url}]: {error}', [
 				'error' => 'Maintenance',
 				'url' => 'unavailable',