Opt-in for generation userid, requiring email addresses #15964
Conversation
blizzz
added
enhancement
3. to review
blizzz
requested review from
georgehrke,
juliusknorr,
rullzer,
schiessle,
skjnldsv and
viniciuscb
blizzz
force-pushed
the
enh/noid/user-creation-options
branch
5 times, most recently
from
3ce09b5 to
ea40345
Compare
| $isAdmin = $this->groupManager->isAdmin($user->getUID()); | ||
| $subAdminManager = $this->groupManager->getSubAdmin(); | ||
|
|
||
| if(empty($userid) && (bool)$this->config->getAppValue('settings', 'newUser.generateUserID', false)) { |
There was a problem hiding this comment.
Casting? Shouldn't we rather check against the exact value? '1'?
Or is it not a bad practice :)
There was a problem hiding this comment.
Eh … yeah, whyever I was doing it this way. I guess I was carried away by my urge to have boolean meaning be treated boolean. Even the cast useless here. I'll change it :)
There was a problem hiding this comment.
@blizzz well, you're the php expert here ;)
So if you say it's fine, it's fine for me! 🚀
skjnldsv
left a comment
skjnldsv
left a comment
There was a problem hiding this comment.
Code and tests are good. A small nitpick that is maybe not an issue! :)
blizzz
force-pushed
the
enh/noid/user-creation-options
branch
2 times, most recently
from
5c7163a to
a0695ed
Compare
|
May I have a second reviewer, please? |
|
@juliushaertl @georgehrke @schiessle @vincchan |
|
Meh, conflicts … who dared to merge other stuff first 😝 OK, gonna rebase.
If this is one of your other personalities speaking, then, I think, yes, why not? 🙃 🤡 |
Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
blizzz
force-pushed
the
enh/noid/user-creation-options
branch
from
a0695ed to
faeb4a6
Compare
Signed-off-by: Arthur Schiwon <[email protected]>
blizzz
force-pushed
the
enh/noid/user-creation-options
branch
from
faeb4a6 to
05b3288
Compare
georgehrke
left a comment
georgehrke
left a comment
There was a problem hiding this comment.
Tiny nitpick, otherwise good to go 👍
| $isAdmin = $this->groupManager->isAdmin($user->getUID()); | ||
| $subAdminManager = $this->groupManager->getSubAdmin(); | ||
|
|
||
| if(empty($userid) && $this->config->getAppValue('settings', 'newUser.generateUserID', '0') === '1') { |
There was a problem hiding this comment.
Nitpick: we usually use 'yes' and 'no' (see the enabled configkey for example)
There was a problem hiding this comment.
we also don't use getAppValue('settings' anywhere, it's usually getAppValue('core'
There was a problem hiding this comment.
Indeed! Ok, will adjust, thank you!
| $generatePasswordResetToken = true; | ||
| } | ||
|
|
||
| if ($email === '' && $this->config->getAppValue('settings', 'newUser.requireEmail', '0') === '1') { |
Signed-off-by: Arthur Schiwon <[email protected]>
blizzz
force-pushed
the
enh/noid/user-creation-options
branch
from
762876b to
29449f8
Compare
| } | ||
|
|
||
| return new DataResponse(); | ||
| return new DataResponse(['UserID' => $userid]); |
There was a problem hiding this comment.
should we use id, so the key is the same as from getUserData?
This PR adds following two switches for creating new users:
The motivation is to allow untechnical (sub)admins to allow to create users on an external backend. The user id does not play a role, but the email has to be enforced.
Therefore, there are no graphical switches. Bu the flags can be set via occ and they also do work with the local backend. For testing:
This flag sets the email field to required when creating a new user. The provsioning API errors with 110 if it is not provided.
This disables the new username field on the users page. The user will be appended to the list after creation with a random 10-character long ascii string. In case an unexisting userid could not be generated, the provisioning API will error with 111 after 10 attempts.