LDAP backup server should not be queried when auth fails

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

apps/user_ldap/lib/Connection.php

@@ -511,6 +511,8 @@ class Connection extends LDAPUtility {
 
 	/**
 	 * Connects and Binds to LDAP
+	 *
+	 * @throws ServerNotAvailableException
 	 */
 	private function establishConnection() {
 		if(!$this->configuration->ldapConfigurationActive) {
@@ -557,18 +559,12 @@ class Connection extends LDAPUtility {
 				|| $this->getFromCache('overrideMainServer'));
 			$isBackupHost = (trim($this->configuration->ldapBackupHost) !== "");
 			$bindStatus = false;
-			$error = -1;
 			try {
 				if (!$isOverrideMainServer) {
 					$this->doConnect($this->configuration->ldapHost,
 						$this->configuration->ldapPort);
-					$bindStatus = $this->bind();
-					$error = $this->ldap->isResource($this->ldapConnectionRes) ?
-						$this->ldap->errno($this->ldapConnectionRes) : -1;
-				}
-				if($bindStatus === true) {
-					return $bindStatus;
 				}
+				return $this->bind();
 			} catch (ServerNotAvailableException $e) {
 				if(!$isBackupHost) {
 					throw $e;
@@ -576,7 +572,7 @@ class Connection extends LDAPUtility {
 			}
 
 			//if LDAP server is not reachable, try the Backup (Replica!) Server
-			if($isBackupHost && ($error !== 0 || $isOverrideMainServer)) {
+			if($isBackupHost || $isOverrideMainServer) {
 				$this->doConnect($this->configuration->ldapBackupHost,
 								 $this->configuration->ldapBackupPort);
 				$this->bindResult = [];

apps/user_ldap/tests/ConnectionTest.php

@@ -38,7 +38,7 @@ use OCA\User_LDAP\ILDAPWrapper;
  * @package OCA\User_LDAP\Tests
  */
 class ConnectionTest extends \Test\TestCase {
-	/** @var \OCA\User_LDAP\ILDAPWrapper  */
+	/** @var \OCA\User_LDAP\ILDAPWrapper|\PHPUnit_Framework_MockObject_MockObject  */
 	protected $ldap;
 
 	/** @var  Connection */
@@ -110,7 +110,7 @@ class ConnectionTest extends \Test\TestCase {
 			->method('setOption')
 			->will($this->returnValue(true));
 
-		$this->ldap->expects($this->exactly(3))
+		$this->ldap->expects($this->exactly(2))
 			->method('connect')
 			->will($this->returnValue('ldapResource'));
 
@@ -119,7 +119,7 @@ class ConnectionTest extends \Test\TestCase {
 			->will($this->returnValue(0));
 
 		// Not called often enough? Then, the fallback to the backup server is broken.
-		$this->connection->expects($this->exactly(4))
+		$this->connection->expects($this->exactly(3))
 			->method('getFromCache')
 			->with('overrideMainServer')
 			->will($this->onConsecutiveCalls(false, false, true, true));
@@ -145,6 +145,53 @@ class ConnectionTest extends \Test\TestCase {
 		$this->connection->init();
 	}
 
+	public function testDontUseBackupServerOnFailedAuth() {
+		$mainHost = 'ldap://nixda.ldap';
+		$backupHost = 'ldap://fallback.ldap';
+		$config = [
+			'ldapConfigurationActive' => true,
+			'ldapHost' => $mainHost,
+			'ldapPort' => 389,
+			'ldapBackupHost' => $backupHost,
+			'ldapBackupPort' => 389,
+			'ldapAgentName' => 'uid=agent',
+			'ldapAgentPassword' => 'SuchASecret'
+		];
+
+		$this->connection->setIgnoreValidation(true);
+		$this->connection->setConfiguration($config);
+
+		$this->ldap->expects($this->any())
+			->method('isResource')
+			->will($this->returnValue(true));
+
+		$this->ldap->expects($this->any())
+			->method('setOption')
+			->will($this->returnValue(true));
+
+		$this->ldap->expects($this->once())
+			->method('connect')
+			->will($this->returnValue('ldapResource'));
+
+		$this->ldap->expects($this->any())
+			->method('errno')
+			->will($this->returnValue(49));
+
+		$this->connection->expects($this->any())
+			->method('getFromCache')
+			->with('overrideMainServer')
+			->willReturn(false);
+
+		$this->connection->expects($this->never())
+			->method('writeToCache');
+
+		$this->ldap->expects($this->exactly(1))
+			->method('bind')
+			->willReturn(false);
+
+		$this->connection->init();
+	}
+
 	public function testBindWithInvalidCredentials() {
 		// background: Bind with invalid credentials should return false
 		// and not throw a ServerNotAvailableException.