Throw a descriptive error when permissions is not set about auth-app.js

[kjc17710]

FWIW, I am also on Enterprise 2.18 and had to add the empty permissions object in order to get the request to work successfully.

[kjc17710]

Can anyone check if the problem persists in 2.19?

We are upgrading at the end of this month, so I can try again at the start of March and let you know.

[kjc17710]

@gr2m My enterprise version is now 2.20.0 and I can generate a token without the empty permissions object now.

[davidxia]

From my testing, the permissions are all the permissions inherited from the installation.

I'd phrase it as

<strong>Required for GitHub Enterprise Server 2.18 and below. Specify an empty object `{}` for all permissions granted by the installation.</strong>

[gr2m]

Thanks for reporting the issue Zachary. I tried to reproduce the problem with the exact same code, but it just worked for me ... 🤔

Before it does the Object.keys(data.permissions) it checks if data.permissions exists

auth-app.js/src/cache.ts

Lines 70 to 72 in 0b50bb4

	const permissionsString = options.permissions
	? ""
	: Object.keys(data.permissions)

Here are the steps I took to in order to try to reproduce the problem

➜  cd "$(mktemp -d)"
➜  tmp.m2bR8hQU npm init -y
Wrote to /private/var/folders/hs/x9qtfmvn1lz1sgml9q21h7k80000gn/T/tmp.m2bR8hQU/package.json:

{
  "name": "tmp.m2bR8hQU",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "author": "Gregor Martynus (https://twitter.com/gr2m)",
  "license": "MIT"
}


➜  tmp.m2bR8hQU npm i @octokit/auth-app
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN [email protected] No description
npm WARN [email protected] No repository field.

+ @octokit/[email protected]
added 52 packages from 79 contributors and audited 130 packages in 6.995s
found 0 vulnerabilities

➜  tmp.m2bR8hQU pbpaste > index.js
➜  tmp.m2bR8hQU node index.js 
{
  type: 'token',
  tokenType: 'installation',
  token: 'v1.547[REDACTED]',
  installationId: 6316925,
  permissions: { checks: 'write', metadata: 'read' },
  expiresAt: '2020-02-14T18:24:43Z',
  repositorySelection: 'selected'
}

Can you please run npm ls @octokit/auth-app to make sure you have the latest version?

[zpartal]

Looking at

auth-app.js/src/cache.ts

Lines 70 to 72 in 0b50bb4

	const permissionsString = options.permissions
	? ""
	: Object.keys(data.permissions)

it looks like options.permissions is being checked, and set to empty string if permissions exists on options. It is also not checking data.permissions. Should it be:

const permissionsString = options.permissions // or data.permissions
  ? Object.keys(data.permissions)
      .map(name => `${name}${data.permissions[name] === "write" ? "!" : ""}`)
      .join(",")
  : "";

From what I can tell, options comes from what I pass into this

auth-app.js/src/auth.ts

Line 19 in 0b50bb4

return getInstallationAuthentication(state, options);

which in my reproducer is {type: 'installation'}, thus why I have to add permissions: {} to make it work.

While this would be okay, I intend to use the hook integration with @octokit/graph and use the following code:

const graphqlWithAuth = graphql.defaults({
  request: {
    hook: auth.hook
  }
});

as per https://github.com/octokit/graphql.js/blob/master/README.md#authentication

Complete Reproducer

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

(async () => {

  const auth = createAppAuth({
    id: 123,
    privateKey: `-----BEGIN RSA PRIVATE KEY-----...`,
    installationId: 456,
    request: request.defaults({
      baseUrl: 'https://api.github.{my-enterprise}.com',
    }),
  });

  const installationAuthentication = await auth({
    type: 'installation',
  });
})().catch()

Output of npm ls @ocktokit/auth-app

~/code/scratch/auth-app-reproducer
❯ npm ls @octokit/auth-app
[email protected] /Users/zpartal/code/scratch/auth-app-reproducer
└── @octokit/[email protected] 

[gr2m]

In your code, can you share how you use the graphql request hook?

const graphqlWithAuth = graphql.defaults({
  request: {
    hook: auth.hook
  }
});

Here is my code trying to reproduce the problem, but it all works as expected

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

main();

async function main() {
  const auth = createAppAuth({
    id: APPI_ID,
    privateKey: PRIVATE_KEY,
    installationId: INSTALLATION_ID
  });

  await auth({
    type: "installation"
  });

  const myRequest = request.defaults({
    request: {
      hook: auth.hook
    }
  });

  const { data } = await myRequest("GET /installation/repositories", {
    mediaType: {
      previews: ["machine-man"]
    },
    per_page: 1
  }).catch(console.log);
  console.log(data);
}

[zpartal]

