Skip to content
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.

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

Jump to bottom

Allow admin to create users without password by sending mail automatic… #8856

Merged
merged 1 commit into from Mar 23, 2018
Merged

Allow admin to create users without password by sending mail automatic… #8856

merged 1 commit into from Mar 23, 2018

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Mar 16, 2018 •
edited

…ally

Required by #8824

@skjnldsv skjnldsv added this to the Nextcloud 14 milestone Mar 16, 2018
@skjnldsv skjnldsv self-assigned this Mar 16, 2018
MorrisJobke
MorrisJobke reviewed Mar 16, 2018
View reviewed changes
apps/provisioning_api/lib/Controller/UsersController.php Outdated
throw new OCSException('To send a password link to the user an email address is required.', 108);
}

$password = $this->secureRandom->generate(30);
Copy link
Member

Choose a reason for hiding this comment

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

30?!? Don't overcomplicate things 😅

Copy link
Member

Choose a reason for hiding this comment

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

But... that is only 1532495540865888858358347027150309183618739122183602176 possibilities!

Copy link
Member Author

Choose a reason for hiding this comment

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

X)
Do we want to be secure or not!!?? :D

@skjnldsv skjnldsv force-pushed the ocs-api-new-user-password-email branch from 7559c7d to 73682c2 Compare March 17, 2018 11:33
@codecov
Copy link

codecov bot commented Mar 17, 2018 •
edited

Codecov Report

Merging #8856 into master will decrease coverage by 45.02%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master   #8856       +/-   ##
============================================
- Coverage     51.88%   6.85%   -45.03%     
- Complexity    25269   25273        +4     
============================================
  Files          1603    1603               
  Lines         94878   94899       +21     
  Branches       1388    1388               
============================================
- Hits          49229    6508    -42721     
- Misses        45649   88391    +42742
Impacted Files Coverage Δ Complexity Δ
...rovisioning_api/lib/Controller/UsersController.php 0% <0%> (-77.21%) 138 <15> (+4)
...c/AppFramework/Http/EmptyContentSecurityPolicy.php 0% <0%> (-100%) 45% <0%> (ø)
lib/private/Group/Database.php 0% <0%> (-100%) 26% <0%> (ø)
lib/private/BackgroundJob/QueuedJob.php 0% <0%> (-100%) 1% <0%> (ø)
.../Exceptions/EncryptionHeaderKeyExistsException.php 0% <0%> (-100%) 1% <0%> (ø)
lib/private/Route/CachingRouter.php 0% <0%> (-100%) 3% <0%> (ø)
lib/private/Files/AppData/Factory.php 0% <0%> (-100%) 2% <0%> (ø)
lib/private/TagManager.php 0% <0%> (-100%) 4% <0%> (ø)
lib/private/Support/CrashReport/Registry.php 0% <0%> (-100%) 3% <0%> (ø)
apps/files_sharing/lib/External/MountProvider.php 0% <0%> (-100%) 4% <0%> (ø)
... and 864 more

@skjnldsv skjnldsv force-pushed the ocs-api-new-user-password-email branch from 73682c2 to 3bba48d Compare March 17, 2018 11:34
@skjnldsv skjnldsv mentioned this pull request Mar 17, 2018
Merged
34 tasks
@skjnldsv skjnldsv force-pushed the ocs-api-new-user-password-email branch 5 times, most recently from 47bae0c to c3f9c7e Compare March 19, 2018 07:19
rullzer
rullzer reviewed Mar 19, 2018
View reviewed changes
apps/provisioning_api/lib/Controller/UsersController.php

$password = $this->secureRandom->generate(10);
// Make sure we pass the password_policy
$password .= $this->secureRandom->generate(2, '$!.,;:-~+*[]{}()');
Copy link
Member

Choose a reason for hiding this comment

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

well this is only 'kind of' true. We don't know if this will pass the password policy test because we don't know what it is set to.

Copy link
Member

Choose a reason for hiding this comment

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

Nothing you can fix here. But still

Copy link
Member Author

@skjnldsv skjnldsv Mar 19, 2018 •
edited

Choose a reason for hiding this comment

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

I used old code, so I'm to blame here :D
To be fair, the goal is to create a secure password that no one will know about since the user will receive a mail to reset it. I suggest we remove the special char part and only use a 15 length generation.

Copy link
Member

Choose a reason for hiding this comment

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

Then this might fail hard when using the password_policy app ;) But like I said fine by me. Lets do this for now.

nickvergessen
nickvergessen approved these changes Mar 22, 2018
View reviewed changes
apps/provisioning_api/lib/Controller/UsersController.php Outdated
* @param array $groups
* @return DataResponse
* @throws OCSException
*/
public function addUser(string $userid, string $password, array $groups = []): DataResponse {
public function addUser(string $userid, string $password = '', $email='', array $groups = []): DataResponse {
Copy link
Member

Choose a reason for hiding this comment

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

string $email = ''

@skjnldsv skjnldsv force-pushed the ocs-api-new-user-password-email branch 2 times, most recently from 02f1087 to bcca3e0 Compare March 22, 2018 13:54
@skjnldsv
Allow admin to create users withoutpassword by sending mail automatic…
41b690e 
…ally

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the ocs-api-new-user-password-email branch from bcca3e0 to 41b690e Compare March 22, 2018 14:06
@skjnldsv
Copy link
Member Author

I think the failure is unrelated!

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 23, 2018
@MorrisJobke MorrisJobke merged commit 6a31203 into master Mar 23, 2018
@MorrisJobke MorrisJobke deleted the ocs-api-new-user-password-email branch March 23, 2018 07:17
@MorrisJobke MorrisJobke mentioned this pull request Apr 4, 2018
Closed
@skjnldsv skjnldsv mentioned this pull request Sep 13, 2018
Closed
@acanrpk acanrpk mentioned this pull request Jan 8, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rullzer rullzer rullzer approved these changes

@nickvergessen nickvergessen nickvergessen approved these changes

@schiessle schiessle Awaiting requested review from schiessle

@MorrisJobke MorrisJobke Awaiting requested review from MorrisJobke

Assignees

@skjnldsv skjnldsv

Labels
4. to release Ready to be released and/or waiting for tests to finish feature: users and groups
Projects
None yet
Milestone
Nextcloud 14
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@skjnldsv @rullzer @nickvergessen @MorrisJobke