From a7aabdd71f7fd92bacb2ccd603ba1d9b4b7263a0 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 22 Apr 2020 15:52:10 +0200 Subject: [PATCH] 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 --- appinfo/app.php | 9 +- lib/Controller/SAMLController.php | 101 ++++++++++----------- lib/UserBackend.php | 107 +++++----------------- lib/UserData.php | 145 ++++++++++++++++++++++++++++++ lib/UserResolver.php | 9 +- 5 files changed, 229 insertions(+), 142 deletions(-) create mode 100644 lib/UserData.php diff --git a/appinfo/app.php b/appinfo/app.php index 43db79e..3a6d7de 100644 --- a/appinfo/app.php +++ b/appinfo/app.php @@ -45,6 +45,12 @@ $samlSettings = new \OCA\User_SAML\SAMLSettings( $session ); +$userData = new \OCA\User_SAML\UserData( + new \OCA\User_SAML\UserResolver(\OC::$server->getUserManager()), + $samlSettings, + $config +); + $userBackend = new \OCA\User_SAML\UserBackend( $config, $urlGenerator, @@ -53,7 +59,8 @@ $userBackend = new \OCA\User_SAML\UserBackend( \OC::$server->getUserManager(), \OC::$server->getGroupManager(), $samlSettings, - \OC::$server->getLogger() + \OC::$server->getLogger(), + $userData ); $userBackend->registerBackends(\OC::$server->getUserManager()->getBackends()); OC_User::useBackend($userBackend); diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php index fd45a9f..f0c13b0 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(!$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; - } + if(!$this->userData->hasUidMappingAttribute()) { + throw new NoUserFoundException('IDP parameter for the UID not found. Possible parameters are: ' . json_encode(array_keys($auth))); } - throw new NoUserFoundException('IDP parameter for the UID (' . $uidMapping . ') not found. Possible parameters are: ' . json_encode(array_keys($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; + } + + $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 dcec830..f578795 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; + /** @var UserData */ + private $userData; - /** - * @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) { + 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 0000000..c032d3d --- /dev/null +++ b/lib/UserData.php @@ -0,0 +1,145 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +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 f355e1d..8a0c43b 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);