My graphql hook looks as follows

  const graphqlWithAuth = graphql.defaults({
    request: {
      hook: auth.hook
    }
  });

Also, since I hadn't posted it before, here is the the error

(node:86097) UnhandledPromiseRejectionWarning: TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at set (/Users/zpartal/code/scratch/auth-app-reproducer/node_modules/@octokit/auth-app/dist-node/index.js:67:63)
    at getInstallationAuthentication (/Users/zpartal/code/scratch/auth-app-reproducer/node_modules/@octokit/auth-app/dist-node/index.js:156:9)
    at processTicksAndRejections (internal/process/task_queues.js:94:5)
    at async /Users/zpartal/code/scratch/auth-app-reproducer/index.js:16:38

Node 12.14.1

Also, I'm still confused about this section of the code

auth-app.js/src/cache.ts

Lines 70 to 72 in 0b50bb4

	const permissionsString = options.permissions
	? ""
	: Object.keys(data.permissions)

The logic is that, if options.permissions exists, permissionString is set to empty string, otherwise use the permissions from data.permissions? If that's the case, then I think the issue is that data.permissions is not populated for me. I'll investigate further as to what that is supposed to be.

[zpartal]

Further inspection shows that the request here

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

Lines 49 to 58 in 0b50bb4

	const {
	data: {
	token,
	expires_at: expiresAt,
	repositories,
	permissions,
	repository_selection: repositorySelection,
	single_file: singleFileName
	}
	} = await request("POST /app/installations/:installation_id/access_tokens", {

is not returning is not returning permissions, only tokens and expires_at. I wonder if this is a quirk of GitHub Enterprise.

[gr2m]

I wonder if this is a quirk of GitHub Enterprise.

Hmm that is well possible :( I'm sorry that I didn't put in the time to test compatibility with GHES yet. I have it on my backlog.

Here is how the response looks like on api.github.com:
https://developer.github.com/v3/apps/#response-5

Here is how the same response looks like for GHES 2.18
https://developer.github.com/enterprise/2.18/v3/apps/#response-5

At least in terms of looking example shown in he documentation it seems to be the same. But I can vaguely remember that I run into a problem when I worked on this library and the API team fixed it for me. It is possible that the "permissions" is what was fixed, so that it's always returned.

I'm not sure if it is fixed in 2.19, I'm sure that it will be fixed in 2.20. I do not have access to instances for these versions in order to test it myself.

I can fix it for you, so that it works in 2.18, the only thing you and everyone working on the code needs to be aware is that the returned authentication object will return and empty array for .permissions if you don't set the options.permissions for auth(options). We can also default it to something more useful, such as ["unknown: see octokit/auth-app.js#47"]

What do you think?

If you like, you can get a Pull request with a failing test going

[gr2m]

Let me update the README to at least give people a heads up.

Can anyone check if the problem persists in 2.19?

[zpartal]

We are on 2.18 as well, I'll revisit this when we are on 2.19.

[zpartal]

Actually, I don't think setting the permissions on auth options will be a workaround for using the hook with @octokit/graphql. Can you elaborate further what you meant here

I can fix it for you, so that it works in 2.18, the only thing you and everyone working on the code needs to be aware is that the returned authentication object will return and empty array for .permissions if you don't set the options.permissions for auth(options). We can also default it to something more useful, such as ["unknown: see #47"]

[gr2m]

I think the API was changed at some point. Initially, the "permissions" was only present if you set the "permissions" parameter when creating an installation access token. After the change the "permissions" key was always present. So if you create a token without setting the "permissions" parameter, the returned authentication object with have an .permission key set to an empty object, as a fallback value.

I think most likely you won't be bothered by this anyway, it's just a heads up

[davidxia]

I'm on enterprise version 2.18.12 and also ran into this today. @kjc17710's workaround here of setting permissions to an empty object worked for me.

Regarding @zpartal's comment up above

it looks like options.permissions is being checked, and set to empty string if permissions exists on options. It is also not checking data.permissions. Should it be:

const permissionsString = options.permissions // or data.permissions
  ? Object.keys(data.permissions)
      .map(name => `${name}${data.permissions[name] === "write" ? "!" : ""}`)
      .join(",")
  : "";

I have the same question. I don't have that much context on this code, but it seems like the logic is inverted?

If the logic is correct, perhaps the doc update here could be even more explicit to include @kjc17710's workaround of setting permissions to explicitly be an empty object for versions <= 2.18?

[gr2m]

How would you phrase it? I also wonder what permissions the created token has when you pass an empty {} as value for the parameters option. Will it inherit all permissions from the installation, or will have read-only access to public data only?

[github-actions]

🎉 This issue has been resolved in version 2.4.4 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