From 62a59854f0478130c23bc7b8bbf4064b38bbeaf8 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Mon, 18 Apr 2016 10:32:15 +0200 Subject: [PATCH] Fix LDAP race conditions * getFromCache is wrapped in isCached * inbetween the two calls the cache entry hits it's TTL * getFromCache returns null * this fix only checkes if the returned value is null and return only non-null values --- apps/user_ldap/group_ldap.php | 32 +++++++++++++++-------------- apps/user_ldap/lib/access.php | 10 +++++---- apps/user_ldap/lib/connection.php | 19 ----------------- apps/user_ldap/lib/user/user.php | 5 +++-- apps/user_ldap/tests/group_ldap.php | 4 ---- apps/user_ldap/user_ldap.php | 10 +++++---- 6 files changed, 32 insertions(+), 48 deletions(-) diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php index c50ab0a8e1..eba39ca50f 100644 --- a/apps/user_ldap/group_ldap.php +++ b/apps/user_ldap/group_ldap.php @@ -72,8 +72,9 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { return false; } $cacheKey = 'inGroup'.$uid.':'.$gid; - if($this->access->connection->isCached($cacheKey)) { - return $this->access->connection->getFromCache($cacheKey); + $inGroup = $this->access->connection->getFromCache($cacheKey); + if(!is_null($inGroup)) { + return (bool)$inGroup; } $userDN = $this->access->username2dn($uid); @@ -84,8 +85,8 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { } $cacheKeyMembers = 'inGroup-members:'.$gid; - if($this->access->connection->isCached($cacheKeyMembers)) { - $members = $this->access->connection->getFromCache($cacheKeyMembers); + $members = $this->access->connection->getFromCache($cacheKeyMembers); + if(!is_null($members)) { $this->cachedGroupMembers[$gid] = $members; $isInGroup = in_array($userDN, $members); $this->access->connection->writeToCache($cacheKey, $isInGroup); @@ -204,8 +205,9 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { } // used extensively in cron job, caching makes sense for nested groups $cacheKey = '_groupMembers'.$dnGroup; - if($this->access->connection->isCached($cacheKey)) { - return $this->access->connection->getFromCache($cacheKey); + $groupMembers = $this->access->connection->getFromCache($cacheKey); + if(!is_null($groupMembers)) { + return $groupMembers; } $seen[$dnGroup] = 1; $members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr, @@ -267,11 +269,9 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { */ public function primaryGroupID2Name($gid, $dn) { $cacheKey = 'primaryGroupIDtoName'; - if($this->access->connection->isCached($cacheKey)) { - $groupNames = $this->access->connection->getFromCache($cacheKey); - if(isset($groupNames[$gid])) { - return $groupNames[$gid]; - } + $groupNames = $this->access->connection->getFromCache($cacheKey); + if(!is_null($groupNames) && isset($groupNames[$gid])) { + return $groupNames[$gid]; } $domainObjectSid = $this->access->getSID($dn); @@ -440,8 +440,9 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { return array(); } $cacheKey = 'getUserGroups'.$uid; - if($this->access->connection->isCached($cacheKey)) { - return $this->access->connection->getFromCache($cacheKey); + $userGroups = $this->access->connection->getFromCache($cacheKey); + if(!is_null($userGroups)) { + return $userGroups; } $userDN = $this->access->username2dn($uid); if(!$userDN) { @@ -861,8 +862,9 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { * @return bool */ public function groupExists($gid) { - if($this->access->connection->isCached('groupExists'.$gid)) { - return $this->access->connection->getFromCache('groupExists'.$gid); + $groupExists = $this->access->connection->getFromCache('groupExists'.$gid); + if(!is_null($groupExists)) { + return (bool)$groupExists; } //getting dn, if false the group does not exist. If dn, it may be mapped diff --git a/apps/user_ldap/lib/access.php b/apps/user_ldap/lib/access.php index 135eca1e62..569d2dbcbe 100644 --- a/apps/user_ldap/lib/access.php +++ b/apps/user_ldap/lib/access.php @@ -364,8 +364,9 @@ class Access extends LDAPUtility implements user\IUserTools { $validGroupDNs = []; foreach($groupDNs as $dn) { $cacheKey = 'groupsMatchFilter-'.$dn; - if($this->connection->isCached($cacheKey)) { - if($this->connection->getFromCache($cacheKey)) { + $groupMatchFilter = $this->connection->getFromCache($cacheKey); + if(!is_null($groupMatchFilter)) { + if($groupMatchFilter) { $validGroupDNs[] = $dn; } continue; @@ -1505,8 +1506,9 @@ class Access extends LDAPUtility implements user\IUserTools { public function getSID($dn) { $domainDN = $this->getDomainDNFromDN($dn); $cacheKey = 'getSID-'.$domainDN; - if($this->connection->isCached($cacheKey)) { - return $this->connection->getFromCache($cacheKey); + $sid = $this->connection->getFromCache($cacheKey); + if(!is_null($sid)) { + return $sid; } $objectSid = $this->readAttribute($domainDN, 'objectsid'); diff --git a/apps/user_ldap/lib/connection.php b/apps/user_ldap/lib/connection.php index 53c9b3790a..974cad6dc0 100644 --- a/apps/user_ldap/lib/connection.php +++ b/apps/user_ldap/lib/connection.php @@ -213,30 +213,11 @@ class Connection extends LDAPUtility { if(is_null($this->cache) || !$this->configuration->ldapCacheTTL) { return null; } - if(!$this->isCached($key)) { - return null; - - } $key = $this->getCacheKey($key); return json_decode(base64_decode($this->cache->get($key)), true); } - /** - * @param string $key - * @return bool - */ - public function isCached($key) { - if(!$this->configured) { - $this->readConfiguration(); - } - if(is_null($this->cache) || !$this->configuration->ldapCacheTTL) { - return false; - } - $key = $this->getCacheKey($key); - return $this->cache->hasKey($key); - } - /** * @param string $key * @param mixed $value diff --git a/apps/user_ldap/lib/user/user.php b/apps/user_ldap/lib/user/user.php index 23aba0e0d8..4da8ae5f09 100644 --- a/apps/user_ldap/lib/user/user.php +++ b/apps/user_ldap/lib/user/user.php @@ -297,8 +297,9 @@ class User { public function getMemberOfGroups() { $cacheKey = 'getMemberOf'.$this->getUsername(); - if($this->connection->isCached($cacheKey)) { - return $this->connection->getFromCache($cacheKey); + $memberOfGroups = $this->connection->getFromCache($cacheKey); + if(!is_null($memberOfGroups)) { + return $memberOfGroups; } $groupDNs = $this->access->readAttribute($this->getDN(), 'memberOf'); $this->connection->writeToCache($cacheKey, $groupDNs); diff --git a/apps/user_ldap/tests/group_ldap.php b/apps/user_ldap/tests/group_ldap.php index 667a1c3acb..51bb1d8473 100644 --- a/apps/user_ldap/tests/group_ldap.php +++ b/apps/user_ldap/tests/group_ldap.php @@ -294,10 +294,6 @@ class Test_Group_Ldap extends \Test\TestCase { $uid = 'someUser'; $gid = 'someGroup'; $cacheKey = 'inGroup'.$uid.':'.$gid; - $access->connection->expects($this->once()) - ->method('isCached') - ->with($cacheKey) - ->will($this->returnValue(true)); $access->connection->expects($this->once()) ->method('getFromCache') diff --git a/apps/user_ldap/user_ldap.php b/apps/user_ldap/user_ldap.php index 26d3429818..224cf5c9a3 100644 --- a/apps/user_ldap/user_ldap.php +++ b/apps/user_ldap/user_ldap.php @@ -250,8 +250,9 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn * @throws \Exception when connection could not be established */ public function userExists($uid) { - if($this->access->connection->isCached('userExists'.$uid)) { - return $this->access->connection->getFromCache('userExists'.$uid); + $userExists = $this->access->connection->getFromCache('userExists'.$uid); + if(!is_null($userExists)) { + return (bool)$userExists; } //getting dn, if false the user does not exist. If dn, he may be mapped only, requires more checking. $user = $this->access->userManager->get($uid); @@ -321,8 +322,9 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn } $cacheKey = 'getHome'.$uid; - if($this->access->connection->isCached($cacheKey)) { - return $this->access->connection->getFromCache($cacheKey); + $path = $this->access->connection->getFromCache($cacheKey); + if(!is_null($path)) { + return $path; } $user = $this->access->userManager->get($uid);