Simplify code and fix cases for AD

Author: jvillafanez

lib/Command/RemapUser.php

@@ -78,7 +78,6 @@ protected function configure() {
 	protected function execute(InputInterface $input, OutputInterface $output) {
 		$uid = $input->getArgument('ocName');
 		$this->isAllowed($input->getOption('force'));
-		//$this->confirmUserIsMapped($uid);
 
 		$mappedData = $this->getMappedUUIDAndDN($uid);
 		if ($mappedData['mappedDN'] === false || $mappedData['mappedUUID'] === false) {
@@ -93,7 +92,6 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 		$table1->render();
 
 		$entries = $this->backend->findUsername($uid);
-		$entryCount = \count($entries);
 
 		$output->writeln('');
 		$output->writeln('Candidates found in LDAP:');
@@ -104,25 +102,13 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 		}
 		$table2->render();
 
-		if ($entryCount > 1) {
-			$output->writeln('<error>Found too many candidates in LDAP for the target user, remapping isn\'t possible</error>');
-			return 1;
-		} elseif ($entryCount < 1) {
-			$output->writeln('<error>User not found in LDAP. Consider removing the ownCloud\'s account</error>');
-			return 2;
-		}
-
-		if ($mappedData['mappedDN'] === $entries[0]['dn'] && $mappedData['mappedUUID'] === $entries[0]['directory_uuid']) {
-			$output->writeln('The same user is already mapped. Nothing to do');
-			return 0;  // just show a message and return a success code
-		}
-
-		$result = $this->mapping->replaceUUIDAndDN($uid, $entries[0]['dn'], $entries[0]['directory_uuid']);
-		if ($result === false) {
-			$output->writeln("<error>Failed to replace mapping data for user {$uid}</error>");
-			return 3;
+		try {
+			$message = $this->remapUser($uid, $mappedData, $entries);
+			$output->writeln($message);
+		} catch (\UnexpectedValueException $e) {
+			$output->writeln("<error>{$e->getMessage()}</error>");
+			return $e->getCode();
 		}
-		$output->writeln('Mapping data replaced');
 	}
 
 	private function getMappedUUIDAndDN($username) {
@@ -134,21 +120,6 @@ private function getMappedUUIDAndDN($username) {
 		];
 	}
 
-	/**
-	 * checks whether a user is actually mapped
-	 * @param string $ocName the username as used in ownCloud
-	 * @throws \Exception
-	 * @return true
-	 */
-	private function confirmUserIsMapped($ocName) {
-		$dn = $this->mapping->getDNByName($ocName);
-		if ($dn === false) {
-			throw new \Exception('The given user is not a recognized LDAP user.');
-		}
-
-		return true;
-	}
-
 	/**
 	 * checks whether the setup allows reliable checking of LDAP user existence
 	 * @throws \Exception
@@ -162,4 +133,23 @@ private function isAllowed($force) {
 
 		return true;
 	}
+
+	private function remapUser($uid, $mappedData, $entries) {
+		$entryCount = \count($entries);
+		if ($entryCount > 1) {
+			throw new \UnexpectedValueException('Found too many candidates in LDAP for the target user, remapping isn\'t possible', 1);
+		} elseif ($entryCount < 1) {
+			throw new \UnexpectedValueException('User not found in LDAP. Consider removing the ownCloud\'s account', 2);
+		}
+
+		if ($mappedData['mappedDN'] === $entries[0]['dn'] && $mappedData['mappedUUID'] === $entries[0]['directory_uuid']) {
+			return 'The same user is already mapped. Nothing to do';
+		}
+
+		$result = $this->mapping->replaceUUIDAndDN($uid, $entries[0]['dn'], $entries[0]['directory_uuid']);
+		if ($result === false) {
+			throw new \UnexpectedValueException("Failed to replace mapping data for user {$uid}", 3);
+		}
+		return 'Mapping data replaced';
+	}
 }

lib/User/Manager.php

@@ -556,7 +556,7 @@ public function findUsersByUsername($uid) {
 		$ldapConfig = $this->getConnection();
 
 		$uuidAttrs = [$ldapConfig->ldapExpertUUIDUserAttr];
-		if ($ldapConfig->ldapExpertUUIDUserAttr === 'auto') {
+		if ($ldapConfig->ldapExpertUUIDUserAttr === 'auto' || $ldapConfig->ldapExpertUUIDUserAttr === '') {
 			$uuidAttrs = $ldapConfig->uuidAttributes;
 		}
 
@@ -566,19 +566,20 @@ public function findUsersByUsername($uid) {
 		}
 
 		$escapedUid = $this->access->escapeFilterPart($uid);
-		if (\count($usernameAttrs) === 1) {
-			$innerFilter = "{$usernameAttrs[0]}={$escapedUid}";
-		} else {
-			$attrFilters = [];
-			foreach ($usernameAttrs as $attr) {
+		$attrFilters = [];
+		foreach ($usernameAttrs as $attr) {
+			if ($attr === 'objectguid' || $attr === 'guid') {
+				// needs special formatting because we need to send as binary data
+				$attrFilters[] = "{$attr}=" . $this->access->formatGuid2ForFilterUser($uid);
+			} else {
 				$attrFilters[] = "{$attr}={$escapedUid}";
 			}
-			$innerFilter = $this->access->combineFilterWithOr($attrFilters);
 		}
+		$innerFilter = $this->access->combineFilterWithOr($attrFilters);
 
 		$filter = $this->access->combineFilterWithAnd([
 			$this->getConnection()->ldapUserFilter,
-			$this->getConnection()->ldapUserDisplayName . '=*', // TODO why do we need this? =* basically selects all
+			$this->getConnection()->ldapUserDisplayName . '=*',
 			$innerFilter,
 		]);
 
@@ -589,18 +590,8 @@ public function findUsersByUsername($uid) {
 
 		$entries = [];
 		foreach ($ldap_users as $ldapEntry) {
-			$chosenUsername = null;
-			foreach ($usernameAttrs as $usernameAttr) {
-				if (isset($ldapEntry[$usernameAttr][0])) {
-					$chosenUsername = $ldapEntry[$usernameAttr][0];
-				}
-			}
-			$chosenUuid = null;
-			foreach ($uuidAttrs as $uuidAttr) {
-				if (isset($ldapEntry[$uuidAttr][0])) {
-					$chosenUuid = $ldapEntry[$uuidAttr][0];
-				}
-			}
+			$chosenUsername = $this->getValueFromEntry($ldapEntry, $usernameAttrs);
+			$chosenUuid = $this->getValueFromEntry($ldapEntry, $uuidAttrs);
 
 			$entryData = [
 				'dn' => $ldapEntry['dn'][0],
@@ -613,6 +604,23 @@ public function findUsersByUsername($uid) {
 		return $entries;
 	}
 
+	/**
+	 * Get the value of the first attribute of the attrs list found inside the ldapEntry
+	 */
+	private function getValueFromEntry($ldapEntry, $attrs) {
+		$chosenValue = null;
+		foreach ($attrs as $attr) {
+			if (isset($ldapEntry[$attr][0])) {
+				$chosenValue = $ldapEntry[$attr][0];
+				if ($attr === 'objectguid' || $attr === 'guid') {
+					$chosenValue = Access::binGUID2str($chosenValue);
+				}
+				break;
+			}
+		}
+		return $chosenValue;
+	}
+
 	// TODO find better places for the delegations to Access
 
 	/**