mbk-lab
/
nextcloud
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Merge pull request #3043 from nextcloud/issue-3038-no-logentry-on-email-login 

Dont create a log entry on email login
This commit is contained in:
Morris Jobke 2017-04-13 01:04:11 -05:00 committed by GitHub
parent 396618fd19 7ad791efb4
commit 7cb6038fca
3 changed files with 83 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
core/Controller/LoginController.php
View File

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

21
lib/private/User/Manager.php
View File

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

108
tests/Core/Controller/LoginControllerTest.php
View File

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