From d85be31e772ad8435ed369d9b49918caf0af17ee Mon Sep 17 00:00:00 2001 From: Ali Asadi Date: Sun, 12 Jul 2026 12:12:31 +0330 Subject: [PATCH] switch chat media messages to client-side MinIO uploads Frontend now uploads media directly to MinIO and sends only the resulting object_key; the backend never touches file bytes. Mattermost post bodies carry ":" instead of a permanent public URL, and download links are signed fresh on every read (send + list) so they can't outlive their expiry. Also wires up MINIO_SECURE, which settings.py previously never read from env. Co-Authored-By: Claude Sonnet 5 --- .env.example | 5 +++ apps/chat/serializers/messages.py | 21 ++++++++- apps/chat/services/message.py | 46 ++++++++++++------- apps/chat/services/storage.py | 47 +++++++++++--------- apps/chat/tests/test_messages.py | 74 ++++++++++++++++++++++--------- apps/chat/views/messages.py | 2 +- env.sample | 2 + main/settings.py | 2 + 8 files changed, 136 insertions(+), 63 deletions(-) diff --git a/.env.example b/.env.example index 18b410d..10cfe8d 100644 --- a/.env.example +++ b/.env.example @@ -27,10 +27,15 @@ MATTERMOST_SERVICE_USERNAME=chat-service MATTERMOST_SERVICE_EMAIL=chat-service@local.invalid # MinIO object storage +# The frontend uploads media directly to MinIO and sends us only the +# resulting object key — these credentials are used solely to sign +# short-lived download URLs when messages are read back. MINIO_ENDPOINT=localhost:9000 MINIO_ACCESS_KEY=minioadmin MINIO_SECRET_KEY=minioadmin MINIO_BUCKET_CHAT=chat +MINIO_SECURE=false +MINIO_PRESIGN_EXPIRY_SECONDS=3600 # Chat long-poll tuning CHAT_LONG_POLL_TIMEOUT_SECONDS=25 diff --git a/apps/chat/serializers/messages.py b/apps/chat/serializers/messages.py index fba8f8c..2ff2781 100644 --- a/apps/chat/serializers/messages.py +++ b/apps/chat/serializers/messages.py @@ -1,11 +1,28 @@ from rest_framework import serializers +MEDIA_MESSAGE_TYPES = {"image", "video", "voice"} + class SendMessageSerializer(serializers.Serializer): sender_id = serializers.UUIDField() - message_type = serializers.ChoiceField(choices=["text", "image", "file"], default="text") + message_type = serializers.ChoiceField( + choices=["text", "image", "video", "voice"], default="text" + ) text = serializers.CharField(required=False, allow_null=True, allow_blank=True) - file = serializers.FileField(required=False, allow_null=True) + object_key = serializers.CharField(required=False, allow_null=True, allow_blank=True) + + def validate(self, attrs): + message_type = attrs.get("message_type", "text") + if message_type in MEDIA_MESSAGE_TYPES: + if not attrs.get("object_key"): + raise serializers.ValidationError( + {"object_key": "object_key is required for image, video, and voice messages."} + ) + elif not attrs.get("text"): + raise serializers.ValidationError( + {"text": "text is required for text messages."} + ) + return attrs class MessageSerializer(serializers.Serializer): diff --git a/apps/chat/services/message.py b/apps/chat/services/message.py index 947c477..0a8cb59 100644 --- a/apps/chat/services/message.py +++ b/apps/chat/services/message.py @@ -10,6 +10,8 @@ from apps.chat.models import Conversation, MattermostAccountMapping from apps.chat.services.account import AccountService from apps.chat.services.storage import StorageService +MEDIA_MESSAGE_TYPES = {"image", "video", "voice"} + class MessageService: def __init__( @@ -40,20 +42,20 @@ class MessageService: sender_id: UUID, message_type: str, text: str | None = None, - file=None, + object_key: str | None = None, ) -> dict: if not self._account.validate_user(sender_id): raise ValueError(f"Sender {sender_id} is not valid") - file_url: str | None = None - if file is not None: - filename = getattr(file, "name", f"{sender_id}") - file_url = self._storage.upload_file(file, filename) - - if message_type == "text": - mm_message = text or "" + # Media was already uploaded straight to MinIO by the client; the + # Mattermost post body only ever carries ":", never + # a URL, so a stored link can't outlive its presigned expiry. + download_url: str | None = None + if message_type in MEDIA_MESSAGE_TYPES: + mm_message = f"{message_type}:{object_key}" + download_url = self._storage.get_download_url(object_key) else: - mm_message = file_url or text or "" + mm_message = text or "" conversation = Conversation.objects.get(id=conversation_id) post_id = self._mm.post_message(conversation.mattermost_channel_id, mm_message) @@ -63,7 +65,7 @@ class MessageService: post_id=post_id, sender_id=sender_id, message_type=message_type, - payload={"text": text, "file_url": file_url}, + payload={"text": text, "object_key": object_key, "url": download_url}, ) for publisher in self._publishers: publisher.publish(event) @@ -73,7 +75,7 @@ class MessageService: "sender_id": sender_id, "message_type": message_type, "text": text, - "url": file_url, + "url": download_url, "created_at": None, } @@ -102,23 +104,33 @@ class MessageService: return [self._normalize_post(p, mappings) for p in raw_posts] - @staticmethod - def _normalize_post(post: dict, mappings: dict) -> dict: + def _normalize_post(self, post: dict, mappings: dict) -> dict: mm_uid = post.get("user_id") sender_id = mappings.get(mm_uid) msg = post.get("message", "") - is_url = msg.startswith("http") created_ms = post.get("create_at") created_at = ( datetime.fromtimestamp(created_ms / 1000, tz=timezone.utc) if created_ms else None ) + + message_type, sep, object_key = msg.partition(":") + if sep and message_type in MEDIA_MESSAGE_TYPES and object_key: + return { + "post_id": post.get("id"), + "sender_id": sender_id, + "message_type": message_type, + "text": None, + "url": self._storage.get_download_url(object_key), + "created_at": created_at, + } + return { "post_id": post.get("id"), "sender_id": sender_id, - "message_type": "file" if is_url else "text", - "text": None if is_url else msg, - "url": msg if is_url else None, + "message_type": "text", + "text": msg, + "url": None, "created_at": created_at, } diff --git a/apps/chat/services/storage.py b/apps/chat/services/storage.py index eff1738..65a1b03 100644 --- a/apps/chat/services/storage.py +++ b/apps/chat/services/storage.py @@ -1,12 +1,17 @@ -import io -import mimetypes -from uuid import uuid4 +from datetime import timedelta from django.conf import settings from minio import Minio class StorageService: + """ + Thin adapter around MinIO for the object-key-in-message-body flow: the + frontend uploads media directly to MinIO and only ever sends us the + resulting object key, so this service's job is limited to turning that + key back into a short-lived download URL on read. + """ + def __init__(self): endpoint = getattr(settings, "MINIO_ENDPOINT", None) if not endpoint: @@ -20,23 +25,21 @@ class StorageService: secure=secure, ) self._bucket = getattr(settings, "MINIO_BUCKET_CHAT", "chat") - scheme = "https" if secure else "http" - self._base_url = f"{scheme}://{endpoint}" - - def upload_file(self, file, filename: str) -> str: - data = file.read() if hasattr(file, "read") else file - size = len(data) - - content_type, _ = mimetypes.guess_type(filename) - if not content_type: - content_type = "application/octet-stream" - - object_name = f"{uuid4().hex}/{filename}" - self._client.put_object( - self._bucket, - object_name, - io.BytesIO(data), - size, - content_type=content_type, + self._expiry = timedelta( + seconds=getattr(settings, "MINIO_PRESIGN_EXPIRY_SECONDS", 3600) ) - return f"{self._base_url}/{self._bucket}/{object_name}" + + def get_download_url(self, object_key: str) -> str | None: + """ + Sign a time-limited download URL for an object the client already + uploaded. Generated fresh on every call rather than cached/stored, + since a stored link would eventually expire while still displayed. + Returns None on failure so one bad key can't fail an entire message + list — callers should treat that as "link unavailable", not an error. + """ + try: + return self._client.presigned_get_object( + self._bucket, object_key, expires=self._expiry + ) + except Exception: + return None diff --git a/apps/chat/tests/test_messages.py b/apps/chat/tests/test_messages.py index afb7fca..dff7097 100644 --- a/apps/chat/tests/test_messages.py +++ b/apps/chat/tests/test_messages.py @@ -7,14 +7,13 @@ from apps.chat.models import Conversation, MattermostAccountMapping from apps.chat.services.message import MessageService -def _make_service(*, mm_post_id="post-1", storage_url=None, publishers=None): +def _make_service(*, mm_post_id="post-1", download_url=None, publishers=None): """Return a MessageService with all external deps mocked.""" mm_client = Mock() mm_client.post_message.return_value = mm_post_id storage = Mock() - if storage_url: - storage.upload_file.return_value = storage_url + storage.get_download_url.return_value = download_url return ( MessageService( @@ -33,11 +32,12 @@ def test_send_text_message(): sender_id = uuid.uuid4() ws_publisher = Mock() - svc, mm_client, _ = _make_service(mm_post_id="post-abc", publishers=[ws_publisher]) + svc, mm_client, storage = _make_service(mm_post_id="post-abc", publishers=[ws_publisher]) result = svc.send(conv.id, sender_id, "text", text="Hello world") mm_client.post_message.assert_called_once_with("ch-send-text", "Hello world") + storage.get_download_url.assert_not_called() ws_publisher.publish.assert_called_once() event = ws_publisher.publish.call_args[0][0] @@ -47,6 +47,7 @@ def test_send_text_message(): assert result["post_id"] == "post-abc" assert result["text"] == "Hello world" + assert result["url"] is None @pytest.mark.django_db @@ -54,17 +55,16 @@ def test_send_image_message(): conv = Conversation.objects.create(mattermost_channel_id="ch-send-image") sender_id = uuid.uuid4() - file_url = "http://minio.local/chat/abc123/photo.jpg" - svc, mm_client, storage = _make_service(storage_url=file_url, publishers=[]) + object_key = "images/abc123/photo.jpg" + download_url = "http://minio.local/chat/images/abc123/photo.jpg?X-Amz-Signature=..." + svc, mm_client, storage = _make_service(download_url=download_url, publishers=[]) - fake_file = Mock() - fake_file.name = "photo.jpg" + result = svc.send(conv.id, sender_id, "image", object_key=object_key) - result = svc.send(conv.id, sender_id, "image", file=fake_file) - - storage.upload_file.assert_called_once_with(fake_file, "photo.jpg") - mm_client.post_message.assert_called_once_with("ch-send-image", file_url) - assert result["url"] == file_url + storage.get_download_url.assert_called_once_with(object_key) + mm_client.post_message.assert_called_once_with("ch-send-image", f"image:{object_key}") + assert result["url"] == download_url + assert result["message_type"] == "image" @pytest.mark.django_db @@ -77,6 +77,7 @@ def test_list_messages_normalized(): user_id=sender_uuid, mattermost_user_id=mm_user_id ) + object_key = "voice/def456/clip.ogg" raw_posts = [ { "id": "p1", @@ -87,14 +88,15 @@ def test_list_messages_normalized(): { "id": "p2", "user_id": mm_user_id, - "message": "http://minio.local/chat/file.pdf", + "message": f"voice:{object_key}", "create_at": 1700000001000, }, ] + download_url = "http://minio.local/chat/voice/def456/clip.ogg?X-Amz-Signature=..." mm_client = Mock() mm_client.get_posts.return_value = raw_posts - svc, _, _ = _make_service(publishers=[]) + svc, _, storage = _make_service(download_url=download_url, publishers=[]) svc._mm = mm_client # inject after construction to keep _make_service simple messages = svc.list_messages(conv.id, page=0, per_page=20) @@ -108,9 +110,39 @@ def test_list_messages_normalized(): assert text_msg["text"] == "First message" assert text_msg["url"] is None - file_msg = messages[1] - assert file_msg["post_id"] == "p2" - assert file_msg["sender_id"] == sender_uuid - assert file_msg["message_type"] == "file" - assert file_msg["url"] == "http://minio.local/chat/file.pdf" - assert file_msg["text"] is None + voice_msg = messages[1] + assert voice_msg["post_id"] == "p2" + assert voice_msg["sender_id"] == sender_uuid + assert voice_msg["message_type"] == "voice" + assert voice_msg["url"] == download_url + assert voice_msg["text"] is None + + storage.get_download_url.assert_called_once_with(object_key) + + +@pytest.mark.django_db +def test_list_messages_treats_colon_in_plain_text_as_text(): + """A plain-text message that happens to contain a colon but doesn't use + a recognized media-type prefix must not be mistaken for a media message. + """ + conv = Conversation.objects.create(mattermost_channel_id="ch-list-colon") + + raw_posts = [ + { + "id": "p1", + "user_id": None, + "message": "Meeting at 10:30", + "create_at": 1700000000000, + }, + ] + + mm_client = Mock() + mm_client.get_posts.return_value = raw_posts + svc, _, storage = _make_service(publishers=[]) + svc._mm = mm_client + + messages = svc.list_messages(conv.id, page=0, per_page=20) + + assert messages[0]["message_type"] == "text" + assert messages[0]["text"] == "Meeting at 10:30" + storage.get_download_url.assert_not_called() diff --git a/apps/chat/views/messages.py b/apps/chat/views/messages.py index 7c0eb06..0287c3c 100644 --- a/apps/chat/views/messages.py +++ b/apps/chat/views/messages.py @@ -27,7 +27,7 @@ class MessageView(APIView): sender_id=data["sender_id"], message_type=data["message_type"], text=data.get("text"), - file=data.get("file"), + object_key=data.get("object_key"), ) return Response(MessageSerializer(result).data, status=status.HTTP_201_CREATED) diff --git a/env.sample b/env.sample index 1dd3033..4177b22 100644 --- a/env.sample +++ b/env.sample @@ -24,3 +24,5 @@ MINIO_ENDPOINT=localhost:9000 MINIO_ACCESS_KEY=minioadmin MINIO_SECRET_KEY=minioadmin MINIO_BUCKET_CHAT=chat +MINIO_SECURE=false +MINIO_PRESIGN_EXPIRY_SECONDS=3600 diff --git a/main/settings.py b/main/settings.py index 47446b4..be4b5d8 100644 --- a/main/settings.py +++ b/main/settings.py @@ -288,5 +288,7 @@ MINIO_ENDPOINT = config('MINIO_ENDPOINT', default=None) MINIO_ACCESS_KEY = config('MINIO_ACCESS_KEY', default=None) MINIO_SECRET_KEY = config('MINIO_SECRET_KEY', default=None) MINIO_BUCKET_CHAT = config('MINIO_BUCKET_CHAT', default='chat') +MINIO_SECURE = config('MINIO_SECURE', default=False, cast=bool) +MINIO_PRESIGN_EXPIRY_SECONDS = config('MINIO_PRESIGN_EXPIRY_SECONDS', default=3600, cast=int) GDAL_LIBRARY_PATH = config('GDAL_LIBRARY_PATH', default=None) GEOS_LIBRARY_PATH = config('GEOS_LIBRARY_PATH', default=None)