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

Improve v2 library block permissions checks for read-only authors #35598

Merged
2 changes: 1 addition & 1 deletion cms/templates/content_libraries/xblock_iframe.html
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@
<!-- The default stylesheet will set the body min-height to 100% (a common strategy to allow for background
images to fill the viewport), but this has the undesireable side-effect of causing an infinite loop via the onResize
event listeners below, in certain situations. Resetting it to the default "auto" skirts the problem.-->
<body style="min-height: auto">
<body style="min-height: auto; background-color: white;">
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think using transparent here will make it more adaptable to mfe theming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think a lot of the XBlocks and course content are designed assuming that there is a white background. This is also used for LTI launches, and then we can't control what background color it appears on. So I think white is the safest for now. But I'm not sure.

<!-- fragment body -->
{{ fragment.body_html | safe }}
<!-- fragment foot -->
Expand Down
1 change: 1 addition & 0 deletions cms/templates/xblock_v2/xblock_iframe.html
13 changes: 13 additions & 0 deletions openedx/core/djangoapps/content_libraries/library_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,19 @@ def can_edit_block(self, user, usage_key):

return self.block_exists(usage_key)

def can_view_block_for_editing(self, user, usage_key):
"""
Does the specified usage key exist in its context, and if so, does the
specified user have permission to view its fields and OLX details (but
not necessarily to make changes to it).
"""
try:
api.require_permission_for_library_key(usage_key.lib_key, user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY)
except (PermissionDenied, api.ContentLibraryNotFound):
return False

return self.block_exists(usage_key)
bradenmacdonald marked this conversation as resolved.
Show resolved Hide resolved

def can_view_block(self, user, usage_key):
"""
Does the specified usage key exist in its context, and if so, does the
Expand Down
15 changes: 12 additions & 3 deletions openedx/core/djangoapps/content_libraries/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
Permissions for Content Libraries (v2, Learning-Core-based)
"""
from bridgekeeper import perms, rules
from bridgekeeper.rules import Attribute, ManyRelation, Relation, in_current_groups
from bridgekeeper.rules import Attribute, ManyRelation, Relation, blanket_rule, in_current_groups
from django.conf import settings

from openedx.core.djangoapps.content_libraries.models import ContentLibraryPermission

Expand Down Expand Up @@ -41,6 +42,12 @@
)


# Are we in Studio? (Is there a better or more contextual way to define this, e.g. get from learning context?)
bradenmacdonald marked this conversation as resolved.
Show resolved Hide resolved
@blanket_rule
def is_studio_request(_):
return settings.ROOT_URLCONF == "cms.urls"


########################### Permissions ###########################

# Is the user allowed to view XBlocks from the specified content library
Expand All @@ -51,10 +58,12 @@
perms[CAN_LEARN_FROM_THIS_CONTENT_LIBRARY] = (
# Global staff can learn from any library:
is_global_staff |
# Regular users can learn if the library allows public learning:
# Regular and even anonymous users can learn if the library allows public learning:
Attribute('allow_public_learning', True) |
# Users/groups who are explicitly granted permission can learn from the library:
(is_user_active & has_explicit_read_permission_for_library)
(is_user_active & has_explicit_read_permission_for_library) |
# Or, in Studio (but not the LMS) any users can access libraries with "public read" permissions:
(is_studio_request & is_user_active & Attribute('allow_public_read', True))
)

# Is the user allowed to create content libraries?
Expand Down
9 changes: 9 additions & 0 deletions openedx/core/djangoapps/content_libraries/tests/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -308,3 +308,12 @@ def _get_block_handler_url(self, block_key, handler_name):
"""
url = URL_BLOCK_GET_HANDLER_URL.format(block_key=block_key, handler_name=handler_name)
return self._api('get', url, None, expect_response=200)["handler_url"]

def _get_library_block_fields(self, block_key, expect_response=200):
""" Get the fields of a specific block in the library. This API is only used by the MFE editors. """
result = self._api('get', URL_BLOCK_FIELDS_URL.format(block_key=block_key), None, expect_response)
return result

def _set_library_block_fields(self, block_key, new_fields, expect_response=200):
""" Set the fields of a specific block in the library. This API is only used by the MFE editors. """
return self._api('post', URL_BLOCK_FIELDS_URL.format(block_key=block_key), new_fields, expect_response)
Original file line number Diff line number Diff line change
Expand Up @@ -635,21 +635,27 @@ def test_library_permissions(self): # pylint: disable=too-many-statements
# A random user cannot read OLX nor assets (this library has allow_public_read False):
with self.as_user(random_user):
self._get_library_block_olx(block3_key, expect_response=403)
self._get_library_block_fields(block3_key, expect_response=404)
self._get_library_block_assets(block3_key, expect_response=403)
self._get_library_block_asset(block3_key, file_name="whatever.png", expect_response=403)
# Nor can they preview the block:
self._render_block_view(block3_key, view_name="student_view", expect_response=404)
# But if we grant allow_public_read, then they can:
with self.as_user(admin):
self._update_library(lib_id, allow_public_read=True)
# self._set_library_block_asset(block3_key, "whatever.png", b"data")
with self.as_user(random_user):
self._get_library_block_olx(block3_key)
self._render_block_view(block3_key, view_name="student_view")
f = self._get_library_block_fields(block3_key)
# self._get_library_block_assets(block3_key)
# self._get_library_block_asset(block3_key, file_name="whatever.png")

# Users without authoring permission cannot edit nor delete XBlocks (this library has allow_public_read False):
# Users without authoring permission cannot edit nor delete XBlocks:
for user in [reader, random_user]:
with self.as_user(user):
self._set_library_block_olx(block3_key, "<problem/>", expect_response=403)
self._set_library_block_fields(block3_key, {"data": "<problem />", "metadata": {}}, expect_response=404)
# self._set_library_block_asset(block3_key, "test.txt", b"data", expect_response=403)
self._delete_library_block(block3_key, expect_response=403)
self._commit_library_changes(lib_id, expect_response=403)
Expand All @@ -659,6 +665,7 @@ def test_library_permissions(self): # pylint: disable=too-many-statements
with self.as_user(author_group_member):
olx = self._get_library_block_olx(block3_key)
self._set_library_block_olx(block3_key, olx)
self._set_library_block_fields(block3_key, {"data": olx, "metadata": {}})
# self._get_library_block_assets(block3_key)
# self._set_library_block_asset(block3_key, "test.txt", b"data")
# self._get_library_block_asset(block3_key, file_name="test.txt")
Expand Down
31 changes: 25 additions & 6 deletions openedx/core/djangoapps/xblock/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
Studio APIs cover use cases like adding/deleting/editing blocks.
"""
# pylint: disable=unused-import

