Moodle
  1. Moodle
  2. MDL-30625

get_admin() seems to return random admin user instead

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.3, 2.2, 2.3
    • Fix Version/s: 2.2.1
    • Component/s: Administration
    • Labels:
    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      1/ Go to administrators UI and try some clicking
      2/ make sure the changes are reflected in get_admin() by creating some test script that dumps current admin

      Show
      1/ Go to administrators UI and try some clicking 2/ make sure the changes are reflected in get_admin() by creating some test script that dumps current admin
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w50_MDL-30625_m23_adminorder
    • Rank:
      33435

      Description

      The get_admin() function is supposed to return the main admin of the moodle installation. The comments in the source code say that it returns the admin user with lowest role_assignment id. However, looking at the code, there is no ordering at all.

      I recently configured a new admin user and now get_admin delivers this lastly assigned user instead of the first assigned admin user.

      From /lib/datalib.php:

      /**
       * Returns $user object of the main admin user
       * primary admin = admin with lowest role_assignment id among admins
       *
       * @static stdClass $mainadmin
       * @return stdClass {@link $USER} record from DB, false if not found
       */
      function get_admin() {
          static $mainadmin = null;
      
          if (!isset($mainadmin)) {
              if (! $admins = get_admins()) {
                  return false;
              }
              //TODO: add some admin setting for specifying of THE main admin
              //      for now return the first assigned admin
              $mainadmin = reset($admins);
          }
          // we must clone this otherwise code outside can break the static var
          return clone($mainadmin);
      }
      
      /**
       * Returns list of all admins, using 1 DB query
       *
       * @return array
       */
      function get_admins() {
          global $DB, $CFG;
      
          if (empty($CFG->siteadmins)) {  // Should not happen on an ordinary site
              return array();
          }
      
          $sql = "SELECT u.*
                    FROM {user} u
                   WHERE u.deleted = 0 AND u.id IN ($CFG->siteadmins)";
      
          return $DB->get_records_sql($sql);
      }
      

      It would be great if there was a admin setting to set the main admin like the source code says. Until that, I think there should be some kind of sorting.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          I think the best solution could be to finally implement the TODO, that is add some UI that allows us to reorder the $CFG->siteadmins where the first userid would be the "main" admin.

          Show
          Petr Škoda added a comment - I think the best solution could be to finally implement the TODO, that is add some UI that allows us to reorder the $CFG->siteadmins where the first userid would be the "main" admin.
          Hide
          Petr Škoda added a comment -

          thanks for the report!

          To integrators: please cherry pick to 2.2stable

          Show
          Petr Škoda added a comment - thanks for the report! To integrators: please cherry pick to 2.2stable
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm, it seems that previously used get_admins() has a "protection" against empty $CFG->siteadmins, returning empty array, that later is checked, leading to return false by is_admin()

          With the patch we lose that protection... just guessing if, at some point, it's called on install or so (before being set) and, after the patch, it may be leading to notice/warning somewhere.

          Note it's only a guess, but perhaps it wouldn't be crazy to add one extra check at the beginning preventing agains unset (or empty) $CFG->siteadmins

          Awaiting for a comment... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, it seems that previously used get_admins() has a "protection" against empty $CFG->siteadmins, returning empty array, that later is checked, leading to return false by is_admin() With the patch we lose that protection... just guessing if, at some point, it's called on install or so (before being set) and, after the patch, it may be leading to notice/warning somewhere. Note it's only a guess, but perhaps it wouldn't be crazy to add one extra check at the beginning preventing agains unset (or empty) $CFG->siteadmins Awaiting for a comment... ciao
          Hide
          Petr Škoda added a comment -

          I do not udnerstand:
          1/ I did not change get_admins()
          2/ the get_admin() validates that something from $CFG->siteadmins is present, if not it returns false

          Show
          Petr Škoda added a comment - I do not udnerstand: 1/ I did not change get_admins() 2/ the get_admin() validates that something from $CFG->siteadmins is present, if not it returns false
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, I know that you did not change get_admins(). But you stopped using it.

          And get_admins() handles undefined / empty $CFG->siteadmins perfectly, by returning empty array.

          Your new code goes straight to use explode(',', $CFG->siteadmins), that can lead to:

          • Some PHP notice if undefined.
          • Some "fake" result array(0 => '') if not set /empty.

          And, just to be in the safe side (if that function is used in early installation or wherever), I was suggesting to add, at the beginning:

          if (empty($CFG->siteadmins)) {  // Should not happen on an ordinary site
              return false;
          }
          

          so behavior will be 100% the same than the old one for undefined / empty situations.

          Just that. Ciao

          I'm going to integrate this right now as is. If you finally consider that it's better to have the check above, plz, add one commit on top and ping me. Else I'll assume you think it's ok to leave it that way.

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, I know that you did not change get_admins(). But you stopped using it. And get_admins() handles undefined / empty $CFG->siteadmins perfectly, by returning empty array. Your new code goes straight to use explode(',', $CFG->siteadmins), that can lead to: Some PHP notice if undefined. Some "fake" result array(0 => '') if not set /empty. And, just to be in the safe side (if that function is used in early installation or wherever), I was suggesting to add, at the beginning: if (empty($CFG->siteadmins)) { // Should not happen on an ordinary site return false ; } so behavior will be 100% the same than the old one for undefined / empty situations. Just that. Ciao I'm going to integrate this right now as is. If you finally consider that it's better to have the check above, plz, add one commit on top and ping me. Else I'll assume you think it's ok to leave it that way.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated into 22 and master, thanks!

          As said, for you to consider if the check in needed or no. Ping me if so.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated into 22 and master, thanks! As said, for you to consider if the check in needed or no. Ping me if so.
          Hide
          Sam Hemelryk added a comment -

          Thanks guys passing this now. Couldn't break the UI or in code.

          Show
          Sam Hemelryk added a comment - Thanks guys passing this now. Couldn't break the UI or in code.
          Hide
          Petr Škoda added a comment -

          GRRRR! you are right Eloy, sorry, extra commit added to my repo.

          To testers: did you test upgrade from 1.9->2.2?

          Show
          Petr Škoda added a comment - GRRRR! you are right Eloy, sorry, extra commit added to my repo. To testers: did you test upgrade from 1.9->2.2?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, merged to master and cherry-picked to 22_STABLE, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, merged to master and cherry-picked to 22_STABLE, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, you did it!

          Now your code is part of the best weeklies released ever, many thanks!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, you did it! Now your code is part of the best weeklies released ever, many thanks! Closing, ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Petr,

          No upgrade from 1.9 was not tested. Install upgrades wasn't mentioned in the test instructions so I never went there.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Petr, No upgrade from 1.9 was not tested. Install upgrades wasn't mentioned in the test instructions so I never went there. Cheers Sam

            People

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

              Dates

              • Created:
                Updated:
                Resolved: