Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -1686,3 +1686,63 @@ def test_delete_component_should_be_ready_to_sync(self):
}

self.assertDictEqual(data[0], expected_results) # noqa: PT009


class PostDownstreamSyncAuthzViewTest(
CourseAuthoringAuthzTestMixin,
_BaseDownstreamViewTestMixin,
ImmediateOnCommitMixin,
SharedModuleStoreTestCase,
):
"""
AuthZ tests for:
POST /api/contentstore/v2/downstreams/{usage_key}/sync

Verifies that a user with the ``course_staff`` authz role (which includes
``courses.manage_library_updates``) can sync a downstream container even
when the user has **no** permissions on the source library.
"""

def call_api(self, usage_key_string):
return self.authorized_client.post(
f"/api/contentstore/v2/downstreams/{usage_key_string}/sync"
)

def test_course_staff_can_sync_container_without_library_access(self):
"""
A user with Course Staff role (which carries
``courses.manage_library_updates``) should be able to sync a
downstream container from its upstream library, even when the user
has no explicit permissions on the library.
"""
# Give the user Course Staff in authz so they get manage_library_updates
from openedx_authz.constants.roles import COURSE_STAFF
self.add_user_to_role_in_course(
self.authorized_user,
COURSE_STAFF.external_key,
self.course.id,
)

# Also give legacy CourseStaffRole so _load_accessible_block passes
add_users(self.superuser, CourseStaffRole(self.course.id), self.authorized_user)

# Confirm the user has NO explicit permissions on the library.
assert lib_api.get_library_user_permissions(
self.library_key, self.authorized_user,
) is None

# The downstream_unit_key is linked to a container upstream in self.library.
# The unit was updated (display_name changed + republished) in setUp,
# so it is ready to sync.
response = self.call_api(self.downstream_unit_key)

assert response.status_code == 200, (
f"Expected 200 but got {response.status_code}: {getattr(response, 'data', '')}"
)

# Same test but for a block sync instead of a container one
response = self.call_api(self.downstream_html_key)

assert response.status_code == 200, (
f"Expected 200 but got {response.status_code}: {getattr(response, 'data', '')}"
)
26 changes: 26 additions & 0 deletions cms/lib/xblock/upstream_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,32 @@
store.update_item(downstream, user_id)


def user_has_manage_library_updates(user: User, course_key: CourseKey | None) -> bool:
"""
Return True if *course_key* is provided and *user* holds the
``courses.manage_library_updates`` permission for that course.

This is intentionally a thin wrapper so that both
``upstream_sync_container`` and ``upstream_sync_block`` can share the
same check without duplicating authz imports.
"""
if course_key is None:
return False

