search

karrot-dev / karrot-frontend

We migrated to https://codeberg.org/karrot/karrot-frontend
https://codeberg.org/karrot/karrot-frontend
427 stars 178 forks source link

User receives unwanted notifications for application chat #2675

Closed nicksellen closed 10 months ago

nicksellen commented 10 months ago

The report is that users can receive notifications for application chats that they did not intend to participate in.

There is no suggestion or evidence that users are receiving messages that they are not allowed to receive, only that their preference is not to receive them.

I compiled a timeline from a reported case. There are 2 users we care about:

Timeline:
  - 2023-08-23 13:10:37.140175+00:00 application created
  - 2023-08-23 13:10:37.183029+00:00 conversation created
  - 2023-08-23 13:10:37.264821+00:00 user [applying] joined conversation
  - 2023-08-25 04:56:12.222258+00:00 user [notified] joined conversation
  - 2023-08-25 05:58:55.561838+00:00 message from [other]
  - 2023-08-27 13:04:51.570100+00:00 message from [applying]
  - 2023-08-27 20:10:08.528774+00:00 message from [applying]
  - 2023-08-27 20:17:26.932752+00:00 message from [notified] (via email)
  - 2023-08-27 21:07:38.266413+00:00 message from [applying]
  - 2023-08-27 21:35:41.759013+00:00 message from [other]
  - 2023-08-27 21:50:09.179863+00:00 message from [applying]
  - 2023-08-28 03:59:00.520509+00:00 message from [other]
  - 2023-08-28 10:42:23.009325+00:00 message from [notified] (via email)

(The script to generate the timeline is https://gist.github.com/nicksellen/0a69c7130030f3b8f52d3df574d261e3)

The interesting thing from here is when [notified] joined the conversation, it is not when the application was created, nor when they sent their first message (which was in response to the notification, via email), but a couple of days after.

Unfortunately, the systemd logs are not available for that time any more. I've enabled persistent logs for journald that might keep them longer... (there are lots of configuration options!).

So, the key question is: why was the user added to the conversation. I have not been able to determine that yet.

One area I am suspicious about is the /conversations/<id>/ update endpoint, which is used for marking where users have read up to, changing notification settings, and maybe other stuff.

The code fetches the participant for the conversation, and if they are not part of it, we create a temporary/mock participant to use for the purpose of the request, which may be saved (e.g. they turn notifications on, which is effectively joining the conversation).

Code where we create the temporary/mock participant:

https://github.com/karrot-dev/karrot-backend/blob/9b3c3710792323223023a47459b2b0b95cf426f9/karrot/conversations/api.py#L174-L178

I wonder if that participant is being saved inadvertently?

I would have liked to look at the logs to look for any relevant requests at that time, but they are not available.

I have looked through the code and the update method seems the most likely to save this temporary participant inadvertently:

https://github.com/karrot-dev/karrot-backend/blob/9b3c3710792323223023a47459b2b0b95cf426f9/karrot/conversations/serializers.py#L408-L423

To my mind there are two scenarios I'd want to look at in more detail:

I have no evidence for the above to scenarios though.

tiltec commented 10 months ago

I ran a quick test in the backend. When frontend sends seen_up_to, the backend seems to create a participant and save it, even when there was no participant before. I guess this shouldn't happen and we can prevent it in backend. Maybe by returning an error. Then we go to frontend and make sure that it doesn't trigger the error...

So in that sense, backend is allowed to create a participant when the notifications field is set properly, but otherwise not.

    def test_mark_seen_up_to_when_not_in_conversation(self):
        user = UserFactory()
        group = GroupFactory(members=[self.user2, user])
        conversation = ConversationFactory(group=group, participants=[self.user2])
        message = conversation.messages.create(author=self.user2, content='yay')
        self.client.force_login(user=user)

        response = self.client.get('/api/conversations/{}/'.format(conversation.id), format='json')
        self.assertEqual(response.status_code, status.HTTP_200_OK)
        self.assertEqual(response.data['seen_up_to'], None)
        self.assertFalse(ConversationParticipant.objects.filter(conversation=conversation.id, user=user.id).exists())

        data = {'seen_up_to': message.id}
        response = self.client.patch('/api/conversations/{}/'.format(conversation.id), data, format='json')
        self.assertEqual(response.status_code, status.HTTP_200_OK)
        self.assertEqual(response.data['seen_up_to'], message.id)

        participant = ConversationParticipant.objects.get(conversation=conversation.id, user=user.id)
        self.assertFalse(ConversationParticipant.objects.filter(conversation=conversation.id, user=user.id).exists())
        print(participant)
        self.assertEqual(participant.seen_up_to, message)
nicksellen commented 10 months ago

Great, sounds promising.... I wonder if it's possible to recreate the frontend sending spurious seen_up_to API requests? That would make it feel quite likely to be the cause. I couldn't find an obvious way from a glance...

We do check whether the user is a participant here before marking as read:

https://github.com/karrot-dev/karrot-frontend/blob/479dbbf7261db604c8f2292248e67547d1cfcddf/src/messages/components/ChatConversation.vue#L298-L315

tiltec commented 10 months ago

But in any case, the backend fix could come first. And then we'll see the failures pop up in frontend sentry.