Skip to content

Commit

Permalink
fix: use org-level permissions when generating search filter
Browse files Browse the repository at this point in the history
Also refactors tests to demonstrate this change for OrgStaff and
OrgInstructor users.
  • Loading branch information
pomegranited committed Apr 9, 2024
1 parent 6a3c30a commit c7215ac
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 51 deletions.
62 changes: 53 additions & 9 deletions openedx/core/djangoapps/content/search/tests/test_models.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
"""Content search model tests"""
from __future__ import annotations

import ddt
from django.test import RequestFactory
from django.utils.crypto import get_random_string
from organizations.models import Organization

from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole, GlobalStaff
from common.djangoapps.student.auth import update_org_role
from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole, OrgInstructorRole, OrgStaffRole
from common.djangoapps.student.tests.factories import UserFactory
from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory
from openedx.core.djangoapps.content_libraries import api as library_api
Expand All @@ -19,23 +23,38 @@
get_access_ids_for_request = lambda request: []


@skip_unless_cms
class StudioSearchAccessTest(SharedModuleStoreTestCase):
class StudioSearchTestMixin:
"""
Tests the SearchAccess model, handlers, and helper functions.
Sets up user, org, course, library, and access for studio search tests.
"""
@classmethod
def setUpClass(cls):
super().setUpClass()
cls.global_staff = UserFactory(
username='staff', email='[email protected]', is_staff=True, password='staff_pass'
)
cls.student = UserFactory.create(
username='student', email='[email protected]', is_staff=False, password='student_pass'
)
cls.course_staff = UserFactory.create(
username='course_staff', email='[email protected]', is_staff=False, password='course_staff_pass'
)
cls.course_instructor = UserFactory.create(
username='course_instr', email='[email protected]', is_staff=False, password='course_instr_pass'
)
cls.org_staff = UserFactory.create(
username='org_staff', email='[email protected]', is_staff=False, password='org_staff_pass'
)
cls.org_instructor = UserFactory.create(
username='org_instr', email='[email protected]', is_staff=False, password='org_instr_pass'
)

def setUp(self):
"""
Add users, orgs, courses, and libraries.
"""
super().setUp()
self.global_staff = UserFactory(password=self.TEST_PASSWORD)
GlobalStaff().add_users(self.global_staff)

self.course_staff = UserFactory(password=self.TEST_PASSWORD)
self.course_instructor = UserFactory(password=self.TEST_PASSWORD)
self.student = UserFactory(password=self.TEST_PASSWORD)
self.course_user_keys = []
self.staff_user_keys = []

Expand All @@ -61,6 +80,8 @@ def setUp(self):
short_name='org2',
defaults={'name': "Org Two"},
)
update_org_role(caller=self.global_staff, role=OrgStaffRole, user=self.org_staff, orgs=['org1'])
update_org_role(caller=self.global_staff, role=OrgInstructorRole, user=self.org_instructor, orgs=['org1'])

# Create a few libraries that global_staff, course_staff and course_instructor can access
for num in range(2):
Expand Down Expand Up @@ -107,6 +128,14 @@ def _create_library(self, org, num):
)
return library


@ddt.ddt
@skip_unless_cms
class StudioSearchAccessTest(StudioSearchTestMixin, SharedModuleStoreTestCase):
"""
Tests the SearchAccess model, handlers, and helper functions.
"""

