Tagging: serialize object permissions to REST API [FC-0036] #138
Conversation
to get test coverage back to 100% for this file.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @pomegranited! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
|
A few thoughts:
|
These permissions reflect the current request.user's allowed actions on the instances in the results list.
frontend will use userPermissions instead
…APIs Uses a custom paginator to add this extra data into the response.
pomegranited
force-pushed
the
jill/serialize-permissions
branch
3 times, most recently
from
dd0d36a to
9df5304
Compare
* model-level permissions: only need can_add * instance-level permissions: only need can_change and can_delete
Also flattens the serialized user permissions so the flags appear directly on the instance instead of in a "user_permissions" dict.
to tell REST API users whether the user may add object tags for this object_id + taxonomy.
* `can_<action>` field will be None unless ?include_perms requested * adds tests to verify query counts are unchanged when perms requested
This permission is needed by the Content Tag Drawer, which fetches the list of taxonomies available for tagging a given content item, and shows an "Add tags" button for the ones the user may add Object Tags for.
fixes type/testing issues with previous commit
to alleviate confusion about what permissions they reflect.
for a given taxonomy + object_id, because we are adding and deleting ObjectTags here, not editing the individual instances.
pomegranited
force-pushed
the
jill/serialize-permissions
branch
4 times, most recently
from
b9797ab to
bb1a0a2
Compare
| if isinstance(instance, Tag): | ||
| return super()._can('change', instance) | ||
| return None |
There was a problem hiding this comment.
from comment:
The permissions for all tags in a taxonomy are always the same, right? ... So I'm tempted to say don't include permissions for them - just include perms at the taxonomy level or course component level. But I realize that's making an assumption that this won't change in the future. If we include the perms now with every Taxonomy/Tag/ObjectTag as you're proposing, and make the frontend work accordingly, then we can be super flexible about how permissions work in the future if we want to, and won't need to make any changes to the frontend. So that makes sense.
Here is where that plan breaks down, because we don't currently have a way to check use permissions for TagData instances (rather than Tag instances). I could create a dummy Tag here from the TagData instance and use that to check the permissions, but that will likely trigger another database query (for the Tag.taxonomy field).
But we could use the UserPermissionsPagination class to put permissions for the taxonomy itself into the top level like we did for can_add?
There was a problem hiding this comment.
@pomegranited For now, the permissions of every tag in the taxonomy are the same. But we want to design the REST API to be flexible in the future, as I mentioned. So I would suggest this:
When you initialize a TagDataSerializer, pass in the can_change and can_delete permissions as context, just the same way that we currently pass in taxonomy_id and request as context. Then just change get_can_change and get_can_delete to return the value from the context, or omit those fields if the data is not available in the context. This will be simple, complete, and works with both TagData and Tag. It also doesn't use any per-instance queries and maintains a flexible API so that if we need perms that vary per-tag in the future, we can change this to do that somehow, without needing to change the REST API or the frontend.
You could also use the self.context["taxonomy_id"] to look up the taxonomy and thus the permissions for each TagData instance, but I think that will result in too many queries unless you add some caching. And then you have to deal with caching.
There was a problem hiding this comment.
I'm failing to find a hypothetical case where we need different Tag and Taxonomy permissions. Do you see any @pomegranited and @bradenmacdonald?
If we can't foresee a use for different permissions, I think we could move closer to removing the Tag permissions and keeping it simple instead of making it more flexible. Of course, only if we can't see a use for it in the future.
So, now, I'm more inclined to use the Taxonomy canTagObject permission for Tagging and the Taxonomy canChange for Tag editing (that we don't have frontend yet), skipping serializing permission at TagDataSerializer.
There was a problem hiding this comment.
So, now, I'm more inclined to use the Taxonomy canTagObject permission for Tagging and the Taxonomy canChange for Tag editing (that we don't have frontend yet), skipping serializing permission at TagDataSerializer.
I ended up going with @bradenmacdonald 's suggestion for the TagData:
When you initialize a TagDataSerializer, pass in the can_change and can_delete permissions as context, just the same way that we currently pass in taxonomy_id and request as context.
...so we have the permissions data in the REST API whenever we start editing taxonomy tags in the UI.
But I'm equally ok with removing this work if we feel like it's more unused code we shouldn't be maintaining yet.
There was a problem hiding this comment.
Hi, @pomegranited!
While reviewing, it took me some time to get the permission context again, and I felt we were overengineering a bit with some "to be used in the future" features.
If you (who wrote this change) didn't feel this way, we probably don't need to simplify it. If this is the case, I'm ok with the current solutions!
There was a problem hiding this comment.
Thank you for your thoughts here @rpenido ! I'll leave this comment thread unresolved so upstream/CCs can see it too, and will see what they say.
| perm_name = f'{app_label}.add_objecttag' | ||
| perm_object = ObjectTagPermissionItem(taxonomy=instance, object_id="") | ||
| return request.user.has_perm(perm_name, perm_object) # type: ignore[arg-type] | ||
|
|
There was a problem hiding this comment.
We have change_objecttag_taxonomy rule that checks only the Taxonomy leg of the permission.
| perm_name = f'{app_label}.add_objecttag' | |
| perm_object = ObjectTagPermissionItem(taxonomy=instance, object_id="") | |
| return request.user.has_perm(perm_name, perm_object) # type: ignore[arg-type] | |
| perm_name = f'{app_label}.change_objecttag_taxonomy' | |
| return request.user.has_perm(perm_name, instance) # type: ignore[arg-type] | |
There was a problem hiding this comment.
Since people may override these rules when they use the library, I'd rather call the canonical permission, and let the rule itself call change_objecttag_taxonomy.
There was a problem hiding this comment.
Sure! Either way is acceptable to me.
| if isinstance(instance, Tag): | ||
| return super()._can('change', instance) | ||
| return None |
There was a problem hiding this comment.
I'm failing to find a hypothetical case where we need different Tag and Taxonomy permissions. Do you see any @pomegranited and @bradenmacdonald?
If we can't foresee a use for different permissions, I think we could move closer to removing the Tag permissions and keeping it simple instead of making it more flexible. Of course, only if we can't see a use for it in the future.
So, now, I'm more inclined to use the Taxonomy canTagObject permission for Tagging and the Taxonomy canChange for Tag editing (that we don't have frontend yet), skipping serializing permission at TagDataSerializer.
There was a problem hiding this comment.
@pomegranited LGTM 👍
- I tested this using the instructions at Use object permissions in Tagging frontend [FC-0036] frontend-app-authoring#787
- I read through the code and considered the security, stability and performance implications of the changes.
- Includes tests for bugfixes and/or features added.
- Includes documentation
pomegranited
force-pushed
the
jill/serialize-permissions
branch
from
8857a9e to
3e24b66
Compare
which means a user may add or delete ObjectTags using a given Taxonomy + object_id. This permission is used in the bulk tag update REST API endpoint, and when serializing Taxonomies in the REST API.
|
@bradenmacdonald could I get your review here? |
| # Enabled taxonomy can be modified by taxonomy admins | ||
| "can_change_taxonomy": is_admin, | ||
| "can_delete_taxonomy": is_admin, | ||
| "can_tag_object": False, |
There was a problem hiding this comment.
Why is this False? Shouldn't an admin (at least) be able to use this taxonomy to tag objects?
There was a problem hiding this comment.
You'd think so, wouldn't you? But the base oel_tagging.can_change_object_tag_objectid always returns False, and so nobody has permission to add base ObjectTags.
I'm more than happy to change this, though, I think it's over cautious.
There was a problem hiding this comment.
So it's False because the admin could add a ContentObjectTag but not an ObjectTag? Is that what you mean?
If so, is there a similar API that includes can_tag_content_object or something? This field just doesn't seem useful at the moment.
There was a problem hiding this comment.
More context here: #74 (comment)
There was a problem hiding this comment.
So it's False because the admin could add a ContentObjectTag but not an ObjectTag? Is that what you mean?
Yes.
If so, is there a similar API that includes
can_tag_content_objector something? This field just doesn't seem useful at the moment.
openedx/edx-platform#34004 shows where can_tag_object is True in the content_tagging context, does that help?
There was a problem hiding this comment.
Yes, that helps :)
Could you please just add a comment here along those lines? Something like "This is False because we default to not allowing users to tag arbitrary objects. But specific uses of this code like content_tagging in edx-platform will override this perm to be true for their use cases"
…views We've demonstrated that adding permissions to the REST API does not add more database queries, so we can always return permissions.
* removes can_change_objecttag * replaces Optional[bool] with bool | None * adds comment about can_tag_object in tests
|
@rpenido @pomegranited Are you both happy with this PR and ready for final review + merge from me? |
|
Thanks for your reviews @bradenmacdonald ! I'm going to merge & tag this and get openedx/edx-platform#34004 ready for merge too. |
|
@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Uses the permissions added to the Tagging REST API by openedx/openedx-learning#138 to decide what actions (e.g. import, export, edit, delete) to present to the current user when viewing Tagging-related content.
Description
Adds "user permissions" to the Tagging REST API to show what permissions the current request.user has on the objects serialized in the response.
Supporting information
Part of openedx/modular-learning#160
Testing instructions
See openedx/frontend-app-authoring#787 for manual testing instructions.
Deadline
None
Other information
TO DO (Re comment:)
Please include a test that loads a bunch of taxonomies/tags/objecttags as a non-superuser and makes sure the query count is only increased by at most 1 when permissions are included in the response.
Done, see test_views.py.
Do we want to allow API consumers to choose whether the perms info is included in the REST API responses or not?
It's probably simpler to just always include them, unless there is some performance or response size penalty.
I decided to add an
include_permsparameter which triggers the permission checks. These checks are cheap to run in this library, but users of the library may customize their permissions (like we did with content_tagging) and so the checks may not always be this quick to run.bump version
Private ref: FAL-3577