Merge pull request #3957 from nextcloud/downstream-27307

Follow up to #3949 (app exists on enable)
This commit is contained in:
Christoph Wurst 2017-03-20 18:10:05 +01:00 committed by GitHub
commit 03a92eaf74
6 changed files with 109 additions and 80 deletions

View File

@ -26,7 +26,9 @@
namespace OCA\Provisioning_API\Controller; namespace OCA\Provisioning_API\Controller;
use \OC_App; use \OC_App;
use OCP\App\AppPathNotFoundException;
use OCP\App\IAppManager; use OCP\App\IAppManager;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCS\OCSException;
use OCP\AppFramework\OCSController; use OCP\AppFramework\OCSController;
@ -99,9 +101,14 @@ class AppsController extends OCSController {
* @PasswordConfirmationRequired * @PasswordConfirmationRequired
* @param string $app * @param string $app
* @return DataResponse * @return DataResponse
* @throws OCSException
*/ */
public function enable($app) { 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(); return new DataResponse();
} }

View File

@ -306,6 +306,12 @@ Feature: provisioning
Then the OCS status code should be "100" Then the OCS status code should be "100"
And the HTTP status code should be "200" 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 Scenario: enable an app
Given As an "admin" Given As an "admin"
And app "testing" is disabled And app "testing" is disabled
@ -314,6 +320,12 @@ Feature: provisioning
And the HTTP status code should be "200" And the HTTP status code should be "200"
And app "testing" is enabled 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 Scenario: disable an app
Given As an "admin" Given As an "admin"
And app "testing" is enabled And app "testing" is enabled

View File

@ -7,3 +7,15 @@ Feature: provisioning
When sending "GET" to "/cloud/users/test" When sending "GET" to "/cloud/users/test"
Then the HTTP status code should be "404" 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"

View File

