From 71023c687fafd2c0a30125dccbe4a2c4316bac8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e?= Date: Sat, 23 Nov 2024 15:08:49 +0000 Subject: [PATCH] Fixes to user creation and name change (#660) * Remove beautifyDuplicateKeyError * Translate UNIQUE_CONSTRAINT errors on user signup * Update slug when changing name, fix permissions on username change endpoint --- locale/en.yaml | 2 + src/controllers/authenticate.js | 39 ++++++------ src/controllers/users.js | 21 +++---- src/errors/index.js | 14 +++++ src/plugins/users.js | 64 +++++++++++++------ src/routes/users.js | 3 +- src/utils/beautifyDuplicateKeyError.js | 36 ----------- test/authenticate.mjs | 41 +++++++++++-- test/users.mjs | 85 ++++++++++++++++++++++++++ 9 files changed, 209 insertions(+), 96 deletions(-) delete mode 100644 src/utils/beautifyDuplicateKeyError.js diff --git a/locale/en.yaml b/locale/en.yaml index 9959196f..554a2787 100644 --- a/locale/en.yaml +++ b/locale/en.yaml @@ -8,7 +8,9 @@ uwave: banned: 'You have been banned.' genericPermission: 'You do not have permission to do that.' invalidEmail: The email address is not formatted correctly. + emailInUse: The email address is already in use. invalidUsername: Invalid username. Names must be 3 to 32 characters long and must not contain spaces. + usernameInUse: The username is already in use. recaptchaFailed: 'ReCaptcha validation failed, please try again.' invalidResetToken: That reset token is invalid. Please double-check your reset token or request a new password reset. incorrectPassword: The password is incorrect. diff --git a/src/controllers/authenticate.js b/src/controllers/authenticate.js index 478f2c9b..299d6282 100644 --- a/src/controllers/authenticate.js +++ b/src/controllers/authenticate.js @@ -12,7 +12,6 @@ import { InvalidResetTokenError, UserNotFoundError, } from '../errors/index.js'; -import beautifyDuplicateKeyError from '../utils/beautifyDuplicateKeyError.js'; import toItemResponse from '../utils/toItemResponse.js'; import toListResponse from '../utils/toListResponse.js'; import { serializeUser } from '../utils/serialize.js'; @@ -333,30 +332,26 @@ async function register(req) { grecaptcha, email, username, password, } = req.body; - try { - const recaptchaOptions = req.authOptions.recaptcha; - if (recaptchaOptions && recaptchaOptions.secret) { - if (grecaptcha) { - await verifyCaptcha(grecaptcha, { - secret: recaptchaOptions.secret, - logger: req.log, - }); - } else { - req.log.warn('missing client-side captcha response'); - throw new ReCaptchaError(); - } + const recaptchaOptions = req.authOptions.recaptcha; + if (recaptchaOptions && recaptchaOptions.secret) { + if (grecaptcha) { + await verifyCaptcha(grecaptcha, { + secret: recaptchaOptions.secret, + logger: req.log, + }); + } else { + req.log.warn('missing client-side captcha response'); + throw new ReCaptchaError(); } + } - const user = await users.createUser({ - email, - username, - password, - }); + const user = await users.createUser({ + email, + username, + password, + }); - return toItemResponse(serializeUser(user)); - } catch (error) { - throw beautifyDuplicateKeyError(error); - } + return toItemResponse(serializeUser(user)); } /** diff --git a/src/controllers/users.js b/src/controllers/users.js index 9c484a1a..350f3a36 100644 --- a/src/controllers/users.js +++ b/src/controllers/users.js @@ -9,7 +9,6 @@ import getOffsetPagination from '../utils/getOffsetPagination.js'; import toItemResponse from '../utils/toItemResponse.js'; import toListResponse from '../utils/toListResponse.js'; import toPaginatedResponse from '../utils/toPaginatedResponse.js'; -import beautifyDuplicateKeyError from '../utils/beautifyDuplicateKeyError.js'; import { muteUser, unmuteUser } from './chat.js'; /** @@ -163,17 +162,17 @@ async function changeUsername(req) { const { username } = req.body; const { users } = req.uwave; - try { - const user = await users.updateUser( - id, - { username }, - { moderator }, - ); - - return toItemResponse(user); - } catch (error) { - throw beautifyDuplicateKeyError(error); + if (id !== moderator.id) { + throw new PermissionError(); } + + const user = await users.updateUser( + id, + { username }, + { moderator }, + ); + + return toItemResponse(user); } /** diff --git a/src/errors/index.js b/src/errors/index.js index 69b1865d..c4340929 100644 --- a/src/errors/index.js +++ b/src/errors/index.js @@ -145,12 +145,24 @@ const InvalidEmailError = createErrorClass('InvalidEmailError', { base: UnprocessableEntity, }); +const UsedEmailError = createErrorClass('UsedEmailError', { + code: 'invalid-email', + string: 'errors.emailInUse', + base: UnprocessableEntity, +}); + const InvalidUsernameError = createErrorClass('InvalidUsernameError', { code: 'invalid-username', string: 'errors.invalidUsername', base: UnprocessableEntity, }); +const UsedUsernameError = createErrorClass('UsedUsernameError', { + code: 'invalid-username', + string: 'errors.usernameInUse', + base: UnprocessableEntity, +}); + const ReCaptchaError = createErrorClass('ReCaptchaError', { code: 'recaptcha-failed', string: 'errors.recaptchaFailed', @@ -269,7 +281,9 @@ export { RateLimitError, NameChangeRateLimitError, InvalidEmailError, + UsedEmailError, InvalidUsernameError, + UsedUsernameError, InvalidResetTokenError, ReCaptchaError, IncorrectPasswordError, diff --git a/src/plugins/users.js b/src/plugins/users.js index 5734e545..e00e17ef 100644 --- a/src/plugins/users.js +++ b/src/plugins/users.js @@ -1,11 +1,13 @@ +import { randomUUID } from 'crypto'; import lodash from 'lodash'; +import { sql } from 'kysely'; +import { slugify } from 'transliteration'; import bcrypt from 'bcryptjs'; import Page from '../Page.js'; -import { IncorrectPasswordError, UserNotFoundError } from '../errors/index.js'; -import { slugify } from 'transliteration'; +import { + IncorrectPasswordError, UsedEmailError, UsedUsernameError, UserNotFoundError, +} from '../errors/index.js'; import { jsonGroupArray } from '../utils/sqlite.js'; -import { sql } from 'kysely'; -import { randomUUID } from 'crypto'; const { pick, omit } = lodash; @@ -31,6 +33,24 @@ const avatarColumn = (eb) => eb.fn.coalesce( /** @type {import('kysely').RawBuilder} */ (sql`concat('https://sigil.u-wave.net/', ${eb.ref('users.id')})`), ); +/** + * Translate a SQLite error into a HTTP error explaining the problem. + * + * @param {unknown} err + * @returns {never} + */ +function rethrowInsertError(err) { + if (err instanceof Error && 'code' in err && err.code === 'SQLITE_CONSTRAINT_UNIQUE') { + if (err.message.includes('users.email')) { + throw new UsedEmailError(); + } + if (err.message.includes('users.username') || err.message.includes('users.slug')) { + throw new UsedUsernameError(); + } + } + throw err; +} + class UsersRepository { #uw; @@ -290,7 +310,7 @@ class UsersRepository { return user; } - }); + }).catch(rethrowInsertError); return user; } @@ -307,7 +327,7 @@ class UsersRepository { const hash = await encryptPassword(password); - const user = await db.insertInto('users') + const insert = db.insertInto('users') .values({ id: /** @type {UserID} */ (randomUUID()), username, @@ -325,8 +345,14 @@ class UsersRepository { 'users.pendingActivation', 'users.createdAt', 'users.updatedAt', - ]) - .executeTakeFirstOrThrow(); + ]); + + let user; + try { + user = await insert.executeTakeFirstOrThrow(); + } catch (err) { + rethrowInsertError(err); + } const roles = ['user']; await acl.allow(user, roles); @@ -380,29 +406,27 @@ class UsersRepository { }); Object.assign(user, update); + const derivedUpdates = {}; + if ('username' in update && update.username != null) { + derivedUpdates.slug = slugify(update.username); + } + const updatesFromDatabase = await db.updateTable('users') .where('id', '=', id) - .set(update) - .returning(['username']) - .executeTakeFirst(); + .set({ ...update, ...derivedUpdates }) + .returning(['username', 'slug']) + .executeTakeFirst() + .catch(rethrowInsertError); if (!updatesFromDatabase) { throw new UserNotFoundError({ id }); } Object.assign(user, updatesFromDatabase); - // Take updated keys from the Model again, - // as it may apply things like Unicode normalization on the values. - Object.keys(update).forEach((key) => { - // @ts-expect-error Infeasible to statically check properties here - // Hopefully the caller took care - update[key] = user[key]; - }); - this.#uw.publish('user:update', { userID: user.id, moderatorID: moderator ? moderator.id : null, old, - new: update, + new: updatesFromDatabase, }); return user; diff --git a/src/routes/users.js b/src/routes/users.js index f278862f..d1a212bd 100644 --- a/src/routes/users.js +++ b/src/routes/users.js @@ -60,6 +60,7 @@ function userRoutes() { // PUT /users/:id/username - Change a user's username. .put( '/:id/username', + protect(), schema(validations.setUserName), rateLimit('change-username', { max: 5, @@ -68,7 +69,7 @@ function userRoutes() { }), route(controller.changeUsername), ) - // PUT /users/:id/avatar - Change a user's username. + // PUT /users/:id/avatar - Change a user's avatar. .put( '/:id/avatar', protect(), diff --git a/src/utils/beautifyDuplicateKeyError.js b/src/utils/beautifyDuplicateKeyError.js deleted file mode 100644 index 73ad8cf7..00000000 --- a/src/utils/beautifyDuplicateKeyError.js +++ /dev/null @@ -1,36 +0,0 @@ -import { HTTPError } from '../errors/index.js'; - -const MONGO_DUPLICATE_KEY_ERROR = 11000; -const MONGO_DUPLICATE_KEY_ERROR2 = 11001; - -/** - * @param {Error | import('mongodb').MongoError} error - * @returns {boolean} - */ -function isDuplicateKeyError(error) { - return 'code' in error && ( - error.code === MONGO_DUPLICATE_KEY_ERROR - || error.code === MONGO_DUPLICATE_KEY_ERROR2 - ); -} - -/** - * Turn duplicate key errors from Mongo into useful-for-humans error messages. - * - * @param {Error} error Error instance that may be a duplicate key error. - * @returns {Error} More useful error if a MongoDB duplicate key error was given, - * otherwise the given error, unchanged. - */ -function beautifyDuplicateKeyError(error) { - if (isDuplicateKeyError(error)) { - if (error.message.indexOf('username') !== -1) { - return new HTTPError(400, 'That username is in use.'); - } - if (error.message.indexOf('email') !== -1) { - return new HTTPError(400, 'That email address is in use.'); - } - } - return error; -} - -export default beautifyDuplicateKeyError; diff --git a/test/authenticate.mjs b/test/authenticate.mjs index 8705fce3..c37df6ea 100644 --- a/test/authenticate.mjs +++ b/test/authenticate.mjs @@ -6,6 +6,7 @@ import testKeys from 'recaptcha-test-keys'; import createUwave from './utils/createUwave.mjs'; const sandbox = sinon.createSandbox(); +const TEST_PASSWORD = 'testtest'; describe('Authentication', () => { let uw; @@ -93,19 +94,19 @@ describe('Authentication', () => { .expect(400); await supertest(uw.server) .post('/api/auth/register') - .send({ email: 'name@example.com', username: 'name', password: 'testtest' }) + .send({ email: 'name@example.com', username: 'name', password: TEST_PASSWORD }) .expect(200); await supertest(uw.server) .post('/api/auth/register') - .send({ email: 'name@example.com', name: 'something with spaces', password: 'testtest' }) + .send({ email: 'name@example.com', name: 'something with spaces', password: TEST_PASSWORD }) .expect(400); }); it('creates a user', async () => { const res = await supertest(uw.server) .post('/api/auth/register') - .send({ email: 'name@example.com', username: 'name', password: 'testtest' }) + .send({ email: 'name@example.com', username: 'name', password: TEST_PASSWORD }) .expect(200); sinon.assert.match(res.body.data, { @@ -121,7 +122,7 @@ describe('Authentication', () => { it('slugifies names well', async () => { const res = await supertest(uw.server) .post('/api/auth/register') - .send({ email: 'name@example.com', username: '테스트네임', password: 'testtest' }) + .send({ email: 'name@example.com', username: '테스트네임', password: TEST_PASSWORD }) .expect(200); assert.strictEqual(res.body.data.slug, 'teseuteuneim'); @@ -134,7 +135,7 @@ describe('Authentication', () => { .send({ email: 'name@example.com', username: 'name', - password: 'testtest', + password: TEST_PASSWORD, }) .expect(400); @@ -155,7 +156,7 @@ describe('Authentication', () => { .send({ email: 'name@example.com', username: 'name', - password: 'testtest', + password: TEST_PASSWORD, grecaptcha: 'sample recaptcha challenge for test :)', }) .expect(200); @@ -163,6 +164,34 @@ describe('Authentication', () => { assert.strictEqual(goodRes.body.data.username, 'name'); assert(scope.isDone()); }); + + it('gracefully rejects duplicate email', async () => { + await supertest(uw.server) + .post('/api/auth/register') + .send({ email: 'name@example.com', username: 'name', password: TEST_PASSWORD }) + .expect(200); + + const res = await supertest(uw.server) + .post('/api/auth/register') + .send({ email: 'name@example.com', username: 'unique', password: TEST_PASSWORD }) + .expect(422); + + sinon.assert.match(res.body.errors[0], { code: 'invalid-email' }); + }); + + it('gracefully rejects duplicate username', async () => { + await supertest(uw.server) + .post('/api/auth/register') + .send({ email: 'name@example.com', username: 'name', password: TEST_PASSWORD }) + .expect(200); + + const res = await supertest(uw.server) + .post('/api/auth/register') + .send({ email: 'unique@example.com', username: 'name', password: TEST_PASSWORD }) + .expect(422); + + sinon.assert.match(res.body.errors[0], { code: 'invalid-username' }); + }); }); }); diff --git a/test/users.mjs b/test/users.mjs index 707688b0..19d8cfa4 100644 --- a/test/users.mjs +++ b/test/users.mjs @@ -1,4 +1,5 @@ import supertest from 'supertest'; +import * as sinon from 'sinon'; import createUwave from './utils/createUwave.mjs'; describe('Users', () => { @@ -36,4 +37,88 @@ describe('Users', () => { .expect(200); }); }); + + // TODO: this would make more sense as PATCH /api/users/:id... + describe('PUT /api/users/:id/username', () => { + it('requires authentication', async () => { + await supertest(uw.server) + .put(`/api/users/${user.id}/username`) + .send({ username: 'notloggedin' }) + .expect(401); + }); + + it('rejects invalid input', async () => { + const token = await uw.test.createTestSessionToken(user); + + await supertest(uw.server) + .put(`/api/users/${user.id}/username`) + .set('Cookie', `uwsession=${token}`) + .send({ username: 'with spaces' }) + .expect(400); + }); + + it('changes the username', async () => { + const token = await uw.test.createTestSessionToken(user); + + const res = await supertest(uw.server) + .put(`/api/users/${user.id}/username`) + .set('Cookie', `uwsession=${token}`) + .send({ username: 'new_username' }) + .expect(200); + + sinon.assert.match(res.body.data, { + username: 'new_username', + slug: 'new_username', + }); + }); + + it('slugifies the new name well', async () => { + const token = await uw.test.createTestSessionToken(user); + + const res = await supertest(uw.server) + .put(`/api/users/${user.id}/username`) + .set('Cookie', `uwsession=${token}`) + .send({ username: '테스트네임' }) + .expect(200); + + sinon.assert.match(res.body.data, { + username: '테스트네임', + slug: 'teseuteuneim', + }); + }); + + it("can not change someone else's username", async () => { + const token = await uw.test.createTestSessionToken(user); + const secondUser = await uw.test.createUser(); + + await supertest(uw.server) + .put(`/api/users/${secondUser.id}/username`) + .set('Cookie', `uwsession=${token}`) + .send({ username: 'new_username' }) + .expect(403); + }); + + it('reports conflicts', async () => { + const token = await uw.test.createTestSessionToken(user); + + const secondUser = await uw.test.createUser(); + const secondToken = await uw.test.createTestSessionToken(secondUser); + + await supertest(uw.server) + .put(`/api/users/${user.id}/username`) + .set('Cookie', `uwsession=${token}`) + .send({ username: 'thisWillConflict' }) + .expect(200); + + const res = await supertest(uw.server) + .put(`/api/users/${secondUser.id}/username`) + .set('Cookie', `uwsession=${secondToken}`) + .send({ username: 'thisWillConflict!' }) + .expect(422); + + sinon.assert.match(res.body.errors[0], { + code: 'invalid-username', + }); + }); + }); });