Commit f15c3c21 authored by Vincent Petry's avatar Vincent Petry Committed by GitHub
Browse files

Merge pull request #27011 from owncloud/fix_owncloud_user_enumeration_vulnerbility_#23595

fix user enumeration with logging
parents 6c42cf4e c3039740
......@@ -72,7 +72,8 @@ class Application extends App {
$c->query('DefaultEmailAddress'),
$c->query('IsEncryptionEnabled'),
$c->query('Mailer'),
$c->query('TimeFactory')
$c->query('TimeFactory'),
$c->query('Logger')
);
});
$container->registerService('UserController', function(SimpleContainer $c) {
......
......@@ -31,6 +31,7 @@ namespace OC\Core\Controller;
use \OCP\AppFramework\Controller;
use \OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\ILogger;
use \OCP\IURLGenerator;
use \OCP\IRequest;
use \OCP\IL10N;
......@@ -70,6 +71,8 @@ class LostController extends Controller {
protected $mailer;
/** @var ITimeFactory */
protected $timeFactory;
/** @var ILogger */
protected $logger;
/**
* @param string $appName
......@@ -84,6 +87,7 @@ class LostController extends Controller {
* @param string $isDataEncrypted
* @param IMailer $mailer
* @param ITimeFactory $timeFactory
* @param ILogger $logger
*/
public function __construct($appName,
IRequest $request,
......@@ -96,7 +100,8 @@ class LostController extends Controller {
$from,
$isDataEncrypted,
IMailer $mailer,
ITimeFactory $timeFactory) {
ITimeFactory $timeFactory,
ILogger $logger) {
parent::__construct($appName, $request);
$this->urlGenerator = $urlGenerator;
$this->userManager = $userManager;
......@@ -108,6 +113,7 @@ class LostController extends Controller {
$this->config = $config;
$this->mailer = $mailer;
$this->timeFactory = $timeFactory;
$this->logger = $logger;
}
/**
......@@ -236,42 +242,41 @@ class LostController extends Controller {
* @throws \Exception
*/
protected function sendEmail($user) {
if (!$this->userManager->userExists($user)) {
throw new \Exception($this->l10n->t('Couldn\'t send reset email. Please make sure your username is correct.'));
}
$userObject = $this->userManager->get($user);
$email = $userObject->getEMailAddress();
if ($this->userManager->userExists($user)) {
if (empty($email)) {
throw new \Exception(
$this->l10n->t('Could not send reset email because there is no email address for this username. Please contact your administrator.')
);
}
$userObject = $this->userManager->get($user);
$email = $userObject->getEMailAddress();
$token = $this->secureRandom->generate(21,
ISecureRandom::CHAR_DIGITS.
ISecureRandom::CHAR_LOWER.
ISecureRandom::CHAR_UPPER);
$this->config->setUserValue($user, 'owncloud', 'lostpassword', $this->timeFactory->getTime() .':'. $token);
if (!empty($email)) {
$token = $this->secureRandom->generate(21,
ISecureRandom::CHAR_DIGITS .
ISecureRandom::CHAR_LOWER .
ISecureRandom::CHAR_UPPER);
$this->config->setUserValue($user, 'owncloud', 'lostpassword', $this->timeFactory->getTime() . ':' . $token);
$link = $this->urlGenerator->linkToRouteAbsolute('core.lost.resetform', ['userId' => $user, 'token' => $token]);
$link = $this->urlGenerator->linkToRouteAbsolute('core.lost.resetform', ['userId' => $user, 'token' => $token]);
$tmpl = new \OC_Template('core', 'lostpassword/email');
$tmpl->assign('link', $link);
$msg = $tmpl->fetchPage();
$tmpl = new \OC_Template('core', 'lostpassword/email');
$tmpl->assign('link', $link);
$msg = $tmpl->fetchPage();
try {
$message = $this->mailer->createMessage();
$message->setTo([$email => $user]);
$message->setSubject($this->l10n->t('%s password reset', [$this->defaults->getName()]));
$message->setPlainBody($msg);
$message->setFrom([$this->from => $this->defaults->getName()]);
$this->mailer->send($message);
} catch (\Exception $e) {
throw new \Exception($this->l10n->t(
'Couldn\'t send reset email. Please contact your administrator.'
));
try {
$message = $this->mailer->createMessage();
$message->setTo([$email => $user]);
$message->setSubject($this->l10n->t('%s password reset', [$this->defaults->getName()]));
$message->setPlainBody($msg);
$message->setFrom([$this->from => $this->defaults->getName()]);
$this->mailer->send($message);
} catch (\Exception $e) {
throw new \Exception($this->l10n->t(
'Couldn\'t send reset email. Please contact your administrator.'
));
}
} else {
$this->logger->error('Could not send reset email because there is no email address for this username. User: {user}', ['app' => 'core', 'user' => $user]);
}
} else {
$this->logger->error('Could not send reset email because User does not exist. User: {user}', ['app' => 'core', 'user' => $user]);
}
}
......
......@@ -26,6 +26,7 @@ use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\IConfig;
use OCP\IL10N;
use OCP\ILogger;
use OCP\IRequest;
use OCP\IURLGenerator;
use OCP\IUser;
......@@ -63,6 +64,8 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
private $timeFactory;
/** @var IRequest */
private $request;
/** @var ILogger */
private $logger;
protected function setUp() {
......@@ -98,6 +101,8 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
->disableOriginalConstructor()->getMock();
$this->request = $this->getMockBuilder('OCP\IRequest')
->disableOriginalConstructor()->getMock();
$this->logger = $this->getMockBuilder('OCP\ILogger')
->disableOriginalConstructor()->getMock();
$this->lostController = new LostController(
'Core',
$this->request,
......@@ -110,7 +115,8 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
'lostpassword-noreply@localhost',
true,
$this->mailer,
$this->timeFactory
$this->timeFactory,
$this->logger
);
}
......@@ -236,7 +242,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
}
public function testEmailUnsucessful() {
$existingUser = 'ExistingUser';
$existingUser = 'ExistingUser1';
$nonExistingUser = 'NonExistingUser';
$this->userManager
->expects($this->any())
......@@ -249,9 +255,11 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
// With a non existing user
$response = $this->lostController->email($nonExistingUser);
$expectedResponse = [
'status' => 'error',
'msg' => 'Couldn\'t send reset email. Please make sure your username is correct.'
'status' => 'success'
];
$this->logger->expects($this->any())
->method('error')
->with('Could not send reset email because User does not exist. User: {user}');
$this->assertSame($expectedResponse, $response);
// With no mail address
......@@ -259,11 +267,13 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
->expects($this->any())
->method('getUserValue')
->with($existingUser, 'settings', 'email')
->will($this->returnValue(null));
->will($this->returnValue(''));
$response = $this->lostController->email($existingUser);
$this->logger->expects($this->any())
->method('error')
->with('Could not send reset email because there is no email address for this username. User: {user}');
$expectedResponse = [
'status' => 'error',
'msg' => 'Couldn\'t send reset email. Please make sure your username is correct.'
'status' => 'success'
];
$this->assertSame($expectedResponse, $response);
}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment