From 3d868e139228fbbe0ba4e6bc3f5d56321fdca2b9 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Tue, 11 Jun 2024 15:59:20 -0700 Subject: [PATCH] Check dataset verbs in endpoints about forms (#1146) * dataset verb wip * improved dataset functionality * Small changes/comments based on code review --- lib/model/query/datasets.js | 48 ++++++- test/integration/api/datasets.js | 215 +++++++++++++++++++++++++++++-- 2 files changed, 247 insertions(+), 16 deletions(-) diff --git a/lib/model/query/datasets.js b/lib/model/query/datasets.js index e41107653..852c095fc 100644 --- a/lib/model/query/datasets.js +++ b/lib/model/query/datasets.js @@ -11,7 +11,7 @@ const { sql } = require('slonik'); const { extender, insert, QueryOptions, equals, unjoiner, updater } = require('../../util/db'); const { Dataset, Form, Audit } = require('../frames'); const { validatePropertyName } = require('../../data/dataset'); -const { isEmpty, isNil, either, reduceBy, groupBy, uniqWith, equals: rEquals, map } = require('ramda'); +const { difference, isEmpty, isNil, either, reduceBy, groupBy, uniqWith, equals: rEquals, map } = require('ramda'); const Problem = require('../../util/problem'); const { construct } = require('../../util/util'); @@ -83,7 +83,7 @@ SELECT "action", "id" FROM ds // * form (a Form frame or object containing projectId, formDefId, and schemaId) // * array of field objects and property names that were parsed from the form xml // (form fields do not need any extra IDs of the form, form def, or schema.) -const createOrMerge = (parsedDataset, form, fields) => async ({ one, Actees, Datasets, Projects }) => { +const createOrMerge = (parsedDataset, form, fields) => async ({ context, one, Actees, Datasets, Projects }) => { const { projectId } = form; const { id: formDefId, schemaId } = form.def; @@ -126,6 +126,21 @@ const createOrMerge = (parsedDataset, form, fields) => async ({ one, Actees, Dat throw error; }); + // Verify that user has ability to create dataset + // Updating an unpublished dataset is equivalent to creating the dataset + if (result.action === 'created' || (result.action === 'updated' && existingDataset.get().publishedAt == null)) + await context.auth.canOrReject('dataset.create', { acteeId }); + + // Verify that user has ability to update dataset + if (result.action === 'updated') { + // Only consider updates that add new properties to the dataset + const existingProperties = await Datasets.getProperties(existingDataset.get().id); + const existingNames = existingProperties.map(f => f.name); + const newNames = dsPropertyFields.map(f => f.propertyName); + if (difference(newNames, existingNames).length > 0) + await context.auth.canOrReject('dataset.update', { acteeId }); + } + // Partial contains acteeId which is needed for auditing. // Additional form fields and properties needed for audit logging as well. return partial.with({ action: result.action, fields: dsPropertyFields }); @@ -180,7 +195,7 @@ createPublishedProperty.audit = (property, dataset) => (log) => // `IfExists` part is particularly helpful for `Forms.publish`, // with that it doesn't need to ensure the existence of the Dataset // and it can call this function blindly -const publishIfExists = (formDefId, publishedAt) => async ({ all, maybeOne }) => { +const publishIfExists = (formDefId, publishedAt) => async ({ all, context, maybeOne }) => { // Helper sub-queries const _publishIfExists = () => sql` @@ -238,7 +253,32 @@ const publishIfExists = (formDefId, publishedAt) => async ({ all, maybeOne }) => const { current, provided } = d.get(); throw Problem.user.datasetNameConflict({ current, provided }); } - return all(_publishIfExists()); + + const properties = await all(_publishIfExists()); + // `properties` contains a list of objects with the following structure: + // property id, property name, dataset id, dataset actee id, and action + // The action for a property may be 'propertyAdded' if it is newly published + // or 'noop' if it existed in the dataset already. + // A fake property with id -1 is included when dataset is newly created, + // with action 'datasetCreated' + const datasets = groupBy(c => c.acteeId, properties); + + for (const acteeId of Object.keys(datasets)) { + if (datasets[acteeId].some(p => p.action === 'datasetCreated')) { + // Dataset has been newly created (published) so check dataset.create + + // eslint-disable-next-line no-await-in-loop + await context.auth.canOrReject('dataset.create', { acteeId }); + } else if (datasets[acteeId].some(p => p.action === 'propertyAdded')) { + // Dataset has newly published properties so check dataset.update + + // eslint-disable-next-line no-await-in-loop + await context.auth.canOrReject('dataset.update', { acteeId }); + } + // Else the dataset had no new properties added, so no need to check any verbs. + } + + return properties; }; publishIfExists.audit = (properties) => (log) => { diff --git a/test/integration/api/datasets.js b/test/integration/api/datasets.js index a5abd3fc2..f089954e3 100644 --- a/test/integration/api/datasets.js +++ b/test/integration/api/datasets.js @@ -1,6 +1,6 @@ const { readFileSync } = require('fs'); const appRoot = require('app-root-path'); -const { testService } = require('../setup'); +const { testService, testServiceFullTrx } = require('../setup'); const testData = require('../../data/xml'); const config = require('config'); const { Form } = require('../../../lib/model/frames'); @@ -3376,17 +3376,6 @@ describe('datasets and entities', () => { describe('parsing datasets on form upload', () => { describe('parsing datasets at /projects/:id/forms POST', () => { - it('should allow someone without dataset.create to create a dataset through posting a form', testService(async (service, { run }) => { - await run(sql`UPDATE roles SET verbs = (verbs - 'dataset.create') WHERE system in ('manager')`); - - const asBob = await service.login('bob'); - - await asBob.post('/v1/projects/1/forms') - .send(testData.forms.simpleEntity) - .set('Content-Type', 'text/xml') - .expect(200); - })); - it('should return a Problem if the entity xml has the wrong version', testService((service) => service.login('alice', (asAlice) => asAlice.post('/v1/projects/1/forms') @@ -3904,6 +3893,208 @@ describe('datasets and entities', () => { })); })); }); + + describe.only('dataset-specific verbs', () => { + describe('dataset.create', () => { + it('should NOT allow a new form that creates a dataset without user having dataset.create verb', testServiceFullTrx(async (service, { run }) => { + await run(sql`UPDATE roles SET verbs = (verbs - 'dataset.create') WHERE system in ('manager')`); + + const asBob = await service.login('bob'); + + await asBob.post('/v1/projects/1/forms') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(403); + + // shouldn't work with immediate publish, either + await asBob.post('/v1/projects/1/forms?publish=true') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(403); + })); + + it('should NOT allow "creating" of a dataset when the dataset exists but unpublished', testServiceFullTrx(async (service, { run }) => { + const asAlice = await service.login('alice'); + const asBob = await service.login('bob'); + await run(sql`UPDATE roles SET verbs = (verbs - 'dataset.create') WHERE system in ('manager')`); + + // Alice can upload first version of form that creates unpublished "people" dataset + await asAlice.post('/v1/projects/1/forms') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(200); + + // Should not be OK for bob to "create" people dataset + await asBob.post('/v1/projects/1/forms') + .send(testData.forms.simpleEntity + .replace('simpleEntity', 'simpleEntity2')) + .set('Content-Type', 'text/xml') + .expect(403); + })); + + it('should NOT allow updating a form about an unpublished dataset, which is similar to creating that dataset', testServiceFullTrx(async (service, { run }) => { + const asAlice = await service.login('alice'); + const asBob = await service.login('bob'); + await run(sql`UPDATE roles SET verbs = (verbs - 'dataset.create') WHERE system in ('manager')`); + + // Alice can upload first version of form that creates unpublished "people" dataset + await asAlice.post('/v1/projects/1/forms') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(200); + + // Should not be OK for bob to update this form + await asBob.post('/v1/projects/1/forms/simpleEntity/draft') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(403); + })); + + it('should NOT allow updating a draft that creates a dataset without user having dataset.create verb', testServiceFullTrx(async (service, { run }) => { + const asAlice = await service.login('alice'); + const asBob = await service.login('bob'); + await run(sql`UPDATE roles SET verbs = (verbs - 'dataset.create') WHERE system in ('manager')`); + + // Alice can upload first version of form + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(200); + + await asBob.post('/v1/projects/1/forms/simpleEntity/draft') + .send(testData.forms.simpleEntity.replace(/people/g, 'trees')) + .set('Content-Type', 'text/xml') + .expect(403); + })); + + it('should NOT allow unpublished dataset to be published on form publish if user does not have dataset.create verb', testServiceFullTrx(async (service, { run }) => { + const asAlice = await service.login('alice'); + const asBob = await service.login('bob'); + await run(sql`UPDATE roles SET verbs = (verbs - 'dataset.create') WHERE system in ('manager')`); + + // Alice can upload first version of form + await asAlice.post('/v1/projects/1/forms') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(200); + + // Bob should not be able to publish form because it will publish the new dataset + await asBob.post('/v1/projects/1/forms/simpleEntity/draft/publish') + .expect(403); + })); + }); + + describe('dataset.update', () => { + it('should NOT allow a new form that updates a dataset without user having dataset.update verb', testServiceFullTrx(async (service, { run }) => { + const asAlice = await service.login('alice'); + const asBob = await service.login('bob'); + + await run(sql`UPDATE roles SET verbs = (verbs - 'dataset.update') WHERE system in ('manager')`); + + await asAlice.post('/v1/projects/1/datasets') + .send({ name: 'people' }) + .expect(200); + + // Form mentions properties 'age' and 'first_name' in dataset 'people' + // But Bob is not allowed to add these new properties to an existing dataset. + await asBob.post('/v1/projects/1/forms') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(403); + + // shouldn't work with immediate publish, either + await asBob.post('/v1/projects/1/forms?publish=true') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(403); + })); + + it('should NOT allow update draft that updates a dataset without user having dataset.update verb', testServiceFullTrx(async (service, { run }) => { + const asAlice = await service.login('alice'); + const asBob = await service.login('bob'); + await run(sql`UPDATE roles SET verbs = (verbs - 'dataset.update') WHERE system in ('manager')`); + + // Alice can upload first version of form + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(200); + + await asBob.post('/v1/projects/1/forms/simpleEntity/draft') + .send(testData.forms.simpleEntity.replace('saveto="age"', 'saveto="birth_year"')) + .set('Content-Type', 'text/xml') + .expect(403); + })); + + it('should NOT allow unpublished properties to be published on form publish if user does not have dataset.update verb', testServiceFullTrx(async (service, { run }) => { + const asAlice = await service.login('alice'); + const asBob = await service.login('bob'); + await run(sql`UPDATE roles SET verbs = (verbs - 'dataset.update') WHERE system in ('manager')`); + + // Alice can upload and publish first version of form + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(200); + + // Alice can upload new version of form with new property + await asAlice.post('/v1/projects/1/forms/simpleEntity/draft') + .send(testData.forms.simpleEntity + .replace('saveto="age"', 'saveto="birth_year"') + .replace('orx:version="1.0"', 'orx:version="2.0"') + ) + .set('Content-Type', 'text/xml') + .expect(200); + + // Bob should not be able to publish form because it will publish the new property + await asBob.post('/v1/projects/1/forms/simpleEntity/draft/publish') + .expect(403); + })); + + it('should ALLOW update of form draft that does not modify existing dataset', testServiceFullTrx(async (service, { run }) => { + const asAlice = await service.login('alice'); + const asBob = await service.login('bob'); + await run(sql`UPDATE roles SET verbs = (verbs - 'dataset.update') WHERE system in ('manager')`); + + // Alice can upload first version of form + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(200); + + // Should be OK for bob to update draft if not updating dataset + await asBob.post('/v1/projects/1/forms/simpleEntity/draft') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(200); + })); + + it('should ALLOW new form about existing dataset that does not update it', testServiceFullTrx(async (service, { run }) => { + const asAlice = await service.login('alice'); + const asBob = await service.login('bob'); + await run(sql`UPDATE roles SET verbs = (verbs - 'dataset.update') WHERE system in ('manager')`); + + // Alice can create the dataset + await asAlice.post('/v1/projects/1/datasets') + .send({ name: 'people' }) + .expect(200); + + // And create the properties + await asAlice.post('/v1/projects/1/datasets/people/properties') + .send({ name: 'age' }) + .expect(200); + await asAlice.post('/v1/projects/1/datasets/people/properties') + .send({ name: 'first_name' }) + .expect(200); + + // Should be OK for bob to upload form that uses existing dataset and properties + await asBob.post('/v1/projects/1/forms') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'text/xml') + .expect(200); + })); + }); + }); }); describe('dataset audit logging at /projects/:id/forms POST', () => {