Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Authz reification of species and verbs, and verb implication #1205

Closed
wants to merge 6 commits into from
Closed

Authz reification of species and verbs, and verb implication #1205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b49c6d4
retooling migration
brontolosone Sep 29, 2024
89ceb67
fix aggregation
brontolosone Sep 29, 2024
0ee78ed
adapt tests for new permission storage structure
brontolosone Sep 29, 2024
c685211
fix more tests
brontolosone Sep 30, 2024
dbf0892
roles table is now a view; no longer writable
brontolosone Sep 30, 2024
a4c7078
fallout: open_form.* -> form.*_open name change, and remove redundanc…
brontolosone Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/model/frames.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines -166 to -168
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch. I don't think these should be here even today. writable means "writable via the API", but none of these fields should be writable in that way. To be clear, I don't think it's a vulnerability: the only role endpoints are GET endpoints, so there shouldn't be any endpoints that write to roles. That said, there's no reason for writable to be here, so it's probably best to remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I bet writable was added originally with the idea that we would soon add support for user-creatable custom roles. If endpoints like that existed, name and verbs would indeed need to be writable. I still think we might as well remove writable, as we aren't planning to add custom roles in the near term. All this said though, I also don't think this change is needed for the rest of your PR.

'updatedAt', readable, 'verbs', readable
);

class Session extends Frame.define(
Expand Down
286 changes: 286 additions & 0 deletions lib/model/migrations/20240925-01-retool-authz.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 };
2 changes: 1 addition & 1 deletion lib/model/query/analytics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")`);
Copy link
Member

@matthew-white matthew-white Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing that this worked before because r.id is the primary key. I doubt either r.name or r.system was strictly necessary here, as if you're grouping by r.id, you're automatically grouping by r.id, r.system (r.system doesn't do any additional grouping beyond what r.id already does). For example, this query works even though it groups by projects.id, not projects.id, projects.name:

SELECT projects.id, projects.name, count(*) AS form_count
FROM projects
JOIN forms ON forms."projectId" = projects.id
GROUP by projects.id
LIMIT 5;

Maybe if roles is a view, r.id doesn't work in this same way?

Copy link
Contributor Author

@brontolosone brontolosone Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That explanation makes sense, indeed in the case of a view there is no primary key anymore, and no way for the DB to infer that you don't need to group by all that.

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
Expand Down
4 changes: 0 additions & 4 deletions lib/model/query/auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
Expand Down
4 changes: 2 additions & 2 deletions lib/model/query/forms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/model/query/projects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
`}
Expand Down
6 changes: 2 additions & 4 deletions lib/resources/forms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
Loading