Email confirmation for changing email in Settings by imujjwal96 · Pull Request #27277 · owncloud/core

imujjwal96 · 2017-02-28T11:35:35Z

Can be tested at: http://163.172.11.243:8080/core
Username: owncloud
Password: password

Description

When a user sets his/her email address for the first time, a confirmation mail is sent to the inputted address. From then onwards, every time a user tries to change/remove the email address, a confirmation mail is sent to the previous mail.
It also uses token validation (similar to reset password.)

Set Email / Change Email button added in front of the email input. Instead of changing email at every key delay, confirmation mail is sent when the user clicks on the button or when an Enter is pressed. Proper tests have been done.

Related Issue

Issue: #7326

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)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2017-02-28T11:35:37Z

@imujjwal96, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @phisch and @mmattel to be potential reviewers.

noveens · 2017-02-28T20:24:33Z

.htaccess

 <IfModule pagespeed_module>
  ModPagespeed Off
 </IfModule>
+#### DO NOT CHANGE ANYTHING ABOVE THIS LINE ####


Remove these lines.

Oh! Didn't intend to add them. Guess they were added automatically. Will delete them in the next push.

PVince81 · 2017-03-01T14:23:55Z

settings/Controller/UsersController.php

+		if ($splittedToken[0] < ($this->timeFactory->getTime() - 60*60*12) ||
+			$user->getLastLogin() > $splittedToken[0]) {
+			$this->config->deleteUserValue($userId, 'owncloud', 'changemail');
+			throw new \Exception($this->l10n->t('Couldn\'t change the email address because the token is expired'));


I suggest to always say "invalid", so an attacker cannot guess whether a token used to be valid.

This piece of code is inspired from the Reset password feature (tbh a copy+paste). Guess we'll have to correct it there too.

PVince81 · 2017-03-01T14:24:19Z

settings/Controller/UsersController.php

+     * @throws \Exception
+    */
+    public function sendEmail($user, $mailAddress) {
+    	$token = $this->secureRandom->generate(21,


some indent problems

PVince81 · 2017-03-01T14:25:40Z

settings/templates/panels/personal/profile.php

 	<input type="email" name="email" id="email" value="<?php p($_['email']); ?>"
 		placeholder="<?php p($l->t('Your email address'));?>"
 		autocomplete="on" autocapitalize="off" autocorrect="off" />
+	<input id="emailbutton" type="button" value="<?php if(isset($_['email'][0])) { echo $l->t('Change email'); } else { echo $l->t('Set email'); }?>" />


would it be possible to display an indicator that tells whether the email address has been confirmed already or whether it's still pending ?

I will look into that.

PVince81 · 2017-03-01T14:26:55Z

settings/Controller/UsersController.php

+				[
+					'status' => 'success',
+					'data' => [
+						'message' => (string)$this->l10n->t('Email Sent.')


Also display a text like "An email has been sent to this address for confirmation".

Since the text is long, it might fit better in an orange temporary notification: OC.Notification.showTemporary(...) instead of inline.

@PVince81 All notifications in the settings/personal route work similar to what i implemented. In fact the reason I did it was to maintain consistency. Changing the approach in one would require change in all of the forms in that page

makes sense, thanks

PVince81 · 2017-03-01T14:27:39Z

@imujjwal96 it seems you linked to the wrong issue ? #6633 is about limiting password reset.

I think there was another ticket about sending a confirmation email.

imujjwal96 · 2017-03-01T14:48:14Z

@PVince81 A request for this feature was made in one of the comments of this issue so I referred it. I will search for the issue (if it is raised) and refer that instead.

imujjwal96 · 2017-03-07T22:14:52Z

@PVince81 All checks have passed. I guess this PR is ready for a merge. Please have a look.

PVince81

Overall this is a great addition!

Please see my comments for several minor issues in the code.

I'm going to test this now.

PVince81 · 2017-03-08T08:46:38Z

settings/Controller/UsersController.php

+	 * @throws \Exception
+	 */
+    public function sendEmail($user, $mailAddress, $action='change') {
+    	$token = $this->secureRandom->generate(21,


indent is off

PVince81 · 2017-03-08T08:47:24Z

settings/Controller/UsersController.php

+		$userObject = $this->userManager->get($user);
+		$email = $userObject->getEMailAddress();
+
+		if ($action == 'change' || $action == 'add') {


use strict equality ===, see https://doc.owncloud.org/server/10.0/developer_manual/general/codingguidelines.html#operators

PVince81 · 2017-03-08T08:47:41Z

settings/Controller/UsersController.php

+	 * @param string $userId
+	 * @param string $mailAddress
+	 */
+    public function setEmailAddress($userId, $mailAddress) {


indent is off

PVince81 · 2017-03-08T08:48:06Z

settings/Controller/UsersController.php

+	 * @param $mailAddress
+	 * @return TemplateResponse
+	 */
+	public function changemail($token, $userId, $mailAddress) {


changeMail ? (camel case)

PVince81 · 2017-03-08T08:48:28Z

settings/js/personal.js

+ * @param allowEmptyValue if this is set to true the callback is also called when the value is empty
+ */
+
+jQuery.fn.Enter = function (callback, allowEmptyValue) {


enter lowercase

PVince81 · 2017-03-08T08:51:05Z

settings/templates/error.php

+				<p class='hint'><?php print_unescaped($error['hint']) ?></p>
+			<?php endif;?>
+		</li>
+	<?php endforeach ?>


this could also be in a generic status template but only displayed if errors exist

If showing status on the notification bar in the settings url is to be implemented, then i guess this will also be shown as a notification

Good point. Yes, it could make it easier as it would be the same code path than the success message.

PVince81 · 2017-03-08T08:53:31Z

.htaccess

 <IfModule pagespeed_module>
  ModPagespeed Off
-</IfModule>
+</IfModule>


this change is not needed

no idea how this is messed.

PVince81 · 2017-03-08T08:54:33Z

settings/Controller/UsersController.php

+	 * @param string $userId
+	 * @throws \Exception
+	 */
+	private function checkEmailChangeToken($token, $userId) {


You might need to check if $user is null and show an error/exception message. If the user was deleted in-between it could cause trouble.

PVince81 · 2017-03-08T08:55:39Z

settings/Controller/UsersController.php

 			);
 		}
+		$action = 'change';
+		$message = 'An email has been sent to the old address for confirmation';


please use the t() translation function here directly instead of below.

the reason is because there's a bot that parses the code to find the texts to submit for translation and it can only find the texts if they are directly wrapped in t()

PVince81 · 2017-03-08T08:55:59Z

settings/Controller/UsersController.php


-		// delete user value if email address is empty
-		$user->setEMailAddress($mailAddress);
+		if ($mailAddress == '') {


use strict equality === (also check other places)

PVince81 · 2017-03-08T09:22:41Z

imujjwal96 · 2017-03-08T18:12:42Z

Bug1: the action was supposed to perform whenever a user presses Enter or when "Set email" is clicked. if it is happening otherwise, i will try to fix it
Bug4: the email is allowed to be empty as it was before.

PVince81 · 2017-03-09T08:31:32Z

Bug4: the email is allowed to be empty as it was before.

Yes, it is allowed. But an email is still sent out. When emptying the field no email should be sent.

imujjwal96 · 2017-03-09T08:36:50Z

Yes, it is allowed. But an email is still sent out. When emptying the field no email should be sent.

@PVince81 The email is always sent to the previous email address (one that was set before). This would prevent spamming mails to some extent

PVince81 · 2017-03-09T08:40:11Z

The email is always sent to the previous email address (one that was set before). This would prevent spamming mails to some extent

But emptying should not send any email, there is no confirmation needed.

I think that the purpose of the email itself is to confirm the email address itself, not to confirm the change. (even if the change leads to an empty value)

Another issue if you send to the old email address is that the old email might not be valid / accessible any more. For example let's say a user has lost their email account / password (or left a university and was revoked access). Then they want to put their new email address in OC. They won't be able to because this PR sends to the old email address instead of the new one.

I think we should always send to the new email address.

Your concern about spamming is valid, not sure how other projects deal with that ? Maybe there needs to be a delay before sending email. For example don't let a user change their email more than once an hour or so.

imujjwal96 · 2017-03-09T08:45:28Z

@PVince81 I totally agree with you. I will make the necessary changes to it then.

PS: I did it according to a TODO in LukasReschke's comment in issue #6633.

pmaier1 · 2017-03-09T13:30:19Z

should we add a config.php switch to let admins disable email confirmation to go back to old behavior ? not sure if this is desirable.

I think confirmation mails are always desirable and can't imagine why an admin would want to disable this. A config.php switch should not be necessary therefore.

I think we should always send to the new email address.

Yes, good behavior would be to require the user password first and then send a confirmation link to the new address to confirm the change. Additionally it would be nice to send an email to the old address to inform about the change (this is how Dropbox handles it):

"Your Dropbox account's email address was recently changed.

Old email address: a@b.com
New email address: c@d.com

If you didn't make this change, please let us know "

Your concern about spamming is valid, not sure how other projects deal with that ? Maybe there needs to be a delay before sending email. For example don't let a user change their email more than once an hour or so.

Yep, a short delay would make sense, maybe around 5 minutes.

imujjwal96 · 2017-03-09T13:38:45Z

@pmaier1 I've now made the change. It sends confirmation only to the new mail address.

t would be nice to send an email to the old address to inform about the change (this is how Dropbox handles it):

Can work on it.

imujjwal96 · 2017-03-13T20:33:16Z

TODO: don't send a confirmation email if the email was set by the administrator

@PVince81 : Is this applicable to admin changing his own email address?
I was thinking of checking that if the user in session is an admin, then directly change the email address

TODO: don't send a confirmation email if the email was set by the administrator in the users page or provisioning API:

in apps/provisioning_api/lib/Users.php, the email address is changed by directly calling the function setEMailAddress on a User object. I guess it shouldn't send any confirmation email

PVince81 · 2017-03-14T08:53:20Z

I was thinking of checking that if the user in session is an admin, then directly change the email address

I'd say let's do it based on where it is changed.
If the email address is changed in the users page or provisioning API, do not send a confirmation.

If the email address is changed in the settings page of the user (personal section), then always send a confirmation regardless.

imujjwal96 · 2017-03-14T15:40:49Z

@PVince81 For Notifications, I was thinking to set the response message in the session and the get the message in the JS file and then use OC.Notif......

PVince81 · 2017-03-14T17:00:56Z

For Notifications, I was thinking to set the response message in the session and the get the message in the JS file and then use OC.Notif......

This won't work. Please don't store anything in the session. Also the JS code has no access to the session.

One way to pass the notification type is to pass an additional argument in the URL: "#email_saved_status=value" where value could be "success", "error", etc... Then you can read the URL hash from the JS code and decide what message to display based on it. Then you can also remove the hash part after processing.

imujjwal96 · 2017-03-17T14:37:31Z

@PVince81 There are some test files in the project which have in their setUp function

$this->container['UserManager'] = $this->getMockBuilder('\OCP\IUserManager')
		->disableOriginalConstructor()->getMock();

whereas other files have

$this->userManager = $this->getMockBuilder('\OCP\IUserManager')
		->disableOriginalConstructor()->getMock();

Does one need to be refactored to other?

PVince81 · 2017-03-17T14:38:37Z

Does one need to be refactored to other?

No need to refactor right now, thanks

PVince81 · 2017-03-20T09:07:00Z

BUG: log out before opening the link from the email: getting an error that email could not be changed
BUG: don't log out before opening the link from the email: Internal Server Error

{"reqId":"\/tTSkuJ+Umwo+3bbtqX6","remoteAddr":"127.0.0.1","app":"PHP","message":"Undefined offset: 1 at \/srv\/www\/htdocs\/owncloud\/lib\/private\/Mail\/Message.php#59","level":3,"time":"2017-03-20T09:05:38+00:00","method":"GET","url":"\/owncloud\/index.php\/settings\/mailaddress\/change\/SmQUG6rKn9S4l485fPM94\/user2\/vincent@vvortex.ttv","user":"user2"}
{"reqId":"\/tTSkuJ+Umwo+3bbtqX6","remoteAddr":"127.0.0.1","app":"index","message":"Exception: {\"Exception\":\"Exception\",\"Message\":\"Couldn't send email address change notification mail. Please contact your administrator.\",\"Code\":0,\"Trace\":\"#0 [internal function]: OC\\\\Settings\\\\Controller\\\\UsersController->changeMail('SmQUG6rKn9S4l48...', 'user2', 'vincent@vvortex...')\\n#1 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/AppFramework\\\/Http\\\/Dispatcher.php(159): call_user_func_array(Array, Array)\\n#2 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/AppFramework\\\/Http\\\/Dispatcher.php(89): OC\\\\AppFramework\\\\Http\\\\Dispatcher->executeController(Object(OC\\\\Settings\\\\Controller\\\\UsersController), 'changeMail')\\n#3 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/AppFramework\\\/App.php(98): OC\\\\AppFramework\\\\Http\\\\Dispatcher->dispatch(Object(OC\\\\Settings\\\\Controller\\\\UsersController), 'changeMail')\\n#4 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/AppFramework\\\/Routing\\\/RouteActionHandler.php(46): OC\\\\AppFramework\\\\App::main('UsersController', 'changeMail', Object(OC\\\\AppFramework\\\\DependencyInjection\\\\DIContainer), Array)\\n#5 [internal function]: OC\\\\AppFramework\\\\Routing\\\\RouteActionHandler->__invoke(Array)\\n#6 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/private\\\/Route\\\/Router.php(299): call_user_func(Object(OC\\\\AppFramework\\\\Routing\\\\RouteActionHandler), Array)\\n#7 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/lib\\\/base.php(898): OC\\\\Route\\\\Router->match('\\\/settings\\\/maila...')\\n#8 \\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/index.php(49): OC::handleRequest()\\n#9 {main}\",\"File\":\"\\\/srv\\\/www\\\/htdocs\\\/owncloud\\\/settings\\\/Controller\\\/UsersController.php\",\"Line\":832}","level":3,"time":"2017-03-20T09:05:38+00:00","method":"GET","url":"\/owncloud\/index.php\/settings\/mailaddress\/change\/SmQUG6rKn9S4l485fPM94\/user2\/vincent@vvortex.ttv","user":"user2"}

In both cases I didn't have any email address set initially.
Is it trying to send an email to the empty previous email address ?

imujjwal96 · 2017-03-21T16:06:03Z

@PVince81 What should be expected when a logged out user changes his email address?
Changing it anyway?

PVince81 · 2017-03-21T16:10:12Z

What should be expected when a logged out user changes his email address?

If by "changes his email address" you mean "open the link from the email", then the following should happen:

Login page appears with the correct redirect URL
User logs in with correct credentials
Login page redirects to the settings page (as per redirect URL parameter)
Email address is changed

Now normally this should already work because the redirect logic already exists, but for some reason maybe some parameters are lost. I seem to remember advising you to use the hash part of the URL ? ("#something"). I think such parts might be lost on redirect, so maybe try using a real URL parameter instead like "&something"

imujjwal96 · 2017-03-21T18:05:11Z

BUG: log out before opening the link from the email: getting an error that email could not be changed

it was because there was some error in checking the validity of the email change token. This bug does not occur after correcting that.

imujjwal96 · 2017-03-21T18:09:25Z

PVince81 · 2017-03-21T18:19:55Z

TEST: change own email from empty to something else: after confirmation there are no errors nor attempts to send an email to the previous address

@imujjwal96 otherwise the other test cases look good.

@imujjwal96 side note: it seems there are conflicts that you'll need to resolve. Let me know if you need guidance for doing that as it can be tricky especially when working on your own fork.

imujjwal96 · 2017-03-21T19:19:16Z

I've tested all the cases and they're working fine on my server.

it seems there are conflicts that you'll need to resolve. Let me know if you need guidance for doing that as it can be tricky especially when working on your own fork.

What should I do? @PVince81

PVince81 · 2017-03-21T19:36:12Z

@imujjwal96 first make a backup of your branch:
git checkout -b backup
then go back to your master:
git checkout master

@imujjwal96 I see github has a button "Resolve conflicts" with a web editor, maybe you could first try with that ?

imujjwal96 · 2017-03-21T19:38:49Z

Should I push the changes i made recently?

PVince81 · 2017-03-21T19:40:02Z

Should I push the changes i made recently?

sounds good

imujjwal96 · 2017-03-21T19:53:21Z

@PVince81 I have made some changes through Github in order to resolve the conflicts. Can you review them now or do I have to first commit them?

PVince81 · 2017-03-21T20:03:18Z

It seems the conflits still appear so maybe you need to commit them

PVince81 · 2017-03-22T10:05:19Z

settings/templates/changemail/notify.php

@@ -0,0 +1,2 @@
+<?php
+echo str_replace('{mailAddress}', $_['mailAddress'], $l->t('Email address changed to {mailAddress} successfully.'));


please delete this file it is not needed any more

never mind, this is for the email contents

PVince81 · 2017-03-22T10:05:48Z

Code looks good otherwise, great job !

I'll redo a quick test and then I think this is good to go.

PVince81 · 2017-03-22T10:22:40Z

👍

PVince81 · 2017-03-22T10:25:08Z

@imujjwal96 congrats! Merged.

I think this PR doesn't fix the password reset limitation, right ? #6633

imujjwal96 · 2017-03-22T12:32:32Z

@PVince81 No it does not. Forgot to remove the reference to that issue.

PVince81 · 2017-05-31T15:10:01Z

Regression found for group admins (aka subadmins) who cannot change a group member's email any more: #28035

phil-davis · 2018-05-21T14:16:08Z

settings/Controller/UsersController.php

+		return new RedirectResponse($this->urlGenerator->linkToRoute('settings.SettingsPage.getPersonal', ['changestatus' => 'success']));
+  }
+
+  /*


Accident here (missing 2nd asterisk for docblock) that causes regression #31280 "subadmin cannot disable member of his group"

Can you check if there are other locations where there might be a docblock issue like that?

For reference, fix is in owncloud-archive/user_management#30

lock · 2019-07-31T08:02:10Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Email confirmation for changing email in Settings

cbceeb7

noveens reviewed Feb 28, 2017

View reviewed changes

PVince81 added the 3 - To Review label Mar 1, 2017

PVince81 reviewed Mar 1, 2017

View reviewed changes

PVince81 mentioned this pull request Mar 1, 2017

trigger event on input autofill #27273

Merged

9 tasks

imujjwal96 added 3 commits March 5, 2017 15:31

Update with docs and code cleanup

64900a7

Remove token on success and confirmation on setting email

cbf20f8

Update UsersControllerTest

33e81ab

PVince81 reviewed Mar 8, 2017

View reviewed changes

Send mail to new address instead of old

5af6197

Fix bugs from PR #27277

e4c8082

Update test cases for PR 27346

843fdb6

PVince81 mentioned this pull request Mar 20, 2017

Code review for first version owncloud/guests#23

Closed

Solving bugs - setting email and logged out users

2395e2c

Merge branch 'master' into master

6ff9168

PVince81 reviewed Mar 22, 2017

View reviewed changes

PVince81 merged commit 82c529c into owncloud:master Mar 22, 2017

PVince81 mentioned this pull request Mar 22, 2017

Verify email addresses #7326

Closed

PVince81 mentioned this pull request May 31, 2017

Group Admins can not save Mailadresses after upgrade OC9->OC10 #28035

Closed

phil-davis reviewed May 21, 2018

View reviewed changes

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

		@@ -0,0 +1,2 @@
		<?php
		echo str_replace('{mailAddress}', $_['mailAddress'], $l->t('Email address changed to {mailAddress} successfully.'));

Conversation

imujjwal96 commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

mention-bot commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imujjwal96 Mar 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Mar 1, 2017

Uh oh!

imujjwal96 commented Mar 1, 2017

Uh oh!

imujjwal96 commented Mar 7, 2017

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Mar 8, 2017

Comments/tasks

Test plan:

Uh oh!

imujjwal96 commented Mar 8, 2017

Uh oh!

PVince81 commented Mar 9, 2017

Uh oh!

imujjwal96 commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Mar 9, 2017

imujjwal96 commented Feb 28, 2017 •

edited

Loading

imujjwal96 Mar 5, 2017 •

edited

Loading

imujjwal96 commented Mar 9, 2017 •

edited

Loading

imujjwal96 commented Mar 9, 2017 •

edited

Loading

imujjwal96 commented Mar 13, 2017 •

edited

Loading

PVince81 commented Mar 20, 2017 •

edited

Loading

imujjwal96 commented Mar 21, 2017 •

edited

Loading

imujjwal96 commented Mar 21, 2017 •

edited

Loading