Node.js resource based ACL

Question

I am implementing a simple Access Control system in Node, and I am wondering what can be the best approach for what I am doing.

I am using Node ACL and it is not clear to me how to block on a per-resource basis.

Let's take the following example: USER ->* PROJECT ->* ENTRY. Users can have multiple projects which contains many entries. Users can be ADMIN or USER.

I created an endpoint /entry/{ID} where user can access an entry detail. The endpoint is accessible to everyone, ADMINs can see all entries, but for User I need to do something similar:

app.get('/entry/{id}', (req, res) => {
    if (user.admin) {
        // Return eveything
    }
    else {
       if (entry.project == user.project) {
           // return it
       }
       else {
           // Unathorized
       }
    }
})

Is there a better approach/pattern to implement this checks on ownership on a resource?

Answer 1

It's a very broad question so I'll try to give you a couple hints as my answer, but

Is there an ACL pattern in javascript?

There's a number of solutions but I wouldn't call any of those a pattern. I'll be very subjective now, but the ways of passport.js and similar modules are non-transparent to say the least - and it's not really an ACL...

Someone may say - hey, it's node.js, there must be module to do that and make your node_modules heavier but searching for a good acl module in npm, I only found some outdated ones and tightly bound with express. Since your question wasn't which is the best npm module for acl I gave up looking for such at page 3, which doesn't mean there ain't something ready so you may want to look more closely.

I think your implementation could be considered acceptable, with some minor corrections or hints as I mentioned:

Separate your request logic from access control logic

In your code everything happens in one callback - that's definitely very efficient, but also very hard to support in longer term. You see, it'll end up in the same code in lots of those if's above in all the callbacks. It's very simple to separate the logic - simply implement the same path in two callbacks (they'll be run in the order they were defined), so:

app.all('/entry/{id}', (req, res, next) => {
    const {user, entry} = extractFromRequest(req);
    if (user.admin || entry.project === user.project) {
        next();
    } else {
        res.status(403).send("Forbidden");
    }
});

app.get('/entry/{id}', (req, res) => {
    // simply respond here
})

This way the first callback checks if the user has access and this won't affect the logic of the response. The usage of the next() is specific to express-like frameworks which I assumed you use looking at your code - when you call it the next handler will be executed, otherwise no other handlers will be run.

See Express.js app.all documentation for an acl example.

Use a service wide acl

It's much more secure to keep a basic ACL in a single place and not to define it per path unless necessary. This way you won't omit one path and won't leave a security hole somewhere in middle of request. For this we need to split the ACL into parts:

• URL access check (if path is public/open for all users)
• User and session validity check (user is logged in, session is not expired)
• Admin/user check (so permission level)
• Otherwise we don't allow anything.

    app.all('*', (req, res, next) => {
        if (path.isPublic) next(); // public paths can be unlogged
        else if (user.valid && user.expires > Date.now()) next(); // session and user must be valid
        else if (user.admin) next(); // admin can go anywhere
        else if (path.isOpen && user.valid) next(); // paths for logged in users may also pass
        else throw new Error("Forbidden");
    });

This check is not very restrictive but we won't need to repeat ourselves. Also notice the throw Error at the bottom - we'll handle this in an error handler:

app.use(function (err, req, res, next) {
    if (err.message === "Forbidden") res.status(403).send("Forbidden");
    else res.status(500).send("Something broke");
})

Any handler with 4 arguments will be considered an error handler by Express.js.

On the specific path level if there's any need for ACL's, simply throw an error to the handler:

app.all('/entry/{id}', (req, res, next) => {
    if (!user.admin && user.project !== entry.project) throw new Error("Forbidden");
    // then respond...
});

Which reminds me of another hint...

Don't use user.admin

Ok, fine, use it if you like. I don't. The first attempt to hack your code will be by trying to set admin on any object that has properties. It's a common name in a common security check so it's like leaving your WiFI AP login at factory defaults.

I'd recommend using roles and permissions. A role contains a set of permissions, a user has some roles (or one role which is simpler but gives you less options). Roles may be also assigned to project.

It's easily a whole article about this so here's some further reading on Role-based ACL.

Use standard HTTP responses

Some of this mentioned above, but it's a good practice to simply use one of standard 4xx HTTP code status as response - this will be meaningful for the client. In essence reply 401 when the user is not logged in (or session expired), 403 when there's no sufficient priviledge, 429 when use limits are exceeded. more codes and what to do when the request is a teapot in Wikipedia.

As to implementation itself I do like to create a simple AuthError class and use it to throw errors from the app.

class AuthError extends Error {
    constructor(status, message = "Access denied") {
        super(message);
        this.status = status;
    }
}

It's really easy to both handle and throw such an error in the code, like this:

app.all('*', (req, res, next) => {
    // check if all good, but be more talkative otherwise
    if (!path.isOpen && !user.valid) throw new AuthError(401, "Unauthenticated");
    throw new AuthError(403);
});

function checkRoles(user, entry) {
    // do some checks or...
    throw new AuthError(403, "Insufficient Priviledges");
}

app.get('/entry/{id}', (req, res) => {
    checkRoles(user, entry); // throws AuthError
    // or respond...
})

And in your error handler you send your status/message as caught from your code:

app.use(function (err, req, res, next) {
    if (err instanceof AuthError) res.send(err.status).send(err.message);
    else res.status(500).send('Something broke!')
})

Don't reply immediately

Finally - this is more of a security feature and a safety feature at the same time. Every time you respond with an error message, why not sleep a couple seconds? This will hurt you in terms of memory, but it will hurt just a little and it'll hurt a possible attacker a lot because they wait for the outcome longer. Moreover it's super simple to implement in just one place:

app.use(function (err, req, res, next) {
    // some errors from the app can be handled here - you can respond immediately if
    // you think it's better.
    if (err instanceof AppError) return res.send(err.status).send(err.message);
    setTimeout(() => {
        if (err instanceof AuthError) res.send(err.status).send(err.message);
        else res.status(500).send('Something broke!')
    }, 3000);
})

Phew... I don't think this list is exhaustive, but in my view it's a sensible start.