From 19f7cc9e9243c8ce4f3a5cba2c6fc286b1e32f38 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 17 Jan 2018 13:42:02 +0100 Subject: Make Update notficiations strict and fix all inspections Signed-off-by: Joas Schilling --- .../updatenotification/lib/AppInfo/Application.php | 1 + .../lib/Controller/AdminController.php | 5 +- .../lib/Notification/BackgroundJob.php | 56 +++++++++++++--------- .../lib/Notification/Notifier.php | 18 +++---- .../lib/ResetTokenBackgroundJob.php | 1 + apps/updatenotification/lib/Settings/Admin.php | 7 +-- apps/updatenotification/lib/UpdateChecker.php | 11 +++-- apps/updatenotification/templates/admin.php | 9 ++-- .../tests/Controller/AdminControllerTest.php | 5 +- .../tests/Notification/BackgroundJobTest.php | 41 ++++++++-------- .../tests/Notification/NotifierTest.php | 7 +-- .../tests/ResetTokenBackgroundJobTest.php | 1 + .../tests/Settings/AdminTest.php | 1 + .../updatenotification/tests/UpdateCheckerTest.php | 8 ++-- 14 files changed, 95 insertions(+), 76 deletions(-) (limited to 'apps/updatenotification') diff --git a/apps/updatenotification/lib/AppInfo/Application.php b/apps/updatenotification/lib/AppInfo/Application.php index ef6f79e4eb5..d79f14764de 100644 --- a/apps/updatenotification/lib/AppInfo/Application.php +++ b/apps/updatenotification/lib/AppInfo/Application.php @@ -1,4 +1,5 @@ * diff --git a/apps/updatenotification/lib/Controller/AdminController.php b/apps/updatenotification/lib/Controller/AdminController.php index ba16f114f61..57f89969db4 100644 --- a/apps/updatenotification/lib/Controller/AdminController.php +++ b/apps/updatenotification/lib/Controller/AdminController.php @@ -1,4 +1,5 @@ config->setAppValue('core', 'lastupdatedat', 0); return new DataResponse(['status' => 'success', 'data' => ['message' => $this->l10n->t('Channel updated')]]); @@ -85,7 +86,7 @@ class AdminController extends Controller { /** * @return DataResponse */ - public function createCredentials() { + public function createCredentials(): DataResponse { // Create a new job and store the creation date $this->jobList->add(ResetTokenBackgroundJob::class); $this->config->setAppValue('core', 'updater.secret.created', $this->timeFactory->getTime()); diff --git a/apps/updatenotification/lib/Notification/BackgroundJob.php b/apps/updatenotification/lib/Notification/BackgroundJob.php index 3c0cac60cde..c010ccc89e5 100644 --- a/apps/updatenotification/lib/Notification/BackgroundJob.php +++ b/apps/updatenotification/lib/Notification/BackgroundJob.php @@ -1,4 +1,5 @@ getChannel(), ['daily', 'git'], true)) { + if (\in_array($this->getChannel(), ['daily', 'git'], true)) { // "These aren't the update channels you're looking for." - Ben Obi-Wan Kenobi return; } @@ -102,10 +103,10 @@ class BackgroundJob extends TimedJob { $errors = 1 + (int) $this->config->getAppValue('updatenotification', 'update_check_errors', 0); $this->config->setAppValue('updatenotification', 'update_check_errors', $errors); - if (in_array($errors, $this->connectionNotifications, true)) { + if (\in_array($errors, $this->connectionNotifications, true)) { $this->sendErrorNotifications($errors); } - } else if (is_array($status)) { + } else if (\is_array($status)) { $this->config->setAppValue('updatenotification', 'update_check_errors', 0); $this->clearErrorNotifications(); @@ -178,26 +179,31 @@ class BackgroundJob extends TimedJob { if ($lastNotification === $version) { // We already notified about this update return; - } else if ($lastNotification !== false) { + } + + if ($lastNotification !== false) { // Delete old updates $this->deleteOutdatedNotifications($app, $lastNotification); } - $notification = $this->notificationManager->createNotification(); - $notification->setApp('updatenotification') - ->setDateTime(new \DateTime()) - ->setObject($app, $version); - - if ($visibleVersion !== '') { - $notification->setSubject('update_available', ['version' => $visibleVersion]); - } else { - $notification->setSubject('update_available'); - } + try { + $notification->setApp('updatenotification') + ->setDateTime(new \DateTime()) + ->setObject($app, $version); - foreach ($this->getUsersToNotify() as $uid) { - $notification->setUser($uid); - $this->notificationManager->notify($notification); + if ($visibleVersion !== '') { + $notification->setSubject('update_available', ['version' => $visibleVersion]); + } else { + $notification->setSubject('update_available'); + } + + foreach ($this->getUsersToNotify() as $uid) { + $notification->setUser($uid); + $this->notificationManager->notify($notification); + } + } catch (\InvalidArgumentException $e) { + return; } $this->config->setAppValue('updatenotification', $app, $version); @@ -206,12 +212,12 @@ class BackgroundJob extends TimedJob { /** * @return string[] */ - protected function getUsersToNotify() { + protected function getUsersToNotify(): array { if ($this->users !== null) { return $this->users; } - $notifyGroups = json_decode($this->config->getAppValue('updatenotification', 'notify_groups', '["admin"]'), true); + $notifyGroups = (array) json_decode($this->config->getAppValue('updatenotification', 'notify_groups', '["admin"]'), true); $this->users = []; foreach ($notifyGroups as $group) { $groupToNotify = $this->groupManager->get($group); @@ -235,15 +241,19 @@ class BackgroundJob extends TimedJob { */ protected function deleteOutdatedNotifications($app, $version) { $notification = $this->notificationManager->createNotification(); - $notification->setApp('updatenotification') - ->setObject($app, $version); + try { + $notification->setApp('updatenotification') + ->setObject($app, $version); + } catch (\InvalidArgumentException $e) { + return; + } $this->notificationManager->markProcessed($notification); } /** * @return VersionCheck */ - protected function createVersionCheck() { + protected function createVersionCheck(): VersionCheck { return new VersionCheck( $this->client, $this->config @@ -253,7 +263,7 @@ class BackgroundJob extends TimedJob { /** * @return string */ - protected function getChannel() { + protected function getChannel(): string { return \OC_Util::getChannel(); } diff --git a/apps/updatenotification/lib/Notification/Notifier.php b/apps/updatenotification/lib/Notification/Notifier.php index d18a2663702..c88937f0dff 100644 --- a/apps/updatenotification/lib/Notification/Notifier.php +++ b/apps/updatenotification/lib/Notification/Notifier.php @@ -1,4 +1,5 @@ getApp() !== 'updatenotification') { - throw new \InvalidArgumentException(); + throw new \InvalidArgumentException('Unknown app id'); } $l = $this->l10NFactory->get('updatenotification', $languageCode); @@ -94,7 +96,7 @@ class Notifier implements INotifier { $errors = (int) $this->config->getAppValue('updatenotification', 'update_check_errors', 0); if ($errors === 0) { $this->notificationManager->markProcessed($notification); - throw new \InvalidArgumentException(); + throw new \InvalidArgumentException('Update checked worked again'); } $notification->setParsedSubject($l->t('The update server could not be reached since %d days to check for new updates.', [$errors])) @@ -145,14 +147,14 @@ class Notifier implements INotifier { protected function updateAlreadyInstalledCheck(INotification $notification, $installedVersion) { if (version_compare($notification->getObjectId(), $installedVersion, '<=')) { $this->notificationManager->markProcessed($notification); - throw new \InvalidArgumentException(); + throw new \InvalidArgumentException('Update already installed'); } } /** * @return bool */ - protected function isAdmin() { + protected function isAdmin(): bool { $user = $this->userSession->getUser(); if ($user instanceof IUser) { @@ -162,11 +164,11 @@ class Notifier implements INotifier { return false; } - protected function getCoreVersions() { - return implode('.', \OCP\Util::getVersion()); + protected function getCoreVersions(): string { + return implode('.', Util::getVersion()); } - protected function getAppVersions() { + protected function getAppVersions(): array { return \OC_App::getAppVersions(); } diff --git a/apps/updatenotification/lib/ResetTokenBackgroundJob.php b/apps/updatenotification/lib/ResetTokenBackgroundJob.php index 5dd7c4f35f1..87cca466ec7 100644 --- a/apps/updatenotification/lib/ResetTokenBackgroundJob.php +++ b/apps/updatenotification/lib/ResetTokenBackgroundJob.php @@ -1,4 +1,5 @@ config->getAppValue('core', 'lastupdatedat'); $lastUpdateCheck = $this->dateTimeFormatter->formatDateTime($lastUpdateCheckTimestamp); @@ -99,7 +100,7 @@ class Admin implements ISettings { /** * @return string the section ID, e.g. 'sharing' */ - public function getSection() { + public function getSection(): string { return 'server'; } @@ -110,7 +111,7 @@ class Admin implements ISettings { * * E.g.: 70 */ - public function getPriority() { + public function getPriority(): int { return 1; } } diff --git a/apps/updatenotification/lib/UpdateChecker.php b/apps/updatenotification/lib/UpdateChecker.php index cabdfe8b9ff..5f2712423d2 100644 --- a/apps/updatenotification/lib/UpdateChecker.php +++ b/apps/updatenotification/lib/UpdateChecker.php @@ -1,4 +1,5 @@ updater->check(); $result = []; - if(isset($data['version']) && $data['version'] !== '' && $data['version'] !== []) { + if (isset($data['version']) && $data['version'] !== '' && $data['version'] !== []) { $result['updateAvailable'] = true; $result['updateVersion'] = $data['versionstring']; $result['updaterEnabled'] = $data['autoupdater'] === '1'; - if(substr($data['web'], 0, 8) === 'https://') { + if (strpos($data['web'], 'https://') === 0) { $result['updateLink'] = $data['web']; } - if(substr($data['url'], 0, 8) === 'https://') { + if (strpos($data['url'], 'https://') === 0) { $result['downloadLink'] = $data['url']; } @@ -68,7 +69,7 @@ class UpdateChecker { $data['array']['oc_updateState'] = json_encode([ 'updateAvailable' => true, 'updateVersion' => $this->getUpdateState()['updateVersion'], - 'updateLink' => isset($this->getUpdateState()['updateLink']) ? $this->getUpdateState()['updateLink'] : '', + 'updateLink' => $this->getUpdateState()['updateLink'] ?? '', ]); } } diff --git a/apps/updatenotification/templates/admin.php b/apps/updatenotification/templates/admin.php index e09d19848e7..19bbec769d0 100644 --- a/apps/updatenotification/templates/admin.php +++ b/apps/updatenotification/templates/admin.php @@ -1,4 +1,5 @@
- t("A non-default update server is in use to be checked for updates:")); ?> + t('A non-default update server is in use to be checked for updates:')); ?> @@ -65,10 +66,10 @@

t('Notify members of the following groups about available updates:')); ?>
- + t('Only notification for app updates are available.')); ?> - t('The selected update channel makes dedicated notifications for the server obsolete.')); ?> - t('The selected update channel does not support updates of the server.')); ?> + t('The selected update channel makes dedicated notifications for the server obsolete.')); } ?> + t('The selected update channel does not support updates of the server.')); } ?>

