Skip to content

Commit c868ce2

Browse files
ChristophWurstChristophWurst
committed
Add a login chain to reduce the complexity of LoginController::tryLogin
Signed-off-by: Christoph Wurst <[email protected]>
1 parent 16d696e commit c868ce2

34 files changed

+2361
-454
lines changed

‎core/Controller/LoginController.php‎

Lines changed: 37 additions & 126 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,8 @@
3333

3434
namespace OC\Core\Controller;
3535

36-
use OC\Authentication\Token\IToken;
36+
use OC\Authentication\Login\Chain;
37+
use OC\Authentication\Login\LoginData;
3738
use OC\Authentication\TwoFactorAuth\Manager;
3839
use OC\Security\Bruteforce\Throttler;
3940
use OC\User\Session;
@@ -44,17 +45,14 @@
4445
use OCP\AppFramework\Http\DataResponse;
4546
use OCP\AppFramework\Http\RedirectResponse;
4647
use OCP\AppFramework\Http\TemplateResponse;
47-
use OCP\Authentication\TwoFactorAuth\IProvider;
4848
use OCP\Defaults;
4949
use OCP\IConfig;
5050
use OCP\ILogger;
5151
use OCP\IRequest;
5252
use OCP\ISession;
5353
use OCP\IURLGenerator;
54-
use OCP\IUser;
5554
use OCP\IUserManager;
5655
use OCP\IUserSession;
57-
use OC\Hooks\PublicEmitter;
5856
use OCP\Util;
5957

