Skip to content

Commit

Permalink
fix(#9065): block offline users from using api/v1/person endpoint (#9205
Browse files Browse the repository at this point in the history 
)
  • Loading branch information
@jkuester
jkuester authored Jun 25, 2024
1 parent 8ce2fd0 commit 8ed110d
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 79 deletions.
5 changes: 4 additions & 1 deletion api/src/controllers/person.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ const getPerson = ({ with_lineage }) => ctx.bind(
module.exports = {
v1: {
get: serverUtils.doOrError(async (req, res) => {
await auth.check(req, 'can_view_contacts');
const userCtx = await auth.getUserCtx(req);
if (!auth.isOnlineOnly(userCtx) || !auth.hasAllPermissions(userCtx, 'can_view_contacts')) {
return Promise.reject({ code: 403, message: 'Insufficient privileges' });
}
const { uuid } = req.params;
const person = await getPerson(req.query)(Qualifier.byUuid(uuid));
if (!person) {
Expand Down
56 changes: 46 additions & 10 deletions api/tests/mocha/controllers/person.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,21 @@ const dataContext = require('../../../src/services/data-context');
const serverUtils = require('../../../src/server-utils');

describe('Person Controller', () => {
let authCheck;
const userCtx = { hello: 'world' };
let getUserCtx;
let isOnlineOnly;
let hasAllPermissions;
let dataContextBind;
let serverUtilsError;
let req;
let res;

beforeEach(() => {
authCheck = sinon.stub(auth, 'check');
getUserCtx = sinon
.stub(auth, 'getUserCtx')
.resolves(userCtx);
isOnlineOnly = sinon.stub(auth, 'isOnlineOnly');
hasAllPermissions = sinon.stub(auth, 'hasAllPermissions');
dataContextBind = sinon.stub(dataContext, 'bind');
serverUtilsError = sinon.stub(serverUtils, 'error');
res = {
Expand Down Expand Up @@ -49,13 +56,20 @@ describe('Person Controller', () => {
.returns(personGetWithLineage);
});

afterEach(() => {
expect(getUserCtx.calledOnceWithExactly(req)).to.be.true;
expect(isOnlineOnly.calledOnceWithExactly(userCtx)).to.be.true;
});

it('returns a person', async () => {
isOnlineOnly.returns(true);
hasAllPermissions.returns(true);
const person = { name: 'John Doe' };
personGet.resolves(person);

await controller.v1.get(req, res);

expect(authCheck.calledOnceWithExactly(req, 'can_view_contacts')).to.be.true;
expect(hasAllPermissions.calledOnceWithExactly(userCtx, 'can_view_contacts')).to.be.true;
expect(dataContextBind.calledOnceWithExactly(Person.v1.get)).to.be.true;
expect(byUuid.calledOnceWithExactly(req.params.uuid)).to.be.true;
expect(personGet.calledOnceWithExactly(qualifier)).to.be.true;
Expand All @@ -65,13 +79,15 @@ describe('Person Controller', () => {
});

it('returns a person with lineage when the query parameter is set to "true"', async () => {
isOnlineOnly.returns(true);
hasAllPermissions.returns(true);
const person = { name: 'John Doe' };
personGetWithLineage.resolves(person);
req.query.with_lineage = 'true';

await controller.v1.get(req, res);

expect(authCheck.calledOnceWithExactly(req, 'can_view_contacts')).to.be.true;
expect(hasAllPermissions.calledOnceWithExactly(userCtx, 'can_view_contacts')).to.be.true;
expect(dataContextBind.calledOnceWithExactly(Person.v1.getWithLineage)).to.be.true;
expect(byUuid.calledOnceWithExactly(req.params.uuid)).to.be.true;
expect(personGet.notCalled).to.be.true;
Expand All @@ -81,13 +97,15 @@ describe('Person Controller', () => {
});

it('returns a person without lineage when the query parameter is set something else', async () => {
isOnlineOnly.returns(true);
hasAllPermissions.returns(true);
const person = { name: 'John Doe' };
personGet.resolves(person);
req.query.with_lineage = '1';

await controller.v1.get(req, res);

expect(authCheck.calledOnceWithExactly(req, 'can_view_contacts')).to.be.true;
expect(hasAllPermissions.calledOnceWithExactly(userCtx, 'can_view_contacts')).to.be.true;
expect(dataContextBind.calledOnceWithExactly(Person.v1.get)).to.be.true;
expect(byUuid.calledOnceWithExactly(req.params.uuid)).to.be.true;
expect(personGet.calledOnceWithExactly(qualifier)).to.be.true;
Expand All @@ -97,11 +115,13 @@ describe('Person Controller', () => {
});

it('returns a 404 error if person is not found', async () => {
isOnlineOnly.returns(true);
hasAllPermissions.returns(true);
personGet.resolves(null);

await controller.v1.get(req, res);

expect(authCheck.calledOnceWithExactly(req, 'can_view_contacts')).to.be.true;
expect(hasAllPermissions.calledOnceWithExactly(userCtx, 'can_view_contacts')).to.be.true;
expect(dataContextBind.calledOnceWithExactly(Person.v1.get)).to.be.true;
expect(byUuid.calledOnceWithExactly(req.params.uuid)).to.be.true;
expect(personGet.calledOnceWithExactly(qualifier)).to.be.true;
Expand All @@ -114,13 +134,29 @@ describe('Person Controller', () => {
)).to.be.true;
});

it('returns error if user unauthorized', async () => {
const error = new Error('Unauthorized');
authCheck.rejects(error);
it('returns error if user does not have can_view_contacts permission', async () => {
const error = { code: 403, message: 'Insufficient privileges' };
isOnlineOnly.returns(true);
hasAllPermissions.returns(false);

await controller.v1.get(req, res);

expect(hasAllPermissions.calledOnceWithExactly(userCtx, 'can_view_contacts')).to.be.true;
expect(dataContextBind.notCalled).to.be.true;
expect(byUuid.notCalled).to.be.true;
expect(personGet.notCalled).to.be.true;
expect(personGetWithLineage.notCalled).to.be.true;
expect(res.json.notCalled).to.be.true;
expect(serverUtilsError.calledOnceWithExactly(error, req, res)).to.be.true;
});

it('returns error if not an online user', async () => {
const error = { code: 403, message: 'Insufficient privileges' };
isOnlineOnly.returns(false);

await controller.v1.get(req, res);

expect(authCheck.calledOnceWithExactly(req, 'can_view_contacts')).to.be.true;
expect(hasAllPermissions.notCalled).to.be.true;
expect(dataContextBind.notCalled).to.be.true;
expect(byUuid.notCalled).to.be.true;
expect(personGet.notCalled).to.be.true;
Expand Down
3 changes: 1 addition & 2 deletions shared-libs/cht-datasource/src/local/person.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { UuidQualifier } from '../qualifier';
import * as Person from '../person';
import { getDocById, getDocsByIds } from './libs/doc';
import { LocalDataContext, SettingsService } from './libs/data-context';
import { isNormalizedParent } from '../libs/contact';
import logger from '@medic/logger';
import { getLineageDocsById, getPrimaryContactIds, hydrateLineage, hydratePrimaryContact } from './libs/lineage';

Expand All @@ -17,7 +16,7 @@ export namespace v1 {
return false;
}
const hasPersonType = contactTypeUtils.isPerson(settings.getAll(), doc);
if (!hasPersonType || !isNormalizedParent(doc)) {
if (!hasPersonType) {
logger.warn(`Document [${uuid}] is not a valid person.`);
return false;
}
Expand Down
77 changes: 20 additions & 57 deletions shared-libs/cht-datasource/test/local/person.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import sinon, { SinonStub } from 'sinon';
import contactTypeUtils from '@medic/contact-types-utils';
import logger from '@medic/logger';
import { Doc } from '../../src/libs/doc';
import * as Contact from '../../src/libs/contact';
import * as Person from '../../src/local/person';
import * as LocalDoc from '../../src/local/libs/doc';
import * as Lineage from '../../src/local/libs/lineage';
Expand All @@ -16,7 +15,6 @@ describe('local person', () => {
let warn: SinonStub;
let debug: SinonStub;
let isPerson: SinonStub;
let isNormalizedParent: SinonStub;

beforeEach(() => {
settingsGetAll = sinon.stub();
Expand All @@ -27,7 +25,6 @@ describe('local person', () => {
warn = sinon.stub(logger, 'warn');
debug = sinon.stub(logger, 'debug');
isPerson = sinon.stub(contactTypeUtils, 'isPerson');
isNormalizedParent = sinon.stub(Contact, 'isNormalizedParent');
});

afterEach(() => sinon.restore());
Expand All @@ -50,15 +47,13 @@ describe('local person', () => {
getDocByIdInner.resolves(doc);
settingsGetAll.returns(settings);
isPerson.returns(true);
isNormalizedParent.returns(true);

const result = await Person.v1.get(localContext)(identifier);

expect(result).to.equal(doc);
expect(getDocByIdOuter.calledOnceWithExactly(localContext.medicDb)).to.be.true;
expect(getDocByIdInner.calledOnceWithExactly(identifier.uuid)).to.be.true;
expect(isPerson.calledOnceWithExactly(settings, doc)).to.be.true;
expect(isNormalizedParent.calledOnceWithExactly(doc)).to.be.true;
expect(warn.notCalled).to.be.true;
});

Expand All @@ -74,24 +69,6 @@ describe('local person', () => {
expect(getDocByIdOuter.calledOnceWithExactly(localContext.medicDb)).to.be.true;
expect(getDocByIdInner.calledOnceWithExactly(identifier.uuid)).to.be.true;
expect(isPerson.calledOnceWithExactly(settings, doc)).to.be.true;
expect(isNormalizedParent.notCalled).to.be.true;
expect(warn.calledOnceWithExactly(`Document [${identifier.uuid}] is not a valid person.`)).to.be.true;
});

it('returns null if the identified doc does not have a valid normalized parent lineage', async () => {
const doc = { type: 'not-person' };
getDocByIdInner.resolves(doc);
settingsGetAll.returns(settings);
isPerson.returns(true);
isNormalizedParent.returns(false);

const result = await Person.v1.get(localContext)(identifier);

expect(result).to.be.null;
expect(getDocByIdOuter.calledOnceWithExactly(localContext.medicDb)).to.be.true;
expect(getDocByIdInner.calledOnceWithExactly(identifier.uuid)).to.be.true;
expect(isPerson.calledOnceWithExactly(settings, doc)).to.be.true;
expect(isNormalizedParent.calledOnceWithExactly(doc)).to.be.true;
expect(warn.calledOnceWithExactly(`Document [${identifier.uuid}] is not a valid person.`)).to.be.true;
});

Expand All @@ -105,7 +82,6 @@ describe('local person', () => {
expect(getDocByIdInner.calledOnceWithExactly(identifier.uuid)).to.be.true;
expect(settingsGetAll.notCalled).to.be.true;
expect(isPerson.notCalled).to.be.true;
expect(isNormalizedParent.notCalled).to.be.true;
expect(warn.calledOnceWithExactly(`No person found for identifier [${identifier.uuid}].`)).to.be.true;
});
});
Expand Down Expand Up @@ -155,7 +131,6 @@ describe('local person', () => {
getLineageDocsByIdInner.resolves([person, place0, place1, place2]);
isPerson.returns(true);
settingsGetAll.returns(settings);
isNormalizedParent.returns(true);
getPrimaryContactIds.returns([contact0._id, contact1._id, person._id]);
getDocsByIdsInner.resolves([contact0, contact1]);
const place0WithContact = { ...place0, contact: contact0 };
Expand All @@ -173,7 +148,6 @@ describe('local person', () => {
expect(result).to.equal(copiedPerson);
expect(getLineageDocsByIdInner.calledOnceWithExactly(identifier.uuid)).to.be.true;
expect(isPerson.calledOnceWithExactly(settings, person)).to.be.true;
expect(isNormalizedParent.calledOnceWithExactly(person)).to.be.true;
expect(warn.notCalled).to.be.true;
expect(debug.notCalled).to.be.true;
expect(getPrimaryContactIds.calledOnceWithExactly([place0, place1, place2])).to.be.true;
Expand All @@ -195,7 +169,6 @@ describe('local person', () => {
expect(result).to.be.null;
expect(getLineageDocsByIdInner.calledOnceWithExactly(identifier.uuid)).to.be.true;
expect(isPerson.notCalled).to.be.true;
expect(isNormalizedParent.notCalled).to.be.true;
expect(warn.calledOnceWithExactly(`No person found for identifier [${identifier.uuid}].`)).to.be.true;
expect(debug.notCalled).to.be.true;
expect(getPrimaryContactIds.notCalled).to.be.true;
Expand All @@ -206,51 +179,41 @@ describe('local person', () => {
expect(deepCopy.notCalled).to.be.true;
});

[false, true].forEach((isPersonResult) => {
it('returns null if the doc returned is not a person', async () => {
const person = { type: 'person', _id: 'uuid', _rev: 'rev' };
const place0 = { _id: 'place0', _rev: 'rev' };
const place1 = { _id: 'place1', _rev: 'rev' };
const place2 = { _id: 'place2', _rev: 'rev' };
getLineageDocsByIdInner.resolves([person, place0, place1, place2]);
isPerson.returns(isPersonResult);
settingsGetAll.returns(settings);
isNormalizedParent.returns(false);
it('returns null if the doc returned is not a person', async () => {
const person = { type: 'person', _id: 'uuid', _rev: 'rev' };
const place0 = { _id: 'place0', _rev: 'rev' };
const place1 = { _id: 'place1', _rev: 'rev' };
const place2 = { _id: 'place2', _rev: 'rev' };
getLineageDocsByIdInner.resolves([person, place0, place1, place2]);
isPerson.returns(false);
settingsGetAll.returns(settings);

const result = await Person.v1.getWithLineage(localContext)(identifier);
const result = await Person.v1.getWithLineage(localContext)(identifier);

expect(result).to.be.null;
expect(getLineageDocsByIdInner.calledOnceWithExactly(identifier.uuid)).to.be.true;
expect(isPerson.calledOnceWithExactly(settings, person)).to.be.true;
if (isPersonResult) {
expect(isNormalizedParent.calledOnceWithExactly(person)).to.be.true;
} else {
expect(isNormalizedParent.notCalled).to.be.true;
}
expect(warn.calledOnceWithExactly(`Document [${identifier.uuid}] is not a valid person.`)).to.be.true;
expect(debug.notCalled).to.be.true;
expect(getPrimaryContactIds.notCalled).to.be.true;
expect(getDocsByIdsInner.notCalled).to.be.true;
expect(hydratePrimaryContactOuter.notCalled).to.be.true;
expect(hydratePrimaryContactInner.notCalled).to.be.true;
expect(hydrateLineage.notCalled).to.be.true;
expect(deepCopy.notCalled).to.be.true;
});
expect(result).to.be.null;
expect(getLineageDocsByIdInner.calledOnceWithExactly(identifier.uuid)).to.be.true;
expect(isPerson.calledOnceWithExactly(settings, person)).to.be.true;
expect(warn.calledOnceWithExactly(`Document [${identifier.uuid}] is not a valid person.`)).to.be.true;
expect(debug.notCalled).to.be.true;
expect(getPrimaryContactIds.notCalled).to.be.true;
expect(getDocsByIdsInner.notCalled).to.be.true;
expect(hydratePrimaryContactOuter.notCalled).to.be.true;
expect(hydratePrimaryContactInner.notCalled).to.be.true;
expect(hydrateLineage.notCalled).to.be.true;
expect(deepCopy.notCalled).to.be.true;
});

it('returns a person if no lineage is found', async () => {
const person = { type: 'person', _id: 'uuid', _rev: 'rev' };
getLineageDocsByIdInner.resolves([person]);
isPerson.returns(true);
settingsGetAll.returns(settings);
isNormalizedParent.returns(true);

const result = await Person.v1.getWithLineage(localContext)(identifier);

expect(result).to.equal(person);
expect(getLineageDocsByIdInner.calledOnceWithExactly(identifier.uuid)).to.be.true;
expect(isPerson.calledOnceWithExactly(settings, person)).to.be.true;
expect(isNormalizedParent.calledOnceWithExactly(person)).to.be.true;
expect(warn.notCalled).to.be.true;
expect(debug.calledOnceWithExactly(`No lineage places found for person [${identifier.uuid}].`)).to.be.true;
expect(getPrimaryContactIds.notCalled).to.be.true;
Expand Down
37 changes: 28 additions & 9 deletions tests/integration/api/controllers/person.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,33 @@ describe('Person API', () => {
short_name: 'Mary'
}));
const userNoPerms = utils.deepFreeze(userFactory.build({
username: 'online-no-perms',
place: place1._id,
contact: {
_id: 'fixture:user:online-no-perms',
name: 'Online User',
},
roles: ['mm-online']
}));
const offlineUser = utils.deepFreeze(userFactory.build({
username: 'offline-has-perms',
place: place0._id,
roles: ['no_perms']
contact: {
_id: 'fixture:user:offline-has-perms',
name: 'Offline User',
},
roles: ['chw']
}));
const dataContext = getRemoteDataContext(utils.getOrigin());

before(async () => {
await utils.saveDocs([contact0, contact1, contact2, place0, place1, place2, patient]);
await utils.createUsers([userNoPerms]);
await utils.createUsers([userNoPerms, offlineUser]);
});

after(async () => {
await utils.revertDb([], true);
await utils.deleteUsers([userNoPerms]);
await utils.deleteUsers([userNoPerms, offlineUser]);
});

describe('GET /api/v1/person/:uuid', async () => {
Expand Down Expand Up @@ -77,12 +91,17 @@ describe('Person API', () => {
expect(person).to.be.null;
});

it('throws error when user does not have can_view_contacts permission', async () => {
const opts = {
path: `/api/v1/person/${patient._id}`,
auth: { username: userNoPerms.username, password: userNoPerms.password },
};
await expect(utils.request(opts)).to.be.rejectedWith('403 - {"code":403,"error":"Insufficient privileges"}');
[
['does not have can_view_contacts permission', userNoPerms],
['is not an online user', offlineUser]
].forEach(([description, user]) => {
it(`throws error when user ${description}`, async () => {
const opts = {
path: `/api/v1/person/${patient._id}`,
auth: { username: user.username, password: user.password },
};
await expect(utils.request(opts)).to.be.rejectedWith('403 - {"code":403,"error":"Insufficient privileges"}');
});
});
});
});

0 comments on commit 8ed110d

Please sign in to comment.