Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37088

bad operator in ldap "sync_user" method

    Details

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks, Julien and Iñaki.

            Show
            salvetore Michael de Raadt added a comment - Thanks, Julien and Iñaki.
            Hide
            poltawski 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
            poltawski 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
            jboulen 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
            jboulen 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
            iarenaza 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
            iarenaza 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
            samhemelryk 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
            samhemelryk 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
            phalacee Jason Fowler added a comment -

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

            Show
            phalacee Jason Fowler added a comment - Hey Inaki, Could I please get some testing instructions for this issue?
            Hide
            iarenaza 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
            iarenaza 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
            phalacee Jason Fowler added a comment -

            works fine, thanks Inaki!

            Show
            phalacee Jason Fowler added a comment - works fine, thanks Inaki!
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  14/Jan/13