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

@@ -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.

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));
 }
 
 /**

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);
 }
 
 /**

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,

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<string>} */ (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;

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(),