Moodle
  1. Moodle
  2. MDL-15175

Moodle 1.8.2 db authentication not retrieving data.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.2
    • Fix Version/s: 1.8.7, 1.9.3
    • Component/s: Authentication
    • Labels:
      None
    • Environment:
      Linux Red hat
    • Database:
      Oracle
    • Affected Branches:
      MOODLE_18_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Rank:
      30846

      Description

      Running auth/db/auth_db_sync_users.php does not retrieve the data from a oracle database, checking using another script the records are retrieved.

      1. auth.php
        27 kB
        Angelo Rigo
      2. auth.php.MDL-15175.patch.txt
        0.8 kB
        Eloy Lafuente (stronk7)

        Issue Links

          Activity

          Hide
          Angelo Rigo added a comment -

          In modle/auth/db/auth.php file inside get_userlist() function i do debug the $result variable using:

          print_r($result);

          and it returns me an array of 30 000 positions (my 30000 records from oracle db) all with empty values.

          Show
          Angelo Rigo added a comment - In modle/auth/db/auth.php file inside get_userlist() function i do debug the $result variable using: print_r($result); and it returns me an array of 30 000 positions (my 30000 records from oracle db) all with empty values.
          Hide
          Angelo Rigo added a comment -

          On auth/db/auth.php
          inside get_userlist function i have these lines :

          while ($rec = rs_fetch_next_record($rs))

          { array_push($result, $rec->username); }

          Printing this array i see all the 30000 positions with empty values

          Now in dmlib.php inside rs_fetch_next_record function line +-764 i do var_dump the $rec variable so that i can see the resultset with all the usernames.

          I am searching now what can be done to retrieve all the recordsets with proper values :-?

          How can i fix it ?

          Show
          Angelo Rigo added a comment - On auth/db/auth.php inside get_userlist function i have these lines : while ($rec = rs_fetch_next_record($rs)) { array_push($result, $rec->username); } Printing this array i see all the 30000 positions with empty values Now in dmlib.php inside rs_fetch_next_record function line +-764 i do var_dump the $rec variable so that i can see the resultset with all the usernames. I am searching now what can be done to retrieve all the recordsets with proper values :-? How can i fix it ?
          Hide
          Angelo Rigo added a comment -

          var_dumping the return of lib/adodb/adodb.inc.php FetchRow function also returns the proper records with no empty values but the users as expected.

          Show
          Angelo Rigo added a comment - var_dumping the return of lib/adodb/adodb.inc.php FetchRow function also returns the proper records with no empty values but the users as expected.
          Hide
          Angelo Rigo added a comment -

          in the auth/db/auth.php i change this line :
          array_push($result, $rec->username);
          To:
          array_push($result, $rec->USERNAME);

          it work´s now i the users are being retrieved from the oracle database but running auth/db/auth.php it stops to insert users in the mdl_user table in a mysql database at 1700 and the total number of users is 27500.

          Show
          Angelo Rigo added a comment - in the auth/db/auth.php i change this line : array_push($result, $rec->username); To: array_push($result, $rec->USERNAME); it work´s now i the users are being retrieved from the oracle database but running auth/db/auth.php it stops to insert users in the mdl_user table in a mysql database at 1700 and the total number of users is 27500.
          Hide
          Iñaki Arenaza added a comment -

          Eloy,

          do you know why ADOdb is behaving like that (making field names uppercase) when dealing with Oracle? Is there a technical reason for it?
          It's bitting a lot of people in the a**.

          Saludos. Iñaki.

          Show
          Iñaki Arenaza added a comment - Eloy, do you know why ADOdb is behaving like that (making field names uppercase) when dealing with Oracle? Is there a technical reason for it? It's bitting a lot of people in the a**. Saludos. Iñaki.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well, it's Oracle, it's Oracle and, by default, returns uppercase field names. Anyway, ADOdb can be forced to return lowercase fieldnames by executing:

          define ('ADODB_ASSOC_CASE', 0);

          BEFORE connection to DB.

          Note that the define above is GLOBAL so it will affect ALL the ADOdb connections. So, if the script has executed another define() like that (with another value), the second one won't have effect.

          And that's exactly the thing happening when auth_db_sync_users.php is executed (it calls config.php, that calls setup.php) and it executes the define, setting it to "2". That means "native", and native=uppercase in Oracle.

          So, to be in the safe side... I think we must do one array_change_key_case() to convert every object to lowercase keys. That way, it should work in all DBs without being affected by the global define.

          Something like:

          $dbrecord = (object)array_change_key_case((array)$dbrecord , CASE_LOWER);

          for each record fetched. Note I haven't tested this under PHP 4.3.

          I's some extra work but provides full cross-db. Saving us from the define() problems.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, it's Oracle, it's Oracle and, by default, returns uppercase field names. Anyway, ADOdb can be forced to return lowercase fieldnames by executing: define ('ADODB_ASSOC_CASE', 0); BEFORE connection to DB. Note that the define above is GLOBAL so it will affect ALL the ADOdb connections. So, if the script has executed another define() like that (with another value), the second one won't have effect. And that's exactly the thing happening when auth_db_sync_users.php is executed (it calls config.php, that calls setup.php) and it executes the define, setting it to "2". That means "native", and native=uppercase in Oracle. So, to be in the safe side... I think we must do one array_change_key_case() to convert every object to lowercase keys. That way, it should work in all DBs without being affected by the global define. Something like: $dbrecord = (object)array_change_key_case((array)$dbrecord , CASE_LOWER); for each record fetched. Note I haven't tested this under PHP 4.3. I's some extra work but provides full cross-db. Saving us from the define() problems. Ciao
          Hide
          Angelo Rigo added a comment -

          About an easiest way just to make it works with the current version.

          In the auth/db/auth.php i change this line :
          array_push($result, $rec->username);
          To:
          array_push($result, $rec->USERNAME);

          so it works!

          In the get_userinfo function i have $fields_obj containing all the needed data
          and inside the foreach in the next line : $textlib->convert($fields_obj->{$localname}, $this->config->extencoding, 'utf-8') brings only NULL values

          if ($rs = $authdb->Execute($sql)) {
          if ( $rs->RecordCount() == 1 ) {
          $fields_obj = rs_fetch_record($rs);
          //error_log (print_r ($fields_obj), TRUE); this brings all the results
          foreach ($selectfields as $localname=>$externalname) {
          var_dump($textlib->convert($fields_obj->{$localname}, $this->config->extencoding, 'utf-8')); //this shows all the resultset as Null
          $result[$localname] = $textlib->convert($fields_obj->{$localname}, $this->config->extencoding, 'utf-8');
          }
          }
          rs_close($rs);
          }

          Show
          Angelo Rigo added a comment - About an easiest way just to make it works with the current version. In the auth/db/auth.php i change this line : array_push($result, $rec->username); To: array_push($result, $rec->USERNAME); so it works! In the get_userinfo function i have $fields_obj containing all the needed data and inside the foreach in the next line : $textlib->convert($fields_obj->{$localname}, $this->config->extencoding, 'utf-8') brings only NULL values if ($rs = $authdb->Execute($sql)) { if ( $rs->RecordCount() == 1 ) { $fields_obj = rs_fetch_record($rs); //error_log (print_r ($fields_obj), TRUE); this brings all the results foreach ($selectfields as $localname=>$externalname) { var_dump($textlib->convert($fields_obj->{$localname}, $this->config->extencoding, 'utf-8')); //this shows all the resultset as Null $result [$localname] = $textlib->convert($fields_obj->{$localname}, $this->config->extencoding, 'utf-8'); } } rs_close($rs); }
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Attaching possible patch to fix this problem in a cross-db way. It guaranties that all the field names will be lowercase.

          Please test. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Attaching possible patch to fix this problem in a cross-db way. It guaranties that all the field names will be lowercase. Please test. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Assigning this to Jerome... I'm going to be out some days...

          Jerome once confirmed it works... could you commit it? Patch above is for Moodle 1.9.

          Targets should be IMO, 18_STABLE, 19_STABLE and HEAD.

          Reference: http://moodle.org/mod/forum/discuss.php?d=100636

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Assigning this to Jerome... I'm going to be out some days... Jerome once confirmed it works... could you commit it? Patch above is for Moodle 1.9. Targets should be IMO, 18_STABLE, 19_STABLE and HEAD. Reference: http://moodle.org/mod/forum/discuss.php?d=100636 Ciao
          Hide
          Angelo Rigo added a comment -

          Hi !

          Excelent! It is so good to see the sync working, now is working for me at 1.8.2, i will also test at a 1.9.1version.

          Thank you so much! for the patch. i will look now at http://tracker.moodle.org/browse/MDL-15475 to make it works like ldap auth does creating a temp table.

          Valeu ! Excelente !

          Show
          Angelo Rigo added a comment - Hi ! Excelent! It is so good to see the sync working, now is working for me at 1.8.2, i will also test at a 1.9.1version. Thank you so much! for the patch. i will look now at http://tracker.moodle.org/browse/MDL-15475 to make it works like ldap auth does creating a temp table. Valeu ! Excelente !
          Hide
          Jérôme Mouneyrac added a comment -

          Hi, I just had a look to this issue.
          My external database is oracle, my moodle database is mysql.
          I confirm the bug on 1.8 and 1.9, and I also confirm that Angelos fix + Eloys patch fix the bug.
          I'll commit on 1.8 and 1.9.

          On HEAD, running the script display HTML code, it doesn't work at all. I'll commit the fix on HEAD anyway as the code is similar, and I'll write a new issue for HEAD.

          Show
          Jérôme Mouneyrac added a comment - Hi, I just had a look to this issue. My external database is oracle, my moodle database is mysql. I confirm the bug on 1.8 and 1.9, and I also confirm that Angelos fix + Eloys patch fix the bug. I'll commit on 1.8 and 1.9. On HEAD, running the script display HTML code, it doesn't work at all. I'll commit the fix on HEAD anyway as the code is similar, and I'll write a new issue for HEAD.
          Hide
          Jérôme Mouneyrac added a comment -

          I commited in 1.8, 1.9 and HEAD. Sync in HEAD doesn't work, I wrote a issue for that: MDL-16023
          Thanks everybody for your participation, issue closed.

          Show
          Jérôme Mouneyrac added a comment - I commited in 1.8, 1.9 and HEAD. Sync in HEAD doesn't work, I wrote a issue for that: MDL-16023 Thanks everybody for your participation, issue closed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Reopening this... I just have seen your patch and the second part:

          • array_push($result, $rec->username);
            + array_push($result, $rec->USERNAME);

          looks wrong for me. I think it effectively will work against Oracle, but... will break under other databases, where field names are returned lowercase.

          Note I haven't tested it, but seems logical it will cause breakage. IMO, we should, always, perform the:

          $rec = (object)array_change_key_case((array)$rec , CASE_LOWER);

          conversion, for each record retrieved. That way we'll have lowercase always and all DBs will work.

          Renote I haven't tested

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Reopening this... I just have seen your patch and the second part: array_push($result, $rec->username); + array_push($result, $rec->USERNAME); looks wrong for me. I think it effectively will work against Oracle, but... will break under other databases, where field names are returned lowercase. Note I haven't tested it, but seems logical it will cause breakage. IMO, we should, always, perform the: $rec = (object)array_change_key_case((array)$rec , CASE_LOWER); conversion, for each record retrieved. That way we'll have lowercase always and all DBs will work. Renote I haven't tested Ciao
          Hide
          Jérôme Mouneyrac added a comment -

          when I tested with an external Oracle database I needed your patch + this line to make it works. I have another look...

          Show
          Jérôme Mouneyrac added a comment - when I tested with an external Oracle database I needed your patch + this line to make it works. I have another look...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          yes, yes, Sure it worked! But only under Oracle!

          $rec->USERNAME only exists under Oracle (the rest of DBs will have $rec->username instead)

          So, my proposal is to apply the conversion AGAIN there.

          In fact, all queries against external DBs having aliases (AS keyword), both in auth and enrol, must guarantee objects returned have all the attributes lowercased if they are used later to push values into Moodle.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - yes, yes, Sure it worked! But only under Oracle! $rec->USERNAME only exists under Oracle (the rest of DBs will have $rec->username instead) So, my proposal is to apply the conversion AGAIN there. In fact, all queries against external DBs having aliases (AS keyword), both in auth and enrol, must guarantee objects returned have all the attributes lowercased if they are used later to push values into Moodle. Ciao
          Hide
          Jérôme Mouneyrac added a comment -

          my apologize, I should have perfectly understood what I committed. I added the username conversion into lowercase, so it should ensure cross-db as Eloy proposal. I commited the changed on 1.8, 1.9 and HEAD.
          I'll check enrol sync and write a new issue if it needs to be fixed as well.

          Show
          Jérôme Mouneyrac added a comment - my apologize, I should have perfectly understood what I committed. I added the username conversion into lowercase, so it should ensure cross-db as Eloy proposal. I commited the changed on 1.8, 1.9 and HEAD. I'll check enrol sync and write a new issue if it needs to be fixed as well.
          Hide
          Iñaki Arenaza added a comment -

          Eloy,

          why doesn't Moodle set a policy stating that all the attribute names coming out of the database queries (via get_*) are always lowercase (or uppercase, for that matter) and enforce it inside the dmllib.php functions? That would make it cross-database safe and consistent.

          Of course that would need a fair amount of code review (so it's clearly a candidate for 2.0), but would solve all of these issues once and for all.

          Saludos. Iñaki.

          Show
          Iñaki Arenaza added a comment - Eloy, why doesn't Moodle set a policy stating that all the attribute names coming out of the database queries (via get_*) are always lowercase (or uppercase, for that matter) and enforce it inside the dmllib.php functions? That would make it cross-database safe and consistent. Of course that would need a fair amount of code review (so it's clearly a candidate for 2.0), but would solve all of these issues once and for all. Saludos. Iñaki.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well,

          $db, (or $DB in Moodle 2.0) connection and all dml stuff enforces it and it works well under all DB, using always lowercase fieldnames.

          It's only "other" connections (auth, enrol...) the ones out of control, because in a lot of cases they are using adodb's code instead of Moodle's connections and dml stuff.

          I hope that, for 2.0... where auth and enrol must be revisited and improved... we'll switch to use new $DB stuff, instead of raw adodb access... that way the problem should be out. Until then... I think it's easier to "enforce" that lowercase thing within auth and enrol.

          But agree 2.0 should be using new database code (instead of raw adodb). That way everything will be, always, lowercase.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, $db, (or $DB in Moodle 2.0) connection and all dml stuff enforces it and it works well under all DB, using always lowercase fieldnames. It's only "other" connections (auth, enrol...) the ones out of control, because in a lot of cases they are using adodb's code instead of Moodle's connections and dml stuff. I hope that, for 2.0... where auth and enrol must be revisited and improved... we'll switch to use new $DB stuff, instead of raw adodb access... that way the problem should be out. Until then... I think it's easier to "enforce" that lowercase thing within auth and enrol. But agree 2.0 should be using new database code (instead of raw adodb). That way everything will be, always, lowercase. Ciao
          Hide
          Jérôme Mouneyrac added a comment -

          I created an issue for the enrolment sync: MDL-16043

          Show
          Jérôme Mouneyrac added a comment - I created an issue for the enrolment sync: MDL-16043
          Hide
          Jérôme Mouneyrac added a comment -
          Show
          Jérôme Mouneyrac added a comment - MDL-16043
          Hide
          Angelo Rigo added a comment -

          Hi

          This file is working under production using Oracle.

          I am really using ->USERNAME at line 394 at sync-users function :

          echo "\t"; print_string('auth_dbreviveuser', 'auth', array(stripslashes($user->USERNAME), $user->id)); echo "\n";

          and line 455 at the get_userlist function :

          array_push($result, $rec->USERNAME);

          I can try in our test server any modifications on these lines to ensure that it can work with Oracle and another databases too.

          Cheers

          Show
          Angelo Rigo added a comment - Hi This file is working under production using Oracle. I am really using ->USERNAME at line 394 at sync-users function : echo "\t"; print_string('auth_dbreviveuser', 'auth', array(stripslashes($user->USERNAME), $user->id)); echo "\n"; and line 455 at the get_userlist function : array_push($result, $rec->USERNAME); I can try in our test server any modifications on these lines to ensure that it can work with Oracle and another databases too. Cheers
          Hide
          Jérôme Mouneyrac added a comment -

          need some retest, see MDL-16043

          Show
          Jérôme Mouneyrac added a comment - need some retest, see MDL-16043
          Hide
          Jérôme Mouneyrac added a comment -

          ok I tested with UPPERCASE fieldnames and UPPERCASE table name into the admin form. It still works. I checked the code as well, nothing need to be changed as it needs for MDL-16043.
          You can retest this issue.

          In order to retest:

          • use Oracle and MySQL as external database
          • if could you retest with PHP4, it would be great
          • activate Users>Authentication>external database plugin
          • run the command /usr/bin/php -c /etc/php5/apache2/php.ini /pathtomoodle/moodle/auth/db/auth_db_sync_users.php
          • check that it works
          Show
          Jérôme Mouneyrac added a comment - ok I tested with UPPERCASE fieldnames and UPPERCASE table name into the admin form. It still works. I checked the code as well, nothing need to be changed as it needs for MDL-16043 . You can retest this issue. In order to retest: use Oracle and MySQL as external database if could you retest with PHP4, it would be great activate Users>Authentication>external database plugin run the command /usr/bin/php -c /etc/php5/apache2/php.ini /pathtomoodle/moodle/auth/db/auth_db_sync_users.php check that it works
          Hide
          Jérôme Mouneyrac added a comment -

          Angelo:
          We forced the keys of $rec array into lowercase.
          "array_push($result, $rec->username);" will work with Oracle. I tested with Oracle 10g and MySQL.

          Show
          Jérôme Mouneyrac added a comment - Angelo: We forced the keys of $rec array into lowercase. "array_push($result, $rec->username);" will work with Oracle. I tested with Oracle 10g and MySQL.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Adding fixfor versions and closing. Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Adding fixfor versions and closing. Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: