sanitize and test user id received from IdP, if original does not match
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
This commit is contained in:
parent
7eff06a6d8
commit
9ed277dc1f
|
@ -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\UserResolver;
|
||||
use OCP\AppFramework\Controller;
|
||||
use OCP\AppFramework\Http;
|
||||
use OCP\IConfig;
|
||||
|
@ -58,12 +59,12 @@ class SAMLController extends Controller {
|
|||
private $config;
|
||||
/** @var IURLGenerator */
|
||||
private $urlGenerator;
|
||||
/** @var IUserManager */
|
||||
private $userManager;
|
||||
/** @var ILogger */
|
||||
private $logger;
|
||||
/** @var IL10N */
|
||||
private $l;
|
||||
/** @var UserResolver */
|
||||
private $userResolver;
|
||||
/**
|
||||
* @var ICrypto
|
||||
*/
|
||||
|
@ -78,22 +79,23 @@ class SAMLController extends Controller {
|
|||
* @param UserBackend $userBackend
|
||||
* @param IConfig $config
|
||||
* @param IURLGenerator $urlGenerator
|
||||
* @param IUserManager $userManager
|
||||
* @param ILogger $logger
|
||||
* @param IL10N $l
|
||||
*/
|
||||
public function __construct($appName,
|
||||
IRequest $request,
|
||||
ISession $session,
|
||||
IUserSession $userSession,
|
||||
SAMLSettings $SAMLSettings,
|
||||
UserBackend $userBackend,
|
||||
IConfig $config,
|
||||
IURLGenerator $urlGenerator,
|
||||
IUserManager $userManager,
|
||||
ILogger $logger,
|
||||
IL10N $l,
|
||||
ICrypto $crypto) {
|
||||
public function __construct(
|
||||
$appName,
|
||||
IRequest $request,
|
||||
ISession $session,
|
||||
IUserSession $userSession,
|
||||
SAMLSettings $SAMLSettings,
|
||||
UserBackend $userBackend,
|
||||
IConfig $config,
|
||||
IURLGenerator $urlGenerator,
|
||||
ILogger $logger,
|
||||
IL10N $l,
|
||||
UserResolver $userResolver,
|
||||
ICrypto $crypto) {
|
||||
) {
|
||||
parent::__construct($appName, $request);
|
||||
$this->session = $session;
|
||||
$this->userSession = $userSession;
|
||||
|
@ -101,9 +103,9 @@ class SAMLController extends Controller {
|
|||
$this->userBackend = $userBackend;
|
||||
$this->config = $config;
|
||||
$this->urlGenerator = $urlGenerator;
|
||||
$this->userManager = $userManager;
|
||||
$this->logger = $logger;
|
||||
$this->l = $l;
|
||||
$this->userResolver = $userResolver;
|
||||
$this->crypto = $crypto;
|
||||
}
|
||||
|
||||
|
@ -129,6 +131,12 @@ class SAMLController extends Controller {
|
|||
}
|
||||
|
||||
$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
|
||||
|
@ -140,7 +148,6 @@ class SAMLController extends Controller {
|
|||
$this->userBackend->createUserIfNotExists($uid);
|
||||
return;
|
||||
}
|
||||
$userExists = $this->userManager->userExists($uid);
|
||||
$autoProvisioningAllowed = $this->userBackend->autoprovisionAllowed();
|
||||
if($userExists === true) {
|
||||
if($autoProvisioningAllowed) {
|
||||
|
@ -150,13 +157,6 @@ class SAMLController extends Controller {
|
|||
}
|
||||
|
||||
if(!$userExists && !$autoProvisioningAllowed) {
|
||||
// it is possible that the user was not logged in before and
|
||||
// thus is not known to the original backend. A search can
|
||||
// help with it and make the user known
|
||||
$this->userManager->search($uid);
|
||||
if($this->userManager->userExists($uid)) {
|
||||
return;
|
||||
}
|
||||
throw new NoUserFoundException('Auto provisioning not allowed and user ' . $uid . ' does not exist');
|
||||
} elseif(!$userExists && $autoProvisioningAllowed) {
|
||||
$this->userBackend->createUserIfNotExists($uid, $auth);
|
||||
|
@ -222,10 +222,7 @@ class SAMLController extends Controller {
|
|||
$this->session->set('user_saml.samlUserData', $_SERVER);
|
||||
try {
|
||||
$this->autoprovisionIfPossible($this->session->get('user_saml.samlUserData'));
|
||||
$user = $this->userManager->get($this->userBackend->getCurrentUserId());
|
||||
if(!($user instanceof IUser)) {
|
||||
throw new NoUserFoundException('User' . $this->userBackend->getCurrentUserId() . ' not valid or not found');
|
||||
}
|
||||
$user = $this->userResolver->findExistingUser($this->userBackend->getCurrentUserId());
|
||||
$user->updateLastLoginTimestamp();
|
||||
} catch (NoUserFoundException $e) {
|
||||
if ($e->getMessage()) {
|
||||
|
@ -358,14 +355,13 @@ class SAMLController extends Controller {
|
|||
$this->session->set('user_saml.samlSessionIndex', $auth->getSessionIndex());
|
||||
$this->session->set('user_saml.samlSessionExpiration', $auth->getSessionExpiration());
|
||||
try {
|
||||
$user = $this->userManager->get($this->userBackend->getCurrentUserId());
|
||||
if (!($user instanceof IUser)) {
|
||||
throw new \InvalidArgumentException('User "' . $this->userBackend->getCurrentUserId() . '" is not valid');
|
||||
}
|
||||
$user = $this->userResolver->findExistingUser($this->userBackend->getCurrentUserId());
|
||||
$firstLogin = $user->updateLastLoginTimestamp();
|
||||
if($firstLogin) {
|
||||
if ($firstLogin) {
|
||||
$this->userBackend->initializeHomeDir($user->getUID());
|
||||
}
|
||||
} catch (NoUserFoundException $e) {
|
||||
throw new \InvalidArgumentException('User is not valid');
|
||||
} catch (\Exception $e) {
|
||||
$this->logger->logException($e, ['app' => $this->appName]);
|
||||
$response = new Http\RedirectResponse($this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.notProvisioned'));
|
||||
|
|
|
@ -0,0 +1,104 @@
|
|||
<?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\IUser;
|
||||
use OCP\IUserManager;
|
||||
|
||||
class UserResolver {
|
||||
/** @var IUserManager */
|
||||
private $userManager;
|
||||
|
||||
public function __construct(IUserManager $userManager) {
|
||||
$this->userManager = $userManager;
|
||||
}
|
||||
|
||||
/**
|
||||
* @throws NoUserFoundException
|
||||
*/
|
||||
public function findExistingUserId(string $rawUidCandidate, bool $force = false): string {
|
||||
if($force) {
|
||||
$this->ensureUser($rawUidCandidate);
|
||||
}
|
||||
if($this->userManager->userExists($rawUidCandidate)) {
|
||||
return $rawUidCandidate;
|
||||
}
|
||||
$sanitized = $this->sanitizeUserIdCandidate($rawUidCandidate);
|
||||
if($this->userManager->userExists($sanitized)) {
|
||||
return $sanitized;
|
||||
}
|
||||
throw new NoUserFoundException('User' . $rawUidCandidate . ' not valid or not found');
|
||||
}
|
||||
|
||||
/**
|
||||
* @throws NoUserFoundException
|
||||
*/
|
||||
public function findExistingUser(string $rawUidCandidate): IUser {
|
||||
$uid = $this->findExistingUserId($rawUidCandidate);
|
||||
$user = $this->userManager->get($uid);
|
||||
if($user === null) {
|
||||
throw new NoUserFoundException('User' . $rawUidCandidate . ' not valid or not found');
|
||||
}
|
||||
return $user;
|
||||
}
|
||||
|
||||
public function userExists(string $uid, bool $force = false): bool {
|
||||
try {
|
||||
$this->findExistingUserId($uid, $force);
|
||||
return true;
|
||||
} catch(NoUserFoundException $e) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
protected function ensureUser($search) {
|
||||
$this->userManager->search($search);
|
||||
}
|
||||
|
||||
protected function sanitizeUserIdCandidate(string $rawUidCandidate): string {
|
||||
//FIXME: adjusted copy of LDAP's Access::sanitizeUsername(), should go to API
|
||||
$sanitized = trim($rawUidCandidate);
|
||||
|
||||
// Transliteration to ASCII
|
||||
$transliterated = @iconv('UTF-8', 'ASCII//TRANSLIT', $sanitized);
|
||||
if($transliterated !== false) {
|
||||
// depending on system config iconv can work or not
|
||||
$sanitized = $transliterated;
|
||||
}
|
||||
|
||||
// Replacements
|
||||
$sanitized = str_replace(' ', '_', $sanitized);
|
||||
|
||||
// Every remaining disallowed characters will be removed
|
||||
$sanitized = preg_replace('/[^a-zA-Z0-9_.@-]/u', '', $sanitized);
|
||||
|
||||
if($sanitized === '') {
|
||||
throw new \InvalidArgumentException('provided name template for username does not contain any allowed characters');
|
||||
}
|
||||
|
||||
return $sanitized;
|
||||
}
|
||||
}
|
|
@ -22,8 +22,10 @@
|
|||
namespace OCA\User_SAML\Tests\Controller;
|
||||
|
||||
use OCA\User_SAML\Controller\SAMLController;
|
||||
use OCA\User_SAML\Exceptions\NoUserFoundException;
|
||||
use OCA\User_SAML\SAMLSettings;
|
||||
use OCA\User_SAML\UserBackend;
|
||||
use OCA\User_SAML\UserResolver;
|
||||
use OCP\AppFramework\Http\RedirectResponse;
|
||||
use OCP\AppFramework\Http\TemplateResponse;
|
||||
use OCP\IConfig;
|
||||
|
@ -33,13 +35,14 @@ use OCP\IRequest;
|
|||
use OCP\ISession;
|
||||
use OCP\IURLGenerator;
|
||||
use OCP\IUser;
|
||||
use OCP\IUserManager;
|
||||
use OCP\IUserSession;
|
||||
use OCP\Security\ICrypto;
|
||||
use PHPUnit\Framework\MockObject\MockObject;
|
||||
use Test\TestCase;
|
||||
|
||||
class SAMLControllerTest extends TestCase {
|
||||
/** @var UserResolver|\PHPUnit\Framework\MockObject\MockObject */
|
||||
protected $userResolver;
|
||||
/** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */
|
||||
private $request;
|
||||
/** @var ISession|\PHPUnit_Framework_MockObject_MockObject */
|
||||
|
@ -54,8 +57,6 @@ class SAMLControllerTest extends TestCase {
|
|||
private $config;
|
||||
/** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */
|
||||
private $urlGenerator;
|
||||
/** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */
|
||||
private $userManager;
|
||||
/** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */
|
||||
private $logger;
|
||||
/** @var IL10N|\PHPUnit_Framework_MockObject_MockObject */
|
||||
|
@ -78,9 +79,9 @@ class SAMLControllerTest extends TestCase {
|
|||
->willReturnArgument(0);
|
||||
$this->config = $this->createMock(IConfig::class);
|
||||
$this->urlGenerator = $this->createMock(IURLGenerator::class);
|
||||
$this->userManager = $this->createMock(IUserManager::class);
|
||||
$this->logger = $this->createMock(ILogger::class);
|
||||
$this->l = $this->createMock(IL10N::class);
|
||||
$this->userResolver = $this->createMock(UserResolver::class);
|
||||
$this->crypto = $this->createMock(ICrypto::class);
|
||||
|
||||
$this->l->expects($this->any())->method('t')->willReturnCallback(
|
||||
|
@ -103,9 +104,9 @@ class SAMLControllerTest extends TestCase {
|
|||
$this->userBackend,
|
||||
$this->config,
|
||||
$this->urlGenerator,
|
||||
$this->userManager,
|
||||
$this->logger,
|
||||
$this->l,
|
||||
$this->userResolver,
|
||||
$this->crypto
|
||||
);
|
||||
|
||||
|
@ -175,8 +176,8 @@ class SAMLControllerTest extends TestCase {
|
|||
->method('getAppValue')
|
||||
->with('user_saml', 'general-uid_mapping')
|
||||
->willReturn('uid');
|
||||
$this->userManager
|
||||
->expects($this->once())
|
||||
$this->userResolver
|
||||
->expects($this->any())
|
||||
->method('userExists')
|
||||
->with('MyUid')
|
||||
->willReturn(true);
|
||||
|
@ -191,9 +192,9 @@ class SAMLControllerTest extends TestCase {
|
|||
->willReturn('MyUid');
|
||||
/** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */
|
||||
$user = $this->createMock(IUser::class);
|
||||
$this->userManager
|
||||
$this->userResolver
|
||||
->expects($this->once())
|
||||
->method('get')
|
||||
->method('findExistingUser')
|
||||
->with('MyUid')
|
||||
->willReturn($user);
|
||||
$user
|
||||
|
@ -224,8 +225,8 @@ class SAMLControllerTest extends TestCase {
|
|||
->method('getAppValue')
|
||||
->with('user_saml', 'general-uid_mapping')
|
||||
->willReturn('uid');
|
||||
$this->userManager
|
||||
->expects($this->once())
|
||||
$this->userResolver
|
||||
->expects($this->any())
|
||||
->method('userExists')
|
||||
->with('MyUid')
|
||||
->willReturn(true);
|
||||
|
@ -235,9 +236,9 @@ class SAMLControllerTest extends TestCase {
|
|||
->willReturn('MyUid');
|
||||
/** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */
|
||||
$user = $this->createMock(IUser::class);
|
||||
$this->userManager
|
||||
$this->userResolver
|
||||
->expects($this->once())
|
||||
->method('get')
|
||||
->method('findExistingUser')
|
||||
->with('MyUid')
|
||||
->willReturn($user);
|
||||
$user
|
||||
|
@ -273,11 +274,16 @@ class SAMLControllerTest extends TestCase {
|
|||
->method('getAppValue')
|
||||
->with('user_saml', 'general-uid_mapping')
|
||||
->willReturn('uid');
|
||||
$this->userManager
|
||||
->expects($this->once())
|
||||
$this->userResolver
|
||||
->expects($this->any())
|
||||
->method('userExists')
|
||||
->with('MyUid')
|
||||
->willReturn(false);
|
||||
$this->userResolver
|
||||
->expects($this->any())
|
||||
->method('findExistingUserId')
|
||||
->with('MyUid', true)
|
||||
->willThrowException(new NoUserFoundException());
|
||||
$this->urlGenerator
|
||||
->expects($this->once())
|
||||
->method('getAbsoluteURL')
|
||||
|
@ -297,9 +303,9 @@ class SAMLControllerTest extends TestCase {
|
|||
->willReturn('MyUid');
|
||||
/** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */
|
||||
$user = $this->createMock(IUser::class);
|
||||
$this->userManager
|
||||
$this->userResolver
|
||||
->expects($this->once())
|
||||
->method('get')
|
||||
->method('findExistingUser')
|
||||
->with('MyUid')
|
||||
->willReturn($user);
|
||||
$user
|
||||
|
@ -330,11 +336,16 @@ class SAMLControllerTest extends TestCase {
|
|||
->method('getAppValue')
|
||||
->with('user_saml', 'general-uid_mapping')
|
||||
->willReturn('uid');
|
||||
$this->userManager
|
||||
->expects($this->once())
|
||||
$this->userResolver
|
||||
->expects($this->any())
|
||||
->method('userExists')
|
||||
->with('MyUid')
|
||||
->willReturn(false);
|
||||
$this->userResolver
|
||||
->expects($this->any())
|
||||
->method('findExistingUserId')
|
||||
->with('MyUid', true)
|
||||
->willThrowException(new NoUserFoundException());
|
||||
$this->urlGenerator
|
||||
->expects($this->once())
|
||||
->method('linkToRouteAbsolute')
|
||||
|
@ -349,14 +360,14 @@ class SAMLControllerTest extends TestCase {
|
|||
->method('createUserIfNotExists')
|
||||
->with('MyUid');
|
||||
$this->userBackend
|
||||
->expects($this->exactly(2))
|
||||
->expects($this->atLeastOnce())
|
||||
->method('getCurrentUserId')
|
||||
->willReturn('MyUid');
|
||||
$this->userManager
|
||||
$this->userResolver
|
||||
->expects($this->once())
|
||||
->method('get')
|
||||
->method('findExistingUser')
|
||||
->with('MyUid')
|
||||
->willReturn(null);
|
||||
->willThrowException(new NoUserFoundException());
|
||||
|
||||
$expected = new RedirectResponse('https://nextcloud.com/notprovisioned/');
|
||||
$this->assertEquals($expected, $this->samlController->login(1));
|
||||
|
@ -382,11 +393,16 @@ class SAMLControllerTest extends TestCase {
|
|||
->method('getAppValue')
|
||||
->with('user_saml', 'general-uid_mapping')
|
||||
->willReturn('uid');
|
||||
$this->userManager
|
||||
$this->userResolver
|
||||
->expects($this->any())
|
||||
->method('userExists')
|
||||
->with('MyUid')
|
||||
->willReturn(false);
|
||||
$this->userResolver
|
||||
->expects($this->any())
|
||||
->method('findExistingUserId')
|
||||
->with('MyUid', $this->anything())
|
||||
->willThrowException(new NoUserFoundException());
|
||||
$this->urlGenerator
|
||||
->expects($this->once())
|
||||
->method('linkToRouteAbsolute')
|
||||
|
@ -421,14 +437,14 @@ class SAMLControllerTest extends TestCase {
|
|||
->method('getAppValue')
|
||||
->with('user_saml', 'general-uid_mapping')
|
||||
->willReturn('uid');
|
||||
$this->userManager
|
||||
->expects($this->exactly(2))
|
||||
->method('userExists')
|
||||
->with('MyUid')
|
||||
->willReturnOnConsecutiveCalls(false, true);
|
||||
$this->userManager
|
||||
$this->userResolver
|
||||
->expects($this->once())
|
||||
->method('get')
|
||||
->method('findExistingUserId')
|
||||
->with('MyUid', true)
|
||||
->willReturn('MyUid');
|
||||
$this->userResolver
|
||||
->expects($this->once())
|
||||
->method('findExistingUser')
|
||||
->with('MyUid')
|
||||
->willReturn($this->createMock(IUser::class));
|
||||
$this->urlGenerator
|
||||
|
|
Loading…
Reference in New Issue