From 591e75df5c3acf51e6968f20b1856481ee56f4de Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 20 Mar 2017 10:02:05 +0100 Subject: [PATCH 1/6] Don't use a generic exception Signed-off-by: Joas Schilling --- lib/private/App/AppManager.php | 9 ++++----- lib/public/App/IAppManager.php | 1 + tests/lib/App/ManagerTest.php | 7 ++++--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/private/App/AppManager.php b/lib/private/App/AppManager.php index 6c1f5ba694..4277726624 100644 --- a/lib/private/App/AppManager.php +++ b/lib/private/App/AppManager.php @@ -32,7 +32,6 @@ namespace OC\App; use OCP\App\AppPathNotFoundException; -use OC_App; use OCP\App\IAppManager; use OCP\App\ManagerEvent; use OCP\IAppConfig; @@ -211,12 +210,12 @@ class AppManager implements IAppManager { * Enable an app for every user * * @param string $appId - * @throws \Exception + * @throws AppPathNotFoundException */ public function enableApp($appId) { - if(OC_App::getAppPath($appId) === false) { - throw new \Exception("$appId can't be enabled since it is not installed."); - } + // Check if app exists + $this->getAppPath($appId); + $this->installedAppsCache[$appId] = 'yes'; $this->appConfig->setValue($appId, 'enabled', 'yes'); $this->dispatcher->dispatch(ManagerEvent::EVENT_APP_ENABLE, new ManagerEvent( diff --git a/lib/public/App/IAppManager.php b/lib/public/App/IAppManager.php index 72c9977712..107297bc89 100644 --- a/lib/public/App/IAppManager.php +++ b/lib/public/App/IAppManager.php @@ -57,6 +57,7 @@ interface IAppManager { * Enable an app for every user * * @param string $appId + * @throws AppPathNotFoundException * @since 8.0.0 */ public function enableApp($appId); diff --git a/tests/lib/App/ManagerTest.php b/tests/lib/App/ManagerTest.php index 8b23168938..59ef9a36bf 100644 --- a/tests/lib/App/ManagerTest.php +++ b/tests/lib/App/ManagerTest.php @@ -134,10 +134,11 @@ class ManagerTest extends TestCase { try { $this->manager->enableApp('some_random_name_which_i_hope_is_not_an_app'); $this->assertFalse(true, 'If this line is reached the expected exception is not thrown.'); - } catch (\Exception $e) { - // excpetion is expected - $this->assertEquals("some_random_name_which_i_hope_is_not_an_app can't be enabled since it is not installed.", $e->getMessage()); + } catch (AppPathNotFoundException $e) { + // Exception is expected + $this->assertEquals('Could not find path for some_random_name_which_i_hope_is_not_an_app', $e->getMessage()); } + $this->assertEquals('no', $this->appConfig->getValue( 'some_random_name_which_i_hope_is_not_an_app', 'enabled', 'no' )); From 3abe86dade02a27c1e26433698ac0702d06b6704 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 20 Mar 2017 10:30:46 +0100 Subject: [PATCH 2/6] Fix provisioning API call Signed-off-by: Joas Schilling --- apps/provisioning_api/lib/Controller/AppsController.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/apps/provisioning_api/lib/Controller/AppsController.php b/apps/provisioning_api/lib/Controller/AppsController.php index e384d5af90..1165c7b856 100644 --- a/apps/provisioning_api/lib/Controller/AppsController.php +++ b/apps/provisioning_api/lib/Controller/AppsController.php @@ -26,7 +26,9 @@ namespace OCA\Provisioning_API\Controller; use \OC_App; +use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; +use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCSController; @@ -99,9 +101,14 @@ class AppsController extends OCSController { * @PasswordConfirmationRequired * @param string $app * @return DataResponse + * @throws OCSException */ public function enable($app) { - $this->appManager->enableApp($app); + try { + $this->appManager->enableApp($app); + } catch (AppPathNotFoundException $e) { + throw new OCSException('The request app was not found', \OCP\API::RESPOND_NOT_FOUND); + } return new DataResponse(); } From 5cf6fc7e8dbe0d2b142953f5671ae8f7c762fccb Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 20 Mar 2017 10:44:40 +0100 Subject: [PATCH 3/6] Add integration tests Signed-off-by: Joas Schilling --- build/integration/features/provisioning-v1.feature | 12 ++++++++++++ build/integration/features/provisioning-v2.feature | 12 ++++++++++++ 2 files changed, 24 insertions(+) diff --git a/build/integration/features/provisioning-v1.feature b/build/integration/features/provisioning-v1.feature index a8501ee887..ad9d901d05 100644 --- a/build/integration/features/provisioning-v1.feature +++ b/build/integration/features/provisioning-v1.feature @@ -306,6 +306,12 @@ Feature: provisioning Then the OCS status code should be "100" And the HTTP status code should be "200" + Scenario: get app info from app that does not exist + Given As an "admin" + When sending "GET" to "/cloud/apps/this_app_should_never_exist" + Then the OCS status code should be "998" + And the HTTP status code should be "200" + Scenario: enable an app Given As an "admin" And app "testing" is disabled @@ -314,6 +320,12 @@ Feature: provisioning And the HTTP status code should be "200" And app "testing" is enabled + Scenario: enable an app that does not exist + Given As an "admin" + When sending "POST" to "/cloud/apps/this_app_should_never_exist" + Then the OCS status code should be "998" + And the HTTP status code should be "200" + Scenario: disable an app Given As an "admin" And app "testing" is enabled diff --git a/build/integration/features/provisioning-v2.feature b/build/integration/features/provisioning-v2.feature index 6140128684..def9b376d2 100644 --- a/build/integration/features/provisioning-v2.feature +++ b/build/integration/features/provisioning-v2.feature @@ -7,3 +7,15 @@ Feature: provisioning When sending "GET" to "/cloud/users/test" Then the HTTP status code should be "404" + Scenario: get app info from app that does not exist + Given As an "admin" + When sending "GET" to "/cloud/apps/this_app_should_never_exist" + Then the OCS status code should be "998" + And the HTTP status code should be "404" + + Scenario: enable an app that does not exist + Given As an "admin" + When sending "POST" to "/cloud/apps/this_app_should_never_exist" + Then the OCS status code should be "998" + And the HTTP status code should be "404" + From 6d30b35ed7f96cfdc593c31cae46ddf9e74bb015 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 20 Mar 2017 10:11:49 +0100 Subject: [PATCH 4/6] Some more code fixes Signed-off-by: Joas Schilling --- lib/private/App/AppManager.php | 43 +++++++++++++++++----------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/lib/private/App/AppManager.php b/lib/private/App/AppManager.php index 4277726624..7c861597c7 100644 --- a/lib/private/App/AppManager.php +++ b/lib/private/App/AppManager.php @@ -55,18 +55,21 @@ class AppManager implements IAppManager { 'prevent_group_restriction', ]; - /** @var \OCP\IUserSession */ + /** @var IUserSession */ private $userSession; - /** @var \OCP\IAppConfig */ + /** @var IAppConfig */ private $appConfig; - /** @var \OCP\IGroupManager */ + /** @var IGroupManager */ private $groupManager; - /** @var \OCP\ICacheFactory */ + /** @var ICacheFactory */ private $memCacheFactory; + /** @var EventDispatcherInterface */ + private $dispatcher; + /** @var string[] $appId => $enabled */ private $installedAppsCache; @@ -76,14 +79,12 @@ class AppManager implements IAppManager { /** @var string[] */ private $alwaysEnabled; - /** @var EventDispatcherInterface */ - private $dispatcher; - /** - * @param \OCP\IUserSession $userSession - * @param \OCP\IAppConfig $appConfig - * @param \OCP\IGroupManager $groupManager - * @param \OCP\ICacheFactory $memCacheFactory + * @param IUserSession $userSession + * @param IAppConfig $appConfig + * @param IGroupManager $groupManager + * @param ICacheFactory $memCacheFactory + * @param EventDispatcherInterface $dispatcher */ public function __construct(IUserSession $userSession, IAppConfig $appConfig, @@ -151,7 +152,7 @@ class AppManager implements IAppManager { if ($this->isAlwaysEnabled($appId)) { return true; } - if (is_null($user)) { + if ($user === null) { $user = $this->userSession->getUser(); } $installedApps = $this->getInstalledAppsValues(); @@ -170,7 +171,7 @@ class AppManager implements IAppManager { private function checkAppForUser($enabled, $user) { if ($enabled === 'yes') { return true; - } elseif (is_null($user)) { + } elseif ($user === null) { return false; } else { if(empty($enabled)){ @@ -187,7 +188,7 @@ class AppManager implements IAppManager { $userGroups = $this->groupManager->getUserGroupIds($user); foreach ($userGroups as $groupId) { - if (array_search($groupId, $groupIds) !== false) { + if (in_array($groupId, $groupIds, true)) { return true; } } @@ -311,12 +312,12 @@ class AppManager implements IAppManager { /** * Returns a list of apps that need upgrade * - * @param array $version ownCloud version as array of version components + * @param string $version Nextcloud version as array of version components * @return array list of app info from apps that need an upgrade * * @internal */ - public function getAppsNeedingUpgrade($ocVersion) { + public function getAppsNeedingUpgrade($version) { $appsToUpgrade = []; $apps = $this->getInstalledApps(); foreach ($apps as $appId) { @@ -325,7 +326,7 @@ class AppManager implements IAppManager { if ($appDbVersion && isset($appInfo['version']) && version_compare($appInfo['version'], $appDbVersion, '>') - && \OC_App::isAppCompatible($ocVersion, $appInfo) + && \OC_App::isAppCompatible($version, $appInfo) ) { $appsToUpgrade[] = $appInfo; } @@ -355,7 +356,7 @@ class AppManager implements IAppManager { /** * Returns a list of apps incompatible with the given version * - * @param array $version ownCloud version as array of version components + * @param string $version Nextcloud version as array of version components * * @return array list of app info from incompatible apps * @@ -378,16 +379,16 @@ class AppManager implements IAppManager { */ public function isShipped($appId) { $this->loadShippedJson(); - return in_array($appId, $this->shippedApps); + return in_array($appId, $this->shippedApps, true); } private function isAlwaysEnabled($appId) { $alwaysEnabled = $this->getAlwaysEnabledApps(); - return in_array($appId, $alwaysEnabled); + return in_array($appId, $alwaysEnabled, true); } private function loadShippedJson() { - if (is_null($this->shippedApps)) { + if ($this->shippedApps === null) { $shippedJson = \OC::$SERVERROOT . '/core/shipped.json'; if (!file_exists($shippedJson)) { throw new \Exception("File not found: $shippedJson"); From 3eb831365770caa81434f5dbe1d659f1728fc121 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 20 Mar 2017 10:21:08 +0100 Subject: [PATCH 5/6] Fix the test Signed-off-by: Joas Schilling --- .../{ManagerTest.php => AppManagerTest.php} | 80 +++++++++++-------- 1 file changed, 46 insertions(+), 34 deletions(-) rename tests/lib/App/{ManagerTest.php => AppManagerTest.php} (84%) diff --git a/tests/lib/App/ManagerTest.php b/tests/lib/App/AppManagerTest.php similarity index 84% rename from tests/lib/App/ManagerTest.php rename to tests/lib/App/AppManagerTest.php index 59ef9a36bf..4ae53801ae 100644 --- a/tests/lib/App/ManagerTest.php +++ b/tests/lib/App/AppManagerTest.php @@ -9,23 +9,33 @@ namespace Test\App; +use OC\App\AppManager; use OC\Group\Group; use OC\User\User; use OCP\App\AppPathNotFoundException; +use OCP\App\IAppManager; +use OCP\ICache; +use OCP\ICacheFactory; +use OCP\IGroupManager; +use OCP\IUserSession; +use OCP\IAppConfig; +use OCP\IConfig; +use OCP\IURLGenerator; +use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Test\TestCase; /** - * Class Manager + * Class AppManagerTest * * @package Test\App */ -class ManagerTest extends TestCase { +class AppManagerTest extends TestCase { /** - * @return \OCP\IAppConfig | \PHPUnit_Framework_MockObject_MockObject + * @return IAppConfig|\PHPUnit_Framework_MockObject_MockObject */ protected function getAppConfig() { $appConfig = array(); - $config = $this->getMockBuilder('\OCP\IAppConfig') + $config = $this->getMockBuilder(IAppConfig::class) ->disableOriginalConstructor() ->getMock(); @@ -49,9 +59,9 @@ class ManagerTest extends TestCase { return $appConfig[$app]; } else { $values = array(); - foreach ($appConfig as $app => $appData) { + foreach ($appConfig as $appid => $appData) { if (isset($appData[$key])) { - $values[$app] = $appData[$key]; + $values[$appid] = $appData[$key]; } } return $values; @@ -61,51 +71,51 @@ class ManagerTest extends TestCase { return $config; } - /** @var \OCP\IUserSession */ + /** @var IUserSession|\PHPUnit_Framework_MockObject_MockObject */ protected $userSession; - /** @var \OCP\IGroupManager */ + /** @var IGroupManager|\PHPUnit_Framework_MockObject_MockObject */ protected $groupManager; - /** @var \OCP\IAppConfig */ + /** @var IAppConfig|\PHPUnit_Framework_MockObject_MockObject */ protected $appConfig; - /** @var \OCP\ICache */ + /** @var ICache|\PHPUnit_Framework_MockObject_MockObject */ protected $cache; - /** @var \OCP\ICacheFactory */ + /** @var ICacheFactory|\PHPUnit_Framework_MockObject_MockObject */ protected $cacheFactory; - /** @var \OCP\App\IAppManager */ - protected $manager; - - /** @var \Symfony\Component\EventDispatcher\EventDispatcherInterface */ + /** @var EventDispatcherInterface|\PHPUnit_Framework_MockObject_MockObject */ protected $eventDispatcher; + /** @var IAppManager */ + protected $manager; + protected function setUp() { parent::setUp(); - $this->userSession = $this->getMockBuilder('\OCP\IUserSession') + $this->userSession = $this->getMockBuilder(IUserSession::class) ->disableOriginalConstructor() ->getMock(); - $this->groupManager = $this->getMockBuilder('\OCP\IGroupManager') + $this->groupManager = $this->getMockBuilder(IGroupManager::class) ->disableOriginalConstructor() ->getMock(); $this->appConfig = $this->getAppConfig(); - $this->cacheFactory = $this->getMockBuilder('\OCP\ICacheFactory') + $this->cacheFactory = $this->getMockBuilder(ICacheFactory::class) ->disableOriginalConstructor() ->getMock(); - $this->cache = $this->getMockBuilder('\OCP\ICache') + $this->cache = $this->getMockBuilder(ICache::class) ->disableOriginalConstructor() ->getMock(); - $this->eventDispatcher = $this->getMockBuilder('\Symfony\Component\EventDispatcher\EventDispatcherInterface') + $this->eventDispatcher = $this->getMockBuilder(EventDispatcherInterface::class) ->disableOriginalConstructor() ->getMock(); $this->cacheFactory->expects($this->any()) ->method('create') ->with('settings') ->willReturn($this->cache); - $this->manager = new \OC\App\AppManager($this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher); + $this->manager = new AppManager($this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher); } protected function expectClearCache() { @@ -178,8 +188,8 @@ class ManagerTest extends TestCase { ); $this->expectClearCache(); - /** @var \OC\App\AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */ - $manager = $this->getMockBuilder('OC\App\AppManager') + /** @var AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */ + $manager = $this->getMockBuilder(AppManager::class) ->setConstructorArgs([ $this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher ]) @@ -221,8 +231,8 @@ class ManagerTest extends TestCase { new Group('group2', array(), null) ); - /** @var \OC\App\AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */ - $manager = $this->getMockBuilder('OC\App\AppManager') + /** @var AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */ + $manager = $this->getMockBuilder(AppManager::class) ->setConstructorArgs([ $this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher ]) @@ -257,10 +267,10 @@ class ManagerTest extends TestCase { } private function newUser($uid) { - $config = $this->getMockBuilder('\OCP\IConfig') + $config = $this->getMockBuilder(IConfig::class) ->disableOriginalConstructor() ->getMock(); - $urlgenerator = $this->getMockBuilder('\OCP\IURLGenerator') + $urlgenerator = $this->getMockBuilder(IURLGenerator::class) ->disableOriginalConstructor() ->getMock(); @@ -312,7 +322,7 @@ class ManagerTest extends TestCase { public function testIsEnabledForUserLoggedOut() { $this->appConfig->setValue('test', 'enabled', '["foo"]'); - $this->assertFalse($this->manager->IsEnabledForUser('test')); + $this->assertFalse($this->manager->isEnabledForUser('test')); } public function testIsEnabledForUserLoggedIn() { @@ -374,7 +384,8 @@ class ManagerTest extends TestCase { } public function testGetAppsNeedingUpgrade() { - $this->manager = $this->getMockBuilder('\OC\App\AppManager') + /** @var AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */ + $manager = $this->getMockBuilder(AppManager::class) ->setConstructorArgs([$this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher]) ->setMethods(['getAppInfo']) ->getMock(); @@ -394,7 +405,7 @@ class ManagerTest extends TestCase { 'workflowengine' => ['id' => 'workflowengine'], ]; - $this->manager->expects($this->any()) + $manager->expects($this->any()) ->method('getAppInfo') ->will($this->returnCallback( function($appId) use ($appInfos) { @@ -411,7 +422,7 @@ class ManagerTest extends TestCase { $this->appConfig->setValue('test4', 'enabled', 'yes'); $this->appConfig->setValue('test4', 'installed_version', '2.4.0'); - $apps = $this->manager->getAppsNeedingUpgrade('8.2.0'); + $apps = $manager->getAppsNeedingUpgrade('8.2.0'); $this->assertCount(2, $apps); $this->assertEquals('test1', $apps[0]['id']); @@ -419,7 +430,8 @@ class ManagerTest extends TestCase { } public function testGetIncompatibleApps() { - $this->manager = $this->getMockBuilder('\OC\App\AppManager') + /** @var AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */ + $manager = $this->getMockBuilder(AppManager::class) ->setConstructorArgs([$this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher]) ->setMethods(['getAppInfo']) ->getMock(); @@ -438,7 +450,7 @@ class ManagerTest extends TestCase { 'workflowengine' => ['id' => 'workflowengine'], ]; - $this->manager->expects($this->any()) + $manager->expects($this->any()) ->method('getAppInfo') ->will($this->returnCallback( function($appId) use ($appInfos) { @@ -450,7 +462,7 @@ class ManagerTest extends TestCase { $this->appConfig->setValue('test2', 'enabled', 'yes'); $this->appConfig->setValue('test3', 'enabled', 'yes'); - $apps = $this->manager->getIncompatibleApps('8.2.0'); + $apps = $manager->getIncompatibleApps('8.2.0'); $this->assertCount(2, $apps); $this->assertEquals('test1', $apps[0]['id']); From 5795482282bed45c678b26799da83e4da4ae2978 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 20 Mar 2017 11:14:14 +0100 Subject: [PATCH 6/6] createMock Signed-off-by: Joas Schilling --- tests/lib/App/AppManagerTest.php | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/tests/lib/App/AppManagerTest.php b/tests/lib/App/AppManagerTest.php index 4ae53801ae..bfb2893955 100644 --- a/tests/lib/App/AppManagerTest.php +++ b/tests/lib/App/AppManagerTest.php @@ -35,9 +35,7 @@ class AppManagerTest extends TestCase { */ protected function getAppConfig() { $appConfig = array(); - $config = $this->getMockBuilder(IAppConfig::class) - ->disableOriginalConstructor() - ->getMock(); + $config = $this->createMock(IAppConfig::class); $config->expects($this->any()) ->method('getValue') @@ -95,22 +93,12 @@ class AppManagerTest extends TestCase { protected function setUp() { parent::setUp(); - $this->userSession = $this->getMockBuilder(IUserSession::class) - ->disableOriginalConstructor() - ->getMock(); - $this->groupManager = $this->getMockBuilder(IGroupManager::class) - ->disableOriginalConstructor() - ->getMock(); + $this->userSession = $this->createMock(IUserSession::class); + $this->groupManager = $this->createMock(IGroupManager::class); $this->appConfig = $this->getAppConfig(); - $this->cacheFactory = $this->getMockBuilder(ICacheFactory::class) - ->disableOriginalConstructor() - ->getMock(); - $this->cache = $this->getMockBuilder(ICache::class) - ->disableOriginalConstructor() - ->getMock(); - $this->eventDispatcher = $this->getMockBuilder(EventDispatcherInterface::class) - ->disableOriginalConstructor() - ->getMock(); + $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->cache = $this->createMock(ICache::class); + $this->eventDispatcher = $this->createMock(EventDispatcherInterface::class); $this->cacheFactory->expects($this->any()) ->method('create') ->with('settings') @@ -267,12 +255,8 @@ class AppManagerTest extends TestCase { } private function newUser($uid) { - $config = $this->getMockBuilder(IConfig::class) - ->disableOriginalConstructor() - ->getMock(); - $urlgenerator = $this->getMockBuilder(IURLGenerator::class) - ->disableOriginalConstructor() - ->getMock(); + $config = $this->createMock(IConfig::class); + $urlgenerator = $this->createMock(IURLGenerator::class); return new User($uid, null, null, $config, $urlgenerator); }