6058
class LoginController extends Controller {
@@ -74,47 +72,34 @@ class LoginController extends Controller {
7472
private $urlGenerator;
7573
/** @var ILogger */
7674
private $logger;
77-
/** @var Manager */
78-
private $twoFactorManager;
7975
/** @var Defaults */
8076
private $defaults;
8177
/** @var Throttler */
8278
private $throttler;
79+
/** @var Chain */
80+
private $loginChain;
8381

84-
/**
85-
* @param string $appName
86-
* @param IRequest $request
87-
* @param IUserManager $userManager
88-
* @param IConfig $config
89-
* @param ISession $session
90-
* @param IUserSession $userSession
91-
* @param IURLGenerator $urlGenerator
92-
* @param ILogger $logger
93-
* @param Manager $twoFactorManager
94-
* @param Defaults $defaults
95-
* @param Throttler $throttler
96-
*/
97-
public function __construct($appName,
82+
public function __construct(?string $appName,
9883
IRequest $request,
9984
IUserManager $userManager,
10085
IConfig $config,
10186
ISession $session,
10287
IUserSession $userSession,
10388
IURLGenerator $urlGenerator,
10489
ILogger $logger,
105-
Manager $twoFactorManager,
10690
Defaults $defaults,
107-
Throttler $throttler) {
91+
Throttler $throttler,
92+
Chain $loginChain) {
10893
parent::__construct($appName, $request);
10994
$this->userManager = $userManager;
11095
$this->config = $config;
11196
$this->session = $session;
11297
$this->userSession = $userSession;
11398
$this->urlGenerator = $urlGenerator;
11499
$this->logger = $logger;
115-
$this->twoFactorManager = $twoFactorManager;
116100
$this->defaults = $defaults;
117101
$this->throttler = $throttler;
102+
$this->loginChain = $loginChain;
118103
}
119104

120105
/**
@@ -226,8 +211,8 @@ public function showLoginForm(string $user = null, string $redirect_url = null):
226211
* @param array $parameters
227212
* @return array
228213
*/
229-
private function setPasswordResetParameters(
230-
string $user = null, array $parameters): array {
214+
private function setPasswordResetParameters(?string $user,
215+
array $parameters): array {
231216
if ($user !== null && $user !== '') {
232217
$userObj = $this->userManager->get($user);
233218
} else {
@@ -250,12 +235,8 @@ private function setPasswordResetParameters(
250235
return $parameters;
251236
}
252237

253-
/**
254-
* @param string $redirectUrl
255-
* @return RedirectResponse
256-
*/
257-
private function generateRedirect($redirectUrl) {
258-
if (!is_null($redirectUrl) && $this->userSession->isLoggedIn()) {
238+
private function generateRedirect(?string $redirectUrl): RedirectResponse {
239+
if ($redirectUrl !== null && $this->userSession->isLoggedIn()) {
259240
$location = $this->urlGenerator->getAbsoluteURL(urldecode($redirectUrl));
260241
// Deny the redirect if the URL contains a @
261242
// This prevents unvalidated redirects like ?redirect_url=:[email protected]
@@ -275,113 +256,43 @@ private function generateRedirect($redirectUrl) {
275256
* @param string $user
276257
* @param string $password
277258
* @param string $redirect_url
278-
* @param boolean $remember_login
279259
* @param string $timezone
280260
* @param string $timezone_offset
281261
* @return RedirectResponse
282262
*/
283-
public function tryLogin($user, $password, $redirect_url, $remember_login = true, $timezone = '', $timezone_offset = '') {
284-
if(!is_string($user)) {
285-
throw new \InvalidArgumentException('Username must be string');
286-
}
287-
263+
public function tryLogin(string $user,
264+
string $password,
265+
string $redirect_url = null,
266+
string $timezone = '',
267+
string $timezone_offset = ''): RedirectResponse {
288268
// If the user is already logged in and the CSRF check does not pass then
289269
// simply redirect the user to the correct page as required. This is the
290270
// case when an user has already logged-in, in another tab.
291271
if(!$this->request->passesCSRFCheck()) {
292272
return $this->generateRedirect($redirect_url);
293273
}
294274

295-
if ($this->userManager instanceof PublicEmitter) {
296-
$this->userManager->emit('\OC\User', 'preLogin', array($user, $password));
297-
}
298-
299-
$originalUser = $user;
300-
301-
$userObj = $this->userManager->get($user);
302-
303-
if ($userObj !== null && $userObj->isEnabled() === false) {
304-
$this->logger->warning('Login failed: \''. $user . '\' disabled' .
305-
' (Remote IP: \''. $this->request->getRemoteAddress(). '\')',
306-
['app' => 'core']);
307-
return $this->createLoginFailedResponse($user, $originalUser,
308-
$redirect_url, self::LOGIN_MSG_USERDISABLED);
309-
}
310-
311-
// TODO: Add all the insane error handling
312-
/* @var $loginResult IUser */
313-
$loginResult = $this->userManager->checkPasswordNoLogging($user, $password);
314-
if ($loginResult === false) {
315-
$users = $this->userManager->getByEmail($user);
316-
// we only allow login by email if unique
317-
if (count($users) === 1) {
318-
$previousUser = $user;
319-
$user = $users[0]->getUID();
320-
if($user !== $previousUser) {
321-
$loginResult = $this->userManager->checkPassword($user, $password);
322-
}
323-
}
324-
}
325-
326-
if ($loginResult === false) {
327-
$this->logger->warning('Login failed: \''. $user .
328-
'\' (Remote IP: \''. $this->request->getRemoteAddress(). '\')',
329-
['app' => 'core']);
330-
return $this->createLoginFailedResponse($user, $originalUser,
331-
$redirect_url, self::LOGIN_MSG_INVALIDPASSWORD);
332-
}
333-
334-
// TODO: remove password checks from above and let the user session handle failures
335-
// requires https://github.com/owncloud/core/pull/24616
336-
$this->userSession->completeLogin($loginResult, ['loginName' => $user, 'password' => $password]);
337-
338-
$tokenType = IToken::REMEMBER;
339-
if ((int)$this->config->getSystemValue('remember_login_cookie_lifetime', 60*60*24*15) === 0) {
340-
$remember_login = false;
341-
$tokenType = IToken::DO_NOT_REMEMBER;
342-
}
343-
344-
$this->userSession->createSessionToken($this->request, $loginResult->getUID(), $user, $password, $tokenType);
345-
$this->userSession->updateTokens($loginResult->getUID(), $password);
346-
347-
// User has successfully logged in, now remove the password reset link, when it is available
348-
$this->config->deleteUserValue($loginResult->getUID(), 'core', 'lostpassword');
349-
350-
$this->session->set('last-password-confirm', $loginResult->getLastLogin());
351-
352-
if ($timezone_offset !== '') {
353-
$this->config->setUserValue($loginResult->getUID(), 'core', 'timezone', $timezone);
354-
$this->session->set('timezone', $timezone_offset);
355-
}
356-
357-
if ($this->twoFactorManager->isTwoFactorAuthenticated($loginResult)) {
358-
$this->twoFactorManager->prepareTwoFactorLogin($loginResult, $remember_login);
359-
360-
$providers = $this->twoFactorManager->getProviderSet($loginResult)->getPrimaryProviders();
361-
if (count($providers) === 1) {
362-
// Single provider, hence we can redirect to that provider's challenge page directly
363-
/* @var $provider IProvider */
364-
$provider = array_pop($providers);
365-
$url = 'core.TwoFactorChallenge.showChallenge';
366-
$urlParams = [
367-
'challengeProviderId' => $provider->getId(),
368-
];
369-
} else {
370-
$url = 'core.TwoFactorChallenge.selectChallenge';
371-
$urlParams = [];
372-
}
373-
374-
if (!is_null($redirect_url)) {
375-
$urlParams['redirect_url'] = $redirect_url;
376-
}
377-
378-
return new RedirectResponse($this->urlGenerator->linkToRoute($url, $urlParams));
275+
$data = new LoginData(
276+
$this->request,
277+
$user,
278+
$password,
279+
$redirect_url,
280+
$timezone,
281+
$timezone_offset
282+
);
283+
$result = $this->loginChain->process($data);
284+
if (!$result->isSuccess()) {
285+
return $this->createLoginFailedResponse(
286+
$data->getUsername(),
287+
$user,
288+
$redirect_url,
289+
$result->getErrorMessage()
290+
);
379291
}
380292

381-
if ($remember_login) {
382-
$this->userSession->createRememberMeToken($loginResult);
293+
if ($result->getRedirectUrl() !== null) {
294+
return new RedirectResponse($result->getRedirectUrl());
383295
}
384-
385296
return $this->generateRedirect($redirect_url);
386297
}
387298

@@ -398,8 +309,8 @@ private function createLoginFailedResponse(
398309
$user, $originalUser, $redirect_url, string $loginMessage) {
399310
// Read current user and append if possible we need to
400311
// return the unmodified user otherwise we will leak the login name
401-
$args = !is_null($user) ? ['user' => $originalUser] : [];
402-
if (!is_null($redirect_url)) {
312+
$args = $user !== null ? ['user' => $originalUser] : [];
313+
if ($redirect_url !== null) {
403314
$args['redirect_url'] = $redirect_url;
404315
}
405316
$response = new RedirectResponse(

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -506,6 +506,22 @@
506506
'OC\\Authentication\\Exceptions\\UserAlreadyLoggedInException' => $baseDir . '/lib/private/Authentication/Exceptions/UserAlreadyLoggedInException.php',
507507
'OC\\Authentication\\LoginCredentials\\Credentials' => $baseDir . '/lib/private/Authentication/LoginCredentials/Credentials.php',
508508
'OC\\Authentication\\LoginCredentials\\Store' => $baseDir . '/lib/private/Authentication/LoginCredentials/Store.php',
509+
'OC\\Authentication\\Login\\ALoginCommand' => $baseDir . '/lib/private/Authentication/Login/ALoginCommand.php',
510+
'OC\\Authentication\\Login\\Chain' => $baseDir . '/lib/private/Authentication/Login/Chain.php',
511+
'OC\\Authentication\\Login\\ClearLostPasswordTokensCommand' => $baseDir . '/lib/private/Authentication/Login/ClearLostPasswordTokensCommand.php',
512+
'OC\\Authentication\\Login\\CompleteLoginCommand' => $baseDir . '/lib/private/Authentication/Login/CompleteLoginCommand.php',
513+
'OC\\Authentication\\Login\\CreateSessionTokenCommand' => $baseDir . '/lib/private/Authentication/Login/CreateSessionTokenCommand.php',
514+
'OC\\Authentication\\Login\\EmailLoginCommand' => $baseDir . '/lib/private/Authentication/Login/EmailLoginCommand.php',
515+
'OC\\Authentication\\Login\\FinishRememberedLoginCommand' => $baseDir . '/lib/private/Authentication/Login/FinishRememberedLoginCommand.php',
516+
'OC\\Authentication\\Login\\LoggedInCheckCommand' => $baseDir . '/lib/private/Authentication/Login/LoggedInCheckCommand.php',
517+
'OC\\Authentication\\Login\\LoginData' => $baseDir . '/lib/private/Authentication/Login/LoginData.php',
518+
'OC\\Authentication\\Login\\LoginResult' => $baseDir . '/lib/private/Authentication/Login/LoginResult.php',
519+
'OC\\Authentication\\Login\\PreLoginHookCommand' => $baseDir . '/lib/private/Authentication/Login/PreLoginHookCommand.php',
520+
'OC\\Authentication\\Login\\SetUserTimezoneCommand' => $baseDir . '/lib/private/Authentication/Login/SetUserTimezoneCommand.php',
521+
'OC\\Authentication\\Login\\TwoFactorCommand' => $baseDir . '/lib/private/Authentication/Login/TwoFactorCommand.php',
522+
'OC\\Authentication\\Login\\UidLoginCommand' => $baseDir . '/lib/private/Authentication/Login/UidLoginCommand.php',
523+
'OC\\Authentication\\Login\\UpdateLastPasswordConfirmCommand' => $baseDir . '/lib/private/Authentication/Login/UpdateLastPasswordConfirmCommand.php',
524+
'OC\\Authentication\\Login\\UserDisabledCheckCommand' => $baseDir . '/lib/private/Authentication/Login/UserDisabledCheckCommand.php',
509525
'OC\\Authentication\\Token\\DefaultToken' => $baseDir . '/lib/private/Authentication/Token/DefaultToken.php',
510526
'OC\\Authentication\\Token\\DefaultTokenCleanupJob' => $baseDir . '/lib/private/Authentication/Token/DefaultTokenCleanupJob.php',
511527
'OC\\Authentication\\Token\\DefaultTokenMapper' => $baseDir . '/lib/private/Authentication/Token/DefaultTokenMapper.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -536,6 +536,22 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
536536
'OC\\Authentication\\Exceptions\\UserAlreadyLoggedInException' => __DIR__ . '/../../..' . '/lib/private/Authentication/Exceptions/UserAlreadyLoggedInException.php',
537537
'OC\\Authentication\\LoginCredentials\\Credentials' => __DIR__ . '/../../..' . '/lib/private/Authentication/LoginCredentials/Credentials.php',
538538
'OC\\Authentication\\LoginCredentials\\Store' => __DIR__ . '/../../..' . '/lib/private/Authentication/LoginCredentials/Store.php',
539+
'OC\\Authentication\\Login\\ALoginCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/ALoginCommand.php',
540+
'OC\\Authentication\\Login\\Chain' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/Chain.php',
541+
'OC\\Authentication\\Login\\ClearLostPasswordTokensCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/ClearLostPasswordTokensCommand.php',
542+
'OC\\Authentication\\Login\\CompleteLoginCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/CompleteLoginCommand.php',
543+
'OC\\Authentication\\Login\\CreateSessionTokenCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/CreateSessionTokenCommand.php',
544+
'OC\\Authentication\\Login\\EmailLoginCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/EmailLoginCommand.php',
545+
'OC\\Authentication\\Login\\FinishRememberedLoginCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/FinishRememberedLoginCommand.php',
546+
'OC\\Authentication\\Login\\LoggedInCheckCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/LoggedInCheckCommand.php',
547+
'OC\\Authentication\\Login\\LoginData' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/LoginData.php',
548+
'OC\\Authentication\\Login\\LoginResult' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/LoginResult.php',
549+
'OC\\Authentication\\Login\\PreLoginHookCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/PreLoginHookCommand.php',
550+
'OC\\Authentication\\Login\\SetUserTimezoneCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/SetUserTimezoneCommand.php',
551+
'OC\\Authentication\\Login\\TwoFactorCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/TwoFactorCommand.php',
552+
'OC\\Authentication\\Login\\UidLoginCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/UidLoginCommand.php',
553+
'OC\\Authentication\\Login\\UpdateLastPasswordConfirmCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/UpdateLastPasswordConfirmCommand.php',
554+
'OC\\Authentication\\Login\\UserDisabledCheckCommand' => __DIR__ . '/../../..' . '/lib/private/Authentication/Login/UserDisabledCheckCommand.php',
539555
'OC\\Authentication\\Token\\DefaultToken' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/DefaultToken.php',
540556
'OC\\Authentication\\Token\\DefaultTokenCleanupJob' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/DefaultTokenCleanupJob.php',
541557
'OC\\Authentication\\Token\\DefaultTokenMapper' => __DIR__ . '/../../..' . '/lib/private/Authentication/Token/DefaultTokenMapper.php',

‎lib/private/Authentication/Login/ALoginCommand.php‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<?php
2+
/**
3+
* @copyright 2019 Christoph Wurst <[email protected]>
4+
*
5+
* @author 2019 Christoph Wurst <[email protected]>
6+
*
7+
* @license GNU AGPL version 3 or any later version
8+
*
9+
* This program is free software: you can redistribute it and/or modify
10+
* it under the terms of the GNU Affero General Public License as
11+
* published by the Free Software Foundation, either version 3 of the
12+
* License, or (at your option) any later version.
13+
*
14+
* This program is distributed in the hope that it will be useful,
15+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
16+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
17+
* GNU Affero General Public License for more details.
18+
*
19+
* You should have received a copy of the GNU Affero General Public License
20+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
21+
*/
22+
23+
declare(strict_types=1);
24+
25+
namespace OC\Authentication\Login;
26+
27+
abstract class ALoginCommand {
28+
29+
/** @var ALoginCommand */
30+
protected $next;
31+
32+
public function setNext(ALoginCommand $next): ALoginCommand {
33+
$this->next = $next;
34+
return $next;
35+
}
36+
37+
protected function processNextOrFinishSuccessfully(LoginData $loginData): LoginResult {
38+
if ($this->next !== null) {
39+
return $this->next->process($loginData);
40+
} else {
41+
return LoginResult::success($loginData);
42+
}
43+
}
44+
45+
public abstract function process(LoginData $loginData): LoginResult;
46+
47+
}

0 commit comments

Comments
 (0)