From 4da80dd81b97c0d34a65e6c55bc6b5437d240f97 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 12 Apr 2024 17:16:36 +0930 Subject: [PATCH] fix: various fixes * generate_superset_context expects a course_id * superset_guest_token_url should be absolute to LMS * CSRF cookie fix from #28 * XBlock uses handler instead of view * view handles Superset server errors --- platform_plugin_aspects/extensions/filters.py | 4 +- .../extensions/tests/test_filters.py | 34 ++++++------ platform_plugin_aspects/signals.py | 4 +- .../static/js/embed_dashboard.js | 38 +++++++++---- platform_plugin_aspects/static/js/superset.js | 2 +- platform_plugin_aspects/tests/test_utils.py | 54 ++++++++++--------- platform_plugin_aspects/utils.py | 33 ++++++++---- platform_plugin_aspects/xblock.py | 9 ++-- test_settings.py | 13 ++--- 9 files changed, 113 insertions(+), 78 deletions(-) diff --git a/platform_plugin_aspects/extensions/filters.py b/platform_plugin_aspects/extensions/filters.py index 90b7c01..982f389 100644 --- a/platform_plugin_aspects/extensions/filters.py +++ b/platform_plugin_aspects/extensions/filters.py @@ -40,6 +40,7 @@ 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, dashboards=dashboards, @@ -55,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 1b69ca9..4748461 100644 --- a/platform_plugin_aspects/signals.py +++ b/platform_plugin_aspects/signals.py @@ -56,7 +56,7 @@ def on_user_profile_updated( # pylint: disable=unused-argument # pragma: no co # (prevents celery errors during tests) _user_profile = get_model("user_profile") if _user_profile: - post_save.connect(on_user_profile_updated, sender=_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 @@ -82,7 +82,7 @@ def on_externalid_saved( # pylint: disable=unused-argument # pragma: no cover # (prevents celery errors during tests) _external_id = get_model("external_id") if _external_id: - post_save.connect(on_externalid_saved, sender=_external_id) + post_save.connect(on_externalid_saved, sender=_external_id) # pragma: no cover @receiver(USER_RETIRE_LMS_MISC) diff --git a/platform_plugin_aspects/static/js/embed_dashboard.js b/platform_plugin_aspects/static/js/embed_dashboard.js index 0a26b44..a0f61fb 100644 --- a/platform_plugin_aspects/static/js/embed_dashboard.js +++ b/platform_plugin_aspects/static/js/embed_dashboard.js @@ -1,16 +1,36 @@ function embedDashboard(dashboard_uuid, superset_url, guest_token_url, xblock_id) { xblock_id = xblock_id || ""; - async fetchGuestToken() { - // Fetch the guest token from your backend - const response = await fetch(guest_token_url, { + 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() { + return fetch(guest_token_url, { method: 'POST', - body: JSON.stringify({ - // TODO csrf_token: csrf_token, - }) + headers: { + "X-CSRFToken": getCookie('csrftoken'), + }, + }).then(response => { + if (response.ok) { + const data = response.json(); + return data.guestToken; + } else { + console.error(response); + } }); - const data = await response.json(); - return data.guestToken; } window.supersetEmbeddedSdk @@ -41,6 +61,6 @@ function embedDashboard(dashboard_uuid, superset_url, guest_token_url, xblock_id if (window.superset_dashboards !== undefined) { window.superset_dashboards.forEach(function(dashboard) { - embedDashboard(dashboard.uuid, window.superset_url, window.superset_guest_token_url); + embedDashboard(dashboard.uuid, window.superset_url, window.superset_guest_token_url, dashboard.uuid); }); } diff --git a/platform_plugin_aspects/static/js/superset.js b/platform_plugin_aspects/static/js/superset.js index 392630c..0198b6d 100644 --- a/platform_plugin_aspects/static/js/superset.js +++ b/platform_plugin_aspects/static/js/superset.js @@ -2,7 +2,7 @@ function SupersetXBlock(runtime, element, context) { const dashboard_uuid = context.dashboard_uuid; const superset_url = context.superset_url; - const superset_guest_token_url = context.superset_guest_token_url; + const superset_guest_token_url = runtime.handlerUrl(element, 'get_superset_guest_token'); const xblock_id = context.xblock_id function initSuperset(supersetEmbeddedSdk) { diff --git a/platform_plugin_aspects/tests/test_utils.py b/platform_plugin_aspects/tests/test_utils.py index ccc187b..11ca2aa 100644 --- a/platform_plugin_aspects/tests/test_utils.py +++ b/platform_plugin_aspects/tests/test_utils.py @@ -7,6 +7,7 @@ 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, @@ -95,21 +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", - }, - ) def test_generate_superset_context(self): """ Test generate_superset_context """ - context = {"course": COURSE_ID} + context = {"course_id": COURSE_ID} dashboards = settings.ASPECTS_INSTRUCTOR_DASHBOARDS dashboards.append( @@ -127,19 +118,44 @@ def test_generate_superset_context(self): ) self.assertEqual( - context["superset_guest_token_url"], f"/superset_guest_token/{COURSE_ID}" + 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_guest_token_with_superset_client_http_error( + self, mock_superset_client + ): + """ + Test generate_guest_token when Superset throws an HTTP error. + """ + filter_mock = Mock() + user_mock = Mock() + response_mock = Mock() + mock_superset_client.side_effect = HTTPError( + "server error", response=response_mock + ) + + 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_with_superset_client_exception( self, mock_superset_client ): """ - Test generate_guest_token + Test generate_guest_token when there's a general Exception. """ filter_mock = Mock() user_mock = Mock() @@ -155,20 +171,10 @@ def test_generate_guest_token_with_superset_client_exception( 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_guest_token_succesful(self, mock_superset_client): """ - Test generate_guest_token + Test generate_guest_token works. """ response_mock = Mock(status_code=200) mock_superset_client.return_value.session.post.return_value = response_mock diff --git a/platform_plugin_aspects/utils.py b/platform_plugin_aspects/utils.py index 40e9c8b..cb1fe39 100644 --- a/platform_plugin_aspects/utils.py +++ b/platform_plugin_aspects/utils.py @@ -8,10 +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 @@ -37,14 +39,14 @@ def generate_superset_context( Update context with superset token and dashboard id. Args: - context (dict): the context for the instructor dashboard. It must include a course ID + 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: @@ -56,9 +58,13 @@ def generate_superset_context( superset_url = _fix_service_url(superset_config.get("service_url")) - guest_token_url = reverse( - "platform_plugin_aspects:superset_guest_token", - kwargs={"course_id": course}, + # 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}, + ), ) context.update( @@ -112,18 +118,27 @@ def generate_guest_token(user, course, dashboards, filters) -> str: username=superset_username, password=superset_password, ) - - logger.info(f"Requesting guest token from Superset, {data}") 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 + 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) raise ImproperlyConfigured( diff --git a/platform_plugin_aspects/xblock.py b/platform_plugin_aspects/xblock.py index 33bd0b0..b6b27a7 100644 --- a/platform_plugin_aspects/xblock.py +++ b/platform_plugin_aspects/xblock.py @@ -101,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, } ) @@ -120,10 +120,9 @@ def student_view(self, context=None): ) context["xblock_id"] = str(self.scope_ids.usage_id.block_id) - # Use our xblock handler instead of the platform plugin's view. - context["superset_guest_token_url"] = self.runtime.handlerUrl( - self, "get_superset_guest_token" - ) + # 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)) diff --git a/test_settings.py b/test_settings.py index 41dd0ef..174c410 100644 --- a/test_settings.py +++ b/test_settings.py @@ -57,6 +57,8 @@ "CUSTOM_COURSES_EDX": True, } +LMS_ROOT_URL = "https://lms.url" + DEBUG = True TEMPLATES = [ @@ -75,18 +77,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", @@ -95,6 +91,7 @@ "allow_translations": True, } ] + EVENT_SINK_CLICKHOUSE_BACKEND_CONFIG = { "url": "https://foo.bar", "username": "bob",