Use postgres UUID types for actee ids

[alxndrsn]

Ref getodk/central#1217

What has been done to verify that this works as intended?

Tests etc.

Why is this the best possible solution? Were any other approaches considered?

There are other suggestions for extending/changing this including:

• defining the values on the db-side: https://github.com/getodk/central-backend/pull/1589/files#r2462973088
• define uuidFor() in postgres (TODO can't find the link for this)
• get rid of all the special actees except * (Use postgres UUID types for actee ids #1589 (comment))

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

• maybe a slow migration
• maybe cases where non-UUIDs have been used; hopefully caught in QA

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

I don't think so.

Before submitting this PR, please make sure you have:

• run make test and confirmed all checks still pass OR confirm CircleCI build passes
• verified that any code from external sources are properly credited in comments or that everything is internally sourced

[brontolosone]

What would be nice is to have DB functions species_from_uuid(input uuid) -> text and uuid_from_species(input text) -> uuid to use for DB queries and interactive DB work.

[brontolosone]

Again I've run into a situation where I have to refer to actees and find it wasteful to have to 2x-bloat a table and 2x-bloat an index because of the stringery...
I think this PR has value!

[matthew-white]

I think this PR has value!

I agree that it's reasonable to use the uuid type for actee IDs, I support that. 👍 It's hard for me to know how much improved performance to expect. But we know there should be some, and the changes in this PR aren't too big. There's a migration and a change to queries that reference actees that are species or *.

I also wanted to link this PR to the discussion at getodk/central#1217. In particular, I still think that we could take this opportunity to do away with species-level actees entirely. Instead of reifying them, we could get rid of them. We assign actors access to specific actees (like a specific project), and we assign actors access to *, but I don't know of any cases where we assign actors access to species-level actees. Actors end up with access to species-level actees only insomuch as they are given access to *.

In other words, I think we could make do with just one special UUID (for *) instead of one for each species.

Concretely, I'm proposing that we change lines like

central-backend/lib/resources/users.js

Line 37 in 7739129

auth.canOrReject('user.create', User.species)

to:

auth.canOrReject('user.create', '*')

I really don't think this would be a huge code change. I'd be happy to create a draft PR along those lines if there's interest.

[matthew-white]

And just to be clear, I'm not suggesting that we get rid of the concept of species entirely. For example, the actees.species column can stay put. I just think that we don't need each species to correspond to a row of the actees table.

[matthew-white]

Now that @brontolosone has added this PR to the project board, I'll go ahead and assign @alxndrsn just so it's clear from a glance who's working on it.

[alxndrsn]

I don't know of any cases where we assign actors access to species-level actees.

Are there any immediate plans to use more fine-grained assignment?

It seems reasonable that there might be different permissions for e.g. creating single-use links vs system settings.

Now the system's in place, maintaining it doesn't seem like a big cost, whereas re-adding it later might be a lot of work.

If it's costing effort to maintain, and isn't going to get used, let's get rid.

[brontolosone]

It's hard for me to know how much improved performance to expect.

Yes. It'd be great to be able to measure it, but the real-world benefits won't let themselves be approximated well through an easy pgbench run I think.

It's more a case of compounding positive effects, as it does cut the size of foreign keys into this table, and indices thereon, in half. Following from that there will be setups and circumstances in which this means that some pages of some table/index will now not be pushed out of memory caches when pages of these tables/indices need to be read, and vice versa. Conversely it's reasonable to expect that there's a performance cliff we fall off once hot path tables/indices don't all fit into memory anymore, especially on DB setups like bottom tier Amazon RDS which has the I/O bandwidth of a 1994 PC. It would be nice to see that dropoff point move in a benchmark, and if we get a benchmark setup going we might backtest this change, but I think we can follow an instinct of "compact dedicated fixed-width datatypes† are probably better" :-)

†) IIRC varchar(36) is not fixed-width, it's variable-width, just with a limit

[brontolosone]

