introduces a single point of saml attribute interpretations

- solved code duplication on uid mapping attribute determiniation
- a single point for user id normalization
- slightly reduces logic in the Controller

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

4 files changed, 219 insertions, 139 deletions

diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php
index fd45a9ff..f0c13b0f 100644
--- a/lib/Controller/SAMLController.php
+++ b/lib/Controller/SAMLController.php
@@ -28,6 +28,7 @@ use OC\Core\Controller\ClientFlowLoginV2Controller;
 use OCA\User_SAML\Exceptions\NoUserFoundException;
 use OCA\User_SAML\SAMLSettings;
 use OCA\User_SAML\UserBackend;
+use OCA\User_SAML\UserData;
 use OCA\User_SAML\UserResolver;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
@@ -65,6 +66,8 @@ class SAMLController extends Controller {
 	private $l;
 	/** @var UserResolver */
 	private $userResolver;
+	/** @var UserData */
+	private $userData;
 	/**
 	 * @var ICrypto
 	 */
@@ -94,7 +97,8 @@ class SAMLController extends Controller {
 		ILogger $logger,
 		IL10N $l,
 		UserResolver $userResolver,
-		ICrypto $crypto) {
+		UserData $userData,
+		ICrypto $crypto
 	) {
 		parent::__construct($appName, $request);
 		$this->session = $session;
@@ -106,66 +110,53 @@ class SAMLController extends Controller {
 		$this->logger = $logger;
 		$this->l = $l;
 		$this->userResolver = $userResolver;
+		$this->userData = $userData;
 		$this->crypto = $crypto;
 	}
 
 	/**
-	 * @param array $auth
 	 * @throws NoUserFoundException
 	 */
-	private function autoprovisionIfPossible(array $auth) {
+	private function autoprovisionIfPossible() {
+		$auth = $this->userData->getAttributes();
 
-		$prefix = $this->SAMLSettings->getPrefix();
-		$uidMapping = $this->config->getAppValue('user_saml', $prefix . 'general-uid_mapping');
-		if(isset($auth[$uidMapping])) {
-			if(is_array($auth[$uidMapping])) {
-				$uid = $auth[$uidMapping][0];
-			} else {
-				$uid = $auth[$uidMapping];
-			}
-
-			// make sure that a valid UID is given
-			if (empty($uid)) {
-				$this->logger->error('Uid "' . $uid . '" is not a valid uid please check your attribute mapping', ['app' => $this->appName]);
-				throw new \InvalidArgumentException('No valid uid given, please check your attribute mapping. Given uid: ' . $uid);
-			}
-
-			$uid = $this->userBackend->testEncodedObjectGUID($uid);
-			try {
-				$userExists = true;
-				$uid = $this->userResolver->findExistingUserId($uid, true);
-			} catch (NoUserFoundException $e) {
-				$userExists = false;
-			}
-
-			// if this server acts as a global scale master and the user is not
-			// a local admin of the server we just create the user and continue
-			// no need to update additional attributes
-			$isGsEnabled = $this->config->getSystemValue('gs.enabled', false);
-			$isGsMaster = $this->config->getSystemValue('gss.mode', 'slave') === 'master';
-			$isGsMasterAdmin = in_array($uid, $this->config->getSystemValue('gss.master.admin', []));
-			if ($isGsEnabled && $isGsMaster && !$isGsMasterAdmin) {
-				$this->userBackend->createUserIfNotExists($uid);
-				return;
-			}
-			$autoProvisioningAllowed = $this->userBackend->autoprovisionAllowed();
-			if($userExists === true) {
-				if($autoProvisioningAllowed) {
-					$this->userBackend->updateAttributes($uid, $auth);
-				}
-				return;
-			}
+		if(!$this->userData->hasUidMappingAttribute()) {
+			throw new NoUserFoundException('IDP parameter for the UID not found. Possible parameters are: ' . json_encode(array_keys($auth)));
+		}
 
-			if(!$userExists && !$autoProvisioningAllowed) {
-				throw new NoUserFoundException('Auto provisioning not allowed and user ' . $uid . ' does not exist');
-			} elseif(!$userExists && $autoProvisioningAllowed) {
-				$this->userBackend->createUserIfNotExists($uid, $auth);
+		if ($this->userData->getOriginalUid() === '') {
+			$this->logger->error('Uid is not a valid uid please check your attribute mapping', ['app' => $this->appName]);
+			throw new \InvalidArgumentException('No valid uid given, please check your attribute mapping.');
+		}
+		$uid = $this->userData->getEffectiveUid();
+		$userExists = $uid !== '';
+
+		// if this server acts as a global scale master and the user is not
+		// a local admin of the server we just create the user and continue
+		// no need to update additional attributes
+		$isGsEnabled = $this->config->getSystemValue('gs.enabled', false);
+		$isGsMaster = $this->config->getSystemValue('gss.mode', 'slave') === 'master';
+		$isGsMasterAdmin = in_array($uid, $this->config->getSystemValue('gss.master.admin', []));
+		if ($isGsEnabled && $isGsMaster && !$isGsMasterAdmin) {
+			$this->userBackend->createUserIfNotExists($this->userData->getOriginalUid());
+			return;
+		}
+		$autoProvisioningAllowed = $this->userBackend->autoprovisionAllowed();
+		if($userExists) {
+			if($autoProvisioningAllowed) {
 				$this->userBackend->updateAttributes($uid, $auth);
-				return;
 			}
+			return;
 		}
 
-		throw new NoUserFoundException('IDP parameter for the UID (' . $uidMapping . ') not found. Possible parameters are: ' . json_encode(array_keys($auth)));
+		$uid = $this->userData->getOriginalUid();
+		if(!$userExists && !$autoProvisioningAllowed) {
+			throw new NoUserFoundException('Auto provisioning not allowed and user ' . $uid . ' does not exist');
+		} elseif(!$userExists && $autoProvisioningAllowed) {
+			$this->userBackend->createUserIfNotExists($uid, $auth);
+			$this->userBackend->updateAttributes($uid, $auth);
+			return;
+		}
 	}
 
 	/**
@@ -221,7 +212,8 @@ class SAMLController extends Controller {
 				}
 				$this->session->set('user_saml.samlUserData', $_SERVER);
 				try {
-					$this->autoprovisionIfPossible($this->session->get('user_saml.samlUserData'));
+					$this->userData->setAttributes($this->session->get('user_saml.samlUserData'));
+					$this->autoprovisionIfPossible();
 					$user = $this->userResolver->findExistingUser($this->userBackend->getCurrentUserId());
 					$user->updateLastLoginTimestamp();
 				} catch (NoUserFoundException $e) {
@@ -339,7 +331,8 @@ class SAMLController extends Controller {
 		// Check whether the user actually exists, if not redirect to an error page
 		// explaining the issue.
 		try {
-			$this->autoprovisionIfPossible($auth->getAttributes());
+			$this->userData->setAttributes($auth->getAttributes());
+			$this->autoprovisionIfPossible();
 		} catch (NoUserFoundException $e) {
 			$this->logger->error($e->getMessage(), ['app' => $this->appName]);
 			$response = new Http\RedirectResponse($this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.notProvisioned'));
diff --git a/lib/UserBackend.php b/lib/UserBackend.php
index dcec830a..f5787957 100644
--- a/lib/UserBackend.php
+++ b/lib/UserBackend.php
@@ -56,25 +56,20 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend {
 	private $settings;
 	/** @var ILogger */
 	private $logger;
-
-	/**
-	 * @param IConfig $config
-	 * @param IURLGenerator $urlGenerator
-	 * @param ISession $session
-	 * @param IDBConnection $db
-	 * @param IUserManager $userManager
-	 * @param IGroupManager $groupManager
-	 * @param SAMLSettings $settings
-	 * @param ILogger $logger
-	 */
-	public function __construct(IConfig $config,
-								IURLGenerator $urlGenerator,
-								ISession $session,
-								IDBConnection $db,
-								IUserManager $userManager,
-								IGroupManager $groupManager,
-								SAMLSettings $settings,
-								ILogger $logger) {
+	/** @var UserData */
+	private $userData;
+
+	public function __construct(
+		IConfig $config,
+		IURLGenerator $urlGenerator,
+		ISession $session,
+		IDBConnection $db,
+		IUserManager $userManager,
+		IGroupManager $groupManager,
+		SAMLSettings $settings,
+		ILogger $logger,
+		UserData $userData
+) {
 		$this->config = $config;
 		$this->urlGenerator = $urlGenerator;
 		$this->session = $session;
@@ -83,6 +78,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend {
 		$this->groupManager = $groupManager;
 		$this->settings = $settings;
 		$this->logger = $logger;
+		$this->userData = $userData;
 	}
 
 	/**
@@ -441,9 +437,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend {
 			throw new \InvalidArgumentException('No valid uid given, please check your attribute mapping. Got uid: ' . $userData['uid']);
 		}
 
-
 		return $userData;
-
 	}
 
 	/**
@@ -453,6 +447,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend {
 	 * @return array
 	 */
 	private function formatUserData($attributes) {
+		$this->userData->setAttributes($attributes);
 
 		$result = ['formatted' => [], 'raw' => $attributes];
 
@@ -481,12 +476,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend {
 			$result['formatted']['groups'] = null;
 		}
 
-		$prefix = $this->settings->getPrefix();
-		$uidMapping = $this->config->getAppValue('user_saml', $prefix . 'general-uid_mapping');
-		$result['formatted']['uid'] = '';
-		if (isset($attributes[$uidMapping])) {
-			$result['formatted']['uid'] = $attributes[$uidMapping][0];
-		}
+		$result['formatted']['uid'] = $this->userData->getEffectiveUid();
 
 		return $result;
 	}
@@ -497,24 +487,14 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend {
 	 * @since 6.0.0
 	 */
 	public function getCurrentUserId() {
-		$samlData = $this->session->get('user_saml.samlUserData');
-		$prefix = $this->settings->getPrefix();
-		$uidMapping = $this->config->getAppValue('user_saml', $prefix . 'general-uid_mapping', '');
-
-		if($uidMapping !== '' && isset($samlData[$uidMapping])) {
-			if(is_array($samlData[$uidMapping])) {
-				$uid = $samlData[$uidMapping][0];
-			} else {
-				$uid = $samlData[$uidMapping];
-			}
+		$this->userData->setAttributes($this->session->get('user_saml.samlUserData') ?? []);
+		$uid = $this->userData->getEffectiveUid();
+		if($uid !== '' && $this->userExists($uid)) {
 			$uid = $this->testEncodedObjectGUID($uid);
 
-			if($this->userExists($uid)) {
-				$this->session->set('last-password-confirm', strtotime('+4 year', time()));
-				return $uid;
-			}
+			$this->session->set('last-password-confirm', strtotime('+4 year', time()));
+			return $uid;
 		}
-
 		return '';
 	}
 
@@ -696,52 +676,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend {
 		}
 	}
 
-	/**
-	 * returns the plain text UUID if the provided $uid string is a
-	 * base64-encoded binary string representing e.g. the objectGUID. Otherwise
-	 *
-	 */
-	public function testEncodedObjectGUID(string $uid): string {
-		if (preg_match('/[^a-zA-Z0-9=+\/]/', $uid) !== 0) {
-			// certainly not encoded
-			return $uid;
-		}
-
-		$candidate = base64_decode($uid, false);
-		if($candidate === false) {
-			return $uid;
-		}
-		$candidate = $this->convertObjectGUID2Str($candidate);
-		// the regex only matches the structure of the UUID, not its semantic
-		// (i.e. version or variant) simply to be future compatible
-		if(preg_match('/^[a-f0-9]{8}(-[a-f0-9]{4}){4}[a-f0-9]{8}$/i', $candidate) === 1) {
-			$uid = $candidate;
-		}
-		return $uid;
-	}
 
-	/**
-	 * @see \OCA\User_LDAP\Access::convertObjectGUID2Str
-	 */
-	protected function convertObjectGUID2Str($oguid) {
-		$hex_guid = bin2hex($oguid);
-		$hex_guid_to_guid_str = '';
-		for($k = 1; $k <= 4; ++$k) {
-			$hex_guid_to_guid_str .= substr($hex_guid, 8 - 2 * $k, 2);
-		}
-		$hex_guid_to_guid_str .= '-';
-		for($k = 1; $k <= 2; ++$k) {
-			$hex_guid_to_guid_str .= substr($hex_guid, 12 - 2 * $k, 2);
-		}
-		$hex_guid_to_guid_str .= '-';
-		for($k = 1; $k <= 2; ++$k) {
-			$hex_guid_to_guid_str .= substr($hex_guid, 16 - 2 * $k, 2);
-		}
-		$hex_guid_to_guid_str .= '-' . substr($hex_guid, 16, 4);
-		$hex_guid_to_guid_str .= '-' . substr($hex_guid, 20);
-
-		return strtoupper($hex_guid_to_guid_str);
-	}
 
 	public function countUsers() {
 		$query = $this->db->getQueryBuilder();
diff --git a/lib/UserData.php b/lib/UserData.php
new file mode 100644
index 00000000..c032d3d7
--- /dev/null
+++ b/lib/UserData.php
@@ -0,0 +1,145 @@
+<?php
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2020 Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OCA\User_SAML;
+
+use OCA\User_SAML\Exceptions\NoUserFoundException;
+use OCP\IConfig;
+
+class UserData {
+	private $uid;
+	/** @var array */
+	private $attributes;
+	/** @var UserResolver */
+	private $userResolver;
+	/** @var SAMLSettings */
+	private $samlSettings;
+	/** @var IConfig */
+	private $config;
+
+	public function __construct(UserResolver $userResolver, SAMLSettings $samlSettings, IConfig $config) {
+		$this->userResolver = $userResolver;
+		$this->samlSettings = $samlSettings;
+		$this->config = $config;
+	}
+
+	public function setAttributes(array $attributes): void {
+		$this->attributes = $attributes;
+		$this->uid = null; // clear the state in case
+	}
+
+	public function getAttributes(): array {
+		$this->assertIsInitialized();
+		return $this->attributes;
+	}
+
+	public function hasUidMappingAttribute(): bool {
+		$this->assertIsInitialized();
+		$prefix = $this->samlSettings->getPrefix();
+		$uidMapping = $this->config->getAppValue('user_saml', $prefix . 'general-uid_mapping');
+		return isset($this->attributes[$uidMapping]);
+	}
+
+	public function getOriginalUid(): string {
+		$this->assertIsInitialized();
+		return $this->extractSamlUserId();
+	}
+
+	public function getEffectiveUid(): string {
+		if($this->uid !== null) {
+			return $this->uid;
+		}
+		$this->assertIsInitialized();
+		try {
+			$uid = $this->extractSamlUserId();
+			$uid = $this->testEncodedObjectGUID($uid);
+			$uid = $this->userResolver->findExistingUserId($uid, true);
+			$this->uid = $uid;
+		} catch (NoUserFoundException $e) {
+			return '';
+		}
+		return $uid;
+	}
+
+	protected function extractSamlUserId(): string {
+		$prefix = $this->samlSettings->getPrefix();
+		$uidMapping = $this->config->getAppValue('user_saml', $prefix . 'general-uid_mapping');
+		if(isset($this->attributes[$uidMapping])) {
+			if (is_array($this->attributes[$uidMapping])) {
+				return trim($this->attributes[$uidMapping][0]);
+			} else {
+				return trim($this->attributes[$uidMapping]);
+			}
+		}
+		return '';
+	}
+
+	/**
+	 * returns the plain text UUID if the provided $uid string is a
+	 * base64-encoded binary string representing e.g. the objectGUID. Otherwise
+	 *
+	 */
+	protected function testEncodedObjectGUID(string $uid): string {
+		$candidate = base64_decode($uid, true);
+		if($candidate === false) {
+			return $uid;
+		}
+		$candidate = $this->convertObjectGUID2Str($candidate);
+		// the regex only matches the structure of the UUID, not its semantic
+		// (i.e. version or variant) simply to be future compatible
+		if(preg_match('/^[a-f0-9]{8}(-[a-f0-9]{4}){4}[a-f0-9]{8}$/i', $candidate) === 1) {
+			$uid = $candidate;
+		}
+		return $uid;
+	}
+
+	/**
+	 * @see \OCA\User_LDAP\Access::convertObjectGUID2Str
+	 */
+	protected function convertObjectGUID2Str($oguid): string {
+		$hex_guid = bin2hex($oguid);
+		$hex_guid_to_guid_str = '';
+		for($k = 1; $k <= 4; ++$k) {
+			$hex_guid_to_guid_str .= substr($hex_guid, 8 - 2 * $k, 2);
+		}
+		$hex_guid_to_guid_str .= '-';
+		for($k = 1; $k <= 2; ++$k) {
+			$hex_guid_to_guid_str .= substr($hex_guid, 12 - 2 * $k, 2);
+		}
+		$hex_guid_to_guid_str .= '-';
+		for($k = 1; $k <= 2; ++$k) {
+			$hex_guid_to_guid_str .= substr($hex_guid, 16 - 2 * $k, 2);
+		}
+		$hex_guid_to_guid_str .= '-' . substr($hex_guid, 16, 4);
+		$hex_guid_to_guid_str .= '-' . substr($hex_guid, 20);
+
+		return strtoupper($hex_guid_to_guid_str);
+	}
+
+	protected function assertIsInitialized() {
+		if($this->attributes === null) {
+			throw new \LogicException('UserData have to be initialized with setAttributes first');
+		}
+	}
+}
diff --git a/lib/UserResolver.php b/lib/UserResolver.php
index f355e1de..8a0c43b1 100644
--- a/lib/UserResolver.php
+++ b/lib/UserResolver.php
@@ -46,7 +46,11 @@ class UserResolver {
 		if($this->userManager->userExists($rawUidCandidate)) {
 			return $rawUidCandidate;
 		}
-		$sanitized = $this->sanitizeUserIdCandidate($rawUidCandidate);
+		try {
+			$sanitized = $this->sanitizeUserIdCandidate($rawUidCandidate);
+		} catch(\InvalidArgumentException $e) {
+			$sanitized = '';
+		}
 		if($this->userManager->userExists($sanitized)) {
 			return $sanitized;
 		}
@@ -78,6 +82,9 @@ class UserResolver {
 		$this->userManager->search($search);
 	}
 
+	/**
+	 * @throws \InvalidArgumentException
+	 */
 	protected function sanitizeUserIdCandidate(string $rawUidCandidate): string {
 		//FIXME: adjusted copy of LDAP's Access::sanitizeUsername(), should go to API
 		$sanitized = trim($rawUidCandidate);