relax strict getHome behaviour for LDAP users in a shadow state

* simplifies deletion process
* less strange behaviour when looking up home storage (as long as it is local)
* thus could enable transfer ownerships after user went invisible on ldap

backport of #17717

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
decouple userExists from userExistsOnLDAP check

allows to mark users as offline right away, avoids a gap of being not a
user and causing weird side effects

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
adjust tests

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
remove superfluous tests

- user_ldap is not exposed to public api, it is always behind ldap_proxy
- this is too much for a unit test
- integration tests cover userExists implicitly

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
ensure that only valid group members are returned

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
index 0c4b7182ec47f35ba0171b911e05882bbe437ca2..6d9acd79d58cf8fd3fc1fe3e13497930ace31096 100644 (file)
--- a/apps/user_ldap/lib/Group_LDAP.php
+++ b/apps/user_ldap/lib/Group_LDAP.php
@@ -811,6 +811,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
         * @param int $limit
         * @param int $offset
         * @return array with user ids
+        * @throws \Exception
         */
        public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
                if(!$this->enabled) {
@@ -862,7 +863,10 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
                                //we got uids, need to get their DNs to 'translate' them to user names
                                $filter = $this->access->combineFilterWithAnd(array(
                                        str_replace('%uid', trim($member), $this->access->connection->ldapLoginFilter),
-                                       $this->access->getFilterPartForUserSearch($search)
+                                       $this->access->combineFilterWithAnd([
+                                               $this->access->getFilterPartForUserSearch($search),
+                                               $this->access->connection->ldapUserFilter
+                                       ])
                                ));
                                $ldap_users = $this->access->fetchListOfUsers($filter, $attrs, 1);
                                if(count($ldap_users) < 1) {
@@ -871,17 +875,32 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
                                $groupUsers[] = $this->access->dn2username($ldap_users[0]['dn'][0]);
                        } else {
                                //we got DNs, check if we need to filter by search or we can give back all of them
-                               if ($search !== '') {
-                                       if(!$this->access->readAttribute($member,
+                               $uid = $this->access->dn2username($member);
+                               if(!$uid) {
+                                       continue;
+                               }
+
+                               $cacheKey = 'userExistsOnLDAP' . $uid;
+                               $userExists = $this->access->connection->getFromCache($cacheKey);
+                               if($userExists === false) {
+                                       continue;
+                               }
+                               if($userExists === null || $search !== '') {
+                                       if (!$this->access->readAttribute($member,
                                                $this->access->connection->ldapUserDisplayName,
-                                               $this->access->getFilterPartForUserSearch($search))) {
+                                               $this->access->combineFilterWithAnd([
+                                                       $this->access->getFilterPartForUserSearch($search),
+                                                       $this->access->connection->ldapUserFilter
+                                               ])))
+                                       {
+                                               if($search === '') {
+                                                       $this->access->connection->writeToCache($cacheKey, false);
+                                               }
                                                continue;
                                        }
+                                       $this->access->connection->writeToCache($cacheKey, true);
                                }
-                               // dn2username will also check if the users belong to the allowed base
-                               if($ocname = $this->access->dn2username($member)) {
-                                       $groupUsers[] = $ocname;
-                               }
+                               $groupUsers[] = $uid;
                        }
                }
 

diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php
index 95e29689224b242d99ce7823bc1cc107dd322416..31a33d6aa1d485bc304b371b6fc80824a2a6208e 100644 (file)
--- a/apps/user_ldap/lib/User/User.php
+++ b/apps/user_ldap/lib/User/User.php
@@ -175,6 +175,21 @@ class User {
                }
        }
 
+       /**
+        * marks a user as deleted
+        *
+        * @throws \OCP\PreConditionNotMetException
+        */
+       public function markUser() {
+               $curValue = $this->config->getUserValue($this->getUsername(), 'user_ldap', 'isDeleted', '0');
+               if($curValue === '1') {
+                       // the user is already marked, do not write to DB again
+                       return;
+               }
+               $this->config->setUserValue($this->getUsername(), 'user_ldap', 'isDeleted', '1');
+               $this->config->setUserValue($this->getUsername(), 'user_ldap', 'foundDeleted', (string)time());
+       }
+
        /**
         * processes results from LDAP for attributes as returned by getAttributesToRead()
         * @param array $ldapEntry the user entry as retrieved from LDAP

diff --git a/apps/user_ldap/lib/User_LDAP.php b/apps/user_ldap/lib/User_LDAP.php
index 85b11acfc5766f0bb017bc10c38a15b03942567b..eb4589ce31d5dff99f816941fb1ca0359e13178d 100644 (file)
--- a/apps/user_ldap/lib/User_LDAP.php
+++ b/apps/user_ldap/lib/User_LDAP.php
@@ -46,7 +46,6 @@ use OCA\User_LDAP\User\OfflineUser;
 use OCA\User_LDAP\User\User;
 use OCP\IConfig;
 use OCP\ILogger;
-use OCP\IUser;
 use OCP\IUserSession;
 use OCP\Notification\IManager as INotificationManager;
 use OCP\Util;
@@ -58,9 +57,6 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
        /** @var INotificationManager */
        protected $notificationManager;
 
-       /** @var string */
-       protected $currentUserInDeletionProcess;
-
        /** @var UserPluginManager */
        protected $userPluginManager;
 
@@ -75,20 +71,6 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
                $this->ocConfig = $ocConfig;
                $this->notificationManager = $notificationManager;
                $this->userPluginManager = $userPluginManager;
-               $this->registerHooks($userSession);
-       }
-
-       protected function registerHooks(IUserSession $userSession) {
-               $userSession->listen('\OC\User', 'preDelete', [$this, 'preDeleteUser']);
-               $userSession->listen('\OC\User', 'postDelete', [$this, 'postDeleteUser']);
-       }
-
-       public function preDeleteUser(IUser $user) {
-               $this->currentUserInDeletionProcess = $user->getUID();
-       }
-
-       public function postDeleteUser() {
-               $this->currentUserInDeletionProcess = null;
        }
 
        /**
@@ -314,6 +296,12 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
                if(is_null($user)) {
                        return false;
                }
+               $uid = $user instanceof User ? $user->getUsername() : $user->getOCName();
+               $cacheKey = 'userExistsOnLDAP' . $uid;
+               $userExists = $this->access->connection->getFromCache($cacheKey);
+               if(!is_null($userExists)) {
+                       return (bool)$userExists;
+               }
 
                $dn = $user->getDN();
                //check if user really still exists by reading its entry
@@ -321,18 +309,22 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
                        try {
                                $uuid = $this->access->getUserMapper()->getUUIDByDN($dn);
                                if (!$uuid) {
+                                       $this->access->connection->writeToCache($cacheKey, false);
                                        return false;
                                }
                                $newDn = $this->access->getUserDnByUuid($uuid);
                                //check if renamed user is still valid by reapplying the ldap filter
                                if ($newDn === $dn || !is_array($this->access->readAttribute($newDn, '', $this->access->connection->ldapUserFilter))) {
+                                       $this->access->connection->writeToCache($cacheKey, false);
                                        return false;
                                }
                                $this->access->getUserMapper()->setDNbyUUID($newDn, $uuid);
+                               $this->access->connection->writeToCache($cacheKey, true);
                                return true;
                        } catch (ServerNotAvailableException $e) {
                                throw $e;
                        } catch (\Exception $e) {
+                               $this->access->connection->writeToCache($cacheKey, false);
                                return false;
                        }
                }
@@ -341,6 +333,7 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
                        $user->unmark();
                }
 
+               $this->access->connection->writeToCache($cacheKey, true);
                return true;
        }
 
@@ -363,15 +356,10 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
                                $this->access->connection->ldapHost, ILogger::DEBUG);
                        $this->access->connection->writeToCache('userExists'.$uid, false);
                        return false;
-               } else if($user instanceof OfflineUser) {
-                       //express check for users marked as deleted. Returning true is
-                       //necessary for cleanup
-                       return true;
                }
 
-               $result = $this->userExistsOnLDAP($user);
-               $this->access->connection->writeToCache('userExists'.$uid, $result);
-               return $result;
+               $this->access->connection->writeToCache('userExists'.$uid, true);
+               return true;
        }
 
        /**
@@ -429,21 +417,13 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 
                // early return path if it is a deleted user
                $user = $this->access->userManager->get($uid);
-               if($user instanceof OfflineUser) {
-                       if($this->currentUserInDeletionProcess !== null
-                               && $this->currentUserInDeletionProcess === $user->getOCName()
-                       ) {
-                               return $user->getHomePath();
-                       } else {
-                               throw new NoUserException($uid . ' is not a valid user anymore');
-                       }
-               } else if ($user === null) {
+               if($user instanceof User || $user instanceof OfflineUser) {
+                       $path = $user->getHomePath() ?: false;
+               } else {
                        throw new NoUserException($uid . ' is not a valid user anymore');
                }
 
-               $path = $user->getHomePath();
                $this->access->cacheUserHome($uid, $path);
-
                return $path;
        }
 

diff --git a/apps/user_ldap/lib/User_Proxy.php b/apps/user_ldap/lib/User_Proxy.php
index a3f9e67764e1f43aafa5998dc4c087e6d6db2ff4..359554a2bbc1f14f0706058c87d61ca71562bd00 100644 (file)
--- a/apps/user_ldap/lib/User_Proxy.php
+++ b/apps/user_ldap/lib/User_Proxy.php
@@ -37,7 +37,8 @@ use OCP\IUserSession;
 use OCP\Notification\IManager as INotificationManager;
 
 class User_Proxy extends Proxy implements \OCP\IUserBackend, \OCP\UserInterface, IUserLDAP {
-       private $backends = array();
+       private $backends = [];
+       /** @var User_LDAP */
        private $refBackend = null;
 
        /**
@@ -49,9 +50,14 @@ class User_Proxy extends Proxy implements \OCP\IUserBackend, \OCP\UserInterface,
         * @param INotificationManager $notificationManager
         * @param IUserSession $userSession
         */
