Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check dataset verbs in form endpoints #612

Closed
matthew-white opened this issue Mar 14, 2024 · 3 comments · Fixed by getodk/central-backend#1146
Closed

Check dataset verbs in form endpoints #612

matthew-white opened this issue Mar 14, 2024 · 3 comments · Fixed by getodk/central-backend#1146
Assignees
Labels
backend Requires a change to the API server entities Multiple Encounter workflows roles User roles

Comments

@matthew-white
Copy link
Member

Once #575 is complete, Backend will have a new dataset.create verb that is checked in the dataset creation endpoint. With that change in place, we could start checking for dataset.create in some cases when a form or form draft is created. We could also check for dataset.update in form endpoints. Specifically:

  • If a creating a form or form draft would create a dataset (even an unpublished one), and the user cannot dataset.create, then the request should result in a 403 error.
  • If creating a form or form draft would add a property to an existing dataset (even without publishing the property), and the user cannot dataset.update, then the request should result in a 403 error.
  • If publishing a form would publish a dataset or property, and the user cannot dataset.update, then the request should result in a 403.

I had been thinking that we already had an endpoint where we checked both a form verb and a dataset verb, and it looks like we do that for /v1/projects/:projectId/forms/:id/draft/dataset-diff. The comments above that endpoint seem relevant to this issue. The last sentence makes it seem like we previously decided not to check for dataset.update during form publish.

@matthew-white matthew-white added backend Requires a change to the API server entities Multiple Encounter workflows labels Mar 14, 2024
@github-project-automation github-project-automation bot moved this to 🕒 backlog in ODK Central Mar 14, 2024
@ktuite
Copy link
Member

ktuite commented Mar 14, 2024

Great idea to separate out this issue. Some of the challenges/things to think through:

Text of the 403 message
By default, it would say something like "this user is not authorized to perform this action", which is too cryptic to explain the situation of a person who can publish a form but can't create a dataset uploading a form that creates a dataset.

Dealing with dataset.createOrMerge
This comment from another PR is about how we parse the dataset information from the form definition and upsert it into the database without knowing ahead of time if it is a create or update action. We would need to take a different approach to do proper gating, e.g. check what happened and then decide to roll back the transaction or not.

Checking auth in a less common way
Related to the above issue, we usually do auth checks near where some resource access is attempted. E.g. fetching a form and then checking if a certain verb is allowed on that form. With dataset definitions that come from forms, we are deep in form parsing code. We can certainly do auth checks in there/after there, it's just a matter of doing it elegantly.

@lognaturel lognaturel added the roles User roles label Mar 19, 2024
@ktuite ktuite moved this from 🕒 backlog to ✏️ in progress in ODK Central May 17, 2024
@ktuite
Copy link
Member

ktuite commented May 20, 2024

I have some questions about edge cases:

Say Alice is an admin and Bob is someone who can do a lot of things except create (or update) Datasets.

  1. If Alice uploads a form that makes a draft dataset people, what does that mean for Bob? (Say Bob can only update.)

    • If Bob uploaded another form referencing that dataset, should it be counted as creating it or updating it, since it's not published?
    • Say Bob can't create datasets but can update them. Can he update the draft of the form because it's not really creating the dataset? What about when it comes time to publish? Can he publish it because he can update datasets? Even though at some level, we log this as creating a dataset?
  2. What if Bob can only create? He could upload the first version of a form, but then never update it?

    • If Bob is not allowed to update a dataset, that just means he's not allowed to add new properties to it? (Asking b/c code doesn't have this level of granularity at the moment.)
    • Is he not allowed to add properties in the context of a form, or overall? Say one version of a form references dataset people and properties first_name and age. Then another property hometown gets added (maybe via API). Can Bob change the form to write to the new property, too, or should he not be editing that form's interface with the dataset?

Some facts about the code right now:

  • Any form that references an existing dataset is considered "updating" the dataset, even if properties don't change, because the form def / form fields linking to the dataset has updated.

@ktuite ktuite linked a pull request May 20, 2024 that will close this issue
2 tasks
@matthew-white
Copy link
Member Author

Here are my two cents!

Say Alice is an admin and Bob is someone who can do a lot of things except create (or update) Datasets.

  1. If Alice uploads a form that makes a draft dataset people, what does that mean for Bob? (Say Bob can only update.)
    • If Bob uploaded another form referencing that dataset, should it be counted as creating it or updating it, since it's not published?

I know the code thinks of this as updating an existing draft dataset, but I think users would think of it as creating a dataset. We don't surface the concept of draft vs. published datasets to users. For example, we don't log dataset.create until a dataset is published. I think any reference to a draft dataset should be treated like creating the dataset. Related: getodk/central-backend#664

  1. What if Bob can only create? He could upload the first version of a form, but then never update it?
    • If Bob is not allowed to update a dataset, that just means he's not allowed to add new properties to it? (Asking b/c code doesn't have this level of granularity at the moment.)
    • Is he not allowed to add properties in the context of a form, or overall? Say one version of a form references dataset people and properties first_name and age. Then another property hometown gets added (maybe via API). Can Bob change the form to write to the new property, too, or should he not be editing that form's interface with the dataset?

If Bob can only create, then Bob should be able to create a dataset, but not add properties to it. The parallel for forms is that a user who can form.create but not form.update can upload the first version of a form, but then can never update the form or its fields. In addition to creating a dataset, Bob should be able to upload a form that just references the dataset without adding properties to it.

Right now, the /v1/projects/:id/datasets endpoint to create a dataset just seems to check the dataset.create verb. The /v1/projects/:projectId/datasets/:name/properties endpoint to add a property just seems to check the dataset.update verb. That makes sense to me. Bob should be able to use the first endpoint, but not the second endpoint. He should be able to upload forms that have the effect of the first endpoint (creating a dataset), but not forms that have the effect of the second (adding a property).

If another user (Alice) adds a property (either via the API or by publishing a form draft), Bob should be able to reference the new property.

@github-project-automation github-project-automation bot moved this from ✏️ in progress to ✅ done in ODK Central Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Requires a change to the API server entities Multiple Encounter workflows roles User roles
Projects
Status: ✅ done
Development

Successfully merging a pull request may close this issue.

3 participants