Feature/260 email the upload link #292
@@ -2,7 +2,10 @@ import { Router, Request, Response } from 'express';
|
|||||||
import { pool, requireRow } from '../db';
|
import { pool, requireRow } from '../db';
|
||||||
import { asyncRoute } from '../asyncRoute';
|
import { asyncRoute } from '../asyncRoute';
|
||||||
import { generateToken, hashToken } from '../uploadLinks';
|
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();
|
const router = Router();
|
||||||
|
|
||||||
@@ -27,7 +30,7 @@ const router = Router();
|
|||||||
* `token_hash` into every listing the moment somebody added a convenience.
|
* `token_hash` into every listing the moment somebody added a convenience.
|
||||||
*/
|
*/
|
||||||
const LINK_SELECT = `
|
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
|
FROM upload_links
|
||||||
`;
|
`;
|
||||||
|
|
||||||
@@ -44,6 +47,7 @@ const DEFAULT_MAX_SUBMISSIONS = 25;
|
|||||||
interface UploadLinkRow {
|
interface UploadLinkRow {
|
||||||
id: number;
|
id: number;
|
||||||
label: string;
|
label: string;
|
||||||
|
contact_email: string | null;
|
||||||
revoked_at: string | null;
|
revoked_at: string | null;
|
||||||
submission_count: number;
|
submission_count: number;
|
||||||
max_submissions: number | null;
|
max_submissions: number | null;
|
||||||
@@ -51,6 +55,19 @@ interface UploadLinkRow {
|
|||||||
created_at: string;
|
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) => {
|
router.get('/', asyncRoute(async (_req: Request, res: Response) => {
|
||||||
const { rows } = await pool.query<UploadLinkRow>(`${LINK_SELECT} ORDER BY created_at DESC`);
|
const { rows } = await pool.query<UploadLinkRow>(`${LINK_SELECT} ORDER BY created_at DESC`);
|
||||||
res.json(rows);
|
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' });
|
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
|
// Three cases, deliberately distinct. Absent means nobody decided, which
|
||||||
// gets the bounded default. An explicit null means unlimited — a decision
|
// gets the bounded default. An explicit null means unlimited — a decision
|
||||||
// someone made, visible in the request. A number is itself. Reading absent
|
// 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 token = generateToken();
|
||||||
const { rows } = await pool.query<UploadLinkRow>(
|
const { rows } = await pool.query<UploadLinkRow>(
|
||||||
`INSERT INTO upload_links (label, token_hash, max_submissions)
|
`INSERT INTO upload_links (label, token_hash, max_submissions, contact_email)
|
||||||
VALUES ($1, $2, $3)
|
VALUES ($1, $2, $3, $4)
|
||||||
RETURNING id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at`,
|
RETURNING id, label, contact_email, revoked_at, submission_count, max_submissions, last_used_at, created_at`,
|
||||||
[label, hashToken(token), maxSubmissions]
|
[label, hashToken(token), maxSubmissions, email]
|
||||||
);
|
);
|
||||||
const link = requireRow(rows, 'the upload_links INSERT');
|
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
|
// outbound link is built from. Absent in local development, which yields a
|
||||||
// relative URL the admin screen can still show and copy usefully.
|
// relative URL the admin screen can still show and copy usefully.
|
||||||
const base = trimTrailingSlashes(process.env.PUBLIC_URL ?? '');
|
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 }
|
||||||
|
});
|
||||||
}));
|
}));
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -30,6 +30,7 @@ describe('GET /api/admin/email-templates', () => {
|
|||||||
'favoriteWithdrawn',
|
'favoriteWithdrawn',
|
||||||
'intakeDraft',
|
'intakeDraft',
|
||||||
'passwordReset',
|
'passwordReset',
|
||||||
|
'uploadLink',
|
||||||
'verification'
|
'verification'
|
||||||
]);
|
]);
|
||||||
|
|
||||||
|
|||||||
@@ -34,7 +34,7 @@ async function storedFiles(): Promise<string[]> {
|
|||||||
async function issueLink(label = 'Sarah', maxSubmissions?: number | null): Promise<string> {
|
async function issueLink(label = 'Sarah', maxSubmissions?: number | null): Promise<string> {
|
||||||
const res = await request(app)
|
const res = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.post('/api/admin/upload-links')
|
||||||
.send({ label, ...(maxSubmissions === undefined ? {} : { maxSubmissions }) });
|
.send({ label, email: 'sarah@example.com', ...(maxSubmissions === undefined ? {} : { maxSubmissions }) });
|
||||||
expect(res.status).toBe(201);
|
expect(res.status).toBe(201);
|
||||||
return res.body.token as string;
|
return res.body.token as string;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -20,7 +20,9 @@ afterAll(async () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
async function makeLink(): Promise<string> {
|
async function makeLink(): Promise<string> {
|
||||||
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);
|
expect(res.status).toBe(201);
|
||||||
return res.body.token;
|
return res.body.token;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -16,7 +16,7 @@ describe('issuing an upload link', () => {
|
|||||||
it('returns the token exactly once, at creation', async () => {
|
it('returns the token exactly once, at creation', async () => {
|
||||||
const created = await request(app)
|
const created = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.post('/api/admin/upload-links')
|
||||||
.send({ label: 'Sarah' });
|
.send({ label: 'Sarah', email: 'sarah@example.com' });
|
||||||
|
|
||||||
expect(created.status).toBe(201);
|
expect(created.status).toBe(201);
|
||||||
expect(created.body.label).toBe('Sarah');
|
expect(created.body.label).toBe('Sarah');
|
||||||
@@ -34,7 +34,7 @@ describe('issuing an upload link', () => {
|
|||||||
it('stores the digest rather than the token', async () => {
|
it('stores the digest rather than the token', async () => {
|
||||||
const created = await request(app)
|
const created = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.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 }>(
|
const { rows } = await pool.query<{ token_hash: string }>(
|
||||||
`SELECT token_hash FROM upload_links`
|
`SELECT token_hash FROM upload_links`
|
||||||
@@ -51,7 +51,7 @@ describe('issuing an upload link', () => {
|
|||||||
it('refuses a non-positive submission cap', async () => {
|
it('refuses a non-positive submission cap', async () => {
|
||||||
const res = await request(app)
|
const res = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.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);
|
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
|
// safe. An unbounded link should be something asked for, not something that
|
||||||
// happens when nobody thought about it.
|
// happens when nobody thought about it.
|
||||||
it('bounds a link that was created without a cap', async () => {
|
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.status).toBe(201);
|
||||||
expect(res.body.max_submissions).toBe(25);
|
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 () => {
|
it('allows unlimited when it is asked for explicitly', async () => {
|
||||||
const res = await request(app)
|
const res = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.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.status).toBe(201);
|
||||||
expect(res.body.max_submissions).toBeNull();
|
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 () => {
|
it('stamps revoked_at and reports it in the listing', async () => {
|
||||||
const created = await request(app)
|
const created = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.post('/api/admin/upload-links')
|
||||||
.send({ label: 'Temporary' });
|
.send({ label: 'Temporary', email: 'sarah@example.com' });
|
||||||
|
|
||||||
const revoked = await request(app)
|
const revoked = await request(app)
|
||||||
.post(`/api/admin/upload-links/${created.body.id}/revoke`);
|
.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 () => {
|
it('is idempotent, keeping the original timestamp', async () => {
|
||||||
const created = await request(app)
|
const created = await request(app)
|
||||||
.post('/api/admin/upload-links')
|
.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 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`);
|
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);
|
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');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user