open-dsi
/
nextcloud-server
mirror of https://github.com/nextcloud/server
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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

Browse Source 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
pull/40443/head
Arthur Schiwon 2 years ago
parent b848841e3d
commit cce8d0a7a5
No known key found for this signature in database
GPG Key ID: 7424F1874854DF23
3 changed files with 131 additions and 7 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 45
      apps/user_ldap/lib/Group_LDAP.php
  2. 6
      apps/user_ldap/lib/Group_Proxy.php
  3. 87
      apps/user_ldap/tests/Group_LDAPTest.php

45
apps/user_ldap/lib/Group_LDAP.php
Unescape View File

@ -53,6 +53,7 @@ use OCP\Group\Backend\ABackend;
use OCP\Group\Backend\IDeleteGroupBackend;
use OCP\Group\Backend\IGetDisplayNameBackend;
use OCP\IConfig;
use OCP\IUserManager;
use OCP\Server;
use Psr\Log\LoggerInterface;
use function json_decode;
@ -75,8 +76,14 @@ class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP, IGetDis
*/
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
) {
$this->access = $access;
$filter = $this->access->connection->ldapGroupFilter;
$gAssoc = $this->access->connection->ldapGroupMemberAssocAttr;
@ -91,6 +98,7 @@ class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP, IGetDis
$this->logger = Server::get(LoggerInterface::class);
$this->ldapGroupMemberAssocAttr = strtolower((string)$gAssoc);
$this->config = $config;
$this->ncUserManager = $ncUserManager;
}
/**
@ -445,6 +453,7 @@ class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP, IGetDis
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;
@ -659,6 +668,25 @@ class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP, IGetDis
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.
@ -686,8 +714,7 @@ class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP, IGetDis
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);
@ -801,8 +828,18 @@ class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP, IGetDis
$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);

6
apps/user_ldap/lib/Group_Proxy.php
Unescape View File

@ -36,6 +36,7 @@ use OCP\Group\Backend\IGroupDetailsBackend;
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, IBatchMethodsBackend {
private $backends = [];
@ -44,6 +45,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,
@ -51,11 +53,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 {
@ -66,7 +70,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];
}

87
apps/user_ldap/tests/Group_LDAPTest.php
Unescape View File

@ -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],

Write Preview
Loading…
Cancel
Save