Enumerator should be able to access only open Form#968
Enumerator should be able to access only open Form#968sadiqkhoja merged 3 commits intogetodk:masterfrom
Conversation
d002b19 to
7d6f380
Compare
| // `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'); |
There was a problem hiding this comment.
Rather than inferring open_form.list and open_form.read from form.list and form.read, could we add open_form.list and open_form.read to the list of verbs for admins and project managers? I think that would simplify some of the SQL below as well.
ktuite
left a comment
There was a problem hiding this comment.
Talked to Sadiq interactively just now and I'm convinced that this is a sensible tradeoff of code complexity vs. cleanly reasoning about the verbs.
With this approach, the meaning of form.list and form.read doesn't change for viewers/managers/admins, it continues to mean "access to all forms regardless of their state".
The lower access roles have their form verbs removed and replaced with open_form verbs, which also makes sense -- we're limiting the access for these roles.
If we were instead to add open_form.list/read to the higher access roles and try to use it, we would need to forever maintain this double verb (form + open_form) and form on its own might take on a kind of unintentional closed_form meaning. Anyone who happened to create custom roles (through the DB) would also need to know about this double verb and need to remember to add the lower level open_form verb to make things work.
(One example where we already have to check a double verb is in the _getSql query of Projects where users must have project.read and form.list. This check makes sense to me, though, and isn't nearly as complicated as the Forms queries.)
Caveats I remember from code review:
canAssignRole()has to change because higher access roles like admins/managers are expected to have all the verbs they assign to lower access roles, but admins don't haveopen_formverb so the function fakes it if that role has the supersetformverb.
|
Very helpful explanation, that sounds reasonable to me. I can see how there are tradeoffs here. It's a tricky problem! |
|
Just to make sure I'm understanding, Data Collectors will still be able to access closing forms over the API, right? So they'll see a closing form in the forms table, but then Frontend will restrict them from actually navigating to the form page? If that's the case, I'm thinking of filing a separate Frontend issue about that. Frontend could hide closing forms from Data Collectors in the forms table. |
I am not sure what's the ideal here. We allow "App Users" and "Public links" to submit against "Closing Forms", by that logic "Data Collector" should also be able to submit. But if they can't see the form in the form list then how are they gonna submit, by saving Enketo URL somewhere offline? |
|
Hmm I see what you mean. I don't remember whether we've discussed how Data Collectors should interact with closing forms. Maybe it's not likely that a project with Data Collectors would use the closing state. For what it's worth, here's what's currently shown in Frontend to a Data Collector: 
So I think a Data Collector won't be able to use Frontend to submit to a closing form. If a Data Collector should be able to, we could change the behavior in Frontend, though maybe this situation just doesn't come up that much.
For some reason, I was thinking that Frontend would link to the form page, but that's not the case. So I don't think I'd file an issue after all. |
so that enumerators can access only open Forms
519fa19 to
02d3bcb
Compare
…t forms via API Closes getodk#968
* Fixes sec#2: add verbs so that enumerators can access only open Forms * let Data collector access closing forms * polish some of the tests
Closes sec#2
Impact:
Following endpoints were accessible for "App User", "Data Collector" and "Public link" when Form was closed, after this change they return 403:
What has been done to verify that this works as intended?
Integration tests + Manual testing
Why is this the best possible solution? Were any other approaches considered?
Discussed the solution over Slack
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?
None
Does this change require updates to the API documentation? If so, please update docs/api.md as part of this PR.
No
Before submitting this PR, please make sure you have:
make test-fulland confirmed all checks still pass OR confirm CircleCI build passes