Skip to content

Remove the unused Content Libraries v2 group-grant path #545

Open

Description

@rodmgwgu

ContentLibraryPermission in edx-platform can grant library access to a Django
auth.Group as well as to a user. The group half of that model has no working
write path, no test coverage, and no equivalent concept in openedx-authz. It
should be removed ahead of the broader legacy-permissions removal tracked in
#325.

This is deliberately scoped to the group half only. The user half of
ContentLibraryPermission is still a live fallback in the bridgekeeper rules
(has_explicit_*_permission_for_library), so it cannot be removed until authz
enforcement is complete — that is #325's job. The group half has no such
constraint and is unblocked today.

Why it is unused

The write path does not work.

  • LibraryTeamGroupView is routed at team/group/<str:group_name>/
    (content_libraries/urls.py:48) and defined at
    content_libraries/rest_api/libraries.py:453.
  • PUT works but is explicitly deprecated in favour of authz's
    RoleUserAPIView (rest_api/libraries.py:466).
  • DELETE has never worked. The route supplies group_name, but the method
    signature is delete(self, request, lib_key_str, username)
    (rest_api/libraries.py:479), so dispatch raises
    TypeError: delete() got an unexpected keyword argument 'group_name'. Even if
    it got that far, get_object_or_404(Group, username=username)
    (rest_api/libraries.py:493) queries a field Group does not have, which
    raises FieldError, not a 404.
  • set_library_group_permissions (content_libraries/api/libraries.py:602,
    exported in __all__ at :102) emits a DeprecationWarning pointing at
    assign_library_role_to_user.

Nothing exercises it. A grep of edx-platform for LibraryTeamGroupView,
set_library_group_permissions and team/group returns only the three
implementation files — no test, no internal caller. (Someone should confirm
frontend-app-authoring does not call the endpoint either.)

The read path is live but never satisfiable. The group branch is still
evaluated on every library permission check:
(Attribute('user', ...) | Relation('group', in_current_groups)) at
content_libraries/permissions.py:27, :35 and :45, consumed by
perms[...] at :235, :258, :264, :274 and :280. With no working
writer, that OR-branch can only ever match rows created before the endpoint
broke, or created directly in the ORM/admin.

Why this issue lives in openedx-authz

There is openedx-authz code to change, and it has to change before the
platform drops the field.

migrate_legacy_permissions (src/openedx_authz/engine/utils.py:142) reads
group grants and flattens them into per-user role assignments:

legacy_permissions = ContentLibraryPermission.objects.select_related(
    "library", "library__org", "user", "group"
).all()
...
if permission.group:
    users = [user.username for user in permission.group.user_set.all()]
    batch_assign_role_to_users_in_scope(...)

It runs from the data migration 0006_migrate_legacy_permissions.py, whose
dependencies list contains only ("openedx_authz", "0005_policycachecontrol")
— no content_libraries dependency. So the historical model that
apps.get_model("content_libraries", "ContentLibraryPermission") returns can be
a post-removal state, at which point select_related("group") raises
FieldError and the migration fails. _log_migration_errors in the same file
also dereferences permission.group.name.

Data impact

Existing group grants are already preserved by migration 0006 as individual
per-user role assignments. That flattening is a one-time snapshot — later changes
to group membership were never propagated to authz — so removing the concept
loses nothing that authz tracked.

Operators should still count before assuming zero, since PUT did work:

from openedx.core.djangoapps.content_libraries.models import ContentLibraryPermission
ContentLibraryPermission.objects.filter(group__isnull=False).count()

Acceptance criteria

openedx-authz, first:

  • Make migrate_legacy_permissions tolerate a ContentLibraryPermission
    model with no group field (drop it from select_related, guard the
    branch on field introspection rather than if permission.group).
  • Same for _log_migration_errors in
    migrations/0006_migrate_legacy_permissions.py.
  • Decide whether to pin a content_libraries dependency on migration 0006
    or keep it defensive. Defensive is probably right, since the app is
    optional (LookupError is already handled).
  • Drop group from the test stub (src/openedx_authz/tests/stubs/models.py:99)
    and retire the group fixtures in src/openedx_authz/tests/test_migrations.py,
    keeping a regression test that the migration runs against a group-less model.

edx-platform, after that (coordinate with #325):

  • Remove LibraryTeamGroupView and its route.
  • Remove set_library_group_permissions and its __all__ entry.
  • Remove the Relation('group', in_current_groups) branch from the three
    bridgekeeper helper rules and the now-unused in_current_groups import.
  • Remove ContentLibraryPermission.group, the ('library', 'group')
    unique_together, the group__name ordering, the user/group XOR check in
    save(), and the group branch in __str__; add the schema migration.
  • Decide the fate of group_name in ContentLibraryPermissionSerializer
    (rest_api/serializers.py:116) — dropping it is a response-shape change
    on GET team/, so it may be worth keeping it as a hardcoded null for a
    deprecation window.

Related

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions