Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
117 changes: 107 additions & 10 deletions openedx/core/djangoapps/content_libraries/api/libraries.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,11 @@
from django.db import IntegrityError, transaction
from django.db.models import Q, QuerySet
from django.utils.translation import gettext as _
from opaque_keys.edx.locator import (
LibraryLocatorV2,
LibraryUsageLocatorV2,
)
from openedx_events.content_authoring.data import (
ContentLibraryData,
)
from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2
from openedx_authz import api as authz_api
from openedx_authz.api import assign_role_to_user_in_scope
from openedx_authz.constants import permissions as authz_permissions
from openedx_events.content_authoring.data import ContentLibraryData
from openedx_events.content_authoring.signals import (
CONTENT_LIBRARY_CREATED,
CONTENT_LIBRARY_DELETED,
Expand All @@ -70,14 +68,14 @@
from organizations.models import Organization
from user_tasks.models import UserTaskArtifact, UserTaskStatus
from xblock.core import XBlock
from openedx_authz.api import assign_role_to_user_in_scope

from openedx.core.types import User as UserType

from .. import permissions, tasks
from ..constants import ALL_RIGHTS_RESERVED
from ..models import ContentLibrary, ContentLibraryPermission
from .exceptions import LibraryAlreadyExists, LibraryPermissionIntegrityError
from .permissions import LEGACY_LIB_PERMISSIONS

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -109,6 +107,7 @@
"revert_changes",
"get_backup_task_status",
"assign_library_role_to_user",
"user_has_permission_across_lib_authz_systems",
]


Expand Down Expand Up @@ -245,7 +244,18 @@ def user_can_create_library(user: AbstractUser) -> bool:
"""
Check if the user has permission to create a content library.
"""
return user.has_perm(permissions.CAN_CREATE_CONTENT_LIBRARY)
library_permission = permissions.CAN_CREATE_CONTENT_LIBRARY
lib_permission_in_authz = _transform_legacy_lib_permission_to_authz_permission(library_permission)
# The authz_api.is_user_allowed check only validates permissions within a specific library context. Since
# creating a library is not tied to an existing one, we use user.has_perm (via Bridgekeeper) to check if the user
# can create libraries, meaning they have the course creator role. In the future, this should rely on a global (*)
# role defined in the Authorization Framework for instance-level resource creation.
has_perms = user.has_perm(library_permission) or authz_api.is_user_allowed(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we differentiate between legacy permissions and new ones? I'm thinking on an edge case where a permission may have the same name in both new and old permission systems, would this be possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

How are legacy permissions going to be managed going forward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we differentiate between legacy permissions and new ones? I'm thinking on an edge case where a permission may have the same name in both new and old permission systems, would this be possible? - @rodmgwgu

For now it won't happend because the old permission contains a namespace 'content_libraries.<permission_name>', and what we use in the checks for authz is only the '<permission_name>', - So for the MVP and what is defined the permissions, they won't have the same name.

But for future migration in the new system, this is something to think about, and it depends on the rules for naming we decide to manage in the openedx-authz permissions.

References:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How are legacy permissions going to be managed going forward? - @bmtcril

For this release we still check them, and for future release the idea is to remove them, to apply the expand/contract pattern to change the libraries permissions system.

Reference: #37409

Copy link
Member

Choose a reason for hiding this comment

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

@MaferMazu: I think in openedx-authz we use the namespace as part of the scope, so I'm not sure if we should revisit adding it to the permission. Do you see a specific use case for this? If so, can you open an issue to do a proper follow-up on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mariajgrimaldi I created a issue related to this openedx/openedx-authz#136, but I think it is something we can hanlde out of the mvp.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note that I think if we make this change after MVP we will need another migration to update existing names. Does that sound right?

Copy link
Member

Choose a reason for hiding this comment

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

yes

user,
lib_permission_in_authz,
authz_api.data.GLOBAL_SCOPE_WILDCARD,
)
return has_perms


def get_libraries_for_user(user, org=None, text_search=None, order=None) -> QuerySet[ContentLibrary]:
Expand Down Expand Up @@ -336,7 +346,7 @@ def require_permission_for_library_key(library_key: LibraryLocatorV2, user: User
library_obj = ContentLibrary.objects.get_by_key(library_key)
# obj should be able to read any valid model object but mypy thinks it can only be
# "User | AnonymousUser | None"
if not user.has_perm(permission, obj=library_obj): # type:ignore[arg-type]
if not user_has_permission_across_lib_authz_systems(user, permission, library_obj):
raise PermissionDenied

return library_obj
Expand Down Expand Up @@ -754,3 +764,90 @@ def get_backup_task_status(
result['file'] = artifact.file

return result


def _transform_legacy_lib_permission_to_authz_permission(permission: str) -> str:
"""
Transform a legacy content library permission to an openedx-authz permission.
"""
# There is no dedicated permission or role for can_create_content_library in openedx-authz yet,
# so we reuse the same permission to rely on user.has_perm via Bridgekeeper.
return {
permissions.CAN_CREATE_CONTENT_LIBRARY: permissions.CAN_CREATE_CONTENT_LIBRARY,
permissions.CAN_DELETE_THIS_CONTENT_LIBRARY: authz_permissions.DELETE_LIBRARY.identifier,
permissions.CAN_EDIT_THIS_CONTENT_LIBRARY: authz_permissions.EDIT_LIBRARY_CONTENT.identifier,
permissions.CAN_EDIT_THIS_CONTENT_LIBRARY_TEAM: authz_permissions.MANAGE_LIBRARY_TEAM.identifier,
permissions.CAN_VIEW_THIS_CONTENT_LIBRARY: authz_permissions.VIEW_LIBRARY.identifier,
permissions.CAN_VIEW_THIS_CONTENT_LIBRARY_TEAM: authz_permissions.VIEW_LIBRARY_TEAM.identifier,
}.get(permission, permission)


def _transform_authz_permission_to_legacy_lib_permission(permission: str) -> str:
"""
Transform an openedx-authz permission to a legacy content library permission.
"""
return {
authz_permissions.PUBLISH_LIBRARY_CONTENT.identifier: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY,
authz_permissions.CREATE_LIBRARY_COLLECTION.identifier: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY,
authz_permissions.EDIT_LIBRARY_COLLECTION.identifier: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY,
authz_permissions.DELETE_LIBRARY_COLLECTION.identifier: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY,
}.get(permission, permission)


def user_has_permission_across_lib_authz_systems(
user: UserType,
permission: str | authz_api.data.PermissionData,
library_obj: ContentLibrary,
) -> bool:
"""
Check whether a user has a given permission on a content library across both the
legacy edx-platform permission system and the newer openedx-authz system.

The provided permission name is normalized to both systems (legacy and authz), and
authorization is granted if either:
- the user holds the legacy object-level permission on the ContentLibrary instance, or
- the openedx-authz API allows the user for the corresponding permission on the library.

**Note:**
Temporary: this function uses Bridgekeeper-based logic for cases not yet modeled in openedx-authz.

Current gaps covered here:
- CAN_CREATE_CONTENT_LIBRARY: we call user.has_perm via Bridgekeeper to verify the user is a course creator.
- CAN_VIEW_THIS_CONTENT_LIBRARY: we respect the allow_public_read flag via Bridgekeeper.

Replace these with authz_api.is_user_allowed once openedx-authz supports
these conditions natively (including global (*) roles).

Args:
user: The Django user (or user-like object) to check.
permission: The permission identifier (either a legacy codename or an openedx-authz name).
library_obj: The ContentLibrary instance to check against.

Returns:
bool: True if the user is authorized by either system; otherwise False.
"""
if isinstance(permission, authz_api.data.PermissionData):
permission = permission.identifier
if _is_legacy_permission(permission):
legacy_permission = permission
authz_permission = _transform_legacy_lib_permission_to_authz_permission(permission)
else:
authz_permission = permission
legacy_permission = _transform_authz_permission_to_legacy_lib_permission(permission)
return (
# Check both the legacy and the new openedx-authz permissions
user.has_perm(perm=legacy_permission, obj=library_obj)
or authz_api.is_user_allowed(
user,
authz_permission,
str(library_obj.library_key),
)
)


def _is_legacy_permission(permission: str) -> bool:
"""
Determine if the specified library permission is part of the legacy
or the new openedx-authz system.
"""
return permission in LEGACY_LIB_PERMISSIONS
10 changes: 10 additions & 0 deletions openedx/core/djangoapps/content_libraries/api/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,13 @@
CAN_VIEW_THIS_CONTENT_LIBRARY,
CAN_VIEW_THIS_CONTENT_LIBRARY_TEAM
)

LEGACY_LIB_PERMISSIONS = frozenset({
CAN_CREATE_CONTENT_LIBRARY,
CAN_DELETE_THIS_CONTENT_LIBRARY,
CAN_EDIT_THIS_CONTENT_LIBRARY,
CAN_EDIT_THIS_CONTENT_LIBRARY_TEAM,
CAN_LEARN_FROM_THIS_CONTENT_LIBRARY,
CAN_VIEW_THIS_CONTENT_LIBRARY,
CAN_VIEW_THIS_CONTENT_LIBRARY_TEAM,
})
3 changes: 2 additions & 1 deletion openedx/core/djangoapps/content_libraries/rest_api/blocks.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from django.utils.decorators import method_decorator
from drf_yasg.utils import swagger_auto_schema
from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2
from openedx_authz.constants import permissions as authz_permissions
from openedx_learning.api import authoring as authoring_api
from rest_framework import status
from rest_framework.exceptions import NotFound, ValidationError
Expand Down Expand Up @@ -238,7 +239,7 @@ def post(self, request, usage_key_str):
api.require_permission_for_library_key(
key.lib_key,
request.user,
permissions.CAN_EDIT_THIS_CONTENT_LIBRARY
authz_permissions.PUBLISH_LIBRARY_CONTENT
)
api.publish_component_changes(key, request.user)
return Response({})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from rest_framework.status import HTTP_204_NO_CONTENT

from opaque_keys.edx.locator import LibraryLocatorV2
from openedx_authz.constants import permissions as authz_permissions
from openedx_learning.api import authoring as authoring_api
from openedx_learning.api.authoring_models import Collection

Expand Down Expand Up @@ -56,7 +57,6 @@ def get_content_library(self) -> ContentLibrary:
if self.request.method in ['OPTIONS', 'GET']
else permissions.CAN_EDIT_THIS_CONTENT_LIBRARY
)

