[stable10] Backport of Share notification recipients should be added …

…to list Share notification recipients should be added to list. Adding them to bcc was causing spam filters to block the emails. This change would solve the issue and users get notified. Signed-off-by: Sujith H <[email protected]>
owncloud · Sep 3, 2018 · 2be6894 · 2be6894
1 parent 12a482d
commit 2be6894
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 54 deletions.
diff --git a/core/ajax/share.php b/core/ajax/share.php
@@ -179,48 +179,57 @@ function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
 			]);
 
 			$options = [];
-			$options['bcc'] = $filter->getToAddress();
+			$results = [];
+			$toEmails = \explode(',', $filter->getToAddress());
+
+			//Lets get the senders email address and add to the 'to'
 			if (isset($_POST['bccSelf']) && $_POST['bccSelf'] === 'true') {
-				$options['bcc'] .= ',' . \OC::$server->getUserSession()->getUser()->getEMailAddress();
+				$toEmails [] = \OC::$server->getUserSession()->getUser()->getEMailAddress();
 			}
-
-			$l10n = \OC::$server->getL10N('lib');
-
-			$sendingUser = \OC::$server->getUserSession()->getUser();
-			$mailNotification = new \OC\Share\MailNotifications(
-				$sendingUser,
-				$l10n,
-				\OC::$server->getMailer(),
-				\OC::$server->getConfig(),
-				\OC::$server->getLogger(),
-				$defaults,
-				\OC::$server->getURLGenerator(),
-				\OC::$server->getEventDispatcher()
-			);
-
-			$expiration = null;
-			if ($filter->getExpirationDate() !== '') {
-				try {
-					$date = new DateTime($filter->getExpirationDate());
-					$expiration = $date->getTimestamp();
-				} catch (Exception $e) {
-					\OCP\Util::writeLog('sharing', "Couldn't read date: " . $e->getMessage(), \OCP\Util::ERROR);
+			//Send separate email
+			foreach ($toEmails as $toEmail) {
+				$options['to'] = $toEmail;
+
+				$l10n = \OC::$server->getL10N('lib');
+
+				$sendingUser = \OC::$server->getUserSession()->getUser();
+				$mailNotification = new \OC\Share\MailNotifications(
+					$sendingUser,
+					$l10n,
+					\OC::$server->getMailer(),
+					\OC::$server->getConfig(),
+					\OC::$server->getLogger(),
+					$defaults,
+					\OC::$server->getURLGenerator(),
+					\OC::$server->getEventDispatcher()
+				);
+
+				$expiration = null;
+				if ($filter->getExpirationDate() !== '') {
+					try {
+						$date = new DateTime($filter->getExpirationDate());
+						$expiration = $date->getTimestamp();
+					} catch (Exception $e) {
+						\OCP\Util::writeLog('sharing', "Couldn't read date: " . $e->getMessage(), \OCP\Util::ERROR);
+					}
 				}
-			}
 
-			if ($emailBody !== null || $emailBody !== '') {
-				$emailBody = \strip_tags($emailBody);
+				if ($emailBody !== null || $emailBody !== '') {
+					$emailBody = \strip_tags($emailBody);
+				}
+				$result = $mailNotification->sendLinkShareMail(
+					null,
+					$filter->getFile(),
+					$filter->getLink(),
+					$expiration,
+					$filter->getPersonalNote(),
+					$options
+				);
+
+				$results = \array_merge($results, $result);
 			}
-			$result = $mailNotification->sendLinkShareMail(
-				null,
-				$filter->getFile(),
-				$filter->getLink(),
-				$expiration,
-				$filter->getPersonalNote(),
-				$options
-			);
 
-			if (empty($result)) {
+			if (empty($results)) {
 				// Get the token from the link
 				$linkParts = \explode('/', $filter->getLink());
 				$token = \array_pop($linkParts);

diff --git a/lib/private/Share/MailNotifications.php b/lib/private/Share/MailNotifications.php
@@ -209,9 +209,10 @@ public function sendLinkShareMail($recipient, $filename, $link, $expiration, $pe
 		 * - cc ( the cc recipient in mail )
 		 * - bcc ( the bcc recipient in mail )
 		 */
+		$toRecipients = isset($options['to']) ? $options['to'] : '';
 		$ccRecipients = isset($options['cc']) ? $options['cc'] : '';
 		$bccRecipients = isset($options['bcc']) ? $options['bcc'] : '';
-		$event = new GenericEvent(null, ['link' => $link, 'to' => $recipient, 'cc' => $ccRecipients, 'bcc' => $bccRecipients]);
+		$event = new GenericEvent(null, ['link' => $link, 'to' => $toRecipients, 'cc' => $ccRecipients, 'bcc' => $bccRecipients]);
 		$this->eventDispatcher->dispatch('share.sendmail', $event);
 		$options['l10n'] = $l10n;
 		return $this->sendLinkShareMailFromBody($recipient, $subject, $htmlBody, $textBody, $options);
@@ -223,7 +224,7 @@ public function sendLinkShareMail($recipient, $filename, $link, $expiration, $pe
 	 * @param string $recipient recipient email address
 	 * @param string $filename the shared file
 	 * @param string $link the public link
-	 * @param array $options allows ['cc'] and ['bcc'] recipients
+	 * @param array $options allows ['to], ['cc'] and ['bcc'] recipients
 	 * @param int $expiration expiration date (timestamp)
 	 * @return string[] $result of failed recipients
 	 */
@@ -232,6 +233,7 @@ public function sendLinkShareMailFromBody($recipient, $subject, $htmlBody, $text
 		if ($recipient !== null) {
 			$recipients    = $this->_mailStringToArray($recipient);
 		}
+		$toRecipients  = (isset($options['to']) && $options['to'] !== '') ? $this->_mailStringToArray($options['to']) : null;
 		$ccRecipients  = (isset($options['cc']) && $options['cc'] !== '') ? $this->_mailStringToArray($options['cc']) : null;
 		$bccRecipients = (isset($options['bcc']) && $options['bcc'] !== '') ? $this->_mailStringToArray($options['bcc']) : null;
 		$l10n = (isset($options['l10n'])) ? $options['l10n'] : $this->l;
@@ -243,6 +245,9 @@ public function sendLinkShareMailFromBody($recipient, $subject, $htmlBody, $text
 			if ($htmlBody !== null) {
 				$message->setHtmlBody($htmlBody);
 			}
+			if ($toRecipients !== null) {
+				$message->setTo($toRecipients);
+			}
 			if ($bccRecipients !== null) {
 				$message->setBcc($bccRecipients);
 			}
@@ -261,6 +266,12 @@ public function sendLinkShareMailFromBody($recipient, $subject, $htmlBody, $text
 			if ($recipient !== null && $recipient !== '') {
 				$allRecipientsArr = \explode(',', $recipient);
 			}
+			if (isset($options['to']) && $options['to'] !== '') {
+				$allRecipientsArr = \array_merge(
+					$allRecipientsArr,
+					\explode(',', $options['to'])
+				);
+			}
 			if (isset($options['cc']) && $options['cc'] !== '') {
 				$allRecipientsArr = \array_merge(
 					$allRecipientsArr,

diff --git a/tests/lib/Share/MailNotificationsTest.php b/tests/lib/Share/MailNotificationsTest.php
@@ -151,7 +151,7 @@ public function testSendLinkShareMailWithRecipientAndOptions() {
 			->method('setSubject')
 			->with('TestUser shared »MyFile« with you');
 		$message
-			->expects($this->once())
+			->expects($this->exactly(2))
 			->method('setTo')
 			->with(['[email protected]']);
 		$message
@@ -195,7 +195,7 @@ public function testSendLinkShareMailWithRecipientAndOptions() {
 			$calledEvent[] = 'share.sendmail';
 			$calledEvent[] = $event;
 		});
-		$this->assertSame([], $mailNotifications->sendLinkShareMail('[email protected]', 'MyFile', 'https://owncloud.com/file/?foo=bar', 3600, "personal note\n", ['bcc' => '[email protected],[email protected]', 'cc' => '[email protected],[email protected]']));
+		$this->assertSame([], $mailNotifications->sendLinkShareMail('[email protected]', 'MyFile', 'https://owncloud.com/file/?foo=bar', 3600, "personal note\n", ['to' => '[email protected]', 'bcc' => '[email protected],[email protected]', 'cc' => '[email protected],[email protected]']));
 
 		$this->assertEquals('share.sendmail', $calledEvent[0]);
 		$this->assertInstanceOf(GenericEvent::class, $calledEvent[1]);
@@ -218,7 +218,7 @@ public function testSendLinkShareMailPersonalNote() {
 			->method('setSubject')
 			->with('TestUser shared »MyFile« with you');
 		$message
-			->expects($this->once())
+			->expects($this->exactly(2))
 			->method('setTo')
 			->with(['[email protected]']);
 		$message
@@ -262,7 +262,7 @@ public function testSendLinkShareMailPersonalNote() {
 			$calledEvent[] = 'share.sendmail';
 			$calledEvent[] = $event;
 		});
-		$this->assertSame([], $mailNotifications->sendLinkShareMail('[email protected]', 'MyFile', 'https://owncloud.com/file/?foo=bar', 3600, "personal note\n"));
+		$this->assertSame([], $mailNotifications->sendLinkShareMail('[email protected]', 'MyFile', 'https://owncloud.com/file/?foo=bar', 3600, "personal note\n", ['to' => '[email protected]']));
 
 		$this->assertEquals('share.sendmail', $calledEvent[0]);
 		$this->assertInstanceOf(GenericEvent::class, $calledEvent[1]);
@@ -340,13 +340,13 @@ public function testSendLinkShareMailWithReplyTo($to, array $expectedTo) {
 
 	public function dataSendLinkShareMailException() {
 		return [
-			['[email protected]', '', '', '[email protected]'],
-			['[email protected]', '[email protected],[email protected]', '', '[email protected],[email protected],[email protected]'],
-			['[email protected]', '', '[email protected],[email protected]', '[email protected],[email protected],[email protected]'],
-			['[email protected]', '[email protected],[email protected]', '[email protected],[email protected]', '[email protected],[email protected],[email protected],[email protected],[email protected]'],
-			['', '[email protected],[email protected]', '', '[email protected],[email protected]'],
-			['', '', '[email protected],[email protected]', '[email protected],[email protected]'],
-			['', '[email protected],[email protected]', '[email protected],[email protected]', '[email protected],[email protected],[email protected],[email protected]'],
+			['[email protected]', '', '', '[email protected]', 'lukas@owncloud.com,abc@foo.com'],
+			['[email protected]', '[email protected],[email protected]', '', '', '[email protected],[email protected],[email protected]'],
+			['[email protected]', '', '[email protected],[email protected]', '', '[email protected],[email protected],[email protected]'],
+			['[email protected]', '[email protected],[email protected]', '[email protected],[email protected]', '', '[email protected],[email protected],[email protected],[email protected],[email protected]'],
+			['', '[email protected],[email protected]', '', '', '[email protected],[email protected]'],
+			['', '', '[email protected],[email protected]', '', '[email protected],[email protected]'],
+			['', '[email protected],[email protected]', '[email protected],[email protected]', '', '[email protected],[email protected],[email protected],[email protected]'],
 		];
 	}
 
@@ -357,7 +357,7 @@ public function dataSendLinkShareMailException() {
 	 * @param string $bcc
 	 * @param string $expectedRecipientErrorList
 	 */
-	public function testSendLinkShareMailException($to, $cc, $bcc, $expectedRecipientErrorList) {
+	public function testSendLinkShareMailException($to, $cc, $bcc, $toList, $expectedRecipientErrorList) {
 		$this->setupMailerMock('TestUser shared »MyFile« with you', [$to]);
 
 		$mailNotifications = new MailNotifications(
@@ -373,6 +373,10 @@ public function testSendLinkShareMailException($to, $cc, $bcc, $expectedRecipien
 
 		$options = [];
 
+		if ($toList !== '') {
+			$options['to'] = $toList;
+		}
+
 		if ($cc !== '') {
 			$options['cc'] = $cc;
 		}
@@ -621,17 +625,17 @@ protected function setupMailerMock($subject, $to, $exceptionOnSend = true) {
 				->method('setSubject')
 				->with($subject);
 		$message
-				->expects($this->once())
+				->expects($this->any())
 				->method('setTo')
 				->with($to);
 		$message
 				->expects($this->once())
 				->method('setHtmlBody');
 		$message
-				->expects($this->once())
+				->expects($this->any())
 				->method('setPlainBody');
 		$message
-				->expects($this->once())
+				->expects($this->any())
 				->method('setFrom')
 				->with([Util::getDefaultEmailAddress('sharing-noreply') => 'TestUser via UnitTestCloud']);
 
@@ -641,7 +645,7 @@ protected function setupMailerMock($subject, $to, $exceptionOnSend = true) {
 				->will($this->returnValue($message));
 		if ($exceptionOnSend) {
 			$this->mailer
-					->expects($this->once())
+					->expects($this->any())
 					->method('send')
 					->with($message)
 					->will($this->throwException(new \Exception('Some Exception Message')));