@ -32,7 +32,6 @@
namespace OC\App; namespace OC\App;
use OCP\App\AppPathNotFoundException; use OCP\App\AppPathNotFoundException;
use OC_App;
use OCP\App\IAppManager; use OCP\App\IAppManager;
use OCP\App\ManagerEvent; use OCP\App\ManagerEvent;
use OCP\IAppConfig; use OCP\IAppConfig;
@ -56,18 +55,21 @@ class AppManager implements IAppManager {
'prevent_group_restriction', 'prevent_group_restriction',
]; ];
/** @var \OCP\IUserSession */ /** @var IUserSession */
private $userSession; private $userSession;
/** @var \OCP\IAppConfig */ /** @var IAppConfig */
private $appConfig; private $appConfig;
/** @var \OCP\IGroupManager */ /** @var IGroupManager */
private $groupManager; private $groupManager;
/** @var \OCP\ICacheFactory */ /** @var ICacheFactory */
private $memCacheFactory; private $memCacheFactory;
/** @var EventDispatcherInterface */
private $dispatcher;
/** @var string[] $appId => $enabled */ /** @var string[] $appId => $enabled */
private $installedAppsCache; private $installedAppsCache;
@ -77,14 +79,12 @@ class AppManager implements IAppManager {
/** @var string[] */ /** @var string[] */
private $alwaysEnabled; private $alwaysEnabled;
/** @var EventDispatcherInterface */
private $dispatcher;
/** /**
* @param \OCP\IUserSession $userSession * @param IUserSession $userSession
* @param \OCP\IAppConfig $appConfig * @param IAppConfig $appConfig
* @param \OCP\IGroupManager $groupManager * @param IGroupManager $groupManager
* @param \OCP\ICacheFactory $memCacheFactory * @param ICacheFactory $memCacheFactory
* @param EventDispatcherInterface $dispatcher
*/ */
public function __construct(IUserSession $userSession, public function __construct(IUserSession $userSession,
IAppConfig $appConfig, IAppConfig $appConfig,
@ -152,7 +152,7 @@ class AppManager implements IAppManager {
if ($this->isAlwaysEnabled($appId)) { if ($this->isAlwaysEnabled($appId)) {
return true; return true;
} }
if (is_null($user)) { if ($user === null) {
$user = $this->userSession->getUser(); $user = $this->userSession->getUser();
} }
$installedApps = $this->getInstalledAppsValues(); $installedApps = $this->getInstalledAppsValues();
@ -171,7 +171,7 @@ class AppManager implements IAppManager {
private function checkAppForUser($enabled, $user) { private function checkAppForUser($enabled, $user) {
if ($enabled === 'yes') { if ($enabled === 'yes') {
return true; return true;
} elseif (is_null($user)) { } elseif ($user === null) {
return false; return false;
} else { } else {
if(empty($enabled)){ if(empty($enabled)){
@ -188,7 +188,7 @@ class AppManager implements IAppManager {
$userGroups = $this->groupManager->getUserGroupIds($user); $userGroups = $this->groupManager->getUserGroupIds($user);
foreach ($userGroups as $groupId) { foreach ($userGroups as $groupId) {
if (array_search($groupId, $groupIds) !== false) { if (in_array($groupId, $groupIds, true)) {
return true; return true;
} }
} }
@ -211,12 +211,12 @@ class AppManager implements IAppManager {
* Enable an app for every user * Enable an app for every user
* *
* @param string $appId * @param string $appId
* @throws \Exception * @throws AppPathNotFoundException
*/ */
public function enableApp($appId) { public function enableApp($appId) {
if(OC_App::getAppPath($appId) === false) { // Check if app exists
throw new \Exception("$appId can't be enabled since it is not installed."); $this->getAppPath($appId);
}
$this->installedAppsCache[$appId] = 'yes'; $this->installedAppsCache[$appId] = 'yes';
$this->appConfig->setValue($appId, 'enabled', 'yes'); $this->appConfig->setValue($appId, 'enabled', 'yes');
$this->dispatcher->dispatch(ManagerEvent::EVENT_APP_ENABLE, new ManagerEvent( $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 * 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 * @return array list of app info from apps that need an upgrade
* *
* @internal * @internal
*/ */
public function getAppsNeedingUpgrade($ocVersion) { public function getAppsNeedingUpgrade($version) {
$appsToUpgrade = []; $appsToUpgrade = [];
$apps = $this->getInstalledApps(); $apps = $this->getInstalledApps();
foreach ($apps as $appId) { foreach ($apps as $appId) {
@ -326,7 +326,7 @@ class AppManager implements IAppManager {
if ($appDbVersion if ($appDbVersion
&& isset($appInfo['version']) && isset($appInfo['version'])
&& version_compare($appInfo['version'], $appDbVersion, '>') && version_compare($appInfo['version'], $appDbVersion, '>')
&& \OC_App::isAppCompatible($ocVersion, $appInfo) && \OC_App::isAppCompatible($version, $appInfo)
) { ) {
$appsToUpgrade[] = $appInfo; $appsToUpgrade[] = $appInfo;
} }
@ -356,7 +356,7 @@ class AppManager implements IAppManager {
/** /**
* Returns a list of apps incompatible with the given version * 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 * @return array list of app info from incompatible apps
* *
@ -379,16 +379,16 @@ class AppManager implements IAppManager {
*/ */
public function isShipped($appId) { public function isShipped($appId) {
$this->loadShippedJson(); $this->loadShippedJson();
return in_array($appId, $this->shippedApps); return in_array($appId, $this->shippedApps, true);
} }
private function isAlwaysEnabled($appId) { private function isAlwaysEnabled($appId) {
$alwaysEnabled = $this->getAlwaysEnabledApps(); $alwaysEnabled = $this->getAlwaysEnabledApps();
return in_array($appId, $alwaysEnabled); return in_array($appId, $alwaysEnabled, true);
} }
private function loadShippedJson() { private function loadShippedJson() {
if (is_null($this->shippedApps)) { if ($this->shippedApps === null) {
$shippedJson = \OC::$SERVERROOT . '/core/shipped.json'; $shippedJson = \OC::$SERVERROOT . '/core/shipped.json';
if (!file_exists($shippedJson)) { if (!file_exists($shippedJson)) {
throw new \Exception("File not found: $shippedJson"); throw new \Exception("File not found: $shippedJson");

View File

@ -57,6 +57,7 @@ interface IAppManager {
* Enable an app for every user * Enable an app for every user
* *
* @param string $appId * @param string $appId
* @throws AppPathNotFoundException
* @since 8.0.0 * @since 8.0.0
*/ */
public function enableApp($appId); public function enableApp($appId);

View File

@ -9,25 +9,33 @@
namespace Test\App; namespace Test\App;
use OC\App\AppManager;
use OC\Group\Group; use OC\Group\Group;
use OC\User\User; use OC\User\User;
use OCP\App\AppPathNotFoundException; 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; use Test\TestCase;
/** /**
* Class Manager * Class AppManagerTest
* *
* @package Test\App * @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() { protected function getAppConfig() {
$appConfig = array(); $appConfig = array();
$config = $this->getMockBuilder('\OCP\IAppConfig') $config = $this->createMock(IAppConfig::class);
->disableOriginalConstructor()
->getMock();
$config->expects($this->any()) $config->expects($this->any())
->method('getValue') ->method('getValue')
@ -49,9 +57,9 @@ class ManagerTest extends TestCase {
return $appConfig[$app]; return $appConfig[$app];
} else { } else {
$values = array(); $values = array();
foreach ($appConfig as $app => $appData) { foreach ($appConfig as $appid => $appData) {
if (isset($appData[$key])) { if (isset($appData[$key])) {
$values[$app] = $appData[$key]; $values[$appid] = $appData[$key];
} }
} }
return $values; return $values;
@ -61,51 +69,41 @@ class ManagerTest extends TestCase {
return $config; return $config;
} }
/** @var \OCP\IUserSession */ /** @var IUserSession|\PHPUnit_Framework_MockObject_MockObject */
protected $userSession; protected $userSession;
/** @var \OCP\IGroupManager */ /** @var IGroupManager|\PHPUnit_Framework_MockObject_MockObject */
protected $groupManager; protected $groupManager;
/** @var \OCP\IAppConfig */ /** @var IAppConfig|\PHPUnit_Framework_MockObject_MockObject */
protected $appConfig; protected $appConfig;
/** @var \OCP\ICache */ /** @var ICache|\PHPUnit_Framework_MockObject_MockObject */
protected $cache; protected $cache;
/** @var \OCP\ICacheFactory */ /** @var ICacheFactory|\PHPUnit_Framework_MockObject_MockObject */
protected $cacheFactory; protected $cacheFactory;
/** @var \OCP\App\IAppManager */ /** @var EventDispatcherInterface|\PHPUnit_Framework_MockObject_MockObject */
protected $manager;
/** @var \Symfony\Component\EventDispatcher\EventDispatcherInterface */
protected $eventDispatcher; protected $eventDispatcher;
/** @var IAppManager */
protected $manager;
protected function setUp() { protected function setUp() {
parent::setUp(); parent::setUp();
$this->userSession = $this->getMockBuilder('\OCP\IUserSession') $this->userSession = $this->createMock(IUserSession::class);
->disableOriginalConstructor() $this->groupManager = $this->createMock(IGroupManager::class);
->getMock();
$this->groupManager = $this->getMockBuilder('\OCP\IGroupManager')
->disableOriginalConstructor()
->getMock();
$this->appConfig = $this->getAppConfig(); $this->appConfig = $this->getAppConfig();
$this->cacheFactory = $this->getMockBuilder('\OCP\ICacheFactory') $this->cacheFactory = $this->createMock(ICacheFactory::class);
->disableOriginalConstructor() $this->cache = $this->createMock(ICache::class);
->getMock(); $this->eventDispatcher = $this->createMock(EventDispatcherInterface::class);
$this->cache = $this->getMockBuilder('\OCP\ICache')
->disableOriginalConstructor()
->getMock();
$this->eventDispatcher = $this->getMockBuilder('\Symfony\Component\EventDispatcher\EventDispatcherInterface')
->disableOriginalConstructor()
->getMock();
$this->cacheFactory->expects($this->any()) $this->cacheFactory->expects($this->any())
->method('create') ->method('create')
->with('settings') ->with('settings')
->willReturn($this->cache); ->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() { protected function expectClearCache() {
@ -134,10 +132,11 @@ class ManagerTest extends TestCase {
try { try {
$this->manager->enableApp('some_random_name_which_i_hope_is_not_an_app'); $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.'); $this->assertFalse(true, 'If this line is reached the expected exception is not thrown.');
} catch (\Exception $e) { } catch (AppPathNotFoundException $e) {
// excpetion is expected // Exception 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()); $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( $this->assertEquals('no', $this->appConfig->getValue(
'some_random_name_which_i_hope_is_not_an_app', 'enabled', 'no' 'some_random_name_which_i_hope_is_not_an_app', 'enabled', 'no'
)); ));
@ -177,8 +176,8 @@ class ManagerTest extends TestCase {
); );
$this->expectClearCache(); $this->expectClearCache();
/** @var \OC\App\AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */ /** @var AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */
$manager = $this->getMockBuilder('OC\App\AppManager') $manager = $this->getMockBuilder(AppManager::class)
->setConstructorArgs([ ->setConstructorArgs([
$this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher $this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher
]) ])
@ -220,8 +219,8 @@ class ManagerTest extends TestCase {
new Group('group2', array(), null) new Group('group2', array(), null)
); );
/** @var \OC\App\AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */ /** @var AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */
$manager = $this->getMockBuilder('OC\App\AppManager') $manager = $this->getMockBuilder(AppManager::class)
->setConstructorArgs([ ->setConstructorArgs([
$this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher $this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher
]) ])
@ -256,12 +255,8 @@ class ManagerTest extends TestCase {
} }
private function newUser($uid) { private function newUser($uid) {
$config = $this->getMockBuilder('\OCP\IConfig') $config = $this->createMock(IConfig::class);
->disableOriginalConstructor() $urlgenerator = $this->createMock(IURLGenerator::class);
->getMock();
$urlgenerator = $this->getMockBuilder('\OCP\IURLGenerator')
->disableOriginalConstructor()
->getMock();
return new User($uid, null, null, $config, $urlgenerator); return new User($uid, null, null, $config, $urlgenerator);
} }
@ -311,7 +306,7 @@ class ManagerTest extends TestCase {
public function testIsEnabledForUserLoggedOut() { public function testIsEnabledForUserLoggedOut() {
$this->appConfig->setValue('test', 'enabled', '["foo"]'); $this->appConfig->setValue('test', 'enabled', '["foo"]');
$this->assertFalse($this->manager->IsEnabledForUser('test')); $this->assertFalse($this->manager->isEnabledForUser('test'));
} }
public function testIsEnabledForUserLoggedIn() { public function testIsEnabledForUserLoggedIn() {
@ -373,7 +368,8 @@ class ManagerTest extends TestCase {
} }
public function testGetAppsNeedingUpgrade() { 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]) ->setConstructorArgs([$this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher])
->setMethods(['getAppInfo']) ->setMethods(['getAppInfo'])
->getMock(); ->getMock();
@ -393,7 +389,7 @@ class ManagerTest extends TestCase {
'workflowengine' => ['id' => 'workflowengine'], 'workflowengine' => ['id' => 'workflowengine'],
]; ];
$this->manager->expects($this->any()) $manager->expects($this->any())
->method('getAppInfo') ->method('getAppInfo')
->will($this->returnCallback( ->will($this->returnCallback(
function($appId) use ($appInfos) { function($appId) use ($appInfos) {
@ -410,7 +406,7 @@ class ManagerTest extends TestCase {
$this->appConfig->setValue('test4', 'enabled', 'yes'); $this->appConfig->setValue('test4', 'enabled', 'yes');
$this->appConfig->setValue('test4', 'installed_version', '2.4.0'); $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->assertCount(2, $apps);
$this->assertEquals('test1', $apps[0]['id']); $this->assertEquals('test1', $apps[0]['id']);
@ -418,7 +414,8 @@ class ManagerTest extends TestCase {
} }
public function testGetIncompatibleApps() { 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]) ->setConstructorArgs([$this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher])
->setMethods(['getAppInfo']) ->setMethods(['getAppInfo'])
->getMock(); ->getMock();
@ -437,7 +434,7 @@ class ManagerTest extends TestCase {
'workflowengine' => ['id' => 'workflowengine'], 'workflowengine' => ['id' => 'workflowengine'],
]; ];
$this->manager->expects($this->any()) $manager->expects($this->any())
->method('getAppInfo') ->method('getAppInfo')
->will($this->returnCallback( ->will($this->returnCallback(
function($appId) use ($appInfos) { function($appId) use ($appInfos) {
@ -449,7 +446,7 @@ class ManagerTest extends TestCase {
$this->appConfig->setValue('test2', 'enabled', 'yes'); $this->appConfig->setValue('test2', 'enabled', 'yes');
$this->appConfig->setValue('test3', '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->assertCount(2, $apps);
$this->assertEquals('test1', $apps[0]['id']); $this->assertEquals('test1', $apps[0]['id']);