-
Notifications
You must be signed in to change notification settings - Fork 26
feat: implement tagging rest api tag object [FC-0030] #74
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| """ | ||
| Open edX Learning ("Learning Core"). | ||
| """ | ||
| __version__ = "0.1.5" | ||
| __version__ = "0.1.6" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,8 +8,9 @@ | |
| import django.contrib.auth.models | ||
| # typing support in rules depends on https://github.com/dfunckt/django-rules/pull/177 | ||
| import rules # type: ignore[import] | ||
| from attrs import define | ||
|
|
||
| from .models import ObjectTag, Tag, Taxonomy | ||
| from .models import Tag, Taxonomy | ||
|
|
||
| UserType = Union[django.contrib.auth.models.User, django.contrib.auth.models.AnonymousUser] | ||
|
|
||
|
|
@@ -19,6 +20,16 @@ | |
| is_taxonomy_admin: Callable[[UserType], bool] = rules.is_staff | ||
|
|
||
|
|
||
| @define | ||
| class ChangeObjectTagPermissionItem: | ||
| """ | ||
| Pair of taxonomy and object_id used for permission checking. | ||
| """ | ||
|
|
||
| taxonomy: Taxonomy | ||
| object_id: str | ||
|
|
||
|
|
||
| @rules.predicate | ||
| def can_view_taxonomy(user: UserType, taxonomy: Taxonomy | None = None) -> bool: | ||
| """ | ||
|
|
@@ -53,18 +64,39 @@ def can_change_tag(user: UserType, tag: Tag | None = None) -> bool: | |
|
|
||
|
|
||
| @rules.predicate | ||
| def can_change_object_tag(user: UserType, object_tag: ObjectTag | None = None) -> bool: | ||
| def can_change_object_tag_objectid(_user: UserType, _object_id: str) -> bool: | ||
| """ | ||
| Taxonomy admins can create or modify object tags on enabled taxonomies. | ||
| Nobody can create or modify object tags without checking the permission for the tagged object. | ||
|
|
||
| This rule should be defined in other apps for proper permission checking. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have some options here:
In the actual context,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In edx-platform, we'll be adding a rule that defines how "content objects" are tagged. So in the tests, can't we add a new rule just for tests that defines how "Foo objects" are tagged? And then confirms that the rule is enforced by trying to tag some Foo objects? In other words, the tests should work in a similar way to how the platform code is going to work. I would prefer no patching and I want to see the actual functionality being tested so I don't like 2 or 3. Please review openedx/modular-learning#98 , I created this ticket for next sprint to clearly define the missed requirements here around permissions. If you think these tests will take a while, we can just return True for now and make sure the permissions are properly tested when we implement that ticket but I'd prefer to make sure that the base functionality in openedx-learning is implemented correctly now.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Awesome idea @bradenmacdonald!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bradenmacdonald I would like to add some tests to check if we can't tag anything but "abc". Just give me a minute before merge!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phew! It had a copy/paste error. Tested and fixed 85b320a I think we are good to go now @bradenmacdonald!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like a build error. No rush, just let me know when it's green and ready :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Too soon. Where mypi gets these types from?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rpenido According to the django docs for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you @bradenmacdonald! We are good now!² |
||
| """ | ||
| taxonomy = ( | ||
| object_tag.taxonomy.cast() if (object_tag and object_tag.taxonomy) else None | ||
| ) | ||
| object_tag = taxonomy.object_tag_class.cast(object_tag) if taxonomy else object_tag | ||
| return is_taxonomy_admin(user) and ( | ||
| not object_tag or not taxonomy or (taxonomy and taxonomy.enabled) | ||
| return False | ||
|
|
||
|
|
||
| @rules.predicate | ||
| def can_change_object_tag(user: UserType, perm_obj: ChangeObjectTagPermissionItem | None = None) -> bool: | ||
| """ | ||
| Checks if the user has permissions to create or modify tags on the given taxonomy and object_id. | ||
| """ | ||
|
|
||
| # The following code allows METHOD permission (PUT) in the viewset for everyone | ||
| if perm_obj is None: | ||
| return True | ||
|
|
||
| # Checks the permission for the taxonomy | ||
| taxonomy_perm = user.has_perm("oel_tagging.change_objecttag_taxonomy", perm_obj.taxonomy) | ||
| if not taxonomy_perm: | ||
| return False | ||
|
|
||
| # Checks the permission for the object_id | ||
| objectid_perm = user.has_perm( | ||
| "oel_tagging.change_objecttag_objectid", | ||
| # The obj arg expects an object, but we are passing a string | ||
| perm_obj.object_id, # type: ignore[arg-type] | ||
| ) | ||
|
|
||
| return objectid_perm | ||
|
|
||
|
|
||
| # Taxonomy | ||
| rules.add_perm("oel_tagging.add_taxonomy", can_change_taxonomy) | ||
|
|
@@ -81,5 +113,9 @@ def can_change_object_tag(user: UserType, object_tag: ObjectTag | None = None) - | |
| # ObjectTag | ||
| rules.add_perm("oel_tagging.add_objecttag", can_change_object_tag) | ||
| rules.add_perm("oel_tagging.change_objecttag", can_change_object_tag) | ||
| rules.add_perm("oel_tagging.delete_objecttag", is_taxonomy_admin) | ||
| rules.add_perm("oel_tagging.delete_objecttag", can_change_object_tag) | ||
| rules.add_perm("oel_tagging.view_objecttag", rules.always_allow) | ||
|
|
||
| # Users can tag objects using tags from any taxonomy that they have permission to view | ||
| rules.add_perm("oel_tagging.change_objecttag_taxonomy", can_view_taxonomy) | ||
| rules.add_perm("oel_tagging.change_objecttag_objectid", can_change_object_tag_objectid) | ||
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.
The response isn't really paginated is it? I don't think we need pagination.
Also, do we have a limit on how many tags can be applied to one object? If not, I'd like to add a limit in. Something like 100 so that we can avoid any need for pagination in the object tag APIs. That can come in a separate task though, no need to do in this PR. Just let me know what the current status is.
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.
It calls the retrieve method, which is paginated.
We don't have a limit on tags per object.
I think the idea here is to return the default GET format to make the refresh on the frontend easier and save 1 extra call.
Personally, I prefer to return no data in the PUT/POST requests and let the frontend handle the logic to make another GET call to get refreshed data. We can fine-tune this when we make the frontend.
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.
@rpenido We will need to have a limit on tags per object. I created openedx/modular-learning#99 but it's not ready yet as I'm waiting for the product folks to confirm what the actual limit is. When we implement that ticket we can probably remove pagination from the GET API.
I personally usually like to make the POST/PUT API return the same thing as the GET API. It saves the frontend from making an extra request.