From 0fd4ea44318d58e7b3c756fe338f7b8758ed9ea0 Mon Sep 17 00:00:00 2001 From: Johan Haals Date: Fri, 21 May 2021 14:21:58 +0200 Subject: [PATCH 1/4] Exclude emails using app auth with `GithubOrgReaderProcessor` Co-authored-by: Ben Lambert Signed-off-by: Johan Haals --- .changeset/giant-rivers-protect.md | 10 + .../github/GithubCredentialsProvider.test.ts | 7 +- .../src/github/GithubCredentialsProvider.ts | 6 + packages/integration/src/github/index.ts | 1 + .../GithubOrgReaderProcessor.test.ts | 171 ++++++++++++------ .../processors/GithubOrgReaderProcessor.ts | 81 ++------- .../processors/github/github.test.ts | 4 +- .../src/ingestion/processors/github/github.ts | 14 +- 8 files changed, 167 insertions(+), 127 deletions(-) create mode 100644 .changeset/giant-rivers-protect.md diff --git a/.changeset/giant-rivers-protect.md b/.changeset/giant-rivers-protect.md new file mode 100644 index 0000000000..cbf11a47c0 --- /dev/null +++ b/.changeset/giant-rivers-protect.md @@ -0,0 +1,10 @@ +--- +'@backstage/integration': patch +'@backstage/plugin-catalog-backend': minor +--- + +Updates the `GithubCredentialsProvider` to return the token type, it can either be `token` or `app` depending on the authentication method. + +Update the `GithubOrgReaderProcessor` NOT to query for email addresses if GitHub Apps is used for authentication, this is due to inconsistencies in the GitHub API when using server to server communications and installation tokens. https://github.community/t/api-v4-unable-to-retrieve-email-resource-not-accessible-by-integration/13831/4 for more info. + +**Removes** deprecated GithubOrgReaderProcessor provider configuration(`catalog.processors.githubOrg`). If you're using the deprecated config section make sure to migrate to [integrations](https://backstage.io/docs/integrations/github/locations) instead. diff --git a/packages/integration/src/github/GithubCredentialsProvider.test.ts b/packages/integration/src/github/GithubCredentialsProvider.test.ts index d295677c55..271d4b9920 100644 --- a/packages/integration/src/github/GithubCredentialsProvider.test.ts +++ b/packages/integration/src/github/GithubCredentialsProvider.test.ts @@ -81,13 +81,13 @@ describe('GithubCredentialsProvider tests', () => { }, } as RestEndpointMethodTypes['apps']['createInstallationAccessToken']['response']); - const { token, headers } = await github.getCredentials({ + const { token, headers, type } = await github.getCredentials({ url: 'https://github.com/backstage/foobar', }); const { token: accessToken2 } = await github.getCredentials({ url: 'https://github.com/backstage/foobar', }); - + expect(type).toEqual('app'); expect(token).toEqual('secret_token'); expect(token).toEqual(accessToken2); expect(headers).toEqual({ Authorization: 'Bearer secret_token' }); @@ -102,6 +102,7 @@ describe('GithubCredentialsProvider tests', () => { Authorization: 'Bearer hardcoded_token', }, token: 'hardcoded_token', + type: 'token', }); }); @@ -260,6 +261,6 @@ describe('GithubCredentialsProvider tests', () => { githubProvider.getCredentials({ url: 'https://github.com/backstage', }), - ).resolves.toEqual({ headers: undefined, token: undefined }); + ).resolves.toEqual({ headers: undefined, token: undefined, type: 'token' }); }); }); diff --git a/packages/integration/src/github/GithubCredentialsProvider.ts b/packages/integration/src/github/GithubCredentialsProvider.ts index 0bf217c824..c9fa6cf4ce 100644 --- a/packages/integration/src/github/GithubCredentialsProvider.ts +++ b/packages/integration/src/github/GithubCredentialsProvider.ts @@ -192,9 +192,12 @@ export class GithubAppCredentialsMux { } } +export type GithubCredentialType = 'app' | 'token'; + export type GithubCredentials = { headers?: { [name: string]: string }; token?: string; + type: GithubCredentialType; }; // TODO: Possibly move this to a backend only package so that it's not used in the frontend by mistake @@ -226,8 +229,10 @@ export class GithubCredentialsProvider { const owner = parsed.owner || parsed.name; const repo = parsed.owner ? parsed.name : undefined; + let type: 'app' | 'token' = 'app'; let token = await this.githubAppCredentialsMux.getAppToken(owner, repo); if (!token) { + type = 'token'; token = this.token; } @@ -238,6 +243,7 @@ export class GithubCredentialsProvider { } : undefined, token, + type, }; } } diff --git a/packages/integration/src/github/index.ts b/packages/integration/src/github/index.ts index 99d2f56d0f..ee92b7d934 100644 --- a/packages/integration/src/github/index.ts +++ b/packages/integration/src/github/index.ts @@ -21,4 +21,5 @@ export { export type { GitHubIntegrationConfig } from './config'; export { getGitHubFileFetchUrl, getGitHubRequestOptions } from './core'; export { GithubCredentialsProvider } from './GithubCredentialsProvider'; +export type { GithubCredentialType } from './GithubCredentialsProvider'; export { GitHubIntegration } from './GitHubIntegration'; diff --git a/plugins/catalog-backend/src/ingestion/processors/GithubOrgReaderProcessor.test.ts b/plugins/catalog-backend/src/ingestion/processors/GithubOrgReaderProcessor.test.ts index 9b724def28..dc6a2e5ec2 100644 --- a/plugins/catalog-backend/src/ingestion/processors/GithubOrgReaderProcessor.test.ts +++ b/plugins/catalog-backend/src/ingestion/processors/GithubOrgReaderProcessor.test.ts @@ -13,15 +13,16 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - +jest.mock('@octokit/graphql'); import { getVoidLogger } from '@backstage/backend-common'; import { LocationSpec } from '@backstage/catalog-model'; import { - GitHubIntegration, ScmIntegrations, - ScmIntegrationsGroup, + GithubCredentialsProvider, } from '@backstage/integration'; import { GithubOrgReaderProcessor, parseUrl } from './GithubOrgReaderProcessor'; +import { graphql } from '@octokit/graphql'; +import { ConfigReader } from '@backstage/config'; describe('GithubOrgReaderProcessor', () => { describe('parseUrl', () => { @@ -34,67 +35,27 @@ describe('GithubOrgReaderProcessor', () => { }); describe('implementation', () => { - let integrations: ScmIntegrations; - let github: jest.Mocked>; + const logger = getVoidLogger(); + const integrations = ScmIntegrations.fromConfig( + new ConfigReader({ + integrations: { + github: [ + { + host: 'github.com', + }, + ], + }, + }), + ); beforeEach(() => { - github = { - byHost: jest.fn(), - byUrl: jest.fn(), - list: jest.fn(), - }; - integrations = ({ - github, - } as Partial) as ScmIntegrations; - }); - - it('rejects unknown types', async () => { - const processor = new GithubOrgReaderProcessor({ - providers: [ - { - target: 'https://github.com', - apiBaseUrl: 'https://api.github.com', - }, - ], - integrations, - logger: getVoidLogger(), - }); - const location: LocationSpec = { - type: 'not-github-org', - target: 'https://github.com', - }; - await expect( - processor.readLocation(location, false, () => {}), - ).resolves.toBeFalsy(); - }); - - it('rejects unknown targets from providers', async () => { - const processor = new GithubOrgReaderProcessor({ - providers: [ - { - target: 'https://github.com', - apiBaseUrl: 'https://api.github.com', - }, - ], - integrations, - logger: getVoidLogger(), - }); - const location: LocationSpec = { - type: 'github-org', - target: 'https://not.github.com/apa', - }; - await expect( - processor.readLocation(location, false, () => {}), - ).rejects.toThrow( - /There is no GitHub Org provider that matches https:\/\/not.github.com\/apa/, - ); + jest.resetAllMocks(); }); it('rejects unknown targets from integrations', async () => { const processor = new GithubOrgReaderProcessor({ - providers: [], integrations, - logger: getVoidLogger(), + logger, }); const location: LocationSpec = { type: 'github-org', @@ -106,5 +67,101 @@ describe('GithubOrgReaderProcessor', () => { /There is no GitHub Org provider that matches https:\/\/not.github.com\/apa/, ); }); + + it('should not query for email addresses when GitHub Apps is used for authentication', async () => { + const mockGetCredentials = jest.fn().mockReturnValue({ + headers: { token: 'blah' }, + type: 'app', + }); + + const mockClient = jest.fn(); + + mockClient + .mockResolvedValueOnce({ + organization: { + membersWithRole: { pageInfo: { hasNextPage: false }, nodes: [{}] }, + }, + }) + .mockResolvedValueOnce({ + organization: { + teams: { + pageInfo: { hasNextPage: false }, + nodes: [ + { members: { pageInfo: { hasNextPage: false }, nodes: [{}] } }, + ], + }, + }, + }); + + (graphql.defaults as jest.Mock).mockReturnValue(mockClient); + + jest.spyOn(GithubCredentialsProvider, 'create').mockReturnValue({ + getCredentials: mockGetCredentials, + } as any); + + const processor = new GithubOrgReaderProcessor({ + integrations, + logger, + }); + const location: LocationSpec = { + type: 'github-org', + target: 'https://github.com/backstage', + }; + + await processor.readLocation(location, false, () => {}); + + expect(mockClient).toHaveBeenCalledWith( + expect.stringContaining('@include(if: $email)'), + expect.objectContaining({ email: false }), + ); + }); + + it('should query for email addresses when token is used for authentication', async () => { + const mockGetCredentials = jest.fn().mockReturnValue({ + headers: { token: 'blah' }, + type: 'token', + }); + + const mockClient = jest.fn(); + + mockClient + .mockResolvedValueOnce({ + organization: { + membersWithRole: { pageInfo: { hasNextPage: false }, nodes: [{}] }, + }, + }) + .mockResolvedValueOnce({ + organization: { + teams: { + pageInfo: { hasNextPage: false }, + nodes: [ + { members: { pageInfo: { hasNextPage: false }, nodes: [{}] } }, + ], + }, + }, + }); + + (graphql.defaults as jest.Mock).mockReturnValue(mockClient); + + jest.spyOn(GithubCredentialsProvider, 'create').mockReturnValue({ + getCredentials: mockGetCredentials, + } as any); + + const processor = new GithubOrgReaderProcessor({ + integrations, + logger, + }); + const location: LocationSpec = { + type: 'github-org', + target: 'https://github.com/backstage', + }; + + await processor.readLocation(location, false, () => {}); + + expect(mockClient).toHaveBeenCalledWith( + expect.stringContaining('@include(if: $email)'), + expect.objectContaining({ email: true }), + ); + }); }); }); diff --git a/plugins/catalog-backend/src/ingestion/processors/GithubOrgReaderProcessor.ts b/plugins/catalog-backend/src/ingestion/processors/GithubOrgReaderProcessor.ts index 14204adde7..6a2b4bd8b1 100644 --- a/plugins/catalog-backend/src/ingestion/processors/GithubOrgReaderProcessor.ts +++ b/plugins/catalog-backend/src/ingestion/processors/GithubOrgReaderProcessor.ts @@ -18,16 +18,12 @@ import { LocationSpec } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; import { GithubCredentialsProvider, + GithubCredentialType, ScmIntegrations, } from '@backstage/integration'; import { graphql } from '@octokit/graphql'; import { Logger } from 'winston'; -import { - getOrganizationTeams, - getOrganizationUsers, - ProviderConfig, - readGithubConfig, -} from './github'; +import { getOrganizationTeams, getOrganizationUsers } from './github'; import * as results from './results'; import { CatalogProcessor, CatalogProcessorEmit } from './types'; import { buildOrgHierarchy } from './util/org'; @@ -38,7 +34,6 @@ type GraphQL = typeof graphql; * Extracts teams and users out of a GitHub org. */ export class GithubOrgReaderProcessor implements CatalogProcessor { - private readonly providers: ProviderConfig[]; private readonly integrations: ScmIntegrations; private readonly logger: Logger; @@ -47,17 +42,11 @@ export class GithubOrgReaderProcessor implements CatalogProcessor { return new GithubOrgReaderProcessor({ ...options, - providers: readGithubConfig(config), integrations, }); } - constructor(options: { - providers: ProviderConfig[]; - integrations: ScmIntegrations; - logger: Logger; - }) { - this.providers = options.providers; + constructor(options: { integrations: ScmIntegrations; logger: Logger }) { this.integrations = options.integrations; this.logger = options.logger; } @@ -71,14 +60,14 @@ export class GithubOrgReaderProcessor implements CatalogProcessor { return false; } - const client = await this.createClient(location.target); + const { client, tokenType } = await this.createClient(location.target); const { org } = parseUrl(location.target); // Read out all of the raw data const startTimestamp = Date.now(); this.logger.info('Reading GitHub users and groups'); - const { users } = await getOrganizationUsers(client, org); + const { users } = await getOrganizationUsers(client, org, tokenType); const { groups, groupMemberUsers } = await getOrganizationTeams( client, org, @@ -112,65 +101,31 @@ export class GithubOrgReaderProcessor implements CatalogProcessor { return true; } - private async createClient(orgUrl: string): Promise { - let client = await this.createClientFromIntegrations(orgUrl); + private async createClient( + orgUrl: string, + ): Promise<{ client: GraphQL; tokenType: GithubCredentialType }> { + const gitHubConfig = this.integrations.github.byUrl(orgUrl)?.config; - if (!client) { - client = await this.createClientFromProvider(orgUrl); - } - - if (!client) { + if (!gitHubConfig) { throw new Error( - `There is no GitHub Org provider that matches ${orgUrl}. Please add a configuration for an integration or add an entry for it under catalog.processors.githubOrg.providers.`, + `There is no GitHub Org provider that matches ${orgUrl}. Please add a configuration for an integration.`, ); } - return client; - } - - private async createClientFromProvider( - orgUrl: string, - ): Promise { - const provider = this.providers.find(p => - orgUrl.startsWith(`${p.target}/`), - ); - - if (!provider) { - return undefined; - } - - this.logger.warn( - 'GithubOrgReaderProcessor uses provider defined in catalog.processors.githubOrg.providers, migrate to integrations instead. See https://backstage.io/docs/integrations/github/locations', - ); - - return !provider.token - ? graphql - : graphql.defaults({ - baseUrl: provider.apiBaseUrl, - headers: { - authorization: `token ${provider.token}`, - }, - }); - } - - private async createClientFromIntegrations( - orgUrl: string, - ): Promise { - const gitHubConfig = this.integrations.github.byUrl(orgUrl)?.config; - if (!gitHubConfig) { - return undefined; - } - const credentialsProvider = GithubCredentialsProvider.create(gitHubConfig); - - const { headers } = await credentialsProvider.getCredentials({ + const { + headers, + type: tokenType, + } = await credentialsProvider.getCredentials({ url: orgUrl, }); - return graphql.defaults({ + const client = graphql.defaults({ baseUrl: gitHubConfig.apiBaseUrl, headers, }); + + return { client, tokenType }; } } diff --git a/plugins/catalog-backend/src/ingestion/processors/github/github.test.ts b/plugins/catalog-backend/src/ingestion/processors/github/github.test.ts index 88bddc93fb..c5094230f8 100644 --- a/plugins/catalog-backend/src/ingestion/processors/github/github.test.ts +++ b/plugins/catalog-backend/src/ingestion/processors/github/github.test.ts @@ -65,7 +65,9 @@ describe('github', () => { graphqlMsw.query('users', (_req, res, ctx) => res(ctx.data(input))), ); - await expect(getOrganizationUsers(graphql, 'a')).resolves.toEqual(output); + await expect( + getOrganizationUsers(graphql, 'a', 'token'), + ).resolves.toEqual(output); }); }); diff --git a/plugins/catalog-backend/src/ingestion/processors/github/github.ts b/plugins/catalog-backend/src/ingestion/processors/github/github.ts index f6cfcb8901..2cc97d2ec5 100644 --- a/plugins/catalog-backend/src/ingestion/processors/github/github.ts +++ b/plugins/catalog-backend/src/ingestion/processors/github/github.ts @@ -15,6 +15,7 @@ */ import { GroupEntity, UserEntity } from '@backstage/catalog-model'; +import { GithubCredentialType } from '@backstage/integration'; import { graphql } from '@octokit/graphql'; // Graphql types @@ -77,13 +78,20 @@ export type Connection = { export async function getOrganizationUsers( client: typeof graphql, org: string, + tokenType: GithubCredentialType, ): Promise<{ users: UserEntity[] }> { const query = ` - query users($org: String!, $cursor: String) { + query users($org: String!, $email: Boolean!, $cursor: String) { organization(login: $org) { membersWithRole(first: 100, after: $cursor) { pageInfo { hasNextPage, endCursor } - nodes { avatarUrl, bio, email, login, name } + nodes { + avatarUrl, + bio, + email @include(if: $email), + login, + name + } } } }`; @@ -119,7 +127,7 @@ export async function getOrganizationUsers( query, r => r.organization?.membersWithRole, mapper, - { org }, + { org, email: tokenType === 'token' }, ); return { users }; From cdda0401293fadf8f3529cd9d53839bcb1a9af13 Mon Sep 17 00:00:00 2001 From: Johan Haals Date: Fri, 21 May 2021 14:28:42 +0200 Subject: [PATCH 2/4] Update api-report Signed-off-by: Johan Haals --- packages/integration/api-report.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/integration/api-report.md b/packages/integration/api-report.md index 5387eb12db..31cbc8b5d8 100644 --- a/packages/integration/api-report.md +++ b/packages/integration/api-report.md @@ -115,6 +115,9 @@ export class GithubCredentialsProvider { }): Promise; } +// @public (undocumented) +export type GithubCredentialType = 'app' | 'token'; + // @public (undocumented) export class GitHubIntegration implements ScmIntegration { constructor(integrationConfig: GitHubIntegrationConfig); From 66fee560e51b5085d3ade369850292a54606a368 Mon Sep 17 00:00:00 2001 From: Johan Haals Date: Fri, 21 May 2021 14:46:51 +0200 Subject: [PATCH 3/4] Update packages/integration/src/github/GithubCredentialsProvider.ts Signed-off-by: Johan Haals Co-authored-by: Ben Lambert <3645856+benjdlambert@users.noreply.github.com> --- packages/integration/src/github/GithubCredentialsProvider.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/integration/src/github/GithubCredentialsProvider.ts b/packages/integration/src/github/GithubCredentialsProvider.ts index c9fa6cf4ce..24f0b13a33 100644 --- a/packages/integration/src/github/GithubCredentialsProvider.ts +++ b/packages/integration/src/github/GithubCredentialsProvider.ts @@ -229,7 +229,7 @@ export class GithubCredentialsProvider { const owner = parsed.owner || parsed.name; const repo = parsed.owner ? parsed.name : undefined; - let type: 'app' | 'token' = 'app'; + let type: GithubCredentialType = 'app'; let token = await this.githubAppCredentialsMux.getAppToken(owner, repo); if (!token) { type = 'token'; From 7417241fcc044abc008132be779ff9cf5a074b06 Mon Sep 17 00:00:00 2001 From: Johan Haals Date: Fri, 21 May 2021 14:52:20 +0200 Subject: [PATCH 4/4] fix format Signed-off-by: Johan Haals --- packages/integration/src/github/GithubCredentialsProvider.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/integration/src/github/GithubCredentialsProvider.ts b/packages/integration/src/github/GithubCredentialsProvider.ts index 24f0b13a33..9bc09deab3 100644 --- a/packages/integration/src/github/GithubCredentialsProvider.ts +++ b/packages/integration/src/github/GithubCredentialsProvider.ts @@ -229,7 +229,7 @@ export class GithubCredentialsProvider { const owner = parsed.owner || parsed.name; const repo = parsed.owner ? parsed.name : undefined; - let type: GithubCredentialType = 'app'; + let type: GithubCredentialType = 'app'; let token = await this.githubAppCredentialsMux.getAppToken(owner, repo); if (!token) { type = 'token';