Add "Reply-To" on ShareByMailProvider mails

Fixes https://github.com/nextcloud/server/issues/4209

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php
index 83170c5648cfceda19ade2629a529e89c5064307..473613e542e0828f787466aae2594360e09823f8 100644 (file)
--- a/apps/sharebymail/lib/ShareByMailProvider.php
+++ b/apps/sharebymail/lib/ShareByMailProvider.php
@@ -26,6 +26,7 @@ use OC\Share20\Exception\InvalidShare;
 use OCA\ShareByMail\Settings\SettingsManager;
 use OCP\Activity\IManager;
 use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\Defaults;
 use OCP\Files\Folder;
 use OCP\Files\IRootFolder;
 use OCP\Files\Node;
@@ -80,6 +81,9 @@ class ShareByMailProvider implements IShareProvider {
        /** @var SettingsManager */
        private $settingsManager;
 
+       /** @var Defaults */
+       private $defaults;
+
        /**
         * Return the identifier of this provider.
         *
@@ -102,6 +106,7 @@ class ShareByMailProvider implements IShareProvider {
         * @param IURLGenerator $urlGenerator
         * @param IManager $activityManager
         * @param SettingsManager $settingsManager
+        * @param Defaults $defaults
         */
        public function __construct(
                IDBConnection $connection,
@@ -113,7 +118,8 @@ class ShareByMailProvider implements IShareProvider {
                IMailer $mailer,
                IURLGenerator $urlGenerator,
                IManager $activityManager,
-               SettingsManager $settingsManager
+               SettingsManager $settingsManager,
+               Defaults $defaults
        ) {
                $this->dbConnection = $connection;
                $this->secureRandom = $secureRandom;
@@ -125,6 +131,7 @@ class ShareByMailProvider implements IShareProvider {
                $this->urlGenerator = $urlGenerator;
                $this->activityManager = $activityManager;
                $this->settingsManager = $settingsManager;
+               $this->defaults = $defaults;
        }
 
        /**
@@ -229,10 +236,13 @@ class ShareByMailProvider implements IShareProvider {
                try {
                        $link = $this->urlGenerator->linkToRouteAbsolute('files_sharing.sharecontroller.showShare',
                                ['token' => $share->getToken()]);
-                       $this->sendMailNotification($share->getNode()->getName(),
+                       $this->sendMailNotification(
+                               $share->getNode()->getName(),
                                $link,
                                $share->getShareOwner(),
-                               $share->getSharedBy(), $share->getSharedWith());
+                               $share->getSharedBy(),
+                                       $share->getSharedWith()
+                       );
                } catch (HintException $hintException) {
                        $this->logger->error('Failed to send share by mail: ' . $hintException->getMessage());
                        $this->removeShareFromTable($shareId);
@@ -248,7 +258,19 @@ class ShareByMailProvider implements IShareProvider {
 
        }
 
-       protected function sendMailNotification($filename, $link, $owner, $initiator, $shareWith) {
+       /**
+        * @param string $filename
+        * @param string $link
+        * @param string $owner
+        * @param string $initiator
+        * @param string $shareWith
+        * @throws \Exception If mail couldn't be sent
+        */
+       protected function sendMailNotification($filename,
+                                                                                       $link,
+                                                                                       $owner,
+                                                                                       $initiator,
+                                                                                       $shareWith) {
                $ownerUser = $this->userManager->get($owner);
                $initiatorUser = $this->userManager->get($initiator);
                $ownerDisplayName = ($ownerUser instanceof IUser) ? $ownerUser->getDisplayName() : $owner;
@@ -281,14 +303,34 @@ class ShareByMailProvider implements IShareProvider {
                        $this->l->t('Open »%s«', [$filename]),
                        $link
                );
-               $emailTemplate->addFooter();
 
                $message->setTo([$shareWith]);
+
+               // The "From" contains the sharers name
+               $instanceName = $this->defaults->getName();
+               $senderName = $this->l->t(
+                       '%s via %s',
+                       [
+                               $ownerDisplayName,
+                               $instanceName
+                       ]
+               );
+               $message->setFrom([\OCP\Util::getDefaultEmailAddress($instanceName) => $senderName]);
+
+               // The "Reply-To" is set to the sharer if an mail address is configured
+               // also the default footer contains a "Do not reply" which needs to be adjusted.
+               $ownerEmail = $ownerUser->getEMailAddress();
+               if($ownerEmail !== null) {
+                       $message->setReplyTo([$ownerEmail => $ownerDisplayName]);
+                       $emailTemplate->addFooter($instanceName . ' - ' . $this->defaults->getSlogan());
+               } else {
+                       $emailTemplate->addFooter();
+               }
+
                $message->setSubject($subject);
-               $message->setBody($emailTemplate->renderText(), 'text/plain');
+               $message->setPlainBody($emailTemplate->renderText());
                $message->setHtmlBody($emailTemplate->renderHTML());
                $this->mailer->send($message);
-
        }
 
        /**

diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php
index 13fb5d03bd8822c196c516ffaa710e8b9ca2b89e..8761f49f0f44443e8844525f790527b8e12ee8ab 100644 (file)
--- a/apps/sharebymail/tests/ShareByMailProviderTest.php
+++ b/apps/sharebymail/tests/ShareByMailProviderTest.php
@@ -23,14 +23,18 @@
 namespace OCA\ShareByMail\Tests;
 
 
+use OC\Mail\Message;
 use OCA\ShareByMail\Settings\SettingsManager;
 use OCA\ShareByMail\ShareByMailProvider;
+use OCP\Defaults;
 use OCP\Files\IRootFolder;
 use OCP\IDBConnection;
 use OCP\IL10N;
 use OCP\ILogger;
 use OCP\IURLGenerator;
+use OCP\IUser;
 use OCP\IUserManager;
+use OCP\Mail\IEMailTemplate;
 use OCP\Mail\IMailer;
 use OCP\Security\ISecureRandom;
 use OCP\Share\IManager;
@@ -81,6 +85,9 @@ class ShareByMailProviderTest extends TestCase {
        /** @var  SettingsManager | \PHPUnit_Framework_MockObject_MockObject */
        private $settingsManager;
 
+       /** @var Defaults|\PHPUnit_Framework_MockObject_MockObject */
+       private $defaults;
+
        public function setUp() {
                parent::setUp();
 
@@ -101,6 +108,7 @@ class ShareByMailProviderTest extends TestCase {
                $this->share = $this->getMockBuilder('\OCP\Share\IShare')->getMock();
                $this->activityManager = $this->getMockBuilder('OCP\Activity\IManager')->getMock();
                $this->settingsManager = $this->getMockBuilder(SettingsManager::class)->disableOriginalConstructor()->getMock();
+               $this->defaults = $this->createMock(Defaults::class);
 
                $this->userManager->expects($this->any())->method('userExists')->willReturn(true);
        }
@@ -125,7 +133,8 @@ class ShareByMailProviderTest extends TestCase {
                                        $this->mailer,
                                        $this->urlGenerator,
                                        $this->activityManager,
-                                       $this->settingsManager
+                                       $this->settingsManager,
+                                       $this->defaults
                                ]
                        );
 
@@ -144,7 +153,8 @@ class ShareByMailProviderTest extends TestCase {
                        $this->mailer,
                        $this->urlGenerator,
                        $this->activityManager,
-                       $this->settingsManager
+                       $this->settingsManager,
+                       $this->defaults
                );
 
        }
@@ -726,4 +736,229 @@ class ShareByMailProviderTest extends TestCase {
                $u1->delete();
                $u2->delete();
        }
+
+       public function testSendMailNotificationWithSameUserAndUserEmail() {
+               $provider = $this->getInstance();
+               $user = $this->createMock(IUser::class);
+               $this->userManager
+                       ->expects($this->exactly(2))
+                       ->method('get')
+                       ->with('OwnerUser')
+                       ->willReturn($user);
+               $user
+                       ->expects($this->exactly(2))
+                       ->method('getDisplayName')
+                       ->willReturn('Mrs. Owner User');
+               $message = $this->createMock(Message::class);
+               $this->mailer
+                       ->expects($this->once())
+                       ->method('createMessage')
+                       ->willReturn($message);
+               $template = $this->createMock(IEMailTemplate::class);
+               $this->mailer
+                       ->expects($this->once())
+                       ->method('createEMailTemplate')
+                       ->willReturn($template);
+               $template
+                       ->expects($this->once())
+                       ->method('addHeader');
+               $template
+                       ->expects($this->once())
+                       ->method('addHeading')
+                       ->with('Mrs. Owner User shared »file.txt« with you');
+               $template
+                       ->expects($this->once())
+                       ->method('addBodyText')
+                       ->with(
+                               'Mrs. Owner User shared »file.txt« with you. Click the button below to open it.',
+                               'Mrs. Owner User shared »file.txt« with you.'
+                       );
+               $template
+                       ->expects($this->once())
+                       ->method('addBodyButton')
+                       ->with(
+                               'Open »file.txt«',
+                               'https://example.com/file.txt'
+                       );
+               $message
+                       ->expects($this->once())
+                       ->method('setTo')
+                       ->with(['john@doe.com']);
+               $this->defaults
+                       ->expects($this->once())
+                       ->method('getName')
+                       ->willReturn('UnitTestCloud');
+               $message
+                       ->expects($this->once())
+                       ->method('setFrom')
+                       ->with([
+                               \OCP\Util::getDefaultEmailAddress('UnitTestCloud') => 'Mrs. Owner User via UnitTestCloud'
+                       ]);
+               $user
+                       ->expects($this->once())
+                       ->method('getEMailAddress')
+                       ->willReturn('owner@example.com');
+               $message
+                       ->expects($this->once())
+                       ->method('setReplyTo')
+                       ->with(['owner@example.com' => 'Mrs. Owner User']);
+               $this->defaults
+                       ->expects($this->once())
+                       ->method('getSlogan')
+                       ->willReturn('Testing like 1990');
+               $template
+                       ->expects($this->once())
+                       ->method('addFooter')
+                       ->with('UnitTestCloud - Testing like 1990');
+               $message
+                       ->expects($this->once())
+                       ->method('setSubject')
+                       ->willReturn('Mrs. Owner User shared »file.txt« with you');
+               $template
+                       ->expects($this->once())
+                       ->method('renderText')
+                       ->willReturn('Text Render');
+               $message
+                       ->expects($this->once())
+                       ->method('setPlainBody')
+                       ->with('Text Render');
+               $template
+                       ->expects($this->once())
+                       ->method('renderHTML')
+                       ->willReturn('HTML Render');
+               $message
+                       ->expects($this->once())
+                       ->method('setHtmlBody')
+                       ->with('HTML Render');
+               $this->mailer
+                       ->expects($this->once())
+                       ->method('send')
+                       ->with($message);
+
+               self::invokePrivate(
+                       $provider,
+                       'sendMailNotification',
+                       [
+                               'file.txt',
+                               'https://example.com/file.txt',
+                               'OwnerUser',
+                               'OwnerUser',
+                               'john@doe.com',
+                       ]);
+       }
+
+       public function testSendMailNotificationWithDifferentUserAndNoUserEmail() {
+               $provider = $this->getInstance();
+               $ownerUser = $this->createMock(IUser::class);
+               $initiatorUser = $this->createMock(IUser::class);
+               $this->userManager
+                       ->expects($this->at(0))
+                       ->method('get')
+                       ->with('OwnerUser')
+                       ->willReturn($ownerUser);
+               $this->userManager
+                       ->expects($this->at(1))
+                       ->method('get')
+                       ->with('InitiatorUser')
+                       ->willReturn($initiatorUser);
+               $ownerUser
+                       ->expects($this->once())
+                       ->method('getDisplayName')
+                       ->willReturn('Mrs. Owner User');
+               $initiatorUser
+                       ->expects($this->once())
+                       ->method('getDisplayName')
+                       ->willReturn('Mr. Initiator User');
+               $message = $this->createMock(Message::class);
+               $this->mailer
+                       ->expects($this->once())
+                       ->method('createMessage')
+                       ->willReturn($message);
+               $template = $this->createMock(IEMailTemplate::class);
+               $this->mailer
+                       ->expects($this->once())
+                       ->method('createEMailTemplate')
+                       ->willReturn($template);
+               $template
+                       ->expects($this->once())
+                       ->method('addHeader');
+               $template
+                       ->expects($this->once())
+                       ->method('addHeading')
+                       ->with('Mrs. Owner User shared »file.txt« with you');
+               $template
+                       ->expects($this->once())
+                       ->method('addBodyText')
+                       ->with(
+                               'Mrs. Owner User shared »file.txt« with you on behalf of InitiatorUser. Click the button below to open it.',
+                               'Mrs. Owner User shared »file.txt« with you on behalf of InitiatorUser.'
+                       );
+               $template
+                       ->expects($this->once())
+                       ->method('addBodyButton')
+                       ->with(
+                               'Open »file.txt«',
+                               'https://example.com/file.txt'
+                       );
+               $message
+                       ->expects($this->once())
+                       ->method('setTo')
+                       ->with(['john@doe.com']);
+               $this->defaults
+                       ->expects($this->once())
+                       ->method('getName')
+                       ->willReturn('UnitTestCloud');
+               $message
+                       ->expects($this->once())
+                       ->method('setFrom')
+                       ->with([
+                               \OCP\Util::getDefaultEmailAddress('UnitTestCloud') => 'Mrs. Owner User via UnitTestCloud'
+                       ]);
+               $ownerUser
+                       ->expects($this->once())
+                       ->method('getEMailAddress')
+                       ->willReturn(null);
+               $message
+                       ->expects($this->never())
+                       ->method('setReplyTo');
+               $template
+                       ->expects($this->once())
+                       ->method('addFooter')
+                       ->with('');
+               $message
+                       ->expects($this->once())
+                       ->method('setSubject')
+                       ->willReturn('Mrs. Owner User shared »file.txt« with you');
+               $template
+                       ->expects($this->once())
+                       ->method('renderText')
+                       ->willReturn('Text Render');
+               $message
+                       ->expects($this->once())
+                       ->method('setPlainBody')
+                       ->with('Text Render');
+               $template
+                       ->expects($this->once())
+                       ->method('renderHTML')
+                       ->willReturn('HTML Render');
+               $message
+                       ->expects($this->once())
+                       ->method('setHtmlBody')
+                       ->with('HTML Render');
+               $this->mailer
+                       ->expects($this->once())
+                       ->method('send')
+                       ->with($message);
+
+               self::invokePrivate(
+                       $provider,
+                       'sendMailNotification',
+                       [
+                               'file.txt',
+                               'https://example.com/file.txt',
+                               'OwnerUser',
+                               'InitiatorUser',
+                               'john@doe.com',
+                       ]);
+       }
 }

diff --git a/lib/private/Share20/ProviderFactory.php b/lib/private/Share20/ProviderFactory.php
index ba6699ae7ad805228152f321468ae41886820aab..beb3a0965d6cce4b572e752a5c3d161e721d20fc 100644 (file)
--- a/lib/private/Share20/ProviderFactory.php
+++ b/lib/private/Share20/ProviderFactory.php
@@ -30,6 +30,7 @@ use OCA\FederatedFileSharing\Notifications;
 use OCA\FederatedFileSharing\TokenHandler;
 use OCA\ShareByMail\Settings\SettingsManager;
 use OCA\ShareByMail\ShareByMailProvider;
+use OCP\Defaults;
 use OCP\Share\IProviderFactory;
 use OC\Share20\Exception\ProviderException;
 use OCP\IServerContainer;
@@ -158,7 +159,8 @@ class ProviderFactory implements IProviderFactory {
                                $this->serverContainer->getMailer(),
                                $this->serverContainer->getURLGenerator(),
                                $this->serverContainer->getActivityManager(),
-                               $settingsManager
+                               $settingsManager,
+                               $this->serverContainer->query(Defaults::class)
                        );
                }