Merge pull request #7983 from nextcloud/fix/comment_mention_outsiders_notifications

fix(notifciations)

Author: grnd-alt

lib/Notification/NotificationHelper.php

@@ -176,12 +176,28 @@ public function sendBoardShared(int $boardId, Acl $acl, bool $markAsRead = false
 	}
 
 	public function sendMention(IComment $comment): void {
-		foreach ($comment->getMentions() as $mention) {
-			if ((string)$mention['id'] === $this->userId) {
+		$mentions = $comment->getMentions();
+		if (empty($mentions)) {
+			return;
+		}
+
+		$card = $this->cardMapper->find($comment->getObjectId());
+		$boardId = $this->cardMapper->findBoardId($card->getId());
+		$boardUsers = $this->permissionService->findUsers($boardId);
+
+		foreach ($mentions as $mention) {
+			if (($mention['type'] ?? 'users') !== 'users') {
+				// skip non-user mentions
+				continue;
+			}
+			$mentionedUserId = (string)$mention['id'];
+			if ($mentionedUserId === $this->userId) {
+				continue;
+			}
+			if (!array_key_exists($mentionedUserId, $boardUsers)) {
+				// skip users that don't have access to the board
 				continue;
 			}
-			$card = $this->cardMapper->find($comment->getObjectId());
-			$boardId = $this->cardMapper->findBoardId($card->getId());
 			$notification = $this->notificationManager->createNotification();
 			$notification
 				->setApp('deck')

tests/unit/Notification/NotificationHelperTest.php

@@ -489,8 +489,8 @@ public function testSendMention() {
 		$comment->expects($this->once())
 			->method('getMentions')
 			->willReturn([
-				['id' => 'user1'],
-				['id' => 'user2']
+				['id' => 'user1', 'type' => 'users'],
+				['id' => 'user2', 'type' => 'users'],
 			]);
 		$card = new Card();
 		$card->setId(123);
@@ -503,6 +503,13 @@ public function testSendMention() {
 			->method('findBoardId')
 			->with(123)
 			->willReturn(1);
+		$this->permissionService->expects($this->once())
+			->method('findUsers')
+			->with(1)
+			->willReturn([
+				'user1' => $this->createUserMock('user1'),
+				'user2' => $this->createUserMock('user2'),
+			]);
 
 		$notification1 = $this->createMock(INotification::class);
 		$notification1->expects($this->once())->method('setApp')->with('deck')->willReturn($notification1);
@@ -543,8 +550,8 @@ public function testSendMentionSkipsSelf() {
 		$comment->expects($this->once())
 			->method('getMentions')
 			->willReturn([
-				['id' => 'admin'],
-				['id' => 'user1']
+				['id' => 'admin', 'type' => 'users'],
+				['id' => 'user1', 'type' => 'users'],
 			]);
 		$card = new Card();
 		$card->setId(123);
@@ -557,6 +564,13 @@ public function testSendMentionSkipsSelf() {
 			->method('findBoardId')
 			->with(123)
 			->willReturn(1);
+		$this->permissionService->expects($this->once())
+			->method('findUsers')
+			->with(1)
+			->willReturn([
+				'admin' => $this->createUserMock('admin'),
+				'user1' => $this->createUserMock('user1'),
+			]);
 
 		// Only one notification should be created — for user1, not for admin
 		$notification = $this->createMock(INotification::class);
@@ -643,4 +657,58 @@ public function testSendBoardSharedSelfMarkAsReadStillCleansUp() {
 
 		$this->notificationHelper->sendBoardShared(123, $acl, true);
 	}
+
+	/**
+	 * A mention of a user who is not a board member must not trigger a notification.
+	 */
+	public function testSendMentionSkipsNonBoardMembers(): void {
+		$comment = $this->createMock(IComment::class);
+		$comment->expects($this->any())
+			->method('getObjectId')
+			->willReturn(123);
+		$comment->expects($this->any())
+			->method('getMessage')
+			->willReturn('@user1 @outsider This is a message.');
+		$comment->expects($this->once())
+			->method('getMentions')
+			->willReturn([
+				['id' => 'user1', 'type' => 'users'],
+				['id' => 'outsider', 'type' => 'users'],
+			]);
+
+		$card = new Card();
+		$card->setId(123);
+		$card->setTitle('MyCard');
+		$this->cardMapper->expects($this->once())
+			->method('find')
+			->with(123)
+			->willReturn($card);
+		$this->cardMapper->expects($this->once())
+			->method('findBoardId')
+			->with(123)
+			->willReturn(1);
+		// Only user1 is a board member; outsider is not
+		$this->permissionService->expects($this->once())
+			->method('findUsers')
+			->with(1)
+			->willReturn([
+				'user1' => $this->createUserMock('user1'),
+			]);
+
+		// Exactly one notification must be created and sent — for user1 only
+		$notification = $this->createMock(INotification::class);
+		$notification->expects($this->once())->method('setApp')->with('deck')->willReturn($notification);
+		$notification->expects($this->once())->method('setUser')->with('user1')->willReturn($notification);
+		$notification->expects($this->once())->method('setObject')->with('card', 123)->willReturn($notification);
+		$notification->expects($this->once())->method('setSubject')->with('card-comment-mentioned', ['MyCard', 1, 'admin'])->willReturn($notification);
+		$notification->expects($this->once())->method('setDateTime')->willReturn($notification);
+
+		$this->notificationManager->expects($this->once())
+			->method('createNotification')
+			->willReturn($notification);
+		$this->notificationManager->expects($this->once())
+			->method('notify')
+			->with($notification);
+		$this->notificationHelper->sendMention($comment);
+	}
 }