From c44cd034d97e072d422a63fe359f0aac9cdc4855 Mon Sep 17 00:00:00 2001 From: Nathan Lie Date: Tue, 10 Dec 2024 14:21:01 -0800 Subject: [PATCH] fix: always expect tenant id in request --- packages/backend/src/shared/utils.test.ts | 157 +++++++++++++++------- packages/backend/src/shared/utils.ts | 23 +--- 2 files changed, 118 insertions(+), 62 deletions(-) diff --git a/packages/backend/src/shared/utils.test.ts b/packages/backend/src/shared/utils.test.ts index 5540b4a178..92171efe8c 100644 --- a/packages/backend/src/shared/utils.test.ts +++ b/packages/backend/src/shared/utils.test.ts @@ -2,6 +2,7 @@ import crypto from 'crypto' import { IocContract } from '@adonisjs/fold' import { Redis } from 'ioredis' import { faker } from '@faker-js/faker' +import { v4 } from 'uuid' import { isValidHttpUrl, poll, @@ -273,10 +274,11 @@ describe('utils', (): void => { let deps: IocContract let appContainer: TestContainer let tenant: Tenant + let operator: Tenant let config: IAppConfig let redis: Redis - const operatorApiSecret = 'test-operator-secret' + const operatorApiSecret = crypto.randomBytes(8).toString('base64') beforeAll(async (): Promise => { deps = initIocContainer({ @@ -296,6 +298,14 @@ describe('utils', (): void => { idpConsentUrl: faker.internet.url(), idpSecret: 'test-idp-secret' }) + + operator = await Tenant.query(appContainer.knex).insertAndFetch({ + email: faker.internet.email(), + publicName: faker.company.name(), + apiSecret: operatorApiSecret, + idpConsentUrl: faker.internet.url(), + idpSecret: 'test-idp-secret' + }) }) afterEach(async (): Promise => { @@ -308,18 +318,17 @@ describe('utils', (): void => { }) test.each` - tenanted | isOperator | description - ${true} | ${false} | ${'tenanted non-operator'} - ${true} | ${true} | ${'tenanted operator'} - ${false} | ${true} | ${'non-tenanted operator'} + isOperator | description + ${false} | ${'tenanted non-operator'} + ${true} | ${'tenanted operator'} `( 'returns true if $description request has valid signature', - async ({ tenanted, isOperator }): Promise => { + async ({ isOperator }): Promise => { const requestBody = { test: 'value' } const signature = isOperator ? generateApiSignature( - config.adminApiSecret as string, + operator.apiSecret, Config.adminApiSignatureVersion, requestBody ) @@ -334,7 +343,7 @@ describe('utils', (): void => { headers: { Accept: 'application/json', signature, - tenantId: tenanted ? tenant.id : undefined + 'tenant-id': isOperator ? operator.id : tenant.id }, url: '/graphql' }, @@ -343,14 +352,25 @@ describe('utils', (): void => { ) ctx.request.body = requestBody + if (isOperator) { + console.log( + 'tenant secret=', + operator.apiSecret, + 'config secret=', + config.adminApiSecret + ) + } else { + console.log( + 'tenant secret=', + tenant.apiSecret, + 'config secret=', + config.adminApiSecret + ) + } const result = await verifyTenantOrOperatorApiSignature(ctx, config) expect(result).toEqual(true) - if (tenanted) { - expect(ctx.tenant).toEqual(tenant) - } else { - expect(ctx.tenant).toBeUndefined() - } + expect(ctx.tenant).toEqual(isOperator ? operator : tenant) if (isOperator) { expect(ctx.isOperator).toEqual(true) @@ -360,39 +380,86 @@ describe('utils', (): void => { } ) - test.each` - failurePoint - ${'tenant'} - ${'operator'} - `( - "returns false when $failurePoint signature isn't valid", - async ({ failurePoint }): Promise => { - const tenantedRequest = failurePoint === 'tenant' - const requestBody = { test: 'value' } - const signature = generateApiSignature( - 'wrongsecret', - Config.adminApiSignatureVersion, - requestBody - ) - const ctx = createContext( - { - headers: { - Accept: 'application/json', - signature, - tenantId: tenantedRequest ? tenant.id : undefined - }, - url: '/graphql' + test("returns false when signature isn't signed with tenant secret", async (): Promise => { + const requestBody = { test: 'value' } + const signature = generateApiSignature( + 'wrongsecret', + Config.adminApiSignatureVersion, + requestBody + ) + const ctx = createContext( + { + headers: { + Accept: 'application/json', + signature, + 'tenant-id': tenant.id }, - {}, - appContainer.container - ) - ctx.request.body = requestBody + url: '/graphql' + }, + {}, + appContainer.container + ) + ctx.request.body = requestBody - const result = await verifyTenantOrOperatorApiSignature(ctx, config) - expect(result).toEqual(false) - expect(ctx.tenant).toBeUndefined() - expect(ctx.isOperator).toEqual(false) - } - ) + const result = await verifyTenantOrOperatorApiSignature(ctx, config) + expect(result).toEqual(false) + expect(ctx.tenant).toBeUndefined() + expect(ctx.isOperator).toEqual(false) + }) + + test('returns false if tenant id is not included', async (): Promise => { + const requestBody = { test: 'value' } + const signature = generateApiSignature( + tenant.apiSecret, + Config.adminApiSignatureVersion, + requestBody + ) + const ctx = createContext( + { + headers: { + Accept: 'application/json', + signature + }, + url: '/graphql' + }, + {}, + appContainer.container + ) + + ctx.request.body = requestBody + + const result = await verifyTenantOrOperatorApiSignature(ctx, config) + expect(result).toEqual(false) + expect(ctx.tenant).toBeUndefined() + expect(ctx.isOperator).toEqual(false) + }) + + test('returns false if tenant does not exist', async (): Promise => { + const requestBody = { test: 'value' } + const signature = generateApiSignature( + tenant.apiSecret, + Config.adminApiSignatureVersion, + requestBody + ) + const ctx = createContext( + { + headers: { + Accept: 'application/json', + signature, + 'tenant-id': v4() + }, + url: '/graphql' + }, + {}, + appContainer.container + ) + + ctx.request.body = requestBody + + const result = await verifyTenantOrOperatorApiSignature(ctx, config) + expect(result).toEqual(false) + expect(ctx.tenant).toBeUndefined() + expect(ctx.isOperator).toEqual(false) + }) }) }) diff --git a/packages/backend/src/shared/utils.ts b/packages/backend/src/shared/utils.ts index 62132c5116..cc63458130 100644 --- a/packages/backend/src/shared/utils.ts +++ b/packages/backend/src/shared/utils.ts @@ -177,7 +177,7 @@ async function canApiSignatureBeProcessed( associated with a tenant id in the headers, then with the configured admin secret. If a tenant secret can replicate the signature, the request is tenanted to that particular tenant. - If the environment admin secret replicates the signature, then it is an operator request with elevated permissions. + If the environment admin secret matches the tenant's secret, then it is an operator request with elevated permissions. If neither can replicate the signature then it is unauthorized. */ export async function verifyTenantOrOperatorApiSignature( @@ -193,15 +193,17 @@ export async function verifyTenantOrOperatorApiSignature( } const tenantService = await ctx.container.use('tenantService') - const tenantId = headers['tenantid'] + const tenantId = headers['tenant-id'] const tenant = tenantId ? await tenantService.get(tenantId) : undefined + if (!tenant) return false + if (!(await canApiSignatureBeProcessed(signature as string, ctx, config))) return false // First, try validating with the tenant api secret if ( - tenant?.apiSecret && + tenant.apiSecret && verifyApiSignatureDigest( signature as string, ctx.request, @@ -210,20 +212,7 @@ export async function verifyTenantOrOperatorApiSignature( ) ) { ctx.tenant = tenant - return true - } - - // Fall back on validating with operator api secret if prior validation fails - if ( - verifyApiSignatureDigest( - signature as string, - ctx.request, - config, - config.adminApiSecret as string - ) - ) { - ctx.tenant = tenant - ctx.isOperator = true + ctx.isOperator = tenant.apiSecret === config.adminApiSecret return true }