Merge pull request #19919 from nextcloud/enh/noid/ldpa_group_perf

LDAP Group Backend optimizations
nextcloud · Apr 24, 2020 · 212138d · 212138d
2 parents 84a3536 + 4babdc0
commit 212138d
Show file tree

Hide file tree

Showing 10 changed files with 428 additions and 206 deletions.
diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php
diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
diff --git a/apps/user_ldap/lib/Group_Proxy.php b/apps/user_ldap/lib/Group_Proxy.php
@@ -36,6 +36,7 @@ class Group_Proxy extends Proxy implements \OCP\GroupInterface, IGroupLDAP, IGet
 
 	/**
 	 * Constructor
+	 *
 	 * @param string[] $serverConfigPrefixes array containing the config Prefixes
 	 */
 	public function __construct($serverConfigPrefixes, ILDAPWrapper $ldap, GroupPluginManager $groupPluginManager) {
@@ -51,6 +52,7 @@ public function __construct($serverConfigPrefixes, ILDAPWrapper $ldap, GroupPlug
 
 	/**
 	 * Tries the backends one after the other until a positive result is returned from the specified method
+	 *
 	 * @param string $gid the gid connected to the request
 	 * @param string $method the method of the group backend that shall be called
 	 * @param array $parameters an array of parameters to be passed
@@ -60,7 +62,9 @@ protected function walkBackends($gid, $method, $parameters) {
 		$cacheKey = $this->getGroupCacheKey($gid);
 		foreach ($this->backends as $configPrefix => $backend) {
 			if ($result = call_user_func_array([$backend, $method], $parameters)) {
-				$this->writeToCache($cacheKey, $configPrefix);
+				if (!$this->isSingleBackend()) {
+					$this->writeToCache($cacheKey, $configPrefix);
+				}
 				return $result;
 			}
 		}
@@ -69,6 +73,7 @@ protected function walkBackends($gid, $method, $parameters) {
 
 	/**
 	 * Asks the backend connected to the server that supposely takes care of the gid from the request.
+	 *
 	 * @param string $gid the gid connected to the request
 	 * @param string $method the method of the group backend that shall be called
 	 * @param array $parameters an array of parameters to be passed
@@ -99,8 +104,13 @@ protected function callOnLastSeenOn($gid, $method, $parameters, $passOnWhen) {
 		return false;
 	}
 
+	protected function activeBackends(): int {
+		return count($this->backends);
+	}
+
 	/**
 	 * is user in group?
+	 *
 	 * @param string $uid uid of the user
 	 * @param string $gid gid of the group
 	 * @return bool
@@ -113,6 +123,7 @@ public function inGroup($uid, $gid) {
 
 	/**
 	 * Get all groups a user belongs to
+	 *
 	 * @param string $uid Name of the user
 	 * @return string[] with group names
 	 *
@@ -134,6 +145,7 @@ public function getUserGroups($uid) {
 
 	/**
 	 * get a list of all users in a group
+	 *
 	 * @return string[] with user ids
 	 */
 	public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
@@ -160,6 +172,7 @@ public function createGroup($gid) {
 
 	/**
 	 * delete a group
+	 *
 	 * @param string $gid gid of the group to delete
 	 * @return bool
 	 */
@@ -170,6 +183,7 @@ public function deleteGroup($gid) {
 
 	/**
 	 * Add a user to a group
+	 *
 	 * @param string $uid Name of the user to add to group
 	 * @param string $gid Name of the group in which add the user
 	 * @return bool
@@ -183,6 +197,7 @@ public function addToGroup($uid, $gid) {
 
 	/**
 	 * Removes a user from a group
+	 *
 	 * @param string $uid Name of the user to remove from group
 	 * @param string $gid Name of the group from which remove the user
 	 * @return bool
@@ -196,6 +211,7 @@ public function removeFromGroup($uid, $gid) {
 
 	/**
 	 * returns the number of users in a group, who match the search term
+	 *
 	 * @param string $gid the internal group name
 	 * @param string $search optional, a search string
 	 * @return int|bool
@@ -207,6 +223,7 @@ public function countUsersInGroup($gid, $search = '') {
 
 	/**
 	 * get an array with group details
+	 *
 	 * @param string $gid
 	 * @return array|false
 	 */
@@ -217,6 +234,7 @@ public function getGroupDetails($gid) {
 
 	/**
 	 * get a list of all groups
+	 *
 	 * @return string[] with group names
 	 *
 	 * Returns a list with all groups
@@ -236,6 +254,7 @@ public function getGroups($search = '', $limit = -1, $offset = 0) {
 
 	/**
 	 * check if a group exists
+	 *
 	 * @param string $gid
 	 * @return bool
 	 */
@@ -245,6 +264,7 @@ public function groupExists($gid) {
 
 	/**
 	 * Check if backend implements actions
+	 *
 	 * @param int $actions bitwise-or'ed actions
 	 * @return boolean
 	 *
@@ -258,6 +278,7 @@ public function implementsActions($actions) {
 
 	/**
 	 * Return access for LDAP interaction.
+	 *
 	 * @param string $gid
 	 * @return Access instance of Access for LDAP interaction
 	 */
@@ -268,6 +289,7 @@ public function getLDAPAccess($gid) {
 	/**
 	 * Return a new LDAP connection for the specified group.
 	 * The connection needs to be closed manually.
+	 *
 	 * @param string $gid
 	 * @return resource of the LDAP connection
 	 */

diff --git a/apps/user_ldap/lib/Helper.php b/apps/user_ldap/lib/Helper.php
@@ -34,24 +34,30 @@
 
 namespace OCA\User_LDAP;
 
+use OC\Cache\CappedMemoryCache;
 use OCP\IConfig;
 
 class Helper {
 
 	/** @var IConfig */
 	private $config;
 
+	/** @var CappedMemoryCache */
+	protected $sanitizeDnCache;
+
 	/**
 	 * Helper constructor.
 	 *
 	 * @param IConfig $config
 	 */
 	public function __construct(IConfig $config) {
 		$this->config = $config;
+		$this->sanitizeDnCache = new CappedMemoryCache(10000);
 	}
 
 	/**
 	 * returns prefixes for each saved LDAP/AD server configuration.
+	 *
 	 * @param bool $activeConfigurations optional, whether only active configuration shall be
 	 * retrieved, defaults to false
 	 * @return array with a list of the available prefixes
@@ -92,6 +98,7 @@ public function getServerConfigurationPrefixes($activeConfigurations = false) {
 	/**
 	 *
 	 * determines the host for every configured connection
+	 *
 	 * @return array an array with configprefix as keys
 	 *
 	 */
@@ -144,6 +151,7 @@ private function getServersConfig($value) {
 
 	/**
 	 * deletes a given saved LDAP/AD server configuration.
+	 *
 	 * @param string $prefix the configuration prefix of the config to delete
 	 * @return bool true on success, false otherwise
 	 */
@@ -161,11 +169,11 @@ public function deleteServerConfiguration($prefix) {
 			DELETE
 			FROM `*PREFIX*appconfig`
 			WHERE `configkey` LIKE ?
-				'.$saveOtherConfigurations.'
+				' . $saveOtherConfigurations . '
 				AND `appid` = \'user_ldap\'
 				AND `configkey` NOT IN (\'enabled\', \'installed_version\', \'types\', \'bgjUpdateGroupsLastRun\')
 		');
-		$delRows = $query->execute([$prefix.'%']);
+		$delRows = $query->execute([$prefix . '%']);
 
 		if ($delRows === null) {
 			return false;
@@ -180,8 +188,9 @@ public function deleteServerConfiguration($prefix) {
 
 	/**
 	 * checks whether there is one or more disabled LDAP configurations
-	 * @throws \Exception
+	 *
 	 * @return bool
+	 * @throws \Exception
 	 */
 	public function haveDisabledConfigurations() {
 		$all = $this->getServerConfigurationPrefixes(false);
@@ -196,6 +205,7 @@ public function haveDisabledConfigurations() {
 
 	/**
 	 * extracts the domain from a given URL
+	 *
 	 * @param string $url the URL
 	 * @return string|false domain as string on success, false otherwise
 	 */
@@ -229,6 +239,7 @@ public function setLDAPProvider() {
 
 	/**
 	 * sanitizes a DN received from the LDAP server
+	 *
 	 * @param array $dn the DN in question
 	 * @return array|string the sanitized DN
 	 */
@@ -242,12 +253,20 @@ public function sanitizeDN($dn) {
 			return $result;
 		}
 
+		if (!is_string($dn)) {
+			throw new \LogicException('String expected ' . \gettype($dn) . ' given');
+		}
+
+		if (($sanitizedDn = $this->sanitizeDnCache->get($dn)) !== null) {
+			return $sanitizedDn;
+		}
+
 		//OID sometimes gives back DNs with whitespace after the comma
 		// a la "uid=foo, cn=bar, dn=..." We need to tackle this!
-		$dn = preg_replace('/([^\\\]),(\s+)/u', '\1,', $dn);
+		$sanitizedDn = preg_replace('/([^\\\]),(\s+)/u', '\1,', $dn);
 
 		//make comparisons and everything work
-		$dn = mb_strtolower($dn, 'UTF-8');
+		$sanitizedDn = mb_strtolower($sanitizedDn, 'UTF-8');
 
 		//escape DN values according to RFC 2253 – this is already done by ldap_explode_dn
 		//to use the DN in search filters, \ needs to be escaped to \5c additionally
@@ -261,17 +280,19 @@ public function sanitizeDN($dn) {
 			'\;' => '\5c3B',
 			'\"' => '\5c22',
 			'\#' => '\5c23',
-			'('  => '\28',
-			')'  => '\29',
-			'*'  => '\2A',
+			'(' => '\28',
+			')' => '\29',
+			'*' => '\2A',
 		];
-		$dn = str_replace(array_keys($replacements), array_values($replacements), $dn);
+		$sanitizedDn = str_replace(array_keys($replacements), array_values($replacements), $sanitizedDn);
+		$this->sanitizeDnCache->set($dn, $sanitizedDn);
 
-		return $dn;
+		return $sanitizedDn;
 	}
 
 	/**
 	 * converts a stored DN so it can be used as base parameter for LDAP queries, internally we store them for usage in LDAP filters
+	 *
 	 * @param string $dn the DN
 	 * @return string
 	 */
@@ -302,7 +323,7 @@ public static function loginName2UserName($param) {
 		$userSession = \OC::$server->getUserSession();
 		$userPluginManager = \OC::$server->query('LDAPUserPluginManager');
 
-		$userBackend  = new User_Proxy(
+		$userBackend = new User_Proxy(
 			$configPrefixes, $ldapWrapper, $ocConfig, $notificationManager, $userSession, $userPluginManager
 		);
 		$uid = $userBackend->loginName2UserName($param['uid']);