Skip to content

Commit

Permalink
fix: fix code and tests after merge
Browse files Browse the repository at this point in the history
  • Loading branch information
@rpenido
rpenido committed Apr 17, 2024
1 parent cbf4a46 commit 670c04a
Show file tree
Hide file tree
Showing 5 changed files with 114 additions and 54 deletions.
57 changes: 51 additions & 6 deletions openedx/core/djangoapps/content/search/api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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]:
Expand Down Expand Up @@ -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 ##############
Expand Down Expand Up @@ -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(
Expand Down
15 changes: 13 additions & 2 deletions openedx/core/djangoapps/content/search/tests/test_api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/"


Expand Down Expand Up @@ -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")
Expand All @@ -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 = {
Expand All @@ -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:
Expand All @@ -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 = {
Expand All @@ -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)
Expand Down
19 changes: 16 additions & 3 deletions openedx/core/djangoapps/content/search/tests/test_handlers.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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])
Expand Down Expand Up @@ -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 = {
Expand All @@ -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])
Expand Down
76 changes: 33 additions & 43 deletions openedx/core/djangoapps/content/search/tests/test_views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
"""
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand All @@ -189,15 +179,15 @@ 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(
'org_staff',
'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.
Expand All @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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,
)
Loading

0 comments on commit 670c04a

Please sign in to comment.