From 873222f0c154fe58e61c9984fd98bb9c60820c12 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 11 Oct 2023 14:06:27 +0200 Subject: [PATCH] fix(LDAP): solve race condition reading groups of disappeared LDAP user Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Group_LDAP.php | 45 +++++++++++-- apps/user_ldap/lib/Group_Proxy.php | 6 +- apps/user_ldap/tests/Group_LDAPTest.php | 87 ++++++++++++++++++++++++- 3 files changed, 131 insertions(+), 7 deletions(-) diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 429818503f3..b2bdd09ef9f 100644 --- 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 ae355f8e53e..f96feb7b023 100644 --- 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 764ce8a2844..8d6c4539cec 100644 --- 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], -- 2.39.5