Email change notification #2500

Uplinger · 2018-05-02T16:40:19Z

This is to address the Email Change Notification for BOINC. Please view this issue with more information: #2451

…hema.sql for user

…r email address in the system.

…in 7 days.

Conflicts: html/user/edit_email_action.php

…r_change_time to the user table

…atabase time.

…notification Conflicts: html/ops/db_update.php

…rom an email link.

Prevented user on change email form page to actually do anything if within 7 days of previous email change. Added more text to email about recovering email address.

…ncryption. Also, fixed the logout issue when recovering your email address.

…fication

… within 7 days of being changed

…t previuos email address isn't in the 7 day change period

…be logged in when trying to reset email address.

Moved edit_email_action to fix time of email change not being passed to next function. Added check for previous email address still within 7 days for create_account.php call.

…d along to email change function

TheAspens · 2018-05-02T18:20:52Z

html/inc/email.inc

    if (function_exists("make_php_mailer")) {
-        require_once("../inc/phpmailer/class.phpmailer.php");
+        require_once("../inc/PHPMailer/src/PHPMailer.php");
+        require_once("../inc/PHPMailer/src/SMTP.php");


Please update https://boinc.berkeley.edu/trac/wiki/ServerIntro#PHPMailer so that has the correct changes so that projects know what to do.

I have updated the instructions to point to 6.0.x PHPMailer and commited a code change to allow for both options, this way it won't break older version of PHPMailer. 4aa5acb

… as well as the latest 6.0.x version.

TheAspens · 2018-05-02T18:49:29Z

Although I reviewed and approved this change, since I collaborated with Keith on this and we work on the same team I think that others should do reviews and merge.

TheAspens

Trying to revoke approval since I don't want it showing up that I approved (I do but since I was involved closely on this I shouldn't be the approver).

…_notification Conflicts: html/user/am_set_info.php html/user/edit_email_form.php html/user/recover_email.php

…g with changing the email address.

…s changed within 7 days.

…m_set_info.php

…at is not allowed int the default schema definition.

Uplinger · 2018-05-08T19:33:05Z

@brevilo @davidpanderson @TheAspens @JuhaSointusalo I have incorporated all the changes as well as fixed a few bugs from merging of code. I have tested it locally, let me know your thoughts.

Thanks,
-Keith

JuhaSointusalo · 2018-05-08T19:05:57Z

html/inc/email.inc

@@ -86,6 +99,36 @@ For further information and assistance with ".PROJECT.", visit
    return send_email($user, $subject, $body);
 }

