Merge pull request #62776 from nextcloud/fix/59329-welcome-email-config-type

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

Author: susnux

apps/provisioning_api/lib/Controller/UsersController.php

@@ -597,7 +597,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

@@ -734,6 +734,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())

build/psalm-baseline.xml

@@ -2272,7 +2272,6 @@
       <code><![CDATA[generate]]></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

@@ -183,7 +183,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

@@ -100,8 +100,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

@@ -494,6 +494,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));