Basic node login routes for authentication system

Question

I am building a basic authentication system with a node backend and wonder whether I am using the try-catch block excessively. I have this example controller:

export const getUser = async (req: Request, res: Response) => {
  try {
    const user = await User.findById(req.body.id);

    if (!user) {
      return res.status(404).send('No user found');
    }

    res.send(user);
  } catch (err: any) {
    return res.status(400).send(err.message);
  }
};

And this findById User class method:

  static async findById(id: string): Promise<any> {
    // try {
    const collection = getCollection(USERS_COLLECTION_NAME);
    const user = await collection.findOne({ _id: new mongodb.ObjectId(id) });
    return user;
    // } catch (err) {
    //   throw err;
    // }
  }

The controller already uses a try-catch block, so is having another try-catch block in the class method necessary? It still works after commenting out the try-catch block. How far should its usage be taken?

To give some more context, here is the User class:

import { USERS_COLLECTION_NAME } from '../utils/db';
import { getCollection } from '../utils/helpers';
import jwt from 'jsonwebtoken';
const mongodb = require('mongodb'); // do not convert to import (otherwise undefined)

// does anywhere here NOT need try catch?

export class User {
  email: string;
  hashedPassword: any;
  dateCreated: Date;

  constructor(email: string, hashedPassword: any) {
    this.email = email.toLowerCase();
    this.hashedPassword = hashedPassword;
    this.dateCreated = new Date();
  }

  signToken(): string {
    try {
      // { ...this } overcomes error `Expected "payload" to be a plain object`
      const token = jwt.sign({ ...this }, this.email, {
        expiresIn: 60 * 24,
      });
      return token;
    } catch (err) {
      throw err;
    }
  }

  async saveToDb(): Promise<any> {
    try {
      const collection = getCollection(USERS_COLLECTION_NAME);
      const saveResult = await collection.insertOne(this);
      return saveResult;
    } catch (err) {
      throw err;
    }
  }

  static async delete(id: string): Promise<any> {
    try {
      const collection = getCollection(USERS_COLLECTION_NAME);
      const user = await User.findById(id);

      if (!user) throw new Error('404 - User not found');

      const deleteResult = await collection.deleteOne(user);

      if (!deleteResult) throw new Error('500 - Could not delete user');

      return deleteResult;
    } catch (err) {
      throw err;
    }
  }

  static async fetchAll(): Promise<any> {
    try {
      const collection = getCollection(USERS_COLLECTION_NAME);
      const users = await collection.find().toArray(); // @todo: may need to optimise as it gets ALL users - what if a million users existed?
      return users;
    } catch (err) {
      throw err;
    }
  }

  static async findByEmail(email: string): Promise<any> {
    try {
      const collection = getCollection(USERS_COLLECTION_NAME);
      const user = await collection.findOne({ email });
      return user;
    } catch (err) {
      throw err;
    }
  }

  static async findById(id: string): Promise<any> {
    // try {
    const collection = getCollection(USERS_COLLECTION_NAME);
    const user = await collection.findOne({ _id: new mongodb.ObjectId(id) });
    return user;
    // } catch (err) {
    //   throw err;
    // }
  }
}

And the controllers:

import { Request, Response } from 'express';
import { User } from '../models/auth';
import bcrypt from 'bcrypt';
import { USERS_COLLECTION_NAME } from '../utils/db';
import { getCollection } from '../utils/helpers';
const mongodb = require('mongodb'); // do not convert to import (otherwise undefined)

export const createUser = async (req: Request, res: Response) => {
  const { email, password } = req.body;
  const existingUser = await User.findByEmail(email);

  if (existingUser) {
    return res.status(409).send('User already exists. Please log in.');
  }

  const hashedPassword = await bcrypt.hash(password, 10);
  const newUser = new User(email, hashedPassword);
  const saveToDb = await newUser.saveToDb();

  if (!saveToDb) {
    return res.status(500).send('Could not insert user into the database.');
  }

  const token = newUser.signToken();

  res.status(201).json({ token, id: saveToDb.insertedId });
};

