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().

diff --git a/core/ajax/update.php b/core/ajax/update.php
index 627ada080c87c6cdab4493c43025a4c79be43aff..1e280a822274a0360fece5d26f9e186342b2018a 100644 (file)
--- 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 70f8980d2c15e36396ef4be549e4ef2dd00142e4..754d27c18495fedc40b00bcb16d7ae3079347191 100644 (file)
--- 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 0696a79a7e508590239632c2db7f0ecb0cacabd1..0d6e11dab67dd7fd89ac65cefd2ddfb7cb047fcc 100755 (executable)
--- a/lib/private/util.php
+++ b/lib/private/util.php
@@ -1457,9 +1457,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)) {
@@ -1469,14 +1471,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 e2b578fe6b9185d2223d5a075bf7c4415085e65b..9873d42baf0cf1eb3db0213bba443fd60394c5c6 100644 (file)
--- 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 c2bb99c3b2eaf5d116816c5df1fca39925ded3a5..98257e4c6020f08a1a0d1c367d580ffbd278a494 100644 (file)
--- 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() {