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

@@ -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);

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;
 					}
 				}
 			}

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;
 		}

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;
+		});
+	}
 }
+

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