diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index d8cd1a4e13006..1b76d1bc61e46 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -37,6 +37,7 @@ use OCP\Files\IRootFolder; use OCP\Group\ISubAdmin; use OCP\HintException; +use OCP\IAppConfig; use OCP\IConfig; use OCP\IGroup; use OCP\IGroupManager; @@ -83,6 +84,7 @@ public function __construct( private IEventDispatcher $eventDispatcher, private IPhoneNumberUtil $phoneNumberUtil, private IAppManager $appManager, + private IAppConfig $appConfig, GroupDisplayNameCache $groupDisplayNameCache, ) { parent::__construct( @@ -596,7 +598,7 @@ public function addUser( // Send new user mail only if a mail is set if ($email !== '') { $newUser->setSystemEMailAddress($email); - if ($this->config->getAppValue('core', 'newUser.sendEmail', 'yes') === 'yes') { + if ($this->appConfig->getValueBool('core', 'newUser.sendEmail', true)) { try { $emailTemplate = $this->newUserMailHelper->generateTemplate($newUser, $generatePasswordResetToken); $this->newUserMailHelper->sendMail($newUser, $emailTemplate); diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 492b6666f4ba5..84e4f15b4b913 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -27,6 +27,7 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\IRootFolder; use OCP\Group\ISubAdmin; +use OCP\IAppConfig; use OCP\IConfig; use OCP\IGroup; use OCP\IL10N; @@ -67,6 +68,7 @@ class UsersControllerTest extends TestCase { private IRootFolder $rootFolder; private IPhoneNumberUtil $phoneNumberUtil; private IAppManager $appManager; + private IAppConfig&MockObject $appConfig; private GroupDisplayNameCache&MockObject $groupDisplayNameCache; protected function setUp(): void { @@ -89,6 +91,7 @@ protected function setUp(): void { $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->phoneNumberUtil = new PhoneNumberUtil(); $this->appManager = $this->createMock(IAppManager::class); + $this->appConfig = $this->createMock(IAppConfig::class); $this->rootFolder = $this->createMock(IRootFolder::class); $this->groupDisplayNameCache = $this->createMock(GroupDisplayNameCache::class); @@ -117,6 +120,7 @@ protected function setUp(): void { $this->eventDispatcher, $this->phoneNumberUtil, $this->appManager, + $this->appConfig, $this->groupDisplayNameCache, ]) ->onlyMethods(['fillStorageInfo']) @@ -270,6 +274,7 @@ public function testGetUsersDetailsReturnsEmptyGroupsList(): void { $this->eventDispatcher, $this->phoneNumberUtil, $this->appManager, + $this->appConfig, $this->groupDisplayNameCache, ]) ->onlyMethods(['getUserData']) @@ -587,6 +592,7 @@ public function testAddUserSuccessfulWithDisplayName(): void { $this->eventDispatcher, $this->phoneNumberUtil, $this->appManager, + $this->appConfig, $this->groupDisplayNameCache, ]) ->onlyMethods(['editUser']) @@ -730,6 +736,67 @@ public function testAddUserSuccessfulGeneratePassword(): void { )); } + /** + * `newUser.sendEmail` has to be read as a boolean. It is stored as an untyped + * 'yes'/'no' string on instances created before Nextcloud 33 and as a typed + * boolean once the account settings toggle has been used, so comparing it to + * the string 'yes' silently skipped the mail on upgraded instances. + */ + #[\PHPUnit\Framework\Attributes\DataProvider('dataAddUserWelcomeMail')] + public function testAddUserSendsWelcomeMailWhenEnabled(bool $enabled): void { + $this->appConfig + ->expects($this->atLeastOnce()) + ->method('getValueBool') + ->with('core', 'newUser.sendEmail', true) + ->willReturn($enabled); + + $newUser = $this->createMock(IUser::class); + $newUser->expects($this->once()) + ->method('setSystemEMailAddress') + ->with('foo@bar.com'); + $this->userManager + ->expects($this->once()) + ->method('userExists') + ->with('NewUser') + ->willReturn(false); + $this->userManager + ->expects($this->once()) + ->method('createUser') + ->willReturn($newUser); + $loggedInUser = $this->createMock(IUser::class); + $loggedInUser + ->method('getUID') + ->willReturn('adminUser'); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->willReturn($loggedInUser); + $this->groupManager + ->expects($this->once()) + ->method('isAdmin') + ->with('adminUser') + ->willReturn(true); + + $emailTemplate = $this->createMock(IEMailTemplate::class); + $this->newUserMailHelper + ->expects($enabled ? $this->once() : $this->never()) + ->method('generateTemplate') + ->willReturn($emailTemplate); + $this->newUserMailHelper + ->expects($enabled ? $this->once() : $this->never()) + ->method('sendMail') + ->with($newUser, $emailTemplate); + + $this->api->addUser('NewUser', 'PasswordOfTheNewUser', '', 'foo@bar.com'); + } + + public static function dataAddUserWelcomeMail(): array { + return [ + 'enabled' => [true], + 'disabled' => [false], + ]; + } + public function testAddUserSuccessfulLowercaseEmail(): void { $this->userManager ->expects($this->once()) @@ -3928,6 +3995,7 @@ public function testGetCurrentUserLoggedIn(): void { $this->eventDispatcher, $this->phoneNumberUtil, $this->appManager, + $this->appConfig, $this->groupDisplayNameCache, ]) ->onlyMethods(['getUserData']) @@ -4023,6 +4091,7 @@ public function testGetUser(): void { $this->eventDispatcher, $this->phoneNumberUtil, $this->appManager, + $this->appConfig, $this->groupDisplayNameCache, ]) ->onlyMethods(['getUserData']) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 530548f1896e6..4663e682ff9ad 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -2204,7 +2204,6 @@ - diff --git a/core/Command/User/Add.php b/core/Command/User/Add.php index 440bced4c2e37..4870c192dbf9b 100644 --- a/core/Command/User/Add.php +++ b/core/Command/User/Add.php @@ -180,7 +180,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $user->setSystemEMailAddress($email); - if ($this->appConfig->getValueString('core', 'newUser.sendEmail', 'yes') === 'yes') { + if ($this->appConfig->getValueBool('core', 'newUser.sendEmail', true)) { try { $this->mailHelper->sendMail($user, $this->mailHelper->generateTemplate($user, true)); $output->writeln('Welcome email sent to ' . $email); diff --git a/tests/Core/Command/User/AddTest.php b/tests/Core/Command/User/AddTest.php index 748a9b60ef632..9adf30ac49f9e 100644 --- a/tests/Core/Command/User/AddTest.php +++ b/tests/Core/Command/User/AddTest.php @@ -99,8 +99,8 @@ public function testAddEmail( $this->userManager->method('createUser') ->willReturn($this->user); - $this->appConfig->method('getValueString') - ->willReturn($shouldSendEmail ? 'yes' : 'no'); + $this->appConfig->method('getValueBool') + ->willReturn($shouldSendEmail); $this->mailHelper->method('generateTemplate') ->willReturn(static::createMock(IEMailTemplate::class)); diff --git a/tests/lib/AppConfigIntegrationTest.php b/tests/lib/AppConfigIntegrationTest.php index b0e400dd9e3fc..2bd0eea90be6f 100644 --- a/tests/lib/AppConfigIntegrationTest.php +++ b/tests/lib/AppConfigIntegrationTest.php @@ -519,6 +519,34 @@ public function testGetValueBool(): void { $this->assertSame(true, $config->getValueBool('typed', 'bool')); } + /** + * Untyped values predate the typed config API and are still in the database of + * every upgraded instance, so they have to keep resolving to a boolean. The + * deprecated setter is used on purpose, as it is the only way to write a value + * without a type. + */ + #[\PHPUnit\Framework\Attributes\DataProvider('dataUntypedBool')] + public function testGetValueBoolOnUntypedValue(string $stored, bool $expected): void { + /** @var AppConfig $config */ + $config = $this->generateAppConfig(); + $config->setValue('feed', 'untyped-bool', $stored); + + $this->assertSame($expected, $config->getValueBool('feed', 'untyped-bool')); + } + + public static function dataUntypedBool(): array { + return [ + 'yes' => ['yes', true], + 'no' => ['no', false], + 'true' => ['true', true], + 'false' => ['false', false], + 'on' => ['on', true], + '1' => ['1', true], + '0' => ['0', false], + 'empty' => ['', false], + ]; + } + public function testGetValueBoolOnUnknownAppReturnsDefault(): void { $config = $this->generateAppConfig(); $this->assertSame(false, $config->getValueBool('typed-1', 'bool', false));