self._content_library = api.require_permission_for_library_key(
library_key,
self.request.user,
Expand Down Expand Up @@ -110,6 +110,11 @@ def create(self, request: RestRequest, *args, **kwargs) -> Response:
Create a Collection that belongs to a Content Library
"""
content_library = self.get_content_library()
api.require_permission_for_library_key(
content_library.library_key,
request.user,
authz_permissions.CREATE_LIBRARY_COLLECTION
)
create_serializer = ContentLibraryCollectionUpdateSerializer(data=request.data)
create_serializer.is_valid(raise_exception=True)

Expand Down Expand Up @@ -144,6 +149,11 @@ def partial_update(self, request: RestRequest, *args, **kwargs) -> Response:
Update a Collection that belongs to a Content Library
"""
content_library = self.get_content_library()
api.require_permission_for_library_key(
content_library.library_key,
request.user,
authz_permissions.EDIT_LIBRARY_COLLECTION
)
collection_key = kwargs["key"]

update_serializer = ContentLibraryCollectionUpdateSerializer(
Expand All @@ -165,6 +175,12 @@ def destroy(self, request: RestRequest, *args, **kwargs) -> Response:
"""
Soft-deletes a Collection that belongs to a Content Library
"""
content_library = self.get_content_library()
api.require_permission_for_library_key(
content_library.library_key,
request.user,
authz_permissions.DELETE_LIBRARY_COLLECTION
)
collection = super().get_object()
assert collection.learning_package_id
authoring_api.delete_collection(
Expand All @@ -181,6 +197,11 @@ def restore(self, request: RestRequest, *args, **kwargs) -> Response:
Restores a soft-deleted Collection that belongs to a Content Library
"""
content_library = self.get_content_library()
api.require_permission_for_library_key(
content_library.library_key,
request.user,
authz_permissions.EDIT_LIBRARY_COLLECTION
)
assert content_library.learning_package_id
collection_key = kwargs["key"]
authoring_api.restore_collection(
Expand All @@ -198,6 +219,11 @@ def update_items(self, request: RestRequest, *args, **kwargs) -> Response:
Collection and items must all be part of the given library/learning package.
"""
content_library = self.get_content_library()
api.require_permission_for_library_key(
content_library.library_key,
request.user,
authz_permissions.EDIT_LIBRARY_COLLECTION
)
collection_key = kwargs["key"]

serializer = ContentLibraryItemKeysSerializer(data=request.data)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from drf_yasg import openapi

from opaque_keys.edx.locator import LibraryLocatorV2, LibraryContainerLocator
from openedx_authz.constants import permissions as authz_permissions
from openedx_learning.api import authoring as authoring_api
from rest_framework.generics import GenericAPIView
from rest_framework.response import Response
Expand Down Expand Up @@ -379,7 +380,7 @@ def post(self, request: RestRequest, container_key: LibraryContainerLocator) ->
api.require_permission_for_library_key(
container_key.lib_key,
request.user,
permissions.CAN_EDIT_THIS_CONTENT_LIBRARY,
authz_permissions.PUBLISH_LIBRARY_CONTENT
)
api.publish_container_changes(container_key, request.user.id)
# If we need to in the future, we could return a list of all the child containers/components that were
Expand Down
11 changes: 8 additions & 3 deletions openedx/core/djangoapps/content_libraries/rest_api/libraries.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
from user_tasks.models import UserTaskStatus

from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2
from openedx_authz.constants import permissions as authz_permissions
from organizations.api import ensure_organization
from organizations.exceptions import InvalidOrganizationException
from organizations.models import Organization
Expand Down Expand Up @@ -219,7 +220,7 @@ def post(self, request):
"""
Create a new content library.
"""
if not request.user.has_perm(permissions.CAN_CREATE_CONTENT_LIBRARY):
if not api.user_can_create_library(request.user):
raise PermissionDenied
serializer = ContentLibraryMetadataSerializer(data=request.data)
serializer.is_valid(raise_exception=True)
Expand Down Expand Up @@ -479,7 +480,11 @@ def post(self, request, lib_key_str):
descendants.
"""
key = LibraryLocatorV2.from_string(lib_key_str)
api.require_permission_for_library_key(key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY)
api.require_permission_for_library_key(
key,
request.user,
authz_permissions.PUBLISH_LIBRARY_CONTENT
)
api.publish_changes(key, request.user.id)
return Response({})

Expand Down Expand Up @@ -838,7 +843,7 @@ def post(self, request):
"""
Restore a library from a backup file.
"""
if not request.user.has_perm(permissions.CAN_CREATE_CONTENT_LIBRARY):
if not api.user_can_create_library(request.user):
raise PermissionDenied

serializer = LibraryRestoreFileSerializer(data=request.data)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from user_tasks.models import UserTaskStatus

from openedx.core.djangoapps.content_libraries.tasks import LibraryRestoreTask
from openedx.core.djangoapps.content_libraries import api
from openedx.core.djangoapps.content_libraries.api.containers import ContainerType
from openedx.core.djangoapps.content_libraries.constants import ALL_RIGHTS_RESERVED, LICENSE_OPTIONS
from openedx.core.djangoapps.content_libraries.models import (
Expand Down Expand Up @@ -75,7 +76,8 @@ def get_can_edit_library(self, obj):
return False

library_obj = ContentLibrary.objects.get_by_key(obj.key)
return user.has_perm(permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, obj=library_obj)
return api.user_has_permission_across_lib_authz_systems(
user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, library_obj)


class ContentLibraryUpdateSerializer(serializers.Serializer):
Expand Down
Loading
Loading