Merge pull request #24402 from nextcloud/fix/24252/ldap-ingroup-memberid

LDAP: fix inGroup for memberUid type of group memberships

2 files changed, 238 insertions, 54 deletions

diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
index 5444f0815e3..be2fbecad85 100644
--- a/apps/user_ldap/lib/Group_LDAP.php
+++ b/apps/user_ldap/lib/Group_LDAP.php
@@ -55,7 +55,7 @@ use OCP\ILogger;
 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 */
 	protected $cachedGroupsByMember;
@@ -136,17 +136,13 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
 
 		//usually, LDAP attributes are said to be case insensitive. But there are exceptions of course.
 		$members = $this->_groupMembers($groupDN);
-		if (!is_array($members) || count($members) === 0) {
-			$this->access->connection->writeToCache($cacheKey, false);
-			return false;
-		}
 
 		//extra work if we don't get back user DNs
 		switch ($this->ldapGroupMemberAssocAttr) {
 			case 'memberuid':
 			case 'zimbramailforwardingaddress':
 				$requestAttributes = $this->access->userManager->getAttributes(true);
-				$dns = [];
+				$users = [];
 				$filterParts = [];
 				$bytes = 0;
 				foreach ($members as $mid) {
@@ -160,22 +156,37 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
 					if ($bytes >= 9000000) {
 						// AD has a default input buffer of 10 MB, we do not want
 						// to take even the chance to exceed it
+						// so we fetch results with the filterParts we collected so far
 						$filter = $this->access->combineFilterWithOr($filterParts);
-						$users = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
+						$search = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
 						$bytes = 0;
 						$filterParts = [];
-						$dns = array_merge($dns, $users);
+						$users = array_merge($users, $search);
 					}
 				}
+
 				if (count($filterParts) > 0) {
+					// if there are filterParts left we need to add their result
 					$filter = $this->access->combineFilterWithOr($filterParts);
-					$users = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
-					$dns = array_merge($dns, $users);
+					$search = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
+					$users = array_merge($users, $search);
 				}
-				$members = $dns;
+
+				// now we cleanup the users array to get only dns
+				$dns = [];
+				foreach ($users as $record) {
+					$dns[$record['dn'][0]] = 1;
+				}
+				$members = array_keys($dns);
+
 				break;
 		}
 
+		if (count($members) === 0) {
+			$this->access->connection->writeToCache($cacheKey, false);
+			return false;
+		}
+
 		$isInGroup = in_array($userDN, $members);
 		$this->access->connection->writeToCache($cacheKey, $isInGroup);
 		$this->access->connection->writeToCache($cacheKeyMembers, $members);
diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php
index 74dd2b467cf..bc582ab90b5 100644
--- a/apps/user_ldap/tests/Group_LDAPTest.php
+++ b/apps/user_ldap/tests/Group_LDAPTest.php
@@ -50,6 +50,47 @@ use Test\TestCase;
  * @package OCA\User_LDAP\Tests
  */
 class Group_LDAPTest extends TestCase {
+	public function testCountEmptySearchString() {
+		$access = $this->getAccessMock();
+		$pluginManager = $this->getPluginManagerMock();
+		$groupDN = 'cn=group,dc=foo,dc=bar';
+
+		$this->enableGroups($access);
+
+		$access->expects($this->any())
+			->method('groupname2dn')
+			->willReturn($groupDN);
+		$access->expects($this->any())
+			->method('readAttribute')
+			->willReturnCallback(function ($dn) use ($groupDN) {
+				if ($dn === $groupDN) {
+					return [
+						'uid=u11,ou=users,dc=foo,dc=bar',
+						'uid=u22,ou=users,dc=foo,dc=bar',
+						'uid=u33,ou=users,dc=foo,dc=bar',
+						'uid=u34,ou=users,dc=foo,dc=bar'
+					];
+				}
+				return [];
+			});
+		$access->expects($this->any())
+			->method('isDNPartOfBase')
+			->willReturn(true);
+		// for primary groups
+		$access->expects($this->once())
+			->method('countUsers')
+			->willReturn(2);
+
+		$access->userManager->expects($this->any())
+			->method('getAttributes')
+			->willReturn(['displayName', 'mail']);
+
+		$groupBackend = new GroupLDAP($access, $pluginManager);
+		$users = $groupBackend->countUsersInGroup('group');
+
+		$this->assertSame(6, $users);
+	}
+
 	/**
 	 * @return MockObject|Access
 	 */
@@ -98,47 +139,6 @@ class Group_LDAPTest extends TestCase {
 			});
 	}
 
