Merge pull request #15964 from nextcloud/enh/noid/user-creation-options

Opt-in for generation userid, requiring email addresses

apps/provisioning_api/lib/Controller/UsersController.php

@@ -46,7 +46,6 @@ use OCP\IGroup;
 use OCP\IGroupManager;
 use OCP\ILogger;
 use OCP\IRequest;
-use OCP\IUser;
 use OCP\IUserManager;
 use OCP\IUserSession;
 use OCP\L10N\IFactory;
@@ -196,6 +195,21 @@ class UsersController extends AUserData {
 		]);
 	}
 
+	/**
+	 * @throws OCSException
+	 */
+	private function createNewUserId(): string {
+		$attempts = 0;
+		do {
+			$uidCandidate = $this->secureRandom->generate(10, ISecureRandom::CHAR_HUMAN_READABLE);
+			if (!$this->userManager->userExists($uidCandidate)) {
+				return $uidCandidate;
+			}
+			$attempts++;
+		} while ($attempts < 10);
+		throw new OCSException('Could not create non-existing user id', 111);
+	}
+
 	/**
 	 * @PasswordConfirmationRequired
 	 * @NoAdminRequired
@@ -223,6 +237,10 @@ class UsersController extends AUserData {
 		$isAdmin = $this->groupManager->isAdmin($user->getUID());
 		$subAdminManager = $this->groupManager->getSubAdmin();
 
+		if(empty($userid) && $this->config->getAppValue('core', 'newUser.generateUserID', 'no') === 'yes') {
+			$userid = $this->createNewUserId();
+		}
+
 		if ($this->userManager->userExists($userid)) {
 			$this->logger->error('Failed addUser attempt: User already exists.', ['app' => 'ocs_api']);
 			throw new OCSException('User already exists', 102);
@@ -275,6 +293,10 @@ class UsersController extends AUserData {
 			$generatePasswordResetToken = true;
 		}
 
+		if ($email === '' && $this->config->getAppValue('core', 'newUser.requireEmail', 'no') === 'yes') {
+			throw new OCSException('Required email address was not provided', 110);
+		}
+
 		try {
 			$newUser = $this->userManager->createUser($userid, $password);
 			$this->logger->info('Successful addUser call with userid: ' . $userid, ['app' => 'ocs_api']);
@@ -315,7 +337,7 @@ class UsersController extends AUserData {
 				}
 			}
 
-			return new DataResponse();
+			return new DataResponse(['UserID' => $userid]);
 
 		} catch (HintException $e ) {
 			$this->logger->logException($e, [

apps/provisioning_api/tests/Controller/UsersControllerTest.php

@@ -356,7 +356,10 @@ class UsersControllerTest extends TestCase {
 			->with('adminUser')
 			->willReturn(true);
 
-		$this->assertEquals([], $this->api->addUser('NewUser', 'PasswordOfTheNewUser')->getData());
+		$this->assertTrue(key_exists(
+			'UserID',
+			$this->api->addUser('NewUser', 'PasswordOfTheNewUser')->getData()
+		));
 	}
 
 	public function testAddUserSuccessfulWithDisplayName() {
@@ -413,7 +416,149 @@ class UsersControllerTest extends TestCase {
 			->method('editUser')
 			->with('NewUser', 'display', 'DisplayNameOfTheNewUser');
 
-		$this->assertEquals([], $api->addUser('NewUser', 'PasswordOfTheNewUser', 'DisplayNameOfTheNewUser')->getData());
+		$this->assertTrue(key_exists(
+			'UserID',
+			$api->addUser('NewUser', 'PasswordOfTheNewUser', 'DisplayNameOfTheNewUser')->getData()
+		));
+	}
+
+	public function testAddUserSuccessfulGenerateUserID() {
+		$this->config
+			->expects($this->any())
+			->method('getAppValue')
+			->willReturnCallback(function($appid, $key, $default) {
+				if($key === 'newUser.generateUserID') {
+					return 'yes';
+				}
+				return null;
+			});
+		$this->userManager
+			->expects($this->any())
+			->method('userExists')
+			->with($this->anything())
+			->will($this->returnValue(false));
+		$this->userManager
+			->expects($this->once())
+			->method('createUser')
+			->with($this->anything(), 'PasswordOfTheNewUser');
+		$this->logger
+			->expects($this->once())
+			->method('info')
+			->with($this->stringStartsWith('Successful addUser call with userid: '), ['app' => 'ocs_api']);
+		$loggedInUser = $this->getMockBuilder(IUser::class)
+			->disableOriginalConstructor()
+			->getMock();
+		$loggedInUser
+			->expects($this->once())
+			->method('getUID')
+			->will($this->returnValue('adminUser'));
+		$this->userSession
+			->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($loggedInUser));
+		$this->groupManager
+			->expects($this->once())
+			->method('isAdmin')
+			->with('adminUser')
+			->willReturn(true);
+		$this->secureRandom->expects($this->any())
+			->method('generate')
+			->with(10)
+			->willReturnCallback(function() { return (string)rand(1000000000, 9999999999); });
+
+		$this->assertTrue(key_exists(
+			'UserID',
+			$this->api->addUser('', 'PasswordOfTheNewUser')->getData()
+		));
+	}
+
+	/**
+	 * @expectedException \OCP\AppFramework\OCS\OCSException
+	 * @expectedExceptionCode 111
+	 * @expectedExceptionMessage Could not create non-existing user id
+	 */
+	public function testAddUserFailedToGenerateUserID() {
+		$this->config
+			->expects($this->any())
+			->method('getAppValue')
+			->willReturnCallback(function($appid, $key, $default) {
+				if($key === 'newUser.generateUserID') {
+					return 'yes';
+				}
+				return null;
+			});
+		$this->userManager
+			->expects($this->any())
+			->method('userExists')
+			->with($this->anything())
+			->will($this->returnValue(true));
+		$this->userManager
+			->expects($this->never())
+			->method('createUser');
+		$loggedInUser = $this->getMockBuilder(IUser::class)
+			->disableOriginalConstructor()
+			->getMock();
+		$loggedInUser
+			->expects($this->once())
+			->method('getUID')
+			->will($this->returnValue('adminUser'));
+		$this->userSession
+			->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($loggedInUser));
+		$this->groupManager
+			->expects($this->once())
+			->method('isAdmin')
+			->with('adminUser')
+			->willReturn(true);
+
+		$this->api->addUser('', 'PasswordOfTheNewUser')->getData();
+	}
+
+	/**
+	 * @expectedException \OCP\AppFramework\OCS\OCSException
+	 * @expectedExceptionCode 110
+	 * @expectedExceptionMessage Required email address was not provided
+	 */
+	public function testAddUserEmailRequired() {
+		$this->config
+			->expects($this->any())
+			->method('getAppValue')
+			->willReturnCallback(function($appid, $key, $default) {
+				if($key === 'newUser.requireEmail') {
+					return 'yes';
+				}
+				return null;
+			});
+		$this->userManager
+			->expects($this->once())
+			->method('userExists')
+			->with('NewUser')
+			->will($this->returnValue(false));
+		$this->userManager
+			->expects($this->never())
+			->method('createUser');
+		$loggedInUser = $this->getMockBuilder(IUser::class)
+			->disableOriginalConstructor()
+			->getMock();
+		$loggedInUser
+			->expects($this->once())
+			->method('getUID')
+			->will($this->returnValue('adminUser'));
+		$this->userSession
+			->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($loggedInUser));
+		$this->groupManager
+			->expects($this->once())
+			->method('isAdmin')
+			->with('adminUser')
+			->willReturn(true);
+
+		$this->assertTrue(key_exists(
+			'UserID',
+			$this->api->addUser('NewUser', 'PasswordOfTheNewUser')->getData()
+		));
 	}
 
 	public function testAddUserExistingGroup() {
@@ -471,7 +616,10 @@ class UsersControllerTest extends TestCase {
 				['Added userid NewUser to group ExistingGroup', ['app' => 'ocs_api']]
 			);
 
-		$this->assertEquals([], $this->api->addUser('NewUser', 'PasswordOfTheNewUser', '', '', ['ExistingGroup'])->getData());
+		$this->assertTrue(key_exists(
+			'UserID',
+				$this->api->addUser('NewUser', 'PasswordOfTheNewUser', '', '', ['ExistingGroup'])->getData()
+		));
 	}
 
 	/**
@@ -689,7 +837,10 @@ class UsersControllerTest extends TestCase {
 			)
 			->willReturn(true);
 
-		$this->assertEquals([], $this->api->addUser('NewUser', 'PasswordOfTheNewUser', '', '', ['ExistingGroup1', 'ExistingGroup2'])->getData());
+		$this->assertTrue(key_exists(
+			'UserID',
+			$this->api->addUser('NewUser', 'PasswordOfTheNewUser', '', '', ['ExistingGroup1', 'ExistingGroup2'])->getData()
+		));
 	}
 
 	/**

apps/user_ldap/lib/Access.php

@@ -635,7 +635,7 @@ class Access extends LDAPUtility {
 		return false;
 	}
 
-	protected function mapAndAnnounceIfApplicable(
+	public function mapAndAnnounceIfApplicable(
 		AbstractMapping $mapper,
 		string $fdn,
 		string $name,

apps/user_ldap/lib/Group_LDAP.php

@@ -110,7 +110,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 		$members = $this->access->connection->getFromCache($cacheKeyMembers);
 		if(!is_null($members)) {
 			$this->cachedGroupMembers[$gid] = $members;
-			$isInGroup = in_array($userDN, $members);
+			$isInGroup = in_array($userDN, $members, true);
 			$this->access->connection->writeToCache($cacheKey, $isInGroup);
 			return $isInGroup;
 		}

apps/user_ldap/lib/User_LDAP.php

@@ -622,8 +622,26 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 		if ($this->userPluginManager->implementsActions(Backend::CREATE_USER)) {
 			if ($dn = $this->userPluginManager->createUser($username, $password)) {
 				if (is_string($dn)) {
-					//updates user mapping
-					$this->access->dn2ocname($dn, $username, true);
+					// the NC user creation work flow requires a know user id up front
+					$uuid = $this->access->getUUID($dn, true);
+					if(is_string($uuid)) {
+						$this->access->mapAndAnnounceIfApplicable(
+							$this->access->getUserMapper(),
+							$dn,
+							$username,
+							$uuid,
+							true
+						);
+						$this->access->cacheUserExists($username);
+					} else {
+						\OC::$server->getLogger()->warning(
+							'Failed to map created LDAP user with userid {userid}, because UUID could not be determined',
+							[
+								'app' => 'user_ldap',
+								'userid' => $username,
+							]
+						);
+					}
 				} else {
 					throw new \UnexpectedValueException("LDAP Plugin: Method createUser changed to return the user DN instead of boolean.");
 				}

apps/user_ldap/tests/User_LDAPTest.php

@@ -35,6 +35,7 @@ use OC\User\Backend;
 use OC\User\Session;
 use OCA\User_LDAP\Access;
 use OCA\User_LDAP\Connection;
+use OCA\User_LDAP\Mapping\AbstractMapping;
 use OCA\User_LDAP\Mapping\UserMapping;
 use OCA\User_LDAP\User\Manager;
 use OCA\User_LDAP\User\OfflineUser;
@@ -1437,16 +1438,30 @@ class User_LDAPTest extends TestCase {
 	}
 
 	public function testCreateUserWithPlugin() {
+		$uid = 'alien6372';
+		$uuid = '123-2345-36756-123-2345234-4431';
+		$pwd = 'passwørd';
+
 		$this->pluginManager->expects($this->once())
 			->method('implementsActions')
 			->with(Backend::CREATE_USER)
 			->willReturn(true);
 		$this->pluginManager->expects($this->once())
 			->method('createUser')
-			->with('uid','password')
+			->with($uid, $pwd)
 			->willReturn('result');
 
-		$this->assertEquals($this->backend->createUser('uid', 'password'),true);
+		$this->access->expects($this->atLeastOnce())
+			->method('getUUID')
+			->willReturn($uuid);
+		$this->access->expects($this->once())
+			->method('mapAndAnnounceIfApplicable')
+			->with($this->isInstanceOf(AbstractMapping::class), $this->anything(), $uid, $uuid, true);
+		$this->access->expects($this->any())
+			->method('getUserMapper')
+			->willReturn($this->createMock(AbstractMapping::class));
+
+		$this->assertEquals($this->backend->createUser($uid, $pwd),true);
 	}
 
 	public function testCreateUserFailing() {

settings/Controller/UsersController.php

@@ -246,6 +246,8 @@ class UsersController extends Controller {
 		// Settings
 		$serverData['defaultQuota'] = $defaultQuota;
 		$serverData['canChangePassword'] = $canChangePassword;
+		$serverData['newUserGenerateUserID'] = $this->config->getAppValue('core', 'newUser.generateUserID', 'no') === 'yes';
+		$serverData['newUserRequireEmail'] = $this->config->getAppValue('core', 'newUser.requireEmail', 'no') === 'yes';
 
 		return new TemplateResponse('settings', 'settings-vue', ['serverData' => $serverData]);
 	}

settings/src/components/userList.vue

@@ -49,9 +49,12 @@
 			<div :class="loading.all?'icon-loading-small':'icon-add'"></div>
 			<div class="name">
 				<input id="newusername" type="text" required v-model="newUser.id"
-					   :placeholder="t('settings', 'Username')" name="username"
-					   autocomplete="off" autocapitalize="none" autocorrect="off"
-					   ref="newusername" pattern="[a-zA-Z0-9 _\.@\-']+">
+					   :placeholder="this.settings.newUserGenerateUserID
+						   ? t('settings', 'Will be autogenerated')
+						   : t('settings', 'Username')"
+					   name="username" autocomplete="off" autocapitalize="none"
+					   autocorrect="off" ref="newusername" pattern="[a-zA-Z0-9 _\.@\-']+"
+					   :disabled="this.settings.newUserGenerateUserID">
 			</div>
 			<div class="displayName">
 				<input id="newdisplayname" type="text" v-model="newUser.displayName"
@@ -67,7 +70,7 @@
 			</div>
 			<div class="mailAddress">
 				<input id="newemail" type="email" v-model="newUser.mailAddress"
-					   :required="newUser.password===''"
+					   :required="newUser.password==='' || this.settings.newUserRequireEmail"
 					   :placeholder="t('settings', 'Email')" name="email"
 					   autocomplete="off" autocapitalize="none" autocorrect="off">
 			</div>

settings/src/store/users.js

@@ -429,7 +429,7 @@ const actions = {
 	addUser({commit, dispatch}, { userid, password, displayName, email, groups, subadmin, quota, language }) {
 		return api.requireAdmin().then((response) => {
 			return api.post(OC.linkToOCS(`cloud/users`, 2), { userid, password, displayName, email, groups, subadmin, quota, language })
-				.then((response) => dispatch('addUserData', userid))
+				.then((response) => dispatch('addUserData', userid || response.data.ocs.data.UserID))
 				.catch((error) => {throw error;});
 		}).catch((error) => {
 			commit('API_FAILURE', { userid, error });