0011554: improve ldap account status handling
authorPhilipp Schüle <p.schuele@metaways.de>
Mon, 18 Jan 2016 16:47:23 +0000 (17:47 +0100)
committerPhilipp Schüle <p.schuele@metaways.de>
Wed, 20 Jan 2016 11:48:55 +0000 (12:48 +0100)
* improves current user status detection in LDAP backend
* allows to configure account status sync in syncUser()
* refactors syncUser() and LDAP status handling

https://forge.tine20.org/view.php?id=11554

Change-Id: I68424aee8ed4dac90f0e1e12b8ebb7a4cd812559
Reviewed-on: http://gerrit.tine20.com/customers/2594
Tested-by: Jenkins CI (http://ci.tine20.com/)
Reviewed-by: Philipp Schüle <p.schuele@metaways.de>
Tested-by: Philipp Schüle <p.schuele@metaways.de>
tests/tine20/Tinebase/User/LdapTest.php
tine20/Setup/Frontend/Cli.php
tine20/Setup/Server/Cli.php
tine20/Tinebase/User.php
tine20/Tinebase/User/Ldap.php
tine20/Tinebase/User/Sql.php

index 2d8d2e6..f3c9b86 100644 (file)
@@ -63,7 +63,7 @@ class Tinebase_User_LdapTest extends TestCase
      */
     public function testGetUsers()
     {
-        $user = $this->testAddUser();
+        $this->testAddUser();
         
         $users = $this->_backend->getUsers('phpunit', 'accountStatus');
         
@@ -112,6 +112,7 @@ class Tinebase_User_LdapTest extends TestCase
         $groupsBeforeUpdate = $groupsBackend->getGroupMembershipsFromSyncBackend($user);
         
         $user->accountLoginName = 'tine20phpunituser-updated';
+        $this->_usernamesToDelete[] = $user->accountLoginName;
         $testUser = $this->_backend->updateUser($user);
         $groupsAfterUpdate = $groupsBackend->getGroupMembershipsFromSyncBackend($testUser);
         
@@ -210,7 +211,7 @@ class Tinebase_User_LdapTest extends TestCase
         
         $this->setExpectedException('Tinebase_Exception_NotFound');
         
-        $testUser = $this->_backend->getUserById($user, 'Tinebase_Model_FullUser');
+        $this->_backend->getUserById($user, 'Tinebase_Model_FullUser');
     }
     
     /**
@@ -300,4 +301,30 @@ class Tinebase_User_LdapTest extends TestCase
             $this->assertContains('Addressbook_Model_Contact record with id = ' . $sqlUser->contact_id, $tenf->getMessage());
         }
     }
+
+    /**
+     * test user status sync tine <-> ldap
+     *
+     * @see 0011554: improve ldap account status handling
+     */
+    public function testSyncUserStatus()
+    {
+        $user = $this->testAddUser();
+
+        // set user status in tine (disabled, expired, enabled)
+        $statusToTest = array(
+            Tinebase_Model_User::ACCOUNT_STATUS_EXPIRED,
+            Tinebase_Model_User::ACCOUNT_STATUS_DISABLED,
+            Tinebase_Model_User::ACCOUNT_STATUS_EXPIRED,
+            Tinebase_Model_User::ACCOUNT_STATUS_ENABLED,
+        );
+
+        foreach($statusToTest as $status) {
+            Tinebase_User::getInstance()->setStatus($user, $status);
+
+            // sync user -> user status should be the same
+            $syncedUser = Tinebase_User::syncUser($user, array('syncAccountStatus' => true));
+            $this->assertEquals($status, $syncedUser->accountStatus, print_r($syncedUser->toArray(), true));
+        }
+    }
 }
index 16cbd6e..e2ad2fc 100644 (file)
@@ -435,6 +435,10 @@ class Setup_Frontend_Cli
             $options['deleteUsers'] = true;
         }
 
+        if ($_opts->syncaccountstatus) {
+            $options['syncAccountStatus'] = true;
+        }
+
         Tinebase_User::syncUsers($options);
     }
     
