diff --git a/backend/src/routes/adminUploadLinks.ts b/backend/src/routes/adminUploadLinks.ts index 71edd0c..d1f5b7c 100644 --- a/backend/src/routes/adminUploadLinks.ts +++ b/backend/src/routes/adminUploadLinks.ts @@ -2,7 +2,10 @@ import { Router, Request, Response } from 'express'; import { pool, requireRow } from '../db'; import { asyncRoute } from '../asyncRoute'; import { generateToken, hashToken } from '../uploadLinks'; -import { trimTrailingSlashes } from '../utils'; +import { trimTrailingSlashes, isValidEmail } from '../utils'; +import { sendMail, MailOutcome } from '../mailer'; +import { renderTemplate } from '../emailTemplates'; +import { loadStoredTemplate } from './adminEmailTemplates'; const router = Router(); @@ -27,7 +30,7 @@ const router = Router(); * `token_hash` into every listing the moment somebody added a convenience. */ const LINK_SELECT = ` - SELECT id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at + SELECT id, label, contact_email, revoked_at, submission_count, max_submissions, last_used_at, created_at FROM upload_links `; @@ -44,6 +47,7 @@ const DEFAULT_MAX_SUBMISSIONS = 25; interface UploadLinkRow { id: number; label: string; + contact_email: string | null; revoked_at: string | null; submission_count: number; max_submissions: number | null; @@ -51,6 +55,19 @@ interface UploadLinkRow { created_at: string; } +/** + * What the mail tells the recipient about how much they may send. + * + * Words rather than a bare number for an uncapped link, so the sentence reads + * as a sentence instead of showing an empty space where a figure should be. + * An uncapped link is a deliberate choice the admin already had to make, so it + * is emailable like any other. + */ +function submissionsAllowed(maxSubmissions: number | null): string { + if (maxSubmissions === null) return 'as many items as you like'; + return maxSubmissions === 1 ? '1 item' : `${maxSubmissions} items`; +} + router.get('/', asyncRoute(async (_req: Request, res: Response) => { const { rows } = await pool.query(`${LINK_SELECT} ORDER BY created_at DESC`); res.json(rows); @@ -62,6 +79,11 @@ router.post('/', asyncRoute(async (req: Request, res: Response) => { return res.status(400).json({ error: 'a label is required' }); } + const email = typeof req.body?.email === 'string' ? req.body.email.trim() : ''; + if (email === '' || !isValidEmail(email)) { + return res.status(400).json({ error: 'a valid email address is required' }); + } + // Three cases, deliberately distinct. Absent means nobody decided, which // gets the bounded default. An explicit null means unlimited — a decision // someone made, visible in the request. A number is itself. Reading absent @@ -80,10 +102,10 @@ router.post('/', asyncRoute(async (req: Request, res: Response) => { const token = generateToken(); const { rows } = await pool.query( - `INSERT INTO upload_links (label, token_hash, max_submissions) - VALUES ($1, $2, $3) - RETURNING id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at`, - [label, hashToken(token), maxSubmissions] + `INSERT INTO upload_links (label, token_hash, max_submissions, contact_email) + VALUES ($1, $2, $3, $4) + RETURNING id, label, contact_email, revoked_at, submission_count, max_submissions, last_used_at, created_at`, + [label, hashToken(token), maxSubmissions, email] ); const link = requireRow(rows, 'the upload_links INSERT'); @@ -91,7 +113,43 @@ router.post('/', asyncRoute(async (req: Request, res: Response) => { // outbound link is built from. Absent in local development, which yields a // relative URL the admin screen can still show and copy usefully. const base = trimTrailingSlashes(process.env.PUBLIC_URL ?? ''); - res.status(201).json({ ...link, token, url: `${base}/submit/${token}` }); + const url = `${base}/submit/${token}`; + + // Awaited, and its outcome reported rather than swallowed. Every other sender + // in this codebase fires and forgets because nobody is waiting on the answer; + // here somebody is — the admin is looking at the screen, and whether they + // now have to send the link by hand is the thing they need to know. + // + // A failure does not roll the link back. The token is shown exactly once, so + // a rollback would leave the admin retrying and holding a different link, + // discarding work that succeeded for the sake of tidiness. + let outcome: MailOutcome = 'skipped-unconfigured'; + try { + const template = renderTemplate('uploadLink', await loadStoredTemplate('uploadLink'), { + submitUrl: url, + label: link.label, + submissionsAllowed: submissionsAllowed(link.max_submissions) + }); + outcome = await sendMail(email, template.subject, template.html); + } catch (err) { + // Reported, not thrown. The link exists and is usable; the admin needs to + // be told the mail did not go, not handed a 500 for a link that was made. + // + // An SMTP rejection also lands here and is reported the same way as an + // unconfigured environment, which is not strictly accurate — a fourth + // outcome would be a real distinction, but nothing consumes it and the + // admin's next action is identical either way: copy the link and send it + // by hand. + console.error(`[upload-links] could not email ${email}:`, err); + outcome = 'skipped-unconfigured'; + } + + res.status(201).json({ + ...link, + token, + url, + mail: { sent: outcome === 'sent', outcome } + }); })); /** diff --git a/backend/tests/integration/emailTemplates.integration.test.ts b/backend/tests/integration/emailTemplates.integration.test.ts index bd07bce..a665464 100644 --- a/backend/tests/integration/emailTemplates.integration.test.ts +++ b/backend/tests/integration/emailTemplates.integration.test.ts @@ -30,6 +30,7 @@ describe('GET /api/admin/email-templates', () => { 'favoriteWithdrawn', 'intakeDraft', 'passwordReset', + 'uploadLink', 'verification' ]); diff --git a/backend/tests/integration/intake.integration.test.ts b/backend/tests/integration/intake.integration.test.ts index bc212f4..3b21868 100644 --- a/backend/tests/integration/intake.integration.test.ts +++ b/backend/tests/integration/intake.integration.test.ts @@ -34,7 +34,7 @@ async function storedFiles(): Promise { async function issueLink(label = 'Sarah', maxSubmissions?: number | null): Promise { const res = await request(app) .post('/api/admin/upload-links') - .send({ label, ...(maxSubmissions === undefined ? {} : { maxSubmissions }) }); + .send({ label, email: 'sarah@example.com', ...(maxSubmissions === undefined ? {} : { maxSubmissions }) }); expect(res.status).toBe(201); return res.body.token as string; } diff --git a/backend/tests/integration/intakeCeiling.integration.test.ts b/backend/tests/integration/intakeCeiling.integration.test.ts index 3c06cf5..6e125dc 100644 --- a/backend/tests/integration/intakeCeiling.integration.test.ts +++ b/backend/tests/integration/intakeCeiling.integration.test.ts @@ -20,7 +20,9 @@ afterAll(async () => { }); async function makeLink(): Promise { - const res = await request(app).post('/api/admin/upload-links').send({ label: 'ceiling spec' }); + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'ceiling spec', email: 'sarah@example.com' }); expect(res.status).toBe(201); return res.body.token; } diff --git a/backend/tests/integration/uploadLinks.integration.test.ts b/backend/tests/integration/uploadLinks.integration.test.ts index 6b7c31b..112cb5f 100644 --- a/backend/tests/integration/uploadLinks.integration.test.ts +++ b/backend/tests/integration/uploadLinks.integration.test.ts @@ -16,7 +16,7 @@ describe('issuing an upload link', () => { it('returns the token exactly once, at creation', async () => { const created = await request(app) .post('/api/admin/upload-links') - .send({ label: 'Sarah' }); + .send({ label: 'Sarah', email: 'sarah@example.com' }); expect(created.status).toBe(201); expect(created.body.label).toBe('Sarah'); @@ -34,7 +34,7 @@ describe('issuing an upload link', () => { it('stores the digest rather than the token', async () => { const created = await request(app) .post('/api/admin/upload-links') - .send({ label: 'Estate sale box 3' }); + .send({ label: 'Estate sale box 3', email: 'sarah@example.com' }); const { rows } = await pool.query<{ token_hash: string }>( `SELECT token_hash FROM upload_links` @@ -51,7 +51,7 @@ describe('issuing an upload link', () => { it('refuses a non-positive submission cap', async () => { const res = await request(app) .post('/api/admin/upload-links') - .send({ label: 'Bad cap', maxSubmissions: 0 }); + .send({ label: 'Bad cap', email: 'sarah@example.com', maxSubmissions: 0 }); expect(res.status).toBe(400); }); @@ -59,7 +59,9 @@ describe('issuing an upload link', () => { // safe. An unbounded link should be something asked for, not something that // happens when nobody thought about it. it('bounds a link that was created without a cap', async () => { - const res = await request(app).post('/api/admin/upload-links').send({ label: 'Sarah' }); + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Sarah', email: 'sarah@example.com' }); expect(res.status).toBe(201); expect(res.body.max_submissions).toBe(25); @@ -68,7 +70,7 @@ describe('issuing an upload link', () => { it('allows unlimited when it is asked for explicitly', async () => { const res = await request(app) .post('/api/admin/upload-links') - .send({ label: 'Always on', maxSubmissions: null }); + .send({ label: 'Always on', email: 'sarah@example.com', maxSubmissions: null }); expect(res.status).toBe(201); expect(res.body.max_submissions).toBeNull(); @@ -79,7 +81,7 @@ describe('revoking an upload link', () => { it('stamps revoked_at and reports it in the listing', async () => { const created = await request(app) .post('/api/admin/upload-links') - .send({ label: 'Temporary' }); + .send({ label: 'Temporary', email: 'sarah@example.com' }); const revoked = await request(app) .post(`/api/admin/upload-links/${created.body.id}/revoke`); @@ -94,7 +96,7 @@ describe('revoking an upload link', () => { it('is idempotent, keeping the original timestamp', async () => { const created = await request(app) .post('/api/admin/upload-links') - .send({ label: 'Temporary' }); + .send({ label: 'Temporary', email: 'sarah@example.com' }); const first = await request(app).post(`/api/admin/upload-links/${created.body.id}/revoke`); const second = await request(app).post(`/api/admin/upload-links/${created.body.id}/revoke`); @@ -108,3 +110,99 @@ describe('revoking an upload link', () => { expect(res.status).toBe(404); }); }); + +describe('emailing the link to its recipient', () => { + const original = { ...process.env }; + + afterEach(() => { + process.env = { ...original }; + }); + + it('refuses to create a link with no address', async () => { + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Sarah' }); + + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/email/i); + }); + + it('refuses an address that is not one', async () => { + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Sarah', email: 'not-an-address' }); + + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/email/i); + }); + + it('stores the address on the link', async () => { + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Sarah', email: 'sarah@example.com' }); + + expect(res.status).toBe(201); + + const { rows } = await pool.query<{ contact_email: string }>( + `SELECT contact_email FROM upload_links WHERE id = $1`, + [res.body.id] + ); + expect(rows[0]?.contact_email).toBe('sarah@example.com'); + }); + + // The point of the whole change: QA blocks delivery by design, so a link that + // was not actually emailed must not be reported as though it was. + it('says the mail was not sent when SMTP is not configured', async () => { + delete process.env.SMTP_USER; + delete process.env.SMTP_PASSWORD; + + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Sarah', email: 'sarah@example.com' }); + + expect(res.status).toBe(201); + expect(res.body.mail).toEqual({ sent: false, outcome: 'skipped-unconfigured' }); + }); + + it('says the mail was not sent when the address is not allowlisted', async () => { + process.env.SMTP_USER = 'user'; + process.env.SMTP_PASSWORD = 'password'; + process.env.MAIL_ALLOWLIST = 'someone@example.com'; + + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Sarah', email: 'sarah@example.com' }); + + expect(res.status).toBe(201); + expect(res.body.mail).toEqual({ sent: false, outcome: 'skipped-blocked' }); + }); + + // A send that could not happen must never cost the admin the link, because + // the token is shown exactly once and a rollback would hand them a different + // one on the retry. + it('still returns a usable link when the mail did not go', async () => { + delete process.env.SMTP_USER; + + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Sarah', email: 'sarah@example.com' }); + + expect(res.body.token).toBeTruthy(); + expect(res.body.url).toContain(res.body.token); + expect(res.body.mail.sent).toBe(false); + }); + + it('lists the address, and tolerates a link that has none', async () => { + await pool.query(`INSERT INTO upload_links (label, token_hash) VALUES ('Older link', 'digest')`); + await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Newer link', email: 'sarah@example.com' }); + + const res = await request(app).get('/api/admin/upload-links'); + + const older = res.body.find((row: { label: string }) => row.label === 'Older link'); + const newer = res.body.find((row: { label: string }) => row.label === 'Newer link'); + expect(older.contact_email).toBeNull(); + expect(newer.contact_email).toBe('sarah@example.com'); + }); +});