Add check before sending email that email address is valid

Signed-off-by: Carl Schwan <carl@carlschwan.eu>

diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php
index 62ba9d35f3f03cf7bcceff80fd052341b5c12ea0..65a7498bcd67414da189a8717ddf45b2141941d6 100644 (file)
--- 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($e->getMessage(), [
+                               'message' => 'Failed to send share by mail. Got an invalid email address ' . $share->getSharedWith(),
+                               'app' => 'sharebymail',
+                       ]);
+               }
+
                try {
                        $link = $this->urlGenerator->linkToRouteAbsolute('files_sharing.sharecontroller.showShare',
                                ['token' => $share->getToken()]);

diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php
index bbe5516408dcdb7ace3961b7aaeb57812782423f..45cab303669b286b1fc6006aa70e564456c08b85 100644 (file)
--- 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');
 

diff --git a/lib/private/Collaboration/Collaborators/MailPlugin.php b/lib/private/Collaboration/Collaborators/MailPlugin.php
index 53a223b38ddb15d2524136634f5e547e900de2d2..aae6f305981d38b173672c7f280afebc720f574b 100644 (file)
--- 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,14 +252,10 @@ class MailPlugin implements ISearchPlugin {
                        $userResults['wide'] = array_slice($userResults['wide'], $offset, $limit);
                }
 
-               [$username, $domain] = explode('@', $search);
-               $domain = idn_to_ascii($domain);
-               $searchIdn = $username . '@' . $domain;
-
-               if (!$searchResult->hasExactIdMatch($emailType) && filter_var($searchIdn, FILTER_VALIDATE_EMAIL)) {
+               if (!$searchResult->hasExactIdMatch($emailType) && $this->mailer->validateMailAddress($search)) {
                        $result['exact'][] = [
                                'label' => $search,
-                               'uuid' => $searchIdn,
+                               'uuid' => $search,
                                'value' => [
                                        'shareType' => IShare::TYPE_EMAIL,
                                        'shareWith' => $search,

diff --git a/tests/lib/Collaboration/Collaborators/MailPluginTest.php b/tests/lib/Collaboration/Collaborators/MailPluginTest.php
index 2b71820af52e50967c8b5392e69ed7870ea84a2b..702c1d6be6e08f1d637b21e29149875b0abf7ac0 100644 (file)
--- 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,
                        ]
                ];
        }