fix: update taxonomies permission rules by rpenido · Pull Request #33413 · openedx/openedx-platform

rpenido · 2023-10-04T14:50:27Z

Description

Updates the content tagging rest API permissions according to spec.

Supporting Information

Closes [Tagging] Permissions for Taxonomies List Page and Taxonomy Detail Page modular-learning#129
Depends:
- feat: add object tag view permissions [FC-0036] openedx-core#94
- Continued simplification of Taxonomy model, squashed migrations [FC-0030] #33438

Testing instructions

Please ensure that the tests cover the expected behavior of the view as described in the related issue.

Before Merge

Update openedx-learning version

Private-ref: FAL-3518

openedx-webhooks · 2023-10-04T14:50:33Z

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.

pomegranited · 2023-10-09T05:13:06Z

I'm not sure how we're supposed to handle a Taxonomy that's linked to "all orgs" and linked to one or more specific orgs -- which takes precedence, the "all orgs" row, or the specific org rows?

I would guess that "all orgs" takes precedence.

So I'd expect this check to look like this, and is_all_org to not be referenced again in this function:

Suggested change

# Only taxonomy admins can view disabled all org taxonomies

if is_all_org and not taxonomy.enabled:

return False

# Only taxonomy admins can view disabled all org taxonomies

if is_all_org:

return taxonomy.enabled

I agree that "all orgs" should take precedence.

We can't change the check that way because we would enable an ordinary user (without org) to see this taxonomy. We can short circuit a False here (denying permission) but to return a True we need additional checks.

But reviewing the code, I think we don't need to check is_all_org after:

# Org-level staff can view any taxonomy that is associated with one of their orgs. if is_org_admin(taxonomy_orgs, user): return True

@rpenido I don't think your conclusion ^ is correct?

We can't change the check that way because we would enable an ordinary user (without org) to see this taxonomy.

This comment says:

Enabled Taxonomy linked to all orgs - any registered user can view the taxonomy and its tags (3)

Disabled Taxonomy linked to all orgs - only global staff can view the taxonomy and its tags (5)

So my original suggestion stands:

Suggested change

# Only taxonomy admins can view disabled all org taxonomies

if is_all_org and not taxonomy.enabled:

return False

# Enabled all-org taxonomies can be viewed by any registered user.

if is_all_org:

return taxonomy.enabled

You're right @pomegranited !
Done: dd4696a110c9d22baa1780fc8fcf6d63438cef01

I'll check/fix the related test tomorrow

pomegranited · 2023-10-09T05:38:02Z

"Only taxonomy admins can tag objects using tags from disabled taxonomies."

I'm not sure about this.. I think "disabled" means it can't be used to tag objects, even if you're an admin.

I think that it could make sense to let an admin clean tags from a disabled taxonomy. But we can simplify and let only enabled taxonomies be used.

What do you think?

"Only taxonomy admins can tag objects using tags from disabled taxonomies."

Yeah I agree with @pomegranited. I don't think we have any need for this. In fact for the MVP we won't really be using disabled taxonomies at all.

Either of these options is fine with me:

If the taxonomy is disabled, nobody can modify its tags on an object.

If the taxonomy is disabled, admins can remove tags from objects, but not add new ones.

This only affects the API. In the UI, we will never display tags from disabled taxonomies, even to admins. (I think)

Agreed!
I went to: "If the taxonomy is disabled, nobody can modify its tags on an object."

pomegranited · 2023-10-09T05:40:51Z

Why did this have to change? I think it's still useful to include a taxonomy in the tests which is disabled + all_orgs.

I changed it to fix the following test:

https://github.com/open-craft/edx-platform/blob/aeeebf948941ed37c9cc8b28e15315e1d8d4f62a/openedx/core/djangoapps/content_tagging/tests/test_rules.py#L259C1-L278C57

We have disabled + all_orgs test here:
https://github.com/open-craft/edx-platform/blob/aeeebf948941ed37c9cc8b28e15315e1d8d4f62a/openedx/core/djangoapps/content_tagging/tests/test_rules.py#L375C1-L392C57

rpenido · 2023-10-12T21:16:07Z

Hi @pomegranited !
Tests refactored, and we are ready for a review here.

We have a Lint error:

Do not depend on non-public API of isolated apps.
-------------------------------------------------

openedx.core.djangoapps.content_tagging.rest_api.v1.tests.test_views:31: imported from non-public API of openedx.core.djangoapps.content_libraries:
    from openedx.core.djangoapps.content_libraries.models import ContentLibrary

I needed to create a ContentLibrary for the tests (to check our condition no. 8). Should we ignore this lint or is there a better way to handle this in the tests?

Users who can view any v2 content libraries belonging to an org (CAN_VIEW_THIS_CONTENT_LIBRARY) can view that org's enabled taxonomies.

Also, after implementing condition 8, I saw a potential problem.
I'm using get_libraries_for_user to see if the user has permission on some library from that org to show them the taxonomies (there is a better way?).

If an Org has a ContentLibrary with allow_public_read = True, everyone who has access to studio will see its taxonomies. I think this is not desirable, but is it a real problem that should be discussed now?
The allow_public_learn=true, which gives access to everyone (not only studio users) does not impact us right now.

I also noted that we are not preventing a user with access to more than one org from "cross tag" objects.
I.e: use a taxonomy from OrgA to tag an object from OrbB. Probably, the frontend will avoid this. Should I also handle this problem in the backend on this task?

bradenmacdonald · 2023-10-12T21:37:01Z

@rpenido

I needed to create a ContentLibrary for the tests (to check our condition no. 8). Should we ignore this lint or is there a better way to handle this in the tests?

You need to use the public create_library API - not import the app's internal ContentLibrary Django model.

bradenmacdonald · 2023-10-12T21:38:32Z

If an Org has a ContentLibrary with allow_public_read = True, everyone who has access to studio will see its taxonomies.

I think that's actually fine. Taxonomies aren't super private.

pomegranited · 2023-10-16T01:15:18Z

@@ -735,61 +1164,66 @@ def test_tag_course(self, user_attr, taxonomy_attr, tag_values, expected_status)
            assert set(t["value"] for t in response.data) == set(tag_values)



@rpenido

Subjective comment: you could split test_tag_course into two test methods, one for the successful staffA and staff users, and one for the forbidden user attempts. That would reduce the number of ddt parameters required, and negate the need for this if status.is_success clause.

But objectively: I still don't see any tests for the GET ObjectTag endpoint in this file? You could add get view tests after these successful PUTs like this:

Suggested change

# Check that re-fetching the tags returns what we set

response = self.client.get(url, format="json")

assert status.is_success(response.status_code)

assert set(t["value"] for t in response.data) == set(tag_values)

Done 9695d67

pomegranited · 2023-10-16T01:19:55Z

@rpenido One comment about missing tests, but this still looks good 👍 .

FYI, while a PR is actively being reviewed, if you merge latest master instead of rebase, then you don't have to force-push, and your reviewer can easily see what's been changed since they last looked at your PR. These show up as merge commits on your PR, but disappear when the PR gets merged.

rpenido · 2023-10-16T21:32:52Z

@bradenmacdonald Package updated and tests green.
This is ready for an review when you have time!

rpenido · 2023-10-16T21:35:47Z

@rpenido One comment about missing tests, but this still looks good 👍 .

FYI, while a PR is actively being reviewed, if you merge latest master instead of rebase, then you don't have to force-push, and your reviewer can easily see what's been changed since they last looked at your PR. These show up as merge commits on your PR, but disappear when the PR gets merged.

Sorry for that @pomegranited!
The problem wasn't a rebase: I needed to force push to reword a commit that was not passing the lint (tests: vs. test:, I always use the wrong one!)

bradenmacdonald

Looks great! Just one minor thing and question, then I'll approve.

bradenmacdonald · 2023-10-16T19:01:01Z

+    )
+    def test_detail_taxonomy_staff_see_all(self, taxonomy_attr: str) -> None:
+        """
+        Tests that org users can't see taxonomies from other orgs


This docstring is wrong.

Done 66158a7!

bradenmacdonald · 2023-10-17T17:48:47Z

+class TestTaxonomyListCreateViewSet(TestTaxonomyObjectsMixin, APITestCase):
    """
-    Test cases for TaxonomyViewSet when ENABLE_CREATOR_GROUP is True
+    Test cases for TestTaxonomyReadViewSet when ENABLE_CREATOR_GROUP is True


Does ENABLE_CREATOR_GROUP have any effect at all on the taxonomy code/permissions? I'm unclear why we have different tests based on that.

No.
After we stopped using the is_content_creator check, the FLAG didn't impact our code anymore.

If any, these tests tell us that the FLAG is not affecting our permissions.
I think we can safely clear this. What do you think?

I think it's confusing to mention that flag if it has no effect. Maybe just add one little test case that ensures things work the same way with or without that flag, but even that is probably not necessary.

Yeah I agree -- we used to care about this flag, but we don't anymore with the new permissions, so these extra tests can be removed.

Great! I will fix this tomorrow ping you here!

Done 217cedb
@bradenmacdonald @pomegranited

openedx-webhooks · 2023-10-19T17:54:34Z

@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.

edx-pipeline-bot · 2023-10-19T19:02:27Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-10-19T19:20:57Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2023-10-19T19:20:58Z

2U Release Notice: This PR has been deployed to the edX production environment.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 4, 2023

pomegranited reviewed Oct 9, 2023

View reviewed changes

