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

6 files changed, 32 insertions, 48 deletions

diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php
index 38f8b98b4f9..3a0ce72a853 100644
--- a/apps/user_ldap/group_ldap.php
+++ b/apps/user_ldap/group_ldap.php
@@ -69,8 +69,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);
@@ -81,8 +82,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);
@@ -161,8 +162,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,
@@ -221,11 +223,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);
@@ -392,8 +392,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) {
@@ -778,8 +779,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 09f808f8d0e..743704869e3 100644
--- a/apps/user_ldap/lib/access.php
+++ b/apps/user_ldap/lib/access.php
@@ -362,8 +362,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;
@@ -1464,8 +1465,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 2ab1a0fc6c2..c5a0331df07 100644
--- a/apps/user_ldap/lib/connection.php
+++ b/apps/user_ldap/lib/connection.php
@@ -211,10 +211,6 @@ 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);
@@ -222,21 +218,6 @@ class Connection extends LDAPUtility {
 
 	/**
 	 * @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
 	 *
 	 * @return string
diff --git a/apps/user_ldap/lib/user/user.php b/apps/user_ldap/lib/user/user.php
index 4707f0d0dd2..c47cbf3a523 100644
--- a/apps/user_ldap/lib/user/user.php
+++ b/apps/user_ldap/lib/user/user.php
@@ -287,8 +287,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 49af5e3fe34..c28fd401727 100644
--- a/apps/user_ldap/tests/group_ldap.php
+++ b/apps/user_ldap/tests/group_ldap.php
@@ -282,10 +282,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 80b22a48528..246eb149297 100644
--- a/apps/user_ldap/user_ldap.php
+++ b/apps/user_ldap/user_ldap.php
@@ -247,8 +247,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);
@@ -318,8 +319,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);