From a2a49b8e45c4aed3d2e41d57ee79c32d527e92fa Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 13 Jul 2018 14:36:54 -0600 Subject: [PATCH] feat: improved user creation --- src/core/server/app/handlers/auth/local.ts | 6 +- src/core/server/app/index.ts | 5 +- src/core/server/app/middleware/error.ts | 6 + src/core/server/app/middleware/logging.ts | 6 +- .../server/app/middleware/passport/oidc.ts | 153 +++++++++++------- .../request/__snapshots__/body.spec.ts.snap | 3 + src/core/server/app/request/body.spec.ts | 32 ++++ src/core/server/app/request/body.ts | 5 +- src/core/server/app/router.ts | 41 +++-- src/core/server/models/user.ts | 72 +++++++-- src/core/server/services/users/index.ts | 8 +- 11 files changed, 246 insertions(+), 91 deletions(-) create mode 100644 src/core/server/app/middleware/error.ts create mode 100644 src/core/server/app/request/__snapshots__/body.spec.ts.snap create mode 100644 src/core/server/app/request/body.spec.ts diff --git a/src/core/server/app/handlers/auth/local.ts b/src/core/server/app/handlers/auth/local.ts index 3bd3d2eb7..777d64dc8 100644 --- a/src/core/server/app/handlers/auth/local.ts +++ b/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, }); diff --git a/src/core/server/app/index.ts b/src/core/server/app/index.ts index ef65f8157..0fefb561f 100644 --- a/src/core/server/app/index.ts +++ b/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"; diff --git a/src/core/server/app/middleware/error.ts b/src/core/server/app/middleware/error.ts new file mode 100644 index 000000000..2f666260b --- /dev/null +++ b/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 }); +}; diff --git a/src/core/server/app/middleware/logging.ts b/src/core/server/app/middleware/logging.ts index 9db48fa31..ebe1fe53e 100644 --- a/src/core/server/app/middleware/logging.ts +++ b/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); }; diff --git a/src/core/server/app/middleware/passport/oidc.ts b/src/core/server/app/middleware/passport/oidc.ts index 4892cd918..a26ff32dd 100644 --- a/src/core/server/app/middleware/passport/oidc.ts +++ b/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 }); } diff --git a/src/core/server/app/request/__snapshots__/body.spec.ts.snap b/src/core/server/app/request/__snapshots__/body.spec.ts.snap new file mode 100644 index 000000000..823d9cb68 --- /dev/null +++ b/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]"`; diff --git a/src/core/server/app/request/body.spec.ts b/src/core/server/app/request/body.spec.ts new file mode 100644 index 000000000..98fd1376d --- /dev/null +++ b/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(); +}); diff --git a/src/core/server/app/request/body.ts b/src/core/server/app/request/body.ts index 9b7cabb13..a8e34412c 100644 --- a/src/core/server/app/request/body.ts +++ b/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; diff --git a/src/core/server/app/router.ts b/src/core/server/app/router.ts index 7b2338e0f..1011e532f 100644 --- a/src/core/server/app/router.ts +++ b/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; } diff --git a/src/core/server/models/user.ts b/src/core/server/models/user.ts index 0631e9e85..04da3214d 100644 --- a/src/core/server/models/user.ts +++ b/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 = { - id: uuid.v4(), + const defaults: Sub = { + 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 = 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 } = { + $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) => { + const query: FilterQuery = { + // 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; diff --git a/src/core/server/services/users/index.ts b/src/core/server/services/users/index.ts index 6906a7d3b..1a0a62b4f 100644 --- a/src/core/server/services/users/index.ts +++ b/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; }