From 670c04ad95f2ea54f69ff92de1c135b11e88bd45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 17 Apr 2024 17:20:11 -0300 Subject: [PATCH] fix: fix code and tests after merge --- openedx/core/djangoapps/content/search/api.py | 57 ++++++++++++-- .../content/search/tests/test_api.py | 15 +++- .../content/search/tests/test_handlers.py | 19 ++++- .../content/search/tests/test_views.py | 76 ++++++++----------- .../core/djangoapps/content/search/views.py | 1 + 5 files changed, 114 insertions(+), 54 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 3e1e12dcdfc8..fb76148bf65b 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -18,6 +18,10 @@ from meilisearch.models.task import TaskInfo from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import LibraryLocatorV2 +from common.djangoapps.student.roles import GlobalStaff +from rest_framework.request import Request +from common.djangoapps.student.role_helpers import get_course_roles +from openedx.core.djangoapps.content.search.models import get_access_ids_for_request from openedx.core.djangoapps.content_libraries import api as lib_api from xmodule.modulestore import ModuleStoreEnum @@ -48,6 +52,9 @@ LOCK_EXPIRE = 24 * 60 * 60 # Lock expires in 24 hours +MAX_ACCESS_IDS_IN_FILTER = 1_000 +MAX_ORGS_IN_FILTER = 1_000 + @contextmanager def _index_rebuild_lock() -> Generator[str, None, None]: @@ -310,6 +317,15 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.org, Fields.tags, Fields.type, + Fields.access_id, + ]) + # Mark which attributes are used for keyword search, in order of importance: + client.index(temp_index_name).update_searchable_attributes([ + Fields.display_name, + Fields.block_id, + Fields.content, + Fields.tags, + # Keyword search does _not_ search the course name, course ID, breadcrumbs, block type, or other fields. ]) ############## Libraries ############## @@ -435,17 +451,46 @@ def upsert_content_library_index_docs(library_key: LibraryLocatorV2) -> None: _update_index_docs(docs) -def generate_user_token_for_studio_search(user): +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'] + )) + + +def _get_meili_access_filter(request: Request) -> dict: + """ + Return meilisearch filter based on the requesting user's permissions. + """ + # Global staff can see anything, so no filters required. + if GlobalStaff().has_user(request.user): + return {} + + # 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, omit_orgs=user_orgs)[:MAX_ACCESS_IDS_IN_FILTER] + return { + "filter": f"org IN {user_orgs} OR access_id IN {access_ids}", + } + + +def generate_user_token_for_studio_search(request): """ Returns a Meilisearch API key that only allows the user to search content that they have permission to view """ expires_at = datetime.now(tz=timezone.utc) + timedelta(days=7) + search_rules = { - STUDIO_INDEX_NAME: { - # TODO: Apply filters here based on the user's permissions, so they can only search for content - # that they have permission to view. Example: - # 'filter': 'org = BradenX' - } + STUDIO_INDEX_NAME: _get_meili_access_filter(request), } # Note: the following is just generating a JWT. It doesn't actually make an API call to Meilisearch. restricted_api_key = _get_meilisearch_client().generate_tenant_token( diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index aa425c06e7c6..01929c049a67 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -16,6 +16,12 @@ from .. import api +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/" @@ -52,6 +58,7 @@ def setUp(self): self.user_id, fields={"display_name": "Test Course"}, ) + course_access, _ = SearchAccess.objects.get_or_create(context_key=self.course.id) # Create XBlocks self.sequential = self.store.create_child(self.user_id, self.course.location, "sequential", "test_sequential") @@ -65,7 +72,8 @@ def setUp(self): 'context_key': 'course-v1:org1+test_course+test_run', 'org': 'org1', 'breadcrumbs': [{'display_name': 'Test Course'}], - 'content': {} + 'content': {}, + 'access_id': course_access.id, } self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical") self.doc_vertical = { @@ -81,7 +89,8 @@ def setUp(self): {'display_name': 'Test Course'}, {'display_name': 'sequential'} ], - 'content': {} + 'content': {}, + 'access_id': course_access.id, } # Create a content library: @@ -91,6 +100,7 @@ def setUp(self): slug="lib", title="Library", ) + lib_access, _ = SearchAccess.objects.get_or_create(context_key=self.library.key) # Populate it with a problem: self.problem = library_api.create_library_block(self.library.key, "problem", "p1") self.doc_problem = { @@ -104,6 +114,7 @@ def setUp(self): "breadcrumbs": [{"display_name": "Library"}], "content": {"problem_types": [], "capa_content": " "}, "type": "library_block", + "access_id": lib_access.id, } @override_settings(MEILISEARCH_ENABLED=False) diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py index d6ccdd5a343f..22beb6551f1f 100644 --- a/openedx/core/djangoapps/content/search/tests/test_handlers.py +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -14,6 +14,13 @@ from .. import api +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 = {} + + @patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", new=MagicMock(return_value=None)) @patch("openedx.core.djangoapps.content.search.api.MeilisearchClient") @@ -53,6 +60,7 @@ def test_create_delete_xblock(self, meilisearch_client): self.user_id, fields={"display_name": "Test Course"}, ) + course_access, _ = SearchAccess.objects.get_or_create(context_key=course.id) # Create XBlocks sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") @@ -65,7 +73,9 @@ def test_create_delete_xblock(self, meilisearch_client): "block_type": "sequential", "context_key": "course-v1:orgA+test_course+test_run", "org": "orgA", - "breadcrumbs": [{"display_name": "Test Course"}], "content": {} + "breadcrumbs": [{"display_name": "Test Course"}], "content": {}, + "access_id": course_access.id, + } meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_sequential]) vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") @@ -79,7 +89,8 @@ def test_create_delete_xblock(self, meilisearch_client): "context_key": "course-v1:orgA+test_course+test_run", "org": "orgA", "breadcrumbs": [{"display_name": "Test Course"}, {"display_name": "sequential"}], - "content": {} + "content": {}, + "access_id": course_access.id, } meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_vertical]) @@ -112,6 +123,7 @@ def test_create_delete_library_block(self, meilisearch_client): title="Library Org A", description="This is a library from Org A", ) + lib_access, _ = SearchAccess.objects.get_or_create(context_key=library.key) problem = library_api.create_library_block(library.key, "problem", "Problem1") doc_problem = { @@ -124,7 +136,8 @@ def test_create_delete_library_block(self, meilisearch_client): "context_key": "lib:orgA:lib_a", "org": "orgA", "breadcrumbs": [{"display_name": "Library Org A"}], - "content": {"problem_types": [], "capa_content": " "} + "content": {"problem_types": [], "capa_content": " "}, + "access_id": lib_access.id, } meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_problem]) diff --git a/openedx/core/djangoapps/content/search/tests/test_views.py b/openedx/core/djangoapps/content/search/tests/test_views.py index 83751eef37c3..9b32bdf327a8 100644 --- a/openedx/core/djangoapps/content/search/tests/test_views.py +++ b/openedx/core/djangoapps/content/search/tests/test_views.py @@ -4,7 +4,7 @@ from __future__ import annotations import functools -from unittest import mock +from unittest.mock import ANY, MagicMock, Mock, patch import ddt from django.test import override_settings @@ -32,7 +32,7 @@ def mock_meilisearch(enabled=True): """ - Decorator that mocks the required parts of content.search.views to simulate a running Meilisearch instance. + Decorator that mocks the required parts of content.search.api to simulate a running Meilisearch instance. """ def decorator(func): """ @@ -44,17 +44,19 @@ def wrapper(*args, **kwargs): MEILISEARCH_ENABLED=enabled, MEILISEARCH_PUBLIC_URL="http://meilisearch.url", ): - with mock.patch( - 'openedx.core.djangoapps.content.search.views._get_meili_api_key_uid', + with patch( + 'openedx.core.djangoapps.content.search.api._get_meili_api_key_uid', return_value=MOCK_API_KEY_UID, ): return func(*args, **kwargs) + return wrapper return decorator @ddt.ddt @skip_unless_cms +@patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", new=MagicMock(return_value=None)) class StudioSearchViewTest(StudioSearchTestMixin, SharedModuleStoreTestCase): """ General tests for the Studio search REST API. @@ -91,45 +93,33 @@ def test_studio_search_disabled(self): result = self.client.get(STUDIO_SEARCH_ENDPOINT_URL) assert result.status_code == 404 + def _mock_generate_tenant_token(self, mock_search_client): + """ + Return a mocked meilisearch.Client.generate_tenant_token method. + """ + mock_generate_tenant_token = Mock(return_value='restricted_api_key') + mock_search_client.return_value = Mock( + generate_tenant_token=mock_generate_tenant_token, + ) + return mock_generate_tenant_token + @mock_meilisearch(enabled=True) - def test_studio_search_enabled(self): + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') + def test_studio_search_enabled(self, mock_search_client): """ We've implement fine-grained permissions on the meilisearch content, so any logged-in user can get a restricted API key for Meilisearch using the REST API. """ self.client.login(username='student', password='student_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 assert result.data["index_name"] == "studio_content" assert result.data["url"] == "http://meilisearch.url" assert result.data["api_key"] and isinstance(result.data["api_key"], str) - @override_settings(MEILISEARCH_ENABLED=True) - def test_studio_search_staff(self, _meilisearch_client): - """ - Global staff can get a restricted API key for Meilisearch using the REST - API. - """ - _meilisearch_client.return_value.generate_tenant_token.return_value = "api_key" - self.client.login(username='staff', password='staff_pass') - result = self.client.get(STUDIO_SEARCH_ENDPOINT_URL) - assert result.status_code == 200 - assert result.data["index_name"] == "studio_content" - assert result.data["url"] == "http://meilisearch.url" - assert result.data["api_key"] and isinstance(result.data["api_key"], str) - - def _mock_generate_tenant_token(self, mock_search_client): - """ - Return a mocked meilisearch.Client.generate_tenant_token method. - """ - mock_generate_tenant_token = mock.Mock(return_value='restricted_api_key') - mock_search_client.return_value = mock.Mock( - generate_tenant_token=mock_generate_tenant_token, - ) - return mock_generate_tenant_token - @mock_meilisearch(enabled=True) - @mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client') + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') def test_studio_search_student_no_access(self, mock_search_client): """ Users without access to any courses or libraries will have all documents filtered out. @@ -145,11 +135,11 @@ def test_studio_search_student_no_access(self, mock_search_client): "filter": "org IN [] OR access_id IN []", } }, - expires_at=mock.ANY, + expires_at=ANY, ) @mock_meilisearch(enabled=True) - @mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client') + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') def test_studio_search_staff(self, mock_search_client): """ Users with global staff access can search any document. @@ -163,11 +153,11 @@ def test_studio_search_staff(self, mock_search_client): search_rules={ "studio_content": {} }, - expires_at=mock.ANY, + expires_at=ANY, ) @mock_meilisearch(enabled=True) - @mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client') + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') def test_studio_search_course_staff_access(self, mock_search_client): """ Users with staff or instructor access to a course or library will be limited to these courses/libraries. @@ -189,7 +179,7 @@ def test_studio_search_course_staff_access(self, mock_search_client): "filter": f"org IN [] OR access_id IN {expected_access_ids}", } }, - expires_at=mock.ANY, + expires_at=ANY, ) @ddt.data( @@ -197,7 +187,7 @@ def test_studio_search_course_staff_access(self, mock_search_client): 'org_instr', ) @mock_meilisearch(enabled=True) - @mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client') + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') 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. @@ -213,11 +203,11 @@ def test_studio_search_org_access(self, username, mock_search_client): "filter": "org IN ['org1'] OR access_id IN []", } }, - expires_at=mock.ANY, + expires_at=ANY, ) @mock_meilisearch(enabled=True) - @mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client') + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') def test_studio_search_omit_orgs(self, mock_search_client): """ Grant org access to our staff user to ensure that org's access_ids are omitted from the search filter. @@ -240,13 +230,13 @@ def test_studio_search_omit_orgs(self, mock_search_client): "filter": f"org IN ['org1'] OR access_id IN {expected_access_ids}", } }, - expires_at=mock.ANY, + expires_at=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_access_ids_for_request') - @mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client') + @patch('openedx.core.djangoapps.content.search.api._get_user_orgs') + @patch('openedx.core.djangoapps.content.search.api.get_access_ids_for_request') + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') def test_studio_search_limits(self, mock_search_client, mock_get_access_ids, mock_get_user_orgs): """ Users with access to many courses/libraries or orgs will only be able to search content @@ -275,5 +265,5 @@ def test_studio_search_limits(self, mock_search_client, mock_get_access_ids, moc "filter": f"org IN {expected_user_orgs} OR access_id IN {expected_access_ids}", } }, - expires_at=mock.ANY, + expires_at=ANY, ) diff --git a/openedx/core/djangoapps/content/search/views.py b/openedx/core/djangoapps/content/search/views.py index e6dd172bc006..f747fe77e8b5 100644 --- a/openedx/core/djangoapps/content/search/views.py +++ b/openedx/core/djangoapps/content/search/views.py @@ -2,6 +2,7 @@ REST API for content search """ from __future__ import annotations + import logging from django.contrib.auth import get_user_model