diff options
author | Joas Schilling <213943+nickvergessen@users.noreply.github.com> | 2020-10-01 16:16:23 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-10-01 16:16:23 +0300 |
commit | bf8574a5e38a5cd33f41e61552692b048dc4b3cf (patch) | |
tree | 734a37b62383e15bd3f44f12f042c26e2dd1183e | |
parent | bd8b6a491db5563057ee13c116111199075db386 (diff) | |
parent | 938c005d1a6d87a31060d164ff7dbe3edf72510a (diff) |
Merge pull request #760 from nextcloud/backport/758/stable19v19.0.4RC2v19.0.40RC1v19.0.4
[stable19] Also check responses of 400 errors and so delete unknown devices
-rw-r--r-- | lib/Push.php | 68 | ||||
-rw-r--r-- | tests/Unit/PushTest.php | 97 |
2 files changed, 114 insertions, 51 deletions
diff --git a/lib/Push.php b/lib/Push.php index d377df8..c3f6405 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; @@ -263,44 +265,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 be6c36d..f05be9c 100644 --- a/tests/Unit/PushTest.php +++ b/tests/Unit/PushTest.php @@ -23,6 +23,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,11 +405,11 @@ 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); - $notification->expects($this->exactly(3)) + $notification ->method('getUser') ->willReturn('valid'); @@ -447,12 +449,16 @@ class PushTest extends TestCase { 'token' => 64, 'apptype' => 'other', ], + [ + 'proxyserver' => 'badrequest-with-devices', + 'token' => 128, + 'apptype' => 'other', + ], ]); - $this->config->expects($this->exactly(2)) + $this->config->expects($this->exactly(1)) ->method('getSystemValue') ->willReturnMap([ - ['debug', false, $isDebug], ['force_language', false, false], ]); @@ -474,11 +480,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']); @@ -501,17 +507,17 @@ class PushTest extends TestCase { $client->expects($this->at(0)) ->method('post') ->with('proxyserver1/notifications', [ - 'body' => [ - 'notifications' => ['["Payload"]', '["Payload"]'], - ], - ]) + 'body' => [ + 'notifications' => ['["Payload"]', '["Payload"]'], + ], + ]) ->willThrowException($e); $this->logger->expects($this->at(0)) ->method('logException') ->with($e, [ 'app' => 'notifications', - 'level' => ILogger::WARN, + 'level' => ILogger::ERROR, ]); /** @var IResponse|MockObject $response1 */ @@ -522,17 +528,20 @@ 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', [ - 'body' => [ - 'notifications' => ['["Payload"]'], - ], - ]) - ->willReturn($response1); + 'body' => [ + 'notifications' => ['["Payload"]'], + ], + ]) + ->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', @@ -542,21 +551,21 @@ 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', [ - 'body' => [ - 'notifications' => ['["Payload"]'], - ], - ]) - ->willReturn($response2); + 'body' => [ + 'notifications' => ['["Payload"]'], + ], + ]) + ->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', @@ -572,12 +581,40 @@ class PushTest extends TestCase { $client->expects($this->at(3)) ->method('post') ->with('ok/notifications', [ - 'body' => [ - 'notifications' => ['["Payload"]'], - ], - ]) + 'body' => [ + 'notifications' => ['["Payload"]'], + ], + ]) ->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); } |