feat(auth): unique, changeable, verifiable email addresses (engagement Phase 1b)
Makes `users.email` unique, de-duplicates the addresses an upgrade will find, and builds the self-service change-and-verify flow that did not exist. The uniqueness index is on a generated `email_norm AS (LOWER(email)) STORED` column under `utf8mb4_bin`, NOT on `email` under a `_ci` collation as the plan specified. Every case-insensitive collation this server offers is also accent-insensitive: `josé@x.com` and `jose@x.com` compare equal, and those are two different mailboxes. The plan's index would have refused the second address forever and the de-duplication would have nulled a legitimate account's. A requested address is STAGED in `email_pending` and only a tokened link installs it, so a typo cannot silently redirect account-recovery mail. `isDuplicateUsername()` now distinguishes the two indexes. All five call sites branch on it; each answers differently on purpose, because a public form, an IdP callback, a half-completed invite and an admin screen do not owe the same person the same amount of truth. SSO reads the IdP's actual `email_verified`/`verified` claim instead of inferring verification from an address merely being present. Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
194
server/test/emailCollisionSurfaces.test.js
Normal file
194
server/test/emailCollisionSurfaces.test.js
Normal file
@@ -0,0 +1,194 @@
|
||||
// Engagement Phase 1b — how each of the five write paths answers a duplicate
|
||||
// EMAIL, now that `users` has two unique indexes.
|
||||
//
|
||||
// Before this phase every one of them either misreported the collision as a
|
||||
// username clash or fell through to an opaque 500. The answers are deliberately
|
||||
// NOT uniform, and the differences are the point:
|
||||
//
|
||||
// register generic 400, unscored — a public form; the truth would make
|
||||
// account existence queryable, and
|
||||
// scoring an honest typo would push a
|
||||
// real user toward an IP ban
|
||||
// SSO provision stops, names the reason — the caller already authenticated
|
||||
// with the IdP; retrying usernames can
|
||||
// never clear an email conflict
|
||||
// invite accept 409, explains — the invitee has already clicked a
|
||||
// link and typed a password
|
||||
// admin create 409, names the field — an admin can already list every
|
||||
// admin update 409, names the field account, so there is nothing to leak
|
||||
process.env.DB_HOST = '127.0.0.1'
|
||||
process.env.DB_PORT = '59999'
|
||||
|
||||
const { test, after, beforeEach, afterEach } = require('node:test')
|
||||
const assert = require('node:assert/strict')
|
||||
|
||||
const authCtrl = require('../src/router/v1/auth/auth.controller')
|
||||
const inviteCtrl = require('../src/router/v1/auth/invite.controller')
|
||||
const adminCtrl = require('../src/router/v1/admin/admin.controller')
|
||||
const users = require('../src/model/users/users.model')
|
||||
const invites = require('../src/model/invites/invites.model')
|
||||
const settings = require('../src/model/settings/settings.model')
|
||||
const activity = require('../src/model/activity/activity.model')
|
||||
const botScore = require('../src/middleware/botScore')
|
||||
const loginProtection = require('../src/middleware/loginProtection')
|
||||
const db = require('../src/utils/db')
|
||||
|
||||
after(() => db.close())
|
||||
|
||||
function mockRes() {
|
||||
return {
|
||||
statusCode: 200,
|
||||
body: null,
|
||||
status(c) {
|
||||
this.statusCode = c
|
||||
return this
|
||||
},
|
||||
json(b) {
|
||||
this.body = b
|
||||
return this
|
||||
},
|
||||
cookie() {
|
||||
return this
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
const patched = []
|
||||
function stub(obj, name, fn) {
|
||||
patched.push([obj, name, obj[name]])
|
||||
obj[name] = fn
|
||||
}
|
||||
afterEach(() => {
|
||||
while (patched.length) {
|
||||
const [obj, name, fn] = patched.pop()
|
||||
obj[name] = fn
|
||||
}
|
||||
})
|
||||
|
||||
// The exact error the mariadb connector raises for each index, captured from
|
||||
// MariaDB 11.8. Note the address is inside the message: that is why none of these
|
||||
// paths may echo it.
|
||||
function dupEmailError(value = 'taken@example.com') {
|
||||
const err = new Error(
|
||||
`(conn:60, no: 1062, SQLState: 23000) Duplicate entry '${value}' for key 'uq_users_email_norm'\n` +
|
||||
`sql: INSERT INTO users ... - parameters:['someone','${value}']`,
|
||||
)
|
||||
err.code = 'ER_DUP_ENTRY'
|
||||
err.errno = 1062
|
||||
err.sqlMessage = `Duplicate entry '${value}' for key 'uq_users_email_norm'`
|
||||
return err
|
||||
}
|
||||
|
||||
function dupUsernameError() {
|
||||
const err = new Error("(conn:60, no: 1062, SQLState: 23000) Duplicate entry 'someone' for key 'username'")
|
||||
err.code = 'ER_DUP_ENTRY'
|
||||
err.errno = 1062
|
||||
err.sqlMessage = "Duplicate entry 'someone' for key 'username'"
|
||||
return err
|
||||
}
|
||||
|
||||
let scored
|
||||
|
||||
beforeEach(() => {
|
||||
scored = []
|
||||
stub(activity, 'log', async () => {})
|
||||
stub(botScore, 'recordHoneypot', (ip) => scored.push(['honeypot', ip]))
|
||||
stub(botScore, 'recordLoginFailure', (ip) => scored.push(['loginFailure', ip]))
|
||||
stub(loginProtection, 'recordFailure', (ip) => scored.push(['backoff', ip]))
|
||||
})
|
||||
|
||||
// ── register: generic, and NOT scored ──────────────────────────────────────
|
||||
|
||||
test('register answers a duplicate email generically and never says "username"', async () => {
|
||||
stub(settings, 'getRegistrationMode', async () => 'password')
|
||||
stub(users, 'createUser', async () => {
|
||||
throw dupEmailError()
|
||||
})
|
||||
const res = mockRes()
|
||||
await authCtrl.register(
|
||||
{ body: { username: 'newperson', password: 'abcd1234', email: 'taken@example.com' }, ip: '9.9.9.9' },
|
||||
res,
|
||||
)
|
||||
assert.equal(res.statusCode, 400, 'not the 409 a username clash gets - the shape itself must not distinguish')
|
||||
assert.match(res.body.message, /Registration failed/i)
|
||||
assert.doesNotMatch(res.body.message, /username/i, 'must not misattribute to the field they did NOT collide on')
|
||||
assert.doesNotMatch(res.body.message, /email/i, 'and must not confirm the address exists')
|
||||
assert.doesNotMatch(res.body.message, /taken@example\.com/, 'the address must never come back')
|
||||
})
|
||||
|
||||
test('a duplicate email at register feeds NOTHING to the bot scorer or the backoff', async () => {
|
||||
stub(settings, 'getRegistrationMode', async () => 'password')
|
||||
stub(users, 'createUser', async () => {
|
||||
throw dupEmailError()
|
||||
})
|
||||
await authCtrl.register(
|
||||
{ body: { username: 'newperson', password: 'abcd1234', email: 'taken@example.com' }, ip: '9.9.9.9' },
|
||||
mockRes(),
|
||||
)
|
||||
// A legitimate user typing a colleague's address is not an attacker. Scoring
|
||||
// this would walk them toward an automatic IP ban for an honest mistake.
|
||||
assert.deepEqual(scored, [], 'no bot score, no backoff')
|
||||
})
|
||||
|
||||
test('register still reports a genuine username clash as 409', async () => {
|
||||
stub(settings, 'getRegistrationMode', async () => 'password')
|
||||
stub(users, 'createUser', async () => {
|
||||
throw dupUsernameError()
|
||||
})
|
||||
const res = mockRes()
|
||||
await authCtrl.register({ body: { username: 'someone', password: 'abcd1234' }, ip: '9.9.9.9' }, res)
|
||||
assert.equal(res.statusCode, 409)
|
||||
assert.match(res.body.message, /username/i)
|
||||
})
|
||||
|
||||
// ── invite accept: survivable, and distinguishable from a username clash ───
|
||||
|
||||
test('invite accept explains a duplicate email instead of blaming the username', async () => {
|
||||
stub(invites, 'findValidByToken', async () => ({ id: 5, email: 'taken@example.com', role: 'player' }))
|
||||
stub(users, 'createUser', async () => {
|
||||
throw dupEmailError()
|
||||
})
|
||||
const res = mockRes()
|
||||
await inviteCtrl.acceptInvite(
|
||||
{ body: { username: 'newperson', password: 'abcd1234' }, params: { token: 't' }, ip: '9.9.9.9' },
|
||||
res,
|
||||
)
|
||||
assert.equal(res.statusCode, 409)
|
||||
assert.match(res.body.message, /email address is already in use/i)
|
||||
assert.doesNotMatch(res.body.message, /username is already taken/i)
|
||||
})
|
||||
|
||||
// ── admin user CRUD: was an opaque 500, now a 409 that names the field ─────
|
||||
|
||||
test('admin createUser answers a duplicate email with 409, not a 500', async () => {
|
||||
stub(users, 'getRawByUsername', async () => null)
|
||||
stub(users, 'createUser', async () => {
|
||||
throw dupEmailError()
|
||||
})
|
||||
const res = mockRes()
|
||||
await adminCtrl.createUser({ body: { username: 'newperson', password: 'abcd1234', email: 'taken@example.com' } }, res)
|
||||
assert.equal(res.statusCode, 409, 'before Phase 1b this had no catch at all and became a 500')
|
||||
assert.match(res.body.message, /email address/i)
|
||||
})
|
||||
|
||||
test('admin updateUser answers a duplicate email with 409, not a 500', async () => {
|
||||
stub(users, 'getById', async () => ({ id: 3, username: 'existing', role: 'player', status: 'active' }))
|
||||
stub(users, 'update', async () => {
|
||||
throw dupEmailError()
|
||||
})
|
||||
const res = mockRes()
|
||||
await adminCtrl.updateUser({ params: { id: '3' }, body: { email: 'taken@example.com' } }, res)
|
||||
assert.equal(res.statusCode, 409)
|
||||
assert.match(res.body.message, /email address/i)
|
||||
})
|
||||
|
||||
test('admin createUser still reports a username clash as a username clash', async () => {
|
||||
stub(users, 'getRawByUsername', async () => null)
|
||||
stub(users, 'createUser', async () => {
|
||||
throw dupUsernameError()
|
||||
})
|
||||
const res = mockRes()
|
||||
await adminCtrl.createUser({ body: { username: 'someone', password: 'abcd1234' } }, res)
|
||||
assert.equal(res.statusCode, 409)
|
||||
assert.match(res.body.message, /Username already taken/i)
|
||||
})
|
||||
Reference in New Issue
Block a user