From 2030b5e80dadf3de04a793ce5a17e8f2c14f53c1 Mon Sep 17 00:00:00 2001 From: Iain Billett Date: Tue, 18 Jan 2022 18:01:07 +0000 Subject: [PATCH 1/3] Return an empty token where an installation owner is not allowed While this is less obvious to the user this facilitates anonymous access to (public) files where the app is installed but the owner is not in the allowed list. Signed-off-by: Iain Billett --- ...eInstanceGithubCredentialsProvider.test.ts | 38 +++++++++++++++++++ ...SingleInstanceGithubCredentialsProvider.ts | 4 +- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts index e5c6307338..246a859b7a 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts @@ -136,6 +136,44 @@ describe('SingleInstanceGithubCredentialsProvider tests', () => { expect(token).toEqual('secret_token'); }); + it('does not return a token where the organisation is not in the allowedInstallationsList', async () => { + github = SingleInstanceGithubCredentialsProvider.create({ + host: 'github.com', + apps: [ + { + appId: 1, + privateKey: 'privateKey', + webhookSecret: '123', + clientId: 'CLIENT_ID', + clientSecret: 'CLIENT_SECRET', + allowedInstallationOwners: ['backstage'], + }, + ], + }); + + octokit.apps.listInstallations.mockResolvedValue({ + headers: { + etag: '123', + }, + data: [ + { + id: 1, + repository_selection: 'all', + account: { + login: 'backstage', + }, + }, + ], + } as RestEndpointMethodTypes['apps']['listInstallations']['response']); + + const { token, headers } = await github.getCredentials({ + url: 'https://github.com/RoadiehHQ', + }); + + expect(headers).toEqual(undefined); + expect(token).toEqual(undefined); + }); + it('should not fail to issue tokens for an organization when the app is installed for a single repo', async () => { octokit.apps.listInstallations.mockResolvedValue({ headers: { diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts index 408c5b1348..b3fd26cddf 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts @@ -96,9 +96,7 @@ class GithubAppManager { const { installationId, suspended } = await this.getInstallationData(owner); if (this.allowedInstallationOwners) { if (!this.allowedInstallationOwners?.includes(owner)) { - throw new Error( - `The GitHub application for ${owner} is not included in the allowed installation list (${installationId}).`, - ); + return { accessToken: '' }; // An empty token allows anonymous access to public repos } } if (suspended) { From f45e99e5da40d5eeca7303212ec02fa99dfe1036 Mon Sep 17 00:00:00 2001 From: Iain Billett Date: Tue, 18 Jan 2022 18:09:14 +0000 Subject: [PATCH 2/3] Changeset Signed-off-by: Iain Billett --- .changeset/chatty-pens-try.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/chatty-pens-try.md diff --git a/.changeset/chatty-pens-try.md b/.changeset/chatty-pens-try.md new file mode 100644 index 0000000000..e8b50bf8f2 --- /dev/null +++ b/.changeset/chatty-pens-try.md @@ -0,0 +1,6 @@ +--- +'@backstage/integration': patch +--- + +Return an empty token rather than fail where the owner is not in the allowed installation owners +for a GitHub app. This allows anonymous access to public files in the organisation. From 0e76e264e9bec1df68a73b75b6d8d361418893d1 Mon Sep 17 00:00:00 2001 From: Iain Billett Date: Tue, 18 Jan 2022 18:30:27 +0000 Subject: [PATCH 3/3] Return undefined rather than an empty string Signed-off-by: Iain Billett --- .changeset/chatty-pens-try.md | 2 +- .../src/github/SingleInstanceGithubCredentialsProvider.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.changeset/chatty-pens-try.md b/.changeset/chatty-pens-try.md index e8b50bf8f2..1c92bb82cb 100644 --- a/.changeset/chatty-pens-try.md +++ b/.changeset/chatty-pens-try.md @@ -2,5 +2,5 @@ '@backstage/integration': patch --- -Return an empty token rather than fail where the owner is not in the allowed installation owners +Do not return a token rather than fail where the owner is not in the allowed installation owners for a GitHub app. This allows anonymous access to public files in the organisation. diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts index b3fd26cddf..bfc360cbe2 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts @@ -92,11 +92,11 @@ class GithubAppManager { async getInstallationCredentials( owner: string, repo?: string, - ): Promise<{ accessToken: string }> { + ): Promise<{ accessToken: string | undefined }> { const { installationId, suspended } = await this.getInstallationData(owner); if (this.allowedInstallationOwners) { if (!this.allowedInstallationOwners?.includes(owner)) { - return { accessToken: '' }; // An empty token allows anonymous access to public repos + return { accessToken: undefined }; // An empty token allows anonymous access to public repos } } if (suspended) {