3

I'm using koa2 and koa-router together with sequelize on top. I want to be able to control user access based on their roles in the database, and it's been working somewhat so far. I made my own RBAC implementation, but I'm having some trouble.

I need to quit execution BEFORE any endpoint is hit if the user doesn't have access, considering endpoints can do any action (like inserting a new item etc.). This makes perfect sense, I realize I could potentially use transactions with Sequelize, but I find that would add more overhead and deadline is closing in.

My implementation so far looks somewhat like the following:

// initialize.js

initalizeRoutes()
initializeServerMiddleware()

Server middleware is registered after routes.

// function initializeRoutes
app.router = require('koa-router')

app.router.use('*', access_control(app))

require('./routes_init')

routes_init just runs a function which recursively parses a folder and imports all middleware definitions.

// function initializeServerMiddleware
// blah blah bunch of middleware
app.server.use(app.router.routes()).use(app.router.allowedMethods())

This is just regular koa-router.

However, the issue arises in access_control.

I have one file (access_control_definitions.js) where I specify named routes, their respective sequelize model name, and what rules exists for the route. (e.g. what role, if the owner is able to access their own resource...) I calculate whether the requester owns a resource by a route param (e.g. resource ID is ctx.params.id). However, in this implementation, params don't seem to be parsed. I don't think it's right that I have to manually parse the params before koa-router does it. Is anyone able to identify a better way based on this that would solve ctx.params not being filled with the actual named parameter?

edit: I also created a GitHub issue for this, considering it seems to me like there's some funny business going on.

Nict
  • 3,066
  • 5
  • 26
  • 41

1 Answers1

7

So if you look at router.js

layerChain = matchedLayers.reduce(function(memo, layer) {
  memo.push(function(ctx, next) {
    ctx.captures = layer.captures(path, ctx.captures);
    ctx.params = layer.params(path, ctx.captures, ctx.params);
    ctx.routerName = layer.name;
    return next();
  });
  return memo.concat(layer.stack);
}, []);

return compose(layerChain)(ctx, next);

What it does is that for every route function that you have, it add its own capturing layer to generate the params

LayerMap

Now this actually does make sense because you can have two middleware for same url with different parameters

router.use('/abc/:did', (ctx, next) => {
    // ctx.router available
    console.log('my request came here too', ctx.params.did)
    if (next)
        next();
});

router.get('/abc/:id', (ctx, next) => {
    console.log('my request came here', ctx.params.id)
});

Now for the first handler a parameter id makes no sense and for the second one parameter did doesn't make any sense. Which means these parameters are specific to a handler and only make sense inside the handler. That is why it makes sense to not have the params that you expect to be there. I don't think it is a bug

And since you already found the workaround

const fromRouteId = pathToRegexp(ctx._matchedRoute).exec(ctx.captures[0])

You should use the same. Or a better one might be

var lastMatch = ctx.matched[ctx.matched.length-1];
params = lastMatch.params(ctx.originalUrl, lastMatch.captures(ctx.originalUrl), {})
Tarun Lalwani
  • 142,312
  • 9
  • 204
  • 265
  • Thank you for your detailed answer. I wasn't certain this was in fact a bug, but rather a logical/architectural error on my part. If you have any input towards my workaround that would be greatly appreciated, as it isn't as robust as I'd like it to be. – Nict Apr 09 '18 at 11:35
  • 1
    @NicT, see the updated answer, the code I posted might be more reliable. You can improve it even more by inspect the layers and deciding which one to pick or you can pick even all of them and update params in a loop – Tarun Lalwani Apr 09 '18 at 12:01
  • Thank you a million times over. This is exactly the solution I was looking for, I just had an uneasy feeling. :) – Nict Apr 11 '18 at 07:27
  • It seems to me that you need `ctx.path`, not `ctx.originalUrl`, as the original url contains the querystring, which will cause the regex to fail. The `Layer.prototype.captures` expects a `path`. – nicodemus13 Jun 14 '18 at 07:54