From 0b8fa1dbeae2b07b8ada75d766ac005b52b80e39 Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Wed, 28 Feb 2024 20:09:41 -0500 Subject: [PATCH 1/6] feat: add session checking middleware to trpc --- .env.example | 5 +++++ env.mjs | 11 +++++++++++ src/pages/api/trpc/[trpc].ts | 3 ++- src/server/lib/auth.ts | 23 +++++++++++++++++++++++ src/server/trpc.ts | 19 +++++++++++++++++-- src/types/next-auth.d.ts | 9 +++++++++ 6 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 src/server/lib/auth.ts create mode 100644 src/types/next-auth.d.ts diff --git a/.env.example b/.env.example index 2e8af2d..0691136 100644 --- a/.env.example +++ b/.env.example @@ -9,6 +9,7 @@ PRIVATE_KEY=-----BEGIN RSA PRIVATE KEY-----\nYOUR KEY HERE WITH \n INCLUDED=\n-- # Auth configs NEXTAUTH_SECRET=bad-secret NEXTAUTH_URL=http://localhost:3000 +ALLOWED_HANDLES= # Go to https://smee.io/new set this to the URL that you are redirected to. WEBHOOK_PROXY_URL= @@ -19,3 +20,7 @@ LOG_LEVEL=debug # Used for settings various configuration in the app NODE_ENV=development + +# Used for GHEC configs +PUBLIC_ORG= +PRIVATE_ORG= diff --git a/env.mjs b/env.mjs index 4e1c85f..a0522c1 100644 --- a/env.mjs +++ b/env.mjs @@ -22,6 +22,16 @@ export const env = createEnv({ NODE_ENV: z.string().optional().default('development'), PUBLIC_ORG: z.string().optional(), PRIVATE_ORG: z.string().optional(), + // Custom validation for a comma separated list of strings + // ex: ajhenry,github,ahpook + ALLOWED_HANDLES: z + .string() + .optional() + .default('') + .refine((val) => { + if (val === '') return true + return val.split(',').every((handle) => handle.trim().length > 0) + }, 'Invalid comma separated list of GitHub handles'), }, /* * Environment variables available on the client (and server). @@ -48,6 +58,7 @@ export const env = createEnv({ NODE_ENV: process.env.NODE_ENV, PUBLIC_ORG: process.env.PUBLIC_ORG, PRIVATE_ORG: process.env.PRIVATE_ORG, + ALLOWED_HANDLES: process.env.ALLOWED_HANDLES, }, skipValidation: process.env.SKIP_ENV_VALIDATIONS === 'true', }) diff --git a/src/pages/api/trpc/[trpc].ts b/src/pages/api/trpc/[trpc].ts index c8634c9..c630e92 100644 --- a/src/pages/api/trpc/[trpc].ts +++ b/src/pages/api/trpc/[trpc].ts @@ -1,9 +1,10 @@ import * as trpcNext from '@trpc/server/adapters/next' +import { createContext } from 'server/trpc' import { appRouter } from '../../../server/routers/_app' // export API handler // @see https://trpc.io/docs/server/adapters export default trpcNext.createNextApiHandler({ router: appRouter, - createContext: () => ({}), + createContext, }) diff --git a/src/server/lib/auth.ts b/src/server/lib/auth.ts new file mode 100644 index 0000000..f1c58e8 --- /dev/null +++ b/src/server/lib/auth.ts @@ -0,0 +1,23 @@ +import { TRPCError } from '@trpc/server' +import { personalOctokit } from 'bot/octokit' +import { Middleware } from 'server/trpc' + +export const verifyAuth: Middleware = async (opts) => { + const { ctx } = opts + + if (!ctx.session?.user?.accessToken) { + throw new TRPCError({ code: 'UNAUTHORIZED' }) + } + + // Check validity of token + const octokit = personalOctokit(ctx.session.user.accessToken) + const user = await octokit.rest.users.getAuthenticated() + + if (!user) { + throw new TRPCError({ code: 'UNAUTHORIZED' }) + } + + return opts.next({ + ctx, + }) +} diff --git a/src/server/trpc.ts b/src/server/trpc.ts index ed6a764..96e676d 100644 --- a/src/server/trpc.ts +++ b/src/server/trpc.ts @@ -3,14 +3,28 @@ import { createTRPCNext } from '@trpc/next' import type { AppRouter } from './routers/_app' import { initTRPC } from '@trpc/server' +import { CreateNextContextOptions } from '@trpc/server/adapters/next' +import { getServerSession } from 'next-auth' +import { nextAuthOptions } from 'pages/api/auth/[...nextauth]' +import { verifyAuth } from './lib/auth' + +export const createContext = async (opts: CreateNextContextOptions) => { + const session = await getServerSession(opts.req, opts.res, nextAuthOptions) + + return { + session, + } +} + // Avoid exporting the entire t-object // since it's not very descriptive. // For instance, the use of a t variable // is common in i18n libraries. -const t = initTRPC.create() +const t = initTRPC.context().create() // Base router and procedure helpers export const router = t.router -export const procedure = t.procedure +export type Middleware = Parameters<(typeof t.procedure)['use']>[0] +export const procedure = t.procedure.use(verifyAuth) function getBaseUrl() { if (typeof window !== 'undefined') @@ -25,6 +39,7 @@ function getBaseUrl() { // assume localhost return `http://localhost:${process.env.PORT ?? 3000}` } + export const trpc = createTRPCNext({ config(_) { return { diff --git a/src/types/next-auth.d.ts b/src/types/next-auth.d.ts new file mode 100644 index 0000000..90c1a6d --- /dev/null +++ b/src/types/next-auth.d.ts @@ -0,0 +1,9 @@ +import { DefaultSession } from 'next-auth' + +declare module 'next-auth' { + interface Session { + user: { + accessToken: string + } & DefaultSession['user'] + } +} From 203b304a4a0793fac1912a3e1d2fa99586ac35bb Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Fri, 1 Mar 2024 16:23:08 -0500 Subject: [PATCH 2/6] feat: add auth handling middleware to routes - adds a new health endpoint - adds auth middleware and github token checking - adds tests for auth --- src/server/lib/auth.ts | 13 ++------ src/server/middleware/auth.ts | 13 ++++++++ src/server/routers/_app.ts | 2 ++ src/server/routers/health.ts | 9 +++++ src/server/trpc.ts | 2 +- src/types/next-auth.d.ts | 2 +- test/octomock/index.ts | 3 ++ test/server/auth.test.ts | 63 +++++++++++++++++++++++++++++++++++ test/server/git.test.ts | 11 ++++-- test/utils/auth.ts | 18 ++++++++++ 10 files changed, 121 insertions(+), 15 deletions(-) create mode 100644 src/server/middleware/auth.ts create mode 100644 src/server/routers/health.ts create mode 100644 test/server/auth.test.ts create mode 100644 test/utils/auth.ts diff --git a/src/server/lib/auth.ts b/src/server/lib/auth.ts index f1c58e8..d990f66 100644 --- a/src/server/lib/auth.ts +++ b/src/server/lib/auth.ts @@ -1,23 +1,16 @@ import { TRPCError } from '@trpc/server' import { personalOctokit } from 'bot/octokit' -import { Middleware } from 'server/trpc' -export const verifyAuth: Middleware = async (opts) => { - const { ctx } = opts - - if (!ctx.session?.user?.accessToken) { +export const checkGitHubAuth = async (accessToken: string | undefined) => { + if (!accessToken) { throw new TRPCError({ code: 'UNAUTHORIZED' }) } // Check validity of token - const octokit = personalOctokit(ctx.session.user.accessToken) + const octokit = personalOctokit(accessToken) const user = await octokit.rest.users.getAuthenticated() if (!user) { throw new TRPCError({ code: 'UNAUTHORIZED' }) } - - return opts.next({ - ctx, - }) } diff --git a/src/server/middleware/auth.ts b/src/server/middleware/auth.ts new file mode 100644 index 0000000..bef8e2f --- /dev/null +++ b/src/server/middleware/auth.ts @@ -0,0 +1,13 @@ +import { checkGitHubAuth } from 'server/lib/auth' +import { Middleware } from 'server/trpc' + +export const verifyAuth: Middleware = async (opts) => { + const { ctx } = opts + + // Check validity of token + checkGitHubAuth(ctx.session?.user?.accessToken) + + return opts.next({ + ctx, + }) +} diff --git a/src/server/routers/_app.ts b/src/server/routers/_app.ts index 57cad6f..9d90665 100644 --- a/src/server/routers/_app.ts +++ b/src/server/routers/_app.ts @@ -1,5 +1,6 @@ import { router } from '../trpc' import { gitRouter } from './git' +import { healthRouter } from './health' import { octokitRouter } from './octokit' import { reposRouter } from './repos' @@ -7,6 +8,7 @@ export const appRouter = router({ git: gitRouter, octokit: octokitRouter, repos: reposRouter, + health: healthRouter, }) // export type definition of API diff --git a/src/server/routers/health.ts b/src/server/routers/health.ts new file mode 100644 index 0000000..ddcd650 --- /dev/null +++ b/src/server/routers/health.ts @@ -0,0 +1,9 @@ +// Simple health check endpoint +import { procedure, router } from '../trpc' + +export const healthRouter = router({ + // Queries + ping: procedure.query(async () => { + return 'pong' + }), +}) diff --git a/src/server/trpc.ts b/src/server/trpc.ts index 96e676d..a4680f8 100644 --- a/src/server/trpc.ts +++ b/src/server/trpc.ts @@ -6,7 +6,7 @@ import { initTRPC } from '@trpc/server' import { CreateNextContextOptions } from '@trpc/server/adapters/next' import { getServerSession } from 'next-auth' import { nextAuthOptions } from 'pages/api/auth/[...nextauth]' -import { verifyAuth } from './lib/auth' +import { verifyAuth } from './middleware/auth' export const createContext = async (opts: CreateNextContextOptions) => { const session = await getServerSession(opts.req, opts.res, nextAuthOptions) diff --git a/src/types/next-auth.d.ts b/src/types/next-auth.d.ts index 90c1a6d..528d2bc 100644 --- a/src/types/next-auth.d.ts +++ b/src/types/next-auth.d.ts @@ -3,7 +3,7 @@ import { DefaultSession } from 'next-auth' declare module 'next-auth' { interface Session { user: { - accessToken: string + accessToken: string | undefined } & DefaultSession['user'] } } diff --git a/test/octomock/index.ts b/test/octomock/index.ts index 0a1f764..10af856 100644 --- a/test/octomock/index.ts +++ b/test/octomock/index.ts @@ -293,6 +293,9 @@ let mockFunctions = { saveState: jest.fn(), getState: jest.fn(), }, + users: { + getAuthenticated: jest.fn(), + }, }, } diff --git a/test/server/auth.test.ts b/test/server/auth.test.ts new file mode 100644 index 0000000..f684942 --- /dev/null +++ b/test/server/auth.test.ts @@ -0,0 +1,63 @@ +import { healthRouter } from '../../src/server/routers/health' +import { Octomock } from '../octomock' +import { createTestContext } from '../utils/auth' +const om = new Octomock() + +jest.mock('../../src/bot/octokit', () => ({ + personalOctokit: () => om.getOctokitImplementation(), +})) + +describe('Git router', () => { + beforeEach(() => { + om.resetMocks() + jest.resetAllMocks() + }) + + it('should allow users that are authenticated', async () => { + const caller = healthRouter.createCaller(createTestContext()) + + om.mockFunctions.rest.users.getAuthenticated.mockResolvedValue({ + status: 200, + data: { + login: 'test-user', + }, + }) + + const res = await caller.ping() + + expect(res).toEqual('pong') + + expect(om.mockFunctions.rest.users.getAuthenticated).toHaveBeenCalledTimes( + 1, + ) + }) + + it('should throw on invalid sessions', async () => { + const caller = healthRouter.createCaller( + createTestContext({ + user: { + name: 'fake-username', + email: 'fake-email', + image: 'fake-image', + accessToken: 'bad-token', + }, + expires: new Date('2030-01-01').toISOString(), + }), + ) + + om.mockFunctions.rest.users.getAuthenticated.mockResolvedValue({ + status: 401, + data: { + message: 'Bad credentials', + }, + }) + + await caller.ping().catch((error) => { + expect(error.code).toContain('UNAUTHORIZED') + }) + + expect(om.mockFunctions.rest.users.getAuthenticated).toHaveBeenCalledTimes( + 1, + ) + }) +}) diff --git a/test/server/git.test.ts b/test/server/git.test.ts index 9563ec1..2175e68 100644 --- a/test/server/git.test.ts +++ b/test/server/git.test.ts @@ -14,8 +14,10 @@ jest.mock('simple-git', () => { }) import * as config from '../../src/bot/config' +import * as auth from '../../src/server/lib/auth' import { gitRouter } from '../../src/server/routers/git' import { Octomock } from '../octomock' +import { createTestContext } from '../utils/auth' const om = new Octomock() jest.mock('../../src/bot/config') @@ -24,6 +26,7 @@ jest.mock('../../src/bot/octokit', () => ({ appOctokit: () => om.getOctokitImplementation(), installationOctokit: () => om.getOctokitImplementation(), })) +jest.mock('../../src/server/lib/auth') const fakeForkRepo = { status: 200, @@ -70,6 +73,8 @@ const repoNotFound = { }, } +jest.spyOn(auth, 'checkGitHubAuth').mockResolvedValue() + describe('Git router', () => { beforeEach(() => { om.resetMocks() @@ -77,7 +82,7 @@ describe('Git router', () => { }) it('should create a mirror when repo does not exist exist', async () => { - const caller = gitRouter.createCaller({}) + const caller = gitRouter.createCaller(createTestContext()) const configSpy = jest.spyOn(config, 'getConfig').mockResolvedValue({ publicOrg: 'github', @@ -116,7 +121,7 @@ describe('Git router', () => { }) it('should throw an error when repo already exists', async () => { - const caller = gitRouter.createCaller({}) + const caller = gitRouter.createCaller(createTestContext()) const configSpy = jest.spyOn(config, 'getConfig').mockResolvedValue({ publicOrg: 'github', @@ -150,7 +155,7 @@ describe('Git router', () => { }) it('should cleanup repos when there is an error', async () => { - const caller = gitRouter.createCaller({}) + const caller = gitRouter.createCaller(createTestContext()) const configSpy = jest.spyOn(config, 'getConfig').mockResolvedValue({ publicOrg: 'github', diff --git a/test/utils/auth.ts b/test/utils/auth.ts new file mode 100644 index 0000000..5079414 --- /dev/null +++ b/test/utils/auth.ts @@ -0,0 +1,18 @@ +import { Session } from 'next-auth' +import { createContext } from '../../src/server/trpc' + +export const createTestContext = ( + session?: Session, +): Awaited> => { + return { + session: session ?? { + user: { + name: 'fake-username', + email: 'fake-email', + image: 'fake-image', + accessToken: 'fake-access-token', + }, + expires: new Date('2030-01-01').toISOString(), + }, + } +} From 4fe843aba4f7ac9f89c79043ac143c1f6f145a16 Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Fri, 1 Mar 2024 16:35:43 -0500 Subject: [PATCH 3/6] feat: add ALLOWED_HANDLES to sign in check --- src/pages/api/auth/[...nextauth].ts | 30 ++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/src/pages/api/auth/[...nextauth].ts b/src/pages/api/auth/[...nextauth].ts index bfa5a5a..832a67e 100644 --- a/src/pages/api/auth/[...nextauth].ts +++ b/src/pages/api/auth/[...nextauth].ts @@ -1,5 +1,5 @@ import { personalOctokit } from 'bot/octokit' -import NextAuth, { AuthOptions } from 'next-auth' +import NextAuth, { AuthOptions, Profile } from 'next-auth' import GitHub from 'next-auth/providers/github' import { logger } from '../../../utils/logger' @@ -52,6 +52,34 @@ export const nextAuthOptions: AuthOptions = { }, }, callbacks: { + signIn: async (params) => { + const profile = params.profile as Profile & { login?: string } + authLogger.debug('Sign in callback') + const allowedHandles = ( + process.env.ALLOWED_HANDLES?.split(',') ?? [] + ).filter((handle) => handle !== '') + + if (allowedHandles.length === 0) { + authLogger.debug( + 'No allowed handles specified via ALLOWED_HANDLES, allowing all users.', + ) + return true + } + + if (!profile?.login) { + return false + } + + if (allowedHandles.includes(profile.login)) { + return true + } + + authLogger.warn( + `User "${profile.login}" is not in the allowed handles list`, + ) + + return false + }, session: async ({ session, token }) => { authLogger.debug('Session callback') From 6bc03530402242cba83dc9d58d7b1a9a106f7077 Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Fri, 1 Mar 2024 16:37:13 -0500 Subject: [PATCH 4/6] chore: add logging to sign in --- src/pages/api/auth/[...nextauth].ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pages/api/auth/[...nextauth].ts b/src/pages/api/auth/[...nextauth].ts index 832a67e..0aab148 100644 --- a/src/pages/api/auth/[...nextauth].ts +++ b/src/pages/api/auth/[...nextauth].ts @@ -70,6 +70,8 @@ export const nextAuthOptions: AuthOptions = { return false } + authLogger.debug('Trying to sign in with handle:', profile.login) + if (allowedHandles.includes(profile.login)) { return true } From 22230e065b04f417c4ff7209469a4af0f4b4cd09 Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Fri, 1 Mar 2024 16:54:49 -0500 Subject: [PATCH 5/6] fix: add better github auth checks to middleware --- src/server/lib/auth.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/server/lib/auth.ts b/src/server/lib/auth.ts index d990f66..1234f63 100644 --- a/src/server/lib/auth.ts +++ b/src/server/lib/auth.ts @@ -1,16 +1,25 @@ import { TRPCError } from '@trpc/server' import { personalOctokit } from 'bot/octokit' +import { logger } from 'utils/logger' + +const middlewareLogger = logger.getSubLogger({ name: 'middleware' }) export const checkGitHubAuth = async (accessToken: string | undefined) => { if (!accessToken) { + middlewareLogger.error('No access token provided') throw new TRPCError({ code: 'UNAUTHORIZED' }) } // Check validity of token const octokit = personalOctokit(accessToken) - const user = await octokit.rest.users.getAuthenticated() - - if (!user) { + try { + const user = await octokit.rest.users.getAuthenticated() + if (!user) { + middlewareLogger.error('No user found') + throw new TRPCError({ code: 'UNAUTHORIZED' }) + } + } catch (error) { + middlewareLogger.error('Error checking github auth', error) throw new TRPCError({ code: 'UNAUTHORIZED' }) } } From 9886aeac7e4794be03c5ef7a5056c6c709f53ca6 Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Fri, 1 Mar 2024 16:55:51 -0500 Subject: [PATCH 6/6] chore: various cleanup --- src/pages/api/auth/[...nextauth].ts | 5 +++-- src/server/middleware/auth.ts | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/pages/api/auth/[...nextauth].ts b/src/pages/api/auth/[...nextauth].ts index 0aab148..ca30766 100644 --- a/src/pages/api/auth/[...nextauth].ts +++ b/src/pages/api/auth/[...nextauth].ts @@ -53,14 +53,15 @@ export const nextAuthOptions: AuthOptions = { }, callbacks: { signIn: async (params) => { - const profile = params.profile as Profile & { login?: string } authLogger.debug('Sign in callback') + + const profile = params.profile as Profile & { login?: string } const allowedHandles = ( process.env.ALLOWED_HANDLES?.split(',') ?? [] ).filter((handle) => handle !== '') if (allowedHandles.length === 0) { - authLogger.debug( + authLogger.info( 'No allowed handles specified via ALLOWED_HANDLES, allowing all users.', ) return true diff --git a/src/server/middleware/auth.ts b/src/server/middleware/auth.ts index bef8e2f..3d1695a 100644 --- a/src/server/middleware/auth.ts +++ b/src/server/middleware/auth.ts @@ -4,7 +4,7 @@ import { Middleware } from 'server/trpc' export const verifyAuth: Middleware = async (opts) => { const { ctx } = opts - // Check validity of token + // Verify valid github session checkGitHubAuth(ctx.session?.user?.accessToken) return opts.next({