From 5fd31c2f46502d77f5a3818e9a4cc3c65b09fb92 Mon Sep 17 00:00:00 2001 From: Brian Fletcher Date: Mon, 19 Jul 2021 17:20:59 +0100 Subject: [PATCH 1/7] remove repo filtering on the github credentials The github-org processor does not use the concept of repositories. As such filtering based on selected repositories does not make sense for that processer. The GithubCredentials was imposing what appears like an unnessesary restriction on the tokens using the repo name. By removing this restriction, it enalbes the github-org processor to work when a github app installation has a repository restriction. Signed-off-by: Brian Fletcher --- .changeset/serious-kiwis-wonder.md | 5 +++++ .../github/GithubCredentialsProvider.test.ts | 13 ++++++------ .../src/github/GithubCredentialsProvider.ts | 20 +++---------------- 3 files changed, 14 insertions(+), 24 deletions(-) create mode 100644 .changeset/serious-kiwis-wonder.md diff --git a/.changeset/serious-kiwis-wonder.md b/.changeset/serious-kiwis-wonder.md new file mode 100644 index 0000000000..4c6daa768b --- /dev/null +++ b/.changeset/serious-kiwis-wonder.md @@ -0,0 +1,5 @@ +--- +'@backstage/integration': patch +--- + +remove repo restriction from github creds provider diff --git a/packages/integration/src/github/GithubCredentialsProvider.test.ts b/packages/integration/src/github/GithubCredentialsProvider.test.ts index 85df1bb4e1..9472e0ae58 100644 --- a/packages/integration/src/github/GithubCredentialsProvider.test.ts +++ b/packages/integration/src/github/GithubCredentialsProvider.test.ts @@ -156,13 +156,12 @@ describe('GithubCredentialsProvider tests', () => { }, } as RestEndpointMethodTypes['apps']['createInstallationAccessToken']['response']); - await expect( - github.getCredentials({ - url: 'https://github.com/backstage', - }), - ).rejects.toThrow( - 'The Backstage GitHub application used in the backstage organization must be installed for the entire organization to be able to issue credentials without a specified repository.', - ); + const { token, headers } = await github.getCredentials({ + url: 'https://github.com/backstage', + }); + + expect(headers).toEqual({ Authorization: 'Bearer secret_token' }); + expect(token).toEqual('secret_token'); }); it('should throw if the app is suspended', async () => { diff --git a/packages/integration/src/github/GithubCredentialsProvider.ts b/packages/integration/src/github/GithubCredentialsProvider.ts index 899b233195..ea4fa737ec 100644 --- a/packages/integration/src/github/GithubCredentialsProvider.ts +++ b/packages/integration/src/github/GithubCredentialsProvider.ts @@ -23,7 +23,6 @@ import { DateTime } from 'luxon'; type InstallationData = { installationId: number; suspended: boolean; - repositorySelection: 'selected' | 'all'; }; class Cache { @@ -85,33 +84,21 @@ class GithubAppManager { owner: string, repo?: string, ): Promise<{ accessToken: string }> { - const { - installationId, - suspended, - repositorySelection, - } = await this.getInstallationData(owner); + const { installationId, suspended } = await this.getInstallationData(owner); if (suspended) { throw new Error( - `The GitHub application for ${[owner, repo] + `The GitHub application for ${[owner] .filter(Boolean) .join('/')} is suspended`, ); } - if (repositorySelection !== 'all' && !repo) { - throw new Error( - `The Backstage GitHub application used in the ${owner} organization must be installed for the entire organization to be able to issue credentials without a specified repository.`, - ); - } - const cacheKey = !repo ? owner : `${owner}/${repo}`; - const repositories = repositorySelection !== 'all' ? [repo!] : undefined; + const cacheKey = repo ? `${owner}/${repo}` : owner; - // Go and grab an access token for the app scoped to a repository if provided, if not use the organisation installation. return this.cache.getOrCreateToken(cacheKey, async () => { const result = await this.appClient.apps.createInstallationAccessToken({ installation_id: installationId, headers: HEADERS, - repositories, }); return { token: result.data.token, @@ -135,7 +122,6 @@ class GithubAppManager { return { installationId: installation.id, suspended: Boolean(installation.suspended_by), - repositorySelection: installation.repository_selection, }; } const notFoundError = new Error( From 2473096aa6176a71af4af6993262d935da49896b Mon Sep 17 00:00:00 2001 From: Brian Fletcher Date: Mon, 19 Jul 2021 18:13:00 +0100 Subject: [PATCH 2/7] Fix typos in the changeset file Signed-off-by: Brian Fletcher --- .changeset/serious-kiwis-wonder.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/serious-kiwis-wonder.md b/.changeset/serious-kiwis-wonder.md index 4c6daa768b..0deba49616 100644 --- a/.changeset/serious-kiwis-wonder.md +++ b/.changeset/serious-kiwis-wonder.md @@ -2,4 +2,4 @@ '@backstage/integration': patch --- -remove repo restriction from github creds provider +Remove repo restriction from GitHub credentials provider From 619cb40662dc7a7a114e4c62975fea70fe950379 Mon Sep 17 00:00:00 2001 From: Brian Fletcher Date: Mon, 19 Jul 2021 18:51:10 +0100 Subject: [PATCH 3/7] try to work around codeql failure for test purposes Signed-off-by: Brian Fletcher --- .../src/github/GithubCredentialsProvider.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/integration/src/github/GithubCredentialsProvider.test.ts b/packages/integration/src/github/GithubCredentialsProvider.test.ts index 9472e0ae58..a0eb3d9596 100644 --- a/packages/integration/src/github/GithubCredentialsProvider.test.ts +++ b/packages/integration/src/github/GithubCredentialsProvider.test.ts @@ -133,7 +133,7 @@ describe('GithubCredentialsProvider tests', () => { expect(token).toEqual('secret_token'); }); - it('should fail to issue tokens for an organization when the app is installed for a single repo', async () => { + 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: { etag: '123', @@ -159,8 +159,8 @@ describe('GithubCredentialsProvider tests', () => { const { token, headers } = await github.getCredentials({ url: 'https://github.com/backstage', }); - - expect(headers).toEqual({ Authorization: 'Bearer secret_token' }); + const expectedToken = 'secret_token'; + expect(headers).toEqual({ Authorization: `Bearer ${expectedToken}` }); expect(token).toEqual('secret_token'); }); From b0578306b69aba6eb754119a7a2bf45cdb6b2eb0 Mon Sep 17 00:00:00 2001 From: Brian Fletcher Date: Tue, 20 Jul 2021 13:57:10 +0100 Subject: [PATCH 4/7] fix the github-disovery when the repos are filtered Signed-off-by: Brian Fletcher --- .../src/github/GithubCredentialsProvider.ts | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/integration/src/github/GithubCredentialsProvider.ts b/packages/integration/src/github/GithubCredentialsProvider.ts index ea4fa737ec..ebd9fce745 100644 --- a/packages/integration/src/github/GithubCredentialsProvider.ts +++ b/packages/integration/src/github/GithubCredentialsProvider.ts @@ -100,6 +100,20 @@ class GithubAppManager { installation_id: installationId, headers: HEADERS, }); + if (repo && result.data.repository_selection === 'selected') { + const installationClient = new Octokit({ + auth: result.data.token, + }); + const repos = await installationClient.apps.listReposAccessibleToInstallation(); + const hasRepo = repos.data.repositories.some(repository => { + return repository.name === repo; + }); + if (!hasRepo) { + throw new Error( + `The Backstage GitHub application used in the ${owner} organization does not have access to a repository with the name ${repo}`, + ); + } + } return { token: result.data.token, expiresAt: DateTime.fromISO(result.data.expires_at), @@ -219,7 +233,10 @@ export class GithubCredentialsProvider { const parsed = parseGitUrl(opts.url); const owner = parsed.owner || parsed.name; - const repo = parsed.owner ? parsed.name : undefined; + let repo = parsed.owner ? parsed.name : undefined; + // the github-discovery plugin passes an • as a repo name. This simply means it wants access to all repos + // that are available to the installation. + repo = repo === '*' ? undefined : repo; let type: GithubCredentialType = 'app'; let token = await this.githubAppCredentialsMux.getAppToken(owner, repo); From 6d9698eaa92c5b5ed9b646cbe5c26e61a3d597e6 Mon Sep 17 00:00:00 2001 From: Brian Fletcher Date: Fri, 6 Aug 2021 11:03:12 +0100 Subject: [PATCH 5/7] move special handling for gh disc processor This change moves the special handling for the GitHub Discovery processor from the GitHub credentials provider into the GitHub Discovery logic. Signed-off-by: Brian Fletcher --- .../src/github/GithubCredentialsProvider.ts | 11 ++--------- .../ingestion/processors/GithubDiscoveryProcessor.ts | 9 ++++++++- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/integration/src/github/GithubCredentialsProvider.ts b/packages/integration/src/github/GithubCredentialsProvider.ts index ebd9fce745..fd5d13caa2 100644 --- a/packages/integration/src/github/GithubCredentialsProvider.ts +++ b/packages/integration/src/github/GithubCredentialsProvider.ts @@ -86,11 +86,7 @@ class GithubAppManager { ): Promise<{ accessToken: string }> { const { installationId, suspended } = await this.getInstallationData(owner); if (suspended) { - throw new Error( - `The GitHub application for ${[owner] - .filter(Boolean) - .join('/')} is suspended`, - ); + throw new Error(`The GitHub application for ${owner} is suspended`); } const cacheKey = repo ? `${owner}/${repo}` : owner; @@ -233,10 +229,7 @@ export class GithubCredentialsProvider { const parsed = parseGitUrl(opts.url); const owner = parsed.owner || parsed.name; - let repo = parsed.owner ? parsed.name : undefined; - // the github-discovery plugin passes an • as a repo name. This simply means it wants access to all repos - // that are available to the installation. - repo = repo === '*' ? undefined : repo; + const repo = parsed.owner ? parsed.name : undefined; let type: GithubCredentialType = 'app'; let token = await this.githubAppCredentialsMux.getAppToken(owner, repo); diff --git a/plugins/catalog-backend/src/ingestion/processors/GithubDiscoveryProcessor.ts b/plugins/catalog-backend/src/ingestion/processors/GithubDiscoveryProcessor.ts index 2219e02406..e28e56941b 100644 --- a/plugins/catalog-backend/src/ingestion/processors/GithubDiscoveryProcessor.ts +++ b/plugins/catalog-backend/src/ingestion/processors/GithubDiscoveryProcessor.ts @@ -25,6 +25,7 @@ import { Logger } from 'winston'; import { getOrganizationRepositories } from './github'; import * as results from './results'; import { CatalogProcessor, CatalogProcessorEmit } from './types'; +import parseGitUrl from 'git-url-parse'; /** * Extracts repositories out of a GitHub org. @@ -63,9 +64,15 @@ export class GithubDiscoveryProcessor implements CatalogProcessor { `There is no GitHub integration that matches ${location.target}. Please add a configuration entry for it under integrations.github`, ); } + + // Building the org url here so that the github creds provider doesn't need to know + // about how to handle the wild card which is special for this processor. + const { source, organization } = parseGitUrl(location.target); + const orgUrl = `https://${source}/${organization}`; + const { headers } = await GithubCredentialsProvider.create( gitHubConfig, - ).getCredentials({ url: location.target }); + ).getCredentials({ url: orgUrl }); const { org, repoSearchPath, catalogPath } = parseUrl(location.target); const client = graphql.defaults({ From 5072d05d12dcfe21a5392c017a3a1c76cb1c8d90 Mon Sep 17 00:00:00 2001 From: Brian Fletcher Date: Mon, 9 Aug 2021 09:20:06 +0100 Subject: [PATCH 6/7] use existing parseUrl function to get host and org Signed-off-by: Brian Fletcher --- .../ingestion/processors/GithubDiscoveryProcessor.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/plugins/catalog-backend/src/ingestion/processors/GithubDiscoveryProcessor.ts b/plugins/catalog-backend/src/ingestion/processors/GithubDiscoveryProcessor.ts index e28e56941b..b444c23924 100644 --- a/plugins/catalog-backend/src/ingestion/processors/GithubDiscoveryProcessor.ts +++ b/plugins/catalog-backend/src/ingestion/processors/GithubDiscoveryProcessor.ts @@ -25,7 +25,6 @@ import { Logger } from 'winston'; import { getOrganizationRepositories } from './github'; import * as results from './results'; import { CatalogProcessor, CatalogProcessorEmit } from './types'; -import parseGitUrl from 'git-url-parse'; /** * Extracts repositories out of a GitHub org. @@ -65,15 +64,17 @@ export class GithubDiscoveryProcessor implements CatalogProcessor { ); } + const { org, repoSearchPath, catalogPath, host } = parseUrl( + location.target, + ); + // Building the org url here so that the github creds provider doesn't need to know // about how to handle the wild card which is special for this processor. - const { source, organization } = parseGitUrl(location.target); - const orgUrl = `https://${source}/${organization}`; + const orgUrl = `https://${host}/${org}`; const { headers } = await GithubCredentialsProvider.create( gitHubConfig, ).getCredentials({ url: orgUrl }); - const { org, repoSearchPath, catalogPath } = parseUrl(location.target); const client = graphql.defaults({ baseUrl: gitHubConfig.apiBaseUrl, @@ -119,7 +120,7 @@ export class GithubDiscoveryProcessor implements CatalogProcessor { export function parseUrl( urlString: string, -): { org: string; repoSearchPath: RegExp; catalogPath: string } { +): { org: string; repoSearchPath: RegExp; catalogPath: string; host: string } { const url = new URL(urlString); const path = url.pathname.substr(1).split('/'); @@ -129,6 +130,7 @@ export function parseUrl( org: decodeURIComponent(path[0]), repoSearchPath: escapeRegExp(decodeURIComponent(path[1])), catalogPath: `/${decodeURIComponent(path.slice(2).join('/'))}`, + host: url.host, }; } From 57dd0fae8edd34afaa1f924d72ac40ccf683c9fd Mon Sep 17 00:00:00 2001 From: Brian Fletcher Date: Mon, 9 Aug 2021 10:40:14 +0100 Subject: [PATCH 7/7] fix discovery processor test Signed-off-by: Brian Fletcher --- .../src/ingestion/processors/GithubDiscoveryProcessor.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/catalog-backend/src/ingestion/processors/GithubDiscoveryProcessor.test.ts b/plugins/catalog-backend/src/ingestion/processors/GithubDiscoveryProcessor.test.ts index 467e9c1ea2..d4d80453ce 100644 --- a/plugins/catalog-backend/src/ingestion/processors/GithubDiscoveryProcessor.test.ts +++ b/plugins/catalog-backend/src/ingestion/processors/GithubDiscoveryProcessor.test.ts @@ -32,6 +32,7 @@ describe('GithubDiscoveryProcessor', () => { parseUrl('https://github.com/foo/proj/blob/master/catalog.yaml'), ).toEqual({ org: 'foo', + host: 'github.com', repoSearchPath: /^proj$/, catalogPath: '/blob/master/catalog.yaml', }); @@ -39,6 +40,7 @@ describe('GithubDiscoveryProcessor', () => { parseUrl('https://github.com/foo/proj*/blob/master/catalog.yaml'), ).toEqual({ org: 'foo', + host: 'github.com', repoSearchPath: /^proj.*$/, catalogPath: '/blob/master/catalog.yaml', });