Moodle
  1. Moodle
  2. MDL-21342

Account lockout after failed login attempts

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      1/ run phpunit tests
      2/ set some low threshold and timeouts and try lockouts

      • verify user gets email with unlock instructions
      • verify account is unlocked automatically after selected time without failed logins
      • verify that more attempts in some longer window do not trigger lockout
      • verify admin may unlock accounts manually from the Admins / Users / Accounts / Browse list of users
      Show
      1/ run phpunit tests 2/ set some low threshold and timeouts and try lockouts verify user gets email with unlock instructions verify account is unlocked automatically after selected time without failed logins verify that more attempts in some longer window do not trigger lockout verify admin may unlock accounts manually from the Admins / Users / Accounts / Browse list of users
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull 2.4 Branch:
      w51_MDL-21342_m24_lockout
    • Pull Master Branch:
      w51_MDL-21342_m25_lockout
    • Rank:
      793

      Description

      Implement a lockout system for web services => when wrong password too many time, lock the user (except if IP restriction field is not empty)
      Maybe do an administration too to unlock and visualize locked
      nothing needed for token
      Specs needed.
      log needed when a lockout happens.

      1. documentation_MDL-21342.pdf
        202 kB
        Steve Massicotte
      2. testingInstructions.pdf
        177 kB
        Steve Massicotte
      3. webservice lockout.bmml
        3 kB
        Jérôme Mouneyrac
      1. webservice lockout.png
        43 kB
      2. wslockoutadmin.png
        52 kB

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          Added UI Mockup: <webservice lockout>

          Show
          Jérôme Mouneyrac added a comment - Added UI Mockup: <webservice lockout>
          Hide
          Jérôme Mouneyrac added a comment -

          I attached a mockup I did with the last version of Blasamiq for desktop (our Jira plugin doesnt support all features)

          Show
          Jérôme Mouneyrac added a comment - I attached a mockup I did with the last version of Blasamiq for desktop (our Jira plugin doesnt support all features)
          Hide
          Jérôme Mouneyrac added a comment -

          we could also lock an IP that try a wrong token more than 5 times.

          Show
          Jérôme Mouneyrac added a comment - we could also lock an IP that try a wrong token more than 5 times.
          Hide
          Jérôme Mouneyrac added a comment -

          for the IP probably better to have a kind of administration alert instead of locking the IP

          Show
          Jérôme Mouneyrac added a comment - for the IP probably better to have a kind of administration alert instead of locking the IP
          Hide
          Eloy Lafuente (stronk7) added a comment -

          NOTE: This issue was assigned to the STABLE backlog without complete triaging process. Marking it as triaged, but with this note for future reference.

          Show
          Eloy Lafuente (stronk7) added a comment - NOTE: This issue was assigned to the STABLE backlog without complete triaging process. Marking it as triaged, but with this note for future reference.
          Hide
          Petr Škoda added a comment -

          Raising severity, we need the same lockout for standard login too because the current which is based on sessions can not be considered a security feature.

          I am moving this to DEV backlog because it will most probably require changes in user table, abusing of user preferences does not seem to be a good idea.

          When implementing this we would probably need to tracker number of failed logins, last time when login failed and a lockout flag. The tricky part is how to prevent lockout attacks - imagine somebody intentionally failed login right before the test deadline.

          Potential solutions:
          1/ prevent lockout on some accounts - usually admin does not have lockout and instead very strong password is requested
          2/ ip whitelisting - admin may log from special ip even when locked out
          3/ blacklisting of IPs
          4/ progressive timeout duration
          5/ account lock out reset via email (similar to password reset, but easier) - this one is my favourite together with 4/

          Show
          Petr Škoda added a comment - Raising severity, we need the same lockout for standard login too because the current which is based on sessions can not be considered a security feature. I am moving this to DEV backlog because it will most probably require changes in user table, abusing of user preferences does not seem to be a good idea. When implementing this we would probably need to tracker number of failed logins, last time when login failed and a lockout flag. The tricky part is how to prevent lockout attacks - imagine somebody intentionally failed login right before the test deadline. Potential solutions: 1/ prevent lockout on some accounts - usually admin does not have lockout and instead very strong password is requested 2/ ip whitelisting - admin may log from special ip even when locked out 3/ blacklisting of IPs 4/ progressive timeout duration 5/ account lock out reset via email (similar to password reset, but easier) - this one is my favourite together with 4/
          Hide
          Petr Škoda added a comment -

          Raising priority to blocker because I believe we should not release Moodle 2.3 without this new feature which is already long overdue. Backporting to stable should not be imo a priority because there is a risk of regressions and this was a known issues since early 1.x anyway.

          Show
          Petr Škoda added a comment - Raising priority to blocker because I believe we should not release Moodle 2.3 without this new feature which is already long overdue. Backporting to stable should not be imo a priority because there is a risk of regressions and this was a known issues since early 1.x anyway.
          Hide
          moodle.com added a comment -

          Petr, can you put this on your list for 2.3? -MD

          Show
          moodle.com added a comment - Petr, can you put this on your list for 2.3? -MD
          Hide
          Petr Škoda added a comment -

          yes, I can

          Show
          Petr Škoda added a comment - yes, I can
          Hide
          Petr Škoda added a comment -

          6/ recaptcha lockout reset

          Show
          Petr Škoda added a comment - 6/ recaptcha lockout reset
          Hide
          David Kettle added a comment -

          I was looking at the attached images. They include the password attempts in clear text, I have a concern that including these could be considered a security issue in it's own right for the same reasons passwords are not shown in the edit account screen.

          I think they would be better left out. I don't see a situation where they would be needed, the user could request a password reset if they can't remember their password.

          Having said that, the lockout reset will be a useful feature I'm looking forward to it in 2.3 (fingers crossed).

          Show
          David Kettle added a comment - I was looking at the attached images. They include the password attempts in clear text, I have a concern that including these could be considered a security issue in it's own right for the same reasons passwords are not shown in the edit account screen. I think they would be better left out. I don't see a situation where they would be needed, the user could request a password reset if they can't remember their password. Having said that, the lockout reset will be a useful feature I'm looking forward to it in 2.3 (fingers crossed).
          Hide
          Petr Škoda added a comment -

          David, yes indeed - we must not display/store any passwords in plaintext - even failed attempts.

          Show
          Petr Škoda added a comment - David, yes indeed - we must not display/store any passwords in plaintext - even failed attempts.
          Hide
          Steve Massicotte added a comment -

          Hi,

          in our installation we are only using CAS. But, we are working on using the manuals users. My security officer ask me if Moodle got a mecanism to block the brute force attack on password. I check and found the 10 attempts feature. But I notice that there is not a feature to lock the user for about 10 minutes. After some research I found this task. I got some questions :

          1. What I understand is that after x failed attempts on the authentification the account will be lock for an y amount of minutes. Your improvement will also provide an interface to see which account is lock and give the possibility to unlock the account
          2. How will you make the difference between an account that is lock because of an authentification issue and an another account that was lock manually or for another reason ?
          3. Are you planning to have a cron job to unlock the account after the delay ?
          4. Are you planning to add a captcha ?
          5. Do you have an idea of when this task will be completed ?

          Also I notice a small problem with the 10 attempt feature. In the file login/index.php there is no check if the authentification as work before calling the update_login_count function (lib/moodlelib.php). In that case, on the 10 attempts if you enter a valid login and password you will get the errortoomanylogins error message and be needing to restart your browser.

          Show
          Steve Massicotte added a comment - Hi, in our installation we are only using CAS. But, we are working on using the manuals users. My security officer ask me if Moodle got a mecanism to block the brute force attack on password. I check and found the 10 attempts feature. But I notice that there is not a feature to lock the user for about 10 minutes. After some research I found this task. I got some questions : What I understand is that after x failed attempts on the authentification the account will be lock for an y amount of minutes. Your improvement will also provide an interface to see which account is lock and give the possibility to unlock the account How will you make the difference between an account that is lock because of an authentification issue and an another account that was lock manually or for another reason ? Are you planning to have a cron job to unlock the account after the delay ? Are you planning to add a captcha ? Do you have an idea of when this task will be completed ? Also I notice a small problem with the 10 attempt feature. In the file login/index.php there is no check if the authentification as work before calling the update_login_count function (lib/moodlelib.php). In that case, on the 10 attempts if you enter a valid login and password you will get the errortoomanylogins error message and be needing to restart your browser.
          Hide
          Steve Massicotte added a comment - - edited

          I have begin to work on a solution and I want to explain what I'm planning to do. Note that I did not check for a solution for the web services.

          Changes in login process

          1. Add 2 new columns to mdl_user
            1. numberfailedloginattempt
            2. suspendedtomanyattempt
          2. Add to new configuration
            1. Hom many attempt before suspend the account (maxauthfailattempt)
            2. How much time the account will be suspend (timesuspendtomanyattempt)
          3. If a user try to authenticate with a valid login but fail to do it
            1. Increment the numberfailedloginattempt
            2. If the numberfailedloginattempt is greater than maxauthfailattempt
              1. suspended = 1
              2. timemodified = time()
              3. session_kill_user
              4. events_trigger 'user_updated'
              5. suspendedtomanyattempt = 1
              6. mail to the user to inform him that is account suspended

          Changes in cron.php

          1. Search mdl_user for
            suspended =  1 
            AND suspendedtomanyattempt = 1 
            AND now > timemodified
            
          2. for each account
              1. suspended = 0
              2. timemodified = time()
              3. suspendedtomanyattempt = 0
              4. numberfailedloginattempt = 0
              5. events_trigger 'user_updated'
              6. mail to the user to inform him that is account is unsuspended

          Mail decision

          I decided to send mail because it's the cron.php that call the unsunspend_user function and in this case it will never be exactly for example 10 minutes after the account was suspend.

          I also think it's make sense to get a mail in your inbox that said

          • account suspend
          • account unsuspend

          and that why I decided to send an email for the suspend action too.

          Other type of authentification that manual

          I think that the solution will work with manual authentication but also with other type that use the moodle login interface (ldap for example). The only thing to mention is that it will only lock the Moodle account not the account on the distant system.

          Authentification like CAS who use their own interface for authentification will have to manage the failed attempt (which is the CAS for our implentation of CAS)

          Show
          Steve Massicotte added a comment - - edited I have begin to work on a solution and I want to explain what I'm planning to do. Note that I did not check for a solution for the web services. Changes in login process Add 2 new columns to mdl_user numberfailedloginattempt suspendedtomanyattempt Add to new configuration Hom many attempt before suspend the account (maxauthfailattempt) How much time the account will be suspend (timesuspendtomanyattempt) If a user try to authenticate with a valid login but fail to do it Increment the numberfailedloginattempt If the numberfailedloginattempt is greater than maxauthfailattempt suspended = 1 timemodified = time() session_kill_user events_trigger 'user_updated' suspendedtomanyattempt = 1 mail to the user to inform him that is account suspended Changes in cron.php Search mdl_user for suspended = 1 AND suspendedtomanyattempt = 1 AND now > timemodified for each account suspended = 0 timemodified = time() suspendedtomanyattempt = 0 numberfailedloginattempt = 0 events_trigger 'user_updated' mail to the user to inform him that is account is unsuspended Mail decision I decided to send mail because it's the cron.php that call the unsunspend_user function and in this case it will never be exactly for example 10 minutes after the account was suspend. I also think it's make sense to get a mail in your inbox that said account suspend account unsuspend and that why I decided to send an email for the suspend action too. Other type of authentification that manual I think that the solution will work with manual authentication but also with other type that use the moodle login interface (ldap for example). The only thing to mention is that it will only lock the Moodle account not the account on the distant system. Authentification like CAS who use their own interface for authentification will have to manage the failed attempt (which is the CAS for our implentation of CAS)
          Hide
          Petr Škoda added a comment -

          I am afraid your proposal has several potential problems. I believe we should not modify the 'user' table at all and instead track all login attempts in a separate table (including correct and incorrect attempts, IPs, browser signatures, etc.) - we must not disclose if account with given username exists or not. I think it should not depend on cron because it is not reliable enough. Killing of already active session is not acceptable in my opinion. The most difficult part seems to be the protection against DoS attacks.

          Show
          Petr Škoda added a comment - I am afraid your proposal has several potential problems. I believe we should not modify the 'user' table at all and instead track all login attempts in a separate table (including correct and incorrect attempts, IPs, browser signatures, etc.) - we must not disclose if account with given username exists or not. I think it should not depend on cron because it is not reliable enough. Killing of already active session is not acceptable in my opinion. The most difficult part seems to be the protection against DoS attacks.
          Hide
          Steve Massicotte added a comment - - edited

          One thing I forget to mention is that the failed attempt (numberfailedloginattempt) are reset on a succesfull login.

          I believe we should not modify the 'user' table at all and instead track all login attempts in a separate table (including correct and incorrect attempts, IPs, browser signatures, etc.)

          Not a problem for me to do it. The only thing I see with that is how do you remove old record in that table ? Do you use the setting for mdl_log or create a new one ?

          Also, do you keep track of the numer of failed attempt it that table also ? I think it's more simple in mdl_user but on the other hand you got the trace for everything that happened in the login process.

          It's possible to get the number of failed attempt with a query like this one :

          SELECT COUNT(*)
          FROM mdl_login_attempt
          WHERE status = 'F'
          AND userid = 4
          AND timecreated > (SELECT MAX(timecreated)
          FROM mdl_login_attempt
          WHERE status = 'S'
          AND userid = 4)
          

          To add more data in the table we could add a column suspendedtomanyattempt to know that on this failed attempt the account got lock.

          we must not disclose if account with given username exists or not.

          The only thing I can do in that case is to log in the table you talk earlier (the one with IP, browser, etc...). I cannot suspend any account because it does not exist.

          I think it should not depend on cron because it is not reliable enough.

          I think you are right. What is better is to put the call to the unsuspend function in the login process and reactivate the account base on the time of the last failed attempt (the one that suspend the account).

          If the login is valid 
             AND the account is suspend
             AND the account is suspend because of to many attempt 
             AND the time of the suspend is reach
               unsuspend the account
               try to log the user
          

          I also think we should not use the timemodified column of mdl_user to do the check because this column could be change by something else.

          Killing of already active session is not acceptable in my opinion.

          I take the code that already exist to suspend the code. To avoid duplication of code I was planning to create a function suspend_user and use this function in the new change I will made in the login process and at the place where I took the code (admin/user.php).

          Maybe the session_kill_user is not necessary in the login process. I can remove it.

          The most difficult part seems to be the protection against DoS attacks

          Maybe I'm wrong but the purpose of this task was not to prevent against brute force attack on password ?

          Show
          Steve Massicotte added a comment - - edited One thing I forget to mention is that the failed attempt (numberfailedloginattempt) are reset on a succesfull login. I believe we should not modify the 'user' table at all and instead track all login attempts in a separate table (including correct and incorrect attempts, IPs, browser signatures, etc.) Not a problem for me to do it. The only thing I see with that is how do you remove old record in that table ? Do you use the setting for mdl_log or create a new one ? Also, do you keep track of the numer of failed attempt it that table also ? I think it's more simple in mdl_user but on the other hand you got the trace for everything that happened in the login process. It's possible to get the number of failed attempt with a query like this one : SELECT COUNT(*) FROM mdl_login_attempt WHERE status = 'F' AND userid = 4 AND timecreated > (SELECT MAX(timecreated) FROM mdl_login_attempt WHERE status = 'S' AND userid = 4) To add more data in the table we could add a column suspendedtomanyattempt to know that on this failed attempt the account got lock. we must not disclose if account with given username exists or not. The only thing I can do in that case is to log in the table you talk earlier (the one with IP, browser, etc...). I cannot suspend any account because it does not exist. I think it should not depend on cron because it is not reliable enough. I think you are right. What is better is to put the call to the unsuspend function in the login process and reactivate the account base on the time of the last failed attempt (the one that suspend the account). If the login is valid AND the account is suspend AND the account is suspend because of to many attempt AND the time of the suspend is reach unsuspend the account try to log the user I also think we should not use the timemodified column of mdl_user to do the check because this column could be change by something else. Killing of already active session is not acceptable in my opinion. I take the code that already exist to suspend the code. To avoid duplication of code I was planning to create a function suspend_user and use this function in the new change I will made in the login process and at the place where I took the code (admin/user.php). Maybe the session_kill_user is not necessary in the login process. I can remove it. The most difficult part seems to be the protection against DoS attacks Maybe I'm wrong but the purpose of this task was not to prevent against brute force attack on password ?
          Hide
          Simon Coggins added a comment -

          Hi Petr,

          This has come up for one of our clients too so we'd be willing to work on implementing a solution. Would you be able to provide any more details on what you think the most appropriate approach would be?

          Would a combination of your points 4/ 5/ and 6/ be sufficient for a first version? Is it enough to just log IPs or would you expect the lockout to be IP address aware?

          I'm happy to put together a brief spec for review once we have a better understanding of what would be required.

          Show
          Simon Coggins added a comment - Hi Petr, This has come up for one of our clients too so we'd be willing to work on implementing a solution. Would you be able to provide any more details on what you think the most appropriate approach would be? Would a combination of your points 4/ 5/ and 6/ be sufficient for a first version? Is it enough to just log IPs or would you expect the lockout to be IP address aware? I'm happy to put together a brief spec for review once we have a better understanding of what would be required.
          Hide
          Steve Massicotte added a comment -

          Hi Simon,

          I'm working on the solution right now. I hope to have something by the end of the week.

          I have integrated most of the suggestion that Petr have told me :

          • Don't modify the mdl_user table
          • a table specifictly for the login attempts
          • Log IP and browser type
          • Integrated the unsuspended actions in the login process
          Show
          Steve Massicotte added a comment - Hi Simon, I'm working on the solution right now. I hope to have something by the end of the week. I have integrated most of the suggestion that Petr have told me : Don't modify the mdl_user table a table specifictly for the login attempts Log IP and browser type Integrated the unsuspended actions in the login process
          Hide
          Simon Coggins added a comment -

          Hi Steve,

          That's good to hear, look forward to seeing what you've come up with.

          Show
          Simon Coggins added a comment - Hi Steve, That's good to hear, look forward to seeing what you've come up with.
          Hide
          Steve Massicotte added a comment -

          I will add the documentation of the solution suggest by Petr when I will have finish with the code.

          Show
          Steve Massicotte added a comment - I will add the documentation of the solution suggest by Petr when I will have finish with the code.
          Hide
          Steve Massicotte added a comment -

          I finally manage finish something.

          I need feedback on the solution.

          To help people to understand I put the file documentation_MDL-21342.pdf.

          Also, because there is a lot of stuff to test I create a file testingInstructions.pdf.

          I got some questions :

          • What is the version number I should put into the lib/db/upgrade.php ? (For the creation of the table)
          • I have deleted the function reset_login_count. Do I have to mark it as deprecated ? It's not use anymore.
          • Why there is an update_login_count in the file login/forgot_password.php
            • It's possible to suspend the account for too many attempt failed with that page (by refreshing the page)
          • I saw in the function authenticate_user a call to create_user_record. In which case does it happened ? Should it be considered as a login attempt or not ? Right now I do not consider it as a login attempt (no log).

          There is also some stuff I had not working on because I think it's not needed in the first iteration (a new task should be create) or I want the solution to be accepted before working on it.

          • Check the login attempt for the Web Services
          • Change in the interface to see the difference between the type of suspension (by admi and too many attempts)
          • The right message for an account this is suspended by an admin
          Show
          Steve Massicotte added a comment - I finally manage finish something. I need feedback on the solution. To help people to understand I put the file documentation_MDL-21342.pdf . Also, because there is a lot of stuff to test I create a file testingInstructions.pdf . I got some questions : What is the version number I should put into the lib/db/upgrade.php ? (For the creation of the table) I have deleted the function reset_login_count. Do I have to mark it as deprecated ? It's not use anymore. Why there is an update_login_count in the file login/forgot_password.php It's possible to suspend the account for too many attempt failed with that page (by refreshing the page) I saw in the function authenticate_user a call to create_user_record. In which case does it happened ? Should it be considered as a login attempt or not ? Right now I do not consider it as a login attempt (no log). There is also some stuff I had not working on because I think it's not needed in the first iteration (a new task should be create) or I want the solution to be accepted before working on it. Check the login attempt for the Web Services Change in the interface to see the difference between the type of suspension (by admi and too many attempts) The right message for an account this is suspended by an admin
          Hide
          Damyon Wiese added a comment -

          Hi Steve,

          I have had a look at your patch and it looks great. The code is well organised and easy to read.

          Some feedback below:

          First - minor issues:

          • There is trailing whitespace in admin/settings/plugins.php (line 74)
          • There are some very long lines (more than 132 characters) in admin/settings/plugins.php and admin/settings/server.php
          • if else formatting does not match coding guidelines in admin/user.php
          • The string "suspendedtoomanyattempttext" does not need the paragraph about clicking the link (there is no link).
          • Some inline comments do not start with a capital or end with a full stop in "lib/accesslib.php"
          • More trailing white space in lib/accesslib.php
          • in suspendedtoomanyattempt_and_mail() and unsuspendedtoomanyattempt_and_mail() there are 2 spaces before "= get_site()"
          • You have hand-written the code for the upgrade step (lib/db/upgrade.php) - you should get this from the xmldb editor instead (gurantees correctness).
          • You should also use the xmldb editor to generate your lib/db/install.xml file (Your version just has white space errors)
          • Your database columns should have defaults.
          • Your define for AGENT_CRON should be at the top of accesslib.php with the other defines
          • More trailing whitespace in lib/cronlib.php
          • Your class doesn't need a subpackage in lib/login_attempt.class.php
          • Trailing whitespace in lib/login_attempt.class.php
          • Remove the underscore from the filename for login_attempt.class.php
          • If the class login_attempt extends stdClass you won't need "clone_to_stdclass"
          • else if formatting does not match coding guidelines in user/editadvanced.php

          You get more information on these issues here:
          http://docs.moodle.org/dev/Coding_style

          More general issues:

          Answers to your questions:
          1. What is the version number I should put into the lib/db/upgrade.php ?
          – You can make it the same or greater than the previous weekly release. The version will get bumped for the next weekly which will trigger the update code.
          2. I have deleted the function reset_login_count. Do I have to mark it as deprecated ? It's not use anymore.
          – This should at least be noted in the lib/upgrade.txt file
          3. Why there is an update_login_count in the file login/forgot_password.php
          – It gets called when a user clicks on a link in the forgot password email. They get a session at that point so it should count as a login.
          4. I saw in the function authenticate_user a call to create_user_record. In which case does it happened?
          – Email-based self-registration will trigger this. I think this should count as a login attempt.

          Re: not implemented stuff:
          – Anything we are not implementing as part of this change needs to have a new Tracker Issue so it doesn't get forgotten.

          Note: There is alot of feedback here - that is mainly because it reduces the amount of review cycles than if we give it to you in dribs and drabs. These are mostly small issues that should be quick to fix. If you have any more questions, just add a comment.

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Steve, I have had a look at your patch and it looks great. The code is well organised and easy to read. Some feedback below: First - minor issues: There is trailing whitespace in admin/settings/plugins.php (line 74) There are some very long lines (more than 132 characters) in admin/settings/plugins.php and admin/settings/server.php if else formatting does not match coding guidelines in admin/user.php The string "suspendedtoomanyattempttext" does not need the paragraph about clicking the link (there is no link). Some inline comments do not start with a capital or end with a full stop in "lib/accesslib.php" More trailing white space in lib/accesslib.php in suspendedtoomanyattempt_and_mail() and unsuspendedtoomanyattempt_and_mail() there are 2 spaces before "= get_site()" You have hand-written the code for the upgrade step (lib/db/upgrade.php) - you should get this from the xmldb editor instead (gurantees correctness). You should also use the xmldb editor to generate your lib/db/install.xml file (Your version just has white space errors) Your database columns should have defaults. Your define for AGENT_CRON should be at the top of accesslib.php with the other defines More trailing whitespace in lib/cronlib.php Your class doesn't need a subpackage in lib/login_attempt.class.php Trailing whitespace in lib/login_attempt.class.php Remove the underscore from the filename for login_attempt.class.php If the class login_attempt extends stdClass you won't need "clone_to_stdclass" else if formatting does not match coding guidelines in user/editadvanced.php You get more information on these issues here: http://docs.moodle.org/dev/Coding_style More general issues: This change should really have unit tests: http://docs.moodle.org/dev/PHPUnit Answers to your questions: 1. What is the version number I should put into the lib/db/upgrade.php ? – You can make it the same or greater than the previous weekly release. The version will get bumped for the next weekly which will trigger the update code. 2. I have deleted the function reset_login_count. Do I have to mark it as deprecated ? It's not use anymore. – This should at least be noted in the lib/upgrade.txt file 3. Why there is an update_login_count in the file login/forgot_password.php – It gets called when a user clicks on a link in the forgot password email. They get a session at that point so it should count as a login. 4. I saw in the function authenticate_user a call to create_user_record. In which case does it happened? – Email-based self-registration will trigger this. I think this should count as a login attempt. Re: not implemented stuff: – Anything we are not implementing as part of this change needs to have a new Tracker Issue so it doesn't get forgotten. Note: There is alot of feedback here - that is mainly because it reduces the amount of review cycles than if we give it to you in dribs and drabs. These are mostly small issues that should be quick to fix. If you have any more questions, just add a comment. Thanks, Damyon
          Hide
          Petr Škoda added a comment -

          Please do not change meaning of the user->suspended flag, it was not intended for login lockout.

          Show
          Petr Škoda added a comment - Please do not change meaning of the user->suspended flag, it was not intended for login lockout.
          Hide
          Petr Škoda added a comment -

          Hello, I have decided to finish my own implementation of login lockout, see https://github.com/skodak/moodle/compare/w51_MDL-21342_m25_lockout
          If there are no objections I would like to submit it for integration into master and MOODLE_24_STABLE branches. Ciao

          Show
          Petr Škoda added a comment - Hello, I have decided to finish my own implementation of login lockout, see https://github.com/skodak/moodle/compare/w51_MDL-21342_m25_lockout If there are no objections I would like to submit it for integration into master and MOODLE_24_STABLE branches. Ciao
          Hide
          Damyon Wiese added a comment -

          Looks good Petr,

          I notice in your patch you define these values both as 3.

          
           /** Can not login because user is locked out */
          define('AUTH_LOGIN_LOCKOUT', 3);
          
          /** Can not login, most probably password did not match. */
          define('AUTH_LOGIN_FAILED', 3);
          

          The $failurereason in authenticate_user_login is not currently used by our code - but it would be good to be able to distinguish.

          Show
          Damyon Wiese added a comment - Looks good Petr, I notice in your patch you define these values both as 3. /** Can not login because user is locked out */ define('AUTH_LOGIN_LOCKOUT', 3); /** Can not login, most probably password did not match. */ define('AUTH_LOGIN_FAILED', 3); The $failurereason in authenticate_user_login is not currently used by our code - but it would be good to be able to distinguish.
          Hide
          Petr Škoda added a comment -

          grrr, right, I have fixed that constant, thanks a lot!

          Show
          Petr Škoda added a comment - grrr, right, I have fixed that constant, thanks a lot!
          Hide
          Petr Škoda added a comment -

          To integrators: I suppose this should go to 2.4.x stable branch, there should not be any backwards compatibility problems.

          My patch adds 3 new lockout settings similar to account lockout in Microsoft Windows. The authenticate_user_login() function also returns the reason why the login failed - I thought it could be useful in custom login page renderers in the future.

          Thanks everybody for feedback, participation and patches.

          Show
          Petr Škoda added a comment - To integrators: I suppose this should go to 2.4.x stable branch, there should not be any backwards compatibility problems. My patch adds 3 new lockout settings similar to account lockout in Microsoft Windows. The authenticate_user_login() function also returns the reason why the login failed - I thought it could be useful in custom login page renderers in the future. Thanks everybody for feedback, participation and patches.
          Hide
          Dan Poltawski added a comment -

          I have removed the security flags on this issue as a security improvement I think that this can be be publiclicly disclosed and does not need to apply for CVE etc.

          Show
          Dan Poltawski added a comment - I have removed the security flags on this issue as a security improvement I think that this can be be publiclicly disclosed and does not need to apply for CVE etc.
          Hide
          Sam Hemelryk added a comment -

          Thanks everyone.

          After much reading, testing, and deliberation this has been integrated.

          There are a couple of things to note:

          1. This issues reads as though it was originally created to address the lack of a lockout for web services. This still needs to be done so I will be opening an issue shortly to address that.
          2. I made a small commit during the integration of this issue to fix up a couple of very minor things/
          3. master only at the moment. I'll open a new issue to consider backporting this to 2.4. If you feel strongly about it please look to the linked issue and vote.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks everyone. After much reading, testing, and deliberation this has been integrated. There are a couple of things to note: This issues reads as though it was originally created to address the lack of a lockout for web services. This still needs to be done so I will be opening an issue shortly to address that. I made a small commit during the integration of this issue to fix up a couple of very minor things/ master only at the moment. I'll open a new issue to consider backporting this to 2.4. If you feel strongly about it please look to the linked issue and vote. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Linked the following two issues:

          1. MDL-37405 to consider backporting this to 2.4
          2. MDL-37406 to find a webservice solution.
          Show
          Sam Hemelryk added a comment - Linked the following two issues: MDL-37405 to consider backporting this to 2.4 MDL-37406 to find a webservice solution.
          Hide
          Petr Škoda added a comment -

          Thanks a lot!

          Show
          Petr Škoda added a comment - Thanks a lot!
          Hide
          Petr Škoda added a comment -

          thanks, looks ok

          Show
          Petr Škoda added a comment - thanks, looks ok
          Hide
          Steve Massicotte added a comment -

          Hi Peter,

          I think your solution is better than mine. But I find it was a waste of time that we have both worked on this issue. You should have send me some feedback or your code or both. During that time you will have work on something else and get 2 issues solved and not just 1.

          Well, never mind, that's alright.

          I made some tests and found some things :

          • No error message for the user
            • As a user I always got the message Invalid login, please try again. I need to check my email to see that my account is lockout. I think a message like the one I got in my solution is better :

              Sorry, your account is lockout because you have exceeded the allowed number of login attempts. You will be unlock in {$a->minutes} minutes.

              You will also received an email with a link to unlock your account by yourself.

            • In a security point of view I don't know what are the best practice but I don't think it's reveale to much information.
          • It's not possible to unlock the account in the /user/editadvanced.php page
            • I found strange to no be able to do that in that interface. It's possible to unsuspended.
          • There is no log of the lockout in the mdl_log table, only login error. I think it's something you might want to log. But there is a log in the error.log file.

          Bye

          Steve

          Show
          Steve Massicotte added a comment - Hi Peter, I think your solution is better than mine. But I find it was a waste of time that we have both worked on this issue. You should have send me some feedback or your code or both. During that time you will have work on something else and get 2 issues solved and not just 1. Well, never mind, that's alright. I made some tests and found some things : No error message for the user As a user I always got the message Invalid login, please try again . I need to check my email to see that my account is lockout. I think a message like the one I got in my solution is better : Sorry, your account is lockout because you have exceeded the allowed number of login attempts. You will be unlock in {$a->minutes} minutes. You will also received an email with a link to unlock your account by yourself. In a security point of view I don't know what are the best practice but I don't think it's reveale to much information. It's not possible to unlock the account in the /user/editadvanced.php page I found strange to no be able to do that in that interface. It's possible to unsuspended. There is no log of the lockout in the mdl_log table, only login error. I think it's something you might want to log. But there is a log in the error.log file. Bye Steve
          Hide
          Petr Škoda added a comment -

          1/ I do not think anybody wasted time, for me sending feedback means I would have to completely understand the problem and solution. It is problematic for me to invoice Moodle HQ if I did not solve the problem completely, sorry.
          2/ The current policy is to not disclose existence of usernames, that is why there is no feedback on the login page. I intentionally added the new parameter in case the policy changes in the future.
          3/ I agree it would be nice to have the lockout info in the advanced edit form. I did not know if my changes get accepted, that is why I could not spend more time on this.
          4/ The current state of moodle logging is quite bad in general, hopefully somebody will step up and redesign it soon enough for the 2.5 release.

          Thanks a lot for the feedback and feel free to create new issues for the missing things you discovered, ciao.

          Show
          Petr Škoda added a comment - 1/ I do not think anybody wasted time, for me sending feedback means I would have to completely understand the problem and solution. It is problematic for me to invoice Moodle HQ if I did not solve the problem completely, sorry. 2/ The current policy is to not disclose existence of usernames, that is why there is no feedback on the login page. I intentionally added the new parameter in case the policy changes in the future. 3/ I agree it would be nice to have the lockout info in the advanced edit form. I did not know if my changes get accepted, that is why I could not spend more time on this. 4/ The current state of moodle logging is quite bad in general, hopefully somebody will step up and redesign it soon enough for the 2.5 release. Thanks a lot for the feedback and feel free to create new issues for the missing things you discovered, ciao.
          Hide
          Damyon Wiese added a comment -

          Also - thanks for re-reviewing Petrs patch Steve - you obviously know this issue very well and the extra time you have spent re-reviewing this is very helpful.

          Show
          Damyon Wiese added a comment - Also - thanks for re-reviewing Petrs patch Steve - you obviously know this issue very well and the extra time you have spent re-reviewing this is very helpful.
          Hide
          Jason Fowler added a comment -

          Works perfectly

          Show
          Jason Fowler added a comment - Works perfectly
          Hide
          Steve Massicotte added a comment -

          Thanks Damyon. No problem.

          Show
          Steve Massicotte added a comment - Thanks Damyon. No problem.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And your fantastic code has met core, hope they become good friends for a long period.

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!
          Hide
          Andrew Nicols added a comment -

          Just noticed update_login_count and reset_login_count in lib/deprecatedlib.php. The TODO for it suggests that it will be removed in 2.6 but it was only introduced in 2.5 so shouldn't be removed until 2.7+ (http://docs.moodle.org/dev/Deprecation).
          Additionally, no issue was created for the deprecation itself.

          Show
          Andrew Nicols added a comment - Just noticed update_login_count and reset_login_count in lib/deprecatedlib.php. The TODO for it suggests that it will be removed in 2.6 but it was only introduced in 2.5 so shouldn't be removed until 2.7+ ( http://docs.moodle.org/dev/Deprecation ). Additionally, no issue was created for the deprecation itself.
          Hide
          Petr Škoda added a comment -

          Hello Andrew, the rules are intended for public API, these functions were internal, no plugin should be using them. I do not think we need to file issues in advance, the fact that something was moved to deprecatedlib.php itself is the best reminder that it needs to be removed completely in some future version.

          Show
          Petr Škoda added a comment - Hello Andrew, the rules are intended for public API, these functions were internal, no plugin should be using them. I do not think we need to file issues in advance, the fact that something was moved to deprecatedlib.php itself is the best reminder that it needs to be removed completely in some future version.
          Hide
          Helen Foster added a comment -

          Removing docs_required label as we now have documentation for this new feature: http://docs.moodle.org/25/en/Site_policies#Account_lockout

          I also deleted http://docs.moodle.org/24/en/error/moodle/errortoomanylogins in the 25 wiki as I assumed it no longer applied.

          Anyone please shout or edit the docs if anything else needs adding.

          Show
          Helen Foster added a comment - Removing docs_required label as we now have documentation for this new feature: http://docs.moodle.org/25/en/Site_policies#Account_lockout I also deleted http://docs.moodle.org/24/en/error/moodle/errortoomanylogins in the 25 wiki as I assumed it no longer applied. Anyone please shout or edit the docs if anything else needs adding.
          Hide
          Helge Wiethoff added a comment -

          Hi Petr,
          > 2/ The current policy is to not disclose existence of usernames, that is why there is no feedback on the login page. I intentionally added the new parameter in case the policy changes in the future.

          i guess we have a slightly different security policy for our own moodle-instance than moodle.org We think, that a feedback on the loginpage is more useful for the user, than that the security suffer from this.
          You said, that you added this parameter already. Is there a chance, that you could tell me a bit more about this? i have no idea how to implement this, because i dont know how to get access to the user-object and therefore get the count of failed logins from get_user_preferences.

          Show
          Helge Wiethoff added a comment - Hi Petr, > 2/ The current policy is to not disclose existence of usernames, that is why there is no feedback on the login page. I intentionally added the new parameter in case the policy changes in the future. i guess we have a slightly different security policy for our own moodle-instance than moodle.org We think, that a feedback on the loginpage is more useful for the user, than that the security suffer from this. You said, that you added this parameter already. Is there a chance, that you could tell me a bit more about this? i have no idea how to implement this, because i dont know how to get access to the user-object and therefore get the count of failed logins from get_user_preferences.

            People

            • Votes:
              10 Vote for this issue
              Watchers:
              21 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: