Moodle
  1. Moodle
  2. MDL-37088

bad operator in ldap "sync_user" method

    Details

    • Rank:
      46645

      Description

      Hello,

      There is a little mistake here :
      http://git.moodle.org/gw?p=moodle.git;a=blob;f=auth/ldap/auth.php;h=fed0bae7aadf466717deba63f921425e3f63097b;hb=HEAD#l714

      Operator should be != or variables must be casted (because one is int and other is string).

      Currently this condition always return true. Fortunately there is no bad effect, just a resource/time waste.

      Thanks.

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks, Julien and Iñaki.

        Show
        Michael de Raadt added a comment - Thanks, Julien and Iñaki.
        Hide
        Dan Poltawski added a comment -

        Am I right in thinking this means users will have never been deleted using LDAP? In which case, the implications of this change could be huge? Once its fixed many users could be deleted?

        Show
        Dan Poltawski added a comment - Am I right in thinking this means users will have never been deleted using LDAP? In which case, the implications of this change could be huge? Once its fixed many users could be deleted?
        Hide
        Julien Boulen added a comment -

        Not exactly. Before the fix even if you choose "keep internal users", the sql query to find who wasn't anymore in your ldap was executed. Now, and only for this option, this part of code is skipped.

        For "suspend" and "delete" options, there is no change.

        Thanks Iñaki for commit so quickly.

        Show
        Julien Boulen added a comment - Not exactly. Before the fix even if you choose "keep internal users", the sql query to find who wasn't anymore in your ldap was executed. Now, and only for this option, this part of code is skipped. For "suspend" and "delete" options, there is no change. Thanks Iñaki for commit so quickly.
        Hide
        Iñaki Arenaza added a comment -

        Hi Dan,

        as Julien points out, no users are deleted unless the config setting was set to FULLDELETE o SUSPEND. There's and additional check inside the foreach loop that tests for this condition, and only performs any action if the setting is set to FULLDELETE or SUSPEND. Otherwise it just loops over the retrieved users, doing nothing at all (except waste server resources, as Julien pointed out as well).

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - Hi Dan, as Julien points out, no users are deleted unless the config setting was set to FULLDELETE o SUSPEND. There's and additional check inside the foreach loop that tests for this condition, and only performs any action if the setting is set to FULLDELETE or SUSPEND. Otherwise it just loops over the retrieved users, doing nothing at all (except waste server resources, as Julien pointed out as well). Saludos. Iñaki.
        Hide
        Sam Hemelryk added a comment -

        Hi guys,

        Thanks for clarifying that, all looks to make good sense.
        Has been integrated now

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Hi guys, Thanks for clarifying that, all looks to make good sense. Has been integrated now Many thanks Sam
        Hide
        Jason Fowler added a comment -

        Hey Inaki, Could I please get some testing instructions for this issue?

        Show
        Jason Fowler added a comment - Hey Inaki, Could I please get some testing instructions for this issue?
        Hide
        Iñaki Arenaza added a comment -

        Hi Jason,

        you just need to run auth/ldap/cli/sync_user.php (once you have configured LDAP auth plugin to Keep internal users). There's almost no observable difference from the outside, just a slightly longer execution time and higher cpu and memory consumption.

        The only "visual" difference is that without the fix Moodle will print "User entries to be removed: nnn" (or "No user entries to be removed" if all the internal users are still in the external LDAP server) as part of the execution output, while with the fix it won't print any of those two strings.

        Saludos.
        Iñaki.

        Show
        Iñaki Arenaza added a comment - Hi Jason, you just need to run auth/ldap/cli/sync_user.php (once you have configured LDAP auth plugin to Keep internal users). There's almost no observable difference from the outside, just a slightly longer execution time and higher cpu and memory consumption. The only "visual" difference is that without the fix Moodle will print "User entries to be removed: nnn" (or "No user entries to be removed" if all the internal users are still in the external LDAP server) as part of the execution output, while with the fix it won't print any of those two strings. Saludos. Iñaki.
        Hide
        Jason Fowler added a comment -

        works fine, thanks Inaki!

        Show
        Jason Fowler added a comment - works fine, thanks Inaki!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: