Skip to content

Opt-in for generation userid, requiring email addresses #15964

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 21, 2019
Merged

Opt-in for generation userid, requiring email addresses #15964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5b754a6
extends Provisioning API with optional userid and required email
blizzz Jun 14, 2019
660fbd6
ensures mapping of chosen userid
blizzz Jun 14, 2019
bbe2f25
makes users management page aware of the introduced options
blizzz Jun 14, 2019
05b3288
rebuilt js files after rebase
blizzz Jun 19, 2019
29449f8
uses "yes" and "no" for config switch over stringified numbers
blizzz Jun 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions apps/provisioning_api/lib/Controller/UsersController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
use OCP\IGroupManager;
use OCP\ILogger;
use OCP\IRequest;
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use OCP\L10N\IFactory;
Expand Down Expand Up @@ -196,6 +195,21 @@ public function getUsersDetails(string $search = '', $limit = null, $offset = 0)
]);
}

/**
* @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
Expand Down Expand Up @@ -223,6 +237,10 @@ public function addUser(string $userid,
$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);
Expand Down Expand Up @@ -275,6 +293,10 @@ public function addUser(string $userid,
$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']);
Expand Down Expand Up @@ -315,7 +337,7 @@ public function addUser(string $userid,
}
}

return new DataResponse();
return new DataResponse(['UserID' => $userid]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use id, so the key is the same as from getUserData?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AH yes maybe we should!
@blizzz ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#16036


} catch (HintException $e ) {
$this->logger->logException($e, [
Expand Down
159 changes: 155 additions & 4 deletions apps/provisioning_api/tests/Controller/UsersControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,10 @@ public function testAddUserSuccessful() {
->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() {
Expand Down Expand Up @@ -413,7 +416,149 @@ public function testAddUserSuccessfulWithDisplayName() {
->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() {
Expand Down Expand Up @@ -471,7 +616,10 @@ public function testAddUserExistingGroup() {
['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()
));
}

/**
Expand Down Expand Up @@ -689,7 +837,10 @@ public function testAddUserAsSubAdminExistingGroups() {
)
->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()
));
}

/**
Expand Down
2 changes: 1 addition & 1 deletion apps/user_ldap/lib/Access.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped
return false;
}

protected function mapAndAnnounceIfApplicable(
public function mapAndAnnounceIfApplicable(
AbstractMapping $mapper,
string $fdn,
string $name,
Expand Down
2 changes: 1 addition & 1 deletion apps/user_ldap/lib/Group_LDAP.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public function inGroup($uid, $gid) {
$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;
}
Expand Down
22 changes: 20 additions & 2 deletions apps/user_ldap/lib/User_LDAP.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,8 +622,26 @@ public function createUser($username, $password) {
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.");
}
Expand Down
19 changes: 17 additions & 2 deletions apps/user_ldap/tests/User_LDAPTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
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;
Expand Down Expand Up @@ -1437,16 +1438,30 @@ public function testSetDisplayNameFailing() {
}

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() {
Expand Down
2 changes: 2 additions & 0 deletions settings/Controller/UsersController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,8 @@ public function usersList() {
// 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]);
}
Expand Down
2 changes: 1 addition & 1 deletion settings/js/vue-6.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion settings/js/vue-6.js.map
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions settings/js/vue-settings-apps-users-management.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion settings/js/vue-settings-apps-users-management.js.map
View file
Open in desktop

Large diffs are not rendered by default.

11 changes: 7 additions & 4 deletions settings/src/components/userList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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>
Expand Down
2 changes: 1 addition & 1 deletion settings/src/store/users.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down