Merge pull request #2562 from nextcloud/fix/bulk-receive-email-notification-bypass

fix(bulkReceive): honour admin email toggle and ISetting notification defaults

Author: miaulalala

lib/Consumer.php

@@ -16,6 +16,7 @@
 use OCP\Config\IUserConfig;
 use OCP\Config\ValueType;
 use OCP\DB\Exception;
+use OCP\IAppConfig;
 
 class Consumer implements IConsumer, IBulkConsumer {
 
@@ -25,6 +26,7 @@ public function __construct(
 		protected UserSettings $userSettings,
 		protected NotificationGenerator $notificationGenerator,
 		protected IUserConfig $userConfig,
+		protected IAppConfig $appConfig,
 	) {
 	}
 
@@ -78,13 +80,16 @@ public function bulkReceive(IEvent $event, array $affectedUserIds, ISetting $set
 
 		$canChangeMail = $setting->canChangeMail();
 		$canChangePush = $setting instanceof ActivitySettings && $setting->canChangeNotification() === true;
+		$defaultPushEnabled = $setting instanceof ActivitySettings && $setting->isDefaultEnabledNotification();
 
-		$userPushSettings = $userEmailSettings = $batchTimeSettings = null;
+		$userPushSettings = null;
 		if ($canChangePush === true) {
 			$userPushSettings = $this->userConfig->getValuesByUsers('activity', 'notify_notification_' . $event->getType(), ValueType::BOOL, $affectedUserIds);
 		}
 
-		if ($canChangeMail === true || $setting->isDefaultEnabledMail() === true) {
+		$userEmailSettings = $batchTimeSettings = null;
+		$emailEnabled = $this->appConfig->getValueString('activity', 'enable_email', 'yes') !== 'no';
+		if ($emailEnabled && ($canChangeMail === true || $setting->isDefaultEnabledMail() === true)) {
 			$userEmailSettings = $this->userConfig->getValuesByUsers('activity', 'notify_email_' . $event->getType(), ValueType::BOOL, $affectedUserIds);
 			$batchTimeSettings = $this->userConfig->getValuesByUsers('activity', 'notify_setting_batchtime', ValueType::INT, $affectedUserIds);
 		}
@@ -95,14 +100,17 @@ public function bulkReceive(IEvent $event, array $affectedUserIds, ISetting $set
 				continue;
 			}
 			$event->setAffectedUser($affectedUser);
-			$notificationSetting = $userPushSettings[$affectedUser] ?? null;
-			if ($notificationSetting !== null) {
-				$notificationSetting = (bool)$notificationSetting;
+
+			if ($canChangePush === true) {
+				$notificationSetting = isset($userPushSettings[$affectedUser]) ? (bool)$userPushSettings[$affectedUser] : $defaultPushEnabled;
+			} else {
+				$notificationSetting = $defaultPushEnabled;
 			}
+
 			$emailSetting = $userEmailSettings[$affectedUser] ?? false;
 			$emailSetting = ($emailSetting) ? ($batchTimeSettings[$affectedUser] ?? false) : false;
 
-			if ($notificationSetting !== false) {
+			if ($notificationSetting === true) {
 				$this->notificationGenerator->sendNotificationForEvent($event, $activityId, $notificationSetting);
 			}
 

tests/ConsumerTest.php

@@ -33,6 +33,7 @@
 use OCP\Activity\IManager;
 use OCP\Activity\ISetting;
 use OCP\Config\IUserConfig;
+use OCP\IAppConfig;
 use OCP\IDBConnection;
 use OCP\IL10N;
 use OCP\L10N\IFactory;
@@ -53,6 +54,7 @@ class ConsumerTest extends TestCase {
 	protected NotificationGenerator&MockObject $notificationGenerator;
 	protected UserSettings $userSettings;
 	private IUserConfig&MockObject $userConfig;
+	private IAppConfig&MockObject $appConfig;
 	private IEvent $event;
 	private Consumer $consumer;
 
@@ -70,7 +72,10 @@ protected function setUp(): void {
 		$this->notificationGenerator = $this->createMock(NotificationGenerator::class);
 		$this->l10nFactory = $this->createMock(IFactory::class);
 		$this->userConfig = $this->createMock(IUserConfig::class);
-
+		$this->appConfig = $this->createMock(IAppConfig::class);
+		$this->appConfig->method('getValueString')
+			->with('activity', 'enable_email', 'yes')
+			->willReturn('yes');
 		$this->data->method('send')
 			->willReturn(1);
 		$this->l10nFactory
@@ -95,6 +100,7 @@ protected function setUp(): void {
 			$this->userSettings,
 			$this->notificationGenerator,
 			$this->userConfig,
+			$this->appConfig,
 		);
 
 		$this->event = Server::get(IManager::class)->generateEvent();
@@ -143,6 +149,7 @@ public function testReceiveStream(string $type, string $author, string $affected
 			$this->userSettings,
 			$this->notificationGenerator,
 			$this->userConfig,
+			$this->appConfig,
 		);
 		$event = Server::get(IManager::class)->generateEvent();
 		$event->setApp('test')
@@ -322,9 +329,7 @@ public function testBulkReceiveNoMailWhenSettingDisabled(): void {
 
 		$this->data->expects($this->never())
 			->method('storeMail');
-		// Notification is still sent because $notificationSetting defaults to null
-		// and null !== false, so the default is to send notifications
-		$this->notificationGenerator->expects($this->once())
+		$this->notificationGenerator->expects($this->never())
 			->method('sendNotificationForEvent');
 
 		$this->consumer->bulkReceive($this->event, ['affectedUser'], $settings);
@@ -434,8 +439,8 @@ public function testBulkReceiveMultipleUsersWithMixedSettings(): void {
 				return [];
 			});
 
-		// user1 and user2 get notifications (user2 has null setting which defaults to send), author is skipped
-		$this->notificationGenerator->expects($this->exactly(2))
+		// only user1 has an explicit notification=true in DB; user2 has no entry so falls back to isDefaultEnabledNotification()=false
+		$this->notificationGenerator->expects($this->once())
 			->method('sendNotificationForEvent');
 		// user2 gets email, author is skipped
 		$this->data->expects($this->once())
@@ -445,6 +450,50 @@ public function testBulkReceiveMultipleUsersWithMixedSettings(): void {
 		$this->consumer->bulkReceive($this->event, ['user1', 'user2', 'author'], $settings);
 	}
 
+	public function testBulkReceiveNoMailWhenAdminEmailDisabled(): void {
+		$time = time();
+		$this->event->setApp('activity')
+			->setType('type')
+			->setAuthor('author')
+			->setTimestamp($time)
+			->setSubject('subject', ['subjectParam1'])
+			->setMessage('message', ['messageParam1'])
+			->setObject('', 0, 'file')
+			->setLink('link');
+
+		$appConfig = $this->createMock(IAppConfig::class);
+		$appConfig->method('getValueString')
+			->with('activity', 'enable_email', 'yes')
+			->willReturn('no');
+
+		$consumer = new Consumer(
+			$this->data,
+			$this->activityManager,
+			$this->userSettings,
+			$this->notificationGenerator,
+			$this->userConfig,
+			$appConfig,
+		);
+
+		$settings = $this->createMock(ActivitySettings::class);
+		$settings->method('canChangeMail')->willReturn(true);
+		$settings->method('isDefaultEnabledMail')->willReturn(true);
+		$settings->method('canChangeNotification')->willReturn(false);
+		$settings->method('isDefaultEnabledNotification')->willReturn(false);
+
+		$this->data->expects($this->once())
+			->method('bulkSend')
+			->willReturn([1 => 'affectedUser']);
+
+		$this->userConfig->expects($this->never())
+			->method('getValuesByUsers');
+
+		$this->data->expects($this->never())
+			->method('storeMail');
+
+		$consumer->bulkReceive($this->event, ['affectedUser'], $settings);
+	}
+
 	public function testBulkReceiveWithISetting(): void {
 		$this->event->setApp('activity')
 			->setType('type')
@@ -463,9 +512,8 @@ public function testBulkReceiveWithISetting(): void {
 			->method('bulkSend')
 			->willReturn([1 => 'affectedUser']);
 
-		// Notification is still sent because $notificationSetting defaults to null (not false)
-		// when ISetting is used (canChangeNotification not available), and null !== false
-		$this->notificationGenerator->expects($this->once())
+		// ISetting is not ActivitySettings so $defaultPushEnabled is false — no notification sent
+		$this->notificationGenerator->expects($this->never())
 			->method('sendNotificationForEvent');
 		$this->data->expects($this->never())
 			->method('storeMail');