feat: improved user creation

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

@@ -6,7 +6,7 @@ import { handle } from "talk-server/app/middleware/passport";
 import { validate } from "talk-server/app/request/body";
 import { GQLUSER_ROLE } from "talk-server/graph/tenant/schema/__generated__/types";
 import { LocalProfile } from "talk-server/models/user";
-import { create } from "talk-server/services/users";
+import { upsert } from "talk-server/services/users";
 import { Request } from "talk-server/types/express";
 
 export interface SignupBody {
@@ -67,12 +67,14 @@ export const signup = ({ db }: SignupOptions): RequestHandler => async (
     };
 
     // Create the new user.
-    const user = await create(db, tenant, {
+    const user = await upsert(db, tenant, {
       email,
       username,
       displayName,
       password,
       profiles: [profile],
+      // New users signing up via local auth will have the commenter role to
+      // start with.
       role: GQLUSER_ROLE.COMMENTER,
     });
 

src/core/server/app/index.ts

@@ -8,10 +8,7 @@ import { Config } from "talk-server/config";
 import { handleSubscriptions } from "talk-server/graph/common/subscriptions/middleware";
 import { Schemas } from "talk-server/graph/schemas";
 
-import {
-  access as accessLogger,
-  error as errorLogger,
-} from "./middleware/logging";
+import { accessLogger, errorLogger } from "./middleware/logging";
 import serveStatic from "./middleware/serveStatic";
 import { createRouter } from "./router";
 

src/core/server/app/middleware/error.ts

@@ -0,0 +1,6 @@
+import { ErrorRequestHandler } from "express";
+
+export const apiErrorHandler: ErrorRequestHandler = (err, req, res, next) => {
+  // TODO: handle better when we improve errors.
+  res.status(500).json({ error: err.message });
+};

src/core/server/app/middleware/logging.ts

@@ -2,7 +2,7 @@ import { ErrorRequestHandler, RequestHandler } from "express";
 import now from "performance-now";
 import logger from "../../logger";
 
-export const access: RequestHandler = (req, res, next) => {
+export const accessLogger: RequestHandler = (req, res, next) => {
   const startTime = now();
   const end = res.end;
   res.end = (chunk: any, encodingOrCb?: any, cb?: any) => {
@@ -37,7 +37,7 @@ export const access: RequestHandler = (req, res, next) => {
   next();
 };
 
-export const error: ErrorRequestHandler = (err, req, res, next) => {
-  logger.error({ err }, "http error");
+export const errorLogger: ErrorRequestHandler = (err, req, res, next) => {
+  logger.error(err, "http error");
   next(err);
 };

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

@@ -1,27 +1,33 @@
 import jwt from "jsonwebtoken";
 import jwks, { JwksClient } from "jwks-rsa";
 import { Db } from "mongodb";
-import { Strategy as OAuth2Strategy } from "passport-oauth2";
+import { Strategy as OAuth2Strategy, VerifyCallback } from "passport-oauth2";
 import { Strategy } from "passport-strategy";
 
 import { reconstructURL } from "talk-server/app/url";
 import { GQLUSER_ROLE } from "talk-server/graph/tenant/schema/__generated__/types";
 import { OIDCAuthIntegration, Tenant } from "talk-server/models/tenant";
 import { OIDCProfile, retrieveUserWithProfile } from "talk-server/models/user";
-import { create } from "talk-server/services/users";
+import { upsert } from "talk-server/services/users";
 import { Request } from "talk-server/types/express";
 
-import { VerifyCallback } from "./index";
-
 export interface Params {
   id_token?: string;
 }
 
+/**
+ * OIDCIDToken describes the set of claims that are present in a ID Token. This
+ * interface confirms with the ID Token specification as defined:
+ * https://openid.net/specs/openid-connect-core-1_0.html#IDToken
+ */
 export interface OIDCIDToken {
+  aud: string;
   iss: string;
   sub: string;
-  email: string;
+  exp: number; // TODO: use this as the source for how long an OIDC user can be logged in for
+  email?: string;
   email_verified?: boolean;
+  picture?: string;
 }
 
 export interface StrategyItem {
@@ -48,13 +54,14 @@ export function isOIDCToken(token: OIDCIDToken | object): token is OIDCIDToken {
 export async function findOrCreateOIDCUser(
   db: Db,
   tenant: Tenant,
-  { iss, sub, email, email_verified }: OIDCIDToken
+  token: OIDCIDToken
 ) {
   // Construct the profile that will be used to query for the user.
   const profile: OIDCProfile = {
     type: "oidc",
-    provider: iss,
-    id: sub,
+    id: token.sub,
+    issuer: token.iss,
+    audience: token.aud,
   };
 
   // Try to lookup user given their id provided in the `sub` claim.
@@ -63,11 +70,12 @@ export async function findOrCreateOIDCUser(
     // FIXME: implement rules.
 
     // Create the new user, as one didn't exist before!
-    user = await create(db, tenant, {
+    user = await upsert(db, tenant, {
       username: null,
       role: GQLUSER_ROLE.COMMENTER,
-      email,
-      email_verified,
+      email: token.email,
+      email_verified: token.email_verified,
+      avatar: token.picture,
       profiles: [profile],
     });
   }
@@ -75,6 +83,11 @@ export async function findOrCreateOIDCUser(
   return user;
 }
 
+/**
+ * OIDC_SCOPE is the set of scopes requested for users signing up via OIDC.
+ */
+const OIDC_SCOPE = "openid email profile";
+
 // FIXME: attach strategy to cache updates of the tenants
 
 export default class OIDCStrategy extends Strategy {
@@ -138,14 +151,14 @@ export default class OIDCStrategy extends Strategy {
     return entry.jwksClient;
   }
 
-  private verifyCallback(
+  private verifyCallback = (
     req: Request,
-    accessToken: string,
-    refreshToken: string,
+    accessToken: string, // ignore the access token, we don't use it.
+    refreshToken: string, // ignore the refresh token, we don't use it.
     params: Params,
-    profile: any,
+    profile: any, // we don't look inside the profile (yet).
     done: VerifyCallback
-  ) {
+  ) => {
     // Try to lookup user given their id provided in the `sub` claim of the
     // `id_token`.
     const { id_token } = params;
@@ -156,36 +169,30 @@ export default class OIDCStrategy extends Strategy {
 
     // Grab the tenant out of the request, as we need some more details.
     const { tenant } = req;
+    if (!tenant) {
+      // TODO: return a better error.
+      return done(new Error("tenant not found"));
+    }
+
+    // Get the integration from the tenant. If needed, it will be used to create
+    // a new strategy.
+    let integration: OIDCAuthIntegration;
+    try {
+      integration = getEnabledIntegration(tenant);
+    } catch (err) {
+      // TODO: wrap error?
+      return done(err);
+    }
 
     // Grab the JWKSClient.
-    const client = this.lookupJWKSClient(
-      req,
-      tenant!.id,
-      tenant!.auth.integrations.oidc!
-    );
+    const client = this.lookupJWKSClient(req, tenant.id, integration);
 
     // Verify that the id_token is valid or not.
     jwt.verify(
       id_token,
-      ({ kid }, callback) => {
-        if (!kid) {
-          // TODO: return better error.
-          return callback(new Error("no kid in id_token"));
-        }
-
-        // Get the signing key from the jwks provider.
-        client.getSigningKey(kid, (err, key) => {
-          if (err) {
-            // TODO: wrap error?
-            return callback(err);
-          }
-
-          const signingKey = key.publicKey || key.rsaPublicKey;
-          callback(null, signingKey);
-        });
-      },
+      this.keyFunc(client),
       {
-        issuer: tenant!.auth.integrations.oidc!.issuer,
+        issuer: integration.issuer,
       },
       (err, decoded) => {
         if (err) {
@@ -193,10 +200,39 @@ export default class OIDCStrategy extends Strategy {
           return done(err);
         }
 
-        this.verify(tenant!, decoded as OIDCIDToken, done);
+        // Delegate the verify method off to the passed in verify method.
+        this.verify(tenant, decoded as OIDCIDToken, done);
       }
     );
-  }
+  };
+
+  /**
+   * keyFunc will provide the secret based on the given jwkw client.
+   *
+   * @param client the jwks client for the specific request being made
+   */
+  private keyFunc = (client: jwks.JwksClient): jwt.KeyFunction => (
+    { kid },
+    callback
+  ) => {
+    if (!kid) {
+      // TODO: return better error.
+      return callback(new Error("no kid in id_token"));
+    }
+
+    // Get the signing key from the jwks provider.
+    client.getSigningKey(kid, (err, key) => {
+      if (err) {
+        // TODO: wrap error?
+        return callback(err);
+      }
+
+      // Grab the signingKey out of the provided key.
+      const signingKey = key.publicKey || key.rsaPublicKey;
+
+      callback(null, signingKey);
+    });
+  };
 
   private createStrategy(
     req: Request,
@@ -218,7 +254,7 @@ export default class OIDCStrategy extends Strategy {
         tokenURL,
         callbackURL,
       },
-      this.verifyCallback.bind(this)
+      this.verifyCallback
     );
   }
 
@@ -231,17 +267,7 @@ export default class OIDCStrategy extends Strategy {
 
     // Get the integration from the tenant. If needed, it will be used to create
     // a new strategy.
-    const integration = tenant.auth.integrations.oidc;
-    if (!integration) {
-      // TODO: return a better error.
-      throw new Error("integration not found");
-    }
-
-    // Handle when the integration is enabled/disabled.
-    if (!integration.enabled) {
-      // TODO: return a better error.
-      throw new Error("integration not enabled");
-    }
+    const integration = getEnabledIntegration(tenant);
 
     // Try to get the Tenant's cached integrations.
     let entry = this.cache.get(tenant.id);
@@ -279,13 +305,32 @@ export default class OIDCStrategy extends Strategy {
       // Authenticate with the strategy, binding the current context to the method
       // to provide it with the augmented passport handlers. We also request the
       // 'openid' scope so we can get an id_token back.
-      strategy.authenticate(req, { scope: "openid email", session: false });
+      strategy.authenticate(req, {
+        scope: OIDC_SCOPE,
+        session: false,
+      });
     } catch (err) {
       return this.error(err);
     }
   }
 }
 
+function getEnabledIntegration(tenant: Tenant) {
+  const integration = tenant.auth.integrations.oidc;
+  if (!integration) {
+    // TODO: return a better error.
+    throw new Error("integration not found");
+  }
+
+  // Handle when the integration is enabled/disabled.
+  if (!integration.enabled) {
+    // TODO: return a better error.
+    throw new Error("integration not enabled");
+  }
+
+  return integration;
+}
+
 export function createOIDCStrategy({ db }: OIDCStrategyOptions) {
   return new OIDCStrategy({ db });
 }

src/core/server/app/request/__snapshots__/body.spec.ts.snap

@@ -0,0 +1,3 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`throws an error for missing fields 1`] = `"child \\"d\\" fails because [\\"d\\" is required]"`;

src/core/server/app/request/body.spec.ts

@@ -0,0 +1,32 @@
+import Joi from "joi";
+
+import { validate } from "talk-server/app/request/body";
+
+it("strips out unknown fields", () => {
+  const payload = { a: 1, b: 2, c: 3 };
+  const schema = Joi.object().keys({});
+
+  expect(validate(schema, payload)).toEqual({});
+});
+
+it("allows valid fields", () => {
+  const payload = { a: 1, b: 2, c: 3 };
+  const schema = Joi.object().keys({ a: Joi.number() });
+
+  expect(validate(schema, payload)).toEqual({ a: 1 });
+});
+
+it("allows valid fields from extended schema", () => {
+  const payload = { a: 1, b: 2, c: 3 };
+  const schema = Joi.object().keys({ a: Joi.number() });
+  const extendedSchema = schema.keys({ b: Joi.number() });
+
+  expect(validate(extendedSchema, payload)).toEqual({ a: 1, b: 2 });
+});
+
+it("throws an error for missing fields", () => {
+  const payload = { a: 1, b: 2, c: 3 };
+  const schema = Joi.object().keys({ d: Joi.number() });
+
+  expect(() => validate(schema, payload)).toThrowErrorMatchingSnapshot();
+});

src/core/server/app/request/body.ts

@@ -12,11 +12,12 @@ export const validate = (schema: Joi.SchemaLike, body: any) => {
   const { value, error: err } = Joi.validate(body, schema, {
     stripUnknown: true,
     presence: "required",
+    abortEarly: false,
   });
 
   if (err) {
-    // TODO: return better error.
-    throw new Error("Validation Error");
+    // TODO: wrap error?
+    throw err;
   }
 
   return value;

src/core/server/app/router.ts

@@ -6,6 +6,8 @@ import managementGraphMiddleware from "talk-server/graph/management/middleware";
 import tenantGraphMiddleware from "talk-server/graph/tenant/middleware";
 
 import { signup } from "talk-server/app/handlers/auth/local";
+import { apiErrorHandler } from "talk-server/app/middleware/error";
+import { errorLogger } from "talk-server/app/middleware/logging";
 import { authenticate } from "talk-server/app/middleware/passport";
 import { AppOptions } from "./index";
 import playground from "./middleware/playground";
@@ -33,19 +35,11 @@ async function createTenantRouter(app: AppOptions, options: RouterOptions) {
   // Tenant identification middleware.
   router.use(tenantMiddleware({ db: app.mongo }));
 
+  // Setup Passport middleware.
   router.use(options.passport.initialize());
-  router.use(
-    "/auth/local",
-    express.json(),
-    authenticate(options.passport, "local")
-  );
-  router.use("/auth/local/signup", express.json(), signup({ db: app.mongo }));
-  router.use("/auth/oidc", authenticate(options.passport, "oidc"));
-  router.use("/auth/oidc/callback", authenticate(options.passport, "oidc"));
-  // router.use("/auth/google", options.passport.authenticate("google"));
-  // router.use("/auth/google/callback", options.passport.authenticate("google"));
-  // router.use("/auth/facebook", options.passport.authenticate("facebook"));
-  // router.use("/auth/facebook/callback", options.passport.authenticate("facebook"));
+
+  // Setup auth routes.
+  router.use("/auth", createNewAuthRouter(app, options));
 
   // Tenant API
   router.use(
@@ -57,6 +51,25 @@ async function createTenantRouter(app: AppOptions, options: RouterOptions) {
   return router;
 }
 
+function createNewAuthRouter(app: AppOptions, options: RouterOptions) {
+  const router = express.Router();
+
+  router.post(
+    "/local",
+    express.json(),
+    authenticate(options.passport, "local")
+  );
+  router.post("/local/signup", express.json(), signup({ db: app.mongo }));
+  router.get("/oidc", authenticate(options.passport, "oidc"));
+  router.get("/oidc/callback", authenticate(options.passport, "oidc"));
+  // router.get("/google", options.passport.authenticate("google"));
+  // router.get("/google/callback", options.passport.authenticate("google"));
+  // router.get("/facebook", options.passport.authenticate("facebook"));
+  // router.get("/facebook/callback", options.passport.authenticate("facebook"));
+
+  return router;
+}
+
 async function createAPIRouter(app: AppOptions, options: RouterOptions) {
   // Create a router.
   const router = express.Router();
@@ -67,6 +80,10 @@ async function createAPIRouter(app: AppOptions, options: RouterOptions) {
   // Configure the management routes.
   router.use("/management", await createManagementRouter(app, options));
 
+  // General API error handler.
+  router.use(errorLogger);
+  router.use(apiErrorHandler);
+
   return router;
 }
 

src/core/server/models/user.ts

@@ -9,6 +9,7 @@ import {
   GQLUSER_USERNAME_STATUS,
 } from "talk-server/graph/tenant/schema/__generated__/types";
 import { ActionCounts } from "talk-server/models/actions";
+import { FilterQuery } from "talk-server/models/query";
 import { TenantResource } from "talk-server/models/tenant";
 
 function collection(db: Db) {
@@ -23,7 +24,8 @@ export interface LocalProfile {
 export interface OIDCProfile {
   type: "oidc";
   id: string;
-  provider: string;
+  issuer: string;
+  audience: string;
 }
 
 export interface SSOProfile {
@@ -62,6 +64,7 @@ export interface User extends TenantResource {
   username: string | null;
   displayName?: string;
   password?: string;
+  avatar?: string;
   email?: string;
   email_verified?: boolean;
   profiles: Profile[];
@@ -73,7 +76,7 @@ export interface User extends TenantResource {
   created_at: Date;
 }
 
-export type CreateUserInput = Omit<
+export type UpsertUserInput = Omit<
   User,
   | "id"
   | "tenant_id"
@@ -84,17 +87,20 @@ export type CreateUserInput = Omit<
   | "created_at"
 >;
 
-export async function createUser(
+export async function upsertUser(
   db: Db,
   tenantID: string,
-  input: CreateUserInput
+  input: UpsertUserInput
 ) {
   const now = new Date();
 
+  // Create a new ID for the user.
+  const id = uuid.v4();
+
   // default are the properties set by the application when a new user is
   // created.
-  const defaults: Sub<User, CreateUserInput> = {
-    id: uuid.v4(),
+  const defaults: Sub<User, UpsertUserInput> = {
+    id,
     tenant_id: tenantID,
     tokens: [],
     action_counts: {},
@@ -118,15 +124,61 @@ export async function createUser(
     created_at: now,
   };
 
+  if (input.password) {
+    // Hash the user's password with bcrypt.
+    input.password = await bcrypt.hash(input.password, 10);
+  }
+
   // Merge the defaults and the input together.
   const user: Readonly<User> = merge({}, defaults, input);
 
-  // Insert it into the database.
-  await collection(db).insertOne(user);
+  // Create a query that will utilize a findOneAndUpdate to facilitate an upsert
+  // operation to ensure no user has the same profile and/or email address. If
+  // any user is found to have the same profile as any of the profiles specified
+  // in the new user object, then we should error here.
+  const filter = createUpsertUserFilter(user);
 
-  return user;
+  // Create the upsert/update operation.
+  const update: { $setOnInsert: Readonly<User> } = {
+    $setOnInsert: user,
+  };
+
+  // Insert it into the database. This may throw an error.
+  const result = await collection(db).findOneAndUpdate(filter, update, {
+    // We are using this to create a user, so we need to upsert it.
+    upsert: true,
+
+    // False to return the updated document instead of the original document.
+    // This lets us detect if the document was updated or not.
+    returnOriginal: false,
+  });
+
+  // Check to see if this was a new user that was upserted, or one was found
+  // that matched existing records. We are sure here that the record exists
+  // because we're returning the updated document and performing an upsert
+  // operation.
+  if (result.value!.id !== id) {
+    // TODO: return better error.
+    throw new Error("user already found");
+  }
+
+  return result.value!;
 }
 
+const createUpsertUserFilter = (user: Readonly<User>) => {
+  const query: FilterQuery<User> = {
+    // Query by the profiles if the user is being created with one.
+    $or: user.profiles.map(profile => ({ profiles: { $elemMatch: profile } })),
+  };
+
+  if (user.email) {
+    // Query by the email address if the user is being created with one.
+    query.$or.push({ email: user.email });
+  }
+
+  return query;
+};
+
 export async function retrieveUser(db: Db, tenantID: string, id: string) {
   return collection(db).findOne({ id, tenant_id: tenantID });
 }
@@ -178,7 +230,7 @@ export async function updateUserRole(
 
 export async function verifyUserPassword(user: User, password: string) {
   if (user.password) {
-    return bcrypt.compare(user.password, password);
+    return bcrypt.compare(password, user.password);
   }
 
   return false;

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

@@ -1,12 +1,12 @@
 import { Db } from "mongodb";
 
 import { Tenant } from "talk-server/models/tenant";
-import { createUser, CreateUserInput } from "talk-server/models/user";
+import { upsertUser, UpsertUserInput } from "talk-server/models/user";
 
-export type CreateUser = CreateUserInput;
+export type UpsertUser = UpsertUserInput;
 
-export async function create(db: Db, tenant: Tenant, input: CreateUser) {
-  const user = await createUser(db, tenant.id, input);
+export async function upsert(db: Db, tenant: Tenant, input: UpsertUser) {
+  const user = await upsertUser(db, tenant.id, input);
 
   return user;
 }