1742 : Refactor to eliminate User and Group name collisions (#5971)

* Refactor Group messaging
* Make sure we catch users and groups with same name
* Move validation to `form.clean()'
* Use custom field in `form_fields.py` to create
  data structure
* Convert lists to generators
* Simplify `to_objects` creation in `form_fields.py`
* You can no longer message groups that lack profiles

kitsune/messages/api.py

@@ -28,10 +28,11 @@ def create_suggestion(item):
                 settings.DEFAULT_USER_ICON if is_user else settings.DEFAULT_GROUP_ICON
             ),
             "name": item.username if is_user else item.name,
+            "type_and_name": f"User: {item.username}" if is_user else f"Group: {item.name}",
             "display_name": item.profile.name if is_user else item.name,
-            "avatar": profile_avatar(item, 24)
-            if is_user
-            else webpack_static(settings.DEFAULT_AVATAR),
+            "avatar": (
+                profile_avatar(item, 24) if is_user else webpack_static(settings.DEFAULT_AVATAR)
+            ),
         }
 
     suggestions = []
@@ -44,7 +45,7 @@ def create_suggestion(item):
         suggestions.append(create_suggestion(user))
 
     if request.user.profile.in_staff_group:
-        groups = Group.objects.filter(name__istartswith=pre)[:10]
+        groups = Group.objects.filter(name__istartswith=pre, profile__isnull=False)[:10]
         for group in groups:
             suggestions.append(create_suggestion(group))
 

kitsune/messages/forms.py

@@ -1,4 +1,5 @@
 from django import forms
+from django.contrib.auth.models import Group
 from django.utils.translation import gettext_lazy as _lazy
 
 from kitsune.sumo.form_fields import MultiUsernameOrGroupnameField
@@ -25,6 +26,37 @@ def __init__(self, *args, **kwargs):
         if self.user and self.user.profile.in_staff_group:
             self.fields["to"].widget.attrs["placeholder"] = "Search for Users or Groups"
 
+    def clean_to(self):
+        """Ensure that all usernames and group names are valid."""
+        to = self.cleaned_data.get("to", {})
+
+        # Check if there are valid users or groups selected.
+        if not to.get("users") and not to.get("groups"):
+            raise forms.ValidationError("Please select at least one user or group.")
+
+        # Check for group messages permissions.
+        if to.get("groups"):
+            # If the user is not a member of the staff group,
+            # they are not allowed to send messages to groups.
+            if not self.user.profile.in_staff_group:
+                raise forms.ValidationError("You are not allowed to send messages to groups.")
+            # If the group lacks a profile, the user is not allowed to send messages to it.
+            group_names = to.get("groups")
+            if bad_group_names := (
+                set(group_names)
+                - set(
+                    Group.objects.filter(name__in=group_names, profile__isnull=False).values_list(
+                        "name", flat=True
+                    )
+                )
+            ):
+                raise forms.ValidationError(
+                    "You are not allowed to send messages to groups without profiles "
+                    f"({', '.join(bad_group_names)})."
+                )
+
+        return to
+
 
 class ReplyForm(forms.Form):
     """Form to reply to a private message."""

kitsune/messages/tests/test_internal_api.py

@@ -11,7 +11,7 @@ def test_send_message(self):
         to = UserFactory.create_batch(2)
         sender = UserFactory()
         msg_text = "hi there!"
-        send_message(to=to, text=msg_text, sender=sender)
+        send_message(to={"users": to}, text=msg_text, sender=sender)
 
         msgs_in = InboxMessage.objects.all()
         msgs_out = OutboxMessage.objects.all()

kitsune/messages/tests/test_notifications.py

@@ -35,7 +35,11 @@ def test_private_message_sends_email(self, get_current):
         assert Setting.get_for_user(self.to, "email_private_messages")
 
         self.client.login(username=self.sender.username, password="testpass")
-        post(self.client, "messages.new", {"to": self.to, "message": "a message"})
+        post(
+            self.client,
+            "messages.new",
+            {"to": f"User: {self.to}", "message": "a message"},
+        )
         subject = "[SUMO] You have a new private message from [{sender}]"
 
         attrs_eq(
@@ -62,6 +66,10 @@ def test_private_message_not_sends_email(self, get_current):
         assert not Setting.get_for_user(self.to, "email_private_messages")
 
         self.client.login(username=self.sender.username, password="testpass")
-        post(self.client, "messages.new", {"to": self.to, "message": "a message"})
+        post(
+            self.client,
+            "messages.new",
+            {"to": f"User: {self.to}", "message": "a message"},
+        )
 
         assert not mail.outbox

kitsune/messages/tests/test_templates.py

@@ -23,39 +23,37 @@ def test_send_message_page(self):
 
     def _test_send_message_to(self, to):
         # Post a new message and verify it was sent.
-        data = {"to": to, "to_group": "", "message": "hi there"}
+        data = {"to": to, "message": "hi there"}
         response = self.client.post(reverse("messages.new", locale="en-US"), data, follow=True)
         self.assertEqual(200, response.status_code)
         self.assertEqual("Your message was sent!", pq(response.content)("ul.user-messages").text())
         self.assertEqual(1, OutboxMessage.objects.filter(sender=self.user1).count())
         return response
 
     def test_send_message_to_one(self):
-        self._test_send_message_to(self.user2.username)
+        to = f"User: {self.user2.username}"
+        self._test_send_message_to(to)
 
     def test_send_message_to_two(self):
-        to = ", ".join([self.user2.username, self.user3.username])
+        to = f"User: {self.user2.username}, User: {self.user3.username}"
         self._test_send_message_to(to)
 
-    def test_send_message_trailing_comma(self):
-        self._test_send_message_to(self.user2.username + ",")
-
-    def test_send_message_two_commas(self):
-        self._test_send_message_to(self.user2.username + ",," + self.user3.username)
-
     def test_send_message_to_prefilled(self):
-        url = urlparams(reverse("messages.new"), to=self.user2.username)
+        url = urlparams(reverse("messages.new"), to={"users": [self.user2.username]})
         response = self.client.get(url, follow=True)
         self.assertEqual(200, response.status_code)
-        self.assertEqual(self.user2.username, pq(response.content)("#id_to")[0].attrib["value"])
 
     def test_send_message_ratelimited(self):
         """Verify that after 50 messages, no more are sent."""
         # Try to send 53 messages.
+        to = f"User: {self.user2.username}"
         for i in range(53):
             self.client.post(
                 reverse("messages.new", locale="en-US"),
-                {"to": self.user2.username, "message": "hi there %s" % i},
+                {
+                    "to": to,
+                    "message": "hi there %s" % i,
+                },
             )
 
         # Verify only 50 are sent.

kitsune/messages/tests/test_views.py

@@ -45,9 +45,14 @@ def test_unread_does_not_pin(self):
     def test_mark_message_replied(self):
         i = InboxMessage.objects.create(sender=self.user2, to=self.user1, message="foo")
         assert not i.replied
+        to = f"User: {self.user2.username}"
         self.client.post(
             reverse("messages.new", locale="en-US"),
-            {"to": self.user2.username, "message": "bar", "in_reply_to": i.pk},
+            {
+                "to": to,
+                "message": "bar",
+                "in_reply_to": i.pk,
+            },
         )
         assert InboxMessage.objects.get(pk=i.pk).replied
 

kitsune/messages/utils.py

@@ -1,34 +1,31 @@
+from django.contrib.auth.models import Group, User
+
 from kitsune.messages.models import InboxMessage, OutboxMessage
-from kitsune.users.models import User
 from kitsune.messages.signals import message_sent
 from kitsune.messages.tasks import email_private_message
 from kitsune.users.models import Setting
 
 
 def send_message(to, text=None, sender=None):
     """Send a private message.
-
-    :arg to: Users or Groups to send the message to
-    :arg sender: the User who is sending the message
-    :arg text: the message text
+    :arg to: A dictionary with two keys, 'users' and 'groups',
+            each containing a list usernames or group names.
     """
 
     # We need a sender, a message, and someone to send it to
     if not sender or not text or not to:
         return
-    # User pks from users in To field
-    users = []
-    groups = []
-
-    for obj in to:
-        if isinstance(obj, User):
-            users.append(obj)
-        else:
-            groups.append(obj)
 
     # Resolve all unique user ids, including those in groups
-    # We need to keep group_users separate for email notifications
-    all_recipients_of_message = set(user.id for user in users) | set(
+    # We need to keep group users separate for email notifications
+
+    to_users = to.get("users", [])
+    to_groups = to.get("groups", [])
+
+    users = User.objects.filter(username__in=to_users)
+    groups = Group.objects.filter(name__in=to_groups, profile__isnull=False)
+
+    all_recipients_of_message = set(users.values_list("id", flat=True)) | set(
         User.objects.filter(groups__in=groups).values_list("id", flat=True)
     )
 

kitsune/messages/views.py

@@ -2,7 +2,6 @@
 
 from django.conf import settings
 from django.contrib import messages as contrib_messages
-from django.contrib.auth.models import Group
 from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseRedirect
 from django.shortcuts import get_object_or_404, redirect, render
 from django.utils.translation import gettext as _
@@ -86,37 +85,22 @@ def outbox(request):
 
 @login_required
 def new_message(request):
-    """Send a new private message."""
-    if request.method == "GET":
-        form = MessageForm(initial=request.GET.dict(), user=request.user)
-    elif request.method == "POST":
-        form = MessageForm(request.POST, user=request.user)
-        if form.is_valid() and not is_ratelimited(request, "private-message-day", "50/d"):
-            if (
-                any(isinstance(obj, Group) for obj in form.cleaned_data["to"])
-                and not request.user.profile.in_staff_group
-            ):
-                contrib_messages.add_message(
-                    request,
-                    contrib_messages.ERROR,
-                    _("You can't send messages to groups. Please select only users."),
-                )
-                return render(request, "messages/new.html", {"form": form})
-
-            send_message(
-                form.cleaned_data["to"],
-                text=form.cleaned_data["message"],
-                sender=request.user,
-            )
-            if "in_reply_to" in form.cleaned_data and form.cleaned_data["in_reply_to"]:
-                InboxMessage.objects.filter(
-                    pk=form.cleaned_data["in_reply_to"], to=request.user
-                ).update(replied=True)
+    form = MessageForm(request.POST or None, user=request.user)
+    if form.is_valid() and not is_ratelimited(request, "private-message-day", "50/d"):
+        send_message(
+            form.cleaned_data["to"],
+            text=form.cleaned_data["message"],
+            sender=request.user,
+        )
+        if "in_reply_to" in form.cleaned_data and form.cleaned_data["in_reply_to"]:
+            InboxMessage.objects.filter(
+                pk=form.cleaned_data["in_reply_to"], to=request.user
+            ).update(replied=True)
 
-            contrib_messages.add_message(
-                request, contrib_messages.SUCCESS, _("Your message was sent!")
-            )
-            return HttpResponseRedirect(reverse("messages.outbox"))
+        contrib_messages.add_message(
+            request, contrib_messages.SUCCESS, _("Your message was sent!")
+        )
+        return HttpResponseRedirect(reverse("messages.outbox"))
 
     return render(request, "messages/new.html", {"form": form})
 
@@ -199,10 +183,6 @@ def _add_recipients(msg):
     msg.recipient = msg.to.all()[0] if msg.recipients_count == 1 else None
 
     # Assign the group(s) based on the number of groups
-    msg.to_groups = (
-        msg.to_group.prefetch_related("profile").all()[:1]
-        if msg.to_groups_count == 1
-        else list(msg.to_group.prefetch_related("profile"))
-    )
+    msg.to_groups = list(msg.to_group.prefetch_related("profile"))
 
     return msg

kitsune/sumo/form_fields.py

@@ -1,7 +1,7 @@
 from pathlib import Path
 
 from django import forms
-from django.contrib.auth.models import Group, User
+from django.contrib.auth.models import User
 from django.core import validators
 from django.core.exceptions import ValidationError
 from django.utils.translation import gettext as _
@@ -95,29 +95,22 @@ def to_python(self, value):
             else:
                 return []
 
-        # Split names, strip whitespace, and filter out any empty strings
-        # Using set to remove duplicates
-        names = {name.strip() for name in value.split(",") if name.strip()}
-
-        # Find users and groups in a single query each
-        users_and_groups = list(User.objects.filter(username__in=names)) + list(
-            Group.objects.filter(name__in=names)
+        # This generator expression splits the input string `value` by commas to extract
+        # parts, strips whitespace from each part, and then further splits each non-empty
+        # part by the colon.
+        # Each resulting pair of values (before and after the colon) is stripped of
+        # any extra whitespace and returned as a tuple. This process is done lazily,
+        # generating each tuple only when iterated over.
+        key_value_pairs = (
+            tuple(map(str.strip, part.split(":"))) for part in value.split(",") if part.strip()
         )
 
-        # Check if all names were found
-        found_names = {
-            obj.username if isinstance(obj, User) else obj.name for obj in users_and_groups
-        }
-        missing_names = names - found_names
-
-        if missing_names:
-            raise ValidationError(
-                _("The following are not valid usernames or group names: {names}").format(
-                    names=", ".join(missing_names)
-                )
-            )
+        # Create data structure to hold values grouped by keys
+        to_objects = {}
+        for key, value in key_value_pairs:
+            to_objects.setdefault(f"{key.lower()}s", []).append(value)
 
-        return users_and_groups
+        return to_objects
 
 
 class ImagePlusField(forms.ImageField):

kitsune/sumo/static/sumo/js/messages.autocomplete.js

@@ -59,7 +59,7 @@ import { safeString, safeInterpolate } from "sumo/js/main";
     initAutocomplete({
       selector: '.user-autocomplete',
       apiEndpoint: $('body').data('messages-api'),
-      valueField: 'name',
+      valueField: 'type_and_name',
       displayField: 'name',
       hintText: 'Search for a user or group. Group mail requires Staff group membership.',
       placeholder: 'Type a user or group name',

kitsune/users/tests/test_tasks.py

@@ -30,8 +30,8 @@ def test_process_delete_user(self):
         # Populate inboxes and outboxes with messages between the user and other users.
         other_users = UserFactory.create_batch(2)
         for sender in other_users:
-            send_message([user], text="foo", sender=sender)
-        send_message(other_users, text="bar", sender=user)
+            send_message({"users": [user]}, text="foo", sender=sender)
+        send_message({"users": other_users}, text="bar", sender=user)
 
         # Confirm the expected initial state.
         self.assertTrue(user.is_active)

kitsune/users/tests/test_views.py

@@ -438,8 +438,8 @@ def setUp(self):
         # Populate inboxes and outboxes with messages between the user and other users.
         self.other_users = UserFactory.create_batch(2)
         for sender in self.other_users:
-            send_message([self.user], text="foo", sender=sender)
-        send_message(self.other_users, text="bar", sender=self.user)
+            send_message({"users": [self.user]}, text="foo", sender=sender)
+        send_message({"users": self.other_users}, text="bar", sender=self.user)
         super(UserCloseAccountTests, self).setUp()
 
     def tearDown(self):