Merge pull request #17896 from nextcloud/fix/noid/consider-create-group-result

take group creation result into consideration
2026-05-28 04:32:30 -04:00 · 2019-11-25 17:16:40 +01:00 · 2019-11-25 17:16:40 +01:00 · b1dffc5c2d
commit b1dffc5c2d
parent 279c0cb2ed b4408e4245
6 changed files with 143 additions and 88 deletions
--- a/core/Command/Group/Add.php
+++ b/core/Command/Group/Add.php
@ -25,6 +25,7 @@ declare(strict_types=1);
 namespace OC\Core\Command\Group;

 use OC\Core\Command\Base;
+use OCP\IGroup;
 use OCP\IGroupManager;
 use Symfony\Component\Console\Input\InputArgument;
 use Symfony\Component\Console\Input\InputInterface;
@ -68,12 +69,17 @@ class Add extends Base {
 			return 1;
 		} else {
 			$group = $this->groupManager->createGroup($gid);
+			if (!$group instanceof IGroup) {
+				$output->writeln('<error>Could not create group</error>');
+				return 2;
+			}
 			$output->writeln('Created group "' . $group->getGID() . '"');

-			$displayName = trim((string) $input->getOption('display-name'));
+			$displayName = trim((string)$input->getOption('display-name'));
 			if ($displayName !== '') {
 				$group->setDisplayName($displayName);
 			}
 		}
+		return 0;
 	}
 }
--- a/core/Command/User/Add.php
+++ b/core/Command/User/Add.php
@ -25,6 +25,7 @@
 namespace OC\Core\Command\User;

 use OC\Files\Filesystem;
+use OCP\IGroup;
 use OCP\IGroupManager;
 use OCP\IUser;
 use OCP\IUserManager;
@ -152,10 +153,14 @@ class Add extends Command {
 			if (!$group) {
 				$this->groupManager->createGroup($groupName);
 				$group = $this->groupManager->get($groupName);
-				$output->writeln('Created group "' . $group->getGID() . '"');
+				if($group instanceof IGroup) {
+					$output->writeln('Created group "' . $group->getGID() . '"');
+				}
+			}
+			if($group instanceof IGroup) {
+				$group->addUser($user);
+				$output->writeln('User "' . $user->getUID() . '" added to group "' . $group->getGID() . '"');
 			}
-			$group->addUser($user);
-			$output->writeln('User "' . $user->getUID() . '" added to group "' . $group->getGID() . '"');
 		}
 	}
 }
