diff --git a/public/js/admin.js b/public/js/admin.js index 622710c6..c83947d1 100644 --- a/public/js/admin.js +++ b/public/js/admin.js @@ -1765,15 +1765,20 @@ initImageSettings(); .catch(function(err) { showToast(err.message, 'error'); }); } + // Used, or expired without being redeemed. A revoked code keeps its row: it + // records a decision somebody took, and it is not cluttering anything the way + // a pile of expired codes does. + var SPENT_STATUS = ['used', 'expired']; + // Spent invitations in one go, which is what a cluttered list actually wants. // Confirmed first: it is a delete, even if everything it removes is finished. function clearUsedInvites() { - showConfirm('Delete every used invitation? Unused and revoked ones are kept.', function() { - fetch('/api/admin/invites/used', { method: 'DELETE', headers: getAuthHeaders() }) + showConfirm('Delete every used and expired invitation? Live and revoked ones are kept.', function() { + fetch('/api/admin/invites/spent', { method: 'DELETE', headers: getAuthHeaders() }) .then(function(r) { return r.json(); }) .then(function(data) { if (!data.success) throw new Error(data.error || 'Could not clear them'); - showToast('Removed ' + data.removed + ' used invitation' + (data.removed === 1 ? '' : 's'), 'success'); + showToast('Removed ' + data.removed + ' spent invitation' + (data.removed === 1 ? '' : 's'), 'success'); loadInvites(); }) .catch(function(err) { showToast(err.message, 'error'); }); @@ -1799,21 +1804,22 @@ initImageSettings(); '' + esc(row.note || '') + '' + '' + esc(when) + who + '' + (row.status === 'active' ? '' : '') + - // Delete is offered on spent codes only. One that has not been used may - // still be in somebody's inbox: taking it off this list would not take - // it out of their hands, and nothing would then say who held it. - // Revoking is what stops a live code, and it leaves the row behind. - (row.status === 'used' - ? '' + // Delete is offered on spent codes only — used, or expired unredeemed. + // One that could still be redeemed may be sitting in somebody's inbox: + // taking it off this list would not take it out of their hands, and + // nothing would then say who held it. Revoking is what stops a live + // code, and it leaves the row behind, marked. + (SPENT_STATUS.indexOf(row.status) !== -1 + ? '' : '') + ''; }).join(''); - var used = rows.filter(function(row) { return row.status === 'used'; }).length; + var spent = rows.filter(function(row) { return SPENT_STATUS.indexOf(row.status) !== -1; }).length; var clear = document.getElementById('btn-clear-used-invites'); if (clear) { - clear.hidden = used === 0; - clear.textContent = 'Clear ' + used + ' used'; + clear.hidden = spent === 0; + clear.textContent = 'Clear ' + spent + ' spent'; } } } diff --git a/src/routes/adminConfig.js b/src/routes/adminConfig.js index 57b67dad..7845692b 100644 --- a/src/routes/adminConfig.js +++ b/src/routes/adminConfig.js @@ -739,16 +739,16 @@ router.post('/invites/:id/revoke', async function(req, res) { } catch (e) { return serverError(res, 'Invite revoke', e, 'Could not revoke the invitation'); } }); -// Clearing away spent invitations. Used ones only — an unused code may still be -// in somebody's inbox, and deleting the row takes it off the list without taking -// it out of their hands, leaving nothing to say who held it. Revoke is what -// stops a live code, and it leaves the row behind, marked. -router.delete('/invites/used', async function(req, res) { +// Clearing away spent invitations — used, or expired without being used. A code +// that could still be redeemed is never deleted: that would take it off the list +// without taking it out of anybody's inbox, leaving nothing to say who held it. +// Revoke is what stops a live code, and it leaves the row behind, marked. +router.delete('/invites/spent', async function(req, res) { try { - var removed = await invites.removeUsed(); - logger.audit(req.user.id, 'invite_delete', 'Cleared ' + removed + ' used invitations', req, { category: 'admin' }); + var removed = await invites.removeSpent(); + logger.audit(req.user.id, 'invite_delete', 'Cleared ' + removed + ' spent invitations', req, { category: 'admin' }); res.json({ success: true, removed: removed }); - } catch (e) { return serverError(res, 'Invite clear', e, 'Could not clear used invitations'); } + } catch (e) { return serverError(res, 'Invite clear', e, 'Could not clear spent invitations'); } }); router.delete('/invites/:id', async function(req, res) { @@ -756,7 +756,7 @@ router.delete('/invites/:id', async function(req, res) { if (!await invites.remove(req.params.id)) { // Said plainly rather than as "not found": the row is very likely there, // and the reason it cannot go is worth knowing. - return res.status(409).json({ error: 'Only a used invitation can be deleted. Revoke it instead.' }); + return res.status(409).json({ error: 'That invitation can still be used. Revoke it instead.' }); } logger.audit(req.user.id, 'invite_delete', 'Deleted invitation ' + req.params.id, req, { category: 'admin' }); res.json({ success: true }); diff --git a/src/utils/registrationInvites.js b/src/utils/registrationInvites.js index 1fe20ed2..9e7ab565 100644 --- a/src/utils/registrationInvites.js +++ b/src/utils/registrationInvites.js @@ -89,28 +89,33 @@ async function revoke(id, adminUserId) { return result.changes > 0; } +// Spent: used, or run out of time without being used. The one thing never +// deletable is a code that could still be redeemed — deleting that takes it off +// the list without taking it out of anybody's inbox, so the holder keeps +// something that looks valid, it quietly stops working, and nothing is left to +// say who had it. Revoke is what stops a live code, and it leaves the row. +// +// Written to match the status the list shows rather than as +// "used OR expires_at <= NOW()", which would also catch a revoked code whose +// date had passed — one the screen still labels revoked, with no delete on it. +var SPENT = '(used_at IS NOT NULL OR (revoked_at IS NULL AND expires_at <= NOW()))'; + /** * Delete a spent invitation. * - * Used codes only. A code that has not been used yet is one somebody may still - * be holding: deleting it takes it out of the list without taking it out of - * their inbox, and there is then no record of who it went to or why it stopped - * working. Revoke does that job — it leaves the row, marked. This is only for - * clearing away codes whose whole story is already told. - * - * Returns false for a code that is not spent, which the caller reports rather + * Returns false for one that is still live, which the caller reports rather * than treating as a missing row. */ async function remove(id) { var result = await db().run( - 'DELETE FROM registration_invites WHERE id = $1 AND used_at IS NOT NULL', [id]); + 'DELETE FROM registration_invites WHERE id = $1 AND ' + SPENT, [id]); return result.changes > 0; } // Every spent invitation at once, which is what "they clutter the list" asks -// for. Same rule: nothing unused is touched. -async function removeUsed() { - var result = await db().run('DELETE FROM registration_invites WHERE used_at IS NOT NULL'); +// for. Same rule: nothing still redeemable is touched. +async function removeSpent() { + var result = await db().run('DELETE FROM registration_invites WHERE ' + SPENT); return result.changes || 0; } @@ -153,7 +158,7 @@ module.exports = { list, revoke, remove, - removeUsed, + removeSpent, claim, inviteOnly }; diff --git a/test/backend-hardening.test.js b/test/backend-hardening.test.js index 8d57d02d..471ba477 100644 --- a/test/backend-hardening.test.js +++ b/test/backend-hardening.test.js @@ -153,28 +153,34 @@ test('.env.example documents every variable the app reads', () => { // registration_enabled is open-or-closed. Invite-only is the middle setting, // and it has to hold up against someone probing codes. -test('an invitation can only be deleted once it has been used', () => { +test('an invitation is deletable only once it can no longer be used', () => { const src = read('src/utils/registrationInvites.js'); const route = read('src/routes/adminConfig.js'); - // An unused code may still be in somebody's inbox. Deleting the row takes it - // off the list without taking it out of their hands, and nothing is then left - // to say who held it or why it stopped working. Revoke does that job and - // leaves the row behind, marked. - assert.match(src, /DELETE FROM registration_invites WHERE id = \$1 AND used_at IS NOT NULL/); - assert.match(src, /DELETE FROM registration_invites WHERE used_at IS NOT NULL/, 'and in bulk'); + // A code that could still be redeemed may be sitting in somebody's inbox. + // Deleting the row takes it off the list without taking it out of their + // hands: it quietly stops working and nothing is left to say who held it. + // Revoke does that job and leaves the row behind, marked. + assert.match(src, /var SPENT = '\(used_at IS NOT NULL OR \(revoked_at IS NULL AND expires_at <= NOW\(\)\)\)';/); + assert.match(src, /DELETE FROM registration_invites WHERE id = \$1 AND ' \+ SPENT/); + assert.match(src, /DELETE FROM registration_invites WHERE ' \+ SPENT/, 'and in bulk'); + // Written to match the status the list shows: "used OR expired" would also + // catch a revoked code past its date, which the screen still calls revoked + // and offers no delete on. + assert.match(src, /revoked_at IS NULL AND expires_at <= NOW\(\)/); // Refused with the reason, not as a missing row: the row is very likely there. - assert.match(route, /Only a used invitation can be deleted\. Revoke it instead\./); + assert.match(route, /That invitation can still be used\. Revoke it instead\./); assert.match(route, /res\.status\(409\)/); - // The bulk route is declared before /invites/:id, or "used" is read as an id. - assert.ok(route.indexOf("router.delete('/invites/used'") < route.indexOf("router.delete('/invites/:id'")); + // The bulk route is declared before /invites/:id, or "spent" is read as an id. + assert.ok(route.indexOf("router.delete('/invites/spent'") < route.indexOf("router.delete('/invites/:id'")); // And the button is only offered where it can work. const js = read('public/js/admin.js'); - assert.match(js, /row\.status === 'used'\s*\n?\s*\? '