+function send_changed_email($user) {
+    $body_new = "";
+    $body_old = "";


This is really a minor issue but Scrutinizer is flagging these as major issue. The variables are not used before they are overwritten. You could remove them from here.

JuhaSointusalo · 2018-05-08T19:35:25Z

html/user/am_set_info.php

+    //
+    $tmpuser = BoincUser::lookup_prev_email_addr($email_addr);
+    if ($tmpuser) {
+        xml_error(ERR_BAD_EMAIL_ADDR, "Email address is already in use");


This and the above should probably be ERR_DB_NOT_UNIQUE to match creating accounts.

JuhaSointusalo · 2018-05-08T19:45:05Z

html/user/edit_email_form.php

-form_submit(tra("Change email address"));
-form_end();
+if ($user->email_addr_change_time + 604800 > time()) {
+    echo tra("Email address was changed within the past 7 days. Please look for an email to $user->previous_email_addr to verify this change.");


The word "verify" feels wrong to me. You can't verify the change but revert it with the email sent to the previous email address.

JuhaSointusalo · 2018-05-08T19:45:35Z

html/user/recover_email.php

+// You should have received a copy of the GNU Lesser General Public License
+// along with BOINC.  If not, see <http://www.gnu.org/licenses/>.
+
+// ADD COMMENT


Missed this one.

…red later with string value in inc/email.inc

Uplinger · 2018-05-08T20:55:01Z

@JuhaSointusalo Please take another look. I think I incorporated your suggestions completely.

Thanks,
-Keith

JuhaSointusalo · 2018-05-08T21:05:18Z

I haven't tested this so I don't formally stamp this Approved but it looks ok to me.

…e_time to be indexed per davids suggestion.

Uplinger · 2018-05-10T15:30:37Z

@davidpanderson Can you take a look at the pull request. I have made the change to the indexes to help when doing a query. I have also change the order of a query to use the indexes first.

Thanks,
-Keith

TheAspens · 2018-05-10T18:39:45Z

html/ops/db_update.php

@@ -1107,6 +1107,7 @@ function update_4_19_2018() {
 	add column previous_email_addr varchar(254) not null default '',
 	add column email_addr_change_time double not null default 0
    ");
+    do_query("alter table user add index user_email_time (email_addr_change_time)");


I think this needs a semi-colon at the end doesn't it?

Where are you thinking the semicolon goes, there is one after the do_query close bracket? From previous examples of do_query, a semicolon is not needed before the end quotes. Note, you should be able to see similar update query in function update_5_12_2004.

Ok - I was under the impression that mysql required a semi-colon at the end of statements, but if you've tested it and it works then clearly it is fine. I retract my question.

TheAspens · 2018-05-11T14:43:20Z

@brevilo and @davidpanderson - can either of you guys complete the review for this? Thanks!

davidpanderson · 2018-05-14T23:25:21Z

The new user fields also should be added to DB_USER::db_parse() and DB_USER::db_print() (in db/boinc_db.cpp), even if they aren't used in the C++ code. Otherwise if someone adds a field to user that IS used from C++, confusion will result.

Uplinger · 2018-05-15T15:24:35Z

@davidpanderson I need to run my tests again to make sure this change didn't break anything. I should have that done today and I'll send you a message on here. Thanks, -Keith

Uplinger · 2018-05-15T16:35:19Z

@davidpanderson My local tests succeeded. You can merge when ready.

Thanks for the feedback from everyone on this ticket!
-Keith

brevilo · 2018-05-16T09:07:42Z

Sorry for being too late this time.

David fixed the schema.sql changes (BOINC needs add columns always at the end due to its select * from habit) I would have flagged and also commented on the then missing C++ API changes.

Thanks everyone!

When changing an email, use receives notification at new and old email address. Add checks for previous email address. Also add check for boincuser_delete account to terms of use form. Matches UX from upstream: BOINC#2451 Resolves: https://dev.gridrepublic.org/browse/DBOINCP-436 Also see: * BOINC#2500 * https://boinc.berkeley.edu/trac/wiki/EmailChangeNotification

Uplinger and others added 25 commits April 13, 2018 13:55

Include previous_email_addr and email_addr_change_time to database sc…

2b16e60

…hema.sql for user

Update to have a changed email sent to members after they change thei…

23a9b2b

…r email address in the system.

Updated to send the email out properly.

8956751

Update to actually print the word at in email message.

cc38a9d

Added text to the website that will show if an email was changed with…

863cf3d

…in 7 days.

Merge branch 'master' into email_change_notification

a2a78a6

Conflicts: html/user/edit_email_action.php

Update to have db_update.php to add previous_email_addr and email_add…

db4e020

…r_change_time to the user table

Changed to use php time passed to the database instead of using the d…

36a8b1f

…atabase time.

Merge remote-tracking branch 'upstream/knr_token2' into email_change_…

a1b03c8

…notification Conflicts: html/ops/db_update.php

Change to use token.inc for email_change_notification

dbafb59

Fixed error in string as well as force email.inc to include token.inc.

c516ed3

Updated user pages to include a reset_email_addr.php that is called f…

fa4603f

…rom an email link.

Renamed to recover_email.php from reset_email_addr.php

6602dcf

Changed to fix datbase update error on recover_email.php.

50a8756

Prevented user on change email form page to actually do anything if within 7 days of previous email change. Added more text to email about recovering email address.

Fixed email changes so that database_hash is used instead of simple e…

79b169c

…ncryption. Also, fixed the logout issue when recovering your email address.

Merge remote-tracking branch 'upstream/master' into email_change_noti…

09c54fb

…fication

Updated to have function to look up previous email addresses that are…

389ede3

… within 7 days of being changed

Account manager set info and create user has a check to make sure tha…

c226ee7

…t previuos email address isn't in the 7 day change period

Fixing missing ending call in function.

4785a05

Fix bug where I was sending it a null string to lookup_prev_email_addr

69f78a6

Fixed a typo in recover_email.php as well as not require the user to …

3011ccf

…be logged in when trying to reset email address.

Delete token for email recovery.

aaf866e

Fix setting previous email address in AccountManager set info.

7916631

Clean up boinc_db email calls.

93d6e0b

Moved edit_email_action to fix time of email change not being passed to next function. Added check for previous email address still within 7 days for create_account.php call.

Fixed bug where new email address and change time was not being passe…

7c9b4ad

…d along to email change function

TheAspens reviewed May 2, 2018

View reviewed changes

Changed email.inc so that it will look for older version of PHPMailer…

4aa5acb

… as well as the latest 6.0.x version.

TheAspens previously approved these changes May 2, 2018

View reviewed changes

TheAspens reviewed May 2, 2018

View reviewed changes

Keith Uplinger added 7 commits May 8, 2018 08:28

Merge remote-tracking branch 'upstream/keith_email' into email_change…

cbcbb5e

…_notification Conflicts: html/user/am_set_info.php html/user/edit_email_form.php html/user/recover_email.php

Updated comments in a few places for email address changes.

6964382

Added database call for previous email address in c++ code.

7749c64

am_set_info, if email address is the same as the user, then do nothin…

94a6687

…g with changing the email address.

Added check in am_set_info to return an error if the email address wa…

5cead5f

…s changed within 7 days.

Fixed problem with bad constant called for invalid email address in a…

387d596

…m_set_info.php

Changed setting previous_email_addr to blank instead of null since th…

8c9626f

…at is not allowed int the default schema definition.

JuhaSointusalo reviewed May 8, 2018

View reviewed changes

Keith Uplinger added 4 commits May 8, 2018 15:14

Fixed Scrutinizer issue where variables were declared blank but decla…

e82a698

…red later with string value in inc/email.inc

Change error code for duplicate email addresses found in am_set_info.php

9cd7d63

Change phrasing in the edit_email_form.php

0d78176

Added comments for recover_email.php

4168861

Modified the table definition for user table to have email_addr_chang…

fecbfa3

…e_time to be indexed per davids suggestion.

TheAspens reviewed May 10, 2018

View reviewed changes

TheAspens requested a review from davidpanderson May 14, 2018 13:53

Updated boinc_db.cpp to print email address change columns.

12a3933

davidpanderson merged commit 02a5980 into BOINC:master May 15, 2018

drshawnkwang mentioned this pull request May 21, 2018

Email Change Notification #2451

Closed

drshawnkwang mentioned this pull request Jun 21, 2018

Web RPC am_set_info should not email user when email changes. #2565

Open

drshawnkwang mentioned this pull request Jun 21, 2018

Drupal: Harden email change procedure #2566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Email change notification #2500

Email change notification #2500

Uplinger commented May 2, 2018

TheAspens May 2, 2018

Uplinger May 2, 2018

TheAspens commented May 2, 2018

TheAspens left a comment

Uplinger commented May 8, 2018

JuhaSointusalo May 8, 2018

JuhaSointusalo May 8, 2018

JuhaSointusalo May 8, 2018

JuhaSointusalo May 8, 2018

Uplinger commented May 8, 2018

JuhaSointusalo commented May 8, 2018

Uplinger commented May 10, 2018

TheAspens May 10, 2018

Uplinger May 10, 2018

TheAspens May 11, 2018

TheAspens commented May 11, 2018

davidpanderson commented May 14, 2018

Uplinger commented May 15, 2018

Uplinger commented May 15, 2018

brevilo commented May 16, 2018

Email change notification #2500

Email change notification #2500

Conversation

Uplinger commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheAspens commented May 2, 2018

TheAspens left a comment

Choose a reason for hiding this comment

Uplinger commented May 8, 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

Uplinger commented May 8, 2018

JuhaSointusalo commented May 8, 2018

Uplinger commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheAspens commented May 11, 2018

davidpanderson commented May 14, 2018

Uplinger commented May 15, 2018

Uplinger commented May 15, 2018

brevilo commented May 16, 2018