Moodle

CLONE -auth_ldap_bulk_insert() potential problem for MSSQL and Oracle

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.7
  • Fix Version/s: 1.8
  • Component/s: Authentication
  • Labels:
    None
  • Database:
    Microsoft SQL, Oracle
  • Affected Branches:
    MOODLE_17_STABLE
  • Fixed Branches:
    MOODLE_18_STABLE

Description

In auth_sync_users() we user auth_ldap_bulk_insert() to insert temporary user data into the database. Depending on the underlying database, we insert one value at a time, or a number of them (1000 by default).

Inside the function, in order to quote all the values before sending them to the database, we execute the following code:

// make those values safe
array_map('addslashes', $users);

but this is wrong. array_map() doesn't modify the array argument, but instead it returns a modified array (see http://www.php.net/array_map). So the code should be:

// make those values safe
$users = array_map('addslashes', $users);

If there is a username with a single quote in it, the value is not scaped and the insert fails silently (we don't check the return code in the next execute_sql() call). This in turn makes a lot of external users 'inexistent' and their corresponding internal users are deleted from Moodle.

See http://moodle.org/mod/forum/discuss.php?d=59753 for more details on this.

By the way, currently auth_sync_users() just tests for MySQL and Postgresql explicitly, and set bulk_insert_records = 1 just for Postgresl, so Oracle and MS SQL get the defauknowledge
lt of 1000. I don't know if Oracle and MS SQL allow the extended insert syntax or not, so anyone who knows Oracle and MS SQL better should have a look at it.

Saludos. Iñaki.

Issue Links

Activity

Hide
Petr Škoda (skodak) added a comment -

assigning to our db guru
the array_map problem is already fixed..

Show
Petr Škoda (skodak) added a comment - assigning to our db guru the array_map problem is already fixed..
Hide
Iñaki Arenaza added a comment -

This same issue (bulk insertion with non-MySQL dbs) is also addressed in MDL-7525. There is even a proposed patch there.

Saludos. Iñaki.

Show
Iñaki Arenaza added a comment - This same issue (bulk insertion with non-MySQL dbs) is also addressed in MDL-7525. There is even a proposed patch there. Saludos. Iñaki.
Hide
Eloy Lafuente (stronk7) added a comment -

Later but getting this right now.

Show
Eloy Lafuente (stronk7) added a comment - Later but getting this right now.
Hide
Martin Dougiamas added a comment -

Thanks, Eloy, this seems like a big problem for people.

Show
Martin Dougiamas added a comment - Thanks, Eloy, this seems like a big problem for people.
Hide
Eloy Lafuente (stronk7) added a comment -

Changes committed both to 18_STABLE and HEAD. Bulk insert aren't allowed by anyone but MySQL . Should be working now, together with MDL-7525 and MDL-8153.

It would be great if you can test it both under MSSQL and Oracle ! (my Oracle box is broken and I'm missing LDAP here too).

Show
Eloy Lafuente (stronk7) added a comment - Changes committed both to 18_STABLE and HEAD. Bulk insert aren't allowed by anyone but MySQL . Should be working now, together with MDL-7525 and MDL-8153. It would be great if you can test it both under MSSQL and Oracle ! (my Oracle box is broken and I'm missing LDAP here too).
Hide
Eloy Lafuente (stronk7) added a comment -

Working under MySQL, PostgreSQL and MSSQL.

Only Oracle testing is pending. Feedback will be much appreciated.

Show
Eloy Lafuente (stronk7) added a comment - Working under MySQL, PostgreSQL and MSSQL. Only Oracle testing is pending. Feedback will be much appreciated.

People

Dates

  • Created:
    Updated:
    Resolved: