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

          Attachments

            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