[next] Error and Logging Improvements (#2152)

* feat: added locale support for Tenant

* feat: added secret scrubbing to logs

* chore: cleanup logger

* chore: logger improvements

* feat: re-introduce scoped pretty logger

* feat: added initial error support

* refactor: replace trace-error.TraceError with talk.InternalError

* fix: fixed error logging

* refactor: replaced Error with VError

* fix: repaired issue with error management on api

* fix: patched bug with not found handler

* feat: added translations

* feat: added location path to invalid entries

* refactor: refactored error handling on graph

* fix: moved indexing operations to master node

* refactor: added throw for when the message isn't found in testing

* fix: removed duplicate log

* fix: fixed naming on environment variable

src/core/server/app/middleware/context/tenant.ts

@@ -4,6 +4,7 @@ import { Db } from "mongodb";
 import { Config } from "talk-server/config";
 import TenantContext from "talk-server/graph/tenant/context";
 import { TaskQueue } from "talk-server/queue";
+import { I18n } from "talk-server/services/i18n";
 import { JWTSigningConfig } from "talk-server/services/jwt";
 import { AugmentedRedis } from "talk-server/services/redis";
 import { Request } from "talk-server/types/express";
@@ -14,6 +15,7 @@ export interface TenantContextMiddlewareOptions {
   queue: TaskQueue;
   config: Config;
   signingConfig: JWTSigningConfig;
+  i18n: I18n;
 }
 
 export const tenantContext = ({
@@ -22,6 +24,7 @@ export const tenantContext = ({
   queue,
   config,
   signingConfig,
+  i18n,
 }: TenantContextMiddlewareOptions): RequestHandler => (
   req: Request,
   res,
@@ -52,6 +55,7 @@ export const tenantContext = ({
       tenantCache: cache.tenant,
       queue,
       signingConfig,
+      i18n,
     }),
   };
 

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

@@ -1,17 +1,61 @@
 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 });
+import { InternalError, TalkError } from "talk-server/errors";
+import { I18n } from "talk-server/services/i18n";
+import { Request } from "talk-server/types/express";
+
+/**
+ * wrapError ensures that the error being propagated is a TalkError.
+ *
+ * @param err the error to be wrapped
+ */
+const wrapError = (err: Error) =>
+  err instanceof TalkError
+    ? err
+    : new InternalError(err, "wrapped internal error");
+
+/**
+ * serializeError will return a serialized error that can be returned via the
+ * API response.
+ *
+ * @param err the TalkError that should be serialized
+ * @param bundles the translation bundles
+ * @param tenant the optional tenant to use when selecting the language
+ */
+const serializeError = (err: TalkError, req: Request, bundles: I18n) => {
+  // Get the translation bundle.
+  let bundle = bundles.getDefaultBundle();
+  if (req.talk && req.talk.tenant) {
+    bundle = bundles.getBundle(req.talk.tenant.locale);
+  }
+
+  return {
+    error: err.serializeExtensions(bundle),
+  };
 };
 
-export const errorHandler: ErrorRequestHandler = (err, req, res, next) => {
-  // TODO: handle better when we improve errors.
-  if (err.message === "not found") {
-    // TODO: handle better when we improve errors.
-    res.status(404).send(err.message);
-  } else {
-    // TODO: handle better when we improve errors.
-    res.status(500).send(err.message);
-  }
+export const JSONErrorHandler = (bundles: I18n): ErrorRequestHandler => (
+  err,
+  req,
+  res,
+  next
+) => {
+  // Wrap the error if it needs to be wrapped.
+  err = wrapError(err);
+
+  // Send the response via JSON.
+  res.status(err.status).json(serializeError(err, req, bundles));
+};
+
+export const HTMLErrorHandler = (bundles: I18n): ErrorRequestHandler => (
+  err,
+  req,
+  res,
+  next
+) => {
+  // Wrap the error if it needs to be wrapped.
+  err = wrapError(err);
+
+  // Send the response via HTML.
+  res.status(err.status).render("error", serializeError(err, req, bundles));
 };

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

@@ -39,10 +39,7 @@ export const accessLogger: RequestHandler = (req, res, next) => {
 };
 
 export const errorLogger: ErrorRequestHandler = (err, req, res, next) => {
-  // TODO: handle better when we improve errors.
-  if (err.message !== "not found") {
-    logger.error({ err }, "http error");
-  }
+  logger.error({ err }, "http error");
 
   next(err);
 };

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

@@ -1,6 +1,7 @@
 import { RequestHandler } from "express";
 
+import { NotFoundError } from "talk-server/errors";
+
 export const notFoundMiddleware: RequestHandler = (req, res, next) => {
-  // FIXME: (wyattjoh) send an error that won't log as crazily as this one does.
-  next(new Error("not found"));
+  next(new NotFoundError(req.method, req.originalUrl));
 };

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

@@ -16,9 +16,9 @@ import { Config } from "talk-server/config";
 import logger from "talk-server/logger";
 import { User } from "talk-server/models/user";
 import {
-  blacklistJWT,
   extractJWTFromRequest,
   JWTSigningConfig,
+  revokeJWT,
   SigningTokenOptions,
   signTokenString,
 } from "talk-server/services/jwt";
@@ -97,8 +97,8 @@ export async function handleLogout(redis: Redis, req: Request, res: Response) {
   const validFor = exp - Date.now() / 1000;
   if (validFor > 0) {
     // Invalidate the token, the expiry is in the future and it needs to be
-    // blacklisted.
-    await blacklistJWT(redis, jti, validFor);
+    // revoked.
+    await revokeJWT(redis, jti, validFor);
   }
 
   return res.sendStatus(204);
@@ -233,7 +233,29 @@ export const wrapAuthn = (
         return next(new Error("no user on request"));
       }
 
+      // Pass the login off to be signed.
       handleSuccessfulLogin(user, signingConfig, req, res, next);
     }
   )(req, res, next);
 };