from enum import Enum
from datetime import datetime
import logging
import threading
Expand Down Expand Up @@ -43,6 +43,16 @@
log = logging.getLogger(__name__)


class CheckPerm(Enum):
""" Options for the default permission check done by load_block() """
# can view the published block and call handlers etc. but not necessarily view its OLX source nor field data
CAN_LEARN = 1
# read-only studio view: can see the block (draft or published), see its OLX, see its field data, etc.
CAN_READ_AS_AUTHOR = 2
# can view everything and make changes to the block
CAN_EDIT = 3


def get_runtime_system():
"""
Return a new XBlockRuntimeSystem.
Expand Down Expand Up @@ -74,7 +84,7 @@ def get_runtime_system():
return runtime


def load_block(usage_key, user):
def load_block(usage_key, user, *, check_permission: CheckPerm | None = CheckPerm.CAN_LEARN):
"""
Load the specified XBlock for the given user.

Expand All @@ -94,10 +104,19 @@ def load_block(usage_key, user):

# Now, check if the block exists in this context and if the user has
# permission to render this XBlock view:
if user is not None and not context_impl.can_view_block(user, usage_key):
# We do not know if the block was not found or if the user doesn't have
# permission, but we want to return the same result in either case:
raise NotFound(f"XBlock {usage_key} does not exist, or you don't have permission to view it.")
if check_permission and user is not None:
if check_permission == CheckPerm.CAN_EDIT:
has_perm = context_impl.can_edit_block(user, usage_key)
elif check_permission == CheckPerm.CAN_READ_AS_AUTHOR:
has_perm = context_impl.can_view_block_for_editing(user, usage_key)
elif check_permission == CheckPerm.CAN_LEARN:
has_perm = context_impl.can_view_block(user, usage_key)
else:
has_perm = False
if not has_perm:
# We do not know if the block was not found or if the user doesn't have
# permission, but we want to return the same result in either case:
raise NotFound(f"XBlock {usage_key} does not exist, or you don't have permission to view it.")

# TODO: load field overrides from the context
# e.g. a course might specify that all 'problem' XBlocks have 'max_attempts'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ def can_edit_block(self, user, usage_key): # pylint: disable=unused-argument
"""
return False

def can_view_block_for_editing(self, user, usage_key):
"""
Does the specified usage key exist in its context, and if so, does the
specified user have permission to view its fields and OLX details (but
not necessarily to make changes to it).
"""
return self.can_edit_block(user, usage_key)

def can_view_block(self, user, usage_key): # pylint: disable=unused-argument
"""
Does the specified usage key exist in its context, and if so, does the
Expand Down
10 changes: 6 additions & 4 deletions openedx/core/djangoapps/xblock/rest_api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl
from openedx.core.lib.api.view_utils import view_auth_classes
from ..api import (
CheckPerm,
get_block_metadata,
get_block_display_name,
get_handler_url as _get_handler_url,
Expand Down Expand Up @@ -108,7 +109,7 @@ def embed_block_view(request, usage_key_str, view_name):
raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e

try:
block = load_block(usage_key, request.user)
block = load_block(usage_key, request.user, check_permission=CheckPerm.CAN_LEARN)
except NoSuchUsage as exc:
raise NotFound(f"{usage_key} not found") from exc

Expand All @@ -124,7 +125,7 @@ def embed_block_view(request, usage_key_str, view_name):
'lms_root_url': lms_root_url,
'is_development': settings.DEBUG,
}
response = render(request, 'xblock_v2/xblock_iframe.html', context, content_type='text/html')
response = render(request, 'xblock_v2/xblock_iframe.html', context, content_type='text/html', using='django')

# Only allow this iframe be embedded if the parent is in the CORS_ORIGIN_WHITELIST
cors_origin_whitelist = configuration_helpers.get_value('CORS_ORIGIN_WHITELIST', settings.CORS_ORIGIN_WHITELIST)
Expand Down Expand Up @@ -246,7 +247,8 @@ def get(self, request, usage_key_str):
except InvalidKeyError as e:
raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e

block = load_block(usage_key, request.user)
# The "fields" view requires "read as author" permissions because the fields can contain answers, etc.
block = load_block(usage_key, request.user, check_permission=CheckPerm.CAN_READ_AS_AUTHOR)
block_dict = {
"display_name": get_block_display_name(block), # potentially duplicated from metadata
"data": block.data,
Expand All @@ -265,7 +267,7 @@ def post(self, request, usage_key_str):
raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e

user = request.user
block = load_block(usage_key, user)
block = load_block(usage_key, user, check_permission=CheckPerm.CAN_EDIT)
data = request.data.get("data")
metadata = request.data.get("metadata")

Expand Down
Loading