From c289f260264528988ba1bfa407e9676566a31633 Mon Sep 17 00:00:00 2001 From: Nestor Carvantes Date: Wed, 21 Jul 2021 03:51:19 -0500 Subject: [PATCH 1/2] test: add multi-tenancy integ tests --- integration-tests/bulkExport.test.ts | 6 + integration-tests/multitenancy.test.ts | 147 ++++++++++++++++++++++ integration-tests/rbac-permission.test.ts | 8 +- integration-tests/utils.ts | 112 +++++++++++++---- package.json | 4 +- yarn.lock | 7 ++ 6 files changed, 255 insertions(+), 29 deletions(-) create mode 100644 integration-tests/multitenancy.test.ts diff --git a/integration-tests/bulkExport.test.ts b/integration-tests/bulkExport.test.ts index aa3e0002..556f625d 100644 --- a/integration-tests/bulkExport.test.ts +++ b/integration-tests/bulkExport.test.ts @@ -8,6 +8,10 @@ import { getFhirClient } from './utils'; const FIVE_MINUTES_IN_MS = 5 * 60 * 1000; jest.setTimeout(FIVE_MINUTES_IN_MS); +const sleep = async (milliseconds: number) => { + return new Promise(resolve => setTimeout(resolve, milliseconds)); +}; + describe('Bulk Export', () => { let bulkExportTestHelper: BulkExportTestHelper; @@ -21,6 +25,8 @@ describe('Bulk Export', () => { // BUILD const oldCreatedResourceBundleResponse = await bulkExportTestHelper.sendCreateResourcesRequest(); const resTypToResNotExpectedInExport = bulkExportTestHelper.getResources(oldCreatedResourceBundleResponse); + // sleep 30 seconds to make tests more resilient to clock skew when running locally. + await sleep(30_000); const currentTime = new Date(); const newCreatedResourceBundleResponse = await bulkExportTestHelper.sendCreateResourcesRequest(); const resTypToResExpectedInExport = bulkExportTestHelper.getResources(newCreatedResourceBundleResponse); diff --git a/integration-tests/multitenancy.test.ts b/integration-tests/multitenancy.test.ts new file mode 100644 index 00000000..b89930a0 --- /dev/null +++ b/integration-tests/multitenancy.test.ts @@ -0,0 +1,147 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 * + */ + +import { AxiosInstance } from 'axios'; +import { + expectResourceToNotBePartOfSearchResults, + getFhirClient, + randomPatient, + waitForResourceToBeSearchable, +} from './utils'; +import BulkExportTestHelper from './bulkExportTestHelper'; + +jest.setTimeout(300_000); + +test('empty test placeholder', () => { + // empty test to avoid the "Your test suite must contain at least one test." error +}); + +if (process.env.MULTI_TENANCY_ENABLED === 'true') { + describe('tenant data isolation', () => { + let client: AxiosInstance; + let clientForAnotherTenant: AxiosInstance; + beforeAll(async () => { + client = await getFhirClient({ tenant: 'tenant1' }); + clientForAnotherTenant = await getFhirClient({ tenant: 'tenant2' }); + }); + + test('tenant cannot READ resources from another tenant', async () => { + const testPatient: ReturnType = (await client.post('Patient', randomPatient())).data; + + await expect(clientForAnotherTenant.get(`Patient/${testPatient.id}`)).rejects.toMatchObject({ + response: { status: 404 }, + }); + }); + + test('tenant cannot UPDATE resources from another tenant', async () => { + const testPatient: ReturnType = (await client.post('Patient', randomPatient())).data; + + await expect(clientForAnotherTenant.put(`Patient/${testPatient.id}`, testPatient)).rejects.toMatchObject({ + response: { status: 404 }, + }); + }); + + test('tenant cannot DELETE resources from another tenant', async () => { + const testPatient: ReturnType = (await client.post('Patient', randomPatient())).data; + + await expect(clientForAnotherTenant.delete(`Patient/${testPatient.id}`)).rejects.toMatchObject({ + response: { status: 404 }, + }); + }); + + test('tenant cannot SEARCH resources from another tenant', async () => { + const testPatient: ReturnType = (await client.post('Patient', randomPatient())).data; + + await waitForResourceToBeSearchable(client, testPatient); + + await expectResourceToNotBePartOfSearchResults( + clientForAnotherTenant, + { url: 'Patient', params: { _id: testPatient.id } }, + testPatient, + ); + }); + + test('tenant cannot EXPORT resources from another tenant', async () => { + const testPatient: ReturnType = (await client.post('Patient', randomPatient())).data; + const bulkExportTestHelper = new BulkExportTestHelper(clientForAnotherTenant); + + const testPatientFromAnotherTenant: ReturnType = ( + await clientForAnotherTenant.post('Patient', randomPatient()) + ).data; + + const statusPollUrl = await bulkExportTestHelper.startExportJob({ + since: new Date(Date.now() - 600_000), + }); + const responseBody = await bulkExportTestHelper.getExportStatus(statusPollUrl); + + const expectedResources = { Patient: testPatientFromAnotherTenant }; + const notExpectedResources = { Patient: testPatient }; + + return bulkExportTestHelper.checkResourceInExportedFiles( + responseBody.output, + expectedResources, + notExpectedResources, + ); + }); + }); + + describe('routing', () => { + let client: AxiosInstance; + beforeAll(async () => { + client = await getFhirClient({ tenant: 'tenant1' }); + }); + test('requests without /tenant/ in path should fail', async () => { + await expect(client.get(`${process.env.API_URL}/Patient`)).rejects.toMatchObject({ + response: { status: 404 }, + }); + + await expect(client.get(`${process.env.API_URL}/Patient/123`)).rejects.toMatchObject({ + response: { status: 404 }, + }); + + await expect(client.post(`${process.env.API_URL}/Patient/123`)).rejects.toMatchObject({ + response: { status: 404 }, + }); + + await expect(client.put(`${process.env.API_URL}/Patient/123`)).rejects.toMatchObject({ + response: { status: 404 }, + }); + + await expect(client.delete(`${process.env.API_URL}/Patient/123`)).rejects.toMatchObject({ + response: { status: 404 }, + }); + }); + + test('requests with tenantId in path different from the tenantId in access token should fail', async () => { + await expect(client.get(`${process.env.API_URL}/tenant/anotherTenantId/Patient`)).rejects.toMatchObject({ + response: { status: 401 }, + }); + + await expect(client.get(`${process.env.API_URL}/tenant/anotherTenantId/Patient/123`)).rejects.toMatchObject( + { + response: { status: 401 }, + }, + ); + + await expect( + client.post(`${process.env.API_URL}/tenant/anotherTenantId/Patient/123`), + ).rejects.toMatchObject({ + response: { status: 401 }, + }); + + await expect(client.put(`${process.env.API_URL}/tenant/anotherTenantId/Patient/123`)).rejects.toMatchObject( + { + response: { status: 401 }, + }, + ); + + await expect( + client.delete(`${process.env.API_URL}/tenant/anotherTenantId/Patient/123`), + ).rejects.toMatchObject({ + response: { status: 401 }, + }); + }); + }); +} diff --git a/integration-tests/rbac-permission.test.ts b/integration-tests/rbac-permission.test.ts index 34c802db..44212b97 100644 --- a/integration-tests/rbac-permission.test.ts +++ b/integration-tests/rbac-permission.test.ts @@ -3,7 +3,7 @@ import { getFhirClient, randomPatient } from './utils'; jest.setTimeout(60 * 1000); test('practitioner role can create new patient', async () => { - const client = await getFhirClient('practitioner'); + const client = await getFhirClient({ role: 'practitioner' }); const patientRecord: any = randomPatient(); delete patientRecord.id; await expect(client.post('Patient', patientRecord)).resolves.toMatchObject({ @@ -14,16 +14,16 @@ test('practitioner role can create new patient', async () => { describe('Negative tests', () => { test('invalid token', async () => { - const client = await getFhirClient('practitioner', 'Invalid token'); + const client = await getFhirClient({ role: 'practitioner', providedAccessToken: 'Invalid token' }); await expect(client.post('Patient', randomPatient())).rejects.toMatchObject({ response: { status: 401 }, }); }); test('auditor role cannot create new patient record', async () => { - const client = await getFhirClient('auditor'); + const client = await getFhirClient({ role: 'auditor' }); await expect(client.post('Patient', randomPatient())).rejects.toMatchObject({ - response: { status: 403 }, + response: { status: 401 }, }); }); }); diff --git a/integration-tests/utils.ts b/integration-tests/utils.ts index f1d4e34c..83bb952d 100644 --- a/integration-tests/utils.ts +++ b/integration-tests/utils.ts @@ -7,21 +7,76 @@ import * as AWS from 'aws-sdk'; import axios, { AxiosInstance } from 'axios'; import { Chance } from 'chance'; import qs from 'qs'; +import { decode } from 'jsonwebtoken'; import waitForExpect from 'wait-for-expect'; -export const getFhirClient = async ( - role: 'auditor' | 'practitioner' = 'practitioner', - providedAccessToken?: string, +const DEFAULT_TENANT_ID = 'tenant1'; + +const getAuthParameters: (role: string, tenantId: string) => { PASSWORD: string; USERNAME: string } = ( + role: string, + tenantId: string, ) => { const { - API_URL, - API_KEY, - API_AWS_REGION, COGNITO_USERNAME_PRACTITIONER, COGNITO_USERNAME_AUDITOR, COGNITO_PASSWORD, - COGNITO_CLIENT_ID, + COGNITO_USERNAME_PRACTITIONER_ANOTHER_TENANT, + MULTI_TENANCY_ENABLED, } = process.env; + + if (COGNITO_USERNAME_PRACTITIONER === undefined) { + throw new Error('COGNITO_USERNAME_PRACTITIONER environment variable is not defined'); + } + if (COGNITO_USERNAME_AUDITOR === undefined) { + throw new Error('COGNITO_USERNAME_AUDITOR environment variable is not defined'); + } + if (COGNITO_PASSWORD === undefined) { + throw new Error('COGNITO_PASSWORD environment variable is not defined'); + } + + if (MULTI_TENANCY_ENABLED === 'true') { + if (COGNITO_USERNAME_PRACTITIONER_ANOTHER_TENANT === undefined) { + throw new Error('COGNITO_USERNAME_PRACTITIONER_ANOTHER_TENANT environment variable is not defined'); + } + } + + // for simplicity the different test users have the same password + const password = COGNITO_PASSWORD; + let username: string | undefined; + switch (role) { + case 'practitioner': + if (tenantId === undefined || tenantId === DEFAULT_TENANT_ID) { + username = COGNITO_USERNAME_PRACTITIONER; + break; + } + if (tenantId === 'tenant2') { + username = COGNITO_USERNAME_PRACTITIONER_ANOTHER_TENANT!; + break; + } + break; + case 'auditor': + username = COGNITO_USERNAME_AUDITOR; + break; + default: + break; + } + + if (username === undefined) { + throw new Error('Could not find a username. Did you set up the integ tests correctly'); + } + + return { + USERNAME: username, + PASSWORD: password, + }; +}; + +export const getFhirClient = async ({ + role = 'practitioner', + providedAccessToken, + tenant = 'tenant1', +}: { role?: 'auditor' | 'practitioner'; providedAccessToken?: string; tenant?: string } = {}) => { + const { API_URL, API_KEY, API_AWS_REGION, COGNITO_CLIENT_ID, MULTI_TENANCY_ENABLED } = process.env; if (API_URL === undefined) { throw new Error('API_URL environment variable is not defined'); } @@ -34,37 +89,46 @@ export const getFhirClient = async ( if (COGNITO_CLIENT_ID === undefined) { throw new Error('COGNITO_CLIENT_ID environment variable is not defined'); } - if (COGNITO_USERNAME_PRACTITIONER === undefined) { - throw new Error('COGNITO_USERNAME_PRACTITIONER environment variable is not defined'); - } - if (COGNITO_USERNAME_AUDITOR === undefined) { - throw new Error('COGNITO_USERNAME_AUDITOR environment variable is not defined'); - } - if (COGNITO_PASSWORD === undefined) { - throw new Error('COGNITO_PASSWORD environment variable is not defined'); - } AWS.config.update({ region: API_AWS_REGION }); const Cognito = new AWS.CognitoIdentityServiceProvider(); - const accessToken = + const IdToken = providedAccessToken ?? ( await Cognito.initiateAuth({ ClientId: COGNITO_CLIENT_ID, AuthFlow: 'USER_PASSWORD_AUTH', - AuthParameters: { - USERNAME: role === 'auditor' ? COGNITO_USERNAME_AUDITOR : COGNITO_USERNAME_PRACTITIONER, - PASSWORD: COGNITO_PASSWORD, - }, + AuthParameters: getAuthParameters(role, tenant), }).promise() - ).AuthenticationResult!.AccessToken; + ).AuthenticationResult!.IdToken!; + + let baseURL = API_URL; + + if (MULTI_TENANCY_ENABLED === 'true') { + const decoded = decode(IdToken) as any; + let tenantIdFromToken; + if (!decoded) { + // This only happens when the jwt token is invalid. + tenantIdFromToken = DEFAULT_TENANT_ID; + } else { + tenantIdFromToken = decoded['custom:tenantId']; + } + if (!tenantIdFromToken) { + throw new Error( + 'Attempted to run multi-tenancy tests but the tenantId is not present in the token. Did you set up the integ tests correctly?', + ); + } + + baseURL = `${API_URL}/tenant/${tenantIdFromToken}`; + } + return axios.create({ headers: { 'x-api-key': API_KEY, - Authorization: `Bearer ${accessToken}`, + Authorization: `Bearer ${IdToken}`, }, - baseURL: API_URL, + baseURL, }); }; diff --git a/package.json b/package.json index 29b46328..10d5443b 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ "lint-fix": "eslint --fix . --ext .ts,.tsx", "build": "tsc", "watch": "tsc -w", - "test": "jest --silent --passWithNoTests --testPathPattern=src/*", + "test": "jest --silent --passWithNoTests --testPathPattern=src/* --runInBand", "int-test": "jest --testPathPattern=integration-test/*", "test-coverage": "jest --coverage", "release": "yarn run build && yarn run lint && yarn run test", @@ -46,6 +46,7 @@ "@types/chance": "^1.1.1", "@types/express": "^4.17.2", "@types/jest": "^26.0.19", + "@types/jsonwebtoken": "^8.5.4", "@types/node": "^12", "@types/sinon": "^9.0.0", "@typescript-eslint/eslint-plugin": "^4.11.1", @@ -59,6 +60,7 @@ "jest": "^26.6.3", "jest-circus": "^26.6.3", "jest-mock-extended": "^1.0.8", + "jsonwebtoken": "^8.5.1", "lodash": "^4.17.21", "prettier": "^1.19.1", "qs": "^6.10.1", diff --git a/yarn.lock b/yarn.lock index 86b82f8e..337d95b5 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2094,6 +2094,13 @@ resolved "https://registry.yarnpkg.com/@types/json5/-/json5-0.0.29.tgz#ee28707ae94e11d2b827bcbe5270bcea7f3e71ee" integrity sha1-7ihweulOEdK4J7y+UnC86n8+ce4= +"@types/jsonwebtoken@^8.5.4": + version "8.5.4" + resolved "https://registry.yarnpkg.com/@types/jsonwebtoken/-/jsonwebtoken-8.5.4.tgz#50ccaf0aa6f5d7b9956e70fe323b76e582991913" + integrity sha512-4L8msWK31oXwdtC81RmRBAULd0ShnAHjBuKT9MRQpjP0piNrZdXyTRcKY9/UIfhGeKIT4PvF5amOOUbbT/9Wpg== + dependencies: + "@types/node" "*" + "@types/keyv@*": version "3.1.1" resolved "https://registry.yarnpkg.com/@types/keyv/-/keyv-3.1.1.tgz#e45a45324fca9dab716ab1230ee249c9fb52cfa7" From e9bd5dd231adcd36c2b37f0b4e26c034f7933703 Mon Sep 17 00:00:00 2001 From: Nestor Carvantes Date: Thu, 22 Jul 2021 14:37:02 -0500 Subject: [PATCH 2/2] add include and revinclude test --- integration-tests/multitenancy.test.ts | 53 ++++++++++++++++++++++++++ package.json | 4 +- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/integration-tests/multitenancy.test.ts b/integration-tests/multitenancy.test.ts index b89930a0..ec8c045d 100644 --- a/integration-tests/multitenancy.test.ts +++ b/integration-tests/multitenancy.test.ts @@ -63,6 +63,59 @@ if (process.env.MULTI_TENANCY_ENABLED === 'true') { ); }); + test('tenant cannot SEARCH _include or _revinclude resources from another tenant', async () => { + const testOrganization = { + resourceType: 'Organization', + name: 'Some Organization', + }; + + const testOrganizationResource = (await client.post('Organization', testOrganization)).data; + + const testPatientWithRelativeReferenceToOrg: ReturnType = ( + await clientForAnotherTenant.post('Patient', { + ...randomPatient(), + managingOrganization: { + reference: `Organization/${testOrganizationResource.id}`, + }, + }) + ).data; + + const testPatientWithAbsoluteReferenceToOrg: ReturnType = ( + await clientForAnotherTenant.post('Patient', { + ...randomPatient(), + managingOrganization: { + reference: `${process.env.API_URL}/tenant/tenant1/Organization/${testOrganizationResource.id}`, + }, + }) + ).data; + + await waitForResourceToBeSearchable(clientForAnotherTenant, testPatientWithAbsoluteReferenceToOrg); + + await expectResourceToNotBePartOfSearchResults( + clientForAnotherTenant, + { url: 'Patient', params: { _id: testPatientWithRelativeReferenceToOrg.id, _include: '*' } }, + testOrganizationResource, + ); + + await expectResourceToNotBePartOfSearchResults( + clientForAnotherTenant, + { url: 'Patient', params: { _id: testPatientWithAbsoluteReferenceToOrg.id, _include: '*' } }, + testOrganizationResource, + ); + + await expectResourceToNotBePartOfSearchResults( + client, + { url: 'Organization', params: { _id: testOrganizationResource.id, _revinclude: '*' } }, + testPatientWithAbsoluteReferenceToOrg, + ); + + await expectResourceToNotBePartOfSearchResults( + client, + { url: 'Organization', params: { _id: testOrganizationResource.id, _revinclude: '*' } }, + testPatientWithRelativeReferenceToOrg, + ); + }); + test('tenant cannot EXPORT resources from another tenant', async () => { const testPatient: ReturnType = (await client.post('Patient', randomPatient())).data; const bulkExportTestHelper = new BulkExportTestHelper(clientForAnotherTenant); diff --git a/package.json b/package.json index 10d5443b..cae8a051 100644 --- a/package.json +++ b/package.json @@ -20,8 +20,8 @@ "lint-fix": "eslint --fix . --ext .ts,.tsx", "build": "tsc", "watch": "tsc -w", - "test": "jest --silent --passWithNoTests --testPathPattern=src/* --runInBand", - "int-test": "jest --testPathPattern=integration-test/*", + "test": "jest --silent --passWithNoTests --testPathPattern=src/*", + "int-test": "jest --testPathPattern=integration-test/* --runInBand", "test-coverage": "jest --coverage", "release": "yarn run build && yarn run lint && yarn run test", "clean": "rm -rf build/* node_modules/* dist/* .serverless/* .nyc_output/*",