fix(LDAP): solve race condition reading groups of disappeared LDAP user

blizzz · juliusknorr · commit dee73ac390db · 2023-10-25T10:31:15.000+02:00
Signed-off-by: Arthur Schiwon &lt;blizzz@arthur-schiwon.de&gt;
diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
@@ -52,6 +52,7 @@
 use OCP\Group\Backend\IGetDisplayNameBackend;
 use OC\ServerNotAvailableException;
 use OCP\IConfig;
+use OCP\IUserManager;
 use OCP\Server;
 use Psr\Log\LoggerInterface;
 use function json_decode;
@@ -73,8 +74,14 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
 	 */
 	protected string $ldapGroupMemberAssocAttr;
 	private IConfig $config;
-
-	public function __construct(Access $access, GroupPluginManager $groupPluginManager, IConfig $config) {
+	private IUserManager $ncUserManager;
+
+	public function __construct(
+		Access $access,
+		GroupPluginManager $groupPluginManager,
+		IConfig $config,
+		IUserManager $ncUserManager
+	) {
 		parent::__construct($access);
 		$filter = $this->access->connection->ldapGroupFilter;
 		$gAssoc = $this->access->connection->ldapGroupMemberAssocAttr;
@@ -89,6 +96,7 @@ public function __construct(Access $access, GroupPluginManager $groupPluginManag
 		$this->logger = Server::get(LoggerInterface::class);
 		$this->ldapGroupMemberAssocAttr = strtolower((string)$gAssoc);
 		$this->config = $config;
+		$this->ncUserManager = $ncUserManager;
 	}
 
 	/**
@@ -443,6 +451,7 @@ public function getGroupGidNumber(string $dn) {
 	public function getUserGidNumber(string $dn) {
 		$gidNumber = false;
 		if ($this->access->connection->hasGidNumber) {
+			// FIXME: when $dn does not exist on LDAP anymore, this will be set wrongly to false :/
 			$gidNumber = $this->getEntryGidNumber($dn, $this->access->connection->ldapGidNumber);
 			if ($gidNumber === false) {
 				$this->access->connection->hasGidNumber = false;
@@ -656,6 +665,25 @@ public function getUserPrimaryGroup(string $dn) {
 		return false;
 	}
 
+	private function isUserOnLDAP(string $uid): bool {
+		// forces a user exists check - but does not help if a positive result is cached, while group info is not
+		$ncUser = $this->ncUserManager->get($uid);
+		if ($ncUser === null) {
+			return false;
+		}
+		$backend = $ncUser->getBackend();
+		if ($backend instanceof User_Proxy) {
+			// ignoring cache as safeguard (and we are behind the group cache check anyway)
+			return $backend->userExistsOnLDAP($uid, true);
+		}
+		return false;
+	}
+
+	protected function getCachedGroupsForUserId(string $uid): array {
+		$groupStr = $this->config->getUserValue($uid, 'user_ldap', 'cached-group-memberships-' . $this->access->connection->getConfigPrefix(), '[]');
+		return json_decode($groupStr) ?? [];
+	}
+
 	/**
 	 * This function fetches all groups a user belongs to. It does not check
 	 * if the user exists at all.
@@ -683,8 +711,7 @@ public function getUserGroups($uid): array {
 		if ($user instanceof OfflineUser) {
 			// We load known group memberships from configuration for remnants,
 			// because LDAP server does not contain them anymore
-			$groupStr = $this->config->getUserValue($uid, 'user_ldap', 'cached-group-memberships-' . $this->access->connection->getConfigPrefix(), '[]');
-			return json_decode($groupStr) ?? [];
+			return $this->getCachedGroupsForUserId($uid);
 		}
 
 		$userDN = $this->access->username2dn($uid);
@@ -798,8 +825,18 @@ public function getUserGroups($uid): array {
 			$groups[] = $gidGroupName;
 		}
 
+		if (empty($groups) && !$this->isUserOnLDAP($ncUid)) {
+			// Groups are enabled, but you user has none? Potentially suspicious:
+			// it could be that the user was deleted from LDAP, but we are not
+			// aware of it yet.
+			$groups = $this->getCachedGroupsForUserId($ncUid);
+			$this->access->connection->writeToCache($cacheKey, $groups);
+			return $groups;
+		}
+
 		$groups = array_unique($groups, SORT_LOCALE_STRING);
 		$this->access->connection->writeToCache($cacheKey, $groups);
+
 		$groupStr = \json_encode($groups);
 		$this->config->setUserValue($ncUid, 'user_ldap', 'cached-group-memberships-' . $this->access->connection->getConfigPrefix(), $groupStr);
 
diff --git a/apps/user_ldap/lib/Group_Proxy.php b/apps/user_ldap/lib/Group_Proxy.php
@@ -33,6 +33,7 @@
 use OCP\Group\Backend\INamedBackend;
 use OCP\GroupInterface;
 use OCP\IConfig;
+use OCP\IUserManager;
 
 class Group_Proxy extends Proxy implements \OCP\GroupInterface, IGroupLDAP, IGetDisplayNameBackend, INamedBackend, IDeleteGroupBackend {
 	private $backends = [];
@@ -41,18 +42,21 @@ class Group_Proxy extends Proxy implements \OCP\GroupInterface, IGroupLDAP, IGet
 	private GroupPluginManager $groupPluginManager;
 	private bool $isSetUp = false;
 	private IConfig $config;
+	private IUserManager $ncUserManager;
 
 	public function __construct(
 		Helper $helper,
 		ILDAPWrapper $ldap,
 		AccessFactory $accessFactory,
 		GroupPluginManager $groupPluginManager,
 		IConfig $config,
+		IUserManager $ncUserManager,
 	) {
 		parent::__construct($ldap, $accessFactory);
 		$this->helper = $helper;
 		$this->groupPluginManager = $groupPluginManager;
 		$this->config = $config;
+		$this->ncUserManager = $ncUserManager;
 	}
 
 	protected function setup(): void {
@@ -63,7 +67,7 @@ protected function setup(): void {
 		$serverConfigPrefixes = $this->helper->getServerConfigurationPrefixes(true);
 		foreach ($serverConfigPrefixes as $configPrefix) {
 			$this->backends[$configPrefix] =
-				new Group_LDAP($this->getAccess($configPrefix), $this->groupPluginManager, $this->config);
+				new Group_LDAP($this->getAccess($configPrefix), $this->groupPluginManager, $this->config, $this->ncUserManager);
 			if (is_null($this->refBackend)) {
 				$this->refBackend = &$this->backends[$configPrefix];
 			}
diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php
@@ -38,8 +38,12 @@
 use OCA\User_LDAP\Mapping\GroupMapping;
 use OCA\User_LDAP\User\Manager;
 use OCA\User_LDAP\User\OfflineUser;
+use OCA\User_LDAP\User\User;
+use OCA\User_LDAP\User_Proxy;
 use OCP\GroupInterface;
 use OCP\IConfig;
+use OCP\IUser;
+use OCP\IUserManager;
 use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
@@ -54,6 +58,7 @@ class Group_LDAPTest extends TestCase {
 	private MockObject|Access $access;
 	private MockObject|GroupPluginManager $pluginManager;
 	private MockObject|IConfig $config;
+	private MockObject|IUserManager $ncUserManager;
 	private GroupLDAP $groupBackend;
 
 	public function setUp(): void {
@@ -62,10 +67,11 @@ public function setUp(): void {
 		$this->access = $this->getAccessMock();
 		$this->pluginManager = $this->createMock(GroupPluginManager::class);
 		$this->config = $this->createMock(IConfig::class);
+		$this->ncUserManager = $this->createMock(IUserManager::class);
 	}
 
 	public function initBackend(): void {
-		$this->groupBackend = new GroupLDAP($this->access, $this->pluginManager, $this->config);
+		$this->groupBackend = new GroupLDAP($this->access, $this->pluginManager, $this->config, $this->ncUserManager);
 	}
 
 	public function testCountEmptySearchString() {
@@ -786,12 +792,14 @@ public function testGetUserGroupsMemberOf() {
 		$this->access->connection->hasPrimaryGroups = false;
 		$this->access->connection->hasGidNumber = false;
 
+		$expectedGroups = ['cn=groupA,dc=foobar', 'cn=groupB,dc=foobar'];
+
 		$this->access->expects($this->any())
 			->method('username2dn')
 			->willReturn($dn);
 		$this->access->expects($this->exactly(5))
 			->method('readAttribute')
-			->will($this->onConsecutiveCalls(['cn=groupA,dc=foobar', 'cn=groupB,dc=foobar'], [], [], [], []));
+			->will($this->onConsecutiveCalls($expectedGroups, [], [], [], []));
 		$this->access->expects($this->any())
 			->method('dn2groupname')
 			->willReturnArgument(0);
@@ -802,6 +810,10 @@ public function testGetUserGroupsMemberOf() {
 			->method('isDNPartOfBase')
 			->willReturn(true);
 
+		$this->config->expects($this->once())
+			->method('setUserValue')
+			->with('userX', 'user_ldap', 'cached-group-memberships-', \json_encode($expectedGroups));
+
 		$this->initBackend();
 		$groups = $this->groupBackend->getUserGroups('userX');
 
@@ -835,6 +847,34 @@ public function testGetUserGroupsMemberOfDisabled() {
 			->method('nextcloudGroupNames')
 			->willReturn([]);
 
+		// empty group result should not be oer
+		$this->config->expects($this->once())
+			->method('setUserValue')
+			->with('userX', 'user_ldap', 'cached-group-memberships-', '[]');
+
+		$ldapUser = $this->createMock(User::class);
+
+		$this->access->userManager->expects($this->any())
+			->method('get')
+			->with('userX')
+			->willReturn($ldapUser);
+
+		$userBackend = $this->createMock(User_Proxy::class);
+		$userBackend->expects($this->once())
+			->method('userExistsOnLDAP')
+			->with('userX', true)
+			->willReturn(true);
+
+		$ncUser = $this->createMock(IUser::class);
+		$ncUser->expects($this->any())
+			->method('getBackend')
+			->willReturn($userBackend);
+
+		$this->ncUserManager->expects($this->once())
+			->method('get')
+			->with('userX')
+			->willReturn($ncUser);
+
 		$this->initBackend();
 		$this->groupBackend->getUserGroups('userX');
 	}
@@ -861,6 +901,49 @@ public function testGetUserGroupsOfflineUser() {
 		$this->assertTrue(in_array('groupF', $returnedGroups));
 	}
 
+	public function testGetUserGroupsUnrecognizedOfflineUser() {
+		$this->enableGroups();
+		$dn = 'cn=userX,dc=foobar';
+
+		$ldapUser = $this->createMock(User::class);
+
+		$userBackend = $this->createMock(User_Proxy::class);
+		$userBackend->expects($this->once())
+			->method('userExistsOnLDAP')
+			->with('userX', true)
+			->willReturn(false);
+
+		$ncUser = $this->createMock(IUser::class);
+		$ncUser->expects($this->any())
+			->method('getBackend')
+			->willReturn($userBackend);
+
+		$this->config->expects($this->atLeastOnce())
+			->method('getUserValue')
+			->with('userX', 'user_ldap', 'cached-group-memberships-', $this->anything())
+			->willReturn(\json_encode(['groupB', 'groupF']));
+
+		$this->access->expects($this->any())
+			->method('username2dn')
+			->willReturn($dn);
+
+		$this->access->userManager->expects($this->any())
+			->method('get')
+			->with('userX')
+			->willReturn($ldapUser);
+
+		$this->ncUserManager->expects($this->once())
+			->method('get')
+			->with('userX')
+			->willReturn($ncUser);
+
+		$this->initBackend();
+		$returnedGroups = $this->groupBackend->getUserGroups('userX');
+		$this->assertCount(2, $returnedGroups);
+		$this->assertTrue(in_array('groupB', $returnedGroups));
+		$this->assertTrue(in_array('groupF', $returnedGroups));
+	}
+
 	public function nestedGroupsProvider(): array {
 		return [
 			[true],
