read records from DB for lists at once, not one by one.

Keep a runtime cache of dn-id-mapping

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

diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php
index c087211cec7a23f943eb7b0109c857757e55f360..a564f6183bcb83a10426972c4d4fb16be5247a8f 100644 (file)
--- a/apps/user_ldap/lib/Access.php
+++ b/apps/user_ldap/lib/Access.php
@@ -871,9 +871,17 @@ class Access extends LDAPUtility {
                if (!$forceApplyAttributes) {
                        $isBackgroundJobModeAjax = $this->config
                                        ->getAppValue('core', 'backgroundjobs_mode', 'ajax') === 'ajax';
-                       $recordsToUpdate = array_filter($ldapRecords, function ($record) use ($isBackgroundJobModeAjax) {
+                       $listOfDNs = array_reduce($ldapRecords, function ($listOfDNs, $entry) {
+                               $listOfDNs[] = $entry['dn'][0];
+                               return $listOfDNs;
+                       }, []);
+                       $idsByDn = $this->userMapper->getListOfIdsByDn($listOfDNs);
+                       $recordsToUpdate = array_filter($ldapRecords, function($record) use ($isBackgroundJobModeAjax, $idsByDn) {
                                $newlyMapped = false;
-                               $uid = $this->dn2ocname($record['dn'][0], null, true, $newlyMapped, $record);
+                               $uid = $idsByDn[$record['dn'][0]] ?? null;
+                               if($uid === null) {
+                                       $uid = $this->dn2ocname($record['dn'][0], null, true, $newlyMapped, $record);
+                               }
                                if (is_string($uid)) {
                                        $this->cacheUserExists($uid);
                                }
@@ -925,9 +933,19 @@ class Access extends LDAPUtility {
         */
        public function fetchListOfGroups($filter, $attr, $limit = null, $offset = null) {
                $groupRecords = $this->searchGroups($filter, $attr, $limit, $offset);
-               array_walk($groupRecords, function ($record) {
+
+               $listOfDNs = array_reduce($groupRecords, function ($listOfDNs, $entry) {
+                       $listOfDNs[] = $entry['dn'][0];
+                       return$listOfDNs;
+               }, []);
+               $idsByDn = $this->groupMapper->getListOfIdsByDn($listOfDNs);
+
+               array_walk($groupRecords, function($record) use ($idsByDn) {
                        $newlyMapped = false;
-                       $gid = $this->dn2ocname($record['dn'][0], null, false, $newlyMapped, $record);
+                       $gid = $uidsByDn[$record['dn'][0]] ?? null;
+                       if($gid === null) {
+                               $gid = $this->dn2ocname($record['dn'][0], null, false, $newlyMapped, $record);
+                       }
                        if (!$newlyMapped && is_string($gid)) {
                                $this->cacheGroupExists($gid);
                        }

diff --git a/apps/user_ldap/lib/Mapping/AbstractMapping.php b/apps/user_ldap/lib/Mapping/AbstractMapping.php
index e14c9a572de445c0baa176e2cbc9b7b0ff6dd073..c88b84635f9b831fec22b243a0758578cc2416e6 100644 (file)
--- a/apps/user_ldap/lib/Mapping/AbstractMapping.php
+++ b/apps/user_ldap/lib/Mapping/AbstractMapping.php
@@ -26,8 +26,11 @@
 
 namespace OCA\User_LDAP\Mapping;
 
+use OC\DB\QueryBuilder\QueryBuilder;
+
 /**
  * Class AbstractMapping
+*
  * @package OCA\User_LDAP\Mapping
  */
 abstract class AbstractMapping {
@@ -40,7 +43,7 @@ abstract class AbstractMapping {
         * returns the DB table name which holds the mappings
         * @return string
         */
-       abstract protected function getTableName();
+       abstract protected function getTableName(bool $includePrefix = true);
 
        /**
         * @param \OCP\IDBConnection $dbc
@@ -49,6 +52,9 @@ abstract class AbstractMapping {
                $this->dbc = $dbc;
        }
 
+       /** @var array caches Names (value) by DN (key) */
+       protected $cache = [];
+
        /**
         * checks whether a provided string represents an existing table col
         * @param string $col
@@ -111,7 +117,12 @@ abstract class AbstractMapping {
         * @return string|false
         */
        public function getDNByName($name) {
-               return $this->getXbyY('ldap_dn', 'owncloud_name', $name);
+               $dn = array_search($name, $this->cache);
+               if($dn === false) {
+                       $dn = $this->getXbyY('ldap_dn', 'owncloud_name', $name);
+                       $this->cache[$dn] = $name;
+               }
+               return $dn;
        }
 
        /**
@@ -121,13 +132,21 @@ abstract class AbstractMapping {
         * @return bool
         */
        public function setDNbyUUID($fdn, $uuid) {
+               $oldDn = $this->getDnByUUID($uuid);
                $query = $this->dbc->prepare('
                        UPDATE `' . $this->getTableName() . '`
                        SET `ldap_dn` = ?
                        WHERE `directory_uuid` = ?
                ');
 
-               return $this->modify($query, [$fdn, $uuid]);
+               $r = $this->modify($query, [$fdn, $uuid]);
+
+               if($r && is_string($oldDn) && isset($this->cache[$oldDn])) {
+                       $this->cache[$fdn] = $this->cache[$oldDn];
+                       unset($this->cache[$oldDn]);
+               }
+
+               return $r;
        }
 
        /**
@@ -146,6 +165,8 @@ abstract class AbstractMapping {
                        WHERE `ldap_dn` = ?
                ');
 
+               unset($this->cache[$fdn]);
+
                return $this->modify($query, [$uuid, $fdn]);
        }
 
@@ -155,7 +176,27 @@ abstract class AbstractMapping {
         * @return string|false
         */
        public function getNameByDN($fdn) {
-               return $this->getXbyY('owncloud_name', 'ldap_dn', $fdn);
+               if(!isset($this->cache[$fdn])) {
+                       $this->cache[$fdn] = $this->getXbyY('owncloud_name', 'ldap_dn', $fdn);
+               }
+               return $this->cache[$fdn];
+       }
+
+       public function getListOfIdsByDn(array $fdns): array {
+               $qb = $this->dbc->getQueryBuilder();
+               $qb->select('owncloud_name', 'ldap_dn')
+                       ->from($this->getTableName(false))
+                       ->where($qb->expr()->in('ldap_dn', $qb->createNamedParameter($fdns, QueryBuilder::PARAM_STR_ARRAY)));
+               $stmt = $qb->execute();
+
+               $results = $stmt->fetchAll(\Doctrine\DBAL\FetchMode::ASSOCIATIVE);
+               foreach ($results as $key => $entry) {
+                       unset($results[$key]);
+                       $results[$entry['ldap_dn']] = $entry['owncloud_name'];
+                       $this->cache[$entry['ldap_dn']] = $entry['owncloud_name'];
+               }
+
+               return $results;
        }
 
        /**
@@ -191,6 +232,10 @@ abstract class AbstractMapping {
                return $this->getXbyY('owncloud_name', 'directory_uuid', $uuid);
        }
 
+       public function getDnByUUID($uuid) {
+               return $this->getXbyY('ldap_dn', 'directory_uuid', $uuid);
+       }
+
        /**
         * Gets the UUID based on the provided LDAP DN
         * @param string $dn
@@ -249,6 +294,9 @@ abstract class AbstractMapping {
 
                try {
                        $result = $this->dbc->insertIfNotExist($this->getTableName(), $row);
+                       if((bool)$result === true) {
+                               $this->cache[$fdn] = $name;
+                       }
                        // insertIfNotExist returns values as int
                        return (bool)$result;
                } catch (\Exception $e) {

diff --git a/apps/user_ldap/lib/Mapping/GroupMapping.php b/apps/user_ldap/lib/Mapping/GroupMapping.php
index b2c1b9c99af4222129c0884ae5fdb2f7bd5a00d8..703cc56a02af39b3b1147b5c026e119a60d14eea 100644 (file)
--- a/apps/user_ldap/lib/Mapping/GroupMapping.php
+++ b/apps/user_ldap/lib/Mapping/GroupMapping.php
@@ -33,7 +33,8 @@ class GroupMapping extends AbstractMapping {
         * returns the DB table name which holds the mappings
         * @return string
         */
-       protected function getTableName() {
-               return '*PREFIX*ldap_group_mapping';
+       protected function getTableName(bool $includePrefix = true) {
+               $p = $includePrefix ? '*PREFIX*' : '';
+               return $p . 'ldap_group_mapping';
        }
 }

diff --git a/apps/user_ldap/lib/Mapping/UserMapping.php b/apps/user_ldap/lib/Mapping/UserMapping.php
index 556f7ecf1a493591266cfd49af88ed1ae5f8319b..b70cb866904062171798aaa8dd3d25ab29f50137 100644 (file)
--- a/apps/user_ldap/lib/Mapping/UserMapping.php
+++ b/apps/user_ldap/lib/Mapping/UserMapping.php
@@ -33,7 +33,8 @@ class UserMapping extends AbstractMapping {
         * returns the DB table name which holds the mappings
         * @return string
         */
-       protected function getTableName() {
-               return '*PREFIX*ldap_user_mapping';
+       protected function getTableName(bool $includePrefix = true) {
+               $p = $includePrefix ? '*PREFIX*' : '';
+               return $p . 'ldap_user_mapping';
        }
 }

diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php
index ae637e0e584bbbefc2df0e5f50674cdaf1a519f8..8dfcd335e85695db39641aa4e278d0d62d8daf89 100644 (file)
--- a/apps/user_ldap/tests/Group_LDAPTest.php
+++ b/apps/user_ldap/tests/Group_LDAPTest.php
@@ -79,7 +79,7 @@ class Group_LDAPTest extends TestCase {
        private function getPluginManagerMock() {
                return $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')->getMock();
        }
-       
+
        /**
         * @param Access|\PHPUnit_Framework_MockObject_MockObject $access
         */
@@ -120,6 +120,9 @@ class Group_LDAPTest extends TestCase {
                                }
                                return [];
                        });
+               $access->expects($this->any())
+                       ->method('isDNPartOfBase')
+                       ->willReturn(true);
 
                // for primary groups
                $access->expects($this->once())
@@ -141,11 +144,9 @@ class Group_LDAPTest extends TestCase {
                $access->expects($this->any())
                        ->method('groupname2dn')
                        ->willReturn('cn=group,dc=foo,dc=bar');
-
                $access->expects($this->any())
                        ->method('fetchListOfUsers')
                        ->willReturn([]);
-
                $access->expects($this->any())
                        ->method('readAttribute')
                        ->willReturnCallback(function ($name) {
@@ -159,12 +160,14 @@ class Group_LDAPTest extends TestCase {
                                }
                                return ['u11', 'u22', 'u33', 'u34'];
                        });
-
                $access->expects($this->any())
                        ->method('dn2username')
                        ->willReturnCallback(function () {
                                return 'foobar' . \OC::$server->getSecureRandom()->generate(7);
                        });
+               $access->expects($this->any())
+                       ->method('isDNPartOfBase')
+                       ->willReturn(true);
 
                $groupBackend = new GroupLDAP($access,$pluginManager);
                $users = $groupBackend->countUsersInGroup('group', '3');
@@ -534,6 +537,9 @@ class Group_LDAPTest extends TestCase {
                $access->expects($this->exactly(2))
                        ->method('nextcloudUserNames')
                        ->willReturnOnConsecutiveCalls(['lisa', 'bart', 'kira', 'brad'], ['walle', 'dino', 'xenia']);
+               $access->expects($this->any())
+                       ->method('isDNPartOfBase')
+                       ->willReturn(true);
                $access->userManager = $this->createMock(Manager::class);
 
                $groupBackend = new GroupLDAP($access, $pluginManager);
@@ -569,6 +575,9 @@ class Group_LDAPTest extends TestCase {
                $access->expects($this->once())
                        ->method('nextcloudUserNames')
                        ->willReturn(['lisa', 'bart', 'kira', 'brad']);
+               $access->expects($this->any())
+                       ->method('isDNPartOfBase')
+                       ->willReturn(true);
                $access->userManager = $this->createMock(Manager::class);
 
                $groupBackend = new GroupLDAP($access, $pluginManager);
@@ -599,14 +608,15 @@ class Group_LDAPTest extends TestCase {
                                }
                                return [];
                        });
-
                $access->expects($this->any())
                        ->method('groupname2dn')
                        ->willReturn('cn=foobar,dc=foo,dc=bar');
-
                $access->expects($this->once())
                        ->method('countUsers')
                        ->willReturn(4);
+               $access->expects($this->any())
+                       ->method('isDNPartOfBase')
+                       ->willReturn(true);
 
                $groupBackend = new GroupLDAP($access, $pluginManager);
                $users = $groupBackend->countUsersInGroup('foobar');
@@ -629,17 +639,19 @@ class Group_LDAPTest extends TestCase {
                        ->method('username2dn')
                        ->willReturn($dn);
 
-               $access->expects($this->exactly(3))
+               $access->expects($this->exactly(5))
                        ->method('readAttribute')
-                       ->will($this->onConsecutiveCalls(['cn=groupA,dc=foobar', 'cn=groupB,dc=foobar'], [], []));
+                       ->will($this->onConsecutiveCalls(['cn=groupA,dc=foobar', 'cn=groupB,dc=foobar'], [], [], [], []));
 
-               $access->expects($this->exactly(2))
+               $access->expects($this->any())
                        ->method('dn2groupname')
                        ->willReturnArgument(0);
-
-               $access->expects($this->exactly(1))
-                       ->method('groupsMatchFilter')
+               $access->expects($this->any())
+                       ->method('groupname2dn')
                        ->willReturnArgument(0);
+               $access->expects($this->any())
+                       ->method('isDNPartOfBase')
+                       ->willReturn(true);
 
                $groupBackend = new GroupLDAP($access, $pluginManager);
                $groups = $groupBackend->getUserGroups('userX');
@@ -677,9 +689,6 @@ class Group_LDAPTest extends TestCase {
                $access->expects($this->once())
                        ->method('nextcloudGroupNames')
                        ->willReturn([]);
-               $access->expects($this->any())
-                       ->method('groupsMatchFilter')
-                       ->willReturnArgument(0);
 
                $groupBackend = new GroupLDAP($access, $pluginManager);
                $groupBackend->getUserGroups('userX');
@@ -715,9 +724,9 @@ class Group_LDAPTest extends TestCase {
                        ->method('username2dn')
                        ->willReturn($dn);
 
-               $access->expects($this->never())
+               $access->expects($this->any())
                        ->method('readAttribute')
-                       ->with($dn, 'memberOf');
+                       ->willReturn([]);
 
                $group1 = [
                        'cn' => 'group1',
@@ -736,8 +745,23 @@ class Group_LDAPTest extends TestCase {
                        ->method('fetchListOfGroups')
                        ->willReturn([$group1, $group2]);
                $access->expects($this->any())
-                       ->method('groupsMatchFilter')
-                       ->willReturnArgument(0);
+                       ->method('dn2groupname')
+                       ->willReturnCallback(function(string $dn) {
+                               return ldap_explode_dn($dn, 1)[0];
+                       });
+               $access->expects($this->any())
+                       ->method('groupname2dn')
+                       ->willReturnCallback(function (string $gid) use ($group1, $group2) {
+                               if($gid === $group1['cn']) {
+                                       return $group1['dn'][0];
+                               }
+                               if($gid === $group2['cn']) {
+                                       return $group2['dn'][0];
+                               }
+                       });
+               $access->expects($this->any())
+                       ->method('isDNPartOfBase')
+                       ->willReturn(true);
 
                $groupBackend = new GroupLDAP($access, $pluginManager);
                $groups = $groupBackend->getUserGroups('userX');
@@ -771,7 +795,7 @@ class Group_LDAPTest extends TestCase {
                $this->assertEquals($ldap->createGroup('gid'),true);
        }
 
-       
+
        public function testCreateGroupFailing() {
                $this->expectException(\Exception::class);
 
@@ -826,7 +850,7 @@ class Group_LDAPTest extends TestCase {
                $this->assertEquals($ldap->deleteGroup('gid'),'result');
        }
 
-       
+
        public function testDeleteGroupFailing() {
                $this->expectException(\Exception::class);
 
@@ -872,7 +896,7 @@ class Group_LDAPTest extends TestCase {
                $this->assertEquals($ldap->addToGroup('uid', 'gid'),'result');
        }
 
-       
+
        public function testAddToGroupFailing() {
                $this->expectException(\Exception::class);
 
@@ -918,7 +942,7 @@ class Group_LDAPTest extends TestCase {
                $this->assertEquals($ldap->removeFromGroup('uid', 'gid'),'result');
        }
 
-       
+
        public function testRemoveFromGroupFailing() {
                $this->expectException(\Exception::class);
 
@@ -964,7 +988,7 @@ class Group_LDAPTest extends TestCase {
                $this->assertEquals($ldap->getGroupDetails('gid'),'result');
        }
 
-       
+
        public function testGetGroupDetailsFailing() {
                $this->expectException(\Exception::class);