From e05b95636b975dd119b19c14b48f291341464a19 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 2 Sep 2014 14:30:46 +0200 Subject: [PATCH 1/3] Fix upgrade process when apps enabled for specific groups Fix issue where the currently logged user was causing side-effects when upgrading. Now setting incognito mode (no user) on update to make sure the whole apps list is taken into account with getEnabledApps() or isEnabled(). --- core/ajax/update.php | 4 ++ lib/private/app.php | 31 +++++++-- lib/private/util.php | 16 +++-- tests/lib/app.php | 162 +++++++++++++++++++++++++++++++++++++++++++ tests/lib/util.php | 3 + 5 files changed, 203 insertions(+), 13 deletions(-) diff --git a/core/ajax/update.php b/core/ajax/update.php index 627ada080c..1e280a8222 100644 --- a/core/ajax/update.php +++ b/core/ajax/update.php @@ -3,6 +3,10 @@ set_time_limit(0); require_once '../../lib/base.php'; if (OC::checkUpgrade(false)) { + // if a user is currently logged in, their session must be ignored to + // avoid side effects + \OC_User::setIncognitoMode(true); + $l = new \OC_L10N('core'); $eventSource = new OC_EventSource(); $updater = new \OC\Updater(\OC_Log::$object); diff --git a/lib/private/app.php b/lib/private/app.php index d10d352b43..3eed9e3c44 100644 --- a/lib/private/app.php +++ b/lib/private/app.php @@ -172,17 +172,29 @@ class OC_App { */ protected static $enabledAppsCache = array(); - public static function getEnabledApps($forceRefresh = false) { + /** + * Returns apps enabled for the current user. + * + * @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 + */ + public static function getEnabledApps($forceRefresh = false, $all = false) { if (!OC_Config::getValue('installed', false)) { return array(); } - if (!$forceRefresh && !empty(self::$enabledAppsCache)) { + // 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(); + } + if (is_string($user) && !$forceRefresh && !empty(self::$enabledAppsCache)) { return self::$enabledAppsCache; } $apps = array(); $appConfig = \OC::$server->getAppConfig(); $appStatus = $appConfig->getValues(false, 'enabled'); - $user = \OC_User::getUser(); foreach ($appStatus as $app => $enabled) { if ($app === 'files') { continue; @@ -192,11 +204,16 @@ class OC_App { } else if ($enabled !== 'no') { $groups = json_decode($enabled); if (is_array($groups)) { - foreach ($groups as $group) { - if (\OC_Group::inGroup($user, $group)) { - $apps[] = $app; - break; + 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; } } } diff --git a/lib/private/util.php b/lib/private/util.php index adb7fb83d2..d690880f98 100755 --- a/lib/private/util.php +++ b/lib/private/util.php @@ -1490,9 +1490,11 @@ class OC_Util { } /** - * Check whether the instance needs to preform an upgrade + * Check whether the instance needs to perform an upgrade, + * either when the core version is higher or any app requires + * an upgrade. * - * @return bool + * @return bool whether the core or any app needs an upgrade */ public static function needUpgrade() { if (OC_Config::getValue('installed', false)) { @@ -1502,14 +1504,16 @@ class OC_Util { return true; } - // also check for upgrades for apps - $apps = \OC_App::getEnabledApps(); + // also check for upgrades for apps (independently from the user) + $apps = \OC_App::getEnabledApps(false, true); + $shouldUpgrade = false; foreach ($apps as $app) { if (\OC_App::shouldUpgrade($app)) { - return true; + $shouldUpgrade = true; + break; } } - return false; + return $shouldUpgrade; } else { return false; } diff --git a/tests/lib/app.php b/tests/lib/app.php index e2b578fe6b..9873d42baf 100644 --- a/tests/lib/app.php +++ b/tests/lib/app.php @@ -9,6 +9,14 @@ class Test_App extends PHPUnit_Framework_TestCase { + private $oldAppConfigService; + + const TEST_USER1 = 'user1'; + const TEST_USER2 = 'user2'; + const TEST_USER3 = 'user3'; + const TEST_GROUP1 = 'group1'; + const TEST_GROUP2 = 'group2'; + function appVersionsProvider() { return array( // exact match @@ -236,4 +244,158 @@ class Test_App extends PHPUnit_Framework_TestCase { array_unshift($sortedApps, 'files'); $this->assertEquals($sortedApps, $apps); } + + /** + * Providers for the app config values + */ + function appConfigValuesProvider() { + return array( + // logged in user1 + array( + self::TEST_USER1, + array( + 'files', + 'app1', + 'app3', + 'appforgroup1', + 'appforgroup12', + ), + false + ), + // logged in user2 + array( + self::TEST_USER2, + array( + 'files', + 'app1', + 'app3', + 'appforgroup12', + 'appforgroup2', + ), + false + ), + // logged in user3 + array( + self::TEST_USER3, + array( + 'files', + 'app1', + 'app3', + 'appforgroup1', + 'appforgroup12', + 'appforgroup2', + ), + false + ), + // no user, returns all apps + array( + null, + array( + 'files', + 'app1', + 'app3', + 'appforgroup1', + 'appforgroup12', + 'appforgroup2', + ), + false, + ), + // user given, but ask for all + array( + self::TEST_USER1, + array( + 'files', + 'app1', + 'app3', + 'appforgroup1', + 'appforgroup12', + 'appforgroup2', + ), + true, + ), + ); + } + + /** + * Test enabled apps + * + * @dataProvider appConfigValuesProvider + */ + public function testEnabledApps($user, $expectedApps, $forceAll) { + $userManager = \OC::$server->getUserManager(); + $groupManager = \OC::$server->getGroupManager(); + $user1 = $userManager->createUser(self::TEST_USER1, self::TEST_USER1); + $user2 = $userManager->createUser(self::TEST_USER2, self::TEST_USER2); + $user3 = $userManager->createUser(self::TEST_USER3, self::TEST_USER3); + + $group1 = $groupManager->createGroup(self::TEST_GROUP1); + $group1->addUser($user1); + $group1->addUser($user3); + $group2 = $groupManager->createGroup(self::TEST_GROUP2); + $group2->addUser($user2); + $group2->addUser($user3); + + \OC_User::setUserId($user); + + $appConfig = $this->getMock( + '\OC\AppConfig', + array('getValues'), + array(\OC_DB::getConnection()), + '', + false + ); + + $appConfig->expects($this->once()) + ->method('getValues') + ->will($this->returnValue( + array( + 'app3' => 'yes', + 'app2' => 'no', + 'app1' => 'yes', + 'appforgroup1' => '["group1"]', + 'appforgroup2' => '["group2"]', + 'appforgroup12' => '["group2","group1"]', + ) + ) + ); + $this->registerAppConfig($appConfig); + + $apps = \OC_App::getEnabledApps(true, $forceAll); + $this->assertEquals($expectedApps, $apps); + + $this->restoreAppConfig(); + \OC_User::setUserId(null); + + $user1->delete(); + $user2->delete(); + $user3->delete(); + // clear user cache... + $userManager->delete(self::TEST_USER1); + $userManager->delete(self::TEST_USER2); + $userManager->delete(self::TEST_USER3); + $group1->delete(); + $group2->delete(); + } + + /** + * Register an app config mock for testing purposes. + * @param $appConfig app config mock + */ + private function registerAppConfig($appConfig) { + $this->oldAppConfigService = \OC::$server->query('AppConfig'); + \OC::$server->registerService('AppConfig', function ($c) use ($appConfig) { + return $appConfig; + }); + } + + /** + * Restore the original app config service. + */ + private function restoreAppConfig() { + $oldService = $this->oldAppConfigService; + \OC::$server->registerService('AppConfig', function ($c) use ($oldService){ + return $oldService; + }); + } } + diff --git a/tests/lib/util.php b/tests/lib/util.php index c2bb99c3b2..98257e4c60 100644 --- a/tests/lib/util.php +++ b/tests/lib/util.php @@ -303,6 +303,8 @@ class Test_Util extends PHPUnit_Framework_TestCase { \OC::$WEBROOT = ''; Dummy_OC_App::setEnabledApps($enabledApps); + // need to set a user id to make sure enabled apps are read from cache + \OC_User::setUserId(uniqid()); \OCP\Config::setSystemValue('defaultapp', $defaultAppConfig); $this->assertEquals('http://localhost/' . $expectedPath, \OC_Util::getDefaultPageUrl()); @@ -310,6 +312,7 @@ class Test_Util extends PHPUnit_Framework_TestCase { \OC::$WEBROOT = $oldWebRoot; Dummy_OC_App::restore(); \OCP\Config::setSystemValue('defaultapp', $oldDefaultApps); + \OC_User::setUserId(null); } function defaultAppsProvider() { From 9d5f18c02f102024ea273476e760a3029b9c88b1 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 2 Sep 2014 17:28:05 +0200 Subject: [PATCH 2/3] Added test for needUpgrade for core --- tests/lib/util.php | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/lib/util.php b/tests/lib/util.php index 98257e4c60..a2efcca260 100644 --- a/tests/lib/util.php +++ b/tests/lib/util.php @@ -344,6 +344,25 @@ class Test_Util extends PHPUnit_Framework_TestCase { ); } + /** + * Test needUpgrade() when the core version is increased + */ + public function testNeedUpgradeCore() { + $oldConfigVersion = OC_Config::getValue('version', '0.0.0'); + $oldSessionVersion = \OC::$server->getSession()->get('OC_Version'); + + $this->assertFalse(\OCP\Util::needUpgrade()); + + OC_Config::setValue('version', '7.0.0.0'); + \OC::$server->getSession()->set('OC_Version', array(7, 0, 0, 1)); + + $this->assertTrue(\OCP\Util::needUpgrade()); + + OC_Config::setValue('version', $oldConfigVersion); + $oldSessionVersion = \OC::$server->getSession()->set('OC_Version', $oldSessionVersion); + + $this->assertFalse(\OCP\Util::needUpgrade()); + } } /** From 4a93a6e0600e4445333bd22015fc9d22d4251219 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 3 Sep 2014 11:01:59 +0200 Subject: [PATCH 3/3] Added unit tests for cache of enabled apps --- tests/lib/app.php | 84 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 74 insertions(+), 10 deletions(-) diff --git a/tests/lib/app.php b/tests/lib/app.php index 9873d42baf..e538ebec8a 100644 --- a/tests/lib/app.php +++ b/tests/lib/app.php @@ -337,15 +337,7 @@ class Test_App extends PHPUnit_Framework_TestCase { \OC_User::setUserId($user); - $appConfig = $this->getMock( - '\OC\AppConfig', - array('getValues'), - array(\OC_DB::getConnection()), - '', - false - ); - - $appConfig->expects($this->once()) + $this->setupAppConfigMock()->expects($this->once()) ->method('getValues') ->will($this->returnValue( array( @@ -358,7 +350,6 @@ class Test_App extends PHPUnit_Framework_TestCase { ) ) ); - $this->registerAppConfig($appConfig); $apps = \OC_App::getEnabledApps(true, $forceAll); $this->assertEquals($expectedApps, $apps); @@ -377,6 +368,79 @@ class Test_App extends PHPUnit_Framework_TestCase { $group2->delete(); } + /** + * Test isEnabledApps() with cache, not re-reading the list of + * enabled apps more than once when a user is set. + */ + public function testEnabledAppsCache() { + $userManager = \OC::$server->getUserManager(); + $user1 = $userManager->createUser(self::TEST_USER1, self::TEST_USER1); + + \OC_User::setUserId(self::TEST_USER1); + + $this->setupAppConfigMock()->expects($this->once()) + ->method('getValues') + ->will($this->returnValue( + array( + 'app3' => 'yes', + 'app2' => 'no', + ) + ) + ); + + $apps = \OC_App::getEnabledApps(true); + $this->assertEquals(array('files', 'app3'), $apps); + + // mock should not be called again here + $apps = \OC_App::getEnabledApps(false); + $this->assertEquals(array('files', 'app3'), $apps); + + $this->restoreAppConfig(); + \OC_User::setUserId(null); + + $user1->delete(); + // clear user cache... + $userManager->delete(self::TEST_USER1); + } + + /** + * 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( + '\OC\AppConfig', + array('getValues'), + array(\OC_DB::getConnection()), + '', + false + ); + + $this->registerAppConfig($appConfig); + return $appConfig; + } + /** * Register an app config mock for testing purposes. * @param $appConfig app config mock