From 28ce7dd262fbf748c46b915b67ac6c332fed8420 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Tue, 24 May 2016 14:08:42 +0200 Subject: [PATCH] do not allow client password logins if token auth is enforced or 2FA is enabled --- apps/dav/lib/Connector/Sabre/Auth.php | 3 +- apps/dav/tests/unit/connector/sabre/auth.php | 8 +- lib/private/Server.php | 2 +- lib/private/User/Session.php | 70 ++++++++++++++-- tests/lib/User/SessionTest.php | 84 +++++++++++++++++--- 5 files changed, 142 insertions(+), 25 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Auth.php b/apps/dav/lib/Connector/Sabre/Auth.php index 88898f272c..cbb2c2b63b 100644 --- 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()); diff --git a/apps/dav/tests/unit/connector/sabre/auth.php b/apps/dav/tests/unit/connector/sabre/auth.php index 42be21d90f..d3f697ba8e 100644 --- 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); diff --git a/lib/private/Server.php b/lib/private/Server.php index c7b3799448..0b42501326 100644 --- 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)); }); diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 749f395e28..cd867dace7 100644 --- 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 diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index c4424c0348..5ff2a16acb 100644 --- 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();