Dont create a log entry on email login

Signed-off-by: Joas Schilling <coding@schilljs.com>

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) {

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;
 	}
 

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())