3
\$\begingroup\$

Background:

I have been working on a service which allows users to signup on different apps and on each login checks if the request is valid or not based on a series of checks.

The below snippet is small part of the whole application but covers my question and is working fine for now:

Code:

'use strict';

const bcrypt = require('bcrypt');
const boom = require('boom');
const joi = require('joi');
const flatten = require('lodash/flatten');
const pick = require('lodash/pick');

const models = require('../../models');
const { AccessToken, App, User } = models;

const debug = require('debug')('microauth:test');

const loginSchema = joi
  .object({
    appname: joi.string().required(),
    email: joi.string().required(),
    password: joi.string().required(),
  })
  .required();

async function run(req, res, next) {
  const { appname, email, password } = joi.attempt(req.body, loginSchema);

  const app = await getApp(appname);
  if (!app) {
    throw boom.badRequest(`Invalid app name: ${appname}.`);
  }
  if (app.isInactive()) {
    throw boom.badRequest('App is not active.');
  }

  const { isAuthorized, user } = await authorize({ email, password });
  if (!user) {
    throw boom.notFound('User not found.');
  }
  debug(`User ${user.get('email')} is authorised? ${isAuthorized}`);
  if (!isAuthorized) {
    throw boom.unauthorized('Invalid email or password.');
  }

  const { result } = await isUserBelongsToApp(user, app.get('name'));
  if (!result) {
    throw boom.badRequest(`User is not authorised to access app.`);
  }

  return successResponse(email, app.get('secret'), res);
}

async function getApp(name) {
  return await App.findOne({ name });
}

async function authorize({ email, password }) {
  const user = await User.findOne(
    { email, status: 'active' },
    { withRelated: ['apps', 'roles.permissions'] }
  );

  let isAuthorized = false;
  if (user) {
    isAuthorized = await bcrypt.compare(password, user.get('password'));
  }
  return { isAuthorized, user };
}

async function isUserBelongsToApp(user, appname) {
  let result = false;
  let app = null;
  app = user.related('apps').findWhere({ name: appname });
  if (app) {
    result = true;
  }
  return { result, app };
}

async function successResponse(email, secret, res) {
  const userFields = [
    'device',
    'email',
    'firstname',
    'language',
    'lastname',
    'phone',
    'uid',
  ];
  const roleFields = ['name', 'description'];
  const permissionFields = ['name', 'object', 'action'];

  let user = await User.findOne(
    {
      email: email,
    },
    {
      withRelated: ['roles.permissions'],
    }
  );
  user = user.toJSON();
  const result = Object.assign({}, { ...user });
  result.roles = [];
  result.permissions = [];

  if (user.roles) {
    result.roles = user.roles.map(role => pick(role, roleFields));
    result.permissions = user.roles.map(role => {
      return role.permissions.map(permission =>
        pick(permission, permissionFields)
      );
    });
  }
  result.permissions = flatten(result.permissions);
  const { token, expiration } = new AccessToken(secret).create(result);
  res.json({ token, expiration });
}

module.exports = run;

Questions:

The code above belongs to the controller of the applications, is that the right place to do all these checks?

Right now the main logic seems pretty obvious but each step depends of the previous step. Is there any better way to write the same logic?

\$\endgroup\$
  • \$\begingroup\$ Please accept an answer or put a bounty. This helps users know if you need further assistance or if you are satisfied with what you have gotten. \$\endgroup\$ – FreezePhoenix Oct 4 '18 at 13:24
2
\$\begingroup\$

This is only a partial review.

I would declare your constants outside of your functions, as they are constant. Also, the constants you did declare outside of your functions should be chained. Finally, you should never call a function more than once. If you are truly using then you should do the following:

const required = joi.string().required(),
      loginSchema = joi
  .object({
    appname: required,
    email: required,
    password: required,
  })
  .required();

because a given function must return the same output for the same input.

Rewrite

'use strict';

const bcrypt = require('bcrypt'),
      boom = require('boom'),
      joi = require('joi'),
      flatten = require('lodash/flatten'),
      pick = require('lodash/pick');

const models = require('../../models'),
      { AccessToken, App, User } = models;

const debug = require('debug')('microauth:test');

const userFields = [
    'device',
    'email',
    'firstname',
    'language',
    'lastname',
    'phone',
    'uid',
  ],
      roleFields = ['name', 'description'],
      permissionFields = ['name', 'object', 'action'];

const required = joi.string().required(),
      loginSchema = joi
  .object({
    appname: required,
    email: required,
    password: required,
  })
  .required();

async function run(req, res, next) {
  const { appname, email, password } = await joi.attempt(req.body, loginSchema);

  const app = await getApp(appname);
  (!app) && (throw boom.badRequest(`Invalid app name: ${appname}.`);)

  (app.isInactive()) && (throw boom.badRequest('App is not active.');)

  const { isAuthorized, user } = await authorize({ email, password });
  (!user) && (throw boom.notFound('User not found.');)

  debug(`User ${user.get('email')} is authorised? ${isAuthorized}`);
  (!isAuthorized) && (throw boom.unauthorized('Invalid email or password.');)

  const { result } = await isUserBelongsToApp(user, app.get('name'));
  (!result) && (throw boom.badRequest(`User is not authorised to access app.`);)

  return successResponse(email, app.get('secret'), res);
}

async function getApp(name) {
  return await App.findOne({ name });
}

async function authorize({ email, password }) {
  const user = await User.findOne(
    { email, status: 'active' },
    { withRelated: ['apps', 'roles.permissions'] }
  );

  let isAuthorized = false;
  if (user) {
    isAuthorized = await bcrypt.compare(password, user.get('password'));
  }
  return { isAuthorized, user };
}

async function isUserBelongsToApp(user, appname) {
  let result = false;
  let app = null;
  app = user.related('apps').findWhere({ name: appname });
  if (app) {
    result = true;
  }
  return { result, app };
}

async function successResponse(email, secret, res) {

  let user = await User.findOne(
    {
      email: email,
    },
    {
      withRelated: ['roles.permissions'],
    }
  );
  user = user.toJSON();
  const result = Object.assign({}, { ...user });
  result.roles = [];
  result.permissions = [];

  if (user.roles) {
    result.roles = user.roles.map(role => pick(role, roleFields));
    result.permissions = user.roles.map(role => {
      return role.permissions.map(permission =>
        pick(permission, permissionFields)
      );
    });
  }
  result.permissions = flatten(result.permissions);
  const { token, expiration } = new AccessToken(secret).create(result);
  res.json({ token, expiration });
}

module.exports = run;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.