From d90ca95322e35d35fef9f5bba8d7dc0ea5158ced Mon Sep 17 00:00:00 2001 From: Ryan Andrews Date: Mon, 16 Sep 2024 17:36:20 +0100 Subject: [PATCH 01/11] INCIDEN-922: Adds permissions to update-client handler The Update Client handler will now need to query the dynamo table, as we will need to ensure there is a serviceId-userId relation to be able to correctly authorise an update to a client --- backend/api/api.template.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/backend/api/api.template.yml b/backend/api/api.template.yml index 5a5a275af..30e5dfe95 100644 --- a/backend/api/api.template.yml +++ b/backend/api/api.template.yml @@ -915,9 +915,14 @@ Resources: - Effect: Allow Action: states:StartSyncExecution Resource: !Ref UpdateClientStepFunction + - DynamoDBReadPolicy: + TableName: + Fn::ImportValue: !Sub ${DeploymentName}-DynamoDB-DataTableName Environment: Variables: STATE_MACHINE_ARN: !Ref UpdateClientStepFunction + TABLE: + Fn::ImportValue: !Sub ${DeploymentName}-DynamoDB-DataTableName Events: Api: Type: Api From b16341a2f8ede4484e30d243bcac4a90d26aa0ae Mon Sep 17 00:00:00 2001 From: Ryan Andrews Date: Mon, 16 Sep 2024 17:41:31 +0100 Subject: [PATCH 02/11] INCIDEN-922: Adds access token to listServices method Our lambda on the backend will now expect an access token, so we need to start passing it in the GET request. We already validate the token so this is just a case of updating the interface and ensuring we pass it through --- express/src/services/lambda-facade/LambdaFacade.ts | 4 ++-- .../src/services/lambda-facade/LambdaFacadeInterface.ts | 2 +- express/src/services/self-service-services-service.ts | 7 +++++-- .../tests/services/self-service-services-service.test.ts | 2 +- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/express/src/services/lambda-facade/LambdaFacade.ts b/express/src/services/lambda-facade/LambdaFacade.ts index dc57ea72e..32f8baa13 100644 --- a/express/src/services/lambda-facade/LambdaFacade.ts +++ b/express/src/services/lambda-facade/LambdaFacade.ts @@ -107,8 +107,8 @@ export default class LambdaFacade implements LambdaFacadeInterface { } } - listServices(userId: string): Promise { - return this.get(`/get-services/${userId}`); + listServices(userId: string, accessToken: string): Promise { + return this.get(`/get-services/${userId}`, accessToken); } listClients(serviceId: string, accessToken: string): Promise> { diff --git a/express/src/services/lambda-facade/LambdaFacadeInterface.ts b/express/src/services/lambda-facade/LambdaFacadeInterface.ts index 773889b89..babcb311b 100644 --- a/express/src/services/lambda-facade/LambdaFacadeInterface.ts +++ b/express/src/services/lambda-facade/LambdaFacadeInterface.ts @@ -20,7 +20,7 @@ export default interface LambdaFacadeInterface { generateClient(service: Service, authenticationResult: AuthenticationResultType): Promise; - listServices(userId: string): Promise; + listServices(userId: string, accessToken: string): Promise; // TODO The QueryCommandOutput type should be replaced by a class shared between the frontend and the API (contract) listClients(serviceId: string, accessToken: string): Promise>; diff --git a/express/src/services/self-service-services-service.ts b/express/src/services/self-service-services-service.ts index 151c384ae..3fe0bcfa1 100644 --- a/express/src/services/self-service-services-service.ts +++ b/express/src/services/self-service-services-service.ts @@ -268,7 +268,7 @@ export default class SelfServiceServicesService { async listServices(userId: string, accessToken: string): Promise { console.info("In self-service-services-service:listServices()"); await this.validateToken(accessToken, "listServices"); - return dynamoServicesToDomainServices((await this.lambda.listServices(userId)).data.Items); + return dynamoServicesToDomainServices((await this.lambda.listServices(userId, accessToken)).data.Items); } async updateUser(userId: string, updates: UserUpdates, accessToken: string): Promise { @@ -285,6 +285,7 @@ export default class SelfServiceServicesService { } async globalSignOut(userEmail: string, accessToken: string): Promise { + await this.validateToken(accessToken, "Global sign out"); await this.cognito.globalSignOut(accessToken); return this.lambda.globalSignOut(userEmail); } @@ -362,7 +363,9 @@ export default class SelfServiceServicesService { async recreateDynamoDBAccountLinks(authenticationResult: AuthenticationResultType, oldUserID: string) { console.info("In self-service-services-service:createNewDynamoDBAccountLinks()"); - const serviceListToReplicate = dynamoServicesToDomainServices((await this.lambda.listServices(oldUserID)).data.Items); + const serviceListToReplicate = dynamoServicesToDomainServices( + (await this.lambda.listServices(oldUserID, nonNull(authenticationResult.AccessToken))).data.Items + ); for (const serviceItem of serviceListToReplicate) { const serviceID: string = serviceItem.id; diff --git a/express/tests/services/self-service-services-service.test.ts b/express/tests/services/self-service-services-service.test.ts index 44bfb341e..28a471066 100644 --- a/express/tests/services/self-service-services-service.test.ts +++ b/express/tests/services/self-service-services-service.test.ts @@ -370,7 +370,7 @@ describe("SelfServiceServicesService tests", () => { const receivedServices = await mockS4Instance.listServices(TEST_USER_ID, TEST_ACCESS_TOKEN); expect(mockCognitoInterface.getUser).toHaveBeenCalledWith(TEST_ACCESS_TOKEN); - expect(mockLambdaFacade.listServices).toHaveBeenCalledWith(TEST_USER_ID); + expect(mockLambdaFacade.listServices).toHaveBeenCalledWith(TEST_USER_ID, TEST_ACCESS_TOKEN); expect(receivedServices).toStrictEqual([TEST_SERVICE, TEST_SERVICE]); expect(console.info).toHaveBeenCalledWith("In self-service-services-service:listServices()"); }); From 6b4467cfe9b3004e8ba4537c18de6f5f50194b8e Mon Sep 17 00:00:00 2001 From: Ryan Andrews Date: Tue, 17 Sep 2024 10:05:39 +0100 Subject: [PATCH 03/11] INCIDEN-922: Adds authorisation to dynamoDb handlers We have purposely left getDynamoDBEntriesHandler handler without authorisation as it is called when a useer may not have an access token present as part of the forgot password flow. I have also removed including the userEmail in the error message as this is unneccesary --- .../handlers/dynamodb/dynamo-db-service.ts | 63 ++++++-- .../dynamodb/dynamo-db-service.test.ts | 152 +++++++++++++++--- 2 files changed, 180 insertions(+), 35 deletions(-) diff --git a/backend/api/src/handlers/dynamodb/dynamo-db-service.ts b/backend/api/src/handlers/dynamodb/dynamo-db-service.ts index d959f92d6..ca588dca9 100644 --- a/backend/api/src/handlers/dynamodb/dynamo-db-service.ts +++ b/backend/api/src/handlers/dynamodb/dynamo-db-service.ts @@ -1,19 +1,30 @@ import DynamoDbClient from "../../dynamodb-client"; import console from "console"; import {APIGatewayProxyEvent, APIGatewayProxyResult} from "aws-lambda"; +import {validateAuthorisationHeader} from "../helper/validate-authorisation-header"; const dynamoDBClient = new DynamoDbClient(); export const getDynamoDBEntriesHandler = async (event: APIGatewayProxyEvent): Promise => { + //This endpoint is unauthorised because it is called in controllers + //where the user may not have an access token present in their session + //such as in the forgot password flow before login console.log("dynamo-db-inspector - In getDynamoDBEntriesHandler"); const response = {statusCode: 404, body: JSON.stringify("")}; const userEmail = event.pathParameters?.userEmail; + if (!userEmail) { + return { + statusCode: 400, + body: "Missing userEmail parameter in request" + }; + } + const result = await dynamoDBClient.getListOfClients(); - if (userEmail == null || result == null || result.Items == null) { - throw new Error("Unable to check DynamoDB for Items of User Email =>" + userEmail); + if (result == null || result.Items == null) { + throw new Error("Unable to check DynamoDB for Items of User Email"); } else { response.statusCode = 200; // Need to set Response here because List might be empty (i.e. non-existent account). @@ -31,24 +42,49 @@ export const getDynamoDBEntriesHandler = async (event: APIGatewayProxyEvent): Pr export const deleteDynamoDBClientEntriesHandler = async (event: APIGatewayProxyEvent): Promise => { console.log("dynamo-db-inspector - In deleteDynamoDBClientEntriesHandler"); + const authHeader = event.headers.Authorization; + + const authorisationHeaderValidation = validateAuthorisationHeader(authHeader); + + if (!authorisationHeaderValidation.valid) { + return authorisationHeaderValidation.errorResponse; + } const response = {statusCode: 500, body: JSON.stringify("")}; const payload = event?.body ? JSON.parse(event.body as string) : event; - const userID = payload.userId; + const userId = payload.userId; const serviceID = payload.serviceId; - if (userID == null || serviceID == null) { + if (userId == null || serviceID == null) { throw new Error("No details provided for DeleteDynamoDBClientEntries"); - } else { - await dynamoDBClient.deleteDynamoDBClientEntries(userID, serviceID); - response.statusCode = 200; } + const isUserAuthorised = await dynamoDBClient.checkServiceUserExists(serviceID, authorisationHeaderValidation.userId); + + const userIdWithoutPrefix = userId.includes("user#") ? userId.substring("user#".length) : userId; + + if (!isUserAuthorised || userIdWithoutPrefix !== authorisationHeaderValidation.userId) { + return { + statusCode: 403, + body: "Forbidden" + }; + } + + await dynamoDBClient.deleteDynamoDBClientEntries(userId, serviceID); + response.statusCode = 200; + return response; }; export const deleteDynamoDBServiceEntriesHandler = async (event: APIGatewayProxyEvent): Promise => { console.log("dynamo-db-inspector - In deleteDynamoDBServiceEntriesHandler"); + const authHeader = event.headers.Authorization; + + const authorisationHeaderValidation = validateAuthorisationHeader(authHeader); + + if (!authorisationHeaderValidation.valid) { + return authorisationHeaderValidation.errorResponse; + } const response = {statusCode: 500, body: JSON.stringify("")}; const payload = event?.body ? JSON.parse(event.body as string) : event; @@ -56,10 +92,17 @@ export const deleteDynamoDBServiceEntriesHandler = async (event: APIGatewayProxy if (serviceID == null) { throw new Error("No Service ID provided for DeleteDynamoDBServiceEntries"); - } else { - await dynamoDBClient.deleteDynamoDBServiceEntries(serviceID); - response.statusCode = 200; } + const isUserAuthorised = await dynamoDBClient.checkServiceUserExists(serviceID, authorisationHeaderValidation.userId); + + if (!isUserAuthorised) { + return { + statusCode: 403, + body: "Forbidden" + }; + } + await dynamoDBClient.deleteDynamoDBServiceEntries(serviceID); + response.statusCode = 200; return response; }; diff --git a/backend/api/tests/handlers/dynamodb/dynamo-db-service.test.ts b/backend/api/tests/handlers/dynamodb/dynamo-db-service.test.ts index 37c5b5a5f..c76a106ee 100644 --- a/backend/api/tests/handlers/dynamodb/dynamo-db-service.test.ts +++ b/backend/api/tests/handlers/dynamodb/dynamo-db-service.test.ts @@ -5,7 +5,7 @@ import { } from "../../../src/handlers/dynamodb/dynamo-db-service"; import DynamoDbClient from "../../../src/dynamodb-client"; import {constructTestApiGatewayEvent} from "../utils"; -import {TEST_DATA_TABLE_ITEM, TEST_SERVICE_ID, TEST_USER_EMAIL, TEST_USER_ID} from "../constants"; +import {TEST_ACCESS_TOKEN, TEST_DATA_TABLE_ITEM, TEST_SERVICE_ID, TEST_USER_EMAIL, TEST_USER_ID} from "../constants"; describe("getDynamoDBEntriesHandler tests", () => { beforeEach(() => { @@ -18,10 +18,14 @@ describe("getDynamoDBEntriesHandler tests", () => { .mockResolvedValue({Items: [TEST_DATA_TABLE_ITEM], $metadata: {httpStatusCode: 200}}); const serviceHandlerResponse = await getDynamoDBEntriesHandler( - constructTestApiGatewayEvent({body: "", pathParameters: {userEmail: TEST_USER_EMAIL}}) + constructTestApiGatewayEvent({ + body: "", + pathParameters: {userEmail: TEST_USER_EMAIL}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }) ); - expect(getListOfItemsSpy).toBeCalledTimes(1); + expect(getListOfItemsSpy).toHaveBeenCalledTimes(1); expect(serviceHandlerResponse).toStrictEqual({ statusCode: 200, body: JSON.stringify(TEST_DATA_TABLE_ITEM) @@ -31,21 +35,29 @@ describe("getDynamoDBEntriesHandler tests", () => { it("calls the dynamo client with a scan command with the expected values and throws an error when the dynamo client throws an error", async () => { const error = "SomeAwsError"; const getListOfItemsSpy = jest.spyOn(DynamoDbClient.prototype, "getListOfClients").mockRejectedValue(new Error(error)); - const testEvent = constructTestApiGatewayEvent({body: "", pathParameters: {userEmail: TEST_USER_EMAIL}}); + const testEvent = constructTestApiGatewayEvent({ + body: "", + pathParameters: {userEmail: TEST_USER_EMAIL}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }); - await expect(getDynamoDBEntriesHandler(testEvent)).rejects.toThrowError(error); - expect(getListOfItemsSpy).toBeCalledTimes(1); + await expect(getDynamoDBEntriesHandler(testEvent)).rejects.toThrow(error); + expect(getListOfItemsSpy).toHaveBeenCalledTimes(1); }); - it("throws an error if there is no userEmail pathParameter", async () => { + it("returns a 400 for no userEmail parameter", async () => { const getListOfItemsSpy = jest .spyOn(DynamoDbClient.prototype, "getListOfClients") .mockResolvedValue({Items: [TEST_DATA_TABLE_ITEM], $metadata: {httpStatusCode: 200}}); - await expect(getDynamoDBEntriesHandler(constructTestApiGatewayEvent())).rejects.toThrowError( - "Unable to check DynamoDB for Items of User Email =>" + undefined + const getUserResponse = await getDynamoDBEntriesHandler( + constructTestApiGatewayEvent({body: "", pathParameters: {}, headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`}}) ); - expect(getListOfItemsSpy).toBeCalledTimes(1); + expect(getUserResponse).toStrictEqual({ + statusCode: 400, + body: "Missing userEmail parameter in request" + }); + expect(getListOfItemsSpy).not.toHaveBeenCalled(); }); it("returns an empty string in the body field if there is no matching email in the list of clients", async () => { @@ -62,10 +74,14 @@ describe("getDynamoDBEntriesHandler tests", () => { .mockResolvedValue({Items: [TEST_CLIENT_ENTRY_WITH_DIFFERENT_EMAIL], $metadata: {httpStatusCode: 200}}); const serviceHandlerResponse = await getDynamoDBEntriesHandler( - constructTestApiGatewayEvent({body: "", pathParameters: {userEmail: TEST_USER_EMAIL}}) + constructTestApiGatewayEvent({ + body: "", + pathParameters: {userEmail: TEST_USER_EMAIL}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }) ); - expect(getListOfItemsSpy).toBeCalledTimes(1); + expect(getListOfItemsSpy).toHaveBeenCalledTimes(1); expect(serviceHandlerResponse).toStrictEqual({ statusCode: 200, body: JSON.stringify("") @@ -80,6 +96,7 @@ describe("deleteDynamoDBClientEntriesHandler tests", () => { it("calls the dynamo client with a delete command with the expected values and returns a 200 with the expected response body", async () => { const deleteEntriesSpy = jest.spyOn(DynamoDbClient.prototype, "deleteDynamoDBClientEntries").mockResolvedValue(); + const checkServiceUserExistSpy = jest.spyOn(DynamoDbClient.prototype, "checkServiceUserExists").mockResolvedValue(true); const serviceHandlerResponse = await deleteDynamoDBClientEntriesHandler( constructTestApiGatewayEvent({ @@ -87,10 +104,12 @@ describe("deleteDynamoDBClientEntriesHandler tests", () => { userId: TEST_USER_ID, serviceId: TEST_SERVICE_ID }), - pathParameters: {} + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} }) ); + expect(checkServiceUserExistSpy).toHaveBeenCalledWith(TEST_SERVICE_ID, TEST_USER_ID); expect(deleteEntriesSpy).toHaveBeenCalledWith(TEST_USER_ID, TEST_SERVICE_ID); expect(serviceHandlerResponse).toStrictEqual({ statusCode: 200, @@ -107,10 +126,11 @@ describe("deleteDynamoDBClientEntriesHandler tests", () => { body: JSON.stringify({ serviceId: TEST_SERVICE_ID }), - pathParameters: {} + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} }) ) - ).rejects.toThrowError("No details provided for DeleteDynamoDBClientEntries"); + ).rejects.toThrow("No details provided for DeleteDynamoDBClientEntries"); expect(deleteEntriesSpy).not.toHaveBeenCalled(); }); @@ -123,10 +143,11 @@ describe("deleteDynamoDBClientEntriesHandler tests", () => { body: JSON.stringify({ userId: TEST_USER_ID }), - pathParameters: {} + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} }) ) - ).rejects.toThrowError("No details provided for DeleteDynamoDBClientEntries"); + ).rejects.toThrow("No details provided for DeleteDynamoDBClientEntries"); expect(deleteEntriesSpy).not.toHaveBeenCalled(); }); @@ -143,13 +164,60 @@ describe("deleteDynamoDBClientEntriesHandler tests", () => { userId: TEST_USER_ID, serviceId: TEST_SERVICE_ID }), - pathParameters: {} + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} }) ) - ).rejects.toThrowError(error); + ).rejects.toThrow(error); expect(deleteClientEntriesSpy).toHaveBeenCalledWith(TEST_USER_ID, TEST_SERVICE_ID); }); + + it("returns a 403 when there is no service-user relation", async () => { + const deleteEntriesSpy = jest.spyOn(DynamoDbClient.prototype, "deleteDynamoDBClientEntries").mockResolvedValue(); + const checkServiceUserExistSpy = jest.spyOn(DynamoDbClient.prototype, "checkServiceUserExists").mockResolvedValue(false); + + const serviceHandlerResponse = await deleteDynamoDBClientEntriesHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify({ + userId: TEST_USER_ID, + serviceId: TEST_SERVICE_ID + }), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }) + ); + + expect(checkServiceUserExistSpy).toHaveBeenCalledWith(TEST_SERVICE_ID, TEST_USER_ID); + expect(deleteEntriesSpy).not.toHaveBeenCalled(); + expect(serviceHandlerResponse).toStrictEqual({ + statusCode: 403, + body: "Forbidden" + }); + }); + + it("returns a 403 when the userId in the path params does not match the access token", async () => { + const deleteEntriesSpy = jest.spyOn(DynamoDbClient.prototype, "deleteDynamoDBClientEntries").mockResolvedValue(); + const checkServiceUserExistSpy = jest.spyOn(DynamoDbClient.prototype, "checkServiceUserExists").mockResolvedValue(true); + + const serviceHandlerResponse = await deleteDynamoDBClientEntriesHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify({ + userId: TEST_USER_ID + "djnerdjnr34rnjf", + serviceId: TEST_SERVICE_ID + }), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }) + ); + + expect(checkServiceUserExistSpy).toHaveBeenCalledWith(TEST_SERVICE_ID, TEST_USER_ID); + expect(deleteEntriesSpy).not.toHaveBeenCalled(); + expect(serviceHandlerResponse).toStrictEqual({ + statusCode: 403, + body: "Forbidden" + }); + }); }); describe("deleteDynamoDBServiceEntriesHandler tests", () => { @@ -159,26 +227,52 @@ describe("deleteDynamoDBServiceEntriesHandler tests", () => { it("calls the dynamo client with a delete command with the expected values and returns a 200 with the expected response body", async () => { const deleteEntriesSpy = jest.spyOn(DynamoDbClient.prototype, "deleteDynamoDBServiceEntries").mockResolvedValue(); + const checkServiceUserExistSpy = jest.spyOn(DynamoDbClient.prototype, "checkServiceUserExists").mockResolvedValue(true); const serviceHandlerResponse = await deleteDynamoDBServiceEntriesHandler( constructTestApiGatewayEvent({ body: JSON.stringify({ serviceId: TEST_SERVICE_ID }), - pathParameters: {} + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} }) ); expect(deleteEntriesSpy).toHaveBeenCalledWith(TEST_SERVICE_ID); + expect(checkServiceUserExistSpy).toHaveBeenCalledWith(TEST_SERVICE_ID, TEST_USER_ID); expect(serviceHandlerResponse).toStrictEqual({ statusCode: 200, body: JSON.stringify("") }); }); + it("returns a 403 for no service-user relationship", async () => { + const deleteEntriesSpy = jest.spyOn(DynamoDbClient.prototype, "deleteDynamoDBServiceEntries").mockResolvedValue(); + const checkServiceUserExistSpy = jest.spyOn(DynamoDbClient.prototype, "checkServiceUserExists").mockResolvedValue(false); + + const serviceHandlerResponse = await deleteDynamoDBServiceEntriesHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify({ + serviceId: TEST_SERVICE_ID + }), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }) + ); + + expect(deleteEntriesSpy).not.toHaveBeenCalled(); + expect(checkServiceUserExistSpy).toHaveBeenCalledWith(TEST_SERVICE_ID, TEST_USER_ID); + expect(serviceHandlerResponse).toStrictEqual({ + statusCode: 403, + body: "Forbidden" + }); + }); + it("calls the dynamo client with a delete command with the expected values and throws an error when the dynamo client throws an error", async () => { const error = "SomeAwsError"; const deleteEntriesSpy = jest.spyOn(DynamoDbClient.prototype, "deleteDynamoDBServiceEntries").mockRejectedValue(new Error(error)); + const checkServiceUserExistSpy = jest.spyOn(DynamoDbClient.prototype, "checkServiceUserExists").mockResolvedValue(true); await expect( deleteDynamoDBServiceEntriesHandler( @@ -186,19 +280,27 @@ describe("deleteDynamoDBServiceEntriesHandler tests", () => { body: JSON.stringify({ serviceId: TEST_SERVICE_ID }), - pathParameters: {} + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} }) ) - ).rejects.toThrowError(error); + ).rejects.toThrow(error); expect(deleteEntriesSpy).toHaveBeenCalledWith(TEST_SERVICE_ID); + expect(checkServiceUserExistSpy).toHaveBeenCalledWith(TEST_SERVICE_ID, TEST_USER_ID); }); it("throws an error when there is no serviceId provided", async () => { const deleteEntriesSpy = jest.spyOn(DynamoDbClient.prototype, "deleteDynamoDBServiceEntries").mockResolvedValue(); - await expect(deleteDynamoDBServiceEntriesHandler(constructTestApiGatewayEvent())).rejects.toThrowError( - "No Service ID provided for DeleteDynamoDBServiceEntries" - ); + await expect( + deleteDynamoDBServiceEntriesHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify({}), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }) + ) + ).rejects.toThrow("No Service ID provided for DeleteDynamoDBServiceEntries"); expect(deleteEntriesSpy).not.toHaveBeenCalled(); }); }); From fce2285fc9dcf8188308a15cc324bfe0ef8fef4a Mon Sep 17 00:00:00 2001 From: Ryan Andrews Date: Tue, 17 Sep 2024 10:06:45 +0100 Subject: [PATCH 04/11] INCIDEN-922: Adds comment to update lambdas These lambdas are called by a step function, and the authorisation happens before they are called. This means we do not need to implment additonal authorisation here --- backend/api/src/handlers/dynamodb/update-service-client.ts | 4 +++- backend/api/src/handlers/dynamodb/update-service.ts | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/backend/api/src/handlers/dynamodb/update-service-client.ts b/backend/api/src/handlers/dynamodb/update-service-client.ts index 9231b6db5..0177abb82 100644 --- a/backend/api/src/handlers/dynamodb/update-service-client.ts +++ b/backend/api/src/handlers/dynamodb/update-service-client.ts @@ -10,7 +10,9 @@ export type clientRegistryUpdateResponse = { clientId: string; updates: Updates; }; - +//This lambda is called by the step function after updating the Client registry +//Given we authorise the user before we call the step function, there is no need +//to implement additional authorisation here export const updateServiceClientHandler = async (event: handlerInvokeEvent): Promise<{statusCode: number; body: string}> => { const payload: clientRegistryUpdateResponse = JSON.parse(event.body); diff --git a/backend/api/src/handlers/dynamodb/update-service.ts b/backend/api/src/handlers/dynamodb/update-service.ts index 3fa9cd1a0..f5ab7032b 100644 --- a/backend/api/src/handlers/dynamodb/update-service.ts +++ b/backend/api/src/handlers/dynamodb/update-service.ts @@ -3,6 +3,9 @@ import {handlerInvokeEvent} from "../handler-utils"; const client = new DynamoDbClient(); +//This lambda is called by the step function after updating the Client registry +//Given we authorise the user before we call the step function, there is no need +//to implement additional authorisation here export const updateServiceHandler = async (event: handlerInvokeEvent): Promise<{statusCode: number; body: string}> => { const body = JSON.parse(event.body as string); const response = {statusCode: 200, body: JSON.stringify("OK")}; From e0a2c85cc50596b3106917554b9510d2e6db842f Mon Sep 17 00:00:00 2001 From: Ryan Andrews Date: Tue, 17 Sep 2024 10:10:03 +0100 Subject: [PATCH 05/11] INCIDEN-922: Adds userId test constant --- express/tests/constants.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/express/tests/constants.ts b/express/tests/constants.ts index 42a8e6120..88d54dc2c 100644 --- a/express/tests/constants.ts +++ b/express/tests/constants.ts @@ -6,6 +6,7 @@ import {Client} from "../@types/client"; export const TEST_TEMPLATE_PATH = "some/template/to/render"; export const TEST_PASSWORD = "somePassword"; +export const TEST_USER_ID = "1ded3d65-d088-4319-9431-ea5a3323799d"; export const TEST_EMAIL = "someEmail"; export const TEST_SESSION_ID = "someSessionId"; export const TEST_IP_ADDRESS = "1.1.1.1"; @@ -31,7 +32,7 @@ export const TEST_MFA_RESPONSE = { codeSentTo: TEST_PHONE_NUMBER }; export const TEST_SECURITY_CODE = "123456"; -export const TEST_ACCESS_TOKEN = "someAccessToken"; +export const TEST_ACCESS_TOKEN = `.${Buffer.from(JSON.stringify({sub: TEST_USER_ID})).toString("base64url")}.`; export const TEST_REFRESH_TOKEN = "someRefreshToken"; export const TEST_ID_TOKEN = "1.2.3"; export const TEST_AUTHENTICATION_RESULT = { @@ -55,7 +56,6 @@ export const TEST_JWT = JSON.stringify({ email: TEST_EMAIL, scopes: TEST_SCOPES }); -export const TEST_USER_ID = "user#1234"; export const TEST_USER = { id: TEST_USER_ID.substring("user#".length), fullName: TEST_FULL_NAME, From 379b0a092af09c55b31cd9c9705379e2d51fc574 Mon Sep 17 00:00:00 2001 From: Ryan Andrews Date: Tue, 17 Sep 2024 10:15:53 +0100 Subject: [PATCH 06/11] INCIDEN-922: Adds authorisation to getUser lambda This checks that the user Id included in the request body matches the userId in the access token --- backend/api/src/handlers/dynamodb/get-user.ts | 17 +++++++++ .../tests/handlers/dynamodb/get-user.test.ts | 37 ++++++++++++++++--- 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/backend/api/src/handlers/dynamodb/get-user.ts b/backend/api/src/handlers/dynamodb/get-user.ts index ffed4ec81..74a7a11f3 100644 --- a/backend/api/src/handlers/dynamodb/get-user.ts +++ b/backend/api/src/handlers/dynamodb/get-user.ts @@ -1,10 +1,27 @@ import {APIGatewayProxyEvent, APIGatewayProxyResult} from "aws-lambda"; import DynamoDbClient from "../../dynamodb-client"; +import {validateAuthorisationHeader} from "../helper/validate-authorisation-header"; const client = new DynamoDbClient(); export const getUserHandler = async (event: APIGatewayProxyEvent): Promise => { const cognitoId = JSON.parse(event.body as string); + const authHeader = event.headers.Authorization; + + const authorisationHeaderValidation = validateAuthorisationHeader(authHeader); + + if (!authorisationHeaderValidation.valid) { + return authorisationHeaderValidation.errorResponse; + } + const userIdWithoutPrefix = cognitoId.includes("user#") ? cognitoId.substring("user#".length) : cognitoId; + + if (authorisationHeaderValidation.userId !== userIdWithoutPrefix) { + return { + statusCode: 403, + body: "Forbidden" + }; + } + const response = {statusCode: 200, body: JSON.stringify("OK")}; await client diff --git a/backend/api/tests/handlers/dynamodb/get-user.test.ts b/backend/api/tests/handlers/dynamodb/get-user.test.ts index 42da3b05c..35fd6f774 100644 --- a/backend/api/tests/handlers/dynamodb/get-user.test.ts +++ b/backend/api/tests/handlers/dynamodb/get-user.test.ts @@ -1,7 +1,7 @@ import {getUserHandler} from "../../../src/handlers/dynamodb/get-user"; import DynamoDbClient from "../../../src/dynamodb-client"; import {constructTestApiGatewayEvent} from "../utils"; -import {TEST_COGNITO_USER_ID, TEST_USER_CONFIG} from "../constants"; +import {TEST_ACCESS_TOKEN, TEST_USER_CONFIG, TEST_USER_ID} from "../constants"; import {GetItemCommandOutput} from "@aws-sdk/client-dynamodb"; describe("getUserHandler tests", () => { @@ -14,10 +14,14 @@ describe("getUserHandler tests", () => { const getUserSpy = jest.spyOn(DynamoDbClient.prototype, "getUser").mockResolvedValue(testGetUserResponse); const serviceHandlerResponse = await getUserHandler( - constructTestApiGatewayEvent({body: JSON.stringify(TEST_COGNITO_USER_ID), pathParameters: {}}) + constructTestApiGatewayEvent({ + body: JSON.stringify(TEST_USER_ID), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }) ); - expect(getUserSpy).toHaveBeenCalledWith(TEST_COGNITO_USER_ID); + expect(getUserSpy).toHaveBeenCalledWith(TEST_USER_ID); expect(serviceHandlerResponse).toStrictEqual({ statusCode: 200, body: JSON.stringify(testGetUserResponse) @@ -29,13 +33,36 @@ describe("getUserHandler tests", () => { const getUserSpy = jest.spyOn(DynamoDbClient.prototype, "getUser").mockRejectedValue(error); const serviceHandlerResponse = await getUserHandler( - constructTestApiGatewayEvent({body: JSON.stringify(TEST_COGNITO_USER_ID), pathParameters: {}}) + constructTestApiGatewayEvent({ + body: JSON.stringify(TEST_USER_ID), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }) ); - expect(getUserSpy).toHaveBeenCalledWith(TEST_COGNITO_USER_ID); + expect(getUserSpy).toHaveBeenCalledWith(TEST_USER_ID); expect(serviceHandlerResponse).toStrictEqual({ statusCode: 500, body: JSON.stringify(error) }); }); + + it("returns a 403 if the userId in the access token does not match the access token", async () => { + const error = "SomeAwsError"; + const getUserSpy = jest.spyOn(DynamoDbClient.prototype, "getUser").mockRejectedValue(error); + + const serviceHandlerResponse = await getUserHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify("aNotMatchingUserId"), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }) + ); + + expect(getUserSpy).not.toHaveBeenCalled(); + expect(serviceHandlerResponse).toStrictEqual({ + statusCode: 403, + body: "Forbidden" + }); + }); }); From 6330d6c4cd39ad7785d9e78811ab9e961ae9eb35 Mon Sep 17 00:00:00 2001 From: Ryan Andrews Date: Tue, 17 Sep 2024 10:45:12 +0100 Subject: [PATCH 07/11] INCIDEN-921: Adds authorisation to the update-client lambda --- .../handlers/step-functions/update-client.ts | 34 +++++ .../step-functions/update-client.test.ts | 116 +++++++++++++++--- 2 files changed, 134 insertions(+), 16 deletions(-) diff --git a/backend/api/src/handlers/step-functions/update-client.ts b/backend/api/src/handlers/step-functions/update-client.ts index 881f6c996..19d8a0a54 100644 --- a/backend/api/src/handlers/step-functions/update-client.ts +++ b/backend/api/src/handlers/step-functions/update-client.ts @@ -1,6 +1,40 @@ import {APIGatewayProxyEvent, APIGatewayProxyResult, Context} from "aws-lambda"; import {stepFunctionHandler} from "./step-function-handler"; +import {validateAuthorisationHeader} from "../helper/validate-authorisation-header"; +import DynamoDbClient from "../../dynamodb-client"; + +const client = new DynamoDbClient(); export const doUpdateClientHandler = async (event: APIGatewayProxyEvent, context: Context): Promise => { + if (!event.body) { + return {statusCode: 400, body: "Invalid Request, missing body"}; + } + + const updateClientBody: { + serviceId: string; + updates: Record; + } = JSON.parse(event.body); + + const serviceId = updateClientBody.serviceId; + + if (!serviceId) { + return {statusCode: 400, body: "Invalid Request, missing serviceId"}; + } + + const authHeaderValidationResult = validateAuthorisationHeader(event.headers.Authorization); + + if (!authHeaderValidationResult.valid) { + return authHeaderValidationResult.errorResponse; + } + + const isUserAuthorised = await client.checkServiceUserExists(serviceId, authHeaderValidationResult.userId); + + if (!isUserAuthorised) { + return { + statusCode: 403, + body: "Forbidden" + }; + } + return stepFunctionHandler(event, context); }; diff --git a/backend/api/tests/handlers/step-functions/update-client.test.ts b/backend/api/tests/handlers/step-functions/update-client.test.ts index e99060e54..85ac4985a 100644 --- a/backend/api/tests/handlers/step-functions/update-client.test.ts +++ b/backend/api/tests/handlers/step-functions/update-client.test.ts @@ -2,23 +2,17 @@ import {SFNClient, StartSyncExecutionCommand, SyncExecutionStatus} from "@aws-sd import {mockClient} from "aws-sdk-client-mock"; import {constructTestApiGatewayEvent, mockLambdaContext} from "../utils"; import {doUpdateClientHandler} from "../../../src/handlers/step-functions/update-client"; -import {TEST_SERVICE_ID, TEST_SERVICE_NAME, TEST_USER_EMAIL} from "../constants"; +import {TEST_ACCESS_TOKEN, TEST_SERVICE_ID, TEST_USER_EMAIL, TEST_USER_ID} from "../constants"; import {TEST_STATE_MACHINE_ARN} from "../../setup"; import "aws-sdk-client-mock-jest"; +import DynamoDbClient from "../../../src/dynamodb-client"; -const TEST_UPDATE_CLIENT = { - service: { - serviceName: TEST_SERVICE_NAME, - id: TEST_SERVICE_ID - }, +const TEST_UPDATE_CLIENT_PAYLOAD = { + serviceId: TEST_SERVICE_ID, updates: { contactEmail: TEST_USER_EMAIL } }; -const TEST_NEW_CLIENT_EVENT = constructTestApiGatewayEvent({ - body: JSON.stringify(TEST_UPDATE_CLIENT), - pathParameters: {} -}); const mockSfnClient = mockClient(SFNClient); describe("newClientHandler tests", () => { @@ -26,6 +20,73 @@ describe("newClientHandler tests", () => { jest.clearAllMocks(); }); + it("returns a 400 for no body", async () => { + const serviceUserSpy = jest.spyOn(DynamoDbClient.prototype, "checkServiceUserExists"); + + const newClientResponse = await doUpdateClientHandler( + constructTestApiGatewayEvent({ + body: "", + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }), + mockLambdaContext + ); + + expect(serviceUserSpy).not.toHaveBeenCalled(); + expect(mockSfnClient).not.toHaveReceivedAnyCommand(); + + expect(newClientResponse).toStrictEqual({ + statusCode: 400, + body: "Invalid Request, missing body" + }); + }); + + it("returns a 400 for no serviceId in the body", async () => { + const serviceUserSpy = jest.spyOn(DynamoDbClient.prototype, "checkServiceUserExists"); + + const newClientResponse = await doUpdateClientHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify({ + updates: { + contactEmail: TEST_USER_EMAIL + } + }), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }), + mockLambdaContext + ); + + expect(serviceUserSpy).not.toHaveBeenCalled(); + expect(mockSfnClient).not.toHaveReceivedAnyCommand(); + + expect(newClientResponse).toStrictEqual({ + statusCode: 400, + body: "Invalid Request, missing serviceId" + }); + }); + + it("returns a 403 if the service user relation does not exist", async () => { + const serviceUserSpy = jest.spyOn(DynamoDbClient.prototype, "checkServiceUserExists").mockResolvedValue(false); + + const newClientResponse = await doUpdateClientHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify(TEST_UPDATE_CLIENT_PAYLOAD), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }), + mockLambdaContext + ); + + expect(serviceUserSpy).toHaveBeenCalledWith(TEST_SERVICE_ID, TEST_USER_ID); + expect(mockSfnClient).not.toHaveReceivedAnyCommand(); + + expect(newClientResponse).toStrictEqual({ + statusCode: 403, + body: "Forbidden" + }); + }); + it("returns a 200 for a successful state machine invoke", async () => { const mockSfnResponse = { status: SyncExecutionStatus.SUCCEEDED, @@ -33,11 +94,20 @@ describe("newClientHandler tests", () => { executionArn: TEST_STATE_MACHINE_ARN }; mockSfnClient.on(StartSyncExecutionCommand).resolves(mockSfnResponse); + const serviceUserSpy = jest.spyOn(DynamoDbClient.prototype, "checkServiceUserExists").mockResolvedValue(true); - const newClientResponse = await doUpdateClientHandler(TEST_NEW_CLIENT_EVENT, mockLambdaContext); + const newClientResponse = await doUpdateClientHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify(TEST_UPDATE_CLIENT_PAYLOAD), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }), + mockLambdaContext + ); + expect(serviceUserSpy).toHaveBeenCalledWith(TEST_SERVICE_ID, TEST_USER_ID); expect(mockSfnClient).toHaveReceivedCommandWith(StartSyncExecutionCommand, { - input: JSON.stringify(TEST_UPDATE_CLIENT), + input: JSON.stringify(TEST_UPDATE_CLIENT_PAYLOAD), stateMachineArn: TEST_STATE_MACHINE_ARN }); @@ -54,10 +124,17 @@ describe("newClientHandler tests", () => { executionArn: TEST_STATE_MACHINE_ARN }); - const newClientResponse = await doUpdateClientHandler(TEST_NEW_CLIENT_EVENT, mockLambdaContext); + const newClientResponse = await doUpdateClientHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify(TEST_UPDATE_CLIENT_PAYLOAD), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }), + mockLambdaContext + ); expect(mockSfnClient).toHaveReceivedCommandWith(StartSyncExecutionCommand, { - input: JSON.stringify(TEST_UPDATE_CLIENT), + input: JSON.stringify(TEST_UPDATE_CLIENT_PAYLOAD), stateMachineArn: TEST_STATE_MACHINE_ARN }); @@ -79,10 +156,17 @@ describe("newClientHandler tests", () => { cause: stateMachineErrorCause }); - const newClientResponse = await doUpdateClientHandler(TEST_NEW_CLIENT_EVENT, mockLambdaContext); + const newClientResponse = await doUpdateClientHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify(TEST_UPDATE_CLIENT_PAYLOAD), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }), + mockLambdaContext + ); expect(mockSfnClient).toHaveReceivedCommandWith(StartSyncExecutionCommand, { - input: JSON.stringify(TEST_UPDATE_CLIENT), + input: JSON.stringify(TEST_UPDATE_CLIENT_PAYLOAD), stateMachineArn: TEST_STATE_MACHINE_ARN }); From 61e03a3badc52d458bbf7bc917819e7f0358a958 Mon Sep 17 00:00:00 2001 From: Ryan Andrews Date: Tue, 17 Sep 2024 10:52:48 +0100 Subject: [PATCH 08/11] INCIDEN-922: Removes unused param from method interface The updateUser method does not use the cognitoUserId argument, and it is redundant as the userId provided is the cognitoUserId with a `user#` prefix. This also makes the lambda code less confusing as where we call the `/update-user` endpoint the body does not include this value. --- backend/api/src/dynamodb-client.ts | 2 +- backend/api/tests/dynamodb-client.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/api/src/dynamodb-client.ts b/backend/api/src/dynamodb-client.ts index 98e90a93e..820dd9094 100644 --- a/backend/api/src/dynamodb-client.ts +++ b/backend/api/src/dynamodb-client.ts @@ -175,7 +175,7 @@ export default class DynamoDbClient { return this.update("service", serviceId, "client", clientId, updates); } - async updateUser(userId: string, cognitoUserId: string, updates: Updates): Promise { + async updateUser(userId: string, updates: Updates): Promise { return this.update("user", userId, "user", userId, updates); } diff --git a/backend/api/tests/dynamodb-client.test.ts b/backend/api/tests/dynamodb-client.test.ts index f95906665..9e4a585f4 100644 --- a/backend/api/tests/dynamodb-client.test.ts +++ b/backend/api/tests/dynamodb-client.test.ts @@ -174,7 +174,7 @@ describe("DynamoDB client", () => { }); it("should invoke updateUser successfully", async () => { - await client.updateUser("423423ada-32123892", "c-id-0001", {Name: "Robert"}); + await client.updateUser("423423ada-32123892", {Name: "Robert"}); expect(updateItemCommand).toBeCalledTimes(1); }); From f9a0fd8890ce5b80d0e452390cf2713eccffd10f Mon Sep 17 00:00:00 2001 From: Ryan Andrews Date: Tue, 17 Sep 2024 10:59:46 +0100 Subject: [PATCH 09/11] INCIDEN-922: Adds authorisation to update user handler This checks that the userId in the access token matches the userId in the update request body, ensuring a user can only update their own entries --- .../api/src/handlers/dynamodb/update-user.ts | 31 ++++++- .../handlers/dynamodb/update-user.test.ts | 85 +++++++++++++++---- 2 files changed, 98 insertions(+), 18 deletions(-) diff --git a/backend/api/src/handlers/dynamodb/update-user.ts b/backend/api/src/handlers/dynamodb/update-user.ts index 3b35d11d2..327f18499 100644 --- a/backend/api/src/handlers/dynamodb/update-user.ts +++ b/backend/api/src/handlers/dynamodb/update-user.ts @@ -1,14 +1,39 @@ +import {APIGatewayEvent} from "aws-lambda"; import DynamoDbClient from "../../dynamodb-client"; -import {handlerInvokeEvent} from "../handler-utils"; +import {validateAuthorisationHeader} from "../helper/validate-authorisation-header"; const client = new DynamoDbClient(); -export const updateUserHandler = async (event: handlerInvokeEvent): Promise<{statusCode: number; body: string}> => { +export const updateUserHandler = async (event: APIGatewayEvent): Promise<{statusCode: number; body: string}> => { const body = JSON.parse(event.body as string); const response = {statusCode: 200, body: JSON.stringify("OK")}; + const userId = body.userId; + + if (!userId) { + return { + statusCode: 400, + body: "No userId provided in request body" + }; + } + + const authHeader = event.headers.Authorization; + const authorisationHeaderValidation = validateAuthorisationHeader(authHeader); + + if (!authorisationHeaderValidation.valid) { + return authorisationHeaderValidation.errorResponse; + } + + const userIdWithoutPrefix = userId.includes("user#") ? userId.substring("user#".length) : userId; + + if (userIdWithoutPrefix !== authorisationHeaderValidation.userId) { + return { + statusCode: 403, + body: "Forbidden" + }; + } await client - .updateUser(body.userId, body.cognitoUserId, body.updates) + .updateUser(body.userId, body.updates) .then(updateItemCommandOutput => { response.statusCode = 200; response.body = JSON.stringify(updateItemCommandOutput); diff --git a/backend/api/tests/handlers/dynamodb/update-user.test.ts b/backend/api/tests/handlers/dynamodb/update-user.test.ts index c00bebef6..5f106246a 100644 --- a/backend/api/tests/handlers/dynamodb/update-user.test.ts +++ b/backend/api/tests/handlers/dynamodb/update-user.test.ts @@ -1,34 +1,83 @@ import {updateUserHandler} from "../../../src/handlers/dynamodb/update-user"; import DynamoDbClient from "../../../src/dynamodb-client"; -import {TEST_COGNITO_USER_ID, TEST_SERVICE_NAME, TEST_USER_ID} from "../constants"; +import {TEST_ACCESS_TOKEN, TEST_COGNITO_USER_ID, TEST_SERVICE_NAME, TEST_USER_ID, TEST_USER_PHONE_NUMBER} from "../constants"; import {UpdateItemCommandOutput} from "@aws-sdk/client-dynamodb"; -import {handlerInvokeEvent} from "../../../src/handlers/handler-utils"; +import {constructTestApiGatewayEvent} from "../utils"; const TEST_USER_UPDATES = { userId: TEST_USER_ID, - cognitoUserId: TEST_COGNITO_USER_ID, updates: { - serviceName: TEST_SERVICE_NAME + phone: TEST_USER_PHONE_NUMBER } }; - -const TEST_UPDATE_USER_EVENT: handlerInvokeEvent = { - statusCode: 200, - body: JSON.stringify(TEST_USER_UPDATES) -}; - -describe("handlerName tests", () => { +describe("updateUserHandler tests", () => { beforeEach(() => { jest.clearAllMocks(); }); + it("returns a 400 for no UserId in the update body", async () => { + const updateUserResponse: UpdateItemCommandOutput = {$metadata: {httpStatusCode: 200}}; + const updateUserSpy = jest.spyOn(DynamoDbClient.prototype, "updateUser").mockResolvedValue(updateUserResponse); + + const response = await updateUserHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify({ + cognitoUserId: TEST_COGNITO_USER_ID, + updates: { + serviceName: TEST_SERVICE_NAME + } + }), + pathParameters: {} + }) + ); + + expect(updateUserSpy).not.toHaveBeenCalled(); + expect(response).toStrictEqual({ + statusCode: 400, + body: "No userId provided in request body" + }); + }); + + it("returns a 403 if the userId in the request body does not match the access token", async () => { + const updateUserResponse: UpdateItemCommandOutput = {$metadata: {httpStatusCode: 200}}; + const updateUserSpy = jest.spyOn(DynamoDbClient.prototype, "updateUser").mockResolvedValue(updateUserResponse); + + const response = await updateUserHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify({ + cognitoUserId: TEST_COGNITO_USER_ID, + updates: { + serviceId: "aDifferentUserId", + serviceName: TEST_SERVICE_NAME + } + }), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }) + ); + + expect(updateUserSpy).not.toHaveBeenCalled(); + expect(response).toStrictEqual({ + statusCode: 400, + body: "No userId provided in request body" + }); + }); + it("returns a 200 and the stringified record in the response when the update is successful", async () => { const updateUserResponse: UpdateItemCommandOutput = {$metadata: {httpStatusCode: 200}}; const updateUserSpy = jest.spyOn(DynamoDbClient.prototype, "updateUser").mockResolvedValue(updateUserResponse); - const response = await updateUserHandler(TEST_UPDATE_USER_EVENT); + const response = await updateUserHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify(TEST_USER_UPDATES), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }) + ); - expect(updateUserSpy).toHaveBeenCalledWith(TEST_USER_UPDATES.userId, TEST_USER_UPDATES.cognitoUserId, TEST_USER_UPDATES.updates); + expect(updateUserSpy).toHaveBeenCalledWith(TEST_USER_ID, { + phone: TEST_USER_PHONE_NUMBER + }); expect(response).toStrictEqual({ statusCode: 200, body: JSON.stringify(updateUserResponse) @@ -39,9 +88,15 @@ describe("handlerName tests", () => { const dynamoErr = "SomeDynamoErr"; const updateUserSpy = jest.spyOn(DynamoDbClient.prototype, "updateUser").mockRejectedValue(dynamoErr); - const response = await updateUserHandler(TEST_UPDATE_USER_EVENT); + const response = await updateUserHandler( + constructTestApiGatewayEvent({ + body: JSON.stringify(TEST_USER_UPDATES), + pathParameters: {}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }) + ); - expect(updateUserSpy).toHaveBeenCalledWith(TEST_USER_UPDATES.userId, TEST_USER_UPDATES.cognitoUserId, TEST_USER_UPDATES.updates); + expect(updateUserSpy).toHaveBeenCalledWith(TEST_USER_UPDATES.userId, TEST_USER_UPDATES.updates); expect(response).toStrictEqual({ statusCode: 500, body: JSON.stringify(dynamoErr) From ffb21b3fd0f5287f821fe67a4a9d8c03c0e1d909 Mon Sep 17 00:00:00 2001 From: Ryan Andrews Date: Tue, 17 Sep 2024 11:05:40 +0100 Subject: [PATCH 10/11] INCIDEN-922: Adds access control to the getService handler [deploy] --- .../api/src/handlers/dynamodb/get-services.ts | 17 ++++++ .../handlers/dynamodb/get-services.test.ts | 55 ++++++++++++++----- 2 files changed, 59 insertions(+), 13 deletions(-) diff --git a/backend/api/src/handlers/dynamodb/get-services.ts b/backend/api/src/handlers/dynamodb/get-services.ts index 9753c7f1b..f71b00af2 100644 --- a/backend/api/src/handlers/dynamodb/get-services.ts +++ b/backend/api/src/handlers/dynamodb/get-services.ts @@ -1,5 +1,6 @@ import {APIGatewayProxyEvent, APIGatewayProxyResult} from "aws-lambda"; import DynamoDbClient from "../../dynamodb-client"; +import {validateAuthorisationHeader} from "../helper/validate-authorisation-header"; const client = new DynamoDbClient(); @@ -9,6 +10,22 @@ export const getServicesHandler = async (event: APIGatewayProxyEvent): Promise { beforeEach(() => { jest.clearAllMocks(); }); + it("returns a 400 when there is no userId Path parameter and does not call the dynamo client", async () => { + const serviceHandlerResponse = await getServicesHandler(constructTestApiGatewayEvent()); + expect(serviceHandlerResponse).toStrictEqual({ + statusCode: 400, + body: JSON.stringify("No userId request parameter supplied") + }); + }); + + it("returns a 403 if the userId in the pathParams does not match the access token", async () => { + const mockDynamoResponse: GetItemCommandOutput = { + $metadata: {}, + Item: TEST_DATA_TABLE_ITEM + }; + const getServicesSpy = jest.spyOn(DynamoDbClient.prototype, "getServices").mockResolvedValue(mockDynamoResponse); + + const testApiGatewayEvent = constructTestApiGatewayEvent({ + body: "", + pathParameters: {userId: "aDifferentUserId"}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }); + const serviceHandlerResponse = await getServicesHandler(testApiGatewayEvent); + + expect(getServicesSpy).not.toHaveBeenCalled(); + expect(serviceHandlerResponse).toStrictEqual({ + statusCode: 403, + body: "Forbidden" + }); + }); + it("calls the dynamo client with a get command with the expected values and returns a 200 with the expected response body", async () => { const mockDynamoResponse: GetItemCommandOutput = { - $metadata: {} + $metadata: {}, + Item: TEST_DATA_TABLE_ITEM }; const getServicesSpy = jest.spyOn(DynamoDbClient.prototype, "getServices").mockResolvedValue(mockDynamoResponse); - const testApiGatewayEvent = constructTestApiGatewayEvent({body: "", pathParameters: {userId: TEST_USER_ID}}); + const testApiGatewayEvent = constructTestApiGatewayEvent({ + body: "", + pathParameters: {userId: TEST_USER_ID}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }); const serviceHandlerResponse = await getServicesHandler(testApiGatewayEvent); expect(getServicesSpy).toHaveBeenCalledWith(TEST_USER_ID); @@ -30,7 +64,11 @@ describe("getServicesHandler tests", () => { const error = "SomeAwsError"; const getServicesSpy = jest.spyOn(DynamoDbClient.prototype, "getServices").mockRejectedValue(error); - const testApiGatewayEvent = constructTestApiGatewayEvent({body: "", pathParameters: {userId: TEST_USER_ID}}); + const testApiGatewayEvent = constructTestApiGatewayEvent({ + body: "", + pathParameters: {userId: TEST_USER_ID}, + headers: {Authorization: `Bearer ${TEST_ACCESS_TOKEN}`} + }); const serviceHandlerResponse = await getServicesHandler(testApiGatewayEvent); expect(getServicesSpy).toHaveBeenCalledWith(TEST_USER_ID); @@ -39,13 +77,4 @@ describe("getServicesHandler tests", () => { body: JSON.stringify(error) }); }); - - it("returns a 400 when there is no userId Path parameter and does not call the dynamo client", async () => { - const error = "No userId request parameter supplied"; - const serviceHandlerResponse = await getServicesHandler(constructTestApiGatewayEvent()); - expect(serviceHandlerResponse).toStrictEqual({ - statusCode: 400, - body: JSON.stringify(error) - }); - }); }); From ef5c25a6498ba70b6f46dfa7eff7816ebaa302f3 Mon Sep 17 00:00:00 2001 From: Ryan Andrews Date: Wed, 18 Sep 2024 11:35:13 +0100 Subject: [PATCH 11/11] INCIDEN-922: Pass access token to delete entries methods [deploy] We're now expecting an access token on the backend, so we need to start passing it through here not to break any existing functionality. --- express/src/controllers/register.ts | 4 ++-- .../src/services/lambda-facade/LambdaFacade.ts | 8 ++++---- .../lambda-facade/LambdaFacadeInterface.ts | 4 ++-- .../services/self-service-services-service.ts | 14 +++++++------- express/tests/controllers/register.test.ts | 3 ++- .../lambda-facade/LambdaFacade.test.ts | 18 ++++++++++++++---- 6 files changed, 31 insertions(+), 20 deletions(-) diff --git a/express/src/controllers/register.ts b/express/src/controllers/register.ts index c8c42890c..40313b092 100644 --- a/express/src/controllers/register.ts +++ b/express/src/controllers/register.ts @@ -344,7 +344,7 @@ export const processAddServiceForm: RequestHandler = async (req, res, next) => { let serviceId = ""; let body; - + const accessToken = nonNull(req.session.authenticationResult?.AccessToken); try { console.info("Generating Client to Service:" + service.serviceName); const generatedClient = await s4.generateClient(service, nonNull(req.session.authenticationResult)); @@ -353,7 +353,7 @@ export const processAddServiceForm: RequestHandler = async (req, res, next) => { } catch (error) { console.error("Unable to Register Client to Service - Service Items removed"); console.error(error); - await s4.deleteServiceEntries(uuid); + await s4.deleteServiceEntries(uuid, accessToken); return res.render("there-is-a-problem.njk"); } diff --git a/express/src/services/lambda-facade/LambdaFacade.ts b/express/src/services/lambda-facade/LambdaFacade.ts index 32f8baa13..e3bf550d5 100644 --- a/express/src/services/lambda-facade/LambdaFacade.ts +++ b/express/src/services/lambda-facade/LambdaFacade.ts @@ -153,7 +153,7 @@ export default class LambdaFacade implements LambdaFacadeInterface { } } - async deleteClientEntries(userID: string, serviceID: string): Promise { + async deleteClientEntries(userID: string, serviceID: string, accessToken: string): Promise { console.log("In LambdaFacade-deleteClientEntries"); const body = { @@ -162,14 +162,14 @@ export default class LambdaFacade implements LambdaFacadeInterface { }; try { - await this.post("/delete-dynamodb-client-entries/", JSON.stringify(body)); + await this.post("/delete-dynamodb-client-entries/", JSON.stringify(body), accessToken); } catch (error) { console.error(error as Error); throw error; } } - async deleteServiceEntries(serviceID: string): Promise { + async deleteServiceEntries(serviceID: string, accessToken: string): Promise { console.log("In LambdaFacade-deleteServiceEntries"); const body = { @@ -177,7 +177,7 @@ export default class LambdaFacade implements LambdaFacadeInterface { }; try { - await this.post("/delete-dynamodb-service-entries/", JSON.stringify(body)); + await this.post("/delete-dynamodb-service-entries/", JSON.stringify(body), accessToken); } catch (error) { console.error(error as Error); throw error; diff --git a/express/src/services/lambda-facade/LambdaFacadeInterface.ts b/express/src/services/lambda-facade/LambdaFacadeInterface.ts index babcb311b..e06bc40e1 100644 --- a/express/src/services/lambda-facade/LambdaFacadeInterface.ts +++ b/express/src/services/lambda-facade/LambdaFacadeInterface.ts @@ -49,6 +49,6 @@ export default interface LambdaFacadeInterface { getDynamoDBEntries(email: string): Promise; - deleteClientEntries(oldUserID: string, serviceID: string): Promise; - deleteServiceEntries(serviceID: string): Promise; + deleteClientEntries(oldUserID: string, serviceID: string, accessToken: string): Promise; + deleteServiceEntries(serviceID: string, accessToken: string): Promise; } diff --git a/express/src/services/self-service-services-service.ts b/express/src/services/self-service-services-service.ts index 3fe0bcfa1..5dfe3d18c 100644 --- a/express/src/services/self-service-services-service.ts +++ b/express/src/services/self-service-services-service.ts @@ -362,16 +362,15 @@ export default class SelfServiceServicesService { async recreateDynamoDBAccountLinks(authenticationResult: AuthenticationResultType, oldUserID: string) { console.info("In self-service-services-service:createNewDynamoDBAccountLinks()"); + const accessToken = nonNull(authenticationResult.AccessToken); + await this.validateToken(accessToken, "recreateDynamoDBAccountLinks"); - const serviceListToReplicate = dynamoServicesToDomainServices( - (await this.lambda.listServices(oldUserID, nonNull(authenticationResult.AccessToken))).data.Items - ); + const serviceListToReplicate = dynamoServicesToDomainServices((await this.lambda.listServices(oldUserID, accessToken)).data.Items); for (const serviceItem of serviceListToReplicate) { const serviceID: string = serviceItem.id; const serviceName: string = serviceItem.serviceName; const currentUserID = AuthenticationResultParser.getCognitoId(authenticationResult); - const accessToken: string = authenticationResult.AccessToken as string; const emailAddress = AuthenticationResultParser.getEmail(authenticationResult); const mobileNumber = AuthenticationResultParser.getPhoneNumber(authenticationResult); @@ -392,13 +391,14 @@ export default class SelfServiceServicesService { await this.putUser(dynamoUser, accessToken); await this.newService(service, currentUserID, authenticationResult); - await this.lambda.deleteClientEntries(oldUserID, serviceID); + await this.lambda.deleteClientEntries(oldUserID, serviceID, accessToken); } } - async deleteServiceEntries(serviceID: string) { + async deleteServiceEntries(serviceID: string, accessToken: string) { console.info("In self-service-services-service:deleteServiceEntries()"); console.log("Service ID => " + serviceID); - await this.lambda.deleteServiceEntries(serviceID); + await this.validateToken(accessToken, "deleteServiceEntries"); + await this.lambda.deleteServiceEntries(serviceID, accessToken); } } diff --git a/express/tests/controllers/register.test.ts b/express/tests/controllers/register.test.ts index 62d2af687..c5505977d 100644 --- a/express/tests/controllers/register.test.ts +++ b/express/tests/controllers/register.test.ts @@ -18,6 +18,7 @@ import { } from "../../src/controllers/register"; import {request, response} from "../mocks"; import { + TEST_ACCESS_TOKEN, TEST_AUTHENTICATION_RESULT, TEST_COGNITO_ID, TEST_COGNITO_SESSION_STRING, @@ -792,7 +793,7 @@ describe("processAddServiceForm controller tests", () => { {id: `service#${TEST_UUID}`, serviceName: TEST_SERVICE_NAME}, TEST_AUTHENTICATION_RESULT ); - expect(s4DeleteServiceEntriesSpy).toHaveBeenCalledWith(TEST_UUID); + expect(s4DeleteServiceEntriesSpy).toHaveBeenCalledWith(TEST_UUID, TEST_ACCESS_TOKEN); expect(mockRes.render).toHaveBeenCalledWith("there-is-a-problem.njk"); }); diff --git a/express/tests/services/lambda-facade/LambdaFacade.test.ts b/express/tests/services/lambda-facade/LambdaFacade.test.ts index 84e054f4f..5c2a2f376 100644 --- a/express/tests/services/lambda-facade/LambdaFacade.test.ts +++ b/express/tests/services/lambda-facade/LambdaFacade.test.ts @@ -152,26 +152,36 @@ describe("Lambda Facade class tests", () => { it("POSTs the /delete-dynamodb-client-entries when the deleteClientEntries method is called", async () => { const mockLambdaFacade = new LambdaFacade(); - await mockLambdaFacade.deleteClientEntries(TEST_COGNITO_ID, TEST_SERVICE_ID); + await mockLambdaFacade.deleteClientEntries(TEST_COGNITO_ID, TEST_SERVICE_ID, TEST_ACCESS_TOKEN); expect(mockPost).toHaveBeenCalledWith( "/delete-dynamodb-client-entries/", JSON.stringify({ userId: TEST_COGNITO_ID, serviceId: TEST_SERVICE_ID - }) + }), + { + headers: { + Authorization: `Bearer ${TEST_ACCESS_TOKEN}` + } + } ); }); it("POSTs the /delete-dynamodb-service-entries when the deleteServiceEntries method is called", async () => { const mockLambdaFacade = new LambdaFacade(); - await mockLambdaFacade.deleteServiceEntries(TEST_SERVICE_ID); + await mockLambdaFacade.deleteServiceEntries(TEST_SERVICE_ID, TEST_ACCESS_TOKEN); expect(mockPost).toHaveBeenCalledWith( "/delete-dynamodb-service-entries/", JSON.stringify({ serviceId: TEST_SERVICE_ID - }) + }), + { + headers: { + Authorization: `Bearer ${TEST_ACCESS_TOKEN}` + } + } ); });