Merge pull request #21171 from nextcloud/enh/noid/tidy-up-group-ldap

tidy up Group_LDAP

4 files changed, 198 insertions, 235 deletions

diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php
index b93d92b81dd..ef4b5c9434e 100644
--- a/apps/user_ldap/lib/Access.php
+++ b/apps/user_ldap/lib/Access.php
@@ -1435,7 +1435,7 @@ class Access extends LDAPUtility {
 	 * @param bool $allowAsterisk whether in * at the beginning should be preserved
 	 * @return string the escaped string
 	 */
-	public function escapeFilterPart($input, $allowAsterisk = false) {
+	public function escapeFilterPart($input, $allowAsterisk = false): string {
 		$asterisk = '';
 		if ($allowAsterisk && strlen($input) > 0 && $input[0] === '*') {
 			$asterisk = '*';
@@ -1452,7 +1452,7 @@ class Access extends LDAPUtility {
 	 * @param string[] $filters the filters to connect
 	 * @return string the combined filter
 	 */
-	public function combineFilterWithAnd($filters) {
+	public function combineFilterWithAnd($filters): string {
 		return $this->combineFilter($filters, '&');
 	}
 
diff --git a/apps/user_ldap/lib/Connection.php b/apps/user_ldap/lib/Connection.php
index 0cc93a08e6f..079429027c5 100644
--- a/apps/user_ldap/lib/Connection.php
+++ b/apps/user_ldap/lib/Connection.php
@@ -67,6 +67,11 @@ use OCP\ILogger;
  * @property string[] ldapBaseGroups
  * @property string ldapGroupFilter
  * @property string ldapGroupDisplayName
+ * @property string ldapLoginFilter
+ * @property string ldapDynamicGroupMemberURL
+ * @property string ldapGidNumber
+ * @property int hasMemberOfFilterSupport
+ * @property int useMemberOfToDetectMembership
  */
 class Connection extends LDAPUtility {
 	private $ldapConnectionRes = null;
diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
index 6b62f88123e..6f6e7362e97 100644
--- a/apps/user_ldap/lib/Group_LDAP.php
+++ b/apps/user_ldap/lib/Group_LDAP.php
@@ -43,37 +43,34 @@
 
 namespace OCA\User_LDAP;
 
+use Closure;
+use Exception;
+use OC;
 use OC\Cache\CappedMemoryCache;
+use OC\ServerNotAvailableException;
 use OCP\Group\Backend\IGetDisplayNameBackend;
 use OCP\GroupInterface;
 use OCP\ILogger;
 
-class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLDAP, IGetDisplayNameBackend {
+class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend {
 	protected $enabled = false;
 
-	/**
-	 * @var string[] $cachedGroupMembers array of users with gid as key
-	 */
+	/** @var string[] $cachedGroupMembers array of users with gid as key */
 	protected $cachedGroupMembers;
-
-	/**
-	 * @var string[] $cachedGroupsByMember array of groups with uid as key
-	 */
+	/** @var string[] $cachedGroupsByMember array of groups with uid as key */
 	protected $cachedGroupsByMember;
-
-	/**
-	 * @var string[] $cachedNestedGroups array of groups with gid (DN) as key
-	 */
+	/** @var string[] $cachedNestedGroups array of groups with gid (DN) as key */
 	protected $cachedNestedGroups;
-
 	/** @var GroupPluginManager */
 	protected $groupPluginManager;
+	/** @var ILogger */
+	protected $logger;
 
 	public function __construct(Access $access, GroupPluginManager $groupPluginManager) {
 		parent::__construct($access);
 		$filter = $this->access->connection->ldapGroupFilter;
-		$gassoc = $this->access->connection->ldapGroupMemberAssocAttr;
-		if (!empty($filter) && !empty($gassoc)) {
+		$gAssoc = $this->access->connection->ldapGroupMemberAssocAttr;
+		if (!empty($filter) && !empty($gAssoc)) {
 			$this->enabled = true;
 		}
 
@@ -81,6 +78,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 		$this->cachedGroupsByMember = new CappedMemoryCache();
 		$this->cachedNestedGroups = new CappedMemoryCache();
 		$this->groupPluginManager = $groupPluginManager;
+		$this->logger = OC::$server->getLogger();
 	}
 
 	/**
@@ -89,8 +87,8 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 * @param string $uid uid of the user
 	 * @param string $gid gid of the group
 	 * @return bool
-	 *
-	 * Checks whether the user is member of a group or not.
+	 * @throws Exception
+	 * @throws ServerNotAvailableException
 	 */
 	public function inGroup($uid, $gid) {
 		if (!$this->enabled) {
@@ -105,8 +103,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 		$userDN = $this->access->username2dn($uid);
 
 		if (isset($this->cachedGroupMembers[$gid])) {
-			$isInGroup = in_array($userDN, $this->cachedGroupMembers[$gid]);
-			return $isInGroup;
+			return in_array($userDN, $this->cachedGroupMembers[$gid]);
 		}
 
 		$cacheKeyMembers = 'inGroup-members:' . $gid;
@@ -175,13 +172,13 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	}
 
 	/**
-	 * @param string $dnGroup
-	 * @return array
+	 * For a group that has user membership defined by an LDAP search url
+	 * attribute returns the users that match the search url otherwise returns
+	 * an empty array.
 	 *
-	 * For a group that has user membership defined by an LDAP search url attribute returns the users
-	 * that match the search url otherwise returns an empty array.
+	 * @throws ServerNotAvailableException
 	 */
-	public function getDynamicGroupMembers($dnGroup) {
+	public function getDynamicGroupMembers(string $dnGroup): array {
 		$dynamicGroupMemberURL = strtolower($this->access->connection->ldapDynamicGroupMemberURL);
 
 		if (empty($dynamicGroupMemberURL)) {
@@ -207,26 +204,26 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 					$dynamicMembers[$value['dn'][0]] = 1;
 				}
 			} else {
-				\OCP\Util::writeLog('user_ldap', 'No search filter found on member url ' .
-					'of group ' . $dnGroup, ILogger::DEBUG);
+				$this->logger->debug('No search filter found on member url of group {dn}',
+					[
+						'app' => 'user_ldap',
+						'dn' => $dnGroup,
+					]
+				);
 			}
 		}
 		return $dynamicMembers;
 	}
 
 	/**
-	 * @param string $dnGroup
-	 * @param array|null &$seen
-	 * @return array|mixed|null
-	 * @throws \OC\ServerNotAvailableException
+	 * @throws ServerNotAvailableException
 	 */
-	private function _groupMembers($dnGroup, &$seen = null) {
+	private function _groupMembers(string $dnGroup, ?array &$seen = null): array {
 		if ($seen === null) {
 			$seen = [];
 		}
 		$allMembers = [];
 		if (array_key_exists($dnGroup, $seen)) {
-			// avoid loops
 			return [];
 		}
 		// used extensively in cron job, caching makes sense for nested groups
@@ -251,13 +248,10 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	}
 
 	/**
-	 * @param string $DN
-	 * @param array|null &$seen
-	 * @return array
-	 * @throws \OC\ServerNotAvailableException
+	 * @throws ServerNotAvailableException
 	 */
-	private function _getGroupDNsFromMemberOf($DN) {
-		$groups = $this->access->readAttribute($DN, 'memberOf');
+	private function _getGroupDNsFromMemberOf(string $dn): array {
+		$groups = $this->access->readAttribute($dn, 'memberOf');
 		if (!is_array($groups)) {
 			return [];
 		}
@@ -275,17 +269,11 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 			return $nestedGroups;
 		};
 
-		$groups = $this->walkNestedGroups($DN, $fetcher, $groups);
+		$groups = $this->walkNestedGroups($dn, $fetcher, $groups);
 		return $this->filterValidGroups($groups);
 	}
 
-	/**
-	 * @param string $dn
-	 * @param \Closure $fetcher args: string $dn, array $seen, returns: string[] of dns
-	 * @param array $list
-	 * @return array
-	 */
-	private function walkNestedGroups(string $dn, \Closure $fetcher, array $list): array {
+	private function walkNestedGroups(string $dn, Closure $fetcher, array $list): array {
 		$nesting = (int)$this->access->connection->ldapNestedGroups;
 		// depending on the input, we either have a list of DNs or a list of LDAP records
 		// also, the output expects either DNs or records. Testing the first element should suffice.
@@ -322,11 +310,11 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	/**
 	 * translates a gidNumber into an ownCloud internal name
 	 *
-	 * @param string $gid as given by gidNumber on POSIX LDAP
-	 * @param string $dn a DN that belongs to the same domain as the group
 	 * @return string|bool
+	 * @throws Exception
+	 * @throws ServerNotAvailableException
 	 */
-	public function gidNumber2Name($gid, $dn) {
+	public function gidNumber2Name(string $gid, string $dn) {
 		$cacheKey = 'gidNumberToName' . $gid;
 		$groupName = $this->access->connection->getFromCache($cacheKey);
 		if (!is_null($groupName) && isset($groupName)) {
@@ -339,14 +327,22 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 			'objectClass=posixGroup',
 			$this->access->connection->ldapGidNumber . '=' . $gid
 		]);
+		return $this->getNameOfGroup($filter, $cacheKey) ?? false;
+	}
+
+	/**
+	 * @throws ServerNotAvailableException
+	 * @throws Exception
+	 */
+	private function getNameOfGroup(string $filter, string $cacheKey) {
 		$result = $this->access->searchGroups($filter, ['dn'], 1);
 		if (empty($result)) {
-			return false;
+			return null;
 		}
 		$dn = $result[0]['dn'][0];
 
 		//and now the group name
-		//NOTE once we have separate ownCloud group IDs and group names we can
+		//NOTE once we have separate Nextcloud group IDs and group names we can
 		//directly read the display name attribute instead of the DN
 		$name = $this->access->dn2groupname($dn);
 
@@ -358,11 +354,10 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	/**
 	 * returns the entry's gidNumber
 	 *
-	 * @param string $dn
-	 * @param string $attribute
 	 * @return string|bool
+	 * @throws ServerNotAvailableException
 	 */
-	private function getEntryGidNumber($dn, $attribute) {
+	private function getEntryGidNumber(string $dn, string $attribute) {
 		$value = $this->access->readAttribute($dn, $attribute);
 		if (is_array($value) && !empty($value)) {
 			return $value[0];
@@ -371,22 +366,20 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	}
 
 	/**
-	 * returns the group's primary ID
-	 *
-	 * @param string $dn
 	 * @return string|bool
+	 * @throws ServerNotAvailableException
 	 */
-	public function getGroupGidNumber($dn) {
+	public function getGroupGidNumber(string $dn) {
 		return $this->getEntryGidNumber($dn, 'gidNumber');
 	}
 
 	/**
 	 * returns the user's gidNumber
 	 *
-	 * @param string $dn
 	 * @return string|bool
+	 * @throws ServerNotAvailableException
 	 */
-	public function getUserGidNumber($dn) {
+	public function getUserGidNumber(string $dn) {
 		$gidNumber = false;
 		if ($this->access->connection->hasGidNumber) {
 			$gidNumber = $this->getEntryGidNumber($dn, $this->access->connection->ldapGidNumber);
@@ -398,17 +391,13 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	}
 
 	/**
-	 * returns a filter for a "users has specific gid" search or count operation
-	 *
-	 * @param string $groupDN
-	 * @param string $search
-	 * @return string
-	 * @throws \Exception
+	 * @throws ServerNotAvailableException
+	 * @throws Exception
 	 */
-	private function prepareFilterForUsersHasGidNumber($groupDN, $search = '') {
+	private function prepareFilterForUsersHasGidNumber(string $groupDN, string $search = ''): string {
 		$groupID = $this->getGroupGidNumber($groupDN);
 		if ($groupID === false) {
-			throw new \Exception('Not a valid group');
+			throw new Exception('Not a valid group');
 		}
 
 		$filterParts = [];
@@ -424,13 +413,14 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	/**
 	 * returns a list of users that have the given group as gid number
 	 *
-	 * @param string $groupDN
-	 * @param string $search
-	 * @param int $limit
-	 * @param int $offset
-	 * @return string[]
+	 * @throws ServerNotAvailableException
 	 */
-	public function getUsersInGidNumber($groupDN, $search = '', $limit = -1, $offset = 0) {
+	public function getUsersInGidNumber(
+		string $groupDN,
+		string $search = '',
+		?int $limit = -1,
+		?int $offset = 0
+	): array {
 		try {
 			$filter = $this->prepareFilterForUsersHasGidNumber($groupDN, $search);
 			$users = $this->access->fetchListOfUsers(
@@ -440,37 +430,18 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 				$offset
 			);
 			return $this->access->nextcloudUserNames($users);
-		} catch (\Exception $e) {
+		} catch (ServerNotAvailableException $e) {
+			throw $e;
+		} catch (Exception $e) {
 			return [];
 		}
 	}
 
 	/**
-	 * returns the number of users that have the given group as gid number
-	 *
-	 * @param string $groupDN
-	 * @param string $search
-	 * @param int $limit
-	 * @param int $offset
-	 * @return int
-	 */
-	public function countUsersInGidNumber($groupDN, $search = '', $limit = -1, $offset = 0) {
-		try {
-			$filter = $this->prepareFilterForUsersHasGidNumber($groupDN, $search);
-			$users = $this->access->countUsers($filter, ['dn'], $limit, $offset);
-			return (int)$users;
-		} catch (\Exception $e) {
-			return 0;
-		}
-	}
-
-	/**
-	 * gets the gidNumber of a user
-	 *
-	 * @param string $dn
-	 * @return string
+	 * @throws ServerNotAvailableException
+	 * @return bool
 	 */
-	public function getUserGroupByGid($dn) {
+	public function getUserGroupByGid(string $dn) {
 		$groupID = $this->getUserGidNumber($dn);
 		if ($groupID !== false) {
 			$groupName = $this->gidNumber2Name($groupID, $dn);
@@ -485,11 +456,11 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	/**
 	 * translates a primary group ID into an Nextcloud internal name
 	 *
-	 * @param string $gid as given by primaryGroupID on AD
-	 * @param string $dn a DN that belongs to the same domain as the group
 	 * @return string|bool
+	 * @throws Exception
+	 * @throws ServerNotAvailableException
 	 */
-	public function primaryGroupID2Name($gid, $dn) {
+	public function primaryGroupID2Name(string $gid, string $dn) {
 		$cacheKey = 'primaryGroupIDtoName';
 		$groupNames = $this->access->connection->getFromCache($cacheKey);
 		if (!is_null($groupNames) && isset($groupNames[$gid])) {
@@ -506,30 +477,16 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 			$this->access->connection->ldapGroupFilter,
 			'objectsid=' . $domainObjectSid . '-' . $gid
 		]);
-		$result = $this->access->searchGroups($filter, ['dn'], 1);
-		if (empty($result)) {
-			return false;
-		}
-		$dn = $result[0]['dn'][0];
-
-		//and now the group name
-		//NOTE once we have separate Nextcloud group IDs and group names we can
-		//directly read the display name attribute instead of the DN
-		$name = $this->access->dn2groupname($dn);
-
-		$this->access->connection->writeToCache($cacheKey, $name);
-
-		return $name;
+		return $this->getNameOfGroup($filter, $cacheKey) ?? false;
 	}
 
 	/**
 	 * returns the entry's primary group ID
 	 *
-	 * @param string $dn
-	 * @param string $attribute
 	 * @return string|bool
+	 * @throws ServerNotAvailableException
 	 */
-	private function getEntryGroupID($dn, $attribute) {
+	private function getEntryGroupID(string $dn, string $attribute) {
 		$value = $this->access->readAttribute($dn, $attribute);
 		if (is_array($value) && !empty($value)) {
 			return $value[0];
@@ -538,22 +495,18 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	}
 
 	/**
-	 * returns the group's primary ID
-	 *
-	 * @param string $dn
 	 * @return string|bool
+	 * @throws ServerNotAvailableException
 	 */
-	public function getGroupPrimaryGroupID($dn) {
+	public function getGroupPrimaryGroupID(string $dn) {
 		return $this->getEntryGroupID($dn, 'primaryGroupToken');
 	}
 
 	/**
-	 * returns the user's primary group ID
-	 *
-	 * @param string $dn
 	 * @return string|bool
+	 * @throws ServerNotAvailableException
 	 */
-	public function getUserPrimaryGroupIDs($dn) {
+	public function getUserPrimaryGroupIDs(string $dn) {
 		$primaryGroupID = false;
 		if ($this->access->connection->hasPrimaryGroups) {
 			$primaryGroupID = $this->getEntryGroupID($dn, 'primaryGroupID');
@@ -565,17 +518,13 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	}
 
 	/**
-	 * returns a filter for a "users in primary group" search or count operation
-	 *
-	 * @param string $groupDN
-	 * @param string $search
-	 * @return string
-	 * @throws \Exception
+	 * @throws Exception
+	 * @throws ServerNotAvailableException
 	 */
-	private function prepareFilterForUsersInPrimaryGroup($groupDN, $search = '') {
+	private function prepareFilterForUsersInPrimaryGroup(string $groupDN, string $search = ''): string {
 		$groupID = $this->getGroupPrimaryGroupID($groupDN);
 		if ($groupID === false) {
-			throw new \Exception('Not a valid group');
+			throw new Exception('Not a valid group');
 		}
 
 		$filterParts = [];
@@ -589,15 +538,14 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	}
 
 	/**
-	 * returns a list of users that have the given group as primary group
-	 *
-	 * @param string $groupDN
-	 * @param string $search
-	 * @param int $limit
-	 * @param int $offset
-	 * @return string[]
+	 * @throws ServerNotAvailableException
 	 */
-	public function getUsersInPrimaryGroup($groupDN, $search = '', $limit = -1, $offset = 0) {
+	public function getUsersInPrimaryGroup(
+		string $groupDN,
+		string $search = '',
+		?int $limit = -1,
+		?int $offset = 0
+	): array {
 		try {
 			$filter = $this->prepareFilterForUsersInPrimaryGroup($groupDN, $search);
 			$users = $this->access->fetchListOfUsers(
@@ -607,37 +555,38 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 				$offset
 			);
 			return $this->access->nextcloudUserNames($users);
-		} catch (\Exception $e) {
+		} catch (ServerNotAvailableException $e) {
+			throw $e;
+		} catch (Exception $e) {
 			return [];
 		}
 	}
 
 	/**
-	 * returns the number of users that have the given group as primary group
-	 *
-	 * @param string $groupDN
-	 * @param string $search
-	 * @param int $limit
-	 * @param int $offset
-	 * @return int
+	 * @throws ServerNotAvailableException
 	 */
-	public function countUsersInPrimaryGroup($groupDN, $search = '', $limit = -1, $offset = 0) {
+	public function countUsersInPrimaryGroup(
+		string $groupDN,
+		string $search = '',
+		int $limit = -1,
+		int $offset = 0
+	): int {
 		try {
 			$filter = $this->prepareFilterForUsersInPrimaryGroup($groupDN, $search);
 			$users = $this->access->countUsers($filter, ['dn'], $limit, $offset);
 			return (int)$users;
-		} catch (\Exception $e) {
+		} catch (ServerNotAvailableException $e) {
+			throw $e;
+		} catch (Exception $e) {
 			return 0;
 		}
 	}
 
 	/**
-	 * gets the primary group of a user
-	 *
-	 * @param string $dn
-	 * @return string
+	 * @return string|bool
+	 * @throws ServerNotAvailableException
 	 */
-	public function getUserPrimaryGroup($dn) {
+	public function getUserPrimaryGroup(string $dn) {
 		$groupID = $this->getUserPrimaryGroupIDs($dn);
 		if ($groupID !== false) {
 			$groupName = $this->primaryGroupID2Name($groupID, $dn);
@@ -650,15 +599,15 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	}
 
 	/**
-	 * Get all groups a user belongs to
-	 *
-	 * @param string $uid Name of the user
-	 * @return array with group names
-	 *
 	 * This function fetches all groups a user belongs to. It does not check
 	 * if the user exists at all.
 	 *
 	 * This function includes groups based on dynamic group membership.
+	 *
+	 * @param string $uid Name of the user
+	 * @return array with group names
+	 * @throws Exception
+	 * @throws ServerNotAvailableException
 	 */
 	public function getUserGroups($uid) {
 		if (!$this->enabled) {
@@ -709,8 +658,12 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 						}
 					}
 				} else {
-					\OCP\Util::writeLog('user_ldap', 'No search filter found on member url ' .
-						'of group ' . print_r($dynamicGroup, true), ILogger::DEBUG);
+					$this->logger->debug('No search filter found on member url of group {dn}',
+						[
+							'app' => 'user_ldap',
+							'dn' => $dynamicGroup,
+						]
+					);
 				}
 			}
 		}
@@ -752,8 +705,13 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 		} elseif (strtolower($this->access->connection->ldapGroupMemberAssocAttr) === 'memberuid') {
 			$result = $this->access->readAttribute($userDN, 'uid');
 			if ($result === false) {
-				\OCP\Util::writeLog('user_ldap', 'No uid attribute found for DN ' . $userDN . ' on ' .
-					$this->access->connection->ldapHost, ILogger::DEBUG);
+				$this->logger->debug('No uid attribute found for DN {dn} on {host}',
+					[
+						'app' => 'user_ldap',
+						'dn' => $userDN,
+						'host' => $this->access->connection->ldapHost,
+					]
+				);
 				$uid = false;
 			} else {
 				$uid = $result[0];
@@ -788,11 +746,9 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	}
 
 	/**
-	 * @param string $dn
-	 * @param array|null &$seen
-	 * @return array
+	 * @throws ServerNotAvailableException
 	 */
-	private function getGroupsByMember($dn, &$seen = null) {
+	private function getGroupsByMember(string $dn, array &$seen = null): array {
 		if ($seen === null) {
 			$seen = [];
 		}
@@ -826,7 +782,8 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 * @param int $limit
 	 * @param int $offset
 	 * @return array with user ids
-	 * @throws \Exception
+	 * @throws Exception
+	 * @throws ServerNotAvailableException
 	 */
 	public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
 		if (!$this->enabled) {
@@ -934,6 +891,8 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 * @param string $gid the internal group name
 	 * @param string $search optional, a search string
 	 * @return int|bool
+	 * @throws Exception
+	 * @throws ServerNotAvailableException
 	 */
 	public function countUsersInGroup($gid, $search = '') {
 		if ($this->groupPluginManager->implementsActions(GroupInterface::COUNT_USERS)) {
@@ -1003,8 +962,8 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 					continue;
 				}
 				// dn2username will also check if the users belong to the allowed base
-				if ($ocname = $this->access->dn2username($member)) {
-					$groupUsers[] = $ocname;
+				if ($ncGroupId = $this->access->dn2username($member)) {
+					$groupUsers[] = $ncGroupId;
 				}
 			}
 		}
@@ -1027,6 +986,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 * Uses a paged search if available to override a
 	 * server side search limit.
 	 * (active directory has a limit of 1000 by default)
+	 * @throws Exception
 	 */
 	public function getGroups($search = '', $limit = -1, $offset = 0) {
 		if (!$this->enabled) {
@@ -1035,7 +995,6 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 		$cacheKey = 'getGroups-' . $search . '-' . $limit . '-' . $offset;
 
 		//Check cache before driving unnecessary searches
-		\OCP\Util::writeLog('user_ldap', 'getGroups ' . $cacheKey, ILogger::DEBUG);
 		$ldap_groups = $this->access->connection->getFromCache($cacheKey);
 		if (!is_null($ldap_groups)) {
 			return $ldap_groups;
@@ -1050,7 +1009,6 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 			$this->access->connection->ldapGroupFilter,
 			$this->access->getFilterPartForGroupSearch($search)
 		]);
-		\OCP\Util::writeLog('user_ldap', 'getGroups Filter ' . $filter, ILogger::DEBUG);
 		$ldap_groups = $this->access->fetchListOfGroups($filter,
 			[$this->access->connection->ldapGroupDisplayName, 'dn'],
 			$limit,
@@ -1062,18 +1020,11 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	}
 
 	/**
-	 * @param string $group
-	 * @return bool
-	 */
-	public function groupMatchesFilter($group) {
-		return (strripos($group, $this->groupSearch) !== false);
-	}
-
-	/**
 	 * check if a group exists
 	 *
 	 * @param string $gid
 	 * @return bool
+	 * @throws ServerNotAvailableException
 	 */
 	public function groupExists($gid) {
 		$groupExists = $this->access->connection->getFromCache('groupExists' . $gid);
@@ -1094,7 +1045,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 			return false;
 		}
 
-		//if group really still exists, we will be able to read its objectclass
+		//if group really still exists, we will be able to read its objectClass
 		if (!is_array($this->access->readAttribute($dn, '', $this->access->connection->ldapGroupFilter))) {
 			$this->access->connection->writeToCache('groupExists' . $gid, false);
 			return false;
@@ -1104,6 +1055,10 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 		return true;
 	}
 
+	/**
+	 * @throws ServerNotAvailableException
+	 * @throws Exception
+	 */
 	protected function filterValidGroups(array $listOfGroups): array {
 		$validGroupDNs = [];
 		foreach ($listOfGroups as $key => $item) {
@@ -1147,7 +1102,8 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 *
 	 * @param string $gid
 	 * @return bool
-	 * @throws \Exception
+	 * @throws Exception
+	 * @throws ServerNotAvailableException
 	 */
 	public function createGroup($gid) {
 		if ($this->groupPluginManager->implementsActions(GroupInterface::CREATE_GROUP)) {
@@ -1167,7 +1123,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 			}
 			return $dn != null;
 		}
-		throw new \Exception('Could not create group in LDAP backend.');
+		throw new Exception('Could not create group in LDAP backend.');
 	}
 
 	/**
@@ -1175,7 +1131,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 *
 	 * @param string $gid gid of the group to delete
 	 * @return bool
-	 * @throws \Exception
+	 * @throws Exception
 	 */
 	public function deleteGroup($gid) {
 		if ($this->groupPluginManager->implementsActions(GroupInterface::DELETE_GROUP)) {
@@ -1186,7 +1142,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 			}
 			return $ret;
 		}
-		throw new \Exception('Could not delete group in LDAP backend.');
+		throw new Exception('Could not delete group in LDAP backend.');
 	}
 
 	/**
@@ -1195,7 +1151,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 * @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
-	 * @throws \Exception
+	 * @throws Exception
 	 */
 	public function addToGroup($uid, $gid) {
 		if ($this->groupPluginManager->implementsActions(GroupInterface::ADD_TO_GROUP)) {
@@ -1205,7 +1161,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 			}
 			return $ret;
 		}
-		throw new \Exception('Could not add user to group in LDAP backend.');
+		throw new Exception('Could not add user to group in LDAP backend.');
 	}
 
 	/**
@@ -1214,7 +1170,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 * @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
-	 * @throws \Exception
+	 * @throws Exception
 	 */
 	public function removeFromGroup($uid, $gid) {
 		if ($this->groupPluginManager->implementsActions(GroupInterface::REMOVE_FROM_GROUP)) {
@@ -1224,21 +1180,21 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 			}
 			return $ret;
 		}
-		throw new \Exception('Could not remove user from group in LDAP backend.');
+		throw new Exception('Could not remove user from group in LDAP backend.');
 	}
 
 	/**
 	 * Gets group details
 	 *
 	 * @param string $gid Name of the group
-	 * @return array | false
-	 * @throws \Exception
+	 * @return array|false
+	 * @throws Exception
 	 */
 	public function getGroupDetails($gid) {
 		if ($this->groupPluginManager->implementsActions(GroupInterface::GROUP_DETAILS)) {
 			return $this->groupPluginManager->getGroupDetails($gid);
 		}
-		throw new \Exception('Could not get group details in LDAP backend.');
+		throw new Exception('Could not get group details in LDAP backend.');
 	}
 
 	/**
@@ -1248,6 +1204,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 *
 	 * @param string $gid
 	 * @return resource of the LDAP connection
+	 * @throws ServerNotAvailableException
 	 */
 	public function getNewLDAPConnection($gid) {
 		$connection = clone $this->access->getConnection();
@@ -1255,7 +1212,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	}
 
 	/**
-	 * @throws \OC\ServerNotAvailableException
+	 * @throws ServerNotAvailableException
 	 */
 	public function getDisplayName(string $gid): string {
 		if ($this->groupPluginManager instanceof IGetDisplayNameBackend) {
diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php
index 60b8b06b3cf..f8dd5af167f 100644
--- a/apps/user_ldap/tests/Group_LDAPTest.php
+++ b/apps/user_ldap/tests/Group_LDAPTest.php
@@ -36,8 +36,10 @@ use OCA\User_LDAP\Connection;
 use OCA\User_LDAP\Group_LDAP as GroupLDAP;
 use OCA\User_LDAP\GroupPluginManager;
 use OCA\User_LDAP\ILDAPWrapper;
+use OCA\User_LDAP\Mapping\GroupMapping;
 use OCA\User_LDAP\User\Manager;
 use OCP\GroupInterface;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
 /**
@@ -49,18 +51,18 @@ use Test\TestCase;
  */
 class Group_LDAPTest extends TestCase {
 	/**
-	 * @return \PHPUnit_Framework_MockObject_MockObject|Access
+	 * @return MockObject|Access
 	 */
 	private function getAccessMock() {
 		static $conMethods;
 		static $accMethods;
 
 		if (is_null($conMethods) || is_null($accMethods)) {
-			$conMethods = get_class_methods('\OCA\User_LDAP\Connection');
-			$accMethods = get_class_methods('\OCA\User_LDAP\Access');
+			$conMethods = get_class_methods(Connection::class);
+			$accMethods = get_class_methods(Access::class);
 		}
 		$lw = $this->createMock(ILDAPWrapper::class);
-		$connector = $this->getMockBuilder('\OCA\User_LDAP\Connection')
+		$connector = $this->getMockBuilder(Connection::class)
 			->setMethods($conMethods)
 			->setConstructorArgs([$lw, null, null])
 			->getMock();
@@ -74,14 +76,14 @@ class Group_LDAPTest extends TestCase {
 		return $access;
 	}
 
+	/**
+	 * @return MockObject|GroupPluginManager
+	 */
 	private function getPluginManagerMock() {
-		return $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')->getMock();
+		return $this->createMock(GroupPluginManager::class);
 	}
 
-	/**
-	 * @param Access|\PHPUnit_Framework_MockObject_MockObject $access
-	 */
-	private function enableGroups($access) {
+	private function enableGroups(Access $access) {
 		$access->connection = $this->createMock(Connection::class);
 
 		$access->connection->expects($this->any())
@@ -104,7 +106,6 @@ class Group_LDAPTest extends TestCase {
 		$access->expects($this->any())
 			->method('groupname2dn')
 			->willReturn($groupDN);
-
 		$access->expects($this->any())
 			->method('readAttribute')
 			->willReturnCallback(function ($dn) use ($groupDN) {
@@ -121,7 +122,6 @@ class Group_LDAPTest extends TestCase {
 		$access->expects($this->any())
 			->method('isDNPartOfBase')
 			->willReturn(true);
-
 		// for primary groups
 		$access->expects($this->once())
 			->method('countUsers')
@@ -166,6 +166,9 @@ class Group_LDAPTest extends TestCase {
 		$access->expects($this->any())
 			->method('isDNPartOfBase')
 			->willReturn(true);
+		$access->expects($this->any())
+			->method('escapeFilterPart')
+			->willReturnArgument(0);
 
 		$groupBackend = new GroupLDAP($access, $pluginManager);
 		$users = $groupBackend->countUsersInGroup('group', '3');
@@ -174,8 +177,8 @@ class Group_LDAPTest extends TestCase {
 	}
 
 	public function testCountUsersWithPlugin() {
-		/** @var GroupPluginManager|\PHPUnit_Framework_MockObject_MockObject $pluginManager */
-		$pluginManager = $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')
+		/** @var GroupPluginManager|MockObject $pluginManager */
+		$pluginManager = $this->getMockBuilder(GroupPluginManager::class)
 			->setMethods(['implementsActions', 'countUsersInGroup'])
 			->getMock();
 
@@ -642,11 +645,9 @@ class Group_LDAPTest extends TestCase {
 		$access->expects($this->any())
 			->method('username2dn')
 			->willReturn($dn);
-
 		$access->expects($this->exactly(5))
 			->method('readAttribute')
 			->will($this->onConsecutiveCalls(['cn=groupA,dc=foobar', 'cn=groupB,dc=foobar'], [], [], [], []));
-
 		$access->expects($this->any())
 			->method('dn2groupname')
 			->willReturnArgument(0);
@@ -776,8 +777,8 @@ class Group_LDAPTest extends TestCase {
 	}
 
 	public function testCreateGroupWithPlugin() {
-		/** @var GroupPluginManager|\PHPUnit_Framework_MockObject_MockObject $pluginManager */
-		$pluginManager = $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')
+		/** @var GroupPluginManager|MockObject $pluginManager */
+		$pluginManager = $this->getMockBuilder(GroupPluginManager::class)
 			->setMethods(['implementsActions', 'createGroup'])
 			->getMock();
 
@@ -803,8 +804,8 @@ class Group_LDAPTest extends TestCase {
 	public function testCreateGroupFailing() {
 		$this->expectException(\Exception::class);
 
-		/** @var GroupPluginManager|\PHPUnit_Framework_MockObject_MockObject $pluginManager */
-		$pluginManager = $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')
+		/** @var GroupPluginManager|MockObject $pluginManager */
+		$pluginManager = $this->getMockBuilder(GroupPluginManager::class)
 			->setMethods(['implementsActions', 'createGroup'])
 			->getMock();
 
@@ -822,8 +823,8 @@ class Group_LDAPTest extends TestCase {
 	}
 
 	public function testDeleteGroupWithPlugin() {
-		/** @var GroupPluginManager|\PHPUnit_Framework_MockObject_MockObject $pluginManager */
-		$pluginManager = $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')
+		/** @var GroupPluginManager|MockObject $pluginManager */
+		$pluginManager = $this->getMockBuilder(GroupPluginManager::class)
 			->setMethods(['implementsActions', 'deleteGroup'])
 			->getMock();
 
@@ -837,7 +838,7 @@ class Group_LDAPTest extends TestCase {
 			->with('gid')
 			->willReturn('result');
 
-		$mapper = $this->getMockBuilder('\OCA\User_LDAP\Mapping\GroupMapping')
+		$mapper = $this->getMockBuilder(GroupMapping::class)
 			->setMethods(['unmap'])
 			->disableOriginalConstructor()
 			->getMock();
@@ -858,8 +859,8 @@ class Group_LDAPTest extends TestCase {
 	public function testDeleteGroupFailing() {
 		$this->expectException(\Exception::class);
 
-		/** @var GroupPluginManager|\PHPUnit_Framework_MockObject_MockObject $pluginManager */
-		$pluginManager = $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')
+		/** @var GroupPluginManager|MockObject $pluginManager */
+		$pluginManager = $this->getMockBuilder(GroupPluginManager::class)
 			->setMethods(['implementsActions', 'deleteGroup'])
 			->getMock();
 
@@ -877,8 +878,8 @@ class Group_LDAPTest extends TestCase {
 	}
 
 	public function testAddToGroupWithPlugin() {
-		/** @var GroupPluginManager|\PHPUnit_Framework_MockObject_MockObject $pluginManager */
-		$pluginManager = $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')
+		/** @var GroupPluginManager|MockObject $pluginManager */
+		$pluginManager = $this->getMockBuilder(GroupPluginManager::class)
 			->setMethods(['implementsActions', 'addToGroup'])
 			->getMock();
 
@@ -904,8 +905,8 @@ class Group_LDAPTest extends TestCase {
 	public function testAddToGroupFailing() {
 		$this->expectException(\Exception::class);
 
-		/** @var GroupPluginManager|\PHPUnit_Framework_MockObject_MockObject $pluginManager */
-		$pluginManager = $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')
+		/** @var GroupPluginManager|MockObject $pluginManager */
+		$pluginManager = $this->getMockBuilder(GroupPluginManager::class)
 			->setMethods(['implementsActions', 'addToGroup'])
 			->getMock();
 
@@ -923,8 +924,8 @@ class Group_LDAPTest extends TestCase {
 	}
 
 	public function testRemoveFromGroupWithPlugin() {
-		/** @var GroupPluginManager|\PHPUnit_Framework_MockObject_MockObject $pluginManager */
-		$pluginManager = $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')
+		/** @var GroupPluginManager|MockObject $pluginManager */
+		$pluginManager = $this->getMockBuilder(GroupPluginManager::class)
 			->setMethods(['implementsActions', 'removeFromGroup'])
 			->getMock();
 
@@ -950,8 +951,8 @@ class Group_LDAPTest extends TestCase {
 	public function testRemoveFromGroupFailing() {
 		$this->expectException(\Exception::class);
 
-		/** @var GroupPluginManager|\PHPUnit_Framework_MockObject_MockObject $pluginManager */
-		$pluginManager = $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')
+		/** @var GroupPluginManager|MockObject $pluginManager */
+		$pluginManager = $this->getMockBuilder(GroupPluginManager::class)
 			->setMethods(['implementsActions', 'removeFromGroup'])
 			->getMock();
 
@@ -969,8 +970,8 @@ class Group_LDAPTest extends TestCase {
 	}
 
 	public function testGetGroupDetailsWithPlugin() {
-		/** @var GroupPluginManager|\PHPUnit_Framework_MockObject_MockObject $pluginManager */
-		$pluginManager = $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')
+		/** @var GroupPluginManager|MockObject $pluginManager */
+		$pluginManager = $this->getMockBuilder(GroupPluginManager::class)
 			->setMethods(['implementsActions', 'getGroupDetails'])
 			->getMock();
 
@@ -996,8 +997,8 @@ class Group_LDAPTest extends TestCase {
 	public function testGetGroupDetailsFailing() {
 		$this->expectException(\Exception::class);
 
-		/** @var GroupPluginManager|\PHPUnit_Framework_MockObject_MockObject $pluginManager */
-		$pluginManager = $this->getMockBuilder('\OCA\User_LDAP\GroupPluginManager')
+		/** @var GroupPluginManager|MockObject $pluginManager */
+		$pluginManager = $this->getMockBuilder(GroupPluginManager::class)
 			->setMethods(['implementsActions', 'getGroupDetails'])
 			->getMock();