Moodle
  1. Moodle
  2. MDL-24699

update_user_record function does not specify mnethostid when updating the user table

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.8
    • Fix Version/s: 1.9.11, 2.0
    • Component/s: Authentication
    • Labels:
      None
    • Database:
      MySQL
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      1337

      Description

      Most of our users log in with external (Shibboleth) authentication. A lot of these queries show up in our MySQL slow query log.

      Count: 2  Time=1.00s (2s)  Lock=0.00s (0s)  Rows=0.0 (0), moodle[moodle]@localhost
       UPDATE mdl_user SET firstname = 'FIRST' WHERE username = 'user1@institution.edu'
      
      Count: 1  Time=1.00s (1s)  Lock=0.00s (0s)  Rows=0.0 (0), moodle[moodle]@localhost
       UPDATE mdl_user SET lastname = 'LAST' WHERE username = 'user2@institution.edu'
      
      Count: 1  Time=1.00s (1s)  Lock=0.00s (0s)  Rows=0.0 (0), moodle[moodle]@localhost
       UPDATE mdl_user SET email = 'user3@institution.edu' WHERE username = 'user3@institution.edu'
      
      Count: 2  Time=1.00s (2s)  Lock=0.00s (0s)  Rows=0.0 (0), moodle[moodle]@localhost
       UPDATE mdl_user SET institution = 'institution.edu' WHERE username = 'user4@institution.edu'
      
      Count: 1  Time=1.00s (1s)  Lock=0.00s (0s)  Rows=0.0 (0), moodle[moodle]@localhost
       UPDATE mdl_user SET idnumber = '123456789' WHERE username = 'user5@institution.edu'
      

      We tracked down these queries to a set_field function call in the update_user_record function in moodlelib.php. The user table has a unique key on the (mnethostid, username) pair but not on the username field alone. It also makes sense that (mnethostid, username) is unique and username is not, because users across different MNet sites can happen to have the same username. The set_field function call should have specified the mnethostid together with the username but it does not. Our site does not use MNet but if it did, the function call could have incorrectly updated records of more than one actual user.

      The same logic exists in the code we are using (1.9.8) as well as in MOODLE_19_WEEKLY and HEAD.

      Also, the update_user_record function updates multiple fields of a user record by calling the set_field function multiple time. They can be replaced with a single update_record function call.

        Issue Links

          Activity

          Hide
          Kai Chan added a comment -

          Here is a patch I have created by starting with the Moodle 1.98 version of moodlelib.php and copying over the changes we made. Before the code change, each user login adds several UPDATE statements (described in the ticket before) to the MySQL slow query log. According to the EXPLAIN command, MySQL reads the whole mdl_user table for each of these statements. With the code change in effect, the slow query log no longer shows these statements.

          Show
          Kai Chan added a comment - Here is a patch I have created by starting with the Moodle 1.98 version of moodlelib.php and copying over the changes we made. Before the code change, each user login adds several UPDATE statements (described in the ticket before) to the MySQL slow query log. According to the EXPLAIN command, MySQL reads the whole mdl_user table for each of these statements. With the code change in effect, the slow query log no longer shows these statements.
          Hide
          Nick Thompson added a comment -

          The fix that we've made for this is as follows:

          In the function:
          function update_user_record($username, $authplugin)

          { change: $oldinfo = get_record('user', 'username', $username, '','','','', 'username, auth'); to: $oldinfo = get_record('user', 'mnethostid', $CFG->mnet_localhost_id, 'username', $username, '', '', 'id, username, auth'); and change: set_field('user', $key, $value, 'username', $username) || error_log("Error updating $key for $username"); }

          }
          }

          to:
          $updateinfo[$key] = $value;
          }
          }
          }

          // MDL-24699: update user record only if we have recorded key-value pairs to update earlier
          if (isset($oldinfo->id) && isset($updateinfo) && count($updateinfo) > 0)

          { $updateinfo['id'] = $oldinfo->id; update_record('user', (object) $updateinfo) || error_log("Error updating record for $username"); }


          The entire function looks like this (remove ucla-specific comments when migrating to core):
          function update_user_record($username, $authplugin) {
          global $CFG;

          $username = trim(moodle_strtolower($username)); /// just in case check text case

          // START UCLA Modification
          // SSC-800, MDL-24699: specify (mnethostid, username) instead of just username
          $oldinfo = get_record('user', 'mnethostid', $CFG->mnet_localhost_id, 'username', $username, '', '', 'id, username, auth');
          // END UCLA Modification

          $userauth = get_auth_plugin($oldinfo->auth);

          if ($newinfo = $userauth->get_userinfo($username)) {
          $newinfo = truncate_userinfo($newinfo);
          foreach ($newinfo as $key => $value){
          if ($key === 'username') { // 'username' is not a mapped updateable/lockable field, so skip it. continue; }
          $confval = $userauth->config->{'field_updatelocal_' . $key};
          $lockval = $userauth->config->{'field_lock_' . $key};
          if (empty($confval) || empty($lockval)) { continue; }
          if ($confval === 'onlogin') {
          $value = addslashes($value);
          // MDL-4207 Don't overwrite modified user profile values with
          // empty LDAP values when 'unlocked if empty' is set. The purpose
          // of the setting 'unlocked if empty' is to allow the user to fill
          // in a value for the selected field _if LDAP is giving
          // nothing_ for this field. Thus it makes sense to let this value
          // stand in until LDAP is giving a value for this field.
          if (!(empty($value) && $lockval === 'unlockedifempty')) { // START UCLA Modification // SSC-800, MDL-24699: instead of calling set_field() right away, store the new key-value $updateinfo[$key] = $value; // END UCLA Modification }
          }
          }

          // START UCLA Modification
          // SSC-800, MDL-24699: update user record only if we have recorded key-value pairs to update earlier
          if (isset($oldinfo->id) && isset($updateinfo) && count($updateinfo) > 0) { $updateinfo['id'] = $oldinfo->id; update_record('user', (object) $updateinfo) || error_log("Error updating record for $username"); }

          // END UCLA Modification
          }

          return get_complete_user_data('username', $username);
          }

          Show
          Nick Thompson added a comment - The fix that we've made for this is as follows: In the function: function update_user_record($username, $authplugin) { change: $oldinfo = get_record('user', 'username', $username, '','','','', 'username, auth'); to: $oldinfo = get_record('user', 'mnethostid', $CFG->mnet_localhost_id, 'username', $username, '', '', 'id, username, auth'); and change: set_field('user', $key, $value, 'username', $username) || error_log("Error updating $key for $username"); } } } to: $updateinfo [$key] = $value; } } } // MDL-24699 : update user record only if we have recorded key-value pairs to update earlier if (isset($oldinfo->id) && isset($updateinfo) && count($updateinfo) > 0) { $updateinfo['id'] = $oldinfo->id; update_record('user', (object) $updateinfo) || error_log("Error updating record for $username"); } The entire function looks like this (remove ucla-specific comments when migrating to core): function update_user_record($username, $authplugin) { global $CFG; $username = trim(moodle_strtolower($username)); /// just in case check text case // START UCLA Modification // SSC-800, MDL-24699 : specify (mnethostid, username) instead of just username $oldinfo = get_record('user', 'mnethostid', $CFG->mnet_localhost_id, 'username', $username, '', '', 'id, username, auth'); // END UCLA Modification $userauth = get_auth_plugin($oldinfo->auth); if ($newinfo = $userauth->get_userinfo($username)) { $newinfo = truncate_userinfo($newinfo); foreach ($newinfo as $key => $value){ if ($key === 'username') { // 'username' is not a mapped updateable/lockable field, so skip it. continue; } $confval = $userauth->config->{'field_updatelocal_' . $key}; $lockval = $userauth->config->{'field_lock_' . $key}; if (empty($confval) || empty($lockval)) { continue; } if ($confval === 'onlogin') { $value = addslashes($value); // MDL-4207 Don't overwrite modified user profile values with // empty LDAP values when 'unlocked if empty' is set. The purpose // of the setting 'unlocked if empty' is to allow the user to fill // in a value for the selected field _if LDAP is giving // nothing_ for this field. Thus it makes sense to let this value // stand in until LDAP is giving a value for this field. if (!(empty($value) && $lockval === 'unlockedifempty')) { // START UCLA Modification // SSC-800, MDL-24699: instead of calling set_field() right away, store the new key-value $updateinfo[$key] = $value; // END UCLA Modification } } } // START UCLA Modification // SSC-800, MDL-24699 : update user record only if we have recorded key-value pairs to update earlier if (isset($oldinfo->id) && isset($updateinfo) && count($updateinfo) > 0) { $updateinfo['id'] = $oldinfo->id; update_record('user', (object) $updateinfo) || error_log("Error updating record for $username"); } // END UCLA Modification } return get_complete_user_data('username', $username); }
          Hide
          Petr Škoda added a comment -

          I have used a bit different code to fix 2.0dev and I have tried to make minimal changes in 1.9 in order to prevent potential regressions.
          Please test and reopen if necessary.

          Thanks for the report and patches!

          Petr

          Show
          Petr Škoda added a comment - I have used a bit different code to fix 2.0dev and I have tried to make minimal changes in 1.9 in order to prevent potential regressions. Please test and reopen if necessary. Thanks for the report and patches! Petr

            People

            • Votes:
              6 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: