ACL express middleware implementation

Question

I come to see you to help me improve an express middleware that allows you to control access resources by checking the rights of the user. The following code is supposed to be as generic as possible so that it can easily be added to another project.

But I have doubts both on its genericity and the clarity of its prototype, I let you judge :)

acl.ts

export let config: ACLConfig = {
  userField: 'me',
  roleField: 'roles'
};

export function isAllowed(roles: String[], ...cbs: Function[]) {
  return async function (req, res, next) {
    let allowed = false;
    let user = req[config.userField];
    if (!user) throw new APIError('ACL module cannot find req.{{userField}}', APICodes.ACL_INTERNAL_ERROR);

    let userRoles = user[config.roleField] || '';
    userRoles = userRoles.split(',');

    if (roles && roles.length) {
      for (let role of roles) {
        if (userRoles.indexOf(role) !== -1) return next();
      }
    }

    if (cbs && cbs.length) {
      for (let cb of cbs) {
        if (cb(req) === true) return next();
      }
    }

    throw new APIError('You do not have the required privileges', APICodes.AUTH_ACCESS_FORBIDDEN, HttpStatus.UNAUTHORIZED, true);
  }
}

must be used like this

route('/...').all(verifyToken, isAllowed(['staff, admin'], policies.isMyArticle))

maybe this following prototype is better ?

isAllow(...args: String | Function)
isAllow('staff', policies.isMyArticle, 'staff');

Answer 1

The code you've provided is fairly thin and so there's not to much to pick at here, however, I think your checks on roles and cbs can be simplified using Array.isArray(), because the for of iterator cannot iterate an empty array.

if (roles && roles.length) {
  for (let role of roles) {
    if (userRoles.indexOf(role) !== -1) return next();
  }
}

can be transformed to:

if (Array.isArray(roles)) {
  for (let role of roles) {
    if (userRoles.indexOf(role) !== -1) return next();
  }
}

Although I think these checks would be better moved up and for you to return a 500 Server Error if roles or cbs aren't arrays.

In the same block of code you have indexOf(role) !== -1, which was simplified to .includes(role) in ES6.

The only other thing that comes to mind is the userRoles.split(',') call, because, if userRoles isn't a string, it'll raise an exception. However, that would depend on how malleable your database / validation is.

Additionally, your second suggested prototype has way more confusing usage than your implementation, I would stick with what you've created.

Answer 2

After a few hours of research for a good logical and semantic prototype, i found a good solution.

router.put('/:articleId', AuthGuard([{ role: 'user', when: isOwner }, { role: 'admin' }]), async (req, res) => {

});

A predicate function can be added to a specific role for more flexibility. In this example, the user can only update an item "when" it owns it.

The little module :

import passport from 'passport';
import { Request, Response, NextFunction } from 'express';

export const AuthenticateGuard = passport.authenticate('jwt', { session: false });

export interface IAuthorizeItem {
  role: string;
  when?: (req: Request) => boolean;
};

export default function AuthGuard(items?: IAuthorizeItem[]) {
  return [AuthenticateGuard, AuthorizeGuard(items)];
};

export function AuthorizeGuard(items?: IAuthorizeItem[]) {
  return async function (req: Request, res: Response, next: NextFunction) {
    let user = req.user;
    if (!user) {
      return res.status(500).send('Access not allowed !');
    }

    if (typeof req.user.hasRole !== 'function') {
      throw new Error('User object must have hasRole method');
    }

    if (!items || items.length == 0) {
      return next();
    }

    for (let item of items) {
      if (req.user.hasRole(item.role)) {
        if (!item.when) {
          return next();
        }
        else {
          if (item.when(req)) {
            return next();
          }
        }
      }
    }

    res.status(500).send('Access not allowed !');
  }
};

Note: I avoid any dynamic types checking because i used typescript and this module must be used only with static object.