Updates for 5922 - Staff can send Group messages (#5957)

* Fixes privelege escalation - 1721
* Fixes bolded message text - 1726
* Removes unneeded Avatars - 1724
* Don't show a To if there is no To - 1723
* Attempts to resolve 502 issue - 1720

kitsune/messages/jinja2/messages/includes/macros.html

@@ -51,37 +51,33 @@
 
 
 {% macro outbox_message(message) -%}
-  <span class="avatar">
-    {% if message.recipients > 1 -%}
-      {{ avatar_link(request.user) }}
-    {% else %}
-      {{ avatar_link(message.recipient) }}
-    {% endif %}
-  </span>
   <span class="to">
+  {% if message.recipient %}
     <p><strong>{{ _('To') }}:</strong>
       {% if message.recipients > 1 -%}
         {% set comma = joiner(', ') %}
         {% for user in message.to.all() -%}
           {{ comma() }}
           {{ name_link(user) }}
         {%- endfor %}
-        </p>
-        </span>
-        {% if in_staff_group(request.user) and message.to_group %}
-          <span class="to-group">
-            <p><strong>{{ _('To Groups') }}:</strong> 
-            {% set comma = joiner(', ') %}
-            {% for group in message.to_group.all() -%}
-              {{ comma() }}
-              {{ group_link(group) }}
-            {% endfor %}
-            </p>
-          </span>
-        {% endif %}
-    {% endif %}
-    <span class="time">{{ datetimeformat(message.created) }}</span>
+      {% else %}
+        {{ name_link(message.recipient) }}
+      {% endif %}
+    </p>
+  {% endif %}
   </span>
+  {% if in_staff_group(request.user) and message.to_group %}
+    <span class="to-group">
+      <p><strong>{{ _('To Groups') }}:</strong> 
+      {% set comma = joiner(', ') %}
+      {% for group in message.to_group.all() -%}
+        {{ comma() }}
+        {{ group_link(group) }}
+      {% endfor %}
+      </p>
+    </span>
+  {% endif %}
+  <span class="time">{{ datetimeformat(message.created) }}</span>
   <div class="message read-message">{{ message.content_parsed }}</div>
 {%- endmacro %}
 

kitsune/messages/jinja2/messages/outbox.html

@@ -22,8 +22,6 @@ <h1 class="sumo-page-heading">{{ title }}</h1>
                 <div class="field checkbox no-label">
                 </div>
                 <div class="avatar-details">
-                  <span class="avatar">
-                  </span>
                   <span class="to user">
                     <b>{{ _('Sent') }}</b>
                   </span>
@@ -50,13 +48,6 @@ <h1 class="sumo-page-heading">{{ title }}</h1>
                   <label for="id_checkbox_{{ message.id }}"></label>
                 </div>
                 <div class="avatar-details">
-                  <span class="avatar">
-                    {% if message.recipients > 1 -%}
-                      {{ avatar_link(request.user) }}
-                    {% else %}
-                      {{ avatar_link(message.recipient) }}
-                    {% endif %}
-                  </span>
                   <span class="time">
                     <p>{{ datetimeformat(message.created) }}</p>
                   </span>
@@ -72,7 +63,7 @@ <h1 class="sumo-page-heading">{{ title }}</h1>
                           {%- if not loop.last -%},{%- endif -%}
                         </a>
                       {% endfor %}
-                      {% if message.recipients > 3 -%}
+                      {% if message.recipients >= 3 -%}
                       ...
                       {% endif %}
                     {% else %}

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, to_group="", text=msg_text, sender=sender)
+        send_message(to=to, text=msg_text, sender=sender)
 
         msgs_in = InboxMessage.objects.all()
         msgs_out = OutboxMessage.objects.all()

kitsune/messages/utils.py

@@ -1,39 +1,56 @@
+from django.contrib.auth.models import Group
+
 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, to_group=None, text=None, sender=None):
+def send_message(to, text=None, sender=None):
     """Send a private message.
 
-    :arg to: a list of Users to send the message to
-    :arg to_groups: Groups to send the message to
+    :arg to: Users or Groups to send the message to
     :arg sender: the User who is sending the message
     :arg text: the message text
     """
-    if not sender or not text:
-        return
 
-    # Assuming OutboxMessage doesn't necessarily need to be created for every recipient
-    if to:
-        outbox_message = OutboxMessage.objects.create(sender=sender, message=text)
-        outbox_message.to.set(to)
-        if to_group:
-            outbox_message.to_group.set(to_group)
-
-    for recipient in to:
-        if isinstance(recipient, User):
-            inbox_message = InboxMessage.objects.create(sender=sender, to=recipient, message=text)
-            # If we had a user, and we made them an inbox message,
-            # we should also add the to_groups to their message as well
-            if to_group:
-                inbox_message.to_group.set(to_group)
-            if Setting.get_for_user(recipient, "email_private_messages"):
-                email_private_message(inbox_message_id=inbox_message.id)
-
-    message_sent.send(sender=InboxMessage, to=to, to_group=to_group, text=text, msg_sender=sender)
+    # We need a sender, a message, and someone to send it to
+    if not sender or not text or not to:
+        return
+    # This is users from the To field
+    users = {user for user in to if isinstance(user, User)}
+    # This is groups from the To field
+    groups = {group for group in to if isinstance(group, Group)}
+    # This is all the users that are going to receive the message
+    receivers = users | set(User.objects.filter(groups__in=groups).distinct())
+
+    outbox_message = OutboxMessage.objects.create(sender=sender, message=text)
+    # Add the users from the To field to the outbox message, not all the users
+    # that are going to receive the message - this way we don't overwhelm the
+    # message UI
+    outbox_message.to.set(users)
+
+    if groups:
+        # Add the groups from the To field to the message
+        outbox_message.to_group.set(groups)
+
+    set_of_user_pks_to_email_private_message = set(
+        Setting.objects.filter(
+            user__in=receivers, name="email_private_messages", value=True
+        ).values_list("user__pk", flat=True)
+    )
+
+    for recipient in receivers:
+        inbox_message = InboxMessage.objects.create(sender=sender, to=recipient, message=text)
+        # If we had a user, and we made them an inbox message,
+        # we should also add the groups to their message as well
+        if groups:
+            inbox_message.to_group.set(groups)
+        if recipient.pk in set_of_user_pks_to_email_private_message:
+            email_private_message(inbox_message_id=inbox_message.id)
+
+    message_sent.send(sender=InboxMessage, to=to, text=text, msg_sender=sender)
 
 
 def unread_count_for(user):

kitsune/messages/views.py

@@ -1,7 +1,7 @@
 import json
 
 from django.contrib import messages as contrib_messages
-from django.contrib.auth.models import Group, User
+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 _
@@ -75,18 +75,19 @@ def new_message(request):
     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"):
-            receivers = {user for user in form.cleaned_data["to"] if isinstance(user, User)}
-            groups = [group for group in form.cleaned_data["to"] if isinstance(group, Group)]
-            prefetched_groups = Group.objects.filter(
-                pk__in=[group.pk for group in groups]
-            ).prefetch_related("user_set")
-
-            for group in prefetched_groups:
-                receivers.update(group.user_set.all())
+            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(
-                list(receivers),
-                to_group=groups,
+                form.cleaned_data["to"],
                 text=form.cleaned_data["message"],
                 sender=request.user,
             )

kitsune/sumo/static/sumo/scss/components/_inbox.scss

@@ -1,17 +1,15 @@
 @use '../config' as c;
 @use 'protocol/css/includes/lib' as p;
 
-
 .message-list {
   border-top: 1px solid var(--color-border);
 
   &--item {
-
     display: grid;
     grid-gap: p.$spacing-xs 10px;
     grid-template-columns: 36px 1fr 30px;
     grid-template-areas:
-      "checkbox avatar  delete"
+      "checkbox avatar delete"
       "checkbox details delete";
     padding: p.$spacing-sm 0;
     border-bottom: 1px solid var(--color-border);
@@ -24,6 +22,18 @@
 
     .avatar-details {
       grid-area: avatar;
+      display: flex;
+      align-items: center;
+      justify-content: space-between;
+      width: 100%;
+
+      span {
+        flex: 1;
+        text-align: left;
+        white-space: nowrap;
+        overflow: hidden;
+        text-overflow: ellipsis;
+      }
     }
 
     .read {
@@ -66,7 +76,6 @@
       }
 
       .avatar-details {
-        width: 100%;
         flex: 0 0 auto;
       }
     }
@@ -81,6 +90,7 @@
 .avatar-details {
   display: flex;
   align-items: center;
+  justify-content: space-between;
   gap: 12px;
 
   span {
@@ -97,15 +107,16 @@
     @include p.bidi(((margin, 0 12px 0 0, 0 0 0 12px),));
     flex: 0 0 auto;
     width: 44px;
+    max-width: 44px;
 
     img {
       width: 100%;
+      max-width: 40px;
       height: auto;
     }
   }
 
   .user, .group {
-
     a {
       color: var(--color-heading);
       text-decoration: none;
@@ -120,7 +131,7 @@
       font-weight: bold;
       color: var(--color-heading);
       text-decoration: none;
-      padding-right:5px;
+      padding-right: 5px;
     }
   }
 }
@@ -145,16 +156,17 @@ time {
 }
 
 .to, 
-.to-group {
+.to-group,
+.to-user {
   p {
-  margin: 0;
-  padding: 0;
-  font-weight: bold;
-  color: var(--color-heading);
-  text-decoration: none;
+    margin: 0;
+    padding: 0;
+    font-weight: bold;
+    color: var(--color-heading);
+    text-decoration: none;
   }
 }
 
 .read-message {
   margin-top: 20px;
-}
+}

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], to_group="", text="foo", sender=sender)
-        send_message(other_users, to_group="", text="bar", sender=user)
+            send_message([user], text="foo", sender=sender)
+        send_message(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], to_group="", text="foo", sender=sender)
-        send_message(self.other_users, to_group="", text="bar", sender=self.user)
+            send_message([self.user], text="foo", sender=sender)
+        send_message(self.other_users, text="bar", sender=self.user)
         super(UserCloseAccountTests, self).setUp()
 
     def tearDown(self):