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

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
index 429818503f3f9778d4243111f7239a8ecd5b9251..b2bdd09ef9f447249d4d0c238d78fdbc3048bb2d 100644 (file)
--- a/apps/user_ldap/lib/Group_LDAP.php
+++ b/apps/user_ldap/lib/Group_LDAP.php
@@ -52,6 +52,7 @@ use OCP\Group\Backend\IDeleteGroupBackend;
 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 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
                $this->logger = Server::get(LoggerInterface::class);
                $this->ldapGroupMemberAssocAttr = strtolower((string)$gAssoc);
                $this->config = $config;
+               $this->ncUserManager = $ncUserManager;
        }
 
        /**
@@ -443,6 +451,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
        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 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
                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 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
                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 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
                        $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
index ae355f8e53ed20f41a0a89f8ffb945ea469ff1ee..f96feb7b023e47537a9ec141c73687594dee0e7f 100644 (file)
--- a/apps/user_ldap/lib/Group_Proxy.php
+++ b/apps/user_ldap/lib/Group_Proxy.php
@@ -33,6 +33,7 @@ use OCP\Group\Backend\IGetDisplayNameBackend;
 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,6 +42,7 @@ 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,
@@ -48,11 +50,13 @@ class Group_Proxy extends Proxy implements \OCP\GroupInterface, IGroupLDAP, IGet
                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 @@ class Group_Proxy extends Proxy implements \OCP\GroupInterface, IGroupLDAP, IGet
                $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
index 764ce8a284449a8343242adaf38aa16d9879728a..8d6c4539cec80feeea95a6d803e0e41abbf0bcd5 100644 (file)
--- a/apps/user_ldap/tests/Group_LDAPTest.php
+++ b/apps/user_ldap/tests/Group_LDAPTest.php
@@ -38,8 +38,12 @@ use OCA\User_LDAP\ILDAPWrapper;
 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 @@ class Group_LDAPTest extends TestCase {
                $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 @@ class Group_LDAPTest extends TestCase {
                $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 @@ class Group_LDAPTest extends TestCase {
                        ->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 @@ class Group_LDAPTest extends TestCase {
                        ->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 @@ class Group_LDAPTest extends TestCase {
                $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],