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.

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

Jump to bottom

Force reset password #18914

Closed
wants to merge 2 commits into from
Closed

Force reset password #18914

wants to merge 2 commits into from

Conversation

ein-giga-self
Copy link

Added a functionality that forces users to reset their password on first login, if the initial password was set by the admin

@ein-giga-self ein-giga-self mentioned this pull request Jan 15, 2020
Open
@gary-kim gary-kim added this to the Nextcloud 19 milestone Jan 17, 2020
@gary-kim
Copy link
Member

gary-kim commented Jan 17, 2020 •
edited
Loading

Welcome!
I think it would be nice to have some way to set a password without requiring a password reset in the interface as well.

kesselb
kesselb reviewed Jan 17, 2020
View reviewed changes
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

Thank you for picking this up 👍

I'm sorry for all those remarks. Please wait for another review for a second opinion. I only know the authentication in general. Probably there are better ways than my suggestions (especially for the tokens).

core/Controller/LostController.php
$token = $this->config->getUserKeys($userId, 'login_token')[0];
$token = str_replace('/', 'A', $token);

if($token === $token) {
Copy link
Contributor

Choose a reason for hiding this comment

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

😕

core/Controller/LoginController.php

if($this->config->getUserValue($user, 'core', 'initial') === 'true') {
$token = $this->config->getUserKeys($user, 'login_token')[0];
$token = str_replace('/', 'A', $token);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this about making the value a get parameter? If so please use urlencode.

core/Controller/LostController.php
Comment on lines +202 to +209
if($this->config->getUserValue($userId, 'core', 'initial') === 'true')

$token = $this->config->getUserKeys($userId, 'login_token')[0];
$token = str_replace('/', 'A', $token);

if($token === $token) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

That change makes it possible to overtake any account if you know the userId and the user has not changed the password yet.

apps/provisioning_api/lib/Controller/UsersController.php
@@ -594,6 +598,9 @@ public function editUser(string $userId, string $key, string $value): DataRespon
$this->accountManager->updateUser($targetUser, $userAccount);
}
break;
case 'initial':
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create a constant like AccountManager::PROPERTY_INITIAL

apps/provisioning_api/lib/Controller/UsersController.php
@@ -520,6 +523,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon
$permittedFields[] = AccountManager::PROPERTY_WEBSITE;
$permittedFields[] = AccountManager::PROPERTY_TWITTER;
$permittedFields[] = 'quota';
$permittedFields[] = 'initial';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you find something more specific? Perhaps forcePasswordReset? initial is very generic.

core/Controller/LoginController.php
@@ -306,6 +306,13 @@ public function tryLogin(string $user,
$result->getErrorMessage()
);
}

if($this->config->getUserValue($user, 'core', 'initial') === 'true') {
$token = $this->config->getUserKeys($user, 'login_token')[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

OK. The right way to do this:

Take the password reset token logic from NewUserMailHelper. Perhaps we need to find a better way for this. A generatePasswordResetToken method. If the user login and initial is set generate a password reset token and forward the user.

@rullzer @ChristophWurst

apps/provisioning_api/lib/Controller/UsersController.php
@@ -349,6 +349,9 @@ public function addUser(string $userid,
'app' => 'ocs_api',
]);
}
} else {
//Password was provided by the admin
Copy link
Contributor

Choose a reason for hiding this comment

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

Case 1: No password, No email = failure
Case 2: No password, Email given = username and password reset link is mailed to user
Case 3: Password given, Email given = username is mailed to user
Case 4: Password given, No email = password reset forced after login

At least that must be documented somewhere. I'm against any more magic here. I don't think the behaviour is guessable or intuitive even without this pr. We should explain what Nextcloud is actually are going to do and/or give the admin a change to pick the action.

@jancborchardt

Copy link
Contributor

Choose a reason for hiding this comment

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

Also Password given, No email is what I usually do for developing 🤣 Need to test something with a different user. Create user with password and no email, Login => Forced password reset 😠

I know that's unusual but this PR is a behavioural change. I'm sure there is someone out there happy with the current approach. People will complain about this change if there is no way to turn it off.

For example: A self-service for account creation. People are able to create a user account for Nextcloud with a internal tool. Password is only visible to user. With this change they have to reset their password again for a account just created seconds ago.

This was referenced Apr 4, 2020
Merged
Merged
This was referenced Apr 15, 2020
Merged
Merged
@rullzer rullzer mentioned this pull request Apr 23, 2020
Merged
11 tasks
@rullzer rullzer removed this from the Nextcloud 19 milestone Apr 24, 2020
@rullzer rullzer added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 24, 2020
@szaimen szaimen added this to the Nextcloud 23 milestone Aug 8, 2021
@ChristophWurst ChristophWurst removed their request for review August 19, 2021 10:38
@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
Merged
@skjnldsv
Copy link
Member

As there is no feedback since a while I will close this PR. If you are still willing to get this in, please address the potential comments and rebase to latest master. Then, feel free to re-open.

@skjnldsv skjnldsv closed this Oct 13, 2021
@skjnldsv skjnldsv removed this from the Nextcloud 23 milestone Oct 13, 2021
@delacosta456
Copy link

hi @ein-giga-self
while looking for something similar to what you have listed in your feature i cam to this post.
I would like to know what is the exact state of those functionalities .. Did you make another PR about these somewhere else ?
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kesselb kesselb kesselb left review comments

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@rullzer rullzer Awaiting requested review from rullzer

Assignees
No one assigned
Labels
2. developing Work in progress enhancement feature: users and groups
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ein-giga-self @gary-kim @skjnldsv @delacosta456 @kesselb @rullzer @szaimen