From f56c8ad1566159964abbeaf0181e30bdf76125e3 Mon Sep 17 00:00:00 2001 From: Stephen Hand Date: Wed, 27 Nov 2024 13:41:19 +0000 Subject: [PATCH 1/2] Fix in progress contact permission check --- .../hrm-core/contact/canPerformContactAction.ts | 15 ++++++--------- hrm-domain/hrm-core/contact/contactRoutesV0.ts | 2 -- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/hrm-domain/hrm-core/contact/canPerformContactAction.ts b/hrm-domain/hrm-core/contact/canPerformContactAction.ts index 756eddc7..49aeda9f 100644 --- a/hrm-domain/hrm-core/contact/canPerformContactAction.ts +++ b/hrm-domain/hrm-core/contact/canPerformContactAction.ts @@ -58,7 +58,7 @@ const canPerformActionOnContact = ( // This is a dirty hack that relies on the catch block in the try/catch below to return a 404 throw new Error('contact not found'); } - if (contactObj.finalizedAt || action !== 'editContact') { + if (contactObj.finalizedAt || action !== actionsMaps.contact.EDIT_CONTACT) { if (can(user, action, contactObj)) { await permitIfAdditionalValidationPasses( req, @@ -70,7 +70,7 @@ const canPerformActionOnContact = ( console.debug( `[Permission - BLOCKED] User ${user.workerSid} is not permitted to perform ${action} on contact ${hrmAccountId}/${contactId} - rules failure`, ); - if (action === 'viewContact') { + if (action === actionsMaps.contact.VIEW_CONTACT) { throw createError(404); } else { req.block(); @@ -88,12 +88,14 @@ const canPerformActionOnContact = ( ); req.block(); } - // If there is no finalized date, then the contact is a draft and can only be edited by the worker who created it or the one who owns it. + // If there is no finalized date, then the contact is a draft and can only be edited by the worker who created it or the one who owns it, or those with EDIT_INPROGRESS_CONTACT permission. + // Should we remove the hardcoded checks now we have the permission? Without the hardcoded allows, an incorrect permission config could break Aselo // Offline contacts potentially need to be edited by a creator that won't own them. // Transferred tasks need to be edited by an owner that didn't create them. if ( contactObj.createdBy === user.workerSid || - contactObj.twilioWorkerId === user.workerSid + contactObj.twilioWorkerId === user.workerSid || + can(user, actionsMaps.contact.EDIT_INPROGRESS_CONTACT, contactObj) ) { await permitIfAdditionalValidationPasses( req, @@ -158,11 +160,6 @@ export const canPerformEditContactAction = canPerformActionOnContact( checkFinalizedContactEditsOnlyChangeForm, ); -export const canPerformEditInProgressContactAction = canPerformActionOnContact( - actionsMaps.contact.EDIT_INPROGRESS_CONTACT, - checkFinalizedContactEditsOnlyChangeForm, -); - export const canPerformViewContactAction = canPerformActionOnContact( actionsMaps.contact.VIEW_CONTACT, ); diff --git a/hrm-domain/hrm-core/contact/contactRoutesV0.ts b/hrm-domain/hrm-core/contact/contactRoutesV0.ts index 6a323f52..5ec4d222 100644 --- a/hrm-domain/hrm-core/contact/contactRoutesV0.ts +++ b/hrm-domain/hrm-core/contact/contactRoutesV0.ts @@ -32,7 +32,6 @@ import { canChangeContactConnection, canDisconnectContact, canPerformEditContactAction, - canPerformEditInProgressContactAction, canPerformViewContactAction, } from './canPerformContactAction'; @@ -174,7 +173,6 @@ contactsRouter.patch( '/:contactId', validatePatchPayload, canPerformEditContactAction, - canPerformEditInProgressContactAction, async (req, res) => { const { hrmAccountId, user } = req; const { contactId } = req.params; From 754bd33dde286e90d27922f83fb9eefd444144a2 Mon Sep 17 00:00:00 2001 From: Stephen Hand Date: Wed, 27 Nov 2024 17:00:34 +0000 Subject: [PATCH 2/2] Add tests around EDIT_INPROGRESS_CONTACT cases --- .../contact/canPerformContactAction.test.ts | 98 ++++++++++++++----- 1 file changed, 75 insertions(+), 23 deletions(-) diff --git a/hrm-domain/hrm-core/unit-tests/contact/canPerformContactAction.test.ts b/hrm-domain/hrm-core/unit-tests/contact/canPerformContactAction.test.ts index 7327923b..e6c1dde1 100644 --- a/hrm-domain/hrm-core/unit-tests/contact/canPerformContactAction.test.ts +++ b/hrm-domain/hrm-core/unit-tests/contact/canPerformContactAction.test.ts @@ -26,6 +26,7 @@ import { } from '../../contact/canPerformContactAction'; import { CaseService, getCase } from '../../case/caseService'; import { actionsMaps } from '../../permissions'; +import { SafeRouterRequest } from '../../permissions/safe-router'; jest.mock('@tech-matters/twilio-client', () => ({ getClient: jest.fn().mockResolvedValue({}), @@ -50,20 +51,35 @@ const accountSid1 = 'ACtwilio-hrm1'; const thisWorkerSid = 'WK-thisWorker'; const otherWorkerSid = 'WK-otherWorker'; -let req: any; +let req: Partial; +let mockRequestMethods: { + permit: jest.MockedFunction; + isPermitted: jest.MockedFunction; + block: jest.MockedFunction; + can: jest.MockedFunction; +}; const next = jest.fn(); beforeEach(() => { - req = { - isPermitted: jest.fn().mockReturnValue(false), - params: { contactId: 'contact1' }, + mockRequestMethods = { permit: jest.fn(), block: jest.fn(), can: jest.fn(), - user: { workerSid: 'WK-worker1', accountSid: 'ACtwilio' }, + isPermitted: jest.fn().mockReturnValue(false), + }; + + req = { + ...mockRequestMethods, + params: { contactId: 'contact1' }, + user: { + workerSid: 'WK-worker1', + accountSid: 'ACtwilio', + isSupervisor: false, + roles: [], + }, hrmAccountId: accountSid1, body: {}, - }; + } as Partial; next.mockClear(); mockGetContactById.mockClear(); mockCreateError.mockClear(); @@ -98,9 +114,9 @@ const draftContactTests = const expectation = expectToPermit ? expectToBePermitted : expectToBeBlocked; beforeEach(async () => { // Draft contact authorization doesn't care about the can response, so always return false - req.can.mockReturnValue(false); + mockRequestMethods.can.mockReturnValue(false); req.body = { conversationDuration: 123 }; - req.user = { accountSid: 'ACtwilio', workerSid: thisWorkerSid }; + req.user = { ...req.user, accountSid: 'ACtwilio', workerSid: thisWorkerSid }; process.env.TWILIO_AUTH_TOKEN_ACtwilio = 'account1 token'; await setup(); }); @@ -151,6 +167,28 @@ const draftContactTests = expectation(); }); + test(`Request user is not the owner or the creator, but has EDIT_INPROGRESS_CONTACT - ${expectedDescription}`, async () => { + mockGetContactById.mockResolvedValue( + new ContactBuilder() + .withCreatedBy(otherWorkerSid) + .withTwilioWorkerId(otherWorkerSid) + .withTaskId('original task') + .build(), + ); + req.body.taskId = 'transfer task'; + mockRequestMethods.can.mockImplementation( + (user, action) => action === actionsMaps.contact.EDIT_INPROGRESS_CONTACT, + ); + mockIsTwilioTaskTransferTarget.mockResolvedValue(false); + await canPerformEditContactAction(req, {}, next); + expect(getClient).toHaveBeenCalledWith({ + accountSid: 'ACtwilio', + authToken: 'account1 token', + }); + + expectation(); + }); + test('Request user is not the owner or the creator, nor target of a transfer - blocks', async () => { mockGetContactById.mockResolvedValue( new ContactBuilder() @@ -175,15 +213,16 @@ const draftContactTests = expectToBeBlocked(); }); }; + describe('canPerformEditContactAction', () => { test("Request is already permitted - doesn't permit or block", async () => { - req.isPermitted.mockReturnValue(true); + mockRequestMethods.isPermitted.mockReturnValue(true); await canPerformEditContactAction(req, {}, next); expectNoop(); }); test('Request is not already permitted - looks up contact using contactID parameter', async () => { - req.can.mockReturnValue(false); + mockRequestMethods.can.mockReturnValue(false); mockGetContactById.mockResolvedValue( new ContactBuilder().withFinalizedAt(BASELINE_DATE).build(), ); @@ -220,31 +259,42 @@ describe('canPerformEditContactAction', () => { test('Modifying rawJson / resource referrals and can returns true - permits', async () => { req.body = validFinalizedContactPatchPayload; - req.can.mockReturnValue(true); + mockRequestMethods.can.mockReturnValue(true); await canPerformEditContactAction(req, {}, next); expectToBePermitted(); }); test('Modifying values other than rawJson / resource referrals and can returns true - blocks', async () => { req.body = { ...validFinalizedContactPatchPayload, conversationDuration: 100 }; - req.can.mockReturnValue(true); + mockRequestMethods.can.mockReturnValue(true); await canPerformEditContactAction(req, {}, next); expectToBeBlocked(); }); test('Modifying rawJson / resource referrals and can returns false - blocks', async () => { req.body = validFinalizedContactPatchPayload; - req.can.mockReturnValue(false); + mockRequestMethods.can.mockReturnValue(false); await canPerformEditContactAction(req, {}, next); expectToBeBlocked(); }); + + test('Modifying rawJson / resource referrals and can returns false but EDIT_INPROGRESS_CAN is permitted - blocks', async () => { + req.body = validFinalizedContactPatchPayload; + mockRequestMethods.can.mockImplementation( + (user, action) => action === actionsMaps.contact.EDIT_INPROGRESS_CONTACT, + ); + await canPerformEditContactAction(req, {}, next); + expectToBeBlocked(); + }); + }); + describe('draft contact', () => { + draftContactTests(true); }); - describe('draft contact', draftContactTests(true)); }); describe('canDisconnectContact', () => { test('Request is already permitted - skips authorization', async () => { - req.isPermitted.mockReturnValue(true); + mockRequestMethods.isPermitted.mockReturnValue(true); await canDisconnectContact(req, {}, next); expectNoop(); }); @@ -257,7 +307,7 @@ describe('canDisconnectContact', () => { }); test('can returns true to permit & case id not set on contact - permits', async () => { delete contact.caseId; - req.can.mockImplementation( + mockRequestMethods.can.mockImplementation( (user, action) => action === actionsMaps.contact.REMOVE_CONTACT_FROM_CASE, ); await canDisconnectContact(req, {}, next); @@ -271,7 +321,7 @@ describe('canDisconnectContact', () => { test('can returns true to permit & case not found to disconnect from - permits', async () => { mockGetCase.mockResolvedValue(undefined); - req.can.mockImplementation( + mockRequestMethods.can.mockImplementation( (user, action) => action === actionsMaps.contact.REMOVE_CONTACT_FROM_CASE, ); await canDisconnectContact(req, {}, next); @@ -285,7 +335,7 @@ describe('canDisconnectContact', () => { test('Can returns true for contact and case checks - permits', async () => { const mockCase = {} as CaseService; mockGetCase.mockResolvedValue(mockCase); - req.can.mockReturnValue(true); + mockRequestMethods.can.mockReturnValue(true); await canDisconnectContact(req, {}, next); expect(req.can).toHaveBeenCalledWith( req.user, @@ -302,7 +352,7 @@ describe('canDisconnectContact', () => { test('Can returns false - blocks', async () => { const mockCase = {} as CaseService; mockGetCase.mockResolvedValue(mockCase); - req.can.mockReturnValue(false); + mockRequestMethods.can.mockReturnValue(false); await canDisconnectContact(req, {}, next); expect(req.can).toHaveBeenCalledWith( req.user, @@ -314,7 +364,7 @@ describe('canDisconnectContact', () => { test('Can returns true for contact but false for case - blocks', async () => { const mockCase = {} as CaseService; mockGetCase.mockResolvedValue(mockCase); - req.can.mockImplementation( + mockRequestMethods.can.mockImplementation( (user, action) => action === actionsMaps.contact.REMOVE_CONTACT_FROM_CASE, ); await canDisconnectContact(req, {}, next); @@ -335,7 +385,7 @@ describe('canDisconnectContact', () => { describe( 'can update case contacts', draftContactTests(true, async () => { - req.can.mockImplementation( + mockRequestMethods.can.mockImplementation( (user, action) => action === actionsMaps.case.UPDATE_CASE_CONTACTS, ); }), @@ -343,8 +393,10 @@ describe('canDisconnectContact', () => { describe( 'cannot update case contacts', draftContactTests(true, async () => { - req.can.mockImplementation( - (user, action) => action !== actionsMaps.case.UPDATE_CASE_CONTACTS, + mockRequestMethods.can.mockImplementation( + (user, action) => + action !== actionsMaps.case.UPDATE_CASE_CONTACTS && + action !== actionsMaps.contact.EDIT_INPROGRESS_CONTACT, ); }), );