-	public function testCountEmptySearchString() {
-		$access = $this->getAccessMock();
-		$pluginManager = $this->getPluginManagerMock();
-		$groupDN = 'cn=group,dc=foo,dc=bar';
-
-		$this->enableGroups($access);
-
-		$access->expects($this->any())
-			->method('groupname2dn')
-			->willReturn($groupDN);
-		$access->expects($this->any())
-			->method('readAttribute')
-			->willReturnCallback(function ($dn) use ($groupDN) {
-				if ($dn === $groupDN) {
-					return [
-						'uid=u11,ou=users,dc=foo,dc=bar',
-						'uid=u22,ou=users,dc=foo,dc=bar',
-						'uid=u33,ou=users,dc=foo,dc=bar',
-						'uid=u34,ou=users,dc=foo,dc=bar'
-					];
-				}
-				return [];
-			});
-		$access->expects($this->any())
-			->method('isDNPartOfBase')
-			->willReturn(true);
-		// for primary groups
-		$access->expects($this->once())
-			->method('countUsers')
-			->willReturn(2);
-
-		$access->userManager->expects($this->any())
-			->method('getAttributes')
-			->willReturn(['displayName', 'mail']);
-
-		$groupBackend = new GroupLDAP($access, $pluginManager);
-		$users = $groupBackend->countUsersInGroup('group');
-
-		$this->assertSame(6, $users);
-	}
-
 	public function testCountWithSearchString() {
 		$access = $this->getAccessMock();
 		$pluginManager = $this->getPluginManagerMock();
@@ -503,6 +503,179 @@ class Group_LDAPTest extends TestCase {
 		$groupBackend->inGroup($uid, $gid);
 	}
 
+	public function groupWithMembersProvider() {
+		return [
+			[
+				'someGroup',
+				'cn=someGroup,ou=allTheGroups,ou=someDepartment,dc=someDomain,dc=someTld',
+				[
+					'uid=oneUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld',
+					'uid=someUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld',
+					'uid=anotherUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld',
+					'uid=differentUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld',
+				],
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider groupWithMembersProvider
+	 */
+	public function testInGroupMember(string $gid, string $groupDn, array $memberDNs) {
+		$access = $this->getAccessMock();
+		$pluginManager = $this->getPluginManagerMock();
+
+		$access->connection = $this->createMock(Connection::class);
+
+		$uid = 'someUser';
+		$userDn = $memberDNs[0];
+
+		$access->connection->expects($this->any())
+			->method('__get')
+			->willReturnCallback(function ($name) {
+				switch ($name) {
+					case 'ldapGroupMemberAssocAttr':
+						return 'member';
+					case 'ldapDynamicGroupMemberURL':
+						return '';
+					case 'hasPrimaryGroups':
+					case 'ldapNestedGroups':
+						return 0;
+					default:
+						return 1;
+				}
+			});
+		$access->connection->expects($this->any())
+			->method('getFromCache')
+			->willReturn(null);
+
+		$access->expects($this->once())
+			->method('username2dn')
+			->with($uid)
+			->willReturn($userDn);
+		$access->expects($this->once())
+			->method('groupname2dn')
+			->willReturn($groupDn);
+		$access->expects($this->any())
+			->method('readAttribute')
+			->willReturn($memberDNs);
+
+		$groupBackend = new GroupLDAP($access, $pluginManager);
+		$this->assertTrue($groupBackend->inGroup($uid, $gid));
+	}
+
+	/**
+	 * @dataProvider groupWithMembersProvider
+	 */
+	public function testInGroupMemberNot(string $gid, string $groupDn, array $memberDNs) {
+		$access = $this->getAccessMock();
+		$pluginManager = $this->getPluginManagerMock();
+
+		$access->connection = $this->createMock(Connection::class);
+
+		$uid = 'unelatedUser';
+		$userDn = 'uid=unrelatedUser,ou=unrelatedTeam,ou=unrelatedDepartment,dc=someDomain,dc=someTld';
+
+		$access->connection->expects($this->any())
+			->method('__get')
+			->willReturnCallback(function ($name) {
+				switch ($name) {
+					case 'ldapGroupMemberAssocAttr':
+						return 'member';
+					case 'ldapDynamicGroupMemberURL':
+						return '';
+					case 'hasPrimaryGroups':
+					case 'ldapNestedGroups':
+						return 0;
+					default:
+						return 1;
+				}
+			});
+		$access->connection->expects($this->any())
+			->method('getFromCache')
+			->willReturn(null);
+
+		$access->expects($this->once())
+			->method('username2dn')
+			->with($uid)
+			->willReturn($userDn);
+		$access->expects($this->once())
+			->method('groupname2dn')
+			->willReturn($groupDn);
+		$access->expects($this->any())
+			->method('readAttribute')
+			->willReturn($memberDNs);
+
+		$groupBackend = new GroupLDAP($access, $pluginManager);
+		$this->assertFalse($groupBackend->inGroup($uid, $gid));
+	}
+
+	/**
+	 * @dataProvider groupWithMembersProvider
+	 */
+	public function testInGroupMemberUid(string $gid, string $groupDn, array $memberDNs) {
+		$access = $this->getAccessMock();
+		$pluginManager = $this->getPluginManagerMock();
+
+		$memberUids = [];
+		$userRecords = [];
+		foreach ($memberDNs as $dn) {
+			$memberUids[] = ldap_explode_dn($dn, false)[0];
+			$userRecords[] = ['dn' => [$dn]];
+		}
+
+
+		$access->connection = $this->createMock(Connection::class);
+
+		$uid = 'someUser';
+		$userDn = $memberDNs[0];
+
+		$access->connection->expects($this->any())
+			->method('__get')
+			->willReturnCallback(function ($name) {
+				switch ($name) {
+					case 'ldapGroupMemberAssocAttr':
+						return 'memberUid';
+					case 'ldapDynamicGroupMemberURL':
+						return '';
+					case 'ldapLoginFilter':
+						return 'uid=%uid';
+					case 'hasPrimaryGroups':
+					case 'ldapNestedGroups':
+						return 0;
+					default:
+						return 1;
+				}
+			});
+		$access->connection->expects($this->any())
+			->method('getFromCache')
+			->willReturn(null);
+
+		$access->userManager->expects($this->any())
+			->method('getAttributes')
+			->willReturn(['uid', 'mail', 'displayname']);
+
+		$access->expects($this->once())
+			->method('username2dn')
+			->with($uid)
+			->willReturn($userDn);
+		$access->expects($this->once())
+			->method('groupname2dn')
+			->willReturn($groupDn);
+		$access->expects($this->any())
+			->method('readAttribute')
+			->willReturn($memberUids);
+		$access->expects($this->any())
+			->method('fetchListOfUsers')
+			->willReturn($userRecords);
+		$access->expects($this->any())
+			->method('combineFilterWithOr')
+			->willReturn('(|(pseudo=filter)(filter=pseudo))');
+
+		$groupBackend = new GroupLDAP($access, $pluginManager);
+		$this->assertTrue($groupBackend->inGroup($uid, $gid));
+	}
+
 	public function testGetGroupsWithOffset() {
 		$access = $this->getAccessMock();
 		$pluginManager = $this->getPluginManagerMock();
@@ -721,8 +894,8 @@ class Group_LDAPTest extends TestCase {
 
 	public function nestedGroupsProvider(): array {
 		return [
-			[ true ],
-			[ false ],
+			[true],
+			[false],
 		];
 	}