Bug: As an admin or superuser, I can't manage access for a library

[mariajgrimaldi]

Description

Actual behavior

When a Django admin (or superuser) tries to access the Admin Console for a Content Library they didn’t create, a 403 Forbidden error is shown. This happens even though a Django superuser should, by definition, have access to manage roles and permissions for any library.

Expected behavior

Superusers (and staff, if applicable) should always have access to manage roles and permissions for any library.

Screenshot:

Steps to Reproduce

1. Log in as a Django administrator (superuser).
2. Go to any Content Library you didn’t create.
3. Click on Manage Access.
4. You’ll see a 403 Forbidden message

Possible Approach

In the short term, ensure all authorization endpoints use the same base permission class that checks whether a user is staff or superuser, and grant access accordingly.

In the long term, this logic should be centralized, not repeated per API:

• Possibly integrated with a core access control mechanism like BridgeKeeper.
• Or handled through a custom Casbin matcher that evaluates staff/superuser status transparently (though this approach may reduce explicitness in permission checks).

[BryanttV]

Hi @mariajgrimaldi

This is an issue that was planned to be handled through the custom matcher implemented in this Draft PR: #114. The matcher looks something like this (for now):

def check_custom_conditions(request_user: str, request_action: str, request_scope: str) -> bool:
    """
    Evaluates custom, non-role-based conditions for library actions.

    Checks attribute-based conditions that don't rely on role assignments:
    - Staff and superusers have full access
    - create_library: requires granted course creator status
    - view_library: allowed if library has public read enabled

    Args:
        request_user (str): Namespaced user key
        request_action (str): Namespaced action key
        request_scope (str): Namespaced scope key

    Returns:
        bool: True if the condition is satisfied, False otherwise
    """
    try:
        username = UserData(namespaced_key=request_user).external_key
        user = get_user_by_username_or_email(username)
    except User.DoesNotExist:
        return False

    scope = ScopeData(namespaced_key=request_scope)
    scope_obj = scope.get_object()

    if scope_obj is None:
        return False

    if user.is_staff or user.is_superuser:
        return True

    action = ActionData(namespaced_key=request_action)

    if isinstance(scope, ContentLibraryData):
        if is_studio_request():
            if action.external_key == "create_library":
                return is_course_creator(user)
            if action.external_key == "view_library":
                return scope_obj.allow_public_read and is_course_creator(user)

    return False

In the matcher, it checks whether the user is staff or superuser; in that case, the user will always have guaranteed permissions, regardless of the action (permission) or scope being validated.

Why not do it with a Permission class?

In fact, what’s being validated is not whether the user has access to the permissions/validate/me endpoint, since any authenticated user can access it. Instead, based on the received body, the system checks whether the user has permissions according to the action and scope, for example:

Body

[
    {
        "action": "view_library_team",
        "scope": "lib:OpenedX:CSPROB"
    },
    {
        "action": "manage_library_team",
        "scope": "lib:OpenedX:CSPROB"
    }
]

Since these are two independent validations, an independent response is returned.

Response

[
    {
        "action": "view_library_team",
        "scope": "lib:OpenedX:CSPROB",
        "allowed": true
    },
    {
        "action": "manage_library_team",
        "scope": "lib:OpenedX:CSPROB",
        "allowed": true
    }
]

That’s why the custom matcher is used, so that when is_user_allowed is called, each one is validated, and an appropriate response is returned. In this case, if the user is an admin or staff, the enforce call will always return True.

[MaferMazu]

This bug won't happen in the rest of edx-platform calls because the old permission system will handle it. Still, regarding the team permissions endpoints, we will have a problem unless we apply the attribute checks, as @BryanttV mentioned. Also, this will ensure our system handles "contextual" checks that are part of the current permissions.

The option I see is:

• Continue the work with the custom attribute checks.
  Take into account:
• Be careful of the implications of modifying the matcher (for the engine and migration things).
  Other notes about this:
  The team rest_api endpoints only take into account the new system (so it will be mandatory to migrate data from the old to the new system)

Option I would discard:

• Use the old check in the rest api.
  Take into account:
• We won't need the attribute check, but we rely on the old permission system.
• People with old permissions still have access to the console withouth migrating.

[mariajgrimaldi]

Thank you both for the context!

About validate/me, I was thinking about this and I don’t think we should call the matcher for something handled at the Django level. We already have access to the user in memory, and we know if they’re staff or a superuser. So it doesn’t seem useful for those users to go through individual permission checks when we already know the result will be true.

BUT what makes sense is that the matcher will help when calling is_user_allowed, so the more maintainable option could be to keep the additional matcher. However, I think we can avoid hitting the database by adding more attributes to UserData, like whether the user is staff or a superuser.

I’m fine with the matcher as long as it stays simple and only checks for those two cases for the MVP. Then, we can revisit how to sustainably support new matchers and so on.