diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index abb1df4bcd..a61d087965 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -40,6 +40,7 @@ use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; use OCP\Authentication\TwoFactorAuth\IProvider; use OCP\IConfig; +use OCP\ILogger; use OCP\IRequest; use OCP\ISession; use OCP\IURLGenerator; @@ -59,6 +60,8 @@ class LoginController extends Controller { private $userSession; /** @var IURLGenerator */ private $urlGenerator; + /** @var ILogger */ + private $logger; /** @var Manager */ private $twoFactorManager; /** @var Throttler */ @@ -72,16 +75,18 @@ class LoginController extends Controller { * @param ISession $session * @param IUserSession $userSession * @param IURLGenerator $urlGenerator + * @param ILogger $logger * @param Manager $twoFactorManager * @param Throttler $throttler */ - function __construct($appName, + public function __construct($appName, IRequest $request, IUserManager $userManager, IConfig $config, ISession $session, IUserSession $userSession, IURLGenerator $urlGenerator, + ILogger $logger, Manager $twoFactorManager, Throttler $throttler) { parent::__construct($appName, $request); @@ -90,6 +95,7 @@ class LoginController extends Controller { $this->session = $session; $this->userSession = $userSession; $this->urlGenerator = $urlGenerator; + $this->logger = $logger; $this->twoFactorManager = $twoFactorManager; $this->throttler = $throttler; } @@ -224,13 +230,15 @@ class LoginController extends Controller { $originalUser = $user; // TODO: Add all the insane error handling /* @var $loginResult IUser */ - $loginResult = $this->userManager->checkPassword($user, $password); + $loginResult = $this->userManager->checkPasswordNoLogging($user, $password); if ($loginResult === false) { $users = $this->userManager->getByEmail($user); // we only allow login by email if unique if (count($users) === 1) { $user = $users[0]->getUID(); $loginResult = $this->userManager->checkPassword($user, $password); + } else { + $this->logger->warning('Login failed: \''. $user .'\' (Remote IP: \''. $this->request->getRemoteAddress(). '\')', ['app' => 'core']); } } if ($loginResult === false) { diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 77741efcc7..b62b04feba 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -185,9 +185,27 @@ class Manager extends PublicEmitter implements IUserManager { * @return mixed the User object on success, false otherwise */ public function checkPassword($loginName, $password) { + $result = $this->checkPasswordNoLogging($loginName, $password); + + if ($result === false) { + \OC::$server->getLogger()->warning('Login failed: \''. $loginName .'\' (Remote IP: \''. \OC::$server->getRequest()->getRemoteAddress(). '\')', ['app' => 'core']); + } + + return $result; + } + + /** + * Check if the password is valid for the user + * + * @internal + * @param string $loginName + * @param string $password + * @return mixed the User object on success, false otherwise + */ + public function checkPasswordNoLogging($loginName, $password) { $loginName = str_replace("\0", '', $loginName); $password = str_replace("\0", '', $password); - + foreach ($this->backends as $backend) { if ($backend->implementsActions(Backend::CHECK_PASSWORD)) { $uid = $backend->checkPassword($loginName, $password); @@ -197,7 +215,6 @@ class Manager extends PublicEmitter implements IUserManager { } } - \OC::$server->getLogger()->warning('Login failed: \''. $loginName .'\' (Remote IP: \''. \OC::$server->getRequest()->getRemoteAddress(). '\')', ['app' => 'core']); return false; } diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index 72f921724a..c8e85a1994 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -24,53 +24,51 @@ namespace Tests\Core\Controller; use OC\Authentication\TwoFactorAuth\Manager; use OC\Core\Controller\LoginController; use OC\Security\Bruteforce\Throttler; +use OC\User\Session; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; use OCP\IConfig; +use OCP\ILogger; use OCP\IRequest; use OCP\ISession; use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; -use OCP\IUserSession; use Test\TestCase; class LoginControllerTest extends TestCase { /** @var LoginController */ private $loginController; - /** @var IRequest | \PHPUnit_Framework_MockObject_MockObject */ + /** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */ private $request; - /** @var IUserManager | \PHPUnit_Framework_MockObject_MockObject */ + /** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */ private $userManager; - /** @var IConfig | \PHPUnit_Framework_MockObject_MockObject */ + /** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */ private $config; - /** @var ISession | \PHPUnit_Framework_MockObject_MockObject */ + /** @var ISession|\PHPUnit_Framework_MockObject_MockObject */ private $session; - /** @var IUserSession | \PHPUnit_Framework_MockObject_MockObject */ + /** @var Session|\PHPUnit_Framework_MockObject_MockObject */ private $userSession; - /** @var IURLGenerator | \PHPUnit_Framework_MockObject_MockObject */ + /** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */ private $urlGenerator; - /** @var Manager | \PHPUnit_Framework_MockObject_MockObject */ + /** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */ + private $logger; + /** @var Manager|\PHPUnit_Framework_MockObject_MockObject */ private $twoFactorManager; - /** @var Throttler */ + /** @var Throttler|\PHPUnit_Framework_MockObject_MockObject */ private $throttler; public function setUp() { parent::setUp(); - $this->request = $this->getMockBuilder('\\OCP\\IRequest')->getMock(); - $this->userManager = $this->getMockBuilder('\\OCP\\IUserManager')->getMock(); - $this->config = $this->getMockBuilder('\\OCP\\IConfig')->getMock(); - $this->session = $this->getMockBuilder('\\OCP\\ISession')->getMock(); - $this->userSession = $this->getMockBuilder('\\OC\\User\\Session') - ->disableOriginalConstructor() - ->getMock(); - $this->urlGenerator = $this->getMockBuilder('\\OCP\\IURLGenerator')->getMock(); - $this->twoFactorManager = $this->getMockBuilder('\OC\Authentication\TwoFactorAuth\Manager') - ->disableOriginalConstructor() - ->getMock(); - $this->throttler = $this->getMockBuilder('\OC\Security\Bruteforce\Throttler') - ->disableOriginalConstructor() - ->getMock(); + $this->request = $this->createMock(IRequest::class); + $this->userManager = $this->createMock(\OC\User\Manager::class); + $this->config = $this->createMock(IConfig::class); + $this->session = $this->createMock(ISession::class); + $this->userSession = $this->createMock(Session::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->logger = $this->createMock(ILogger::class); + $this->twoFactorManager = $this->createMock(Manager::class); + $this->throttler = $this->createMock(Throttler::class); $this->loginController = new LoginController( 'core', @@ -80,6 +78,7 @@ class LoginControllerTest extends TestCase { $this->session, $this->userSession, $this->urlGenerator, + $this->logger, $this->twoFactorManager, $this->throttler ); @@ -110,7 +109,7 @@ class LoginControllerTest extends TestCase { ->method('getCookie') ->with('nc_token') ->willReturn('MyLoginToken'); - $user = $this->getMockBuilder('\\OCP\\IUser')->getMock(); + $user = $this->createMock(IUser::class); $user ->expects($this->once()) ->method('getUID') @@ -217,7 +216,7 @@ class LoginControllerTest extends TestCase { ->method('getSystemValue') ->with('lost_password_link') ->willReturn(false); - $user = $this->getMockBuilder('\\OCP\\IUser')->getMock(); + $user = $this->createMock(IUser::class); $user ->expects($this->once()) ->method('canChangePassword') @@ -255,7 +254,7 @@ class LoginControllerTest extends TestCase { ->method('getSystemValue') ->with('lost_password_link') ->willReturn(false); - $user = $this->getMockBuilder('\\OCP\\IUser')->getMock(); + $user = $this->createMock(IUser::class); $user ->expects($this->once()) ->method('canChangePassword') @@ -289,7 +288,7 @@ class LoginControllerTest extends TestCase { $loginPageUrl = 'some url'; $this->request - ->expects($this->exactly(4)) + ->expects($this->exactly(5)) ->method('getRemoteAddress') ->willReturn('192.168.0.1'); $this->request @@ -310,7 +309,7 @@ class LoginControllerTest extends TestCase { ->method('registerAttempt') ->with('login', '192.168.0.1', ['user' => 'MyUserName']); $this->userManager->expects($this->once()) - ->method('checkPassword') + ->method('checkPasswordNoLogging') ->will($this->returnValue(false)); $this->urlGenerator->expects($this->once()) ->method('linkToRoute') @@ -329,8 +328,8 @@ class LoginControllerTest extends TestCase { } public function testLoginWithValidCredentials() { - /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->getMockBuilder('\OCP\IUser')->getMock(); + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock(IUser::class); $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('uid')); @@ -358,7 +357,7 @@ class LoginControllerTest extends TestCase { ->with('192.168.0.1') ->willReturn(200); $this->userManager->expects($this->once()) - ->method('checkPassword') + ->method('checkPasswordNoLogging') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('login') @@ -391,8 +390,8 @@ class LoginControllerTest extends TestCase { } public function testLoginWithValidCredentialsAndRememberMe() { - /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->getMockBuilder('\OCP\IUser')->getMock(); + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock(IUser::class); $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('uid')); @@ -417,7 +416,7 @@ class LoginControllerTest extends TestCase { ->with('192.168.0.1') ->willReturn(200); $this->userManager->expects($this->once()) - ->method('checkPassword') + ->method('checkPasswordNoLogging') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('login') @@ -441,8 +440,8 @@ class LoginControllerTest extends TestCase { } public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn() { - /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->getMockBuilder('\OCP\IUser')->getMock(); + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock(IUser::class); $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('jane')); @@ -480,8 +479,8 @@ class LoginControllerTest extends TestCase { } public function testLoginWithoutPassedCsrfCheckAndLoggedIn() { - /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->getMockBuilder('\OCP\IUser')->getMock(); + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock(IUser::class); $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('jane')); @@ -524,8 +523,8 @@ class LoginControllerTest extends TestCase { } public function testLoginWithValidCredentialsAndRedirectUrl() { - /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->getMockBuilder('\OCP\IUser')->getMock(); + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock(IUser::class); $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('jane')); @@ -551,7 +550,7 @@ class LoginControllerTest extends TestCase { ->with('192.168.0.1') ->willReturn(200); $this->userManager->expects($this->once()) - ->method('checkPassword') + ->method('checkPasswordNoLogging') ->with('Jane', $password) ->will($this->returnValue($user)); $this->userSession->expects($this->once()) @@ -574,8 +573,8 @@ class LoginControllerTest extends TestCase { } public function testLoginWithOneTwoFactorProvider() { - /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->getMockBuilder('\OCP\IUser')->getMock(); + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock(IUser::class); $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('john')); @@ -601,7 +600,7 @@ class LoginControllerTest extends TestCase { ->with('192.168.0.1') ->willReturn(200); $this->userManager->expects($this->once()) - ->method('checkPassword') + ->method('checkPasswordNoLogging') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('login') @@ -640,8 +639,8 @@ class LoginControllerTest extends TestCase { } public function testLoginWithMultpleTwoFactorProviders() { - /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->getMockBuilder('\OCP\IUser')->getMock(); + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock(IUser::class); $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('john')); @@ -668,7 +667,7 @@ class LoginControllerTest extends TestCase { ->with('192.168.0.1') ->willReturn(200); $this->userManager->expects($this->once()) - ->method('checkPassword') + ->method('checkPasswordNoLogging') ->will($this->returnValue($user)); $this->userSession->expects($this->once()) ->method('login') @@ -706,18 +705,19 @@ class LoginControllerTest extends TestCase { } public function testToNotLeakLoginName() { - /** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */ - $user = $this->getMockBuilder('\OCP\IUser')->getMock(); + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock(IUser::class); $user->expects($this->any()) ->method('getUID') ->will($this->returnValue('john')); - $this->userManager->expects($this->exactly(2)) + $this->userManager->expects($this->once()) + ->method('checkPasswordNoLogging') + ->with('john@doe.com', 'just wrong') + ->willReturn(false); + $this->userManager->expects($this->once()) ->method('checkPassword') - ->withConsecutive( - ['john@doe.com', 'just wrong'], - ['john', 'just wrong'] - ) + ->with('john', 'just wrong') ->willReturn(false); $this->userManager->expects($this->once())