Merge pull request #30600 from nextcloud/fix/30595/idn-email-share

Fix idn emails not working in shares

4 files changed, 76 insertions, 16 deletions

diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php
index 62ba9d35f3f..c1438c01125 100644
--- a/apps/sharebymail/lib/ShareByMailProvider.php
+++ b/apps/sharebymail/lib/ShareByMailProvider.php
@@ -334,6 +334,16 @@ class ShareByMailProvider implements IShareProvider {
 			$share->getNote()
 		);
 
+		if (!$this->mailer->validateMailAddress($share->getSharedWith())) {
+			$this->removeShareFromTable($shareId);
+			$e = new HintException('Failed to send share by mail. Got an invalid email address: ' . $share->getSharedWith(),
+				$this->l->t('Failed to send share by email. Got an invalid email address'));
+			$this->logger->error('Failed to send share by mail. Got an invalid email address ' . $share->getSharedWith(), [
+				'app' => 'sharebymail',
+				'exception' => $e,
+			]);
+		}
+
 		try {
 			$link = $this->urlGenerator->linkToRouteAbsolute('files_sharing.sharecontroller.showShare',
 				['token' => $share->getToken()]);
@@ -679,7 +689,7 @@ class ShareByMailProvider implements IShareProvider {
 	 * @param \DateTime|null $expirationTime
 	 * @return int
 	 */
-	protected function addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $uidOwner, $permissions, $token, $password, $sendPasswordByTalk, $hideDownload, $label, $expirationTime, $note = '') {
+	protected function addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $uidOwner, $permissions, $token, $password, $sendPasswordByTalk, $hideDownload, $label, $expirationTime, $note = ''): int {
 		$qb = $this->dbConnection->getQueryBuilder();
 		$qb->insert('share')
 			->setValue('share_type', $qb->createNamedParameter(IShare::TYPE_EMAIL))
@@ -774,7 +784,7 @@ class ShareByMailProvider implements IShareProvider {
 		} catch (\Exception $e) {
 		}
 
-		$this->removeShareFromTable($share->getId());
+		$this->removeShareFromTable((int)$share->getId());
 	}
 
 	/**
@@ -970,9 +980,9 @@ class ShareByMailProvider implements IShareProvider {
 	/**
 	 * remove share from table
 	 *
-	 * @param string $shareId
+	 * @param int $shareId
 	 */
-	protected function removeShareFromTable($shareId) {
+	protected function removeShareFromTable(int $shareId): void {
 		$qb = $this->dbConnection->getQueryBuilder();
 		$qb->delete('share')
 			->where($qb->expr()->eq('id', $qb->createNamedParameter($shareId)));
diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php
index bbe5516408d..af2d5bad57c 100644
--- a/apps/sharebymail/tests/ShareByMailProviderTest.php
+++ b/apps/sharebymail/tests/ShareByMailProviderTest.php
@@ -217,7 +217,7 @@ class ShareByMailProviderTest extends TestCase {
 
 	public function testCreateSendPasswordByMailWithoutEnforcedPasswordProtection() {
 		$share = $this->getMockBuilder(IShare::class)->getMock();
-		$share->expects($this->any())->method('getSharedWith')->willReturn('receiver@example.com');
+		$share->expects($this->any())->method('getSharedWith')->willReturn('receiver@examplelölöl.com');
 		$share->expects($this->any())->method('getSendPasswordByTalk')->willReturn(false);
 		$share->expects($this->any())->method('getSharedBy')->willReturn('owner');
 
@@ -459,6 +459,7 @@ class ShareByMailProviderTest extends TestCase {
 	public function testCreateMailShare() {
 		$this->share->expects($this->any())->method('getToken')->willReturn('token');
 		$this->share->expects($this->once())->method('setToken')->with('token');
+		$this->share->expects($this->any())->method('getSharedWith')->willReturn('valid@valid.com');
 		$node = $this->getMockBuilder('OCP\Files\Node')->getMock();
 		$node->expects($this->any())->method('getName')->willReturn('fileName');
 		$this->share->expects($this->any())->method('getNode')->willReturn($node);
@@ -483,6 +484,7 @@ class ShareByMailProviderTest extends TestCase {
 
 		$this->share->expects($this->any())->method('getToken')->willReturn('token');
 		$this->share->expects($this->once())->method('setToken')->with('token');
+		$this->share->expects($this->any())->method('getSharedWith')->willReturn('valid@valid.com');
 		$node = $this->getMockBuilder('OCP\Files\Node')->getMock();
 		$node->expects($this->any())->method('getName')->willReturn('fileName');
 		$this->share->expects($this->any())->method('getNode')->willReturn($node);
@@ -987,6 +989,7 @@ class ShareByMailProviderTest extends TestCase {
 			->willReturn(new \OC\Share20\Share($rootFolder, $userManager));
 
 		$provider = $this->getInstance(['sendMailNotification', 'createShareActivity']);
+		$this->mailer->expects($this->any())->method('validateMailAddress')->willReturn(true);
 
 		$u1 = $userManager->createUser('testFed', md5(time()));
 		$u2 = $userManager->createUser('testFed2', md5(time()));
@@ -1033,6 +1036,7 @@ class ShareByMailProviderTest extends TestCase {
 			->willReturn(new \OC\Share20\Share($rootFolder, $userManager));
 
 		$provider = $this->getInstance(['sendMailNotification', 'createShareActivity']);
+		$this->mailer->expects($this->any())->method('validateMailAddress')->willReturn(true);
 
 		$u1 = $userManager->createUser('testFed', md5(time()));
 		$u2 = $userManager->createUser('testFed2', md5(time()));
diff --git a/lib/private/Collaboration/Collaborators/MailPlugin.php b/lib/private/Collaboration/Collaborators/MailPlugin.php
index 9e35aa828da..aae6f305981 100644
--- a/lib/private/Collaboration/Collaborators/MailPlugin.php
+++ b/lib/private/Collaboration/Collaborators/MailPlugin.php
@@ -38,6 +38,7 @@ use OCP\IGroupManager;
 use OCP\IUser;
 use OCP\IUserSession;
 use OCP\Share\IShare;
+use OCP\Mail\IMailer;
 
 class MailPlugin implements ISearchPlugin {
 	/* @var bool */
@@ -64,19 +65,23 @@ class MailPlugin implements ISearchPlugin {
 	private $knownUserService;
 	/** @var IUserSession */
 	private $userSession;
+	/** @var IMailer */
+	private $mailer;
 
 	public function __construct(IManager $contactsManager,
 								ICloudIdManager $cloudIdManager,
 								IConfig $config,
 								IGroupManager $groupManager,
 								KnownUserService $knownUserService,
-								IUserSession $userSession) {
+								IUserSession $userSession,
+								IMailer $mailer) {
 		$this->contactsManager = $contactsManager;
 		$this->cloudIdManager = $cloudIdManager;
 		$this->config = $config;
 		$this->groupManager = $groupManager;
 		$this->knownUserService = $knownUserService;
 		$this->userSession = $userSession;
+		$this->mailer = $mailer;
 
 		$this->shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
 		$this->shareWithGroupOnly = $this->config->getAppValue('core', 'shareapi_only_share_with_group_members', 'no') === 'yes';
@@ -247,8 +252,7 @@ class MailPlugin implements ISearchPlugin {
 			$userResults['wide'] = array_slice($userResults['wide'], $offset, $limit);
 		}
 
-
-		if (!$searchResult->hasExactIdMatch($emailType) && filter_var($search, FILTER_VALIDATE_EMAIL)) {
+		if (!$searchResult->hasExactIdMatch($emailType) && $this->mailer->validateMailAddress($search)) {
 			$result['exact'][] = [
 				'label' => $search,
 				'uuid' => $search,
diff --git a/tests/lib/Collaboration/Collaborators/MailPluginTest.php b/tests/lib/Collaboration/Collaborators/MailPluginTest.php
index 2b71820af52..702c1d6be6e 100644
--- a/tests/lib/Collaboration/Collaborators/MailPluginTest.php
+++ b/tests/lib/Collaboration/Collaborators/MailPluginTest.php
@@ -37,6 +37,7 @@ use OCP\IUser;
 use OCP\IUserManager;
 use OCP\IUserSession;
 use OCP\Share\IShare;
+use OCP\Mail\IMailer;
 use Test\TestCase;
 
 class MailPluginTest extends TestCase {
@@ -64,6 +65,9 @@ class MailPluginTest extends TestCase {
 	/** @var  IUserSession|\PHPUnit\Framework\MockObject\MockObject */
 	protected $userSession;
 
+	/** @var IMailer|\PHPUnit\Framework\MockObject\MockObject */
+	protected $mailer;
+
 	protected function setUp(): void {
 		parent::setUp();
 
@@ -72,6 +76,7 @@ class MailPluginTest extends TestCase {
 		$this->groupManager = $this->createMock(IGroupManager::class);
 		$this->knownUserService = $this->createMock(KnownUserService::class);
 		$this->userSession = $this->createMock(IUserSession::class);
+		$this->mailer = $this->createMock(IMailer::class);
 		$this->cloudIdManager = new CloudIdManager($this->contactsManager, $this->createMock(IURLGenerator::class), $this->createMock(IUserManager::class));
 
 		$this->searchResult = new SearchResult();
@@ -84,7 +89,8 @@ class MailPluginTest extends TestCase {
 			$this->config,
 			$this->groupManager,
 			$this->knownUserService,
-			$this->userSession
+			$this->userSession,
+			$this->mailer
 		);
 	}
 
@@ -97,7 +103,7 @@ class MailPluginTest extends TestCase {
 	 * @param array $expected
 	 * @param bool $reachedEnd
 	 */
-	public function testSearch($searchTerm, $contacts, $shareeEnumeration, $expected, $exactIdMatch, $reachedEnd) {
+	public function testSearch($searchTerm, $contacts, $shareeEnumeration, $expected, $exactIdMatch, $reachedEnd, $validEmail) {
 		$this->config->expects($this->any())
 			->method('getAppValue')
 			->willReturnCallback(
@@ -117,6 +123,9 @@ class MailPluginTest extends TestCase {
 		$this->userSession->method('getUser')
 			->willReturn($currentUser);
 
+		$this->mailer->method('validateMailAddress')
+			->willReturn($validEmail);
+
 		$this->contactsManager->expects($this->any())
 			->method('search')
 			->willReturnCallback(function ($search, $searchAttributes) use ($searchTerm, $contacts) {
@@ -137,9 +146,9 @@ class MailPluginTest extends TestCase {
 	public function dataGetEmail() {
 		return [
 			// data set 0
-			['test', [], true, ['emails' => [], 'exact' => ['emails' => []]], false, false],
+			['test', [], true, ['emails' => [], 'exact' => ['emails' => []]], false, false, false],
 			// data set 1
-			['test', [], false, ['emails' => [], 'exact' => ['emails' => []]], false, false],
+			['test', [], false, ['emails' => [], 'exact' => ['emails' => []]], false, false, false],
 			// data set 2
 			[
 				'test@remote.com',
@@ -148,6 +157,7 @@ class MailPluginTest extends TestCase {
 				['emails' => [], 'exact' => ['emails' => [['uuid' => 'test@remote.com', 'label' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
 				false,
 				false,
+				true,
 			],
 			// data set 3
 			[ // no valid email address
@@ -157,6 +167,7 @@ class MailPluginTest extends TestCase {
 				['emails' => [], 'exact' => ['emails' => []]],
 				false,
 				false,
+				false,
 			],
 			// data set 4
 			[
@@ -166,6 +177,7 @@ class MailPluginTest extends TestCase {
 				['emails' => [], 'exact' => ['emails' => [['uuid' => 'test@remote.com', 'label' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
 				false,
 				false,
+				true,
 			],
 			// data set 5
 			[
@@ -193,6 +205,7 @@ class MailPluginTest extends TestCase {
 				['emails' => [['uuid' => 'uid1', 'name' => 'User @ Localhost', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]], 'exact' => ['emails' => []]],
 				false,
 				false,
+				false,
 			],
 			// data set 6
 			[
@@ -221,6 +234,7 @@ class MailPluginTest extends TestCase {
 				['emails' => [], 'exact' => ['emails' => []]],
 				false,
 				false,
+				false,
 			],
 			// data set 7
 			[
@@ -248,6 +262,7 @@ class MailPluginTest extends TestCase {
 				['emails' => [['uuid' => 'uid1', 'name' => 'User @ Localhost', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]], 'exact' => ['emails' => [['label' => 'test@remote.com', 'uuid' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
 				false,
 				false,
+				true,
 			],
 			// data set 8
 			[
@@ -276,6 +291,7 @@ class MailPluginTest extends TestCase {
 				['emails' => [], 'exact' => ['emails' => [['label' => 'test@remote.com', 'uuid' => 'test@remote.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@remote.com']]]]],
 				false,
 				false,
+				true,
 			],
 			// data set 9
 			[
@@ -303,6 +319,7 @@ class MailPluginTest extends TestCase {
 				['emails' => [], 'exact' => ['emails' => [['name' => 'User @ Localhost', 'uuid' => 'uid1', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]]]],
 				true,
 				false,
+				false,
 			],
 			// data set 10
 			[
@@ -330,6 +347,7 @@ class MailPluginTest extends TestCase {
 				['emails' => [], 'exact' => ['emails' => [['name' => 'User @ Localhost', 'uuid' => 'uid1', 'type' => '', 'label' => 'User @ Localhost (username@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'username@localhost']]]]],
 				true,
 				false,
+				false,
 			],
 			// data set 11
 			// contact with space
@@ -358,6 +376,7 @@ class MailPluginTest extends TestCase {
 				['emails' => [], 'exact' => ['emails' => [['name' => 'User Name @ Localhost', 'uuid' => 'uid1', 'type' => '', 'label' => 'User Name @ Localhost (user name@localhost)', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'user name@localhost']]]]],
 				true,
 				false,
+				false,
 			],
 			// data set 12
 			// remote with space, no contact
@@ -387,6 +406,7 @@ class MailPluginTest extends TestCase {
 				['emails' => [], 'exact' => ['emails' => []]],
 				false,
 				false,
+				false,
 			],
 			// data set 13
 			// Local user found by email
@@ -405,6 +425,7 @@ class MailPluginTest extends TestCase {
 				['users' => [], 'exact' => ['users' => [['uuid' => 'uid1', 'name' => 'User', 'label' => 'User (test@example.com)','value' => ['shareType' => IShare::TYPE_USER, 'shareWith' => 'test'], 'shareWithDisplayNameUnique' => 'test@example.com']]]],
 				true,
 				false,
+				true,
 			],
 			// data set 14
 			// Current local user found by email => no result
@@ -423,6 +444,7 @@ class MailPluginTest extends TestCase {
 				['exact' => []],
 				false,
 				false,
+				true,
 			],
 			// data set 15
 			// Pagination and "more results" for user matches byyyyyyy emails
@@ -465,6 +487,7 @@ class MailPluginTest extends TestCase {
 				], 'emails' => [], 'exact' => ['users' => [], 'emails' => []]],
 				false,
 				true,
+				false,
 			],
 			// data set 16
 			// Pagination and "more results" for normal emails
@@ -503,6 +526,7 @@ class MailPluginTest extends TestCase {
 				], 'exact' => ['emails' => []]],
 				false,
 				true,
+				false,
 			],
 			// data set 17
 			// multiple email addresses with type
@@ -536,6 +560,18 @@ class MailPluginTest extends TestCase {
 				]]],
 				false,
 				false,
+				false,
+			],
+			// data set 18
+			// idn email
+			[
+				'test@lölölölölölölöl.com',
+				[],
+				true,
+				['emails' => [], 'exact' => ['emails' => [['uuid' => 'test@lölölölölölölöl.com', 'label' => 'test@lölölölölölölöl.com', 'value' => ['shareType' => IShare::TYPE_EMAIL, 'shareWith' => 'test@lölölölölölölöl.com']]]]],
+				false,
+				false,
+				true,
 			],
 		];
 	}
@@ -550,7 +586,7 @@ class MailPluginTest extends TestCase {
 	 * @param bool $reachedEnd
 	 * @param array groups
 	 */
-	public function testSearchGroupsOnly($searchTerm, $contacts, $expected, $exactIdMatch, $reachedEnd, $userToGroupMapping) {
+	public function testSearchGroupsOnly($searchTerm, $contacts, $expected, $exactIdMatch, $reachedEnd, $userToGroupMapping, $validEmail) {
 		$this->config->expects($this->any())
 			->method('getAppValue')
 			->willReturnCallback(
@@ -573,6 +609,9 @@ class MailPluginTest extends TestCase {
 			->method('getUID')
 			->willReturn('currentUser');
 
+		$this->mailer->method('validateMailAddress')
+			->willReturn($validEmail);
+
 		$this->contactsManager->expects($this->any())
 			->method('search')
 			->willReturnCallback(function ($search, $searchAttributes) use ($searchTerm, $contacts) {
@@ -626,7 +665,8 @@ class MailPluginTest extends TestCase {
 				[
 					"currentUser" => ["group1"],
 					"User" => ["group1"]
-				]
+				],
+				false,
 			],
 			// The user `User` cannot share with the current user
 			[
@@ -646,7 +686,8 @@ class MailPluginTest extends TestCase {
 				[
 					"currentUser" => ["group1"],
 					"User" => ["group2"]
-				]
+				],
+				false,
 			],
 			// The user `User` cannot share with the current user, but there is an exact match on the e-mail address -> share by e-mail
 			[
@@ -666,7 +707,8 @@ class MailPluginTest extends TestCase {
 				[
 					"currentUser" => ["group1"],
 					"User" => ["group2"]
-				]
+				],
+				true,
 			]
 		];
 	}