Do not change the old behavior of user creation

owncloud · Dec 18, 2019 · 97eeafc · 97eeafc
1 parent 712327a
commit 97eeafc
Show file tree

Hide file tree

Showing 11 changed files with 102 additions and 92 deletions.
diff --git 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
@@ -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
@@ -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('<error>--password-from-env given, but OC_PASS is empty!</error>');
 				return 1;
 			}
@@ -136,12 +144,6 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 		} catch (InvalidEmailException $e) {
 			$output->writeln('<error>Invalid email address supplied</error>');
 			return 1;
-		} catch (CannotCreateUserException $e) {
-			$output->writeln("<error>" . $e->getMessage() .  "</error>");
-			return 1;
-		} catch (UserAlreadyExistsException $e) {
-			$output->writeln("<error>" . $e->getMessage() .  "</error>");
-			return 1;
 		}
 
 		if ($user instanceof IUser) {

diff --git 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
@@ -4,6 +4,7 @@
  * @author Morris Jobke <[email protected]>
  * @author Thomas Müller <[email protected]>
  * @author Sujith Haridasan <[email protected]>
+ * @author Semih Serhat Karakaya <[email protected]>
  *
  * @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,15 +71,16 @@ 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,
 								$userManager,
 								$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
@@ -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
@@ -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);