From ce24923f4c3ab18c5082fc0d9a206177d9964e18 Mon Sep 17 00:00:00 2001 From: Anupam Kumar Date: Sat, 24 Feb 2024 04:51:48 +0530 Subject: [PATCH] add generate-password option and flow fixes Signed-off-by: Anupam Kumar --- core/Command/User/Add.php | 58 ++++++------- tests/Core/Command/User/AddTest.php | 122 +++++++++++++++++++--------- 2 files changed, 109 insertions(+), 71 deletions(-) diff --git a/core/Command/User/Add.php b/core/Command/User/Add.php index b456f636859..3411946fdeb 100644 --- a/core/Command/User/Add.php +++ b/core/Command/User/Add.php @@ -29,7 +29,7 @@ namespace OC\Core\Command\User; use OC\Files\Filesystem; use OCA\Settings\Mailer\NewUserMailHelper; use OCP\EventDispatcher\IEventDispatcher; -use OCP\IConfig; +use OCP\IAppConfig; use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; @@ -50,7 +50,7 @@ class Add extends Command { protected IUserManager $userManager, protected IGroupManager $groupManager, protected IMailer $mailer, - private IConfig $config, + private IAppConfig $appConfig, private NewUserMailHelper $mailHelper, private IEventDispatcher $eventDispatcher, private ISecureRandom $secureRandom, @@ -58,7 +58,7 @@ class Add extends Command { parent::__construct(); } - protected function configure() { + protected function configure(): void { $this ->setName('user:add') ->setDescription('adds an account') @@ -73,6 +73,12 @@ class Add extends Command { InputOption::VALUE_NONE, 'read password from environment variable OC_PASS' ) + ->addOption( + 'generate-password', + null, + InputOption::VALUE_NONE, + 'Generate a secure password. A welcome email with a reset link will be sent to the user via an email if --email option and newUser.sendEmail config are set' + ) ->addOption( 'display-name', null, @@ -89,7 +95,7 @@ class Add extends Command { 'email', null, InputOption::VALUE_REQUIRED, - 'When set, users may register using the default E-Mail verification workflow' + 'When set, users may register using the default email verification workflow' ); } @@ -101,19 +107,6 @@ class Add extends Command { } $password = ''; - $sendPasswordEmail = false; - - $email = $input->getOption('email'); - if (!empty($email)) { - if (!$this->mailer->validateMailAddress($email)) { - $output->writeln(\sprintf( - 'The given E-Mail address "%s" is invalid', - $email, - )); - - return 1; - } - } // Setup password. if ($input->getOption('password-from-env')) { @@ -123,13 +116,10 @@ class Add extends Command { $output->writeln('--password-from-env given, but OC_PASS is empty!'); return 1; } - } elseif (!empty($email)) { - + } elseif ($input->getOption('generate-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'); @@ -147,7 +137,7 @@ class Add extends Command { return 1; } } else { - $output->writeln("Interactive input or --password-from-env is needed for entering a password!"); + $output->writeln("Interactive input or --password-from-env or --generate-password is needed for setting a password!"); return 1; } @@ -173,10 +163,6 @@ class Add extends Command { $output->writeln('Display name set to "' . $user->getDisplayName() . '"'); } - if (!empty($email)) { - $user->setSystemEMailAddress($email); - } - $groups = $input->getOption('group'); if (!empty($groups)) { @@ -200,15 +186,25 @@ class Add extends Command { } } - // Send email to user if we set a temporary password - if ($sendPasswordEmail) { + $email = $input->getOption('email'); + if (!empty($email)) { + if (!$this->mailer->validateMailAddress($email)) { + $output->writeln(\sprintf( + 'The given email address "%s" is invalid. Email not set for the user.', + $email, + )); - if ($this->config->getAppValue('core', 'newUser.sendEmail', 'yes') === 'yes') { + return 1; + } + + $user->setSystemEMailAddress($email); + + if ($this->appConfig->getValueString('core', 'newUser.sendEmail', 'yes') === 'yes') { try { $this->mailHelper->sendMail($user, $this->mailHelper->generateTemplate($user, true)); - $output->writeln('Invitation E-Mail sent to ' . $email); + $output->writeln('Welcome email sent to ' . $email); } catch (\Exception $e) { - $output->writeln('Unable to send the invitation mail to ' . $email); + $output->writeln('Unable to send the welcome email to ' . $email); } } } diff --git a/tests/Core/Command/User/AddTest.php b/tests/Core/Command/User/AddTest.php index 3dffd713242..3ca6b635a94 100644 --- a/tests/Core/Command/User/AddTest.php +++ b/tests/Core/Command/User/AddTest.php @@ -28,7 +28,7 @@ namespace Core\Command\User; use OC\Core\Command\User\Add; use OCA\Settings\Mailer\NewUserMailHelper; use OCP\EventDispatcher\IEventDispatcher; -use OCP\IConfig; +use OCP\IAppConfig; use OCP\IGroupManager; use OCP\IUser; use OCP\IUserManager; @@ -40,61 +40,103 @@ use Symfony\Component\Console\Output\OutputInterface; use Test\TestCase; class AddTest extends TestCase { + /** @var IUserManager|\PHPUnit\Framework\MockObject\MockObject */ + private $userManager; + + /** @var IGroupManager|\PHPUnit\Framework\MockObject\MockObject */ + private $groupManager; + + /** @var IMailer|\PHPUnit\Framework\MockObject\MockObject */ + private $mailer; + + /** @var IAppConfig|\PHPUnit\Framework\MockObject\MockObject */ + private $appConfig; + + /** @var NewUserMailHelper|\PHPUnit\Framework\MockObject\MockObject */ + private $mailHelper; + + /** @var IEventDispatcher|\PHPUnit\Framework\MockObject\MockObject */ + private $eventDispatcher; + + /** @var ISecureRandom|\PHPUnit\Framework\MockObject\MockObject */ + private $secureRandom; + + /** @var IUser|\PHPUnit\Framework\MockObject\MockObject */ + private $user; + + /** @var InputInterface|\PHPUnit\Framework\MockObject\MockObject */ + private $consoleInput; + + /** @var OutputInterface|\PHPUnit\Framework\MockObject\MockObject */ + private $consoleOutput; + + /** @var Add */ + private $addCommand; + + public function setUp(): void { + parent::setUp(); + + $this->userManager = static::createMock(IUserManager::class); + $this->groupManager = static::createStub(IGroupManager::class); + $this->mailer = static::createMock(IMailer::class); + $this->appConfig = static::createMock(IAppConfig::class); + $this->mailHelper = static::createMock(NewUserMailHelper::class); + $this->eventDispatcher = static::createStub(IEventDispatcher::class); + $this->secureRandom = static::createStub(ISecureRandom::class); + + $this->user = static::createMock(IUser::class); + + $this->consoleInput = static::createMock(InputInterface::class); + $this->consoleOutput = static::createMock(OutputInterface::class); + + $this->addCommand = new Add( + $this->userManager, + $this->groupManager, + $this->mailer, + $this->appConfig, + $this->mailHelper, + $this->eventDispatcher, + $this->secureRandom + ); + } + /** * @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); - $eventDispatcher = static::createStub(IEventDispatcher::class); - $secureRandom = static::createStub(ISecureRandom::class); - - $consoleInput = static::createMock(InputInterface::class); - $consoleOutput = static::createMock(OutputInterface::class); - - $user->expects($isValid ? static::once() : static::never()) + public function testAddEmail( + ?string $email, + bool $isEmailValid, + bool $shouldSendEmail, + ): void { + $this->user->expects($isEmailValid ? static::once() : static::never()) ->method('setSystemEMailAddress') ->with(static::equalTo($email)); - $userManager->method('createUser') - ->willReturn($user); + $this->userManager->method('createUser') + ->willReturn($this->user); - $config->method('getAppValue') - ->willReturn($shouldSendMail ? 'yes' : 'no'); + $this->appConfig->method('getValueString') + ->willReturn($shouldSendEmail ? 'yes' : 'no'); - $mailer->method('validateMailAddress') - ->willReturn($isValid); + $this->mailer->method('validateMailAddress') + ->willReturn($isEmailValid); - $mailHelper->method('generateTemplate') + $this->mailHelper->method('generateTemplate') ->willReturn(static::createMock(IEMailTemplate::class)); - $mailHelper->expects($isValid && $shouldSendMail ? static::once() : static::never()) + $this->mailHelper->expects($isEmailValid && $shouldSendEmail ? static::once() : static::never()) ->method('sendMail'); - $consoleInput->method('getOption') + $this->consoleInput->method('getOption') ->will(static::returnValueMap([ - ['password-from-env', ''], + ['generate-password', 'true'], ['email', $email], ['group', []], ])); - $addCommand = new Add( - $userManager, - $groupManager, - $mailer, - $config, - $mailHelper, - $eventDispatcher, - $secureRandom - ); - - $this->invokePrivate($addCommand, 'execute', [ - $consoleInput, - $consoleOutput + $this->invokePrivate($this->addCommand, 'execute', [ + $this->consoleInput, + $this->consoleOutput ]); } @@ -111,12 +153,12 @@ class AddTest extends TestCase { 'Invalid E-Mail' => [ 'info@@example.com', false, - true, + false, ], 'No E-Mail' => [ '', false, - true, + false, ], 'Valid E-Mail, but no mail should be sent' => [ 'info@example.com',