Add a permissions system

[evansd]

This adds a simple, generic permissions system to ehrQL. In this first instance this is only used to restrict access to certain tables, but I anticipate extending it to e.g. gate access to Event Level Data and for the various opt-out controls.

ehrQL table definitions can now be annotated with metadata using an internal _meta class. This can included a required_permission attribute e.g.

@table
class some_table(EventFrame):
    class _meta:
        required_permission = "some_restricted_dataset"

A check at the documentation build stage ensures that every restricted table references the required permissions somewhere in the table documentation.

In Production

The RAP Controller already supplies an EHRQL_PERMISSIONS environment variable with a JSON-encoded list of permissions that the current job has. If a dataset or measure definition uses a restricted table without the relevant permission then this results in an immediate error, before any queries are executed. For example:

EHRQLPermissionError: You do not currently have all the permissions needed for this action.

Missing permissions are:

    * appointments: required for access to the `tpp.appointments` table
    * waiting_list: required for access to the `tpp.wl_clockstops` and `tpp.wl_openpathways` tables

If you think this is a mistake and that you should have these permissions please contact OpenSAFELY support.

ehrQL then exits with a specific status code which the RAP Agent can identify and use to inform the user that their code failed due to a permissions error.

Locally

Running the same code locally currently results in a warning in the logs:

Some of the tables or features you are using require special permission to use with real
patient data. The permissions needed are:

    * appointments: required for access to the `tpp.appointments` table
    * waiting_list: required for access to the `tpp.wl_clockstops` and `tpp.wl_openpathways` tables   

You can continue to work on your code using dummy data by “claiming” the required permisions:

    from ehrql import claim_permissions
    claim_permissions("appointments", "waiting_list")

The intention is that once this change has been circulated and users have had time to made the necessary changes we can make this a hard error rather than a warning so it will be impossible to miss in future.

See the documentation preview here:
https://evansd-table-permissions.databuilder.pages.dev/reference/language/#permissions

Note that the claim_permissions() function acts globally, rather than being a dataset/measure specific method. This seems like a better fit with the project-wide nature of permissions and less fiddly for the user who just has to stick the relevant line somewhere in their Python file.

Note also the deliberate decision to use strings for permissions rather than a dedicated enum type. The autocomplete benefits of enums are largely obviated by the fact that the error message gives the user exactly the code they need to copy. And the typo-resistance is less important given that mistyped permissions will result in immediate permissions failures anyway. Attempting to use a dedicated Permission type turned out to be architecturally awkward and made for a more cumbersome user story as well, so strings felt like the better compromise.

Closes: #2514 #2515

[cloudflare-workers-and-pages]

Deploying databuilder-docs with    Cloudflare Pages

Latest commit:	a476462
Status:	✅  Deploy successful!
Preview URL:	https://9e5fc93d.databuilder.pages.dev
Branch Preview URL:	https://evansd-table-permissions.databuilder.pages.dev

View logs

[suzannehamilton]

I've just had a look to see the shape of the change, so these comments are fairly bikesheddy rather than a thorough review!

[suzannehamilton]

Will researchers know what values to use? Would it be worth linking to the schema docs below and/or including a couple of real examples like "waiting_list" and "isaric"?

[evansd]

My thought was that the first time researchers would encounter this function would be from the warning (later error) message they get locally which will tell them exactly what names to use. And I worried that the real names might be a bit cryptic to anyone not familiar with the particular tables in question. I wanted to have something in the docstring, but I don't think it's ever going to serve as a comprehensive introduction to the topic.

[suzannehamilton]

Is it safer for the default permission to be some placeholder value rather than None, so that all tables have to override it? Or is this something we'll definitely consider when we add a new table?

[evansd]

I think the default here, in the query model, has to be None otherwise every single table we ever construct (which we do many times in tests) needs to be updated.

If we're worried about forgetting we could have a separate check that looks at every table in our official schemas and make sure they all explicitly specify a required_permission attribute, which may be None for many tables.

[suzannehamilton]

Typo: inital

[suzannehamilton]

👍

Is doing this part of #2515 or should we make a new issue to make sure we don't forget it?

[evansd]

I was going to wait for feedback from the co-pilots before creating an issue for this because I'm not quite sure what steps we need to do first before we can reasonably make this a hard error. But yeah, I think it's a separate step which warrants a separate ticket.

[bloodearnest]

This broadly makes sense to me. Interesting to see how it is plumbed in to the table definitiones and qm.

Left a couple of questions, more for my own understanding of future intent.

[bloodearnest]

lol, I had not see this decorator before! Evil/Genius hack! I love it.

[evansd]

Thanks, I was quite pleased with that. Makes defining test fixtures much more pleasant I think.

[bloodearnest]

I think that using a inner class is fine, we at least have the precedent of django to follow, folks should be somewhat familiar.

But for my own education, could you give an example of a speculative meta inner classes method that would be used for future dummy data?

[evansd]

Good question. I haven't got as far as thinking what the API might look like. But there are a couple of tables which are currently special-cased in the dummy data generator:

ehrql/ehrql/dummy_data/generator.py

Lines 203 to 223 in 29f56df

	def rows_for_patients(self, table_info):
	row = {
	"date_of_birth": self.date_of_birth,
	"date_of_death": self.date_of_death,
	}
	# Apply any FirstOfMonth constraints
	for key, value in row.items():
	if key in table_info.columns and value is not None:
	if table_info.columns[key].get_constraint(Constraint.FirstOfMonth):
	row[key] = value.replace(day=1)
	return [row]
	def rows_for_practice_registrations(self, table_info):
	# TODO: Generate more interesting registration histories; for now, we just
	# assume that every patient is permanently registered with a single practice
	# from birth
	row = {
	"start_date": self.events_start,
	"end_date": None,
	}
	return [row]

And my thought was that we can move some of this logic to the table definitions themselves, and then apply it more broadly to different kinds of table.

[bloodearnest]

AIUI, this supports a single purpose list, e.g. dataset permissions.

If we want to do T100 or old-patient-id flags, presumably we'd need to either have multiple env var for each, or have a more structured value for EHRQL_PERMISSIONS. This is obviously out of scope for this PR, but I was wondering how you had been thinking to handle that when we get to it?

[evansd]

Well, strictly speaking I think we can still handle those things with just a flat list of permissions e.g. t1oo,untagged_patient_ids,event_level_data. There's nothing specifically about datasets in the permissions, that just happens to be the only thing which is currently checked.

But I do agree that we may want more structured data here and I wondered about switching to a JSON list rather than comma separated strings. I figured it would be easy enough to do that later and I didn't want to over-complicate things. but now you've raised it, maybe we would be better off tackling this now. It would be a trivial change in the controller.

[evansd]

Prompted by Simon's question, I've switch the format of the EHRQL_PERMISSIONS variable to be JSON rather than comma-separated strings. This gives us a bit more future-proofing and generally seems more robust. This means we need to merge and deploy this RAP Controller change first:

• Use JSON to encode ehrQL permissions job-runner#1190