5c85e0c0e4
* fix(guests): surface duplicate guest registrations, and stop making so many (#1210) Guest registration always inserts. A client whose token expired — or who opens the gallery on a second device — becomes a new gallery_guests row, and their likes and favourites split across the copies. The photographer's 'final selection' is then only trustworthy if somebody notices two Tinas with half the picks each. Two halves, neither of which touches the registration path. **Say which rows are the same person.** Merging already worked, endpoint and UI both; nothing said WHICH rows to merge. The guests list now marks each row with the others sharing its email and returns a count for the banner, and the admin list offers the group straight to the merge mode that already exists. Case-folded and trimmed, because the same person types Tina@ one day and tina@ the next and both read as distinct rows. Email only — two guests called Anna are not evidence of anything, and rows without an email are not grouped at all since require_name_email is off by default and a shared link produces plenty of them. It preselects rather than merges: which row survives decides the name and verification state the merged guest keeps, and that is the admin's call. **Create fewer of them.** The guest token was 24h and every call site took that default, so even the same browser lost its identity after a day of inactivity. Now 30 days, GUEST_TOKEN_TTL to override. A guest token is scoped to one event, carries no admin capability, and the gallery is already behind whatever protects it — 30 days is the shape of a real proofing cycle. Deliberately NOT done: reusing a guest row when a typed email matches, which the report suggests first. It would let anyone who knows an address inherit that person's identity and selections, and answering differently for a known email would leak which addresses are in the gallery — the thing /guest/recover already goes out of its way to avoid. Prevention at the entry path needs the verification round-trip, which is a separate decision about friction. 13 tests; 8 of the 9 backend ones fail without the change. The frontend ones caught a real bug while being written — the new useMemo sat after the loading early-return, so the hook count changed between renders. * fix(guests): merge must not strand a pending invite (#1210) Three findings from external review of #1216. **A merge could kill an emailed invite link.** Creating an invite inserts a real gallery_guests row, so an admin who pre-mints one and then sees the guest self-register has two rows sharing an email — which this feature now points out and offers to merge. Redemption resolves guest_invites.guest_id with is_deleted: false, so merging soft-deleted the row the link pointed at: the client got 404 guest_missing while the invite dialog still showed the invite as Pending. Nothing anywhere said the link was dead. Unredeemed, unrevoked invites now move to the survivor first. Spent ones stay put — a redeemed invite records who redeemed what, and retargeting it would rewrite that. **The preselection silently chose the survivor.** performMerge keeps mergeSelection[0], and the group was handed over in API order, which is newest-first — so Review then Merge discarded an older, email-verified row holding most of the picks in favour of a fresh re-registration. The proposal is now ordered deliberately: verified first, then whoever holds the most feedback, then the oldest. Still only a proposal, and the confirmation now names the survivor by email as well as name, because duplicates share a name and 'Merge 2 guests into Tina?' said nothing. **duplicate_of was quadratic.** Every row carried the other n-1 ids, so a group of n serialised n² of them — and nothing consumed the list: the UI asked only whether a row was in a group, then regrouped by email itself. Replaced with duplicate_group, the normalised email, which keeps the payload linear and the case/whitespace folding in one place instead of reimplemented on the client. Two new backend tests for the invite paths, one frontend test asserting the merge call keeps the verified row. The invite test fails against the un-fixed code. * fix(guests): keep guest-controlled input out of who survives a merge (#1210) Round 2 of external review on #1216. **The survivor ranking used an attacker-controlled signal.** Preferring whoever holds the most feedback looked like the obvious tiebreak and is exactly the wrong one: registration does not verify the address, so anyone who knows a guest's email can register with it, mark enough photos to out-rank the real person, and be preselected as the survivor. An admin accepting a confirmation between two rows with the same name and email would then move the victim's picks onto an identity whose token the visitor still holds. distinct_photos is guest-controlled and has no business deciding this. The ranking is now email_verified_at then created_at — both server-set. **A merge could make the survivor unrecoverable.** Rows are grouped with case and whitespace folded out, so a merge can be proposed between tina@example.com and Tina@Example.com. /guest/recover lowercases what the guest types and then matches on equality, so a survivor left holding the raw value can never be recovered by email again. The kept row's address is now canonicalised during the merge. Both write paths normalise today, so this covers rows that predate that — which are exactly the rows case-folded grouping surfaces. Two more backend tests. The residual, stated plainly: an admin can still merge two unverified rows in either order. What is gone is the tool ranking them by something a visitor controls. * fix(compose): pass GUEST_TOKEN_TTL through to the backend (#1210) The override was documented in .env.example and could never take effect: the backend service takes an explicit environment list, so a variable not named there never reaches the container. An operator following the documentation would have shortened the guest session and seen nothing change. docker-compose.production.yml uses env_file: .env and already passed it through; docker-compose.dev.yml is gitignored, so only this file needs it. * fix(guests): the admin picks the merge survivor, the tool does not (#1210) Fourth review round on the same point, and the right conclusion is that there is no correct automatic answer. Every rule tried was wrong somewhere. Most-feedback is guest-controlled — the address is never verified at registration, so anyone who knows it can register and mark photos until they out-rank the real person. Oldest-first, the replacement, is worse for the ordinary case: when a token expires the OLD row is the dead identity and the new one is the visitor's live session, so keeping the oldest deletes the identity they are actually using, and the frontend holds that deleted guest in sessionStorage without clearing it on a 401. Registration timing is visitor-controlled too. The data does not say which row is really the person. So the UI asks: merge mode gains a Keep column, the button stays disabled until a row is nominated, and only rows included in the merge can be nominated. The group is still preselected — finding the duplicates was always the point — but nothing about who survives is decided by sort order any more. This also makes the claim in the PR description true. It said the admin decides which row survives; until now the preselection quietly decided it for them. Two rewritten frontend tests: the merge is blocked until a survivor is chosen and then keeps exactly that row, and a row outside the group cannot be nominated. The test i18n mock now interpolates, so aria-labels are queryable by their rendered text. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
230 lines
9.8 KiB
JavaScript
230 lines
9.8 KiB
JavaScript
/**
|
|
* Spotting one person registered twice (#1210).
|
|
*
|
|
* Guest registration always inserts. A client whose token expired, or who
|
|
* opens the gallery on a second device, becomes a new gallery_guests row and
|
|
* their likes and favourites split across the copies — so the photographer's
|
|
* "final selection" is only trustworthy after someone notices two Tinas with
|
|
* half the picks each and merges them.
|
|
*
|
|
* Merging already worked. This is the half that was missing: saying which rows
|
|
* are the same person, so the admin does not have to find them by eye.
|
|
*
|
|
* Detection only — the registration path is deliberately untouched. Reusing a
|
|
* row because someone typed a matching email would let anyone who knows that
|
|
* email inherit the identity and its selections, and answering differently for
|
|
* a known email would leak which addresses are in the gallery, which is
|
|
* exactly what /guest/recover goes out of its way to avoid.
|
|
*/
|
|
|
|
const request = require('supertest');
|
|
const express = require('express');
|
|
|
|
const { bootCrmDb, seedMinimal } = require('./helpers/crmDb');
|
|
|
|
describe('duplicate guest detection (#1210)', () => {
|
|
let db; let cleanup; let app; let eventId;
|
|
|
|
const listGuests = async () => {
|
|
const res = await request(app).get(`/api/admin/events/${eventId}/guests`);
|
|
expect(res.status).toBe(200);
|
|
return res.body;
|
|
};
|
|
|
|
const addGuest = async (name, email, extra = {}) => {
|
|
const [g] = await db('gallery_guests').insert({
|
|
event_id: eventId, name, email,
|
|
identifier: `id-${name}-${Math.random()}`,
|
|
created_at: new Date().toISOString(),
|
|
last_seen_at: new Date().toISOString(),
|
|
is_deleted: false,
|
|
...extra,
|
|
}).returning('id');
|
|
return typeof g === 'object' ? g.id : g;
|
|
};
|
|
|
|
const byId = (body, id) => body.guests.find((g) => g.id === id);
|
|
|
|
beforeAll(async () => {
|
|
jest.resetModules();
|
|
jest.doMock('../../src/middleware/auth', () => ({
|
|
adminAuth: (req, _res, next) => { req.admin = { id: 1, username: 'tester' }; next(); },
|
|
}));
|
|
jest.doMock('../../src/middleware/permissions', () => ({
|
|
requirePermission: () => (_req, _res, next) => next(),
|
|
}));
|
|
jest.doMock('../../src/middleware/ownership', () => ({
|
|
requireEventOwnership: (_req, _res, next) => next(),
|
|
}));
|
|
jest.doMock('../../src/utils/logger', () => ({
|
|
debug: jest.fn(), info: jest.fn(), warn: jest.fn(), error: jest.fn(),
|
|
}));
|
|
|
|
({ db, cleanup } = await bootCrmDb());
|
|
await seedMinimal(db);
|
|
|
|
const [ev] = await db('events').insert({
|
|
slug: 'dupe-guests', event_type: 'wedding', event_name: 'Dupe Guests',
|
|
event_date: '2026-08-01', host_email: 'h@example.com', admin_email: 'a@example.com',
|
|
password_hash: 'x', share_link: '/gallery/dupe-guests/share',
|
|
expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(),
|
|
is_active: 1, is_archived: 0, is_draft: 0, created_at: new Date().toISOString(),
|
|
}).returning('id');
|
|
eventId = typeof ev === 'object' ? ev.id : ev;
|
|
|
|
app = express();
|
|
app.use(express.json());
|
|
app.use('/api/admin', require('../../src/routes/adminGuests'));
|
|
}, 180000);
|
|
|
|
afterAll(async () => { if (cleanup) await cleanup(); });
|
|
|
|
beforeEach(async () => {
|
|
await db('gallery_guests').where({ event_id: eventId }).del();
|
|
});
|
|
|
|
it('groups duplicates under a shared key', async () => {
|
|
const first = await addGuest('Tina', 'tina@example.com');
|
|
const second = await addGuest('Tina', 'tina@example.com');
|
|
const other = await addGuest('Marc', 'marc@example.com');
|
|
|
|
const body = await listGuests();
|
|
|
|
// A shared group key rather than a list of sibling ids: the payload stays
|
|
// linear in the number of guests, and the case/whitespace folding lives in
|
|
// one place instead of being reimplemented on the client.
|
|
expect(byId(body, first).duplicate_group).toBe('tina@example.com');
|
|
expect(byId(body, second).duplicate_group).toBe('tina@example.com');
|
|
expect(byId(body, other).duplicate_group).toBeNull();
|
|
});
|
|
|
|
it('counts the groups and the rows in them', async () => {
|
|
await addGuest('Tina', 'tina@example.com');
|
|
await addGuest('Tina', 'tina@example.com');
|
|
await addGuest('Ben', 'ben@example.com');
|
|
await addGuest('Ben', 'ben@example.com');
|
|
await addGuest('Ben', 'ben@example.com');
|
|
await addGuest('Marc', 'marc@example.com');
|
|
|
|
expect((await listGuests()).duplicates).toEqual({ groups: 2, guests: 5 });
|
|
});
|
|
|
|
it('matches the same address typed with different capitals or a stray space', async () => {
|
|
// The same person on a different day. Both read as distinct rows in the
|
|
// admin list, which is precisely why they need catching here.
|
|
const a = await addGuest('Tina', 'tina@example.com');
|
|
const b = await addGuest('Tina', 'Tina@Example.com ');
|
|
|
|
const body = await listGuests();
|
|
expect(byId(body, a).duplicate_group).toBe('tina@example.com');
|
|
expect(byId(body, b).duplicate_group).toBe('tina@example.com');
|
|
expect(body.duplicates).toEqual({ groups: 1, guests: 2 });
|
|
});
|
|
|
|
it('does not treat two guests without an email as the same person', async () => {
|
|
// require_name_email is off by default, so a shared gallery link produces
|
|
// plenty of these. Grouping them would merge strangers.
|
|
const a = await addGuest('Anon', null);
|
|
const b = await addGuest('Anon', null);
|
|
|
|
const body = await listGuests();
|
|
expect(byId(body, a).duplicate_group).toBeNull();
|
|
expect(byId(body, b).duplicate_group).toBeNull();
|
|
expect(body.duplicates).toEqual({ groups: 0, guests: 0 });
|
|
});
|
|
|
|
it('does not treat a shared name as evidence of anything', async () => {
|
|
const a = await addGuest('Anna', 'anna.k@example.com');
|
|
const b = await addGuest('Anna', 'anna.m@example.com');
|
|
|
|
const body = await listGuests();
|
|
expect(byId(body, a).duplicate_group).toBeNull();
|
|
expect(byId(body, b).duplicate_group).toBeNull();
|
|
});
|
|
|
|
it('moves a pending invite to the survivor instead of stranding it', async () => {
|
|
// Creating an invite inserts a real gallery_guests row, so an admin who
|
|
// pre-mints one and then sees the guest self-register has two rows — and
|
|
// this feature now points that pair out and offers the merge. Redemption
|
|
// resolves guest_invites.guest_id with `is_deleted: false`, so merging
|
|
// without moving the invite leaves the emailed link returning 404
|
|
// guest_missing while the invite dialog still shows it as Pending.
|
|
const placeholder = await addGuest('Tina', 'tina@example.com');
|
|
const selfRegistered = await addGuest('Tina', 'tina@example.com');
|
|
const [inv] = await db('guest_invites').insert({
|
|
event_id: eventId, guest_id: placeholder, token: 'invite-token-1',
|
|
created_by_admin_id: 1, created_at: new Date().toISOString(),
|
|
}).returning('id');
|
|
const inviteId = typeof inv === 'object' ? inv.id : inv;
|
|
|
|
const res = await request(app)
|
|
.post(`/api/admin/events/${eventId}/guests/${selfRegistered}/merge`)
|
|
.send({ mergeIds: [placeholder] });
|
|
expect(res.status).toBe(200);
|
|
|
|
const invite = await db('guest_invites').where({ id: inviteId }).first();
|
|
expect(invite.guest_id).toBe(selfRegistered);
|
|
// And it still resolves: the survivor is not soft-deleted.
|
|
const target = await db('gallery_guests').where({ id: invite.guest_id }).first();
|
|
expect(Boolean(target.is_deleted)).toBe(false);
|
|
});
|
|
|
|
it('leaves a spent invite pointing at what it actually redeemed', async () => {
|
|
// A redeemed invite is a record of who redeemed what. Retargeting it would
|
|
// rewrite that history to name a guest who was never on the other end.
|
|
const old = await addGuest('Tina', 'tina@example.com');
|
|
const kept = await addGuest('Tina', 'tina@example.com');
|
|
const [inv] = await db('guest_invites').insert({
|
|
event_id: eventId, guest_id: old, token: 'invite-token-2',
|
|
created_by_admin_id: 1, redeemed_at: new Date().toISOString(),
|
|
created_at: new Date().toISOString(),
|
|
}).returning('id');
|
|
const inviteId = typeof inv === 'object' ? inv.id : inv;
|
|
|
|
await request(app)
|
|
.post(`/api/admin/events/${eventId}/guests/${kept}/merge`)
|
|
.send({ mergeIds: [old] });
|
|
|
|
expect((await db('guest_invites').where({ id: inviteId }).first()).guest_id).toBe(old);
|
|
});
|
|
|
|
it('canonicalises the survivor\'s address so recovery can still find them', async () => {
|
|
// Grouping folds case and whitespace, so a merge can be proposed between a
|
|
// clean address and a legacy one that is not. /guest/recover lowercases
|
|
// what the guest types and matches on equality, so a survivor left holding
|
|
// the raw value becomes permanently unrecoverable by email.
|
|
const legacy = await addGuest('Tina', 'Tina@Example.com ');
|
|
const other = await addGuest('Tina', 'tina@example.com');
|
|
|
|
const res = await request(app)
|
|
.post(`/api/admin/events/${eventId}/guests/${legacy}/merge`)
|
|
.send({ mergeIds: [other] });
|
|
expect(res.status).toBe(200);
|
|
|
|
expect((await db('gallery_guests').where({ id: legacy }).first()).email).toBe('tina@example.com');
|
|
});
|
|
|
|
it('leaves an already-canonical survivor untouched', async () => {
|
|
const kept = await addGuest('Tina', 'tina@example.com');
|
|
const dupe = await addGuest('Tina', 'Tina@Example.com');
|
|
|
|
await request(app)
|
|
.post(`/api/admin/events/${eventId}/guests/${kept}/merge`)
|
|
.send({ mergeIds: [dupe] });
|
|
|
|
expect((await db('gallery_guests').where({ id: kept }).first()).email).toBe('tina@example.com');
|
|
});
|
|
|
|
it('ignores a removed guest', async () => {
|
|
const kept = await addGuest('Tina', 'tina@example.com');
|
|
await addGuest('Tina', 'tina@example.com', { is_deleted: true });
|
|
|
|
const body = await listGuests();
|
|
// The deleted row is not listed at all, so the survivor is not a duplicate
|
|
// of something the admin cannot see or merge.
|
|
expect(body.guests.map((g) => g.id)).toEqual([kept]);
|
|
expect(byId(body, kept).duplicate_group).toBeNull();
|
|
expect(body.duplicates).toEqual({ groups: 0, guests: 0 });
|
|
});
|
|
});
|