Moodle
  1. Moodle
  2. MDL-32584

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

    Details

      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.

        Gliffy Diagrams

          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 Skoda added a comment -

            reassigning to our LDAP expert, thanks

            Show
            Petr Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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: