feat: implement tagging rest api tag object [FC-0030]

[rpenido]

Description

This PR implements the tagging REST API for updating object tags.

Supporting Information

• Closes https://github.com/openedx/modular-learning/issues/82

Testing instructions

• Ensure that the tests cover the expected behavior of the view as described in the related issue.

  ---

  Private-ref: FAL-3474

[openedx-webhooks]

Thanks for the pull request, @rpenido! 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:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

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.

[bradenmacdonald]

Let me know when you want a full review of this.

[rpenido]

I think it's ready for a review @bradenmacdonald!

[bradenmacdonald]

@rpenido I went to review but I'm getting hung up on the permissions issue. We can't allow users to tag any objects they want; they can only tag objects that they have permission to edit. So I think this API needs to be an abstract class that is not actually accessible, and then in edx-platform we can inherit it and implement a "get/set content object tags" API that uses this same code but adds appropriate permissions checks like "is this user allowed to edit this XBlock". What do you think?

[rpenido]

@rpenido I went to review but I'm getting hung up on the permissions issue. We can't allow users to tag any objects they want; they can only tag objects that they have permission to edit. So I think this API needs to be an abstract class that is not actually accessible, and then in edx-platform we can inherit it and implement a "get/set content object tags" API that uses this same code but adds appropriate permissions checks like "is this user allowed to edit this XBlock". What do you think?

I agree with you. But with YAGNI in mind, I think that the MVP permission model is broad enough to allow this (if a user can create a course, it can tag objects). But I wouldn't want to make assumptions without checking. Can you help us here with the requirements @ChrisChV?

[bradenmacdonald]

@rpenido Here's what I'd like to see:

• The docstring for this new "tag object" endpoint declares it as abstract, and explains why - it doesn't know how to check object permissions. It should not appear in urls.py
• In edx-platform we'll subclass this endpoint, and add a very simple permissions check: if you can edit an XBlock (in Studio), you can edit that XBlock's tags. If you can edit a course (in Studio), you can edit that course's tags. Otherwise, permission denied. This is in addition to the checks in the base class which check the taxonomy side of things: which taxonomies/tags you are allowed to use when you make these edits.

If the written requirements anywhere are different than this, please point it out and I'll get them updated.

[rpenido]

Hi @bradenmacdonald! I was thinking about it and would like to make sure we are on the right path. Today, the permissions are defined in the rules.py (for openedx-learning and edx-platform. So, I created a new rule to check the object_id permission and made the calls accordingly.

I still can remove the update method from the View and create a Mixin to be used in edx-platform. The side effect will be that we will not be able to test the code here.

Also, as soon we change the object_id permission here to False, we will not be able to test the endpoint here.

Let me know what you think! The changes are here: 5851ee21254eda73a36b0c46726590eb6a0fc57b

The initial requirements are here:

• https://github.com/openedx/modular-learning/issues/82

[bradenmacdonald]

I still can remove the update method from the View and create a Mixin to be used in edx-platform. The side effect will be that we will not be able to test the code here.

I was thinking the whole view should just be an independent class, because every CRUD action depends on the user's permissions. Even just reading the tags should only be allowed if the user can "read" the object that is tagged too.

So what if we just make the view so that it is designed to be used as an abstract view. It should work, but it won't be in urls.py by default. However, for tests in this repo, it can be added to urls.py so that it can be tested.

Then, in edx-platform, we can subclass the view, and give it a new URL endpoint, and implement the permissions checks for "can user read/write this content object". This view also will only work with ContentObjectTags.

Then if there are other use cases for tagging in the future, each use case can subclass the base view and implement its own permission checks on top of it.

---

Another way is what you are saying, where the base implementation in openedx-learning just checks the rules and applications like edx-platform that are using it just define the rules. If we want to go that approach, it makes sense, but we need some way to say "this is the rule for ContentObjectTags" and "this is the rule for ObjectTags in general" and set other rules for other subclasses of ObjectTag. I think that would work well too, and then we wouldn't need to subclass the view at all. I'm just not sure if that's how rules works. Were you able to get something like this working?

[rpenido]

Another way is what you are saying, where the base implementation in openedx-learning just checks the rules and applications like edx-platform that are using it just define the rules. If we want to go that approach, it makes sense, but we need some way to say "this is the rule for ContentObjectTags" and "this is the rule for ObjectTags in general" and set other rules for other subclasses of ObjectTag.

It is more like this rule is for 'oel_tagging' (openedx-learning) and this rule is for 'content_tagging' (edx-platform). But we are talking about the same thing here, right? We use the app_label to define witch rule should be checked. The Django ModelViewSet with DjangoObjectPermissions and the rules package works out of the box that way.

I think that would work well too, and then we wouldn't need to subclass the view at all. I'm just not sure if that's how rules works. Were you able to get something like this working?

Yes, if "one rule" per app_label is enough for us (of course we could do any type check inside our rule function to handle anything we want). I think we should stick with the rules only because the other rest apis are using it.

And I if the requirements where made that way because openedx-learning could work as a stand alone app and they want the tagging feature there. If we leave the rule oel_tagging.change_objecttag_objectid True here in oel_tagging, this doesn't affect edx-platform at all.

[bradenmacdonald]

One rule per app_label is fine yep. But how does "rules" know to use a different app label for content tags like tags on an XBlock? Won't it still think the app is oel_tagging because that's where the API is defined?

[rpenido]

One rule per app_label is fine yep. But how does "rules" know to use a different app label for content tags like tags on an XBlock? Won't it still think the app is oel_tagging because that's where the API is defined?

The DjangoObjectPermissions uses the app that owns the Model.

https://github.com/encode/django-rest-framework/blob/40eccb0d6cdb769876dbb79724c5871b4f04163d/rest_framework/permissions.py#L280-L284

But now I understand what you are saying. We can't use the taxonomy_class model app (i.e. LanguageTaxonomy is from oel_tagging). But I'm intrigued by how it worked in the Taxonomy API (the rules work out of the box without issues); I will take a look at it tomorrow.

Thank you for your insight and input @bradenmacdonald !

[rpenido]

I got it. We explicitly override the permission from oel_tagging (not content_tagging) in our rules.py of edx_platform app.

https://github.com/openedx/edx-platform/blob/98fda4110056077fbdd325f325ad8fcf76460124/openedx/features/content_tagging/rules.py#L113

Do you fell that we can go on this route @bradenmacdonald? And is it okay to let the tag permission be enabled in openedx-learning to allow tests?

[bradenmacdonald]

I'm not sure. But if you try it out, and it allows edx-platform to specify the permissions for content objects, other future apps to specify the permissions for tagging their own models, and openedx-tagging to test its API code, then it sounds good.

I see there is both https://github.com/openedx/openedx-learning/blob/f73adefbcf9748dafe8ebfb7e8e6590df6780bb2/openedx_tagging/core/tagging/rules.py#L71 and https://github.com/openedx/edx-platform/blob/98fda4110056077fbdd325f325ad8fcf76460124/openedx/features/content_tagging/rules.py#L113 - how do we know that the platform one always overrides the other? And what is the point of the override when the implementation of can_change_taxonomy() is the same on both sides?

[rpenido]

I checked now (using breakpoints), and I'm sure that NOW the edx-platform overrides the rule.

Is it safe to say that the platform will always override because we do import openedx_tagging.core.tagging.rules as oel_tagging there (so it runs after because of the dependency)? Just want to double-check that.

And what is the point of the override when the implementation of can_change_taxonomy() is the same on both sides?

The code started differently (we had an Organization permission check) and converged when we simplified it. I didn't notice that in the Taxonomy Org PR. My bad!

[bradenmacdonald]

Is it safe to say that the platform will always override because we do import openedx_tagging.core.tagging.rules as oel_tagging there (so it runs after because of the dependency)? Just want to double-check that.

I think so, yes. But there should be some unit tests in edx-platform to check that the permissions are correct just in case.

[rpenido]

Is it safe to say that the platform will always override because we do import openedx_tagging.core.tagging.rules as oel_tagging there (so it runs after because of the dependency)? Just want to double-check that.

I think so, yes. But there should be some unit tests in edx-platform to check that the permissions are correct just in case.

Yes! We have the rule tests here: https://github.com/openedx/edx-platform/blob/98fda4110056077fbdd325f325ad8fcf76460124/openedx/features/content_tagging/tests/test_rules.py

The view tests also cover that!

So, are we good to go that way?

PS: It is better to handle the duplicate rule in the next edx-platform task related to the tagging or create a microtask for that?

[bradenmacdonald]

You can add the cleanup to FAL-3477.

I'm not sure those tests are comprehensive. Because there was a missed requirement here: the requirements/specs for this ticket focused too much on the taxonomies and org-level permissions; those tests are right. But we also need to check if the user has permissions on the object, and I don't see any tests doing that. For example https://github.com/openedx/edx-platform/blob/98fda4110056077fbdd325f325ad8fcf76460124/openedx/features/content_tagging/tests/test_rules.py#L380-L394 is not checking the object permissions at all. We'll have to address that missed requirement in the future.

Obviously if I can't edit an XBlock in Studio, I shouldn't be able to edit that XBlock's tags regardless of what other permissions I have at the org and taxonomy level.

[rpenido]

But we also need to check if the user has permissions on the object, and I don't see any tests doing that.

Yes. The rules don't cover the object permission. This could be done when we create the tag object rest api (I didn't find this task in the project)

[rpenido]

Are we good for a final review here @bradenmacdonald? I think we addressed all the questions here.

The only loose end is the object permission in edx-platform, which should be fixed (the rule and the view with tests) in the task (not created) that implement the rest API for edx-platform.

Am I missing something?

Thank you!

[bradenmacdonald]

@rpenido Sorry, it says it needs to be rebased before it merges.

[rpenido]

Rebased and squashed @bradenmacdonald !

[openedx-webhooks]

@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.