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
    • Rank:
      18134

      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

        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: