Merge pull request #3957 from nextcloud/downstream-27307

Follow up to #3949 (app exists on enable)

6 files changed, 109 insertions, 80 deletions

diff --git a/apps/provisioning_api/lib/Controller/AppsController.php b/apps/provisioning_api/lib/Controller/AppsController.php
index e384d5af907..1165c7b8564 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();
 	}
 
diff --git a/build/integration/features/provisioning-v1.feature b/build/integration/features/provisioning-v1.feature
index a8501ee8873..ad9d901d051 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 6140128684d..def9b376d21 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"
+
diff --git a/lib/private/App/AppManager.php b/lib/private/App/AppManager.php
index 6c1f5ba6940..7c861597c77 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;
@@ -56,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;
 
@@ -77,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,
@@ -152,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();
@@ -171,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)){
@@ -188,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;
 				}
 			}
@@ -211,12 +211,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(
@@ -312,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) {
@@ -326,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;
 			}
@@ -356,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
 	 *
@@ -379,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");
diff --git a/lib/public/App/IAppManager.php b/lib/public/App/IAppManager.php
index 72c99777124..107297bc890 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/AppManagerTest.php
index 8b23168938c..bfb2893955f 100644
--- a/tests/lib/App/ManagerTest.php
+++ b/tests/lib/App/AppManagerTest.php
@@ -9,25 +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')
-			->disableOriginalConstructor()
-			->getMock();
+		$config = $this->createMock(IAppConfig::class);
 
 		$config->expects($this->any())
 			->method('getValue')
@@ -49,9 +57,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 +69,41 @@ 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')
-			->disableOriginalConstructor()
-			->getMock();
-		$this->groupManager = $this->getMockBuilder('\OCP\IGroupManager')
-			->disableOriginalConstructor()
-			->getMock();
+		$this->userSession = $this->createMock(IUserSession::class);
+		$this->groupManager = $this->createMock(IGroupManager::class);
 		$this->appConfig = $this->getAppConfig();
-		$this->cacheFactory = $this->getMockBuilder('\OCP\ICacheFactory')
-			->disableOriginalConstructor()
-			->getMock();
-		$this->cache = $this->getMockBuilder('\OCP\ICache')
-			->disableOriginalConstructor()
-			->getMock();
-		$this->eventDispatcher = $this->getMockBuilder('\Symfony\Component\EventDispatcher\EventDispatcherInterface')
-			->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')
 			->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() {
@@ -134,10 +132,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'
 		));
@@ -177,8 +176,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
 			])
@@ -220,8 +219,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
 			])
@@ -256,12 +255,8 @@ class ManagerTest extends TestCase {
 	}
 
 	private function newUser($uid) {
-		$config = $this->getMockBuilder('\OCP\IConfig')
-			->disableOriginalConstructor()
-			->getMock();
-		$urlgenerator = $this->getMockBuilder('\OCP\IURLGenerator')
-			->disableOriginalConstructor()
-			->getMock();
+		$config = $this->createMock(IConfig::class);
+		$urlgenerator = $this->createMock(IURLGenerator::class);
 
 		return new User($uid, null, null, $config, $urlgenerator);
 	}
@@ -311,7 +306,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() {
@@ -373,7 +368,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();
@@ -393,7 +389,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) {
@@ -410,7 +406,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']);
@@ -418,7 +414,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();
@@ -437,7 +434,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) {
@@ -449,7 +446,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']);