Add "Reply-To" on ShareByMailProvider mails

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

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

3 files changed, 289 insertions, 10 deletions

diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php
index 83170c5648c..473613e542e 100644
--- 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 13fb5d03bd8..8761f49f0f4 100644
--- 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 ba6699ae7ad..beb3a0965d6 100644
--- 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)
 			);
 		}