from openedx.core.djangoapps.authz.decorators import ( # pylint: disable=wrong-import-order

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there any reason why we are importing this here instead of at the top of the file?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, putting those imports at the top of the file will cause a crash

Traceback (most recent call last):  
  File "/openedx/edx-platform/./manage.py", line 93, in <module>  
    execute_from_command_line([sys.argv[0]] + django_args)  
  File "/openedx/venv/lib/python3.12/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line  
    utility.execute()  
  File "/openedx/venv/lib/python3.12/site-packages/django/core/management/__init__.py", line 382, in execute  
    settings.INSTALLED_APPS  
  File "/openedx/venv/lib/python3.12/site-packages/django/conf/__init__.py", line 81, in __getattr__  
    self._setup(name)  
  File "/openedx/venv/lib/python3.12/site-packages/django/conf/__init__.py", line 68, in _setup  
    self._wrapped = Settings(settings_module)  
                    ^^^^^^^^^^^^^^^^^^^^^^^^^  
  File "/openedx/venv/lib/python3.12/site-packages/django/conf/__init__.py", line 166, in __init__  
    mod = importlib.import_module(self.SETTINGS_MODULE)  
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  
  File "/opt/pyenv/versions/3.12.13/lib/python3.12/importlib/__init__.py", line 90, in import_module  
    return _bootstrap._gcd_import(name[level:], package, level)  
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import  
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load  
  File "<frozen importlib._bootstrap>", line 1331, in _find_and_load_unlocked  
  File "<frozen importlib._bootstrap>", line 935, in _load_unlocked  
  File "<frozen importlib._bootstrap_external>", line 999, in exec_module  
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed  
  File "/openedx/edx-platform/cms/envs/tutor/development.py", line 3, in <module>  
    from cms.envs.devstack import *  
  File "/openedx/edx-platform/cms/envs/devstack.py", line 12, in <module>  
    from .production import *  # pylint: disable=wildcard-import, unused-wildcard-import  # noqa: F403  
    ^^^^^^^^^^^^^^^^^^^^^^^^^  
  File "/openedx/edx-platform/cms/envs/production.py", line 32, in <module>  
    from .common import *  # noqa: F403  
    ^^^^^^^^^^^^^^^^^^^^^  
  File "/openedx/edx-platform/cms/envs/common.py", line 51, in <module>  
    from cms.lib.xblock.upstream_sync import UpstreamSyncMixin  
  File "/openedx/edx-platform/cms/lib/xblock/upstream_sync.py", line 31, in <module>  
    from openedx.core.djangoapps.authz.decorators import (  
  File "/openedx/edx-platform/openedx/core/djangoapps/authz/decorators.py", line 6, in <module>  
    from django.contrib.auth.models import AbstractUser  
  File "/openedx/venv/lib/python3.12/site-packages/django/contrib/auth/models.py", line 5, in <module>  
    from django.contrib.auth.base_user import AbstractBaseUser, BaseUserManager  
  File "/openedx/venv/lib/python3.12/site-packages/django/contrib/auth/base_user.py", line 43, in <module>  
    class AbstractBaseUser(models.Model):  
  File "/openedx/venv/lib/python3.12/site-packages/django/db/models/base.py", line 131, in __new__  
    app_config = apps.get_containing_app_config(module)  
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  
  File "/openedx/venv/lib/python3.12/site-packages/django/apps/registry.py", line 260, in get_containing_app_config  
    self.check_apps_ready()  
  File "/openedx/venv/lib/python3.12/site-packages/django/apps/registry.py", line 138, in check_apps_ready  
    raise AppRegistryNotReady("Apps aren't loaded yet.")  
django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet.  

Alternatively I am thinking I can put that helper function, user_has_manage_library_updates in its own file or another file instead of upstream_sync.py

Any thoughts?

@carlos-marquez-wgu carlos-marquez-wgu Sep 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

After checking #39009 from @wgu-taylor-payne , I think it might make more sense to use a single file and share it in both solutions as we are doing the same validations

... Or just use user_has_course_permission directly instead of relying on the thin wrapper, any thoughts?

LegacyAuthoringPermission,
user_has_course_permission,
)
from openedx_authz.constants.permissions import COURSES_MANAGE_LIBRARY_UPDATES # pylint: disable=wrong-import-order

Check failure on line 378 in cms/lib/xblock/upstream_sync.py

View workflow job for this annotation

GitHub Actions / Quality Others (ubuntu-24.04, 3.12, 20)

ruff (I001)

cms/lib/xblock/upstream_sync.py:374:5: I001 Import block is un-sorted or un-formatted help: Organize imports

return user_has_course_permission(
user,
COURSES_MANAGE_LIBRARY_UPDATES.identifier,
course_key,
LegacyAuthoringPermission.WRITE,
)


def _update_children_top_level_parent(
downstream: XBlock,
new_top_level_parent_key: str | None,
Expand Down
15 changes: 13 additions & 2 deletions cms/lib/xblock/upstream_sync_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from xblock.core import XBlock
from xblock.fields import Scope

from .upstream_sync import BadDownstream, BadUpstream, UpstreamLink
from .upstream_sync import BadDownstream, BadUpstream, UpstreamLink, user_has_manage_library_updates

if t.TYPE_CHECKING:
from django.contrib.auth.models import User # pylint: disable=imported-auth-user
Expand Down Expand Up @@ -94,18 +94,29 @@ def _load_upstream_block(downstream: XBlock, user: User) -> XBlock:
library. This assumption may need to be relaxed in the future (see module docstring).

If `downstream` lacks a valid+supported upstream link, this raises an UpstreamLinkException.

If the user holds ``courses.manage_library_updates`` for the course that
owns ``downstream``, the library-level permission check is bypassed.
Otherwise the default ``CAN_READ_AS_AUTHOR`` check is applied.
"""
# We import load_block here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready.
from openedx.core.djangoapps.xblock.api import ( # pylint: disable=wrong-import-order
CheckPerm,
LatestVersion,
load_block,
)

# Try course-level permission first; fall back to library-level check.
if user_has_manage_library_updates(user, downstream.usage_key.context_key):
check_perm = None
else:
check_perm = CheckPerm.CAN_READ_AS_AUTHOR

try:
lib_block: XBlock = load_block(
LibraryUsageLocatorV2.from_string(downstream.upstream),
user,
check_permission=CheckPerm.CAN_READ_AS_AUTHOR,
check_permission=check_perm,
version=LatestVersion.PUBLISHED,
)
except (NotFound, PermissionDenied) as exc:
Expand Down
19 changes: 13 additions & 6 deletions cms/lib/xblock/upstream_sync_container.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

from openedx.core.djangoapps.content_libraries import api as lib_api

from .upstream_sync import UpstreamLink
from .upstream_sync import UpstreamLink, user_has_manage_library_updates

if t.TYPE_CHECKING:
from django.contrib.auth.models import User # pylint: disable=imported-auth-user
Expand All @@ -37,15 +37,22 @@ def sync_from_upstream_container(

Should children be handled in here? Maybe if sync_from_upstream_block
were updated to handle static assets and also save changes to modulestore.

The library-level permission check is skipped when the user holds
``courses.manage_library_updates`` for ``downstream``'s course (derived
from ``downstream.usage_key.context_key``).
"""
link = UpstreamLink.get_for_block(downstream) # can raise UpstreamLinkException
if not isinstance(link.upstream_key, LibraryContainerLocator):
raise TypeError("sync_from_upstream_container() only supports Container upstreams, not containers")
lib_api.require_permission_for_library_key( # TODO: should permissions be checked at this low level?
link.upstream_key.lib_key,
user,
permission=lib_api.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY,
)

# Try course-level permission first; fall back to library-level check.
if not user_has_manage_library_updates(user, downstream.usage_key.context_key):
lib_api.require_permission_for_library_key(
link.upstream_key.lib_key,
user,
permission=lib_api.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY,
)
upstream_meta = lib_api.get_container(link.upstream_key)
upstream_children = lib_api.get_container_children(link.upstream_key, published=True)
_update_customizable_fields(upstream=upstream_meta, downstream=downstream, only_fetch=False)
Expand Down
Loading