Moodle
  1. Moodle
  2. MDL-31540

auth/ldap: Try to remove duplicates before storing LDAP search contexts

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1
    • Fix Version/s: 2.3
    • Component/s: Authentication, Enrolments
    • Labels:
    • Environment:
      Server: CentOS 6.2 64 bit / LDAP: MS AD Windows 2003 R2 32bit
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      1. Go to the LDAP auth settings configuration page
      2. Specify several contexts (separated by ';'). Make sure you include one or more contexts more than once.
      3. Save the settings.
      4. Go to the LDAP auth settings page again and check that the duplicated contexts are removed (an every context will we lowercased).

      Show
      1. Go to the LDAP auth settings configuration page 2. Specify several contexts (separated by ';'). Make sure you include one or more contexts more than once. 3. Save the settings. 4. Go to the LDAP auth settings page again and check that the duplicated contexts are removed (an every context will we lowercased).
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip_mdl-31540-master
    • Rank:
      38093

      Description

      When I try to use php -f moodle-dir/auth/ldap/sync_users.php I receive the following error:

      Default exception handler: Error writing to database Debug: Duplicate entry '1-moodleadmin' for key 'mdl_tmpextu_mneuse_uix'
      INSERT INTO mdl_tmp_extuser (username,mnethostid) VALUES(?,?)
      [array (
        0 => 'moodleadmin',
        1 => '1',
      )]
      * line 397 of /lib/dml/moodle_database.php: dml_write_exception thrown
      * line 893 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
      * line 939 of /auth/ldap/auth.php: call to mysqli_native_moodle_database->insert_record_raw()
      * line 671 of /auth/ldap/auth.php: call to auth_plugin_ldap->ldap_bulk_insert()
      * line 65 of /auth/ldap/cli/sync_users.php: call to auth_plugin_ldap->sync_users()
      
      !!! Error writing to database !!!
      Potential coding error - existing temptables found when disposing database. Must be dropped!
      

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for reporting that.

        Show
        Michael de Raadt added a comment - Thanks for reporting that.
        Hide
        Michael de Raadt added a comment -

        Hi, Iñaki.

        I've added you as a watcher as I thought you might be interested.

        Show
        Michael de Raadt added a comment - Hi, Iñaki. I've added you as a watcher as I thought you might be interested.
        Hide
        Pedro Crispim added a comment - - edited

        If theres anything else I can do to help, please let me know.

        Show
        Pedro Crispim added a comment - - edited If theres anything else I can do to help, please let me know.
        Hide
        Iñaki Arenaza added a comment -

        Hi Pedro,

        could you paste your current LDAP auth config settings here? (or send them to me privately at iarenaza @ mondragon dot edu) I highly suspect this is a configuration issue, rather than a bug.

        Make sure you remove or obscure any sensitive information like usernames and passwords

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - Hi Pedro, could you paste your current LDAP auth config settings here? (or send them to me privately at iarenaza @ mondragon dot edu) I highly suspect this is a configuration issue, rather than a bug. Make sure you remove or obscure any sensitive information like usernames and passwords Saludos. Iñaki.
        Hide
        Petr Škoda added a comment -

        Reassigning to our ldap guru, thanks a lot.

        Show
        Petr Škoda added a comment - Reassigning to our ldap guru, thanks a lot.
        Hide
        Iñaki Arenaza added a comment -

        It looks like it was a configuration issue (specifying a context twice in the list of contexts).

        It would be a good idea to automatically detect and remove duplicates before storing the contexts, but I need to investigate a bit. As the user can specify the contexts using different cases, we would need to compare then in a case-insensitive way. But I don't know if LDAP contexts are case-insensitive for all LDAP servers, so I need to check the relevant standards before I propose anything about this.

        I'll left the bug open until I check the standards.

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - It looks like it was a configuration issue (specifying a context twice in the list of contexts). It would be a good idea to automatically detect and remove duplicates before storing the contexts, but I need to investigate a bit. As the user can specify the contexts using different cases, we would need to compare then in a case-insensitive way. But I don't know if LDAP contexts are case-insensitive for all LDAP servers, so I need to check the relevant standards before I propose anything about this. I'll left the bug open until I check the standards. Saludos. Iñaki.
        Hide
        Iñaki Arenaza added a comment -

        Hi again,

        according to my reading of RFC-4517 (section 4.2.15. distinguishedNameMatch) and RFC-4519 (where the EQUAILITY property is defined for the different user application attribute types), we could use case-insensitive comparisons. The attribute types used in 99.999% of the distinguished names used for contexts out there (that is: dc, ou, cn, o, l and c) use case insentive match rules.

        So I'd say the strategy outlined before (detect and remove duplicates on settings' saving) is 100% feasible.

        What do you think Petr?

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - Hi again, according to my reading of RFC-4517 (section 4.2.15. distinguishedNameMatch) and RFC-4519 (where the EQUAILITY property is defined for the different user application attribute types), we could use case-insensitive comparisons. The attribute types used in 99.999% of the distinguished names used for contexts out there (that is: dc, ou, cn, o, l and c) use case insentive match rules. So I'd say the strategy outlined before (detect and remove duplicates on settings' saving) is 100% feasible. What do you think Petr? Saludos. Iñaki.
        Hide
        Petr Škoda added a comment -

        hehe, I do not understand it much, but I trust 100% your judgement, +1

        Show
        Petr Škoda added a comment - hehe, I do not understand it much, but I trust 100% your judgement, +1
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi Iñaki,

        the code to ensure unique contexts looks 99% perfect. Just 2 small thing:

        1- Under 22_STABLE and master, the moodle_strtolower function is deprecated and static textlib::strtolower() should be be used instead. Under 21_STABLE, the function is ok.

        2- I always "pair" explode() with implode(). You are using join() instead. No big problem, just noticing it.

        As far as the default targets for this are 21, 22 and master (unless you state another thing), it would be great to get that 1 amended.

        So I'm reopening this...thanks and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi Iñaki, the code to ensure unique contexts looks 99% perfect. Just 2 small thing: 1- Under 22_STABLE and master, the moodle_strtolower function is deprecated and static textlib::strtolower() should be be used instead. Under 21_STABLE, the function is ok. 2- I always "pair" explode() with implode(). You are using join() instead. No big problem, just noticing it. As far as the default targets for this are 21, 22 and master (unless you state another thing), it would be great to get that 1 amended. So I'm reopening this...thanks and ciao
        Hide
        Iñaki Arenaza added a comment -

        Hi Eloy,

        1. I was proposing this for master only as it's not actually a bugfix, but an improvement to avoid some incorrect configurations (and I thought the policy was: improvements and new features in master only). So I've changed moodle_strtolower to textlib::strlower() as suggested.

        2. Replaced join() with implode()

        I've published the amended commit in a new branch (and will update the branch details in the bug in a moment).

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - Hi Eloy, 1. I was proposing this for master only as it's not actually a bugfix, but an improvement to avoid some incorrect configurations (and I thought the policy was: improvements and new features in master only). So I've changed moodle_strtolower to textlib::strlower() as suggested. 2. Replaced join() with implode() I've published the amended commit in a new branch (and will update the branch details in the bug in a moment). Saludos. Iñaki.
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Dan Poltawski added a comment -

        Thanks, this has been integrated now.

        Iñaki - it seemed like your change hadn't been based anywhere close to current master as there were conflicts with the strlower changes, however I fixed them. (Just FYI in case you thought you had rebased against master)

        Show
        Dan Poltawski added a comment - Thanks, this has been integrated now. Iñaki - it seemed like your change hadn't been based anywhere close to current master as there were conflicts with the strlower changes, however I fixed them. (Just FYI in case you thought you had rebased against master)
        Hide
        Rossiani Wijaya added a comment -

        This is working great.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This is working great. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This has been near becoming rejected, because it's not the best code you are able to produce.

        But, luckily, at the end, it has landed and has been spread to all repos out there.

        Many thanks and, don't forget it, keep improving your skills, you can!

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: