rabbit review

Author: sdemagny

src/backend/core/api/openapi.json

@@ -5266,23 +5266,12 @@
                         "readOnly": true,
                         "title": "Updated on",
                         "description": "date and time at which a record was last updated"
-                    },
-                    "abilities": {
-                        "type": "object",
-                        "description": "Instance permissions and capabilities",
-                        "additionalProperties": {
-                            "type": "boolean"
-                        },
-                        "nullable": true,
-                        "readOnly": true
                     }
                 },
                 "required": [
-                    "abilities",
                     "created_at",
                     "id",
                     "name",
-                    "raw_blob",
                     "type",
                     "updated_at"
                 ]
@@ -5337,7 +5326,6 @@
                 },
                 "required": [
                     "name",
-                    "raw_blob",
                     "type"
                 ]
             },
@@ -5696,7 +5684,7 @@
                     },
                     "is_forced": {
                         "type": "boolean",
-                        "description": "Whether this template is forced no other same type template can be used"
+                        "description": "Whether this template is forced; no other template of the same type can be used in the same scope"
                     },
                     "created_at": {
                         "type": "string",
@@ -5711,19 +5699,9 @@
                         "readOnly": true,
                         "title": "Updated on",
                         "description": "date and time at which a record was last updated"
-                    },
-                    "abilities": {
-                        "type": "object",
-                        "description": "Instance permissions and capabilities",
-                        "additionalProperties": {
-                            "type": "boolean"
-                        },
-                        "nullable": true,
-                        "readOnly": true
                     }
                 },
                 "required": [
-                    "abilities",
                     "created_at",
                     "id",
                     "name",

src/backend/core/api/serializers.py

@@ -919,7 +919,7 @@ class ImportIMAPSerializer(ImportBaseSerializer):
     )
 
 
-class ReadOnlyMessageTemplateSerializer(AbilitiesModelSerializer):
+class ReadOnlyMessageTemplateSerializer(serializers.ModelSerializer):
     """Serialize message templates for read-only operations."""
 
     type = IntegerChoicesField(choices_class=models.MessageTemplateTypeChoices)
@@ -946,7 +946,7 @@ class Meta:
         read_only_fields = ["id", "created_at", "updated_at", "raw_blob"]
 
 
-class MessageTemplateSerializer(AbilitiesModelSerializer):
+class MessageTemplateSerializer(serializers.ModelSerializer):
     """Serialize message templates for POST/PUT/PATCH operations."""
 
     type = IntegerChoicesField(choices_class=models.MessageTemplateTypeChoices)
@@ -1068,7 +1068,7 @@ def update(self, instance, validated_data):
         maildomain_id = validated_data.pop("maildomain_id", None)
         raw_blob_content = validated_data.pop("raw_blob", None)
 
-        # Update mailbox or maildomain if provided
+        # Update mailbox or maildomain if provided (validate consistency first)
         if mailbox_id is not None and maildomain_id is not None:
             # check if domain is the same as the mailbox domain
             mailbox = models.Mailbox.objects.get(id=mailbox_id)

src/backend/core/api/viewsets/message_template.py

@@ -110,6 +110,7 @@ def get_queryset_for_list(self):
         maildomain_id = self.request.query_params.get("maildomain_id")
 
         # Apply filters based on query parameters
+        # Admin manage domain view (case 1)
         if maildomain_id:
             # When filtering by maildomain_id, only show templates the user has direct access to
             # through maildomain access (not through mailbox access)
@@ -121,6 +122,7 @@ def get_queryset_for_list(self):
                 )
             else:
                 queryset = queryset.filter(maildomain_id=maildomain_id)
+        # New message creation view (case 2)
         elif mailbox_id:
             # Get the mailbox to find its domain
             try:
@@ -142,16 +144,21 @@ def get_queryset_for_list(self):
                     queryset = queryset.filter(
                         Q(mailbox_id=mailbox_id) | Q(maildomain_id=mailbox.domain_id)
                     )
+                # if a forced template exists, user can only see it
+                if queryset.filter(is_forced=True).exists():
+                    queryset = queryset.filter(is_forced=True)
             except Mailbox.DoesNotExist:
                 return MessageTemplate.objects.none()
 
         # Apply additional filters
         is_forced = self.request.query_params.get("is_forced")
-        _type = self.request.query_params.get("type")
+        template_type = self.request.query_params.get("type")
         is_active = self.request.query_params.get("is_active")
 
-        if _type:
-            queryset = queryset.filter(type=MessageTemplateTypeChoices[_type.upper()])
+        if template_type:
+            queryset = queryset.filter(
+                type=MessageTemplateTypeChoices[template_type.upper()]
+            )
         if is_active is not None:
             is_active_bool = is_active.lower() in ("true", "1", "yes")
             queryset = queryset.filter(is_active=is_active_bool)

src/backend/core/migrations/0007_alter_messagetemplate_type_blob_blob_has_owner_and_more.py

@@ -0,0 +1,34 @@
+# Generated by Django 5.1.11 on 2025-09-01 22:13
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('core', '0006_blob_maildomain_alter_blob_mailbox_and_more'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='messagetemplate',
+            name='type',
+            field=models.SmallIntegerField(choices=[(1, 'reply'), (2, 'new_message'), (3, 'signature')], default=1, help_text='Type of template (reply, new_message, signature)', verbose_name='type'),
+        ),
+        migrations.AddConstraint(
+            model_name='blob',
+            constraint=models.CheckConstraint(condition=models.Q(('mailbox__isnull', False), ('maildomain__isnull', False), _connector='OR'), name='blob_has_owner'),
+        ),
+        migrations.AddConstraint(
+            model_name='messagetemplate',
+            constraint=models.CheckConstraint(condition=models.Q(('mailbox__isnull', False), ('maildomain__isnull', False), _connector='OR'), name='messagetemplate_has_owner'),
+        ),
+        migrations.AddConstraint(
+            model_name='messagetemplate',
+            constraint=models.UniqueConstraint(condition=models.Q(('is_forced', True)), fields=('mailbox', 'type'), name='uniq_forced_template_mailbox_type'),
+        ),
+        migrations.AddConstraint(
+            model_name='messagetemplate',
+            constraint=models.UniqueConstraint(condition=models.Q(('is_forced', True)), fields=('maildomain', 'type'), name='uniq_forced_template_maildomain_type'),
+        ),
+    ]

src/backend/core/migrations/0008_alter_messagetemplate_is_forced_and_more.py

@@ -0,0 +1,26 @@
+# Generated by Django 5.1.11 on 2025-09-01 22:34
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('core', '0007_alter_messagetemplate_type_blob_blob_has_owner_and_more'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='messagetemplate',
+            name='is_forced',
+            field=models.BooleanField(default=False, help_text='Whether this template is forced; no other template of the same type can be used in the same scope', verbose_name='is forced'),
+        ),
+        migrations.AddIndex(
+            model_name='messagetemplate',
+            index=models.Index(fields=['mailbox', 'type', 'is_active'], name='messages_me_mailbox_00a4d4_idx'),
+        ),
+        migrations.AddIndex(
+            model_name='messagetemplate',
+            index=models.Index(fields=['maildomain', 'type', 'is_active'], name='messages_me_maildom_4a0e75_idx'),
+        ),
+    ]

src/backend/core/models.py

@@ -14,7 +14,8 @@
 from django.contrib.auth.base_user import AbstractBaseUser
 from django.core import validators
 from django.core.exceptions import ValidationError
-from django.db import models
+from django.db import models, transaction
+from django.utils.html import escape
 from django.utils.text import slugify
 from django.utils.translation import gettext_lazy as _
 
@@ -1283,15 +1284,24 @@ class Meta:
         verbose_name = _("blob")
         verbose_name_plural = _("blobs")
         ordering = ["-created_at"]
+        constraints = [
+            models.CheckConstraint(
+                check=(
+                    models.Q(mailbox__isnull=False) | models.Q(maildomain__isnull=False)
+                ),
+                name="blob_has_owner",
+            ),
+        ]
 
     def __str__(self):
         return f"Blob {self.id} ({self.size} bytes)"
 
