[CORl-640] User Registration Race (#2583)

*  fix: fixes user registration endpoints

- fixes #2579

* feat: cleanup from review

- Added seperate create function
- Moved some validation around

* fix: linting

src/core/server/app/handlers/api/auth/local/signup.ts

@@ -8,7 +8,7 @@ import { RequestLimiter } from "coral-server/app/request/limiter";
 import { IntegrationDisabled } from "coral-server/errors";
 import { GQLUSER_ROLE } from "coral-server/graph/tenant/schema/__generated__/types";
 import { LocalProfile, User } from "coral-server/models/user";
-import { insert } from "coral-server/services/users";
+import { create } from "coral-server/services/users";
 import { sendConfirmationEmail } from "coral-server/services/users/auth";
 import { RequestHandler } from "coral-server/types/express";
 
@@ -82,13 +82,13 @@ export const signupHandler = ({
       };
 
       // Create the new user.
-      const user = await insert(
+      const user = await create(
         mongo,
         tenant,
         {
           email,
           username,
-          profiles: [profile],
+          profile,
           // New users signing up via local auth will have the commenter role to
           // start with.
           role: GQLUSER_ROLE.COMMENTER,

src/core/server/app/handlers/api/install.ts

@@ -9,14 +9,14 @@ import { TenantInstalledAlreadyError } from "coral-server/errors";
 import { GQLUSER_ROLE } from "coral-server/graph/tenant/schema/__generated__/types";
 import { LocalProfile } from "coral-server/models/user";
 import { install, InstallTenant } from "coral-server/services/tenant";
-import { insert, InsertUser } from "coral-server/services/users";
+import { create, CreateUser } from "coral-server/services/users";
 import { RequestHandler } from "coral-server/types/express";
 
 export interface TenantInstallBody {
   tenant: Omit<InstallTenant, "domain" | "locale"> & {
     locale: LanguageCode | null;
   };
-  user: Required<Pick<InsertUser, "username" | "email"> & { password: string }>;
+  user: Required<Pick<CreateUser, "username" | "email"> & { password: string }>;
 }
 
 const TenantInstallBodySchema = Joi.object().keys({
@@ -120,13 +120,13 @@ export const installHandler = ({
     };
 
     // Create the first admin user.
-    await insert(
+    await create(
       mongo,
       tenant,
       {
         email,
         username,
-        profiles: [profile],
+        profile,
         role: GQLUSER_ROLE.ADMIN,
       },
       req.coral.now

src/core/server/app/middleware/passport/strategies/facebook.ts

@@ -14,7 +14,7 @@ import {
   FacebookProfile,
   retrieveUserWithProfile,
 } from "coral-server/models/user";
-import { insert } from "coral-server/services/users";
+import { findOrCreate } from "coral-server/services/users";
 
 export type FacebookStrategyOptions = OAuth2StrategyOptions;
 
@@ -72,7 +72,7 @@ export default class FacebookStrategy extends OAuth2Strategy<
         emailVerified = false;
       }
 
-      user = await insert(
+      user = await findOrCreate(
         this.mongo,
         tenant,
         {
@@ -80,7 +80,7 @@ export default class FacebookStrategy extends OAuth2Strategy<
           email,
           emailVerified,
           avatar,
-          profiles: [profile],
+          profile,
         },
         now
       );

src/core/server/app/middleware/passport/strategies/google.ts

@@ -14,7 +14,7 @@ import {
   GoogleProfile,
   retrieveUserWithProfile,
 } from "coral-server/models/user";
-import { insert } from "coral-server/services/users";
+import { findOrCreate } from "coral-server/services/users";
 
 export type GoogleStrategyOptions = OAuth2StrategyOptions;
 
@@ -71,7 +71,7 @@ export default class GoogleStrategy extends OAuth2Strategy<
         emailVerified = false;
       }
 
-      user = await insert(
+      user = await findOrCreate(
         this.mongo,
         tenant,
         {
@@ -79,7 +79,7 @@ export default class GoogleStrategy extends OAuth2Strategy<
           email,
           emailVerified,
           avatar,
-          profiles: [profile],
+          profile,
         },
         now
       );

src/core/server/app/middleware/passport/strategies/oidc/index.ts

@@ -21,7 +21,7 @@ import {
 import { AsymmetricSigningAlgorithm } from "coral-server/services/jwt";
 import TenantCache from "coral-server/services/tenant/cache";
 import { TenantCacheAdapter } from "coral-server/services/tenant/cache/adapter";
-import { insert } from "coral-server/services/users";
+import { findOrCreate } from "coral-server/services/users";
 import { validateUsername } from "coral-server/services/users/helpers";
 import { Request } from "coral-server/types/express";
 
@@ -181,7 +181,7 @@ export async function findOrCreateOIDCUser(
     }
 
     // Create the new user, as one didn't exist before!
-    user = await insert(
+    user = await findOrCreate(
       mongo,
       tenant,
       {
@@ -190,7 +190,7 @@ export async function findOrCreateOIDCUser(
         email,
         emailVerified: email_verified,
         avatar: picture,
-        profiles: [profile],
+        profile,
       },
       now
     );

src/core/server/app/middleware/passport/strategies/verifiers/sso.ts

@@ -14,7 +14,7 @@ import {
   SSOProfile,
   updateUserFromSSO,
 } from "coral-server/models/user";
-import { insert } from "coral-server/services/users";
+import { findOrCreate } from "coral-server/services/users";
 
 import {
   getSSOProfile,
@@ -127,7 +127,7 @@ export async function findOrCreateSSOUser(
     };
 
     // Create the new user, as one didn't exist before!
-    user = await insert(
+    user = await findOrCreate(
       mongo,
       tenant,
       {
@@ -136,7 +136,8 @@ export async function findOrCreateSSOUser(
         role: role || GQLUSER_ROLE.COMMENTER,
         badges,
         email,
-        profiles: [profile],
+        emailVerified: true,
+        profile,
       },
       now
     );

src/core/server/models/user/user.ts

@@ -468,29 +468,32 @@ function hashPassword(password: string): Promise<string> {
   return bcrypt.hash(password, 10);
 }
 
-export type InsertUserInput = Omit<
-  User,
-  | "id"
-  | "tenantID"
-  | "tokens"
-  | "status"
-  | "ignoredUsers"
-  | "notifications"
-  | "digests"
-  | "emailVerificationID"
-  | "createdAt"
-> &
-  Partial<Pick<User, "id">>;
+export interface FindOrCreateUserInput {
+  id?: string;
+  username?: string;
+  avatar?: string;
+  email?: string;
+  badges?: string[];
+  emailVerified?: boolean;
+  role: GQLUSER_ROLE;
+  profile: Profile;
+}
 
-export async function insertUser(
-  mongo: Db,
+/**
+ * findOrCreateUserInput converts the FindOrCreateUserInput input into aUse.
+ *
+ * @param tenantID ID of the Tenant to create the user for
+ * @param input the input for creating a User
+ * @param now the current date
+ */
+async function findOrCreateUserInput(
   tenantID: string,
-  { id = uuid.v4(), ...input }: InsertUserInput,
-  now = new Date()
-) {
+  { id = uuid.v4(), profile, ...input }: FindOrCreateUserInput,
+  now: Date
+): Promise<Readonly<User>> {
   // default are the properties set by the application when a new user is
   // created.
-  const defaults: Sub<User, InsertUserInput> = {
+  const defaults: Sub<User, FindOrCreateUserInput> = {
     tenantID,
     tokens: [],
     ignoredUsers: [],
@@ -508,11 +511,13 @@ export async function insertUser(
       onStaffReplies: false,
       digestFrequency: GQLDIGEST_FREQUENCY.NONE,
     },
+    profiles: [],
     digests: [],
     createdAt: now,
   };
 
   if (input.username) {
+    // Add the username history to the user.
     defaults.status.username.history.push({
       id: uuid.v4(),
       username: input.username,
@@ -521,39 +526,54 @@ export async function insertUser(
     });
   }
 
-  // Guard against empty login profiles (they need some way to login).
-  if (input.profiles.length === 0) {
-    throw new Error("users require at least one profile");
-  }
-
   // Mutate the profiles to ensure we mask handle any secrets.
-  const profiles: Profile[] = [];
-  for (let profile of input.profiles) {
-    switch (profile.type) {
-      case "local":
-        // Hash the user's password with bcrypt.
-        const password = await hashPassword(profile.password);
-        profile = {
-          ...profile,
-          password,
-        };
-        break;
-    }
-    // Save a copy.
-    profiles.push(profile);
+  switch (profile.type) {
+    case "local":
+      // Hash the user's password with bcrypt.
+      const password = await hashPassword(profile.password);
+      defaults.profiles.push({ ...profile, password });
+      break;
+    default:
+      // Push the profile onto the User.
+      defaults.profiles.push(profile);
+      break;
   }
 
   // Merge the defaults and the input together.
-  const user: Readonly<User> = {
+  return {
     ...defaults,
     ...input,
     id,
-    profiles,
   };
+}
+
+export async function findOrCreateUser(
+  mongo: Db,
+  tenantID: string,
+  input: FindOrCreateUserInput,
+  now: Date
+) {
+  const user = await findOrCreateUserInput(tenantID, input, now);
 
   try {
-    // Insert it into the database. This may throw an error.
-    await collection(mongo).insert(user);
+    await collection(mongo).findOneAndUpdate(
+      {
+        tenantID,
+        profiles: {
+          $elemMatch: {
+            id: input.profile.id,
+            type: input.profile.type,
+          },
+        },
+      },
+      { $setOnInsert: user },
+      {
+        // False to return the updated document instead of the original
+        // document.
+        returnOriginal: false,
+        upsert: true,
+      }
+    );
   } catch (err) {
     // Evaluate the error, if it is in regards to violating the unique index,
     // then return a duplicate User error.
@@ -571,6 +591,37 @@ export async function insertUser(
   return user;
 }
 
+export type CreateUserInput = FindOrCreateUserInput;
+
+export async function createUser(
+  mongo: Db,
+  tenantID: string,
+  input: CreateUserInput,
+  now: Date
+) {
+  const user = await findOrCreateUserInput(tenantID, input, now);
+
+  try {
+    // Insert it into the database. This may throw an error.
+    await collection(mongo).insertOne(user);
+  } catch (err) {
+    // Evaluate the error, if it is in regards to violating the unique index,
+    // then return a duplicate User error.
+    if (err instanceof MongoError && err.code === 11000) {
+      // Check if duplicate index was about the email.
+      if (err.errmsg && err.errmsg.includes("tenantID_1_email_1")) {
+        throw new DuplicateEmailError(input.email!);
+      }
+
+      throw new DuplicateUserError();
+    }
+
+    throw err;
+  }
+
+  return user;
+}
+
 export async function retrieveUser(mongo: Db, tenantID: string, id: string) {
   return collection(mongo).findOne({ tenantID, id });
 }
@@ -581,15 +632,15 @@ export async function retrieveManyUsers(
   ids: string[]
 ) {
   const cursor = await collection(mongo).find({
+    tenantID,
     id: {
       $in: ids,
     },
-    tenantID,
   });
 
   const users = await cursor.toArray();
 
-  return ids.map(id => users.find(comment => comment.id === id) || null);
+  return ids.map(id => users.find(user => user.id === id) || null);
 }
 
 export async function retrieveUserWithProfile(

src/core/server/queue/Task.ts

@@ -70,7 +70,10 @@ export default class Task<T, U = any> {
    */
   public process() {
     this.queue.process(async (job: Job<T>) => {
-      const log = this.log.child({ jobID: job.id }, true);
+      const log = this.log.child(
+        { jobID: job.id, attemptsMade: job.attemptsMade },
+        true
+      );
 
       log.trace("processing job from queue");
 

src/core/server/services/users/auth/invite.ts

@@ -14,7 +14,7 @@ import {
 } from "coral-server/models/invite";
 import { Tenant } from "coral-server/models/tenant";
 import {
-  insertUser,
+  createUser,
   LocalProfile,
   retrieveUserWithEmail,
   User,
@@ -310,14 +310,14 @@ export async function redeem(
   };
 
   // Create the new user based on the invite.
-  const user = await insertUser(
+  const user = await createUser(
     mongo,
     tenant.id,
     {
       username,
       email,
       emailVerified: true, // Verified because the invite link was clicked.
-      profiles: [profile],
+      profile,
       role,
     },
     now

src/core/server/services/users/index.ts

@@ -34,11 +34,12 @@ import {
   clearDeletionDate,
   consolidateUserBanStatus,
   consolidateUserSuspensionStatus,
+  createUser,
   createUserToken,
   deactivateUserToken,
+  findOrCreateUser,
+  FindOrCreateUserInput,
   ignoreUser,
-  insertUser,
-  InsertUserInput,
   NotificationSettingsInput,
   removeActiveUserSuspensions,
   removeUserBan,
@@ -76,28 +77,65 @@ import {
 } from "./download/token";
 import { validateEmail, validatePassword, validateUsername } from "./helpers";
 
-export type InsertUser = InsertUserInput;
-
-/**
- * insert will upsert the User into the database for the Tenant.
- *
- * @param mongo mongo database to interact with
- * @param tenant Tenant where the User will be added to
- * @param input the input for creating the User
- */
-export async function insert(
-  mongo: Db,
-  tenant: Tenant,
-  input: InsertUser,
-  now = new Date()
-) {
+function validateFindOrCreateUserInput(input: FindOrCreateUser) {
   if (input.username) {
     validateUsername(input.username);
   }
 
   if (input.email) {
     validateEmail(input.email);
+  }
 
+  const localProfile = getLocalProfile({ profiles: [input.profile] });
+  if (localProfile) {
+    validateEmail(localProfile.id);
+    validatePassword(localProfile.password);
+
+    if (input.email !== localProfile.id) {
+      throw new Error("email addresses don't match profile");
+    }
+  }
+}
+
+export type FindOrCreateUser = FindOrCreateUserInput;
+
+export async function findOrCreate(
+  mongo: Db,
+  tenant: Tenant,
+  input: FindOrCreateUser,
+  now: Date
+) {
+  // Validate the input.
+  validateFindOrCreateUserInput(input);
+
+  const user = await findOrCreateUser(mongo, tenant.id, input, now);
+
+  // TODO: (wyattjoh) evaluate the tenant to determine if we should send the verification email.
+
+  return user;
+}
+
+export type CreateUser = FindOrCreateUserInput;
+
+export async function create(
+  mongo: Db,
+  tenant: Tenant,
+  input: CreateUser,
+  now: Date
+) {
+  // Validate the input.
+  validateFindOrCreateUserInput(input);
+
+  if (input.id) {
+    // Try to check to see if there is a user with the same ID before we try to
+    // create the user again.
+    const alreadyFoundUser = await retrieveUser(mongo, tenant.id, input.id);
+    if (alreadyFoundUser) {
+      throw new DuplicateUserError();
+    }
+  }
+
+  if (input.email) {
     // Try to lookup the user to see if this user already has an account if they
     // do, we can short circuit the database index hit.
     const alreadyFoundUser = await retrieveUserWithEmail(
@@ -110,37 +148,9 @@ export async function insert(
     }
   }
 
-  if (input.id) {
-    // Try to check to see if there is a user with the same ID before we try to
-    // create the user again.
-    const alreadyFoundUser = await retrieveUser(mongo, tenant.id, input.id);
-    if (alreadyFoundUser) {
-      throw new DuplicateUserError();
-    }
-  }
+  const user = await createUser(mongo, tenant.id, input, now);
 
-  const localProfile = getLocalProfile(input);
-  if (localProfile) {
-    validateEmail(localProfile.id);
-    validatePassword(localProfile.password);
-
-    if (input.email !== localProfile.id) {
-      throw new Error("email addresses don't match profile");
-    }
-  }
-
-  const user = await insertUser(mongo, tenant.id, input, now);
-
-  // // TODO: (wyattjoh) evaluate the tenant to determine if we should send the verification email.
-  // if (localProfile && user.email) {
-  //   if (mailer) {
-  //     // // Send the email confirmation email.
-  //     // await sendConfirmationEmail(mongo, mailer, tenant, user, user.email);
-  //   } else {
-  //     // FIXME: (wyattjoh) extract the local profile based inserts into another function.
-  //     throw new Error("local profile was provided, but the mailer was not");
-  //   }
-  // }
+  // TODO: (wyattjoh) evaluate the tenant to determine if we should send the verification email.
 
   return user;
 }