Req Access Pattern Overrides ApolloServer's Context Config

[Phylodome]

Cross-post and extension of this issue, as it may be more readily addressed here.

At present, graphql-auth-directives usage effectively co-opts the entire context object via returning only one sub-field of the context object it processes:

const server = new ApolloServer({
  schema,
  context: ({ req }) => { /* Here we pluck the variable `req` from context, via a destructured fn param */
    return req; /* Here we substitute req for the entire context object from which it came */
  }
});

If we inspect graphql-auth-directives' verifyAndDecodeToken function, we find that it takes for granted that the context object is the req object, which would not be the case if one were to configure the context object with both driver and req, as is necessary when instantiating ApolloServer.

Presently, if one instantiates ApolloServer like so:

const driver = neo4j.driver(
  "bolt://localhost:7687",
  neo4j.auth.basic("neo4j", "letmein")
);
const server = new ApolloServer({
  schema,
  context: ({ req }) => {
    return {req, driver};
  }
});

Then the logic within graphql-auth-directives will no longer find the auth header:

var verifyAndDecodeToken = function verifyAndDecodeToken(_ref) {
  var context = _ref.context; /* <-- Note: I *will not* find the headers on "_ref.context.req" */
  if (
    !context ||
    !context.headers ||
    (!context.headers.authorization && !context.headers.Authorization)
  ) {
    throw new _errors.AuthorizationError({
      message: "No authorization token."
    });
  }

Perhaps something like (in ES6, rather than the transpiled output above):

const verifyAndDecodeToken = function verifyAndDecodeToken({context}) {
  const req = context instanceof http.IncomingMessage ? context : (context.req || context.request);
...

Which would maintain backwards compatibility while allowing for context objects that possess req as a property.

That said, this still has a bit of a code smell, as in the present code what's being called context isn't necessarily the context object in question: it's either the context or a req of type http.IncomingMessage.

Whereas the above suggestion should fix the issue in a backwards-compatible manner, it also unfortunately turns the local context variable into a polymorphic type whose name doesn't closely match its underlying reality (e.g. it's really contextOrReq).

Ideally the API for verifyAndDecodeToken would take the full context as its only argument, then look for a http.IncomingMessage at either of context.req or context.request, afterwards using the proper req semantics in reference to the object in question (req.headers, etc...). In this way the API of this library would make as few assumptions as possible concerning the structure of the context object passed to the ApolloServer constructor, and minimize semantic confusion.

[Phylodome]

Thoughts, @johnymontana?

[Phylodome]

Anyone? If the above is correct, then the interface between this library and neo4j-graphql-js is basically failing silently in a rather misleading manner, and should be updated asap.

If the above is incorrect, I would be happy to learn what I've missed, and how to correct my (mis)-usage of the lib.

[johnymontana]

Hey @Phylodome - yes, I see what you mean. There is some inconsistency between the example code snippets there. Thanks for pointing this out.

To work properly with neo4j-graphql-js you'd do something like this:

const driver = neo4j.driver(
  "bolt://localhost:7687",
  neo4j.auth.basic("neo4j", "letmein")
);

const server = new ApolloServer({
  schema,
  context: ({ req }) => {
    return {
      headers: req.headers, 
      driver
     };
  }
});

For example see this demo using neo4j-graphql.js and graphql-auth-directives.

I like your point about being less opinionated about the structure of the context object and I believe your PR #5 addresses this, but I'd like to add some more tests to handle the different cases.

[Phylodome]

Will the accepted PR be brought into NPM (graphql-auth-directives via neo4j-graphql-js) any time soon?

[hacker-DOM]

Spent 2 hours debugging this yesterday! It doesn't make it any easier that the source code is correct, although the npm dist code differs from the source code for both 2.0.0 and 2.1.0!

Find my own way to the hack above, but hope it the dist code on npm will soon reflect the source code, to make our project more succinct, straightforward and not to hoodwink any newcomers... :(

[hacker-DOM]

In light of this behavior, I think the grandstack docs are incorrect.

I would prefer if the behavior of this library changes (to reflect source code) rather than the docs, though.

[TheWix]

Same as @hacker-DOM. Spent a while on this. Given the scant typescript support in the neo4j side of grandstack we should take care to throw the correct errors. When I wasn't passing the context object correctly when setting up the ApolloServer it was telling me the Authentication Headers were not found when in reality they were there but I had not done the setup correctly.

It might be better to treat context/request/headers being null vs not finding the header in the headers object. This way the developer can be told that they may not have done something correctly when setting up there server.

[iPwnPancakes]

Currently on: graphql-auth-directives: 2.2.0, apollo-server: 2.12.0

Have to do this in order for the package not to error out due to the request not containing any cookies:

const server = new ApolloServer({
    context: ({ req }) => {
        return {
            req: { ...req, cookies: {} },
            driver
        };
    },
    schema: augmentedSchema
});