fix: sync the installation matching GH_ORG during full sync

.env.example

@@ -1,6 +1,8 @@
 # The organization where you want to register the app in the app creation manifest flow.
 # If set, the app is registered for an organization (https://github.com/organizations/ORGANIZATION/settings/apps/new),
 # if not set, the GitHub app would be registered for the user account (https://github.com/settings/apps/new).
+# It also scopes the full sync: the installation on this account is the one that
+# gets synced, instead of whichever installation the API lists first.
 # GH_ORG=
 
 # The ID of your GitHub App

README.md

@@ -549,6 +549,19 @@ You can pass environment variables; the easiest way to do it is via a `.env` fil
   ```
   BLOCK_REPO_RENAME_BY_HUMAN=true
   ```
+1. Scope the full sync to one account using `GH_ORG`. For e.g.
+  ```
+  GH_ORG=my-org
+  ```
+  If the app is installed on more than one account, a full sync (`CRON` or
+  `npm run full-sync`) picks the installation on the `GH_ORG` account and reads
+  its configuration from `GH_ORG/<ADMIN_REPO>`. If `GH_ORG` is set but the app
+  is not installed on it, the full sync fails instead of syncing a different
+  account. When `GH_ORG` is not set, the first installation returned by the API
+  is used, so setting it is recommended whenever the app is installed on more
+  than one account. Note that `GH_ORG` is also used by the
+  [manifest flow](https://docs.github.com/en/apps/sharing-github-apps/registering-a-github-app-from-a-manifest)
+  to decide which account the app is registered for.
 
 
 ### Runtime Settings

docs/github-action.md

@@ -14,6 +14,8 @@ Running a full-sync with `safe-settings` can be done via `npm run full-sync`. Th
 ### Example GHA Workflow
 The below example uses the GHA "cron" feature to run a full-sync every 4 hours. While not required, this example uses the `.github` repo as the `admin` repo (set via `ADMIN_REPO` env var) and the safe-settings configurations are stored in the `safe-settings/` directory (set via `CONFIG_PATH` and `DEPLOYMENT_CONFIG_FILE`).
 
+`GH_ORG` names the account to sync. If the App is installed on more than one account, set it: it selects the installation to sync, and the full-sync fails rather than syncing a different account if the App is not installed on it.
+
 ```yaml
 name: Safe Settings Sync
 on:

index.js

@@ -231,8 +231,24 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
       github.rest.apps.listInstallations.endpoint.merge({ per_page: 100 })
     )
 
-    if (installations.length > 0) {
-      const installation = installations[0]
+    // When GH_ORG is set, sync the installation on that account instead of
+    // whichever one the API happens to list first. The order of
+    // `GET /app/installations` is not guaranteed to keep the account you care
+    // about at index 0, so an app installed on more than one account can
+    // otherwise start reading its config from, and applying settings to, a
+    // different account than the operator intended. Without GH_ORG the
+    // behavior is unchanged.
+    const installation = env.GH_ORG
+      ? installations.find(i => i.account?.login?.toLowerCase() === env.GH_ORG.toLowerCase())
+      : installations[0]
+
+    if (env.GH_ORG && !installation) {
+      const accounts = installations.map(i => i.account?.login).join(', ')
+      throw new Error(`No app installation found for GH_ORG '${env.GH_ORG}'. Installed on: [${accounts}]`)
+    }
+
+    if (installation) {
+      robot.log.info(`Syncing installation ${installation.id} on account ${installation.account?.login}`)
       const github = await robot.auth(installation.id)
       const context = {
         payload: {

lib/env.js

@@ -7,6 +7,7 @@ module.exports = {
   CREATE_ERROR_ISSUE: process.env.CREATE_ERROR_ISSUE || 'true',
   BLOCK_REPO_RENAME_BY_HUMAN: process.env.BLOCK_REPO_RENAME_BY_HUMAN || 'false',
   FULL_SYNC_NOP: process.env.FULL_SYNC_NOP === 'true',
+  GH_ORG: process.env.GH_ORG,
   GHE_HOST: process.env.GHE_HOST,
   GHE_PROTOCOL: process.env.GHE_PROTOCOL,
 }

test/unit/lib/env.test.js

@@ -32,6 +32,11 @@ describe('env', () => {
       const FULL_SYNC_NOP = envTest.FULL_SYNC_NOP
       expect(FULL_SYNC_NOP).toEqual(false)
     })
+
+    it('leaves GH_ORG undefined if not passed', () => {
+      const GH_ORG = envTest.GH_ORG
+      expect(GH_ORG).toBeUndefined()
+    })
   })
 
   describe('load override values', () => {
@@ -43,6 +48,7 @@ describe('env', () => {
       process.env.DEPLOYMENT_CONFIG_FILE = 'safe-settings-deployment.yml'
       process.env.CREATE_PR_COMMENT = 'false'
       process.env.FULL_SYNC_NOP = false
+      process.env.GH_ORG = 'my-org'
     })
 
     it('loads override values if passed', () => {
@@ -59,6 +65,8 @@ describe('env', () => {
       expect(CREATE_PR_COMMENT).toEqual('false')
       const FULL_SYNC_NOP = envTest.FULL_SYNC_NOP
       expect(FULL_SYNC_NOP).toEqual(false)
+      const GH_ORG = envTest.GH_ORG
+      expect(GH_ORG).toEqual('my-org')
     })
   })
 })

test/unit/sync-installation.test.js

@@ -0,0 +1,171 @@
+/* eslint-disable no-undef */
+const path = require('path')
+
+// The plugin reads GH_ORG through lib/env, which snapshots process.env at
+// require time, so each scenario needs a freshly required copy of both.
+function loadPlugin (ghOrg) {
+  jest.resetModules()
+  if (ghOrg === undefined) {
+    delete process.env.GH_ORG
+  } else {
+    process.env.GH_ORG = ghOrg
+  }
+  return require('../../index')
+}
+
+function installation (id, login) {
+  return { id, account: { login } }
+}
+
+describe('syncInstallation', () => {
+  const originalGhOrg = process.env.GH_ORG
+  const originalDeploymentConfigFile = process.env.DEPLOYMENT_CONFIG_FILE
+  let robot, octokit, syncAll
+
+  beforeAll(() => {
+    // Point the deployment config at a file that does not exist so
+    // loadYamlFileSystem() falls back to its built-in defaults instead of
+    // picking up whatever happens to sit in the working directory.
+    process.env.DEPLOYMENT_CONFIG_FILE = path.join(__dirname, 'no-such-deployment-settings.yml')
+  })
+
+  afterAll(() => {
+    if (originalGhOrg === undefined) {
+      delete process.env.GH_ORG
+    } else {
+      process.env.GH_ORG = originalGhOrg
+    }
+    if (originalDeploymentConfigFile === undefined) {
+      delete process.env.DEPLOYMENT_CONFIG_FILE
+    } else {
+      process.env.DEPLOYMENT_CONFIG_FILE = originalDeploymentConfigFile
+    }
+  })
+
+  beforeEach(() => {
+    octokit = {
+      paginate: jest.fn(),
+      rest: {
+        apps: {
+          listInstallations: { endpoint: { merge: jest.fn(options => options) } },
+          getAuthenticated: jest.fn().mockResolvedValue({ data: { slug: 'safe-settings' } })
+        },
+        repos: {
+          // The global settings file is irrelevant here: these tests assert
+          // which installation is selected, not what gets synced.
+          getContent: jest.fn().mockResolvedValue({ data: { content: '' } })
+        }
+      }
+    }
+    robot = {
+      auth: jest.fn().mockResolvedValue(octokit),
+      log: Object.assign(jest.fn(), {
+        debug: jest.fn(),
+        info: jest.fn(),
+        warn: jest.fn(),
+        error: jest.fn(),
+        trace: jest.fn()
+      }),
+      on: jest.fn()
+    }
+    syncAll = jest.fn().mockResolvedValue({ errors: [] })
+  })
+
+  // Returns the `repo` argument Settings.syncAll was called with, i.e. the
+  // admin repo of the account safe-settings decided to sync.
+  function syncedRepo () {
+    expect(syncAll).toHaveBeenCalledTimes(1)
+    return syncAll.mock.calls[0][2]
+  }
+
+  it('syncs the installation matching GH_ORG, not the first one listed', async () => {
+    const plugin = loadPlugin('my-org')
+    octokit.paginate.mockResolvedValue([
+      installation(1, 'another-account'),
+      installation(2, 'my-org')
+    ])
+
+    const app = plugin(robot, {}, { syncAll, handleError: jest.fn() })
+    await app.syncInstallation()
+
+    expect(syncedRepo()).toEqual({ owner: 'my-org', repo: 'admin' })
+    // The context handed to syncAll must be authenticated as the GH_ORG
+    // installation. (info() separately authenticates as installations[0] to
+    // read the app slug; that is left as-is, see the PR description.)
+    expect(robot.auth).toHaveBeenLastCalledWith(2)
+  })
+
+  it('matches GH_ORG case-insensitively, as GitHub account names are', async () => {
+    const plugin = loadPlugin('My-Org')
+    octokit.paginate.mockResolvedValue([
+      installation(1, 'another-account'),
+      installation(7, 'my-org')
+    ])
+
+    const app = plugin(robot, {}, { syncAll, handleError: jest.fn() })
+    await app.syncInstallation()
+
+    expect(syncedRepo()).toEqual({ owner: 'my-org', repo: 'admin' })
+  })
+
+  it('throws when GH_ORG has no installation instead of syncing another account', async () => {
+    const plugin = loadPlugin('my-org')
+    octokit.paginate.mockResolvedValue([
+      installation(1, 'another-account'),
+      installation(2, 'yet-another-account')
+    ])
+
+    const app = plugin(robot, {}, { syncAll, handleError: jest.fn() })
+
+    await expect(app.syncInstallation()).rejects.toThrow(
+      "No app installation found for GH_ORG 'my-org'. Installed on: [another-account, yet-another-account]"
+    )
+    expect(syncAll).not.toHaveBeenCalled()
+  })
+
+  it('falls back to the first installation when GH_ORG is not set', async () => {
+    const plugin = loadPlugin(undefined)
+    octokit.paginate.mockResolvedValue([
+      installation(1, 'first-account'),
+      installation(2, 'second-account')
+    ])
+
+    const app = plugin(robot, {}, { syncAll, handleError: jest.fn() })
+    await app.syncInstallation()
+
+    expect(syncedRepo()).toEqual({ owner: 'first-account', repo: 'admin' })
+  })
+
+  it('returns null without syncing when the app has no installations', async () => {
+    const plugin = loadPlugin(undefined)
+    octokit.paginate.mockResolvedValue([])
+
+    const app = plugin(robot, {}, { syncAll, handleError: jest.fn() })
+
+    await expect(app.syncInstallation()).resolves.toBeNull()
+    expect(syncAll).not.toHaveBeenCalled()
+  })
+
+  it('passes the nop flag through to the sync', async () => {
+    const plugin = loadPlugin('my-org')
+    octokit.paginate.mockResolvedValue([installation(2, 'my-org')])
+
+    const app = plugin(robot, {}, { syncAll, handleError: jest.fn() })
+    await app.syncInstallation(true)
+
+    expect(syncAll).toHaveBeenCalledWith(true, expect.anything(), expect.anything(), expect.anything())
+  })
+
+  it('logs which account is being synced', async () => {
+    const plugin = loadPlugin('my-org')
+    octokit.paginate.mockResolvedValue([
+      installation(1, 'another-account'),
+      installation(2, 'my-org')
+    ])
+
+    const app = plugin(robot, {}, { syncAll, handleError: jest.fn() })
+    await app.syncInstallation()
+
+    expect(robot.log.info).toHaveBeenCalledWith('Syncing installation 2 on account my-org')
+  })
+})