def _check_access_ids(self, access_ids, expected_keys):
"""
Checks the returned list of access_ids to ensure:
Expand Down Expand Up @@ -142,6 +171,21 @@ def test_course_instructor_get_access_ids_for_request(self):
access_ids = get_access_ids_for_request(request)
self._check_access_ids(access_ids, self.course_user_keys)

@ddt.data(
'org_staff',
'org_instructor',
)
def test_org_get_access_ids_for_request(self, user_attr):
"""
Org staff & instructors can see all courses and libraries in their org.
But if they don't have any individual access granted, then no access_ids will be returned.
"""
request = RequestFactory().get('/course')
request.user = getattr(self, user_attr)

access_ids = get_access_ids_for_request(request)
self._check_access_ids(access_ids, [])

def test_staff_get_access_ids_for_request(self):
"""
Global staff can see all courses and libraries, but they only have individual access granted for libraries.
Expand Down
98 changes: 62 additions & 36 deletions openedx/core/djangoapps/content/search/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,23 @@
Tests for the Studio content search REST API.
"""
import functools
from django.test import override_settings
from rest_framework.test import APITestCase, APIClient
from unittest import mock

from organizations.models import Organization
from common.djangoapps.student.tests.factories import UserFactory
import ddt
from django.test import override_settings
from rest_framework.test import APIClient

from openedx.core.djangolib.testing.utils import skip_unless_cms
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase

from .test_models import StudioSearchTestMixin

try:
# This import errors in the lms because content.search is not an installed app there.
from openedx.core.djangoapps.content.search.models import SearchAccess
except RuntimeError:
SearchAccess = {}


STUDIO_SEARCH_ENDPOINT_URL = "/api/content_search/v2/studio/"
MOCK_API_KEY_UID = "3203d764-370f-4e99-a917-d47ab7f29739"
Expand Down Expand Up @@ -37,22 +47,12 @@ def wrapper(*args, **kwargs):
return decorator


@ddt.ddt
@skip_unless_cms
class StudioSearchViewTest(APITestCase):
class StudioSearchViewTest(StudioSearchTestMixin, SharedModuleStoreTestCase):
"""
General tests for the Studio search REST API.
"""

@classmethod
def setUpClass(cls):
super().setUpClass()
cls.staff = UserFactory.create(
username='staff', email='[email protected]', is_staff=True, password='staff_pass'
)
cls.student = UserFactory.create(
username='student', email='[email protected]', is_staff=False, password='student_pass'
)

def setUp(self):
super().setUp()
self.client = APIClient()
Expand Down Expand Up @@ -144,21 +144,21 @@ def test_studio_search_staff(self, mock_search_client):
)

@mock_meilisearch(enabled=True)
@mock.patch('openedx.core.djangoapps.content.search.views.get_access_ids_for_request')
@mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client')
def test_studio_search_limit_access_ids(self, mock_search_client, mock_get_access_ids):
def test_studio_search_course_staff_access(self, mock_search_client):
"""
Users with access to many courses or libraries will only be able to search content
from the most recent 1_000 courses/libraries.
Users with staff or instructor access to a course or library will be limited to these courses/libraries.
"""
self.client.login(username='student', password='student_pass')
self.client.login(username='course_staff', password='course_staff_pass')
mock_generate_tenant_token = self._mock_generate_tenant_token(mock_search_client)
mock_get_access_ids.return_value = list(range(2000))
expected_access_ids = list(range(1000))

result = self.client.get(STUDIO_SEARCH_ENDPOINT_URL)
assert result.status_code == 200
mock_get_access_ids.assert_called_once()

expected_access_ids = list(SearchAccess.objects.filter(
context_key__in=self.course_user_keys,
).only('id').values_list('id', flat=True))
expected_access_ids.sort(reverse=True)

mock_generate_tenant_token.assert_called_once_with(
api_key_uid=MOCK_API_KEY_UID,
search_rules={
Expand All @@ -169,34 +169,60 @@ def test_studio_search_limit_access_ids(self, mock_search_client, mock_get_acces
expires_at=mock.ANY,
)

@ddt.data(
'org_staff',
'org_instr',
)
@mock_meilisearch(enabled=True)
@mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client')
def test_studio_search_org_access(self, username, mock_search_client):
"""
Users with org access to any courses or libraries will use the org filter.
"""
self.client.login(username=username, password=f'{username}_pass')
mock_generate_tenant_token = self._mock_generate_tenant_token(mock_search_client)
result = self.client.get(STUDIO_SEARCH_ENDPOINT_URL)
assert result.status_code == 200
mock_generate_tenant_token.assert_called_once_with(
api_key_uid=MOCK_API_KEY_UID,
search_rules={
"studio_content": {
"filter": "org IN ['org1'] OR access_id IN []",
}
},
expires_at=mock.ANY,
)

@mock_meilisearch(enabled=True)
@mock.patch('openedx.core.djangoapps.content.search.views.get_user_orgs')
@mock.patch('openedx.core.djangoapps.content.search.views._get_user_orgs')
@mock.patch('openedx.core.djangoapps.content.search.views.get_access_ids_for_request')
@mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client')
def test_studio_search_limit_orgs(self, mock_search_client, mock_get_user_orgs):
def test_studio_search_limits(self, mock_search_client, mock_get_access_ids, mock_get_user_orgs):
"""
Users with access to many courses or libraries will only be able to search content
from the most recent 1_000 courses/libraries.
Users with access to many courses/libraries or orgs will only be able to search content
from the most recent 1_000 courses/libraries and orgs.
"""
self.client.login(username='student', password='student_pass')
mock_generate_tenant_token = self._mock_generate_tenant_token(mock_search_client)

mock_get_access_ids.return_value = list(range(2000))
expected_access_ids = list(range(1000))

mock_get_user_orgs.return_value = [
Organization.objects.create(
short_name=f"org{x}",
description=f"Org {x}",
) for x in range(2000)
f"studio-search-org{x}" for x in range(2000)
]
expected_user_orgs = [
f"org{x}" for x in range(1000)
f"studio-search-org{x}" for x in range(1000)
]

result = self.client.get(STUDIO_SEARCH_ENDPOINT_URL)
assert result.status_code == 200
mock_get_user_orgs.assert_called_once()
mock_get_access_ids.assert_called_once()
mock_generate_tenant_token.assert_called_once_with(
api_key_uid=MOCK_API_KEY_UID,
search_rules={
"studio_content": {
"filter": f"org IN {expected_user_orgs} OR access_id IN []",
"filter": f"org IN {expected_user_orgs} OR access_id IN {expected_access_ids}",
}
},
expires_at=mock.ANY,
Expand Down
25 changes: 19 additions & 6 deletions openedx/core/djangoapps/content/search/views.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
"""
REST API for content search
"""
from __future__ import annotations

from datetime import datetime, timedelta, timezone
import logging

Expand All @@ -12,12 +14,11 @@
from rest_framework.response import Response
from rest_framework.views import APIView

from common.djangoapps.student.role_helpers import get_course_roles
from common.djangoapps.student.roles import GlobalStaff
from openedx.core.lib.api.view_utils import view_auth_classes
from openedx.core.djangoapps.content.search.documents import STUDIO_INDEX_NAME
from openedx.core.djangoapps.content.search.models import get_access_ids_for_request
from openedx.core.djangoapps.content_tagging.rules import get_user_orgs


User = get_user_model()
log = logging.getLogger(__name__)
Expand All @@ -43,10 +44,8 @@ def _get_meili_access_filter(request: Request) -> dict:
if GlobalStaff().has_user(request.user):
return {}

# Everyone else is limited to their org roles...
user_orgs = [
org.short_name for org in get_user_orgs(request.user)[:MAX_ORGS_IN_FILTER]
]
# Everyone else is limited to their org staff roles...
user_orgs = _get_user_orgs(request)[:MAX_ORGS_IN_FILTER]

# ...or the N most recent courses and libraries they can access.
access_ids = get_access_ids_for_request(request)[:MAX_ACCESS_IDS_IN_FILTER]
Expand All @@ -55,6 +54,20 @@ def _get_meili_access_filter(request: Request) -> dict:
}


def _get_user_orgs(request: Request) -> list[str]:
"""
Get the org.short_names for the organizations that the requesting user has OrgStaffRole or OrgInstructorRole.
Note: org-level roles have course_id=None to distinguish them from course-level roles.
"""
course_roles = get_course_roles(request.user)
return list(set(
role.org
for role in course_roles
if role.course_id is None and role.role in ['staff', 'instructor']
))


@view_auth_classes(is_authenticated=True)
class StudioSearchView(APIView):
"""
Expand Down

0 comments on commit c7215ac

Please sign in to comment.