Skip to content

Commit 210a0e3

Browse files
ktuitektuite
authored
Log creating and setting properties on app users and public links (#1830)
1 parent de1134a commit 210a0e3

8 files changed

Lines changed: 162 additions & 13 deletions

File tree

‎lib/model/query/actor-properties.js‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,15 @@ const { construct } = require('../../util/util');
55

66

77
// Creates a new actor property name for a project.
8-
const create = (projectId, name) => ({ one }) =>
8+
const create = (project, name) => ({ one }) =>
99
one(sql`
1010
INSERT INTO actor_properties ("projectId", "name")
11-
VALUES (${projectId}, ${name})
11+
VALUES (${project.id}, ${name})
1212
RETURNING *`)
1313
.then(construct(ActorProperty));
1414

15+
create.audit = (project, name) => (log) => log('actor_property.create', project, { name });
16+
1517
// Returns all actor property names for a project.
1618
const getAllForProject = (projectId) => ({ all }) =>
1719
all(sql`
@@ -39,15 +41,15 @@ ORDER BY ap.name`)
3941
// one to delete rows for null-valued properties, one to upsert non-null ones.
4042
// properties is a plain object mapping property names to values (string or null),
4143
// as returned by extractActorProperties() in lib/data/actor-properties.js.
42-
const setValuesForActor = (projectId, actorId, properties) => async ({ run }) => {
44+
const setValuesForActor = (projectId, fieldKeyOrLink, properties) => async ({ run }) => {
4345
const entries = Object.entries(properties);
4446
const toDelete = entries.filter(([, v]) => v === null).map(([name]) => name);
4547
const toUpsert = entries.filter(([, v]) => v !== null);
4648

4749
if (toDelete.length > 0) {
4850
await run(sql`
4951
DELETE FROM actor_property_values
50-
WHERE "actorId" = ${actorId}
52+
WHERE "actorId" = ${fieldKeyOrLink.actorId}
5153
AND "actorPropertyId" IN (
5254
SELECT id FROM actor_properties
5355
WHERE "projectId" = ${projectId} AND name = ANY(${sql.array(toDelete, 'text')})
@@ -57,12 +59,20 @@ WHERE "actorId" = ${actorId}
5759
if (toUpsert.length > 0) {
5860
await run(sql`
5961
INSERT INTO actor_property_values ("actorId", "actorPropertyId", "value")
60-
SELECT ${actorId}, ap.id, v.value
62+
SELECT ${fieldKeyOrLink.actorId}, ap.id, v.value
6163
FROM ${sql.unnest(toUpsert, ['text', 'text'])} AS v(name, value)
6264
JOIN actor_properties ap ON ap."projectId" = ${projectId} AND ap.name = v.name
6365
ON CONFLICT ("actorId", "actorPropertyId") DO UPDATE SET "value" = EXCLUDED."value"`);
6466
}
6567
};
6668

69+
setValuesForActor.audit = (_, fieldKeyOrLink, properties) => (log) => {
70+
if (Object.keys(properties).length > 0) {
71+
// Look up `public_link` or `field_key` type from fieldKeyOrLink
72+
const action = `${fieldKeyOrLink.constructor.actorType}.property.set`;
73+
return log(action, fieldKeyOrLink.actor, { properties });
74+
}
75+
};
76+
6777
module.exports = { create, getAllForProject, getAllForProjectWithValues, setValuesForActor };
6878

‎lib/model/query/audits.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,11 @@ const actionCondition = (action) => {
4242
else if (action === 'user')
4343
return sql`action in ('user.create', 'user.update', 'user.delete', 'user.assignment.create', 'user.assignment.delete', 'user.session.create')`;
4444
else if (action === 'field_key')
45-
return sql`action in ('field_key.create', 'field_key.assignment.create', 'field_key.assignment.delete', 'field_key.session.end', 'field_key.delete')`;
45+
return sql`action in ('field_key.create', 'field_key.assignment.create', 'field_key.assignment.delete', 'field_key.session.end', 'field_key.delete', 'field_key.property.set')`;
4646
else if (action === 'public_link')
47-
return sql`action in ('public_link.create', 'public_link.assignment.create', 'public_link.assignment.delete', 'public_link.session.end', 'public_link.delete')`;
47+
return sql`action in ('public_link.create', 'public_link.assignment.create', 'public_link.assignment.delete', 'public_link.session.end', 'public_link.delete', 'public_link.property.set')`;
4848
else if (action === 'project')
49-
return sql`action in ('project.create', 'project.update', 'project.delete')`;
49+
return sql`action in ('project.create', 'project.update', 'project.delete', 'actor_property.create')`;
5050
else if (action === 'form')
5151
return sql`action in ('form.create', 'form.update', 'form.delete', 'form.restore', 'form.purge', 'form.attachment.update', 'form.submission.export', 'form.update.draft.set', 'form.update.draft.delete', 'form.update.draft.replace', 'form.update.publish')`;
5252
else if (action === 'submission')

‎lib/resources/actor-properties.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ module.exports = (service, endpoint) => {
1414
if (name == null || !validateActorPropertyName(name))
1515
throw Problem.user.unexpectedValue({ field: 'name', value: name, reason: 'This is not a valid property name.' });
1616

17-
await ActorProperties.create(project.id, name);
17+
await ActorProperties.create(project, name);
1818
return success;
1919
}));
2020

‎lib/resources/app-users.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ module.exports = (service, endpoint) => {
3030
if (body.properties != null) {
3131
const knownProperties = await ActorProperties.getAllForProject(project.id);
3232
const properties = extractActorProperties(body.properties, knownProperties.map((p) => p.name));
33-
await ActorProperties.setValuesForActor(project.id, fk.actorId, properties);
33+
await ActorProperties.setValuesForActor(project.id, fk, properties);
3434
}
3535

3636
// Returns the raw FK from create() without new properties
@@ -64,7 +64,7 @@ module.exports = (service, endpoint) => {
6464
if (body.properties != null) {
6565
const knownProperties = await ActorProperties.getAllForProject(project.id);
6666
const properties = extractActorProperties(body.properties, knownProperties.map((p) => p.name));
67-
await ActorProperties.setValuesForActor(project.id, fk.actorId, properties);
67+
await ActorProperties.setValuesForActor(project.id, fk, properties);
6868
}
6969

7070
return FieldKeys.getByProjectAndActorId(project.id, params.id, QueryOptions.extended).then(getOrNotFound);

‎lib/resources/public-links.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ module.exports = (service, endpoint) => {
3030
if (body.properties != null) {
3131
const knownProperties = await ActorProperties.getAllForProject(form.projectId);
3232
const properties = extractActorProperties(body.properties, knownProperties.map((p) => p.name));
33-
await ActorProperties.setValuesForActor(form.projectId, pl.actorId, properties);
33+
await ActorProperties.setValuesForActor(form.projectId, pl, properties);
3434
}
3535

3636
// Returns the raw public link from create() without new properties
@@ -54,7 +54,7 @@ module.exports = (service, endpoint) => {
5454
if (body.properties != null) {
5555
const knownProperties = await ActorProperties.getAllForProject(form.projectId);
5656
const properties = extractActorProperties(body.properties, knownProperties.map((p) => p.name));
57-
await ActorProperties.setValuesForActor(form.projectId, pl.actorId, properties);
57+
await ActorProperties.setValuesForActor(form.projectId, pl, properties);
5858
}
5959

6060
return PublicLinks.getByFormAndActorId(form.id, params.id, QueryOptions.extended).then(getOrNotFound);

‎test/integration/api/actor-properties.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,21 @@ describe('api: /projects/:id/actor-properties', () => {
1616
.expect(200);
1717
}));
1818

19+
it('should log the property creation in the audit log', testService(async (service, container) => {
20+
const asAlice = await service.login('alice');
21+
await asAlice.post('/v1/projects/1/actor-properties')
22+
.send({ name: 'region' })
23+
.expect(200);
24+
25+
const project = await container.Projects.getById(1).then((o) => o.get());
26+
27+
const { body: audits } = await asAlice.get('/v1/audits?action=actor_property.create').expect(200);
28+
audits.length.should.equal(1);
29+
audits[0].actorId.should.equal(5); // alice
30+
audits[0].acteeId.should.equal(project.acteeId);
31+
audits[0].details.should.eql({ name: 'region' });
32+
}));
33+
1934
it('should reject if name is missing', testService(async (service) => {
2035
const asAlice = await service.login('alice');
2136
await asAlice.post('/v1/projects/1/actor-properties')

‎test/integration/api/app-users.js‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,68 @@ describe('api: /projects/:id/app-users', () => {
8080
body.properties.should.eql({ region: 'north' });
8181
});
8282
}));
83+
84+
it('should log the property set in the audit log', testService(async (service, container) => {
85+
const asAlice = await service.login('alice');
86+
await asAlice.post('/v1/projects/1/actor-properties').send({ name: 'region' }).expect(200);
87+
88+
const { body: appUser } = await asAlice.post('/v1/projects/1/app-users')
89+
.send({
90+
displayName: 'test user',
91+
properties: { region: 'north' }
92+
})
93+
.expect(200);
94+
95+
const actor = await container.Actors.getById(appUser.id).then((o) => o.get());
96+
97+
const { body: audits } = await asAlice.get('/v1/audits?action=field_key.property.set').expect(200);
98+
audits.length.should.equal(1);
99+
audits[0].actorId.should.equal(5); // alice
100+
audits[0].acteeId.should.equal(actor.acteeId);
101+
audits[0].details.properties.should.eql({ region: 'north' });
102+
}));
103+
104+
it('should not log add audit log event if property set is empty', testService(async (service) => {
105+
const asAlice = await service.login('alice');
106+
await asAlice.post('/v1/projects/1/actor-properties').send({ name: 'region' }).expect(200);
107+
108+
await asAlice.post('/v1/projects/1/app-users')
109+
.send({
110+
displayName: 'test user',
111+
properties: { }
112+
})
113+
.expect(200);
114+
115+
const { body: audits } = await asAlice.get('/v1/audits?action=field_key.property.set').expect(200);
116+
audits.length.should.equal(0);
117+
}));
118+
119+
it('should log when a property is unset in the audit log', testService(async (service, container) => {
120+
const asAlice = await service.login('alice');
121+
await asAlice.post('/v1/projects/1/actor-properties').send({ name: 'region' }).expect(200);
122+
await asAlice.post('/v1/projects/1/actor-properties').send({ name: 'team' }).expect(200);
123+
124+
const { body: appUser } = await asAlice.post('/v1/projects/1/app-users')
125+
.send({
126+
displayName: 'test user',
127+
properties: { team: 'abc', region: 'north' }
128+
})
129+
.expect(200);
130+
131+
// unset a property
132+
await asAlice.patch(`/v1/projects/1/app-users/${appUser.id}`)
133+
.send({ properties: { region: '' } })
134+
.expect(200);
135+
136+
const actor = await container.Actors.getById(appUser.id).then((o) => o.get());
137+
138+
const { body: audits } = await asAlice.get('/v1/audits?action=field_key.property.set').expect(200);
139+
audits.length.should.equal(2);
140+
audits[0].actorId.should.equal(5); // alice
141+
audits[0].acteeId.should.equal(actor.acteeId);
142+
143+
audits[0].details.properties.should.eql({ region: null });
144+
}));
83145
});
84146

85147
describe('GET', () => {
@@ -432,6 +494,27 @@ describe('api: /projects/:id/app-users', () => {
432494
// The property-setting logic (validation, coercion, bulk upsert/delete) is shared
433495
// under the hood with public links. The tests above cover it fully; public-links
434496
// tests only verify the happy-path integration without re-testing every edge case.
497+
498+
it('should log the property set in the audit log', testService(async (service, container) => {
499+
const asAlice = await service.login('alice');
500+
await asAlice.post('/v1/projects/1/actor-properties').send({ name: 'region' }).expect(200);
501+
502+
const { body: appUser } = await asAlice.post('/v1/projects/1/app-users')
503+
.send({ displayName: 'test user' })
504+
.expect(200);
505+
506+
await asAlice.patch(`/v1/projects/1/app-users/${appUser.id}`)
507+
.send({ properties: { region: 'north' } })
508+
.expect(200);
509+
510+
const actor = await container.Actors.getById(appUser.id).then((o) => o.get());
511+
512+
const { body: audits } = await asAlice.get('/v1/audits?action=field_key.property.set').expect(200);
513+
audits.length.should.equal(1);
514+
audits[0].actorId.should.equal(5); // alice
515+
audits[0].acteeId.should.equal(actor.acteeId);
516+
audits[0].details.properties.should.eql({ region: 'north' });
517+
}));
435518
});
436519

437520
describe('/:id DELETE', () => {

‎test/integration/api/public-links.js‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,26 @@ describe('api: /projects/:id/forms/:id/public-links', () => {
7979
body.properties.should.eql({ region: 'north' });
8080
});
8181
}));
82+
83+
it('should log the property set in the audit log', testService(async (service, container) => {
84+
const asAlice = await service.login('alice');
85+
await asAlice.post('/v1/projects/1/actor-properties').send({ name: 'region' }).expect(200);
86+
87+
const { body: pl } = await asAlice.post('/v1/projects/1/forms/simple/public-links')
88+
.send({
89+
displayName: 'test link',
90+
properties: { region: 'north' }
91+
})
92+
.expect(200);
93+
94+
const actor = await container.Actors.getById(pl.id).then((o) => o.get());
95+
96+
const { body: audits } = await asAlice.get('/v1/audits?action=public_link.property.set').expect(200);
97+
audits.length.should.equal(1);
98+
audits[0].actorId.should.equal(5); // alice
99+
audits[0].acteeId.should.equal(actor.acteeId);
100+
audits[0].details.properties.should.eql({ region: 'north' });
101+
}));
82102
});
83103

84104
describe('GET', () => {
@@ -313,6 +333,27 @@ describe('api: /projects/:id/forms/:id/public-links', () => {
313333
}));
314334

315335
// Additional behavior tested in app-users test because the machinery of setting properties is the same.
336+
337+
it('should log the property set in the audit log', testService(async (service, container) => {
338+
const asAlice = await service.login('alice');
339+
await asAlice.post('/v1/projects/1/actor-properties').send({ name: 'region' }).expect(200);
340+
341+
const { body: pl } = await asAlice.post('/v1/projects/1/forms/simple/public-links')
342+
.send({ displayName: 'test link' })
343+
.expect(200);
344+
345+
await asAlice.patch(`/v1/projects/1/forms/simple/public-links/${pl.id}`)
346+
.send({ properties: { region: 'north' } })
347+
.expect(200);
348+
349+
const actor = await container.Actors.getById(pl.id).then((o) => o.get());
350+
351+
const { body: audits } = await asAlice.get('/v1/audits?action=public_link.property.set').expect(200);
352+
audits.length.should.equal(1);
353+
audits[0].actorId.should.equal(5); // alice
354+
audits[0].acteeId.should.equal(actor.acteeId);
355+
audits[0].details.properties.should.eql({ region: 'north' });
356+
}));
316357
});
317358

318359
describe('/:id DELETE', () => {

0 commit comments

Comments
 (0)