Form count is not always filtered

[matthew-white]

Problem description

Central filters the list of forms that it returns to the user based on the user's role on the project. For example:

• A Data Collector user will not see closed or draft forms.
• A Project Viewer will not see closing forms.

That's true both on the homepage and on the forms page.

However, this filter is not applied to the forms count returned with the extended metadata of the individual project response (on the forms page). The forms count always includes all forms.

That differs from the forms count returned with the project list on the homepage (/v1/projects?forms=true). That count does seem to be filtered. That said, I suspect that if ?forms=true were not specified, then the count would not be filtered (though I haven't confirmed this).

To summarize:

• /v1/projects/:id/forms: filters the list
• /v1/projects/:id (extended metadata): doesn't filter forms
• /v1/projects?forms=true: filters formList and forms
• /v1/projects (extended metadata but not ?forms=true): doesn't filter forms (I think)

I don't think we surface the forms count in a lot of places in Frontend. If it differs from the form list response on a project page, Frontend will automatically update it to match the form list in order to reduce inconsistency. There is one case I can think of where a Project Viewer would briefly see the non-filtered count. If a Project Viewer navigates to the forms page, then between when they receive the project response and the form list response, they will see the non-filtered count.

URL of the page

https://staging.getodk.cloud/#/projects/90

Steps to reproduce the problem

• Create a new project.
• Create a form, publish it, then change its form state to closing.
• Create a form, but don't publish it or change its form state.
• Add a non-admin user to the project as a Project Viewer. Log in as that user.
• Navigate to the forms page. Only one form is shown. However, if you open the Network tab of Chrome devtools, you will see that 2 was returned for the forms count.
• Log back in as the first user.
• Change the non-admin user to a Data Collector. Log back in as that user.
• Navigate to the forms page. Again, only one form is shown, but the Network tab shows 2 for the forms count.

Expected behavior

The forms count should always be filtered.

Central version shown in version.txt

versions:
e49518adb84f88d7bc6c3626fc77584dfc935435 (v2024.1.0-6-ge49518a)
+2a291e718f2d342148f3f958691875bacadc7596 client (v2024.1.0-22-g2a291e71)
+beea5c81bf225b5e9da613c38269f07466cae613 server (v2024.1.0-21-gbeea5c81)

[brontolosone]

Taking this requirement as illustrative:

• A Project Viewer will not see closing forms.

Going by the book on this (well, reading the DB schema), it looks like the underlying thought is that we should be using each role's permissions to inform the predicates to apply to the form count (to weed out forms in closing state from the count). That's very doable.

But that presupposes that any actual permissions exist to make the distinction between, in this case, a Project Viewer of a project (who's not supposed to see/count that project's the "closing" state forms) and a Project Manager of that same project, who's of course supposed to see (and count) "closing" forms.

So let's look at their permission sets:

odkcentral> select system as projectrole, jsonb_pretty(verbs) as perms from roles where system in ('viewer', 'manager')
+-------------+---------------------------+
| projectrole | perms                     |
|-------------+---------------------------|
| viewer      | [                         |
|             |     "project.read",       |
|             |     "form.list",          |
|             |     "form.read",          |
|             |     "submission.read",    |
|             |     "submission.list",    |
|             |     "dataset.list",       |
|             |     "entity.list",        |
|             |     "dataset.read",       |
|             |     "entity.read"         |
|             | ]                         |
| manager     | [                         |
|             |     "project.read",       |
|             |     "project.update",     |
|             |     "project.delete",     |
|             |     "form.create",        |
|             |     "form.delete",        |
|             |     "form.list",          |
|             |     "form.read",          |
|             |     "form.update",        |
|             |     "submission.create",  |
|             |     "submission.read",    |
|             |     "submission.list",    |
|             |     "field_key.create",   |
|             |     "field_key.delete",   |
|             |     "field_key.list",     |
|             |     "assignment.list",    |
|             |     "assignment.create",  |
|             |     "assignment.delete",  |
|             |     "public_link.create", |
|             |     "public_link.list",   |
|             |     "public_link.read",   |
|             |     "public_link.update", |
|             |     "public_link.delete", |
|             |     "session.end",        |
|             |     "form.restore",       |
|             |     "dataset.list",       |
|             |     "entity.list",        |
|             |     "dataset.read",       |
|             |     "entity.read",        |
|             |     "entity.create",      |
|             |     "entity.update",      |
|             |     "dataset.update",     |
|             |     "entity.delete",      |
|             |     "submission.update",  |
|             |     "dataset.create",     |
|             |     "submission.delete",  |
|             |     "submission.restore"  |
|             | ]                         |
+-------------+---------------------------+

I don't see any permission here that a Manager has and a Viewer hasn't that could be interpreted as "can list closing forms".
So I'm wondering if the abovequoted requirement can be cleanly expressed with the existing permissions at all.

But we could bend (= abuse) the semantics of one of the existing permissions, such as form.update. We could say "yeah form.update means you get to draft and close forms etc, so if you have that, then you should be able to list those drafted/closing/closed forms as well". Basically, overloading form.update, but that's a slippery slope as that means semantics change over time, are not well defined, and not fine-grained anymore, and that's poor form. Also, reductio ad absurdum: why overload the permissions, why not jump straight to overloading the roles themselves then and do away with the permissions 😜

If we add permissions, perhaps form.list_draft, form.list_closing, form.list_closed then we can actually express the distinction declaratively, in the roles in the database, which seems to me like the intention of the design.
The downside is of course that then we'll have to backport these permissions into the queries where they matter (where permission predicates have currently been implemented by other means (= by any means necessary)).
The upside of that downside is that I might see a way to do that neatly, cleaning up a whole bunch of ad-hoc permission-checking approaches we see in the queries in lib/model/query/* in the process, and making things easier to debug along the way. But that would take a bit of time.

[matthew-white]

One thing to keep in mind is that we already do this filtering for the form list (e.g., from the /v1/projects/:id/forms endpoint). The goal of this issue is to apply that same filtering to the form count. I think this is where we do the filtering for the form list. I see the verb open_form.read, but also form.update, which maybe speaks to what you wrote above.

One option is that we port that filtering logic from the Forms query to the part of the Projects query module that counts forms (here). Alternatively, maybe we do what we do when formList is returned from /v1/projects?forms=true, which is to query the form list and use the result to also generate the form count (here). Part of what's weird about all this is that the form count is sometimes filtered and sometimes not.

But that would take a bit of time.

I wouldn't worry too much about this part right now! If the end result is that you've learned more about extended metadata and roles and this unusual case of filtering, that'll be a useful thing. But also, if all this seems like too much detail, and the requirements aren't clear, feel free to focus on another issue.

[brontolosone]

Ah yes you're right, there is prior art of course! Unevenly applied, that's the problem to be addressed for this issue indeed.

Now that you've shown me a usage site of a permission (or "verb") called open_form.read, I have another question. On the face of it it looks useful, especially if "open form" is defined as "a form not closed, closing, or draft".
But. None of the roles of my Project Viewer user has a verb open_form.read. In fact none of the users in my database has that verb:

odkcentral> SELECT count(*) FROM users u INNER JOIN assignments assign ON (u."actorId" = assign."actorId") INNER JOIN roles ON (assign."roleId" = roles.id AND roles.verbs ? 'open_form.read') 
+-------+
| count |
|-------|
| 0     |
+-------+

I suppose the thinking is that it is not necessary - my users don't need open_form.read because that's already implied by them having form.read perhaps? So there's some implicit entailment, one permission encompasses others?

[ktuite]

I suppose the thinking is that it is not necessary - my users don't need open_form.read because that's already implied by them having form.read perhaps? So there's some implicit entailment, one permission encompasses others?

This hopefully has enough clues about how we ended up with open_form.read getodk/central-backend#968