Moodle
  1. Moodle
  2. MDL-28585

LDAP Auth does'nt handle password expiration [W/Fix]

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1, 2.3.3, 2.4.1
    • Fix Version/s: 2.5
    • Component/s: Authentication
    • Labels:
    • Environment:
      Any
    • Database:
      Any
    • Testing Instructions:
      Hide

      These testing instructions assume you have an Active Directory domain controller acting as the LDAP server for Moodle. It only works with Active Directory.

      1. Login as admin.
      2. Configure LDAP auth plugin to use 'MS Active Directory' as the 'User type' setting. Also configure 'Expiration' setting to use 'LDAP' value. Finally either specify 'Yes' for the 'Use standard page for changing password' setting or provide a URL for the 'Password-change URL' setting.
      3. Log out.
      4. Before applying the fix, try to log in with one of the existing users in Active Directory that can access Moodle. It should log in without problem.
      5. Log out.
      6. In Active Directory edit that user and configure the account so she has to change the password on first login.
      7. Try to log in with that user in Moodle. The login should be refused, saying that either the user or the password is incorrect.
      8. Now apply the fix.
      9. Try to login with that user again. Now Moodle should tell the user that the password has expired and whether she wants to change it now.
      10. Don't change the password and log out.
      11. Log in as admin and reconfigure the LDAP plugin to either not use 'MS Active Directory' as the user type, or set 'Expiration' to 'no', or set 'Use standard page for changing password' to 'No', or clear the 'Password-change URL'.
      12. Log out.
      13. Try to log in with the same regular user as before. The login should be refused again, saying that either the user or the password is incorrect.
      Show
      These testing instructions assume you have an Active Directory domain controller acting as the LDAP server for Moodle. It only works with Active Directory. Login as admin. Configure LDAP auth plugin to use 'MS Active Directory' as the 'User type' setting. Also configure 'Expiration' setting to use 'LDAP' value. Finally either specify 'Yes' for the 'Use standard page for changing password' setting or provide a URL for the 'Password-change URL' setting. Log out. Before applying the fix, try to log in with one of the existing users in Active Directory that can access Moodle. It should log in without problem. Log out. In Active Directory edit that user and configure the account so she has to change the password on first login. Try to log in with that user in Moodle. The login should be refused, saying that either the user or the password is incorrect. Now apply the fix. Try to login with that user again. Now Moodle should tell the user that the password has expired and whether she wants to change it now. Don't change the password and log out. Log in as admin and reconfigure the LDAP plugin to either not use 'MS Active Directory' as the user type, or set 'Expiration' to 'no', or set 'Use standard page for changing password' to 'No', or clear the 'Password-change URL'. Log out. Try to log in with the same regular user as before. The login should be refused again, saying that either the user or the password is incorrect.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip_master_mdl-28585_ldap_auth_doesnt_handle_password_expiration
    • Rank:
      18267

      Description

      The LDAP Authorisation plugin does a great job of allowing users to log in with their domain credentials and even notifying users of how long they have until their password expires. When the account has actually expired however ldap_bind returns FALSE to say that the credentials are invalid (auth\ldap\auth.php around line 163) and the plugin interprets this as it would an incorrect password being entered..

      I have spotted a useful comment under the ldap_bind function on php.net: http://www.php.net/manual/en/function.ldap-bind.php#103034 . It basically suggests that there is a way to identify not just an expired password but also a first login attempt (which could be very useful for distance learning).

      There are two stages we could target for this improvement. The first step would be to enhance the current code to identify an expired password and to give a different, more user friendly error message. Building on this we could then develop a mechanism which allows users to reset their password when an expired password is detected.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for suggesting this.

          If you can propose a code solution, that will increase the chance of this improvement/feature coming about sooner.

          Show
          Michael de Raadt added a comment - Thanks for suggesting this. If you can propose a code solution, that will increase the chance of this improvement/feature coming about sooner.
          Hide
          Mark Ward added a comment -

          Hi Michael,

          I have already made the modification necessary for Stage 1 although the notification happens in the form of a JavaScript alert since we can directly alter the "invalid login" message. It's enough to demonstrate the code is functioning correctly. I will create and upload a patch ASAP.

          As far as the second step, having some mechanism for resetting the password, I tried redirecting the log-in attempt into the Forgotten Password system which resets a users password without logging them in (perfect I thought). Unfortunately I didn't realise that the forgotten password system relies on emailing the new password to the user, and on a network like ours their email account uses the same password as Moodle and the rest of the domain (ie. they can't actually retrieve it ).

          We could look at making a third password mechanism OR we could modify "change password" to force users to update their password before using the site, but since this is now well and truly outside of the realm of plug-ins I am unsure how to proceed.

          Show
          Mark Ward added a comment - Hi Michael, I have already made the modification necessary for Stage 1 although the notification happens in the form of a JavaScript alert since we can directly alter the "invalid login" message. It's enough to demonstrate the code is functioning correctly. I will create and upload a patch ASAP. As far as the second step, having some mechanism for resetting the password, I tried redirecting the log-in attempt into the Forgotten Password system which resets a users password without logging them in (perfect I thought). Unfortunately I didn't realise that the forgotten password system relies on emailing the new password to the user, and on a network like ours their email account uses the same password as Moodle and the rest of the domain (ie. they can't actually retrieve it ). We could look at making a third password mechanism OR we could modify "change password" to force users to update their password before using the site, but since this is now well and truly outside of the realm of plug-ins I am unsure how to proceed.
          Hide
          Mark Ward added a comment -

          Fancy that, you good folks already have method for forcing a password change! I've hooked up LDAP Auth to enable forcepasswordchange before returning true when it detects an expired password and I think we are pretty much there now .

          I've written this so it should work for any LDAP service but since I only have WS2003 here I cant be 100% sure. We will need someone to try out the alternatives.

          Show
          Mark Ward added a comment - Fancy that, you good folks already have method for forcing a password change! I've hooked up LDAP Auth to enable forcepasswordchange before returning true when it detects an expired password and I think we are pretty much there now . I've written this so it should work for any LDAP service but since I only have WS2003 here I cant be 100% sure. We will need someone to try out the alternatives.
          Hide
          Mark Ward added a comment -

          Sorry for the delay, summer holidays are a killer!

          This is the current modification I am working with. It kicks in when the user authentication fails and basically asks for a more detailed explanation to the failed bind. If it finds the user's password is correct but has expired it allows the user to log in but sets the "forcepasswordchange" value.

          I'm not sure whether we need to add any configuration checks to see whether this advanced step should be used - what happens on a site which doesn't support password resetting?

          We could also expand this to include some more configurable options. For instance, should someone be allowed to log in more than once after their password has expired? Should we allow brand new user accounts ("user must change password on first login) to access the site?

          Show
          Mark Ward added a comment - Sorry for the delay, summer holidays are a killer! This is the current modification I am working with. It kicks in when the user authentication fails and basically asks for a more detailed explanation to the failed bind. If it finds the user's password is correct but has expired it allows the user to log in but sets the "forcepasswordchange" value. I'm not sure whether we need to add any configuration checks to see whether this advanced step should be used - what happens on a site which doesn't support password resetting? We could also expand this to include some more configurable options. For instance, should someone be allowed to log in more than once after their password has expired? Should we allow brand new user accounts ("user must change password on first login) to access the site?
          Hide
          Michael de Raadt added a comment -

          There has been some work on a related issue. I'm not sure if it is along the same lines as your work.

          Could you please look at MDL-28402 to see if this will help resolve the same problem you have reported?

          Show
          Michael de Raadt added a comment - There has been some work on a related issue. I'm not sure if it is along the same lines as your work. Could you please look at MDL-28402 to see if this will help resolve the same problem you have reported?
          Hide
          Mark Ward added a comment -

          Hi Michael, I actually reported that one too, but that fix was just to get the LDAP configuration working for WS2003 again. This is an improvement to the LDAP Authorization plugin which will allow users to change their password once it has expired.

          Show
          Mark Ward added a comment - Hi Michael, I actually reported that one too, but that fix was just to get the LDAP configuration working for WS2003 again. This is an improvement to the LDAP Authorization plugin which will allow users to change their password once it has expired.
          Hide
          Michael de Raadt added a comment -

          Thanks for clarifying that. Sorry for the confusion.

          Hopefully we can look a this in an upcoming sprint.

          Show
          Michael de Raadt added a comment - Thanks for clarifying that. Sorry for the confusion. Hopefully we can look a this in an upcoming sprint.
          Hide
          Mark Ward added a comment -

          I've been using this on our site for a few months now and it seems to be working fine. There is one thing I overlooked however and I was hoping to get some thoughts on it.

          If a user has never logged into Moodle before but wants to reset their password through it we would want them to be able to log in and immediately redirected to the change password screen. The issue is that where I make my checks, a first-time user may not have an account and therefore I cannot apply the "must change password" setting to an account.

          I have two solutions but would like to hear thoughts on them. First is very simple, if the site configuration of "force password change" is enabled allow the new user through anyway - they will have to change their password. This works 100% of the time but it isn't ideal IMO.

          The second solution is to look at the current MAX id number within mdl_users, increment, and set the "must change password" config for that ID number. When the user account is created it will match up and they have to change their password. I have this implemented and running here but I don't know how suitable it would be for vanilla Moodle because if more than one user is signing in for the first time at the same time, checking the MAX ID number may not give us our intended result.

          I would like to update my patch to resolve this some how, but I am not sure which approach to take or if there is an alternative I haven't considered.

          Show
          Mark Ward added a comment - I've been using this on our site for a few months now and it seems to be working fine. There is one thing I overlooked however and I was hoping to get some thoughts on it. If a user has never logged into Moodle before but wants to reset their password through it we would want them to be able to log in and immediately redirected to the change password screen. The issue is that where I make my checks, a first-time user may not have an account and therefore I cannot apply the "must change password" setting to an account. I have two solutions but would like to hear thoughts on them. First is very simple, if the site configuration of "force password change" is enabled allow the new user through anyway - they will have to change their password. This works 100% of the time but it isn't ideal IMO. The second solution is to look at the current MAX id number within mdl_users, increment, and set the "must change password" config for that ID number. When the user account is created it will match up and they have to change their password. I have this implemented and running here but I don't know how suitable it would be for vanilla Moodle because if more than one user is signing in for the first time at the same time, checking the MAX ID number may not give us our intended result. I would like to update my patch to resolve this some how, but I am not sure which approach to take or if there is an alternative I haven't considered.
          Hide
          Michael de Raadt added a comment -

          Bumping this issue.

          Show
          Michael de Raadt added a comment - Bumping this issue.
          Hide
          Stuart Lang added a comment -

          Mark, love the patch!
          I've back ported this to our moodle 1.9 instance and have also written a patch for the issue where the user is not yet in moodle so that it creates their account after they change their password on first login. This is something we have been looking for for some time as we have distance learners we can now create new active directory accounts for, they can log in remotely, change their password and get straight onto moodle without having to come onsite or us compromising our password policy.

          So a big thank you from us.

          What I will do is when I migrate this to our moodle 2 instance, I will post the patch here.
          Stu

          Show
          Stuart Lang added a comment - Mark, love the patch! I've back ported this to our moodle 1.9 instance and have also written a patch for the issue where the user is not yet in moodle so that it creates their account after they change their password on first login. This is something we have been looking for for some time as we have distance learners we can now create new active directory accounts for, they can log in remotely, change their password and get straight onto moodle without having to come onsite or us compromising our password policy. So a big thank you from us. What I will do is when I migrate this to our moodle 2 instance, I will post the patch here. Stu
          Hide
          Mark Ward added a comment - - edited

          I've attached an updated version of the modification for Moodle 2.1. This version also handles new users on their first login to a site.

          We have been using this on our live server with WS2003 for over 6 months now. It should be a really simple task to integrate this into the LDAP auth plugin, but it just needs to be tested with other LDAP servers.

          Show
          Mark Ward added a comment - - edited I've attached an updated version of the modification for Moodle 2.1. This version also handles new users on their first login to a site. We have been using this on our live server with WS2003 for over 6 months now. It should be a really simple task to integrate this into the LDAP auth plugin, but it just needs to be tested with other LDAP servers.
          Hide
          Jason Fowler added a comment -

          Mark, trying to work on this now, can you please explain the difference between the first patch and the second patch?

          Show
          Jason Fowler added a comment - Mark, trying to work on this now, can you please explain the difference between the first patch and the second patch?
          Hide
          Dan Poltawski added a comment -

          I assume the second patch is the one we need to care about because its the newest so i'm reviewing that.

          Show
          Dan Poltawski added a comment - I assume the second patch is the one we need to care about because its the newest so i'm reviewing that.
          Hide
          Dan Poltawski added a comment -

          Hi Mark,

          Thanks a lot for contributing this patch, unfortunately I don't think that this can go into moodle as is and my comments are below:

          • We really need Iñaki's comments here if you have any Iñaki? He is the expert in this area.
          • Additional DB queries are being generated on every user login - I don't think this is necessary and should be avoided.
          • Not all LDAP servers will allow users to change their password so I don't think that setting the auth_forcepasswordchange flag on the user is a robust solution here.
          • Using MAX(id) and adding one from the user table is NOT safe as a method for determining the new users userid. 10 users could be created in between, or indeed the userid may not increase sequentially (I ran a server which did not)
          • Using a manual query to check the user_preferences table should be discouraged and get_user_prefences() should be used - we may change this API in future
          • Moodle coding guidelines state that underscores should not be in variable names so $extended_error is not permitted

          Again, I am interested in what Iñaki has to say on this.

          thanks!
          Dan

          Show
          Dan Poltawski added a comment - Hi Mark, Thanks a lot for contributing this patch, unfortunately I don't think that this can go into moodle as is and my comments are below: We really need Iñaki's comments here if you have any Iñaki? He is the expert in this area. Additional DB queries are being generated on every user login - I don't think this is necessary and should be avoided. Not all LDAP servers will allow users to change their password so I don't think that setting the auth_forcepasswordchange flag on the user is a robust solution here. Using MAX(id) and adding one from the user table is NOT safe as a method for determining the new users userid. 10 users could be created in between, or indeed the userid may not increase sequentially (I ran a server which did not) Using a manual query to check the user_preferences table should be discouraged and get_user_prefences() should be used - we may change this API in future Moodle coding guidelines state that underscores should not be in variable names so $extended_error is not permitted Again, I am interested in what Iñaki has to say on this. thanks! Dan
          Hide
          Michael de Raadt added a comment -

          I'm shifting this out of the stable sprint. If there are further improvements, please post a comment and we can potentially work on it again later.

          Show
          Michael de Raadt added a comment - I'm shifting this out of the stable sprint. If there are further improvements, please post a comment and we can potentially work on it again later.
          Hide
          Mark Ward added a comment - - edited
          • Mark, trying to work on this now, can you please explain the difference between the first patch and the second patch?

          The second patch is modified to handle new users who don't actually have an account on the system. It is also based on Moodle 2.1 rather than 2.0.

          • Additional DB queries are being generated on every user login - I don't think this is necessary and should be avoided.

          OK, so we have three DB queries which I will go through. The first one is to get the userID from the username. I'm doing this because we don't seem to have the User object available at this time through global $USER. Is the user ID already available in another form?

          Second is the use of DB->get_record_sql which is necessary to find the current highest user ID. This is only ran if the userID is not available (ie: only first time log in).

          The other use of DB is DB->record_exists which is simply being used to verify that the setting has been applied correctly by set_user_preference. I've checked this and it set_user_preference should return true if it is successful so we can easily bypass this.

          • Not all LDAP servers will allow users to change their password so I don't think that setting the auth_forcepasswordchange flag on the user is a robust solution here.

          Admins can disable password changing by setting "stdchangepassword" to 0 and leaving "changepasswordurl" blank. As things stand this modification will set the auth_forcepasswordchange flag anyway will will result in the user recieving a "You cannot proceed without changing your password, however there is no available page for changing it. Please contact your Moodle Administrator." error.

          This can be fixed by adding a check on $this->can_change_password() at step 1 before the auth_forcepasswordchange flag is even set. I'd also suggest ensuring that auth_forcepasswordchange flag is set to 0 just in case the setting is changed while that flag is set on a user (we don't want to strand them!).

          • Using MAX(id) and adding one from the user table is NOT safe as a method for determining the new users userid. 10 users could be created in between, or indeed the userid may not increase sequentially (I ran a server which did not)

          Agreed, this is why my first patch didn't include this but since I had no feedback to my post on 06th October 2011 and the feature had been requested I included it in patch 2. Please can you help by suggesting a more secure solution?

          • Using a manual query to check the user_preferences table should be discouraged and get_user_prefences() should be used - we may change this API in future

          As mentioned earlier, this check shouldn't be necessary if we verify that set_user_preferences has returned true.

          • Moodle coding guidelines state that underscores should not be in variable names so $extended_error is not permitted

          I can replace this variable name with $extldaperror without any trouble.

          Please indicate any problems with the changes I have suggested above before I implement them, in the meanwhile I will look again at where I can get the userid without using $DB. In terms of how we handle non-existing users I would really appreciate ideas and suggestions in order to move this forward.

          Show
          Mark Ward added a comment - - edited Mark, trying to work on this now, can you please explain the difference between the first patch and the second patch? The second patch is modified to handle new users who don't actually have an account on the system. It is also based on Moodle 2.1 rather than 2.0. Additional DB queries are being generated on every user login - I don't think this is necessary and should be avoided. OK, so we have three DB queries which I will go through. The first one is to get the userID from the username. I'm doing this because we don't seem to have the User object available at this time through global $USER. Is the user ID already available in another form? Second is the use of DB->get_record_sql which is necessary to find the current highest user ID. This is only ran if the userID is not available (ie: only first time log in). The other use of DB is DB->record_exists which is simply being used to verify that the setting has been applied correctly by set_user_preference. I've checked this and it set_user_preference should return true if it is successful so we can easily bypass this. Not all LDAP servers will allow users to change their password so I don't think that setting the auth_forcepasswordchange flag on the user is a robust solution here. Admins can disable password changing by setting "stdchangepassword" to 0 and leaving "changepasswordurl" blank. As things stand this modification will set the auth_forcepasswordchange flag anyway will will result in the user recieving a "You cannot proceed without changing your password, however there is no available page for changing it. Please contact your Moodle Administrator." error. This can be fixed by adding a check on $this->can_change_password() at step 1 before the auth_forcepasswordchange flag is even set. I'd also suggest ensuring that auth_forcepasswordchange flag is set to 0 just in case the setting is changed while that flag is set on a user (we don't want to strand them!). Using MAX(id) and adding one from the user table is NOT safe as a method for determining the new users userid. 10 users could be created in between, or indeed the userid may not increase sequentially (I ran a server which did not) Agreed, this is why my first patch didn't include this but since I had no feedback to my post on 06th October 2011 and the feature had been requested I included it in patch 2. Please can you help by suggesting a more secure solution? Using a manual query to check the user_preferences table should be discouraged and get_user_prefences() should be used - we may change this API in future As mentioned earlier, this check shouldn't be necessary if we verify that set_user_preferences has returned true. Moodle coding guidelines state that underscores should not be in variable names so $extended_error is not permitted I can replace this variable name with $extldaperror without any trouble. Please indicate any problems with the changes I have suggested above before I implement them, in the meanwhile I will look again at where I can get the userid without using $DB. In terms of how we handle non-existing users I would really appreciate ideas and suggestions in order to move this forward.
          Hide
          Dan Poltawski added a comment -

          With regard to the database queries - you need the userid because its being used to set the user preference that a password change is not required on every logon. I don't think this is the right way to go about it. (I also don't have an immediate better solution sorry - I am not trying to be unhelpful I just haven't studied the problem in detail.)

          For the normal case where a user is successfully logged in then we shouldn't be generating an extra two database queries where previously not required. The extra database queries make sense where you hit the password expired case (and when they have been unexpired) but not for every login. I know this sounds pedantic but these extra database queries here and there are what accumulate and reduce Moodle performance.

          • Using MAX(id) and adding one from the user table is NOT safe as a method for determining the new users userid. 10 users could be created in between, or indeed the userid may not increase sequentially (I ran a server which did not)

          Agreed, this is why my first patch didn't include this but since I had no feedback to my post on 06th October 2011 and the feature had been requested I included it in patch 2. Please can you help by suggesting a more secure solution?

          First of all, sorry for the delay in getting feedback to you, we aim for much better.

          Again I don't know the solution, but it 'feels' like this should be happening later in the plugin, after the user has been created. There is no good way to guess and in the future we hope to enable foreign key constraints in Moodle which would prevent this sort of thing happening before the user is created.

          Like I said i'd really like Iñaki's views here as he is much more familiar with the area than me.

          Show
          Dan Poltawski added a comment - With regard to the database queries - you need the userid because its being used to set the user preference that a password change is not required on every logon. I don't think this is the right way to go about it. (I also don't have an immediate better solution sorry - I am not trying to be unhelpful I just haven't studied the problem in detail.) For the normal case where a user is successfully logged in then we shouldn't be generating an extra two database queries where previously not required. The extra database queries make sense where you hit the password expired case (and when they have been unexpired) but not for every login. I know this sounds pedantic but these extra database queries here and there are what accumulate and reduce Moodle performance. Using MAX(id) and adding one from the user table is NOT safe as a method for determining the new users userid. 10 users could be created in between, or indeed the userid may not increase sequentially (I ran a server which did not) Agreed, this is why my first patch didn't include this but since I had no feedback to my post on 06th October 2011 and the feature had been requested I included it in patch 2. Please can you help by suggesting a more secure solution? First of all, sorry for the delay in getting feedback to you, we aim for much better. Again I don't know the solution, but it 'feels' like this should be happening later in the plugin, after the user has been created. There is no good way to guess and in the future we hope to enable foreign key constraints in Moodle which would prevent this sort of thing happening before the user is created. Like I said i'd really like Iñaki's views here as he is much more familiar with the area than me.
          Hide
          Iñaki Arenaza added a comment -

          Hi Dan and Mark,

          I'm having a look at it and will comment back in a 3 hours or so (I' m heading to a work meeting in a few minutes)

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Dan and Mark, I'm having a look at it and will comment back in a 3 hours or so (I' m heading to a work meeting in a few minutes) Saludos. Iñaki.
          Hide
          Mark Ward added a comment -

          Hi Dan,

          No worries! I'm definitely on the same wavelength as you with the DB queries happening at this stage, it's not right to be looking up a user before Moodle has created the USER object and that's where it all goes wrong.

          The problem is that user_login() can only return true or false. We definitely HAVE to check whether the password is expired at this point because the information will be lost when the ldap connection is closed, but without being able to pass on that information to a later stage of authentication when we actually have a userID we are stuck.

          Even if we can pass the information on by returning false we would instantly kick the user back to an "invalid login" screen, so we would have to return true. Having looked through authlib.php I don't see where this flag could be picked back up again because after returning true Moodle just gets on with letting the user in.

          Hopefully Iñaki will have some ideas

          Show
          Mark Ward added a comment - Hi Dan, No worries! I'm definitely on the same wavelength as you with the DB queries happening at this stage, it's not right to be looking up a user before Moodle has created the USER object and that's where it all goes wrong. The problem is that user_login() can only return true or false. We definitely HAVE to check whether the password is expired at this point because the information will be lost when the ldap connection is closed, but without being able to pass on that information to a later stage of authentication when we actually have a userID we are stuck. Even if we can pass the information on by returning false we would instantly kick the user back to an "invalid login" screen, so we would have to return true. Having looked through authlib.php I don't see where this flag could be picked back up again because after returning true Moodle just gets on with letting the user in. Hopefully Iñaki will have some ideas
          Hide
          Iñaki Arenaza added a comment -

          Hi Dan and Mark,

          a bit latar than I expected, but here it goes.

          Something worth noticing is that this feature can only work with MS Active Directory (as far as I know), as the other supported LDAP servers don't return such an information as part of the bind call (even less in that particular format; fortunately the format seems to be the same in both W2003 and W2008 at least). So we should check that we have 'ad' as the configured LDAP server type first, and skip the whole thing otherwise. We should also skip it if it looks like we haven't configured things to allow users to change their passwords.

          With that out of the way, I've been having a look at the whole authentication process, and I think we can deal with expired or change-on-first-login passwords without doing any tricks at all. We only need to fix a bug in the existing code (I discovered it as part of my tests to try to make this work).

          Things would work like this:

          1. login/index.php calls authenticate_user_login which in turn calls auth_plugin_ldap::user_login()
          2. We try to login to LDAP server with username and password and we fail.
          3. We retrieve the diagnostic message and see if it's one of expired password or change-on-first-login. It it is, the username and (existing) password are good, so we return true instead of false, and let the login proceed.
          4. Login code deals with user creation and/or user details update and the rest of the normal login code and returns to login/index.php and does some more regular stuff.
          5. login/index.php calls auth_plugin_ldap::password_expire(). If we return a negative number here, moodle will tell the user the password has expired and should be changed. The good news is (apart from the bug I mentionned above), current code alreay works as expected in this case: it returns -1 for must-change-now passwords, and a negative number of days for expired passwords.
          6. if we have setup LDAP auth settings properly, users will be able to change their password from within moodle. Or if we have specified an external URL to do it, they will be sent there.

          Trouble with this schema:

          1. If the admin doesn't enable password expiration in the LDAP auth plugin, we let the users in with expired passwords, unless we explicitly check for this in user_login() while we check the diagnostic message. Which we problably should (I didn't realize this before, so my proposed patch doesn't take this into account. I'll upload a new version in a few minutes).
          2. Currently login/index.php offers the possibility to change and expired password (see step 5 above), but doesn't force it. So a user can simply click on cancel and she remains logged in with the old password. This has been like this for years (since password expiration was first implemented, according to git blame).

          What do you think about it?

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Dan and Mark, a bit latar than I expected, but here it goes. Something worth noticing is that this feature can only work with MS Active Directory (as far as I know), as the other supported LDAP servers don't return such an information as part of the bind call (even less in that particular format; fortunately the format seems to be the same in both W2003 and W2008 at least). So we should check that we have 'ad' as the configured LDAP server type first, and skip the whole thing otherwise. We should also skip it if it looks like we haven't configured things to allow users to change their passwords. With that out of the way, I've been having a look at the whole authentication process, and I think we can deal with expired or change-on-first-login passwords without doing any tricks at all. We only need to fix a bug in the existing code (I discovered it as part of my tests to try to make this work). Things would work like this: 1. login/index.php calls authenticate_user_login which in turn calls auth_plugin_ldap::user_login() 2. We try to login to LDAP server with username and password and we fail. 3. We retrieve the diagnostic message and see if it's one of expired password or change-on-first-login. It it is, the username and (existing) password are good, so we return true instead of false, and let the login proceed. 4. Login code deals with user creation and/or user details update and the rest of the normal login code and returns to login/index.php and does some more regular stuff. 5. login/index.php calls auth_plugin_ldap::password_expire(). If we return a negative number here, moodle will tell the user the password has expired and should be changed. The good news is (apart from the bug I mentionned above), current code alreay works as expected in this case: it returns -1 for must-change-now passwords, and a negative number of days for expired passwords. 6. if we have setup LDAP auth settings properly, users will be able to change their password from within moodle. Or if we have specified an external URL to do it, they will be sent there. Trouble with this schema: 1. If the admin doesn't enable password expiration in the LDAP auth plugin, we let the users in with expired passwords, unless we explicitly check for this in user_login() while we check the diagnostic message. Which we problably should (I didn't realize this before, so my proposed patch doesn't take this into account. I'll upload a new version in a few minutes). 2. Currently login/index.php offers the possibility to change and expired password (see step 5 above), but doesn't force it. So a user can simply click on cancel and she remains logged in with the old password. This has been like this for years (since password expiration was first implemented, according to git blame). What do you think about it? Saludos. Iñaki.
          Hide
          Jon Bolton added a comment -

          @Iñaki - will your patch/solution be able to be backported to 1.9? I really need this for a project that is unlikely to be migrated to version 2 for quite some time

          Show
          Jon Bolton added a comment - @Iñaki - will your patch/solution be able to be backported to 1.9? I really need this for a project that is unlikely to be migrated to version 2 for quite some time
          Hide
          Iñaki Arenaza added a comment -

          Here's the link for the proposed (amended) patch:

          https://github.com/iarenaza/moodle/compare/master...wip_MDL-28585_ldap_auth_doesnt_handle_password_expiration_master

          The bug I mentionned before is at line 584 (old file)/603 (new file). MS-AD returns 0 (zero) as the expiration value for must-change-now passwords, and we need to take that value into account and process it (which !empty() doesn't).

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Here's the link for the proposed (amended) patch: https://github.com/iarenaza/moodle/compare/master...wip_MDL-28585_ldap_auth_doesnt_handle_password_expiration_master The bug I mentionned before is at line 584 (old file)/603 (new file). MS-AD returns 0 (zero) as the expiration value for must-change-now passwords, and we need to take that value into account and process it (which !empty() doesn't). Saludos. Iñaki.
          Hide
          Dan Poltawski added a comment -

          Your proposed solution sounds good to me.

          I guess one question in my mind - can we depend on this expired password diagnostic message as a way to let users in when authentication bind has failed?

          Show
          Dan Poltawski added a comment - Your proposed solution sounds good to me. I guess one question in my mind - can we depend on this expired password diagnostic message as a way to let users in when authentication bind has failed?
          Hide
          Mark Ward added a comment - - edited

          Hi Iñaki.

          Sounds great, I didn't think about password_expire() getting called each log in! So we can use that function to redetermine if the user's password has expired and then force them to change password.

          • 2. Currently login/index.php offers the possibility to change and expired password (see step 5 above), but doesn't force it.

          On my patch I used set_user_preference('auth_forcepasswordchange', true, $userid); which forces the user to change their password. Although they can still click "cancel" they will always be brought back to the password change screen as soon as they click on a course. Is this an option?

          EDIT: Ah, I see, we are still only getting passed the "username" value. The above suggestion relies on having the userid. Is the global $USER object set up at this stage, and would it be safe to access that?

          Many thanks,

          Mark

          Show
          Mark Ward added a comment - - edited Hi Iñaki. Sounds great, I didn't think about password_expire() getting called each log in! So we can use that function to redetermine if the user's password has expired and then force them to change password. 2. Currently login/index.php offers the possibility to change and expired password (see step 5 above), but doesn't force it. On my patch I used set_user_preference('auth_forcepasswordchange', true, $userid); which forces the user to change their password. Although they can still click "cancel" they will always be brought back to the password change screen as soon as they click on a course. Is this an option? EDIT: Ah, I see, we are still only getting passed the "username" value. The above suggestion relies on having the userid. Is the global $USER object set up at this stage, and would it be safe to access that? Many thanks, Mark
          Hide
          Mark Ward added a comment - - edited

          Another quick note, I'm just trying out your patch now and the check on "$userauth->config->expiration == 1" (line 196) is stopping the expiration check on our configuration. Debugging this shows that $userauth is an undefined variable.

          In regards to setting the auth_forcepasswordchange flag, I poked around a bit and found that the $USER object is available which means we can set the flag if the password has expired. I'm not sure what implications there would be around referring to this global from password_expire() so please feel free to shoot this down if it isn't good practice.

          The downside is that once this flag has been set on a user account Moodle just bypasses password_expire until the password has been reset and the flag is unset. In other words we can set the flag but we cannot currently unset if the password is reset outside of Moodle.

          In summary, we could force the user to change their password but it is a bit of a hack to get around Moodle. It would probably be better to start a new issue for "Moodle doesn't force user to reset password if password is expired" and work on a resolution for that.

          Show
          Mark Ward added a comment - - edited Another quick note, I'm just trying out your patch now and the check on "$userauth->config->expiration == 1" (line 196) is stopping the expiration check on our configuration. Debugging this shows that $userauth is an undefined variable. In regards to setting the auth_forcepasswordchange flag, I poked around a bit and found that the $USER object is available which means we can set the flag if the password has expired. I'm not sure what implications there would be around referring to this global from password_expire() so please feel free to shoot this down if it isn't good practice. The downside is that once this flag has been set on a user account Moodle just bypasses password_expire until the password has been reset and the flag is unset. In other words we can set the flag but we cannot currently unset if the password is reset outside of Moodle. In summary, we could force the user to change their password but it is a bit of a hack to get around Moodle. It would probably be better to start a new issue for "Moodle doesn't force user to reset password if password is expired" and work on a resolution for that.
          Hide
          Iñaki Arenaza added a comment -

          Sorry Mark, totally my fault!

          I added the check at the last minute copying the test from login/index.php and didn't test it at all (it was a bit late here).

          Simply change $userauth->config->expiration to $this->config->expiration and it should work ok.

          Sorry about that!

          (I'm a bit busy today, but will have a look at your other comments asap)

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Sorry Mark, totally my fault! I added the check at the last minute copying the test from login/index.php and didn't test it at all (it was a bit late here). Simply change $userauth->config->expiration to $this->config->expiration and it should work ok. Sorry about that! (I'm a bit busy today, but will have a look at your other comments asap) Saludos. Iñaki.
          Hide
          Mark Ward added a comment -

          Yep that worked great, many thanks for the speedy reply

          Show
          Mark Ward added a comment - Yep that worked great, many thanks for the speedy reply
          Hide
          Dan Poltawski added a comment -

          Hi Iñaki,

          Assiging this issue to you since its really your issue Please let me know if you need any help

          Show
          Dan Poltawski added a comment - Hi Iñaki, Assiging this issue to you since its really your issue Please let me know if you need any help
          Hide
          Iñaki Arenaza added a comment -

          2. Currently login/index.php offers the possibility to change and expired password (see step 5 above), but doesn't force it. So a user can simply click on cancel and she remains logged in with the old password. This has been like this for years.

          After talking to Petr Skoda in the developer chat, he said that he would personally vote for a redirect after login only (soft change password requirement), just the way it works now (all the details, if needed, can be found in the developer chat log for 2012.10.09).

          So I'll prepare patches for master, 2.3 and 2.2 and send them for peer reviwing. I'll see how hard it is to backport the patch to 1.9, but can't promise anything.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - 2. Currently login/index.php offers the possibility to change and expired password (see step 5 above), but doesn't force it. So a user can simply click on cancel and she remains logged in with the old password. This has been like this for years. After talking to Petr Skoda in the developer chat, he said that he would personally vote for a redirect after login only (soft change password requirement), just the way it works now (all the details, if needed, can be found in the developer chat log for 2012.10.09). So I'll prepare patches for master, 2.3 and 2.2 and send them for peer reviwing. I'll see how hard it is to backport the patch to 1.9, but can't promise anything. Saludos. Iñaki.
          Hide
          Mark Ward added a comment -

          Hi Iñaki

          There are many organisations with a security policy which dictates that passwords must be changed after a certain period of time. I think the best thing for me to do would be to start a new issue requesting a "site configuration option to force password change" once the work here is finished, that way it can't get confused as an LDAP issue.

          Thanks very much for your time on this, it is much appreciated!

          Mark

          Show
          Mark Ward added a comment - Hi Iñaki There are many organisations with a security policy which dictates that passwords must be changed after a certain period of time. I think the best thing for me to do would be to start a new issue requesting a "site configuration option to force password change" once the work here is finished, that way it can't get confused as an LDAP issue. Thanks very much for your time on this, it is much appreciated! Mark
          Hide
          Mark Ward added a comment -

          Hi Iñaki

          Just wondering if there was any news on this? Your patch has been ready to roll since February but there doesn't seem to be any plans for testing and integrating it.

          Show
          Mark Ward added a comment - Hi Iñaki Just wondering if there was any news on this? Your patch has been ready to roll since February but there doesn't seem to be any plans for testing and integrating it.
          Hide
          Iñaki Arenaza added a comment -

          Sorry Mark,

          totally my fault I have just pushed and updated version to github (see "Pull Master Diff URL" to get the diff), based on master current as of today. It should also cleanly apply to 2.4 current as of today.

          As soon as I write some sensible testing instructions, I'll submit it for integration.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Sorry Mark, totally my fault I have just pushed and updated version to github (see "Pull Master Diff URL" to get the diff), based on master current as of today. It should also cleanly apply to 2.4 current as of today. As soon as I write some sensible testing instructions, I'll submit it for integration. Saludos. Iñaki.
          Hide
          Dan Poltawski added a comment -

          Hi Guys, we are keeping 2.4 and master in sync for a few more weeks, so i've aded the integration_held flag and we'll look at this as soon as we're off the 'in sync' period.

          Show
          Dan Poltawski added a comment - Hi Guys, we are keeping 2.4 and master in sync for a few more weeks, so i've aded the integration_held flag and we'll look at this as soon as we're off the 'in sync' period.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, thanks guys

          Show
          Dan Poltawski added a comment - Integrated to master, thanks guys
          Hide
          Dan Poltawski added a comment -

          As we don't have the infrastructure to test this within todays timeframe and the issue only affects master we are deferring the testing to a MDLTEST-353.

          Show
          Dan Poltawski added a comment - As we don't have the infrastructure to test this within todays timeframe and the issue only affects master we are deferring the testing to a MDLTEST-353 .
          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
          Jason Fowler added a comment -

          Created an Active Directory server to test this. Works fine!

          Show
          Jason Fowler added a comment - Created an Active Directory server to test this. Works fine!
          Hide
          Helen Foster added a comment -

          Please could anyone help out by documenting this improvement here: http://docs.moodle.org/25/en/admin/auth/ldap#LDAP_password_expiration_settings .

          Show
          Helen Foster added a comment - Please could anyone help out by documenting this improvement here: http://docs.moodle.org/25/en/admin/auth/ldap#LDAP_password_expiration_settings .
          Hide
          Iñaki Arenaza added a comment -

          Hi Helen,

          I'm not sure how to document this, as it doesn't add anything that wasn't already described in the link you provide (though in a rather terse way).

          The only difference is that before this improvement you got a "invalid login" message, and now you get a "your password has expired" message (and if you are able to change your password from Moodle, you are offered to do so).

          Would that be enough?

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Helen, I'm not sure how to document this, as it doesn't add anything that wasn't already described in the link you provide (though in a rather terse way). The only difference is that before this improvement you got a "invalid login" message, and now you get a "your password has expired" message (and if you are able to change your password from Moodle, you are offered to do so). Would that be enough? Saludos. Iñaki.
          Hide
          Helen Foster added a comment -

          Thanks Iñaki, I have added a note about when the password expires to http://docs.moodle.org/25/en/LDAP_authentication#LDAP_password_expiration_settings and so am removing the docs_required label from this issue.

          Show
          Helen Foster added a comment - Thanks Iñaki, I have added a note about when the password expires to http://docs.moodle.org/25/en/LDAP_authentication#LDAP_password_expiration_settings and so am removing the docs_required label from this issue.

            People

            • Votes:
              7 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: