From b49c6d4fdedcab44ffb901668319957887396c1c Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Sun, 29 Sep 2024 17:16:48 +0000 Subject: [PATCH 1/6] retooling migration --- .../migrations/20240925-01-retool-authz.js | 286 ++++++++++++++++++ 1 file changed, 286 insertions(+) create mode 100644 lib/model/migrations/20240925-01-retool-authz.js diff --git a/lib/model/migrations/20240925-01-retool-authz.js b/lib/model/migrations/20240925-01-retool-authz.js new file mode 100644 index 000000000..723c59c4d --- /dev/null +++ b/lib/model/migrations/20240925-01-retool-authz.js @@ -0,0 +1,286 @@ +// Copyright 2024 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. + +// From earlier form-purging, there could have been leftover client audits +// that were not properly purged because of how they joined to submissions. +// This migration deletes those client audit rows, allowing the blobs to finally +// be de-referenced and also eventually purged (by the puring cron job). + +const up = (db) => db.raw(` +BEGIN; + +-- Species as citizens. Table for species declarations + +CREATE TABLE species ( + -- 36 chars as that's what's in the "actees" table currently, we'll just roll with that, someone thought it reasonable + name varchar(36) CHECK (name ~* '^(\\*|[a-z_]+)$') PRIMARY KEY +); + + +-- Verbs are about a species (as a subject/actee). +-- Though they don't have to be placed on the species that is its subject. +-- For instance, one can place a form.read permission on a project species, +-- and have that project's forms inherit a role's permission to form.read +-- from the project it is part of (via "actees" table). +-- Exclude "open_form.(read|list)" permissions - we'll reformulate those in terms of +-- a "read_open" and "list_open" verb later on. +WITH theverbs AS ( + SELECT DISTINCT + (jsonb_array_elements_text(verbs)) AS verb + FROM + roles + ORDER BY + verb + ) +INSERT INTO species SELECT DISTINCT + (substring(verb FROM '^[a-zA-Z]+')) AS species_substring +FROM + theverbs +WHERE + verb NOT LIKE 'open_form.%' +ORDER BY + species_substring; + +-- Existing species as mentioned in actees +INSERT INTO species ( + SELECT + species + FROM + actees + ORDER BY + species) +ON CONFLICT (name) + DO NOTHING; + +-- Other discovered species +INSERT INTO species VALUES + ('field_key'), + ('public_link'), + ('singleUse'), + ('actor') +ON CONFLICT (name) + DO NOTHING; + +-- Now we can make the actees table have a proper foreign key to species +ALTER TABLE actees + ADD CONSTRAINT "actees_species_foreign" FOREIGN KEY (species) REFERENCES species (name) +; + + +-- Verbs as citizens. Table: +CREATE TABLE authorization_verbs ( + species varchar(36) NOT NULL REFERENCES species (name), + verbname varchar(36) NOT NULL CHECK (verbname = '*' OR verbname ~ '^[a-z_\\.]+$'), + CONSTRAINT "authorization_verbs_primary_key" PRIMARY KEY ("species", "verbname") +); + + +-- Fill up the verbs table +INSERT INTO authorization_verbs +SELECT + transplant.* +FROM ( + WITH theverbs AS ( + SELECT DISTINCT + (jsonb_array_elements_text(verbs)) AS verb + FROM + roles + ) + SELECT + substring(verb FROM '^[a-zA-Z_]*') AS speciespart, + substring(verb FROM '^[^\\.]+\\.(.*)$') AS verbnamepart + FROM + theverbs + WHERE + verb NOT LIKE 'open_form.%' + ORDER BY + speciespart, + verbnamepart ASC +) as transplant; + +-- Add "form.list_open" and "form.read_open", related: https://github.com/getodk/central-backend/pull/968#pullrequestreview-1621236723 +INSERT INTO authorization_verbs + VALUES + ('form', 'read_open'), + ('form', 'list_open') +; + + +-- Table for implied authorization verbs +CREATE TABLE authorization_verbs_implied ( + species varchar(36) NOT NULL, + verbname varchar(36) NOT NULL, + implied_species varchar(36) NOT NULL, + implied_verbname varchar(36) NOT NULL, + CHECK ( (species, verbname) != (implied_species, implied_verbname) ), + CONSTRAINT authorization_verbs_implied_fk_implicator FOREIGN KEY (species, verbname) REFERENCES authorization_verbs (species, verbname) ON DELETE CASCADE, + CONSTRAINT authorization_verbs_implied_fk_implicatee FOREIGN KEY (implied_species, implied_verbname) REFERENCES authorization_verbs (species, verbname) ON DELETE CASCADE, + CONSTRAINT authorization_verbs_implied_primary_key PRIMARY KEY (species, verbname, implied_species, implied_verbname) +); + + + +-- If you can list or read a form (in general), you can list or read it regardless of whether it's open. +-- related: https://github.com/getodk/central-backend/pull/968#pullrequestreview-1621236723 +INSERT INTO authorization_verbs_implied + VALUES + ('form', 'read', 'form', 'read_open'), + ('form', 'list', 'form', 'list_open') +; + + +-- We need a table to store the role-permission associations +CREATE TABLE role_verbs ( + id integer PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, + role_id integer NOT NULL REFERENCES roles (id) ON DELETE CASCADE, + species varchar(36) NOT NULL, + verbname varchar(36) NOT NULL, + CONSTRAINT role_verbs_fk_authorization_verb FOREIGN KEY (species, verbname) REFERENCES authorization_verbs (species, verbname) ON DELETE CASCADE, + CONSTRAINT role_verbs_unique_triple UNIQUE (role_id, species, verbname) +); + +-- Fill that table with the associations, reaped from the "roles" table... +INSERT INTO role_verbs (role_id, species, verbname) +SELECT + transplant.* +FROM ( + WITH theverbs_split AS ( + WITH theverbs AS ( + SELECT + id AS role_id, + (jsonb_array_elements_text(verbs)) AS verb + FROM + roles + ) + SELECT + role_id, + substring(verb FROM '^[a-zA-Z_]*') AS species, + substring(verb FROM '[^\\.]+\\.(.*)') AS verbname + FROM + theverbs + ) + SELECT + role_id, + CASE species + WHEN 'open_form' THEN + 'form' + ELSE + species + END AS species, + CASE species + WHEN 'open_form' THEN + verbname || '_open' + ELSE + verbname + END AS verbname + FROM + theverbs_split + ORDER BY + role_id, + species, + verbname +) as transplant; + +-- We'll put a view in place of the table, and so the table needs to be renamed. +ALTER TABLE roles RENAME TO role_definitions; +-- We don't need the "verbs" column anymore; we've store them in the "role_verbs" table now +ALTER TABLE role_definitions DROP COLUMN verbs; + + +-- Helper view that calculates all (directly, or transitively) implied permissions +CREATE VIEW "public"."authorization_verbs_implication_closure" AS + WITH RECURSIVE implication_closure ( + species, + verbname, + species_next, + verbname_next + ) AS ( + SELECT + av.species, + av.verbname, + avedges1.implied_species, + avedges1.implied_verbname + FROM + authorization_verbs av + LEFT OUTER JOIN + authorization_verbs_implied avedges1 + ON ( + (av.species, av.verbname) + = + (avedges1.species, avedges1.verbname) + ) + UNION ALL + SELECT + implication_closure.species, + implication_closure.verbname, + avedges2.implied_species, + avedges2.implied_verbname + FROM + implication_closure + INNER JOIN + authorization_verbs_implied avedges2 + ON ( + (implication_closure.species_next, implication_closure.verbname_next) + = + (avedges2.species, avedges2.verbname) + ) + ) + SELECT + ic.species, + ic.verbname, + coalesce(ic.species_next, ic.species) AS implied_species, -- regularize: a verb implies itself + coalesce(ic.verbname_next, ic.verbname) AS implied_verbname -- regularize: a verb implies itself + FROM + implication_closure ic + UNION -- For regularity, add the un-expanded variants in + SELECT + species, + verbname, + species, + verbname + FROM + authorization_verbs +; + + +CREATE VIEW "public"."roles" AS + WITH roleverbs_unique AS ( + WITH roleverbs_implied AS ( + SELECT + rv.role_id, + imps.implied_species AS species, + imps.implied_verbname AS verbname + FROM + role_verbs rv + NATURAL JOIN authorization_verbs_implication_closure imps + ) + SELECT + * + FROM + roleverbs_implied + GROUP BY + (role_id, species, verbname) + ) + SELECT + rd.*, + jsonb_agg(format('%s.%s', rvu.species, rvu.verbname)) AS verbs + FROM + role_definitions rd + INNER JOIN roleverbs_unique rvu ON (rd.id = rvu.role_id) + GROUP BY + rd.id +; + + +COMMIT; +`); + +const down = () => {}; + +module.exports = { up, down }; From 89ceb67a2fddc4bdbee445712f0068bd2e3f723a Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Sun, 29 Sep 2024 17:20:44 +0000 Subject: [PATCH 2/6] fix aggregation --- lib/model/query/analytics.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/model/query/analytics.js b/lib/model/query/analytics.js index 40a8baba0..10d63755e 100644 --- a/lib/model/query/analytics.js +++ b/lib/model/query/analytics.js @@ -152,7 +152,7 @@ left join ( where au."loggedAt" >= ${_cutoffDate} group by au."actorId" ) as activeUsers on activeUsers."actorId" = u."actorId" -group by (p."id", r."id", r."name")`); +group by (p."id", r."id", r."system")`); const countAppUsers = () => ({ all }) => all(sql` select fk."projectId", count(fk."actorId") as total, count(recentsubs."activeActorId") as recent From 0ee78ed0f6f14f71f2c81c00e03e4cc0bf1692ab Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Sun, 29 Sep 2024 20:44:14 +0000 Subject: [PATCH 3/6] adapt tests for new permission storage structure --- test/integration/api/datasets.js | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/test/integration/api/datasets.js b/test/integration/api/datasets.js index b854a5de3..d48d10c7c 100644 --- a/test/integration/api/datasets.js +++ b/test/integration/api/datasets.js @@ -3963,7 +3963,7 @@ describe('datasets and entities', () => { describe('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')`); + await run(sql`DELETE FROM role_verbs WHERE species = 'dataset' AND verbname = 'create' AND role_id = (SELECT id FROM roles WHERE system = 'manager')`); const asBob = await service.login('bob'); @@ -3982,7 +3982,7 @@ describe('datasets and entities', () => { 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')`); + await run(sql`DELETE FROM role_verbs WHERE species = 'dataset' AND verbname = 'create' AND role_id = (SELECT id FROM roles WHERE system = 'manager')`); // Alice can upload first version of form that creates unpublished "people" dataset await asAlice.post('/v1/projects/1/forms') @@ -4001,7 +4001,7 @@ describe('datasets and entities', () => { 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')`); + await run(sql`DELETE FROM role_verbs WHERE species = 'dataset' AND verbname = 'create' AND role_id = (SELECT id FROM roles WHERE system = 'manager')`); // Alice can upload first version of form that creates unpublished "people" dataset await asAlice.post('/v1/projects/1/forms') @@ -4019,7 +4019,7 @@ describe('datasets and entities', () => { 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')`); + await run(sql`DELETE FROM role_verbs WHERE species = 'dataset' AND verbname = 'create' AND role_id = (SELECT id FROM roles WHERE system = 'manager')`); // Alice can upload first version of form await asAlice.post('/v1/projects/1/forms?publish=true') @@ -4036,7 +4036,7 @@ describe('datasets and entities', () => { 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')`); + await run(sql`DELETE FROM role_verbs WHERE species = 'dataset' AND verbname = 'create' AND role_id = (SELECT id FROM roles WHERE system = 'manager')`); // Alice can upload first version of form await asAlice.post('/v1/projects/1/forms') @@ -4054,8 +4054,7 @@ describe('datasets and entities', () => { 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 run(sql`DELETE FROM role_verbs WHERE species = 'dataset' AND verbname = 'update' AND role_id = (SELECT id FROM roles WHERE system = 'manager')`); await asAlice.post('/v1/projects/1/datasets') .send({ name: 'people' }) @@ -4078,7 +4077,7 @@ describe('datasets and entities', () => { 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')`); + await run(sql`DELETE FROM role_verbs WHERE species = 'dataset' AND verbname = 'update' AND role_id = (SELECT id FROM roles WHERE system = 'manager')`); // Alice can upload first version of form await asAlice.post('/v1/projects/1/forms?publish=true') @@ -4095,7 +4094,7 @@ describe('datasets and entities', () => { 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')`); + await run(sql`DELETE FROM role_verbs WHERE species = 'dataset' AND verbname = 'update' AND role_id = (SELECT id FROM roles WHERE system = 'manager')`); // Alice can upload and publish first version of form await asAlice.post('/v1/projects/1/forms?publish=true') @@ -4120,7 +4119,7 @@ describe('datasets and entities', () => { 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')`); + await run(sql`DELETE FROM role_verbs WHERE species = 'dataset' AND verbname = 'update' AND role_id = (SELECT id FROM roles WHERE system = 'manager')`); // Alice can upload first version of form await asAlice.post('/v1/projects/1/forms?publish=true') @@ -4138,7 +4137,7 @@ describe('datasets and entities', () => { 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')`); + await run(sql`DELETE FROM role_verbs WHERE species = 'dataset' AND verbname = 'update' AND role_id = (SELECT id FROM roles WHERE system = 'manager')`); // Alice can create the dataset await asAlice.post('/v1/projects/1/datasets') From c6852114b683b3f92ba029a94daf240d63d4adfc Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Mon, 30 Sep 2024 12:34:35 +0000 Subject: [PATCH 4/6] fix more tests --- test/integration/api/projects.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/test/integration/api/projects.js b/test/integration/api/projects.js index ac17ee92d..37d74136b 100644 --- a/test/integration/api/projects.js +++ b/test/integration/api/projects.js @@ -422,8 +422,8 @@ describe('api: /projects', () => { body.verbs.should.eqlInAnyOrder([ // following verbs from role: formfill 'project.read', - 'open_form.list', - 'open_form.read', + 'form.list_open', + 'form.read_open', 'submission.create', ]); })))))); @@ -445,8 +445,8 @@ describe('api: /projects', () => { // following roles from formfill + viewer: 'project.read', // following roles from formfill only: - 'open_form.list', - 'open_form.read', + 'form.list_open', + 'form.read_open', 'submission.create', // following roles from viewer only: 'form.list', @@ -1652,8 +1652,8 @@ describe('api: /projects?forms=true', () => { // following roles from formfill + viewer: 'project.read', // following roles from formfill only: - 'open_form.list', - 'open_form.read', + 'form.list_open', + 'form.read_open', 'submission.create', // following roles from viewer only: 'form.list', @@ -1698,11 +1698,12 @@ describe('api: /projects?forms=true', () => { //none // following roles from app-user only: - 'open_form.read', + 'form.read_open', 'submission.create', // following roles from viewer only: 'project.read', 'form.list', + 'form.list_open', 'form.read', 'submission.read', 'submission.list', From dbf089255e533b176916ec1cefcdaf7445f37a25 Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Mon, 30 Sep 2024 12:37:16 +0000 Subject: [PATCH 5/6] roles table is now a view; no longer writable --- lib/model/frames.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/model/frames.js b/lib/model/frames.js index 3dace2844..0c735e34e 100644 --- a/lib/model/frames.js +++ b/lib/model/frames.js @@ -163,9 +163,9 @@ class PublicLink extends Frame.define( const Role = Frame.define( table('roles'), - 'id', readable, 'name', readable, writable, + 'id', readable, 'name', readable, 'system', readable, 'createdAt', readable, - 'updatedAt', readable, 'verbs', readable, writable + 'updatedAt', readable, 'verbs', readable ); class Session extends Frame.define( From a4c7078253e8e4d2924f5be64e266774b208d3df Mon Sep 17 00:00:00 2001 From: brontolosone <177225737+brontolosone@users.noreply.github.com> Date: Mon, 30 Sep 2024 12:42:19 +0000 Subject: [PATCH 6/6] fallout: open_form.* -> form.*_open name change, and remove redundancy since now we have formalized verb implication of (read, list) -> (read_open, list_open) --- lib/model/query/auth.js | 4 ---- lib/model/query/forms.js | 4 ++-- lib/model/query/projects.js | 2 +- lib/resources/forms.js | 6 ++---- 4 files changed, 5 insertions(+), 11 deletions(-) diff --git a/lib/model/query/auth.js b/lib/model/query/auth.js index 6280ee7d9..da74c8185 100644 --- a/lib/model/query/auth.js +++ b/lib/model/query/auth.js @@ -51,10 +51,6 @@ limit 1`) const canAssignRole = (actor, role, actee) => ({ Auth }) => Auth.verbsOn(actor.id, actee).then((hasArray) => { const has = new Set(hasArray); - // `open_form` is subset of `form` so if someone has grant access on `form` - // they should be able do it on `open_form` as well - if (has.has('form.list')) has.add('open_form.list'); - if (has.has('form.read')) has.add('open_form.read'); for (const required of role.verbs) if (!has.has(required)) return false; return true; }); diff --git a/lib/model/query/forms.js b/lib/model/query/forms.js index 476f3ce62..cbe6b1a38 100644 --- a/lib/model/query/forms.js +++ b/lib/model/query/forms.js @@ -528,7 +528,7 @@ inner join inner join projects on projects.id=forms."projectId" inner join (select "acteeId" from assignments - inner join (select id from roles where verbs ? 'form.read' or verbs ? 'open_form.read') as role + inner join (select id from roles where verbs ? 'form.read_open') as role on role.id=assignments."roleId" where "actorId"=${auth.actor.map((actor) => actor.id).orElse(-1)}) as assignment on assignment."acteeId" in ('*', 'form', projects."acteeId", forms."acteeId") @@ -616,7 +616,7 @@ inner join (select id, max(assignment."showDraft") as "showDraft", max(assignment."showNonOpen") as "showNonOpen" from projects inner join (select "acteeId", 0 as "showDraft", case when verbs ? 'form.read' then 1 else 0 end as "showNonOpen" from assignments - inner join (select id, verbs from roles where verbs ? 'form.read' or verbs ? 'open_form.read') as role + inner join (select id, verbs from roles where verbs ? 'form.read_open') as role on role.id=assignments."roleId" where "actorId"=${actorId} union all diff --git a/lib/model/query/projects.js b/lib/model/query/projects.js index 8005780e2..d2b1458cd 100644 --- a/lib/model/query/projects.js +++ b/lib/model/query/projects.js @@ -112,7 +112,7 @@ inner join where "actorId"=${actorId}) as assignment on assignment."acteeId" in ('*', 'project', projects."acteeId") group by id - having array_agg(distinct verb) @> array['project.read', 'form.list'] or array_agg(distinct verb) @> array['project.read', 'open_form.list'] + having array_agg(distinct verb) @> array['project.read', 'form.list_open'] ) as filtered on filtered.id=projects.id `} diff --git a/lib/resources/forms.js b/lib/resources/forms.js index fc2c0f913..6ee670576 100644 --- a/lib/resources/forms.js +++ b/lib/resources/forms.js @@ -29,9 +29,7 @@ const excelMimeTypes = { xlsx: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet' }; -const canReadForm = (auth, form) => (form.state === 'closed' - ? auth.canOrReject('form.read', form) - : auth.canOrReject(['open_form.read', 'form.read'], form)); +const canReadForm = (auth, form) => auth.canOrReject((form.state === 'closed' ? 'form.read' : 'form.read_open'), form); const streamAttachment = async (container, attachment, response) => { const { Blobs, Datasets, Entities } = container; @@ -63,7 +61,7 @@ module.exports = (service, endpoint) => { service.get('/projects/:projectId/forms', endpoint(({ Forms, Projects }, { auth, params, query, queryOptions }) => Projects.getById(params.projectId) .then(getOrNotFound) - .then((project) => auth.canOrReject(['form.list', 'open_form.list'], project)) + .then((project) => auth.canOrReject(['form.list', 'form.list_open'], project)) .then((project) => Forms.getByProjectId(auth, project.id, false, undefined, queryOptions, isTrue(query.deleted))))); // non-REST openrosa endpoint for project-specialized formlist.