Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-13811

Add a confirmation step when a user changes their own email address in their profile.

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 1.8.6, 1.9.2
    • Component/s: Authentication
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE

      Description

      Currently there is no process and any new email address is accepted.

      I think we could do it like this instead:

      • User edits profile and submits form
      • If email is different then:
      • Do NOT update the real profile email yet.
      • Save that new email in a user preference together with a random key.
      • Send an email to the NEW address with instructions and a link containing the secret key.
        eg http://moodle.org/user/emailchange.php?id=y73nj3bh3b3m7678bbhbhbhbh3bh34
      • User finds the email, clicks the link, and a script:
      • verifies the secret key,
      • changes the profile email to the new one and
      • deletes the user preference

        Gliffy Diagrams

          Attachments

          1. MDL-13811_002.patch
            8 kB
          2. MDL-13811_003.patch
            9 kB
          3. MDL-13811_004.patch
            8 kB
          4. MDL-13811_005.patch
            9 kB
          5. MDL-13811.patch
            8 kB
          6. MDL-13811.png
            MDL-13811.png
            102 kB
          7. MDL-13811b.png
            MDL-13811b.png
            88 kB

            Issue Links

              Activity

              Hide
              skodak Petr Skoda added a comment - - edited

              the only troubly is how to inform user that:

              • email change is pending (some info in edit form)
              • email will not be changed immediately (info in form)

              Also there should be an option to cancel the change.
              Users which are allowed to edit profiles (such as admins) should not be asked for confirmation.
              Maybe a resend option.

              Looking forward to patch, this will be a great improvement

              Show
              skodak Petr Skoda added a comment - - edited the only troubly is how to inform user that: email change is pending (some info in edit form) email will not be changed immediately (info in form) Also there should be an option to cancel the change. Users which are allowed to edit profiles (such as admins) should not be asked for confirmation. Maybe a resend option. Looking forward to patch, this will be a great improvement
              Hide
              skodak Petr Skoda added a comment -

              This could be implemented by adding two different form fields and removing the one that is not used in definition_after_data() - one for old style change, the other for change with confirmation.

              Show
              skodak Petr Skoda added a comment - This could be implemented by adding two different form fields and removing the one that is not used in definition_after_data() - one for old style change, the other for change with confirmation.
              Hide
              dougiamas Martin Dougiamas added a comment -

              Agreed that people with the ability to edit all profiles don't need this.

              For others, I don't think we need to maintain or show anything about the pending change in the main form interface, if we do it like this:

              When you hit submit:

              iff the email was changed then you see the message telling you that the email will not be changed to the new value until you read the email at that address and click on the link found there, with a continue link.

              if the email was not changed then just go straight to the profile page as we do now.

              Since nothing happens until they press the link in the email there is no need to do anything special for resending etc.

              Show
              dougiamas Martin Dougiamas added a comment - Agreed that people with the ability to edit all profiles don't need this. For others, I don't think we need to maintain or show anything about the pending change in the main form interface, if we do it like this: When you hit submit: iff the email was changed then you see the message telling you that the email will not be changed to the new value until you read the email at that address and click on the link found there, with a continue link. if the email was not changed then just go straight to the profile page as we do now. Since nothing happens until they press the link in the email there is no need to do anything special for resending etc.
              Hide
              nicolasconnault Nicolas Connault added a comment -

              Attaching patch

              Show
              nicolasconnault Nicolas Connault added a comment - Attaching patch
              Hide
              nicolasconnault Nicolas Connault added a comment -

              Version 2 of the patch attached.

              Show
              nicolasconnault Nicolas Connault added a comment - Version 2 of the patch attached.
              Hide
              nicolasconnault Nicolas Connault added a comment -

              Ignore the last file, try number 3 instead!

              Show
              nicolasconnault Nicolas Connault added a comment - Ignore the last file, try number 3 instead!
              Hide
              skodak Petr Skoda added a comment - - edited

              1/ we must add email duplicate test into user/emailupdate.php just before saving the new one - the timing could work against us there
              2/ admin option to turn it off
              3/ print_continue("$CFG->wwwroot/user/view.php?id=$user->id", true); – true does not print the continue button
              4/ name of site in email header missing
              5/ missing name of user in that email too
              6/ I think the key may be shorter - 20 chars?
              7/ oh! print_simple_box() is deprecated - use the alternatives instead

              Show
              skodak Petr Skoda added a comment - - edited 1/ we must add email duplicate test into user/emailupdate.php just before saving the new one - the timing could work against us there 2/ admin option to turn it off 3/ print_continue("$CFG->wwwroot/user/view.php?id=$user->id", true); – true does not print the continue button 4/ name of site in email header missing 5/ missing name of user in that email too 6/ I think the key may be shorter - 20 chars? 7/ oh! print_simple_box() is deprecated - use the alternatives instead
              Hide
              tsala Helen Foster added a comment -

              Hi, how is this going?

              Show
              tsala Helen Foster added a comment - Hi, how is this going?
              Hide
              nicolasconnault Nicolas Connault added a comment -

              Attached a patch with all the fixes Petr suggested

              Show
              nicolasconnault Nicolas Connault added a comment - Attached a patch with all the fixes Petr suggested
              Hide
              skodak Petr Skoda added a comment -

              thanks, I will review it on Wednesday

              Show
              skodak Petr Skoda added a comment - thanks, I will review it on Wednesday
              Hide
              skodak Petr Skoda added a comment -

              for some reason the patch does not apply for me to current 19x

              1/ missing $a->fullname = fullname($user, true); in auth_emailupdatemessage
              2/ there should be some info when editing profile that email change is pending

              the rest seems ok

              Show
              skodak Petr Skoda added a comment - for some reason the patch does not apply for me to current 19x 1/ missing $a->fullname = fullname($user, true); in auth_emailupdatemessage 2/ there should be some info when editing profile that email change is pending the rest seems ok
              Hide
              nicolasconnault Nicolas Connault added a comment -

              Fixed in 1.9: Also added a "Cancel email change" link and an admin option in Security -> Site policies section

              Show
              nicolasconnault Nicolas Connault added a comment - Fixed in 1.9: Also added a "Cancel email change" link and an admin option in Security -> Site policies section
              Hide
              skodak Petr Skoda added a comment -

              oops, this does not work because useredit_update_user_preference() was not intended for this

              working on a fix...

              Show
              skodak Petr Skoda added a comment - oops, this does not work because useredit_update_user_preference() was not intended for this working on a fix...
              Hide
              skodak Petr Skoda added a comment - - edited

              had to tweak it a bit, tested, works for me, yay!
              thanks everybody

              Show
              skodak Petr Skoda added a comment - - edited had to tweak it a bit, tested, works for me, yay! thanks everybody
              Hide
              jerome Jérôme Mouneyrac added a comment -

              there are some logs displayed on the top of the confirmation page (1.9):

              SMTP -> get_lines(): $data was ""
              SMTP -> get_lines(): $str is "220 outbound.icp-qv1-irony-out4.iinet.net.au ESMTP
              "
              SMTP -> get_lines(): $data is "220 outbound.icp-qv1-irony-out4.iinet.net.au ESMTP
              "
              SMTP -> FROM SERVER:
              220 outbound.icp-qv1-irony-out4.iinet.net.au ESMTP
              ...........

              Show
              jerome Jérôme Mouneyrac added a comment - there are some logs displayed on the top of the confirmation page (1.9): SMTP -> get_lines(): $data was "" SMTP -> get_lines(): $str is "220 outbound.icp-qv1-irony-out4.iinet.net.au ESMTP " SMTP -> get_lines(): $data is "220 outbound.icp-qv1-irony-out4.iinet.net.au ESMTP " SMTP -> FROM SERVER: 220 outbound.icp-qv1-irony-out4.iinet.net.au ESMTP ...........
              Hide
              aborrow Anthony Borrow added a comment -

              Where are you seeing these? I just tested with the latest from CVS and all seems to be well. I tested with both sendmail and SMTP server. I'm curious what might be causing this. Peace - Anthony

              Show
              aborrow Anthony Borrow added a comment - Where are you seeing these? I just tested with the latest from CVS and all seems to be well. I tested with both sendmail and SMTP server. I'm curious what might be causing this. Peace - Anthony
              Hide
              aborrow Anthony Borrow added a comment -

              Is this what Jerome means by confirmation page?

              Show
              aborrow Anthony Borrow added a comment - Is this what Jerome means by confirmation page?
              Hide
              aborrow Anthony Borrow added a comment -

              here is the confirmation after clicking on the link (w/smtp server)

              Show
              aborrow Anthony Borrow added a comment - here is the confirmation after clicking on the link (w/smtp server)
              Hide
              jerome Jérôme Mouneyrac added a comment -

              Anthony, it's apparently in lib/phpmailer/class.smtp.php, there is echo "SMTP -> get_lines(): \$data was \"$data\"" ...
              I keep going to test this issue and I'll write a bug for the log stuff if needed.

              Show
              jerome Jérôme Mouneyrac added a comment - Anthony, it's apparently in lib/phpmailer/class.smtp.php, there is echo "SMTP -> get_lines(): \$data was \"$data\"" ... I keep going to test this issue and I'll write a bug for the log stuff if needed.
              Hide
              jerome Jérôme Mouneyrac added a comment -

              tested on 1.9, 1.8 and Head

              My tests:
              When option is turned off:

              • student can change his email address

              When option is turned on

              • admin can change email address
              • student needs to confirm by email (try accept and cancel)
              • if email address has been taken by someone else before validation, an error message appears when student click on the email confirmation link

              Remarks:

              • In 1.8, the option is located in the "Module Security" folder. In 1.9 and HEAD it's in Site Policies. It would be better to be consistent between version.
              • If I don't set any SMPT server and turn the option on, a student can change the email address, then he gets an error (not very friendly on a blank page), and the profil page says that Moodle is waiting for a confirmation. However no email was sent. The student has to cancel the change. (behaviour tested on 1.9, 1.8 and 2.0)
              • my previous comment (08/Jul/08 11:16 AM) concerns only 1.9. These SMTP logs appear on the page displayed just after clicking on Update on the profil page. I'll write a new issue for that.

              Otherwise all works fine
              If the remarks are not going to be fixed, you can close the issue. Thanks.

              Show
              jerome Jérôme Mouneyrac added a comment - tested on 1.9, 1.8 and Head My tests: When option is turned off: student can change his email address When option is turned on admin can change email address student needs to confirm by email (try accept and cancel) if email address has been taken by someone else before validation, an error message appears when student click on the email confirmation link Remarks: In 1.8, the option is located in the "Module Security" folder. In 1.9 and HEAD it's in Site Policies. It would be better to be consistent between version. If I don't set any SMPT server and turn the option on, a student can change the email address, then he gets an error (not very friendly on a blank page), and the profil page says that Moodle is waiting for a confirmation. However no email was sent. The student has to cancel the change. (behaviour tested on 1.9, 1.8 and 2.0) my previous comment (08/Jul/08 11:16 AM) concerns only 1.9. These SMTP logs appear on the page displayed just after clicking on Update on the profil page. I'll write a new issue for that. Otherwise all works fine If the remarks are not going to be fixed, you can close the issue. Thanks.
              Hide
              jerome Jérôme Mouneyrac added a comment -

              about SMTP logs, it's not a bug, I had SMTP debug turned on in administration. Oops...

              Show
              jerome Jérôme Mouneyrac added a comment - about SMTP logs, it's not a bug, I had SMTP debug turned on in administration. Oops...
              Hide
              skodak Petr Skoda added a comment -

              I have moved the setting to correct category.

              Show
              skodak Petr Skoda added a comment - I have moved the setting to correct category.
              Hide
              skodak Petr Skoda added a comment -

              there is a die("could not send email!"); in user/edit.php, that should help diagnose smtp sending problems for now

              thanks for the review, reclosing

              Show
              skodak Petr Skoda added a comment - there is a die("could not send email!"); in user/edit.php, that should help diagnose smtp sending problems for now thanks for the review, reclosing
              Hide
              jerome Jérôme Mouneyrac added a comment -

              Ok 1.8, option setting is now in Sites Policies. All good.

              Show
              jerome Jérôme Mouneyrac added a comment - Ok 1.8, option setting is now in Sites Policies. All good.
              Hide
              dougiamas Martin Dougiamas added a comment -

              I made a little fix to make this work even when that user's email is currently disabled.

              Show
              dougiamas Martin Dougiamas added a comment - I made a little fix to make this work even when that user's email is currently disabled.
              Hide
              skodak Petr Skoda added a comment -

              enabled by default now

              Show
              skodak Petr Skoda added a comment - enabled by default now

                People

                • Votes:
                  3 Vote for this issue
                  Watchers:
                  4 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    11/Jul/08