I've read @matthew-white 's point above, and the earlier considerations. There is redundancy (along multiple axes! eg, various belongs-to relationships are also expressed in the orthodox FK ways, for example, forms rows have an FK to their projects AND they have that same parentage expressed through the actees table †) in this system, that's also what I concluded when I was exploring a backwards-compatible alternative.

And just to be clear, I'm not suggesting that we get rid of the concept of species entirely. For example, the actees.species column can stay put. I just think that we don't need each species to correspond to a row of the actees table.

Can't we drop that column? :machete: :rambo:
@matthew-white you're proposing keeping around just * (or its UUID representation), would that purely be to express "admins can do anything"? Because while I understand the philosophical-ontological attraction of separating "is-an-admin?" from "can-an-admin?", the reality is that of course, an admin can perform any exposed action, and the "admin-ness" of a user is already expressed through assignment of the admin role to a user.
IOW,

auth.canOrReject('user.create', User.species) 

could conceivably just shortcut for admins, the answer is "Yes They Can". No need to plunge into the actees recursion pool. Or am I missing some detail?

†) And nothing guarantees consistency between those two. "If you have one watch you know what time it is, if you have two watches, you don't." (A saying from the era of analog spring-powered watches; it's not going to work for a generation growing up with GPS-timesynced smartwatches).

[matthew-white]

Are there any immediate plans to use more fine-grained assignment?

It seems reasonable that there might be different permissions for e.g. creating single-use links vs system settings.

Different permissions are driven by (1) verbs on the role and (2) the actee.

• Creating single-use links: actor must have the public_link.create verb on an assignment to the form actee, the project actee, the form species ("all forms"), the project species ("all projects"), or *.
• System settings: actor must have the config.set verb on an assignment to the config species or *.

In both of these cases, I don't think the species actees are needed. We could remove the parts above about the form species, the project species, and the config species without changing any user-facing behavior. That's because there's no way to use the API to create an assignment to a species actee. The ways to create an assignment are for a form, for a project, or for everything (*), where the assignment is limited to a specific set of verbs.

We've definitely discussed creating more roles: getodk/central#736, getodk/central#737. However, I see those issues as being about changing the roles/verbs that can be used for assignment, not changing which actees are assignable. I don't expect species actees to be relevant to that work.

Similarly, for API keys (getodk/central#1335), I expect API keys to provide access to specific projects or to a select list of verbs on *, but I don't expect API keys to need to do anything with species actees.

Now the system's in place, maintaining it doesn't seem like a big cost, whereas re-adding it later might be a lot of work.

If it's costing effort to maintain, and isn't going to get used, let's get rid.

I think the main reason to remove it is code clarity and making it easier for developers on the project to understand the auth system. We rarely touch this area of the code, so I don't see it as being much cost to maintain. It just needs like a good opportunity to simplify things if we're going to be refactoring this code anyway.

I also don't think there's much risk of removing it and needing to add it back later. I can't think of a time when we've ever needed these species actees, and I don't know of any upcoming work that will require them.

I don't want to block progress on this PR though. The main goal is to make acteeIds of type UUID. So if this conversation feels like a tangent/separate, I'm happy to come back to it later. There's no urgency in removing species actees now. I could file a separate issue about that, or we could just remember the idea for later.

[matthew-white]

you're proposing keeping around just * (or its UUID representation), would that purely be to express "admins can do anything"? Because while I understand the philosophical-ontological attraction of separating "is-an-admin?" from "can-an-admin?", the reality is that of course, an admin can perform any exposed action, and the "admin-ness" of a user is already expressed through assignment of the admin role to a user.

I think it's possible that in the future, there could be additional top-level roles or assignments. For example, an API key that has the submission.read verb on *, giving it read access to submissions from all projects.

I think you're probably right though that there's a way we could remove * and represent that on the roles table instead. Like a column that indicates whether a role is a top-level role or a project-level role. Right now, Frontend just hard-codes which roles are top-level (admin) and which are project-level (manager, viewer, formfill).

Under the current setup, users can technically do things via the assignments API that aren't possible in Frontend. For example, creating a top-level project viewer who can view all projects. I don't know of any such use cases (they would be unexpected), just saying that our API allows such things right now.

I'd be open to getting rid of the * actee. I think I'm more excited about removing species actees, because we just never create assignments to those actees. In contrast, we do create assignments to * and allow users to do so. If we removed species actees, this PR wouldn't have to introduce new code to maintain them.

And just to be clear, I'm not suggesting that we get rid of the concept of species entirely. For example, the actees.species column can stay put. I just think that we don't need each species to correspond to a row of the actees table.

Can't we drop that column? :machete: :rambo:

Haha machete most welcome. Maybe we can drop that column, that would be exciting. I'm not sure I see any uses of it from a quick search of the codebase. What do you think about doing that in a separate PR? I could review it. For this PR, I think we need to figure out the near-term destiny of species actees (rows), but I think we could remove the species column separately.

[alxndrsn]

Similarly, for API keys (getodk/central#1335), I expect API keys to provide access to specific projects or to a select list of verbs on *, but I don't expect API keys to need to do anything with species actees.

Would this be an example where an API key would want species-level actee permissions: a system to sync odk-central users with a 3rd-party IAM system? E.g. if an organisation wants to manage their user accounts for ODK and other systems centrally.

[alxndrsn]

Opening for review, as this PR seems to be generally positively received, but also have a number of suggestions for different directions it could be taken in next.

[brontolosone]

By and large 👍, and also 🎊 , but I think there are a few non-radical improvements to make, please see inline comments!

[brontolosone]

I don't feel we really need this little inline test-in-a-migration.

[brontolosone]

is this for aesthetics?

[brontolosone]

Would be nice. Ktuite likes to use down-migrations.

[brontolosone]

This (from 83ebf49) is bycatch, right? Bycatch as in an improvement (or bugfix) that you stumble upon while fullfilling the primary mission? Personally I'm not against bycatch especially if it's denoted as such in a commitmessage (the one of 83ebf49 suffices, but it could be made more distinct with a bycatch: prefix convention) (and then the commit should IMO ideally not be squash-merged).
I have the impression that a bycatch-positive disposition is not present across the whole team and perhaps another reviewer would ask you to put it in a separate micro-PR but hey, I'm the reviewer here so let's smuggle that contraband commit in 😆

[brontolosone]

That'd be nice. I know ktuite likes to use down-migrations.

[brontolosone]

I would not store these constants as sql-templates. That's a boxing (of sorts) for a particular use case for a particular DB driver. So I'd leave the base definition assumption-free which in this case would mean storing the UUIDs as string values as there is no UUID type in the stdlib.

And then of course you can construct this sqlSpecial special purpose mapping automatically from that base assumption-free definition. Alternatively the ensql-ing it could take place inside uuidFor() though of course from a performance-aesthetics POV it's nicer to not repeat that work, which speaks for creating a mapping once on initialization. And then that sql-specific stuff should then probably go into query/actees.js, and read the table of constants from a new lib/constants/actees.js which contains just the mapping constants and no presumptions about the usage. (actees.js is imo preferable to actz.js).

[brontolosone]

review? is this addressed to me? 🤔

[brontolosone]

is this purely for aesthetics? We don't look at the migration output that much, especially not on an ongoing basis going forward, so... leave it out, less is more? If I want to look at formatted SQL code I'll just pipe it through my SQL formatter on an ad-hoc basis.

[brontolosone]

I think the typename is actually in lowercase, it just so happens that things get converted to lowercase unless quoted, so this happens to work, but I don't like things that happen to work if there's a more to-the-point way. Witness:

odkcentral> select 'd7bb7054-1f61-4cd3-9a90-ac4f18b8a060'::"uuid"
+--------------------------------------+
| uuid                                 |
|--------------------------------------|
| d7bb7054-1f61-4cd3-9a90-ac4f18b8a060 |
+--------------------------------------+

Versus:

odkcentral> select 'd7bb7054-1f61-4cd3-9a90-ac4f18b8a060'::"UUID"
type "UUID" does not exist              

So the type is called uuid, not UUID.