From 43bd8a49a49f23882ab8b6d4ea7ea7da0bfb5bb2 Mon Sep 17 00:00:00 2001 From: Matthew White Date: Thu, 14 Dec 2023 00:48:48 -0500 Subject: [PATCH] Check entity actions in submission against those permitted by form def (#1061) Closes getodk/central#518. --- lib/data/dataset.js | 75 ++++++--- .../20231208-01-dataset-form-def-actions.js | 20 +++ lib/model/query/datasets.js | 20 ++- lib/model/query/entities.js | 17 +- lib/model/query/forms.js | 12 +- lib/util/problem.js | 3 + test/data/xml.js | 22 +-- test/integration/api/entities.js | 62 ++++++- test/unit/data/dataset.js | 154 +++++++++++------- 9 files changed, 273 insertions(+), 112 deletions(-) create mode 100644 lib/model/migrations/20231208-01-dataset-form-def-actions.js diff --git a/lib/data/dataset.js b/lib/data/dataset.js index 143e68497..4a25fc00e 100644 --- a/lib/data/dataset.js +++ b/lib/data/dataset.js @@ -8,15 +8,23 @@ // except according to the terms contained in the LICENSE file. // // This is an extension of schema.js where we define *datasest-specific* functions -// for dealing with the XForms XML schema. When a form us uploaded to Central, +// for dealing with the XForms XML schema. When a form is uploaded to Central, // we check the XML for an block, which will contain information // about datasets and mappings from form fields to // dataset properties. const Option = require('../util/option'); const Problem = require('../util/problem'); -const { traverseXml, findOne, and, root, node, hasAttr, tree, attr } = require('../util/xml'); +const { traverseXml, findOne, root, node, attr, stripNamespacesFromPath } = require('../util/xml'); +/* +Validates a dataset name: + + - valid xml identifier + - does not contain `.` + - `__` prefix reserved + - case insensitive? +*/ const validateDatasetName = (name) => { // Regex explanation: // Check for a match with a valid string @@ -26,11 +34,10 @@ const validateDatasetName = (name) => { // If there's a match, return true (valid)! // Note that '.' is not in the valid character set. // Non-letter unicode characters also not currently allowed - const match = /^(?!__)[\p{L}:_][\p{L}:\d_-]*$/u.exec(name.trim()); + const match = /^(?!__)[\p{L}:_][\p{L}:\d_-]*$/u.exec(name); return (match !== null); }; - const validatePropertyName = (name) => { // Regex explanation // (similar to above dataset name check with some slight differences) @@ -46,37 +53,53 @@ const validatePropertyName = (name) => { return (match !== null); }; -// Here we are looking for dataset-registrating information within the tag. -// We mainly need the dataset name, but we also do a bit of validation: -// 1. namespace validation: -// e.g. xmlns:entities="http://www.opendatakit.org/xforms/entities -// 2. dataset name validation: -// - valid xml identifier -// - does not contain `.` -// - `__` prefix reserved -// - case insensitive? -// -// Like with getFormFields in schema.js, we assume the form is otherwise valid. -const getDataset = (xml) => { - const metaNode = findOne(root('html'), node('head'), node('model'), node('instance'), node(), node('meta')); - return traverseXml(xml, [ +/* +getDataset() parses form XML for dataset-related information on the +tag: + + - The dataset name + - The entity actions permitted by the form def + +getDataset() also does some validation, including of: + + - The version of the entities spec + - The dataset name + +Like with getFormFields() in schema.js, we assume the form is otherwise valid. +*/ +const getDataset = (xml) => + traverseXml(xml, [ findOne(root('html'), node('head'), node('model'))(attr('entities-version')), - metaNode(findOne(root(), node('entity'))(tree())), - metaNode(findOne(root(), and(node('entity'), hasAttr('dataset')))(attr('dataset'))) - ]).then(([ version, entityTag, datasetName ]) => { - if (entityTag.isEmpty() && datasetName.isEmpty()) + findOne(root('html'), node('head'), node('model'), node('instance'), node(), node('meta'), node('entity'))(attr()) + ]).then(([ version, entityAttrs ]) => { + if (entityAttrs.isEmpty()) return Option.none(); + if (version.isEmpty()) throw Problem.user.invalidEntityForm({ reason: 'Entities specification version is missing.' }); else if (!(version.get().startsWith('2022.1.') || version.get().startsWith('2023.1.'))) throw Problem.user.invalidEntityForm({ reason: `Entities specification version [${version.get()}] is not supported.` }); + + const strippedAttrs = Object.create(null); + for (const [name, value] of Object.entries(entityAttrs.get())) + strippedAttrs[stripNamespacesFromPath(name)] = value; + // check that dataset name is valid - if (datasetName.isEmpty()) + const datasetName = strippedAttrs.dataset?.trim(); + if (datasetName == null) throw Problem.user.invalidEntityForm({ reason: 'Dataset name is missing.' }); - if (!validateDatasetName(datasetName.get())) + if (!validateDatasetName(datasetName)) throw Problem.user.invalidEntityForm({ reason: 'Invalid dataset name.' }); - return Option.of(datasetName.get().trim()); + + // Entity actions permitted by the form def + const actions = []; + const { create, update } = strippedAttrs; + if (create != null) actions.push('create'); + if (update != null) actions.push('update'); + if (actions.length === 0) + throw Problem.user.invalidEntityForm({ reason: 'The form must specify at least one entity action, for example, create or update.' }); + + return Option.of({ name: datasetName, actions }); }); -}; module.exports = { getDataset, validateDatasetName, validatePropertyName }; diff --git a/lib/model/migrations/20231208-01-dataset-form-def-actions.js b/lib/model/migrations/20231208-01-dataset-form-def-actions.js new file mode 100644 index 000000000..3787bc952 --- /dev/null +++ b/lib/model/migrations/20231208-01-dataset-form-def-actions.js @@ -0,0 +1,20 @@ +// Copyright 2023 ODK Central Developers +// See the NOTICE file at the top-level directory of this distribution and at +// https://github.com/getodk/central-backend/blob/master/NOTICE. +// This file is part of ODK Central. It is subject to the license terms in +// the LICENSE file found in the top-level directory of this distribution and at +// https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central, +// including this file, may be copied, modified, propagated, or distributed +// except according to the terms contained in the LICENSE file. + +const up = async (db) => { + await db.raw('ALTER TABLE dataset_form_defs ADD COLUMN actions jsonb'); + // eslint-disable-next-line quotes + await db.raw(`UPDATE dataset_form_defs SET actions = '["create"]'`); + await db.raw('ALTER TABLE dataset_form_defs ALTER COLUMN actions SET NOT NULL'); +}; + +const down = (db) => + db.raw('ALTER TABLE dataset_form_defs DROP COLUMN actions'); + +module.exports = { up, down }; diff --git a/lib/model/query/datasets.js b/lib/model/query/datasets.js index 3f74ba13c..6223c8f49 100644 --- a/lib/model/query/datasets.js +++ b/lib/model/query/datasets.js @@ -20,7 +20,7 @@ const { sanitizeOdataIdentifier } = require('../../util/util'); //////////////////////////////////////////////////////////////////////////// // DATASET CREATE AND UPDATE -const _insertDatasetDef = (dataset, acteeId) => sql` +const _insertDatasetDef = (dataset, acteeId, actions) => sql` WITH ds_ins AS ( INSERT INTO datasets (id, name, "projectId", "createdAt", "acteeId") VALUES (nextval(pg_get_serial_sequence('datasets', 'id')), ${dataset.name}, ${dataset.projectId}, clock_timestamp(), ${acteeId}) @@ -35,7 +35,7 @@ const _insertDatasetDef = (dataset, acteeId) => sql` ), ds_defs AS ( INSERT INTO dataset_form_defs - SELECT ds.id, ${dataset.aux.formDefId} FROM ds + SELECT ds.id, ${dataset.aux.formDefId}, ${JSON.stringify(actions)} FROM ds ON CONFLICT ON CONSTRAINT dataset_form_defs_datasetid_formdefid_unique DO NOTHING ) @@ -69,25 +69,27 @@ insert_property_fields AS ( // should be moved to util.js or we already have similar func somewhere? const isNilOrEmpty = either(isNil, isEmpty); -const _createOrMerge = (dataset, fields, acteeId) => sql` -${_insertDatasetDef(dataset, acteeId)} +const _createOrMerge = (dataset, acteeId, actions, fields) => sql` +${_insertDatasetDef(dataset, acteeId, actions)} ${isNilOrEmpty(fields) ? sql`` : _insertProperties(fields)} SELECT "action", "id" FROM ds `; -// Takes the dataset name and field->property mappings defined in a form -// and creates or updates the named dataset. +// Takes information about the dataset parsed from the form XML, as well as +// field->property mappings defined in a form, then creates or updates the named +// dataset. // Arguments: -// * dataset name +// * information about the dataset parsed from the form XML // * 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 -const createOrMerge = (name, form, fields) => async ({ one, Actees, Datasets, Projects }) => { +const createOrMerge = (parsedDataset, form, fields) => async ({ one, Actees, Datasets, Projects }) => { const { projectId } = form; const { id: formDefId, schemaId } = form.def; // Provision acteeId if necessary. // Need to check for existing dataset, and if not found, need to also // fetch the project since the dataset will be an actee child of the project. + const { name } = parsedDataset; const existingDataset = await Datasets.get(projectId, name, false); const acteeId = existingDataset.isDefined() ? existingDataset.get().acteeId @@ -107,7 +109,7 @@ const createOrMerge = (name, form, fields) => async ({ one, Actees, Datasets, Pr // Insert or update: update dataset, dataset properties, and links to fields and current form // result contains action (created or updated) and the id of the dataset. - const result = await one(_createOrMerge(partial, dsPropertyFields, acteeId)) + const result = await one(_createOrMerge(partial, acteeId, parsedDataset.actions, dsPropertyFields)) .catch(error => { if (error.constraint === 'ds_property_fields_dspropertyid_formdefid_unique') { throw Problem.user.invalidEntityForm({ reason: 'Multiple Form Fields cannot be saved to a single property.' }); diff --git a/lib/model/query/entities.js b/lib/model/query/entities.js index 8101a8364..0298bef49 100644 --- a/lib/model/query/entities.js +++ b/lib/model/query/entities.js @@ -148,6 +148,11 @@ resolveConflict.audit = (entity, dataset) => (log) => log('entity.update.resolve ///////////////////////////////////////////////////////////////////////// // Processing submission events to create and update entities +const _getFormDefActions = (oneFirst, datasetId, formDefId) => oneFirst(sql` +SELECT actions +FROM dataset_form_defs +WHERE "datasetId" = ${datasetId} AND "formDefId" = ${formDefId}`); + const _createEntity = (dataset, entityData, submissionId, submissionDef, submissionDefId, event, parentEvent) => async ({ Audits, Entities }) => { // If dataset requires approval on submission to create an entity and this event is not // an approval event, then don't create an entity @@ -236,7 +241,7 @@ const _updateEntity = (dataset, entityData, submissionId, submissionDef, submiss }; // Entrypoint to where submissions (a specific version) become entities -const _processSubmissionEvent = (event, parentEvent) => async ({ Datasets, Entities, Submissions, Forms }) => { +const _processSubmissionEvent = (event, parentEvent) => async ({ Datasets, Entities, Submissions, Forms, oneFirst }) => { const { submissionId, submissionDefId } = event.details; const form = await Forms.getByActeeId(event.acteeId); @@ -279,6 +284,16 @@ const _processSubmissionEvent = (event, parentEvent) => async ({ Datasets, Entit const dataset = (await Datasets.get(form.get().projectId, entityData.system.dataset, true)) .orThrow(Problem.user.datasetNotFound({ datasetName: entityData.system.dataset })); + // Check that the form permits the entity action(s) specified in the + // submission. + const permittedActions = await _getFormDefActions(oneFirst, dataset.id, submissionDef.formDefId); + for (const action of ['create', 'update']) { + const submissionAction = entityData.system[action]; + if ((submissionAction === '1' || submissionAction === 'true') && + !permittedActions.includes(action)) + throw Problem.user.entityActionNotPermitted({ action, permitted: permittedActions }); + } + // Try update before create (if both are specified) if (entityData.system.update === '1' || entityData.system.update === 'true') try { diff --git a/lib/model/query/forms.js b/lib/model/query/forms.js index 341135127..86cf7cacb 100644 --- a/lib/model/query/forms.js +++ b/lib/model/query/forms.js @@ -114,7 +114,7 @@ const createNew = (partial, project, publish = false) => async ({ run, Actees, D defData.keyId = await partial.aux.key.map(Keys.ensure).orElse(resolve(null)); // Parse form XML for fields and entity/dataset definitions - const [fields, datasetName] = await Promise.all([ + const [fields, parsedDataset] = await Promise.all([ getFormFields(partial.xml), (partial.aux.key.isDefined() ? resolve(Option.none()) : getDataset(partial.xml)) // Don't parse dataset schema if Form has encryption key ]); @@ -163,8 +163,8 @@ const createNew = (partial, project, publish = false) => async ({ run, Actees, D ]); // Update datasets and properties if defined - if (datasetName.isDefined()) { - await Datasets.createOrMerge(datasetName.get(), savedForm, fields); + if (parsedDataset.isDefined()) { + await Datasets.createOrMerge(parsedDataset.get(), savedForm, fields); if (publish) { await Datasets.publishIfExists(savedForm.def.id, savedForm.def.publishedAt.toISOString()); @@ -242,7 +242,7 @@ const createVersion = (partial, form, publish, duplicating = false) => async ({ const keyId = await partial.aux.key.map(Keys.ensure).orElse(resolve(null)); // Parse form fields and dataset/entity definition from form XML - const [fields, datasetName] = await Promise.all([ + const [fields, parsedDataset] = await Promise.all([ getFormFields(partial.xml), (partial.aux.key.isDefined() ? resolve(Option.none()) : getDataset(partial.xml)) ]); @@ -299,8 +299,8 @@ const createVersion = (partial, form, publish, duplicating = false) => async ({ ]); // Update datasets and properties if defined - if (datasetName.isDefined()) { - await Datasets.createOrMerge(datasetName.get(), new Form(form, { def: savedDef }), fieldsForInsert); + if (parsedDataset.isDefined()) { + await Datasets.createOrMerge(parsedDataset.get(), new Form(form, { def: savedDef }), fieldsForInsert); if (publish) { await Datasets.publishIfExists(savedDef.id, savedDef.publishedAt.toISOString()); } diff --git a/lib/util/problem.js b/lib/util/problem.js index b887ca0b0..7a2d74f3c 100644 --- a/lib/util/problem.js +++ b/lib/util/problem.js @@ -136,6 +136,9 @@ const problems = { // TODO: should have details but not sure what yet. insufficientRights: problem(403.1, () => 'The authentication you provided does not have rights to perform that action.'), + entityActionNotPermitted: problem(403.2, ({ action, permitted }) => + `The submission attempts an entity ${action}, but the form does not permit that action. The form permits the following actions: ${permitted.join(', ')}.`), + // no detail information as the problem should be self-evident by REST conventions. notFound: problem(404.1, () => 'Could not find the resource you were looking for.'), diff --git a/test/data/xml.js b/test/data/xml.js index 1d94cd0d4..f9ae509f5 100644 --- a/test/data/xml.js +++ b/test/data/xml.js @@ -603,23 +603,23 @@ module.exports = { Alice (88) one - - Alice - 88 - Chicago - `, + + Alice + 88 + Chicago + `, two: ` two - Jane (30) + Jane (30) two - - Jane - 30 - Boston - `, + + Jane + 30 + Boston + `, three: ` three diff --git a/test/integration/api/entities.js b/test/integration/api/entities.js index c55f9a485..8befcc02e 100644 --- a/test/integration/api/entities.js +++ b/test/integration/api/entities.js @@ -69,9 +69,10 @@ const testEntityUpdates = (test) => testService(async (service, container) => { }) .expect(200); - // create form and submission to update entity + // create a form that can update or create an entity await asAlice.post('/v1/projects/1/forms?publish=true') - .send(testData.forms.updateEntity) + .send(testData.forms.updateEntity + .replace('update=""', 'update="" create=""')) .set('Content-Type', 'application/xml') .expect(200); @@ -2809,4 +2810,61 @@ describe('Entities API', () => { })); }); }); + + describe('permitted entity actions', () => { + it('should result in an error for an update from a create form', testService(async (service, container) => { + const asAlice = await service.login('alice'); + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(testData.forms.simpleEntity) + .set('Content-Type', 'application/xml') + .expect(200); + await asAlice.post('/v1/projects/1/forms/simpleEntity/submissions') + .send(testData.instances.simpleEntity.one) + .set('Content-Type', 'application/xml') + .expect(200); + await exhaust(container); + await asAlice.post('/v1/projects/1/forms/simpleEntity/submissions') + .send(testData.instances.simpleEntity.one + .replace('one', 'another') + .replace('create="1"', 'update="1"')) + .set('Content-Type', 'application/xml') + .expect(200); + await exhaust(container); + const { body: audits } = await asAlice.get('/v1/projects/1/forms/simpleEntity/submissions/another/audits') + .expect(200); + const actions = audits.map(audit => audit.action); + actions.should.eql(['entity.error', 'submission.create']); + audits[0].details.should.containEql({ + problem: { + problemCode: 403.2, + problemDetails: { action: 'update', permitted: ['create'] } + }, + errorMessage: 'The submission attempts an entity update, but the form does not permit that action. The form permits the following actions: create.' + }); + })); + + it('should result in an error for a create from an update form', testService(async (service, container) => { + const asAlice = await service.login('alice'); + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(testData.forms.updateEntity) + .set('Content-Type', 'application/xml') + .expect(200); + await asAlice.post('/v1/projects/1/forms/updateEntity/submissions') + .send(testData.instances.updateEntity.one + .replace('update="1"', 'create="1"')) + .expect(200); + await exhaust(container); + const { body: audits } = await asAlice.get('/v1/projects/1/forms/updateEntity/submissions/one/audits') + .expect(200); + const actions = audits.map(audit => audit.action); + actions.should.eql(['entity.error', 'submission.create']); + audits[0].details.should.containEql({ + problem: { + problemCode: 403.2, + problemDetails: { action: 'create', permitted: ['update'] } + }, + errorMessage: 'The submission attempts an entity create, but the form does not permit that action. The form permits the following actions: update.' + }); + })); + }); }); diff --git a/test/unit/data/dataset.js b/test/unit/data/dataset.js index 32285cbce..c42690abf 100644 --- a/test/unit/data/dataset.js +++ b/test/unit/data/dataset.js @@ -12,15 +12,15 @@ describe('parsing dataset from entity block', () => { res.should.equal(Option.none()))); describe('versioning', () => { - it('should check for any version that starts with 2022.1.', () => + it('should validate any version that starts with 2022.1.', () => getDataset(testData.forms.simpleEntity - .replace('2022.1.0', '2022.1.123')).then((res) => - res.get().should.eql('people'))); + .replace('2022.1.0', '2022.1.123')) + .should.be.fulfilled()); - it('should check for any version that starts with 2023.1.', () => + it('should validate any version that starts with 2023.1.', () => getDataset(testData.forms.updateEntity - .replace('2023.1.0', '2023.1.123')).then((res) => - res.get().should.eql('people'))); + .replace('2023.1.0', '2023.1.123')) + .should.be.fulfilled()); it('should reject probable future version', () => getDataset(testData.forms.simpleEntity @@ -53,7 +53,7 @@ describe('parsing dataset from entity block', () => { - + @@ -63,7 +63,7 @@ describe('parsing dataset from entity block', () => { `; return getDataset(xml).then((res) => - res.get().should.eql('foo')); + res.get().name.should.eql('foo')); }); it('should retrieve the name of a dataset with namespace prefix on dataset attribute ', () => { @@ -77,7 +77,7 @@ describe('parsing dataset from entity block', () => { - + @@ -87,7 +87,7 @@ describe('parsing dataset from entity block', () => { `; return getDataset(xml).then((res) => - res.get().should.eql('foo')); + res.get().name.should.eql('foo')); }); it('should find dataset name even if other fields are in meta block before entity block', () => { @@ -104,7 +104,7 @@ describe('parsing dataset from entity block', () => { - + @@ -115,7 +115,7 @@ describe('parsing dataset from entity block', () => { `; return getDataset(xml).then((res) => - res.get().should.eql('bar')); + res.get().name.should.eql('bar')); }); it('should return rejected promise if dataset name is missing', () => { @@ -129,7 +129,7 @@ describe('parsing dataset from entity block', () => { - +