Using with @octokit/graphql & GHES fails with 406 #111

lencioni · 2020-07-02T14:05:23Z

This is similar to #47, but when using createAppAuth.

We have some code that looks like this:

const { createAppAuth } = require('@octokit/auth-app');
const { graphql } = require('@octokit/graphql');
const { request } = require('@octokit/request');

function graphqlForInstallation(installationId) {
  const auth = createAppAuth({
    id: GITHUB_APP_ISSUER_ID,
    privateKey: PEM,
    installationId,
    request: request.defaults({
      baseUrl: `${GITHUB_BASE_URL}/api/v3`,
    }),
  });
  const graphqlWithAuth = graphql.defaults({
    baseUrl: GITHUB_BASE_URL,
    request: {
      hook: auth.hook,
    },
  });
  return graphqlWithAuth;
}

When this is used with public GitHub it works okay. However, when it is used with GHE v2.18 (specifically tried with 2.18.20), we get an error with the following stack trace (partial, starting at @octokit/auth-app code):

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at set (/usr/src/app/node_modules/@octokit/auth-app/dist-node/index.js:65:63)
    at getInstallationAuthentication (/usr/src/app/node_modules/@octokit/auth-app/dist-node/index.js:161:9)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async hook (/usr/src/app/node_modules/@octokit/auth-app/dist-node/index.js:280:7)

I looked into applying the workaround mentioned in #47, but after reading through some of the code here, I think that may not be possible at this time.

Looking at the stack trace, the problem seems to be when octokit is getting installation authentication via the request auth hook

    at getInstallationAuthentication (/usr/src/app/node_modules/@octokit/auth-app/dist-node/index.js:161:9)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async hook (/usr/src/app/node_modules/@octokit/auth-app/dist-node/index.js:280:7)

I believe that is called right here:

auth-app.js/src/hook.ts

Line 32 in 71e1d1d

const { token } = await getInstallationAuthentication(state, {}, request);

This is calling getInstallationAuthentication with {} as the second argument. Here's the signature:

auth-app.js/src/get-installation-authentication.ts

