Skip to content

Commit e819ca0

Browse files
brontolosonektuite
brontolosone
and
ktuite
authored
OpenRosa attachment update auditing (#1646)
* add failing test: OpenRosa attachment addition submission is not auditlogged * blobs: refactor ensure() - Make inserting a blob which already exists a true no-op (saves DB churn; the sha update of the replaced approach caused a row copy) - Remove comment related to force-a-write-so-we-can-return-the-id (as we do it differently now) - Remove comment about avoiding a TOCTOU issue by sending the file over (potentially unnecessarily). 1. There was almost a TOCTOU issue anyway, but it was elided by touching every used row, which makes concurrent deleters/updaters of that row block until our transaction commit. 2. Avoiding sending the file over *and* avoiding a TOCTOU should be doable with a row lock or advisory lock. * OpenRosa submissions: auditlog updates to attachment set, also on subsequent POSTs (continuations of attachment uploads). Fixes getodk/central#1426 * Handle resubmitting or altering attachment when resending submission with missing attachment --------- Co-authored-by: Kathleen Tuite <[email protected]>
1 parent 5e1b21f commit e819ca0

File tree

5 files changed

+158
-25
lines changed

5 files changed

+158
-25
lines changed

‎docs/api.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -931,7 +931,7 @@ tags:
931931
* `submission.create` when a new Submission is created.
932932
* `submission.update` when a Submission's metadata is updated.
933933
* `submission.update.version` when a Submission XML data is updated.
934-
* `submission.attachment.update` when a Submission Attachment binary is set or cleared, but _only via the REST API_. Attachments created alongside the submission over the OpenRosa `/submission` API (including submissions from Collect) do not generate audit log entries.
934+
* `submission.attachment.update` when a Submission Attachment binary is set or cleared.
935935
* `submission.delete` when a Submission is soft-deleted.
936936
* `submission.purge` when soft-deleted Submissions are purged.
937937
* `submission.restore` when a Submission is restored.

‎lib/model/query/blobs.js‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,20 +12,21 @@ const { isEmpty, map } = require('ramda');
1212
const { Blob } = require('../frames');
1313
const { construct } = require('../../util/util');
1414

15-
// 1. there may be a better way to do this. with this approach, we always
16-
// ship the bits to the database, even if they're already in there. shipping
17-
// bits is expensive and slow.
18-
// (but the old select-insert way was two separate round-trips in the did-not-exist
19-
// case, which wasn't great either. and it has concurrency issues.)
20-
// 2. we /have/ to do an update on conflict in order for values to return.
21-
// so we just set the sha back to what we already know it is.
15+
const DEFAULT_CONTENT_TYPE = 'application/octet-stream';
16+
2217
const ensure = (blob) => ({ oneFirst }) => oneFirst(sql`
23-
with ensured as
24-
(insert into blobs (sha, md5, content, "contentType") values
25-
(${blob.sha}, ${blob.md5}, ${sql.binary(blob.content)}, ${blob.contentType || sql`DEFAULT`})
26-
on conflict (sha, "contentType") do update set sha = EXCLUDED.sha
27-
returning id)
28-
select id from ensured`);
18+
WITH extant AS (
19+
SELECT id FROM blobs WHERE (sha, "contentType") = (${blob.sha}, ${blob.contentType || DEFAULT_CONTENT_TYPE})
20+
),
21+
ensured AS (
22+
INSERT INTO blobs (sha, md5, content, "contentType") VALUES (
23+
${blob.sha}, ${blob.md5}, ${sql.binary(blob.content)}, ${blob.contentType || sql`DEFAULT`}
24+
)
25+
ON CONFLICT (sha, "contentType") DO NOTHING
26+
RETURNING id
27+
)
28+
SELECT coalesce((SELECT id FROM ensured), (SELECT id FROM extant)) as id
29+
`);
2930

3031
const getById = (blobId) => ({ maybeOne }) =>
3132
maybeOne(sql`select * from blobs where id=${blobId}`)

‎lib/model/query/submission-attachments.js‎

Lines changed: 36 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -100,21 +100,48 @@ const create = (submission, form, binaryFields, files = [], deprecated = []) =>
100100
});
101101
};
102102

103-
const upsert = (def, files) => ({ Blobs, SubmissionAttachments }) =>
103+
const upsert = (submission, form, def, files) => ({ run, Blobs, SubmissionAttachments, context }) =>
104104
SubmissionAttachments.getAllByDefId(def.id)
105105
.then((expecteds) => {
106106
const lookup = new Set(expecteds.map((att) => att.name));
107107
const present = files.filter((file) => lookup.has(file.fieldname));
108-
return Promise.all(present
109-
.map((file) => Blobs.ensure(Blob.fromBuffer(file.buffer, file.mimetype || defaultMimetypeFor(file.fieldname)))
110-
.then((blobId) => SubmissionAttachments.attach(def.id, file.fieldname, blobId))));
108+
return Promise.all(
109+
present.map((file) => Blobs.ensure(Blob.fromBuffer(file.buffer, file.mimetype || defaultMimetypeFor(file.fieldname)))
110+
.then((blobId) => SubmissionAttachments.attach(def.id, file.fieldname, blobId))
111+
)
112+
).then(linkResults =>
113+
// auditlog the updates (only those which were added or of which the content changed)
114+
linkResults
115+
.filter(el => el.blobNow && el.blobBefore !== el.blobNow)
116+
.map(linkResult =>
117+
Audit.of(
118+
context.auth.actor,
119+
'submission.attachment.update',
120+
form,
121+
{
122+
instanceId: submission.instanceId,
123+
submissionDefId: def.id,
124+
name: linkResult.name,
125+
newBlobId: linkResult.blobNow,
126+
}
127+
)
128+
)
129+
).then(auditsInSpe => run(insertMany(auditsInSpe)));
111130
});
112131

113-
const attach = (defId, name, blobId) => ({ run }) => run(sql`
114-
update submission_attachments set "blobId"=${blobId}
115-
where "submissionDefId"=${defId} and name=${name}`);
116-
117-
// TODO: this is currently audit logged in resource/submissions. probably deal w it here instead.
132+
const attach = (defId, name, blobId) => ({ one }) => one(sql`
133+
WITH extant AS (
134+
SELECT "blobId"
135+
FROM submission_attachments
136+
WHERE "submissionDefId"=${defId} AND name=${name}
137+
),
138+
attached AS (
139+
UPDATE submission_attachments SET "blobId"=${blobId}
140+
WHERE "submissionDefId"=${defId} AND name=${name} AND "blobId" IS DISTINCT FROM ${blobId}
141+
RETURNING "blobId"
142+
)
143+
SELECT ${name} as name, (SELECT "blobId" FROM extant) as "blobBefore", (SELECT "blobId" FROM attached) as "blobNow"
144+
`);
118145

119146
const clear = (sa) => ({ run }) => run(sql`
120147
update submission_attachments set "blobId"=null

‎lib/resources/submissions.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ module.exports = (service, endpoint, anonymousEndpoint) => {
122122
// EDITED SUBMISSION ATTACHMENT UPSERT REQUEST: check safety then do that.
123123
if (Buffer.compare(Buffer.from(partial.xml), Buffer.from(usurper.xml)) !== 0)
124124
throw Problem.user.xmlConflict();
125-
return SubmissionAttachments.upsert(usurper, files);
125+
return SubmissionAttachments.upsert(partial, form, usurper, files);
126126
})
127127
// SUBMISSION EDIT REQUEST: find the now-deprecated submission and supplant it.
128128
: Promise.all([
@@ -140,7 +140,7 @@ module.exports = (service, endpoint, anonymousEndpoint) => {
140140
// ATTACHMENT UPSERT REQUEST: check safety and add attachments to the extant def.
141141
if (extant.current !== true) throw Problem.user.deprecatingOldSubmission({ instanceId: partial.instanceId });
142142
if (Buffer.compare(Buffer.from(partial.xml), Buffer.from(extant.xml)) !== 0) throw Problem.user.xmlConflict();
143-
return SubmissionAttachments.upsert(extant, files);
143+
return SubmissionAttachments.upsert(partial, form, extant, files);
144144
}
145145
// NEW SUBMISSION REQUEST: create a new submission and attachments.
146146
return Promise.all([

‎test/integration/api/audits.js‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -864,5 +864,110 @@ describe('/audits', () => {
864864
}));
865865
}));
866866
});
867+
868+
describe('audit logs of OpenRosa submission attachment events', () => {
869+
it('should get the attachment events of a (partial) submission', testService((service) =>
870+
service.login('alice', (asAlice) =>
871+
asAlice.post('/v1/projects/1/forms?publish=true')
872+
.set('Content-Type', 'application/xml')
873+
.send(testData.forms.binaryType)
874+
.expect(200)
875+
.then(() => asAlice.post('/v1/projects/1/submission')
876+
.set('X-OpenRosa-Version', '1.0')
877+
.attach('xml_submission_file', Buffer.from(testData.instances.binaryType.both), { filename: 'data.xml' })
878+
.attach('my_file1.mp4', Buffer.from('this is test file one'), { filename: 'my_file1.mp4' })
879+
.expect(201)
880+
.then(() => asAlice.get('/v1/audits?action=submission.attachment.update')
881+
.expect(200))
882+
.then(({ body }) => {
883+
body[0].details.name.should.equal('my_file1.mp4');
884+
})
885+
)
886+
.then(() => asAlice.post('/v1/projects/1/submission')
887+
.set('X-OpenRosa-Version', '1.0')
888+
.attach('xml_submission_file', Buffer.from(testData.instances.binaryType.both), { filename: 'data.xml' })
889+
.attach('here_is_file2.jpg', Buffer.from('this is test file two'), { filename: 'here_is_file2.jpg' })
890+
.expect(201)
891+
.then(() => asAlice.get('/v1/audits?action=submission.attachment.update')
892+
.expect(200))
893+
.then(({ body }) => {
894+
body.length.should.equal(2);
895+
body[0].details.name.should.equal('here_is_file2.jpg');
896+
body[1].details.name.should.equal('my_file1.mp4');
897+
})
898+
)
899+
)
900+
));
901+
902+
it('should handle resubmitting partial submission with all attachments', testService((service) =>
903+
service.login('alice', (asAlice) =>
904+
asAlice.post('/v1/projects/1/forms?publish=true')
905+
.set('Content-Type', 'application/xml')
906+
.send(testData.forms.binaryType)
907+
.expect(200)
908+
.then(() => asAlice.post('/v1/projects/1/submission')
909+
.set('X-OpenRosa-Version', '1.0')
910+
.attach('xml_submission_file', Buffer.from(testData.instances.binaryType.both), { filename: 'data.xml' })
911+
.attach('my_file1.mp4', Buffer.from('this is test file one'), { filename: 'my_file1.mp4' })
912+
.expect(201)
913+
.then(() => asAlice.get('/v1/audits?action=submission.attachment.update')
914+
.expect(200))
915+
.then(({ body }) => {
916+
body[0].details.name.should.equal('my_file1.mp4');
917+
})
918+
)
919+
.then(() => asAlice.post('/v1/projects/1/submission')
920+
.set('X-OpenRosa-Version', '1.0')
921+
.attach('xml_submission_file', Buffer.from(testData.instances.binaryType.both), { filename: 'data.xml' })
922+
.attach('my_file1.mp4', Buffer.from('this is test file one'), { filename: 'my_file1.mp4' })
923+
.attach('here_is_file2.jpg', Buffer.from('this is test file two'), { filename: 'here_is_file2.jpg' })
924+
.expect(201)
925+
.then(() => asAlice.get('/v1/audits?action=submission.attachment.update')
926+
.expect(200))
927+
.then(({ body }) => {
928+
body.length.should.equal(2);
929+
body[0].details.name.should.equal('here_is_file2.jpg');
930+
body[1].details.name.should.equal('my_file1.mp4');
931+
})
932+
)
933+
)
934+
));
935+
936+
it('should handle resubmitting partial submission with contents of attachment changed', testService((service) =>
937+
service.login('alice', (asAlice) =>
938+
asAlice.post('/v1/projects/1/forms?publish=true')
939+
.set('Content-Type', 'application/xml')
940+
.send(testData.forms.binaryType)
941+
.expect(200)
942+
.then(() => asAlice.post('/v1/projects/1/submission')
943+
.set('X-OpenRosa-Version', '1.0')
944+
.attach('xml_submission_file', Buffer.from(testData.instances.binaryType.both), { filename: 'data.xml' })
945+
.attach('my_file1.mp4', Buffer.from('this is test file one'), { filename: 'my_file1.mp4' })
946+
.expect(201)
947+
.then(() => asAlice.get('/v1/audits?action=submission.attachment.update')
948+
.expect(200))
949+
.then(({ body }) => {
950+
body[0].details.name.should.equal('my_file1.mp4');
951+
})
952+
)
953+
.then(() => asAlice.post('/v1/projects/1/submission')
954+
.set('X-OpenRosa-Version', '1.0')
955+
.attach('xml_submission_file', Buffer.from(testData.instances.binaryType.both), { filename: 'data.xml' })
956+
.attach('my_file1.mp4', Buffer.from('file one contents have changed'), { filename: 'my_file1.mp4' })
957+
.attach('here_is_file2.jpg', Buffer.from('this is test file two'), { filename: 'here_is_file2.jpg' })
958+
.expect(201)
959+
.then(() => asAlice.get('/v1/audits?action=submission.attachment.update')
960+
.expect(200))
961+
.then(({ body }) => {
962+
body.length.should.equal(3);
963+
body[0].details.name.should.equal('here_is_file2.jpg');
964+
body[1].details.name.should.equal('my_file1.mp4');
965+
body[2].details.name.should.equal('my_file1.mp4');
966+
body[1].details.newBlobId.should.not.equal(body[2].details.newBlobId);
967+
})
968+
)
969+
)
970+
));
971+
});
867972
});
868973
});

0 commit comments

Comments
 (0)