-       public function __construct(array $serverConfigPrefixes, ILDAPWrapper $ldap, IConfig $ocConfig,
-               INotificationManager $notificationManager, IUserSession $userSession,
-                                                               UserPluginManager $userPluginManager) {
+       public function __construct(
+               array $serverConfigPrefixes,
+               ILDAPWrapper $ldap,
+               IConfig $ocConfig,
+               INotificationManager $notificationManager,
+               IUserSession $userSession,
+               UserPluginManager $userPluginManager
+       ) {
                parent::__construct($ldap);
                foreach($serverConfigPrefixes as $configPrefix) {
                        $this->backends[$configPrefix] =
@@ -105,13 +111,13 @@ class User_Proxy extends Proxy implements \OCP\IUserBackend, \OCP\UserInterface,
                                        && method_exists($this->getAccess($prefix), $method)) {
                                        $instance = $this->getAccess($prefix);
                                }
-                               $result = call_user_func_array(array($instance, $method), $parameters);
+                               $result = call_user_func_array([$instance, $method], $parameters);
                                if($result === $passOnWhen) {
                                        //not found here, reset cache to null if user vanished
                                        //because sometimes methods return false with a reason
                                        $userExists = call_user_func_array(
-                                               array($this->backends[$prefix], 'userExists'),
-                                               array($uid)
+                                               [$this->backends[$prefix], 'userExistsOnLDAP'],
+                                               [$uid]
                                        );
                                        if(!$userExists) {
                                                $this->writeToCache($cacheKey, null);
@@ -170,7 +176,22 @@ class User_Proxy extends Proxy implements \OCP\IUserBackend, \OCP\UserInterface,
         * @return boolean
         */
        public function userExists($uid) {
-               return $this->handleRequest($uid, 'userExists', array($uid));
+               $existsOnLDAP = false;
+               $existsLocally = $this->handleRequest($uid, 'userExists', array($uid));
+               if($existsLocally) {
+                       $existsOnLDAP = $this->userExistsOnLDAP($uid);
+               }
+               if($existsLocally && !$existsOnLDAP) {
+                       try {
+                               $user = $this->getLDAPAccess($uid)->userManager->get($uid);
+                               if($user instanceof User) {
+                                       $user->markUser();
+                               }
+                       } catch (\Exception $e) {
+                               // ignore
+                       }
+               }
+               return $existsLocally;
        }
 
        /**

diff --git a/apps/user_ldap/tests/User_LDAPTest.php b/apps/user_ldap/tests/User_LDAPTest.php
index 7517994b34add1421da6af22ec6cbc79588ab453..d5fc81afe0c8faf059858a25f90e4f71fc379ae4 100644 (file)
--- a/apps/user_ldap/tests/User_LDAPTest.php
+++ b/apps/user_ldap/tests/User_LDAPTest.php
@@ -314,22 +314,12 @@ class User_LDAPTest extends TestCase {
                $offlineUser->expects($this->once())
                        ->method('getHomePath')
                        ->willReturn($home);
-               $offlineUser->expects($this->once())
-                       ->method('getOCName')
-                       ->willReturn($uid);
                $this->userManager->expects($this->atLeastOnce())
                        ->method('get')
                        ->willReturn($offlineUser);
 
                $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager);
 
-               /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */
-               $user = $this->createMock(IUser::class);
-               $user->expects($this->once())
-                       ->method('getUID')
-                       ->willReturn($uid);
-
-               $backend->preDeleteUser($user);
                $result = $backend->deleteUser($uid);
                $this->assertTrue($result);
                /** @noinspection PhpUnhandledExceptionInspection */
@@ -509,18 +499,7 @@ class User_LDAPTest extends TestCase {
                $this->prepareMockForUserExists();
 
                $user = $this->createMock(User::class);
-               $user->expects($this->any())
-                       ->method('getDN')
-                       ->willReturn('dnOfRoland,dc=test');
 
-               $this->access->expects($this->any())
-                       ->method('readAttribute')
-                       ->will($this->returnCallback(function($dn) {
-                               if($dn === 'dnOfRoland,dc=test') {
-                                       return array();
-                               }
-                               return false;
-                       }));
                $this->userManager->expects($this->atLeastOnce())
                        ->method('get')
                        ->willReturn($user);
@@ -544,32 +523,18 @@ class User_LDAPTest extends TestCase {
                        ->with('dnOfFormerUser,dc=test')
                        ->willReturn('45673458748');
 
-               $this->access->expects($this->any())
-                       ->method('readAttribute')
-                       ->will($this->returnCallback(function($dn) {
-                               if($dn === 'dnOfRoland,dc=test') {
-                                       return array();
-                               }
-                               return false;
-                       }));
                $this->access->expects($this->any())
                        ->method('getUserMapper')
                        ->willReturn($mapper);
-               $this->access->expects($this->once())
-                       ->method('getUserDnByUuid')
-                       ->willThrowException(new \Exception());
 
                $user = $this->createMock(User::class);
-               $user->expects($this->any())
-                       ->method('getDN')
-                       ->willReturn('dnOfFormerUser,dc=test');
 
                $this->userManager->expects($this->atLeastOnce())
                        ->method('get')
                        ->willReturn($user);
 
-               //test for deleted user
-               $this->assertFalse($backend->userExists('formerUser'));
+               //test for deleted user – always returns true as long as we have the user in DB
+               $this->assertTrue($backend->userExists('formerUser'));
        }
 
        public function testUserExistsForNeverExisting() {
@@ -621,64 +586,6 @@ class User_LDAPTest extends TestCase {
                $this->assertTrue($result);
        }
 
-       public function testUserExistsPublicAPIForDeleted() {
-               $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager);
-               $this->prepareMockForUserExists();
-               \OC_User::useBackend($backend);
-
-               $mapper = $this->createMock(UserMapping::class);
-               $mapper->expects($this->any())
-                       ->method('getUUIDByDN')
-                       ->with('dnOfFormerUser,dc=test')
-                       ->willReturn('45673458748');
-
-               $this->access->expects($this->any())
-                       ->method('readAttribute')
-                       ->will($this->returnCallback(function($dn) {
-                               if($dn === 'dnOfRoland,dc=test') {
-                                       return array();
-                               }
-                               return false;
-                       }));
-               $this->access->expects($this->any())
-                       ->method('getUserMapper')
-                       ->willReturn($mapper);
-               $this->access->expects($this->once())
-                       ->method('getUserDnByUuid')
-                       ->willThrowException(new \Exception());
-
-               $user = $this->createMock(User::class);
-               $user->expects($this->any())
-                       ->method('getDN')
-                       ->willReturn('dnOfFormerUser,dc=test');
-
-               $this->userManager->expects($this->atLeastOnce())
-                       ->method('get')
-                       ->willReturn($user);
-
-               //test for deleted user
-               $this->assertFalse(\OC::$server->getUserManager()->userExists('formerUser'));
-       }
-
-       public function testUserExistsPublicAPIForNeverExisting() {
-               $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager);
-               $this->prepareMockForUserExists();
-               \OC_User::useBackend($backend);
-
-               $this->access->expects($this->any())
-                       ->method('readAttribute')
-                       ->will($this->returnCallback(function($dn) {
-                               if($dn === 'dnOfRoland,dc=test') {
-                                       return array();
-                               }
-                               return false;
-                       }));
-
-               //test for never-existing user
-               $result = \OC::$server->getUserManager()->userExists('mallory');
-               $this->assertFalse($result);
-       }
-
        public function testDeleteUserExisting() {
                $backend = new UserLDAP($this->access, $this->config, $this->notificationManager, $this->session, $this->pluginManager);
 
@@ -836,9 +743,6 @@ class User_LDAPTest extends TestCase {
                $this->assertFalse($result);
        }
 
-       /**
-        * @expectedException \OC\User\NoUserException
-        */
        public function testGetHomeDeletedUser() {
                $uid = 'newyorker';
 
@@ -869,14 +773,16 @@ class User_LDAPTest extends TestCase {
                        ->will($this->returnValue(true));
 
                $offlineUser = $this->createMock(OfflineUser::class);
-               $offlineUser->expects($this->never())
-                       ->method('getHomePath');
+               $offlineUser->expects($this->atLeastOnce())
+                       ->method('getHomePath')
+                       ->willReturn('');
 
                $this->userManager->expects($this->atLeastOnce())
                        ->method('get')
                        ->willReturn($offlineUser);
 
-               $backend->getHome($uid);
+               $result = $backend->getHome($uid);
+               $this->assertFalse($result);
        }
 
        public function testGetHomeWithPlugin() {
@@ -1112,7 +1018,7 @@ class User_LDAPTest extends TestCase {
                        ->willReturn(42);
 
                $this->assertEquals($this->backend->countUsers(),42);
-       }       
+       }
 
        public function testLoginName2UserNameSuccess() {
                $loginName = 'Alice';
@@ -1294,7 +1200,7 @@ class User_LDAPTest extends TestCase {
 
                $this->assertTrue(\OC_User::setPassword('roland', 'dt'));
        }
-       
+
        public function testSetPasswordValid() {
                $this->prepareAccessForSetPassword($this->access);