forms/draft/get: fix permissions check#1502
Conversation
Resolves TODO ref de-duplicating data on postgres side. Noticed while working on getodk#1502
Noted while working on getodk#1502
Resolves TODO ref de-duplicating data on postgres side. Noticed while working on #1502
Use `SELECT EXISTS (SELECT 1 ...)` in preference to `SELECT COUNT(*) ...` and then checking `result > 0` in javascript. Noted while working on #1502
|
FYI @alxndrsn, in order for this PR to make it into v2025.2, it'll need to get merged tomorrow. I'm not sure whether you'll have time tomorrow to make changes to this PR, but if you do, would it be alright if I merged the PR after approving it? |
Have we reached agreement on what a valid check for "is this form a draft" looks like? If so I'm happy for this PR to be merged. |
I think so! Checking |
lib/resources/forms.js
Outdated
| ? auth.canOrReject('form.read', form) | ||
| : auth.canOrReject(['open_form.read', 'form.read'], form)); | ||
| const canReadForm = (auth, form) => { | ||
| if (form.def.publishedAt === null) { |
There was a problem hiding this comment.
Now that canReadForm() is checking form.def, I'm wondering whether there's something subtle here related to the order of canReadForm() vs. ensureDef(). For example, if I'm a project viewer, and I request a published form version that doesn't exist, then I think form.def will exist, but form.def.publishedAt will be null. I think the logic here would cause the response to be a 403, whereas I think it should be a 404. A project viewer is allowed to read published form versions.
GET /v1/projects/1/forms/some_form/versions/some_version_that_does_not_exist
I don't think we necessarily want to call ensureDef() before canReadForm() or anything like that. I haven't thought this through, but maybe the following would account for that case?
if (form.def.id != null && form.def.publishedAt == null) {There was a problem hiding this comment.
OK, I may have been getting confused about this. I thought canReadForm() was being run before ensureDef(), but I think it's actually the other way around. So we shouldn't need to check form.def.id here, as ensureDef() will have checked that already.
The exception to that is the .../dataset-diff and .../draft/dataset-diff endpoints. Those seem to check canReadForm() before ensureDef(). Maybe they should check ensureDef() first.
There was a problem hiding this comment.
Can we consider this thread as "resolved"?
| return asChelsea.get('/v1/projects/1/forms/simple/draft') | ||
| .expect(403); |
There was a problem hiding this comment.
Could you also test that Chelsea can't access the form itself? In the case where a form only has a draft and no published version, /v1/projects/1/forms/simple and /v1/projects/1/forms/simple/draft return pretty much the same thing. In contrast, in the next test, Chelsea should be able to access the form, just not the draft.
await asChelsea.get('/v1/projects/1/forms/simple')
.expect(403);|
OK, I'm done taking a look for now! It's feeling really close to me. |
|
OK, I'm done taking a look for now! It's feeling really close to me. |
What has been done to verify that this works as intended?
New tests.
Why is this the best possible solution? Were any other approaches considered?
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?
Might affect users who were taking advantage of the lax permissions checks.
Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.
Should not.
Before submitting this PR, please make sure you have:
make testand confirmed all checks still pass OR confirm CircleCI build passes