From 8cd21b9b596f78758c46a249fd8fde979390c4d2 Mon Sep 17 00:00:00 2001 From: Christopher Kruse Date: Thu, 2 Feb 2023 11:40:44 -0800 Subject: [PATCH] fix: check for teams before creating repository Shifts the team check for the 'access' parameter to happen prior to the repo creation, so that repositories are not created and have no permissions set on them due to missing teams. Signed-off-by: Christopher Kruse --- .../builtin/github/githubRepoCreate.test.ts | 8 +++ .../actions/builtin/github/helpers.ts | 46 +++++++++------ .../actions/builtin/publish/github.test.ts | 56 ++++++++++++++----- 3 files changed, 81 insertions(+), 29 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubRepoCreate.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubRepoCreate.test.ts index f15747fd6b..a9ef8c3daa 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubRepoCreate.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubRepoCreate.test.ts @@ -42,6 +42,7 @@ const mockOctokit = { }, teams: { addOrUpdateRepoPermissionsInOrg: jest.fn(), + getByName: jest.fn(), }, }, }; @@ -96,6 +97,13 @@ describe('github:repo:create', () => { data: { type: 'Organization' }, }); + mockOctokit.rest.teams.getByName.mockResolvedValue({ + data: { + name: 'blam', + id: 42, + }, + }); + mockOctokit.rest.repos.createInOrg.mockResolvedValue({ data: {} }); await action.handler(mockContext); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts index 25f343da9e..dc8c4cd856 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/helpers.ts @@ -135,6 +135,10 @@ export async function createGithubRepoWithCollaboratorsAndTopics( username: owner, }); + if (access?.startsWith(`${owner}/`)) { + await validateAccessTeam(client, access); + } + const repoCreationPromise = user.data.type === 'Organization' ? client.rest.repos.createInOrg({ @@ -190,22 +194,13 @@ export async function createGithubRepoWithCollaboratorsAndTopics( if (access?.startsWith(`${owner}/`)) { const [, team] = access.split('/'); - try { - await client.rest.teams.addOrUpdateRepoPermissionsInOrg({ - org: owner, - team_slug: team, - owner, - repo, - permission: 'admin', - }); - } catch (e) { - if (e.data.message === 'Not Found') { - const message = `Received 'Not Found' from the API; one of org: - ${owner}, team: ${team} or repo: ${repo} was not found within GitHub.`; - logger.warn(message); - throw new Error(message, { cause: e }); - } - } + await client.rest.teams.addOrUpdateRepoPermissionsInOrg({ + org: owner, + team_slug: team, + owner, + repo, + permission: 'admin', + }); // No need to add access if it's the person who owns the personal account } else if (access && access !== owner) { await client.rest.repos.addCollaborator({ @@ -360,3 +355,22 @@ function extractCollaboratorName( if ('user' in collaborator) return collaborator.user; return collaborator.team; } + +async function validateAccessTeam(client: Octokit, access: string) { + const [org, team_slug] = access.split('/'); + try { + // Below rule disabled because of a 'getByName' check for a different library + // incorrectly triggers here. + // eslint-disable-next-line testing-library/no-await-sync-query + await client.rest.teams.getByName({ + org, + team_slug, + }); + } catch (e) { + if (e.response.data.message === 'Not Found') { + const message = `Received 'Not Found' from the API; one of org: + ${org} or team: ${team_slug} was not found within GitHub.`; + throw new Error(message, { cause: e }); + } + } +} diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.test.ts index 0edeafecfd..94b51f3e94 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.test.ts @@ -45,6 +45,7 @@ const mockOctokit = { replaceAllTopics: jest.fn(), }, teams: { + getByName: jest.fn(), addOrUpdateRepoPermissionsInOrg: jest.fn(), }, }, @@ -96,11 +97,41 @@ describe('publish:github', () => { }); }); + it('should fail to create if the team is not found in the org', async () => { + mockOctokit.rest.users.getByUsername.mockResolvedValue({ + data: { type: 'Organization' }, + }); + + mockOctokit.rest.teams.getByName.mockRejectedValue({ + response: { + status: 404, + data: { + message: 'Not Found', + documentation_url: + 'https://docs.github.com/en/rest/teams/teams#add-or-update-team-repository-permissions', + }, + }, + }); + + await expect(action.handler(mockContext)).rejects.toThrow( + "Received 'Not Found' from the API;", + ); + + expect(mockOctokit.rest.repos.createInOrg).not.toHaveBeenCalled(); + }); + it('should call the githubApis with the correct values for createInOrg', async () => { mockOctokit.rest.users.getByUsername.mockResolvedValue({ data: { type: 'Organization' }, }); + mockOctokit.rest.teams.getByName.mockResolvedValue({ + data: { + name: 'blam', + id: 42, + }, + }); + mockOctokit.rest.repos.createInOrg.mockResolvedValue({ data: {} }); await action.handler(mockContext); @@ -523,24 +554,23 @@ describe('publish:github', () => { data: { type: 'User' }, }); - mockOctokit.rest.repos.createForAuthenticatedUser.mockResolvedValue({ - data: { - clone_url: 'https://github.com/clone/url.git', - html_url: 'https://github.com/html/url', - }, - }); - - mockOctokit.rest.teams.addOrUpdateRepoPermissionsInOrg.mockRejectedValue({ - status: 404, - data: { - message: 'Not Found', - documentation_url: - 'https://docs.github.com/en/rest/teams/teams#add-or-update-team-repository-permissions', + mockOctokit.rest.teams.getByName.mockRejectedValue({ + response: { + status: 404, + data: { + message: 'Not Found', + documentation_url: + 'https://docs.github.com/en/rest/teams/teams#add-or-update-team-repository-permissions', + }, }, }); await expect(action.handler(mockContext)).rejects.toThrow( "Received 'Not Found' from the API;", ); + + expect( + mockOctokit.rest.repos.createForAuthenticatedUser, + ).not.toHaveBeenCalled(); }); it('should add outside collaborators when provided', async () => {