-    def save(self, *args, **kwargs):
-        # at least one of mailbox or maildomain must be set
-        if not self.mailbox and not self.maildomain:
-            raise ValueError("Blob must have at least a mailbox or maildomain")
-        super().save(*args, **kwargs)
+    def clean(self):
+        if not self.mailbox_id and not self.maildomain_id:
+            raise ValidationError(
+                {"__all__": "Blob must have at least a mailbox or maildomain"}
+            )
+        super().clean()
 
     def get_content(self) -> bytes:
         """
@@ -1492,9 +1502,7 @@ class MessageTemplate(BaseModel):
         _("type"),
         choices=MessageTemplateTypeChoices.choices,
         default=MessageTemplateTypeChoices.REPLY,
-        help_text=_(
-            "Type of template (reply, forward, new message, auto reply, signature)"
-        ),
+        help_text=_("Type of template (reply, new_message, signature)"),
     )
 
     is_active = models.BooleanField(
@@ -1525,64 +1533,69 @@ class MessageTemplate(BaseModel):
         _("is forced"),
         default=False,
         help_text=_(
-            "Whether this template is forced no other same type template can be used"
+            "Whether this template is forced; no other template of the same type can be used in the same scope"
         ),
     )
 
     class Meta:
         db_table = "messages_messagetemplate"
         verbose_name = _("message template")
         verbose_name_plural = _("message templates")
-        ordering = ["name"]
+        ordering = ["-created_at"]
+        constraints = [
+            models.CheckConstraint(
+                check=(
+                    models.Q(mailbox__isnull=False) | models.Q(maildomain__isnull=False)
+                ),
+                name="messagetemplate_has_owner",
+            ),
+            models.UniqueConstraint(
+                fields=("mailbox", "type"),
+                condition=models.Q(is_forced=True),
+                name="uniq_forced_template_mailbox_type",
+            ),
+            models.UniqueConstraint(
+                fields=("maildomain", "type"),
+                condition=models.Q(is_forced=True),
+                name="uniq_forced_template_maildomain_type",
+            ),
+        ]
+        indexes = [
+            models.Index(fields=("mailbox", "type", "is_active")),
+            models.Index(fields=("maildomain", "type", "is_active")),
+        ]
 
     def __str__(self):
         return f"{self.name} ({self.get_type_display()})"
 
     def save(self, *args, **kwargs):
-        # at least one of mailbox or maildomain must be set
+        with transaction.atomic():
+            if self.is_forced:
+                qs = MessageTemplate.objects.filter(
+                    type=self.type, is_forced=True
+                ).exclude(id=self.id)
+                if self.mailbox_id:
+                    qs = qs.filter(mailbox_id=self.mailbox_id)
+                if self.maildomain_id:
+                    qs = qs.filter(maildomain_id=self.maildomain_id)
+                qs.update(is_forced=False)
+            super().save(*args, **kwargs)
+
+    def clean(self):
+        errors = {}
         if not self.mailbox and not self.maildomain:
-            raise ValueError(
+            errors["__all__"] = (
                 "MessageTemplate must have at least a mailbox or maildomain"
             )
-
-        if self.is_forced:
-            # If this template is being set as forced, unforce other templates of the same type
-            # in the same maildomain or mailbox
-            other_forced_templates = MessageTemplate.objects.filter(
-                type=self.type, is_forced=True
-            ).exclude(id=self.id)
-            if self.mailbox:
-                other_forced_templates = other_forced_templates.filter(
-                    mailbox=self.mailbox
-                )
-            if self.maildomain:
-                other_forced_templates = other_forced_templates.filter(
-                    maildomain=self.maildomain
-                )
-
-            if other_forced_templates.exists():
-                other_forced_templates.update(is_forced=False)
-
-        super().save(*args, **kwargs)
-
-    def get_abilities(self, user):
-        """Get abilities for this template based on user permissions."""
-        abilities = []
-
-        # Check if user has access to any of the mailboxes or maildomains
         if (
-            user.is_superuser
-            or (self.mailbox and self.mailbox.accesses.filter(user=user).exists())
-            or (self.maildomain and self.maildomain.accesses.filter(user=user).exists())
+            self.mailbox
+            and self.maildomain
+            and self.mailbox.domain_id != self.maildomain_id
         ):
-            abilities.extend(
-                [
-                    UserAbilities.CAN_VIEW_DOMAIN_ADMIN,
-                    UserAbilities.CAN_CREATE_MAILDOMAINS,
-                ]
-            )
-
-        return abilities
+            errors["mailbox"] = "Mailbox domain does not match maildomain."
+        if errors:
+            raise ValidationError(errors)
+        super().clean()
 
     def render_template(self, user: User) -> Dict[str, str]:
         """
@@ -1609,7 +1622,9 @@ def render_template(self, user: User) -> Dict[str, str]:
         # Simple placeholder substitution
         for key, value in context.items():
             placeholder = f"{{{key}}}"
-            rendered_html_body = rendered_html_body.replace(placeholder, str(value))
+            rendered_html_body = rendered_html_body.replace(
+                placeholder, escape(str(value))
+            )
             rendered_text_body = rendered_text_body.replace(placeholder, str(value))
 
         return {