kassissieh [kassissieh]

1. /**
    * Implementation of hook_install().
    */
   function ldapsync_install() {
     $ret = array();
   
     variable_set('ldapsync_time_interval', 999999);
     variable_set('ldapsync_last_sync_time', 0);
     variable_set('ldapsync_filter', '');
     variable_set('ldapsync_missing_users_action', '');
   
     return $ret;
   }
   

   There is no need to set Drupal variables to their default values; default values are passed to variable_get(), which will return the default values if the variable has not been set before.

2.   require_once(drupal_get_path('module', 'ldapauth') .'/includes/LDAPInterface.inc');
   

   If you are going to include the file unconditionally, then you should merge the files.

3.     $result = db_query("SELECT uid, name, data FROM {users} WHERE status=1");
       while ($row = db_fetch_array($result)) {
         if (!in_array($row['name'], array_keys($ldap_users))) {
           $data = unserialize($row['data']);
           if ($data['ldap_authentified']) {
             // block user if appropriate module setting is set
             if (variable_get('ldapsync_missing_users_action','warn') == 'block') {
               $result2 = db_query("UPDATE {users} SET status=0 WHERE uid=%d", $row['uid']);  // do we need to add another check to the WHERE clause just in case? User_load should provide us only one result, right?
               watchdog('ldapsync', 'Disabled LDAP-authentified user '. $row['name'] .' because the corresponding LDAP account does not exist or is disabled.');
             }
             $count_orphaned_users++;
   

   I am not sure the code is handling users in a secure way. I think that users who are blocked should also be logged out too.

4.   $summary = 'Completed LDAP sync. New users: '. $count_new_users .'. LDAP-authentified users that do not have an enabled LDAP account: '. $count_orphaned_users .'.';
   

   The code should use placeholders, not concatenate strings that are translated.

5.       watchdog('ldapsync', 'ldapsync basedn '.$base_dn.'.');
   

   The same is true here.

6.     '#options' => array('warn'=>'warn', 'block'=>'block'),
   

   Strings used in the user interface should be translated.

7.   $op = $form_state['clicked_button']['#value'];
     $values = $form_state['values'];
     switch ($op) {
       case t('Sync now'):
   

   Submission code should not depend on $op. It is possible to have a submission functions that is specific for a single button. That would eliminate the need to know the operation being executed.

To log out a user, you can look at what the function user_logout() does.

function user_logout() {
  global $user;

  watchdog('user', 'Session closed for %name.', array('%name' => $user->name));

  // Destroy the current session:
  session_destroy();
  // Only variables can be passed by reference workaround.
  $null = NULL;
  user_module_invoke('logout', $null, $user);

  // Load the anonymous user
  $user = drupal_anonymous_user();

  drupal_goto();
}

You could load each user you need to logout as $account and use user_module_invoke('logout', $null, $account) to log them out.

e.g.

if (!$ldapresult) continue;

An IF-statement should always be written as

if (!$ldapresult) {
  continue;
}

I apologize on the delay on approving this application.