diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 66314ec..f07e857 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -16,6 +16,14 @@ Unreleased * +0.7.0 - 2024-04-12 +****************** + +Added +===== + +* Add endpoint for fetchGuestToken + 0.6.0 - 2024-04-08 ****************** diff --git a/platform_plugin_aspects/__init__.py b/platform_plugin_aspects/__init__.py index 8927290..1c8e7db 100644 --- a/platform_plugin_aspects/__init__.py +++ b/platform_plugin_aspects/__init__.py @@ -5,6 +5,6 @@ import os from pathlib import Path -__version__ = "0.6.0" +__version__ = "0.7.0" ROOT_DIRECTORY = Path(os.path.dirname(os.path.abspath(__file__))) diff --git a/platform_plugin_aspects/apps.py b/platform_plugin_aspects/apps.py index fa2b8a4..64197aa 100644 --- a/platform_plugin_aspects/apps.py +++ b/platform_plugin_aspects/apps.py @@ -3,7 +3,7 @@ """ from django.apps import AppConfig -from edx_django_utils.plugins import PluginSettings, PluginSignals +from edx_django_utils.plugins import PluginSettings, PluginSignals, PluginURLs class PlatformPluginAspectsConfig(AppConfig): @@ -14,6 +14,13 @@ class PlatformPluginAspectsConfig(AppConfig): name = "platform_plugin_aspects" plugin_app = { + PluginURLs.CONFIG: { + "lms.djangoapp": { + PluginURLs.NAMESPACE: "", + PluginURLs.REGEX: r"^aspects/", + PluginURLs.RELATIVE_PATH: "urls", + }, + }, PluginSettings.CONFIG: { "lms.djangoapp": { "production": {PluginSettings.RELATIVE_PATH: "settings.production"}, diff --git a/platform_plugin_aspects/extensions/filters.py b/platform_plugin_aspects/extensions/filters.py index 428d53c..982f389 100644 --- a/platform_plugin_aspects/extensions/filters.py +++ b/platform_plugin_aspects/extensions/filters.py @@ -14,12 +14,6 @@ TEMPLATE_ABSOLUTE_PATH = "/instructor_dashboard/" BLOCK_CATEGORY = "aspects" -ASPECTS_SECURITY_FILTERS_FORMAT = [ - "org = '{course.org}'", - "course_name = '{course.display_name}'", - "course_run = '{course.id.run}'", -] - class AddSupersetTab(PipelineStep): """ @@ -36,9 +30,6 @@ def run_filter( """ course = context["course"] dashboards = settings.ASPECTS_INSTRUCTOR_DASHBOARDS - extra_filters_format = settings.SUPERSET_EXTRA_FILTERS_FORMAT - - filters = ASPECTS_SECURITY_FILTERS_FORMAT + extra_filters_format user = get_current_user() @@ -49,11 +40,10 @@ def run_filter( if formatted_language not in settings.SUPERSET_DASHBOARD_LOCALES: formatted_language = "en_US" + context["course_id"] = course.id context = generate_superset_context( context, - user, dashboards=dashboards, - filters=filters, language=formatted_language, ) @@ -66,7 +56,8 @@ def run_filter( "fragment": frag, "section_key": BLOCK_CATEGORY, "section_display_name": _("Analytics"), - "course_id": str(course.id), + "course_id": str(context.get("course_id")), + "superset_guest_token_url": str(context.get("superset_guest_token_url")), "superset_url": str(context.get("superset_url")), "template_path_prefix": TEMPLATE_ABSOLUTE_PATH, } diff --git a/platform_plugin_aspects/extensions/tests/test_filters.py b/platform_plugin_aspects/extensions/tests/test_filters.py index eb64abf..adc229e 100644 --- a/platform_plugin_aspects/extensions/tests/test_filters.py +++ b/platform_plugin_aspects/extensions/tests/test_filters.py @@ -20,12 +20,13 @@ def setUp(self) -> None: """ self.filter = AddSupersetTab(filter_type=Mock(), running_pipeline=Mock()) self.template_name = "test-template-name" - self.context = {"course": Mock()} + self.course_id = "course-v1:org+course+run" + self.context = {"course": Mock(id=self.course_id), "sections": []} - @patch("platform_plugin_aspects.extensions.filters.generate_superset_context") @patch("platform_plugin_aspects.extensions.filters.get_model") def test_run_filter_with_language( - self, mock_get_model, mock_generate_superset_context + self, + mock_get_model, ): """ Check the filter is not executed when there are no LimeSurvey blocks in the course. @@ -33,30 +34,27 @@ def test_run_filter_with_language( Expected result: - The context is returned without modifications. """ - mock_generate_superset_context.return_value = { - "sections": [], - "superset_url": "http://superset.testing", - } - mock_get_model.return_value.get_value.return_value = "not-a-language" context = self.filter.run_filter(self.context, self.template_name) self.assertDictContainsSubset( { - "course_id": str(self.context["course"].id), + "course_id": self.course_id, "section_key": BLOCK_CATEGORY, "section_display_name": "Analytics", - "superset_url": "http://superset.testing", + "superset_url": "http://superset-dummy-url/", + "superset_guest_token_url": f"https://lms.url/superset_guest_token/{self.course_id}", "template_path_prefix": "/instructor_dashboard/", }, context["context"]["sections"][0], ) + mock_get_model.assert_called_once() - @patch("platform_plugin_aspects.extensions.filters.generate_superset_context") @patch("platform_plugin_aspects.extensions.filters.get_model") def test_run_filter_without_language( - self, mock_get_model, mock_generate_superset_context + self, + mock_get_model, ): """ Check the filter is not executed when there are no LimeSurvey blocks in the course. @@ -64,22 +62,20 @@ def test_run_filter_without_language( Expected result: - The context is returned without modifications. """ - mock_generate_superset_context.return_value = { - "sections": [], - "superset_url": "http://superset.testing", - } - mock_get_model.return_value.get_value.return_value = None context = self.filter.run_filter(self.context, self.template_name) self.assertDictContainsSubset( { - "course_id": str(self.context["course"].id), + "course_id": self.course_id, "section_key": BLOCK_CATEGORY, "section_display_name": "Analytics", - "superset_url": "http://superset.testing", + "superset_url": "http://superset-dummy-url/", + "superset_guest_token_url": f"https://lms.url/superset_guest_token/{self.course_id}", "template_path_prefix": "/instructor_dashboard/", }, context["context"]["sections"][0], ) + + mock_get_model.assert_called_once() diff --git a/platform_plugin_aspects/signals.py b/platform_plugin_aspects/signals.py index 314d04e..4748461 100644 --- a/platform_plugin_aspects/signals.py +++ b/platform_plugin_aspects/signals.py @@ -33,7 +33,6 @@ def receive_course_publish( # pylint: disable=unused-argument # pragma: no cov dump_course_to_clickhouse.delay(str(course_key)) -@receiver(post_save, sender=get_model("user_profile")) def on_user_profile_updated( # pylint: disable=unused-argument # pragma: no cover sender, instance, **kwargs ): @@ -53,7 +52,13 @@ def on_user_profile_updated( # pylint: disable=unused-argument # pragma: no co ) -@receiver(post_save, sender=get_model("external_id")) +# Connect the UserProfile.post_save signal handler only if we have a model to attach to. +# (prevents celery errors during tests) +_user_profile = get_model("user_profile") +if _user_profile: + post_save.connect(on_user_profile_updated, sender=_user_profile) # pragma: no cover + + def on_externalid_saved( # pylint: disable=unused-argument # pragma: no cover sender, instance, **kwargs ): @@ -73,6 +78,13 @@ def on_externalid_saved( # pylint: disable=unused-argument # pragma: no cover ) +# Connect the ExternalId.post_save signal handler only if we have a model to attach to. +# (prevents celery errors during tests) +_external_id = get_model("external_id") +if _external_id: + post_save.connect(on_externalid_saved, sender=_external_id) # pragma: no cover + + @receiver(USER_RETIRE_LMS_MISC) def on_user_retirement( # pylint: disable=unused-argument # pragma: no cover sender, user, **kwargs diff --git a/platform_plugin_aspects/static/html/superset.html b/platform_plugin_aspects/static/html/superset.html index 2782da9..fd5e9fb 100644 --- a/platform_plugin_aspects/static/html/superset.html +++ b/platform_plugin_aspects/static/html/superset.html @@ -10,7 +10,7 @@

{{display_name}}

{{exception}}

{% elif not superset_dashboards %}

Dashboard UUID is not set. Please set the dashboard UUID in the Studio.

- {% elif superset_url and superset_token %} {% if xblock_id %} + {% elif superset_url and superset_guest_token_url %} {% if xblock_id %}
{% else %}
@@ -31,7 +31,7 @@

{{display_name}}

{% endif %}
diff --git a/platform_plugin_aspects/static/js/embed_dashboard.js b/platform_plugin_aspects/static/js/embed_dashboard.js index 1013001..fda579a 100644 --- a/platform_plugin_aspects/static/js/embed_dashboard.js +++ b/platform_plugin_aspects/static/js/embed_dashboard.js @@ -1,11 +1,46 @@ -function embedDashboard(dashboard_uuid, superset_url, superset_token, xblock_id) { +function getCookie(name) { + let cookieValue = null; + if (document.cookie && document.cookie !== "") { + const cookies = document.cookie.split(";"); + for (let i = 0; i < cookies.length; i++) { + const cookie = cookies[i].trim(); + // Does this cookie string begin with the name we want? + if (cookie.substring(0, name.length + 1) === name + "=") { + cookieValue = decodeURIComponent(cookie.substring(name.length + 1)); + break; + } + } + } + return cookieValue; +} + +async function fetchGuestToken() { + const response = await fetch(superset_guest_token_url, { + method: 'GET', + headers: { + "X-CSRFToken": getCookie("csrftoken"), + } + }); + + if (!response.ok) { + console.error(await response.json()); + // TODO: Handle error + return null; + } + + const data = await response.json(); + return data.guestToken; +} + +function embedDashboard(dashboard_uuid, superset_url, xblock_id) { xblock_id = xblock_id || ""; + window.supersetEmbeddedSdk .embedDashboard({ id: dashboard_uuid, // given by the Superset embedding UI supersetDomain: superset_url, // your Superset instance mountPoint: document.getElementById(`superset-embedded-container-${xblock_id}`), // any html element that can contain an iframe - fetchGuestToken: () => superset_token, // function that returns a Promise with the guest token + fetchGuestToken: fetchGuestToken, dashboardUiConfig: { // dashboard UI config: hideTitle, hideTab, hideChartControls, filters.visible, filters.expanded (optional) hideTitle: true, @@ -28,6 +63,6 @@ function embedDashboard(dashboard_uuid, superset_url, superset_token, xblock_id) if (window.superset_dashboards !== undefined) { window.superset_dashboards.forEach(function(dashboard) { - embedDashboard(dashboard.uuid, window.superset_url, window.superset_token, dashboard.uuid); + embedDashboard(dashboard.uuid, window.superset_url, dashboard.uuid); }); } diff --git a/platform_plugin_aspects/static/js/superset.js b/platform_plugin_aspects/static/js/superset.js index 14ae644..0198b6d 100644 --- a/platform_plugin_aspects/static/js/superset.js +++ b/platform_plugin_aspects/static/js/superset.js @@ -2,11 +2,11 @@ function SupersetXBlock(runtime, element, context) { const dashboard_uuid = context.dashboard_uuid; const superset_url = context.superset_url; - const superset_token = context.superset_token; + const superset_guest_token_url = runtime.handlerUrl(element, 'get_superset_guest_token'); const xblock_id = context.xblock_id function initSuperset(supersetEmbeddedSdk) { - embedDashboard(dashboard_uuid, superset_url, superset_token, xblock_id); + embedDashboard(dashboard_uuid, superset_url, superset_guest_token_url, xblock_id); } if (typeof require === "function") { diff --git a/platform_plugin_aspects/tests/test_utils.py b/platform_plugin_aspects/tests/test_utils.py index 3839373..11ca2aa 100644 --- a/platform_plugin_aspects/tests/test_utils.py +++ b/platform_plugin_aspects/tests/test_utils.py @@ -2,19 +2,21 @@ Test utils. """ -from collections import namedtuple from unittest.mock import Mock, patch from django.conf import settings +from django.core.exceptions import ImproperlyConfigured from django.test import TestCase +from requests.exceptions import HTTPError from platform_plugin_aspects.utils import ( + generate_guest_token, generate_superset_context, get_ccx_courses, get_model, ) -User = namedtuple("User", ["username"]) +COURSE_ID = "course-v1:org+course+run" class TestUtils(TestCase): @@ -94,25 +96,11 @@ def test_get_ccx_courses_feature_disabled(self): self.assertEqual(list(courses), []) - @patch.object( - settings, - "SUPERSET_CONFIG", - { - "internal_service_url": "http://superset:8088", - "service_url": "http://superset-dummy-url/", - "username": "superset", - "password": "superset", - }, - ) - @patch("platform_plugin_aspects.utils._generate_guest_token") - def test_generate_superset_context(self, mock_generate_guest_token): + def test_generate_superset_context(self): """ Test generate_superset_context """ - course_mock = Mock() - filter_mock = Mock() - user_mock = Mock() - context = {"course": course_mock} + context = {"course_id": COURSE_ID} dashboards = settings.ASPECTS_INSTRUCTOR_DASHBOARDS dashboards.append( @@ -123,97 +111,87 @@ def test_generate_superset_context(self, mock_generate_guest_token): } ) - mock_generate_guest_token.return_value = ("test-token", dashboards) - context = generate_superset_context( context, - user_mock, dashboards=dashboards, - filters=[filter_mock], language="en_US", ) - self.assertEqual(context["superset_token"], "test-token") + self.assertEqual( + context["superset_guest_token_url"], + f"https://lms.url/superset_guest_token/{COURSE_ID}", + ) self.assertEqual(context["superset_dashboards"], dashboards) self.assertEqual(context["superset_url"], "http://superset-dummy-url/") + self.assertNotIn("superset_token", context) self.assertNotIn("exception", context) @patch("platform_plugin_aspects.utils.SupersetClient") - def test_generate_superset_context_with_superset_client_exception( + def test_generate_guest_token_with_superset_client_http_error( self, mock_superset_client ): """ - Test generate_superset_context + Test generate_guest_token when Superset throws an HTTP error. """ - course_mock = Mock() filter_mock = Mock() user_mock = Mock() - context = {"course": course_mock} - mock_superset_client.side_effect = Exception("test-exception") - - context = generate_superset_context( - context, - user_mock, - dashboards=[{"name": "test", "uuid": "test-dashboard-uuid"}], - filters=[filter_mock], + response_mock = Mock() + mock_superset_client.side_effect = HTTPError( + "server error", response=response_mock ) - self.assertIn("exception", context) + with self.assertRaises(ImproperlyConfigured): + generate_guest_token( + user=user_mock, + course=COURSE_ID, + dashboards=[{"name": "test", "uuid": "test-dashboard-uuid"}], + filters=[filter_mock], + ) + + mock_superset_client.assert_called_once() - @patch.object( - settings, - "SUPERSET_CONFIG", - { - "internal_service_url": "http://superset:8088", - "service_url": "http://dummy-superset-url", - "username": "superset", - "password": "superset", - }, - ) @patch("platform_plugin_aspects.utils.SupersetClient") - def test_generate_superset_context_succesful(self, mock_superset_client): + def test_generate_guest_token_with_superset_client_exception( + self, mock_superset_client + ): """ - Test generate_superset_context + Test generate_guest_token when there's a general Exception. """ - course_mock = Mock() filter_mock = Mock() user_mock = Mock() - user_mock.username = "test-user" - context = {"course": course_mock} + mock_superset_client.side_effect = Exception("test-exception") + + with self.assertRaises(ImproperlyConfigured): + generate_guest_token( + user=user_mock, + course=COURSE_ID, + dashboards=[{"name": "test", "uuid": "test-dashboard-uuid"}], + filters=[filter_mock], + ) + + mock_superset_client.assert_called_once() + + @patch("platform_plugin_aspects.utils.SupersetClient") + def test_generate_guest_token_succesful(self, mock_superset_client): + """ + Test generate_guest_token works. + """ response_mock = Mock(status_code=200) mock_superset_client.return_value.session.post.return_value = response_mock response_mock.json.return_value = { "token": "test-token", } - dashboards = [{"name": "test", "uuid": "test-dashboard-uuid"}] - - context = generate_superset_context( - context, - user_mock, - dashboards=dashboards, - filters=[filter_mock], - ) - - self.assertEqual(context["superset_token"], "test-token") - self.assertEqual(context["superset_dashboards"], dashboards) - self.assertEqual(context["superset_url"], "http://dummy-superset-url/") - - def test_generate_superset_context_with_exception(self): - """ - Test generate_superset_context - """ - course_mock = Mock() filter_mock = Mock() user_mock = Mock() - user_mock.username = "test-user" - context = {"course": course_mock} + dashboards = [{"name": "test", "uuid": "test-dashboard-uuid"}] - context = generate_superset_context( - context, - user_mock, - dashboards=[{"name": "test", "uuid": "test-dashboard-uuid"}], + token = generate_guest_token( + user=user_mock, + course=COURSE_ID, + dashboards=dashboards, filters=[filter_mock], ) - self.assertIn("exception", context) + mock_superset_client.assert_called_once() + self.assertEqual(token, "test-token") diff --git a/platform_plugin_aspects/tests/test_views.py b/platform_plugin_aspects/tests/test_views.py new file mode 100644 index 0000000..4d5a61e --- /dev/null +++ b/platform_plugin_aspects/tests/test_views.py @@ -0,0 +1,128 @@ +""" +Test views. +""" + +from unittest.mock import Mock, patch + +from django.conf import settings +from django.contrib.auth import get_user_model +from django.core.exceptions import ImproperlyConfigured, ObjectDoesNotExist +from django.test import TestCase +from opaque_keys.edx.keys import CourseKey +from rest_framework.test import APIClient + +from ..views import Course, IsCourseStaffInstructor + +COURSE_ID = "course-v1:org+course+run" +User = get_user_model() + + +class ViewsTestCase(TestCase): + """ + Test cases for the plugin views and URLs. + """ + + def setUp(self): + """ + Set up data used by multiple tests. + """ + super().setUp() + self.client = APIClient() + self.superset_guest_token_url = f"/superset_guest_token/{COURSE_ID}" + self.user = User.objects.create( + username="user", + email="user@example.com", + ) + self.user.set_password("password") + self.user.save() + + def test_guest_token_requires_authorization(self): + response = self.client.get(self.superset_guest_token_url) + self.assertEqual(response.status_code, 403) + + def test_guest_token_requires_course_access(self): + self.client.login(username="user", password="password") + response = self.client.get(self.superset_guest_token_url) + self.assertEqual(response.status_code, 403) + + def test_guest_token_invalid_course_id(self): + superset_guest_token_url = "/superset_guest_token/block-v1:org+course+run" + self.client.login(username="user", password="password") + response = self.client.get(superset_guest_token_url) + self.assertEqual(response.status_code, 404) + + @patch("platform_plugin_aspects.views.get_model") + def test_guest_token_course_not_found(self, mock_get_model): + mock_model_get = Mock(side_effect=ObjectDoesNotExist) + mock_get_model.return_value = Mock( + objects=Mock(get=mock_model_get), + DoesNotExist=ObjectDoesNotExist, + ) + + self.client.login(username="user", password="password") + response = self.client.get(self.superset_guest_token_url) + self.assertEqual(response.status_code, 404) + mock_model_get.assert_called_once() + + @patch.object(IsCourseStaffInstructor, "has_object_permission") + @patch("platform_plugin_aspects.views.generate_guest_token") + def test_guest_token(self, mock_generate_guest_token, mock_has_object_permission): + mock_has_object_permission.return_value = True + mock_generate_guest_token.return_value = "test-token" + + self.client.login(username="user", password="password") + response = self.client.get(self.superset_guest_token_url) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json().get("guestToken"), "test-token") + mock_has_object_permission.assert_called_once() + mock_generate_guest_token.assert_called_once() + + @patch.object(IsCourseStaffInstructor, "has_object_permission") + @patch("platform_plugin_aspects.views.generate_guest_token") + def test_no_guest_token( + self, mock_generate_guest_token, mock_has_object_permission + ): + mock_has_object_permission.return_value = True + mock_generate_guest_token.side_effect = ImproperlyConfigured + + self.client.login(username="user", password="password") + response = self.client.get(self.superset_guest_token_url) + + self.assertEqual(response.status_code, 500) + mock_has_object_permission.assert_called_once() + mock_generate_guest_token.assert_called_once() + + @patch("platform_plugin_aspects.views.get_model") + @patch.object(IsCourseStaffInstructor, "has_object_permission") + @patch("platform_plugin_aspects.views.generate_guest_token") + def test_guest_token_with_course_overview( + self, mock_generate_guest_token, mock_has_object_permission, mock_get_model + ): + mock_has_object_permission.return_value = True + mock_generate_guest_token.return_value = "test-token" + mock_model_get = Mock(return_value=Mock(display_name="Course Title")) + mock_get_model.return_value = Mock( + objects=Mock(get=mock_model_get), + DoesNotExist=ObjectDoesNotExist, + ) + + self.client.login(username="user", password="password") + response = self.client.get(self.superset_guest_token_url) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json().get("guestToken"), "test-token") + mock_has_object_permission.assert_called_once() + mock_model_get.assert_called_once() + mock_generate_guest_token.assert_called_once_with( + user=self.user, + course=Course( + course_id=CourseKey.from_string(COURSE_ID), display_name="Course Title" + ), + dashboards=settings.ASPECTS_INSTRUCTOR_DASHBOARDS, + filters=[ + "org = 'org'", + "course_name = 'Course Title'", + "course_run = 'run'", + ], + ) diff --git a/platform_plugin_aspects/tests/test_xblock.py b/platform_plugin_aspects/tests/test_xblock.py index 2355110..354b52d 100644 --- a/platform_plugin_aspects/tests/test_xblock.py +++ b/platform_plugin_aspects/tests/test_xblock.py @@ -2,10 +2,13 @@ """ Test basic SupersetXBlock display function """ +import json from unittest import TestCase from unittest.mock import Mock, patch +from django.core.exceptions import ImproperlyConfigured from opaque_keys.edx.locator import CourseLocator +from webob import Request from xblock.field_data import DictFieldData from xblock.reference.user_service import XBlockUser @@ -35,11 +38,21 @@ def service(block, service): # pylint: disable=unused-argument def local_resource_url(_self, url): return url + def handler_url(_self, handler, *args, **kwargs): + """ + Mock runtime.handlerUrl + + The LMS and CMS runtimes implement handlerUrl, but we have to mock it here. + """ + return f"/{handler}" + runtime = Mock( course_id=course_id, service=service, local_resource_url=Mock(side_effect=local_resource_url), + handlerUrl=Mock(side_effect=handler_url), ) + scope_ids = Mock() field_data = DictFieldData(kwargs) xblock = SupersetXBlock(runtime, field_data, scope_ids) @@ -52,21 +65,12 @@ class TestRender(TestCase): Test the HTML rendering of the XBlock """ - @patch("platform_plugin_aspects.utils.SupersetClient") - def test_render_instructor(self, mock_superset_client): + def test_render_instructor(self): """ Ensure staff can see the Superset dashboard. """ - mock_superset_client.return_value = Mock( - session=Mock( - post=Mock( - return_value=Mock(json=Mock(return_value={"token": "test_token"})) - ) - ) - ) xblock = make_an_xblock("instructor") student_view = xblock.student_view() - mock_superset_client.assert_called_once() html = student_view.content self.assertIsNotNone(html) self.assertIn( @@ -87,14 +91,10 @@ def test_render_student(self): @patch("platform_plugin_aspects.xblock.pkg_resources.resource_exists") @patch("platform_plugin_aspects.xblock.translation.get_language") - @patch("platform_plugin_aspects.utils._generate_guest_token") - def test_render_translations( - self, mock_generate_guest_token, mock_get_language, mock_resource_exists - ): + def test_render_translations(self, mock_get_language, mock_resource_exists): """ Ensure translated javascript is served. """ - mock_generate_guest_token.return_value = ("test-token", "test-dashboard-uuid") mock_get_language.return_value = "eo" mock_resource_exists.return_value = True xblock = make_an_xblock("instructor") @@ -104,20 +104,56 @@ def test_render_translations( url_resource = resource self.assertIsNotNone(url_resource, "No 'url' resource found in fragment") self.assertIn("eo/text.js", url_resource.data) + mock_get_language.assert_called_once() + mock_resource_exists.assert_called_once() @patch("platform_plugin_aspects.xblock.translation.get_language") - @patch("platform_plugin_aspects.utils._generate_guest_token") def test_render_no_translations( self, - mock_generate_guest_token, mock_get_language, ): """ Ensure translated javascript is served. """ - mock_generate_guest_token.return_value = ("test-token", "test-dashboard-uuid") mock_get_language.return_value = None xblock = make_an_xblock("instructor") student_view = xblock.student_view() for resource in student_view.resources: assert resource.kind != "url" + mock_get_language.assert_called_once() + + @patch("platform_plugin_aspects.utils.SupersetClient") + def test_guest_token_handler(self, mock_superset_client): + response_mock = Mock(status_code=200) + mock_superset_client.return_value.session.post.return_value = response_mock + response_mock.json.return_value = { + "token": "test-token", + } + + request = Request.blank("/") + request.method = "POST" + request.body = b"{}" + xblock = make_an_xblock("instructor") + xblock.dashboard_uuid = "1d6bf904-f53f-47fd-b1c9-6cd7e284d286" + response = xblock.get_superset_guest_token(request) + + assert response.status_code == 200 + data = json.loads(response.body.decode("utf-8")) + assert data.get("guestToken") == "test-token" + mock_superset_client.assert_called_once() + + @patch("platform_plugin_aspects.views.generate_guest_token") + def test_guest_token_handler_failed(self, mock_generate_guest_token): + mock_generate_guest_token.side_effect = ImproperlyConfigured + + request = Request.blank("/") + request.method = "POST" + request.body = b"{}" + xblock = make_an_xblock("instructor") + response = xblock.get_superset_guest_token(request) + + assert response.status_code == 500 + data = json.loads(response.body.decode("utf-8")) + assert data.get("error") == ( + "Unable to fetch Superset guest token, mostly likely due to invalid settings.SUPERSET_CONFIG" + ) diff --git a/platform_plugin_aspects/urls.py b/platform_plugin_aspects/urls.py new file mode 100644 index 0000000..fd8b8d2 --- /dev/null +++ b/platform_plugin_aspects/urls.py @@ -0,0 +1,25 @@ +""" +Urls for the Aspects plugin. +""" + +from django.urls import include, path, re_path + +from . import views + +# Copied from openedx.core.constants +COURSE_ID_PATTERN = r"(?P[^/+]+(/|\+)[^/+]+(/|\+)[^/?]+)" + +app_url_patterns = ( + [ + re_path( + rf"superset_guest_token/{COURSE_ID_PATTERN}/?$", + views.SupersetView.as_view(), + name="superset_guest_token", + ), + ], + "platform_plugin_aspects", +) + +urlpatterns = [ + path("", include(app_url_patterns)), +] diff --git a/platform_plugin_aspects/utils.py b/platform_plugin_aspects/utils.py index 849252f..cb1fe39 100644 --- a/platform_plugin_aspects/utils.py +++ b/platform_plugin_aspects/utils.py @@ -8,8 +8,12 @@ import os import uuid from importlib import import_module +from urllib.parse import urljoin from django.conf import settings +from django.core.exceptions import ImproperlyConfigured +from django.urls import reverse +from requests.exceptions import HTTPError from supersetapiclient.client import SupersetClient from xblock.reference.user_service import XBlockUser @@ -26,73 +30,71 @@ def _(text): return text -def generate_superset_context( # pylint: disable=dangerous-default-value +def generate_superset_context( context, - user, dashboards, - filters=[], language=None, -): +) -> dict: """ Update context with superset token and dashboard id. Args: - context (dict): the context for the instructor dashboard. It must include a course object + context (dict): the context for the instructor dashboard. It must include a course_id. user (XBlockUser or User): the current user. superset_config (dict): superset config. dashboards (list): list of superset dashboard uuid. filters (list): list of filters to apply to the dashboard. language (str): the language code of the end user. """ - course = context["course"] + course_id = context["course_id"] superset_config = settings.SUPERSET_CONFIG if language: for dashboard in dashboards: - if not dashboard["allow_translations"]: + if not dashboard.get("allow_translations"): continue dashboard["slug"] = f"{dashboard['slug']}-{language}" dashboard["uuid"] = str(get_uuid5(dashboard["uuid"], language)) - superset_token, dashboards = _generate_guest_token( - user=user, - course=course, - superset_config=superset_config, - dashboards=dashboards, - filters=filters, + superset_url = _fix_service_url(superset_config.get("service_url")) + + # Use an absolute LMS URL here, just in case we're being rendered in an MFE. + guest_token_url = urljoin( + settings.LMS_ROOT_URL, + reverse( + "platform_plugin_aspects:superset_guest_token", + kwargs={"course_id": course_id}, + ), ) - if superset_token: - superset_url = _fix_service_url(superset_config.get("service_url")) - context.update( - { - "superset_token": superset_token, - "superset_dashboards": dashboards, - "superset_url": superset_url, - } - ) - else: - context.update( - { - "exception": str(dashboards), - } - ) + context.update( + { + "superset_dashboards": dashboards, + "superset_url": superset_url, + "superset_guest_token_url": guest_token_url, + } + ) return context -def _generate_guest_token(user, course, superset_config, dashboards, filters): +def generate_guest_token(user, course, dashboards, filters) -> str: """ - Generate a Superset guest token for the user. + Generate and return a Superset guest token for the user. + + Raise ImproperlyConfigured if the Superset API client request fails for any reason. Args: user: User object. - course: Course object. + course: Course object, used to populate `filters` template strings. + dashboards: list of dashboard UUIDs to grant access to. + filters: list of string filters to apply. Returns: tuple: Superset guest token and dashboard id. or None, exception if Superset is missconfigured or cannot generate guest token. """ + superset_config = settings.SUPERSET_CONFIG superset_internal_host = _fix_service_url( superset_config.get("internal_service_url") or superset_config.get("service_url") @@ -100,16 +102,6 @@ def _generate_guest_token(user, course, superset_config, dashboards, filters): superset_username = superset_config.get("username") superset_password = superset_config.get("password") - try: - client = SupersetClient( - host=superset_internal_host, - username=superset_username, - password=superset_password, - ) - except Exception as exc: # pylint: disable=broad-except - logger.error(exc) - return None, exc - formatted_filters = [filter.format(course=course, user=user) for filter in filters] data = { @@ -121,19 +113,40 @@ def _generate_guest_token(user, course, superset_config, dashboards, filters): } try: - logger.info(f"Requesting guest token from Superset, {data}") + client = SupersetClient( + host=superset_internal_host, + username=superset_username, + password=superset_password, + ) response = client.session.post( url=f"{superset_internal_host}api/v1/security/guest_token/", json=data, headers={"Content-Type": "application/json"}, ) response.raise_for_status() - token = response.json()["token"] + token = response.json().get("token") + return token - return token, dashboards - except Exception as exc: # pylint: disable=broad-except + except HTTPError as err: + # Superset server errors sometimes come with messages, so log the response. + logger.error( + f"{err.response.status_code} {err.response.json()} for url: {err.response.url}, data: {data}" + ) + raise ImproperlyConfigured( + _( + "Unable to fetch Superset guest token, " + "Superset server error {server_response}" + ).format(server_response=err.response.json()) + ) from err + + except Exception as exc: logger.error(exc) - return None, exc + raise ImproperlyConfigured( + _( + "Unable to fetch Superset guest token, " + "mostly likely due to invalid settings.SUPERSET_CONFIG" + ) + ) from exc def _fix_service_url(url: str) -> str: diff --git a/platform_plugin_aspects/views.py b/platform_plugin_aspects/views.py new file mode 100644 index 0000000..7c4b0cf --- /dev/null +++ b/platform_plugin_aspects/views.py @@ -0,0 +1,132 @@ +""" +Endpoints for the Aspects platform plugin. +""" + +from collections import namedtuple + +from django.conf import settings +from django.core.exceptions import ImproperlyConfigured +from django.utils.decorators import method_decorator +from django.views.decorators.cache import cache_page +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey +from rest_framework import permissions +from rest_framework.authentication import SessionAuthentication +from rest_framework.exceptions import APIException, NotFound +from rest_framework.generics import GenericAPIView +from rest_framework.response import Response + +from .utils import _, generate_guest_token, get_model + +try: + from openedx.core.lib.api.permissions import ( + IsCourseStaffInstructor, + IsStaffOrReadOnly, + ) +except ImportError: + + class IsCourseStaffInstructor(permissions.BasePermission): + """ + Permission class to use during tests. + + Importing from edx-platform doesn't work when running tests, + so we declare our own permission class here. + """ + + def has_object_permission(self, request, view, obj): + """ + Return False for security; mock this out during tests. + """ + return False + + class IsStaffOrReadOnly(permissions.BasePermission): + """ + Permission class to use during tests. + + Importing from edx-platform doesn't work when running tests, + so we declare our own permission class here. + """ + + def has_object_permission(self, request, view, obj): + """ + Return False for security; mock this out during tests. + """ + return False + + +# Course fields: +# * course_id: CourseKey; required by IsCourseStaffInstructor +# * display_name: str; optionally fetched from CourseOverview +Course = namedtuple("Course", ["course_id", "display_name"]) + + +class SupersetView(GenericAPIView): + """ + Superset-related endpoints provided by the aspects platform plugin. + """ + + authentication_classes = (SessionAuthentication,) + permission_classes = ( + permissions.IsAuthenticated, + IsStaffOrReadOnly | IsCourseStaffInstructor, + ) + + lookup_field = "course_id" + + def get_object(self): + """ + Return a Course-like object for the requested course_id. + """ + course_id = self.kwargs.get(self.lookup_field, "") + try: + course_key = CourseKey.from_string(course_id) + except InvalidKeyError as exc: + raise NotFound( + _("Invalid course id: '{course_id}'").format(course_id=course_id) + ) from exc + + # Fetch the CourseOverview (if we're running in edx-platform) + display_name = "" + CourseOverview = get_model("course_overviews") + if CourseOverview: + try: + course_overview = CourseOverview.objects.get(id=course_key) + display_name = course_overview.display_name + except CourseOverview.DoesNotExist as exc: + raise NotFound( + _("Course not found: '{course_id}'").format(course_id=course_id) + ) from exc + + course = Course(course_id=course_key, display_name=display_name) + + # May raise a permission denied + self.check_object_permissions(self.request, course) + + return course + + @method_decorator(cache_page(60 * 5)) + def get(self, request, *args, **kwargs): + """ + Return a guest token for accessing the Superset API. + """ + course = self.get_object() + + built_in_filters = [ + f"org = '{course.course_id.org}'", + f"course_name = '{course.display_name}'", + f"course_run = '{course.course_id.run}'", + ] + dashboards = settings.ASPECTS_INSTRUCTOR_DASHBOARDS + extra_filters_format = settings.SUPERSET_EXTRA_FILTERS_FORMAT + + try: + guest_token = generate_guest_token( + user=request.user, + course=course, + dashboards=dashboards, + filters=built_in_filters + extra_filters_format, + ) + except ImproperlyConfigured as exc: + raise APIException() from exc + + return Response({"guestToken": guest_token}) diff --git a/platform_plugin_aspects/xblock.py b/platform_plugin_aspects/xblock.py index 969afb5..b6b27a7 100644 --- a/platform_plugin_aspects/xblock.py +++ b/platform_plugin_aspects/xblock.py @@ -2,17 +2,21 @@ from __future__ import annotations +import json import logging import pkg_resources +from django.core.exceptions import ImproperlyConfigured from django.utils import translation from web_fragments.fragment import Fragment +from webob import Response from xblock.core import XBlock +from xblock.exceptions import JsonHandlerError from xblock.fields import List, Scope, String from xblock.utils.resources import ResourceLoader from xblock.utils.studio_editable import StudioEditableXBlockMixin -from .utils import _, generate_superset_context +from .utils import _, generate_guest_token, generate_superset_context log = logging.getLogger(__name__) loader = ResourceLoader(__name__) @@ -97,7 +101,7 @@ def student_view(self, context=None): context = context or {} context.update( { - "course": self.runtime.course_id, + "course_id": self.runtime.course_id, "display_name": self.display_name, } ) @@ -112,12 +116,14 @@ def student_view(self, context=None): context = generate_superset_context( context=context, - user=user, dashboards=self.dashboards(), - filters=self.filters, ) context["xblock_id"] = str(self.scope_ids.usage_id.block_id) + # Remove this URL from the context to avoid confusion. + # Our XBlock handler URL will be used instead, provided in superset.js + del context["superset_guest_token_url"] + frag = Fragment() frag.add_content(self.render_template("static/html/superset.html", context)) frag.add_css(loader.load_unicode("static/css/superset.css")) @@ -180,3 +186,27 @@ def _get_statici18n_js_url(): ): return text_js.format(locale_code=code) return None + + @XBlock.json_handler + def get_superset_guest_token( + self, request_body, suffix="" + ): # pylint: disable=unused-argument + """Return a guest token for Superset.""" + user_service = self.runtime.service(self, "user") + user = user_service.get_current_user() + + try: + guest_token = generate_guest_token( + user=user, + course=self.runtime.course_id, + dashboards=self.dashboards(), + filters=self.filters, + ) + except ImproperlyConfigured as exc: + raise JsonHandlerError(500, str(exc)) from exc + + return Response( + json.dumps({"guestToken": guest_token}), + content_type="application/json", + charset="UTF-8", + ) diff --git a/test_settings.py b/test_settings.py index 987b008..8ab4b80 100644 --- a/test_settings.py +++ b/test_settings.py @@ -24,7 +24,28 @@ }, } -INSTALLED_APPS = ("platform_plugin_aspects",) +INSTALLED_APPS = ( + "django.contrib.auth", + "django.contrib.contenttypes", + "django.contrib.sessions", + "platform_plugin_aspects", +) + +# Disable caching in tests. +CACHES = { + 'default': { + 'BACKEND': 'django.core.cache.backends.dummy.DummyCache', + } +} + +ROOT_URLCONF = "platform_plugin_aspects.urls" + +SECRET_KEY = "very-insecure-key" + +MIDDLEWARE = [ + "django.contrib.sessions.middleware.SessionMiddleware", + "django.contrib.auth.middleware.AuthenticationMiddleware", +] EVENT_SINK_CLICKHOUSE_MODEL_CONFIG = { "user_profile": { @@ -43,6 +64,8 @@ "CUSTOM_COURSES_EDX": True, } +LMS_ROOT_URL = "https://lms.url" + DEBUG = True TEMPLATES = [ @@ -61,18 +84,12 @@ SUPERSET_EXTRA_FILTERS_FORMAT = [] SUPERSET_CONFIG = { - "internal_service_url": "http://superset:8088", - "service_url": "http://dummy-superset-url", + "internal_service_url": "http://superset:8088/", + "service_url": "http://superset-dummy-url", "username": "superset", "password": "superset", } -SUPERSET_CONFIG = { - "internal_service_url": "http://superset:8088", - "service_url": "http://superset.local.overhang.io", - "username": "superset", - "password": "superset", -} ASPECTS_INSTRUCTOR_DASHBOARDS = [ { "name": "Instructor Dashboard", @@ -81,6 +98,7 @@ "allow_translations": True, } ] + EVENT_SINK_CLICKHOUSE_BACKEND_CONFIG = { "url": "https://foo.bar", "username": "bob",