--- a/lib/private/Group/Manager.php
+++ b/lib/private/Group/Manager.php
@ -93,8 +93,8 @@ class Manager extends PublicEmitter implements IGroupManager {
 		$this->dispatcher = $dispatcher;
 		$this->logger = $logger;

-		$cachedGroups = & $this->cachedGroups;
-		$cachedUserGroups = & $this->cachedUserGroups;
+		$cachedGroups = &$this->cachedGroups;
+		$cachedUserGroups = &$this->cachedUserGroups;
 		$this->listen('\OC\Group', 'postDelete', function ($group) use (&$cachedGroups, &$cachedUserGroups) {
 			/**
 			 * @var \OC\Group\Group $group
@ -149,6 +149,7 @@ class Manager extends PublicEmitter implements IGroupManager {

 	/**
 	 * Get the active backends
+	 *
 	 * @return \OCP\GroupInterface[]
 	 */
 	public function getBackends() {
@ -163,7 +164,7 @@ class Manager extends PublicEmitter implements IGroupManager {

 	/**
 	 * @param string $gid
-	 * @return \OC\Group\Group
+	 * @return IGroup|null
 	 */
 	public function get($gid) {
 		if (isset($this->cachedGroups[$gid])) {
@ -175,12 +176,12 @@ class Manager extends PublicEmitter implements IGroupManager {
 	/**
 	 * @param string $gid
 	 * @param string $displayName
-	 * @return \OCP\IGroup
+	 * @return \OCP\IGroup|null
 	 */
 	protected function getGroupObject($gid, $displayName = null) {
 		$backends = [];
 		foreach ($this->backends as $backend) {
-			if ($backend->implementsActions(\OC\Group\Backend::GROUP_DETAILS)) {
+			if ($backend->implementsActions(Backend::GROUP_DETAILS)) {
 				$groupData = $backend->getGroupDetails($gid);
 				if (is_array($groupData) && !empty($groupData)) {
 					// take the display name from the first backend that has a non-null one
@ -210,21 +211,22 @@ class Manager extends PublicEmitter implements IGroupManager {

 	/**
 	 * @param string $gid
-	 * @return \OC\Group\Group
+	 * @return IGroup|null
 	 */
 	public function createGroup($gid) {
 		if ($gid === '' || $gid === null) {
-			return false;
+			return null;
 		} else if ($group = $this->get($gid)) {
 			return $group;
 		} else {
-			$this->emit('\OC\Group', 'preCreate', array($gid));
+			$this->emit('\OC\Group', 'preCreate', [$gid]);
 			foreach ($this->backends as $backend) {
-				if ($backend->implementsActions(\OC\Group\Backend::CREATE_GROUP)) {
-					$backend->createGroup($gid);
-					$group = $this->getGroupObject($gid);
-					$this->emit('\OC\Group', 'postCreate', array($group));
-					return $group;
+				if ($backend->implementsActions(Backend::CREATE_GROUP)) {
+					if ($backend->createGroup($gid)) {
+						$group = $this->getGroupObject($gid);
+						$this->emit('\OC\Group', 'postCreate', [$group]);
+						return $group;
+					}
 				}
 			}
 			return null;
@ -260,7 +262,7 @@ class Manager extends PublicEmitter implements IGroupManager {
 	 * @param IUser|null $user
 	 * @return \OC\Group\Group[]
 	 */
-	public function getUserGroups(IUser $user= null) {
+	public function getUserGroups(IUser $user = null) {
 		if (!$user instanceof IUser) {
 			return [];
 		}
@ -295,12 +297,13 @@ class Manager extends PublicEmitter implements IGroupManager {

 	/**
 	 * Checks if a userId is in the admin group
+	 *
 	 * @param string $userId
 	 * @return bool if admin
 	 */
 	public function isAdmin($userId) {
 		foreach ($this->backends as $backend) {
-			if ($backend->implementsActions(\OC\Group\Backend::IS_ADMIN) && $backend->isAdmin($userId)) {
+			if ($backend->implementsActions(Backend::IS_ADMIN) && $backend->isAdmin($userId)) {
 				return true;
 			}
 		}
@ -309,6 +312,7 @@ class Manager extends PublicEmitter implements IGroupManager {

 	/**
 	 * Checks if a userId is in a group
+	 *
 	 * @param string $userId
 	 * @param string $group
 	 * @return bool if in group
@ -319,28 +323,31 @@ class Manager extends PublicEmitter implements IGroupManager {

 	/**
 	 * get a list of group ids for a user
+	 *
 	 * @param IUser $user
 	 * @return array with group ids
 	 */
 	public function getUserGroupIds(IUser $user) {
-		return array_map(function($value) {
-			return (string) $value;
+		return array_map(function ($value) {
+			return (string)$value;
 		}, array_keys($this->getUserGroups($user)));
 	}

 	/**
 	 * get an array of groupid and displayName for a user
+	 *
 	 * @param IUser $user
 	 * @return array ['displayName' => displayname]
 	 */
 	public function getUserGroupNames(IUser $user) {
-		return array_map(function($group) {
+		return array_map(function ($group) {
 			return array('displayName' => $group->getDisplayName());
 		}, $this->getUserGroups($user));
 	}

 	/**
 	 * get a list of all display names in a group
+	 *
 	 * @param string $gid
 	 * @param string $search
 	 * @param int $limit
@ -349,32 +356,32 @@ class Manager extends PublicEmitter implements IGroupManager {
 	 */
 	public function displayNamesInGroup($gid, $search = '', $limit = -1, $offset = 0) {
 		$group = $this->get($gid);
-		if(is_null($group)) {
+		if (is_null($group)) {
 			return [];
 		}

 		$search = trim($search);
 		$groupUsers = [];

-		if(!empty($search)) {
+		if (!empty($search)) {
 			// only user backends have the capability to do a complex search for users
 			$searchOffset = 0;
 			$searchLimit = $limit * 100;
-			if($limit === -1) {
+			if ($limit === -1) {
 				$searchLimit = 500;
 			}

 			do {
 				$filteredUsers = $this->userManager->searchDisplayName($search, $searchLimit, $searchOffset);
-				foreach($filteredUsers as $filteredUser) {
-					if($group->inGroup($filteredUser)) {
-						$groupUsers[]= $filteredUser;
+				foreach ($filteredUsers as $filteredUser) {
+					if ($group->inGroup($filteredUser)) {
+						$groupUsers[] = $filteredUser;
 					}
 				}
 				$searchOffset += $searchLimit;
-			} while(count($groupUsers) < $searchLimit+$offset && count($filteredUsers) >= $searchLimit);
+			} while (count($groupUsers) < $searchLimit + $offset && count($filteredUsers) >= $searchLimit);

-			if($limit === -1) {
+			if ($limit === -1) {
 				$groupUsers = array_slice($groupUsers, $offset);
 			} else {
 				$groupUsers = array_slice($groupUsers, $offset, $limit);
@ -384,7 +391,7 @@ class Manager extends PublicEmitter implements IGroupManager {
 		}

 		$matchingUsers = [];
-		foreach($groupUsers as $groupUser) {
+		foreach ($groupUsers as $groupUser) {
 			$matchingUsers[$groupUser->getUID()] = $groupUser->getDisplayName();
 		}
 		return $matchingUsers;
--- a/lib/private/Setup.php
+++ b/lib/private/Setup.php
@ -50,6 +50,7 @@ use OC\Authentication\Token\DefaultTokenProvider;
 use OC\Log\Rotate;
 use OC\Preview\BackgroundCleanupJob;
 use OCP\Defaults;
+use OCP\IGroup;
 use OCP\IL10N;
 use OCP\ILogger;
 use OCP\IUser;
@ -80,14 +81,15 @@ class Setup {
 	 * @param ISecureRandom $random
 	 * @param Installer $installer
 	 */
-	public function __construct(SystemConfig $config,
-						 IniGetWrapper $iniWrapper,
-						 IL10N $l10n,
-						 Defaults $defaults,
-						 ILogger $logger,
-						 ISecureRandom $random,
-						 Installer $installer
-		) {
+	public function __construct(
+		SystemConfig $config,
+		IniGetWrapper $iniWrapper,
+		IL10N $l10n,
+		Defaults $defaults,
+		ILogger $logger,
+		ISecureRandom $random,
+		Installer $installer
+	) {
 		$this->config = $config;
 		$this->iniWrapper = $iniWrapper;
 		$this->l10n = $l10n;
@ -100,13 +102,14 @@ class Setup {
 	static protected $dbSetupClasses = [
 		'mysql' => \OC\Setup\MySQL::class,
 		'pgsql' => \OC\Setup\PostgreSQL::class,
-		'oci'   => \OC\Setup\OCI::class,
+		'oci' => \OC\Setup\OCI::class,
 		'sqlite' => \OC\Setup\Sqlite::class,
 		'sqlite3' => \OC\Setup\Sqlite::class,
 	];

 	/**
 	 * Wrapper around the "class_exists" PHP function to be able to mock it
+	 *
 	 * @param string $name
 	 * @return bool
 	 */
@ -116,6 +119,7 @@ class Setup {

 	/**
 	 * Wrapper around the "is_callable" PHP function to be able to mock it
+	 *
 	 * @param string $name
 	 * @return bool
 	 */
@ -141,7 +145,7 @@ class Setup {
 	 */
 	public function getSupportedDatabases($allowAllDatabases = false) {
 		$availableDatabases = [
-			'sqlite' =>  [
+			'sqlite' => [
 				'type' => 'pdo',
 				'call' => 'sqlite',
 				'name' => 'SQLite',
@ -168,24 +172,24 @@ class Setup {
 			$configuredDatabases = $this->config->getValue('supportedDatabases',
 				['sqlite', 'mysql', 'pgsql']);
 		}
-		if(!is_array($configuredDatabases)) {
+		if (!is_array($configuredDatabases)) {
 			throw new Exception('Supported databases are not properly configured.');
 		}

 		$supportedDatabases = array();

-		foreach($configuredDatabases as $database) {
-			if(array_key_exists($database, $availableDatabases)) {
+		foreach ($configuredDatabases as $database) {
+			if (array_key_exists($database, $availableDatabases)) {
 				$working = false;
 				$type = $availableDatabases[$database]['type'];
 				$call = $availableDatabases[$database]['call'];

 				if ($type === 'function') {
 					$working = $this->is_callable($call);
-				} elseif($type === 'pdo') {
+				} elseif ($type === 'pdo') {
 					$working = in_array($call, $this->getAvailableDbDriversForPdo(), true);
 				}
-				if($working) {
+				if ($working) {
 					$supportedDatabases[$database] = $availableDatabases[$database]['name'];
 				}
 			}
@ -204,14 +208,14 @@ class Setup {
 	public function getSystemInfo($allowAllDatabases = false) {
 		$databases = $this->getSupportedDatabases($allowAllDatabases);

-		$dataDir = $this->config->getValue('datadirectory', \OC::$SERVERROOT.'/data');
+		$dataDir = $this->config->getValue('datadirectory', \OC::$SERVERROOT . '/data');

 		$errors = [];

 		// Create data directory to test whether the .htaccess works
 		// Notice that this is not necessarily the same data directory as the one
 		// that will effectively be used.
-		if(!file_exists($dataDir)) {
+		if (!file_exists($dataDir)) {
 			@mkdir($dataDir);
 		}
 		$htAccessWorking = true;
@ -242,7 +246,7 @@ class Setup {
 			];
 		}

-		if($this->iniWrapper->getString('open_basedir') !== '' && PHP_INT_SIZE === 4) {
+		if ($this->iniWrapper->getString('open_basedir') !== '' && PHP_INT_SIZE === 4) {
 			$errors[] = [
 				'error' => $this->l10n->t(
 					'It seems that this %s instance is running on a 32-bit PHP environment and the open_basedir has been configured in php.ini. ' .
@ -275,14 +279,14 @@ class Setup {
 		$error = array();
 		$dbType = $options['dbtype'];

-		if(empty($options['adminlogin'])) {
+		if (empty($options['adminlogin'])) {
 			$error[] = $l->t('Set an admin username.');
 		}
-		if(empty($options['adminpass'])) {
+		if (empty($options['adminpass'])) {
 			$error[] = $l->t('Set an admin password.');
 		}
-		if(empty($options['directory'])) {
-			$options['directory'] = \OC::$SERVERROOT."/data";
+		if (empty($options['directory'])) {
+			$options['directory'] = \OC::$SERVERROOT . "/data";
 		}

 		if (!isset(self::$dbSetupClasses[$dbType])) {
@ -310,8 +314,8 @@ class Setup {
 		$request = \OC::$server->getRequest();

 		//no errors, good
-		if(isset($options['trusted_domains'])
-		    && is_array($options['trusted_domains'])) {
+		if (isset($options['trusted_domains'])
+			&& is_array($options['trusted_domains'])) {
 			$trustedDomains = $options['trusted_domains'];
 		} else {
 			$trustedDomains = [$request->getInsecureServerHost()];
@ -329,12 +333,12 @@ class Setup {

 		//write the config file
 		$newConfigValues = [
-			'passwordsalt'		=> $salt,
-			'secret'			=> $secret,
-			'trusted_domains'	=> $trustedDomains,
-			'datadirectory'		=> $dataDir,
-			'dbtype'			=> $dbType,
-			'version'			=> implode('.', \OCP\Util::getVersion()),
+			'passwordsalt' => $salt,
+			'secret' => $secret,
+			'trusted_domains' => $trustedDomains,
+			'datadirectory' => $dataDir,
+			'dbtype' => $dbType,
+			'version' => implode('.', \OCP\Util::getVersion()),
 		];

 		if ($this->config->getValue('overwrite.cli.url', null) === null) {
@ -363,13 +367,13 @@ class Setup {
 		}

 		//create the user and group
-		$user =  null;
+		$user = null;
 		try {
 			$user = \OC::$server->getUserManager()->createUser($username, $password);
 			if (!$user) {
 				$error[] = "User <$username> could not be created.";
 			}
-		} catch(Exception $exception) {
+		} catch (Exception $exception) {
 			$error[] = $exception->getMessage();
 		}

@ -379,22 +383,25 @@ class Setup {
 			$config->setAppValue('core', 'lastupdatedat', microtime(true));
 			$config->setAppValue('core', 'vendor', $this->getVendor());

-			$group =\OC::$server->getGroupManager()->createGroup('admin');
-			$group->addUser($user);
+			$group = \OC::$server->getGroupManager()->createGroup('admin');
+			if ($group instanceof IGroup) {
+				$group->addUser($user);
+			}

 			// Install shipped apps and specified app bundles
 			Installer::installShippedApps();
 			$bundleFetcher = new BundleFetcher(\OC::$server->getL10N('lib'));
 			$defaultInstallationBundles = $bundleFetcher->getDefaultInstallationBundle();
-			foreach($defaultInstallationBundles as $bundle) {
+			foreach ($defaultInstallationBundles as $bundle) {
 				try {
 					$this->installer->installAppBundle($bundle);
-				} catch (Exception $e) {}
+				} catch (Exception $e) {
+				}
 			}

 			// create empty file in data dir, so we can later find
 			// out that this is indeed an ownCloud data directory
-			file_put_contents($config->getSystemValue('datadirectory', \OC::$SERVERROOT.'/data').'/.ocdata', '');
+			file_put_contents($config->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data') . '/.ocdata', '');

 			// Update .htaccess files
 			self::updateHtaccess();
@ -434,7 +441,7 @@ class Setup {
 	 * @return string Absolute path to htaccess
 	 */
 	private function pathToHtaccess() {
-		return \OC::$SERVERROOT.'/.htaccess';
+		return \OC::$SERVERROOT . '/.htaccess';
 	}

 	/**
@ -499,7 +506,7 @@ class Setup {

 		// Add rewrite rules if the RewriteBase is configured
 		$rewriteBase = $config->getValue('htaccess.RewriteBase', '');
-		if($rewriteBase !== '') {
+		if ($rewriteBase !== '') {
 			$content .= "\n<IfModule mod_rewrite.c>";
 			$content .= "\n  Options -MultiViews";
 			$content .= "\n  RewriteRule ^core/js/oc.js$ index.php [PT,E=PATH_INFO:$1]";
@ -532,7 +539,7 @@ class Setup {

 		if ($content !== '') {
 			//suppress errors in case we don't have permissions for it
-			return (bool) @file_put_contents($setupHelper->pathToHtaccess(), $htaccessContent.$content . "\n");
+			return (bool)@file_put_contents($setupHelper->pathToHtaccess(), $htaccessContent . $content . "\n");
 		}

 		return false;
@ -540,21 +547,21 @@ class Setup {

 	public static function protectDataDirectory() {
 		//Require all denied
-		$now =  date('Y-m-d H:i:s');
+		$now = date('Y-m-d H:i:s');
 		$content = "# Generated by Nextcloud on $now\n";
-		$content.= "# line below if for Apache 2.4\n";
-		$content.= "<ifModule mod_authz_core.c>\n";
-		$content.= "Require all denied\n";
-		$content.= "</ifModule>\n\n";
-		$content.= "# line below if for Apache 2.2\n";
-		$content.= "<ifModule !mod_authz_core.c>\n";
-		$content.= "deny from all\n";
-		$content.= "Satisfy All\n";
-		$content.= "</ifModule>\n\n";
-		$content.= "# section for Apache 2.2 and 2.4\n";
-		$content.= "<ifModule mod_autoindex.c>\n";
-		$content.= "IndexIgnore *\n";
-		$content.= "</ifModule>\n";
+		$content .= "# line below if for Apache 2.4\n";
+		$content .= "<ifModule mod_authz_core.c>\n";
+		$content .= "Require all denied\n";
+		$content .= "</ifModule>\n\n";
+		$content .= "# line below if for Apache 2.2\n";
+		$content .= "<ifModule !mod_authz_core.c>\n";
+		$content .= "deny from all\n";
+		$content .= "Satisfy All\n";
+		$content .= "</ifModule>\n\n";
+		$content .= "# section for Apache 2.2 and 2.4\n";
+		$content .= "<ifModule mod_autoindex.c>\n";
+		$content .= "IndexIgnore *\n";
+		$content .= "</ifModule>\n";

 		$baseDir = \OC::$server->getConfig()->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data');
 		file_put_contents($baseDir . '/.htaccess', $content);
@ -572,6 +579,6 @@ class Setup {
 		// this should really be a JSON file
 		require \OC::$SERVERROOT . '/version.php';
 		/** @var string $vendor */
-		return (string) $vendor;
+		return (string)$vendor;
 	}
 }
--- a/lib/public/IGroupManager.php
+++ b/lib/public/IGroupManager.php
@ -75,7 +75,7 @@ interface IGroupManager {

 	/**
 	 * @param string $gid
-	 * @return \OCP\IGroup
+	 * @return \OCP\IGroup|null
 	 * @since 8.0.0
 	 */
 	public function get($gid);
@ -89,7 +89,7 @@ interface IGroupManager {

 	/**
 	 * @param string $gid
-	 * @return \OCP\IGroup
+	 * @return \OCP\IGroup|null
 	 * @since 8.0.0
 	 */
 	public function createGroup($gid);
--- a/tests/lib/Group/ManagerTest.php
+++ b/tests/lib/Group/ManagerTest.php
@ -191,6 +191,7 @@ class ManagerTest extends TestCase {
 			->method('createGroup')
 			->will($this->returnCallback(function () use (&$backendGroupCreated) {
 				$backendGroupCreated = true;
+				return true;
 			}));

 		$manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger);
@ -200,6 +201,35 @@ class ManagerTest extends TestCase {
 		$this->assertEquals('group1', $group->getGID());
 	}

+	public function testCreateFailure() {
+		/**@var \PHPUnit_Framework_MockObject_MockObject|\OC\Group\Backend $backend */
+		$backendGroupCreated = false;
+		$backend = $this->getTestBackend(
+			GroupInterface::ADD_TO_GROUP |
+			GroupInterface::REMOVE_FROM_GOUP |
+			GroupInterface::COUNT_USERS |
+			GroupInterface::CREATE_GROUP |
+			GroupInterface::DELETE_GROUP |
+			GroupInterface::GROUP_DETAILS
+		);
+		$backend->expects($this->any())
+			->method('groupExists')
+			->with('group1')
+			->willReturn(false);
+		$backend->expects($this->once())
+			->method('createGroup')
+			->willReturn(false);
+		$backend->expects($this->once())
+			->method('getGroupDetails')
+			->willReturn([]);
+
+		$manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger);
+		$manager->addBackend($backend);
+
+		$group = $manager->createGroup('group1');
+		$this->assertEquals(null, $group);
+	}
+
 	public function testCreateExists() {
 		/** @var \PHPUnit_Framework_MockObject_MockObject|\OC\Group\Backend $backend */
 		$backend = $this->getTestBackend();