diff --git a/apps/updatenotification/tests/Controller/AdminControllerTest.php b/apps/updatenotification/tests/Controller/AdminControllerTest.php index 00d3974ed02..98b08e633bb 100644 --- a/apps/updatenotification/tests/Controller/AdminControllerTest.php +++ b/apps/updatenotification/tests/Controller/AdminControllerTest.php @@ -1,4 +1,5 @@ getJob([ 'getChannel', 'createVersionCheck', @@ -197,13 +198,13 @@ class BackgroundJobTest extends TestCase { ->method('clearErrorNotifications'); $job->expects($this->once()) ->method('createNotifications') - ->willReturn('core', $version, $readableVersion); + ->with('core', $version, $readableVersion); } self::invokePrivate($job, 'checkCoreUpdate'); } - public function dataCheckAppUpdates() { + public function dataCheckAppUpdates(): array { return [ [ ['app1', 'app2'], @@ -235,18 +236,18 @@ class BackgroundJobTest extends TestCase { ->method('getInstalledApps') ->willReturn($apps); - $job->expects($this->exactly(count($apps))) + $job->expects($this->exactly(\count($apps))) ->method('isUpdateAvailable') ->willReturnMap($isUpdateAvailable); - $mockedMethod = $job->expects($this->exactly(count($notifications))) + $mockedMethod = $job->expects($this->exactly(\count($notifications))) ->method('createNotifications'); - call_user_func_array([$mockedMethod, 'withConsecutive'], $notifications); + \call_user_func_array([$mockedMethod, 'withConsecutive'], $notifications); self::invokePrivate($job, 'checkAppUpdates'); } - public function dataCreateNotifications() { + public function dataCreateNotifications(): array { return [ ['app1', '1.0.0', '1.0.0', false, false, null, null], ['app2', '1.0.1', '1.0.0', '1.0.0', true, ['user1'], [['user1']]], @@ -265,7 +266,7 @@ class BackgroundJobTest extends TestCase { * @param string[]|null $users * @param array|null $userNotifications */ - public function testCreateNotifications($app, $version, $lastNotification, $callDelete, $createNotification, $users, $userNotifications) { + public function testCreateNotifications(string $app, string $version, $lastNotification, $callDelete, $createNotification, $users, $userNotifications) { $job = $this->getJob([ 'deleteOutdatedNotifications', 'getUsersToNotify', @@ -319,12 +320,12 @@ class BackgroundJobTest extends TestCase { ->willReturnSelf(); if ($userNotifications !== null) { - $mockedMethod = $notification->expects($this->exactly(count($userNotifications))) + $mockedMethod = $notification->expects($this->exactly(\count($userNotifications))) ->method('setUser') ->willReturnSelf(); - call_user_func_array([$mockedMethod, 'withConsecutive'], $userNotifications); + \call_user_func_array([$mockedMethod, 'withConsecutive'], $userNotifications); - $this->notificationManager->expects($this->exactly(count($userNotifications))) + $this->notificationManager->expects($this->exactly(\count($userNotifications))) ->method('notify') ->willReturn($notification); } @@ -340,7 +341,7 @@ class BackgroundJobTest extends TestCase { self::invokePrivate($job, 'createNotifications', [$app, $version]); } - public function dataGetUsersToNotify() { + public function dataGetUsersToNotify(): array { return [ [['g1', 'g2'], ['g1' => null, 'g2' => ['u1', 'u2']], ['u1', 'u2']], [['g3', 'g4'], ['g3' => ['u1', 'u2'], 'g4' => ['u2', 'u3']], ['u1', 'u2', 'u3']], @@ -353,7 +354,7 @@ class BackgroundJobTest extends TestCase { * @param array $groupUsers * @param string[] $expected */ - public function testGetUsersToNotify($groups, array $groupUsers, array $expected) { + public function testGetUsersToNotify(array $groups, array $groupUsers, array $expected) { $job = $this->getJob(); $this->config->expects($this->once()) @@ -373,7 +374,7 @@ class BackgroundJobTest extends TestCase { } $groupMap[] = [$gid, $group]; } - $this->groupManager->expects($this->exactly(count($groups))) + $this->groupManager->expects($this->exactly(\count($groups))) ->method('get') ->willReturnMap($groupMap); @@ -385,7 +386,7 @@ class BackgroundJobTest extends TestCase { $this->assertEquals($expected, $result); } - public function dataDeleteOutdatedNotifications() { + public function dataDeleteOutdatedNotifications(): array { return [ ['app1', '1.1.0'], ['app2', '1.2.0'], @@ -397,7 +398,7 @@ class BackgroundJobTest extends TestCase { * @param string $app * @param string $version */ - public function testDeleteOutdatedNotifications($app, $version) { + public function testDeleteOutdatedNotifications(string $app, string $version) { $notification = $this->createMock(INotification::class); $notification->expects($this->once()) ->method('setApp') @@ -423,7 +424,7 @@ class BackgroundJobTest extends TestCase { * @param string[] $userIds * @return IUser[]|\PHPUnit_Framework_MockObject_MockObject[] */ - protected function getUsers(array $userIds) { + protected function getUsers(array $userIds): array { $users = []; foreach ($userIds as $uid) { $user = $this->createMock(IUser::class); @@ -436,10 +437,10 @@ class BackgroundJobTest extends TestCase { } /** - * @param $gid + * @param string $gid * @return \OCP\IGroup|\PHPUnit_Framework_MockObject_MockObject */ - protected function getGroup($gid) { + protected function getGroup(string $gid) { $group = $this->createMock(IGroup::class); $group->expects($this->any()) ->method('getGID') diff --git a/apps/updatenotification/tests/Notification/NotifierTest.php b/apps/updatenotification/tests/Notification/NotifierTest.php index 34486bf5ba5..b1ddf7b478d 100644 --- a/apps/updatenotification/tests/Notification/NotifierTest.php +++ b/apps/updatenotification/tests/Notification/NotifierTest.php @@ -1,4 +1,5 @@ getNotifier(); $notification = $this->createMock(INotification::class); @@ -121,7 +122,7 @@ class NotifierTest extends TestCase { } try { - $this->invokePrivate($notifier, 'updateAlreadyInstalledCheck', [$notification, $versionInstalled]); + self::invokePrivate($notifier, 'updateAlreadyInstalledCheck', [$notification, $versionInstalled]); $this->assertFalse($exception); } catch (\Exception $e) { $this->assertTrue($exception); diff --git a/apps/updatenotification/tests/ResetTokenBackgroundJobTest.php b/apps/updatenotification/tests/ResetTokenBackgroundJobTest.php index bd6223bab1d..d78a5ed94cc 100644 --- a/apps/updatenotification/tests/ResetTokenBackgroundJobTest.php +++ b/apps/updatenotification/tests/ResetTokenBackgroundJobTest.php @@ -1,4 +1,5 @@ updater = $this->getMockBuilder('\OC\Updater\VersionCheck') - ->disableOriginalConstructor()->getMock(); + $this->updater = $this->createMock(VersionCheck::class); $this->updateChecker = new UpdateChecker($this->updater); } -- cgit v1.2.3