rpenido changed the title ~~fix: update rules~~ fix: update Taxonomies permission rules Oct 9, 2023

rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from 13da9ee to 62e6984 Compare October 9, 2023 14:51

rpenido mentioned this pull request Oct 9, 2023

Continued simplification of Taxonomy model, squashed migrations [FC-0030] #33438

Merged

rpenido commented Oct 10, 2023

View reviewed changes

Comment thread openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py Outdated

pomegranited reviewed Oct 11, 2023

View reviewed changes

rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from c29f48f to db0229f Compare October 11, 2023 21:54

rpenido changed the title ~~fix: update Taxonomies permission rules~~ fix: update taxonomies permission rules Oct 11, 2023

rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch 2 times, most recently from 851bb83 to 2a419d0 Compare October 12, 2023 01:01

rpenido mentioned this pull request Oct 12, 2023

feat: override permissions for export taxonomy [FC-0036] #33483

Merged

rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch 2 times, most recently from a055fac to ec80122 Compare October 12, 2023 20:45

fix: update taxonomies permission rules

9594728

rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from ec80122 to 9594728 Compare October 12, 2023 21:15

fix: rename ChangeObjectTagPermissionItem -> ObjectTagPermissionItem

fd29a6d

rpenido added 3 commits October 13, 2023 14:50

refactor: use content library api

09b62ff

test: fix tests

a1cc0fa

fix: lint-import

999fe76

rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from 7b12224 to 999fe76 Compare October 13, 2023 18:49

rpenido added 4 commits October 13, 2023 16:00

test: fix constant

2d2555f

test: add export tests

5223bcc

fix: pylint and import

b420d59

style: fix pylint

32d20b2

pomegranited mentioned this pull request Oct 14, 2023

feat: add object tag view permissions [FC-0036] openedx/openedx-core#94

Merged

test: fix test_rules

8c329be

pomegranited reviewed Oct 16, 2023

View reviewed changes

rpenido added 3 commits October 16, 2023 14:19

test: add get objet tags test

9695d67

Merge branch 'master' into rpenido/fal-3518-permissions-for-taxonomies

5af9f13

refactor: remove can_change_object_tag_taxonomy

381f4df

rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from 2510a48 to 381f4df Compare October 16, 2023 18:46

rpenido added 8 commits October 16, 2023 15:51

Merge branch 'master' into rpenido/fal-3518-permissions-for-taxonomies

d1f4091

fix: wrong permission name

575db34

fix: override can_view_object_tag_taxonomy rule

992a28c

Merge branch 'master' into rpenido/fal-3518-permissions-for-taxonomies

4f80c1f

chore: update requirements

10c34b4

refactor: add type guards

9f24e32

fix: add type guards

5398ee7

Merge branch 'master' into rpenido/fal-3518-permissions-for-taxonomies

02dbef4

bradenmacdonald reviewed Oct 17, 2023

View reviewed changes

docs: fix docstring

66158a7

rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch 2 times, most recently from 4d5c569 to 98064bc Compare October 18, 2023 12:47

test: remove ENABLE_CREATOR_GROUP tests and fix some docstrings

217cedb

rpenido force-pushed the rpenido/fal-3518-permissions-for-taxonomies branch from 98064bc to 217cedb Compare October 18, 2023 14:26

bradenmacdonald approved these changes Oct 19, 2023

View reviewed changes

bradenmacdonald merged commit 2482dd4 into openedx:master Oct 19, 2023

bradenmacdonald deleted the rpenido/fal-3518-permissions-for-taxonomies branch October 19, 2023 17:54

-    # Only taxonomy admins can view disabled all org taxonomies
-    if is_all_org and not taxonomy.enabled:
-        return False
+    # Only taxonomy admins can view disabled all org taxonomies
+    if is_all_org:
+        return taxonomy.enabled

		@@ -735,61 +1164,66 @@ def test_tag_course(self, user_attr, taxonomy_attr, tag_values, expected_status)
		assert set(t["value"] for t in response.data) == set(tag_values)

+            # Check that re-fetching the tags returns what we set
+            response = self.client.get(url, format="json")
+            assert status.is_success(response.status_code)
+            assert set(t["value"] for t in response.data) == set(tag_values)

Uh oh!

Conversation

rpenido commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting Information

Testing instructions

Before Merge

Uh oh!

openedx-webhooks commented Oct 4, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rpenido commented Oct 12, 2023

Uh oh!

bradenmacdonald commented Oct 12, 2023

Uh oh!

bradenmacdonald commented Oct 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomegranited commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rpenido commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rpenido commented Oct 16, 2023

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpenido commented Oct 4, 2023 •

edited

Loading

pomegranited commented Oct 16, 2023 •

edited

Loading

rpenido commented Oct 16, 2023 •

edited

Loading

bradenmacdonald Oct 17, 2023 •

edited

Loading