-
Notifications
You must be signed in to change notification settings - Fork 13
Add bulk permission management and copy functionality via API #77
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
base: develop
Are you sure you want to change the base?
Conversation
|
Hey Florian, thank you for the pull request! I've made some small improvements to help our static analyzers pass and to fix one of our tests. I've also moved the function for mapping object permissions to a dict that can be used for JSON to the actual object permissions api module, rather than the utils module for the various APIs. Currently, object permissions are already part of the API, so it'd be best to stay consistent with the existing endpoints and naming. I know For the permissions copying, having an Also, the documentation could still use some improvement, e.g. the PUT on The API endpoints could use some more defensive coding, e.g. you assume that request arguments are the right type, rather than making sure, so rather than giving users a Bad Request error and information on what's wrong, the code will result in an Internal Server Error instead. Finally, if you're okay with that, please add yourself to the list of authors in pyproject.toml :) Let me know if any of that isn't clear enough or you need help! |
|
Hey Florian, thank you for the tips. I've made the adjustments you've mentioned. The naming for the API should now be consistent and for the copy endpoint, I've added the suffix I tried to improve the type checking of the requests arguments. Let me know if there are still situations where the code will result in Internal Server Errors. If there's anything missing or still needing improvements, I am open to have a look at it. |
|
I missed it before, but And maybe include these new endpoints in |
| }, 400 | ||
| # Validating all incoming object ids before changing any permission | ||
| for object_id in [source_object_id, target_object_id]: | ||
| object_id = int(object_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy complains about this line: sampledb/api/server/object_permissions.py:207: error: Incompatible types in assignment (expression has type "int", variable has type "Literal[False]") [assignment] because both source_object_id and target_object_id contains the result of the comparison with None, so both are False when everything goes well, which will be converted to 0 with int, which obviously wasn't your intention. assign source_object_id to item.get("source_object_id") first and then do the comparison, the same for target_object_id and you'll avoid that issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best check that type(source_object_id) is int (and same for target_object_id), instead of attempting a conversion for a copy of them, as this way you'll end up passing strings to copy_permissions. Also fix the error message for target_object_id using source_object_id in its text
| for item in request_json: | ||
| source_object = source_object_id | ||
| target_object = target_object_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-think this loop ;)
| request_json = [ | ||
| { | ||
| "source_object_id": object_id, | ||
| "target_object_id": other_object_id, | ||
| }, | ||
| { | ||
| "source_object_id": other_object_id, | ||
| "target_object_id": object_id, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, multiple copy requests at once is broken. This test doesn't find it, because it copies the permissions back and forth, so there's no way to validate that it worked.
In our hosted instance, we are creating objects with a bot account for specific users because we can't upload them directly from the user's account. Nevertheless we'd like to use the same permissions from the sample on the process we just performed on it. While there's currently just the option to read and set each permission type (users, group, projects, ...) individually, we'd like to reduce API-requests by having an endpoint to get all permissions that are given to an objects at once.
This is why I've added the endpoint /objects/<object_id>/permissions/.
On GET the endpoint will return a dictionary of all "permission-types":
Using PUT on the same endpoint with a dictionary of at least one key from above will perform the given permissions to the object. (This does not remove all existing permissions of the given object and set the new ones. It will only change or add the given permissions)
We also like to add the endpoint /objects/permissions/copy/. This endpoint uses the already implemented function "object_permissions.copy_permissions()" and is able to receive multiple "source"-"target" object pairs, where the target's permissions get overwritten by the source object's permissions.
Performing a POST-request with json-data like:
... will for example overwrite the object-permissions from object {2} with those from object {1}.
You can either perform the request with a list of json-objects or just with one json-object like: