Merge pull request #24242 from owncloud/stable9-backport-24052

[stable9] Fix LDAP race conditions
owncloud · Apr 26, 2016 · 80a31b7 · 80a31b7
2 parents 6c861f7 + 5b126cd
commit 80a31b7
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 48 deletions.
diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php
@@ -72,8 +72,9 @@ public function inGroup($uid, $gid) {
  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 @@ public function inGroup($uid, $gid) {
  }
 
  $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 @@ private function _groupMembers($dnGroup, &$seen = null) {
  }
  // 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 @@ private function _getGroupDNsFromMemberOf($DN, &$seen = null) {
  */
  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 @@ public function getUserGroups($uid) {
  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 @@ public function groupMatchesFilter($group) {
  * @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
@@ -364,8 +364,9 @@ public function groupsMatchFilter($groupDNs) {
  $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;
@@ -1503,8 +1504,9 @@ public function formatGuid2ForFilterUser($guid) {
  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
@@ -204,30 +204,11 @@ public function getFromCache($key) {
  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
@@ -297,8 +297,9 @@ public function getHomePath($valueFromLDAP = null) {
 
  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
@@ -294,10 +294,6 @@ public function testInGroupHitsUidGidCache() {
  $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
@@ -249,8 +249,9 @@ public function userExistsOnLDAP($user) {
  * @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);
@@ -320,8 +321,9 @@ public function getHome($uid) {
  }
 
  $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);