Skip to content

Commit

Permalink
fix: global roles do not have access to views
Browse files Browse the repository at this point in the history
Scenario: the user has one role `support`. The view allows `support`
roles to access it.

Before the fix: the user is not allowed to access the view

After the fix: the user is allowed to access the view
  • Loading branch information
shadinaif committed Nov 19, 2024
1 parent 2cc9465 commit d170c25
Show file tree
Hide file tree
Showing 5 changed files with 118 additions and 29 deletions.
65 changes: 40 additions & 25 deletions futurex_openedx_extensions/helpers/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from rest_framework.exceptions import NotAuthenticated, PermissionDenied
from rest_framework.permissions import BasePermission, IsAuthenticated

from futurex_openedx_extensions.helpers import constants as cs
from futurex_openedx_extensions.helpers.roles import (
check_tenant_access,
get_accessible_tenant_ids,
Expand Down Expand Up @@ -55,6 +56,35 @@ def verify_access_roles(self, request: Any, view: Any) -> bool:
"""Verify access roles."""
raise NotImplementedError(f'(verify_access_roles) is not implemented ({self.__class__.__name__})')

@staticmethod
def _set_view_allowed_info(
request: Any, tenant_ids: List[int], user_roles: dict, view_allowed_roles: List[str]) -> None:
"""Helper method to set view allowed info."""
permitted_orgs: set = set(get_course_org_filter_list(tenant_ids)['course_org_filter_list'])
full_access_orgs: set = set()
course_access_orgs: set = set()
for role in view_allowed_roles:
if role in user_roles:
full_access_orgs.update(
permitted_orgs & set(user_roles[role]['orgs_full_access'])
)
course_access_orgs.update(
permitted_orgs & set(user_roles[role]['orgs_of_courses'])
)
course_access_orgs -= full_access_orgs

full_access_tenant_ids = set()
for org in full_access_orgs:
full_access_tenant_ids.update(get_org_to_tenant_map()[org])

request.fx_permission_info.update({
'view_allowed_full_access_orgs': list(full_access_orgs),
'view_allowed_course_access_orgs': list(course_access_orgs),
'view_allowed_any_access_orgs': list(full_access_orgs | course_access_orgs),
'view_allowed_tenant_ids_full_access': list(full_access_tenant_ids),
'view_allowed_tenant_ids_partial_access': list(set(tenant_ids) - full_access_tenant_ids),
})

def has_permission(self, request: Any, view: Any) -> bool:
"""Check if the user is authenticated."""
if not hasattr(view, 'get_allowed_roles_all_views'):
Expand Down Expand Up @@ -82,18 +112,25 @@ def has_permission(self, request: Any, view: Any) -> bool:
else:
tenant_ids = get_accessible_tenant_ids(user=request.user, roles_filter=view_allowed_roles)

system_staff_user_flag = is_system_staff_user(request.user)
user_roles: dict = get_user_course_access_roles(request.user.id)['roles']
request.fx_permission_info = {
'user': request.user,
'user_roles': user_roles,
'is_system_staff_user': is_system_staff_user(request.user),
'is_system_staff_user': system_staff_user_flag,
'view_allowed_roles': view_allowed_roles,
'view_allowed_tenant_ids_any_access': tenant_ids,
}

if is_system_staff_user(request.user):
if system_staff_user_flag:
request.fx_permission_info.update({
'user_roles': {},
})

if system_staff_user_flag or (
set(user_roles.keys()) & set(cs.COURSE_ACCESS_ROLES_GLOBAL) & set(view_allowed_roles)
):
request.fx_permission_info.update({
'view_allowed_full_access_orgs': get_course_org_filter_list(tenant_ids)['course_org_filter_list'],
'view_allowed_course_access_orgs': [],
'view_allowed_tenant_ids_full_access': tenant_ids,
Expand All @@ -103,29 +140,7 @@ def has_permission(self, request: Any, view: Any) -> bool:
request.fx_permission_info['view_allowed_full_access_orgs']
return True

permitted_orgs: set = set(get_course_org_filter_list(tenant_ids)['course_org_filter_list'])
full_access_orgs: set = set()
course_access_orgs: set = set()
for role in view_allowed_roles:
if role in user_roles:
full_access_orgs.update(
permitted_orgs & set(user_roles[role]['orgs_full_access'])
)
course_access_orgs.update(
permitted_orgs & set(user_roles[role]['orgs_of_courses'])
)
course_access_orgs -= full_access_orgs

full_access_tenant_ids = set()
for org in full_access_orgs:
full_access_tenant_ids.update(get_org_to_tenant_map()[org])
request.fx_permission_info.update({
'view_allowed_full_access_orgs': list(full_access_orgs),
'view_allowed_course_access_orgs': list(course_access_orgs),
'view_allowed_any_access_orgs': list(full_access_orgs | course_access_orgs),
'view_allowed_tenant_ids_full_access': list(full_access_tenant_ids),
'view_allowed_tenant_ids_partial_access': list(set(tenant_ids) - full_access_tenant_ids),
})
self._set_view_allowed_info(request, tenant_ids, user_roles, view_allowed_roles)

return self.verify_access_roles(request, view)

Expand Down
8 changes: 6 additions & 2 deletions futurex_openedx_extensions/helpers/roles.py
Original file line number Diff line number Diff line change
Expand Up @@ -289,20 +289,24 @@ def get_accessible_tenant_ids(user: get_user_model, roles_filter: List[str] | No
:return: List of accessible tenant IDs
:rtype: List[int]
"""
all_tenant_ids = get_all_tenant_ids()
if not user:
return []

if is_system_staff_user(user):
return get_all_tenant_ids()
return all_tenant_ids

user_roles = get_user_course_access_roles(user.id)['roles']

if roles_filter is None:
roles_filter = list(user_roles.keys())
elif not isinstance(roles_filter, list):
raise TypeError('roles_filter must be a list')
elif not roles_filter:
return []

if set(cs.COURSE_ACCESS_ROLES_GLOBAL) & set(user_roles) & set(roles_filter):
return all_tenant_ids

tenant_ids = set()
for role_name in roles_filter:
tenant_ids.update(user_roles.get(role_name, {}).get('tenant_ids', []))
Expand Down
13 changes: 13 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
from organizations.models import Organization

from futurex_openedx_extensions.helpers import constants as cs
from tests.base_test_data import _base_data
from tests.fixture_helpers import get_tenants_of_org, get_user1_fx_permission_info

Expand Down Expand Up @@ -53,6 +54,18 @@ def fx_permission_info():
}


@pytest.fixture
def support_user():
"""Fixture for support user."""
user_id = 55
assert CourseAccessRole.objects.filter(user_id=user_id).count() == 0, 'Bad test data'
user = get_user_model().objects.get(id=user_id)
assert not user.is_staff, 'staff users not allowed in this test'
assert not user.is_superuser, 'staff users not allowed in this test'
CourseAccessRole.objects.create(user_id=user_id, role=cs.COURSE_SUPPORT_ROLE_GLOBAL)
return user


@pytest.fixture
def user1_fx_permission_info():
"""Fixture for permission information for user1."""
Expand Down
48 changes: 46 additions & 2 deletions tests/test_helpers/test_permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ def test_fx_base_authenticated_permission_staff_always_allowed(
"""Verify that FXBaseAuthenticatedPermission returns True when user is staff."""
request = APIRequestFactory().generic('GET', '/dummy/')
set_user(request, user_id)
assert permission.has_permission(request, dummy_view) is True
permission.has_permission(request, dummy_view)
assert request.fx_permission_info == {
'user': request.user,
'user_roles': {},
Expand All @@ -164,6 +164,50 @@ def test_fx_base_authenticated_permission_staff_always_allowed(
}


@pytest.mark.django_db
@pytest.mark.parametrize('view_allowed_roles, allowed_tenant_ids', [
(['staff', 'no-support-in-the-list'], []),
(['support'], [1, 2, 3, 7, 8]),
])
def test_fx_base_authenticated_permission_global_role_allow_all_tenants(
base_data, dummy_view, permission, support_user, view_allowed_roles, allowed_tenant_ids,
): # pylint: disable=unused-argument, redefined-outer-name, too-many-arguments
"""Verify that FXBaseAuthenticatedPermission fills fx_permission_info correctly for global users."""
request = APIRequestFactory().generic('GET', '/dummy/')
set_user(request, support_user.id)
dummy_view.result_of_method['dummyView'] = view_allowed_roles
permission.has_permission(request, dummy_view)
if allowed_tenant_ids:
orgs = get_all_orgs()
else:
orgs = []

assert not DeepDiff(
request.fx_permission_info,
{
'user': request.user,
'user_roles': {
'support': {
'orgs_full_access': [],
'tenant_ids_full_access': [],
'course_limited_access': [],
'orgs_of_courses': [],
'tenant_ids': []
}
},
'is_system_staff_user': False,
'view_allowed_roles': view_allowed_roles,
'view_allowed_full_access_orgs': orgs,
'view_allowed_course_access_orgs': [],
'view_allowed_any_access_orgs': orgs,
'view_allowed_tenant_ids_any_access': allowed_tenant_ids,
'view_allowed_tenant_ids_full_access': allowed_tenant_ids,
'view_allowed_tenant_ids_partial_access': [],
},
ignore_order=True
)


def test_fx_base_authenticated_permission_selected_tenants(
db, dummy_view, permission
): # pylint: disable=unused-argument, redefined-outer-name
Expand All @@ -178,7 +222,7 @@ def test_fx_base_authenticated_permission_selected_tenants(

request = APIRequestFactory().generic('GET', '/dummy/?tenant_ids=1,2')
set_user(request, user_id)
assert permission.has_permission(request, dummy_view) is True
permission.has_permission(request, dummy_view)

request = APIRequestFactory().generic('GET', '/dummy/?tenant_ids=1,2,3')
set_user(request, user_id)
Expand Down
13 changes: 13 additions & 0 deletions tests/test_helpers/test_roles.py
Original file line number Diff line number Diff line change
Expand Up @@ -2041,6 +2041,19 @@ def test_get_accessible_tenant_ids_staff(base_data, user_id, expected): # pylin
assert result == expected


@pytest.mark.django_db
@pytest.mark.parametrize('roles_filter, expected_result, assertion_msg', [
(None, [1, 2, 3, 7, 8], 'when roles_filter is None, global role users must have access to all tenants'),
(cs.COURSE_ACCESS_ROLES_TENANT_OR_COURSE, [], 'access should be denied when the global role is\'t in roles_filter'),
])
def test_get_accessible_tenant_ids_global_role(
base_data, support_user, roles_filter, expected_result, assertion_msg,
): # pylint: disable=unused-argument
"""Verify get_accessible_tenant_ids function for global roles users."""
result = get_accessible_tenant_ids(support_user, roles_filter=roles_filter)
assert result == expected_result, assertion_msg


@pytest.mark.django_db
@pytest.mark.parametrize('user_id, expected', [
(3, [1]),
Expand Down

0 comments on commit d170c25

Please sign in to comment.