From 7179094d9ca33ed2a0d6fc7cabdc943fdf277bef Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Tue, 2 Jul 2024 16:12:04 -0400 Subject: [PATCH] feat: add repository settings to mirror creation page to disable actions This adds the ability to disable actions all together from the mirror creation page. It also always users to specify what types of actions are allowed to run. --- package-lock.json | 25 +++ package.json | 2 + .../[organizationId]/forks/[forkId]/page.tsx | 4 + .../components/dialog/CreateMirrorDialog.tsx | 78 +++++++- src/server/repos/controller.ts | 27 ++- src/server/repos/schema.ts | 9 + test/octomock/index.ts | 2 + test/server/repos.test.ts | 188 +++++++++++++++++- 8 files changed, 327 insertions(+), 8 deletions(-) diff --git a/package-lock.json b/package-lock.json index ad67c85..6d79474 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,6 +9,7 @@ "version": "0.1.0", "license": "MIT", "dependencies": { + "@hookform/resolvers": "3.7.0", "@octokit/auth-app": "6.1.1", "@octokit/graphql-schema": "15.18.1", "@primer/octicons-react": "19.9.0", @@ -27,6 +28,7 @@ "proxy-agent": "6.4.0", "react": "18.3.1", "react-dom": "18.3.1", + "react-hook-form": "7.52.1", "simple-git": "3.25.0", "styled-components": "5.3.11", "superjson": "2.2.1", @@ -2042,6 +2044,14 @@ "resolved": "https://registry.npmjs.org/@hapi/bourne/-/bourne-2.1.0.tgz", "integrity": "sha512-i1BpaNDVLJdRBEKeJWkVO6tYX6DMFBuwMhSuWqLsY4ufeTKGVuV5rBsUhxPayXqnnWHgXUAmWK16H/ykO5Wj4Q==" }, + "node_modules/@hookform/resolvers": { + "version": "3.7.0", + "resolved": "https://registry.npmjs.org/@hookform/resolvers/-/resolvers-3.7.0.tgz", + "integrity": "sha512-42p5X18noBV3xqOpTlf2V5qJZwzNgO4eLzHzmKGh/w7z4+4XqRw5AsESVkqE+qwAuRRlg2QG12EVEjPkrRIbeg==", + "peerDependencies": { + "react-hook-form": "^7.0.0" + } + }, "node_modules/@humanwhocodes/config-array": { "version": "0.11.14", "resolved": "https://registry.npmjs.org/@humanwhocodes/config-array/-/config-array-0.11.14.tgz", @@ -12733,6 +12743,21 @@ "react": "^18.3.1" } }, + "node_modules/react-hook-form": { + "version": "7.52.1", + "resolved": "https://registry.npmjs.org/react-hook-form/-/react-hook-form-7.52.1.tgz", + "integrity": "sha512-uNKIhaoICJ5KQALYZ4TOaOLElyM+xipord+Ha3crEFhTntdLvWZqVY49Wqd/0GiVCA/f9NjemLeiNPjG7Hpurg==", + "engines": { + "node": ">=12.22.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/react-hook-form" + }, + "peerDependencies": { + "react": "^16.8.0 || ^17 || ^18 || ^19" + } + }, "node_modules/react-intersection-observer": { "version": "9.5.3", "resolved": "https://registry.npmjs.org/react-intersection-observer/-/react-intersection-observer-9.5.3.tgz", diff --git a/package.json b/package.json index de19a30..44e437b 100644 --- a/package.json +++ b/package.json @@ -19,6 +19,7 @@ "prepare": "test -d node_modules/husky && husky || echo \"husky is not installed\"" }, "dependencies": { + "@hookform/resolvers": "3.7.0", "@octokit/auth-app": "6.1.1", "@octokit/graphql-schema": "15.18.1", "@primer/octicons-react": "19.9.0", @@ -37,6 +38,7 @@ "proxy-agent": "6.4.0", "react": "18.3.1", "react-dom": "18.3.1", + "react-hook-form": "7.52.1", "simple-git": "3.25.0", "styled-components": "5.3.11", "superjson": "2.2.1", diff --git a/src/app/[organizationId]/forks/[forkId]/page.tsx b/src/app/[organizationId]/forks/[forkId]/page.tsx index e58debb..d6dc52d 100644 --- a/src/app/[organizationId]/forks/[forkId]/page.tsx +++ b/src/app/[organizationId]/forks/[forkId]/page.tsx @@ -36,6 +36,7 @@ import Fuse from 'fuse.js' import { useForkData } from 'hooks/useFork' import { useOrgData } from 'hooks/useOrganization' import { useCallback, useState } from 'react' +import { RepositorySettingsSchema } from 'server/repos/schema' const Fork = () => { const { organizationId } = useParams() @@ -204,9 +205,11 @@ const Fork = () => { async ({ repoName, branchName, + settings, }: { repoName: string branchName: string + settings: RepositorySettingsSchema }) => { // close other flashes and dialogs when this is opened closeAllFlashes() @@ -219,6 +222,7 @@ const Fork = () => { forkRepoName: forkData?.data?.name ?? '', forkRepoOwner: forkData?.data?.owner.login ?? '', forkId: String(forkData?.data?.id), + settings, }) .then((res) => { if (res.success) { diff --git a/src/app/components/dialog/CreateMirrorDialog.tsx b/src/app/components/dialog/CreateMirrorDialog.tsx index 9238ba8..537ee31 100644 --- a/src/app/components/dialog/CreateMirrorDialog.tsx +++ b/src/app/components/dialog/CreateMirrorDialog.tsx @@ -1,8 +1,19 @@ -import { Box, FormControl, Label, Link, Text, TextInput } from '@primer/react' +import { zodResolver } from '@hookform/resolvers/zod' +import { + Box, + Checkbox, + FormControl, + Label, + Link, + Select, + Text, + TextInput, +} from '@primer/react' import { Stack } from '@primer/react/lib-esm/Stack' import { Dialog } from '@primer/react/lib-esm/drafts' - import { useState } from 'react' +import { useForm } from 'react-hook-form' +import { RepositorySettingsSchema } from 'server/repos/schema' interface CreateMirrorDialogProps { orgLogin: string @@ -10,7 +21,11 @@ interface CreateMirrorDialogProps { forkParentName: string isOpen: boolean closeDialog: () => void - createMirror: (data: { repoName: string; branchName: string }) => void + createMirror: (data: { + repoName: string + branchName: string + settings: RepositorySettingsSchema + }) => void } export const CreateMirrorDialog = ({ @@ -23,6 +38,9 @@ export const CreateMirrorDialog = ({ }: CreateMirrorDialogProps) => { // set to default value of 'repository-name' for display purposes const [repoName, setRepoName] = useState('repository-name') + const { register, watch, getValues } = useForm({ + resolver: zodResolver(RepositorySettingsSchema), + }) if (!isOpen) { return null @@ -44,7 +62,11 @@ export const CreateMirrorDialog = ({ content: 'Confirm', variant: 'primary', onClick: () => { - createMirror({ repoName, branchName: repoName }) + createMirror({ + repoName, + branchName: repoName, + settings: getValues(), + }) setRepoName('repository-name') }, disabled: repoName === 'repository-name' || repoName === '', @@ -76,7 +98,7 @@ export const CreateMirrorDialog = ({ - + Mirror location + + Mirror Settings + + + + Enable Actions + + + Allowed Actions Settings + + + + ) diff --git a/src/server/repos/controller.ts b/src/server/repos/controller.ts index c9aba8b..67cf253 100644 --- a/src/server/repos/controller.ts +++ b/src/server/repos/controller.ts @@ -2,6 +2,7 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ /* eslint-disable @typescript-eslint/no-unsafe-member-access */ +import { TRPCError } from '@trpc/server' import simpleGit, { SimpleGitOptions } from 'simple-git' import { generateAuthUrl } from 'utils/auth' import { temporaryDirectory } from 'utils/dir' @@ -18,7 +19,6 @@ import { EditMirrorSchema, ListMirrorsSchema, } from './schema' -import { TRPCError } from '@trpc/server' const reposApiLogger = logger.getSubLogger({ name: 'repos-api' }) @@ -136,6 +136,31 @@ export const createMirrorHandler = async ({ }, }) + reposApiLogger.info('Created new repo', { newRepo }) + + // apply settings for actions + await privateOctokit.request( + 'PUT /repos/{owner}/{repo}/actions/permissions', + { + owner: privateOrg, + repo: input.newRepoName, + enabled: input.settings.actions.enabled, + allowed_actions: input.settings.actions.enabled + ? input.settings.actions.allowedActions + : undefined, + headers: { + 'X-GitHub-Api-Version': '2022-11-28', + }, + }, + ) + + reposApiLogger.debug('Applied settings for actions for repo', { + owner: privateOrg, + repo: input.newRepoName, + enabled: input.settings.actions.enabled, + allowed_actions: input.settings.actions.allowedActions, + }) + const defaultBranch = forkData.data.default_branch // Add the mirror remote diff --git a/src/server/repos/schema.ts b/src/server/repos/schema.ts index 676790a..14e26a8 100644 --- a/src/server/repos/schema.ts +++ b/src/server/repos/schema.ts @@ -1,5 +1,12 @@ import { z } from 'zod' +export const RepositorySettingsSchema = z.object({ + actions: z.object({ + enabled: z.boolean(), + allowedActions: z.enum(['all', 'local_only', 'selected']).optional(), + }), +}) + export const CreateMirrorSchema = z.object({ orgId: z.string(), forkRepoOwner: z.string(), @@ -7,6 +14,7 @@ export const CreateMirrorSchema = z.object({ forkId: z.string(), newRepoName: z.string().max(100), newBranchName: z.string(), + settings: RepositorySettingsSchema, }) export const ListMirrorsSchema = z.object({ @@ -29,3 +37,4 @@ export type CreateMirrorSchema = z.TypeOf export type ListMirrorsSchema = z.TypeOf export type EditMirrorSchema = z.TypeOf export type DeleteMirrorSchema = z.TypeOf +export type RepositorySettingsSchema = z.TypeOf diff --git a/test/octomock/index.ts b/test/octomock/index.ts index c61df3a..cc6cc2b 100644 --- a/test/octomock/index.ts +++ b/test/octomock/index.ts @@ -1,6 +1,7 @@ // This is taken from https://github.com/Chocrates/octomock let mockFunctions = { + request: jest.fn(), config: { get: jest.fn(), }, @@ -306,6 +307,7 @@ export class Octomock { mockFunctions: { rest: Record> config: Record + request: jest.Mock } defaultContext: { payload: { issue: { body: string; user: {} } } } diff --git a/test/server/repos.test.ts b/test/server/repos.test.ts index f8e3303..2f081cb 100644 --- a/test/server/repos.test.ts +++ b/test/server/repos.test.ts @@ -14,11 +14,11 @@ jest.mock('simple-git', () => { }) import * as config from '../../src/bot/config' -import * as auth from '../../src/utils/auth' import reposRouter from '../../src/server/repos/router' +import * as auth from '../../src/utils/auth' +import { t } from '../../src/utils/trpc-server' import { Octomock } from '../octomock' import { createTestContext } from '../utils/auth' -import { t } from '../../src/utils/trpc-server' const om = new Octomock() jest.mock('../../src/bot/config') @@ -127,6 +127,9 @@ describe('Repos router', () => { fakeOrgCustomProperties, ) om.mockFunctions.rest.repos.createInOrg.mockResolvedValue(fakeMirrorRepo) + om.mockFunctions.request.mockResolvedValue({ + status: 204, + }) const res = await caller.createMirror({ forkId: 'test', @@ -135,6 +138,11 @@ describe('Repos router', () => { forkRepoOwner: 'github', newBranchName: 'test', newRepoName: 'test', + settings: { + actions: { + enabled: false, + }, + }, }) // TODO: use real git operations and verify fs state after @@ -144,6 +152,19 @@ describe('Repos router', () => { expect(stubbedGit.addRemote).toHaveBeenCalledTimes(1) expect(stubbedGit.push).toHaveBeenCalledTimes(2) expect(stubbedGit.checkoutBranch).toHaveBeenCalledTimes(1) + expect(om.mockFunctions.request).toHaveBeenCalledTimes(1) + expect(om.mockFunctions.request).toHaveBeenCalledWith( + 'PUT /repos/{owner}/{repo}/actions/permissions', + { + owner: 'github-test', + repo: 'test', + enabled: false, + allowed_actions: undefined, + headers: { + 'X-GitHub-Api-Version': '2022-11-28', + }, + }, + ) expect(res).toEqual({ success: true, @@ -165,6 +186,9 @@ describe('Repos router', () => { om.mockFunctions.rest.orgs.get.mockResolvedValue(fakeOrg) om.mockFunctions.rest.repos.get.mockResolvedValue(fakeMirrorRepo) om.mockFunctions.rest.repos.delete.mockResolvedValue({}) + om.mockFunctions.request.mockResolvedValue({ + status: 204, + }) await caller .createMirror({ @@ -174,6 +198,11 @@ describe('Repos router', () => { forkRepoOwner: 'github', newBranchName: 'test', newRepoName: 'test', + settings: { + actions: { + enabled: false, + }, + }, }) .catch((error) => { expect(error.message).toEqual('Repo github-test/test already exists') @@ -183,6 +212,7 @@ describe('Repos router', () => { expect(om.mockFunctions.rest.repos.get).toHaveBeenCalledTimes(1) expect(om.mockFunctions.rest.repos.delete).toHaveBeenCalledTimes(0) expect(stubbedGit.clone).toHaveBeenCalledTimes(0) + expect(om.mockFunctions.request).toHaveBeenCalledTimes(0) }) it('should cleanup repos when there is an error', async () => { @@ -200,6 +230,9 @@ describe('Repos router', () => { om.mockFunctions.rest.repos.get.mockResolvedValueOnce(repoNotFound) om.mockFunctions.rest.repos.get.mockResolvedValueOnce(fakeMirrorRepo) om.mockFunctions.rest.repos.delete.mockResolvedValue({}) + om.mockFunctions.request.mockResolvedValue({ + status: 204, + }) stubbedGit.clone.mockRejectedValue(new Error('clone error')) @@ -211,6 +244,11 @@ describe('Repos router', () => { forkRepoOwner: 'github', newBranchName: 'test', newRepoName: 'test', + settings: { + actions: { + enabled: false, + }, + }, }) .catch((error) => { expect(error.message).toEqual('clone error') @@ -223,6 +261,7 @@ describe('Repos router', () => { repo: 'test', }) expect(stubbedGit.clone).toHaveBeenCalledTimes(1) + expect(om.mockFunctions.request).toHaveBeenCalledTimes(0) }) it('dual-org: should cleanup repos when there is an error', async () => { @@ -251,6 +290,11 @@ describe('Repos router', () => { forkRepoOwner: 'github', newBranchName: 'test', newRepoName: 'test', + settings: { + actions: { + enabled: false, + }, + }, }) .catch((error) => { expect(error.message).toEqual('clone error') @@ -263,6 +307,141 @@ describe('Repos router', () => { repo: 'test', }) expect(stubbedGit.clone).toHaveBeenCalledTimes(1) + expect(om.mockFunctions.request).toHaveBeenCalledTimes(0) + }) + + it('should change settings of a repo for actions', async () => { + const caller = t.createCallerFactory(reposRouter)(createTestContext()) + + const configSpy = jest.spyOn(config, 'getConfig').mockResolvedValue({ + publicOrg: 'github', + privateOrg: 'github-test', + }) + + om.mockFunctions.rest.apps.getOrgInstallation.mockResolvedValue( + fakeOrgInstallation, + ) + om.mockFunctions.rest.orgs.get.mockResolvedValue(fakeOrg) + om.mockFunctions.rest.repos.get.mockResolvedValueOnce(repoNotFound) + om.mockFunctions.rest.repos.get.mockResolvedValueOnce(fakeForkRepo) + om.mockFunctions.rest.orgs.getAllCustomProperties.mockResolvedValue( + fakeOrgCustomProperties, + ) + om.mockFunctions.rest.orgs.createOrUpdateCustomProperty.mockResolvedValue( + fakeOrgCustomProperties, + ) + om.mockFunctions.rest.repos.createInOrg.mockResolvedValue(fakeMirrorRepo) + om.mockFunctions.request.mockResolvedValue({ + status: 204, + }) + + const res = await caller.createMirror({ + forkId: 'test', + orgId: 'test', + forkRepoName: 'fork-test', + forkRepoOwner: 'github', + newBranchName: 'test', + newRepoName: 'test', + settings: { + actions: { + enabled: true, + allowedActions: 'all', + }, + }, + }) + + // TODO: use real git operations and verify fs state after + expect(configSpy).toHaveBeenCalledTimes(1) + expect(om.mockFunctions.rest.repos.get).toHaveBeenCalledTimes(2) + expect(stubbedGit.clone).toHaveBeenCalledTimes(1) + expect(stubbedGit.addRemote).toHaveBeenCalledTimes(1) + expect(stubbedGit.push).toHaveBeenCalledTimes(2) + expect(stubbedGit.checkoutBranch).toHaveBeenCalledTimes(1) + expect(om.mockFunctions.request).toHaveBeenCalledTimes(1) + expect(om.mockFunctions.request).toHaveBeenCalledWith( + 'PUT /repos/{owner}/{repo}/actions/permissions', + { + owner: 'github-test', + repo: 'test', + enabled: true, + allowed_actions: 'all', + headers: { + 'X-GitHub-Api-Version': '2022-11-28', + }, + }, + ) + + expect(res).toEqual({ + success: true, + data: fakeMirrorRepo.data, + }) + }) + + it('allowed_actions should be undefined when actions are not enabled', async () => { + const caller = t.createCallerFactory(reposRouter)(createTestContext()) + + const configSpy = jest.spyOn(config, 'getConfig').mockResolvedValue({ + publicOrg: 'github', + privateOrg: 'github-test', + }) + + om.mockFunctions.rest.apps.getOrgInstallation.mockResolvedValue( + fakeOrgInstallation, + ) + om.mockFunctions.rest.orgs.get.mockResolvedValue(fakeOrg) + om.mockFunctions.rest.repos.get.mockResolvedValueOnce(repoNotFound) + om.mockFunctions.rest.repos.get.mockResolvedValueOnce(fakeForkRepo) + om.mockFunctions.rest.orgs.getAllCustomProperties.mockResolvedValue( + fakeOrgCustomProperties, + ) + om.mockFunctions.rest.orgs.createOrUpdateCustomProperty.mockResolvedValue( + fakeOrgCustomProperties, + ) + om.mockFunctions.rest.repos.createInOrg.mockResolvedValue(fakeMirrorRepo) + om.mockFunctions.request.mockResolvedValue({ + status: 204, + }) + + const res = await caller.createMirror({ + forkId: 'test', + orgId: 'test', + forkRepoName: 'fork-test', + forkRepoOwner: 'github', + newBranchName: 'test', + newRepoName: 'test', + settings: { + actions: { + enabled: false, + allowedActions: 'all', + }, + }, + }) + + // TODO: use real git operations and verify fs state after + expect(configSpy).toHaveBeenCalledTimes(1) + expect(om.mockFunctions.rest.repos.get).toHaveBeenCalledTimes(2) + expect(stubbedGit.clone).toHaveBeenCalledTimes(1) + expect(stubbedGit.addRemote).toHaveBeenCalledTimes(1) + expect(stubbedGit.push).toHaveBeenCalledTimes(2) + expect(stubbedGit.checkoutBranch).toHaveBeenCalledTimes(1) + expect(om.mockFunctions.request).toHaveBeenCalledTimes(1) + expect(om.mockFunctions.request).toHaveBeenCalledWith( + 'PUT /repos/{owner}/{repo}/actions/permissions', + { + owner: 'github-test', + repo: 'test', + enabled: false, + allowed_actions: undefined, + headers: { + 'X-GitHub-Api-Version': '2022-11-28', + }, + }, + ) + + expect(res).toEqual({ + success: true, + data: fakeMirrorRepo.data, + }) }) it('reject repository names over the character limit', async () => { @@ -276,6 +455,11 @@ describe('Repos router', () => { forkRepoOwner: 'github', newBranchName: 'test', newRepoName: 'a'.repeat(101), + settings: { + actions: { + enabled: false, + }, + }, }) .catch((error) => { expect(error.message).toMatch(