diff --git a/src/db/sqlite/conversations.ts b/src/db/sqlite/conversations.ts index d923556b..df4f46ec 100644 --- a/src/db/sqlite/conversations.ts +++ b/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> { + 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, diff --git a/src/jobs/mediaSweep.ts b/src/jobs/mediaSweep.ts index 4f2333dc..0b937b94 100644 --- a/src/jobs/mediaSweep.ts +++ b/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([...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> { + const sources = await Promise.all([ + getAllReferencedAttachmentIds(), + getAllAvatarFileIds(), + getAllGroupIconFileIds(), + ]); + return new Set(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 { if (fileIds.length === 0) return []; - const [referencedByMessages, referencedByAvatars] = await Promise.all([ - getAllReferencedAttachmentIds(), - getAllAvatarFileIds(), - ]); - const referenced = new Set([...referencedByMessages, ...referencedByAvatars]); + const referenced = await referencedFileIds(); return fileIds.filter((id) => !referenced.has(id)); } diff --git a/src/routes/groupIconUpload.test.ts b/src/routes/groupIconUpload.test.ts new file mode 100644 index 00000000..ad1d9d2d --- /dev/null +++ b/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 { + 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 }; +} + +const png = () => + sharp({ create: { width: 64, height: 64, channels: 3, background: { r: 20, g: 160, b: 140 } } }).png().toBuffer(); + +const SVG = Buffer.from(''); + +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((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((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]), []); + }); +}); diff --git a/src/routes/uploadAuthOrder.test.ts b/src/routes/uploadAuthOrder.test.ts index 986075cb..5cdd1b12 100644 --- a/src/routes/uploadAuthOrder.test.ts +++ b/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", ); }); diff --git a/src/routes/uploadStorage.test.ts b/src/routes/uploadStorage.test.ts index ca63c1d0..ddce17d1 100644 --- a/src/routes/uploadStorage.test.ts +++ b/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", () => { diff --git a/src/routes/uploads.ts b/src/routes/uploads.ts index 3096b126..f2451cc6 100644 --- a/src/routes/uploads.ts +++ b/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(