index d20d261..388e104 100644 (file)
@@ -48,6 +48,7 @@ class Setup_Server_Cli implements Tinebase_Server_Interface
                     'dbmailldap'            => 'Only usable with sync_accounts_from_ldap. Fetches dbmail email user data from LDAP.',
                     'onlyusers'             => 'Only usable with sync_accounts_from_ldap. Fetches only users and no groups from LDAP.',
                     'syncdeletedusers'      => 'Only usable with sync_accounts_from_ldap. Removes users from Tine 2.0 DB that no longer exist in LDAP',
+                    'syncaccountstatus'     => 'Only usable with sync_accounts_from_ldap. Synchronizes current account status from LDAP',
                 'sync_passwords_from_ldap'  => 'Synchronize user passwords from ldap',
                 'egw14import'               => 'Import user and groups from egw14
                          Examples: 
index ac5c60c..1945a5c 100644 (file)
@@ -419,7 +419,7 @@ class Tinebase_User
     }
     
     /**
-     * syncronize user from syncbackend to local sql backend
+     * synchronize user from syncbackend to local sql backend
      * 
      * @param  mixed  $username  the login id of the user to synchronize
      * @param  array $options
@@ -428,7 +428,6 @@ class Tinebase_User
      * 
      * @todo make use of dbmail plugin configurable (should be false by default)
      * @todo switch to new primary group if it could not be found
-     * @todo write a test and refactor this ... :(
      */
     public static function syncUser($username, $options = array())
     {
@@ -461,38 +460,8 @@ class Tinebase_User
         self::getPrimaryGroupForUser($user);
 
         try {
-            $currentUser = $userBackend->getUserByProperty('accountId', $user, 'Tinebase_Model_FullUser');
-        
-            $currentUser->accountLoginName          = $user->accountLoginName;
-            $currentUser->accountLastPasswordChange = $user->accountLastPasswordChange;
-            $currentUser->accountExpires            = $user->accountExpires;
-            $currentUser->accountPrimaryGroup       = $user->accountPrimaryGroup;
-            $currentUser->accountDisplayName        = $user->accountDisplayName;
-            $currentUser->accountLastName           = $user->accountLastName;
-            $currentUser->accountFirstName          = $user->accountFirstName;
-            $currentUser->accountFullName           = $user->accountFullName;
-            $currentUser->accountEmailAddress       = $user->accountEmailAddress;
-            $currentUser->accountHomeDirectory      = $user->accountHomeDirectory;
-            $currentUser->accountLoginShell         = $user->accountLoginShell;
-
-            $currentUser->accountStatus             = isset($user->accountStatus)
-                ? $user->accountStatus
-                : Tinebase_Model_User::ACCOUNT_STATUS_ENABLED;
-
-            if (! empty($user->visibility) && $currentUser->visibility !== $user->visibility) {
-                $currentUser->visibility            = $user->visibility;
-                if (empty($currentUser->contact_id) && $currentUser->visibility == Tinebase_Model_FullUser::VISIBILITY_DISPLAYED) {
-                    self::createContactForSyncedUser($currentUser);
-                }
-            }
-            
-            Tinebase_Timemachine_ModificationLog::setRecordMetaData($currentUser, 'update');
-            $syncedUser = $userBackend->updateUserInSqlBackend($currentUser);
-            if (! empty($user->container_id)) {
-                $syncedUser->container_id = $user->container_id;
-            }
-            $userBackend->updatePluginUser($syncedUser, $user);
-            
+            $syncedUser = self::_syncDataAndUpdateUser($user, $options);
+
         } catch (Tinebase_Exception_NotFound $ten) {
             try {
                 $invalidUser = $userBackend->getUserByPropertyFromSqlBackend('accountLoginName', $username, 'Tinebase_Model_FullUser');
@@ -519,6 +488,53 @@ class Tinebase_User
     }
 
     /**
+     * sync account data and update
+     *
+     * @param Tinebase_Model_FullUser $user
+     * @param array $options
+     * @return mixed
+     * @throws Tinebase_Exception_InvalidArgument
+     */
+    protected static function _syncDataAndUpdateUser($user, $options)
+    {
+        $currentUser = Tinebase_User::getInstance()->getUserByProperty('accountId', $user, 'Tinebase_Model_FullUser');
+
+        $fieldsToSync = array('accountLoginName', 'accountLastPasswordChange', 'accountExpires', 'accountPrimaryGroup',
+            'accountDisplayName', 'accountLastName', 'accountFirstName', 'accountFullName', 'accountEmailAddress',
+            'accountHomeDirectory', 'accountLoginShell');
+        if (isset($options['syncAccountStatus']) && $options['syncAccountStatus']) {
+            $fieldsToSync[] = 'accountStatus';
+        }
+        $recordNeedsUpdate = false;
+        foreach ($fieldsToSync as $field) {
+            if ($currentUser->{$field} !== $user->{$field}) {
+                $currentUser->{$field} = $user->{$field};
+                $recordNeedsUpdate = true;
+            }
+        }
+
+        if (! empty($user->visibility) && $currentUser->visibility !== $user->visibility) {
+            $currentUser->visibility            = $user->visibility;
+            if (empty($currentUser->contact_id) && $currentUser->visibility == Tinebase_Model_FullUser::VISIBILITY_DISPLAYED) {
+                self::createContactForSyncedUser($currentUser);
+            }
+        }
+
+        if ($recordNeedsUpdate) {
+            Tinebase_Timemachine_ModificationLog::setRecordMetaData($currentUser, 'update');
+            $syncedUser = Tinebase_User::getInstance()->updateUserInSqlBackend($currentUser);
+        } else {
+            $syncedUser = $currentUser;
+        }
+        if (! empty($user->container_id)) {
+            $syncedUser->container_id = $user->container_id;
+        }
+        Tinebase_User::getInstance()->updatePluginUser($syncedUser, $user);
+
+        return $syncedUser;
+    }
+
+    /**
      * import contact data(phone, address, fax, birthday. photo)
      * 
      * @param Tinebase_Model_FullUser $syncedUser
index e1bca98..4d52fc1 100644 (file)
@@ -5,7 +5,7 @@
  * @package     Tinebase
  * @subpackage  User
  * @license     http://www.gnu.org/licenses/agpl.html AGPL Version 3
- * @copyright   Copyright (c) 2007-2014 Metaways Infosystems GmbH (http://www.metaways.de)
+ * @copyright   Copyright (c) 2007-2016 Metaways Infosystems GmbH (http://www.metaways.de)
  * @author      Lars Kneschke <l.kneschke@metaways.de>
  */
 
@@ -405,8 +405,27 @@ class Tinebase_User_Ldap extends Tinebase_User_Sql implements Tinebase_User_Inte
         }
         
         $metaData = $this->_getMetaData($_accountId);
+        $ldapData = $this->_getUserStatusValues($_status);
 
-        if ($_status == 'disabled') {
+        foreach ($this->_ldapPlugins as $plugin) {
+            $plugin->inspectStatus($_status, $ldapData);
+        }
+
+        if (Tinebase_Core::isLogLevel(Zend_Log::TRACE)) Tinebase_Core::getLogger()->trace(__METHOD__ . '::' . __LINE__ .
+            " {$metaData['dn']}  ldapData: " . print_r($ldapData, true));
+
+        $this->_ldap->update($metaData['dn'], $ldapData);
+    }
+
+    /**
+     * get LDAP user status values depending on tine20 status
+     *
+     * @param $status one of expired, enabled, disabled
+     * @return array
+     */
+    protected function _getUserStatusValues($status)
+    {
+        if ($status == Tinebase_Model_User::ACCOUNT_STATUS_DISABLED) {
             $ldapData = array(
                 'shadowMax'      => 1,
                 'shadowInactive' => 1
@@ -416,15 +435,13 @@ class Tinebase_User_Ldap extends Tinebase_User_Sql implements Tinebase_User_Inte
                 'shadowMax'      => 999999,
                 'shadowInactive' => array()
             );
+            if ($status == Tinebase_Model_User::ACCOUNT_STATUS_ENABLED) {
+                // remove expiry setting
+                $ldapData['shadowexpire'] = array();
+            }
         }
 
-        foreach ($this->_ldapPlugins as $plugin) {
-            $plugin->inspectStatus($_status, $ldapData);
-        }
-
-        if (Tinebase_Core::isLogLevel(Zend_Log::TRACE)) Tinebase_Core::getLogger()->trace(__METHOD__ . '::' . __LINE__ . " {$metaData['dn']}  $ldapData: " . print_r($ldapData, true));
-
-        $this->_ldap->update($metaData['dn'], $ldapData);
+        return $ldapData;
     }
 
     /**
@@ -454,7 +471,8 @@ class Tinebase_User_Ldap extends Tinebase_User_Sql implements Tinebase_User_Inte
             $plugin->inspectExpiryDate($_expiryDate, $ldapData);
         }
 
-        if (Tinebase_Core::isLogLevel(Zend_Log::TRACE)) Tinebase_Core::getLogger()->trace(__METHOD__ . '::' . __LINE__ . " {$metaData['dn']}  $ldapData: " . print_r($ldapData, true));
+        if (Tinebase_Core::isLogLevel(Zend_Log::TRACE)) Tinebase_Core::getLogger()->trace(__METHOD__ . '::' . __LINE__
+            . " {$metaData['dn']}  ldapData: " . print_r($ldapData, true));
 
         $this->_ldap->update($metaData['dn'], $ldapData);
     }
@@ -481,11 +499,13 @@ class Tinebase_User_Ldap extends Tinebase_User_Sql implements Tinebase_User_Inte
             $plugin->inspectUpdateUser($_account, $ldapData, $ldapEntry);
         }
         
-        // no need to update this attribute, it's not allowed to change and even might not be updateable
+        // no need to update this attribute, it's not allowed to change and even might not be update-able
         unset($ldapData[$this->_userUUIDAttribute]);
         
-        if (Tinebase_Core::isLogLevel(Zend_Log::DEBUG)) Tinebase_Core::getLogger()->debug(__METHOD__ . '::' . __LINE__ . '  $dn: ' . $ldapEntry['dn']);
-        if (Tinebase_Core::isLogLevel(Zend_Log::TRACE)) Tinebase_Core::getLogger()->trace(__METHOD__ . '::' . __LINE__ . '  $ldapData: ' . print_r($ldapData, true));
+        if (Tinebase_Core::isLogLevel(Zend_Log::DEBUG)) Tinebase_Core::getLogger()->debug(__METHOD__ . '::' . __LINE__
+            . ' DN: ' . $ldapEntry['dn']);
+        if (Tinebase_Core::isLogLevel(Zend_Log::TRACE)) Tinebase_Core::getLogger()->trace(__METHOD__ . '::' . __LINE__
+            . ' LDAP data: ' . print_r($ldapData, true));
         
         $this->_ldap->update($ldapEntry['dn'], $ldapData);
         
@@ -496,7 +516,7 @@ class Tinebase_User_Ldap extends Tinebase_User_Sql implements Tinebase_User_Inte
         if (isset($ldapData[key($rdn)]) && $rdn[key($rdn)] != $ldapData[key($rdn)]) {
             $groupsBackend = Tinebase_Group::factory(Tinebase_Group::LDAP);
             
-            // get the current groupmemberships
+            // get the current group memberships
             $memberships = $groupsBackend->getGroupMembershipsFromSyncBackend($_account);
             
             // remove the user from current groups, because the dn/uid has changed
@@ -627,7 +647,9 @@ class Tinebase_User_Ldap extends Tinebase_User_Sql implements Tinebase_User_Inte
         $attributes[] = 'objectclass';
         $attributes[] = 'uidnumber';
         $attributes[] = 'useraccountcontrol';
-        
+        // needed for account status handling (shadowmax: days after which password must be changed)
+        $attributes[] = 'shadowmax';
+
         if (Tinebase_Core::isLogLevel(Zend_Log::DEBUG)) 
             Tinebase_Core::getLogger()->debug(__METHOD__ . '::' . __LINE__ . ' filter ' . $filter);
         if (Tinebase_Core::isLogLevel(Zend_Log::TRACE)) 
@@ -823,26 +845,21 @@ class Tinebase_User_Ldap extends Tinebase_User_Sql implements Tinebase_User_Inte
                             // account does not expire
                             $accountArray[$keyMapping] = null;
                         } else {
-                            $accountArray[$keyMapping] = new Tinebase_DateTime(($shadowExpire < 100000) ? $shadowExpire * 86400 : $shadowExpire);
+                            $accountArray[$keyMapping] = new Tinebase_DateTime(($shadowExpire < 100000)
+                                ? $shadowExpire * 86400
+                                : $shadowExpire);
                         }
                         break;
-                        
+
+                    // shadowInactive
                     case 'accountStatus':
-                        if ((isset($_userData['shadowmax']) || array_key_exists('shadowmax', $_userData)) && (isset($_userData['shadowinactive']) || array_key_exists('shadowinactive', $_userData))) {
-                            $lastChange = (isset($_userData['shadowlastchange']) || array_key_exists('shadowlastchange', $_userData)) ? $_userData['shadowlastchange'] : 0;
-                            if (($lastChange + $_userData['shadowmax'] + $_userData['shadowinactive']) * 86400 <= Tinebase_DateTime::now()->getTimestamp()) {
-                                $accountArray[$keyMapping] = 'enabled';
-                            } else {
-                                $accountArray[$keyMapping] = 'disabled';
-                            }
-                        } else {
-                            $accountArray[$keyMapping] = 'enabled';
+                        if ($this->_isUserDisabled($_userData)) {
+                            $accountArray[$keyMapping] = Tinebase_Model_User::ACCOUNT_STATUS_DISABLED;
                         }
                         break;
 
                     case 'accountId':
                         $accountArray[$keyMapping] = $this->_decodeAccountId($value[0]);
-                        
                         break;
                         
                     default:
@@ -856,14 +873,59 @@ class Tinebase_User_Ldap extends Tinebase_User_Sql implements Tinebase_User_Inte
             $accountArray['accountLastName'] = $accountArray['accountFullName'];
         }
         if ($errors) {
-            Tinebase_Core::getLogger()->warn(__METHOD__ . '::' . __LINE__ . ' Could not instantiate account object for ldap user ' . print_r($_userData, 1));
+            Tinebase_Core::getLogger()->warn(__METHOD__ . '::' . __LINE__ . ' Could not instantiate account object for ldap user '
+                . print_r($_userData, 1));
             $accountObject = null;
         } else {
             $accountObject = new $_accountClass($accountArray, TRUE);
         }
 
+        // normalize account status
+        if ($accountObject instanceof Tinebase_Model_FullUser) {
+            if (!isset($accountObject->accountStatus)) {
+                $accountObject->accountStatus = Tinebase_Model_User::ACCOUNT_STATUS_ENABLED;
+            }
+            if ($accountObject->accountExpires &&
+                $accountObject->accountExpires->isEarlier(Tinebase_DateTime::now()) &&
+                $accountObject->accountStatus === Tinebase_Model_User::ACCOUNT_STATUS_ENABLED
+            ) {
+                $accountObject->accountStatus = Tinebase_Model_User::ACCOUNT_STATUS_EXPIRED;
+            }
+        }
+
         return $accountObject;
     }
+
+    /**
+     * check if user is disabled in LDAP
+     *
+     * @param array $ldapData
+     * @return bool
+     *
+     * TODO fix/improve LDAP disabled user detection
+     */
+    protected function _isUserDisabled($ldapData)
+    {
+        if ((isset($ldapData['shadowmax']) || array_key_exists('shadowmax', $ldapData))) {
+            // FIXME this is very strange code!
+//            $lastChange = (isset($ldapData['shadowlastchange']) || array_key_exists('shadowlastchange', $ldapData)) ? $ldapData['shadowlastchange'][0] : 0;
+//            if (($lastChange + $ldapData['shadowmax'][0] + $ldapData['shadowinactive'][0]) * 86400
+//                <= Tinebase_DateTime::now()->getTimestamp()
+//            ) {
+//                return false;
+//            } else {
+//                return true;
+//            }
+
+            // this is what tine sets for disabled accounts
+            if (isset($ldapData['shadowmax'][0]) && $ldapData['shadowmax'][0] == 1 &&
+                isset($ldapData['shadowinactive'][0]) && $ldapData['shadowinactive'][0] == 1
+            ) {
+                return true;
+            }
+        }
+        return false;
+    }
     
     /**
      * returns properties of last user fetched from sync backend
@@ -876,7 +938,7 @@ class Tinebase_User_Ldap extends Tinebase_User_Sql implements Tinebase_User_Inte
     }
     
     /**
-     * helper function to be overwriten in subclasses
+     * helper function to be overwritten in subclasses
      * 
      * @param  string  $accountId
      * @return string
@@ -887,7 +949,7 @@ class Tinebase_User_Ldap extends Tinebase_User_Sql implements Tinebase_User_Inte
     }
 
     /**
-     * helper function to be overwriten in subclasses
+     * helper function to be overwritten in subclasses
      * 
      * @param  string  $accountId
      * @return string
@@ -967,13 +1029,7 @@ class Tinebase_User_Ldap extends Tinebase_User_Sql implements Tinebase_User_Inte
                         $ldapData[$ldapProperty] = $value instanceof DateTime ? floor($value->getTimestamp() / 86400) : array();
                         break;
                     case 'accountStatus':
-                        if ($value == 'enabled') {
-                            $ldapData['shadowMax']      = 999999;
-                            $ldapData['shadowInactive'] = array();
-                        } else {
-                            $ldapData['shadowMax']      = 1;
-                            $ldapData['shadowInactive'] = 1;
-                        }
+                        $ldapData = array_merge($ldapData, $this->_getUserStatusValues($value));
                         break;
                     case 'accountPrimaryGroup':
                         $ldapData[$ldapProperty] = Tinebase_Group::getInstance()->resolveUUIdToGIdNumber($value);
index fadf34d..2535a35 100644 (file)
@@ -574,25 +574,30 @@ class Tinebase_User_Sql extends Tinebase_User_Abstract
      */
     public function setStatus($_accountId, $_status)
     {
-        if($this instanceof Tinebase_User_Interface_SyncAble) {
+        if ($this instanceof Tinebase_User_Interface_SyncAble) {
             $this->setStatusInSyncBackend($_accountId, $_status);
         }
         
         $accountId = Tinebase_Model_User::convertUserIdToInt($_accountId);
         
         switch($_status) {
-            case 'enabled':
+            case Tinebase_Model_User::ACCOUNT_STATUS_ENABLED:
                 $accountData[$this->rowNameMapping['loginFailures']]  = 0;
                 $accountData[$this->rowNameMapping['accountExpires']] = null;
                 $accountData['status'] = $_status;
                 break;
                 
-            case 'disabled':
+            case Tinebase_Model_User::ACCOUNT_STATUS_DISABLED:
                 $accountData['status'] = $_status;
                 break;
                 
-            case 'expired':
-                $accountData['expires_at'] = Tinebase_DateTime::now()->getTimestamp();
+            case Tinebase_Model_User::ACCOUNT_STATUS_EXPIRED:
+                $expiryDate = Tinebase_DateTime::now()->subSecond(1);
+                $accountData['expires_at'] = $expiryDate->toString();
+                if ($this instanceof Tinebase_User_Interface_SyncAble) {
+                    $this->setExpiryDateInSyncBackend($_accountId, $expiryDate);
+                }
+
                 break;
             
             default:
@@ -619,7 +624,7 @@ class Tinebase_User_Sql extends Tinebase_User_Abstract
     */
     public function setExpiryDate($_accountId, $_expiryDate)
     {
-        if($this instanceof Tinebase_User_Interface_SyncAble) {
+        if ($this instanceof Tinebase_User_Interface_SyncAble) {
             $this->setExpiryDateInSyncBackend($_accountId, $_expiryDate);
         }