new user password email option, improved on #29368

Signed-off-by: Anupam Kumar <[email protected]>

core/Command/User/Add.php

@@ -2,6 +2,7 @@
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
+ * @author Anupam Kumar <[email protected]>
  * @author Arthur Schiwon <[email protected]>
  * @author Christoph Wurst <[email protected]>
  * @author Joas Schilling <[email protected]>
@@ -25,8 +26,6 @@
  */
 namespace OC\Core\Command\User;
 
-use Egulias\EmailValidator\EmailValidator;
-use Egulias\EmailValidator\Validation\RFCValidation;
 use OC\Files\Filesystem;
 use OCA\Settings\Mailer\NewUserMailHelper;
 use OCP\EventDispatcher\IEventDispatcher;
@@ -35,6 +34,7 @@
 use OCP\IGroupManager;
 use OCP\IUser;
 use OCP\IUserManager;
+use OCP\Mail\IMailer;
 use OCP\Security\Events\GenerateSecurePasswordEvent;
 use OCP\Security\ISecureRandom;
 use Symfony\Component\Console\Command\Command;
@@ -46,63 +46,16 @@
 use Symfony\Component\Console\Question\Question;
 
 class Add extends Command {
-	/**
-	 * @var IUserManager
-	 */
-	protected $userManager;
-
-	/**
-	 * @var IGroupManager
-	 */
-	protected $groupManager;
-
-	/**
-	 * @var EmailValidator
-	 */
-	protected $emailValidator;
-
-	/**
-	 * @var IConfig
-	 */
-	private $config;
-
-	/**
-	 * @var NewUserMailHelper
-	 */
-	private $mailHelper;
-
-	/**
-	 * @var IEventDispatcher
-	 */
-	private $eventDispatcher;
-
-	/**
-	 * @var ISecureRandom
-	 */
-	private $secureRandom;
-
-	/**
-	 * @param IUserManager $userManager
-	 * @param IGroupManager $groupManager
-	 * @param EmailValidator $emailValidator
-	 */
 	public function __construct(
-		IUserManager $userManager,
-		IGroupManager $groupManager,
-		EmailValidator $emailValidator,
-		IConfig $config,
-		NewUserMailHelper $mailHelper,
-		IEventDispatcher $eventDispatcher,
-		ISecureRandom $secureRandom
+		protected IUserManager $userManager,
+		protected IGroupManager $groupManager,
+		protected IMailer $mailer,
+		private IConfig $config,
+		private NewUserMailHelper $mailHelper,
+		private IEventDispatcher $eventDispatcher,
+		private ISecureRandom $secureRandom
 	) {
 		parent::__construct();
-		$this->userManager = $userManager;
-		$this->groupManager = $groupManager;
-		$this->emailValidator = $emailValidator;
-		$this->config = $config;
-		$this->mailHelper = $mailHelper;
-		$this->eventDispatcher = $eventDispatcher;
-		$this->secureRandom = $secureRandom;
 	}
 
 	protected function configure() {
@@ -142,23 +95,38 @@ protected function configure() {
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
 		$uid = $input->getArgument('uid');
-		$emailIsSet = \is_string($input->getOption('email')) && \mb_strlen($input->getOption('email')) > 0;
-		$emailIsValid = $this->emailValidator->isValid($input->getOption('email') ?? '', new RFCValidation());
-		$password = '';
-		$temporaryPassword = '';
-
 		if ($this->userManager->userExists($uid)) {
 			$output->writeln('<error>The user "' . $uid . '" already exists.</error>');
 			return 1;
 		}
 
+		$password = '';
+		$sendPasswordEmail = false;
+
 		if ($input->getOption('password-from-env')) {
 			$password = getenv('OC_PASS');
 
 			if (!$password) {
 				$output->writeln('<error>--password-from-env given, but OC_PASS is empty!</error>');
 				return 1;
 			}
+		} elseif (\mb_strlen($input->getOption('email')) > 0) {
+			if (!$this->mailer->validateMailAddress($input->getOption(('email')))) {
+				$output->writeln(\sprintf(
+					'<error>The given E-Mail address "%s" is invalid</error>',
+					$input->getOption('email'),
+				));
+
+				return 1;
+			}
+
+			$output->writeln('Setting a temporary password.');
+
+			$passwordEvent = new GenerateSecurePasswordEvent();
+			$this->eventDispatcher->dispatchTyped($passwordEvent);
+			$password = $passwordEvent->getPassword() ?? $this->secureRandom->generate(20);
+
+			$sendPasswordEmail = true;
 		} elseif ($input->isInteractive()) {
 			/** @var QuestionHelper $helper */
 			$helper = $this->getHelper('question');
@@ -180,26 +148,10 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 			return 1;
 		}
 
-		if (trim($password) === '' && $emailIsSet) {
-			if ($emailIsValid) {
-				$output->writeln('Setting a temporary password.');
-
-				$temporaryPassword = $this->getTemporaryPassword();
-			} else {
-				$output->writeln(\sprintf(
-					'<error>The given E-Mail address "%s" is invalid: %s</error>',
-					$input->getOption('email'),
-					$this->emailValidator->getError()->description()
-				));
-
-				return 1;
-			}
-		}
-
 		try {
 			$user = $this->userManager->createUser(
 				$input->getArgument('uid'),
-				$password ?: $temporaryPassword
+				$password,
 			);
 		} catch (\Exception $e) {
 			$output->writeln('<error>' . $e->getMessage() . '</error>');
@@ -215,24 +167,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 
 		if ($input->getOption('display-name')) {
 			$user->setDisplayName($input->getOption('display-name'));
-			$output->writeln(sprintf('Display name set to "%s"', $user->getDisplayName()));
-		}
-
-		if ($emailIsSet && $emailIsValid) {
-			$user->setSystemEMailAddress($input->getOption('email'));
-			$output->writeln(sprintf('E-Mail set to "%s"', (string) $user->getSystemEMailAddress()));
-
-			if (trim($password) === '' && $this->config->getAppValue('core', 'newUser.sendEmail', 'yes') === 'yes') {
-				try {
-					$this->mailHelper->sendMail(
-						$user,
-						$this->mailHelper->generateTemplate($user, true)
-					);
-					$output->writeln('Invitation E-Mail sent.');
-				} catch (\Exception $e) {
-					$output->writeln(\sprintf('Unable to send the invitation mail to %s', $user->getEMailAddress()));
-				}
-			}
+			$output->writeln(\sprintf('Display name set to "%s"', $user->getDisplayName()));
 		}
 
 		$groups = $input->getOption('group');
@@ -257,18 +192,20 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 				$output->writeln('User "' . $user->getUID() . '" added to group "' . $group->getGID() . '"');
 			}
 		}
-		return 0;
-	}
 
-	/**
-	 * @return string
-	 */
-	protected function getTemporaryPassword(): string
-	{
-		$passwordEvent = new GenerateSecurePasswordEvent();
+		// Send email to user if we set a temporary password
+		if ($sendPasswordEmail && $this->config->getAppValue('core', 'newUser.sendEmail', 'yes') === 'yes') {
+			$email = $input->getOption('email');
+			$user->setSystemEMailAddress($email);
 
-		$this->eventDispatcher->dispatchTyped($passwordEvent);
+			try {
+				$this->mailHelper->sendMail($user, $this->mailHelper->generateTemplate($user, true));
+				$output->writeln(\sprintf('Invitation E-Mail sent to %s', $email));
+			} catch (\Exception $e) {
+				$output->writeln(\sprintf('Unable to send the invitation mail to %s', $email));
+			}
+		}
 
-		return $passwordEvent->getPassword() ?? $this->secureRandom->generate(20);
+		return 0;
 	}
 }

