fix(provisioning_api): read newUser.sendEmail as a boolean

Signed-off-by: Peter Ringelmann <peter.ringelmann@nextcloud.com>

Author: pringelmann

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);

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'])

build/psalm-baseline.xml

@@ -2204,7 +2204,6 @@
       <code><![CDATA[getAppValue]]></code>
       <code><![CDATA[getAppValue]]></code>
       <code><![CDATA[getAppValue]]></code>
-      <code><![CDATA[getAppValue]]></code>
       <code><![CDATA[getUserValue]]></code>
       <code><![CDATA[implementsActions]]></code>
       <code><![CDATA[implementsActions]]></code>

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);

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));

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));