Skip to content

Commit

Permalink
Merge pull request #783 from techmatters/CHI-3099
Browse files Browse the repository at this point in the history 
CHI-3099: Fix in progress contact permission check
  • Loading branch information
@stephenhand
stephenhand authored Nov 28, 2024
2 parents 5ed2987 + 754bd33 commit 1c7aeb3
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 34 deletions.
15 changes: 6 additions & 9 deletions hrm-domain/hrm-core/contact/canPerformContactAction.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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();
Expand All @@ -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,
Expand Down Expand Up @@ -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,
);
Expand Down
2 changes: 0 additions & 2 deletions hrm-domain/hrm-core/contact/contactRoutesV0.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import {
canChangeContactConnection,
canDisconnectContact,
canPerformEditContactAction,
canPerformEditInProgressContactAction,
canPerformViewContactAction,
} from './canPerformContactAction';

Expand Down Expand Up @@ -174,7 +173,6 @@ contactsRouter.patch(
'/:contactId',
validatePatchPayload,
canPerformEditContactAction,
canPerformEditInProgressContactAction,
async (req, res) => {
const { hrmAccountId, user } = req;
const { contactId } = req.params;
Expand Down
98 changes: 75 additions & 23 deletions hrm-domain/hrm-core/unit-tests/contact/canPerformContactAction.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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({}),
Expand All @@ -50,20 +51,35 @@ const accountSid1 = 'ACtwilio-hrm1';
const thisWorkerSid = 'WK-thisWorker';
const otherWorkerSid = 'WK-otherWorker';

let req: any;
let req: Partial<SafeRouterRequest>;
let mockRequestMethods: {
permit: jest.MockedFunction<SafeRouterRequest['permit']>;
isPermitted: jest.MockedFunction<SafeRouterRequest['isPermitted']>;
block: jest.MockedFunction<SafeRouterRequest['block']>;
can: jest.MockedFunction<SafeRouterRequest['can']>;
};
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<SafeRouterRequest>;
next.mockClear();
mockGetContactById.mockClear();
mockCreateError.mockClear();
Expand Down Expand Up @@ -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();
});
Expand Down Expand Up @@ -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()
Expand All @@ -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(),
);
Expand Down Expand Up @@ -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();
});
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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);
Expand All @@ -335,16 +385,18 @@ describe('canDisconnectContact', () => {
describe(
'can update case contacts',
draftContactTests(true, async () => {
req.can.mockImplementation(
mockRequestMethods.can.mockImplementation(
(user, action) => action === actionsMaps.case.UPDATE_CASE_CONTACTS,
);
}),
);
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,
);
}),
);
Expand Down

0 comments on commit 1c7aeb3

Please sign in to comment.