// only updates email for now...
export const updateUser = async (req: Request, res: Response) => {
  const collection = getCollection(USERS_COLLECTION_NAME);

  try {
    const updateResult = await collection.updateOne(
      {
        _id: new mongodb.ObjectId(req.body.id), // new mongodb.ObjectId needed, otherwide null; converts to BSON
      },
      {
        $set: {
          email: req.body.email,
        },
      }
    );

    if (!updateResult.modifiedCount) {
      return res.status(500).send('Could not update user.');
    }

    res.send({ updateResult });
  } catch (err: any) {
    return res.status(400).send(err.message);
  }
};

export const deleteUser = async (req: Request, res: Response) => {
  const { id } = req.body;

  try {
    const deleteUserResult = await User.delete(id);
    res.send({ deleteUserResult });
  } catch (err: any) {
    return res.status(500).send(err.message); // would prefer to set status code from err as it's not always 500! e.g. the delete class method can throw a 404 or 500 error
  }
};

export const getUser = async (req: Request, res: Response) => {
  try {
    const user = await User.findById(req.body.id);

    if (!user) {
      return res.status(404).send('No user found');
    }

    res.send(user);
  } catch (err: any) {
    return res.status(400).send(err.message);
  }
};

export const getUsers = async (req: Request, res: Response) => {
  const users = await User.fetchAll();

  if (!users) {
    return res.status(404).send('No users found');
  }

  res.send(users);
};

export const logUserIn = async (req: Request, res: Response) => {
  const { email, password } = req.body;
  const user = await User.findByEmail(email);

  if (!user) {
    return res.status(404).send('User not found. Please create an account.');
  }

  const correctPassword = await bcrypt.compare(password, user.hashedPassword);

  if (!correctPassword) {
    return res.status(403).send('Wrong password for this account.');
  }

  // assign object methods to the user instance as objects retrieved from db don't have methods
  Object.setPrototypeOf(user, User.prototype);
  const token = user.signToken();
  return res.json({ token, id: user._id });
};

Any other tips or suggestions would also be greatly appreciated, thank you!

Answer 1

Short answer is -- yes, you're using try/catch excessively.

---

When working with errors in general, you'll want to determine which type of error you're throwing.

1. Did the user make a mistake? (400)
2. Is the user accessing a resource they're not allowed to see? (403)
3. Did an unexpected error occur? ⚠️ (5XX)
4. ...etc

You want to be able to differentiate between these different types, because sometimes there are errors you want the user to know about and some others you'd rather not want to expose.

For example, if a malicious user is trying to gain access to your system and they're able to trigger an error (5XX) and you're generous eough to provide them with what exactly went wrong.. they're in heaven.

Usually the standard is to

1. Create custom HTTP errors
2. Have a single try/catch block to forward the error with next.
3. Have a middleware to handle errors

Here is an example, suppose you create a few http errors.

class AppError extends Error {
  constructor(message, status, code) {
     super(message);
     this.status = status;
     this.code = code;
  }
}

class BadRequestError extends AppError {
  constructor(message, code) {
    super(message, 400, code);
  }
}

class NotFoundError extends AppError {
  constructor(message, code) {
    super(message, 404, code);
  }
}

class InternalServerError extends AppError {
  constructor(message, code) {
    super(message, 500, code);
  }
}

You create and register an error handling middleware.

app.use(function(err, req, res, next) {
   if(err instanceof InternalServerError) {
     console.error(err);
     // Do not expose the internal server error to the client
     res.status(err.status).send('Internal Server Error');
     return;
   }
   if(err instanceof AppError && err.status >= 400 && err.status < 500) {
     res.status(err.status).send(err.message);
     return;
   }
   // If nothing matched up till now then you're dealing with
   // an error that you did not expect
   console.error(err);
   res.status(500).send('Unexpected Internal Server Error');
   return;
});

ℹ️ Consider using winston library for logging stuff.

Then in your controller forward the error.

export const getUser = async (req: Request, res: Response) => {
  try {
    const user = await User.findById(req.body.id);

    if (!user) {
      throw new NotFoundError('No user found');
    }

    res.send(user);
  } catch (err: any) {
    next(err);
  }
};

Then in your code when you're throwing an error

  static async delete(id: string): Promise<any> {
      const collection = getCollection(USERS_COLLECTION_NAME);
      const user = await User.findById(id);

      if (!user) throw new NotFoundError('User not found');

      const deleteResult = await collection.deleteOne(user);

      if (!deleteResult) throw new InternalServerError('Could not delete user');

      return deleteResult;
  }