Authz reification of species and verbs, and verb implication

[brontolosone]

Related: getodk/central-frontend#1032

As discussed in yesterday's Meet. For discussion, also see Slack.
This is a stripped down version of this wider exploratory work, keeping just the bits that may be useful right now.

This gives us:

• reified species
• reified verbs
• a rename of verbs open_form.* -> form.*_open (form is a species, open_form isn't)
• verb implication (eg form.read -> form.read_open), which cleans up the code a bit, as you'll only have to check for form.read_open without having to check for other potentially relevant verbs - that thinking work should be done once, and centrally — and now we can. It saves a bunch of cognitive load. We might also want hunt for other "verb implications" that may currently be lurking in ad-hoc dispersed code, and regularize those by putting them into the authorization_verbs_implied registry (and then cleaning up the code).

The whole thing is compatible with existing usage of the roles table, which is now a view.

Commit 687caaa is unrelated to the wider endeavour but my tests were failing on the original code and I couldn't understand how it could have worked in the first place 🤔

There is a companion frontend PR: getodk/central-frontend#1032 with some minor changes that have to do with renamed verbs.

[matthew-white]

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.

[matthew-white]

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.

[matthew-white]

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?

[brontolosone]

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.

[matthew-white]

We might also want hunt for other "verb implications" that may currently be lurking in ad-hoc dispersed code

I think we do assume that if you can update a resource, you can read it. For example, this is the PATCH endpoint for a project, which checks for the verb project.update. At the end, the endpoint returns the patched project, allowing the user to read all fields on the project, even ones they didn't update.

central-backend/lib/resources/projects.js

Lines 47 to 51 in 82696a0

	service.patch('/projects/:id', endpoint(({ Projects }, { auth, body, params }) =>
	Projects.getById(params.id)
	.then(getOrNotFound)
	.then((project) => auth.canOrReject('project.update', project))
	.then((project) => Projects.update(project, Project.fromApi(body)))));

I'm not sure that pattern really needs to be reified in a table, but I wanted to point it out.

Other than that, no verb implications really come to mind. I think the open_form.* verbs are a rare example of that pattern.

[brontolosone]

Slack thread touching on how to go forward with authz in the immediate future. Closing!

[matthew-white]

@brontolosone, would you be up for creating a separate PR for removing writable from the Role frame? It's confusing to see it there, and it's not needed, so I think it'd be a good idea to remove it. We can always add it back if we need to (e.g., if we decide to implement getodk/central#737).