Skip to content

Refactor complex LoginController logic into a login chain of responsibility #15365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MorrisJobke merged 1 commit into from
May 7, 2019
Merged

Refactor complex LoginController logic into a login chain of responsibility #15365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
163 changes: 37 additions & 126 deletions core/Controller/LoginController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@

namespace OC\Core\Controller;

use OC\Authentication\Token\IToken;
use OC\Authentication\Login\Chain;
use OC\Authentication\Login\LoginData;
use OC\Authentication\TwoFactorAuth\Manager;
use OC\Security\Bruteforce\Throttler;
use OC\User\Session;
Expand All @@ -44,17 +45,14 @@
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\Http\RedirectResponse;
use OCP\AppFramework\Http\TemplateResponse;
use OCP\Authentication\TwoFactorAuth\IProvider;
use OCP\Defaults;
use OCP\IConfig;
use OCP\ILogger;
use OCP\IRequest;
use OCP\ISession;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager;
use OCP\IUserSession;
use OC\Hooks\PublicEmitter;
use OCP\Util;

class LoginController extends Controller {
Expand All @@ -74,47 +72,34 @@ class LoginController extends Controller {
private $urlGenerator;
/** @var ILogger */
private $logger;
/** @var Manager */
private $twoFactorManager;
/** @var Defaults */
private $defaults;
/** @var Throttler */
private $throttler;
/** @var Chain */
private $loginChain;

/**
* @param string $appName
* @param IRequest $request
* @param IUserManager $userManager
* @param IConfig $config
* @param ISession $session
* @param IUserSession $userSession
* @param IURLGenerator $urlGenerator
* @param ILogger $logger
* @param Manager $twoFactorManager
* @param Defaults $defaults
* @param Throttler $throttler
*/
public function __construct($appName,
public function __construct(?string $appName,
IRequest $request,
IUserManager $userManager,
IConfig $config,
ISession $session,
IUserSession $userSession,
IURLGenerator $urlGenerator,
ILogger $logger,
Manager $twoFactorManager,
Defaults $defaults,
Throttler $throttler) {
Throttler $throttler,
Chain $loginChain) {
parent::__construct($appName, $request);
$this->userManager = $userManager;
$this->config = $config;
$this->session = $session;
$this->userSession = $userSession;
$this->urlGenerator = $urlGenerator;
$this->logger = $logger;
$this->twoFactorManager = $twoFactorManager;
$this->defaults = $defaults;
$this->throttler = $throttler;
$this->loginChain = $loginChain;
}

/**
Expand Down Expand Up @@ -226,8 +211,8 @@ public function showLoginForm(string $user = null, string $redirect_url = null):
* @param array $parameters
* @return array
*/
private function setPasswordResetParameters(
string $user = null, array $parameters): array {
private function setPasswordResetParameters(?string $user,
array $parameters): array {
if ($user !== null && $user !== '') {
$userObj = $this->userManager->get($user);
} else {
Expand All @@ -250,12 +235,8 @@ private function setPasswordResetParameters(
return $parameters;
}

/**
* @param string $redirectUrl
* @return RedirectResponse
*/
private function generateRedirect($redirectUrl) {
if (!is_null($redirectUrl) && $this->userSession->isLoggedIn()) {
private function generateRedirect(?string $redirectUrl): RedirectResponse {
if ($redirectUrl !== null && $this->userSession->isLoggedIn()) {
$location = $this->urlGenerator->getAbsoluteURL(urldecode($redirectUrl));
// Deny the redirect if the URL contains a @
// This prevents unvalidated redirects like ?redirect_url=:[email protected]
Expand All @@ -275,113 +256,43 @@ private function generateRedirect($redirectUrl) {
* @param string $user
* @param string $password
* @param string $redirect_url
* @param boolean $remember_login
* @param string $timezone
* @param string $timezone_offset
* @return RedirectResponse
*/
public function tryLogin($user, $password, $redirect_url, $remember_login = true, $timezone = '', $timezone_offset = '') {
if(!is_string($user)) {
throw new \InvalidArgumentException('Username must be string');
}

public function tryLogin(string $user,
string $password,
string $redirect_url = null,
string $timezone = '',
string $timezone_offset = ''): RedirectResponse {
// If the user is already logged in and the CSRF check does not pass then
// simply redirect the user to the correct page as required. This is the
// case when an user has already logged-in, in another tab.
if(!$this->request->passesCSRFCheck()) {
return $this->generateRedirect($redirect_url);
}

if ($this->userManager instanceof PublicEmitter) {
$this->userManager->emit('\OC\User', 'preLogin', array($user, $password));
}

$originalUser = $user;

$userObj = $this->userManager->get($user);

if ($userObj !== null && $userObj->isEnabled() === false) {
$this->logger->warning('Login failed: \''. $user . '\' disabled' .
' (Remote IP: \''. $this->request->getRemoteAddress(). '\')',
['app' => 'core']);
return $this->createLoginFailedResponse($user, $originalUser,
$redirect_url, self::LOGIN_MSG_USERDISABLED);
}

// TODO: Add all the insane error handling
/* @var $loginResult IUser */
$loginResult = $this->userManager->checkPasswordNoLogging($user, $password);
if ($loginResult === false) {
$users = $this->userManager->getByEmail($user);
// we only allow login by email if unique
if (count($users) === 1) {
$previousUser = $user;
$user = $users[0]->getUID();
if($user !== $previousUser) {
$loginResult = $this->userManager->checkPassword($user, $password);
}
}
}

if ($loginResult === false) {
$this->logger->warning('Login failed: \''. $user .
'\' (Remote IP: \''. $this->request->getRemoteAddress(). '\')',
['app' => 'core']);
return $this->createLoginFailedResponse($user, $originalUser,
$redirect_url, self::LOGIN_MSG_INVALIDPASSWORD);
}

// TODO: remove password checks from above and let the user session handle failures
// requires https://github.com/owncloud/core/pull/24616
$this->userSession->completeLogin($loginResult, ['loginName' => $user, 'password' => $password]);

$tokenType = IToken::REMEMBER;
if ((int)$this->config->getSystemValue('remember_login_cookie_lifetime', 60*60*24*15) === 0) {
$remember_login = false;
$tokenType = IToken::DO_NOT_REMEMBER;
}

$this->userSession->createSessionToken($this->request, $loginResult->getUID(), $user, $password, $tokenType);
$this->userSession->updateTokens($loginResult->getUID(), $password);

// User has successfully logged in, now remove the password reset link, when it is available
$this->config->deleteUserValue($loginResult->getUID(), 'core', 'lostpassword');

$this->session->set('last-password-confirm', $loginResult->getLastLogin());

if ($timezone_offset !== '') {
$this->config->setUserValue($loginResult->getUID(), 'core', 'timezone', $timezone);
$this->session->set('timezone', $timezone_offset);
}

if ($this->twoFactorManager->isTwoFactorAuthenticated($loginResult)) {
$this->twoFactorManager->prepareTwoFactorLogin($loginResult, $remember_login);

$providers = $this->twoFactorManager->getProviderSet($loginResult)->getPrimaryProviders();
if (count($providers) === 1) {
// Single provider, hence we can redirect to that provider's challenge page directly
/* @var $provider IProvider */
$provider = array_pop($providers);
$url = 'core.TwoFactorChallenge.showChallenge';
$urlParams = [
'challengeProviderId' => $provider->getId(),
];
} else {
$url = 'core.TwoFactorChallenge.selectChallenge';
$urlParams = [];
}

if (!is_null($redirect_url)) {
$urlParams['redirect_url'] = $redirect_url;
}

return new RedirectResponse($this->urlGenerator->linkToRoute($url, $urlParams));
$data = new LoginData(
$this->request,
$user,
$password,
$redirect_url,
$timezone,
$timezone_offset
);
$result = $this->loginChain->process($data);
if (!$result->isSuccess()) {
return $this->createLoginFailedResponse(
$data->getUsername(),
$user,
$redirect_url,
$result->getErrorMessage()
);
}

if ($remember_login) {
$this->userSession->createRememberMeToken($loginResult);
if ($result->getRedirectUrl() !== null) {
return new RedirectResponse($result->getRedirectUrl());
}

return $this->generateRedirect($redirect_url);
}

Expand All @@ -398,8 +309,8 @@ private function createLoginFailedResponse(
$user, $originalUser, $redirect_url, string $loginMessage) {
// Read current user and append if possible we need to
// return the unmodified user otherwise we will leak the login name
$args = !is_null($user) ? ['user' => $originalUser] : [];
if (!is_null($redirect_url)) {
$args = $user !== null ? ['user' => $originalUser] : [];
if ($redirect_url !== null) {
$args['redirect_url'] = $redirect_url;
}
$response = new RedirectResponse(
Expand Down
16 changes: 16 additions & 0 deletions lib/composer/composer/autoload_classmap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,22 @@
'OC\\Authentication\\Exceptions\\UserAlreadyLoggedInException' => $baseDir . '/lib/private/Authentication/Exceptions/UserAlreadyLoggedInException.php',
'OC\\Authentication\\LoginCredentials\\Credentials' => $baseDir . '/lib/private/Authentication/LoginCredentials/Credentials.php',
'OC\\Authentication\\LoginCredentials\\Store' => $baseDir . '/lib/private/Authentication/LoginCredentials/Store.php',
'OC\\Authentication\\Login\\ALoginCommand' => $baseDir . '/lib/private/Authentication/Login/ALoginCommand.php',
'OC\\Authentication\\Login\\Chain' => $baseDir . '/lib/private/Authentication/Login/Chain.php',
'OC\\Authentication\\Login\\ClearLostPasswordTokensCommand' => $baseDir . '/lib/private/Authentication/Login/ClearLostPasswordTokensCommand.php',
'OC\\Authentication\\Login\\CompleteLoginCommand' => $baseDir . '/lib/private/Authentication/Login/CompleteLoginCommand.php',
'OC\\Authentication\\Login\\CreateSessionTokenCommand' => $baseDir . '/lib/private/Authentication/Login/CreateSessionTokenCommand.php',
'OC\\Authentication\\Login\\EmailLoginCommand' => $baseDir . '/lib/private/Authentication/Login/EmailLoginCommand.php',
'OC\\Authentication\\Login\\FinishRememberedLoginCommand' => $baseDir . '/lib/private/Authentication/Login/FinishRememberedLoginCommand.php',
'OC\\Authentication\\Login\\LoggedInCheckCommand' => $baseDir . '/lib/private/Authentication/Login/LoggedInCheckCommand.php',
'OC\\Authentication\\Login\\LoginData' => $baseDir . '/lib/private/Authentication/Login/LoginData.php',
'OC\\Authentication\\Login\\LoginResult' => $baseDir . '/lib/private/Authentication/Login/LoginResult.php',
'OC\\Authentication\\Login\\PreLoginHookCommand' => $baseDir . '/lib/private/Authentication/Login/PreLoginHookCommand.php',
'OC\\Authentication\\Login\\SetUserTimezoneCommand' => $baseDir . '/lib/private/Authentication/Login/SetUserTimezoneCommand.php',
'OC\\Authentication\\Login\\TwoFactorCommand' => $baseDir . '/lib/private/Authentication/Login/TwoFactorCommand.php',
'OC\\Authentication\\Login\\UidLoginCommand' => $baseDir . '/lib/private/Authentication/Login/UidLoginCommand.php',
'OC\\Authentication\\Login\\UpdateLastPasswordConfirmCommand' => $baseDir . '/lib/private/Authentication/Login/UpdateLastPasswordConfirmCommand.php',
'OC\\Authentication\\Login\\UserDisabledCheckCommand' => $baseDir . '/lib/private/Authentication/Login/UserDisabledCheckCommand.php',
'OC\\Authentication\\Token\\DefaultToken' => $baseDir . '/lib/private/Authentication/Token/DefaultToken.php',
'OC\\Authentication\\Token\\DefaultTokenCleanupJob' => $baseDir . '/lib/private/Authentication/Token/DefaultTokenCleanupJob.php',
'OC\\Authentication\\Token\\DefaultTokenMapper' => $baseDir . '/lib/private/Authentication/Token/DefaultTokenMapper.php',
Expand Down
16 changes: 16 additions & 0 deletions lib/composer/composer/autoload_static.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,22 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
'OC\\Authentication\\Exceptions\\UserAlreadyLoggedInException' => __DIR__ . '/../../..' . '/lib/private/Authentication/Exceptions/UserAlreadyLoggedInException.php',
'OC\\Authentication\\LoginCredentials\\Credentials' => __DIR__ . '/../../..' . '/lib/private/Authentication/LoginCredentials/Credentials.php',
'OC\\Authentication\\LoginCredentials\\Store' => __DIR__ . '/../../..' . '/lib/private/Authentication/LoginCredentials/Store.php',
'OC\\Authentication\\Login\\ALoginCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/ALoginCommand.php',
'OC\\Authentication\\Login\\Chain' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/Chain.php',
'OC\\Authentication\\Login\\ClearLostPasswordTokensCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/ClearLostPasswordTokensCommand.php',
'OC\\Authentication\\Login\\CompleteLoginCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/CompleteLoginCommand.php',
'OC\\Authentication\\Login\\CreateSessionTokenCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/CreateSessionTokenCommand.php',
'OC\\Authentication\\Login\\EmailLoginCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/EmailLoginCommand.php',
'OC\\Authentication\\Login\\FinishRememberedLoginCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/FinishRememberedLoginCommand.php',
'OC\\Authentication\\Login\\LoggedInCheckCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/LoggedInCheckCommand.php',
'OC\\Authentication\\Login\\LoginData' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/LoginData.php',
'OC\\Authentication\\Login\\LoginResult' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/LoginResult.php',
'OC\\Authentication\\Login\\PreLoginHookCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/PreLoginHookCommand.php',
'OC\\Authentication\\Login\\SetUserTimezoneCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/SetUserTimezoneCommand.php',
'OC\\Authentication\\Login\\TwoFactorCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/TwoFactorCommand.php',
'OC\\Authentication\\Login\\UidLoginCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/UidLoginCommand.php',
'OC\\Authentication\\Login\\UpdateLastPasswordConfirmCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/UpdateLastPasswordConfirmCommand.php',
'OC\\Authentication\\Login\\UserDisabledCheckCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/UserDisabledCheckCommand.php',
'OC\\Authentication\\Token\\DefaultToken' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/DefaultToken.php',
'OC\\Authentication\\Token\\DefaultTokenCleanupJob' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/DefaultTokenCleanupJob.php',
'OC\\Authentication\\Token\\DefaultTokenMapper' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/DefaultTokenMapper.php',
Expand Down
47 changes: 47 additions & 0 deletions lib/private/Authentication/Login/ALoginCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?php
/**
* @copyright 2019 Christoph Wurst <[email protected]>
*
* @author 2019 Christoph Wurst <[email protected]>
*
* @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/>.
*/

declare(strict_types=1);

namespace OC\Authentication\Login;

abstract class ALoginCommand {

/** @var ALoginCommand */
protected $next;

public function setNext(ALoginCommand $next): ALoginCommand {
$this->next = $next;
return $next;
}

protected function processNextOrFinishSuccessfully(LoginData $loginData): LoginResult {
if ($this->next !== null) {
return $this->next->process($loginData);
} else {
return LoginResult::success($loginData);
}
}

public abstract function process(LoginData $loginData): LoginResult;

}
Loading