diff --git a/apps/provisioning_api/lib/Users.php b/apps/provisioning_api/lib/Users.php index b3b656b6f365..13580b4eb1a9 100644 --- a/apps/provisioning_api/lib/Users.php +++ b/apps/provisioning_api/lib/Users.php @@ -33,7 +33,6 @@ use OC\User\Service\CreateUserService; use OC_Helper; use OCP\API; -use OCP\Files\FileInfo; use OCP\Files\NotFoundException; use OCP\IGroup; use OCP\IGroupManager; @@ -145,11 +144,6 @@ public function addUser() { return new Result(null, API::RESPOND_UNAUTHORISED); } - if ($this->userManager->userExists($userId)) { - $this->logger->error('Failed addUser attempt: User already exists.', ['app' => 'ocs_api']); - return new Result(null, 102, 'User already exists'); - } - if (\is_array($groups) && (\count($groups) > 0)) { foreach ($groups as $group) { if (!$this->groupManager->groupExists($group)) { @@ -165,6 +159,11 @@ public function addUser() { } } + if ($this->userManager->userExists($userId)) { + $this->logger->error('Failed addUser attempt: User already exists.', ['app' => 'ocs_api']); + return new Result(null, 102, 'User already exists'); + } + try { $newUser = $this->createUserService->createUser(['username' => $userId, 'password' => $password, 'email' => $emailAddress]); $this->logger->info('Successful addUser call with userid: '.$userId, ['app' => 'ocs_api']); diff --git a/core/Application.php b/core/Application.php index ecb9b6654948..d8633a471d67 100644 --- a/core/Application.php +++ b/core/Application.php @@ -45,7 +45,6 @@ use OCP\IConfig; use OCP\ILogger; use OCP\IServerContainer; -use OCP\IURLGenerator; use OCP\Util; /** @@ -93,7 +92,8 @@ public function __construct(array $urlParams= []) { $c->query('OC\User\Service\UserSendMailService'), $c->query('URLGenerator'), $c->query('Logger'), - $c->query('L10N') + $c->query('L10N'), + $c->query('UserSession') ); }); $container->registerService('AvatarController', static function (SimpleContainer $c) { diff --git a/core/Command/User/Add.php b/core/Command/User/Add.php index 4a3d9665fc1f..6ea58613a9d7 100644 --- a/core/Command/User/Add.php +++ b/core/Command/User/Add.php @@ -32,6 +32,7 @@ use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; +use Symfony\Component\Console\Output\ConsoleOutputInterface; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Question\Question; @@ -91,6 +92,13 @@ protected function configure() { ); } + /** + * @param InputInterface $input + * @param OutputInterface $output + * @return int + * @throws CannotCreateUserException + * @throws UserAlreadyExistsException + */ protected function execute(InputInterface $input, OutputInterface $output) { $uid = $input->getArgument('uid'); $email = $input->getOption('email'); @@ -110,7 +118,7 @@ protected function execute(InputInterface $input, OutputInterface $output) { $password = ''; if ($passwordFromEnv) { $password = \getenv('OC_PASS'); - if (!$password) { + if ($password === false || $password === "") { $output->writeln('--password-from-env given, but OC_PASS is empty!'); return 1; } @@ -136,12 +144,6 @@ protected function execute(InputInterface $input, OutputInterface $output) { } catch (InvalidEmailException $e) { $output->writeln('Invalid email address supplied'); return 1; - } catch (CannotCreateUserException $e) { - $output->writeln("" . $e->getMessage() . ""); - return 1; - } catch (UserAlreadyExistsException $e) { - $output->writeln("" . $e->getMessage() . ""); - return 1; } if ($user instanceof IUser) { diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php index 4a1f6ccc712a..1c4c63579c0f 100644 --- a/core/Controller/LostController.php +++ b/core/Controller/LostController.php @@ -46,8 +46,6 @@ /** * Class LostController * - * Successfully changing a password will emit the post_passwordReset hook. - * * @package OC\Core\Controller */ class LostController extends Controller { @@ -252,8 +250,7 @@ public function setPassword($token, $userId, $password, $proceed) { throw new \Exception(); } - \OC_Hook::emit('\OC\Core\LostPassword\Controller\LostController', 'post_passwordReset', ['uid' => $userId, 'password' => $password]); - @\OC_User::unsetMagicInCookie(); + $this->userSession->unsetMagicInCookie(); } catch (\Exception $e) { return $this->error($e->getMessage()); } diff --git a/core/Controller/UserController.php b/core/Controller/UserController.php index 12f1a13bcd3b..279f34f1ebf3 100644 --- a/core/Controller/UserController.php +++ b/core/Controller/UserController.php @@ -4,6 +4,7 @@ * @author Morris Jobke * @author Thomas Müller * @author Sujith Haridasan + * @author Semih Serhat Karakaya * * @copyright Copyright (c) 2018, ownCloud GmbH * @license AGPL-3.0 @@ -25,6 +26,7 @@ namespace OC\Core\Controller; use OC\User\Service\UserSendMailService; +use OC\User\Session; use \OCP\AppFramework\Controller; use OCP\AppFramework\Http; use \OCP\AppFramework\Http\JSONResponse; @@ -38,14 +40,10 @@ use OCP\User\Exceptions\UserTokenExpiredException; class UserController extends Controller { - /** - * @var \OCP\IUserManager - */ + /** @var \OCP\IUserManager */ protected $userManager; - /** - * @var \OC_Defaults - */ + /** @var \OC_Defaults */ protected $defaults; /** @var UserSendMailService */ @@ -60,6 +58,9 @@ class UserController extends Controller { /** @var IL10N */ private $l10n; + /** @var Session */ + private $session; + /** * UserController constructor. * @@ -70,7 +71,8 @@ class UserController extends Controller { * @param UserSendMailService $userSendMailService * @param IURLGenerator $urlGenerator * @param ILogger $logger - * @param IL10N $l10n + * @param IL10N $l10n, + * @param Session $session */ public function __construct($appName, IRequest $request, @@ -78,7 +80,7 @@ public function __construct($appName, $defaults, UserSendMailService $userSendMailService, IURLGenerator $urlGenerator, ILogger $logger, - IL10N $l10n + IL10N $l10n, Session $session ) { parent::__construct($appName, $request); $this->userManager = $userManager; @@ -87,6 +89,7 @@ public function __construct($appName, $this->urlGenerator = $urlGenerator; $this->logger = $logger; $this->l10n = $l10n; + $this->session = $session; } /** @@ -147,7 +150,7 @@ public function setPasswordForm($token, $userId) { return new TemplateResponse( 'core', 'error', [ - "errors" => [["error" => $this->l10n->t($e->getMessage())]] + "errors" => [["error" => $e->getMessage()]] ], 'guest' ); } @@ -173,7 +176,7 @@ public function resendToken($userId) { $user = $this->userManager->get($userId); if ($user === null) { - $this->logger->error('User: ' . $userId . ' does not exist', ['app' => 'core']); + $this->logger->error("Failed to create activation link. User $userId does not exists", ['app' => 'core']); return new TemplateResponse( 'core', 'error', [ @@ -231,7 +234,7 @@ public function setPassword($token, $userId, $password) { return new JSONResponse( [ 'status' => 'error', - 'message' => $this->l10n->t('Failed to set password. Please contact the administrator.', [$userId]), + 'message' => $this->l10n->t('Failed to set password for %s. Please contact the administrator.', [$userId]), 'type' => 'usererror' ], Http::STATUS_NOT_FOUND ); @@ -245,14 +248,12 @@ public function setPassword($token, $userId, $password) { return new JSONResponse( [ 'status' => 'error', - 'message' => $this->l10n->t('Failed to set password. Please contact your administrator.', [$userId]), + 'message' => $this->l10n->t('Failed to set password for %s. Please contact your administrator.', [$userId]), 'type' => 'passwordsetfailed' ], Http::STATUS_FORBIDDEN ); } - - \OC_Hook::emit('\OC\Core\LostPassword\Controller\LostController', 'post_passwordReset', ['uid' => $userId, 'password' => $password]); - @\OC_User::unsetMagicInCookie(); + $this->session->unsetMagicInCookie(); } catch (UserTokenException $e) { $this->logger->logException($e, ['app' => 'core']); return new JSONResponse( @@ -267,11 +268,11 @@ public function setPassword($token, $userId, $password) { try { $this->userSendMailService->sendNotificationMail($user); } catch (EmailSendFailedException $e) { - $this->logger->logException($e, ['app' => 'user_management']); + $this->logger->logException($e, ['app' => 'core']); return new JSONResponse( [ 'status' => 'error', - 'message' => $this->l10n->t('Failed to send email. Please contact your administrator.'), + 'message' => $this->l10n->t('Password set successfully but failed to send email. Please contact your administrator for details.'), 'type' => 'emailsendfailed' ], Http::STATUS_INTERNAL_SERVER_ERROR ); diff --git a/core/js/setpassword.js b/core/js/setpassword.js index 35df031ebc3e..3efecfe56e90 100644 --- a/core/js/setpassword.js +++ b/core/js/setpassword.js @@ -23,20 +23,19 @@ passwordObj.val(''); retypePasswordObj.val(''); passwordObj.parent().addClass('shake'); - $('#message').addClass('warning'); - $('#message').text('Passwords do not match'); - $('#message').show(); + $('#message').addClass('warning') + .text('Passwords do not match') + .show(); passwordObj.focus(); } }, _onSetPasswordFail: function(result) { - var responseObj = JSON.parse(result.responseText); + var response = JSON.parse(result.responseText); var errorObject = $('#error-message'); - var showErrorMessage = false; var errorMessage; - errorMessage = responseObj.message; + errorMessage = response.message; if (!errorMessage) { errorMessage = t('core', 'Failed to set password. Please contact your administrator.'); @@ -49,15 +48,15 @@ _resetDone : function(result){ if (result && result.status === 'success') { - var getRootPath = OC.getRootPath(); - if (getRootPath === '') { + var rootPath = OC.getRootPath(); + if (rootPath === '') { /** * If owncloud is not run inside subfolder, the getRootPath * will return empty string */ - getRootPath = "/"; + rootPath = "/"; } - OC.redirect(getRootPath); + OC.redirect(rootPath); } } }; @@ -76,8 +75,7 @@ $(document).ready(function () { Else it should not. */ if (($('#password').val().length >= 0) && ($('#retypepassword').val().length === 0)) { - $('#message').removeClass('warning'); - $('#message').text(''); + $('#message').removeClass('warning').text(''); } }); }); diff --git a/lib/private/User/Service/CreateUserService.php b/lib/private/User/Service/CreateUserService.php index 918edbe084bb..5ff0ad5901f2 100644 --- a/lib/private/User/Service/CreateUserService.php +++ b/lib/private/User/Service/CreateUserService.php @@ -92,12 +92,17 @@ public function __construct(IUserSession $userSession, IGroupManager $groupManag * @throws UserAlreadyExistsException */ public function createUser($arguments) { - $username = $password = $email = ''; - if (\array_key_exists('username', $arguments)) { + $password = $email = ''; + if (isset($arguments['username'])) { $username = $arguments['username']; } else { throw new CannotCreateUserException("Unable to create user due to missing user name"); } + + if ($this->userManager->userExists($username)) { + throw new UserAlreadyExistsException('A user with that name already exists.'); + } + if (\array_key_exists('password', $arguments)) { $password = $arguments['password']; } @@ -109,10 +114,6 @@ public function createUser($arguments) { throw new InvalidEmailException("Invalid mail address"); } - if ($this->userManager->userExists($username)) { - throw new UserAlreadyExistsException('A user with that name already exists.'); - } - try { $oldPassword = $password; if (($password === '') && ($email !== '')) { @@ -125,7 +126,7 @@ public function createUser($arguments) { } $user = $this->userManager->createUser($username, $password); } catch (\Exception $exception) { - throw new CannotCreateUserException("Unable to create user due to exception: {$exception->getMessage()}"); + throw new CannotCreateUserException($exception->getMessage()); } if ($user === false) { @@ -169,7 +170,7 @@ public function createUser($arguments) { public function addUserToGroups(IUser $user, array $groups, $checkInGroup = true) { $failedToAdd = []; - if (\is_array($groups) && \count($groups) > 0) { + if (\count($groups) > 0) { foreach ($groups as $groupName) { $groupObject = $this->groupManager->get($groupName); diff --git a/lib/private/User/Service/UserSendMailService.php b/lib/private/User/Service/UserSendMailService.php index f6ade5a9df58..8d6a70e56cd0 100644 --- a/lib/private/User/Service/UserSendMailService.php +++ b/lib/private/User/Service/UserSendMailService.php @@ -83,8 +83,7 @@ public function __construct(ISecureRandom $secureRandom, IConfig $config, public function generateTokenAndSendMail($userId, $email) { $fromMailAddress = Util::getDefaultEmailAddress('no-reply'); $token = $this->secureRandom->generate(21, - ISecureRandom::CHAR_DIGITS, - ISecureRandom::CHAR_LOWER, ISecureRandom::CHAR_UPPER); + ISecureRandom::CHAR_DIGITS . ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_UPPER); $this->config->setUserValue($userId, 'owncloud', 'lostpassword', $this->timeFactory->getTime() . ':' . $token); @@ -126,18 +125,20 @@ public function checkPasswordSetToken($token, IUser $user) { $splittedToken = \explode(':', $this->config->getUserValue($user->getUID(), 'owncloud', 'lostpassword', null)); if (\count($splittedToken) !== 2) { $this->config->deleteUserValue($user->getUID(), 'owncloud', 'lostpassword'); - throw new InvalidUserTokenException('The token provided is invalid.'); + throw new InvalidUserTokenException($this->l10n->t('The token provided is invalid.')); } //The value 43200 = 60*60*12 = 1/2 day - if ($splittedToken[0] < ($this->timeFactory->getTime() - (int)$this->config->getAppValue('core', 'token_expire_time', '43200')) || + $tokenExpireTime = (int)$this->config->getAppValue('core', 'token_expire_time', '43200'); + $lastValidTimeForToken = $this->timeFactory->getTime() - $tokenExpireTime; + if ($splittedToken[0] < $lastValidTimeForToken || $user->getLastLogin() > $splittedToken[0]) { $this->config->deleteUserValue($user->getUID(), 'owncloud', 'lostpassword'); - throw new UserTokenExpiredException('The token provided had expired.'); + throw new UserTokenExpiredException($this->l10n->t('The token provided had expired.')); } if (!\hash_equals($splittedToken[1], $token)) { - throw new UserTokenMismatchException('The token provided is invalid.'); + throw new UserTokenMismatchException($this->l10n->t('The token provided is invalid.')); } } @@ -153,22 +154,21 @@ public function sendNotificationMail(IUser $user) { $fromMailAddress = Util::getDefaultEmailAddress('no-reply'); if ($email !== '') { + $tmpl = new \OC_Template('core', 'lostpassword/notify'); + $msg = $tmpl->fetchPage(); + $tmplAlt = new \OC_Template('core', 'lostpassword/altnotify'); + $msgAlt = $tmplAlt->fetchPage(); + $message = $this->mailer->createMessage(); + $message->setTo([$email => $user->getUID()]); + $message->setSubject($this->l10n->t('%s password changed successfully', [$this->defaults->getName()])); + $message->setHtmlBody($msg); + $message->setPlainBody($msgAlt); + $message->setFrom([$fromMailAddress => $this->defaults->getName()]); try { - $tmpl = new \OC_Template('core', 'lostpassword/notify'); - $msg = $tmpl->fetchPage(); - $tmplAlt = new \OC_Template('core', 'lostpassword/altnotify'); - $msgAlt = $tmplAlt->fetchPage(); - - $message = $this->mailer->createMessage(); - $message->setTo([$email => $user->getUID()]); - $message->setSubject($this->l10n->t('%s password changed successfully', [$this->defaults->getName()])); - $message->setHtmlBody($msg); - $message->setPlainBody($msgAlt); - $message->setFrom([$fromMailAddress => $this->defaults->getName()]); $this->mailer->send($message); return true; } catch (\Exception $exception) { - throw new EmailSendFailedException("Email could not be sent."); + throw new EmailSendFailedException($this->l10n->t("Email could not be sent.")); } } return false; diff --git a/tests/Core/Controller/UserControllerTest.php b/tests/Core/Controller/UserControllerTest.php index 5648853735a6..997495b4731b 100644 --- a/tests/Core/Controller/UserControllerTest.php +++ b/tests/Core/Controller/UserControllerTest.php @@ -24,6 +24,7 @@ use OC\AppFramework\Http; use OC\Core\Controller\UserController; use OC\User\Service\UserSendMailService; +use OC\User\Session; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\TemplateResponse; use OCP\IL10N; @@ -60,6 +61,8 @@ class UserControllerTest extends TestCase { private $logger; /** @var IL10N | \PHPUnit_Framework_MockObject_MockObject */ private $l10n; + /** @var Session | \PHPUnit_Framework_MockObject_MockObject */ + private $session; /** @var UserController */ private $userController; @@ -73,9 +76,10 @@ public function setUp(): void { $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->logger = $this->createMock(ILogger::class); $this->l10n = $this->createMock(IL10N::class); + $this->session = $this->createMock(Session::class); $this->userController = new UserController('core', $this->request, $this->userManager, $this->defaults, $this->userSendMailService, - $this->urlGenerator, $this->logger, $this->l10n); + $this->urlGenerator, $this->logger, $this->l10n, $this->session); } public function testSetPasswordForm() { diff --git a/tests/lib/User/Service/CreateUserServiceTest.php b/tests/lib/User/Service/CreateUserServiceTest.php index 5b8eb1dafcea..039cf9e1affb 100644 --- a/tests/lib/User/Service/CreateUserServiceTest.php +++ b/tests/lib/User/Service/CreateUserServiceTest.php @@ -116,20 +116,22 @@ public function testAddUserToGroups() { } /** - * @expectedExceptionMessage Invalid mail address - * @expectedException \OCP\User\Exceptions\InvalidEmailException */ public function testInvalidEmail() { + $this->expectException(\OCP\User\Exceptions\InvalidEmailException::class); + $this->expectExceptionMessage('Invalid mail address'); + $this->mailer->method('validateMailAddress') ->willReturn(false); $this->createUserService->createUser(['username' => 'foo', 'password' => '', 'email' => 'foo@bar']); } /** - * @expectedExceptionMessage A user with that name already exists. - * @expectedException \OCP\User\Exceptions\UserAlreadyExistsException */ public function testAlreadyExistingUser() { + $this->expectException(\OCP\User\Exceptions\UserAlreadyExistsException::class); + $this->expectExceptionMessage('A user with that name already exists.'); + $this->userSendMailService->method('validateEmailAddress') ->willReturn(true); @@ -140,10 +142,11 @@ public function testAlreadyExistingUser() { } /** - * @expectedExceptionMessage Unable to create user due to exception: - * @expectedException \OCP\User\Exceptions\CannotCreateUserException */ public function testUserCreateException() { + $this->expectException(\OCP\User\Exceptions\CannotCreateUserException::class); + $this->expectExceptionMessage('Exception Message'); + $this->userSendMailService->method('validateEmailAddress') ->willReturn(true); @@ -151,15 +154,16 @@ public function testUserCreateException() { ->willReturn(false); $this->userManager->method('createUser') - ->willThrowException(new \Exception()); + ->willThrowException(new \Exception("Exception Message")); $this->createUserService->createUser(['username' => 'foo', 'password' => '', 'email' => 'foo@bar.com']); } /** - * @expectedExceptionMessage Unable to create user. - * @expectedException \OCP\User\Exceptions\CannotCreateUserException */ public function testUserCreateFailed() { + $this->expectException(\OCP\User\Exceptions\CannotCreateUserException::class); + $this->expectExceptionMessage('Unable to create user.'); + $this->userSendMailService->method('validateEmailAddress') ->willReturn(true); diff --git a/tests/lib/User/Service/UserSendMailServiceTest.php b/tests/lib/User/Service/UserSendMailServiceTest.php index 8f1ce3b31eb3..e87b401fdfa2 100644 --- a/tests/lib/User/Service/UserSendMailServiceTest.php +++ b/tests/lib/User/Service/UserSendMailServiceTest.php @@ -99,10 +99,11 @@ public function testGenerateTokenAndSendMail() { } /** - * @expectedException OCP\User\Exceptions\InvalidUserTokenException - * @expectedExceptionMessage The token provided is invalid. */ public function testcheckPasswordSetInvalidToken() { + $this->expectException(\OCP\User\Exceptions\InvalidUserTokenException::class); + $this->expectExceptionMessage('The token provided is invalid.'); + $user = $this->createMock(IUser::class); $user->method('getUID') ->willReturn('foo'); @@ -113,10 +114,11 @@ public function testcheckPasswordSetInvalidToken() { } /** - * @expectedException \OCP\User\Exceptions\UserTokenExpiredException - * @expectedExceptionMessage The token provided had expired. */ public function testCheckPasswordSetTokenExpired() { + $this->expectException(\OCP\User\Exceptions\UserTokenExpiredException::class); + $this->expectExceptionMessage('The token provided had expired.'); + $user = $this->createMock(IUser::class); $user->method('getUID') ->willReturn('foo'); @@ -133,10 +135,11 @@ public function testCheckPasswordSetTokenExpired() { } /** - * @expectedException \OCP\User\Exceptions\UserTokenMismatchException - * @expectedExceptionMessage The token provided is invalid. */ public function testCheckPasswordSetTokenMismatch() { + $this->expectException(\OCP\User\Exceptions\UserTokenMismatchException::class); + $this->expectExceptionMessage('The token provided is invalid.'); + $user = $this->createMock(IUser::class); $user->method('getUID') ->willReturn('foo'); @@ -187,10 +190,11 @@ public function testSendNotificationMailFail() { } /** - * @expectedException \OCP\User\Exceptions\EmailSendFailedException - * @expectedExceptionMessage Email could not be sent. */ public function testSendNotificationMailSendFail() { + $this->expectException(\OCP\User\Exceptions\EmailSendFailedException::class); + $this->expectExceptionMessage('Email could not be sent.'); + $user = $this->createMock(IUser::class); $user->method('getEMailAddress') ->willReturn('foo@barr.com');