resetpassword command supports send password reset link and display link #32345

sharidas · 2018-08-15T15:07:04Z

The resetpassword supports options to

display the password reset link in command line.
send email to reset the password for user and
display the link in the command line.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

The resetpassword supports options to

display the password reset link in command line.
send email to reset the password for user and display the link in the command line.

Related Issue

Fixes [FR] OCC command to trigger a password reset email #29542

Motivation and Context

The resetpassword supports options to

display the password reset link in command line.
send email to reset the password for user and display the link in the command line.

How Has This Been Tested?

test environment:
test case 1:
test case 2:
...

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

codecov · 2018-08-15T16:28:12Z

Codecov Report

Merging #32345 into master will increase coverage by 0.05%.
The diff coverage is 84%.

@@             Coverage Diff              @@
##             master   #32345      +/-   ##
============================================
+ Coverage     64.06%   64.11%   +0.05%     
- Complexity    18634    18641       +7     
============================================
  Files          1176     1176              
  Lines         70152    70185      +33     
  Branches       1270     1270              
============================================
+ Hits          44944    45002      +58     
+ Misses        24838    24813      -25     
  Partials        370      370

Flag	Coverage Δ	Complexity Δ
#javascript	`52.89% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`65.39% <84%> (+0.05%)`	`18641 <11> (+7)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
core/register_command.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
core/Controller/LostController.php	`88.72% <100%> (+0.26%)`	`33 <10> (+1)`	⬆️
core/Command/User/ResetPassword.php	`66.26% <93.93%> (+66.26%)`	`18 <1> (+6)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b04a4a7...145a5d2. Read the comment docs.

jvillafanez · 2018-08-16T08:54:11Z

Do we have an use case for both options? Having the "via-mail" option is hard to figure out why we need the "via-link" option.
I'd expect that if the target user doesn't have an email set, you show a warning message in the console (because you couldn't send the email) and show the link (which I'm not really sure if it's needed... how is the link expected to be sent if the admin doesn't know the user email?)

jvillafanez · 2018-08-16T09:07:03Z

In addition, I don't have a clear view of the expected flow.
For example: the command to be focused on setting a new password. The password can be provided interactively or via environment variable. Once the password is correctly set, we send an email to the user to notify the password has been reset.

I don't think this is the flow we want, but the current code doesn't seem to have a clear idea such as the one above.

PVince81 · 2018-08-22T08:53:54Z

@jvillafanez the option to output the link on the CLI is intended for docker setups where VMs are created automatically and a custom email is sent by VM scripts, including the link. For example "your server has been setup, click here to set your initial password: XYZ"

PVince81 · 2018-08-22T08:55:08Z

As far as I understand this PR is only for generating a password reset token and sending the link by email and/or outputting on the CLI. It is not intended to actually set a password.

sharidas · 2018-08-27T11:47:46Z

Review required @PVince81

PVince81 · 2018-08-27T12:01:17Z

core/Command/User/ResetPassword.php

+				'The email-id set while creating the user, will be used to send link for password reset. This option will also display the link sent to user.'
+			)
+			->addOption(
+				'via-link',


"via link" means "something will be send through a link" and doesn't reflect what is happening here
maybe call it "output-link" or something similar

and rename "via-email" to "send-email"

Renamed to "send-email" and "output-link".

PVince81 · 2018-08-27T12:02:01Z

core/Command/User/ResetPassword.php

+				$mail = new TemplateResponse('core', 'lostpassword/email', $mailData, 'blank');
+				$mailContent = $mail->render();
+
+				$subject = $this->l10n->t('Your password is reset');


was this copy-pasted from another location ?

Yes, this was copy-pasted from LostController.

can we make the code reusable ? not too happy to see that many lines copy-pasted...

Agreed. I have refactored the code, in which case at this point of time refactoring LostController, would be too much for this PR, I guess.

I don't see what has been refactored.

Is there a way then to pick smaller pieces ? I'm a bit worried that for example the token generation + token expiration code is duplicated. If one would decide to change the token format there is a risk of mismatch.

Updated the code by modifying LostController, to provide link which can be used by ResetPassword for both send-email and output-link options.

PVince81 · 2018-08-27T12:02:24Z

core/Command/User/ResetPassword.php

+				$output->writeln('<error>Email address is not set for the user ' . $user->getUID() . '</error>');
+				return 1;
+			}
+		} elseif ($displayLink) {


emailing and displaying link aren't mutually exclusive. one could specify both

Updated the code, user can execute either email or displaying link or both.

PVince81

see comments

PVince81 · 2018-08-28T08:23:56Z

core/Command/User/ResetPassword.php

 			if (!$password) {
 				$output->writeln('<error>--password-from-env given, but OC_PASS is empty!</error>');
 				return 1;
 			}
+		} elseif ($emailLink | $displayLink) {


PVince81 · 2018-08-28T08:25:25Z

core/Command/User/ResetPassword.php

+					$message->setFrom([Util::getDefaultEmailAddress('no-reply') => $this->defaults->getName()]);
+					$this->mailer->send($message);
+				} catch (\Exception $e) {
+					$output->writeln('<error>Can\'t send new user mail to ' . $user->getEMailAddress() . ': ' . $e->getMessage());


missing </error>

Added </error>.

PVince81

a few more minor things, see comments

PVince81

Looks better, thanks 👍

The resetpassword supports options to 1) display the password reset link in command line. 2) send email to reset the password for user and display the link in the command line. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas · 2018-08-29T06:18:08Z

Backport stable10 : #32500

sharidas self-assigned this Aug 15, 2018

sharidas requested a review from jvillafanez August 15, 2018 15:07

sharidas added the 3 - To Review label Aug 15, 2018

sharidas added this to the development milestone Aug 15, 2018

sharidas force-pushed the resetpassword-command-update branch from 898e78b to b25dac0 Compare August 15, 2018 16:27

sharidas mentioned this pull request Aug 20, 2018

[FR] OCC command to trigger a password reset email #29542

Closed

PVince81 reviewed Aug 27, 2018

View reviewed changes

PVince81 suggested changes Aug 27, 2018

View reviewed changes

sharidas force-pushed the resetpassword-command-update branch from b25dac0 to c265f4d Compare August 27, 2018 14:02

ownclouders added comp:authentication Effort: 1d enhancement labels Aug 27, 2018

PVince81 reviewed Aug 28, 2018

View reviewed changes

PVince81 suggested changes Aug 28, 2018

View reviewed changes

sharidas force-pushed the resetpassword-command-update branch 2 times, most recently from c1f9b01 to c027c9c Compare August 28, 2018 15:03

PVince81 approved these changes Aug 28, 2018

View reviewed changes

PVince81 added backport-request 4 - To release and removed Effort: 1d 3 - To Review labels Aug 28, 2018

sharidas force-pushed the resetpassword-command-update branch from c027c9c to 145a5d2 Compare August 28, 2018 16:31

ownclouders added the Effort: 1d label Aug 28, 2018

sharidas merged commit 6281d7b into master Aug 29, 2018

sharidas deleted the resetpassword-command-update branch August 29, 2018 05:56

sharidas mentioned this pull request Aug 29, 2018

[stable10] Backport of resetpassword command supports send password r… #32500

Merged

10 tasks

sharidas removed the backport-request label Aug 29, 2018

sharidas mentioned this pull request Sep 4, 2018

Reset password command changes made for the release owncloud-archive/documentation#4409

Closed

settermjd mentioned this pull request Sep 4, 2018

Expand occ user reset password email validation #32569

Merged

10 tasks

lock bot locked as resolved and limited conversation to collaborators Aug 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resetpassword command supports send password reset link and display link #32345

resetpassword command supports send password reset link and display link #32345

sharidas commented Aug 15, 2018 •

edited

Loading

codecov bot commented Aug 15, 2018 •

edited

Loading

jvillafanez commented Aug 16, 2018

jvillafanez commented Aug 16, 2018

PVince81 commented Aug 22, 2018

PVince81 commented Aug 22, 2018

sharidas commented Aug 27, 2018

PVince81 Aug 27, 2018

sharidas Aug 27, 2018

PVince81 Aug 27, 2018

sharidas Aug 27, 2018

PVince81 Aug 27, 2018

sharidas Aug 28, 2018

PVince81 Aug 28, 2018

sharidas Aug 28, 2018

PVince81 Aug 27, 2018

sharidas Aug 27, 2018

PVince81 left a comment

PVince81 Aug 28, 2018

sharidas Aug 28, 2018

PVince81 Aug 28, 2018

sharidas Aug 28, 2018

PVince81 left a comment

PVince81 left a comment

sharidas commented Aug 29, 2018

resetpassword command supports send password reset link and display link #32345

resetpassword command supports send password reset link and display link #32345

Conversation

sharidas commented Aug 15, 2018 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Open tasks:

codecov bot commented Aug 15, 2018 • edited Loading

Codecov Report

jvillafanez commented Aug 16, 2018

jvillafanez commented Aug 16, 2018

PVince81 commented Aug 22, 2018

PVince81 commented Aug 22, 2018

sharidas commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

sharidas commented Aug 29, 2018

sharidas commented Aug 15, 2018 •

edited

Loading

codecov bot commented Aug 15, 2018 •

edited

Loading