Skip to content
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

fix: update taxonomies permission rules #33413

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
9594728
fix: update taxonomies permission rules
rpenido Oct 12, 2023
fd29a6d
fix: rename ChangeObjectTagPermissionItem -> ObjectTagPermissionItem
rpenido Oct 12, 2023
09b62ff
refactor: use content library api
rpenido Oct 13, 2023
a1cc0fa
test: fix tests
rpenido Oct 13, 2023
999fe76
fix: lint-import
rpenido Oct 13, 2023
2d2555f
test: fix constant
rpenido Oct 13, 2023
5223bcc
test: add export tests
rpenido Oct 13, 2023
b420d59
fix: pylint and import
rpenido Oct 13, 2023
32d20b2
style: fix pylint
rpenido Oct 13, 2023
5d85063
fix: use correct ObjectTagOrgViewSet
rpenido Oct 14, 2023
02deb59
refactor: cleaning unused methods
rpenido Oct 14, 2023
43ebe36
chore: update requirements constraints
rpenido Oct 14, 2023
86ee948
test: fix test and remove unecessary checks
rpenido Oct 14, 2023
bfbc1d9
style: remove comments
rpenido Oct 14, 2023
ac71790
Merge branch 'master' into rpenido/fal-3518-permissions-for-taxonomies
rpenido Oct 14, 2023
6803f50
refactor: fix pylint
rpenido Oct 14, 2023
a6b4b46
fix: pylint
rpenido Oct 15, 2023
334822d
style: fix pylint
rpenido Oct 15, 2023
990039c
fix: override export_taxonomy rule
rpenido Oct 15, 2023
7c9a907
fix: update rule use
rpenido Oct 15, 2023
6a04d1b
style: fix pylint
rpenido Oct 15, 2023
8c329be
test: fix test_rules
rpenido Oct 15, 2023
9695d67
test: add get objet tags test
rpenido Oct 16, 2023
5af9f13
Merge branch 'master' into rpenido/fal-3518-permissions-for-taxonomies
rpenido Oct 16, 2023
381f4df
refactor: remove can_change_object_tag_taxonomy
rpenido Oct 16, 2023
d1f4091
Merge branch 'master' into rpenido/fal-3518-permissions-for-taxonomies
rpenido Oct 16, 2023
575db34
fix: wrong permission name
rpenido Oct 16, 2023
992a28c
fix: override can_view_object_tag_taxonomy rule
rpenido Oct 16, 2023
4f80c1f
Merge branch 'master' into rpenido/fal-3518-permissions-for-taxonomies
rpenido Oct 16, 2023
10c34b4
chore: update requirements
rpenido Oct 16, 2023
9f24e32
refactor: add type guards
rpenido Oct 16, 2023
5398ee7
fix: add type guards
rpenido Oct 16, 2023
02dbef4
Merge branch 'master' into rpenido/fal-3518-permissions-for-taxonomies
rpenido Oct 16, 2023
66158a7
docs: fix docstring
rpenido Oct 17, 2023
217cedb
test: remove ENABLE_CREATOR_GROUP tests and fix some docstrings
rpenido Oct 18, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion openedx/core/djangoapps/content_tagging/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,24 @@ def create_taxonomy(
enabled=True,
allow_multiple=False,
allow_free_text=False,
orgs: list[Organization] | None = None,
) -> Taxonomy:
"""
Creates, saves, and returns a new Taxonomy with the given attributes.
"""
return oel_tagging.create_taxonomy(
taxonomy = oel_tagging.create_taxonomy(
name=name,
description=description,
enabled=enabled,
allow_multiple=allow_multiple,
allow_free_text=allow_free_text,
)

if orgs is not None:
set_taxonomy_orgs(taxonomy=taxonomy, all_orgs=False, orgs=orgs)

return taxonomy


def set_taxonomy_orgs(
taxonomy: Taxonomy,
Expand Down
76 changes: 74 additions & 2 deletions openedx/core/djangoapps/content_tagging/rest_api/v1/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,92 @@
API Filters for content tagging org
"""

from django.db.models import Exists, OuterRef, Q
from rest_framework.filters import BaseFilterBackend

import openedx_tagging.core.tagging.rules as oel_tagging
from organizations.models import Organization

from ...rules import get_admin_orgs, get_user_orgs
from ...models import TaxonomyOrg


class UserOrgFilterBackend(BaseFilterBackend):
"""
Filter taxonomies based on user's orgs roles

Taxonomy admin can see all taxonomies
Everyone else can see only enabled taxonomies
Org staff can see all taxonomies from their orgs
Content creators and instructors can see enabled taxonomies avaliable to their orgs
"""

def filter_queryset(self, request, queryset, _):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your filters here look great, @rpenido . But I'm concerned about the low (71%) test coverage of this filters.py file.

It looks like ObjectTagTaxonomyOrgFilterBackend isn't used by the tests at all, I'm guessing because TestObjectTagViewSet only tests ObjectTag creation, not fetching ObjectTags?

If you add tests to GET ObjectTags too for the various types of org/users and object_ids, that should cover the can_view_object_tag_objectid rule too.

Copy link
Contributor Author

@rpenido rpenido Oct 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I messed up on some merge conflicts. I was using the View from oel_tagging, not the one overridden here.
Fixed here (5d85063), but I will check why the tests didn't catch this.

if oel_tagging.is_taxonomy_admin(request.user):
return queryset

orgs = list(Organization.objects.all())
user_admin_orgs = get_admin_orgs(request.user, orgs)
user_orgs = get_user_orgs(request.user, orgs) # Orgs that the user is a content creator or instructor

if len(user_orgs) == 0 and len(user_admin_orgs) == 0:
return queryset.none()

return queryset.filter(
# Get enabled taxonomies available to all orgs, or from orgs that the user is
# a content creator or instructor
Q(
Exists(
TaxonomyOrg.objects
.filter(
taxonomy=OuterRef("pk"),
rel_type=TaxonomyOrg.RelType.OWNER,
)
.filter(
Q(org=None) |
Q(org__in=user_orgs)
)
),
enabled=True,
) |
# Get all taxonomies from orgs that the user is OrgStaff
Q(
Exists(
TaxonomyOrg.objects
.filter(taxonomy=OuterRef("pk"), rel_type=TaxonomyOrg.RelType.OWNER)
.filter(org__in=user_admin_orgs)
)
)
)


class ObjectTagTaxonomyOrgFilterBackend(BaseFilterBackend):
"""
Filter for ObjectTagViewSet to only show taxonomies that the user can view.
"""

def filter_queryset(self, request, queryset, _):
if oel_tagging.is_taxonomy_admin(request.user):
return queryset

return queryset.filter(enabled=True)
orgs = list(Organization.objects.all())
user_admin_orgs = get_admin_orgs(request.user, orgs)
user_orgs = get_user_orgs(request.user, orgs)
user_or_admin_orgs = list(set(user_orgs) | set(user_admin_orgs))

return queryset.filter(taxonomy__enabled=True).filter(
# Get ObjectTags from taxonomies available to all orgs, or from orgs that the user is
# a OrgStaff, content creator or instructor
Q(
Exists(
TaxonomyOrg.objects
.filter(
taxonomy=OuterRef("taxonomy_id"),
rel_type=TaxonomyOrg.RelType.OWNER,
)
.filter(
Q(org=None) |
Q(org__in=user_or_admin_orgs)
)
)
)
)
Loading
Loading