+
+/**
+ * authenticate will wrap the authenticator to forward any error to the error
+ * handler from ExpressJS.
+ *
+ * @param authenticator the authenticator to use
+ */
+export const authenticate = (
+  authenticator: passport.Authenticator
+): RequestHandler => (req, res, next) =>
+  authenticator.authenticate(
+    "jwt",
+    { session: false },
+    (err: Error | null, user: User | null) => {
+      if (err) {
+        return next(err);
+      }
+
+      return next();
+    }
+  )(req, res, next);

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

@@ -11,6 +11,7 @@ import {
   SSOToken,
   SSOVerifier,
 } from "talk-server/app/middleware/passport/strategies/verifiers/sso";
+import { TenantNotFoundError, TokenInvalidError } from "talk-server/errors";
 import { Tenant } from "talk-server/models/tenant";
 import { User } from "talk-server/models/user";
 import {
@@ -71,8 +72,7 @@ export class JWTStrategy extends Strategy {
   private async verify(tokenString: string, tenant: Tenant) {
     const token: Token = jwt.decode(tokenString);
     if (!token || typeof token === "string") {
-      // TODO: (wyattjoh) return a better error.
-      throw new Error("token could not be decoded");
+      throw new TokenInvalidError(tokenString, "token could not be decoded");
     }
 
     // TODO: add OIDC support.
@@ -94,8 +94,10 @@ export class JWTStrategy extends Strategy {
     }
 
     // No verifier could be found.
-    // TODO: (wyattjoh) return a better error.
-    throw new Error("no suitable jwt verifier could be found");
+    throw new TokenInvalidError(
+      tokenString,
+      "no suitable jwt verifier could be found"
+    );
   }
 
   public async authenticate(req: Request) {
@@ -109,8 +111,7 @@ export class JWTStrategy extends Strategy {
 
     const { tenant } = req.talk!;
     if (!tenant) {
-      // TODO: (wyattjoh) log this error, and return a better one?
-      return this.error(new Error("tenant not found"));
+      return this.error(new TenantNotFoundError(req.hostname));
     }
 
     try {
@@ -121,8 +122,7 @@ export class JWTStrategy extends Strategy {
 
       return this.success(user, null);
     } catch (err) {
-      // TODO: (wyattjoh) log this error
-      return this.fail(err);
+      return this.error(err);
     }
   }
 }

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

@@ -6,13 +6,13 @@ import now from "performance-now";
 import logger from "talk-server/logger";
 import { Tenant } from "talk-server/models/tenant";
 import { retrieveUser } from "talk-server/models/user";
-import { checkBlacklistJWT, JWTSigningConfig } from "talk-server/services/jwt";
+import { checkJWTRevoked, JWTSigningConfig } from "talk-server/services/jwt";
 
 export interface JWTToken {
   /**
    * jti is the Token identifier. With normal login tokens, this is a randomly
-   * generated uuid, which is added to a blacklist when the User "logs out". For
-   * Personal Access Tokens, this is the Token identifier.
+   * generated uuid, which is added to a revoke list when the User "logs out".
+   * For Personal Access Tokens, this is the Token identifier.
    */
   jti: string;
 
@@ -102,11 +102,11 @@ export class JWTVerifier {
     logger.trace({ responseTime }, "jwt verification complete");
 
     // Check to see if this is a Personal Access Token, these tokens cannot be
-    // blacklisted.
+    // revoked.
     if (!token.pat) {
-      // Check to see if the token has been blacklisted, as these tokens can be
+      // Check to see if the token has been revoked, as these tokens can be
       // revoked.
-      await checkBlacklistJWT(this.redis, token.jti);
+      await checkJWTRevoked(this.redis, token.jti);
     }
 
     // Find the user.

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

@@ -1,3 +1,4 @@
+import { TenantNotFoundError } from "talk-server/errors";
 import TenantCache from "talk-server/services/tenant/cache";
 import { RequestHandler } from "talk-server/types/express";
 
@@ -11,6 +12,14 @@ export const tenantMiddleware = ({
   passNoTenant = false,
 }: MiddlewareOptions): RequestHandler => async (req, res, next) => {
   try {
+    // Set Talk on the request.
+    req.talk = {
+      cache: {
+        // Attach the tenant cache to the request.
+        tenant: cache,
+      },
+    };
+
     // Attach the tenant to the request.
     const tenant = await cache.retrieveByDomain(req.hostname);
     if (!tenant) {
@@ -18,19 +27,11 @@ export const tenantMiddleware = ({
         return next();
       }
 
-      // TODO: send a http.StatusNotFound?
-      return next(new Error("tenant not found"));
+      return next(new TenantNotFoundError(req.hostname));
     }
 
-    // Set Talk on the request.
-    req.talk = {
-      cache: {
-        // Attach the tenant cache to the request.
-        tenant: cache,
-      },
-      // Attach the tenant to the request.
-      tenant,
-    };
+    // Attach the tenant to the request.
+    req.talk.tenant = tenant;
 
     // Attach the tenant to the view locals.
     res.locals.tenant = tenant;