Merge pull request #8536 from nextcloud/8522-stable13

[stable13] Fix retrieval of group members with numerical uids from LDAP

2 files changed, 92 insertions, 8 deletions

diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
index 3faa96bc2b8..0f4bb2d8b6a 100644
--- a/apps/user_ldap/lib/Group_LDAP.php
+++ b/apps/user_ldap/lib/Group_LDAP.php
@@ -207,6 +207,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 	 * @param string $dnGroup
 	 * @param array|null &$seen
 	 * @return array|mixed|null
+	 * @throws \OC\ServerNotAvailableException
 	 */
 	private function _groupMembers($dnGroup, &$seen = null) {
 		if ($seen === null) {
@@ -220,26 +221,26 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
 		// used extensively in cron job, caching makes sense for nested groups
 		$cacheKey = '_groupMembers'.$dnGroup;
 		$groupMembers = $this->access->connection->getFromCache($cacheKey);
-		if(!is_null($groupMembers)) {
+		if($groupMembers !== null) {
 			return $groupMembers;
 		}
 		$seen[$dnGroup] = 1;
 		$members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr,
 												$this->access->connection->ldapGroupFilter);
 		if (is_array($members)) {
-			foreach ($members as $memberDN) {
-				$allMembers[$memberDN] = 1;
+			foreach ($members as $member) {
+				$allMembers[$member] = 1;
 				$nestedGroups = $this->access->connection->ldapNestedGroups;
 				if (!empty($nestedGroups)) {
-					$subMembers = $this->_groupMembers($memberDN, $seen);
+					$subMembers = $this->_groupMembers($member, $seen);
 					if ($subMembers) {
-						$allMembers = array_merge($allMembers, $subMembers);
+						$allMembers += $subMembers;
 					}
 				}
 			}
 		}
 
-		$allMembers = array_merge($allMembers, $this->getDynamicGroupMembers($dnGroup));
+		$allMembers += $this->getDynamicGroupMembers($dnGroup);
 
 		$this->access->connection->writeToCache($cacheKey, $allMembers);
 		return $allMembers;
diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php
index e758bc9bd02..e9ed1e66b20 100644
--- a/apps/user_ldap/tests/Group_LDAPTest.php
+++ b/apps/user_ldap/tests/Group_LDAPTest.php
@@ -39,6 +39,7 @@ use OCA\User_LDAP\Connection;
 use OCA\User_LDAP\Group_LDAP as GroupLDAP;
 use OCA\User_LDAP\ILDAPWrapper;
 use OCA\User_LDAP\User\Manager;
+use Test\TestCase;
 
 /**
  * Class GroupLDAPTest
@@ -47,7 +48,7 @@ use OCA\User_LDAP\User\Manager;
  *
  * @package OCA\User_LDAP\Tests
  */
-class Group_LDAPTest extends \Test\TestCase {
+class Group_LDAPTest extends TestCase {
 	/**
 	 * @return \PHPUnit_Framework_MockObject_MockObject|Access
 	 */
@@ -965,6 +966,88 @@ class Group_LDAPTest extends \Test\TestCase {
 		$ldap = new GroupLDAP($access, $pluginManager);
 
 		$ldap->getGroupDetails('gid');
-	}	
+	}
+
+	public function groupMemberProvider() {
+		$base = 'dc=species,dc=earth';
+
+		$groups0 = [
+			'uid=3723,' . $base,
+			'uid=8372,' . $base,
+			'uid=8427,' . $base,
+			'uid=2333,' . $base,
+			'uid=4754,' . $base,
+		];
+		$groups1 = [
+			'3723',
+			'8372',
+			'8427',
+			'2333',
+			'4754',
+		];
+		$groups2Nested = ['6642', '1424'];
+		$expGroups2 = array_merge($groups1, $groups2Nested);
+
+		return [
+			[ #0 – test DNs
+				'cn=Birds,' . $base,
+				$groups0,
+				['cn=Birds,' . $base => $groups0]
+			],
+			[ #1 – test uids
+				'cn=Birds,' . $base,
+				$groups1,
+				['cn=Birds,' . $base => $groups1]
+			],
+			[ #2 – test uids with nested groups
+				'cn=Birds,' . $base,
+				$expGroups2,
+				[
+					'cn=Birds,' . $base => $groups1,
+					'8427' => $groups2Nested, // simplified - nested groups would work with DNs
+				],
+			],
+		];
+	}
+
+	/**
+	 * @param string $groupDN
+	 * @param string[] $expectedMembers
+	 * @param array $groupsInfo
+	 * @dataProvider groupMemberProvider
+	 */
+	public function testGroupMembers($groupDN, $expectedMembers, $groupsInfo = null) {
+		$access = $this->getAccessMock();
+		$access->expects($this->any())
+			->method('readAttribute')
+			->willReturnCallback(function($group) use ($groupDN, $expectedMembers, $groupsInfo) {
+				if(isset($groupsInfo[$group])) {
+					return $groupsInfo[$group];
+				}
+				return [];
+			});
+
+		$access->connection = $this->createMock(Connection::class);
+		if(count($groupsInfo) > 1) {
+			$access->connection->expects($this->any())
+				->method('__get')
+				->willReturnCallback(function($name) {
+					if($name === 'ldapNestedGroups') {
+						return 1;
+					}
+					return null;
+				});
+		}
+
+		/** @var GroupPluginManager $pluginManager */
+		$pluginManager = $this->createMock(GroupPluginManager::class);
+
+		$ldap = new GroupLDAP($access, $pluginManager);
+		$resultingMembers = $this->invokePrivate($ldap, '_groupMembers', [$groupDN]);
+
+		$expected = array_keys(array_flip($expectedMembers));
+
+		$this->assertEquals($expected, array_keys($resultingMembers), '', 0.0, 10, true);
+	}
 
 }