Choosing a group picture replaced your own avatar (GRYT-1182)

src/db/sqlite/conversations.ts

@@ -182,6 +182,15 @@ export async function setConversationIcon(
   ).run(fileId, conversationId);
 }
 
+/** For the media sweep, which would otherwise delete a group's picture. */
+export async function getAllGroupIconFileIds(): Promise<Set<string>> {
+  const db = getSqliteDb();
+  const rows = db.prepare(
+    `SELECT icon_file_id FROM conversations WHERE kind = 'group' AND icon_file_id IS NOT NULL`,
+  ).all() as { icon_file_id: string }[];
+  return new Set(rows.map((r) => r.icon_file_id));
+}
+
 /** Name a group, or clear the name so it goes back to reading off its members. */
 export async function setConversationName(
   conversationId: string,

src/jobs/mediaSweep.ts

@@ -3,6 +3,7 @@ import {
   deleteFileRecord,
   getAllAvatarFileIds,
   getAllFileRecords,
+  getAllGroupIconFileIds,
   getAllReferencedAttachmentIds,
 } from "../db";
 import { deleteObject } from "../storage";
@@ -23,13 +24,7 @@ export async function runMediaSweep(): Promise<{ deleted: number; errors: number
   let deleted = 0;
   let errors = 0;
 
-  const [allFiles, referencedByMessages, referencedByAvatars] = await Promise.all([
-    getAllFileRecords(),
-    getAllReferencedAttachmentIds(),
-    getAllAvatarFileIds(),
-  ]);
-
-  const referencedIds = new Set<string>([...referencedByMessages, ...referencedByAvatars]);
+  const [allFiles, referencedIds] = await Promise.all([getAllFileRecords(), referencedFileIds()]);
 
   const orphaned = allFiles.filter((f) => {
     if (referencedIds.has(f.file_id)) return false;
@@ -62,16 +57,22 @@ export async function runMediaSweep(): Promise<{ deleted: number; errors: number
   return { deleted, errors };
 }
 
-/** Which of `fileIds` nothing points at any more. Reads the same two sources as
+/** One list for both deleters, so neither can remove what the other keeps. */
+async function referencedFileIds(): Promise<Set<string>> {
+  const sources = await Promise.all([
+    getAllReferencedAttachmentIds(),
+    getAllAvatarFileIds(),
+    getAllGroupIconFileIds(),
+  ]);
+  return new Set<string>(sources.flatMap((ids) => [...ids]));
+}
+
+/** Which of `fileIds` nothing points at any more. Reads the same sources as
     the periodic sweep, so it cannot select something the sweep would keep. */
 export async function unreferencedAmong(fileIds: string[]): Promise<string[]> {
   if (fileIds.length === 0) return [];
 
-  const [referencedByMessages, referencedByAvatars] = await Promise.all([
-    getAllReferencedAttachmentIds(),
-    getAllAvatarFileIds(),
-  ]);
-  const referenced = new Set<string>([...referencedByMessages, ...referencedByAvatars]);
+  const referenced = await referencedFileIds();
 
   return fileIds.filter((id) => !referenced.has(id));
 }

src/routes/groupIconUpload.test.ts

@@ -0,0 +1,138 @@
+import assert from "node:assert/strict";
+import { mkdtempSync, rmSync } from "node:fs";
+import type { AddressInfo } from "node:net";
+import type { Server } from "node:http";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { after, before, describe, it } from "node:test";
+
+import express from "express";
+import sharp from "sharp";
+
+import { initSqlite } from "../db/sqlite/connection";
+import { createGroupConversation, setConversationIcon } from "../db/sqlite/conversations";
+import { getFile, getFileOwnership } from "../db/sqlite/messages";
+import { createServerConfigIfNotExists, setServerRole } from "../db/sqlite/servers";
+import { getUserByServerId, upsertUser } from "../db/sqlite/users";
+import { unreferencedAmong } from "../jobs/mediaSweep";
+import { fileReadVerdict } from "../services/fileAccess";
+import { initStorage } from "../storage";
+import { generateAccessToken } from "../utils/jwt";
+import { uploadsRouter } from "./uploads";
+
+/**
+ * Choosing a group's picture went through the avatar route, which also made it
+ * the uploader's own avatar (GRYT-1182).
+ */
+
+let dir: string;
+let server: Server;
+let base = "";
+let host = "";
+
+interface Member { serverUserId: string; grytUserId: string; nickname: string }
+
+async function member(nickname: string): Promise<Member> {
+  const grytUserId = `acct-${nickname}`;
+  const user = await upsertUser(grytUserId, nickname);
+  await setServerRole(user.server_user_id, "member");
+  return { serverUserId: user.server_user_id, grytUserId, nickname };
+}
+
+async function upload(path: string, who: Member, body: Buffer, type: string, name: string) {
+  const form = new FormData();
+  form.append("file", new Blob([new Uint8Array(body)], { type }), name);
+  const token = generateAccessToken({ ...who, serverHost: host, tokenVersion: 0 });
+  const res = await fetch(`${base}/api/uploads/${path}`, {
+    method: "POST",
+    headers: { Authorization: `Bearer ${token}` },
+    body: form,
+  });
+  return { status: res.status, body: (await res.json()) as Record<string, unknown> };
+}
+
+const png = () =>
+  sharp({ create: { width: 64, height: 64, channels: 3, background: { r: 20, g: 160, b: 140 } } }).png().toBuffer();
+
+const SVG = Buffer.from('<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16"><rect width="16" height="16" fill="#14a08c"/></svg>');
+
+before(async () => {
+  dir = mkdtempSync(join(tmpdir(), "gryt-groupicon-"));
+  process.env.DATA_DIR = dir;
+  process.env.S3_BUCKET = "gryt-test";
+  process.env.STORAGE_BACKEND = "filesystem";
+  await initSqlite();
+  initStorage();
+  await createServerConfigIfNotExists();
+
+  const app = express();
+  app.use("/api/uploads", uploadsRouter);
+  server = app.listen(0, "127.0.0.1");
+  await new Promise<void>((resolve) => server.once("listening", () => resolve()));
+  const { port } = server.address() as AddressInfo;
+  host = `127.0.0.1:${port}`;
+  base = `http://${host}`;
+});
+
+after(async () => {
+  await new Promise<void>((resolve) => server.close(() => resolve()));
+  delete process.env.DATA_DIR;
+  delete process.env.S3_BUCKET;
+  delete process.env.STORAGE_BACKEND;
+  rmSync(dir, { recursive: true, force: true });
+});
+
+describe("POST /api/uploads/group-icon", () => {
+  it("stores the picture without touching the uploader's avatar", async () => {
+    const alice = await member("alice");
+    const res = await upload("group-icon", alice, await png(), "image/png", "group.png");
+
+    assert.equal(res.status, 201, JSON.stringify(res.body));
+    const fileId = res.body.fileId as string;
+    assert.ok(fileId, "no fileId in the reply");
+    assert.equal(res.body.avatarFileId, undefined);
+
+    assert.equal((await getUserByServerId(alice.serverUserId))?.avatar_file_id ?? null, null);
+    const row = await getFile(fileId);
+    assert.equal(row?.mime, "image/avif", "the picture skipped the avatar processing");
+    assert.equal((await getFileOwnership(fileId))?.uploadedBy, alice.serverUserId);
+    // What `dm:group:update` checks before it will set the icon.
+    assert.equal(await fileReadVerdict(fileId, alice.serverUserId, alice.grytUserId), "allowed");
+  });
+
+  it("leaves the avatar alone on the SVG path too", async () => {
+    const bob = await member("bob");
+    const res = await upload("group-icon", bob, SVG, "image/svg+xml", "group.svg");
+
+    assert.equal(res.status, 200, JSON.stringify(res.body));
+    assert.ok(res.body.fileId);
+    assert.equal((await getUserByServerId(bob.serverUserId))?.avatar_file_id ?? null, null);
+  });
+
+  it("refuses what the avatar route refuses", async () => {
+    const carol = await member("carol");
+    const res = await upload("group-icon", carol, Buffer.from("not a picture"), "text/plain", "x.txt");
+    assert.equal(res.status, 400);
+  });
+
+  it("is still what sets an avatar on the avatar route", async () => {
+    const dave = await member("dave");
+    const res = await upload("avatar", dave, await png(), "image/png", "me.png");
+
+    assert.equal(res.status, 201, JSON.stringify(res.body));
+    assert.equal((await getUserByServerId(dave.serverUserId))?.avatar_file_id, res.body.avatarFileId);
+  });
+
+  it("is kept by the media sweep once a group wears it", async () => {
+    const [erin, frank, gina] = [await member("erin"), await member("frank"), await member("gina")];
+    const res = await upload("group-icon", erin, await png(), "image/png", "group.png");
+    const fileId = res.body.fileId as string;
+
+    // Nothing points at it yet, so a picture chosen and then abandoned is swept.
+    assert.deepEqual(await unreferencedAmong([fileId]), [fileId]);
+
+    const group = await createGroupConversation(erin.serverUserId, [frank.serverUserId, gina.serverUserId]);
+    await setConversationIcon(group.conversation_id, fileId);
+    assert.deepEqual(await unreferencedAmong([fileId]), []);
+  });
+});

src/routes/uploadAuthOrder.test.ts

@@ -49,7 +49,7 @@ describe("multer never runs before authentication", () => {
 
     assert.deepEqual(
       buffering,
-      ["POST /api/server/icon", "POST /api/uploads/", "POST /api/uploads/avatar"],
+      ["POST /api/server/icon", "POST /api/uploads/", "POST /api/uploads/avatar", "POST /api/uploads/group-icon"],
       "the set of body-buffering routes changed; if that is deliberate, update this list",
     );
   });

src/routes/uploadStorage.test.ts

@@ -119,7 +119,8 @@ describe("the route uses it", () => {
   /** The attachment route only. The avatar route's own `insertFile` calls are
       unreachable with a sealed upload and would fail this for no reason. */
   const routeStart = file.indexOf("uploadsRouter.post(");
-  const routeEnd = file.indexOf("uploadsRouter.post(", routeStart + 1);
+  // The route's own closing line: the avatar pipeline now sits between routes.
+  const routeEnd = file.indexOf("\n);\n", routeStart);
   const source = file.slice(routeStart, routeEnd);
 
   it("asks for the decision rather than working it out again", () => {

src/routes/uploads.ts

@@ -275,24 +275,17 @@ uploadsRouter.post(
   },
 );
 
-uploadsRouter.post(
-  "/avatar",
-  requireBearerToken,
-  /* `upload_avatar_image`, not `change_avatar`: this endpoint only ever gets a
-     picture, so there is no flag for a modified client to lie about. */
-  (req: Request, res: Response, next: NextFunction): void => {
-    ensurePermission(req, res, "upload_avatar_image")
-      .then((ok) => { if (ok) next(); })
-      .catch(next);
-  },
-  uploadAvatarToMemory("file"),
+/** One pipeline for both pictures. Only an avatar is put on the uploader's own
+    row; a group picture doing that was GRYT-1182. */
+const storeAvatarImage = (purpose: "avatar" | "group") =>
   (req: Request, res: Response, next: NextFunction): void => {
     const file = req.file;
     if (!file) { res.status(400).json({ error: "file_required", message: "file is required" }); return; }
     if (!(file.mimetype || "").startsWith("image/")) { res.status(400).json({ error: "invalid_file", message: "Only image files are allowed" }); return; }
 
+    const what = purpose === "avatar" ? "Avatar" : "Group picture";
     const disableS3 = (process.env.DISABLE_S3 || "").toLowerCase() === "true";
-    if (disableS3) { res.status(503).json({ error: "s3_disabled", message: "S3 is disabled (DISABLE_S3=true). Avatar upload is unavailable." }); return; }
+    if (disableS3) { res.status(503).json({ error: "s3_disabled", message: `S3 is disabled (DISABLE_S3=true). ${what} upload is unavailable.` }); return; }
 
     const bucket = process.env.S3_BUCKET as string;
     if (!bucket) { res.status(500).json({ error: "s3_not_configured", message: "S3_BUCKET not configured" }); return; }
@@ -314,7 +307,7 @@ uploadsRouter.post(
         if (typeof maxBytes === "number" && maxBytes > 0 && file.size > maxBytes) {
           res.status(413).json({
             error: "file_too_large",
-            message: `Avatar too large. Max ${(maxBytes / (1024 * 1024)).toFixed(1)}MB.`,
+            message: `${what} too large. Max ${(maxBytes / (1024 * 1024)).toFixed(1)}MB.`,
           });
           return;
         }
@@ -355,7 +348,7 @@ uploadsRouter.post(
             uploaded_by_server_user_id: serverUserId,
           });
 
-          await setUserAvatar(serverUserId, fileId);
+          if (purpose === "avatar") await setUserAvatar(serverUserId, fileId);
           res.json({ fileId, processing: false });
           return;
         }
@@ -454,8 +447,8 @@ uploadsRouter.post(
               : /AccessDenied|Forbidden/i.test(raw)
                 ? "File storage access denied. Please contact the server administrator."
                 : raw.length > 0
-                  ? `Avatar upload failed: ${raw}`
-                  : "Avatar upload failed due to a storage error.";
+                  ? `${what} upload failed: ${raw}`
+                  : `${what} upload failed due to a storage error.`;
           res.status(502).json({ error: "s3_error", message: friendly });
           return;
         }
@@ -479,8 +472,12 @@ uploadsRouter.post(
           created_at: new Date(),
         });
 
-        await updateUserAvatar(serverUserId, fileId);
-        res.status(201).json({ avatarFileId: fileId, processing });
+        if (purpose === "avatar") {
+          await updateUserAvatar(serverUserId, fileId);
+          res.status(201).json({ avatarFileId: fileId, processing });
+        } else {
+          res.status(201).json({ fileId, processing });
+        }
 
         // Background: resize oversized animated file and replace the placeholder
         if (processing) {
@@ -526,7 +523,33 @@ uploadsRouter.post(
         }
       })
       .catch(next);
+  };
+
+uploadsRouter.post(
+  "/avatar",
+  requireBearerToken,
+  /* `upload_avatar_image`, not `change_avatar`: this endpoint only ever gets a
+     picture, so there is no flag for a modified client to lie about. */
+  (req: Request, res: Response, next: NextFunction): void => {
+    ensurePermission(req, res, "upload_avatar_image")
+      .then((ok) => { if (ok) next(); })
+      .catch(next);
   },
+  uploadAvatarToMemory("file"),
+  storeAvatarImage("avatar"),
+);
+
+uploadsRouter.post(
+  "/group-icon",
+  requireBearerToken,
+  // The permission the group handlers ask for, since only they use this file.
+  (req: Request, res: Response, next: NextFunction): void => {
+    ensurePermission(req, res, "send_direct_messages")
+      .then((ok) => { if (ok) next(); })
+      .catch(next);
+  },
+  uploadAvatarToMemory("file"),
+  storeAvatarImage("group"),
 );
 
 uploadsRouter.delete(