Moodle
  1. Moodle
  2. MDL-32584

Add option to forced password change when password_expire() is called

    Details

    • Rank:
      39503

      Description

      Moodle's authentication framework includes a function called password_expire() which is currently only used by the LDAP authentication method. When this function is called Moodle tells the the user that their password has expired and asks them if they would like to update it.

      Many organisations follow a standard practice of forcing their users to change passwords after they have expired. Recommendations for the lifetime of the password vary dependant on the security concerns of each organisation but range from anywhere from 30 to 180 days according to Microsoft guidelines. Our College was recently audited by Tenon and instructed to set password expiration to 60 days.

      As things stand the user can simply click "cancel" or navigate away from the page to avoid changing their password. I am proposing that this should remain the default set up for Moodle, but that a site-wide configuration option should be added which changes the behaviour and forces the user to change their password.

      Moodle does already have a 'forcepasswordchange' flag for the user which, when set, takes the user back to the password reset change even if they navigate away towards another page or course.

        Issue Links

          Activity

          Hide
          Mark Ward added a comment -

          Iñaki Arenaza has written a good solution to MDL-28585, however it relies on password_expire() to force a password change.

          Show
          Mark Ward added a comment - Iñaki Arenaza has written a good solution to MDL-28585 , however it relies on password_expire() to force a password change.
          Hide
          Petr Škoda added a comment -

          reassigning to our LDAP expert, thanks

          Show
          Petr Škoda added a comment - reassigning to our LDAP expert, thanks
          Hide
          Mark Ward added a comment -

          Thanks to Iñaki's work on MDL-28585 the LDAP plugin does now respond correctly to password_expire when a user's password has expired on Active Directory. The problem is now with the way that Moodle deals with that information, in that it doesn't force a password change.

          At the moment password_expire is called directly from a couple of locations including login/index.php and webservice/lib.php. These are the scripts which are dictating how Moodle should behave when it discovers an expired password and would probably be the best place to set the flag if you guys felt that was the best course of action.

          I've also been wondering whether it would be better to define this behaviour in a centralised function in order to ensure that any script which checks the user's status results in the same behaviour?

          Show
          Mark Ward added a comment - Thanks to Iñaki's work on MDL-28585 the LDAP plugin does now respond correctly to password_expire when a user's password has expired on Active Directory. The problem is now with the way that Moodle deals with that information, in that it doesn't force a password change. At the moment password_expire is called directly from a couple of locations including login/index.php and webservice/lib.php. These are the scripts which are dictating how Moodle should behave when it discovers an expired password and would probably be the best place to set the flag if you guys felt that was the best course of action. I've also been wondering whether it would be better to define this behaviour in a centralised function in order to ensure that any script which checks the user's status results in the same behaviour?
          Hide
          Iñaki Arenaza added a comment -

          Hi Petr,

          Could you have a look at this proposed feature? I've tried to keep changes to a mininum and don't need additional code around every call to password_expire(), but there could be a better way to do it.

          By the way, this is a change in behaviour of current API. I'm supposed to document this change somewhere in MoodleDocs, but after wandering a bit over the Development MoodleDocs I'm not really sure where to document it. Suggestions welcome

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Petr, Could you have a look at this proposed feature? I've tried to keep changes to a mininum and don't need additional code around every call to password_expire(), but there could be a better way to do it. By the way, this is a change in behaviour of current API. I'm supposed to document this change somewhere in MoodleDocs, but after wandering a bit over the Development MoodleDocs I'm not really sure where to document it. Suggestions welcome Saludos. Iñaki.
          Hide
          Petr Škoda added a comment -

          Hello,

          I do not understand how the patch resolves this issue. The standard way to enforce anything in moodle is to add checks to require_login(). I do not know any other way to guarantee that anything happens after login or before course access. I do not like the new setting $CFG->forcechangeexpiredpassword = true; because only the plugins know if that makes sense, I do not think we want this for all plugins. Maybe a new callback executed from login page and require_login() - $userauth->password_is_expired($calledfromrequirelogin), by default it would be doing the same things as now (login page only) and plugins could decide what to do, ldap could have a new setting to prevent access if password expired.

          Show
          Petr Škoda added a comment - Hello, I do not understand how the patch resolves this issue. The standard way to enforce anything in moodle is to add checks to require_login(). I do not know any other way to guarantee that anything happens after login or before course access. I do not like the new setting $CFG->forcechangeexpiredpassword = true; because only the plugins know if that makes sense, I do not think we want this for all plugins. Maybe a new callback executed from login page and require_login() - $userauth->password_is_expired($calledfromrequirelogin), by default it would be doing the same things as now (login page only) and plugins could decide what to do, ldap could have a new setting to prevent access if password expired.
          Hide
          Mark Ward added a comment -

          Hi Petr,

          I don't see why it would be a problem for this to be in the plugin configuration, if anything that make's things easier. What do you think Iñaki?

          As far as the functionality being within require_login(); there is already a check that occurs in require_login() at line 2910 which monitors a user preference called auth_forcepasswordchange and redirects a user to change their password when this is set to true. This is how Iñaki's solution works at the moment.

          This means we can continue to check against $auth->password_expire() just once during login, and if the plugin is configured to forceexpiredpasswordchange it can set the auth_forcepasswordchange flag for require_login() to pick up when it is called.

          This will work perfectly with $CFG->forcelogin set to true, but when $CFG->forcelogin is set to false the user will be nicely asked "Your password is expired. Do you want change your password now?". They could respond with "cancel" and then go through several pages before require_login() is triggered and they are told "You must change your password to proceed." This is perfectly functional but will seem disjointed to the user: "Why ask if I have no choice?"

          Show
          Mark Ward added a comment - Hi Petr, I don't see why it would be a problem for this to be in the plugin configuration, if anything that make's things easier. What do you think Iñaki? As far as the functionality being within require_login() ; there is already a check that occurs in require_login() at line 2910 which monitors a user preference called auth_forcepasswordchange and redirects a user to change their password when this is set to true . This is how Iñaki's solution works at the moment. This means we can continue to check against $auth->password_expire() just once during login, and if the plugin is configured to forceexpiredpasswordchange it can set the auth_forcepasswordchange flag for require_login() to pick up when it is called. This will work perfectly with $CFG->forcelogin set to true , but when $CFG->forcelogin is set to false the user will be nicely asked "Your password is expired. Do you want change your password now?" . They could respond with "cancel" and then go through several pages before require_login() is triggered and they are told "You must change your password to proceed." This is perfectly functional but will seem disjointed to the user: "Why ask if I have no choice?"
          Hide
          Petr Škoda added a comment -

          1/ I do not like this new global setting which is trying to work around ldap specific problem.
          2/ I agree the cancel option on the login page is wrong, it should be removed. I hope nobody is abusing it, but I guess this should be announced early in dev cycle.
          3/ I do not understand how is the user preference auth_forcepasswordchange going to be unset when you change password in external system, looks like a potential deadlock to me.

          Show
          Petr Škoda added a comment - 1/ I do not like this new global setting which is trying to work around ldap specific problem. 2/ I agree the cancel option on the login page is wrong, it should be removed. I hope nobody is abusing it, but I guess this should be announced early in dev cycle. 3/ I do not understand how is the user preference auth_forcepasswordchange going to be unset when you change password in external system, looks like a potential deadlock to me.
          Hide
          Mark Ward added a comment -

          1: Apologies, I wasn't sure exactly where you were coming from before. I'm working with a plugin setting on my WIP now.

          2: We could add a check in login/index.php for these whatever flag(s) which redirects the user to do a password change if flag is set to true.

          3: Ah, quite right; we can't just reset it on a successful password_expire() since it might have been set for another reason. The best solution would be to have a second flag which works exactly the same as auth_forcepasswordchange, getting checked in require_login and reset after password change but we can also reset that in password_expire() if the password is found to have not expired. Are there any hidden pitfalls with adding new user preferences?

          Thanks for your time!

          Show
          Mark Ward added a comment - 1: Apologies, I wasn't sure exactly where you were coming from before. I'm working with a plugin setting on my WIP now. 2: We could add a check in login/index.php for these whatever flag(s) which redirects the user to do a password change if flag is set to true. 3: Ah, quite right; we can't just reset it on a successful password_expire() since it might have been set for another reason. The best solution would be to have a second flag which works exactly the same as auth_forcepasswordchange , getting checked in require_login and reset after password change but we can also reset that in password_expire() if the password is found to have not expired. Are there any hidden pitfalls with adding new user preferences? Thanks for your time!
          Hide
          Mark Ward added a comment - - edited

          Based on Inaki's previous work and the discussion above I've added my own wip branch on GitHub. I'd be pleased to hear both of your thoughts on it.

          https://github.com/markward/moodle/commit/2a27dac247ade73162cc1683a714b1c81c46aad7

          Show
          Mark Ward added a comment - - edited Based on Inaki's previous work and the discussion above I've added my own wip branch on GitHub. I'd be pleased to hear both of your thoughts on it. https://github.com/markward/moodle/commit/2a27dac247ade73162cc1683a714b1c81c46aad7
          Hide
          Petr Škoda added a comment -

          I disagree with the proposed change in patch for the reasons above, we need some better way to deal with this, sorry.

          Show
          Petr Škoda added a comment - I disagree with the proposed change in patch for the reasons above, we need some better way to deal with this, sorry.
          Hide
          Mark Ward added a comment -

          Hi Petr. Are you referring to Inaki's original patch or my reworking of it?

          Show
          Mark Ward added a comment - Hi Petr. Are you referring to Inaki's original patch or my reworking of it?
          Hide
          Petr Škoda added a comment -

          Ah, I am always using only the links at the top when issues is waiting for review. I have looked at the link three posts above and logic seems much better, but the coding style in quite bad - see http://docs.moodle.org/dev/Coding_style and http://docs.moodle.org/dev/Commit_cheat_sheet

          1/ On the login page it could probably verify the change password URL and logout user if external before redirecting outside of moodle.
          2/ you can use $CFG->httpswwwroot on the login page instead of if (empty($CFG->loginhttps))
          3/ it is probably better to unset the preference to decrease session size
          4/ I did not look much - where is the auth_passwordexpired unset when external password change used?

          Show
          Petr Škoda added a comment - Ah, I am always using only the links at the top when issues is waiting for review. I have looked at the link three posts above and logic seems much better, but the coding style in quite bad - see http://docs.moodle.org/dev/Coding_style and http://docs.moodle.org/dev/Commit_cheat_sheet 1/ On the login page it could probably verify the change password URL and logout user if external before redirecting outside of moodle. 2/ you can use $CFG->httpswwwroot on the login page instead of if (empty($CFG->loginhttps)) 3/ it is probably better to unset the preference to decrease session size 4/ I did not look much - where is the auth_passwordexpired unset when external password change used?
          Hide
          Mark Ward added a comment -

          OK thanks for the feedback I'll take a look and update things ASAP. Would it be ok to update the issue's PULL information with this?

          If the user triggers auth_passwordexpired but decides to change the password externally the flag is unset during the authentication process when LDAP responds with successful authentication. This is in auth/ldap/auth.php at line 632 where, after a successful audit, we know that the password is valid and that auth_passwordexpired should be false.

          This does mean that a user must log out and back into the site for the external password update to be recognised but that is really essential since it is the only stage at which we audit a password. I did consider whether we could shorten the session period for a user with auth_passwordexpired set to true?

          Appreciate your time on this!

          Show
          Mark Ward added a comment - OK thanks for the feedback I'll take a look and update things ASAP. Would it be ok to update the issue's PULL information with this? If the user triggers auth_passwordexpired but decides to change the password externally the flag is unset during the authentication process when LDAP responds with successful authentication. This is in auth/ldap/auth.php at line 632 where, after a successful audit, we know that the password is valid and that auth_passwordexpired should be false. This does mean that a user must log out and back into the site for the external password update to be recognised but that is really essential since it is the only stage at which we audit a password. I did consider whether we could shorten the session period for a user with auth_passwordexpired set to true? Appreciate your time on this!

            People

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

              Dates

              • Created:
                Updated: