From 2b58e8489fcb5eff0e2312209beca038bfe9b40a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 2 Feb 2015 14:47:29 +0100 Subject: [PATCH 01/10] Add getInstalledApps and getAppsForUser to the app manager --- lib/private/app/appmanager.php | 68 +++++++++++++++++++++++++--------- lib/public/app/iappmanager.php | 17 +++++++++ tests/lib/app/manager.php | 32 ++++++++++++++++ 3 files changed, 99 insertions(+), 18 deletions(-) diff --git a/lib/private/app/appmanager.php b/lib/private/app/appmanager.php index 20a765e343..f35fc3e5e6 100644 --- a/lib/private/app/appmanager.php +++ b/lib/private/app/appmanager.php @@ -12,6 +12,7 @@ namespace OC\App; use OCP\App\IAppManager; use OCP\IAppConfig; use OCP\IGroupManager; +use OCP\IUser; use OCP\IUserSession; class AppManager implements IAppManager { @@ -49,7 +50,7 @@ class AppManager implements IAppManager { /** * @return string[] $appId => $enabled */ - private function getInstalledApps() { + private function getInstalledAppsValues() { if (!$this->installedAppsCache) { $values = $this->appConfig->getValues(false, 'enabled'); $this->installedAppsCache = array_filter($values, function ($value) { @@ -60,6 +61,29 @@ class AppManager implements IAppManager { return $this->installedAppsCache; } + /** + * List all installed apps + * + * @return string[] + */ + public function getInstalledApps() { + return array_keys($this->getInstalledAppsValues()); + } + + /** + * List all apps enabled for a user + * + * @param \OCP\IUser $user + * @return string[] + */ + public function getAppsEnabledForUser(IUser $user) { + $apps = $this->getInstalledAppsValues(); + $appsForUser = array_filter($apps, function ($enabled) use ($user) { + return $this->checkAppForUser($enabled, $user); + }); + return array_keys($appsForUser); + } + /** * Check if an app is enabled for user * @@ -71,28 +95,36 @@ class AppManager implements IAppManager { if (is_null($user)) { $user = $this->userSession->getUser(); } - $installedApps = $this->getInstalledApps(); + $installedApps = $this->getInstalledAppsValues(); if (isset($installedApps[$appId])) { - $enabled = $installedApps[$appId]; - if ($enabled === 'yes') { - return true; - } elseif (is_null($user)) { - return false; - } else { - $groupIds = json_decode($enabled); - $userGroups = $this->groupManager->getUserGroupIds($user); - foreach ($userGroups as $groupId) { - if (array_search($groupId, $groupIds) !== false) { - return true; - } - } - return false; - } + return $this->checkAppForUser($installedApps[$appId], $user); } else { return false; } } + /** + * @param string $enabled + * @param IUser $user + * @return bool + */ + private function checkAppForUser($enabled, $user) { + if ($enabled === 'yes') { + return true; + } elseif (is_null($user)) { + return false; + } else { + $groupIds = json_decode($enabled); + $userGroups = $this->groupManager->getUserGroupIds($user); + foreach ($userGroups as $groupId) { + if (array_search($groupId, $groupIds) !== false) { + return true; + } + } + return false; + } + } + /** * Check if an app is installed in the instance * @@ -100,7 +132,7 @@ class AppManager implements IAppManager { * @return bool */ public function isInstalled($appId) { - $installedApps = $this->getInstalledApps(); + $installedApps = $this->getInstalledAppsValues(); return isset($installedApps[$appId]); } diff --git a/lib/public/app/iappmanager.php b/lib/public/app/iappmanager.php index ebd84a1ce9..c79dcf9a57 100644 --- a/lib/public/app/iappmanager.php +++ b/lib/public/app/iappmanager.php @@ -9,6 +9,8 @@ namespace OCP\App; +use OCP\IUser; + interface IAppManager { /** * Check if an app is enabled for user @@ -48,4 +50,19 @@ interface IAppManager { * @param string $appId */ public function disableApp($appId); + + /** + * List all apps enabled for a user + * + * @param \OCP\IUser $user + * @return string[] + */ + public function getAppsEnabledForUser(IUser $user); + + /** + * List all installed apps + * + * @return string[] + */ + public function getInstalledApps(); } diff --git a/tests/lib/app/manager.php b/tests/lib/app/manager.php index 4c0555b501..4eaf4f08be 100644 --- a/tests/lib/app/manager.php +++ b/tests/lib/app/manager.php @@ -192,4 +192,36 @@ class Manager extends \PHPUnit_Framework_TestCase { $appConfig->setValue('test', 'enabled', '["foo"]'); $this->assertTrue($manager->isEnabledForUser('test')); } + + public function testGetInstalledApps() { + $userSession = $this->getMock('\OCP\IUserSession'); + $groupManager = $this->getMock('\OCP\IGroupManager'); + + $appConfig = $this->getAppConfig(); + $manager = new \OC\App\AppManager($userSession, $appConfig, $groupManager); + $appConfig->setValue('test1', 'enabled', 'yes'); + $appConfig->setValue('test2', 'enabled', 'no'); + $appConfig->setValue('test3', 'enabled', '["foo"]'); + $this->assertEquals(['test1', 'test3'], $manager->getInstalledApps()); + } + + public function testGetAppsForUser() { + $userSession = $this->getMock('\OCP\IUserSession'); + $groupManager = $this->getMock('\OCP\IGroupManager'); + + $user = new User('user1', null); + + $groupManager->expects($this->any()) + ->method('getUserGroupIds') + ->with($user) + ->will($this->returnValue(array('foo', 'bar'))); + + $appConfig = $this->getAppConfig(); + $manager = new \OC\App\AppManager($userSession, $appConfig, $groupManager); + $appConfig->setValue('test1', 'enabled', 'yes'); + $appConfig->setValue('test2', 'enabled', 'no'); + $appConfig->setValue('test3', 'enabled', '["foo"]'); + $appConfig->setValue('test4', 'enabled', '["asd"]'); + $this->assertEquals(['test1', 'test3'], $manager->getAppsEnabledForUser($user)); + } } From 23ab25e93a62fcc4a41ec74db2b32741f98ab34a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 2 Feb 2015 14:47:38 +0100 Subject: [PATCH 02/10] Use the app manager from oc_app --- lib/private/app.php | 142 +++++++++++++++++++++----------------------- tests/lib/app.php | 13 +++- 2 files changed, 77 insertions(+), 78 deletions(-) diff --git a/lib/private/app.php b/lib/private/app.php index a92ddd40e6..c767ddaec9 100644 --- a/lib/private/app.php +++ b/lib/private/app.php @@ -46,6 +46,7 @@ class OC_App { /** * clean the appId + * * @param string|boolean $app AppId that needs to be cleaned * @return string */ @@ -55,6 +56,7 @@ class OC_App { /** * loads all apps + * * @param array $types * @return bool * @@ -200,7 +202,7 @@ class OC_App { * @param bool $forceRefresh whether to refresh the cache * @param bool $all whether to return apps for all users, not only the * currently logged in one - * @return array + * @return string[] */ public static function getEnabledApps($forceRefresh = false, $all = false) { if (!OC_Config::getValue('installed', false)) { @@ -208,52 +210,29 @@ class OC_App { } // in incognito mode or when logged out, $user will be false, // which is also the case during an upgrade - $user = null; - if (!$all) { - $user = \OC_User::getUser(); + $appManager = \OC::$server->getAppManager(); + if ($all) { + $user = null; + } else { + $user = \OC::$server->getUserSession()->getUser(); } - if (is_string($user) && !$forceRefresh && !empty(self::$enabledAppsCache)) { - return self::$enabledAppsCache; - } - $apps = array(); - $appConfig = \OC::$server->getAppConfig(); - $appStatus = $appConfig->getValues(false, 'enabled'); - foreach ($appStatus as $app => $enabled) { - if ($app === 'files') { - continue; - } - if ($enabled === 'yes') { - $apps[] = $app; - } else if ($enabled !== 'no') { - $groups = json_decode($enabled); - if (is_array($groups)) { - if (is_string($user)) { - foreach ($groups as $group) { - if (\OC_Group::inGroup($user, $group)) { - $apps[] = $app; - break; - } - } - } else { - // global, consider app as enabled - $apps[] = $app; - } - } - } + + if (is_null($user)) { + $apps = $appManager->getInstalledApps(); + } else { + $apps = $appManager->getAppsEnabledForUser($user); } + $apps = array_filter($apps, function ($app) { + return $app !== 'files';//we add this manually + }); sort($apps); array_unshift($apps, 'files'); - // Only cache the app list, when the user is logged in. - // Otherwise we cache the list with disabled apps, although - // the apps are enabled for the user after he logged in. - if ($user) { - self::$enabledAppsCache = $apps; - } return $apps; } /** * checks whether or not an app is enabled + * * @param string $app app * @return bool * @@ -263,12 +242,12 @@ class OC_App { if ('files' == $app) { return true; } - $enabledApps = self::getEnabledApps(); - return in_array($app, $enabledApps); + return \OC::$server->getAppManager()->isEnabledForUser($app); } /** * enables an app + * * @param mixed $app app * @param array $groups (optional) when set, only these groups will have access to the app * @throws \Exception @@ -284,7 +263,7 @@ class OC_App { if (!is_null($groups)) { OC_Appconfig::setValue($app, 'enabled', json_encode($groups)); - }else{ + } else { OC_Appconfig::setValue($app, 'enabled', 'yes'); } } @@ -294,13 +273,13 @@ class OC_App { * @return int */ public static function downloadApp($app) { - $appData=OC_OCSClient::getApplication($app); - $download=OC_OCSClient::getApplicationDownload($app, 1); - if(isset($download['downloadlink']) and $download['downloadlink']!='') { + $appData = OC_OCSClient::getApplication($app); + $download = OC_OCSClient::getApplicationDownload($app, 1); + if (isset($download['downloadlink']) and $download['downloadlink'] != '') { // Replace spaces in download link without encoding entire URL $download['downloadlink'] = str_replace(' ', '%20', $download['downloadlink']); - $info = array('source'=>'http', 'href'=>$download['downloadlink'], 'appdata'=>$appData); - $app=OC_Installer::installApp($info); + $info = array('source' => 'http', 'href' => $download['downloadlink'], 'appdata' => $appData); + $app = OC_Installer::installApp($info); } return $app; } @@ -319,6 +298,7 @@ class OC_App { /** * This function set an app as disabled in appconfig. + * * @param string $app app */ public static function disable($app) { @@ -328,11 +308,12 @@ class OC_App { self::$enabledAppsCache = array(); // flush // check if app is a shipped app or not. if not delete \OC_Hook::emit('OC_App', 'pre_disable', array('app' => $app)); - OC_Appconfig::setValue($app, 'enabled', 'no' ); + OC_Appconfig::setValue($app, 'enabled', 'no'); } /** * adds an entry to the navigation + * * @param array $data array containing the data * @return bool * @@ -355,6 +336,7 @@ class OC_App { /** * marks a navigation entry as active + * * @param string $id id of the entry * @return bool * @@ -369,6 +351,7 @@ class OC_App { /** * Get the navigation entries for the $app + * * @param string $app app * @return array an array of the $data added with addNavigationEntry * @@ -385,6 +368,7 @@ class OC_App { /** * gets the active Menu entry + * * @return string id or empty string * * This function returns the id of the active navigation entry (set by @@ -396,6 +380,7 @@ class OC_App { /** * Returns the Settings Navigation + * * @return string * * This function returns an array containing all settings pages added. The @@ -513,6 +498,7 @@ class OC_App { /** * search for an app in all app-directories + * * @param $appId * @return mixed (bool|string) */ @@ -524,21 +510,21 @@ class OC_App { } $possibleApps = array(); - foreach(OC::$APPSROOTS as $dir) { - if(file_exists($dir['path'] . '/' . $appId)) { + foreach (OC::$APPSROOTS as $dir) { + if (file_exists($dir['path'] . '/' . $appId)) { $possibleApps[] = $dir; } } if (empty($possibleApps)) { return false; - } elseif(count($possibleApps) === 1) { + } elseif (count($possibleApps) === 1) { $dir = array_shift($possibleApps); $app_dir[$appId] = $dir; return $dir; } else { $versionToLoad = array(); - foreach($possibleApps as $possibleApp) { + foreach ($possibleApps as $possibleApp) { $version = self::getAppVersionByPath($possibleApp['path']); if (empty($versionToLoad) || version_compare($version, $versionToLoad['version'], '>')) { $versionToLoad = array( @@ -613,15 +599,16 @@ class OC_App { /** * get app's version based on it's path + * * @param string $path * @return string */ public static function getAppVersionByPath($path) { $versionFile = $path . '/appinfo/version'; $infoFile = $path . '/appinfo/info.xml'; - if(is_file($versionFile)) { + if (is_file($versionFile)) { return trim(file_get_contents($versionFile)); - }else{ + } else { $appData = self::getAppInfo($infoFile, true); return isset($appData['version']) ? $appData['version'] : ''; } @@ -649,7 +636,7 @@ class OC_App { $parser = new \OC\App\InfoParser(\OC::$server->getHTTPHelper(), \OC::$server->getURLGenerator()); $data = $parser->parse($file); - if(is_array($data)) { + if (is_array($data)) { $data = OC_App::parseAppInfo($data); } @@ -660,6 +647,7 @@ class OC_App { /** * Returns the navigation + * * @return array * * This function returns an array containing all entries added. The @@ -753,6 +741,7 @@ class OC_App { /** * get a list of all apps in the apps folder + * * @return array an array of app names (string IDs) * @todo: change the name of this method to getInstalledApps, which is more accurate */ @@ -786,6 +775,7 @@ class OC_App { /** * Lists all apps, this is used in apps.php + * * @return array */ public static function listAllApps($onlyLocal = false) { @@ -812,7 +802,7 @@ class OC_App { $info['groups'] = null; if ($enabled === 'yes') { $active = true; - } else if($enabled === 'no') { + } else if ($enabled === 'no') { $active = false; } else { $active = true; @@ -821,7 +811,7 @@ class OC_App { $info['active'] = $active; - if(isset($info['shipped']) and ($info['shipped'] == 'true')) { + if (isset($info['shipped']) and ($info['shipped'] == 'true')) { $info['internal'] = true; $info['internallabel'] = (string)$l->t('Recommended'); $info['internalclass'] = 'recommendedapp'; @@ -833,9 +823,9 @@ class OC_App { $info['update'] = OC_Installer::isUpdateAvailable($app); - $appIcon = self::getAppPath($app) . '/img/' . $app.'.svg'; + $appIcon = self::getAppPath($app) . '/img/' . $app . '.svg'; if (file_exists($appIcon)) { - $info['preview'] = OC_Helper::imagePath($app, $app.'.svg'); + $info['preview'] = OC_Helper::imagePath($app, $app . '.svg'); $info['previewAsIcon'] = true; } else { $appIcon = self::getAppPath($app) . '/img/app.svg'; @@ -859,7 +849,8 @@ class OC_App { foreach ($remoteApps AS $key => $remote) { if ($app['name'] === $remote['name'] || (isset($app['ocsid']) && - $app['ocsid'] === $remote['id'])) { + $app['ocsid'] === $remote['id']) + ) { unset($remoteApps[$key]); } } @@ -902,6 +893,7 @@ class OC_App { /** * get a list of all apps on apps.owncloud.com + * * @return array|false multi-dimensional array of apps. * Keys: id, name, type, typename, personid, license, detailpage, preview, changed, description */ @@ -981,7 +973,7 @@ class OC_App { foreach ($apps as $app) { // check if the app is compatible with this version of ownCloud $info = OC_App::getAppInfo($app); - if(!self::isAppCompatible($version, $info)) { + if (!self::isAppCompatible($version, $info)) { OC_Log::write('core', 'App "' . $info['name'] . '" (' . $app . ') can\'t be used because it is' . ' not compatible with this version of ownCloud', @@ -1028,11 +1020,11 @@ class OC_App { * and "requiremax" => 6 and it will still match ownCloud 6.0.3. * * @param string $ocVersion ownCloud version to check against - * @param array $appInfo app info (from xml) + * @param array $appInfo app info (from xml) * * @return boolean true if compatible, otherwise false */ - public static function isAppCompatible($ocVersion, $appInfo){ + public static function isAppCompatible($ocVersion, $appInfo) { $requireMin = ''; $requireMax = ''; if (isset($appInfo['dependencies']['owncloud']['@attributes']['min-version'])) { @@ -1102,25 +1094,25 @@ class OC_App { public static function installApp($app) { $l = \OC::$server->getL10N('core'); $config = \OC::$server->getConfig(); - $appData=OC_OCSClient::getApplication($app); + $appData = OC_OCSClient::getApplication($app); // check if app is a shipped app or not. OCS apps have an integer as id, shipped apps use a string - if(!is_numeric($app)) { - $shippedVersion=self::getAppVersion($app); - if($appData && version_compare($shippedVersion, $appData['version'], '<')) { + if (!is_numeric($app)) { + $shippedVersion = self::getAppVersion($app); + if ($appData && version_compare($shippedVersion, $appData['version'], '<')) { $app = self::downloadApp($app); } else { $app = OC_Installer::installShippedApp($app); } - }else{ + } else { $app = self::downloadApp($app); } - if($app!==false) { + if ($app !== false) { // check if the app is compatible with this version of ownCloud $info = self::getAppInfo($app); - $version=OC_Util::getVersion(); - if(!self::isAppCompatible($version, $info)) { + $version = OC_Util::getVersion(); + if (!self::isAppCompatible($version, $info)) { throw new \Exception( $l->t('App \"%s\" can\'t be installed because it is not compatible with this version of ownCloud.', array($info['name']) @@ -1131,7 +1123,7 @@ class OC_App { // check for required dependencies $dependencyAnalyzer = new DependencyAnalyzer(new Platform($config), $l); $missing = $dependencyAnalyzer->analyze($app); - if(!empty($missing)) { + if (!empty($missing)) { $missingMsg = join(PHP_EOL, $missing); throw new \Exception( $l->t('App \"%s\" cannot be installed because the following dependencies are not fulfilled: %s', @@ -1141,11 +1133,11 @@ class OC_App { } $config->setAppValue($app, 'enabled', 'yes'); - if(isset($appData['id'])) { - $config->setAppValue($app, 'ocsid', $appData['id'] ); + if (isset($appData['id'])) { + $config->setAppValue($app, 'ocsid', $appData['id']); } \OC_Hook::emit('OC_App', 'post_enable', array('app' => $app)); - }else{ + } else { throw new \Exception($l->t("No app name specified")); } @@ -1223,7 +1215,7 @@ class OC_App { // just modify the description if it is available // otherwise this will create a $data element with an empty 'description' - if(isset($data['description'])) { + if (isset($data['description'])) { // sometimes the description contains line breaks and they are then also // shown in this way in the app management which isn't wanted as HTML // manages line breaks itself diff --git a/tests/lib/app.php b/tests/lib/app.php index 0c0eb28b3b..c16bc0566f 100644 --- a/tests/lib/app.php +++ b/tests/lib/app.php @@ -10,6 +10,7 @@ class Test_App extends \Test\TestCase { private $oldAppConfigService; + private $oldAppManagerService; const TEST_USER1 = 'user1'; const TEST_USER2 = 'user2'; @@ -491,18 +492,24 @@ class Test_App extends \Test\TestCase { */ private function registerAppConfig($appConfig) { $this->oldAppConfigService = \OC::$server->query('AppConfig'); + $this->oldAppManagerService = \OC::$server->query('AppManager'); \OC::$server->registerService('AppConfig', function ($c) use ($appConfig) { return $appConfig; }); + \OC::$server->registerService('AppManager', function (\OC\Server $c) use ($appConfig) { + return new \OC\App\AppManager($c->getUserSession(), $appConfig, $c->getGroupManager()); + }); } /** * Restore the original app config service. */ private function restoreAppConfig() { - $oldService = $this->oldAppConfigService; - \OC::$server->registerService('AppConfig', function ($c) use ($oldService){ - return $oldService; + \OC::$server->registerService('AppConfig', function ($c){ + return $this->oldAppConfigService; + }); + \OC::$server->registerService('AppManager', function ($c) { + return $this->oldAppManagerService; }); // Remove the cache of the mocked apps list with a forceRefresh From 5c68c81d004542e6bf4b88d323b0e9814b52fb37 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 16 Feb 2015 16:44:35 +0100 Subject: [PATCH 03/10] Update cache when enabling/disabling apps --- lib/private/app/appmanager.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/private/app/appmanager.php b/lib/private/app/appmanager.php index f35fc3e5e6..c5006593e5 100644 --- a/lib/private/app/appmanager.php +++ b/lib/private/app/appmanager.php @@ -142,6 +142,7 @@ class AppManager implements IAppManager { * @param string $appId */ public function enableApp($appId) { + $this->installedAppsCache[$appId] = 'yes'; $this->appConfig->setValue($appId, 'enabled', 'yes'); } @@ -156,6 +157,7 @@ class AppManager implements IAppManager { /** @var \OCP\IGroup $group */ return $group->getGID(); }, $groups); + $this->installedAppsCache[$appId] = json_encode($groupIds); $this->appConfig->setValue($appId, 'enabled', json_encode($groupIds)); } @@ -166,9 +168,10 @@ class AppManager implements IAppManager { * @throws \Exception if app can't be disabled */ public function disableApp($appId) { - if($appId === 'files') { + if ($appId === 'files') { throw new \Exception("files can't be disabled."); } + unset($this->installedAppsCache[$appId]); $this->appConfig->setValue($appId, 'enabled', 'no'); } } From 409453bc60ac07392baf4c2b6b6ce800ed11c9ee Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 2 Feb 2015 14:56:01 +0100 Subject: [PATCH 04/10] better user group caching --- lib/private/group/manager.php | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/lib/private/group/manager.php b/lib/private/group/manager.php index 8dcf14fc1d..765675974b 100644 --- a/lib/private/group/manager.php +++ b/lib/private/group/manager.php @@ -217,16 +217,7 @@ class Manager extends PublicEmitter implements IGroupManager { * @return array with group ids */ public function getUserGroupIds($user) { - $groupIds = array(); - $userId = $user->getUID(); - if (isset($this->cachedUserGroups[$userId])) { - return array_keys($this->cachedUserGroups[$userId]); - } else { - foreach ($this->backends as $backend) { - $groupIds = array_merge($groupIds, $backend->getUserGroups($userId)); - } - } - return $groupIds; + return array_keys($this->getUserGroups($user)); } /** From 4d805b52cc802735534f84fdb43be2e555bb68e9 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 2 Feb 2015 15:47:54 +0100 Subject: [PATCH 05/10] Test improvements --- tests/lib/app.php | 47 +++++++++-------------------------------- tests/lib/appconfig.php | 6 ++++++ 2 files changed, 16 insertions(+), 37 deletions(-) diff --git a/tests/lib/app.php b/tests/lib/app.php index c16bc0566f..86a407c1a9 100644 --- a/tests/lib/app.php +++ b/tests/lib/app.php @@ -1,4 +1,5 @@ * Copyright (c) 2014 Vincent Petry @@ -6,12 +7,8 @@ * later. * See the COPYING-README file. */ - class Test_App extends \Test\TestCase { - private $oldAppConfigService; - private $oldAppManagerService; - const TEST_USER1 = 'user1'; const TEST_USER2 = 'user2'; const TEST_USER3 = 'user3'; @@ -399,10 +396,9 @@ class Test_App extends \Test\TestCase { 'appforgroup12' => '["group2","group1"]', ) ) - ); + ); $apps = \OC_App::getEnabledApps(true, $forceAll); - $this->assertEquals($expectedApps, $apps); $this->restoreAppConfig(); \OC_User::setUserId(null); @@ -413,6 +409,8 @@ class Test_App extends \Test\TestCase { $group1->delete(); $group2->delete(); + + $this->assertEquals($expectedApps, $apps); } /** @@ -433,7 +431,7 @@ class Test_App extends \Test\TestCase { 'app2' => 'no', ) ) - ); + ); $apps = \OC_App::getEnabledApps(true); $this->assertEquals(array('files', 'app3'), $apps); @@ -448,30 +446,6 @@ class Test_App extends \Test\TestCase { $user1->delete(); } - /** - * Tests that the apps list is re-requested (not cached) when - * no user is set. - */ - public function testEnabledAppsNoCache() { - $this->setupAppConfigMock()->expects($this->exactly(2)) - ->method('getValues') - ->will($this->returnValue( - array( - 'app3' => 'yes', - 'app2' => 'no', - ) - ) - ); - - $apps = \OC_App::getEnabledApps(true); - $this->assertEquals(array('files', 'app3'), $apps); - - // mock should be called again here - $apps = \OC_App::getEnabledApps(false); - $this->assertEquals(array('files', 'app3'), $apps); - - $this->restoreAppConfig(); - } private function setupAppConfigMock() { $appConfig = $this->getMock( @@ -488,11 +462,10 @@ class Test_App extends \Test\TestCase { /** * Register an app config mock for testing purposes. + * * @param $appConfig app config mock */ private function registerAppConfig($appConfig) { - $this->oldAppConfigService = \OC::$server->query('AppConfig'); - $this->oldAppManagerService = \OC::$server->query('AppManager'); \OC::$server->registerService('AppConfig', function ($c) use ($appConfig) { return $appConfig; }); @@ -505,11 +478,11 @@ class Test_App extends \Test\TestCase { * Restore the original app config service. */ private function restoreAppConfig() { - \OC::$server->registerService('AppConfig', function ($c){ - return $this->oldAppConfigService; + \OC::$server->registerService('AppConfig', function ($c) { + return new \OC\AppConfig(\OC_DB::getConnection()); }); - \OC::$server->registerService('AppManager', function ($c) { - return $this->oldAppManagerService; + \OC::$server->registerService('AppManager', function (\OC\Server $c) { + return new \OC\App\AppManager($c->getUserSession(), $c->getAppConfig(), $c->getGroupManager()); }); // Remove the cache of the mocked apps list with a forceRefresh diff --git a/tests/lib/appconfig.php b/tests/lib/appconfig.php index 188721ff92..ead5b85927 100644 --- a/tests/lib/appconfig.php +++ b/tests/lib/appconfig.php @@ -11,6 +11,12 @@ class Test_Appconfig extends \Test\TestCase { public static function setUpBeforeClass() { parent::setUpBeforeClass(); + $query = \OC_DB::prepare('DELETE FROM `*PREFIX*appconfig` WHERE `appid` = ?'); + $query->execute(array('testapp')); + $query->execute(array('someapp')); + $query->execute(array('123456')); + $query->execute(array('anotherapp')); + $query = \OC_DB::prepare('INSERT INTO `*PREFIX*appconfig` VALUES (?, ?, ?)'); $query->execute(array('testapp', 'enabled', 'true')); From 434835b326bddcd2e53a9ee46734e4f358a9d1f1 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 2 Feb 2015 16:09:30 +0100 Subject: [PATCH 06/10] also set user in UserSession when doing OC_User::setUserId --- lib/private/user.php | 41 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/lib/private/user.php b/lib/private/user.php index 10457c224f..f5baddb470 100644 --- a/lib/private/user.php +++ b/lib/private/user.php @@ -64,6 +64,7 @@ class OC_User { /** * registers backend + * * @param string $backend name of the backend * @deprecated Add classes by calling OC_User::useBackend() with a class instance instead * @return bool @@ -77,6 +78,7 @@ class OC_User { /** * gets available backends + * * @deprecated * @return array an array of backends * @@ -88,6 +90,7 @@ class OC_User { /** * gets used backends + * * @deprecated * @return array an array of backends * @@ -99,6 +102,7 @@ class OC_User { /** * Adds the backend to the list of used backends + * * @param string|OC_User_Interface $backend default: database The backend to use for user management * @return bool * @@ -171,6 +175,7 @@ class OC_User { /** * Create a new user + * * @param string $uid The username of the user to create * @param string $password The password of the new user * @throws Exception @@ -188,6 +193,7 @@ class OC_User { /** * delete a user + * * @param string $uid The username of the user to delete * @return bool * @@ -205,6 +211,7 @@ class OC_User { /** * Try to login a user + * * @param string $loginname The login name of the user to log in * @param string $password The password of the user * @return boolean|null @@ -245,14 +252,14 @@ class OC_User { $uid = $backend->getCurrentUserId(); $run = true; - OC_Hook::emit( "OC_User", "pre_login", array( "run" => &$run, "uid" => $uid )); + OC_Hook::emit("OC_User", "pre_login", array("run" => &$run, "uid" => $uid)); - if($uid) { + if ($uid) { self::setUserId($uid); self::setDisplayName($uid); self::getUserSession()->setLoginName($uid); - OC_Hook::emit( "OC_User", "post_login", array( "uid" => $uid, 'password'=>'' )); + OC_Hook::emit("OC_User", "post_login", array("uid" => $uid, 'password' => '')); return true; } return false; @@ -286,11 +293,18 @@ class OC_User { * Sets user id for session and triggers emit */ public static function setUserId($uid) { - \OC::$server->getSession()->set('user_id', $uid); + $userSession = \OC::$server->getUserSession(); + $userManager = \OC::$server->getUserManager(); + if ($user = $userManager->get($uid)) { + $userSession->setUser($user); + } else { + \OC::$server->getSession()->set('user_id', $uid); + } } /** * Sets user display name for session + * * @param string $uid * @param null $displayName * @return bool Whether the display name could get set @@ -320,13 +334,14 @@ class OC_User { * Tries to login the user with HTTP Basic Authentication */ public static function tryBasicAuthLogin() { - if(!empty($_SERVER['PHP_AUTH_USER']) && !empty($_SERVER['PHP_AUTH_PW'])) { + if (!empty($_SERVER['PHP_AUTH_USER']) && !empty($_SERVER['PHP_AUTH_PW'])) { \OC_User::login($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW']); } } /** * Check if the user is logged in, considers also the HTTP basic credentials + * * @return bool */ public static function isLoggedIn() { @@ -339,6 +354,7 @@ class OC_User { /** * set incognito mode, e.g. if a user wants to open a public link + * * @param bool $status */ public static function setIncognitoMode($status) { @@ -347,6 +363,7 @@ class OC_User { /** * get incognito mode status + * * @return bool */ public static function isIncognitoMode() { @@ -371,6 +388,7 @@ class OC_User { /** * Check if the user is an admin user + * * @param string $uid uid of the admin * @return bool */ @@ -384,6 +402,7 @@ class OC_User { /** * get the user id of the user currently logged in. + * * @return string uid or false */ public static function getUser() { @@ -397,6 +416,7 @@ class OC_User { /** * get the display name of the user currently logged in. + * * @param string $uid * @return string uid or false */ @@ -420,6 +440,7 @@ class OC_User { /** * Autogenerate a password + * * @return string * * generates a password @@ -430,6 +451,7 @@ class OC_User { /** * Set password + * * @param string $uid The username * @param string $password The new password * @param string $recoveryPassword for the encryption app to reset encryption keys @@ -448,6 +470,7 @@ class OC_User { /** * Check whether user can change his avatar + * * @param string $uid The username * @return bool * @@ -464,6 +487,7 @@ class OC_User { /** * Check whether user can change his password + * * @param string $uid The username * @return bool * @@ -480,6 +504,7 @@ class OC_User { /** * Check whether user can change his display name + * * @param string $uid The username * @return bool * @@ -496,6 +521,7 @@ class OC_User { /** * Check if the password is correct + * * @param string $uid The username * @param string $password The password * @return string|false user id a string on success, false otherwise @@ -530,6 +556,7 @@ class OC_User { /** * Get a list of all users + * * @return array an array of all uids * * Get a list of all users. @@ -548,6 +575,7 @@ class OC_User { /** * Get a list of all users display name + * * @param string $search * @param int $limit * @param int $offset @@ -567,6 +595,7 @@ class OC_User { /** * check if a user exists + * * @param string $uid the username * @return boolean */ @@ -615,6 +644,7 @@ class OC_User { /** * Set cookie value to use in next page load + * * @param string $username username to be set * @param string $token */ @@ -631,6 +661,7 @@ class OC_User { /** * Returns the first active backend from self::$_usedBackends. + * * @return OCP\Authentication\IApacheBackend|null if no backend active, otherwise OCP\Authentication\IApacheBackend */ private static function findFirstActiveUsedBackend() { From 04628cf368983a87f9466e02aa569e4ff646880b Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 5 Feb 2015 15:11:07 +0100 Subject: [PATCH 07/10] better name for getAppsEnabledForUser --- lib/private/app.php | 2 +- lib/private/app/appmanager.php | 2 +- lib/public/app/iappmanager.php | 2 +- tests/lib/app/manager.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/private/app.php b/lib/private/app.php index c767ddaec9..2e640aa2a4 100644 --- a/lib/private/app.php +++ b/lib/private/app.php @@ -220,7 +220,7 @@ class OC_App { if (is_null($user)) { $apps = $appManager->getInstalledApps(); } else { - $apps = $appManager->getAppsEnabledForUser($user); + $apps = $appManager->getEnabledAppsForUser($user); } $apps = array_filter($apps, function ($app) { return $app !== 'files';//we add this manually diff --git a/lib/private/app/appmanager.php b/lib/private/app/appmanager.php index c5006593e5..860d9e70a4 100644 --- a/lib/private/app/appmanager.php +++ b/lib/private/app/appmanager.php @@ -76,7 +76,7 @@ class AppManager implements IAppManager { * @param \OCP\IUser $user * @return string[] */ - public function getAppsEnabledForUser(IUser $user) { + public function getEnabledAppsForUser(IUser $user) { $apps = $this->getInstalledAppsValues(); $appsForUser = array_filter($apps, function ($enabled) use ($user) { return $this->checkAppForUser($enabled, $user); diff --git a/lib/public/app/iappmanager.php b/lib/public/app/iappmanager.php index c79dcf9a57..495b82e0bd 100644 --- a/lib/public/app/iappmanager.php +++ b/lib/public/app/iappmanager.php @@ -57,7 +57,7 @@ interface IAppManager { * @param \OCP\IUser $user * @return string[] */ - public function getAppsEnabledForUser(IUser $user); + public function getEnabledAppsForUser(IUser $user); /** * List all installed apps diff --git a/tests/lib/app/manager.php b/tests/lib/app/manager.php index 4eaf4f08be..cb41f73746 100644 --- a/tests/lib/app/manager.php +++ b/tests/lib/app/manager.php @@ -222,6 +222,6 @@ class Manager extends \PHPUnit_Framework_TestCase { $appConfig->setValue('test2', 'enabled', 'no'); $appConfig->setValue('test3', 'enabled', '["foo"]'); $appConfig->setValue('test4', 'enabled', '["asd"]'); - $this->assertEquals(['test1', 'test3'], $manager->getAppsEnabledForUser($user)); + $this->assertEquals(['test1', 'test3'], $manager->getEnabledAppsForUser($user)); } } From 5394a81c05e610b28aae0ebcf5a59c8470b49266 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 16 Feb 2015 16:41:51 +0100 Subject: [PATCH 08/10] Dont fatal error test when encryption is enabled --- apps/files_sharing/tests/testcase.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/files_sharing/tests/testcase.php b/apps/files_sharing/tests/testcase.php index 65fbfac7d1..d76f8f76eb 100644 --- a/apps/files_sharing/tests/testcase.php +++ b/apps/files_sharing/tests/testcase.php @@ -88,13 +88,13 @@ abstract class TestCase extends \Test\TestCase { protected function setUp() { parent::setUp(); - $this->assertFalse(\OC_App::isEnabled('files_encryption')); - //login as user1 self::loginHelper(self::TEST_FILES_SHARING_API_USER1); $this->data = 'foobar'; $this->view = new \OC\Files\View('/' . self::TEST_FILES_SHARING_API_USER1 . '/files'); + + $this->assertFalse(\OC_App::isEnabled('files_encryption')); } protected function tearDown() { From e672f8cc8ff046d847b1a3eaaf9fbc8159fa59f3 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 16 Feb 2015 16:44:51 +0100 Subject: [PATCH 09/10] Use appmanager in OC_App::enable/disable --- lib/private/app.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/private/app.php b/lib/private/app.php index 2e640aa2a4..3f7e46dccd 100644 --- a/lib/private/app.php +++ b/lib/private/app.php @@ -261,10 +261,11 @@ class OC_App { $app = self::installApp($app); } + $appManager = \OC::$server->getAppManager(); if (!is_null($groups)) { - OC_Appconfig::setValue($app, 'enabled', json_encode($groups)); + $appManager->enableAppForGroups($app, $groups); } else { - OC_Appconfig::setValue($app, 'enabled', 'yes'); + $appManager->enableApp($app); } } @@ -308,7 +309,8 @@ class OC_App { self::$enabledAppsCache = array(); // flush // check if app is a shipped app or not. if not delete \OC_Hook::emit('OC_App', 'pre_disable', array('app' => $app)); - OC_Appconfig::setValue($app, 'enabled', 'no'); + $appManager = \OC::$server->getAppManager(); + $appManager->disableApp($app); } /** From 5542fafd3696033ea8bfdcc441c05522cf6a5736 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 18 Feb 2015 14:24:50 +0100 Subject: [PATCH 10/10] allow overwriting the appmanager in oc_util by subclassing --- lib/private/util.php | 6 ++++- tests/lib/util.php | 53 ++++++++++++++++++++++---------------------- 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/lib/private/util.php b/lib/private/util.php index 2be7e8eb29..b51b7ec51c 100644 --- a/lib/private/util.php +++ b/lib/private/util.php @@ -11,6 +11,10 @@ class OC_Util { private static $rootMounted = false; private static $fsSetup = false; + protected static function getAppManager() { + return \OC::$server->getAppManager(); + } + private static function initLocalStorageRootFS() { // mount local file backend as root $configDataDirectory = OC_Config::getValue("datadirectory", OC::$SERVERROOT . "/data"); @@ -926,7 +930,7 @@ class OC_Util { // find the first app that is enabled for the current user foreach ($defaultApps as $defaultApp) { $defaultApp = OC_App::cleanAppId(strip_tags($defaultApp)); - if (OC_App::isEnabled($defaultApp)) { + if (static::getAppManager()->isEnabledForUser($defaultApp)) { $appId = $defaultApp; break; } diff --git a/tests/lib/util.php b/tests/lib/util.php index 25c9e31bea..6870b21807 100644 --- a/tests/lib/util.php +++ b/tests/lib/util.php @@ -1,11 +1,11 @@ * This file is licensed under the Affero General Public License version 3 or * later. * See the COPYING-README file. */ - class Test_Util extends \Test\TestCase { public function testGetVersion() { $version = \OC_Util::getVersion(); @@ -105,7 +105,7 @@ class Test_Util extends \Test\TestCase { $this->assertEquals('This is a good string without HTML.', $result); } - function testEncodePath(){ + function testEncodePath() { $component = '/§#@test%&^ä/-child'; $result = OC_Util::encodePath($component); $this->assertEquals("/%C2%A7%23%40test%25%26%5E%C3%A4/-child", $result); @@ -210,14 +210,12 @@ class Test_Util extends \Test\TestCase { /** * @dataProvider baseNameProvider */ - public function testBaseName($expected, $file) - { + public function testBaseName($expected, $file) { $base = \OC_Util::basename($file); $this->assertEquals($expected, $base); } - public function baseNameProvider() - { + public function baseNameProvider() { return array( array('public_html', '/home/user/public_html/'), array('public_html', '/home/user/public_html'), @@ -288,11 +286,11 @@ class Test_Util extends \Test\TestCase { \OC_User::createUser($uid, "passwd"); - foreach($groups as $group) { + foreach ($groups as $group) { \OC_Group::createGroup($group); } - foreach($membership as $group) { + foreach ($membership as $group) { \OC_Group::addToGroup($uid, $group); } @@ -308,7 +306,7 @@ class Test_Util extends \Test\TestCase { \OC_User::deleteUser($uid); \OC_User::setUserId(''); - foreach($groups as $group) { + foreach ($groups as $group) { \OC_Group::deleteGroup($group); } @@ -317,7 +315,7 @@ class Test_Util extends \Test\TestCase { } - public function dataProviderForTestIsSharingDisabledForUser() { + public function dataProviderForTestIsSharingDisabledForUser() { return array( // existing groups, groups the user belong to, groups excluded from sharing, expected result array(array('g1', 'g2', 'g3'), array(), array('g1'), false), @@ -327,8 +325,8 @@ class Test_Util extends \Test\TestCase { array(array('g1', 'g2', 'g3'), array('g1', 'g2'), array('g1'), false), array(array('g1', 'g2', 'g3'), array('g1', 'g2'), array('g1', 'g2'), true), array(array('g1', 'g2', 'g3'), array('g1', 'g2'), array('g1', 'g2', 'g3'), true), - ); - } + ); + } /** * Test default apps @@ -341,15 +339,21 @@ class Test_Util extends \Test\TestCase { $oldWebRoot = \OC::$WEBROOT; \OC::$WEBROOT = ''; - Dummy_OC_App::setEnabledApps($enabledApps); + $appManager = $this->getMock('\OCP\App\IAppManager'); + $appManager->expects($this->any()) + ->method('isEnabledForUser') + ->will($this->returnCallback(function($appId) use ($enabledApps){ + return in_array($appId, $enabledApps); + })); + Dummy_OC_Util::$appManager = $appManager; + // need to set a user id to make sure enabled apps are read from cache \OC_User::setUserId($this->getUniqueID()); \OCP\Config::setSystemValue('defaultapp', $defaultAppConfig); - $this->assertEquals('http://localhost/' . $expectedPath, \OC_Util::getDefaultPageUrl()); + $this->assertEquals('http://localhost/' . $expectedPath, Dummy_OC_Util::getDefaultPageUrl()); // restore old state \OC::$WEBROOT = $oldWebRoot; - Dummy_OC_App::restore(); \OCP\Config::setSystemValue('defaultapp', $oldDefaultApps); \OC_User::setUserId(null); } @@ -405,18 +409,15 @@ class Test_Util extends \Test\TestCase { } /** - * Dummy OC Apps class to make it possible to override - * enabled apps + * Dummy OC Util class to make it possible to override the app manager */ -class Dummy_OC_App extends OC_App { - private static $enabledAppsCacheBackup; +class Dummy_OC_Util extends OC_Util { + /** + * @var \OCP\App\IAppManager + */ + public static $appManager; - public static function setEnabledApps($enabledApps) { - self::$enabledAppsCacheBackup = self::$enabledAppsCache; - self::$enabledAppsCache = $enabledApps; - } - - public static function restore() { - self::$enabledAppsCache = self::$enabledAppsCacheBackup; + protected static function getAppManager() { + return self::$appManager; } }