Moodle
  1. Moodle
  2. MDL-23692

Change Forgotten Username or Password process

    Details

    • Testing Instructions:
      Hide

      1. Apply this patch, hit notifications to trigger db upgrade
      2. Go to login page, click the forgot password link
      3. Supply valid username
      4. Check email inbox for that user & click link in email from moodle
      5. Enter new password (twice)
      6. Check that you are now logged in, and can logout & log back in with your new password.

      You can re-run this procedure with permutations of

      • protect usernames on/off
      • new install of moodle with this code, vs upgrade.
      • supplying username, or email address at forgot password screen
      • supplying correct details multiple times (should get max 2 emails)
      • supplying invalid user details
        Although obviously the number of trials skyrockets pretty quickly.
      Show
      1. Apply this patch, hit notifications to trigger db upgrade 2. Go to login page, click the forgot password link 3. Supply valid username 4. Check email inbox for that user & click link in email from moodle 5. Enter new password (twice) 6. Check that you are now logged in, and can logout & log back in with your new password. You can re-run this procedure with permutations of protect usernames on/off new install of moodle with this code, vs upgrade. supplying username, or email address at forgot password screen supplying correct details multiple times (should get max 2 emails) supplying invalid user details Although obviously the number of trials skyrockets pretty quickly.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      passwordreset-2013-10-07-1031Z

      Description

      As the administrator for a 1.98 site, I have many users who say that the current forgotten password reset procedure is complicated. Would it be possible to have a system as follows:

      1. On the Forgotten password page, user types in username or email address.
      2. Email is sent to person. If they confirm that they want to change their password, they click on the provided link.
      3. They are taken to a change password page where they would need to enter a password conforming to the standards set up by the security settings.

      So instead of Moodle auto-generating a temporary password and emailing it to the person, the person can directly choose a new password.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Niall Julian added a comment -

            This is a reasonable request and I would like to see something long these lines implemented.

            Moodles problem is it sends automated emails. These get flagged as spam and don't always get through to users which causes a lot of grief. This is very apparent when the temp password email is generated, takes days to arrive and then the user can't use the temp password as it has expired. They then request another password, which gets delayed and so the circle continues.

            The process needs to be simpler while remaining secure.

            Show
            Niall Julian added a comment - This is a reasonable request and I would like to see something long these lines implemented. Moodles problem is it sends automated emails. These get flagged as spam and don't always get through to users which causes a lot of grief. This is very apparent when the temp password email is generated, takes days to arrive and then the user can't use the temp password as it has expired. They then request another password, which gets delayed and so the circle continues. The process needs to be simpler while remaining secure.
            Hide
            Beatriz Beltran added a comment -

            Absolutely needed! Yes! Moodle automated emails do get flagged as spam in most secure emails systems. Many users complain that the emails with their new passwords never arrive.

            Show
            Beatriz Beltran added a comment - Absolutely needed! Yes! Moodle automated emails do get flagged as spam in most secure emails systems. Many users complain that the emails with their new passwords never arrive.
            Hide
            David Bedelis added a comment -

            We run a moodle site with approx 7000 users. The problem with the password recovery process is:

            1. Users find the two step process confusing
            2. The recovery process is dependent on the user data having valid, up to date email addresses. This is problematic for large user bases as the admins will inevitably spend a lot of time updating user profiles.

            Would be good to have Password recovery functionality thats not dependent on the users email -e.g Secret answer type recovery

            Show
            David Bedelis added a comment - We run a moodle site with approx 7000 users. The problem with the password recovery process is: 1. Users find the two step process confusing 2. The recovery process is dependent on the user data having valid, up to date email addresses. This is problematic for large user bases as the admins will inevitably spend a lot of time updating user profiles. Would be good to have Password recovery functionality thats not dependent on the users email -e.g Secret answer type recovery
            Hide
            Peter Bulmer added a comment -

            Hi, without seeing this bug report, I wrote up a quick proposal:
            http://docs.moodle.org/dev/Password_Reset_Proposal

            and created a dicussion thread to get feedback:
            https://moodle.org/mod/forum/discuss.php?d=232821#p1012121

            There is obviously demand/need for it per: https://moodle.org/mod/forum/discuss.php?d=146969

            Can I confirm intrest from HQ? Do I need to write it up in more detail?

            Thanks,
            Peter

            Show
            Peter Bulmer added a comment - Hi, without seeing this bug report, I wrote up a quick proposal: http://docs.moodle.org/dev/Password_Reset_Proposal and created a dicussion thread to get feedback: https://moodle.org/mod/forum/discuss.php?d=232821#p1012121 There is obviously demand/need for it per: https://moodle.org/mod/forum/discuss.php?d=146969 Can I confirm intrest from HQ? Do I need to write it up in more detail? Thanks, Peter
            Hide
            Martin Dougiamas added a comment -

            I think it would be better too. I'm sure there was some reason for the two-step process but for the life of me I can't see it now.

            Note the change password page should require you to be logged in (with the old password). And the token should be short-lived. The link itself is not sufficient (people could steal that link via a number of means).

            Show
            Martin Dougiamas added a comment - I think it would be better too. I'm sure there was some reason for the two-step process but for the life of me I can't see it now. Note the change password page should require you to be logged in (with the old password). And the token should be short-lived. The link itself is not sufficient (people could steal that link via a number of means).
            Hide
            Peter Bulmer added a comment -

            Hi Martin,
            Re: short-lived tokens, I agree. I plan on them being both one-use, but also having an admin configurable validity period, I'm thinking the default should be 30 mins, to allow for some greylisting & other minor delays. Thoughts around what the default should be?

            Re: needing to be logged in for "change password". The current "I forgot my password" process uses both forgot_password.php and change_password.php. The first sets the temp password, emails x2, & applies force-password-change. The second is where the user changes to the final password after they have logged in with the temporary one.

            At this stage I don't envisage any changes for change_password.php. Users who have forgotten their password will now be dealt with entirely by improved forgot_password.php process. Users choosing to change their password will still use the unchanged change_password.php, (which requires login as you mention), and doesn't look to be broken.

            Martin/others, what are your thoughts around the default token validity period?

            Thanks,
            Pete

            Show
            Peter Bulmer added a comment - Hi Martin, Re: short-lived tokens, I agree. I plan on them being both one-use, but also having an admin configurable validity period, I'm thinking the default should be 30 mins, to allow for some greylisting & other minor delays. Thoughts around what the default should be? Re: needing to be logged in for "change password". The current "I forgot my password" process uses both forgot_password.php and change_password.php. The first sets the temp password, emails x2, & applies force-password-change. The second is where the user changes to the final password after they have logged in with the temporary one. At this stage I don't envisage any changes for change_password.php. Users who have forgotten their password will now be dealt with entirely by improved forgot_password.php process. Users choosing to change their password will still use the unchanged change_password.php, (which requires login as you mention), and doesn't look to be broken. Martin/others, what are your thoughts around the default token validity period? Thanks, Pete
            Hide
            Niall Julian added a comment -

            Pete,

            Speaking from personal experience 30mins wouldn't be long enough. Going back as far as Moodle 1.8 we've had major issues with the automated password approach (and the confirmation email for that matter) with many users never receiving the new password request email. When the it does finally arrive, it has expired. Which is frustrating for the user and gives a very bad impression of our site. We've tried everything we can to check mail server settings and have worked with our Moodle partner on it but to no avail. A lot of our users work in hospitals and most UK Trusts greylist everything. So its a major headache and at the of the day, we can't do anything about restrictions with other mail servers.

            Would extending the token validity beyond 30mins increase risk though? Our users just want a quick and simple password recovery process that doesn't send them round in circles (which is the complaint we get a lot).

            Thanks,

            Niall

            Show
            Niall Julian added a comment - Pete, Speaking from personal experience 30mins wouldn't be long enough. Going back as far as Moodle 1.8 we've had major issues with the automated password approach (and the confirmation email for that matter) with many users never receiving the new password request email. When the it does finally arrive, it has expired. Which is frustrating for the user and gives a very bad impression of our site. We've tried everything we can to check mail server settings and have worked with our Moodle partner on it but to no avail. A lot of our users work in hospitals and most UK Trusts greylist everything. So its a major headache and at the of the day, we can't do anything about restrictions with other mail servers. Would extending the token validity beyond 30mins increase risk though? Our users just want a quick and simple password recovery process that doesn't send them round in circles (which is the complaint we get a lot). Thanks, Niall
            Hide
            Petr Skoda added a comment -

            Maybe this should be a bit more pluggable, for example resetting password via sms gateway.

            Show
            Petr Skoda added a comment - Maybe this should be a bit more pluggable, for example resetting password via sms gateway.
            Hide
            Niall Julian added a comment -

            I like the sound of that Petr!

            Show
            Niall Julian added a comment - I like the sound of that Petr!
            Hide
            Peter Bulmer added a comment -

            I can certainly see how in the current system that if your mail is slow you'll have problems. Even normal speed mail will generate problems for some users. Because there are two password reset fields (username or email), and protectusernames means users get an obscure message even on success, AND if you re-submit your reset request, the 'secret' gets changed, you'll get a lot of people retrying, and the emails that eventually come through will be invalid, while the currently-valid email may still be in transit.

            My planned approach will make this better in a couple of ways:
            1. Resubmissions of the forgot my password form while the 1st one is still valid will not result in the 1st email which is already in transit getting it's token made invalid before it even gets to the inbox.
            2. I can make the process willing to send one repeat email with basically the same content as the first if re-requested by the user. In my experience, this often helps with greylisting, the 2nd email either gets through because of the 1st email's earlier attempt, or prompts the 1st email's retry to happen earlier.
            3. I can make the token very long (32 characters), making it impractical to brute force, even if you set your validity much longer than 30 minutes.
            4. With one fewer emails that need to get through, the whole process is at least 50% easier from the get-go.

            Show
            Peter Bulmer added a comment - I can certainly see how in the current system that if your mail is slow you'll have problems. Even normal speed mail will generate problems for some users. Because there are two password reset fields (username or email), and protectusernames means users get an obscure message even on success, AND if you re-submit your reset request, the 'secret' gets changed, you'll get a lot of people retrying, and the emails that eventually come through will be invalid, while the currently-valid email may still be in transit. My planned approach will make this better in a couple of ways: 1. Resubmissions of the forgot my password form while the 1st one is still valid will not result in the 1st email which is already in transit getting it's token made invalid before it even gets to the inbox. 2. I can make the process willing to send one repeat email with basically the same content as the first if re-requested by the user. In my experience, this often helps with greylisting, the 2nd email either gets through because of the 1st email's earlier attempt, or prompts the 1st email's retry to happen earlier. 3. I can make the token very long (32 characters), making it impractical to brute force, even if you set your validity much longer than 30 minutes. 4. With one fewer emails that need to get through, the whole process is at least 50% easier from the get-go.
            Hide
            Peter Bulmer added a comment -

            SMS sounds nice & all, but setting up an SMS gateway for Moodle to talk to is probabally beyond the majority of Moodle administators' time and money budgets, and/or their technical capabilities.

            Plugable can come later IMO, maybe should create a separate bug for that. My client needs to see something in their testing site this week, so what I'm proposing above will probabally be what they see. If wholesale changes are needed after that to get this into core, it probabally won't go in.

            Thanks
            Peter.

            Show
            Peter Bulmer added a comment - SMS sounds nice & all, but setting up an SMS gateway for Moodle to talk to is probabally beyond the majority of Moodle administators' time and money budgets, and/or their technical capabilities. Plugable can come later IMO, maybe should create a separate bug for that. My client needs to see something in their testing site this week, so what I'm proposing above will probabally be what they see. If wholesale changes are needed after that to get this into core, it probabally won't go in. Thanks Peter.
            Hide
            Dan Marsden added a comment -

            didn't get a chance to do a full review but here are my comments so far:

            unsure on extra carriage return after array( - inconsistent in existing code - not sure what the guideline is here.
            https://github.com/peterbulmer/moodle/compare/passwordreset-2013-08-08-0425Z?expand=1#L0R74

            2 spaces after fullstop:
            https://github.com/peterbulmer/moodle/compare/passwordreset-2013-08-08-0425Z?expand=1#L1R280

            typo in lang "validiate":
            https://github.com/peterbulmer/moodle/compare/passwordreset-2013-08-08-0425Z?expand=1#L1R787

            This doesn't read well:
            https://github.com/peterbulmer/moodle/compare/passwordreset-2013-08-08-0425Z?expand=1#L2R537
            maybe "A password reset .."

            https://github.com/peterbulmer/moodle/compare/passwordreset-2013-08-08-0425Z?expand=1#L2R605
            maybe detail->information? "..please check the information you added..." ?

            Camel case:
            https://github.com/peterbulmer/moodle/compare/passwordreset-2013-08-08-0425Z?expand=1#L2R661
            maybe should be "{$a}: Password reset request'

            login/lib.php -
            PHPDOC comments forgotpw_process_request() doesn't look right - have the others been checked?
            Various codechecker errors with comments and line lengths

            set_password_form.php
            few codechecker errors/warnings on php comments.

            Show
            Dan Marsden added a comment - didn't get a chance to do a full review but here are my comments so far: unsure on extra carriage return after array( - inconsistent in existing code - not sure what the guideline is here. https://github.com/peterbulmer/moodle/compare/passwordreset-2013-08-08-0425Z?expand=1#L0R74 2 spaces after fullstop: https://github.com/peterbulmer/moodle/compare/passwordreset-2013-08-08-0425Z?expand=1#L1R280 typo in lang "validiate": https://github.com/peterbulmer/moodle/compare/passwordreset-2013-08-08-0425Z?expand=1#L1R787 This doesn't read well: https://github.com/peterbulmer/moodle/compare/passwordreset-2013-08-08-0425Z?expand=1#L2R537 maybe "A password reset .." https://github.com/peterbulmer/moodle/compare/passwordreset-2013-08-08-0425Z?expand=1#L2R605 maybe detail->information? "..please check the information you added..." ? Camel case: https://github.com/peterbulmer/moodle/compare/passwordreset-2013-08-08-0425Z?expand=1#L2R661 maybe should be "{$a}: Password reset request' login/lib.php - PHPDOC comments forgotpw_process_request() doesn't look right - have the others been checked? Various codechecker errors with comments and line lengths set_password_form.php few codechecker errors/warnings on php comments.
            Hide
            Peter Bulmer added a comment -

            I have addressed the above issues, and pushed to a new branch. New branch & diff url saved against this tracker item.

            Thanks Dan for your review.

            Does this need another review?

            Thanks,
            Pete

            Show
            Peter Bulmer added a comment - I have addressed the above issues, and pushed to a new branch. New branch & diff url saved against this tracker item. Thanks Dan for your review. Does this need another review? Thanks, Pete
            Hide
            Dan Marsden added a comment -

            thanks Pete - yes this needs another review - hopefully someone from HQ will pick it up - (if not I'll bug someone...)

            Show
            Dan Marsden added a comment - thanks Pete - yes this needs another review - hopefully someone from HQ will pick it up - (if not I'll bug someone...)
            Hide
            Frédéric Massart added a comment -

            Hi Peter,

            thanks for working on this. Here are some comments following my core review.

            admin/settings/security.php

            1. I am not sure if it is wise to introduce 'yet another admin setting', could we consider adding that to config-dist.php instead? Or is there really a good reason for administrators to change this setting at all?

            lib/cronlib.php

            1. 176 Would it be safer to ensure that $CFG->pwresettime is valid before using it? I think it could happen that the codebase is upgraded, but not yet the site. If then the cron runs there might be issues. I see you are doing that elsewhere.
            2. Could you comment why it is better to keep the password_reset tokens for one more day?

            lib/db/upgrade.php

            1. Did you use the XMLDB Editor to get the PHP code to create the new table? It seems that some default values are different.

            lib/moodlelib.php

            1. I am not sure I understand the PHP Doc on line 5984. Also, what is '@global object'?
            2. send_email_confirmation(): Should you round() $CFG->pwresettime/MINSECS to ensure that this doesn't end up being a float? You could also add white space around the /. Actually, I guess this value could be passed via $resetrecord, as the token was generated from there based on $CFG->pwresettime.

            forgot_password.php

            1. I would suggest you update the require_once to `require_once(_DIR_ . '/file.php');` this is minor but that ensures that PHP is not trying to include the file from somewhere else, and it is the fastest.
            2. Could you explain why you changed the value of redirect()?
            3. Could we check that the user is not the guest user when checking if we can send the email?

            login/lib.php

            1. 178: I would personally set a reset URL including the USERID in GET parameters, to then only query on that specific user. Would that make sense?
              • If so, you can restore the previous behaviour which was removing the token if someone tried to guess it. Or perhaps it should not remove it but lock it, until it expires, so that you can't just request it again, and guess again, etc...
            2. 195: Rounding the value?
            3. create_reset_record(): There could (very unlikely) be 2 identical tokens for the same user. Perhaps you could sha1(USERID . ':' . random_string(32) . ':' . time()) or so.

            General

            1. Could you deprecate all the functions and files which are not used any more? I guess the files that will be deprecated should redirect to the related new file.
            2. There are a couple of coding style issues, some in legacy code, some in the new one. You don't have to fix everything, but that would be nice if you could fix what you've moved from one place to another.
            3. I don't quite like the fact that all the process is moved to new functions. The fact that those are arbitrarily using $OUTPUT, $PAGE, etc... doesn't make them re-usable. Could you keep the output code in the pages where it belongs, and leave the logic in the functions?
            4. Your commit messages are not consistent and some don't really have any 'story' value. Could you squash some of them?
            5. Would it be a good time to switch all those URLs to use moodle_url()>out(false)? You would still have to use $CFG>httpswwwroot though.
            6. I wonder if instead of creating a lib.php file we should create a new class which encapsulates those. If so, it could benefit from autoloading. Just a thought.

            The patch you've provided is really good, and even if this list can look a bit long, most of your work is ready and doesn't need any change.

            Many thanks!
            Fred

            Show
            Frédéric Massart added a comment - Hi Peter, thanks for working on this. Here are some comments following my core review. admin/settings/security.php I am not sure if it is wise to introduce 'yet another admin setting', could we consider adding that to config-dist.php instead? Or is there really a good reason for administrators to change this setting at all? lib/cronlib.php 176 Would it be safer to ensure that $CFG->pwresettime is valid before using it? I think it could happen that the codebase is upgraded, but not yet the site. If then the cron runs there might be issues. I see you are doing that elsewhere. Could you comment why it is better to keep the password_reset tokens for one more day? lib/db/upgrade.php Did you use the XMLDB Editor to get the PHP code to create the new table? It seems that some default values are different. lib/moodlelib.php I am not sure I understand the PHP Doc on line 5984. Also, what is '@global object'? send_email_confirmation(): Should you round() $CFG->pwresettime/MINSECS to ensure that this doesn't end up being a float? You could also add white space around the /. Actually, I guess this value could be passed via $resetrecord, as the token was generated from there based on $CFG->pwresettime. forgot_password.php I would suggest you update the require_once to `require_once(_ DIR _ . '/file.php');` this is minor but that ensures that PHP is not trying to include the file from somewhere else, and it is the fastest. Could you explain why you changed the value of redirect() ? Could we check that the user is not the guest user when checking if we can send the email? login/lib.php 178: I would personally set a reset URL including the USERID in GET parameters, to then only query on that specific user. Would that make sense? If so, you can restore the previous behaviour which was removing the token if someone tried to guess it. Or perhaps it should not remove it but lock it, until it expires, so that you can't just request it again, and guess again, etc... 195: Rounding the value? create_reset_record(): There could (very unlikely) be 2 identical tokens for the same user. Perhaps you could sha1(USERID . ':' . random_string(32) . ':' . time()) or so. General Could you deprecate all the functions and files which are not used any more? I guess the files that will be deprecated should redirect to the related new file. There are a couple of coding style issues, some in legacy code, some in the new one. You don't have to fix everything, but that would be nice if you could fix what you've moved from one place to another. I don't quite like the fact that all the process is moved to new functions. The fact that those are arbitrarily using $OUTPUT, $PAGE, etc... doesn't make them re-usable. Could you keep the output code in the pages where it belongs, and leave the logic in the functions? Your commit messages are not consistent and some don't really have any 'story' value. Could you squash some of them? Would it be a good time to switch all those URLs to use moodle_url()>out(false)? You would still have to use $CFG>httpswwwroot though. I wonder if instead of creating a lib.php file we should create a new class which encapsulates those. If so, it could benefit from autoloading. Just a thought. The patch you've provided is really good, and even if this list can look a bit long, most of your work is ready and doesn't need any change. Many thanks! Fred
            Hide
            Peter Bulmer added a comment -

            Hi Fred,
            Thanks for your solid review. I haven't yet looked through it all, but some early responses:

            admin/settings/security.php
            1. re: yet another admin setting.
            Open to advice here. I wanted to set a tight default so that sites were highly secure by default, and give admins like Niall Julian flexiblity to deal with slow email systems (see comment dated 26/Jul/13 9:03 AM on this tracker item). Should I change it?

            lib/cronlib.php
            2. re: why keep tokens after they expire?
            The user gets a different error message when they try to use a recently-expired token, compared with one which doesn't exist. If you request a reset, but get distracted before the emails comes, then click the link after 32 minutes, it would be easy to think you clicked within 30 mins; The system looks to be broken if it just says "invalid token".
            I'll add a note in cronlib explaining why the DAYSECS difference.

            Show
            Peter Bulmer added a comment - Hi Fred, Thanks for your solid review. I haven't yet looked through it all, but some early responses: admin/settings/security.php 1. re: yet another admin setting. Open to advice here. I wanted to set a tight default so that sites were highly secure by default, and give admins like Niall Julian flexiblity to deal with slow email systems (see comment dated 26/Jul/13 9:03 AM on this tracker item). Should I change it? lib/cronlib.php 2. re: why keep tokens after they expire? The user gets a different error message when they try to use a recently-expired token, compared with one which doesn't exist. If you request a reset, but get distracted before the emails comes, then click the link after 32 minutes, it would be easy to think you clicked within 30 mins; The system looks to be broken if it just says "invalid token". I'll add a note in cronlib explaining why the DAYSECS difference.
            Hide
            Peter Bulmer added a comment -

            re General 1. deprecate unused functions & files. I'm not sure what needs deprecating. Can you please develop on this point
            re General 2. "coding style issues, some in legacy code, some in the new one" Can you give examples of this. Prior to Dan reviewing my code I was trying to not change code which I moved into functions, even where it contravened what I understood to be coding standards. On his advice, I changed this, and reviewed the code using the code checker, fixing all errors and warnings in code that I had written, or moved. I've re-checked at some length, but can only find two warnings in lib/db/upgrade.php (Which I'll fix). Can you give examples of what you mean by coding style issues?
            re General 4. 'commit messages not consistent, no story value, can some be squashed'. The whole lot can be squashed, I was trying to keep the commits small & understandable, rather than one 5000 line commit, per http://docs.moodle.org/dev/Working_with_the_Community.
            re General 5. 'changing url to moodle_url()>out(false)?'. Not sure what's involved here - like to avoid further complication.

            Show
            Peter Bulmer added a comment - re General 1. deprecate unused functions & files. I'm not sure what needs deprecating. Can you please develop on this point re General 2. "coding style issues, some in legacy code, some in the new one" Can you give examples of this. Prior to Dan reviewing my code I was trying to not change code which I moved into functions, even where it contravened what I understood to be coding standards. On his advice, I changed this, and reviewed the code using the code checker, fixing all errors and warnings in code that I had written, or moved. I've re-checked at some length, but can only find two warnings in lib/db/upgrade.php (Which I'll fix). Can you give examples of what you mean by coding style issues? re General 4. 'commit messages not consistent, no story value, can some be squashed'. The whole lot can be squashed, I was trying to keep the commits small & understandable, rather than one 5000 line commit, per http://docs.moodle.org/dev/Working_with_the_Community . re General 5. 'changing url to moodle_url()>out(false)?'. Not sure what's involved here - like to avoid further complication.
            Hide
            Frédéric Massart added a comment -

            Hi Peter,

            Security 1/ I don't have any strong opinion on this, but if really unlikely to be changed this setting could be set in config-dist.php instead.

            Cronlib 2/ I see, but why an entire day? I would assume that 1 hour after the token expiry is enough. This is not really important to be honest.

            General 1/ Actually, I assumed that you were removing the usage of change_password.php, but apparently you are not. What I meant is that if you were to remove the usage of a file or function, then it should be deprecated.

            General 2/ Well, we do not have a strong policy on this, but usually when we move some code we try to clean it up at the same time. I don't remember precisely, I had seen some extra whitespace between the function name and the parenthesis, perhaps some identation or PHP docs issues as well. Nothing really major, but it's appreciated when the code is cleaned up when moved.

            General 4/ Having multiple commits is not a problem, but "coding styletidyups" followed by "coding style tidyups 2" doesn't have much value, they should could be squashed. Also, you could be consisdent in the way you separate the component from the rest.

            General 5/ Fine with me, it's up to you.

            Cheers,
            Fred

            Show
            Frédéric Massart added a comment - Hi Peter, Security 1/ I don't have any strong opinion on this, but if really unlikely to be changed this setting could be set in config-dist.php instead. Cronlib 2/ I see, but why an entire day? I would assume that 1 hour after the token expiry is enough. This is not really important to be honest. General 1/ Actually, I assumed that you were removing the usage of change_password.php, but apparently you are not. What I meant is that if you were to remove the usage of a file or function, then it should be deprecated. General 2/ Well, we do not have a strong policy on this, but usually when we move some code we try to clean it up at the same time. I don't remember precisely, I had seen some extra whitespace between the function name and the parenthesis, perhaps some identation or PHP docs issues as well. Nothing really major, but it's appreciated when the code is cleaned up when moved. General 4/ Having multiple commits is not a problem, but "coding styletidyups" followed by "coding style tidyups 2" doesn't have much value, they should could be squashed. Also, you could be consisdent in the way you separate the component from the rest. General 5/ Fine with me, it's up to you. Cheers, Fred
            Hide
            Peter Bulmer added a comment -

            Thanks heaps for your ongoing read & review time.

            I've had a read over & investigated all the remaining points, I should have fixes for most of them shortly.

            There are a couple of points I'd like to ask you / other core people about further before changing:

            The first relates to login/lib.php, points 1 & 3; Duplicates / cold guesses have been made very unlikely by lengthening the size of the secret/token. With 32 lowercase, uppercase, and numeric characters, there are 62^32 combinations.

            My maths on this was: assuming there were 500 password reset requests for valid user accounts (leading to 1 Million valid tokens at any one time), and 500 token guesses per second. With these numbers you'd still expect it to be millions of years before you get a hit. Much faster to just guess passwords directly.
            That said I was keen to avoid the 'clear the secret on failure' functionality on the basis that it meant attackers could be a nuisance by preventing real password resets, without providing security improvements. It we wanted to limit the 500/sec, then we could add functionality which requires captcha validation above a certain level of reset requests & invalid token redemptions. But again, an enhancement that can come later.

            The second point I'd like to ask about relates to using OUTPUT, PAGE etc in functions. You're right in pointing out that these vars in functions is an indicator that logic and presentation aren't cleanly separated. I moved code into functions because I thought it more a lot more understandable, and clearly demarked different sections of code. I wasn't worried about using these global vars in functions because logic and presentation were already intertwined - putting the code into functions just made the symptom show up more clearly.
            I see 3 options:

            • Leave it using globals in functions with the logic "it's the same as what it was, only more transparent"
            • Move it out of the functions back to the big if/else block to get rid of the globals
            • Rewrite the functions to separate logic & presentation. This would be a big rewrite, and not something I have time for at this stage. Additionally there would need to be a lot of interaction with HQ ppl about just how to treat the curly cases where intention of the pre-existing code wasn't really clear - (eg unconfirmed users).

            _Really_ hoping I can sell you on the first option.


            Thoughts on those two points?

            Show
            Peter Bulmer added a comment - Thanks heaps for your ongoing read & review time. I've had a read over & investigated all the remaining points, I should have fixes for most of them shortly. There are a couple of points I'd like to ask you / other core people about further before changing: The first relates to login/lib.php, points 1 & 3; Duplicates / cold guesses have been made very unlikely by lengthening the size of the secret/token. With 32 lowercase, uppercase, and numeric characters, there are 62^32 combinations. My maths on this was: assuming there were 500 password reset requests for valid user accounts (leading to 1 Million valid tokens at any one time), and 500 token guesses per second. With these numbers you'd still expect it to be millions of years before you get a hit. Much faster to just guess passwords directly. That said I was keen to avoid the 'clear the secret on failure' functionality on the basis that it meant attackers could be a nuisance by preventing real password resets, without providing security improvements. It we wanted to limit the 500/sec, then we could add functionality which requires captcha validation above a certain level of reset requests & invalid token redemptions. But again, an enhancement that can come later. The second point I'd like to ask about relates to using OUTPUT, PAGE etc in functions. You're right in pointing out that these vars in functions is an indicator that logic and presentation aren't cleanly separated. I moved code into functions because I thought it more a lot more understandable, and clearly demarked different sections of code. I wasn't worried about using these global vars in functions because logic and presentation were already intertwined - putting the code into functions just made the symptom show up more clearly. I see 3 options: Leave it using globals in functions with the logic "it's the same as what it was, only more transparent" Move it out of the functions back to the big if/else block to get rid of the globals Rewrite the functions to separate logic & presentation. This would be a big rewrite, and not something I have time for at this stage. Additionally there would need to be a lot of interaction with HQ ppl about just how to treat the curly cases where intention of the pre-existing code wasn't really clear - (eg unconfirmed users). _ Really _ hoping I can sell you on the first option. Thoughts on those two points?
            Hide
            Dan Marsden added a comment -

            Ping - Fred?

            Show
            Dan Marsden added a comment - Ping - Fred?
            Hide
            Frédéric Massart added a comment -

            Hi Peter,

            sorry for the delay. You don't really have to sell me on any option, I was just raising points. Now I understand that this could take some time and so we might end up integrating this and raising an issue to clean the code later on. The only judges of this will be the integrators I'm afraid.

            If you're confident that your patch is ready for integration, please submit it. If you don't have the rights to do so, let us know. But before that, could you please write some testing instructions that would cover all the required steps in order for a human to test your patch?

            Many thanks!
            Fred

            Show
            Frédéric Massart added a comment - Hi Peter, sorry for the delay. You don't really have to sell me on any option, I was just raising points. Now I understand that this could take some time and so we might end up integrating this and raising an issue to clean the code later on. The only judges of this will be the integrators I'm afraid. If you're confident that your patch is ready for integration, please submit it. If you don't have the rights to do so, let us know. But before that, could you please write some testing instructions that would cover all the required steps in order for a human to test your patch? Many thanks! Fred
            Hide
            Peter Bulmer added a comment -

            Awesome, I'll look to re-read the discussion above, and provide a new patchset & submit for integration.

            Thanks Fred, Dan.

            Peter.

            Show
            Peter Bulmer added a comment - Awesome, I'll look to re-read the discussion above, and provide a new patchset & submit for integration. Thanks Fred, Dan. Peter.
            Hide
            Dan Marsden added a comment -

            Hey Pete - how are you going with this one? - FYI 2.6 freeze occurs on 7th October so it would be good to get this in before then if poss!

            Show
            Dan Marsden added a comment - Hey Pete - how are you going with this one? - FYI 2.6 freeze occurs on 7th October so it would be good to get this in before then if poss!
            Hide
            Petr Skoda added a comment -

            Hello, I do not think this is ready for integration yet:

            1. the PHP docs blocks are often invalid, they must start with /** not /*
            2. the SQL query syntax is not standard - indentation, INNER, single quotes
            3. the function names in login/lib.php are too general and I personally think it is confusing to put that code into lib.php because there is no code duplication
            4. whitespace problems
            5. we started to use markdown in lang strings instead of html
            6. user_password_resets table lacks foreign key to the user table
            7. "@param stdClass $resetrecord An object tracking metadata re password reset request", "re" typo
            8. token comes from random_string() which means it is not PARAM_RAW
            9. use !== instead of <> for string comparisons
            10. why enable password form field autocomplete on reset form? why the styles on the form (style="display: none;")? I think the use of hidden username on the reset form is wrong.
            11. what for is the hidden id parameter on the password reset form?
            12. verifying of $DB->insert result is wrong, you will either get new id or exception

            The list of problems is not complete, this is not a peer-review. In any case thanks for your work.

            Show
            Petr Skoda added a comment - Hello, I do not think this is ready for integration yet: the PHP docs blocks are often invalid, they must start with /** not /* the SQL query syntax is not standard - indentation, INNER, single quotes the function names in login/lib.php are too general and I personally think it is confusing to put that code into lib.php because there is no code duplication whitespace problems we started to use markdown in lang strings instead of html user_password_resets table lacks foreign key to the user table "@param stdClass $resetrecord An object tracking metadata re password reset request", "re" typo token comes from random_string() which means it is not PARAM_RAW use !== instead of <> for string comparisons why enable password form field autocomplete on reset form? why the styles on the form (style="display: none;")? I think the use of hidden username on the reset form is wrong. what for is the hidden id parameter on the password reset form? verifying of $DB->insert result is wrong, you will either get new id or exception The list of problems is not complete, this is not a peer-review. In any case thanks for your work.
            Hide
            Sam Hemelryk added a comment -

            Hi Peter,

            Thanks for the excellent work on this so far.

            I've chosen to reopen this issue this week in light of Petr's review of the code.
            All noted issues look pretty minor and hopefully this is an easy fix. Certainly this is getting very close to being ready for integration but it would be nice to have those things tidied up.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Hi Peter, Thanks for the excellent work on this so far. I've chosen to reopen this issue this week in light of Petr's review of the code. All noted issues look pretty minor and hopefully this is an easy fix. Certainly this is getting very close to being ready for integration but it would be nice to have those things tidied up. Many thanks Sam
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Peter Bulmer added a comment -

            Thanks Sam,

            Can I run through Petr's points & ask you a couple of questions:
            1: re: php doc blocks, I can add extra "*" - no question here
            2: re: using "INNER JOIN" instead of "JOIN", it is redundant, but I always thought being explicit here makes it clearer. Can change, no problem.
            re: indentation, single quotes. Is this a new standard?
            3. re: confusing to put code into function when it's only called once: I think that putting these routines into functions clearly defines scope, and improves readability. It's pretty easy for these things to get several indentations deep, and span vertically across more than one screen. If someone is just reading the code, they won't know that it's only used once, so I can't see how it's confusing. Do I need to put this back into a monolithic file to land this patch? If not, do you have thoughts on the function names?
            4. "Whitespace problems", can you give examples, and suggest ways that I can spot the rest of the problems.
            5. "we started to use markdown in lang strings instead of html". Can you point to an example of where this is a problem in my patch, and a place where moodle does this right, so I can understand what Petr meant by this.
            6. No prob, can find out how to add a foreign key & add it.
            7. re: "re", This isn't a typo, it means "regarding". Is this a problem?
            8. I can try to figure out what he means by this & fix it.
            9. re: "!==" vs "<>", I can find & fix this, but is it covered in some coding guidelines somewhere, I'd like to read the rational behind it.

            10. Autocomplete is enabled on the password set form so that the browser knows that a new password is being set. Most browsers offer to track domain/username/password combinations, "Should firefox remember this password" (or similar prompts). To do this it needs autocomplete on, and it needs to know the username. This is how a lot of people manage their passwords, they get their browser to remember, and each time they reinstall their OS, they do a password reset. Since the user will be logged in after doing the password reset, they won't go through the normal login/index.php until after they have logged out & want to log in again - potentaially a week or more later, and they may have forgotten the password they set. If autocomplete is on, and username is in the form, they'll be asked "do you want firefox to remember ...", and the next time they hit login/index.php, it'll autocomplete => win. Username needs to be hidden because for most users its presence will be confusing.

            11. Alright, I was cargo culting there it seems The same field (id) was already in change_password_form.php & unused there too (I think). I can remove it from my new form.
            12. I can remove verification of DB->insert - no problem there

            Last point (Unlucky #13?)
            This is the killer, and I can't really go ahead with any of the above until I understand this better.
            Re: list of problems not being complete, and Skodak's review not being a peer review. Can you fill in what he means by "not a peer-review", and what the other problems are.

            Thanks for your reply Sam, I really appreciate your positive tone.
            Pete.

            Show
            Peter Bulmer added a comment - Thanks Sam, Can I run through Petr's points & ask you a couple of questions: 1: re: php doc blocks, I can add extra "*" - no question here 2: re: using "INNER JOIN" instead of "JOIN", it is redundant, but I always thought being explicit here makes it clearer. Can change, no problem. re: indentation, single quotes. Is this a new standard? 3. re: confusing to put code into function when it's only called once: I think that putting these routines into functions clearly defines scope, and improves readability. It's pretty easy for these things to get several indentations deep, and span vertically across more than one screen. If someone is just reading the code, they won't know that it's only used once, so I can't see how it's confusing. Do I need to put this back into a monolithic file to land this patch? If not, do you have thoughts on the function names? 4. "Whitespace problems", can you give examples, and suggest ways that I can spot the rest of the problems. 5. "we started to use markdown in lang strings instead of html". Can you point to an example of where this is a problem in my patch, and a place where moodle does this right, so I can understand what Petr meant by this. 6. No prob, can find out how to add a foreign key & add it. 7. re: "re", This isn't a typo, it means "regarding". Is this a problem? 8. I can try to figure out what he means by this & fix it. 9. re: "!==" vs "<>", I can find & fix this, but is it covered in some coding guidelines somewhere, I'd like to read the rational behind it. 10. Autocomplete is enabled on the password set form so that the browser knows that a new password is being set. Most browsers offer to track domain/username/password combinations, "Should firefox remember this password" (or similar prompts). To do this it needs autocomplete on, and it needs to know the username. This is how a lot of people manage their passwords, they get their browser to remember, and each time they reinstall their OS, they do a password reset. Since the user will be logged in after doing the password reset, they won't go through the normal login/index.php until after they have logged out & want to log in again - potentaially a week or more later, and they may have forgotten the password they set. If autocomplete is on, and username is in the form, they'll be asked "do you want firefox to remember ...", and the next time they hit login/index.php, it'll autocomplete => win. Username needs to be hidden because for most users its presence will be confusing. 11. Alright, I was cargo culting there it seems The same field (id) was already in change_password_form.php & unused there too (I think). I can remove it from my new form. 12. I can remove verification of DB->insert - no problem there Last point (Unlucky #13?) This is the killer, and I can't really go ahead with any of the above until I understand this better. Re: list of problems not being complete, and Skodak's review not being a peer review. Can you fill in what he means by "not a peer-review", and what the other problems are. Thanks for your reply Sam, I really appreciate your positive tone. Pete.
            Hide
            Sam Hemelryk added a comment -

            Alrighty - sorry it took me so long to get back here.
            In reply to the question you've got that you don't already have the answer for:

            2) http://docs.moodle.org/dev/SQL_coding_style details our SQL coding standard, that should answer most of your questions there. As for the use of INNER it is used within core, if you really want to leave it there you can however in majority of cases we do not specify it explicility as it is explicitly implied.

            3a). First up what should be a function and what shouldn't.
            Of the functions in lib.php I think there is only one that shouldn't be there and that is forgotpw_process_request. To me that reads as though it doing the work of the forgot_password.php. Especially as it is calling echo $OUTPUT->header/footer. Personally I would look perhaps at how to better separate the output and the logic there, moving the output code back to forgot_password.php and keeping the logic encapsulated within a function in lib.php.
            The same could be said about forgotpw_process_pwset actually.
            Its not a perfect task unfortunately, I think it would be worth looking at how and what you've put into functions there if you have time you can afford to put that way.
            At the end of the day we're very close to freeze (next monday) and as this is an improvement it must be there by then. If you don't have time to rework this I'd suggest still putting this up and the integrators can discuss and decide wether having this as it is currently structured is worth it, and if not provide better instruction thanm I have here.

            3b) I do think Petr has a very good point about the naming of the functions. At present they're not always clear or consistent and in some cases not unique enough.
            Perhaps prefixing them with the 'core_login_' or similiar will help. In some circumstances a complete rename may be best. The following are quick suggestions, I've not given this a lot of time but perhaps will provide some insight into my thoughts on this issue:

            • forgotpw_process_request => core_login_request_password_reset
            • forgotpw_process_pwset => core_login_process_password_reset
            • create_reset_record => core_login_generate_password_reset
            • get_postlogin_redirection => core_login_get_return_url

            4) White space requirements are detailed in http://docs.moodle.org/dev/Coding_style. Really worth mentioning are the useful tools under http://docs.moodle.org/dev/Coding_style#Useful_tools If you run those two tools they will report the issues that exist. They're easy to run and are the perfect final step before pushing code to review.
            Also if you are using an IDE such as PHPStorm or Netbeans you can integrate these tools with it to provide automatic highlighting of issues directly within your IDE. The readme files for the plugins contains details of how to do this I believe).
            Do keep in mind that we are pretty strict about our whitespace, it can take a while to get used to but stick with it.
            (noting that Marina's moodlecheck is available through https://moodle.org/plugins/view.php?plugin=local_moodlecheck as well, I'll find someone to update that link)

            5) As an example of markdown use within lang strings check out https://github.com/samhemelryk/moodle/blob/master/lang/en/badges.php#L304 (there are other strings with HTML in use there). I'm not aware of it being a rule that markdown has to be used (we still have a lot of HTML in lang strings) however certainly with new strings we'd love to see markdown used.
            Perhaps Petr could clarify this but to the best of my knowledge if you use markdown you must also make sure you format the string before displaying it.

            7) I get what you mean by "re" however perhaps its not clear to those for whom English is not their first language. We generally try to be clear and definitive in our language (and naming as above) and I'd suggest using the full term rather than the re abbreviation. Not a biggy though, I'm sure if Petr were to re-read it he'd understand.

            8) Ah PARAM_RAW lets too much through in this case for sure, no doubt there is a more appropriate param type you could use there.

            9) If its going to be documented anywhere it would be in http://docs.moodle.org/dev/Coding_style however I don't believe there is documenation about perferred operators.
            We discussed the use of and/or the other day and came to the conclussion that they are permitable to use as long as they are not mixed with &&/||. Really I would assume the same here, you are free to use <> as long as you don't mix it with other operators.
            The general preference is for && || !== ===.
            Worth noting about <> is that it is not type strict. In situations where you are comparing two strings we would definitely prefer to see !== used as that is of course type strict. That is a rule and the logic behind it being that we want our checks to be as tight as possible.

            13) I can't tell you why Petr put that however most likely it is because he's either not looked at all of the code, or has not gone though it to the best of his capability (as in he's just had a quick look). I'd strongly suggest putting this up for peer-review again when you are ready once more.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Alrighty - sorry it took me so long to get back here. In reply to the question you've got that you don't already have the answer for: 2) http://docs.moodle.org/dev/SQL_coding_style details our SQL coding standard, that should answer most of your questions there. As for the use of INNER it is used within core, if you really want to leave it there you can however in majority of cases we do not specify it explicility as it is explicitly implied. 3a). First up what should be a function and what shouldn't. Of the functions in lib.php I think there is only one that shouldn't be there and that is forgotpw_process_request. To me that reads as though it doing the work of the forgot_password.php. Especially as it is calling echo $OUTPUT->header/footer. Personally I would look perhaps at how to better separate the output and the logic there, moving the output code back to forgot_password.php and keeping the logic encapsulated within a function in lib.php. The same could be said about forgotpw_process_pwset actually. Its not a perfect task unfortunately, I think it would be worth looking at how and what you've put into functions there if you have time you can afford to put that way. At the end of the day we're very close to freeze (next monday) and as this is an improvement it must be there by then. If you don't have time to rework this I'd suggest still putting this up and the integrators can discuss and decide wether having this as it is currently structured is worth it, and if not provide better instruction thanm I have here. 3b) I do think Petr has a very good point about the naming of the functions. At present they're not always clear or consistent and in some cases not unique enough. Perhaps prefixing them with the 'core_login_' or similiar will help. In some circumstances a complete rename may be best. The following are quick suggestions, I've not given this a lot of time but perhaps will provide some insight into my thoughts on this issue: forgotpw_process_request => core_login_request_password_reset forgotpw_process_pwset => core_login_process_password_reset create_reset_record => core_login_generate_password_reset get_postlogin_redirection => core_login_get_return_url 4) White space requirements are detailed in http://docs.moodle.org/dev/Coding_style . Really worth mentioning are the useful tools under http://docs.moodle.org/dev/Coding_style#Useful_tools If you run those two tools they will report the issues that exist. They're easy to run and are the perfect final step before pushing code to review. Also if you are using an IDE such as PHPStorm or Netbeans you can integrate these tools with it to provide automatic highlighting of issues directly within your IDE. The readme files for the plugins contains details of how to do this I believe). Do keep in mind that we are pretty strict about our whitespace, it can take a while to get used to but stick with it. (noting that Marina's moodlecheck is available through https://moodle.org/plugins/view.php?plugin=local_moodlecheck as well, I'll find someone to update that link) 5) As an example of markdown use within lang strings check out https://github.com/samhemelryk/moodle/blob/master/lang/en/badges.php#L304 (there are other strings with HTML in use there). I'm not aware of it being a rule that markdown has to be used (we still have a lot of HTML in lang strings) however certainly with new strings we'd love to see markdown used. Perhaps Petr could clarify this but to the best of my knowledge if you use markdown you must also make sure you format the string before displaying it. 7) I get what you mean by "re" however perhaps its not clear to those for whom English is not their first language. We generally try to be clear and definitive in our language (and naming as above) and I'd suggest using the full term rather than the re abbreviation. Not a biggy though, I'm sure if Petr were to re-read it he'd understand. 8) Ah PARAM_RAW lets too much through in this case for sure, no doubt there is a more appropriate param type you could use there. 9) If its going to be documented anywhere it would be in http://docs.moodle.org/dev/Coding_style however I don't believe there is documenation about perferred operators. We discussed the use of and/or the other day and came to the conclussion that they are permitable to use as long as they are not mixed with &&/||. Really I would assume the same here, you are free to use <> as long as you don't mix it with other operators. The general preference is for && || !== ===. Worth noting about <> is that it is not type strict. In situations where you are comparing two strings we would definitely prefer to see !== used as that is of course type strict. That is a rule and the logic behind it being that we want our checks to be as tight as possible. 13) I can't tell you why Petr put that however most likely it is because he's either not looked at all of the code, or has not gone though it to the best of his capability (as in he's just had a quick look). I'd strongly suggest putting this up for peer-review again when you are ready once more. Many thanks Sam
            Hide
            Peter Bulmer added a comment -

            As discussed with Sam, pretty hard up against the freeze now, and really need to get this in asap.

            I have fixed the reported issues:
            1. re php docs invalid: ran the php doc checker & fixed warnings/errors
            2. re sql formatting: removed "INNER", adjusted spacing, changed quoting.
            3. re function names changed as discussed with Sam
            4. re whitespace: ran code checker & fixed reported errors & warnings.
            5. re markdown, as discussed with Sam, this isn't an essential, and hasn't been done because of other potential complications.
            6. re missing foreign key, added foreign key.
            7. re "re" perceived typo. Expanded "re" to regarding in comment.
            8. re param type - changed param_raw to param_alphanum
            9. re <> vs !==. changed to use !==
            10 re autocomplete - explained why this is necessary
            11 re unnecessary hidden param - removed this param from form
            12 re unnecessary verification of function return - verification removed

            Thanks,
            Peter

            Show
            Peter Bulmer added a comment - As discussed with Sam, pretty hard up against the freeze now, and really need to get this in asap. I have fixed the reported issues: 1. re php docs invalid: ran the php doc checker & fixed warnings/errors 2. re sql formatting: removed "INNER", adjusted spacing, changed quoting. 3. re function names changed as discussed with Sam 4. re whitespace: ran code checker & fixed reported errors & warnings. 5. re markdown, as discussed with Sam, this isn't an essential, and hasn't been done because of other potential complications. 6. re missing foreign key, added foreign key. 7. re "re" perceived typo. Expanded "re" to regarding in comment. 8. re param type - changed param_raw to param_alphanum 9. re <> vs !==. changed to use !== 10 re autocomplete - explained why this is necessary 11 re unnecessary hidden param - removed this param from form 12 re unnecessary verification of function return - verification removed Thanks, Peter
            Hide
            Peter Bulmer added a comment -

            As discussed with Sam a week or so ago, I'm going to submit this for integration, as have run out of time for 3rd peer review before tomorrow's freeze - it was always going to be tight.

            Thanks,
            Peter

            Show
            Peter Bulmer added a comment - As discussed with Sam a week or so ago, I'm going to submit this for integration, as have run out of time for 3rd peer review before tomorrow's freeze - it was always going to be tight. Thanks, Peter
            Hide
            Damyon Wiese added a comment -

            Thanks for working on this Peter.

            I think there are still some small things here that still need fixing:

            1. Random captialisation in lang string files: "A Password reset email has", "Set Password" (twice)

            2. I also don't agree with the autocomplete + hidden username in the form - just let the browser handle it. If they want to save their password in their browser it should be from the main login page, not the reset password page.

            3. You still have PARAM_RAW in "$token = optional_param('token', false, PARAM_RAW);"

            Can you please fix these issues? I'll leave this issue in review for a day to see if it gets done in time for this week.

            Thanks!

            Show
            Damyon Wiese added a comment - Thanks for working on this Peter. I think there are still some small things here that still need fixing: 1. Random captialisation in lang string files: "A Password reset email has", "Set Password" (twice) 2. I also don't agree with the autocomplete + hidden username in the form - just let the browser handle it. If they want to save their password in their browser it should be from the main login page, not the reset password page. 3. You still have PARAM_RAW in "$token = optional_param('token', false, PARAM_RAW);" Can you please fix these issues? I'll leave this issue in review for a day to see if it gets done in time for this week. Thanks!
            Hide
            Peter Bulmer added a comment -

            Hi Damyon,
            Thanks for the review & keeping this open.
            1. Random capitalisation, I'll fix those 3 things asap.
            3. Param raw - thought I'd fixed that, willdo.

            Re item 2, 'let the browser handle it' - from my point of view that's what this patch does; I can change it (and will if you say so), but I think that this is a nice feature I'd like to retain, so can I check that we're on the same page first: Not doing anything special with a normal web form allows autocompletion, "Autocomplete=off" (moodle forms default) is the website telling webbrowsers that they are not to do autocompletion (almost univerially respected by browsers).
            In my patch, by overriding moodleforms default we're re-allowing autocompletion (like the login page), and letting the the browser (in conjunction with the user) choose whether to store the password or not.
            If we're on the same page there, cool, I'll remove the autocomplete from this patch, the downside to this is that once the user has reset their password, they need to remember it once (which could be a days away), before their browser will offer to remember it for them.

            Thanks again,
            Peter

            Show
            Peter Bulmer added a comment - Hi Damyon, Thanks for the review & keeping this open. 1. Random capitalisation, I'll fix those 3 things asap. 3. Param raw - thought I'd fixed that, willdo. Re item 2, 'let the browser handle it' - from my point of view that's what this patch does; I can change it (and will if you say so), but I think that this is a nice feature I'd like to retain, so can I check that we're on the same page first: Not doing anything special with a normal web form allows autocompletion, "Autocomplete=off" (moodle forms default) is the website telling webbrowsers that they are not to do autocompletion (almost univerially respected by browsers). In my patch, by overriding moodleforms default we're re-allowing autocompletion (like the login page), and letting the the browser (in conjunction with the user) choose whether to store the password or not. If we're on the same page there, cool, I'll remove the autocomplete from this patch, the downside to this is that once the user has reset their password, they need to remember it once (which could be a days away), before their browser will offer to remember it for them. Thanks again, Peter
            Hide
            Peter Bulmer added a comment -

            Just pushed an updated commit which addresses items 1&3 and updated the pull request branch details. If you still need me to disable auto completion, I'll do that too (under protest, naturally )

            Thanks Peter.

            Show
            Peter Bulmer added a comment - Just pushed an updated commit which addresses items 1&3 and updated the pull request branch details. If you still need me to disable auto completion, I'll do that too (under protest, naturally ) Thanks Peter.
            Hide
            Damyon Wiese added a comment -

            Thanks Peter,

            Looking at the autocomplete more closely - the behaviour of the login form is to rely on "$CFG->loginpasswordautocomplete" to determine whether to use autocomplete - if you can update this patch to do the same it will be fine.

            Show
            Damyon Wiese added a comment - Thanks Peter, Looking at the autocomplete more closely - the behaviour of the login form is to rely on "$CFG->loginpasswordautocomplete" to determine whether to use autocomplete - if you can update this patch to do the same it will be fine.
            Hide
            Peter Bulmer added a comment -

            Hi Damyon,
            Thanks for looking into that.
            Patchset updated & pushed, ticket updated with the new branch to pull.

            Much appreciated,
            Peter.

            Show
            Peter Bulmer added a comment - Hi Damyon, Thanks for looking into that. Patchset updated & pushed, ticket updated with the new branch to pull. Much appreciated, Peter.
            Hide
            Damyon Wiese added a comment -

            Thanks for working on this Peter, the changes have been integrated to master now.

            Show
            Damyon Wiese added a comment - Thanks for working on this Peter, the changes have been integrated to master now.
            Hide
            Peter Bulmer added a comment -

            Thanks Damyon,

            Show
            Peter Bulmer added a comment - Thanks Damyon,
            Hide
            Adrian Greeve added a comment -

            All works as described.

            I tried out the different permutations and didn't have any trouble there either.

            Test passed.

            Show
            Adrian Greeve added a comment - All works as described. I tried out the different permutations and didn't have any trouble there either. Test passed.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels.

            Or, if you prefer, yes, you fixed that boring issue.

            Thanks anyway! Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels. Or, if you prefer, yes, you fixed that boring issue. Thanks anyway! Ciao
            Hide
            Helen Foster added a comment -

            Removing qa_test_required label as we now have MDLQA-5729 for testing this nice improvement. (I'm especially looking forward to having it on moodle.org, as the old reset password procedure caused tons of problems!)

            Please shout if the QA test needs amending or adding to at all.

            Show
            Helen Foster added a comment - Removing qa_test_required label as we now have MDLQA-5729 for testing this nice improvement. (I'm especially looking forward to having it on moodle.org, as the old reset password procedure caused tons of problems!) Please shout if the QA test needs amending or adding to at all.
            Hide
            Peter Bulmer added a comment -

            That looks awesome (very succinct), thanks Helen.

            Peter

            Show
            Peter Bulmer added a comment - That looks awesome (very succinct), thanks Helen. Peter
            Hide
            Bernd Albers added a comment -

            I have checked this feature in my moodle 2.6 test installation and this is really a great improvement. Thanks for this!!

            But I also found, that moodle does not verify whether the username or email address entered is in the database.
            Many of our students change their mail accounts regularly and don’t remember which username or email-address is used in moodle.
            So I think moodle should give an error message if the username or mail address is not in the database.

            I would even prefer to give my students only the option to enter the mail address and not the username.
            I can achieve this by removing these lines:
            $mform->addElement('header', 'searchbyusername', get_string('searchbyusername'), '');
            $mform->addElement('text', 'username', get_string('username'));
            $mform->setType('username', PARAM_RAW);
            from the “forgot_password_form.php”
            But maybe this could be made into an option somewhere in the moodle admistration?

            Show
            Bernd Albers added a comment - I have checked this feature in my moodle 2.6 test installation and this is really a great improvement. Thanks for this!! But I also found, that moodle does not verify whether the username or email address entered is in the database. Many of our students change their mail accounts regularly and don’t remember which username or email-address is used in moodle. So I think moodle should give an error message if the username or mail address is not in the database. I would even prefer to give my students only the option to enter the mail address and not the username. I can achieve this by removing these lines: $mform->addElement('header', 'searchbyusername', get_string('searchbyusername'), ''); $mform->addElement('text', 'username', get_string('username')); $mform->setType('username', PARAM_RAW); from the “forgot_password_form.php” But maybe this could be made into an option somewhere in the moodle admistration?
            Hide
            Peter Bulmer added a comment -

            Hi Bernd,
            You can enable feedback to the users by changing the setting "protectusernames" to off.

            With this setting off, it will display "No such account found" (or some variation thereof) or "Email has been sent", rather than the NSA/CIA style: "We see what you've entered, and we might have sent you something, we might not have. Can't Say."

            Re simplifying what users see: Couldn't agree more. I'd like to implement (& encourage upstream to accept) this as an admin option "Users reset by (Username|emailaddress|either)" (dropdown box). But there's a lot of things I want to do, and very little time. You can log a bug request, if/when someone implements this, it's helpful to be able to point to other people that see the same problem as you.

            Thanks for the feedback about the fix.

            Thanks
            Peter.

            Show
            Peter Bulmer added a comment - Hi Bernd, You can enable feedback to the users by changing the setting "protectusernames" to off. With this setting off, it will display "No such account found" (or some variation thereof) or "Email has been sent", rather than the NSA/CIA style: "We see what you've entered, and we might have sent you something, we might not have. Can't Say." Re simplifying what users see: Couldn't agree more. I'd like to implement (& encourage upstream to accept) this as an admin option "Users reset by (Username|emailaddress|either)" (dropdown box). But there's a lot of things I want to do, and very little time. You can log a bug request, if/when someone implements this, it's helpful to be able to point to other people that see the same problem as you. Thanks for the feedback about the fix. Thanks Peter.
            Hide
            Helen Foster added a comment -

            This issue is mentioned in http://docs.moodle.org/dev/Moodle_2.6_release_notes however I can't think of anywhere in the user docs where it should be noted; thus I am removing the docs_required label. If anyone thinks otherwise, please comment and re-add the label.

            Show
            Helen Foster added a comment - This issue is mentioned in http://docs.moodle.org/dev/Moodle_2.6_release_notes however I can't think of anywhere in the user docs where it should be noted; thus I am removing the docs_required label. If anyone thinks otherwise, please comment and re-add the label.
            Hide
            Bernd Albers added a comment -

            Thanks Peter

            "You can enable feedback to the users by changing the setting "protectusernames" to off."
            This is very helpful information.

            Entering "protectusernames" in the moodle.docs search I found an explanation to this setting here: http://docs.moodle.org/24/en/Site_policies

            I think the consequences of this setting should be explained a bit more clearly also in the moodle admin settings.

            There is even a tracker entry: https://tracker.moodle.org/browse/MDL-40158

            Show
            Bernd Albers added a comment - Thanks Peter "You can enable feedback to the users by changing the setting "protectusernames" to off." This is very helpful information. Entering "protectusernames" in the moodle.docs search I found an explanation to this setting here: http://docs.moodle.org/24/en/Site_policies I think the consequences of this setting should be explained a bit more clearly also in the moodle admin settings. There is even a tracker entry: https://tracker.moodle.org/browse/MDL-40158
            Hide
            Helen Foster added a comment -

            Bernd, thanks for your comment. I have added further explanation to http://docs.moodle.org/en/Site_policies to hopefully make it clearer. Feel free to log in to Moodle Docs and improve the wording further!

            Show
            Helen Foster added a comment - Bernd, thanks for your comment. I have added further explanation to http://docs.moodle.org/en/Site_policies to hopefully make it clearer. Feel free to log in to Moodle Docs and improve the wording further!
            Hide
            Bernd Albers added a comment - - edited

            Thanks Helen

            I think the new explanation makes things clear now.

            I did not know that I could edit that entry.

            I'll contribute that to the German Moodle Docs then.

            Show
            Bernd Albers added a comment - - edited Thanks Helen I think the new explanation makes things clear now. I did not know that I could edit that entry. I'll contribute that to the German Moodle Docs then.
            Hide
            Helen Foster added a comment -

            Thanks Bernd, I see you have added the explanation to the German Moodle Docs.

            Show
            Helen Foster added a comment - Thanks Bernd, I see you have added the explanation to the German Moodle Docs.
            Hide
            Martin Dougiamas added a comment -

            Bernd, as a note to your comment above "I think moodle should give an error message if the username or mail address is not in the database." ... this would be a security problem because anyone can hit the page with a script and a lot of email addresses or usernames and thus find out which ones are in the Moodle database or not. This makes it much easier to crack an account as a second step.

            Show
            Martin Dougiamas added a comment - Bernd, as a note to your comment above "I think moodle should give an error message if the username or mail address is not in the database." ... this would be a security problem because anyone can hit the page with a script and a lot of email addresses or usernames and thus find out which ones are in the Moodle database or not. This makes it much easier to crack an account as a second step.
            Hide
            Peter Bulmer added a comment -

            Hi Martin,
            I'd petition a change from "this would be a security problem" to "this is be a security problem in certain circumstances".

            Since in many organisations usernames are schema driven, and publicly known, eg student id numbers.

            If an attacker was guessing usernames or passwords, there's a good chance they'd be able to tell when they hit one based on the response time (code path for successful query is very different to unsuccessful query).

            So this creates a security problem where you allow weak passwords, but are saved by having obsecure usernames, and the attacker can't tell based on response time when they've hit an account.

            So for some (many?) users, it makes sense - they get improved usability, and they aren't worried about revealing usernames (they may be obvious anyway), and are happy with their password strength relative to attacker's desire to compromise the system.

            Show
            Peter Bulmer added a comment - Hi Martin, I'd petition a change from "this would be a security problem" to "this is be a security problem in certain circumstances". Since in many organisations usernames are schema driven, and publicly known, eg student id numbers. If an attacker was guessing usernames or passwords, there's a good chance they'd be able to tell when they hit one based on the response time (code path for successful query is very different to unsuccessful query). So this creates a security problem where you allow weak passwords, but are saved by having obsecure usernames, and the attacker can't tell based on response time when they've hit an account. So for some (many?) users, it makes sense - they get improved usability, and they aren't worried about revealing usernames (they may be obvious anyway), and are happy with their password strength relative to attacker's desire to compromise the system.
            Hide
            Petr Skoda added a comment -

            Peter: I disagree, it is a security issue in most cases, let's keep it described that way. It is admins responsibility to decide if they give the hints or not.

            Show
            Petr Skoda added a comment - Peter: I disagree, it is a security issue in most cases, let's keep it described that way. It is admins responsibility to decide if they give the hints or not.
            Hide
            Helen Foster added a comment -

            It seems that the old password reset language strings ([emailpasswordsent,core] and [emailresetconfirmsent,core]?) are still in the en language pack for 2.6. Can they be removed?

            Show
            Helen Foster added a comment - It seems that the old password reset language strings ( [emailpasswordsent,core] and [emailresetconfirmsent,core] ?) are still in the en language pack for 2.6. Can they be removed?
            Hide
            Peter Bulmer added a comment -

            If it looks disused, then no objection from me.

            Do we re-open this bug & delete the strings & file a pull request?

            Show
            Peter Bulmer added a comment - If it looks disused, then no objection from me. Do we re-open this bug & delete the strings & file a pull request?
            Hide
            Helen Foster added a comment -

            Peter, as this issue is closed, a new issue should be created for deleting the strings from the code in the master branch (as advised by David Mudrak - thanks!)

            Show
            Helen Foster added a comment - Peter, as this issue is closed, a new issue should be created for deleting the strings from the code in the master branch (as advised by David Mudrak - thanks!)

              People

              • Votes:
                11 Vote for this issue
                Watchers:
                23 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: