Moodle
  1. Moodle
  2. MDL-32724

LDAP Auth function user_update() returns true even if update has failed

    Details

    • Testing Instructions:
      Hide
      1. Enable and configure the LDAP auth plugin. Use a bind user that doesn't have the privileges needed to change other users LDAP attributes (i.e., a "readonly" bind user).
      2. Map at least one external attribute to one internal Moodle user field (e.g., surname), and configure that mapping to 'Update external' with 'On update'. Make sure you use an attribute that has already a value for the LDAP user we are going to use in the next step.
      3. Log in with the LDAP user (to create the internal Moodle user account). If requested, fill in the missing mandatory user profile fields.
      4. Log out and log in as an admin.
      5. Edit the user details and change the user field that is mapped to 'Update external'. Without the bugfix, the user details will be changed in Moodle without error (but an LDAP error will be present in the PHP error logs). With the bugfix, an error stating 'Failed to update user data on external auth: ldap. See the server logs for more details' will be shown.
      Show
      Enable and configure the LDAP auth plugin. Use a bind user that doesn't have the privileges needed to change other users LDAP attributes (i.e., a "readonly" bind user). Map at least one external attribute to one internal Moodle user field (e.g., surname), and configure that mapping to 'Update external' with 'On update'. Make sure you use an attribute that has already a value for the LDAP user we are going to use in the next step. Log in with the LDAP user (to create the internal Moodle user account). If requested, fill in the missing mandatory user profile fields. Log out and log in as an admin. Edit the user details and change the user field that is mapped to 'Update external'. Without the bugfix, the user details will be changed in Moodle without error (but an LDAP error will be present in the PHP error logs). With the bugfix, an error stating 'Failed to update user data on external auth: ldap. See the server logs for more details' will be shown.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull 2.6 Branch:
      wip_26_mdl-32724
    • Pull Master Branch:
      wip_master_mdl-32724

      Description

      When updating user information and calling $authplugin->user_update($olduser, $newuser) the function returns a true or false based on whether it has been sucessful or not. If the function sucessfully updates the user through LDAP it returns true and conversly if the process fails for some reason it should return false.

      I am currently writing a script which relies on this information to control behaviour. If the user can be updated via LDAP then Moodle's record can also be updated, if not then an email will be sent to have the information updated manually.

      Unforuntately what I have found is that if the function succeeds in every step EXCEPT the actual LDAP command it still returns a true, meaning that if we have users accounts which cannot be modified by our bind user their records will not be updated but the returned value of true gives us no way of knowing this.

      I'm adding a patch which adds a check on the value of $changed at the very end of the script. If $changed equals false it will return false.

        Gliffy Diagrams

          Activity

          Hide
          Mark Ward added a comment -

          This is a very simple patch which simply checks if $changed has been set before returning the failsafe "true".

          Show
          Mark Ward added a comment - This is a very simple patch which simply checks if $changed has been set before returning the failsafe "true".
          Hide
          Petr Skoda added a comment -

          Reassigning to our LDAP guru, thanks for the report.

          Show
          Petr Skoda added a comment - Reassigning to our LDAP guru, thanks for the report.
          Hide
          Iñaki Arenaza added a comment -

          Hi Mark,

          note that your patch may incorrectly return "true" in some circumstances. $changed is reset in each iteration of the

          foreach ($attrmap as $key => $ldapkeys) {
              // Only process if the moodle field ($key) has changed and we
              // are set to update LDAP with it
              ...
          }
          

          loop. So it may happen that you have more than one LDAP attribute configured to be updated externally (say two, for the sake of it). And that you need to update both externallu. Say you fail to modify the first one (getting an entry in the error logs and also leaving $changed as "false"), but you successfully modify the second one.

          As we set $changed = true; in the second iteration of the loop (we successfully updated the second attribute), we "loose" the information about the first failed modification attempt. So even if the user is not fully updated externally, we would return "true".

          So we need to either use another variable to keep track of the "global" success/failure (my choice), or use $changed in a more intelligent/complicated way.

          By the way, I noticed there's a missing $changed = true; after the first call to ldap_modify() and before the continue; (line 1127 in MOODLE_21_STABLE), which is a bug. I'll open a new bug for it.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Mark, note that your patch may incorrectly return "true" in some circumstances. $changed is reset in each iteration of the foreach ($attrmap as $key => $ldapkeys) { // Only process if the moodle field ($key) has changed and we // are set to update LDAP with it ... } loop. So it may happen that you have more than one LDAP attribute configured to be updated externally (say two, for the sake of it). And that you need to update both externallu. Say you fail to modify the first one (getting an entry in the error logs and also leaving $changed as "false"), but you successfully modify the second one. As we set $changed = true; in the second iteration of the loop (we successfully updated the second attribute), we "loose" the information about the first failed modification attempt. So even if the user is not fully updated externally, we would return "true". So we need to either use another variable to keep track of the "global" success/failure (my choice), or use $changed in a more intelligent/complicated way. By the way, I noticed there's a missing $changed = true; after the first call to ldap_modify() and before the continue; (line 1127 in MOODLE_21_STABLE), which is a bug. I'll open a new bug for it. Saludos. Iñaki.
          Hide
          Mark Ward added a comment -

          Hi Iñaki,

          Thanks for taking a look. You are quite right, I hadn't considered that more than one modification may be passed.

          If we created an array of $changed[$ldapkey] we could keep a track of those results individually. The check at the end would then be A)Is $changed an array? and B)Does it contain a "FALSE"?. If you think that sounds like an adequate solution I will write it up and get the patch attached here.

          Thanks again!

          Show
          Mark Ward added a comment - Hi Iñaki, Thanks for taking a look. You are quite right, I hadn't considered that more than one modification may be passed. If we created an array of $changed [$ldapkey] we could keep a track of those results individually. The check at the end would then be A)Is $changed an array? and B)Does it contain a "FALSE"?. If you think that sounds like an adequate solution I will write it up and get the patch attached here. Thanks again!
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d.

          TW9vZGxlDQo=

          Show
          Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d. TW9vZGxlDQo=
          Hide
          Iñaki Arenaza added a comment -

          Hi Michael,

          this issue is still relevant for current versions.

          Saludos.
          Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Michael, this issue is still relevant for current versions. Saludos. Iñaki.
          Hide
          CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          Sam Hemelryk added a comment -

          Thanks guys this has been integrated now.

          Please note it was not backported to 2.4, that branch is only receiving security fixes now.

          Show
          Sam Hemelryk added a comment - Thanks guys this has been integrated now. Please note it was not backported to 2.4, that branch is only receiving security fixes now.
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Iñaki,

          Works as expected. Passing.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Iñaki, Works as expected. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Fetch your remotes, prune them,
          clean your integrated branches and say "Ahem".

          Rebase your ongoing stuff, keep conflicts away
          don't forget to test the code and we'll love you again.

          Thanks, closing!

          Show
          Eloy Lafuente (stronk7) added a comment - Fetch your remotes, prune them, clean your integrated branches and say "Ahem". Rebase your ongoing stuff, keep conflicts away don't forget to test the code and we'll love you again. Thanks, closing!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: