From 17a4968d408b44f857a1eedb0fe2c9b444e3b91b Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Mon, 5 Sep 2022 21:56:28 +0100 Subject: [PATCH 01/10] Avoid loading github installation if owner is not in allowedInstallationOwners Signed-off-by: Alex Crome --- .../src/github/SingleInstanceGithubCredentialsProvider.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts index 2fd2e7f7dc..8b87d985a5 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts @@ -93,12 +93,13 @@ class GithubAppManager { owner: string, repo?: string, ): Promise<{ accessToken: string | undefined }> { - const { installationId, suspended } = await this.getInstallationData(owner); if (this.allowedInstallationOwners) { if (!this.allowedInstallationOwners?.includes(owner)) { return { accessToken: undefined }; // An empty token allows anonymous access to public repos } } + + const { installationId, suspended } = await this.getInstallationData(owner); if (suspended) { throw new Error(`The GitHub application for ${owner} is suspended`); } From d0e8705afd554e953e011696aea75c39c9d3febf Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Mon, 5 Sep 2022 22:05:29 +0100 Subject: [PATCH 02/10] Moved Github Installation loading to be within cached data Signed-off-by: Alex Crome --- .../SingleInstanceGithubCredentialsProvider.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts index 8b87d985a5..5e98ddd3b1 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts @@ -99,19 +99,22 @@ class GithubAppManager { } } - const { installationId, suspended } = await this.getInstallationData(owner); - if (suspended) { - throw new Error(`The GitHub application for ${owner} is suspended`); - } - 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 { installationId, suspended } = await this.getInstallationData( + owner, + ); + if (suspended) { + throw new Error(`The GitHub application for ${owner} is suspended`); + } + const result = await this.appClient.apps.createInstallationAccessToken({ installation_id: installationId, headers: HEADERS, }); + if (repo && result.data.repository_selection === 'selected') { const installationClient = new Octokit({ baseUrl: this.baseUrl, From d57e5f182cd983f4f1007993fa4a13c705072449 Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Mon, 5 Sep 2022 23:20:18 +0100 Subject: [PATCH 03/10] Add initial test around cache Signed-off-by: Alex Crome --- ...eInstanceGithubCredentialsProvider.test.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts index 1dcc45c1ef..3d58ffbb61 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts @@ -400,4 +400,36 @@ describe('SingleInstanceGithubCredentialsProvider tests', () => { }), ).resolves.not.toThrow(); }); + + it('should cache access token', async () => { + octokit.apps.listInstallations.mockResolvedValueOnce({ + headers: { + etag: '123', + }, + data: [ + { + id: 1, + repository_selection: 'all', + account: { + login: 'backstage', + }, + }, + ], + } as RestEndpointMethodTypes['apps']['listInstallations']['response']); + + octokit.apps.createInstallationAccessToken.mockResolvedValueOnce({ + data: { + expires_at: DateTime.local().plus({ hours: 1 }).toString(), + token: 'secret_token', + }, + } as RestEndpointMethodTypes['apps']['createInstallationAccessToken']['response']); + + for (let i = 0; i < 2; i++) { + const { token, headers } = await github.getCredentials({ + url: 'https://github.com/backstage', + }); + expect(headers).toEqual({ Authorization: 'Bearer secret_token' }); + expect(token).toEqual('secret_token'); + } + }); }); From 11904b7616a4cf5261b38271d206ccd5f1f4ef66 Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Tue, 6 Sep 2022 09:26:28 +0100 Subject: [PATCH 04/10] Share access token between repos in the same app Signed-off-by: Alex Crome --- ...eInstanceGithubCredentialsProvider.test.ts | 29 ++++---- ...SingleInstanceGithubCredentialsProvider.ts | 66 ++++++++++++------- 2 files changed, 57 insertions(+), 38 deletions(-) diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts index 3d58ffbb61..b48103eea1 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts @@ -57,6 +57,7 @@ describe('SingleInstanceGithubCredentialsProvider tests', () => { token: 'hardcoded_token', }); }); + it('create repository specific tokens', async () => { octokit.apps.listInstallations.mockResolvedValue({ headers: { @@ -66,11 +67,6 @@ describe('SingleInstanceGithubCredentialsProvider tests', () => { { id: 1, repository_selection: 'selected', - account: null, - }, - { - id: 2, - repository_selection: 'selected', account: { login: 'backstage', }, @@ -195,9 +191,14 @@ describe('SingleInstanceGithubCredentialsProvider tests', () => { data: { expires_at: DateTime.local().plus({ hours: 1 }).toString(), token: 'secret_token', + repository_selection: 'selected', }, } as RestEndpointMethodTypes['apps']['createInstallationAccessToken']['response']); + octokit.apps.listReposAccessibleToInstallation.mockReturnValue({ + data: [{ name: 'some-repo' }], + } as unknown as RestEndpointMethodTypes['apps']['listReposAccessibleToInstallation']['response']); + const { token, headers } = await github.getCredentials({ url: 'https://github.com/backstage', }); @@ -402,7 +403,7 @@ describe('SingleInstanceGithubCredentialsProvider tests', () => { }); it('should cache access token', async () => { - octokit.apps.listInstallations.mockResolvedValueOnce({ + octokit.apps.listInstallations.mockReturnValue({ headers: { etag: '123', }, @@ -417,19 +418,19 @@ describe('SingleInstanceGithubCredentialsProvider tests', () => { ], } as RestEndpointMethodTypes['apps']['listInstallations']['response']); - octokit.apps.createInstallationAccessToken.mockResolvedValueOnce({ + octokit.apps.createInstallationAccessToken.mockReturnValue({ data: { expires_at: DateTime.local().plus({ hours: 1 }).toString(), token: 'secret_token', }, } as RestEndpointMethodTypes['apps']['createInstallationAccessToken']['response']); - for (let i = 0; i < 2; i++) { - const { token, headers } = await github.getCredentials({ - url: 'https://github.com/backstage', - }); - expect(headers).toEqual({ Authorization: 'Bearer secret_token' }); - expect(token).toEqual('secret_token'); - } + await github.getCredentials({ url: 'https://github.com/backstage' }); + await github.getCredentials({ url: 'https://github.com/backstage' }); + + expect(octokit.apps.listInstallations.mock.calls.length).toBe(1); + expect(octokit.apps.createInstallationAccessToken.mock.calls.length).toBe( + 1, + ); }); }); diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts index 5e98ddd3b1..a502590a17 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts @@ -30,29 +30,52 @@ type InstallationData = { suspended: boolean; }; +type InstallationTokenData = { + token: string; + expiresAt: DateTime; + repositories?: String[]; +}; + class Cache { - private readonly tokenCache = new Map< - string, - { token: string; expiresAt: DateTime } - >(); + private readonly tokenCache = new Map(); async getOrCreateToken( - key: string, - supplier: () => Promise<{ token: string; expiresAt: DateTime }>, + owner: string, + repo: string | undefined, + supplier: () => Promise, ): Promise<{ accessToken: string }> { - const item = this.tokenCache.get(key); - if (item && this.isNotExpired(item.expiresAt)) { - return { accessToken: item.token }; + let ownerData = this.tokenCache.get(owner); + + if (!ownerData || !this.isNotExpired(ownerData.expiresAt)) { + ownerData = await supplier(); + this.tokenCache.set(owner, ownerData); } - const result = await supplier(); - this.tokenCache.set(key, result); - return { accessToken: result.token }; + if (!this.appliesToRepo(ownerData, repo)) { + throw new Error( + `The Backstage GitHub application used in the ${owner} organization does not have access to a repository with the name ${repo}`, + ); + } + + return { accessToken: ownerData.token }; } // consider timestamps older than 50 minutes to be expired. private isNotExpired = (date: DateTime) => date.diff(DateTime.local(), 'minutes').minutes > 50; + + private appliesToRepo(tokenData: InstallationTokenData, repo?: string) { + // If no specific repo has been requested the token is applicable + if (repo === undefined) { + return true; + } + // If the token is restricted to repositories, the token only applies if the repo is in the allow list + if (tokenData.repositories !== undefined) { + return tokenData.repositories.includes(repo); + } + // Otherwise the token is applicable + return true; + } } /** @@ -99,10 +122,8 @@ class GithubAppManager { } } - 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 () => { + return this.cache.getOrCreateToken(owner, repo, async () => { const { installationId, suspended } = await this.getInstallationData( owner, ); @@ -115,7 +136,9 @@ class GithubAppManager { headers: HEADERS, }); - if (repo && result.data.repository_selection === 'selected') { + let repositoryNames; + + if (result.data.repository_selection === 'selected') { const installationClient = new Octokit({ baseUrl: this.baseUrl, auth: result.data.token, @@ -126,18 +149,13 @@ class GithubAppManager { // The return type of the paginate method is incorrect. const repositories: RestEndpointMethodTypes['apps']['listReposAccessibleToInstallation']['response']['data']['repositories'] = repos.repositories ?? repos; - const hasRepo = 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}`, - ); - } + + repositoryNames = repositories.map(repository => repository.name); } return { token: result.data.token, expiresAt: DateTime.fromISO(result.data.expires_at), + repositories: repositoryNames, }; }); } From de6c99b043288f9acae6861207f4884c88838d1e Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Tue, 6 Sep 2022 09:35:41 +0100 Subject: [PATCH 05/10] Fix access token cache time Signed-off-by: Alex Crome --- ...eInstanceGithubCredentialsProvider.test.ts | 34 ++++++++++++++++++- ...SingleInstanceGithubCredentialsProvider.ts | 7 ++-- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts index b48103eea1..d9b2a4e749 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts @@ -420,7 +420,7 @@ describe('SingleInstanceGithubCredentialsProvider tests', () => { octokit.apps.createInstallationAccessToken.mockReturnValue({ data: { - expires_at: DateTime.local().plus({ hours: 1 }).toString(), + expires_at: DateTime.local().plus({ minutes: 11 }).toString(), token: 'secret_token', }, } as RestEndpointMethodTypes['apps']['createInstallationAccessToken']['response']); @@ -433,4 +433,36 @@ describe('SingleInstanceGithubCredentialsProvider tests', () => { 1, ); }); + + it('should expire access token cache 10 mins before token expires', async () => { + octokit.apps.listInstallations.mockReturnValue({ + headers: { + etag: '123', + }, + data: [ + { + id: 1, + repository_selection: 'all', + account: { + login: 'backstage', + }, + }, + ], + } as RestEndpointMethodTypes['apps']['listInstallations']['response']); + + octokit.apps.createInstallationAccessToken.mockReturnValue({ + data: { + expires_at: DateTime.local().plus({ minutes: 10 }).toString(), + token: 'secret_token', + }, + } as RestEndpointMethodTypes['apps']['createInstallationAccessToken']['response']); + + await github.getCredentials({ url: 'https://github.com/backstage' }); + await github.getCredentials({ url: 'https://github.com/backstage' }); + + expect(octokit.apps.listInstallations.mock.calls.length).toBe(2); + expect(octokit.apps.createInstallationAccessToken.mock.calls.length).toBe( + 2, + ); + }); }); diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts index a502590a17..02e0893d8a 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts @@ -46,8 +46,10 @@ class Cache { ): Promise<{ accessToken: string }> { let ownerData = this.tokenCache.get(owner); - if (!ownerData || !this.isNotExpired(ownerData.expiresAt)) { + if (!ownerData || this.isExpired(ownerData.expiresAt)) { ownerData = await supplier(); + // Allow 10 minutes grace to account for clock skew + ownerData.expiresAt = ownerData.expiresAt.minus({ minutes: 10 }); this.tokenCache.set(owner, ownerData); } @@ -61,8 +63,7 @@ class Cache { } // consider timestamps older than 50 minutes to be expired. - private isNotExpired = (date: DateTime) => - date.diff(DateTime.local(), 'minutes').minutes > 50; + private isExpired = (date: DateTime) => DateTime.local() > date; private appliesToRepo(tokenData: InstallationTokenData, repo?: string) { // If no specific repo has been requested the token is applicable From f76f22c649ea8a773911829467ad1dfb5360201a Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Wed, 7 Sep 2022 10:11:46 +0100 Subject: [PATCH 06/10] Added changeset + docs Signed-off-by: Alex Crome --- .changeset/sweet-grapes-explain.md | 8 ++++++++ docs/integrations/github/github-apps.md | 4 +++- 2 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 .changeset/sweet-grapes-explain.md diff --git a/.changeset/sweet-grapes-explain.md b/.changeset/sweet-grapes-explain.md new file mode 100644 index 0000000000..3b75b8849d --- /dev/null +++ b/.changeset/sweet-grapes-explain.md @@ -0,0 +1,8 @@ +--- +'@backstage/integration': minor +--- + +Improved caching around github app tokens. +Tokens are now cached for 50 minutes, not 10. +Calls to get app installations are also included in this cache. +If you have more than one github app configured, consider adding `allowedInstallationOwners` to your apps configuration to gain the most benefit from these performance changes. diff --git a/docs/integrations/github/github-apps.md b/docs/integrations/github/github-apps.md index 4f2c58bbec..38459acf75 100644 --- a/docs/integrations/github/github-apps.md +++ b/docs/integrations/github/github-apps.md @@ -98,7 +98,9 @@ integrations: ### Limiting the GitHub App installations If you want to limit the GitHub app installations visible to backstage you may -optionally include the `allowedInstallationOwners` option. +optionally include the `allowedInstallationOwners` option. If you configure +multiple apps, specifying this will bring some small performance benefits +as backstage can more easily select which app to use for a url. ```yaml appId: app id From a7f6f1c3fad15d11a1812058ab58a48791e6faae Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Wed, 7 Sep 2022 11:51:54 +0100 Subject: [PATCH 07/10] Remove erroneous comment Signed-off-by: Alex Crome --- .../src/github/SingleInstanceGithubCredentialsProvider.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts index 02e0893d8a..aeaa0f5e04 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts @@ -62,7 +62,6 @@ class Cache { return { accessToken: ownerData.token }; } - // consider timestamps older than 50 minutes to be expired. private isExpired = (date: DateTime) => DateTime.local() > date; private appliesToRepo(tokenData: InstallationTokenData, repo?: string) { From 13ec891dd37dafc3cb7ee0d0910ceaf1d46f58e7 Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Thu, 8 Sep 2022 12:31:54 +0100 Subject: [PATCH 08/10] Correct URL spelling Signed-off-by: Alex Crome --- docs/integrations/github/github-apps.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/integrations/github/github-apps.md b/docs/integrations/github/github-apps.md index 38459acf75..b526c306e7 100644 --- a/docs/integrations/github/github-apps.md +++ b/docs/integrations/github/github-apps.md @@ -100,7 +100,7 @@ integrations: If you want to limit the GitHub app installations visible to backstage you may optionally include the `allowedInstallationOwners` option. If you configure multiple apps, specifying this will bring some small performance benefits -as backstage can more easily select which app to use for a url. +as backstage can more easily select which app to use for a URL. ```yaml appId: app id From f95ceb273751456e2d2f8e59272d689929f13dec Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Thu, 8 Sep 2022 17:36:28 +0100 Subject: [PATCH 09/10] PR Feedback Signed-off-by: Alex Crome --- .changeset/sweet-grapes-explain.md | 2 +- .../SingleInstanceGithubCredentialsProvider.ts | 18 +++++++++++------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/.changeset/sweet-grapes-explain.md b/.changeset/sweet-grapes-explain.md index 3b75b8849d..014fbb3d41 100644 --- a/.changeset/sweet-grapes-explain.md +++ b/.changeset/sweet-grapes-explain.md @@ -1,5 +1,5 @@ --- -'@backstage/integration': minor +'@backstage/integration': patch --- Improved caching around github app tokens. diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts index aeaa0f5e04..02f050f69f 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.ts @@ -44,22 +44,26 @@ class Cache { repo: string | undefined, supplier: () => Promise, ): Promise<{ accessToken: string }> { - let ownerData = this.tokenCache.get(owner); + let existingInstallationData = this.tokenCache.get(owner); - if (!ownerData || this.isExpired(ownerData.expiresAt)) { - ownerData = await supplier(); + if ( + !existingInstallationData || + this.isExpired(existingInstallationData.expiresAt) + ) { + existingInstallationData = await supplier(); // Allow 10 minutes grace to account for clock skew - ownerData.expiresAt = ownerData.expiresAt.minus({ minutes: 10 }); - this.tokenCache.set(owner, ownerData); + existingInstallationData.expiresAt = + existingInstallationData.expiresAt.minus({ minutes: 10 }); + this.tokenCache.set(owner, existingInstallationData); } - if (!this.appliesToRepo(ownerData, repo)) { + if (!this.appliesToRepo(existingInstallationData, repo)) { throw new Error( `The Backstage GitHub application used in the ${owner} organization does not have access to a repository with the name ${repo}`, ); } - return { accessToken: ownerData.token }; + return { accessToken: existingInstallationData.token }; } private isExpired = (date: DateTime) => DateTime.local() > date; From a68c0b3f94bd5d1a2b64be6110627b8c908aa479 Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Thu, 8 Sep 2022 18:41:09 +0100 Subject: [PATCH 10/10] Fix potentially flakey expiration test by making sure the record has less than 10 mins remaining, rather than exactly 10 mins Signed-off-by: Alex Crome --- .../github/SingleInstanceGithubCredentialsProvider.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts index d9b2a4e749..a1544fbc68 100644 --- a/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts +++ b/packages/integration/src/github/SingleInstanceGithubCredentialsProvider.test.ts @@ -434,7 +434,7 @@ describe('SingleInstanceGithubCredentialsProvider tests', () => { ); }); - it('should expire access token cache 10 mins before token expires', async () => { + it('should expire access token cache when less than 10 mins before token expires', async () => { octokit.apps.listInstallations.mockReturnValue({ headers: { etag: '123', @@ -452,7 +452,9 @@ describe('SingleInstanceGithubCredentialsProvider tests', () => { octokit.apps.createInstallationAccessToken.mockReturnValue({ data: { - expires_at: DateTime.local().plus({ minutes: 10 }).toString(), + expires_at: DateTime.local() + .plus({ minutes: 9, seconds: 59, milliseconds: 999 }) + .toString(), token: 'secret_token', }, } as RestEndpointMethodTypes['apps']['createInstallationAccessToken']['response']);