makeWrapPlansPlugin throws error when adding dependency on a context value

[saranrapjs]

Summary

I'm trying to use makeWrapPlansPlugin to try to check context() values before allowing some plans to execute. I'm trying to do this in a way that may or may not be correct, by adding a lambda step checking the context value as a dependency of the step/plan being wrapped:

const WrapPlansPlugin = makeWrapPlansPlugin(
  (context) => {
    return // some filtering logic here
  },
  (_scope) => (plan, $field, _fieldArgs, _fieldInfo) => {
    const $ctxValue = context().get("valueFromContext");
    const $checkContextStep = lambda($ctxValue, (valueFromContext) => {
      if (!someCustomLogic(valueFromContext)) {
        throw new Error('this resolver should fail');
      }
      return null;
    });
    // @ts-ignore
    $field.addDependency($checkContextStep);
    const $result = plan();
    return $result;
  },
);

Confusingly, this sometimes works, but sometimes throws this error:

Error occurred during pushDown; whilst processing Lambda{2}[25] in dependents-first mode an error occurred: Error: GrafastInternalError<64c07427-4fe2-43c4-9858-272d33bee0b8>: invalid layer plan heirarchy

I have a feeling I'm misusing or misunderstanding the dependency/plan/step system! And am curious if this should work, or if I should be using some other strategy altogether to run some code which executes before every field-scoped resolver automatically produced by postgraphile v5.

(I'm working with the team at the New York Times, in case that helps from a GitHub triage pov!)

[benjie]

Ah; I see what's happening here I think. You're abusing addDependency and it's biting you. It's 1am here so this will be a quick/rough breakdown...

There are up to three layers involved: 0 <= Y <= Z.

flowchart TD
  0 -.-> Y
  Y --> Z

or

flowchart TD
  0 -.-> Y["Y = Z"]

You're currently in layer Z (e.g. 3). Every step that your plan function creates will be created in layer Z automatically.

$field is fed into your step; and it belongs to layer Y (it's pre-existing). This may or may not be the same as Z; remember: Y <= Z.

You then do context() (which is a pre existing step from layer 0, the root layer) and context().get("valueFromContext") which will create an access plan in layer Z.

You create a lambda of this, which is also created in layer Z.

Then you add this lambda (layer Z) as a dependency of $field (layer Y) by abusing the addDependency API. For something to be a dependency it must be in the same or a numerically lower layer (i.e. closer to the root). So you've implicitly added the constraint Z <= Y.

X <= Y <= Z and Z <= Y means that Y must equal Z. If Y doesn't equal Z then you get an invalid hierarchy, as you see here. This is probably why it sometimes works and sometimes doesn't - it depends what layer $field belongs to - the current layer, or a previous layer.

$field is an input to your plan, it is definitely not the right time to modify it (that should be done in the parent field that creates $field in the first place).

As for what you should be doing; I'll have to get back to you when I'm more awake.

[benjie]

Regarding what you actually want, it sounds like you're after the "early exit" functionality (https://github.com/graphile/crystal/issues/2013) which we don't have yet but was recently discussed at the Grafast WG: https://github.com/grafast/wg/blob/main/notes/2023-10-24.md#5-early-exit-proposal-benjie

As a workaround, you might be able to add a lambda step with a side effect which is to throw an error. I wouldn't, personally, recommend that approach though.

[saranrapjs]

Thanks, this overview was helpful for my understanding of the abstractions at play here!

Regarding what you actually want, it sounds like you're after the "early exit" functionality

That sounds right to me.

As a workaround, you might be able to add a lambda step with a side effect which is to throw an error. I wouldn't, personally, recommend that approach though.

Would this be different from the lambda I'd created in my example within makeWrapPlans? Perhaps added in a different place? I tried setting that step to use . hasSideEffects = true, which produces another error; if this just isn't supported yet, that's fine too, but if there's a different/other way to attach the lambda such that it's no longer created within layer Z, I'd be interested to try it out

[benjie]

Really I need to know more about specifically what you're trying to accomplish; but I think "this just isn't supported yet" is generally accurate; there are certain workarounds in certain circumstances but they're heavily dependent on what's going on.

[saranrapjs]

Roger that. I'm trying to implement access control in a way that's field-aware — e.g. I'd like to propagate request-scoped authorization details down into field-scoped resolvers, in order to early exit where a given field isn't authorized for execution. I wasn't sure how to accomplish this:

Plan resolvers can then use the standard context() step to extract the relevant information and pass it through to the business logic.

...where the business logic I'd like to effect a change to are steps/plans that are generated by Postgraphile plugins.

[benjie]

Yeah the issue is that the access control detailed in the Grafast docs is where you have written your own business logic in raw JS, and then you take things from GraphQL and feed them into your business logic via Grafast. Remember: Grafast is equivalent to graphql-js; it is there to implement planning/execution on top of any business logic whatsoever.

PostGraphile treats the database as the business logic, so we pass the information through to the database so it can make the decisions (via RLS, GRANTs, triggers, etc); but what you're trying to do is do it at the planning stage. One of the big issues you'll get from this is side-channel attacks. Say, for example, you have a users table with an email field and you only want that email field to be visible to the current user, all other users should return null for email. Seems simple enough; until you realise there's tonnes of other ways to get that data. For example; you might extract it by base64 decoding the cursor of a collection fetch by incorporating EMAIL_ASC into the orderBy. Or you might do { currentUser { friends(filter:{email:{startsWith: "a"}}) { name } } } and perform a dictionary attack to determine your friends email. Or any number of other ways depending on what plugins you are using (aggregation, etc).

In a PostGraphile context, it's much more secure to have the database enforce your permissions for you; then no matter what query the user issues they'll never be able to see things the database forbids. In most cases, I strongly recommend splitting your database tables on permission boundaries so that you can fully leverage RLS to ensure your security.

---

Another thing to consider is that if you say this thing shouldn't run if certain things aren't true, then you're explicitly implementing an optimization boundary that prevents us from performing inlining of the requested result. This will result in more queries to the database. If all you want to do is "null out" the data at the end (fetch it, but throw it away conditionally) then instead of attempting to do what you're doing above, you can simply return a derivative - this is very much what makeWrapPlansPlugin is intended for. This example is based on one from the docs:

import { makeWrapPlansPlugin } from "postgraphile/utils";
import { context, lambda } from "postgraphile/grafast";

export default makeWrapPlansPlugin({
  User: {
    email(plan, $user) {
      // Get the id for this user
      const $userId = $user.get("id");
      // Get the id of the currently logged in user according to the GraphQL context
      const $currentUserId = context().get("currentUserId");
      // Run the original plan
      const $email = plan();
      // Return a derivative of the previous result
      return lambda(
        [$userId, $currentUserId, $email],
        ([userId, currentUserId, email]) => userId === currentUserId ? email : null
      );
    },
  },
});

[benjie]

Current thoughts on detecting this:

1. addDependency should not be callable in plan phase once the plan has had arguments finalized (we should still allow it in optimize/deduplicate/etc though?)
2. addDependency should throw early if it can detect you're trying to depend on a plan that would make an invalid heirarchy

[benjie]

(Early exit support went out in Beta22 yesterday. Currently undocumented.)

[benjie]

Closing this issue in favour of #1837