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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Petr Skoda 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 Skoda 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 Skoda added a comment -

            thanks for the report!

            To integrators: please cherry pick to 2.2stable

            Show
            Petr Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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: