Merge pull request #16075 from nextcloud/bugfix/15823/app-restricted-groups

Remove deleted groups from app restrictions fixes #15823

lib/base.php

@@ -717,6 +717,7 @@ class OC {
 		self::registerEncryptionHooks();
 		self::registerAccountHooks();
 		self::registerResourceCollectionHooks();
+		self::registerAppRestrictionsHooks();
 
 		// Make sure that the application class is not loaded before the database is setup
 		if ($systemConfig->getValue("installed", false)) {
@@ -848,6 +849,30 @@ class OC {
 		\OCP\Util::connectHook('OC_User', 'changeUser', $hookHandler, 'changeUserHook');
 	}
 
+	private static function registerAppRestrictionsHooks() {
+		$groupManager = self::$server->query(\OCP\IGroupManager::class);
+		$groupManager->listen ('\OC\Group', 'postDelete', function (\OCP\IGroup $group) {
+			$appManager = self::$server->getAppManager();
+			$apps = $appManager->getEnabledAppsForGroup($group);
+			foreach ($apps as $appId) {
+				$restrictions = $appManager->getAppRestriction($appId);
+				if (empty($restrictions)) {
+					continue;
+				}
+				$key = array_search($group->getGID(), $restrictions);
+				unset($restrictions[$key]);
+				$restrictions = array_values($restrictions);
+				if (empty($restrictions)) {
+					$appManager->disableApp($appId);
+				}
+				else{
+					$appManager->enableAppForGroups($appId, $restrictions);
+				}
+
+			}
+		});
+	}
+
 	private static function registerResourceCollectionHooks() {
 		\OC\Collaboration\Resources\Listener::register(\OC::$server->getEventDispatcher());
 	}

lib/private/App/AppManager.php

@@ -37,7 +37,9 @@ use OCP\App\AppPathNotFoundException;
 use OCP\App\IAppManager;
 use OCP\App\ManagerEvent;
 use OCP\ICacheFactory;
+use OCP\IGroup;
 use OCP\IGroupManager;
+use OCP\ILogger;
 use OCP\IUser;
 use OCP\IUserSession;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
@@ -71,6 +73,9 @@ class AppManager implements IAppManager {
 	/** @var EventDispatcherInterface */
 	private $dispatcher;
 
+	/** @var ILogger */
+	private $logger;
+
 	/** @var string[] $appId => $enabled */
 	private $installedAppsCache;
 
@@ -97,12 +102,14 @@ class AppManager implements IAppManager {
 								AppConfig $appConfig,
 								IGroupManager $groupManager,
 								ICacheFactory $memCacheFactory,
-								EventDispatcherInterface $dispatcher) {
+								EventDispatcherInterface $dispatcher,
+								ILogger $logger) {
 		$this->userSession = $userSession;
 		$this->appConfig = $appConfig;
 		$this->groupManager = $groupManager;
 		$this->memCacheFactory = $memCacheFactory;
 		$this->dispatcher = $dispatcher;
+		$this->logger = $logger;
 	}
 
 	/**
@@ -148,6 +155,36 @@ class AppManager implements IAppManager {
 		return array_keys($appsForUser);
 	}
 
+	/**
+	 * @param \OCP\IGroup $group
+	 * @return array
+	 */
+	public function getEnabledAppsForGroup(IGroup $group): array {
+		$apps = $this->getInstalledAppsValues();
+		$appsForGroups = array_filter($apps, function ($enabled) use ($group) {
+			return $this->checkAppForGroups($enabled, $group);
+		});
+		return array_keys($appsForGroups);
+	}
+
+	/**
+	 * @param string $appId
+	 * @return array
+	 */
+	public function getAppRestriction(string $appId): array {
+		$values = $this->getInstalledAppsValues();
+
+		if (!isset($values[$appId])) {
+			return [];
+		}
+
+		if ($values[$appId] === 'yes' || $values[$appId] === 'no') {
+			return [];
+		}
+		return json_decode($values[$appId]);
+	}
+
+
 	/**
 	 * Check if an app is enabled for user
 	 *
@@ -189,7 +226,7 @@ class AppManager implements IAppManager {
 
 			if (!is_array($groupIds)) {
 				$jsonError = json_last_error();
-				\OC::$server->getLogger()->warning('AppManger::checkAppForUser - can\'t decode group IDs: ' . print_r($enabled, true) . ' - json error code: ' . $jsonError, ['app' => 'lib']);
+				$this->logger->warning('AppManger::checkAppForUser - can\'t decode group IDs: ' . print_r($enabled, true) . ' - json error code: ' . $jsonError, ['app' => 'lib']);
 				return false;
 			}
 
@@ -203,12 +240,40 @@ class AppManager implements IAppManager {
 		}
 	}
 
+	/**
+	 * @param string $enabled
+	 * @param IGroup $group
+	 * @return bool
+	 */
+	private function checkAppForGroups(string $enabled, IGroup $group): bool {
+		if ($enabled === 'yes') {
+			return true;
+		} elseif ($group === null) {
+			return false;
+		} else {
+			if (empty($enabled)) {
+				return false;
+			}
+
+			$groupIds = json_decode($enabled);
+
+			if (!is_array($groupIds)) {
+				$jsonError = json_last_error();
+				$this->logger->warning('AppManger::checkAppForUser - can\'t decode group IDs: ' . print_r($enabled, true) . ' - json error code: ' . $jsonError, ['app' => 'lib']);
+				return false;
+			}
+
+			return in_array($group->getGID(), $groupIds);
+		}
+	}
+
 	/**
 	 * Check if an app is enabled in the instance
 	 *
 	 * Notice: This actually checks if the app is enabled and not only if it is installed.
 	 *
 	 * @param string $appId
+	 * @param \OCP\IGroup[]|String[] $groups
 	 * @return bool
 	 */
 	public function isInstalled($appId) {
@@ -268,14 +333,18 @@ class AppManager implements IAppManager {
 
 		$groupIds = array_map(function ($group) {
 			/** @var \OCP\IGroup $group */
-			return $group->getGID();
+			return ($group instanceof IGroup)
+				? $group->getGID()
+				: $group;
 		}, $groups);
+
 		$this->installedAppsCache[$appId] = json_encode($groupIds);
 		$this->appConfig->setValue($appId, 'enabled', json_encode($groupIds));
 		$this->dispatcher->dispatch(ManagerEvent::EVENT_APP_ENABLE_FOR_GROUPS, new ManagerEvent(
 			ManagerEvent::EVENT_APP_ENABLE_FOR_GROUPS, $appId, $groups
 		));
 		$this->clearAppsCache();
+
 	}
 
 	/**

lib/private/Server.php

@@ -694,7 +694,8 @@ class Server extends ServerContainer implements IServerContainer {
 				$c->query(\OC\AppConfig::class),
 				$c->getGroupManager(),
 				$c->getMemCacheFactory(),
-				$c->getEventDispatcher()
+				$c->getEventDispatcher(),
+				$c->getLogger()
 			);
 		});
 		$this->registerAlias('AppManager', AppManager::class);

lib/public/App/IAppManager.php

@@ -28,6 +28,7 @@
 namespace OCP\App;
 
 use OCP\IUser;
+use OCP\IGroup;
 
 /**
  * Interface IAppManager
@@ -158,4 +159,18 @@ interface IAppManager {
 	 * @since 9.0.0
 	 */
 	public function getAlwaysEnabledApps();
+
+	/**
+	 * @param \OCP\IGroup $group
+	 * @return String[]
+	 * @since 17.0.0
+	 */
+	public function getEnabledAppsForGroup(IGroup $group): array;
+
+	/**
+	 * @param String $appId
+	 * @return string[]
+	 * @since 17.0.0
+	 */
+	public function getAppRestriction(string $appId): array;
 }

tests/lib/App/AppManagerTest.php

@@ -19,6 +19,7 @@ use OCP\ICache;
 use OCP\ICacheFactory;
 use OCP\IGroup;
 use OCP\IGroupManager;
+use OCP\ILogger;
 use OCP\IUser;
 use OCP\IUserSession;
 use OCP\IAppConfig;
@@ -90,6 +91,9 @@ class AppManagerTest extends TestCase {
 	/** @var EventDispatcherInterface|\PHPUnit_Framework_MockObject_MockObject */
 	protected $eventDispatcher;
 
+	/** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */
+	protected $logger;
+
 	/** @var IAppManager */
 	protected $manager;
 
@@ -102,11 +106,12 @@ class AppManagerTest extends TestCase {
 		$this->cacheFactory = $this->createMock(ICacheFactory::class);
 		$this->cache = $this->createMock(ICache::class);
 		$this->eventDispatcher = $this->createMock(EventDispatcherInterface::class);
+		$this->logger = $this->createMock(ILogger::class);
 		$this->cacheFactory->expects($this->any())
 			->method('createDistributed')
 			->with('settings')
 			->willReturn($this->cache);
-		$this->manager = new 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, $this->logger);
 	}
 
 	protected function expectClearCache() {
@@ -159,7 +164,7 @@ class AppManagerTest extends TestCase {
 		/** @var AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */
 		$manager = $this->getMockBuilder(AppManager::class)
 			->setConstructorArgs([
-				$this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher
+				$this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher, $this->logger
 			])
 			->setMethods([
 				'getAppPath',
@@ -206,7 +211,7 @@ class AppManagerTest extends TestCase {
 		/** @var AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */
 		$manager = $this->getMockBuilder(AppManager::class)
 			->setConstructorArgs([
-				$this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher
+				$this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher, $this->logger
 			])
 			->setMethods([
 				'getAppPath',
@@ -259,7 +264,7 @@ class AppManagerTest extends TestCase {
 		/** @var AppManager|\PHPUnit_Framework_MockObject_MockObject $manager */
 		$manager = $this->getMockBuilder(AppManager::class)
 			->setConstructorArgs([
-				$this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher
+				$this->userSession, $this->appConfig, $this->groupManager, $this->cacheFactory, $this->eventDispatcher, $this->logger
 			])
 			->setMethods([
 				'getAppPath',
@@ -418,7 +423,7 @@ class AppManagerTest extends TestCase {
 	public function testGetAppsNeedingUpgrade() {
 		/** @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, $this->logger])
 			->setMethods(['getAppInfo'])
 			->getMock();
 
@@ -466,7 +471,7 @@ class AppManagerTest extends TestCase {
 	public function testGetIncompatibleApps() {
 		/** @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, $this->logger])
 			->setMethods(['getAppInfo'])
 			->getMock();
 
@@ -504,4 +509,40 @@ class AppManagerTest extends TestCase {
 		$this->assertEquals('test1', $apps[0]['id']);
 		$this->assertEquals('test3', $apps[1]['id']);
 	}
+
+	public function testGetEnabledAppsForGroup() {
+		$group = $this->createMock(IGroup::class);
+		$group->expects($this->any())
+			->method('getGID')
+			->will($this->returnValue('foo'));
+
+		$this->appConfig->setValue('test1', 'enabled', 'yes');
+		$this->appConfig->setValue('test2', 'enabled', 'no');
+		$this->appConfig->setValue('test3', 'enabled', '["foo"]');
+		$this->appConfig->setValue('test4', 'enabled', '["asd"]');
+		$enabled = [
+			'cloud_federation_api',
+			'dav',
+			'federatedfilesharing',
+			'files',
+			'lookup_server_connector',
+			'oauth2',
+			'provisioning_api',
+			'test1',
+			'test3',
+			'twofactor_backupcodes',
+			'workflowengine',
+		];
+		$this->assertEquals($enabled, $this->manager->getEnabledAppsForGroup($group));
+	}
+
+	public function testGetAppRestriction() {
+		$this->appConfig->setValue('test1', 'enabled', 'yes');
+		$this->appConfig->setValue('test2', 'enabled', 'no');
+		$this->appConfig->setValue('test3', 'enabled', '["foo"]');
+
+		$this->assertEquals([], $this->manager->getAppRestriction('test1'));
+		$this->assertEquals([], $this->manager->getAppRestriction('test2'));
+		$this->assertEquals(['foo'], $this->manager->getAppRestriction('test3'));
+	}
 }

tests/lib/AppTest.php

@@ -542,7 +542,8 @@ class AppTest extends \Test\TestCase {
 			$appConfig,
 			\OC::$server->getGroupManager(),
 			\OC::$server->getMemCacheFactory(),
-			\OC::$server->getEventDispatcher()
+			\OC::$server->getEventDispatcher(),
+			\OC::$server->getLogger()
 		));
 	}