Login hooks (#25260)

* fix login hooks

* adjust user session tests

* fix login return value of successful token logins

* trigger preLogin hook earlier; extract method 'loginWithPassword'

* call postLogin hook earlier; add PHPDoc

Author: ChristophWurst

lib/private/User/Session.php

@@ -280,46 +280,11 @@ public function getLoginName() {
 	 */
 	public function login($uid, $password) {
 		$this->session->regenerateId();
-		if ($this->validateToken($password, $uid)) {
-			// When logging in with token, the password must be decrypted first before passing to login hook
-			try {
-				$token = $this->tokenProvider->getToken($password);
-				try {
-					$loginPassword = $this->tokenProvider->getPassword($token, $password);
-					$this->manager->emit('\OC\User', 'preLogin', array($uid, $loginPassword));
-				} catch (PasswordlessTokenException $ex) {
-					$this->manager->emit('\OC\User', 'preLogin', array($uid, ''));
-				}
-			} catch (InvalidTokenException $ex) {
-				// Invalid token, nothing to do
-			}
 
-			$this->loginWithToken($password);
-			$user = $this->getUser();
+		if ($this->validateToken($password, $uid)) {
+			return $this->loginWithToken($password);
 		} else {
-			$this->manager->emit('\OC\User', 'preLogin', array($uid, $password));
-			$user = $this->manager->checkPassword($uid, $password);
-		}
-		if ($user !== false) {
-			if (!is_null($user)) {
-				if ($user->isEnabled()) {
-					$this->setUser($user);
-					$this->setLoginName($uid);
-					$this->manager->emit('\OC\User', 'postLogin', array($user, $password));
-					if ($this->isLoggedIn()) {
-						$this->prepareUserLogin();
-						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->loginWithPassword($uid, $password);
 		}
 		return false;
 	}
@@ -449,6 +414,49 @@ public function tryBasicAuthLogin(IRequest $request) {
 		return false;
 	}
 
+	/**
+	 * Log an user in via login name and password
+	 *
+	 * @param string $uid
+	 * @param string $password
+	 * @return boolean
+	 * @throws LoginException if an app canceld the login process or the user is not enabled
+	 */
+	private function loginWithPassword($uid, $password) {
+		$this->manager->emit('\OC\User', 'preLogin', array($uid, $password));
+		$user = $this->manager->checkPassword($uid, $password);
+		if ($user === false) {
+			// Password check failed
+			return false;
+		}
+
+		if ($user->isEnabled()) {
+			$this->setUser($user);
+			$this->setLoginName($uid);
+			$this->manager->emit('\OC\User', 'postLogin', array($user, $password));
+			if ($this->isLoggedIn()) {
+				$this->prepareUserLogin();
+				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 false;
+	}
+
+	/**
+	 * Log an user in with a given token (id)
+	 *
+	 * @param string $token
+	 * @return boolean
+	 * @throws LoginException if an app canceld the login process or the user is not enabled
+	 */
 	private function loginWithToken($token) {
 		try {
 			$dbToken = $this->tokenProvider->getToken($token);
@@ -457,12 +465,14 @@ private function loginWithToken($token) {
 		}
 		$uid = $dbToken->getUID();
 
+		// When logging in with token, the password must be decrypted first before passing to login hook
 		$password = '';
 		try {
 			$password = $this->tokenProvider->getPassword($dbToken, $token);
 		} catch (PasswordlessTokenException $ex) {
 			// Ignore and use empty string instead
 		}
+
 		$this->manager->emit('\OC\User', 'preLogin', array($uid, $password));
 
 		$user = $this->manager->get($uid);
@@ -472,13 +482,24 @@ private function loginWithToken($token) {
 		}
 		if (!$user->isEnabled()) {
 			// disabled users can not log in
-			return false;
+			// 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->manager->emit('\OC\User', 'postLogin', array($user, $password));
+
+		if ($this->isLoggedIn()) {
+			$this->prepareUserLogin();
+		} 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;
 	}
 

tests/lib/User/SessionTest.php

@@ -729,6 +729,9 @@ public function testCreateSessionTokenWithNonExistentUser() {
 		$this->assertFalse($userSession->createSessionToken($request, $uid, $loginName, $password));
 	}
 
+	/**
+	 * @expectedException \OC\User\LoginException
+	 */
 	public function testTryTokenLoginWithDisabledUser() {
 		$manager = $this->getMockBuilder('\OC\User\Manager')
 			->disableOriginalConstructor()
@@ -761,7 +764,7 @@ public function testTryTokenLoginWithDisabledUser() {
 			->method('isEnabled')
 			->will($this->returnValue(false));
 
-		$this->assertFalse($userSession->tryTokenLogin($request));
+		$userSession->tryTokenLogin($request);
 	}
 
 	public function testValidateSessionDisabledUser() {