Lines 11 to 15 in 71e1d1d

    
           export async function getInstallationAuthentication( 
        
             state: StrategyOptionsWithDefaults, 
        
             options: InstallationAuthOptions, 
        
             customRequest?: RequestInterface 
        
           ): Promise<InstallationAccessTokenAuthentication> {

The second argument here is options, which is where it pulls permissions when making the request:

auth-app.js/src/get-installation-authentication.ts

Lines 58 to 61 in 71e1d1d

    
           } = await request("POST /app/installations/:installation_id/access_tokens", { 
        
             installation_id: installationId, 
        
             repository_ids: options.repositoryIds, 
        
             permissions: options.permissions,

So since createAppAuth or the hook do not provide a way for us to specify this permissions option, I don't think we can use this workaround with createAppAuth.

Is there an alternative approach we could use here, or would you be open to making a change to this package to make this work?

The text was updated successfully, but these errors were encountered:

gr2m · 2020-07-02T14:53:52Z

Thanks Joe for reporting the issue. I'll be offline until Tuesday, but will look into it afterwards. I'd love a pull request if you get to it, I don't have a way to test against GHES 2.18 myself, I'm afraid

lencioni · 2020-07-02T16:12:39Z

@gr2m Thanks for the quick response! The best approach here is not super clear to me. Three options immediately come to mind:

Allow hook options to be passed to createAppAuth in some way and propagate them down the chain. If we go with this, I'm not sure if the options need to be scoped in some way so they only get passed to the getInstallationAuthentication call (e.g. via a getInstallationApplicationOptions property) or if we can add a more generally named property. Some guidance here would be appreciated.
Change the argument passed to getInstallationAuthentication from {} to { permissions: {} }.
Add a default value to get-installation-authentication.js (e.g. permissions: options.permissions || {}).

Of these options, I think I prefer 3 because the size of the change is relatively small and I think this would help solve this problem everywhere (might allow us to remove the caveat note from the docs). Failing that, my second choice would be 2 because it would still be a small change for me to make.

What is your preference here?

lencioni · 2020-07-09T01:55:35Z

@gr2m I hope you had a great holiday! How do you want to proceed here?

gr2m · 2020-07-09T23:14:21Z

Thanks! I've still lots of notifications to catch up with :)

For option 2. and 3. we have to double check that the token gets any permissions at all, If we explicitly set it to {} instead of not setting it at all. Could you maybe check that with github.com & GHE 2.18?

trotzig · 2020-07-10T18:03:17Z

I've confirmed that setting { permissions: {} } works well against github.com, at least with the permissions we need. I'm not entirely sure how permissions come into play in this case though, since we're not explicitly setting them. Here's how I tested:

changed line 280 of node_modules/@octokit/auth-app/dist-node/index.js into } = await getInstallationAuthentication(state, { permissions: {} }, request);
created a client with app auth:

const auth = createAppAuth({
  id: GITHUB_APP_ISSUER_ID,
  privateKey: PEM,
  installationId,
  request: request.defaults({
    baseUrl: `${GITHUB_BASE_URL}/api/v3`,
  }),
});
const client = graphql.defaults({
  baseUrl: GITHUB_BASE_URL,
  request: {
    hook: auth.hook,
  },
});

Ran a graphql query to fetch commits:

const data = await client(
  `
  query getCommits($name: String!, $owner: String!, $defaultBranch: String!, $since: GitTimestamp) {
    repository(name: $name, owner: $owner) {
      ref(qualifiedName: $defaultBranch) {
        target {
          ... on Commit {
            id
            history(first: 100, since: $since) {
              pageInfo {
                hasNextPage
                endCursor
              }
              edges {
                node {
                  oid
                  committer {
                    date
                  }
                }
              }
            }
          }
        }
      }
    }
  }

`,
  {
    ...repository,
    since: '2017-10-18T00:00:00.000Z',
  },
);

This scenario returns the same things with the getInstallationAuthentication options set to {} (current default) and { permissions: {} } (our modification).

gr2m · 2020-07-10T18:22:02Z

Okay thanks for checking! Would you like to get a pull request started? We can discuss details there

trotzig · 2020-07-10T20:08:16Z

I think we're going to try the same thing with github enterprise before we put together a PR (@lencioni and I).

lencioni · 2020-07-13T14:57:41Z

It was tricky to make a successful request to test this. Here's the script I used:

const { createAppAuth } = require('@octokit/auth-app');
const { graphql } = require('@octokit/graphql');
const jsonwebtoken = require('jsonwebtoken');
const octokit = require('@octokit/rest');

const { request } = require('@octokit/request');

const { GITHUB_BASE_URL, GITHUB_APP_PEM_BASE64, GITHUB_APP_ISSUER_ID } = process.env;

const PEM = Buffer.from(GITHUB_APP_PEM_BASE64.trim(), 'base64').toString('ascii');

function graphqlForInstallation(installationId) {
  const auth = createAppAuth({
    id: GITHUB_APP_ISSUER_ID,
    privateKey: PEM,
    installationId,
    request: request.defaults({
      baseUrl: `${GITHUB_BASE_URL}/api/v3`,
    }),
  });

  const graphqlWithAuth = graphql.defaults({
    baseUrl: `${GITHUB_BASE_URL}/api`,
    request: {
      hook: auth.hook,
    },
  });

  return graphqlWithAuth;
}

async function main () {
  const client = graphqlForInstallation(1);

  let data;
  try {
    data = await client(
    `
      query getCommits($name: String!, $owner: String!, $defaultBranch: String!, $since: GitTimestamp) {
        repository(name: $name, owner: $owner) {
          ref(qualifiedName: $defaultBranch) {
            target {
              ... on Commit {
                id
                history(first: 100, since: $since) {
                  pageInfo {
                    hasNextPage
                    endCursor
                  }
                  edges {
                    node {
                      oid
                      committer {
                        date
                      }
                    }
                  }
                }
              }
            }
          }
        }
      }

    `,
      {
        name: 'repo-name',
        owner: 'repo-owner',
        defaultBranch: 'master',
        since: '2020-06-18T00:00:00.000Z',
      },
    );
  } catch (e) {
    throw e;
  }

  console.log(data);
}

main().catch(e => { console.error(e); });

I was getting stuck on only receiving 406 Not Acceptable responses. I've looked at issues like octokit/request.js#83 which suggest setting the accept header to application/json instead of application/vnd.github.machine-man-preview+json which I tried doing in a hacky way by adding this to @octokit/request/dist-node/index.js at line 27, but I still end up with the same result.

requestOptions.headers.accept = 'application/json';

Looking at the request URL that we are hitting, I suspected that the base URL here might not be correct. So I added this hacky thing to @octokit/request/dist-node/index.js at line 27 as well:

requestOptions.url = requestOptions.url.replace('/app/installations/', '/api/v3/app/installations/');

which gave me a new error response:

415 Unsupported Media Type

If I remove the accept header hack and leave the URL hack, it seems to get past this to actually make the graphql request!

If I leave getInstallationAuthentication with an empty object as the second arg as it is currently published, I get the following error:

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at set (/private/tmp/ghe/node_modules/@octokit/auth-app/dist-node/index.js:65:63)
    at getInstallationAuthentication (/private/tmp/ghe/node_modules/@octokit/auth-app/dist-node/index.js:161:9)
    at process._tickCallback (internal/process/next_tick.js:68:7)

If I set it to be { permissions: {} }, the request is successful.

So I think adding that empty object resolves part of the problem here for me, but I've also uncovered a different issue which is that I need two different baseUrl options here (one with /api/v3 for REST requests and one with /api for graphql requests). If I set the baseUrl in the graphql.defaults call to include /api/v3, then the auth request works but the graphql request tries to hit /api/v3/graphql, which fails. @gr2m do you have any thoughts on how we might be able to solve that problem? Or, am I just configuring something incorrectly here?

  const auth = createAppAuth({
    id: GITHUB_APP_ISSUER_ID,
    privateKey: PEM,
    installationId,
    request: request.defaults({
      baseUrl: `${GITHUB_BASE_URL}/api/v3`,
    }),
  });

  const graphqlWithAuth = graphql.defaults({
    baseUrl: `${GITHUB_BASE_URL}/api`,
    request: {
      hook: auth.hook,
    },
  });

lencioni · 2020-07-14T14:26:58Z

@gr2m did you find out anything useful yesterday?

gr2m · 2020-07-16T00:09:59Z

I did not have time to look into this yet. I will keep you posted.

lencioni · 2020-08-06T00:02:55Z

@gr2m is there any more information I can provide to help move this to the next step?

gr2m · 2020-08-06T06:10:01Z

I'm very sorry for not getting back to you. I had to focusing on wip/app#239 the past weeks, but that is coming to an end soon, after which I'll go through the open issues and PRs that I have neglected :(

lencioni · 2020-08-06T11:45:14Z

Thank you!

lencioni · 2020-08-10T18:17:53Z

We've recently updated GHE to 2.20.12 and I tried this stuff again and I'm seeing similar behavior.

Fresh install of required packages: 406 error
Only change {} to { permissions: {} } in @octokit/auth-app/dist-node/index.js line 288: 406 error
Only add requestOptions.url = requestOptions.url.replace('/api/app/installations/', '/api/v3/app/installations/'); to the fetchWrapper function in @octokit/request/dist-node/index.js (line 28): Successful request

So it seems that the newer version of GHE no longer needs the empty permissions object, but the URLs are still not able to be set correctly to get through a GraphQL request that also ends up making a REST request for authentication, since it tries to POST to /api/app/installations/<APP ISSUER ID>/access_tokens instead of /api/v3/app/installations/<APP ISSUER ID>/access_tokens.

lencioni · 2020-08-18T13:52:43Z

We have now updated GHE to 2.20.14, and I just wanted to report that the behavior is the same as I observed with 2.20.12.

lencioni · 2020-08-31T14:48:42Z

@gr2m Can you think of a workaround that we could use in the meantime while this is still an issue?

gr2m · 2020-09-09T15:56:57Z

Apologies for not getting back to you. I'm looking into it right now

gr2m · 2020-09-09T17:39:03Z

@lencioni can you try the following

install @octokit/graphql from fix: handle baseUrl suffix for GHES graphql.js#186: npm install https://github.pika.dev/octokit/graphql.js/pr/186

Update the baseUrl setting

 const graphqlWithAuth = graphql.defaults({
-  baseUrl: `${GITHUB_BASE_URL}/api`,
+  baseUrl: `${GITHUB_BASE_URL}/api/v3`,
   request: {
     hook: auth.hook,
   },
 });

You can also remove

    request: request.defaults({
      baseUrl: `${GITHUB_BASE_URL}/api/v3`,
    }),

It has no effect in your case

Please let me know if that resolves the 406 error

lencioni · 2020-09-09T17:59:31Z

@gr2m Yep, that works! Here's my diff:

 diff --git a/ghe.js b/ghe.js
index 06a915b..78763ea 100644
--- a/ghe.js
+++ b/ghe.js
@@ -14,13 +14,10 @@ function graphqlForInstallation(installationId) {
     id: GITHUB_APP_ISSUER_ID,
     privateKey: PEM,
     installationId,
-    request: request.defaults({
-      baseUrl: `${GITHUB_BASE_URL}/api/v3`,
-    }),
   });
 
   const graphqlWithAuth = graphql.defaults({
-    baseUrl: `${GITHUB_BASE_URL}/api`,
+    baseUrl: `${GITHUB_BASE_URL}/api/v3`,
     request: {
       hook: auth.hook,
     },
diff --git a/package-lock.json b/package-lock.json
index 4bb03c0..65b523a 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -37,6 +37,18 @@
         "@octokit/types": "^5.0.0",
         "before-after-hook": "^2.1.0",
         "universal-user-agent": "^6.0.0"
+      },
+      "dependencies": {
+        "@octokit/graphql": {
+          "version": "4.5.4",
+          "resolved": "https://registry.npmjs.org/@octokit/graphql/-/graphql-4.5.4.tgz",
+          "integrity": "sha512-ITpZ+dQc0cXAW1FmDkHJJM+8Lb6anUnin0VB5hLBilnYVdLC0ICFU/KIvT7OXfW9S81DE3U4Vx2EypDG1OYaPA==",
+          "requires": {
+            "@octokit/request": "^5.3.0",
+            "@octokit/types": "^5.0.0",
+            "universal-user-agent": "^6.0.0"
+          }
+        }
       }
     },
     "@octokit/endpoint": {
@@ -50,9 +62,8 @@
       }
     },
     "@octokit/graphql": {
-      "version": "4.5.3",
-      "resolved": "https://registry.npmjs.org/@octokit/graphql/-/graphql-4.5.3.tgz",
-      "integrity": "sha512-JyYvi3j2tOb5ofASEpcg1Advs07H+Ag+I+ez7buuZfNVAmh1IYcDTuxd4gnYH8S2PSGu+f5IdDGxMmkK+5zsdA==",
+      "version": "https://github.pika.dev/octokit/graphql.js/pr/186",
+      "integrity": "sha512-hUPAXu5w3+rcJ0AjzuGgoQNhYh5QYygRdxvDAP4Nhu/5t/I3dsjaywuapY6sEsjno+O3ObYzUNDzcr0w4FKGxQ==",
       "requires": {
         "@octokit/request": "^5.3.0",
         "@octokit/types": "^5.0.0",
diff --git a/package.json b/package.json
index 838756e..86a1197 100644
--- a/package.json
+++ b/package.json
@@ -11,7 +11,7 @@
   "license": "ISC",
   "dependencies": {
     "@octokit/auth-app": "^2.4.14",
-    "@octokit/graphql": "^4.5.3",
+    "@octokit/graphql": "https://github.pika.dev/octokit/graphql.js/pr/186",
     "@octokit/request": "^5.4.7",
     "@octokit/rest": "^18.0.3",
     "jsonwebtoken": "^8.5.1"

gr2m added Type: Bug Something isn't working as documented, or is being fixed enterprise labels Jul 2, 2020

gr2m self-assigned this Aug 6, 2020

gr2m changed the title ~~Cannot specify permissions option when using createAppAuth and auth.hook~~ Using with @octokit/graphql & GHES fails with 406 Sep 9, 2020

This was referenced Sep 9, 2020

Setting baseUrl for GHES results in 406 when using with @octokit/auth-app octokit/graphql.js#185

Closed

fix: handle baseUrl suffix for GHES octokit/graphql.js#186

Merged

gr2m closed this as completed in octokit/graphql.js#186 Sep 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using with @octokit/graphql & GHES fails with 406 #111

Using with @octokit/graphql & GHES fails with 406 #111

lencioni commented Jul 2, 2020 •

edited

Loading

gr2m commented Jul 2, 2020

lencioni commented Jul 2, 2020

lencioni commented Jul 9, 2020

gr2m commented Jul 9, 2020

trotzig commented Jul 10, 2020

gr2m commented Jul 10, 2020

trotzig commented Jul 10, 2020

lencioni commented Jul 13, 2020 •

edited

Loading

lencioni commented Jul 14, 2020

gr2m commented Jul 16, 2020

lencioni commented Aug 6, 2020

gr2m commented Aug 6, 2020

lencioni commented Aug 6, 2020

lencioni commented Aug 10, 2020

lencioni commented Aug 18, 2020

lencioni commented Aug 31, 2020

gr2m commented Sep 9, 2020

gr2m commented Sep 9, 2020

lencioni commented Sep 9, 2020

Using with @octokit/graphql & GHES fails with 406 #111

Using with @octokit/graphql & GHES fails with 406 #111

Comments

lencioni commented Jul 2, 2020 • edited Loading

gr2m commented Jul 2, 2020

lencioni commented Jul 2, 2020

lencioni commented Jul 9, 2020

gr2m commented Jul 9, 2020

trotzig commented Jul 10, 2020

gr2m commented Jul 10, 2020

trotzig commented Jul 10, 2020

lencioni commented Jul 13, 2020 • edited Loading

lencioni commented Jul 14, 2020

gr2m commented Jul 16, 2020

lencioni commented Aug 6, 2020

gr2m commented Aug 6, 2020

lencioni commented Aug 6, 2020

lencioni commented Aug 10, 2020

lencioni commented Aug 18, 2020

lencioni commented Aug 31, 2020

gr2m commented Sep 9, 2020

gr2m commented Sep 9, 2020

lencioni commented Sep 9, 2020

lencioni commented Jul 2, 2020 •

edited

Loading

lencioni commented Jul 13, 2020 •

edited

Loading