Moodle
  1. Moodle
  2. MDL-28402

LDAP configuration values being stored in lower case, causing misconfiguration

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Authentication
    • Labels:
    • Environment:
      Windows Server 2003, Apache/2.2.17 (Win32), PHP/5.3.6
    • Database:
      MySQL
    • Testing Instructions:
      Hide
      • Configure the Active Directory domain security policy (Security Settings >> Account Policies >> Password Policiy >> Maximum password age), to expire passwords after a given time (e.g., 7 days). Make sure you use gpupdate to propage security policy changes to all domain controllers (including the local one).
      • Create or edit a user in Active Directory and set a new password for her, and make sure the password is configured to expire.
      • Configure Moodle to use that Active Directory as it's LDAP server (see instructions at http://docs.moodle.org/en/LDAP_authentication)
      • While configuring the LDAP server settings, make sure you enable password expiration (set it to 'LDAP') and set expiration warning to a sensible value (e.g., 5 days).
      • Log in with the Active Directory user created/modified above. Nothing should be notified to the user. Now log out.
      • Change the date in the Moodle Server to make the password be in the 'warning period' or simply wait enough time until we reach it
      • Log in again with the same user. A message should be printed telling the user the password is expiring in X days and offering her to change it (if possible from within moodle).
      Show
      Configure the Active Directory domain security policy (Security Settings >> Account Policies >> Password Policiy >> Maximum password age), to expire passwords after a given time (e.g., 7 days). Make sure you use gpupdate to propage security policy changes to all domain controllers (including the local one). Create or edit a user in Active Directory and set a new password for her, and make sure the password is configured to expire. Configure Moodle to use that Active Directory as it's LDAP server (see instructions at http://docs.moodle.org/en/LDAP_authentication ) While configuring the LDAP server settings, make sure you enable password expiration (set it to 'LDAP') and set expiration warning to a sensible value (e.g., 5 days). Log in with the Active Directory user created/modified above. Nothing should be notified to the user. Now log out. Change the date in the Moodle Server to make the password be in the 'warning period' or simply wait enough time until we reach it Log in again with the same user. A message should be printed telling the user the password is expiring in X days and offering her to change it (if possible from within moodle).
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip_MDL-28402_HEAD_pwd_expiration_time_doesnt_work

      Description

      When configuring the LDAP authentication plugin (Plugins > Authentication > LDAP server) certain configuration fields such as "User attribute" and "Expiration attribute" are being converted to lower-case after clicking "save changes".

      I also did some digging around inside auth/ldap.auth.php and found that at around line 584 which is within the password_expire function, the default value of "$this->config->expireattr" is set to "pwdlastset" whilst in Moodle 1.9 this gives the correct value of "pwdLastSet". Since this shows that default values are being stored as lower-case also the problem may not sit with the edit form.

      I have tried setting these values manually within the database and can confirm that the table allows you to store the correct setting, although as soon as I go to LDAP Server settings and click "save changes" this reverts again.

      Please note that this is currently rendering expired password checking non-functional.

      NOTICED ON: Moodle 2.03 latest

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment -

            Hi Mark,

            Thanks for reporting this - i'm going to take this issue to try and ensure we get this resolved.

            Show
            Dan Poltawski added a comment - Hi Mark, Thanks for reporting this - i'm going to take this issue to try and ensure we get this resolved.
            Hide
            Dan Poltawski added a comment -

            (added Iñaki as a watcher)..

            Hi Iñaki,

            I think you are the resident expert on the LDAP plugin..

            Do you know why we are lowercasing these fields, and would it cause problems if we reversed this change?

            Show
            Dan Poltawski added a comment - (added Iñaki as a watcher).. Hi Iñaki, I think you are the resident expert on the LDAP plugin.. Do you know why we are lowercasing these fields, and would it cause problems if we reversed this change?
            Hide
            Iñaki Arenaza added a comment -

            Hi Dan,

            we lowercase these fields because in the past we had lots of problems with different LDAP servers returning attributes in LDAP responses in different cases (some lowercase, some uppercase, some mixed). And the keys of the arrays returned from several PHP LDAP functions (but not all of them!) were constructed using the case of the attributes returned by the LDAP server.

            This meant you had to type your LDAP setting to exactly match the value returned from your LDAP query, which was a big trouble for some people and definitely a mess for LDAP documentation in Moodle Docs.

            So when we rewrote the LDAP enrolment for 2.x, we refactored some common code between auth & enrol and we also did the "lowercasing" for managing attributes. It looks like I missed some cases for password expiration.

            I'll have a look at it as soon as possible and report back.

            Saludos.
            Iñaki.

            Show
            Iñaki Arenaza added a comment - Hi Dan, we lowercase these fields because in the past we had lots of problems with different LDAP servers returning attributes in LDAP responses in different cases (some lowercase, some uppercase, some mixed). And the keys of the arrays returned from several PHP LDAP functions (but not all of them!) were constructed using the case of the attributes returned by the LDAP server. This meant you had to type your LDAP setting to exactly match the value returned from your LDAP query, which was a big trouble for some people and definitely a mess for LDAP documentation in Moodle Docs. So when we rewrote the LDAP enrolment for 2.x, we refactored some common code between auth & enrol and we also did the "lowercasing" for managing attributes. It looks like I missed some cases for password expiration. I'll have a look at it as soon as possible and report back. Saludos. Iñaki.
            Hide
            Iñaki Arenaza added a comment -

            By the way Mark,

            which LDAP server are you using? (to try and reproduce the bug here).

            Saludos,
            Iñaki.

            Show
            Iñaki Arenaza added a comment - By the way Mark, which LDAP server are you using? (to try and reproduce the bug here). Saludos, Iñaki.
            Hide
            Mark Ward added a comment -

            Thanks for looking at this folks

            We are using Windows Server 2003 as the domain controller and I have set the "user type" to MS Active Directory.

            With these settings on Moodle 1.9, connecting to the same domain controller, everything worked pretty much out of the box, Moodle used the correct "pwdLastSet" as the "Expiration Attribute" and it knew to work out the the actual expiration time from pwdLastSet and maxPwdAge which I haven't been able to verify Moodle 2 is doing correctly.

            The only none default setting I changed on 1.9 was "User attribute" which I used to be able to set to "sAMAccountName" just for compatibility, however this too gets changed into "samaccountname".

            Show
            Mark Ward added a comment - Thanks for looking at this folks We are using Windows Server 2003 as the domain controller and I have set the "user type" to MS Active Directory. With these settings on Moodle 1.9, connecting to the same domain controller, everything worked pretty much out of the box, Moodle used the correct "pwdLastSet" as the "Expiration Attribute" and it knew to work out the the actual expiration time from pwdLastSet and maxPwdAge which I haven't been able to verify Moodle 2 is doing correctly. The only none default setting I changed on 1.9 was "User attribute" which I used to be able to set to "sAMAccountName" just for compatibility, however this too gets changed into "samaccountname".
            Hide
            Iñaki Arenaza added a comment -

            Hi Mark,

            can you try the attached patch? It seems to fix the issue in my test environment.

            If it does, the same patch can be applied to 2.1 and HEAD (it applies cleanly). Dan, would you take care of it or should I?

            Saludos.
            Iñaki.

            Show
            Iñaki Arenaza added a comment - Hi Mark, can you try the attached patch? It seems to fix the issue in my test environment. If it does, the same patch can be applied to 2.1 and HEAD (it applies cleanly). Dan, would you take care of it or should I? Saludos. Iñaki.
            Hide
            Dan Poltawski added a comment -

            Hi Iñaki,

            I'll assign to you since you are doing all the work!

            Let me know if you need anything from me!

            Show
            Dan Poltawski added a comment - Hi Iñaki, I'll assign to you since you are doing all the work! Let me know if you need anything from me!
            Hide
            Mark Ward added a comment - - edited

            Patch has fixed the problem as far as default settings go, thank you! However I still cannot store upper-case letters on the LDAP Auth configuration page, ie: "sAMAccountName" becomes "samaccountname" when I save changes.

            Update: Noticed one more thing, it is calculating I have one extra day until my password requires (currently saying 8 days when I actually have just 7).

            Show
            Mark Ward added a comment - - edited Patch has fixed the problem as far as default settings go, thank you! However I still cannot store upper-case letters on the LDAP Auth configuration page, ie: "sAMAccountName" becomes "samaccountname" when I save changes. Update: Noticed one more thing, it is calculating I have one extra day until my password requires (currently saying 8 days when I actually have just 7).
            Hide
            Iñaki Arenaza added a comment -

            Hi Mark,

            sorry for the delay (we are on summer holiday here and I'm on and off with internet access).

            I'll have a deeper look at the extra password expiration day (calculations are a bit convoluted due to MS strange choice of values to take into account), but I'll open a new bug for it.

            With respect to being unable to save settings in upper/mixed case, as I stated above, this is intentional. It shouldn't pose a problem (unless there are other bugs like this one remaining in the code). In fact, it prevents quite a few problems, so that's why they are forced to lowercase on saving changes to the settings.

            So just to be sure, did the patch fix the password expiration issue? (other than the wrong calculation of remaining days). So I can propose the fix to the core developers for integration.

            Saludos.
            Iñaki.

            Show
            Iñaki Arenaza added a comment - Hi Mark, sorry for the delay (we are on summer holiday here and I'm on and off with internet access). I'll have a deeper look at the extra password expiration day (calculations are a bit convoluted due to MS strange choice of values to take into account), but I'll open a new bug for it. With respect to being unable to save settings in upper/mixed case, as I stated above, this is intentional. It shouldn't pose a problem (unless there are other bugs like this one remaining in the code). In fact, it prevents quite a few problems, so that's why they are forced to lowercase on saving changes to the settings. So just to be sure, did the patch fix the password expiration issue? (other than the wrong calculation of remaining days). So I can propose the fix to the core developers for integration. Saludos. Iñaki.
            Hide
            Michael de Raadt added a comment -

            Hi, All.

            I've just put this on our backlog, but please keep working on it.

            Show
            Michael de Raadt added a comment - Hi, All. I've just put this on our backlog, but please keep working on it.
            Hide
            Mark Ward added a comment -

            Hi Iñaki,

            Yes, that is correct, the patch did fix the password expiration issue aside from the "+1 day" problem.

            Many thanks once again for your efforts!

            Mark

            Show
            Mark Ward added a comment - Hi Iñaki, Yes, that is correct, the patch did fix the password expiration issue aside from the "+1 day" problem. Many thanks once again for your efforts! Mark
            Hide
            Iñaki Arenaza added a comment -

            Hi Mark and Michael,

            thanks for your confirmation

            I've had a look at the "+1 day" problem, and I think the calculations are right.

            I've added lots of debuggining traces to the code, and the password expiration date is calculated right. The "problem" happens in the rounding to a number of days. Say your password expires on "Mon Aug 29 16:38:42 2011" and it's now "Tue Aug 23 00:03:15 2011". If we calculate how many days there are between now and the expiration date, we get 6.69 (rounded to two decimals). Do we tell the user there are six days to expiration or do we tell there are seven? The former is not completely true (you have 6 days plus most of the seventh day), neither is the later (we have a bit less than seven days).

            Current code always rounds days up, so that's why in this case we would tell the user her password expires in seven days. It may be happening the same to you with your "+1 day". As long as the expiration date is only 1 second later than "now + integral-number-of-days", we will tell the user the password expires in "integral-number-of-days + 1".

            So I think the calculations are right and I'm not going to open a new bug for this.

            Michael, I'll make up-to-date patches for 2.0.x, 2.1.x and master and I'll submit them for integration.

            Saludos.
            Iñaki.

            Show
            Iñaki Arenaza added a comment - Hi Mark and Michael, thanks for your confirmation I've had a look at the "+1 day" problem, and I think the calculations are right. I've added lots of debuggining traces to the code, and the password expiration date is calculated right. The "problem" happens in the rounding to a number of days. Say your password expires on "Mon Aug 29 16:38:42 2011" and it's now "Tue Aug 23 00:03:15 2011". If we calculate how many days there are between now and the expiration date, we get 6.69 (rounded to two decimals). Do we tell the user there are six days to expiration or do we tell there are seven? The former is not completely true (you have 6 days plus most of the seventh day), neither is the later (we have a bit less than seven days). Current code always rounds days up, so that's why in this case we would tell the user her password expires in seven days. It may be happening the same to you with your "+1 day". As long as the expiration date is only 1 second later than "now + integral-number-of-days", we will tell the user the password expires in "integral-number-of-days + 1". So I think the calculations are right and I'm not going to open a new bug for this. Michael, I'll make up-to-date patches for 2.0.x, 2.1.x and master and I'll submit them for integration. Saludos. Iñaki.
            Hide
            Iñaki Arenaza added a comment -

            Updated patches submitted for integration!

            Thanks a lot to Mark Ward for reporting the issue, and Dan Poltawski for pointing it to me

            Saludos.
            Iñaki.

            Show
            Iñaki Arenaza added a comment - Updated patches submitted for integration! Thanks a lot to Mark Ward for reporting the issue, and Dan Poltawski for pointing it to me Saludos. Iñaki.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Iñaki Arenaza added a comment -

            PULL branches rebased

            Saludos.
            Iñaki.

            Show
            Iñaki Arenaza added a comment - PULL branches rebased Saludos. Iñaki.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Integrated, thanks!

            Three notes:

            1) I've integrated it by picking and amending the commit to fix one "else" to follow coding guidelines. And then backported to 21 and 20 stables by cherry-picking (the patch was the same).

            2) In cases like this, you can submit only the master fix and then state in the comments of the issue that this should be backported to xx and yy. Something like "Note for integrators: Plz, backport this to xx and yy". Usually only bugfixes and if they apply clean (else you need to created all the branches).

            3) It would be great if, when naming your branches, you put the "branch" information as a suffix, something like "_master", "_21" is perfect. It's a tiny detail but it makes things quicker and easier when integrating multiple branches. Note this is not a must, but one suggestion only.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Integrated, thanks! Three notes: 1) I've integrated it by picking and amending the commit to fix one "else" to follow coding guidelines. And then backported to 21 and 20 stables by cherry-picking (the patch was the same). 2) In cases like this, you can submit only the master fix and then state in the comments of the issue that this should be backported to xx and yy. Something like "Note for integrators: Plz, backport this to xx and yy". Usually only bugfixes and if they apply clean (else you need to created all the branches). 3) It would be great if, when naming your branches, you put the "branch" information as a suffix, something like "_master", "_21" is perfect. It's a tiny detail but it makes things quicker and easier when integrating multiple branches. Note this is not a must, but one suggestion only. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Passing based on feedback from Mark Ward and fully trusting Iñaki.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Passing based on feedback from Mark Ward and fully trusting Iñaki. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            git & cvs repositories have been populated with this solution. Many thanks for your collaboration, yay!

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - git & cvs repositories have been populated with this solution. Many thanks for your collaboration, yay! Closing, ciao

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: