do not allow client password logins if token auth is enforced or 2FA is enabled

2016-05-24 14:08:42 +02:00 · 2016-05-24 14:08:42 +02:00 · 28ce7dd262
parent d3fb5d618e
commit 28ce7dd262
5 changed files with 142 additions and 25 deletions
--- a/apps/dav/lib/Connector/Sabre/Auth.php
+++ b/apps/dav/lib/Connector/Sabre/Auth.php
@ -103,8 +103,7 @@ class Auth extends AbstractBasic {
 			return true;
 		} else {
 			\OC_Util::setUpFS(); //login hooks may need early access to the filesystem
-			// TODO: do not allow basic auth if the user is 2FA enforced
-			if($this->userSession->login($username, $password)) {
+			if($this->userSession->logClientIn($username, $password)) {
 				$this->userSession->createSessionToken($this->request, $this->userSession->getUser()->getUID(), $username, $password);
 				\OC_Util::setUpFS($this->userSession->getUser()->getUID());
 				$this->session->set(self::DAV_AUTHENTICATED, $this->userSession->getUser()->getUID());
--- a/apps/dav/tests/unit/connector/sabre/auth.php
+++ b/apps/dav/tests/unit/connector/sabre/auth.php
@ -167,7 +167,7 @@ class Auth extends TestCase {
 			->will($this->returnValue('AnotherUser'));
 		$this->userSession
 			->expects($this->once())
-			->method('login')
+			->method('logClientIn')
 			->with('MyTestUser', 'MyTestPassword')
 			->will($this->returnValue(true));
 		$this->userSession
@ -192,7 +192,7 @@ class Auth extends TestCase {
 			->will($this->returnValue(false));
 		$this->userSession
 			->expects($this->once())
-			->method('login')
+			->method('logClientIn')
 			->with('MyTestUser', 'MyTestPassword')
 			->will($this->returnValue(false));
 		$this->session
@ -560,7 +560,7 @@ class Auth extends TestCase {
 			->getMock();
 		$this->userSession
 			->expects($this->once())
-			->method('login')
+			->method('logClientIn')
 			->with('username', 'password')
 			->will($this->returnValue(true));
 		$this->userSession
@ -602,7 +602,7 @@ class Auth extends TestCase {
 			->getMock();
 		$this->userSession
 			->expects($this->once())
-			->method('login')
+			->method('logClientIn')
 			->with('username', 'password')
 			->will($this->returnValue(false));
 		$response = $this->auth->check($server->httpRequest, $server->httpResponse);
--- a/lib/private/Server.php
+++ b/lib/private/Server.php
@ -236,7 +236,7 @@ class Server extends ServerContainer implements IServerContainer {
 				$defaultTokenProvider = null;
 			}
 			
-			$userSession = new \OC\User\Session($manager, $session, $timeFactory, $defaultTokenProvider);
+			$userSession = new \OC\User\Session($manager, $session, $timeFactory, $defaultTokenProvider, $c->getConfig());
 			$userSession->listen('\OC\User', 'preCreateUser', function ($uid, $password) {
 				\OC_Hook::emit('OC_User', 'pre_createUser', array('run' => true, 'uid' => $uid, 'password' => $password));
 			});
--- a/lib/private/User/Session.php
+++ b/lib/private/User/Session.php
@ -42,6 +42,7 @@ use OC_User;
 use OC_Util;
 use OCA\DAV\Connector\Sabre\Auth;
 use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\IConfig;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\IUser;
@ -68,8 +69,8 @@ use OCP\Session\Exceptions\SessionNotAvailableException;
 * @package OC\User
 */
 class Session implements IUserSession, Emitter {
-	
-	/** @var Manager $manager */
+
+	/** @var IUserManager $manager */
 	private $manager;

 	/** @var ISession $session */
@ -81,6 +82,9 @@ class Session implements IUserSession, Emitter {
 	/** @var IProvider */
 	private $tokenProvider;

+	/** @var IConfig */
+	private $config;
+
 	/** @var User $activeUser */
 	protected $activeUser;

@ -89,12 +93,14 @@ class Session implements IUserSession, Emitter {
 	 * @param ISession $session
 	 * @param ITimeFactory $timeFacory
 	 * @param IProvider $tokenProvider
+	 * @param IConfig $config
 	 */
-	public function __construct(IUserManager $manager, ISession $session, ITimeFactory $timeFacory, $tokenProvider) {
+	public function __construct(IUserManager $manager, ISession $session, ITimeFactory $timeFacory, $tokenProvider, IConfig $config) {
 		$this->manager = $manager;
 		$this->session = $session;
 		$this->timeFacory = $timeFacory;
 		$this->tokenProvider = $tokenProvider;
+		$this->config = $config;
 	}

 	/**
@ -279,7 +285,7 @@ class Session implements IUserSession, Emitter {
 	}

 	/**
-	 * try to login with the provided credentials
+	 * try to log in with the provided credentials
 	 *
 	 * @param string $uid
 	 * @param string $password
@ -327,6 +333,60 @@ class Session implements IUserSession, Emitter {
 		return false;
 	}

+	/**
+	 * Tries to log in a client
+	 *
+	 * Checks token auth enforced
+	 * Checks 2FA enabled
+	 *
+	 * @param string $user
+	 * @param string $password
+	 * @throws LoginException
+	 * @return boolean
+	 */
+	public function logClientIn($user, $password) {
+		$isTokenPassword = $this->isTokenPassword($password);
+		if (!$isTokenPassword && $this->isTokenAuthEnforced()) {
+			// TODO: throw LoginException instead (https://github.com/owncloud/core/pull/24616)
+			return false;
+		}
+		if (!$isTokenPassword && $this->isTwoFactorEnforced($user)) {
+			// TODO: throw LoginException instead (https://github.com/owncloud/core/pull/24616)
+			return false;
+		}
+		return $this->login($user, $password);
+	}
+
+	private function isTokenAuthEnforced() {
+		return $this->config->getSystemValue('token_auth_enforced', false);
+	}
+
+	protected function isTwoFactorEnforced($username) {
+		\OCP\Util::emitHook(
+			'\OCA\Files_Sharing\API\Server2Server',
+			'preLoginNameUsedAsUserName',
+			array('uid' => &$username)
+		);
+		$user = $this->manager->get($username);
+		// DI not possible due to cyclic dependencies :'-/
+		return OC::$server->getTwoFactorAuthManager()->isTwoFactorAuthenticated($user);
+	}
+
+	/**
+	 * Check if the given 'password' is actually a device token
+	 *
+	 * @param type $password
+	 * @return boolean
+	 */
+	public function isTokenPassword($password) {
+		try {
+			$this->tokenProvider->getToken($password);
+			return true;
+		} catch (InvalidTokenException $ex) {
+			return false;
+		}
+	}
+
 	protected function prepareUserLogin() {
 		// TODO: mock/inject/use non-static
 		// Refresh the token
@ -347,7 +407,7 @@ class Session implements IUserSession, Emitter {
 	 */
 	public function tryBasicAuthLogin(IRequest $request) {
 		if (!empty($request->server['PHP_AUTH_USER']) && !empty($request->server['PHP_AUTH_PW'])) {
-			$result = $this->login($request->server['PHP_AUTH_USER'], $request->server['PHP_AUTH_PW']);
+			$result = $this->logClientIn($request->server['PHP_AUTH_USER'], $request->server['PHP_AUTH_PW']);
 			if ($result === true) {
 				/**
 				 * Add DAV authenticated. This should in an ideal world not be
--- a/tests/lib/User/SessionTest.php
+++ b/tests/lib/User/SessionTest.php
@ -24,6 +24,9 @@ class SessionTest extends \Test\TestCase {
 	/** @var \OC\Authentication\Token\DefaultTokenProvider */
 	protected $defaultProvider;

+	/** @var \OCP\IConfig */
+	private $config;
+
 	protected function setUp() {
 		parent::setUp();

@ -34,6 +37,7 @@ class SessionTest extends \Test\TestCase {
 		$this->defaultProvider = $this->getMockBuilder('\OC\Authentication\Token\DefaultTokenProvider')
 			->disableOriginalConstructor()
 			->getMock();
+		$this->config = $this->getMock('\OCP\IConfig');
 	}

 	public function testGetUser() {
@ -95,7 +99,7 @@ class SessionTest extends \Test\TestCase {
 			->with($expectedUser->getUID())
 			->will($this->returnValue($expectedUser));

-		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider);
+		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider, $this->config);
 		$user = $userSession->getUser();
 		$this->assertSame($expectedUser, $user);
 	}
@ -118,7 +122,7 @@ class SessionTest extends \Test\TestCase {
 			->getMock();

 		$userSession = $this->getMockBuilder('\OC\User\Session')
-			->setConstructorArgs([$manager, $session, $this->timeFactory, $this->defaultProvider])
+			->setConstructorArgs([$manager, $session, $this->timeFactory, $this->defaultProvider, $this->config])
 			->setMethods([
 				'getUser'
 			])
@ -145,7 +149,7 @@ class SessionTest extends \Test\TestCase {
 			->method('getUID')
 			->will($this->returnValue('foo'));

-		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider);
+		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider, $this->config);
 		$userSession->setUser($user);
 	}

@ -197,7 +201,7 @@ class SessionTest extends \Test\TestCase {
 			->will($this->returnValue($user));

 		$userSession = $this->getMockBuilder('\OC\User\Session')
-			->setConstructorArgs([$manager, $session, $this->timeFactory, $this->defaultProvider])
+			->setConstructorArgs([$manager, $session, $this->timeFactory, $this->defaultProvider, $this->config])
 			->setMethods([
 				'prepareUserLogin'
 			])
@ -244,7 +248,7 @@ class SessionTest extends \Test\TestCase {
 			->with('foo', 'bar')
 			->will($this->returnValue($user));

-		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider);
+		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider, $this->config);
 		$userSession->login('foo', 'bar');
 	}

@ -280,7 +284,7 @@ class SessionTest extends \Test\TestCase {
 			->with('foo', 'bar')
 			->will($this->returnValue(false));

-		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider);
+		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider, $this->config);
 		$userSession->login('foo', 'bar');
 	}

@ -300,10 +304,64 @@ class SessionTest extends \Test\TestCase {
 			->with('foo', 'bar')
 			->will($this->returnValue(false));

-		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider);
+		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider, $this->config);
 		$userSession->login('foo', 'bar');
 	}

+	public function testLogClientInNoTokenPasswordWith2fa() {
+		$manager = $this->getMockBuilder('\OC\User\Manager')
+			->disableOriginalConstructor()
+			->getMock();
+		$session = $this->getMock('\OCP\ISession');
+
+		/** @var \OC\User\Session $userSession */
+		$userSession = $this->getMockBuilder('\OC\User\Session')
+			->setConstructorArgs([$manager, $session, $this->timeFactory, $this->defaultProvider, $this->config])
+			->setMethods(['login'])
+			->getMock();
+
+		$this->defaultProvider->expects($this->once())
+			->method('getToken')
+			->with('doe')
+			->will($this->throwException(new \OC\Authentication\Exceptions\InvalidTokenException()));
+		$this->config->expects($this->once())
+			->method('getSystemValue')
+			->with('token_auth_enforced', false)
+			->will($this->returnValue(true));
+
+		$this->assertFalse($userSession->logClientIn('john', 'doe'));
+	}
+
+	public function testLogClientInNoTokenPasswordNo2fa() {
+		$manager = $this->getMockBuilder('\OC\User\Manager')
+			->disableOriginalConstructor()
+			->getMock();
+		$session = $this->getMock('\OCP\ISession');
+		$user = $this->getMock('\OCP\IUser');
+
+		/** @var \OC\User\Session $userSession */
+		$userSession = $this->getMockBuilder('\OC\User\Session')
+			->setConstructorArgs([$manager, $session, $this->timeFactory, $this->defaultProvider, $this->config])
+			->setMethods(['login', 'isTwoFactorEnforced'])
+			->getMock();
+
+		$this->defaultProvider->expects($this->once())
+			->method('getToken')
+			->with('doe')
+			->will($this->throwException(new \OC\Authentication\Exceptions\InvalidTokenException()));
+		$this->config->expects($this->once())
+			->method('getSystemValue')
+			->with('token_auth_enforced', false)
+			->will($this->returnValue(false));
+
+		$userSession->expects($this->once())
+			->method('isTwoFactorEnforced')
+			->with('john')
+			->will($this->returnValue(true));
+
+		$this->assertFalse($userSession->logClientIn('john', 'doe'));
+	}
+
 	public function testRememberLoginValidToken() {
 		$session = $this->getMock('\OC\Session\Memory', array(), array(''));
 		$session->expects($this->exactly(1))
@ -355,7 +413,7 @@ class SessionTest extends \Test\TestCase {
 			//override, otherwise tests will fail because of setcookie()
 			array('setMagicInCookie'),
 			//there  are passed as parameters to the constructor
-			array($manager, $session, $this->timeFactory, $this->defaultProvider));
+			array($manager, $session, $this->timeFactory, $this->defaultProvider, $this->config));

 		$granted = $userSession->loginWithCookie('foo', $token);

@ -400,7 +458,7 @@ class SessionTest extends \Test\TestCase {
 		$token = 'goodToken';
 		\OC::$server->getConfig()->setUserValue('foo', 'login_token', $token, time());

-		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider);
+		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider, $this->config);
 		$granted = $userSession->loginWithCookie('foo', 'badToken');

 		$this->assertSame($granted, false);
@ -443,7 +501,7 @@ class SessionTest extends \Test\TestCase {
 		$token = 'goodToken';
 		\OC::$server->getConfig()->setUserValue('foo', 'login_token', $token, time());

-		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider);
+		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider, $this->config);
 		$granted = $userSession->loginWithCookie('foo', $token);

 		$this->assertSame($granted, false);
@ -468,7 +526,7 @@ class SessionTest extends \Test\TestCase {
 		$session = new Memory('');
 		$session->set('user_id', 'foo');
 		$userSession = $this->getMockBuilder('\OC\User\Session')
-			->setConstructorArgs([$manager, $session, $this->timeFactory, $this->defaultProvider])
+			->setConstructorArgs([$manager, $session, $this->timeFactory, $this->defaultProvider, $this->config])
 			->setMethods([
 				'validateSession'
 			])
@ -491,7 +549,7 @@ class SessionTest extends \Test\TestCase {
 		$session = new Memory('');
 		$token = $this->getMock('\OC\Authentication\Token\IToken');
 		$user = $this->getMock('\OCP\IUser');
-		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider);
+		$userSession = new \OC\User\Session($manager, $session, $this->timeFactory, $this->defaultProvider, $this->config);
 		$request = $this->getMock('\OCP\IRequest');

 		$request->expects($this->once())
@ -522,7 +580,7 @@ class SessionTest extends \Test\TestCase {
 		$timeFactory = $this->getMock('\OCP\AppFramework\Utility\ITimeFactory');
 		$tokenProvider = $this->getMock('\OC\Authentication\Token\IProvider');
 		$userSession = $this->getMockBuilder('\OC\User\Session')
-			->setConstructorArgs([$userManager, $session, $timeFactory, $tokenProvider])
+			->setConstructorArgs([$userManager, $session, $timeFactory, $tokenProvider, $this->config])
 			->setMethods(['logout'])
 			->getMock();