tests/Core/Command/User/AddTest.php

@@ -25,7 +25,6 @@
 
 namespace Core\Command\User;
 
-use Egulias\EmailValidator\EmailValidator;
 use OC\Core\Command\User\Add;
 use OCA\Settings\Mailer\NewUserMailHelper;
 use OCP\EventDispatcher\IEventDispatcher;
@@ -34,19 +33,20 @@
 use OCP\IUser;
 use OCP\IUserManager;
 use OCP\Mail\IEMailTemplate;
+use OCP\mail\IMailer;
 use OCP\Security\ISecureRandom;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 use Test\TestCase;
 
 class AddTest extends TestCase {
-
 	/**
 	 * @dataProvider addEmailDataProvider
 	 */
 	public function testAddEmail(?string $email, bool $isValid, bool $shouldSendMail): void {
 		$userManager = static::createMock(IUserManager::class);
 		$groupManager = static::createStub(IGroupManager::class);
+		$mailer = static::createMock(IMailer::class);
 		$user = static::createMock(IUser::class);
 		$config = static::createMock(IConfig::class);
 		$mailHelper = static::createMock(NewUserMailHelper::class);
@@ -56,7 +56,7 @@ public function testAddEmail(?string $email, bool $isValid, bool $shouldSendMail
 		$consoleInput = static::createMock(InputInterface::class);
 		$consoleOutput = static::createMock(OutputInterface::class);
 
-		$user->expects($isValid ? static::once() : static::never())
+		$user->expects($isValid && $shouldSendMail ? static::once() : static::never())
 			->method('setSystemEMailAddress')
 			->with(static::equalTo($email));
 
@@ -66,6 +66,9 @@ public function testAddEmail(?string $email, bool $isValid, bool $shouldSendMail
 		$config->method('getAppValue')
 			->willReturn($shouldSendMail ? 'yes' : 'no');
 
+		$mailer->method('validateMailAddress')
+			->willReturn($isValid);
+
 		$mailHelper->method('generateTemplate')
 			->willReturn(static::createMock(IEMailTemplate::class));
 
@@ -82,7 +85,7 @@ public function testAddEmail(?string $email, bool $isValid, bool $shouldSendMail
 		$addCommand = new Add(
 			$userManager,
 			$groupManager,
-			new EmailValidator(),
+			$mailer,
 			$config,
 			$mailHelper,
 			$eventDispatcher,
@@ -96,31 +99,30 @@ public function testAddEmail(?string $email, bool $isValid, bool $shouldSendMail
 	}
 
 	/**
-	 * @return \Generator<string, array>
+	 * @return array
 	 */
-	public function addEmailDataProvider(): \Generator {
-		yield 'Valid E-Mail' => [
-			'[email protected]',
-			true,
-			true,
-		];
-
-		yield 'Invalid E-Mail' => [
-			'info@@example.com',
-			false,
-			true,
-		];
-
-		yield 'No E-Mail' => [
-			'',
-			false,
-			true,
-		];
-
-		yield 'Valid E-Mail, but no mail should be sent' => [
-			'[email protected]',
-			true,
-			false,
+	public function addEmailDataProvider(): array {
+		return [
+			'Valid E-Mail' => [
+				'[email protected]',
+				true,
+				true,
+			],
+			'Invalid E-Mail' => [
+				'info@@example.com',
+				false,
+				true,
+			],
+			'No E-Mail' => [
+				'',
+				false,
+				true,
+			],
+			'Valid E-Mail, but no mail should be sent' => [
+				'[email protected]',
+				true,
+				false,
+			],
 		];
 	}
 }