How do I deny access to a resource in single resource requests?

[carlbennettnz]

In requests for collections of resources, I can remove specific resources by having a beforeRender transform return undefined.

beforeRender(resource) {
  return isAllowed ? resource : undefined;
}

That works great, but using the same filter on a request for a specific, disallowed resource (/resource/id) results in a 500 error.

I could change the transform function to instead throw an error if something isn't allowed.

beforeRender(resource) {
  if (isAllowed) {
    return resource;
  } else {
    throw new APIError(403);
  }
}

But now the first example with multiple resources doesn't work.

How should I be doing this?

[ethanresnick]

Right. So, the hacky way to do this would be to inspect the express/koa request object in beforeRender (it's passed in as the second argument) and use that to determine if a single resource request is being made or not. If so, you'd switch from returning undefined to throwing an error. Again, this is hacky, but it should work.

Another imperfect solution would be to 403 at some firewall layer (e.g. if you're using express simple firewall) before the request ever hits the JSON API library. That would only work though if you could determine whether to 403 based largely on the URI, without having the resource object's data. So I'd probably stick with the first approach for now, hacky as it is.

Longer term..... I think my rationale for not supporting return undefined in the single resource case was that it doesn't give the framework enough info to properly create the response (e.g. should it be a 404? a 403? a 200 with a null body?), but this decision should probably be revisited. I'm slowly working on a big refactor of the transform system, so I'll address this as part of that work.

If you have an interim solution that you think would be an improvement and that deals with the issue mentioned above, I'd also accept a PR.

[carlbennettnz]

Thanks for the detailed response. I'm already using my own fork to add DELETE request filtering, so I think for now I'll just patch the transform handler to make it do what I need. Not ideal, but this means I can avoid adding any further complexity to my transforms.

// src/steps/apply-transform.js
if(toTransform instanceof Resource) {
  const doTransform = transform(toTransform, frameworkReq, frameworkRes, mode, registry);

  return Promise.resolve(doTransform).then(transformed => {
    if (transformed == null) {
      throw new APIError(403, undefined, 'Forbidden');
    } else {
      return transformed;
    }
  });
}

[ethanresnick]

Sounds good! I'll leave this open as a reminder to myself.

[ethanresnick]

This has been addressed in c79ec4e. In particular, you can now uniformly return undefined to remove a resource in v3. However, as discussed above, the library can't know that this should trigger an error or what error it should be (404, 403, etc), so the result is a 200 with data: null. A query transform on single resource requests can be used to map this to the error of your choice, e.g.:

app.get('/:type/:id', // singular resource requests
  Front.transformedAPIRequest((req, query) => {
    const origReturning = query.returning;

    return query.resultsIn((...args) => {
      const result = origReturning(...args);
      const resultDoc = result.document;

      // mutate the result document if its primary data is null, 
      // replacing the data with the error of your choice.
      // in the future, there'll likely be an immutable way to do this.
      if(resultDoc && resultDoc.primary && resultDoc.primary.toJSON() === null) {
        delete resultDoc.primary;
        resultDoc.errors = [new APIError(...)];
      }

      return result;
    })
  })
);

[carlbennettnz]

Awesome, thanks!

This solution's working for me at the moment. Ideally though, I would like to avoid loading the records in the first place. My team's planning to add a hook to our database adapter (I wrote my own knex one recently, as we're switching to Postgres) to add to the query's filters before they hit the database. This would also allow us to use pagination, which we're really starting to need in a few places.

[ethanresnick]

Ideally though, I would like to avoid loading the records in the first place. My team's planning to add a hook to our database adapter (I wrote my own knex one recently, as we're switching to Postgres) to add to the query's filters before they hit the database.

Definitely take a look at the v3 beta (docs, upgrading steps). I've reworked the underlying abstractions to be much more flexible.

Regarding not loading the records in the first place, for example: the new query transform system makes it possible to add to the query's filters out of the box, in a generic way, with no need to modify the adapter. Basically, it's something like this:

Front.transformedAPIRequest((req, query) => {
  return query.andWhere({
    /* add new filter criteria here. andWhere() returns 
       a new query instance with these filters added */
  });
});

I realize you've customized the library over time, so a straight upgrade to v3 may be hard/not worth it. That said, the fact that you're working on the same API two years later makes me think it's a long-term investment, so I'd at least take a look, since v3 really does provide a much more solid foundation.

The biggest sticking point may be the new filter syntax in v3, since presumably you still have to support the old syntax for backwards compatibility. However, the filter parser is totally customizable in v3, so supporting the old syntax is probably as easy as constructing your API controller like:

// Support the old filter syntax by parsing old-style query strings into 
// the new, adapter-agnostic filter predicate objects. See MongooseAdapter 
// for how to apply these parsed filters to your query.
new API(registry, { 
  filterParser(legalUnary, legalBinaryOps, rawQuery, params) {
    const filtersByField = (params.filter && params.filter.simple) || {};

    return Object.keys(filtersByField).reduce((acc, fieldName) => {
      const operatorsForField = Object.keys(filtersByField[fieldName] || {});

      if(!operatorsForField.every(op => legalBinaryOps.includes(op))) {
         throw new Error("Invalid filter operator"); // could be an APIError
      }

      return acc.concat(operatorsForField.map(operator => ({ 
          field: fieldName, 
          operator, 
          value: String(operatorsForField[operator])
        })
      );
    }, []);
  }
})

[ethanresnick]

Oh, also, having a Postgres adapter for this library would be huge! Can you share the Knex-based adapter that you wrote?

[carlbennettnz]

Woah, didn't realise how much you've been doing for v3. Loving the TypeScript. Will definitely jump back your version if feasible. Our fork's been diverging for a while now, but it looks like your v3 covers at least the majority of things we've bolted on.

Can you share the Knex-based adapter that you wrote?

Well we haven't got it running in production yet. Our switchover is planned for early next week. Test coverage is pretty good though. I'll add you to the repo.

[carlbennettnz]

(Invite sent. resapi is the name of our fork. The repo should be named 'resapi-knex'. There's not much specific to Postgres in there.)

[ethanresnick]

Woah, didn't realise how much you've been doing for v3. Loving the TypeScript. Will definitely jump back your version if feasible. Our fork's been diverging for a while now, but it looks like your v3 covers at least the majority of things we've bolted on.

Thanks. Also, if there are particular things v3's missing that you need, we can look at adding them to make the migration easier.

Invite sent. Awesome, thanks!