Merge pull request #2424 from nextcloud/fix-login-controller-test-consolidate-login

Fix login controller test and consolidate login

core/Controller/LoginController.php

@@ -213,6 +213,9 @@ class LoginController extends Controller {
 	 * @return RedirectResponse
 	 */
 	public function tryLogin($user, $password, $redirect_url, $remember_login = false, $timezone = '', $timezone_offset = '') {
+		if(!is_string($user)) {
+			throw new \InvalidArgumentException('Username must be string');
+		}
 		$currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress(), 'login');
 		$this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login');
 
@@ -255,7 +258,7 @@ class LoginController extends Controller {
 		}
 		// TODO: remove password checks from above and let the user session handle failures
 		// requires https://github.com/owncloud/core/pull/24616
-		$this->userSession->login($user, $password);
+		$this->userSession->completeLogin($loginResult, ['loginName' => $user, 'password' => $password]);
 		$this->userSession->createSessionToken($this->request, $loginResult->getUID(), $user, $password, (int)$remember_login);
 
 		// User has successfully logged in, now remove the password reset link, when it is available

lib/private/Log.php

@@ -63,6 +63,7 @@ class Log implements ILogger {
 
 	protected $methodsWithSensitiveParameters = [
 		// Session/User
+		'completeLogin',
 		'login',
 		'checkPassword',
 		'loginWithPassword',

lib/private/User/Session.php

@@ -41,6 +41,7 @@ use OC\Authentication\Exceptions\PasswordLoginForbiddenException;
 use OC\Authentication\Token\IProvider;
 use OC\Authentication\Token\IToken;
 use OC\Hooks\Emitter;
+use OC\Hooks\PublicEmitter;
 use OC_User;
 use OC_Util;
 use OCA\DAV\Connector\Sabre\Auth;
@@ -78,7 +79,7 @@ use Symfony\Component\EventDispatcher\GenericEvent;
  */
 class Session implements IUserSession, Emitter {
 
-	/** @var IUserManager $manager */
+	/** @var IUserManager|PublicEmitter $manager */
 	private $manager;
 
 	/** @var ISession $session */
@@ -156,7 +157,7 @@ class Session implements IUserSession, Emitter {
 	/**
 	 * get the manager object
 	 *
-	 * @return Manager
+	 * @return Manager|PublicEmitter
 	 */
 	public function getManager() {
 		return $this->manager;
@@ -324,6 +325,46 @@ class Session implements IUserSession, Emitter {
 		return $this->loginWithPassword($uid, $password);
 	}
 
+	/**
+	 * @param IUser $user
+	 * @param array $loginDetails
+	 * @param bool $regenerateSessionId
+	 * @return true returns true if login successful or an exception otherwise
+	 * @throws LoginException
+	 */
+	public function completeLogin(IUser $user, array $loginDetails, $regenerateSessionId = true) {
+		if (!$user->isEnabled()) {
+			// disabled users can not log in
+			// injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory
+			$message = \OC::$server->getL10N('lib')->t('User disabled');
+			throw new LoginException($message);
+		}
+
+		if($regenerateSessionId) {
+			$this->session->regenerateId();
+		}
+
+		$this->setUser($user);
+		$this->setLoginName($loginDetails['loginName']);
+
+		if(isset($loginDetails['token']) && $loginDetails['token'] instanceof IToken) {
+			$this->setToken($loginDetails['token']->getId());
+			$this->lockdownManager->setToken($loginDetails['token']);
+			$firstTimeLogin = false;
+		} else {
+			$this->setToken(null);
+			$firstTimeLogin = $user->updateLastLoginTimestamp();
+		}
+		$this->manager->emit('\OC\User', 'postLogin', [$user, $loginDetails['password']]);
+		if($this->isLoggedIn()) {
+			$this->prepareUserLogin($firstTimeLogin);
+			return true;
+		} else {
+			$message = \OC::$server->getL10N('lib')->t('Login canceled by app');
+			throw new LoginException($message);
+		}
+	}
+
 	/**
 	 * Tries to log in a client
 	 *
@@ -498,25 +539,7 @@ class Session implements IUserSession, Emitter {
 			return false;
 		}
 
-		if ($user->isEnabled()) {
-			$this->setUser($user);
-			$this->setLoginName($uid);
-			$this->setToken(null);
-			$firstTimeLogin = $user->updateLastLoginTimestamp();
-			$this->manager->emit('\OC\User', 'postLogin', [$user, $password]);
-			if ($this->isLoggedIn()) {
-				$this->prepareUserLogin($firstTimeLogin);
-				return true;
-			} else {
-				// injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory
-				$message = \OC::$server->getL10N('lib')->t('Login canceled by app');
-				throw new LoginException($message);
-			}
-		} else {
-			// injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory
-			$message = \OC::$server->getL10N('lib')->t('User disabled');
-			throw new LoginException($message);
-		}
+		return $this->completeLogin($user, ['loginName' => $uid, 'password' => $password], false);
 	}
 
 	/**
@@ -542,34 +565,22 @@ class Session implements IUserSession, Emitter {
 			// Ignore and use empty string instead
 		}
 
+		$this->manager->emit('\OC\User', 'preLogin', array($uid, $password));
+
 		$user = $this->manager->get($uid);
 		if (is_null($user)) {
 			// user does not exist
 			return false;
 		}
-		if (!$user->isEnabled()) {
-			// disabled users can not log in
-			// injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory
-			$message = \OC::$server->getL10N('lib')->t('User disabled');
-			throw new LoginException($message);
-		}
 
-		//login
-		$this->setUser($user);
-		$this->setLoginName($dbToken->getLoginName());
-		$this->setToken($dbToken->getId());
-		$this->lockdownManager->setToken($dbToken);
-		$this->manager->emit('\OC\User', 'postLogin', array($user, $password));
-
-		if ($this->isLoggedIn()) {
-			$this->prepareUserLogin(false); // token login cant be the first
-		} else {
-			// injecting l10n does not work - there is a circular dependency between session and \OCP\L10N\IFactory
-			$message = \OC::$server->getL10N('lib')->t('Login canceled by app');
-			throw new LoginException($message);
-		}
-
-		return true;
+		return $this->completeLogin(
+			$user,
+			[
+				'loginName' => $dbToken->getLoginName(),
+				'password' => $password,
+				'token' => $dbToken
+			],
+			false);
 	}
 
 	/**

tests/Core/Controller/LoginControllerTest.php

@@ -333,6 +333,7 @@ class LoginControllerTest extends TestCase {
 		$user->expects($this->any())
 			->method('getUID')
 			->will($this->returnValue('uid'));
+		$loginName = 'loginli';
 		$user->expects($this->any())
 			->method('getLastLogin')
 			->willReturn(123456);
@@ -360,11 +361,11 @@ class LoginControllerTest extends TestCase {
 			->method('checkPasswordNoLogging')
 			->will($this->returnValue($user));
 		$this->userSession->expects($this->once())
-			->method('login')
-			->with($user, $password);
+			->method('completeLogin')
+			->with($user, ['loginName' => $loginName, 'password' => $password]);
 		$this->userSession->expects($this->once())
 			->method('createSessionToken')
-			->with($this->request, $user->getUID(), $user, $password, false);
+			->with($this->request, $user->getUID(), $loginName, $password, false);
 		$this->twoFactorManager->expects($this->once())
 			->method('isTwoFactorAuthenticated')
 			->with($user)
@@ -386,7 +387,7 @@ class LoginControllerTest extends TestCase {
 			);
 
 		$expected = new \OCP\AppFramework\Http\RedirectResponse($indexPageUrl);
-		$this->assertEquals($expected, $this->loginController->tryLogin($user, $password, null, false, 'Europe/Berlin', '1'));
+		$this->assertEquals($expected, $this->loginController->tryLogin($loginName, $password, null, false, 'Europe/Berlin', '1'));
 	}
 
 	public function testLoginWithValidCredentialsAndRememberMe() {
@@ -395,6 +396,7 @@ class LoginControllerTest extends TestCase {
 		$user->expects($this->any())
 			->method('getUID')
 			->will($this->returnValue('uid'));
+		$loginName  = 'loginli';
 		$password = 'secret';
 		$indexPageUrl = \OC_Util::getDefaultPageUrl();
 
@@ -419,11 +421,11 @@ class LoginControllerTest extends TestCase {
 			->method('checkPasswordNoLogging')
 			->will($this->returnValue($user));
 		$this->userSession->expects($this->once())
-			->method('login')
-			->with($user, $password);
+			->method('completeLogin')
+			->with($user, ['loginName' => $loginName, 'password' => $password]);
 		$this->userSession->expects($this->once())
 			->method('createSessionToken')
-			->with($this->request, $user->getUID(), $user, $password, true);
+			->with($this->request, $user->getUID(), $loginName, $password, true);
 		$this->twoFactorManager->expects($this->once())
 			->method('isTwoFactorAuthenticated')
 			->with($user)
@@ -436,7 +438,7 @@ class LoginControllerTest extends TestCase {
 			->with($user);
 
 		$expected = new \OCP\AppFramework\Http\RedirectResponse($indexPageUrl);
-		$this->assertEquals($expected, $this->loginController->tryLogin($user, $password, null, true));
+		$this->assertEquals($expected, $this->loginController->tryLogin($loginName, $password, null, true));
 	}
 
 	public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn() {
@@ -603,8 +605,8 @@ class LoginControllerTest extends TestCase {
 			->method('checkPasswordNoLogging')
 			->will($this->returnValue($user));
 		$this->userSession->expects($this->once())
-			->method('login')
-			->with('john@doe.com', $password);
+			->method('completeLogin')
+			->with($user, ['loginName' => 'john@doe.com', 'password' => $password]);
 		$this->userSession->expects($this->once())
 			->method('createSessionToken')
 			->with($this->request, $user->getUID(), 'john@doe.com', $password, false);
@@ -670,8 +672,8 @@ class LoginControllerTest extends TestCase {
 			->method('checkPasswordNoLogging')
 			->will($this->returnValue($user));
 		$this->userSession->expects($this->once())
-			->method('login')
-			->with('john@doe.com', $password);
+			->method('completeLogin')
+			->with($user, ['loginName' => 'john@doe.com', 'password' => $password]);
 		$this->userSession->expects($this->once())
 			->method('createSessionToken')
 			->with($this->request, $user->getUID(), 'john@doe.com', $password, false);